LinuxLists.cc - [PATCH v4 0/5] misc: Add mikroBUS driver

[permalink] [raw]

Subject: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Add DT bindings for mikroBUS interface. MikroBUS is an open standard
developed by MikroElektronika for connecting add-on boards to
microcontrollers or microprocessors.

mikroBUS is a connector and does not have a controller. Instead the
software is responsible for identification of board and setting up /
registering uart, spi, i2c, pwm and other buses. Thus it needs a way to
get uart, spi, i2c, pwm and gpio controllers / adapters.

A mikroBUS addon board is free to leave some of the pins unused which
are marked as NC or Not Connected.

Some of the pins might need to be configured as GPIOs deviating from their
reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.

For some add-on boards the driver may not take care of some additional
signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
(RST pin on the mikrobus port) needs to be pulled high.

Here's the list of pins in mikroBUS connector:
Analog - AN
Reset - RST
SPI Chip Select - CS
SPI Clock - SCK
SPI Master Input Slave Output - MISO
SPI Master Output Slave Input - MOSI
VCC-3.3V power - +3.3V
Reference Ground - GND
PWM - PWM output
INT - Hardware Interrupt
RX - UART Receive
TX - UART Transmit
SCL - I2C Clock
SDA - I2C Data
+5V - VCC-5V power
GND - Reference Ground

Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains
a manifest to describe the addon board to provide plug and play
capabilities.

Link: https://www.mikroe.com/mikrobus
Link:
https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
mikroBUS specification
Link: https://www.mikroe.com/sht1x-click SHT15 Click
Link: https://www.mikroe.com/eth-click ENC28J60 Click
Link: https://www.mikroe.com/clickid ClickID

Co-developed-by: Vaishnav M A <[email protected]>
Signed-off-by: Vaishnav M A <[email protected]>
Signed-off-by: Ayush Singh <[email protected]>
---
.../connector/mikrobus-connector.yaml | 113 ++++++++++++++++++
MAINTAINERS | 6 +
2 files changed, 119 insertions(+)
create mode 100644 Documentation/devicetree/bindings/connector/mikrobus-connector.yaml

diff --git a/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
new file mode 100644
index 000000000000..ee3736add41c
--- /dev/null
+++ b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/connector/mikrobus-connector.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: mikroBUS add-on board socket
+
+maintainers:
+ - Ayush Singh <[email protected]>
+
+properties:
+ compatible:
+ const: mikrobus-connector
+
+ pinctrl-0: true
+ pinctrl-1: true
+ pinctrl-2: true
+ pinctrl-3: true
+ pinctrl-4: true
+ pinctrl-5: true
+ pinctrl-6: true
+ pinctrl-7: true
+ pinctrl-8: true
+
+ pinctrl-names:
+ items:
+ - const: default
+ - const: pwm_default
+ - const: pwm_gpio
+ - const: uart_default
+ - const: uart_gpio
+ - const: i2c_default
+ - const: i2c_gpio
+ - const: spi_default
+ - const: spi_gpio
+
+ mikrobus-gpios:
+ minItems: 11
+ maxItems: 12
+
+ i2c-adapter:
+ description: i2c adapter attached to the mikrobus socket.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
+ spi-controller:
+ description: spi bus number of the spi-master attached to the mikrobus socket.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
+ uart:
+ description: uart port attached to the mikrobus socket
+ $ref: /schemas/types.yaml#/definitions/phandle
+
+ pwms:
+ description: the pwm-controller corresponding to the mikroBUS PWM pin.
+ maxItems: 1
+
+ spi-cs:
+ description: spi chip-select numbers corresponding to the chip-selects on the mikrobus socket.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ items:
+ - description: chip select corresponding to CS pin
+ - description: chip select corresponding to RST pin
+
+required:
+ - compatible
+ - pinctrl-0
+ - pinctrl-1
+ - pinctrl-2
+ - pinctrl-3
+ - pinctrl-4
+ - pinctrl-5
+ - pinctrl-6
+ - pinctrl-7
+ - pinctrl-8
+ - i2c-adapter
+ - spi-controller
+ - spi-cs
+ - uart
+ - pwms
+ - mikrobus-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ mikrobus {
+ compatible = "mikrobus-connector";
+ pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default",
+ "i2c_gpio", "spi_default", "spi_gpio";
+ pinctrl-0 = <&P2_03_gpio_input_pin &P1_04_gpio_pin &P1_02_gpio_pin>;
+ pinctrl-1 = <&P2_01_pwm_pin>;
+ pinctrl-2 = <&P2_01_gpio_pin>;
+ pinctrl-3 = <&P2_05_uart_pin &P2_07_uart_pin>;
+ pinctrl-4 = <&P2_05_gpio_pin &P2_07_gpio_pin>;
+ pinctrl-5 = <&P2_09_i2c_pin &P2_11_i2c_pin>;
+ pinctrl-6 = <&P2_09_gpio_pin &P2_11_gpio_pin>;
+ pinctrl-7 = <&P1_12_spi_pin &P1_10_spi_pin &P1_08_spi_sclk_pin &P1_06_spi_cs_pin>;
+ pinctrl-8 = <&P1_12_gpio_pin &P1_10_gpio_pin &P1_08_gpio_pin &P1_06_gpio_pin>;
+ pwms = <&ehrpwm1 0 500000 0>;
+ i2c-adapter = <&i2c1>;
+ spi-controller = <&spi1>;
+ spi-cs = <0 1>;
+ uart = <&uart1>;
+ mikrobus-gpios = <&gpio1 18 GPIO_ACTIVE_HIGH>, <&gpio0 23 GPIO_ACTIVE_HIGH>,
+ <&gpio0 30 GPIO_ACTIVE_HIGH>, <&gpio0 31 GPIO_ACTIVE_HIGH>,
+ <&gpio0 15 GPIO_ACTIVE_HIGH>, <&gpio0 14 GPIO_ACTIVE_HIGH>,
+ <&gpio0 4 GPIO_ACTIVE_HIGH>, <&gpio0 3 GPIO_ACTIVE_HIGH>,
+ <&gpio0 2 GPIO_ACTIVE_HIGH>, <&gpio0 5 GPIO_ACTIVE_HIGH>,
+ <&gpio2 25 GPIO_ACTIVE_HIGH>, <&gpio2 3 GPIO_ACTIVE_HIGH>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 375d34363777..69418a058c6b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14767,6 +14767,12 @@ M: Oliver Neukum <[email protected]>
S: Maintained
F: drivers/usb/image/microtek.*

+MIKROBUS
+M: Ayush Singh <[email protected]>
+M: Vaishnav M A <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/misc/mikrobus-connector.yaml
+
MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
M: Luka Kovacic <[email protected]>
M: Luka Perkov <[email protected]>
--
2.44.0

2024-03-17 19:38:46

[permalink] [raw]

Subject: [PATCH v4 2/5] spi: Make of_find_spi_controller_by_node() available

DONOTMERGE

This externalizes and exports the symbol
of_find_spi_controller_by_node() from the SPI core akin to how
of_find_i2c_adapter_by_node() is already available. As we will
need this also for non-dynamic OF setups, we move it under a
CONFIG_OF check.

Signed-off-by: Ayush Singh <[email protected]>
---
drivers/spi/spi.c | 206 ++++++++++++++++++++--------------------
include/linux/spi/spi.h | 4 +
2 files changed, 108 insertions(+), 102 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ba4d3fde2054..9ec507d92f80 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2320,6 +2320,93 @@ void spi_flush_queue(struct spi_controller *ctlr)

/*-------------------------------------------------------------------------*/

+static void spi_controller_release(struct device *dev)
+{
+ struct spi_controller *ctlr;
+
+ ctlr = container_of(dev, struct spi_controller, dev);
+ kfree(ctlr);
+}
+
+static struct class spi_master_class = {
+ .name = "spi_master",
+ .dev_release = spi_controller_release,
+ .dev_groups = spi_master_groups,
+};
+
+static ssize_t slave_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct spi_controller *ctlr = container_of(dev, struct spi_controller,
+ dev);
+ struct device *child;
+
+ child = device_find_any_child(&ctlr->dev);
+ return sysfs_emit(buf, "%s\n", child ? to_spi_device(child)->modalias : NULL);
+}
+
+static ssize_t slave_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct spi_controller *ctlr = container_of(dev, struct spi_controller,
+ dev);
+ struct spi_device *spi;
+ struct device *child;
+ char name[32];
+ int rc;
+
+ rc = sscanf(buf, "%31s", name);
+ if (rc != 1 || !name[0])
+ return -EINVAL;
+
+ child = device_find_any_child(&ctlr->dev);
+ if (child) {
+ /* Remove registered slave */
+ device_unregister(child);
+ put_device(child);
+ }
+
+ if (strcmp(name, "(null)")) {
+ /* Register new slave */
+ spi = spi_alloc_device(ctlr);
+ if (!spi)
+ return -ENOMEM;
+
+ strscpy(spi->modalias, name, sizeof(spi->modalias));
+
+ rc = spi_add_device(spi);
+ if (rc) {
+ spi_dev_put(spi);
+ return rc;
+ }
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(slave);
+
+static struct attribute *spi_slave_attrs[] = {
+ &dev_attr_slave.attr,
+ NULL,
+};
+
+static const struct attribute_group spi_slave_group = {
+ .attrs = spi_slave_attrs,
+};
+
+static const struct attribute_group *spi_slave_groups[] = {
+ &spi_controller_statistics_group,
+ &spi_slave_group,
+ NULL,
+};
+
+static struct class spi_slave_class = {
+ .name = "spi_slave",
+ .dev_release = spi_controller_release,
+ .dev_groups = spi_slave_groups,
+};
+
#if defined(CONFIG_OF)
static void of_spi_parse_dt_cs_delay(struct device_node *nc,
struct spi_delay *delay, const char *prop)
@@ -2543,6 +2630,23 @@ static void of_register_spi_devices(struct spi_controller *ctlr)
}
}
}
+
+/* The spi controllers are not using spi_bus, so we find it with another way */
+struct spi_controller *of_find_spi_controller_by_node(struct device_node *node)
+{
+ struct device *dev;
+
+ dev = class_find_device_by_of_node(&spi_master_class, node);
+ if (!dev && IS_ENABLED(CONFIG_SPI_SLAVE))
+ dev = class_find_device_by_of_node(&spi_slave_class, node);
+ if (!dev)
+ return NULL;
+
+ /* Reference got in class_find_device */
+ return container_of(dev, struct spi_controller, dev);
+}
+EXPORT_SYMBOL_GPL(of_find_spi_controller_by_node);
+
#else
static void of_register_spi_devices(struct spi_controller *ctlr) { }
#endif
@@ -2942,20 +3046,6 @@ static void acpi_register_spi_devices(struct spi_controller *ctlr)
static inline void acpi_register_spi_devices(struct spi_controller *ctlr) {}
#endif /* CONFIG_ACPI */

-static void spi_controller_release(struct device *dev)
-{
- struct spi_controller *ctlr;
-
- ctlr = container_of(dev, struct spi_controller, dev);
- kfree(ctlr);
-}
-
-static struct class spi_master_class = {
- .name = "spi_master",
- .dev_release = spi_controller_release,
- .dev_groups = spi_master_groups,
-};
-
#ifdef CONFIG_SPI_SLAVE
/**
* spi_slave_abort - abort the ongoing transfer request on an SPI slave
@@ -2983,79 +3073,6 @@ int spi_target_abort(struct spi_device *spi)
return -ENOTSUPP;
}
EXPORT_SYMBOL_GPL(spi_target_abort);
-
-static ssize_t slave_show(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- struct spi_controller *ctlr = container_of(dev, struct spi_controller,
- dev);
- struct device *child;
-
- child = device_find_any_child(&ctlr->dev);
- return sysfs_emit(buf, "%s\n", child ? to_spi_device(child)->modalias : NULL);
-}
-
-static ssize_t slave_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct spi_controller *ctlr = container_of(dev, struct spi_controller,
- dev);
- struct spi_device *spi;
- struct device *child;
- char name[32];
- int rc;
-
- rc = sscanf(buf, "%31s", name);
- if (rc != 1 || !name[0])
- return -EINVAL;
-
- child = device_find_any_child(&ctlr->dev);
- if (child) {
- /* Remove registered slave */
- device_unregister(child);
- put_device(child);
- }
-
- if (strcmp(name, "(null)")) {
- /* Register new slave */
- spi = spi_alloc_device(ctlr);
- if (!spi)
- return -ENOMEM;
-
- strscpy(spi->modalias, name, sizeof(spi->modalias));
-
- rc = spi_add_device(spi);
- if (rc) {
- spi_dev_put(spi);
- return rc;
- }
- }
-
- return count;
-}
-
-static DEVICE_ATTR_RW(slave);
-
-static struct attribute *spi_slave_attrs[] = {
- &dev_attr_slave.attr,
- NULL,
-};
-
-static const struct attribute_group spi_slave_group = {
- .attrs = spi_slave_attrs,
-};
-
-static const struct attribute_group *spi_slave_groups[] = {
- &spi_controller_statistics_group,
- &spi_slave_group,
- NULL,
-};
-
-static struct class spi_slave_class = {
- .name = "spi_slave",
- .dev_release = spi_controller_release,
- .dev_groups = spi_slave_groups,
-};
#else
extern struct class spi_slave_class; /* dummy */
#endif
@@ -4749,21 +4766,6 @@ static struct spi_device *of_find_spi_device_by_node(struct device_node *node)
return dev ? to_spi_device(dev) : NULL;
}

-/* The spi controllers are not using spi_bus, so we find it with another way */
-static struct spi_controller *of_find_spi_controller_by_node(struct device_node *node)
-{
- struct device *dev;
-
- dev = class_find_device_by_of_node(&spi_master_class, node);
- if (!dev && IS_ENABLED(CONFIG_SPI_SLAVE))
- dev = class_find_device_by_of_node(&spi_slave_class, node);
- if (!dev)
- return NULL;
-
- /* Reference got in class_find_device */
- return container_of(dev, struct spi_controller, dev);
-}
-
static int of_spi_notify(struct notifier_block *nb, unsigned long action,
void *arg)
{
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index e400d454b3f0..f6fb7bad9a90 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -1684,4 +1684,8 @@ spi_transfer_is_last(struct spi_controller *ctlr, struct spi_transfer *xfer)
return list_is_last(&xfer->transfer_list, &ctlr->cur_msg->transfers);
}

+#if IS_ENABLED(CONFIG_OF)
+struct spi_controller *of_find_spi_controller_by_node(struct device_node *node);
+#endif
+
#endif /* __LINUX_SPI_H */
--
2.44.0

2024-03-17 19:39:18

[permalink] [raw]

Subject: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

DONOTMERGE

this patch depends on Patch 1, 2, 3

mikroBUS driver aims to split the platform aspects of mikroBUS (pinmux,
SPI/I2C/GPIO controller .etc) from the add-on board information, thus
requiring one device tree overlay per port and just a single manifest
describing the add-on board.

The driver exposes a sysfs entry that allows passing mikroBUS manifest of
add-on board to the driver. The driver then parses this manifest, sets up
the pins and protocols and allows using the appropriate Linux driver. Here
is an example:

```
cat /lib/firmware/mikrobus/AMBIENT-2-CLICK.mnfb > /sys/bus/mikrobus/devices/mikrobus-0/new_device
```

Another sysfs entry is exposed that allows removing a previously
registered mikrobus add-on board:

```
echo " " > /sys/bus/mikrobus/devices/mikrobus-0/delete_device
```

100s of mikroBUS addon board manifests can be found in the clickID
repository.

In the future the driver also aims to support plug and play support
using 1-wire EEPROM and mikroBUS over greybus.

Link: https://www.mikroe.com/clickid ClickID

Co-developed-by: Vaishnav M A <[email protected]>
Signed-off-by: Vaishnav M A <[email protected]>
Signed-off-by: Ayush Singh <[email protected]>
---
MAINTAINERS | 1 +
drivers/misc/Kconfig | 1 +
drivers/misc/Makefile | 1 +
drivers/misc/mikrobus/Kconfig | 15 +
drivers/misc/mikrobus/Makefile | 5 +
drivers/misc/mikrobus/mikrobus_core.c | 696 ++++++++++++++++++++++
drivers/misc/mikrobus/mikrobus_core.h | 151 +++++
drivers/misc/mikrobus/mikrobus_manifest.c | 503 ++++++++++++++++
drivers/misc/mikrobus/mikrobus_manifest.h | 29 +
9 files changed, 1402 insertions(+)
create mode 100644 drivers/misc/mikrobus/Kconfig
create mode 100644 drivers/misc/mikrobus/Makefile
create mode 100644 drivers/misc/mikrobus/mikrobus_core.c
create mode 100644 drivers/misc/mikrobus/mikrobus_core.h
create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.c
create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 69418a058c6b..83bc5e48bef9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14772,6 +14772,7 @@ M: Ayush Singh <[email protected]>
M: Vaishnav M A <[email protected]>
S: Maintained
F: Documentation/devicetree/bindings/misc/mikrobus-connector.yaml
+F: drivers/misc/mikrobus/*

MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
M: Luka Kovacic <[email protected]>
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 4fb291f0bf7c..3d5c36205c6c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -591,4 +591,5 @@ source "drivers/misc/cardreader/Kconfig"
source "drivers/misc/uacce/Kconfig"
source "drivers/misc/pvpanic/Kconfig"
source "drivers/misc/mchp_pci1xxxx/Kconfig"
+source "drivers/misc/mikrobus/Kconfig"
endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index ea6ea5bbbc9c..b9ac88055b87 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -68,3 +68,4 @@ obj-$(CONFIG_TMR_INJECT) += xilinx_tmr_inject.o
obj-$(CONFIG_TPS6594_ESM) += tps6594-esm.o
obj-$(CONFIG_TPS6594_PFSM) += tps6594-pfsm.o
obj-$(CONFIG_NSM) += nsm.o
+obj-y += mikrobus/
diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig
new file mode 100644
index 000000000000..aa57b994dc66
--- /dev/null
+++ b/drivers/misc/mikrobus/Kconfig
@@ -0,0 +1,15 @@
+menuconfig MIKROBUS
+ tristate "Module for instantiating devices on mikroBUS ports"
+ depends on GPIOLIB
+ help
+ This option enables the mikroBUS driver. mikroBUS is an add-on
+ board socket standard that offers maximum expandability with
+ the smallest number of pins. The mikroBUS driver instantiates
+ devices on a mikroBUS port described mikroBUS manifest which is
+ passed using a sysfs interface.
+
+
+ Say Y here to enable support for this driver.
+
+ To compile this code as a module, chose M here: the module
+ will be called mikrobus.ko
diff --git a/drivers/misc/mikrobus/Makefile b/drivers/misc/mikrobus/Makefile
new file mode 100644
index 000000000000..0e51c5a7db4b
--- /dev/null
+++ b/drivers/misc/mikrobus/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+# mikroBUS Core
+
+obj-$(CONFIG_MIKROBUS) += mikrobus.o
+mikrobus-y := mikrobus_core.o mikrobus_manifest.o
diff --git a/drivers/misc/mikrobus/mikrobus_core.c b/drivers/misc/mikrobus/mikrobus_core.c
new file mode 100644
index 000000000000..6aa20cef8e3b
--- /dev/null
+++ b/drivers/misc/mikrobus/mikrobus_core.c
@@ -0,0 +1,696 @@
+// SPDX-License-Identifier: GPL-2.0:
+/*
+ * mikroBUS driver for instantiating add-on
+ * board devices with an identifier EEPROM
+ *
+ * Copyright 2020 Vaishnav M A, BeagleBoard.org Foundation.
+ * Copyright 2024 Ayush Singh <[email protected]>
+ */
+
+#define pr_fmt(fmt) "mikrobus:%s: " fmt, __func__
+
+#include "linux/gpio/driver.h"
+#include "linux/gpio/machine.h"
+#include "linux/gpio/consumer.h"
+#include "linux/greybus/greybus_manifest.h"
+#include "linux/i2c.h"
+#include "linux/irq.h"
+#include "linux/pinctrl/consumer.h"
+#include "linux/platform_device.h"
+#include "linux/spi/spi.h"
+
+#include "mikrobus_core.h"
+#include "mikrobus_manifest.h"
+
+static struct class_compat *mikrobus_port_compat_class;
+
+static const struct bus_type mikrobus_bus_type = {
+ .name = "mikrobus",
+};
+
+static int mikrobus_board_register(struct mikrobus_port *port,
+ struct addon_board_info *board);
+static void mikrobus_board_unregister(struct mikrobus_port *port,
+ struct addon_board_info *board);
+
+/*
+ * mikrobus_pinctrl_select: Select pinctrl state for mikrobus pin
+ *
+ * @port: mikrobus port
+ * @pinctrl_selected: pinctrl state to be selected
+ */
+static int mikrobus_pinctrl_select(struct mikrobus_port *port,
+ const char *pinctrl_selected)
+{
+ struct pinctrl_state *state;
+ int ret;
+
+ state = pinctrl_lookup_state(port->pinctrl, pinctrl_selected);
+ if (IS_ERR(state)) {
+ return dev_err_probe(&port->dev, PTR_ERR(state),
+ "failed to find state %s",
+ pinctrl_selected);
+ }
+
+ ret = pinctrl_select_state(port->pinctrl, state);
+ if (ret) {
+ return dev_err_probe(&port->dev, ret,
+ "failed to select state %s",
+ pinctrl_selected);
+ }
+ dev_dbg(&port->dev, "setting pinctrl %s", pinctrl_selected);
+
+ return 0;
+}
+
+/*
+ * mikrobus_pinctrl_setup: Setup mikrobus pins to either default of gpio
+ *
+ * @port: mikrobus port
+ * @board: mikrobus board or NULL for default state
+ *
+ * returns 0 on success, negative error code on failure
+ */
+static int mikrobus_pinctrl_setup(struct mikrobus_port *port,
+ struct addon_board_info *board)
+{
+ int ret;
+
+ if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
+ ret = mikrobus_pinctrl_select(port, "pwm_default");
+ else
+ ret = mikrobus_pinctrl_select(port, "pwm_gpio");
+ if (ret)
+ return ret;
+
+ if (!board || board->pin_state[MIKROBUS_PIN_RX] == MIKROBUS_STATE_UART)
+ ret = mikrobus_pinctrl_select(port, "uart_default");
+ else
+ ret = mikrobus_pinctrl_select(port, "uart_gpio");
+ if (ret)
+ return ret;
+
+ if (!board || board->pin_state[MIKROBUS_PIN_SCL] == MIKROBUS_STATE_I2C)
+ ret = mikrobus_pinctrl_select(port, "i2c_default");
+ else
+ ret = mikrobus_pinctrl_select(port, "i2c_gpio");
+ if (ret)
+ return ret;
+
+ if (!board || board->pin_state[MIKROBUS_PIN_MOSI] == MIKROBUS_STATE_SPI)
+ ret = mikrobus_pinctrl_select(port, "spi_default");
+ else
+ ret = mikrobus_pinctrl_select(port, "spi_gpio");
+
+ return ret;
+}
+
+/*
+ * new_device_store: Expose sysfs entry for adding new board
+ *
+ * new_device_store: Allows userspace to add mikroBUS boards that lack 1-wire
+ * EEPROM for board identification by manually passing mikroBUS manifest
+ */
+static ssize_t new_device_store(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t count)
+{
+ struct mikrobus_port *port = to_mikrobus_port(dev);
+ struct addon_board_info *board;
+ int ret;
+
+ if (port->board)
+ return dev_err_probe(dev, -EBUSY,
+ "already has board registered");
+
+ board = devm_kzalloc(&port->dev, sizeof(*board), GFP_KERNEL);
+ if (!board)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&board->manifest_descs);
+ INIT_LIST_HEAD(&board->devices);
+
+ ret = mikrobus_manifest_parse(board, (void *)buf, count);
+ if (ret < 0) {
+ ret = dev_err_probe(dev, -EINVAL, "failed to parse manifest");
+ goto err_free_board;
+ }
+
+ ret = mikrobus_board_register(port, board);
+ if (ret) {
+ ret = dev_err_probe(dev, -EINVAL, "failed to register board %s",
+ board->name);
+ goto err_free_board;
+ }
+
+ return count;
+
+err_free_board:
+ devm_kfree(&port->dev, board);
+ return ret;
+}
+static DEVICE_ATTR_WO(new_device);
+
+/*
+ * delete_device_store: Expose sysfs entry for deleting board
+ */
+static ssize_t delete_device_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct mikrobus_port *port = to_mikrobus_port(dev);
+
+ if (!port->board)
+ return dev_err_probe(dev, -ENODEV,
+ "does not have registered boards");
+
+ mikrobus_board_unregister(port, port->board);
+ return count;
+}
+static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, 0200, NULL,
+ delete_device_store);
+
+static struct attribute *mikrobus_port_attrs[] = { &dev_attr_new_device.attr,
+ &dev_attr_delete_device.attr,
+ NULL };
+ATTRIBUTE_GROUPS(mikrobus_port);
+
+static void mikrobus_port_release(struct device *dev)
+{
+}
+
+static const struct device_type mikrobus_port_type = {
+ .groups = mikrobus_port_groups,
+ .release = mikrobus_port_release,
+};
+
+static int mikrobus_irq_get(struct mikrobus_port *port, int irqno, int irq_type)
+{
+ int irq;
+
+ if (irqno > port->gpios->ndescs - 1)
+ return dev_err_probe(&port->dev, -ENODEV,
+ "GPIO %d does not exist", irqno);
+
+ irq = gpiod_to_irq(port->gpios->desc[irqno]);
+ if (irq < 0)
+ return dev_err_probe(&port->dev, -EINVAL,
+ "could not get irq %d", irqno);
+
+ irq_set_irq_type(irq, irq_type);
+
+ return irq;
+}
+
+static int mikrobus_gpio_setup(struct gpio_desc *gpio, int gpio_state)
+{
+ switch (gpio_state) {
+ case MIKROBUS_STATE_INPUT:
+ return gpiod_direction_input(gpio);
+ case MIKROBUS_STATE_OUTPUT_HIGH:
+ return gpiod_direction_output(gpio, 1);
+ case MIKROBUS_STATE_OUTPUT_LOW:
+ return gpiod_direction_output(gpio, 0);
+ case MIKROBUS_STATE_PWM:
+ case MIKROBUS_STATE_SPI:
+ case MIKROBUS_STATE_I2C:
+ default:
+ return 0;
+ }
+}
+
+static char *mikrobus_gpio_chip_name_get(struct mikrobus_port *port, int gpio)
+{
+ struct gpio_chip *gpiochip;
+
+ if (gpio > port->gpios->ndescs - 1)
+ return NULL;
+
+ gpiochip = gpiod_to_chip(port->gpios->desc[gpio]);
+ return kmemdup(gpiochip->label, strlen(gpiochip->label), GFP_KERNEL);
+}
+
+static int mikrobus_gpio_hwnum_get(struct mikrobus_port *port, int gpio)
+{
+ struct gpio_chip *gpiochip;
+
+ if (gpio > port->gpios->ndescs - 1)
+ return -ENODEV;
+
+ gpiochip = gpiod_to_chip(port->gpios->desc[gpio]);
+ return desc_to_gpio(port->gpios->desc[gpio]) - gpiochip->base;
+}
+
+static void mikrobus_board_device_release_all(struct addon_board_info *info)
+{
+ struct board_device_info *dev, *next;
+
+ list_for_each_entry_safe(dev, next, &info->devices, links) {
+ list_del(&dev->links);
+ kfree(dev);
+ }
+}
+
+static int mikrobus_device_register(struct mikrobus_port *port,
+ struct board_device_info *dev,
+ char *board_name)
+{
+ struct gpiod_lookup_table *lookup;
+ struct spi_board_info *spi_info;
+ struct i2c_board_info *i2c_info;
+ struct platform_device *pdev;
+ struct fwnode_handle *fwnode;
+ struct spi_device *spi;
+ struct i2c_client *i2c;
+ int i, ret;
+
+ dev_info(&port->dev, "registering device : %s", dev->drv_name);
+
+ if (dev->gpio_lookup) {
+ lookup = dev->gpio_lookup;
+
+ switch (dev->protocol) {
+ case GREYBUS_PROTOCOL_SPI:
+ lookup->dev_id = kasprintf(GFP_KERNEL, "%s.%u",
+ dev->drv_name,
+ port->chip_select[dev->reg]);
+ break;
+ case GREYBUS_PROTOCOL_RAW:
+ lookup->dev_id = kasprintf(GFP_KERNEL, "%s.%u",
+ dev->drv_name, dev->reg);
+ break;
+ default:
+ lookup->dev_id = kmemdup(dev->drv_name,
+ strlen(dev->drv_name),
+ GFP_KERNEL);
+ }
+
+ dev_info(&port->dev, "adding lookup table : %s",
+ lookup->dev_id);
+
+ for (i = 0; i < dev->num_gpio_resources; i++) {
+ lookup->table[i].key = mikrobus_gpio_chip_name_get(
+ port, lookup->table[i].chip_hwnum);
+ lookup->table[i].chip_hwnum = mikrobus_gpio_hwnum_get(
+ port, lookup->table[i].chip_hwnum);
+ }
+
+ gpiod_add_lookup_table(lookup);
+ }
+
+ switch (dev->protocol) {
+ case GREYBUS_PROTOCOL_SPI:
+ spi_info =
+ devm_kzalloc(&port->dev, sizeof(*spi_info), GFP_KERNEL);
+ strscpy_pad(spi_info->modalias, dev->drv_name,
+ sizeof(spi_info->modalias));
+ if (dev->irq)
+ spi_info->irq =
+ mikrobus_irq_get(port, dev->irq, dev->irq_type);
+ if (dev->properties) {
+ fwnode = fwnode_create_software_node(dev->properties,
+ NULL);
+ spi_info->swnode = to_software_node(fwnode);
+ }
+ spi_info->chip_select = port->chip_select[dev->reg];
+ spi_info->max_speed_hz = dev->max_speed_hz;
+ spi_info->mode = dev->mode;
+
+ spi = spi_new_device(port->spi_ctrl, spi_info);
+ devm_kfree(&port->dev, spi_info);
+ if (!spi)
+ return dev_err_probe(&port->dev, -ENODEV,
+ "failed to register spi device");
+ dev->dev_client = (void *)spi;
+ break;
+ case GREYBUS_PROTOCOL_I2C:
+ i2c_info =
+ devm_kzalloc(&port->dev, sizeof(*i2c_info), GFP_KERNEL);
+ if (!i2c_info)
+ return -ENOMEM;
+
+ strscpy_pad(i2c_info->type, dev->drv_name,
+ sizeof(i2c_info->type));
+ if (dev->irq)
+ i2c_info->irq =
+ mikrobus_irq_get(port, dev->irq, dev->irq_type);
+ if (dev->properties) {
+ fwnode = fwnode_create_software_node(dev->properties,
+ NULL);
+ i2c_info->swnode = to_software_node(fwnode);
+ }
+ i2c_info->addr = dev->reg;
+
+ i2c = i2c_new_client_device(port->i2c_adap, i2c_info);
+ devm_kfree(&port->dev, i2c_info);
+ if (IS_ERR(dev->dev_client))
+ return dev_err_probe(&port->dev,
+ PTR_ERR(dev->dev_client),
+ "failed to register i2c device");
+ dev->dev_client = (void *)i2c;
+ break;
+ case GREYBUS_PROTOCOL_RAW:
+ pdev = platform_device_alloc(dev->drv_name, 0);
+ if (!pdev)
+ return -ENOMEM;
+
+ if (dev->properties) {
+ ret = device_create_managed_software_node(
+ &pdev->dev, dev->properties, NULL);
+ if (ret)
+ return dev_err_probe(
+ &port->dev, ret,
+ "failed to create software node");
+ }
+ ret = platform_device_add(dev->dev_client);
+ if (ret)
+ return dev_err_probe(
+ &port->dev, ret,
+ "failed to register platform device");
+ dev->dev_client = (void *)pdev;
+ break;
+ case GREYBUS_PROTOCOL_UART:
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void mikrobus_device_unregister(struct mikrobus_port *port,
+ struct board_device_info *dev,
+ char *board_name)
+{
+ dev_info(&port->dev, "removing device %s", dev->drv_name);
+ if (dev->gpio_lookup) {
+ gpiod_remove_lookup_table(dev->gpio_lookup);
+ kfree(dev->gpio_lookup->dev_id);
+ kfree(dev->gpio_lookup);
+ }
+
+ kfree(dev->properties);
+
+ switch (dev->protocol) {
+ case GREYBUS_PROTOCOL_SPI:
+ spi_unregister_device((struct spi_device *)dev->dev_client);
+ break;
+ case GREYBUS_PROTOCOL_I2C:
+ i2c_unregister_device((struct i2c_client *)dev->dev_client);
+ break;
+ case GREYBUS_PROTOCOL_RAW:
+ platform_device_unregister(
+ (struct platform_device *)dev->dev_client);
+ break;
+ case GREYBUS_PROTOCOL_UART:
+ break;
+ }
+}
+
+static int mikrobus_board_register(struct mikrobus_port *port,
+ struct addon_board_info *board)
+{
+ struct board_device_info *devinfo, *next;
+ int ret, i;
+
+ if (WARN_ON(list_empty(&board->devices)))
+ return false;
+
+ if (port->pinctrl) {
+ ret = mikrobus_pinctrl_setup(port, board);
+ if (ret)
+ dev_err(&port->dev,
+ "failed to setup pinctrl state [%d]", ret);
+ }
+
+ if (port->gpios) {
+ for (i = 0; i < port->gpios->ndescs; i++) {
+ ret = mikrobus_gpio_setup(port->gpios->desc[i],
+ board->pin_state[i]);
+ if (ret)
+ dev_err(&port->dev,
+ "failed to setup gpio %d, state %d", i,
+ board->pin_state[i]);
+
+ gpiochip_free_own_desc(port->gpios->desc[i]);
+ }
+ }
+
+ list_for_each_entry_safe(devinfo, next, &board->devices, links)
+ mikrobus_device_register(port, devinfo, board->name);
+
+ port->board = board;
+ return 0;
+}
+
+static void mikrobus_board_unregister(struct mikrobus_port *port,
+ struct addon_board_info *board)
+{
+ struct board_device_info *devinfo, *next;
+
+ if (WARN_ON(list_empty(&board->devices)))
+ return;
+
+ list_for_each_entry_safe(devinfo, next, &board->devices, links)
+ mikrobus_device_unregister(port, devinfo, board->name);
+
+ mikrobus_board_device_release_all(board);
+ devm_kfree(&port->dev, board);
+ port->board = NULL;
+}
+
+static int mikrobus_port_register(struct mikrobus_port *port)
+{
+ int ret;
+
+ port->dev.bus = &mikrobus_bus_type;
+ port->dev.type = &mikrobus_port_type;
+ dev_set_name(&port->dev, "mikrobus-%d", port->id);
+
+ dev_info(&port->dev, "registering port %s", dev_name(&port->dev));
+
+ ret = device_register(&port->dev);
+ if (ret)
+ return dev_err_probe(&port->dev, ret,
+ "port '%d': can't register device (%d)",
+ port->id, ret);
+
+ ret = class_compat_create_link(mikrobus_port_compat_class, &port->dev,
+ port->dev.parent);
+ if (ret)
+ dev_warn(&port->dev,
+ "failed to create compatibility class link");
+
+ return ret;
+}
+
+static void mikrobus_port_delete(struct mikrobus_port *port)
+{
+ if (port->board)
+ return dev_err(
+ &port->dev,
+ "attempting to delete port with registered boards, port [%s]",
+ dev_name(&port->dev));
+
+ class_compat_remove_link(mikrobus_port_compat_class, &port->dev,
+ port->dev.parent);
+
+ devm_pinctrl_put(port->pinctrl);
+ put_device(&port->spi_ctrl->dev);
+ gpiod_put_array(port->gpios);
+ put_device(&port->i2c_adap->dev);
+
+ device_unregister(&port->dev);
+ memset(&port->dev, 0, sizeof(port->dev));
+}
+
+static int mikrobus_port_probe_pinctrl_setup(struct mikrobus_port *port)
+{
+ struct device *dev = port->dev.parent;
+ struct pinctrl_state *state;
+ int ret;
+
+ state = pinctrl_lookup_state(port->pinctrl, PINCTRL_STATE_DEFAULT);
+ if (IS_ERR(state))
+ return dev_err_probe(dev, PTR_ERR(state),
+ "failed to find state %s",
+ PINCTRL_STATE_DEFAULT);
+
+ ret = pinctrl_select_state(port->pinctrl, state);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to select state %s",
+ PINCTRL_STATE_DEFAULT);
+
+ ret = mikrobus_pinctrl_setup(port, NULL);
+ if (ret)
+ dev_err(dev, "failed to select pinctrl states [%d]", ret);
+
+ return ret;
+}
+
+static int mikrobus_port_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mikrobus_port *port;
+ struct device_node *np;
+ int ret;
+
+ port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+ return -ENOMEM;
+
+ port->dev.parent = dev;
+ port->dev.of_node = pdev->dev.of_node;
+
+ /* port id */
+ port->id = of_alias_get_id(dev->of_node, "mikrobus");
+ if (port->id) {
+ ret = dev_err_probe(dev, -EINVAL, "invalid mikrobus id");
+ goto err_port;
+ }
+
+ /* I2C setup */
+ np = of_parse_phandle(dev->of_node, "i2c-adapter", 0);
+ if (!np) {
+ ret = dev_err_probe(dev, -ENODEV, "cannot parse i2c-adapter");
+ goto err_port;
+ }
+ port->i2c_adap = of_find_i2c_adapter_by_node(np);
+ of_node_put(np);
+ if (!port->i2c_adap) {
+ ret = dev_err_probe(dev, -ENODEV, "cannot find i2c adapter");
+ goto err_port;
+ }
+
+ /* GPIO setup */
+ port->gpios = gpiod_get_array(dev, "mikrobus", GPIOD_OUT_LOW);
+ if (IS_ERR(port->gpios)) {
+ ret = dev_err_probe(dev, PTR_ERR(port->gpios),
+ "failed to get gpio array [%ld]",
+ PTR_ERR(port->gpios));
+ goto free_i2c;
+ }
+
+ /* SPI setup */
+ np = of_parse_phandle(dev->of_node, "spi-controller", 0);
+ if (!np) {
+ ret = dev_err_probe(dev, -ENODEV,
+ "cannot parse spi-controller");
+ goto free_gpio;
+ }
+ port->spi_ctrl = of_find_spi_controller_by_node(np);
+ of_node_put(np);
+ if (!port->spi_ctrl) {
+ ret = dev_err_probe(dev, -ENODEV, "cannot find spi controller");
+ goto free_gpio;
+ }
+ ret = device_property_read_u32_array(dev, "spi-cs", port->chip_select,
+ MIKROBUS_NUM_CS);
+ if (ret) {
+ dev_err(dev, "failed to get spi-cs [%d]", ret);
+ goto free_spi;
+ }
+
+ /* pinctrl setup */
+ port->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(port->pinctrl)) {
+ ret = dev_err_probe(dev, PTR_ERR(port->pinctrl),
+ "failed to get pinctrl [%ld]",
+ PTR_ERR(port->pinctrl));
+ goto free_spi;
+ }
+ ret = mikrobus_port_probe_pinctrl_setup(port);
+ if (ret) {
+ dev_err(dev, "failed to setup pinctrl [%d]", ret);
+ goto free_pinctrl;
+ }
+
+ /* TODO: UART */
+ /* TODO: PWM */
+
+ ret = mikrobus_port_register(port);
+ if (ret) {
+ dev_err(dev, "port : can't register port [%d]", ret);
+ goto free_pinctrl;
+ }
+
+ platform_set_drvdata(pdev, port);
+
+ return 0;
+
+free_pinctrl:
+ devm_pinctrl_put(port->pinctrl);
+free_spi:
+ put_device(&port->spi_ctrl->dev);
+free_gpio:
+ gpiod_put_array(port->gpios);
+free_i2c:
+ put_device(&port->i2c_adap->dev);
+err_port:
+ put_device(&port->dev);
+ return ret;
+}
+
+static int mikrobus_port_remove(struct platform_device *pdev)
+{
+ struct mikrobus_port *port = platform_get_drvdata(pdev);
+
+ mikrobus_port_delete(port);
+ return 0;
+}
+
+static const struct of_device_id mikrobus_port_of_match[] = {
+ { .compatible = "mikrobus-connector" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mikrobus_port_of_match);
+
+static struct platform_driver mikrobus_port_driver = {
+ .probe = mikrobus_port_probe,
+ .remove = mikrobus_port_remove,
+ .driver = {
+ .name = "mikrobus",
+ .of_match_table = mikrobus_port_of_match,
+ },
+};
+
+static int mikrobus_init(void)
+{
+ int ret;
+
+ ret = bus_register(&mikrobus_bus_type);
+ if (ret) {
+ pr_err("bus_register failed (%d)", ret);
+ return ret;
+ }
+
+ mikrobus_port_compat_class = class_compat_register("mikrobus-port");
+ if (!mikrobus_port_compat_class) {
+ ret = -ENOMEM;
+ pr_err("class_compat register failed (%d)", ret);
+ goto class_err;
+ }
+
+ ret = platform_driver_register(&mikrobus_port_driver);
+ if (ret)
+ pr_err("driver register failed [%d]", ret);
+
+ return ret;
+
+class_err:
+ bus_unregister(&mikrobus_bus_type);
+ return ret;
+}
+subsys_initcall(mikrobus_init);
+
+static void mikrobus_exit(void)
+{
+ platform_driver_unregister(&mikrobus_port_driver);
+ bus_unregister(&mikrobus_bus_type);
+ class_compat_unregister(mikrobus_port_compat_class);
+}
+module_exit(mikrobus_exit);
+
+MODULE_AUTHOR("Vaishnav M A <[email protected]>");
+MODULE_AUTHOR("Ayush Singh <[email protected]>");
+MODULE_DESCRIPTION("mikroBUS main module");
+MODULE_LICENSE("GPL");
diff --git a/drivers/misc/mikrobus/mikrobus_core.h b/drivers/misc/mikrobus/mikrobus_core.h
new file mode 100644
index 000000000000..1d41ee32ca94
--- /dev/null
+++ b/drivers/misc/mikrobus/mikrobus_core.h
@@ -0,0 +1,151 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * mikroBUS Driver for instantiating add-on board devices
+ *
+ * Copyright 2020 Vaishnav M A, BeagleBoard.org Foundation.
+ * Copyright 2024 Ayush Singh <[email protected]>
+ */
+
+#ifndef __MIKROBUS_H
+#define __MIKROBUS_H
+
+#include "linux/device.h"
+
+#define MIKROBUS_VERSION_MAJOR 0x00
+#define MIKROBUS_VERSION_MINOR 0x03
+
+#define MIKROBUS_NUM_PINCTRL_STATE 4
+#define MIKROBUS_NUM_CS 2
+
+#define MIKROBUS_PINCTRL_PWM 0
+#define MIKROBUS_PINCTRL_UART 1
+#define MIKROBUS_PINCTRL_I2C 2
+#define MIKROBUS_PINCTRL_SPI 3
+
+enum mikrobus_property_type {
+ MIKROBUS_PROPERTY_TYPE_MIKROBUS = 0x00,
+ MIKROBUS_PROPERTY_TYPE_PROPERTY,
+ MIKROBUS_PROPERTY_TYPE_GPIO,
+ MIKROBUS_PROPERTY_TYPE_U8,
+ MIKROBUS_PROPERTY_TYPE_U16,
+ MIKROBUS_PROPERTY_TYPE_U32,
+ MIKROBUS_PROPERTY_TYPE_U64,
+};
+
+enum mikrobus_pin {
+ MIKROBUS_PIN_PWM = 0x00,
+ MIKROBUS_PIN_INT,
+ MIKROBUS_PIN_RX,
+ MIKROBUS_PIN_TX,
+ MIKROBUS_PIN_SCL,
+ MIKROBUS_PIN_SDA,
+ MIKROBUS_PIN_MOSI,
+ MIKROBUS_PIN_MISO,
+ MIKROBUS_PIN_SCK,
+ MIKROBUS_PIN_CS,
+ MIKROBUS_PIN_RST,
+ MIKROBUS_PIN_AN,
+ MIKROBUS_PORT_PIN_COUNT,
+};
+
+enum mikrobus_pin_state {
+ MIKROBUS_STATE_INPUT = 0x01,
+ MIKROBUS_STATE_OUTPUT_HIGH,
+ MIKROBUS_STATE_OUTPUT_LOW,
+ MIKROBUS_STATE_PWM,
+ MIKROBUS_STATE_SPI,
+ MIKROBUS_STATE_I2C,
+ MIKROBUS_STATE_UART,
+};
+
+/*
+ * board_device_info describes a single device on a mikrobus add-on
+ * board, an add-on board can present one or more device to the host
+ *
+ * @gpio_lookup: used to provide the GPIO lookup table for
+ * passing the named GPIOs to device drivers.
+ * @properties: used to provide the property_entry to pass named
+ * properties to device drivers, applicable only when driver uses
+ * device_property_read_* calls to fetch the properties.
+ * @num_gpio_resources: number of named gpio resources for the device,
+ * used mainly for gpiod_lookup_table memory allocation.
+ * @num_properties: number of custom properties for the device,
+ * used mainly for property_entry memory allocation.
+ * @protocol: used to know the type of the device and it should
+ * contain one of the values defined under 'enum greybus_class_type'
+ * under linux/greybus/greybus_manifest.h
+ * @reg: I2C address for the device, for devices on the SPI bus
+ * this field is the chip select address relative to the mikrobus
+ * port:0->device chip select connected to CS pin on mikroBUS port
+ * 1->device chip select connected to RST Pin on mikroBUS port
+ * @mode: SPI mode
+ * @max_speed_hz: SPI max speed(Hz)
+ * @drv_name: device_id to match with the driver
+ * @irq_type: type of IRQ trigger , match with defines in linux/interrupt.h
+ * @irq: irq number relative to the mikrobus port should contain one of the
+ * values defined under 'enum mikrobus_pin'
+ * @id: device id starting from 1
+ */
+struct board_device_info {
+ struct gpiod_lookup_table *gpio_lookup;
+ struct property_entry *properties;
+ struct list_head links;
+ unsigned short num_gpio_resources;
+ unsigned short num_properties;
+ unsigned short protocol;
+ unsigned short reg;
+ unsigned int mode;
+ void *dev_client;
+ u32 max_speed_hz;
+ char *drv_name;
+ int irq_type;
+ int irq;
+ int id;
+};
+
+/*
+ * addon_board_info describes a mikrobus add-on device the add-on
+ * board, an add-on board can present one or more device to the host
+ *
+ * @manifest_descs: list of manifest descriptors
+ * @devices: list of devices on the board
+ * @pin_state: the state of each pin on the mikrobus port required
+ * for the add-on board should contain one of the values defined under
+ * 'enum mikrobus_pin_state' restrictions are as per mikrobus standard
+ * specifications.
+ * @name: add-on board name
+ */
+struct addon_board_info {
+ struct list_head manifest_descs;
+ struct list_head devices;
+ u8 pin_state[MIKROBUS_PORT_PIN_COUNT];
+ char *name;
+};
+
+/*
+ * mikrobus_port describes the peripherals mapped to a mikrobus port.
+ *
+ * @chip_select: chip select number mapped to the SPI CS pin on the
+ * mikrobus port and the RST pin on the mikrobus port
+ * @board: pointer to the attached add-on board.
+ * @spi_ctrl: SPI controller attached to the mikrobus port.
+ * @i2c_adap: I2C adapter attached to the mikrobus port.
+ * @gpios: GPIOs attached to the mikrobus port.
+ * @pinctrl: pinctrl attached to the mikrobus port.
+ * @dev: device structure for the mikrobus port.
+ * @id: port id starting from 1
+ */
+struct mikrobus_port {
+ u32 chip_select[MIKROBUS_NUM_CS];
+ struct addon_board_info *board;
+ struct spi_controller *spi_ctrl;
+ struct i2c_adapter *i2c_adap;
+ struct gpio_descs *gpios;
+ struct pinctrl *pinctrl;
+ struct device dev;
+ int id;
+};
+
+#define to_mikrobus_port(d) container_of(d, struct mikrobus_port, dev)
+
+#endif /* __MIKROBUS_H */
diff --git a/drivers/misc/mikrobus/mikrobus_manifest.c b/drivers/misc/mikrobus/mikrobus_manifest.c
new file mode 100644
index 000000000000..5f30620277be
--- /dev/null
+++ b/drivers/misc/mikrobus/mikrobus_manifest.c
@@ -0,0 +1,503 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * mikroBUS manifest parsing, an
+ * extension to Greybus Manifest Parsing
+ * under drivers/greybus/manifest.c
+ *
+ * Copyright 2014-2015 Google Inc.
+ * Copyright 2014-2015 Linaro Ltd.
+ * Copyright 2024 Ayush Singh <[email protected]>
+ */
+
+#define pr_fmt(fmt) "mikrobus_manifest:%s: " fmt, __func__
+
+#include "linux/gpio/machine.h"
+#include "linux/greybus/greybus_manifest.h"
+#include "linux/property.h"
+#include "mikrobus_manifest.h"
+
+struct manifest_desc {
+ struct list_head links;
+ size_t size;
+ void *data;
+ enum greybus_descriptor_type type;
+};
+
+static void manifest_descriptor_release_all(struct addon_board_info *board)
+{
+ struct manifest_desc *descriptor, *next;
+
+ list_for_each_entry_safe(descriptor, next, &board->manifest_descs,
+ links) {
+ list_del(&descriptor->links);
+ kfree(descriptor);
+ }
+}
+
+static int board_descriptor_add(struct addon_board_info *board,
+ struct greybus_descriptor *desc, size_t size)
+{
+ struct greybus_descriptor_header *desc_header = &desc->header;
+ struct manifest_desc *descriptor;
+ size_t desc_size, expected_size;
+
+ if (size < sizeof(*desc_header)) {
+ pr_err("short descriptor (%zu < %zu)", size,
+ sizeof(*desc_header));
+ return -EINVAL;
+ }
+
+ desc_size = le16_to_cpu(desc_header->size);
+ if (desc_size > size) {
+ pr_err("incorrect descriptor size (%zu != %zu)", size,
+ desc_size);
+ return -EINVAL;
+ }
+
+ expected_size = sizeof(*desc_header);
+ switch (desc_header->type) {
+ case GREYBUS_TYPE_STRING:
+ expected_size += sizeof(struct greybus_descriptor_string);
+ expected_size += desc->string.length;
+ expected_size = ALIGN(expected_size, 4);
+ break;
+ case GREYBUS_TYPE_PROPERTY:
+ expected_size += sizeof(struct greybus_descriptor_property);
+ expected_size += desc->property.length;
+ expected_size = ALIGN(expected_size, 4);
+ break;
+ case GREYBUS_TYPE_DEVICE:
+ expected_size += sizeof(struct greybus_descriptor_device);
+ break;
+ case GREYBUS_TYPE_MIKROBUS:
+ expected_size += sizeof(struct greybus_descriptor_mikrobus);
+ break;
+ case GREYBUS_TYPE_INTERFACE:
+ expected_size += sizeof(struct greybus_descriptor_interface);
+ break;
+ case GREYBUS_TYPE_CPORT:
+ expected_size += sizeof(struct greybus_descriptor_cport);
+ break;
+ case GREYBUS_TYPE_BUNDLE:
+ expected_size += sizeof(struct greybus_descriptor_bundle);
+ break;
+ case GREYBUS_TYPE_INVALID:
+ default:
+ pr_err("invalid descriptor type %d", desc_header->type);
+ return -EINVAL;
+ }
+
+ descriptor = kzalloc(sizeof(*descriptor), GFP_KERNEL);
+ if (!descriptor)
+ return -ENOMEM;
+
+ descriptor->size = desc_size;
+ descriptor->data = (char *)desc + sizeof(*desc_header);
+ descriptor->type = desc_header->type;
+ list_add_tail(&descriptor->links, &board->manifest_descs);
+
+ return desc_size;
+}
+
+static char *mikrobus_string_get(struct addon_board_info *board, u8 string_id)
+{
+ struct greybus_descriptor_string *desc_string;
+ struct manifest_desc *descriptor;
+ bool found = false;
+ char *string;
+
+ if (!string_id)
+ return NULL;
+
+ list_for_each_entry(descriptor, &board->manifest_descs, links) {
+ if (descriptor->type != GREYBUS_TYPE_STRING)
+ continue;
+
+ desc_string = descriptor->data;
+ if (desc_string->id == string_id) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ return ERR_PTR(-ENOENT);
+
+ string = kmemdup(&desc_string->string, desc_string->length + 1,
+ GFP_KERNEL);
+ if (!string)
+ return ERR_PTR(-ENOMEM);
+
+ string[desc_string->length] = '\0';
+
+ return string;
+}
+
+static void mikrobus_state_get(struct addon_board_info *board)
+{
+ struct greybus_descriptor_mikrobus *mikrobus;
+ struct greybus_descriptor_interface *interface;
+ struct manifest_desc *descriptor;
+ bool found = false;
+ size_t i;
+
+ list_for_each_entry(descriptor, &board->manifest_descs, links) {
+ if (descriptor->type == GREYBUS_TYPE_MIKROBUS) {
+ mikrobus = descriptor->data;
+ found = true;
+ break;
+ }
+ }
+
+ if (!found) {
+ pr_err("mikrobus descriptor not found");
+ return;
+ }
+
+ for (i = 0; i < MIKROBUS_PORT_PIN_COUNT; i++)
+ board->pin_state[i] = mikrobus->pin_state[i];
+
+ found = false;
+ list_for_each_entry(descriptor, &board->manifest_descs, links) {
+ if (descriptor->type == GREYBUS_TYPE_INTERFACE) {
+ interface = descriptor->data;
+ found = true;
+ break;
+ }
+ }
+
+ if (!found) {
+ pr_err("interface descriptor not found");
+ return;
+ }
+
+ board->name = mikrobus_string_get(board, interface->product_stringid);
+}
+
+static struct property_entry *
+mikrobus_property_entry_get(struct addon_board_info *board, u8 *prop_link,
+ int num_properties)
+{
+ struct greybus_descriptor_property *desc_property;
+ struct manifest_desc *descriptor;
+ struct property_entry *properties;
+ bool found = false;
+ char *prop_name;
+ int i, ret;
+ u64 *val_u64;
+ u32 *val_u32;
+ u16 *val_u16;
+ u8 *val_u8;
+
+ properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL);
+ if (!properties)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; i < num_properties; i++) {
+ list_for_each_entry(descriptor, &board->manifest_descs, links) {
+ if (descriptor->type != GREYBUS_TYPE_PROPERTY)
+ continue;
+
+ desc_property = descriptor->data;
+ if (desc_property->id == prop_link[i]) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found) {
+ ret = -ENOENT;
+ goto early_exit;
+ }
+
+ prop_name = mikrobus_string_get(
+ board, desc_property->propname_stringid);
+ if (!prop_name) {
+ ret = -ENOENT;
+ goto early_exit;
+ }
+
+ switch (desc_property->type) {
+ case MIKROBUS_PROPERTY_TYPE_U8:
+ val_u8 = kmemdup(&desc_property->value,
+ (desc_property->length) * sizeof(u8),
+ GFP_KERNEL);
+ if (desc_property->length == 1)
+ properties[i] =
+ PROPERTY_ENTRY_U8(prop_name, *val_u8);
+ else
+ properties[i] = PROPERTY_ENTRY_U8_ARRAY_LEN(
+ prop_name, (void *)desc_property->value,
+ desc_property->length);
+ break;
+ case MIKROBUS_PROPERTY_TYPE_U16:
+ val_u16 = kmemdup(&desc_property->value,
+ (desc_property->length) * sizeof(u16),
+ GFP_KERNEL);
+ if (desc_property->length == 1)
+ properties[i] =
+ PROPERTY_ENTRY_U16(prop_name, *val_u16);
+ else
+ properties[i] = PROPERTY_ENTRY_U16_ARRAY_LEN(
+ prop_name, (void *)desc_property->value,
+ desc_property->length);
+ break;
+ case MIKROBUS_PROPERTY_TYPE_U32:
+ val_u32 = kmemdup(&desc_property->value,
+ (desc_property->length) * sizeof(u32),
+ GFP_KERNEL);
+ if (desc_property->length == 1)
+ properties[i] =
+ PROPERTY_ENTRY_U32(prop_name, *val_u32);
+ else
+ properties[i] = PROPERTY_ENTRY_U32_ARRAY_LEN(
+ prop_name, (void *)desc_property->value,
+ desc_property->length);
+ break;
+ case MIKROBUS_PROPERTY_TYPE_U64:
+ val_u64 = kmemdup(&desc_property->value,
+ (desc_property->length) * sizeof(u64),
+ GFP_KERNEL);
+ if (desc_property->length == 1)
+ properties[i] =
+ PROPERTY_ENTRY_U64(prop_name, *val_u64);
+ else
+ properties[i] = PROPERTY_ENTRY_U64_ARRAY_LEN(
+ prop_name, (void *)desc_property->value,
+ desc_property->length);
+ break;
+ default:
+ ret = -EINVAL;
+ goto early_exit;
+ }
+ }
+ return properties;
+
+early_exit:
+ kfree(properties);
+ return ERR_PTR(ret);
+}
+
+static u8 *mikrobus_property_link_get(struct addon_board_info *board,
+ u8 prop_id,
+ struct board_device_info *board_dev,
+ u8 prop_type)
+{
+ struct greybus_descriptor_property *desc_property;
+ struct manifest_desc *descriptor;
+ bool found = false;
+ u8 *val_u8;
+
+ if (!prop_id)
+ return NULL;
+
+ list_for_each_entry(descriptor, &board->manifest_descs, links) {
+ if (descriptor->type != GREYBUS_TYPE_PROPERTY)
+ continue;
+
+ desc_property = descriptor->data;
+ if (desc_property->id == prop_id &&
+ desc_property->type == prop_type) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ return ERR_PTR(-ENOENT);
+
+ val_u8 = kmemdup(&desc_property->value, desc_property->length,
+ GFP_KERNEL);
+ if (prop_type == MIKROBUS_PROPERTY_TYPE_GPIO)
+ board_dev->num_gpio_resources = desc_property->length;
+ else if (prop_type == MIKROBUS_PROPERTY_TYPE_PROPERTY)
+ board_dev->num_properties = desc_property->length;
+
+ return val_u8;
+}
+
+static int
+mikrobus_manifest_attach_device(struct addon_board_info *board,
+ struct greybus_descriptor_device *dev_desc)
+{
+ struct greybus_descriptor_property *desc_property;
+ u8 *gpio_desc_link, *prop_link, *gpioval;
+ struct board_device_info *board_dev;
+ struct gpiod_lookup_table *lookup;
+ struct manifest_desc *descriptor;
+ int ret, i;
+
+ board_dev = kzalloc(sizeof(*board_dev), GFP_KERNEL);
+ if (!board_dev)
+ return -ENOMEM;
+
+ board_dev->id = dev_desc->id;
+ board_dev->drv_name =
+ mikrobus_string_get(board, dev_desc->driver_stringid);
+ if (!board_dev->drv_name) {
+ ret = -ENOENT;
+ goto err_free_board_dev;
+ }
+
+ board_dev->protocol = dev_desc->protocol;
+ board_dev->reg = dev_desc->reg;
+ board_dev->irq = dev_desc->irq;
+ board_dev->irq_type = dev_desc->irq_type;
+ board_dev->max_speed_hz = le32_to_cpu(dev_desc->max_speed_hz);
+ board_dev->mode = dev_desc->mode;
+ pr_info("parsed device %d, driver=%s", board_dev->id,
+ board_dev->drv_name);
+
+ if (dev_desc->prop_link > 0) {
+ prop_link = mikrobus_property_link_get(
+ board, dev_desc->prop_link, board_dev,
+ MIKROBUS_PROPERTY_TYPE_PROPERTY);
+ if (!prop_link) {
+ ret = -ENOENT;
+ goto err_free_board_dev;
+ }
+
+ pr_info("device %d, number of properties=%d", board_dev->id,
+ board_dev->num_properties);
+ board_dev->properties = mikrobus_property_entry_get(
+ board, prop_link, board_dev->num_properties);
+ }
+
+ if (dev_desc->gpio_link > 0) {
+ gpio_desc_link = mikrobus_property_link_get(
+ board, dev_desc->gpio_link, board_dev,
+ MIKROBUS_PROPERTY_TYPE_GPIO);
+ if (!gpio_desc_link) {
+ ret = -ENOENT;
+ goto err_free_board_dev;
+ }
+
+ pr_info("device %d, number of gpio resource=%d", board_dev->id,
+ board_dev->num_gpio_resources);
+ lookup = kzalloc(struct_size(lookup, table,
+ board_dev->num_gpio_resources),
+ GFP_KERNEL);
+ if (!lookup) {
+ ret = -ENOMEM;
+ goto err_free_board_dev;
+ }
+
+ for (i = 0; i < board_dev->num_gpio_resources; i++) {
+ list_for_each_entry(descriptor, &board->manifest_descs,
+ links) {
+ if (descriptor->type != GREYBUS_TYPE_PROPERTY)
+ continue;
+
+ desc_property = descriptor->data;
+ if (desc_property->id == gpio_desc_link[i]) {
+ gpioval = desc_property->value;
+ lookup->table[i].chip_hwnum =
+ gpioval[0];
+ lookup->table[i].flags = gpioval[1];
+ lookup->table[i]
+ .con_id = mikrobus_string_get(
+ board,
+ desc_property
+ ->propname_stringid);
+ break;
+ }
+ }
+ }
+ board_dev->gpio_lookup = lookup;
+ }
+
+ list_add_tail(&board_dev->links, &board->devices);
+ return 0;
+
+err_free_board_dev:
+ kfree(board_dev);
+ return ret;
+}
+
+static int mikrobus_manifest_parse_devices(struct addon_board_info *board)
+{
+ struct greybus_descriptor_device *desc_device;
+ struct manifest_desc *desc, *next;
+ int ret, devcount = 0;
+
+ list_for_each_entry_safe(desc, next, &board->manifest_descs, links) {
+ if (desc->type != GREYBUS_TYPE_DEVICE)
+ continue;
+
+ desc_device = desc->data;
+ ret = mikrobus_manifest_attach_device(board, desc_device);
+ devcount++;
+ }
+
+ return devcount;
+}
+
+static size_t mikrobus_manifest_header_validate(void *data, size_t size)
+{
+ struct greybus_manifest_header *header = data;
+ u16 manifest_size = le16_to_cpu(header->size);
+
+ if (manifest_size < sizeof(*header)) {
+ pr_err("short manifest (%zu < %zu)", size, sizeof(*header));
+ return -EINVAL;
+ }
+
+ if (header->version_major > MIKROBUS_VERSION_MAJOR) {
+ pr_err("manifest version too new (%u.%u > %u.%u)",
+ header->version_major, header->version_minor,
+ MIKROBUS_VERSION_MAJOR, MIKROBUS_VERSION_MINOR);
+ return -EINVAL;
+ }
+
+ return manifest_size;
+}
+
+int mikrobus_manifest_parse(struct addon_board_info *board, void *data,
+ size_t size)
+{
+ struct greybus_manifest_header *header;
+ struct greybus_manifest *manifest;
+ struct greybus_descriptor *desc;
+ int dev_count, desc_size, ret;
+ u16 manifest_size;
+
+ if (size < sizeof(*header)) {
+ pr_err("short manifest (%zu < %zu)", size, sizeof(*header));
+ return -EINVAL;
+ }
+
+ ret = mikrobus_manifest_header_validate(data, size);
+ if (ret < 0) {
+ pr_err("invalid manifest header: %u", manifest_size);
+ return ret;
+ }
+
+ manifest = data;
+ manifest_size = ret;
+
+ if (manifest_size != size) {
+ pr_err("invalid manifest size(%zu < %u)", size, manifest_size);
+ return -EINVAL;
+ }
+
+ desc = manifest->descriptors;
+ size -= sizeof(*header);
+ while (size) {
+ desc_size = board_descriptor_add(board, desc, size);
+ if (desc_size < 0) {
+ pr_err("invalid manifest descriptor, size: %u",
+ desc_size);
+ return -EINVAL;
+ }
+
+ desc = (void *)desc + desc_size;
+ size -= desc_size;
+ }
+
+ mikrobus_state_get(board);
+ dev_count = mikrobus_manifest_parse_devices(board);
+ pr_info("%s manifest parsed with %d devices", board->name, dev_count);
+ manifest_descriptor_release_all(board);
+
+ return dev_count;
+}
diff --git a/drivers/misc/mikrobus/mikrobus_manifest.h b/drivers/misc/mikrobus/mikrobus_manifest.h
new file mode 100644
index 000000000000..39ae53a25fc4
--- /dev/null
+++ b/drivers/misc/mikrobus/mikrobus_manifest.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * mikroBUS manifest definition
+ * extension to Greybus Manifest Definition
+ *
+ * Copyright 2014-2015 Google Inc.
+ * Copyright 2014-2015 Linaro Ltd.
+ *
+ * Released under the GPLv2 and BSD licenses.
+ */
+
+#ifndef __MIKROBUS_MANIFEST_H
+#define __MIKROBUS_MANIFEST_H
+
+#include "mikrobus_core.h"
+
+/*
+ * mikrobus_manifest_header - parse mikroBUS manifest
+ *
+ * @info: addon board info structure to populate with parsed information
+ * @data: pointer to the manifest blob
+ * @size: size of the manifest blob
+ *
+ * returns: number of devices on success, negative error code on failure
+ */
+int mikrobus_manifest_parse(struct addon_board_info *info, void *data,
+ size_t size);
+
+#endif /* __MIKROBUS_MANIFEST_H */
--
2.44.0

2024-03-17 19:39:28

[permalink] [raw]

Subject: [PATCH v4 5/5] dts: ti: k3-am625-beagleplay: Add mikroBUS

DONOTMERGE

this patch depends on patch 1

Add mikroBUS connector support for Beagleplay.

Co-developed-by: Vaishnav M A <[email protected]>
Signed-off-by: Vaishnav M A <[email protected]>
Signed-off-by: Ayush Singh <[email protected]>
---
.../arm64/boot/dts/ti/k3-am625-beagleplay.dts | 80 +++++++++++++++++--
1 file changed, 72 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
index a34e0df2ab86..e1dce1b80153 100644
--- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
+++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
@@ -29,6 +29,7 @@ aliases {
i2c3 = &main_i2c3;
i2c4 = &wkup_i2c0;
i2c5 = &mcu_i2c0;
+ mikrobus0 = &mikrobus0;
mmc0 = &sdhci0;
mmc1 = &sdhci1;
mmc2 = &sdhci2;
@@ -230,6 +231,38 @@ simple-audio-card,codec {
};
};

+ mikrobus0: mikrobus-connector {
+ compatible = "mikrobus-connector";
+ pinctrl-names = "default", "pwm_default", "pwm_gpio",
+ "uart_default", "uart_gpio", "i2c_default",
+ "i2c_gpio", "spi_default", "spi_gpio";
+ pinctrl-0 = <&mikrobus_gpio_pins_default>;
+ pinctrl-1 = <&mikrobus_pwm_pins_default>;
+ pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
+ pinctrl-3 = <&mikrobus_uart_pins_default>;
+ pinctrl-4 = <&mikrobus_uart_pins_gpio>;
+ pinctrl-5 = <&mikrobus_i2c_pins_default>;
+ pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
+ pinctrl-7 = <&mikrobus_spi_pins_default>;
+ pinctrl-8 = <&mikrobus_spi_pins_gpio>;
+ pwms = <&ecap2 0 500000 0>;
+ i2c-adapter = <&main_i2c3>;
+ spi-controller = <&main_spi2>;
+ uart = <&main_uart5>;
+ spi-cs = <0 1>;
+ mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
+ <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
+ };
};

&main_pmx0 {
@@ -389,6 +422,18 @@ AM62X_IOPAD(0x01f0, PIN_OUTPUT, 5) /* (A18) EXT_REFCLK1.CLKOUT0 */
>;
};

+ mikrobus_pwm_pins_default: mikrobus-pwm-default-pins {
+ pinctrl-single,pins = <
+ AM62X_IOPAD(0x01a4, PIN_INPUT, 2) /* (B20) MCASP0_ACLKX.ECAP2_IN_APWM_OUT */
+ >;
+ };
+
+ mikrobus_pwm_pins_gpio: mikrobus-pwm-gpio-pins {
+ pinctrl-single,pins = <
+ AM62X_IOPAD(0x01a4, PIN_INPUT, 7) /* (B20) MCASP0_ACLKX.GPIO1_11 */
+ >;
+ };
+
mikrobus_i2c_pins_default: mikrobus-i2c-default-pins {
pinctrl-single,pins = <
AM62X_IOPAD(0x01d0, PIN_INPUT_PULLUP, 2) /* (A15) UART0_CTSn.I2C3_SCL */
@@ -396,6 +441,13 @@ AM62X_IOPAD(0x01d4, PIN_INPUT_PULLUP, 2) /* (B15) UART0_RTSn.I2C3_SDA */
>;
};

+ mikrobus_i2c_pins_gpio: mikrobus-i2c-gpio-pins {
+ pinctrl-single,pins = <
+ AM62X_IOPAD(0x01d0, PIN_INPUT, 7) /* (A15) UART0_CTSn.GPIO1_22 */
+ AM62X_IOPAD(0x01d4, PIN_INPUT, 7) /* (B15) UART0_RTSn.GPIO1_23 */
+ >;
+ };
+
mikrobus_uart_pins_default: mikrobus-uart-default-pins {
pinctrl-single,pins = <
AM62X_IOPAD(0x01d8, PIN_INPUT, 1) /* (C15) MCAN0_TX.UART5_RXD */
@@ -403,6 +455,13 @@ AM62X_IOPAD(0x01dc, PIN_OUTPUT, 1) /* (E15) MCAN0_RX.UART5_TXD */
>;
};

+ mikrobus_uart_pins_gpio: mikrobus-uart-gpio-pins {
+ pinctrl-single,pins = <
+ AM62X_IOPAD(0x01d8, PIN_INPUT, 7) /* (C15) MCAN0_TX.GPIO1_24 */
+ AM62X_IOPAD(0x01dc, PIN_INPUT, 7) /* (E15) MCAN0_RX.GPIO1_25 */
+ >;
+ };
+
mikrobus_spi_pins_default: mikrobus-spi-default-pins {
pinctrl-single,pins = <
AM62X_IOPAD(0x01b0, PIN_INPUT, 1) /* (A20) MCASP0_ACLKR.SPI2_CLK */
@@ -412,6 +471,15 @@ AM62X_IOPAD(0x0198, PIN_INPUT, 1) /* (A19) MCASP0_AXR2.SPI2_D1 */
>;
};

+ mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
+ pinctrl-single,pins = <
+ AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) MCASP0_AXR3.GPIO1_7 */
+ AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) MCASP0_AXR2.GPIO1_8 */
+ AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) MCASP0_AFSR.GPIO1_13 */
+ AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) MCASP0_ACLKR.GPIO1_14 */
+ >;
+ };
+
mikrobus_gpio_pins_default: mikrobus-gpio-default-pins {
bootph-all;
pinctrl-single,pins = <
@@ -629,8 +697,6 @@ &main_gpio0 {

&main_gpio1 {
bootph-all;
- pinctrl-names = "default";
- pinctrl-0 = <&mikrobus_gpio_pins_default>;
gpio-line-names = "", "", "", "", "", /* 0-4 */
"SPE_RSTN", "SPE_INTN", "MIKROBUS_GPIO1_7", /* 5-7 */
"MIKROBUS_GPIO1_8", "MIKROBUS_GPIO1_9", /* 8-9 */
@@ -803,15 +869,11 @@ it66121_out: endpoint {
};

&main_i2c3 {
- pinctrl-names = "default";
- pinctrl-0 = <&mikrobus_i2c_pins_default>;
clock-frequency = <400000>;
status = "okay";
};

&main_spi2 {
- pinctrl-names = "default";
- pinctrl-0 = <&mikrobus_spi_pins_default>;
status = "okay";
};

@@ -875,8 +937,6 @@ &main_uart1 {
};

&main_uart5 {
- pinctrl-names = "default";
- pinctrl-0 = <&mikrobus_uart_pins_default>;
status = "okay";
};

@@ -926,3 +986,7 @@ &mcasp1 {
tx-num-evt = <32>;
rx-num-evt = <32>;
};
+
+&ecap2 {
+ status = "okay";
+};
--
2.44.0

2024-03-17 19:47:05

[permalink] [raw]

Subject: [PATCH v4 3/5] greybus: Add mikroBUS manifest types

DONOTMERGE

mikroBUS addon boards allow using same mikroBUS connector for a wide
range of peripherals. It is also possible for the addon board not to use
all the pins in mikroBUS socket (marked by NC or Not Connected). This
would require the need to create an almost new overlays for each
permutation of the hardware.

To overcome this, a manifest format based on Greybus manifest
specification was created which allows describing mikroBUS addon boards.
The reason for choosing greybus manifest for the identifier is that so far
we discussed only about physical mikroBUS ports on the board, but we can
also have mikroBUS ports on a remote microcontroller appearing on host
over greybus and there a device tree overlay solution does not work as the
standard identifier mechanism.

The patch introduces 3 new greybus descriptor types:
1. mikrobus-descriptor:
Is a fixed-length descriptor (12 bytes), and the manifest shall have
precisely one mikroBUS descriptor. Each byte describes a configuration
of the corresponding pin on the mikroBUS addon board in a clockwise
direction starting from the PWM pin omitting power (VCC and ground)
pins as same as the default state of the pin.
There are mikroBUS addon boards that use some dedicated SPI, UART, PWM,
and I2C pins as GPIO pins, so it is necessary to redefine the default
pin configuration of that pins on the host system. Also, sometimes it is
required the pull-up on the host pin for correct functionality
2. property-descriptor:
Are used to pass named properties or named GPIOs to the host. The host
system uses this information to properly configure specific board
drivers by passing the properties and GPIO name. There can be multiple
instances of property descriptors per add-on board manifest.
3. device-descriptor:
Describes a device on the mikroBUS port. The device descriptor is a
fixed-length descriptor, and there can be multiple instances of device
descriptors in an add-on board manifest in cases where the add-on board
presents more than one device to the host.

New mikroBUS addon boards also sometimes contain a 1-wire EEPROM with
the mikroBUS manifest, thus enabling plug and play support.

I have also created PR to add mikrobus descriptors in Greybus spec and I
think the old PR on manifesto by Vaishnav should also work. However,
both of these repositories seem to be inactive. I am guessing the
greybus mailing list can provide more information on how I should go
about these.

Here is a sample mikroBUS manifest:
```
;;
; PRESSURE CLICK
; https://www.mikroe.com/pressure-click
; CONFIG_IIO_ST_PRESS
;
; Copyright 2020 BeagleBoard.org Foundation
; Copyright 2020 Texas Instruments
;

[manifest-header]
version-major = 0
version-minor = 1

[interface-descriptor]
vendor-string-id = 1
product-string-id = 2

[string-descriptor 1]
string = MIKROE

[string-descriptor 2]
string = Pressure

[mikrobus-descriptor]
pwm-state = 4
int-state = 1
rx-state = 7
tx-state = 7
scl-state = 6
sda-state = 6
mosi-state = 5
miso-state = 5
sck-state = 5
cs-state = 5
rst-state = 2
an-state = 1

[device-descriptor 1]
driver-string-id = 3
protocol = 0x3
reg = 0x5d

[string-descriptor 3]
string = lps331ap
```

Link: https://www.mikroe.com/clickid ClickID
Link:
https://docs.beagleboard.org/latest/projects/beagleconnect/index.html
beagleconnect
Link: https://github.com/projectara/greybus-spec Greybus Spec
Link: https://github.com/projectara/greybus-spec/pull/4 Greybus Spec PR
Link: https://github.com/projectara/manifesto/pull/2 manifesto PR

Co-developed-by: Vaishnav M A <[email protected]>
Signed-off-by: Vaishnav M A <[email protected]>
Signed-off-by: Ayush Singh <[email protected]>
---
include/linux/greybus/greybus_manifest.h | 49 ++++++++++++++++++++++++
1 file changed, 49 insertions(+)

diff --git a/include/linux/greybus/greybus_manifest.h b/include/linux/greybus/greybus_manifest.h
index bef9eb2093e9..83241e19d9b3 100644
--- a/include/linux/greybus/greybus_manifest.h
+++ b/include/linux/greybus/greybus_manifest.h
@@ -23,6 +23,9 @@ enum greybus_descriptor_type {
GREYBUS_TYPE_STRING = 0x02,
GREYBUS_TYPE_BUNDLE = 0x03,
GREYBUS_TYPE_CPORT = 0x04,
+ GREYBUS_TYPE_MIKROBUS = 0x05,
+ GREYBUS_TYPE_PROPERTY = 0x06,
+ GREYBUS_TYPE_DEVICE = 0x07,
};

enum greybus_protocol {
@@ -151,6 +154,49 @@ struct greybus_descriptor_cport {
__u8 protocol_id; /* enum greybus_protocol */
} __packed;

+/*
+ * A mikrobus descriptor is used to describe the details
+ * about the bus ocnfiguration for the add-on board
+ * connected to the mikrobus port.
+ */
+struct greybus_descriptor_mikrobus {
+ __u8 pin_state[12];
+} __packed;
+
+/*
+ * A property descriptor is used to pass named properties
+ * to device drivers through the unified device properties
+ * interface under linux/property.h
+ */
+struct greybus_descriptor_property {
+ __u8 length;
+ __u8 id;
+ __u8 propname_stringid;
+ __u8 type;
+ __u8 value[];
+} __packed;
+
+/*
+ * A device descriptor is used to describe the
+ * details required by a add-on board device
+ * driver.
+ */
+struct greybus_descriptor_device {
+ __u8 id;
+ __u8 driver_stringid;
+ __u8 protocol;
+ __u8 reg;
+ __le32 max_speed_hz;
+ __u8 irq;
+ __u8 irq_type;
+ __u8 mode;
+ __u8 prop_link;
+ __u8 gpio_link;
+ __u8 reg_link;
+ __u8 clock_link;
+ __u8 pad[1];
+} __packed;
+
struct greybus_descriptor_header {
__le16 size;
__u8 type; /* enum greybus_descriptor_type */
@@ -164,6 +210,9 @@ struct greybus_descriptor {
struct greybus_descriptor_interface interface;
struct greybus_descriptor_bundle bundle;
struct greybus_descriptor_cport cport;
+ struct greybus_descriptor_mikrobus mikrobus;
+ struct greybus_descriptor_property property;
+ struct greybus_descriptor_device device;
};
} __packed;

--
2.44.0

2024-03-17 23:05:06

by Randy Dunlap

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

Hi--

On 3/17/24 12:37, Ayush Singh wrote:
> diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig
> new file mode 100644
> index 000000000000..aa57b994dc66
> --- /dev/null
> +++ b/drivers/misc/mikrobus/Kconfig
> @@ -0,0 +1,15 @@
> +menuconfig MIKROBUS
> + tristate "Module for instantiating devices on mikroBUS ports"
> + depends on GPIOLIB
> + help
> + This option enables the mikroBUS driver. mikroBUS is an add-on
> + board socket standard that offers maximum expandability with
> + the smallest number of pins. The mikroBUS driver instantiates
> + devices on a mikroBUS port described mikroBUS manifest which is

I can't quite parse that line ^^^. Is it like this?

devices on a mikroBUS port described by a mikroBUS manifest which is

> + passed using a sysfs interface.
> +
> +
> + Say Y here to enable support for this driver.
> +
> + To compile this code as a module, chose M here: the module
> + will be called mikrobus.ko

--
#Randy

2024-03-18 12:23:55

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On Sun Mar 17, 2024 at 8:37 PM CET, Ayush Singh wrote:
> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
> developed by MikroElektronika for connecting add-on boards to
> microcontrollers or microprocessors.
>
> mikroBUS is a connector and does not have a controller. Instead the
> software is responsible for identification of board and setting up /
> registering uart, spi, i2c, pwm and other buses. Thus it needs a way to
> get uart, spi, i2c, pwm and gpio controllers / adapters.
>
> A mikroBUS addon board is free to leave some of the pins unused which
> are marked as NC or Not Connected.
>
> Some of the pins might need to be configured as GPIOs deviating from their
> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.
>
> For some add-on boards the driver may not take care of some additional
> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
> (RST pin on the mikrobus port) needs to be pulled high.
>
> Here's the list of pins in mikroBUS connector:
> Analog - AN
> Reset - RST
> SPI Chip Select - CS
> SPI Clock - SCK
> SPI Master Input Slave Output - MISO
> SPI Master Output Slave Input - MOSI
> VCC-3.3V power - +3.3V
> Reference Ground - GND
> PWM - PWM output
> INT - Hardware Interrupt
> RX - UART Receive
> TX - UART Transmit
> SCL - I2C Clock
> SDA - I2C Data
> +5V - VCC-5V power
> GND - Reference Ground
>
> Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains
> a manifest to describe the addon board to provide plug and play
> capabilities.
>
> Link: https://www.mikroe.com/mikrobus
> Link:
> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
> mikroBUS specification
> Link: https://www.mikroe.com/sht1x-click SHT15 Click
> Link: https://www.mikroe.com/eth-click ENC28J60 Click
> Link: https://www.mikroe.com/clickid ClickID
>
> Co-developed-by: Vaishnav M A <[email protected]>
> Signed-off-by: Vaishnav M A <[email protected]>
> Signed-off-by: Ayush Singh <[email protected]>
> ---
> .../connector/mikrobus-connector.yaml | 113 ++++++++++++++++++

See also
https://lore.kernel.org/r/YmFo+EntwxIsco%[email protected]/

Looks like this proposal doesn't have the subnodes. How do you
attach a kernel driver to it's spi port for example? Only through
the manifest files?

-michael

Attachments:

signature.asc (259.00 B)

2024-03-18 17:28:49

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 3/18/24 17:52, Michael Walle wrote:

> On Sun Mar 17, 2024 at 8:37 PM CET, Ayush Singh wrote:
>> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
>> developed by MikroElektronika for connecting add-on boards to
>> microcontrollers or microprocessors.
>>
>> mikroBUS is a connector and does not have a controller. Instead the
>> software is responsible for identification of board and setting up /
>> registering uart, spi, i2c, pwm and other buses. Thus it needs a way to
>> get uart, spi, i2c, pwm and gpio controllers / adapters.
>>
>> A mikroBUS addon board is free to leave some of the pins unused which
>> are marked as NC or Not Connected.
>>
>> Some of the pins might need to be configured as GPIOs deviating from their
>> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
>> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.
>>
>> For some add-on boards the driver may not take care of some additional
>> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
>> (RST pin on the mikrobus port) needs to be pulled high.
>>
>> Here's the list of pins in mikroBUS connector:
>> Analog - AN
>> Reset - RST
>> SPI Chip Select - CS
>> SPI Clock - SCK
>> SPI Master Input Slave Output - MISO
>> SPI Master Output Slave Input - MOSI
>> VCC-3.3V power - +3.3V
>> Reference Ground - GND
>> PWM - PWM output
>> INT - Hardware Interrupt
>> RX - UART Receive
>> TX - UART Transmit
>> SCL - I2C Clock
>> SDA - I2C Data
>> +5V - VCC-5V power
>> GND - Reference Ground
>>
>> Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains
>> a manifest to describe the addon board to provide plug and play
>> capabilities.
>>
>> Link: https://www.mikroe.com/mikrobus
>> Link:
>> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
>> mikroBUS specification
>> Link: https://www.mikroe.com/sht1x-click SHT15 Click
>> Link: https://www.mikroe.com/eth-click ENC28J60 Click
>> Link: https://www.mikroe.com/clickid ClickID
>>
>> Co-developed-by: Vaishnav M A <[email protected]>
>> Signed-off-by: Vaishnav M A <[email protected]>
>> Signed-off-by: Ayush Singh <[email protected]>
>> ---
>> .../connector/mikrobus-connector.yaml | 113 ++++++++++++++++++
> See also
> https://lore.kernel.org/r/YmFo+EntwxIsco%[email protected]/
>
> Looks like this proposal doesn't have the subnodes. How do you
> attach a kernel driver to it's spi port for example? Only through
> the manifest files?
>
> -michael

So I looked at the Patch, and it seems the approach of fundamentally
different than this PR. So, let me try to explain what this patch set
does for an add-on board using SPI.

The device tree defines the SPI controller associated with mikroBUS SPI
pins. The driver on match queries and takes a reference to the SPI
controller but does nothing with it. Once a mikroBUS add-on board is
detected (by passing manifest using sysfs or reading from 1-wire
EEPROM), the driver parses the manifest, and if it detects an SPI device
in manifest, it registers SPI device along with setting properties such
as `chip_select`, `max_speed_hz`, `mode`, etc., which are defined in the
manifest. On board removal, it unregisters the SPI device and waits for
a new mikroBUS board to be detected again.

It is also possible for SPI not to be used by a device, in which case,
no SPI device is registered to the controller. It is also possible that
the SPI pins will be used as normal GPIOs. Everything is identified from
the manifest.

Ayush Singh

2024-03-18 17:37:17

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

…
> +++ b/drivers/misc/mikrobus/mikrobus_core.c
…
> +static int mikrobus_pinctrl_select(struct mikrobus_port *port,
> + const char *pinctrl_selected)
> +{
> + struct pinctrl_state *state;
> + int ret;
> +
> + state = pinctrl_lookup_state(port->pinctrl, pinctrl_selected);
> + if (IS_ERR(state)) {
> + return dev_err_probe(&port->dev, PTR_ERR(state),
> + "failed to find state %s",
> + pinctrl_selected);
> + }
…

I suggest to reconsider the need for extra curly brackets here.

See also:
Section “3) Placing Braces and Spaces”
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.8#n197

Regards,
Markus

2024-03-18 17:58:55

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

…
> +++ b/drivers/misc/mikrobus/mikrobus_core.c
…
> +static int mikrobus_pinctrl_setup(struct mikrobus_port *port,
> + struct addon_board_info *board)
> +{
> + int ret;
> +
> + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> + ret = mikrobus_pinctrl_select(port, "pwm_default");
> + else
> + ret = mikrobus_pinctrl_select(port, "pwm_gpio");
…

How do you think about to avoid the specification of a bit of duplicate source code here
by using conditional operator expressions?

ret = mikrobus_pinctrl_select(port,
((!board ||
board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
? "pwm_default"
: "pwm_gpio"));

Regards,
Markus

2024-03-18 18:12:58

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

…
> +++ b/drivers/misc/mikrobus/mikrobus_core.h
…
> +#ifndef __MIKROBUS_H
> +#define __MIKROBUS_H
…

I suggest to avoid the specification of leading underscores for include guards.

See also:
https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier#DCL37C.Donotdeclareordefineareservedidentifier-NoncompliantCodeExample%28IncludeGuard%29

Regards,
Markus

2024-03-18 18:42:14

by Alex Elder

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

On 3/18/24 12:58 PM, Markus Elfring wrote:
> …
>> +++ b/drivers/misc/mikrobus/mikrobus_core.c
> …
>> +static int mikrobus_pinctrl_setup(struct mikrobus_port *port,
>> + struct addon_board_info *board)
>> +{
>> + int ret;
>> +
>> + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
>> + ret = mikrobus_pinctrl_select(port, "pwm_default");
>> + else
>> + ret = mikrobus_pinctrl_select(port, "pwm_gpio");
> …
>
> How do you think about to avoid the specification of a bit of duplicate source code here
> by using conditional operator expressions?
>
> ret = mikrobus_pinctrl_select(port,
> ((!board ||
> board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> ? "pwm_default"
> : "pwm_gpio"));

No.

It's a complex enough bit of logic without trying to bury
it inside the parameters passed to the function.

-Alex

>
>
> Regards,
> Markus
>

2024-03-18 18:56:00

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

On Mon, Mar 18, 2024 at 01:41:38PM -0500, Alex Elder wrote:
> On 3/18/24 12:58 PM, Markus Elfring wrote:
> > …
> > > +++ b/drivers/misc/mikrobus/mikrobus_core.c
> > …
> > > +static int mikrobus_pinctrl_setup(struct mikrobus_port *port,
> > > + struct addon_board_info *board)
> > > +{
> > > + int ret;
> > > +
> > > + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> > > + ret = mikrobus_pinctrl_select(port, "pwm_default");
> > > + else
> > > + ret = mikrobus_pinctrl_select(port, "pwm_gpio");
> > …
> >
> > How do you think about to avoid the specification of a bit of duplicate source code here
> > by using conditional operator expressions?
> >
> > ret = mikrobus_pinctrl_select(port,
> > ((!board ||
> > board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> > ? "pwm_default"
> > : "pwm_gpio"));
>
> No.
>
> It's a complex enough bit of logic without trying to bury
> it inside the parameters passed to the function.

Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list. I strongly suggest that you not do this anymore. Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot

2024-03-19 05:58:52

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 18/03/2024 18:20, Ayush Singh wrote:
> On 3/18/24 17:52, Michael Walle wrote:
>
>> On Sun Mar 17, 2024 at 8:37 PM CET, Ayush Singh wrote:
>>> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
>>> developed by MikroElektronika for connecting add-on boards to
>>> microcontrollers or microprocessors.
>>>
>>> mikroBUS is a connector and does not have a controller. Instead the
>>> software is responsible for identification of board and setting up /
>>> registering uart, spi, i2c, pwm and other buses. Thus it needs a way to
>>> get uart, spi, i2c, pwm and gpio controllers / adapters.
>>>
>>> A mikroBUS addon board is free to leave some of the pins unused which
>>> are marked as NC or Not Connected.
>>>
>>> Some of the pins might need to be configured as GPIOs deviating from their
>>> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
>>> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.
>>>
>>> For some add-on boards the driver may not take care of some additional
>>> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
>>> (RST pin on the mikrobus port) needs to be pulled high.
>>>
>>> Here's the list of pins in mikroBUS connector:
>>> Analog - AN
>>> Reset - RST
>>> SPI Chip Select - CS
>>> SPI Clock - SCK
>>> SPI Master Input Slave Output - MISO
>>> SPI Master Output Slave Input - MOSI
>>> VCC-3.3V power - +3.3V
>>> Reference Ground - GND
>>> PWM - PWM output
>>> INT - Hardware Interrupt
>>> RX - UART Receive
>>> TX - UART Transmit
>>> SCL - I2C Clock
>>> SDA - I2C Data
>>> +5V - VCC-5V power
>>> GND - Reference Ground
>>>
>>> Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains
>>> a manifest to describe the addon board to provide plug and play
>>> capabilities.
>>>
>>> Link: https://www.mikroe.com/mikrobus
>>> Link:
>>> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
>>> mikroBUS specification
>>> Link: https://www.mikroe.com/sht1x-click SHT15 Click
>>> Link: https://www.mikroe.com/eth-click ENC28J60 Click
>>> Link: https://www.mikroe.com/clickid ClickID
>>>
>>> Co-developed-by: Vaishnav M A <[email protected]>
>>> Signed-off-by: Vaishnav M A <[email protected]>
>>> Signed-off-by: Ayush Singh <[email protected]>
>>> ---
>>> .../connector/mikrobus-connector.yaml | 113 ++++++++++++++++++
>> See also
>> https://lore.kernel.org/r/YmFo+EntwxIsco%[email protected]/
>>
>> Looks like this proposal doesn't have the subnodes. How do you
>> attach a kernel driver to it's spi port for example? Only through
>> the manifest files?
>>
>> -michael
>
>
> So I looked at the Patch, and it seems the approach of fundamentally
> different than this PR. So, let me try to explain what this patch set
> does for an add-on board using SPI.
>
> The device tree defines the SPI controller associated with mikroBUS SPI
> pins. The driver on match queries and takes a reference to the SPI
> controller but does nothing with it. Once a mikroBUS add-on board is
> detected (by passing manifest using sysfs or reading from 1-wire
> EEPROM), the driver parses the manifest, and if it detects an SPI device

As I understood Mikrobus does not have EEPROM.

> in manifest, it registers SPI device along with setting properties such
> as `chip_select`, `max_speed_hz`, `mode`, etc., which are defined in the
> manifest. On board removal, it unregisters the SPI device and waits for
> a new mikroBUS board to be detected again.

You explained drivers, not hardware for DT.

>
> It is also possible for SPI not to be used by a device, in which case,
> no SPI device is registered to the controller. It is also possible that
> the SPI pins will be used as normal GPIOs. Everything is identified from
> the manifest.

Best regards,
Krzysztof

2024-03-19 05:59:26

[permalink] [raw]

Subject: Re: [PATCH v4 5/5] dts: ti: k3-am625-beagleplay: Add mikroBUS

On 17/03/2024 20:37, Ayush Singh wrote:
> DONOTMERGE

Why? Explain then the purpose of this patch.

>
> this patch depends on patch 1

How? I don't see any dependency.

>
> Add mikroBUS connector support for Beagleplay.
>
> Co-developed-by: Vaishnav M A <[email protected]>
> Signed-off-by: Vaishnav M A <[email protected]>
> Signed-off-by: Ayush Singh <[email protected]>
> ---
> .../arm64/boot/dts/ti/k3-am625-beagleplay.dts | 80 +++++++++++++++++--
> 1 file changed, 72 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> index a34e0df2ab86..e1dce1b80153 100644
> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> @@ -29,6 +29,7 @@ aliases {
> i2c3 = &main_i2c3;
> i2c4 = &wkup_i2c0;
> i2c5 = &mcu_i2c0;
> + mikrobus0 = &mikrobus0;
> mmc0 = &sdhci0;
> mmc1 = &sdhci1;
> mmc2 = &sdhci2;
> @@ -230,6 +231,38 @@ simple-audio-card,codec {
> };
> };
>

Best regards,
Krzysztof

2024-03-19 06:03:36

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 17/03/2024 20:37, Ayush Singh wrote:
> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
> developed by MikroElektronika for connecting add-on boards to
> microcontrollers or microprocessors.
>

..

> +title: mikroBUS add-on board socket
> +
> +maintainers:
> + - Ayush Singh <[email protected]>
> +
> +properties:
> + compatible:
> + const: mikrobus-connector
> +
> + pinctrl-0: true
> + pinctrl-1: true
> + pinctrl-2: true
> + pinctrl-3: true
> + pinctrl-4: true
> + pinctrl-5: true
> + pinctrl-6: true
> + pinctrl-7: true
> + pinctrl-8: true
> +
> + pinctrl-names:
> + items:
> + - const: default
> + - const: pwm_default
> + - const: pwm_gpio
> + - const: uart_default
> + - const: uart_gpio
> + - const: i2c_default
> + - const: i2c_gpio
> + - const: spi_default
> + - const: spi_gpio
> +
> + mikrobus-gpios:
> + minItems: 11
> + maxItems: 12

I don't see any of the issues resolved which I raised at v3. I think
Russell pointed that you do not have EEPROM and that some pins are
optional. You do not allow that.

Plus I don't see him being Cced but he had quite detailed look and
comments at your patchset, so *you are supposed to Cc* him.

I also do not see Rob's comments fully addressed.

Do not send next versions before resolving previous discusssion.

> +
> + i2c-adapter:
> + description: i2c adapter attached to the mikrobus socket.
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + spi-controller:
> + description: spi bus number of the spi-master attached to the mikrobus socket.
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + uart:
> + description: uart port attached to the mikrobus socket
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + pwms:
> + description: the pwm-controller corresponding to the mikroBUS PWM pin.
> + maxItems: 1
> +
> + spi-cs:
> + description: spi chip-select numbers corresponding to the chip-selects on the mikrobus socket.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - description: chip select corresponding to CS pin
> + - description: chip select corresponding to RST pin
> +
> +required:
> + - compatible
> + - pinctrl-0
> + - pinctrl-1
> + - pinctrl-2
> + - pinctrl-3
> + - pinctrl-4
> + - pinctrl-5
> + - pinctrl-6
> + - pinctrl-7
> + - pinctrl-8
> + - i2c-adapter
> + - spi-controller
> + - spi-cs
> + - uart
> + - pwms
> + - mikrobus-gpios
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + mikrobus {
> + compatible = "mikrobus-connector";
> + pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default",

Please properly wrap your code according to Linux and DTS coding style
documents.

Best regards,
Krzysztof

2024-03-19 06:04:31

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

On 17/03/2024 20:37, Ayush Singh wrote:
> DONOTMERGE
>
> this patch depends on Patch 1, 2, 3

So none of your work should be reviewed? I don't understand this, but in
such case I am not going to review it.

Best regards,
Krzysztof

2024-03-19 06:40:48

[permalink] [raw]

Subject: Re: [PATCH v4 5/5] dts: ti: k3-am625-beagleplay: Add mikroBUS

On 3/19/24 11:29, Krzysztof Kozlowski wrote:
> On 17/03/2024 20:37, Ayush Singh wrote:
>> DONOTMERGE
> Why? Explain then the purpose of this patch.

Well, it was suggested to have DONOTMERGE by Vaishnav in the patches
until dt bindings have been approved, since this patch touches different
subsystems. Here is the full context from v3:

> The reasoning behind this is that these patches go in to separate maintainer trees and without the bindings merged first the device tree changes cannot be validated, thus it is a usual practice to get the bindings and driver merged first and the device tree changes to go in the next cycle. Another alternative is you can point to your fork with all the changes together.

>> this patch depends on patch 1
> How? I don't see any dependency.

I think it is not allowed to have code in device tree unless a
corresponding dt-binding exists for the device. And thus every time the
dt-binding changes, this patch also needs to change.So I thought it is
dependent on patch 1.

>> Add mikroBUS connector support for Beagleplay.
>>
>> Co-developed-by: Vaishnav M A <[email protected]>
>> Signed-off-by: Vaishnav M A <[email protected]>
>> Signed-off-by: Ayush Singh <[email protected]>
>> ---
>> .../arm64/boot/dts/ti/k3-am625-beagleplay.dts | 80 +++++++++++++++++--
>> 1 file changed, 72 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> index a34e0df2ab86..e1dce1b80153 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> @@ -29,6 +29,7 @@ aliases {
>> i2c3 = &main_i2c3;
>> i2c4 = &wkup_i2c0;
>> i2c5 = &mcu_i2c0;
>> + mikrobus0 = &mikrobus0;
>> mmc0 = &sdhci0;
>> mmc1 = &sdhci1;
>> mmc2 = &sdhci2;
>> @@ -230,6 +231,38 @@ simple-audio-card,codec {
>> };
>> };
>>
>
> Best regards,
> Krzysztof

Link:
https://lore.kernel.org/lkml/CALudOK5v_uCUffxHGKS-jA-DKLNV7xwmKkxJwjHaMWWgDdPDqA@mail.gmail.com/
Patch v3

Ayush Singh

2024-03-19 06:44:11

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 3/19/24 11:33, Krzysztof Kozlowski wrote:
> On 17/03/2024 20:37, Ayush Singh wrote:
>> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
>> developed by MikroElektronika for connecting add-on boards to
>> microcontrollers or microprocessors.
>>
> ...
>
>> +title: mikroBUS add-on board socket
>> +
>> +maintainers:
>> + - Ayush Singh <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + const: mikrobus-connector
>> +
>> + pinctrl-0: true
>> + pinctrl-1: true
>> + pinctrl-2: true
>> + pinctrl-3: true
>> + pinctrl-4: true
>> + pinctrl-5: true
>> + pinctrl-6: true
>> + pinctrl-7: true
>> + pinctrl-8: true
>> +
>> + pinctrl-names:
>> + items:
>> + - const: default
>> + - const: pwm_default
>> + - const: pwm_gpio
>> + - const: uart_default
>> + - const: uart_gpio
>> + - const: i2c_default
>> + - const: i2c_gpio
>> + - const: spi_default
>> + - const: spi_gpio
>> +
>> + mikrobus-gpios:
>> + minItems: 11
>> + maxItems: 12
> I don't see any of the issues resolved which I raised at v3. I think
> Russell pointed that you do not have EEPROM and that some pins are
> optional. You do not allow that.

So this patchset does not contain any EEPROM code. The bindings describe
mikroBUS connector and not mikroBUS addon board. While it is optional
for the mikroBUS addon board to not use sone pins (aka NC), the pins
still exist on the connector on the device side. It is not optional to
have pins in the host device.

> Plus I don't see him being Cced but he had quite detailed look and
> comments at your patchset, so *you are supposed to Cc* him.
>
> I also do not see Rob's comments fully addressed.
>
> Do not send next versions before resolving previous discusssion.

I apologize, I thought he was on the list by get_maintainers.pl, but it
seems I was mistaken. I will try to remember going forward.

>> +
>> + i2c-adapter:
>> + description: i2c adapter attached to the mikrobus socket.
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> + spi-controller:
>> + description: spi bus number of the spi-master attached to the mikrobus socket.
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> + uart:
>> + description: uart port attached to the mikrobus socket
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> + pwms:
>> + description: the pwm-controller corresponding to the mikroBUS PWM pin.
>> + maxItems: 1
>> +
>> + spi-cs:
>> + description: spi chip-select numbers corresponding to the chip-selects on the mikrobus socket.
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + items:
>> + - description: chip select corresponding to CS pin
>> + - description: chip select corresponding to RST pin
>> +
>> +required:
>> + - compatible
>> + - pinctrl-0
>> + - pinctrl-1
>> + - pinctrl-2
>> + - pinctrl-3
>> + - pinctrl-4
>> + - pinctrl-5
>> + - pinctrl-6
>> + - pinctrl-7
>> + - pinctrl-8
>> + - i2c-adapter
>> + - spi-controller
>> + - spi-cs
>> + - uart
>> + - pwms
>> + - mikrobus-gpios
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/gpio/gpio.h>
>> +
>> + mikrobus {
>> + compatible = "mikrobus-connector";
>> + pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default",
> Please properly wrap your code according to Linux and DTS coding style
> documents.
>
>
> Best regards,
> Krzysztof
>
Ayush Singh

2024-03-19 06:47:27

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

On 3/19/24 11:34, Krzysztof Kozlowski wrote:

> On 17/03/2024 20:37, Ayush Singh wrote:
>> DONOTMERGE
>>
>> this patch depends on Patch 1, 2, 3
> So none of your work should be reviewed? I don't understand this, but in
> such case I am not going to review it.
>
> Best regards,
> Krzysztof
>
I am a bit lost here. It was mentioned in the patch v3 that I should
specify the interdependence of patches in v3. And now you are saying I
should not?

Here is the rationale for the dependence:

1. Any changes to the property names in dt-bindings patch 1 will need an
appropriate change here.

2. This patch will fail to build without patch 2.

3. This patch will fail to build without patch 3.

Ayush Singh

2024-03-19 07:37:06

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 3/19/24 11:28, Krzysztof Kozlowski wrote:

> On 18/03/2024 18:20, Ayush Singh wrote:
>> On 3/18/24 17:52, Michael Walle wrote:
>>
>>> On Sun Mar 17, 2024 at 8:37 PM CET, Ayush Singh wrote:
>>>> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
>>>> developed by MikroElektronika for connecting add-on boards to
>>>> microcontrollers or microprocessors.
>>>>
>>>> mikroBUS is a connector and does not have a controller. Instead the
>>>> software is responsible for identification of board and setting up /
>>>> registering uart, spi, i2c, pwm and other buses. Thus it needs a way to
>>>> get uart, spi, i2c, pwm and gpio controllers / adapters.
>>>>
>>>> A mikroBUS addon board is free to leave some of the pins unused which
>>>> are marked as NC or Not Connected.
>>>>
>>>> Some of the pins might need to be configured as GPIOs deviating from their
>>>> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
>>>> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.
>>>>
>>>> For some add-on boards the driver may not take care of some additional
>>>> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
>>>> (RST pin on the mikrobus port) needs to be pulled high.
>>>>
>>>> Here's the list of pins in mikroBUS connector:
>>>> Analog - AN
>>>> Reset - RST
>>>> SPI Chip Select - CS
>>>> SPI Clock - SCK
>>>> SPI Master Input Slave Output - MISO
>>>> SPI Master Output Slave Input - MOSI
>>>> VCC-3.3V power - +3.3V
>>>> Reference Ground - GND
>>>> PWM - PWM output
>>>> INT - Hardware Interrupt
>>>> RX - UART Receive
>>>> TX - UART Transmit
>>>> SCL - I2C Clock
>>>> SDA - I2C Data
>>>> +5V - VCC-5V power
>>>> GND - Reference Ground
>>>>
>>>> Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains
>>>> a manifest to describe the addon board to provide plug and play
>>>> capabilities.
>>>>
>>>> Link: https://www.mikroe.com/mikrobus
>>>> Link:
>>>> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
>>>> mikroBUS specification
>>>> Link: https://www.mikroe.com/sht1x-click SHT15 Click
>>>> Link: https://www.mikroe.com/eth-click ENC28J60 Click
>>>> Link: https://www.mikroe.com/clickid ClickID
>>>>
>>>> Co-developed-by: Vaishnav M A <[email protected]>
>>>> Signed-off-by: Vaishnav M A <[email protected]>
>>>> Signed-off-by: Ayush Singh <[email protected]>
>>>> ---
>>>> .../connector/mikrobus-connector.yaml | 113 ++++++++++++++++++
>>> See also
>>> https://lore.kernel.org/r/YmFo+EntwxIsco%[email protected]/
>>>
>>> Looks like this proposal doesn't have the subnodes. How do you
>>> attach a kernel driver to it's spi port for example? Only through
>>> the manifest files?
>>>
>>> -michael
>>
>> So I looked at the Patch, and it seems the approach of fundamentally
>> different than this PR. So, let me try to explain what this patch set
>> does for an add-on board using SPI.
>>
>> The device tree defines the SPI controller associated with mikroBUS SPI
>> pins. The driver on match queries and takes a reference to the SPI
>> controller but does nothing with it. Once a mikroBUS add-on board is
>> detected (by passing manifest using sysfs or reading from 1-wire
>> EEPROM), the driver parses the manifest, and if it detects an SPI device
> As I understood Mikrobus does not have EEPROM.

mikroBUS add-on boards do not need to have EEPROM, but they can have it.
Simply put, EEPROM is not part of mikroBUS specification, but you will
find a lot (especially newer) addon boards with support for EEPROM manifest.

Regardless, this patch actually does not contain any code for EEPROM
support I have just mentioned it to give more context on why mikroBUS
manifest is the focus of this patch instead of DT overlay or something
else.

>> in manifest, it registers SPI device along with setting properties such
>> as `chip_select`, `max_speed_hz`, `mode`, etc., which are defined in the
>> manifest. On board removal, it unregisters the SPI device and waits for
>> a new mikroBUS board to be detected again.
> You explained drivers, not hardware for DT.

Yes, I was replying to the question posed by Michael. Since this happens
in the driver and not in the devicetree, I needed to explain the working
of the driver:

> How do you attach a kernel driver to it's spi port for example?

For more hardware side, the bindings are for mikrobus connector rather
than for any addon board. Thus, while an addon board might not use some
of the pins, the connector still needs to have all the pins and
associated controllers.

>> It is also possible for SPI not to be used by a device, in which case,
>> no SPI device is registered to the controller. It is also possible that
>> the SPI pins will be used as normal GPIOs. Everything is identified from
>> the manifest.
>
> Best regards,
> Krzysztof
>

Ayush Singh

2024-03-19 08:01:38

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

Hi Ayush,

On 19/03/24 12:17, Ayush Singh wrote:
> On 3/19/24 11:34, Krzysztof Kozlowski wrote:
>
>> On 17/03/2024 20:37, Ayush Singh wrote:
>>> DONOTMERGE
>>>
>>> this patch depends on Patch 1, 2, 3
>> So none of your work should be reviewed? I don't understand this, but in
>> such case I am not going to review it.
>>
>> Best regards,
>> Krzysztof
>>
> I am a bit lost here. It was mentioned in the patch v3 that I should
> specify the interdependence of patches in v3. And now you are saying I
> should not?
>

It was mentioned in v3 that patches that are independent should be sent
separately to the particular subsytem list and the dependencies should
be mentioned in this series, still in this series you have combined SPI
patches/platform DT changes along with the mikroBUS driver patches which
creates confusion.

This is what I mentioned as a response to your v3 series regarding
adding the patches

"The reasoning behind this is that these patches go in to separate
maintainer trees and without the bindings merged first the device tree
changes cannot be validated, thus it is a usual practice to get the
bindings and driver merged first and the device tree changes to go in
the next cycle. Another alternative is you can point to your fork with
all the changes together."

My suggestion was to get your series with the bindings and the base
driver support accepted/ready first and the send the platform specific
DT changes later. The rationale behind pointing to your fork with all
changes is to have all the changes (w1 EEPROM, instantiating remote
mikrobus ports over greybus .etc) together and ensure there are no
conflicts with the base series.

It looks like you have put DONOTMERGE on random patches (why is patch 3
and 4 marked as do not merge?)

Thanks and Regards,
Vaishnav

> Here is the rationale for the dependence:
>
> 1. Any changes to the property names in dt-bindings patch 1 will need an
> appropriate change here.
>
> 2. This patch will fail to build without patch 2.
>
> 3. This patch will fail to build without patch 3.
>
>
> Ayush Singh
>

2024-03-19 08:17:39

[permalink] [raw]

Subject: Re: [PATCH v4 2/5] spi: Make of_find_spi_controller_by_node() available

> This externalizes and exports the symbol
> of_find_spi_controller_by_node() from the SPI core akin to how
> of_find_i2c_adapter_by_node() is already available. As we will
> need this also for non-dynamic OF setups, we move it under a
> CONFIG_OF check.

Would a change description be more desirable with a corresponding imperative wording?

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.8#n94

Regards,
Markus

2024-03-19 08:27:15

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] greybus: Add mikroBUS manifest types

On 18/03/24 01:07, Ayush Singh wrote:
> DONOTMERGE
>

Why?

> mikroBUS addon boards allow using same mikroBUS connector for a wide
> range of peripherals. It is also possible for the addon board not to use
> all the pins in mikroBUS socket (marked by NC or Not Connected). This
> would require the need to create an almost new overlays for each
> permutation of the hardware.
>
> To overcome this, a manifest format based on Greybus manifest
> specification was created which allows describing mikroBUS addon boards.
> The reason for choosing greybus manifest for the identifier is that so far
> we discussed only about physical mikroBUS ports on the board, but we can

you will need to reword the commit message properly in imperative mood,
here and in multiple other places.

> also have mikroBUS ports on a remote microcontroller appearing on host
> over greybus and there a device tree overlay solution does not work as the
> standard identifier mechanism.
>
> The patch introduces 3 new greybus descriptor types:
> 1. mikrobus-descriptor:
> Is a fixed-length descriptor (12 bytes), and the manifest shall have
> precisely one mikroBUS descriptor. Each byte describes a configuration
> of the corresponding pin on the mikroBUS addon board in a clockwise
> direction starting from the PWM pin omitting power (VCC and ground)
> pins as same as the default state of the pin.
> There are mikroBUS addon boards that use some dedicated SPI, UART, PWM,
> and I2C pins as GPIO pins, so it is necessary to redefine the default
> pin configuration of that pins on the host system. Also, sometimes it is
> required the pull-up on the host pin for correct functionality
> 2. property-descriptor:
> Are used to pass named properties or named GPIOs to the host. The host
> system uses this information to properly configure specific board
> drivers by passing the properties and GPIO name. There can be multiple
> instances of property descriptors per add-on board manifest.
> 3. device-descriptor:
> Describes a device on the mikroBUS port. The device descriptor is a
> fixed-length descriptor, and there can be multiple instances of device
> descriptors in an add-on board manifest in cases where the add-on board
> presents more than one device to the host.
>
> New mikroBUS addon boards also sometimes contain a 1-wire EEPROM with
> the mikroBUS manifest, thus enabling plug and play support.
>

new mikroBUS sometimes contain an EEPROM? aren't these called Click ID
compliant add-on boards? there should be clarity in the commit message.

> I have also created PR to add mikrobus descriptors in Greybus spec and I
> think the old PR on manifesto by Vaishnav should also work. However,
> both of these repositories seem to be inactive. I am guessing the
> greybus mailing list can provide more information on how I should go
> about these.

Why is information like these inside the commit message, these go below
the tear line.

>
> Here is a sample mikroBUS manifest:
> ```
> ;;
> ; PRESSURE CLICK
> ; https://www.mikroe.com/pressure-click
> ; CONFIG_IIO_ST_PRESS
> ;
> ; Copyright 2020 BeagleBoard.org Foundation
> ; Copyright 2020 Texas Instruments
> ;
>
> [manifest-header]
> version-major = 0
> version-minor = 1
>
> [interface-descriptor]
> vendor-string-id = 1
> product-string-id = 2
>
> [string-descriptor 1]
> string = MIKROE
>
> [string-descriptor 2]
> string = Pressure
>
> [mikrobus-descriptor]
> pwm-state = 4
> int-state = 1
> rx-state = 7
> tx-state = 7
> scl-state = 6
> sda-state = 6
> mosi-state = 5
> miso-state = 5
> sck-state = 5
> cs-state = 5
> rst-state = 2
> an-state = 1
>
> [device-descriptor 1]
> driver-string-id = 3
> protocol = 0x3
> reg = 0x5d
>
> [string-descriptor 3]
> string = lps331ap
> ```
>
> Link: https://www.mikroe.com/clickid ClickID
> Link:
> https://docs.beagleboard.org/latest/projects/beagleconnect/index.html
> beagleconnect
> Link: https://github.com/projectara/greybus-spec Greybus Spec
> Link: https://github.com/projectara/greybus-spec/pull/4 Greybus Spec PR
> Link: https://github.com/projectara/manifesto/pull/2 manifesto PR
>

The manifesto PR might not be updated.

Thanks and Regards,
Vaishnav

> Co-developed-by: Vaishnav M A <[email protected]>
> Signed-off-by: Vaishnav M A <[email protected]>
> Signed-off-by: Ayush Singh <[email protected]>
> ---
> include/linux/greybus/greybus_manifest.h | 49 ++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/include/linux/greybus/greybus_manifest.h b/include/linux/greybus/greybus_manifest.h
> index bef9eb2093e9..83241e19d9b3 100644
> --- a/include/linux/greybus/greybus_manifest.h
> +++ b/include/linux/greybus/greybus_manifest.h
> @@ -23,6 +23,9 @@ enum greybus_descriptor_type {
> GREYBUS_TYPE_STRING = 0x02,
> GREYBUS_TYPE_BUNDLE = 0x03,
> GREYBUS_TYPE_CPORT = 0x04,
> + GREYBUS_TYPE_MIKROBUS = 0x05,
> + GREYBUS_TYPE_PROPERTY = 0x06,
> + GREYBUS_TYPE_DEVICE = 0x07,
> };
>
> enum greybus_protocol {
> @@ -151,6 +154,49 @@ struct greybus_descriptor_cport {
> __u8 protocol_id; /* enum greybus_protocol */
> } __packed;
>
> +/*
> + * A mikrobus descriptor is used to describe the details
> + * about the bus ocnfiguration for the add-on board
> + * connected to the mikrobus port.
> + */
> +struct greybus_descriptor_mikrobus {
> + __u8 pin_state[12];
> +} __packed;
> +
> +/*
> + * A property descriptor is used to pass named properties
> + * to device drivers through the unified device properties
> + * interface under linux/property.h
> + */
> +struct greybus_descriptor_property {
> + __u8 length;
> + __u8 id;
> + __u8 propname_stringid;
> + __u8 type;
> + __u8 value[];
> +} __packed;
> +
> +/*
> + * A device descriptor is used to describe the
> + * details required by a add-on board device
> + * driver.
> + */
> +struct greybus_descriptor_device {
> + __u8 id;
> + __u8 driver_stringid;
> + __u8 protocol;
> + __u8 reg;
> + __le32 max_speed_hz;
> + __u8 irq;
> + __u8 irq_type;
> + __u8 mode;
> + __u8 prop_link;
> + __u8 gpio_link;
> + __u8 reg_link;
> + __u8 clock_link;
> + __u8 pad[1];
> +} __packed;
> +
> struct greybus_descriptor_header {
> __le16 size;
> __u8 type; /* enum greybus_descriptor_type */
> @@ -164,6 +210,9 @@ struct greybus_descriptor {
> struct greybus_descriptor_interface interface;
> struct greybus_descriptor_bundle bundle;
> struct greybus_descriptor_cport cport;
> + struct greybus_descriptor_mikrobus mikrobus;
> + struct greybus_descriptor_property property;
> + struct greybus_descriptor_device device;
> };
> } __packed;
>

2024-03-19 08:50:22

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

Hi Ayush,

On 18/03/24 01:07, Ayush Singh wrote:
> DONOTMERGE
>
> this patch depends on Patch 1, 2, 3
>
> mikroBUS driver aims to split the platform aspects of mikroBUS (pinmux,
> SPI/I2C/GPIO controller .etc) from the add-on board information, thus
> requiring one device tree overlay per port and just a single manifest
> describing the add-on board.
>
> The driver exposes a sysfs entry that allows passing mikroBUS manifest of
> add-on board to the driver. The driver then parses this manifest, sets up
> the pins and protocols and allows using the appropriate Linux driver. Here
> is an example:

In v3 series, Krzysztof mentioned that you need to document the ABI, the
feedback was not considered in the series, see:
https://www.kernel.org/doc/Documentation/ABI/README

Also he mentioned that the mechanism of adding devices from userspace
needs to be discussed first, that feedback also was not responded to.

These add-on boards are not electrically hot-pluggable and this kind of
sysfs interface seems to give the idea that you can add an add-on board
on a running system (which is not recommended due to latchup/other
electrical failures), I will recommend you discuss the pending feedback
and discuss on acceptable solutions before spinning future revisions.

>
> ```
> cat /lib/firmware/mikrobus/AMBIENT-2-CLICK.mnfb > /sys/bus/mikrobus/devices/mikrobus-0/new_device
> ```
>
> Another sysfs entry is exposed that allows removing a previously
> registered mikrobus add-on board:
>
> ```
> echo " " > /sys/bus/mikrobus/devices/mikrobus-0/delete_device
> ```
>
> 100s of mikroBUS addon board manifests can be found in the clickID
> repository.
>
> In the future the driver also aims to support plug and play support
> using 1-wire EEPROM and mikroBUS over greybus.
>
> Link: https://www.mikroe.com/clickid ClickID
>
> Co-developed-by: Vaishnav M A <[email protected]>
> Signed-off-by: Vaishnav M A <[email protected]>
> Signed-off-by: Ayush Singh <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/misc/Kconfig | 1 +
> drivers/misc/Makefile | 1 +
> drivers/misc/mikrobus/Kconfig | 15 +
> drivers/misc/mikrobus/Makefile | 5 +
> drivers/misc/mikrobus/mikrobus_core.c | 696 ++++++++++++++++++++++
> drivers/misc/mikrobus/mikrobus_core.h | 151 +++++
> drivers/misc/mikrobus/mikrobus_manifest.c | 503 ++++++++++++++++
> drivers/misc/mikrobus/mikrobus_manifest.h | 29 +
> 9 files changed, 1402 insertions(+)
> create mode 100644 drivers/misc/mikrobus/Kconfig
> create mode 100644 drivers/misc/mikrobus/Makefile
> create mode 100644 drivers/misc/mikrobus/mikrobus_core.c
> create mode 100644 drivers/misc/mikrobus/mikrobus_core.h
> create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.c
> create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 69418a058c6b..83bc5e48bef9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14772,6 +14772,7 @@ M: Ayush Singh <[email protected]>
> M: Vaishnav M A <[email protected]>
> S: Maintained
> F: Documentation/devicetree/bindings/misc/mikrobus-connector.yaml
> +F: drivers/misc/mikrobus/*
>
> MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
> M: Luka Kovacic <[email protected]>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 4fb291f0bf7c..3d5c36205c6c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -591,4 +591,5 @@ source "drivers/misc/cardreader/Kconfig"
> source "drivers/misc/uacce/Kconfig"
> source "drivers/misc/pvpanic/Kconfig"
> source "drivers/misc/mchp_pci1xxxx/Kconfig"
> +source "drivers/misc/mikrobus/Kconfig"
> endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index ea6ea5bbbc9c..b9ac88055b87 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -68,3 +68,4 @@ obj-$(CONFIG_TMR_INJECT) += xilinx_tmr_inject.o
> obj-$(CONFIG_TPS6594_ESM) += tps6594-esm.o
> obj-$(CONFIG_TPS6594_PFSM) += tps6594-pfsm.o
> obj-$(CONFIG_NSM) += nsm.o
> +obj-y += mikrobus/
> diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig
> new file mode 100644
> index 000000000000..aa57b994dc66
> --- /dev/null
> +++ b/drivers/misc/mikrobus/Kconfig
> @@ -0,0 +1,15 @@
> +menuconfig MIKROBUS
> + tristate "Module for instantiating devices on mikroBUS ports"
> + depends on GPIOLIB
> + help
> + This option enables the mikroBUS driver. mikroBUS is an add-on
> + board socket standard that offers maximum expandability with
> + the smallest number of pins. The mikroBUS driver instantiates
> + devices on a mikroBUS port described mikroBUS manifest which is
> + passed using a sysfs interface.
> +
> +
> + Say Y here to enable support for this driver.
> +
> + To compile this code as a module, chose M here: the module
> + will be called mikrobus.ko
> diff --git a/drivers/misc/mikrobus/Makefile b/drivers/misc/mikrobus/Makefile
> new file mode 100644
> index 000000000000..0e51c5a7db4b
> --- /dev/null
> +++ b/drivers/misc/mikrobus/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# mikroBUS Core
> +
> +obj-$(CONFIG_MIKROBUS) += mikrobus.o
> +mikrobus-y := mikrobus_core.o mikrobus_manifest.o
> diff --git a/drivers/misc/mikrobus/mikrobus_core.c b/drivers/misc/mikrobus/mikrobus_core.c
> new file mode 100644
> index 000000000000..6aa20cef8e3b
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_core.c
> @@ -0,0 +1,696 @@
> +// SPDX-License-Identifier: GPL-2.0:
> +/*
> + * mikroBUS driver for instantiating add-on
> + * board devices with an identifier EEPROM
> + *
> + * Copyright 2020 Vaishnav M A, BeagleBoard.org Foundation.
> + * Copyright 2024 Ayush Singh <[email protected]>
> + */
> +
> +#define pr_fmt(fmt) "mikrobus:%s: " fmt, __func__
> +
> +#include "linux/gpio/driver.h"
> +#include "linux/gpio/machine.h"
> +#include "linux/gpio/consumer.h"
> +#include "linux/greybus/greybus_manifest.h"
> +#include "linux/i2c.h"
> +#include "linux/irq.h"
> +#include "linux/pinctrl/consumer.h"
> +#include "linux/platform_device.h"
> +#include "linux/spi/spi.h"
> +
> +#include "mikrobus_core.h"
> +#include "mikrobus_manifest.h"
> +
> +static struct class_compat *mikrobus_port_compat_class;
> +
> +static const struct bus_type mikrobus_bus_type = {
> + .name = "mikrobus",
> +};
> +
> +static int mikrobus_board_register(struct mikrobus_port *port,
> + struct addon_board_info *board);
> +static void mikrobus_board_unregister(struct mikrobus_port *port,
> + struct addon_board_info *board);
> +
> +/*
> + * mikrobus_pinctrl_select: Select pinctrl state for mikrobus pin
> + *
> + * @port: mikrobus port
> + * @pinctrl_selected: pinctrl state to be selected
> + */
> +static int mikrobus_pinctrl_select(struct mikrobus_port *port,
> + const char *pinctrl_selected)
> +{
> + struct pinctrl_state *state;
> + int ret;
> +
> + state = pinctrl_lookup_state(port->pinctrl, pinctrl_selected);
> + if (IS_ERR(state)) {
> + return dev_err_probe(&port->dev, PTR_ERR(state),
> + "failed to find state %s",
> + pinctrl_selected);
> + }
> +
> + ret = pinctrl_select_state(port->pinctrl, state);
> + if (ret) {
> + return dev_err_probe(&port->dev, ret,
> + "failed to select state %s",
> + pinctrl_selected);
> + }
> + dev_dbg(&port->dev, "setting pinctrl %s", pinctrl_selected);
> +
> + return 0;
> +}
> +
> +/*
> + * mikrobus_pinctrl_setup: Setup mikrobus pins to either default of gpio
> + *
> + * @port: mikrobus port
> + * @board: mikrobus board or NULL for default state
> + *
> + * returns 0 on success, negative error code on failure
> + */
> +static int mikrobus_pinctrl_setup(struct mikrobus_port *port,
> + struct addon_board_info *board)
> +{
> + int ret;
> +
> + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> + ret = mikrobus_pinctrl_select(port, "pwm_default");
> + else
> + ret = mikrobus_pinctrl_select(port, "pwm_gpio");
> + if (ret)
> + return ret;
> +
> + if (!board || board->pin_state[MIKROBUS_PIN_RX] == MIKROBUS_STATE_UART)
> + ret = mikrobus_pinctrl_select(port, "uart_default");
> + else
> + ret = mikrobus_pinctrl_select(port, "uart_gpio");
> + if (ret)
> + return ret;
> +
> + if (!board || board->pin_state[MIKROBUS_PIN_SCL] == MIKROBUS_STATE_I2C)
> + ret = mikrobus_pinctrl_select(port, "i2c_default");
> + else
> + ret = mikrobus_pinctrl_select(port, "i2c_gpio");
> + if (ret)
> + return ret;
> +
> + if (!board || board->pin_state[MIKROBUS_PIN_MOSI] == MIKROBUS_STATE_SPI)
> + ret = mikrobus_pinctrl_select(port, "spi_default");
> + else
> + ret = mikrobus_pinctrl_select(port, "spi_gpio");
> +
> + return ret;
> +}
> +
> +/*
> + * new_device_store: Expose sysfs entry for adding new board
> + *
> + * new_device_store: Allows userspace to add mikroBUS boards that lack 1-wire
> + * EEPROM for board identification by manually passing mikroBUS manifest
> + */
> +static ssize_t new_device_store(struct device *dev,
> + struct device_attribute *attr, const char *buf,
> + size_t count)
> +{
> + struct mikrobus_port *port = to_mikrobus_port(dev);
> + struct addon_board_info *board;
> + int ret;
> +
> + if (port->board)
> + return dev_err_probe(dev, -EBUSY,
> + "already has board registered");
> +
> + board = devm_kzalloc(&port->dev, sizeof(*board), GFP_KERNEL);
> + if (!board)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&board->manifest_descs);
> + INIT_LIST_HEAD(&board->devices);
> +
> + ret = mikrobus_manifest_parse(board, (void *)buf, count);
> + if (ret < 0) {
> + ret = dev_err_probe(dev, -EINVAL, "failed to parse manifest");
> + goto err_free_board;
> + }
> +
> + ret = mikrobus_board_register(port, board);
> + if (ret) {
> + ret = dev_err_probe(dev, -EINVAL, "failed to register board %s",
> + board->name);
> + goto err_free_board;
> + }
> +
> + return count;
> +
> +err_free_board:
> + devm_kfree(&port->dev, board);
> + return ret;
> +}
> +static DEVICE_ATTR_WO(new_device);
> +
> +/*
> + * delete_device_store: Expose sysfs entry for deleting board
> + */
> +static ssize_t delete_device_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct mikrobus_port *port = to_mikrobus_port(dev);
> +
> + if (!port->board)
> + return dev_err_probe(dev, -ENODEV,
> + "does not have registered boards");
> +
> + mikrobus_board_unregister(port, port->board);
> + return count;
> +}
> +static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, 0200, NULL,
> + delete_device_store);
> +
> +static struct attribute *mikrobus_port_attrs[] = { &dev_attr_new_device.attr,
> + &dev_attr_delete_device.attr,
> + NULL };
> +ATTRIBUTE_GROUPS(mikrobus_port);
> +
> +static void mikrobus_port_release(struct device *dev)
> +{
> +}
> +
> +static const struct device_type mikrobus_port_type = {
> + .groups = mikrobus_port_groups,
> + .release = mikrobus_port_release,
> +};
> +
> +static int mikrobus_irq_get(struct mikrobus_port *port, int irqno, int irq_type)
> +{
> + int irq;
> +
> + if (irqno > port->gpios->ndescs - 1)
> + return dev_err_probe(&port->dev, -ENODEV,
> + "GPIO %d does not exist", irqno);
> +
> + irq = gpiod_to_irq(port->gpios->desc[irqno]);
> + if (irq < 0)
> + return dev_err_probe(&port->dev, -EINVAL,
> + "could not get irq %d", irqno);
> +
> + irq_set_irq_type(irq, irq_type);
> +
> + return irq;
> +}
> +
> +static int mikrobus_gpio_setup(struct gpio_desc *gpio, int gpio_state)
> +{
> + switch (gpio_state) {
> + case MIKROBUS_STATE_INPUT:
> + return gpiod_direction_input(gpio);
> + case MIKROBUS_STATE_OUTPUT_HIGH:
> + return gpiod_direction_output(gpio, 1);
> + case MIKROBUS_STATE_OUTPUT_LOW:
> + return gpiod_direction_output(gpio, 0);
> + case MIKROBUS_STATE_PWM:
> + case MIKROBUS_STATE_SPI:
> + case MIKROBUS_STATE_I2C:
> + default:
> + return 0;
> + }
> +}
> +
> +static char *mikrobus_gpio_chip_name_get(struct mikrobus_port *port, int gpio)
> +{
> + struct gpio_chip *gpiochip;
> +
> + if (gpio > port->gpios->ndescs - 1)
> + return NULL;
> +
> + gpiochip = gpiod_to_chip(port->gpios->desc[gpio]);
> + return kmemdup(gpiochip->label, strlen(gpiochip->label), GFP_KERNEL);
> +}
> +
> +static int mikrobus_gpio_hwnum_get(struct mikrobus_port *port, int gpio)
> +{
> + struct gpio_chip *gpiochip;
> +
> + if (gpio > port->gpios->ndescs - 1)
> + return -ENODEV;
> +
> + gpiochip = gpiod_to_chip(port->gpios->desc[gpio]);
> + return desc_to_gpio(port->gpios->desc[gpio]) - gpiochip->base;
> +}
> +
> +static void mikrobus_board_device_release_all(struct addon_board_info *info)
> +{
> + struct board_device_info *dev, *next;
> +
> + list_for_each_entry_safe(dev, next, &info->devices, links) {
> + list_del(&dev->links);
> + kfree(dev);
> + }
> +}
> +
> +static int mikrobus_device_register(struct mikrobus_port *port,
> + struct board_device_info *dev,
> + char *board_name)
> +{
> + struct gpiod_lookup_table *lookup;
> + struct spi_board_info *spi_info;
> + struct i2c_board_info *i2c_info;
> + struct platform_device *pdev;
> + struct fwnode_handle *fwnode;
> + struct spi_device *spi;
> + struct i2c_client *i2c;
> + int i, ret;
> +
> + dev_info(&port->dev, "registering device : %s", dev->drv_name);
> +
> + if (dev->gpio_lookup) {
> + lookup = dev->gpio_lookup;
> +
> + switch (dev->protocol) {
> + case GREYBUS_PROTOCOL_SPI:
> + lookup->dev_id = kasprintf(GFP_KERNEL, "%s.%u",
> + dev->drv_name,
> + port->chip_select[dev->reg]);
> + break;
> + case GREYBUS_PROTOCOL_RAW:
> + lookup->dev_id = kasprintf(GFP_KERNEL, "%s.%u",
> + dev->drv_name, dev->reg);
> + break;
> + default:
> + lookup->dev_id = kmemdup(dev->drv_name,
> + strlen(dev->drv_name),
> + GFP_KERNEL);
> + }
> +
> + dev_info(&port->dev, "adding lookup table : %s",
> + lookup->dev_id);
> +
> + for (i = 0; i < dev->num_gpio_resources; i++) {
> + lookup->table[i].key = mikrobus_gpio_chip_name_get(
> + port, lookup->table[i].chip_hwnum);
> + lookup->table[i].chip_hwnum = mikrobus_gpio_hwnum_get(
> + port, lookup->table[i].chip_hwnum);
> + }
> +
> + gpiod_add_lookup_table(lookup);
> + }
> +
> + switch (dev->protocol) {
> + case GREYBUS_PROTOCOL_SPI:
> + spi_info =
> + devm_kzalloc(&port->dev, sizeof(*spi_info), GFP_KERNEL);
> + strscpy_pad(spi_info->modalias, dev->drv_name,
> + sizeof(spi_info->modalias));
> + if (dev->irq)
> + spi_info->irq =
> + mikrobus_irq_get(port, dev->irq, dev->irq_type);
> + if (dev->properties) {
> + fwnode = fwnode_create_software_node(dev->properties,
> + NULL);
> + spi_info->swnode = to_software_node(fwnode);
> + }
> + spi_info->chip_select = port->chip_select[dev->reg];
> + spi_info->max_speed_hz = dev->max_speed_hz;
> + spi_info->mode = dev->mode;
> +
> + spi = spi_new_device(port->spi_ctrl, spi_info);
> + devm_kfree(&port->dev, spi_info);
> + if (!spi)
> + return dev_err_probe(&port->dev, -ENODEV,
> + "failed to register spi device");
> + dev->dev_client = (void *)spi;
> + break;
> + case GREYBUS_PROTOCOL_I2C:
> + i2c_info =
> + devm_kzalloc(&port->dev, sizeof(*i2c_info), GFP_KERNEL);
> + if (!i2c_info)
> + return -ENOMEM;
> +
> + strscpy_pad(i2c_info->type, dev->drv_name,
> + sizeof(i2c_info->type));
> + if (dev->irq)
> + i2c_info->irq =
> + mikrobus_irq_get(port, dev->irq, dev->irq_type);
> + if (dev->properties) {
> + fwnode = fwnode_create_software_node(dev->properties,
> + NULL);
> + i2c_info->swnode = to_software_node(fwnode);
> + }
> + i2c_info->addr = dev->reg;
> +
> + i2c = i2c_new_client_device(port->i2c_adap, i2c_info);
> + devm_kfree(&port->dev, i2c_info);
> + if (IS_ERR(dev->dev_client))
> + return dev_err_probe(&port->dev,
> + PTR_ERR(dev->dev_client),
> + "failed to register i2c device");
> + dev->dev_client = (void *)i2c;
> + break;
> + case GREYBUS_PROTOCOL_RAW:
> + pdev = platform_device_alloc(dev->drv_name, 0);
> + if (!pdev)
> + return -ENOMEM;
> +
> + if (dev->properties) {
> + ret = device_create_managed_software_node(
> + &pdev->dev, dev->properties, NULL);
> + if (ret)
> + return dev_err_probe(
> + &port->dev, ret,
> + "failed to create software node");
> + }
> + ret = platform_device_add(dev->dev_client);
> + if (ret)
> + return dev_err_probe(
> + &port->dev, ret,
> + "failed to register platform device");
> + dev->dev_client = (void *)pdev;
> + break;
> + case GREYBUS_PROTOCOL_UART:
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static void mikrobus_device_unregister(struct mikrobus_port *port,
> + struct board_device_info *dev,
> + char *board_name)
> +{
> + dev_info(&port->dev, "removing device %s", dev->drv_name);
> + if (dev->gpio_lookup) {
> + gpiod_remove_lookup_table(dev->gpio_lookup);
> + kfree(dev->gpio_lookup->dev_id);
> + kfree(dev->gpio_lookup);
> + }
> +
> + kfree(dev->properties);
> +
> + switch (dev->protocol) {
> + case GREYBUS_PROTOCOL_SPI:
> + spi_unregister_device((struct spi_device *)dev->dev_client);
> + break;
> + case GREYBUS_PROTOCOL_I2C:
> + i2c_unregister_device((struct i2c_client *)dev->dev_client);
> + break;
> + case GREYBUS_PROTOCOL_RAW:
> + platform_device_unregister(
> + (struct platform_device *)dev->dev_client);
> + break;
> + case GREYBUS_PROTOCOL_UART:
> + break;
> + }
> +}
> +
> +static int mikrobus_board_register(struct mikrobus_port *port,
> + struct addon_board_info *board)
> +{
> + struct board_device_info *devinfo, *next;
> + int ret, i;
> +
> + if (WARN_ON(list_empty(&board->devices)))
> + return false;
> +
> + if (port->pinctrl) {
> + ret = mikrobus_pinctrl_setup(port, board);
> + if (ret)
> + dev_err(&port->dev,
> + "failed to setup pinctrl state [%d]", ret);
> + }
> +
> + if (port->gpios) {
> + for (i = 0; i < port->gpios->ndescs; i++) {
> + ret = mikrobus_gpio_setup(port->gpios->desc[i],
> + board->pin_state[i]);
> + if (ret)
> + dev_err(&port->dev,
> + "failed to setup gpio %d, state %d", i,
> + board->pin_state[i]);
> +
> + gpiochip_free_own_desc(port->gpios->desc[i]);
> + }
> + }
> +
> + list_for_each_entry_safe(devinfo, next, &board->devices, links)
> + mikrobus_device_register(port, devinfo, board->name);
> +
> + port->board = board;
> + return 0;
> +}
> +
> +static void mikrobus_board_unregister(struct mikrobus_port *port,
> + struct addon_board_info *board)
> +{
> + struct board_device_info *devinfo, *next;
> +
> + if (WARN_ON(list_empty(&board->devices)))
> + return;
> +
> + list_for_each_entry_safe(devinfo, next, &board->devices, links)
> + mikrobus_device_unregister(port, devinfo, board->name);
> +
> + mikrobus_board_device_release_all(board);
> + devm_kfree(&port->dev, board);
> + port->board = NULL;
> +}
> +
> +static int mikrobus_port_register(struct mikrobus_port *port)
> +{
> + int ret;
> +
> + port->dev.bus = &mikrobus_bus_type;
> + port->dev.type = &mikrobus_port_type;
> + dev_set_name(&port->dev, "mikrobus-%d", port->id);
> +
> + dev_info(&port->dev, "registering port %s", dev_name(&port->dev));
> +
> + ret = device_register(&port->dev);
> + if (ret)
> + return dev_err_probe(&port->dev, ret,
> + "port '%d': can't register device (%d)",
> + port->id, ret);
> +
> + ret = class_compat_create_link(mikrobus_port_compat_class, &port->dev,
> + port->dev.parent);
> + if (ret)
> + dev_warn(&port->dev,
> + "failed to create compatibility class link");
> +
> + return ret;
> +}
> +
> +static void mikrobus_port_delete(struct mikrobus_port *port)
> +{
> + if (port->board)
> + return dev_err(
> + &port->dev,
> + "attempting to delete port with registered boards, port [%s]",
> + dev_name(&port->dev));
> +
> + class_compat_remove_link(mikrobus_port_compat_class, &port->dev,
> + port->dev.parent);
> +
> + devm_pinctrl_put(port->pinctrl);
> + put_device(&port->spi_ctrl->dev);
> + gpiod_put_array(port->gpios);
> + put_device(&port->i2c_adap->dev);
> +
> + device_unregister(&port->dev);
> + memset(&port->dev, 0, sizeof(port->dev));
> +}
> +
> +static int mikrobus_port_probe_pinctrl_setup(struct mikrobus_port *port)
> +{
> + struct device *dev = port->dev.parent;
> + struct pinctrl_state *state;
> + int ret;
> +
> + state = pinctrl_lookup_state(port->pinctrl, PINCTRL_STATE_DEFAULT);
> + if (IS_ERR(state))
> + return dev_err_probe(dev, PTR_ERR(state),
> + "failed to find state %s",
> + PINCTRL_STATE_DEFAULT);
> +
> + ret = pinctrl_select_state(port->pinctrl, state);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to select state %s",
> + PINCTRL_STATE_DEFAULT);
> +
> + ret = mikrobus_pinctrl_setup(port, NULL);
> + if (ret)
> + dev_err(dev, "failed to select pinctrl states [%d]", ret);
> +
> + return ret;
> +}
> +
> +static int mikrobus_port_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mikrobus_port *port;
> + struct device_node *np;
> + int ret;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + port->dev.parent = dev;
> + port->dev.of_node = pdev->dev.of_node;
> +
> + /* port id */
> + port->id = of_alias_get_id(dev->of_node, "mikrobus");
> + if (port->id) {
> + ret = dev_err_probe(dev, -EINVAL, "invalid mikrobus id");
> + goto err_port;
> + }
> +
> + /* I2C setup */
> + np = of_parse_phandle(dev->of_node, "i2c-adapter", 0);
> + if (!np) {
> + ret = dev_err_probe(dev, -ENODEV, "cannot parse i2c-adapter");
> + goto err_port;
> + }
> + port->i2c_adap = of_find_i2c_adapter_by_node(np);
> + of_node_put(np);
> + if (!port->i2c_adap) {
> + ret = dev_err_probe(dev, -ENODEV, "cannot find i2c adapter");
> + goto err_port;
> + }
> +
> + /* GPIO setup */
> + port->gpios = gpiod_get_array(dev, "mikrobus", GPIOD_OUT_LOW);
> + if (IS_ERR(port->gpios)) {
> + ret = dev_err_probe(dev, PTR_ERR(port->gpios),
> + "failed to get gpio array [%ld]",
> + PTR_ERR(port->gpios));
> + goto free_i2c;
> + }
> +
> + /* SPI setup */
> + np = of_parse_phandle(dev->of_node, "spi-controller", 0);
> + if (!np) {
> + ret = dev_err_probe(dev, -ENODEV,
> + "cannot parse spi-controller");
> + goto free_gpio;
> + }
> + port->spi_ctrl = of_find_spi_controller_by_node(np);
> + of_node_put(np);
> + if (!port->spi_ctrl) {
> + ret = dev_err_probe(dev, -ENODEV, "cannot find spi controller");
> + goto free_gpio;
> + }
> + ret = device_property_read_u32_array(dev, "spi-cs", port->chip_select,
> + MIKROBUS_NUM_CS);
> + if (ret) {
> + dev_err(dev, "failed to get spi-cs [%d]", ret);
> + goto free_spi;
> + }
> +
> + /* pinctrl setup */
> + port->pinctrl = devm_pinctrl_get(dev);
> + if (IS_ERR(port->pinctrl)) {
> + ret = dev_err_probe(dev, PTR_ERR(port->pinctrl),
> + "failed to get pinctrl [%ld]",
> + PTR_ERR(port->pinctrl));
> + goto free_spi;
> + }
> + ret = mikrobus_port_probe_pinctrl_setup(port);
> + if (ret) {
> + dev_err(dev, "failed to setup pinctrl [%d]", ret);
> + goto free_pinctrl;
> + }
> +
> + /* TODO: UART */
> + /* TODO: PWM */
> +
> + ret = mikrobus_port_register(port);
> + if (ret) {
> + dev_err(dev, "port : can't register port [%d]", ret);
> + goto free_pinctrl;
> + }
> +
> + platform_set_drvdata(pdev, port);
> +
> + return 0;
> +
> +free_pinctrl:
> + devm_pinctrl_put(port->pinctrl);
> +free_spi:
> + put_device(&port->spi_ctrl->dev);
> +free_gpio:
> + gpiod_put_array(port->gpios);
> +free_i2c:
> + put_device(&port->i2c_adap->dev);
> +err_port:
> + put_device(&port->dev);
> + return ret;
> +}
> +
> +static int mikrobus_port_remove(struct platform_device *pdev)
> +{
> + struct mikrobus_port *port = platform_get_drvdata(pdev);
> +
> + mikrobus_port_delete(port);
> + return 0;
> +}
> +
> +static const struct of_device_id mikrobus_port_of_match[] = {
> + { .compatible = "mikrobus-connector" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, mikrobus_port_of_match);
> +
> +static struct platform_driver mikrobus_port_driver = {
> + .probe = mikrobus_port_probe,
> + .remove = mikrobus_port_remove,
> + .driver = {
> + .name = "mikrobus",
> + .of_match_table = mikrobus_port_of_match,
> + },
> +};
> +
> +static int mikrobus_init(void)
> +{
> + int ret;
> +
> + ret = bus_register(&mikrobus_bus_type);
> + if (ret) {
> + pr_err("bus_register failed (%d)", ret);
> + return ret;
> + }
> +
> + mikrobus_port_compat_class = class_compat_register("mikrobus-port");
> + if (!mikrobus_port_compat_class) {
> + ret = -ENOMEM;
> + pr_err("class_compat register failed (%d)", ret);
> + goto class_err;
> + }
> +
> + ret = platform_driver_register(&mikrobus_port_driver);
> + if (ret)
> + pr_err("driver register failed [%d]", ret);
> +
> + return ret;
> +
> +class_err:
> + bus_unregister(&mikrobus_bus_type);
> + return ret;
> +}
> +subsys_initcall(mikrobus_init);
> +
> +static void mikrobus_exit(void)
> +{
> + platform_driver_unregister(&mikrobus_port_driver);
> + bus_unregister(&mikrobus_bus_type);
> + class_compat_unregister(mikrobus_port_compat_class);
> +}
> +module_exit(mikrobus_exit);
> +
> +MODULE_AUTHOR("Vaishnav M A <[email protected]>");
> +MODULE_AUTHOR("Ayush Singh <[email protected]>");

This e-mail is different from your Signed-off-by:/ MAINTAINERS entry,
was this intentional?

Thanks and Regards,
Vaishnav

> +MODULE_DESCRIPTION("mikroBUS main module");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/misc/mikrobus/mikrobus_core.h b/drivers/misc/mikrobus/mikrobus_core.h
> new file mode 100644
> index 000000000000..1d41ee32ca94
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_core.h
> @@ -0,0 +1,151 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * mikroBUS Driver for instantiating add-on board devices
> + *
> + * Copyright 2020 Vaishnav M A, BeagleBoard.org Foundation.
> + * Copyright 2024 Ayush Singh <[email protected]>
> + */
> +
> +#ifndef __MIKROBUS_H
> +#define __MIKROBUS_H
> +
> +#include "linux/device.h"
> +
> +#define MIKROBUS_VERSION_MAJOR 0x00
> +#define MIKROBUS_VERSION_MINOR 0x03
> +
> +#define MIKROBUS_NUM_PINCTRL_STATE 4
> +#define MIKROBUS_NUM_CS 2
> +
> +#define MIKROBUS_PINCTRL_PWM 0
> +#define MIKROBUS_PINCTRL_UART 1
> +#define MIKROBUS_PINCTRL_I2C 2
> +#define MIKROBUS_PINCTRL_SPI 3
> +
> +enum mikrobus_property_type {
> + MIKROBUS_PROPERTY_TYPE_MIKROBUS = 0x00,
> + MIKROBUS_PROPERTY_TYPE_PROPERTY,
> + MIKROBUS_PROPERTY_TYPE_GPIO,
> + MIKROBUS_PROPERTY_TYPE_U8,
> + MIKROBUS_PROPERTY_TYPE_U16,
> + MIKROBUS_PROPERTY_TYPE_U32,
> + MIKROBUS_PROPERTY_TYPE_U64,
> +};
> +
> +enum mikrobus_pin {
> + MIKROBUS_PIN_PWM = 0x00,
> + MIKROBUS_PIN_INT,
> + MIKROBUS_PIN_RX,
> + MIKROBUS_PIN_TX,
> + MIKROBUS_PIN_SCL,
> + MIKROBUS_PIN_SDA,
> + MIKROBUS_PIN_MOSI,
> + MIKROBUS_PIN_MISO,
> + MIKROBUS_PIN_SCK,
> + MIKROBUS_PIN_CS,
> + MIKROBUS_PIN_RST,
> + MIKROBUS_PIN_AN,
> + MIKROBUS_PORT_PIN_COUNT,
> +};
> +
> +enum mikrobus_pin_state {
> + MIKROBUS_STATE_INPUT = 0x01,
> + MIKROBUS_STATE_OUTPUT_HIGH,
> + MIKROBUS_STATE_OUTPUT_LOW,
> + MIKROBUS_STATE_PWM,
> + MIKROBUS_STATE_SPI,
> + MIKROBUS_STATE_I2C,
> + MIKROBUS_STATE_UART,
> +};
> +
> +/*
> + * board_device_info describes a single device on a mikrobus add-on
> + * board, an add-on board can present one or more device to the host
> + *
> + * @gpio_lookup: used to provide the GPIO lookup table for
> + * passing the named GPIOs to device drivers.
> + * @properties: used to provide the property_entry to pass named
> + * properties to device drivers, applicable only when driver uses
> + * device_property_read_* calls to fetch the properties.
> + * @num_gpio_resources: number of named gpio resources for the device,
> + * used mainly for gpiod_lookup_table memory allocation.
> + * @num_properties: number of custom properties for the device,
> + * used mainly for property_entry memory allocation.
> + * @protocol: used to know the type of the device and it should
> + * contain one of the values defined under 'enum greybus_class_type'
> + * under linux/greybus/greybus_manifest.h
> + * @reg: I2C address for the device, for devices on the SPI bus
> + * this field is the chip select address relative to the mikrobus
> + * port:0->device chip select connected to CS pin on mikroBUS port
> + * 1->device chip select connected to RST Pin on mikroBUS port
> + * @mode: SPI mode
> + * @max_speed_hz: SPI max speed(Hz)
> + * @drv_name: device_id to match with the driver
> + * @irq_type: type of IRQ trigger , match with defines in linux/interrupt.h
> + * @irq: irq number relative to the mikrobus port should contain one of the
> + * values defined under 'enum mikrobus_pin'
> + * @id: device id starting from 1
> + */
> +struct board_device_info {
> + struct gpiod_lookup_table *gpio_lookup;
> + struct property_entry *properties;
> + struct list_head links;
> + unsigned short num_gpio_resources;
> + unsigned short num_properties;
> + unsigned short protocol;
> + unsigned short reg;
> + unsigned int mode;
> + void *dev_client;
> + u32 max_speed_hz;
> + char *drv_name;
> + int irq_type;
> + int irq;
> + int id;
> +};
> +
> +/*
> + * addon_board_info describes a mikrobus add-on device the add-on
> + * board, an add-on board can present one or more device to the host
> + *
> + * @manifest_descs: list of manifest descriptors
> + * @devices: list of devices on the board
> + * @pin_state: the state of each pin on the mikrobus port required
> + * for the add-on board should contain one of the values defined under
> + * 'enum mikrobus_pin_state' restrictions are as per mikrobus standard
> + * specifications.
> + * @name: add-on board name
> + */
> +struct addon_board_info {
> + struct list_head manifest_descs;
> + struct list_head devices;
> + u8 pin_state[MIKROBUS_PORT_PIN_COUNT];
> + char *name;
> +};
> +
> +/*
> + * mikrobus_port describes the peripherals mapped to a mikrobus port.
> + *
> + * @chip_select: chip select number mapped to the SPI CS pin on the
> + * mikrobus port and the RST pin on the mikrobus port
> + * @board: pointer to the attached add-on board.
> + * @spi_ctrl: SPI controller attached to the mikrobus port.
> + * @i2c_adap: I2C adapter attached to the mikrobus port.
> + * @gpios: GPIOs attached to the mikrobus port.
> + * @pinctrl: pinctrl attached to the mikrobus port.
> + * @dev: device structure for the mikrobus port.
> + * @id: port id starting from 1
> + */
> +struct mikrobus_port {
> + u32 chip_select[MIKROBUS_NUM_CS];
> + struct addon_board_info *board;
> + struct spi_controller *spi_ctrl;
> + struct i2c_adapter *i2c_adap;
> + struct gpio_descs *gpios;
> + struct pinctrl *pinctrl;
> + struct device dev;
> + int id;
> +};
> +
> +#define to_mikrobus_port(d) container_of(d, struct mikrobus_port, dev)
> +
> +#endif /* __MIKROBUS_H */
> diff --git a/drivers/misc/mikrobus/mikrobus_manifest.c b/drivers/misc/mikrobus/mikrobus_manifest.c
> new file mode 100644
> index 000000000000..5f30620277be
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_manifest.c
> @@ -0,0 +1,503 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * mikroBUS manifest parsing, an
> + * extension to Greybus Manifest Parsing
> + * under drivers/greybus/manifest.c
> + *
> + * Copyright 2014-2015 Google Inc.
> + * Copyright 2014-2015 Linaro Ltd.
> + * Copyright 2024 Ayush Singh <[email protected]>
> + */
> +
> +#define pr_fmt(fmt) "mikrobus_manifest:%s: " fmt, __func__
> +
> +#include "linux/gpio/machine.h"
> +#include "linux/greybus/greybus_manifest.h"
> +#include "linux/property.h"
> +#include "mikrobus_manifest.h"
> +
> +struct manifest_desc {
> + struct list_head links;
> + size_t size;
> + void *data;
> + enum greybus_descriptor_type type;
> +};
> +
> +static void manifest_descriptor_release_all(struct addon_board_info *board)
> +{
> + struct manifest_desc *descriptor, *next;
> +
> + list_for_each_entry_safe(descriptor, next, &board->manifest_descs,
> + links) {
> + list_del(&descriptor->links);
> + kfree(descriptor);
> + }
> +}
> +
> +static int board_descriptor_add(struct addon_board_info *board,
> + struct greybus_descriptor *desc, size_t size)
> +{
> + struct greybus_descriptor_header *desc_header = &desc->header;
> + struct manifest_desc *descriptor;
> + size_t desc_size, expected_size;
> +
> + if (size < sizeof(*desc_header)) {
> + pr_err("short descriptor (%zu < %zu)", size,
> + sizeof(*desc_header));
> + return -EINVAL;
> + }
> +
> + desc_size = le16_to_cpu(desc_header->size);
> + if (desc_size > size) {
> + pr_err("incorrect descriptor size (%zu != %zu)", size,
> + desc_size);
> + return -EINVAL;
> + }
> +
> + expected_size = sizeof(*desc_header);
> + switch (desc_header->type) {
> + case GREYBUS_TYPE_STRING:
> + expected_size += sizeof(struct greybus_descriptor_string);
> + expected_size += desc->string.length;
> + expected_size = ALIGN(expected_size, 4);
> + break;
> + case GREYBUS_TYPE_PROPERTY:
> + expected_size += sizeof(struct greybus_descriptor_property);
> + expected_size += desc->property.length;
> + expected_size = ALIGN(expected_size, 4);
> + break;
> + case GREYBUS_TYPE_DEVICE:
> + expected_size += sizeof(struct greybus_descriptor_device);
> + break;
> + case GREYBUS_TYPE_MIKROBUS:
> + expected_size += sizeof(struct greybus_descriptor_mikrobus);
> + break;
> + case GREYBUS_TYPE_INTERFACE:
> + expected_size += sizeof(struct greybus_descriptor_interface);
> + break;
> + case GREYBUS_TYPE_CPORT:
> + expected_size += sizeof(struct greybus_descriptor_cport);
> + break;
> + case GREYBUS_TYPE_BUNDLE:
> + expected_size += sizeof(struct greybus_descriptor_bundle);
> + break;
> + case GREYBUS_TYPE_INVALID:
> + default:
> + pr_err("invalid descriptor type %d", desc_header->type);
> + return -EINVAL;
> + }
> +
> + descriptor = kzalloc(sizeof(*descriptor), GFP_KERNEL);
> + if (!descriptor)
> + return -ENOMEM;
> +
> + descriptor->size = desc_size;
> + descriptor->data = (char *)desc + sizeof(*desc_header);
> + descriptor->type = desc_header->type;
> + list_add_tail(&descriptor->links, &board->manifest_descs);
> +
> + return desc_size;
> +}
> +
> +static char *mikrobus_string_get(struct addon_board_info *board, u8 string_id)
> +{
> + struct greybus_descriptor_string *desc_string;
> + struct manifest_desc *descriptor;
> + bool found = false;
> + char *string;
> +
> + if (!string_id)
> + return NULL;
> +
> + list_for_each_entry(descriptor, &board->manifest_descs, links) {
> + if (descriptor->type != GREYBUS_TYPE_STRING)
> + continue;
> +
> + desc_string = descriptor->data;
> + if (desc_string->id == string_id) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found)
> + return ERR_PTR(-ENOENT);
> +
> + string = kmemdup(&desc_string->string, desc_string->length + 1,
> + GFP_KERNEL);
> + if (!string)
> + return ERR_PTR(-ENOMEM);
> +
> + string[desc_string->length] = '\0';
> +
> + return string;
> +}
> +
> +static void mikrobus_state_get(struct addon_board_info *board)
> +{
> + struct greybus_descriptor_mikrobus *mikrobus;
> + struct greybus_descriptor_interface *interface;
> + struct manifest_desc *descriptor;
> + bool found = false;
> + size_t i;
> +
> + list_for_each_entry(descriptor, &board->manifest_descs, links) {
> + if (descriptor->type == GREYBUS_TYPE_MIKROBUS) {
> + mikrobus = descriptor->data;
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found) {
> + pr_err("mikrobus descriptor not found");
> + return;
> + }
> +
> + for (i = 0; i < MIKROBUS_PORT_PIN_COUNT; i++)
> + board->pin_state[i] = mikrobus->pin_state[i];
> +
> + found = false;
> + list_for_each_entry(descriptor, &board->manifest_descs, links) {
> + if (descriptor->type == GREYBUS_TYPE_INTERFACE) {
> + interface = descriptor->data;
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found) {
> + pr_err("interface descriptor not found");
> + return;
> + }
> +
> + board->name = mikrobus_string_get(board, interface->product_stringid);
> +}
> +
> +static struct property_entry *
> +mikrobus_property_entry_get(struct addon_board_info *board, u8 *prop_link,
> + int num_properties)
> +{
> + struct greybus_descriptor_property *desc_property;
> + struct manifest_desc *descriptor;
> + struct property_entry *properties;
> + bool found = false;
> + char *prop_name;
> + int i, ret;
> + u64 *val_u64;
> + u32 *val_u32;
> + u16 *val_u16;
> + u8 *val_u8;
> +
> + properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL);
> + if (!properties)
> + return ERR_PTR(-ENOMEM);
> +
> + for (i = 0; i < num_properties; i++) {
> + list_for_each_entry(descriptor, &board->manifest_descs, links) {
> + if (descriptor->type != GREYBUS_TYPE_PROPERTY)
> + continue;
> +
> + desc_property = descriptor->data;
> + if (desc_property->id == prop_link[i]) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found) {
> + ret = -ENOENT;
> + goto early_exit;
> + }
> +
> + prop_name = mikrobus_string_get(
> + board, desc_property->propname_stringid);
> + if (!prop_name) {
> + ret = -ENOENT;
> + goto early_exit;
> + }
> +
> + switch (desc_property->type) {
> + case MIKROBUS_PROPERTY_TYPE_U8:
> + val_u8 = kmemdup(&desc_property->value,
> + (desc_property->length) * sizeof(u8),
> + GFP_KERNEL);
> + if (desc_property->length == 1)
> + properties[i] =
> + PROPERTY_ENTRY_U8(prop_name, *val_u8);
> + else
> + properties[i] = PROPERTY_ENTRY_U8_ARRAY_LEN(
> + prop_name, (void *)desc_property->value,
> + desc_property->length);
> + break;
> + case MIKROBUS_PROPERTY_TYPE_U16:
> + val_u16 = kmemdup(&desc_property->value,
> + (desc_property->length) * sizeof(u16),
> + GFP_KERNEL);
> + if (desc_property->length == 1)
> + properties[i] =
> + PROPERTY_ENTRY_U16(prop_name, *val_u16);
> + else
> + properties[i] = PROPERTY_ENTRY_U16_ARRAY_LEN(
> + prop_name, (void *)desc_property->value,
> + desc_property->length);
> + break;
> + case MIKROBUS_PROPERTY_TYPE_U32:
> + val_u32 = kmemdup(&desc_property->value,
> + (desc_property->length) * sizeof(u32),
> + GFP_KERNEL);
> + if (desc_property->length == 1)
> + properties[i] =
> + PROPERTY_ENTRY_U32(prop_name, *val_u32);
> + else
> + properties[i] = PROPERTY_ENTRY_U32_ARRAY_LEN(
> + prop_name, (void *)desc_property->value,
> + desc_property->length);
> + break;
> + case MIKROBUS_PROPERTY_TYPE_U64:
> + val_u64 = kmemdup(&desc_property->value,
> + (desc_property->length) * sizeof(u64),
> + GFP_KERNEL);
> + if (desc_property->length == 1)
> + properties[i] =
> + PROPERTY_ENTRY_U64(prop_name, *val_u64);
> + else
> + properties[i] = PROPERTY_ENTRY_U64_ARRAY_LEN(
> + prop_name, (void *)desc_property->value,
> + desc_property->length);
> + break;
> + default:
> + ret = -EINVAL;
> + goto early_exit;
> + }
> + }
> + return properties;
> +
> +early_exit:
> + kfree(properties);
> + return ERR_PTR(ret);
> +}
> +
> +static u8 *mikrobus_property_link_get(struct addon_board_info *board,
> + u8 prop_id,
> + struct board_device_info *board_dev,
> + u8 prop_type)
> +{
> + struct greybus_descriptor_property *desc_property;
> + struct manifest_desc *descriptor;
> + bool found = false;
> + u8 *val_u8;
> +
> + if (!prop_id)
> + return NULL;
> +
> + list_for_each_entry(descriptor, &board->manifest_descs, links) {
> + if (descriptor->type != GREYBUS_TYPE_PROPERTY)
> + continue;
> +
> + desc_property = descriptor->data;
> + if (desc_property->id == prop_id &&
> + desc_property->type == prop_type) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found)
> + return ERR_PTR(-ENOENT);
> +
> + val_u8 = kmemdup(&desc_property->value, desc_property->length,
> + GFP_KERNEL);
> + if (prop_type == MIKROBUS_PROPERTY_TYPE_GPIO)
> + board_dev->num_gpio_resources = desc_property->length;
> + else if (prop_type == MIKROBUS_PROPERTY_TYPE_PROPERTY)
> + board_dev->num_properties = desc_property->length;
> +
> + return val_u8;
> +}
> +
> +static int
> +mikrobus_manifest_attach_device(struct addon_board_info *board,
> + struct greybus_descriptor_device *dev_desc)
> +{
> + struct greybus_descriptor_property *desc_property;
> + u8 *gpio_desc_link, *prop_link, *gpioval;
> + struct board_device_info *board_dev;
> + struct gpiod_lookup_table *lookup;
> + struct manifest_desc *descriptor;
> + int ret, i;
> +
> + board_dev = kzalloc(sizeof(*board_dev), GFP_KERNEL);
> + if (!board_dev)
> + return -ENOMEM;
> +
> + board_dev->id = dev_desc->id;
> + board_dev->drv_name =
> + mikrobus_string_get(board, dev_desc->driver_stringid);
> + if (!board_dev->drv_name) {
> + ret = -ENOENT;
> + goto err_free_board_dev;
> + }
> +
> + board_dev->protocol = dev_desc->protocol;
> + board_dev->reg = dev_desc->reg;
> + board_dev->irq = dev_desc->irq;
> + board_dev->irq_type = dev_desc->irq_type;
> + board_dev->max_speed_hz = le32_to_cpu(dev_desc->max_speed_hz);
> + board_dev->mode = dev_desc->mode;
> + pr_info("parsed device %d, driver=%s", board_dev->id,
> + board_dev->drv_name);
> +
> + if (dev_desc->prop_link > 0) {
> + prop_link = mikrobus_property_link_get(
> + board, dev_desc->prop_link, board_dev,
> + MIKROBUS_PROPERTY_TYPE_PROPERTY);
> + if (!prop_link) {
> + ret = -ENOENT;
> + goto err_free_board_dev;
> + }
> +
> + pr_info("device %d, number of properties=%d", board_dev->id,
> + board_dev->num_properties);
> + board_dev->properties = mikrobus_property_entry_get(
> + board, prop_link, board_dev->num_properties);
> + }
> +
> + if (dev_desc->gpio_link > 0) {
> + gpio_desc_link = mikrobus_property_link_get(
> + board, dev_desc->gpio_link, board_dev,
> + MIKROBUS_PROPERTY_TYPE_GPIO);
> + if (!gpio_desc_link) {
> + ret = -ENOENT;
> + goto err_free_board_dev;
> + }
> +
> + pr_info("device %d, number of gpio resource=%d", board_dev->id,
> + board_dev->num_gpio_resources);
> + lookup = kzalloc(struct_size(lookup, table,
> + board_dev->num_gpio_resources),
> + GFP_KERNEL);
> + if (!lookup) {
> + ret = -ENOMEM;
> + goto err_free_board_dev;
> + }
> +
> + for (i = 0; i < board_dev->num_gpio_resources; i++) {
> + list_for_each_entry(descriptor, &board->manifest_descs,
> + links) {
> + if (descriptor->type != GREYBUS_TYPE_PROPERTY)
> + continue;
> +
> + desc_property = descriptor->data;
> + if (desc_property->id == gpio_desc_link[i]) {
> + gpioval = desc_property->value;
> + lookup->table[i].chip_hwnum =
> + gpioval[0];
> + lookup->table[i].flags = gpioval[1];
> + lookup->table[i]
> + .con_id = mikrobus_string_get(
> + board,
> + desc_property
> + ->propname_stringid);
> + break;
> + }
> + }
> + }
> + board_dev->gpio_lookup = lookup;
> + }
> +
> + list_add_tail(&board_dev->links, &board->devices);
> + return 0;
> +
> +err_free_board_dev:
> + kfree(board_dev);
> + return ret;
> +}
> +
> +static int mikrobus_manifest_parse_devices(struct addon_board_info *board)
> +{
> + struct greybus_descriptor_device *desc_device;
> + struct manifest_desc *desc, *next;
> + int ret, devcount = 0;
> +
> + list_for_each_entry_safe(desc, next, &board->manifest_descs, links) {
> + if (desc->type != GREYBUS_TYPE_DEVICE)
> + continue;
> +
> + desc_device = desc->data;
> + ret = mikrobus_manifest_attach_device(board, desc_device);
> + devcount++;
> + }
> +
> + return devcount;
> +}
> +
> +static size_t mikrobus_manifest_header_validate(void *data, size_t size)
> +{
> + struct greybus_manifest_header *header = data;
> + u16 manifest_size = le16_to_cpu(header->size);
> +
> + if (manifest_size < sizeof(*header)) {
> + pr_err("short manifest (%zu < %zu)", size, sizeof(*header));
> + return -EINVAL;
> + }
> +
> + if (header->version_major > MIKROBUS_VERSION_MAJOR) {
> + pr_err("manifest version too new (%u.%u > %u.%u)",
> + header->version_major, header->version_minor,
> + MIKROBUS_VERSION_MAJOR, MIKROBUS_VERSION_MINOR);
> + return -EINVAL;
> + }
> +
> + return manifest_size;
> +}
> +
> +int mikrobus_manifest_parse(struct addon_board_info *board, void *data,
> + size_t size)
> +{
> + struct greybus_manifest_header *header;
> + struct greybus_manifest *manifest;
> + struct greybus_descriptor *desc;
> + int dev_count, desc_size, ret;
> + u16 manifest_size;
> +
> + if (size < sizeof(*header)) {
> + pr_err("short manifest (%zu < %zu)", size, sizeof(*header));
> + return -EINVAL;
> + }
> +
> + ret = mikrobus_manifest_header_validate(data, size);
> + if (ret < 0) {
> + pr_err("invalid manifest header: %u", manifest_size);
> + return ret;
> + }
> +
> + manifest = data;
> + manifest_size = ret;
> +
> + if (manifest_size != size) {
> + pr_err("invalid manifest size(%zu < %u)", size, manifest_size);
> + return -EINVAL;
> + }
> +
> + desc = manifest->descriptors;
> + size -= sizeof(*header);
> + while (size) {
> + desc_size = board_descriptor_add(board, desc, size);
> + if (desc_size < 0) {
> + pr_err("invalid manifest descriptor, size: %u",
> + desc_size);
> + return -EINVAL;
> + }
> +
> + desc = (void *)desc + desc_size;
> + size -= desc_size;
> + }
> +
> + mikrobus_state_get(board);
> + dev_count = mikrobus_manifest_parse_devices(board);
> + pr_info("%s manifest parsed with %d devices", board->name, dev_count);
> + manifest_descriptor_release_all(board);
> +
> + return dev_count;
> +}
> diff --git a/drivers/misc/mikrobus/mikrobus_manifest.h b/drivers/misc/mikrobus/mikrobus_manifest.h
> new file mode 100644
> index 000000000000..39ae53a25fc4
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_manifest.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * mikroBUS manifest definition
> + * extension to Greybus Manifest Definition
> + *
> + * Copyright 2014-2015 Google Inc.
> + * Copyright 2014-2015 Linaro Ltd.
> + *
> + * Released under the GPLv2 and BSD licenses.
> + */
> +
> +#ifndef __MIKROBUS_MANIFEST_H
> +#define __MIKROBUS_MANIFEST_H
> +
> +#include "mikrobus_core.h"
> +
> +/*
> + * mikrobus_manifest_header - parse mikroBUS manifest
> + *
> + * @info: addon board info structure to populate with parsed information
> + * @data: pointer to the manifest blob
> + * @size: size of the manifest blob
> + *
> + * returns: number of devices on success, negative error code on failure
> + */
> +int mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> + size_t size);
> +
> +#endif /* __MIKROBUS_MANIFEST_H */

2024-03-19 09:38:48

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Hi,

> Regardless, this patch actually does not contain any code for EEPROM
> support I have just mentioned it to give more context on why mikroBUS
> manifest is the focus of this patch instead of DT overlay or something
> else.

Right, and I think this is the crux here. Why can't you use DT
overlays? The manifest files, seem to be yet another hardware
description (method) and we already have DT. Can't we have some kind
of userspace helper that could translate them to DT overlays? That
way, you could also handle the EEPROM vs non-EEPROM case, or have
some other kind of method to load a DT overlay.

Admittedly, I've never worked with in-kernel overlays, but AFAIK
they work with some subsystems.

-michael

Attachments:

signature.asc (259.00 B)

2024-03-19 11:37:14

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 3/19/24 15:08, Michael Walle wrote:

> Hi,
>
>> Regardless, this patch actually does not contain any code for EEPROM
>> support I have just mentioned it to give more context on why mikroBUS
>> manifest is the focus of this patch instead of DT overlay or something
>> else.
> Right, and I think this is the crux here. Why can't you use DT
> overlays? The manifest files, seem to be yet another hardware
> description (method) and we already have DT. Can't we have some kind
> of userspace helper that could translate them to DT overlays? That
> way, you could also handle the EEPROM vs non-EEPROM case, or have
> some other kind of method to load a DT overlay.
>
> Admittedly, I've never worked with in-kernel overlays, but AFAIK
> they work with some subsystems.
>
> -michael

So let me 1st go over 3 cases that the driver needs to support:

1. Non EEPROM boards:

Using overlays should be pretty similar to current solution. If the
manifest is converted to overlay in userspace, then we do not even need
to do manifest parsing, setting up spi, i2c etc in the kernel driver.

2. EEPROM boards

How do you propose handling these. If you are proposing storing dt
overlay in EEPROM, then this raises some questions regarding support
outside of Linux.

The other option would be generating overlay from manifest in the kernel
driver, which I'm not sure is significantly better than registering the
i2c, spi, etc. interfaces separately using standard kernel APIs.

3. Over Greybus

It is quite important to have mikroBUS over greybus for BeagleConnect.
This is one of the major reasons why greybus manifest was chosen for the
manifest format.

Also, it is important to note that mikroBUS manifest is being used since
2020 now and thus manifests for a lot of boards (both supporting clickID
and not supporting it exist). So I would prefer using it, unless of
course there are strong reasons not to.

Ayush Singh

CorrectBasicCloseSpellingPossible spelling mistake found.GrabsGrey
busIgnore

2024-03-19 12:08:51

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On Tue Mar 19, 2024 at 12:36 PM CET, Ayush Singh wrote:
> >> Regardless, this patch actually does not contain any code for EEPROM
> >> support I have just mentioned it to give more context on why mikroBUS
> >> manifest is the focus of this patch instead of DT overlay or something
> >> else.
> > Right, and I think this is the crux here. Why can't you use DT
> > overlays? The manifest files, seem to be yet another hardware
> > description (method) and we already have DT. Can't we have some kind
> > of userspace helper that could translate them to DT overlays? That
> > way, you could also handle the EEPROM vs non-EEPROM case, or have
> > some other kind of method to load a DT overlay.
> >
> > Admittedly, I've never worked with in-kernel overlays, but AFAIK
> > they work with some subsystems.
> >
> > -michael
>
>
> So let me 1st go over 3 cases that the driver needs to support:
>
> 1. Non EEPROM boards:
>
> Using overlays should be pretty similar to current solution. If the
> manifest is converted to overlay in userspace, then we do not even need
> to do manifest parsing, setting up spi, i2c etc in the kernel driver.
>
>
> 2. EEPROM boards
>
> How do you propose handling these. If you are proposing storing dt
> overlay in EEPROM, then this raises some questions regarding support
> outside of Linux.
>
> The other option would be generating overlay from manifest in the kernel
> driver, which I'm not sure is significantly better than registering the
> i2c, spi, etc. interfaces separately using standard kernel APIs.

You did answer that yourself in (1): you could use a user space
helper to translate it to a DT overlay, I don't think this has to be
done in the kernel. Also how do you know whether there is an EEPROM
or not?

> 3. Over Greybus
>
> It is quite important to have mikroBUS over greybus for BeagleConnect.
> This is one of the major reasons why greybus manifest was chosen for the
> manifest format.
>
> Also, it is important to note that mikroBUS manifest is being used since
> 2020 now and thus manifests for a lot of boards (both supporting clickID
> and not supporting it exist). So I would prefer using it, unless of
> course there are strong reasons not to.

And also here, I'm not really familiar with greybus. Could you give
a more complex example? My concern is that some driver might need
additional properties from DT (or software nodes) to function
properly. It might not only be a node with a compatible string but
also more advanced bindings. How would that play together with this?
My gut feeling is that you can handle any missing properties
easier/better (eg. for existing modules) in user space. But maybe
that is already solved in/with greybus?

Here's a random one: the manifest [1] just lists the compatible
string apparently, but the actual DT binding has also reset-gpios,
some -supply and interrupt properties.

-michael

[1] https://github.com/MikroElektronika/click_id/blob/main/manifests/WEATHER-CLICK.mnfs

Attachments:

signature.asc (259.00 B)

2024-03-19 12:26:04

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

> The device tree defines the SPI controller associated with mikroBUS SPI
> pins. The driver on match queries and takes a reference to the SPI
> controller but does nothing with it. Once a mikroBUS add-on board is
> detected (by passing manifest using sysfs or reading from 1-wire EEPROM),
> the driver parses the manifest, and if it detects an SPI device in manifest,
> it registers SPI device along with setting properties such as `chip_select`,
> `max_speed_hz`, `mode`, etc.,

How complex can the description of the hardware be in the manifest?

Could i describe an SPI to I2C converter? And then a few temperature
sensors, a fan controller, and a GPIO controller on that I2C bus? And
the GPIO controller is then used for LEDs and a push button? DT
overlays could describe that. Can the manifest?

Andrew

2024-03-19 13:04:11

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 3/19/24 17:38, Michael Walle wrote:

> On Tue Mar 19, 2024 at 12:36 PM CET, Ayush Singh wrote:
>>>> Regardless, this patch actually does not contain any code for EEPROM
>>>> support I have just mentioned it to give more context on why mikroBUS
>>>> manifest is the focus of this patch instead of DT overlay or something
>>>> else.
>>> Right, and I think this is the crux here. Why can't you use DT
>>> overlays? The manifest files, seem to be yet another hardware
>>> description (method) and we already have DT. Can't we have some kind
>>> of userspace helper that could translate them to DT overlays? That
>>> way, you could also handle the EEPROM vs non-EEPROM case, or have
>>> some other kind of method to load a DT overlay.
>>>
>>> Admittedly, I've never worked with in-kernel overlays, but AFAIK
>>> they work with some subsystems.
>>>
>>> -michael
>>
>> So let me 1st go over 3 cases that the driver needs to support:
>>
>> 1. Non EEPROM boards:
>>
>> Using overlays should be pretty similar to current solution. If the
>> manifest is converted to overlay in userspace, then we do not even need
>> to do manifest parsing, setting up spi, i2c etc in the kernel driver.
>>
>>
>> 2. EEPROM boards
>>
>> How do you propose handling these. If you are proposing storing dt
>> overlay in EEPROM, then this raises some questions regarding support
>> outside of Linux.
>>
>> The other option would be generating overlay from manifest in the kernel
>> driver, which I'm not sure is significantly better than registering the
>> i2c, spi, etc. interfaces separately using standard kernel APIs.
> You did answer that yourself in (1): you could use a user space
> helper to translate it to a DT overlay, I don't think this has to be
> done in the kernel.

I do not understand what you mean. For EEPROM supported boards, user
space is not involved. The driver can directly read the manifest from
add-on board and setup everything, so it is plug and play.

The manual involvement of user space is only for non EEPROM boards since
we do not have a way to identify the board without the user needing to
provide the manifest.

> Also how do you know whether there is an EEPROM
> or not?

Set RST GPIO to low. clickID supported board will enter ID MODE, Then
check if CS line has a w1 gpio bus.

>> 3. Over Greybus
>>
>> It is quite important to have mikroBUS over greybus for BeagleConnect.
>> This is one of the major reasons why greybus manifest was chosen for the
>> manifest format.
>>
>> Also, it is important to note that mikroBUS manifest is being used since
>> 2020 now and thus manifests for a lot of boards (both supporting clickID
>> and not supporting it exist). So I would prefer using it, unless of
>> course there are strong reasons not to.
> And also here, I'm not really familiar with greybus. Could you give
> a more complex example? My concern is that some driver might need
> additional properties from DT (or software nodes) to function
> properly. It might not only be a node with a compatible string but
> also more advanced bindings. How would that play together with this?
> My gut feeling is that you can handle any missing properties
> easier/better (eg. for existing modules) in user space. But maybe
> that is already solved in/with greybus?

Greybus is a communication protocol designed for modular electronic
devices. It allows different parts of a device to be hot plugged (added
or removed) while the device is still running. Greybus manifest is used
to describe the capabilities of a module in the greybus network. The
host then creates appropriate bidirectional unipro connections with the
module based on the cports described in the manifest. I have added a
link to lwn article that goes into more detail.

BeagleConnect simply allows using greybus over any bidirectional
transport, instead of just Unipro.

I cannot comment much about how greybus handles missing properties.
While greybus also works just in kernel space, greybus protocols are
inherently higher level than kernel driver, so it might have an easier
time with this.

I have also added a link to eLInux page which provides rational for the
mikroBUS manifest. But the crux seems to be that dynamic overlays were
not well-supported back then. Also, the use of mikroBUS using greybus
subsystem was already used. Hence the mikroBUS driver.

Greybus is not a big blocker from my perspective, since it is always
possible to introduce a new protocol for mikroBUS in Greybus spec. I
think as long as both EEPROM and non EEPROM boards can be supported by
mikroBUS driver and dt-bindings, are can be used outside of Linux (eg:
ZephyrRTOS, nuttx, etc), it is fine.

> Here's a random one: the manifest [1] just lists the compatible
> string apparently, but the actual DT binding has also reset-gpios,
> some -supply and interrupt properties.
>
> -michael
>
> [1] https://github.com/MikroElektronika/click_id/blob/main/manifests/WEATHER-CLICK.mnfs

Yes, the concern is valid. Support for validating the manifest is
nowhere near as good as devicetree overlays. But I think that would be a
problem with the device rather than the responsibility of the kernel. It
is up to the manufacturer to have valid manifests.

Link: https://lwn.net/Articles/715955/ Greybus

Link https://elinux.org/Mikrobus eLinux article

Ayush Singh

2024-03-19 14:21:42

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On Tue Mar 19, 2024 at 2:03 PM CET, Ayush Singh wrote:
> >>>> Regardless, this patch actually does not contain any code for EEPROM
> >>>> support I have just mentioned it to give more context on why mikroBUS
> >>>> manifest is the focus of this patch instead of DT overlay or something
> >>>> else.
> >>> Right, and I think this is the crux here. Why can't you use DT
> >>> overlays? The manifest files, seem to be yet another hardware
> >>> description (method) and we already have DT. Can't we have some kind
> >>> of userspace helper that could translate them to DT overlays? That
> >>> way, you could also handle the EEPROM vs non-EEPROM case, or have
> >>> some other kind of method to load a DT overlay.
> >>>
> >>> Admittedly, I've never worked with in-kernel overlays, but AFAIK
> >>> they work with some subsystems.
> >>>
> >>> -michael
> >>
> >> So let me 1st go over 3 cases that the driver needs to support:
> >>
> >> 1. Non EEPROM boards:
> >>
> >> Using overlays should be pretty similar to current solution. If the
> >> manifest is converted to overlay in userspace, then we do not even need
> >> to do manifest parsing, setting up spi, i2c etc in the kernel driver.
> >>
> >>
> >> 2. EEPROM boards
> >>
> >> How do you propose handling these. If you are proposing storing dt
> >> overlay in EEPROM, then this raises some questions regarding support
> >> outside of Linux.
> >>
> >> The other option would be generating overlay from manifest in the kernel
> >> driver, which I'm not sure is significantly better than registering the
> >> i2c, spi, etc. interfaces separately using standard kernel APIs.
> > You did answer that yourself in (1): you could use a user space
> > helper to translate it to a DT overlay, I don't think this has to be
> > done in the kernel.
>
> I do not understand what you mean. For EEPROM supported boards, user
> space is not involved. The driver can directly read the manifest from
> add-on board and setup everything, so it is plug and play.

A driver could call a user-space helper, which will read the EEPROM
content (or maybe the driver already passes the content to the
helper), translate it to a DT overlay, and load it. Wouldn't that
work?

I'm not saying that is the way to go, just evaluate some ideas.

> The manual involvement of user space is only for non EEPROM boards since
> we do not have a way to identify the board without the user needing to
> provide the manifest.

FWIW, I'm not talking about manual steps here. But more of
call_usermodehelper(). Or maybe udev can do it?

Btw, [1] mentions hot-plugging. Is that really hot-plugging while
the system is running? How would that work?

> > Also how do you know whether there is an EEPROM
> > or not?
>
> Set RST GPIO to low. clickID supported board will enter ID MODE, Then
> check if CS line has a w1 gpio bus.

Ok.

> >> 3. Over Greybus
> >>
> >> It is quite important to have mikroBUS over greybus for BeagleConnect.
> >> This is one of the major reasons why greybus manifest was chosen for the
> >> manifest format.
> >>
> >> Also, it is important to note that mikroBUS manifest is being used since
> >> 2020 now and thus manifests for a lot of boards (both supporting clickID
> >> and not supporting it exist). So I would prefer using it, unless of
> >> course there are strong reasons not to.
> > And also here, I'm not really familiar with greybus. Could you give
> > a more complex example? My concern is that some driver might need
> > additional properties from DT (or software nodes) to function
> > properly. It might not only be a node with a compatible string but
> > also more advanced bindings. How would that play together with this?
> > My gut feeling is that you can handle any missing properties
> > easier/better (eg. for existing modules) in user space. But maybe
> > that is already solved in/with greybus?
>
> Greybus is a communication protocol designed for modular electronic
> devices. It allows different parts of a device to be hot plugged (added
> or removed) while the device is still running. Greybus manifest is used
> to describe the capabilities of a module in the greybus network. The
> host then creates appropriate bidirectional unipro connections with the
> module based on the cports described in the manifest. I have added a
> link to lwn article that goes into more detail.
>
> BeagleConnect simply allows using greybus over any bidirectional
> transport, instead of just Unipro.
>
> I cannot comment much about how greybus handles missing properties.
> While greybus also works just in kernel space, greybus protocols are
> inherently higher level than kernel driver, so it might have an easier
> time with this.
>
> I have also added a link to eLInux page which provides rational for the
> mikroBUS manifest. But the crux seems to be that dynamic overlays were
> not well-supported back then. Also, the use of mikroBUS using greybus
> subsystem was already used. Hence the mikroBUS driver.

I see this as an opportunity to improve the in-kernel overlays :)

> Greybus is not a big blocker from my perspective, since it is always
> possible to introduce a new protocol for mikroBUS in Greybus spec. I
> think as long as both EEPROM and non EEPROM boards can be supported by
> mikroBUS driver and dt-bindings, are can be used outside of Linux (eg:
> ZephyrRTOS, nuttx, etc), it is fine.
>
> > Here's a random one: the manifest [1] just lists the compatible
> > string apparently, but the actual DT binding has also reset-gpios,
> > some -supply and interrupt properties.
> >
> > -michael
> >
> > [1] https://github.com/MikroElektronika/click_id/blob/main/manifests/WEATHER-CLICK.mnfs
>
>
> Yes, the concern is valid. Support for validating the manifest is
> nowhere near as good as devicetree overlays. But I think that would be a
> problem with the device rather than the responsibility of the kernel. It
> is up to the manufacturer to have valid manifests.

But does the manifest have the capabilities to express all that
information? To me it looks like just some kind of pinmux, some
vendor strings and a (DT) compatible string.
[coming back to this after seeing [2]: there are more properties,
but it seem just be a list of property=value]

What I'd like to avoid is some kind of in-kernel mapping list from
manifest to actual driver instantiation.

I guess you'll get much of that with DT overlays already and if you
have some kind of automatic translation from manifest to DT overlay,
it will still be plug-and-play. You could fix up any missing
properties, etc. manually loading some manifests/dt overlays for
modules without EEPROMs.

Again, a more complex manifest file would really be appreciated
here. Not just a simple "there is exactly one trivial SPI device on
the bus".

FWIW, here is a more complex example [2] which uses the ssd1306
display driver. Dunno if that is a good example, as it seems to use
the fb_ssd1306 driver (at least that's what I'm deducing by reading
the driver-string-id) in staging and there is also ssd1307fb.c in
drivers/video/fbdev. But how are the additional information like
width and height translate to the properties of the driver (device
tree properties, swnode properties, platform_data*)?

On a side note, does the manifest files use the (linux) kernel
module name for the driver-string-id?

-michael

[1] https://github.com/MikroElektronika/click_id/blob/main/README.md
[2] https://github.com/MikroElektronika/click_id/blob/main/manifests/OLEDB-CLICK.mnfs

> Link: https://lwn.net/Articles/715955/ Greybus
> Link https://elinux.org/Mikrobus eLinux article

Attachments:

signature.asc (259.00 B)

2024-03-19 17:20:05

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Hi Michael,

On 19/03/24 19:51, Michael Walle wrote:
> On Tue Mar 19, 2024 at 2:03 PM CET, Ayush Singh wrote:
>>>>>> Regardless, this patch actually does not contain any code for EEPROM
>>>>>> support I have just mentioned it to give more context on why mikroBUS
>>>>>> manifest is the focus of this patch instead of DT overlay or something
>>>>>> else.
>>>>> Right, and I think this is the crux here. Why can't you use DT
>>>>> overlays? The manifest files, seem to be yet another hardware
>>>>> description (method) and we already have DT. Can't we have some kind
>>>>> of userspace helper that could translate them to DT overlays? That
>>>>> way, you could also handle the EEPROM vs non-EEPROM case, or have
>>>>> some other kind of method to load a DT overlay.
>>>>>
>>>>> Admittedly, I've never worked with in-kernel overlays, but AFAIK
>>>>> they work with some subsystems.
>>>>>
>>>>> -michael
>>>>
>>>> So let me 1st go over 3 cases that the driver needs to support:
>>>>
>>>> 1. Non EEPROM boards:
>>>>
>>>> Using overlays should be pretty similar to current solution. If the
>>>> manifest is converted to overlay in userspace, then we do not even need
>>>> to do manifest parsing, setting up spi, i2c etc in the kernel driver.
>>>>
>>>>
>>>> 2. EEPROM boards
>>>>
>>>> How do you propose handling these. If you are proposing storing dt
>>>> overlay in EEPROM, then this raises some questions regarding support
>>>> outside of Linux.
>>>>
>>>> The other option would be generating overlay from manifest in the kernel
>>>> driver, which I'm not sure is significantly better than registering the
>>>> i2c, spi, etc. interfaces separately using standard kernel APIs.
>>> You did answer that yourself in (1): you could use a user space
>>> helper to translate it to a DT overlay, I don't think this has to be
>>> done in the kernel.
>>
>> I do not understand what you mean. For EEPROM supported boards, user
>> space is not involved. The driver can directly read the manifest from
>> add-on board and setup everything, so it is plug and play.
>
> A driver could call a user-space helper, which will read the EEPROM
> content (or maybe the driver already passes the content to the
> helper), translate it to a DT overlay, and load it. Wouldn't that
> work?
>
> I'm not saying that is the way to go, just evaluate some ideas.
>

This would work in most cases when we want to instantiate devices on a
physical mikroBUS port on the host running Linux, but another use case
we need to support is to instantiate devices on a virtual/greybus
mikroBUS port created through greybus, this is the case when a remote
microcontroller board (Example BeagleConnect Freedom) has mikroBUS ports
and through the magic of greybus these virtual ports (corresponding to
the physical remote ports) appear on the Linux host - now we cannot use
a device tree overlay to instantiate a Weather click (BME280) sensor on
this port, that is why the choice of extending greybus manifest was
chosen, another alternative here is to go and add device tree as a
description mechanism for greybus, please let know if that is the
recommended way forward?

The greybus manifest already is being used in the greybus susbystem for
describing an interface and there are already greybus controllers
(SPI/I2C .etc) being created according to the manifest contents, all
this driver does is to extend that format to be able to instantiate
devices on these buses. The primary goals for introducing the driver for
mikroBUS add-on boards are:

1) A way to isolate platform specific information from add-on board
specific information - so that each permutation of connecting the add-on
board on different ports on different board does not require a new overlay.
2) A way to instantiate add-on boards on greybus created virtual
mikroBUS ports.
3) Both 1 and 2 should use the same add-on board description format.

Standard device tree overlays did not help to achieve this and that is
why the standard interface discovery mechanism in greybus, the manifest
was extended even though it is not the most optimal way to describe
hardware.

>> The manual involvement of user space is only for non EEPROM boards since
>> we do not have a way to identify the board without the user needing to
>> provide the manifest.
>
> FWIW, I'm not talking about manual steps here. But more of
> call_usermodehelper(). Or maybe udev can do it?
>
> Btw, [1] mentions hot-plugging. Is that really hot-plugging while
> the system is running? How would that work?
>

This should be corrected, it is not recommended to hot-plug the board as
the connector standard does not ensure any power sequencing and can
cause damage.

>>> Also how do you know whether there is an EEPROM
>>> or not?
>>
>> Set RST GPIO to low. clickID supported board will enter ID MODE, Then
>> check if CS line has a w1 gpio bus.
>
> Ok.
>
>>>> 3. Over Greybus
>>>>
>>>> It is quite important to have mikroBUS over greybus for BeagleConnect.
>>>> This is one of the major reasons why greybus manifest was chosen for the
>>>> manifest format.
>>>>
>>>> Also, it is important to note that mikroBUS manifest is being used since
>>>> 2020 now and thus manifests for a lot of boards (both supporting clickID
>>>> and not supporting it exist). So I would prefer using it, unless of
>>>> course there are strong reasons not to.
>>> And also here, I'm not really familiar with greybus. Could you give
>>> a more complex example? My concern is that some driver might need
>>> additional properties from DT (or software nodes) to function
>>> properly. It might not only be a node with a compatible string but
>>> also more advanced bindings. How would that play together with this?
>>> My gut feeling is that you can handle any missing properties
>>> easier/better (eg. for existing modules) in user space. But maybe
>>> that is already solved in/with greybus?
>>
>> Greybus is a communication protocol designed for modular electronic
>> devices. It allows different parts of a device to be hot plugged (added
>> or removed) while the device is still running. Greybus manifest is used
>> to describe the capabilities of a module in the greybus network. The
>> host then creates appropriate bidirectional unipro connections with the
>> module based on the cports described in the manifest. I have added a
>> link to lwn article that goes into more detail.
>>
>> BeagleConnect simply allows using greybus over any bidirectional
>> transport, instead of just Unipro.
>>
>> I cannot comment much about how greybus handles missing properties.
>> While greybus also works just in kernel space, greybus protocols are
>> inherently higher level than kernel driver, so it might have an easier
>> time with this.
>>
>> I have also added a link to eLInux page which provides rational for the
>> mikroBUS manifest. But the crux seems to be that dynamic overlays were
>> not well-supported back then. Also, the use of mikroBUS using greybus
>> subsystem was already used. Hence the mikroBUS driver.
>
> I see this as an opportunity to improve the in-kernel overlays :)
>
>> Greybus is not a big blocker from my perspective, since it is always
>> possible to introduce a new protocol for mikroBUS in Greybus spec. I
>> think as long as both EEPROM and non EEPROM boards can be supported by
>> mikroBUS driver and dt-bindings, are can be used outside of Linux (eg:
>> ZephyrRTOS, nuttx, etc), it is fine.
>>
>>> Here's a random one: the manifest [1] just lists the compatible
>>> string apparently, but the actual DT binding has also reset-gpios,
>>> some -supply and interrupt properties.
>>>
>>> -michael
>>>
>>> [1] https://github.com/MikroElektronika/click_id/blob/main/manifests/WEATHER-CLICK.mnfs
>>
>>
>> Yes, the concern is valid. Support for validating the manifest is
>> nowhere near as good as devicetree overlays. But I think that would be a
>> problem with the device rather than the responsibility of the kernel. It
>> is up to the manufacturer to have valid manifests.
>
> But does the manifest have the capabilities to express all that
> information? To me it looks like just some kind of pinmux, some
> vendor strings and a (DT) compatible string.
> [coming back to this after seeing [2]: there are more properties,
> but it seem just be a list of property=value]
>
> What I'd like to avoid is some kind of in-kernel mapping list from
> manifest to actual driver instantiation.

The property descriptor is implemented to account the properties under
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/property.h#n22

There is no in-kernel mapping that needs to be updated per driver, but a
generic mapping and some specific mapping depending on the bus the
device is connected (I2C/SPI/.etc)

>
> I guess you'll get much of that with DT overlays already and if you
> have some kind of automatic translation from manifest to DT overlay,
> it will still be plug-and-play. You could fix up any missing
> properties, etc. manually loading some manifests/dt overlays for
> modules without EEPROMs.
>
> Again, a more complex manifest file would really be appreciated
> here. Not just a simple "there is exactly one trivial SPI device on
> the bus".
>
> FWIW, here is a more complex example [2] which uses the ssd1306
> display driver. Dunno if that is a good example, as it seems to use
> the fb_ssd1306 driver (at least that's what I'm deducing by reading
> the driver-string-id) in staging and there is also ssd1307fb.c in
> drivers/video/fbdev. But how are the additional information like
> width and height translate to the properties of the driver (device
> tree properties, swnode properties, platform_data*)?
>

The driver uses device_property_read_* helpers to fetch the infromation
and the mikroBUS driver populates the table of properties fetching the
information from manifest and combining with platform information.

> On a side note, does the manifest files use the (linux) kernel
> module name for the driver-string-id?
>

The spi_device_id is used for the driver-string-id :
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fbtft/fbtft.h#n361

Thanks and Regards,
Vaishnav

> -michael
>
> [1] https://github.com/MikroElektronika/click_id/blob/main/README.md
> [2] https://github.com/MikroElektronika/click_id/blob/main/manifests/OLEDB-CLICK.mnfs
>
>> Link: https://lwn.net/Articles/715955/ Greybus
>> Link https://elinux.org/Mikrobus eLinux article
>

2024-03-19 17:35:31

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 3/19/24 22:49, Vaishnav Achath wrote:
>> A driver could call a user-space helper, which will read the EEPROM
>> content (or maybe the driver already passes the content to the
>> helper), translate it to a DT overlay, and load it. Wouldn't that
>> work?
>>
>> I'm not saying that is the way to go, just evaluate some ideas.
>>
>
> This would work in most cases when we want to instantiate devices on a
> physical mikroBUS port on the host running Linux, but another use case
> we need to support is to instantiate devices on a virtual/greybus
> mikroBUS port created through greybus, this is the case when a remote
> microcontroller board (Example BeagleConnect Freedom) has mikroBUS
> ports and through the magic of greybus these virtual ports
> (corresponding to the physical remote ports) appear on the Linux host
> - now we cannot use a device tree overlay to instantiate a Weather
> click (BME280) sensor on this port, that is why the choice of
> extending greybus manifest was chosen, another alternative here is to
> go and add device tree as a description mechanism for greybus, please
> let know if that is the recommended way forward?
>
> The greybus manifest already is being used in the greybus susbystem
> for describing an interface and there are already greybus controllers
> (SPI/I2C .etc) being created according to the manifest contents, all
> this driver does is to extend that format to be able to instantiate
> devices on these buses. The primary goals for introducing the driver
> for mikroBUS add-on boards are:
>
> 1) A way to isolate platform specific information from add-on board
> specific information - so that each permutation of connecting the
> add-on board on different ports on different board does not require a
> new overlay.
> 2) A way to instantiate add-on boards on greybus created virtual
> mikroBUS ports.
> 3) Both 1 and 2 should use the same add-on board description format.
>
> Standard device tree overlays did not help to achieve this and that is
> why the standard interface discovery mechanism in greybus, the
> manifest was extended even though it is not the most optimal way to
> describe hardware.

Yes, after discussion with Vaishnav and trying to brainstorm some way to
do the same thing with dt overlays, it seems that trying to use dt
overlays will mean need to have completely separate implementation of
mikroBUS for local ports and mikroBUS over greybus. Additionally, trying
to put dt overlays in EEPROM would mean they will be incompatible with
use in local ports and vice versa.

Also, I feel like jumping to userspace can fail for all sorts of reasons
and undermine the plug and play support that clickID strives for.

>
>>> The manual involvement of user space is only for non EEPROM boards
>>> since
>>> we do not have a way to identify the board without the user needing to
>>> provide the manifest.
>>
>> FWIW, I'm not talking about manual steps here. But more of
>> call_usermodehelper(). Or maybe udev can do it?
>>
>> Btw, [1] mentions hot-plugging. Is that really hot-plugging while
>> the system is running? How would that work?
>>
>
> This should be corrected, it is not recommended to hot-plug the board
> as the connector standard does not ensure any power sequencing and can
> cause damage.

Yes, as long as local port is concerned, hot plugging is not
recommended. However, when using greybus, it is possible to hotplug the
node that mikroBUS is connected to.

>
>>>> Also how do you know whether there is an EEPROM
>>>> or not?
>>>
>>> Set RST GPIO to low. clickID supported board will enter ID MODE, Then
>>> check if CS line has a w1 gpio bus.
>>
>> Ok.
>>
>>>>> 3. Over Greybus
>>>>>
>>>>> It is quite important to have mikroBUS over greybus for
>>>>> BeagleConnect.
>>>>> This is one of the major reasons why greybus manifest was chosen
>>>>> for the
>>>>> manifest format.
>>>>>
>>>>> Also, it is important to note that mikroBUS manifest is being used
>>>>> since
>>>>> 2020 now and thus manifests for a lot of boards (both supporting
>>>>> clickID
>>>>> and not supporting it exist). So I would prefer using it, unless of
>>>>> course there are strong reasons not to.
>>>> And also here, I'm not really familiar with greybus. Could you give
>>>> a more complex example? My concern is that some driver might need
>>>> additional properties from DT (or software nodes) to function
>>>> properly. It might not only be a node with a compatible string but
>>>> also more advanced bindings. How would that play together with this?
>>>> My gut feeling is that you can handle any missing properties
>>>> easier/better (eg. for existing modules) in user space. But maybe
>>>> that is already solved in/with greybus?
>>>
>>> Greybus is a communication protocol designed for modular electronic
>>> devices. It allows different parts of a device to be hot plugged (added
>>> or removed) while the device is still running. Greybus manifest is used
>>> to describe the capabilities of a module in the greybus network. The
>>> host then creates appropriate bidirectional unipro connections with the
>>> module based on the cports described in the manifest. I have added a
>>> link to lwn article that goes into more detail.
>>>
>>> BeagleConnect simply allows using greybus over any bidirectional
>>> transport, instead of just Unipro.
>>>
>>> I cannot comment much about how greybus handles missing properties.
>>> While greybus also works just in kernel space, greybus protocols are
>>> inherently higher level than kernel driver, so it might have an easier
>>> time with this.
>>>
>>> I have also added a link to eLInux page which provides rational for the
>>> mikroBUS manifest. But the crux seems to be that dynamic overlays were
>>> not well-supported back then. Also, the use of mikroBUS using greybus
>>> subsystem was already used. Hence the mikroBUS driver.
>>
>> I see this as an opportunity to improve the in-kernel overlays :)
>>
>>> Greybus is not a big blocker from my perspective, since it is always
>>> possible to introduce a new protocol for mikroBUS in Greybus spec. I
>>> think as long as both EEPROM and non EEPROM boards can be supported by
>>> mikroBUS driver and dt-bindings, are can be used outside of Linux (eg:
>>> ZephyrRTOS, nuttx, etc), it is fine.
>>>
>>>> Here's a random one: the manifest [1] just lists the compatible
>>>> string apparently, but the actual DT binding has also reset-gpios,
>>>> some -supply and interrupt properties.
>>>>
>>>> -michael
>>>>
>>>> [1]
>>>> https://github.com/MikroElektronika/click_id/blob/main/manifests/WEATHER-CLICK.mnfs
>>>
>>>
>>> Yes, the concern is valid. Support for validating the manifest is
>>> nowhere near as good as devicetree overlays. But I think that would
>>> be a
>>> problem with the device rather than the responsibility of the
>>> kernel. It
>>> is up to the manufacturer to have valid manifests.
>>
>> But does the manifest have the capabilities to express all that
>> information? To me it looks like just some kind of pinmux, some
>> vendor strings and a (DT) compatible string.
>> [coming back to this after seeing [2]: there are more properties,
>> but it seem just be a list of property=value]
>>
>> What I'd like to avoid is some kind of in-kernel mapping list from
>> manifest to actual driver instantiation.
>
> The property descriptor is implemented to account the properties under
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/property.h#n22
>
>
> There is no in-kernel mapping that needs to be updated per driver, but
> a generic mapping and some specific mapping depending on the bus the
> device is connected (I2C/SPI/.etc)
>
>>
>> I guess you'll get much of that with DT overlays already and if you
>> have some kind of automatic translation from manifest to DT overlay,
>> it will still be plug-and-play. You could fix up any missing
>> properties, etc. manually loading some manifests/dt overlays for
>> modules without EEPROMs.
>>
>> Again, a more complex manifest file would really be appreciated
>> here. Not just a simple "there is exactly one trivial SPI device on
>> the bus".
>>
>> FWIW, here is a more complex example [2] which uses the ssd1306
>> display driver. Dunno if that is a good example, as it seems to use
>> the fb_ssd1306 driver (at least that's what I'm deducing by reading
>> the driver-string-id) in staging and there is also ssd1307fb.c in
>> drivers/video/fbdev. But how are the additional information like
>> width and height translate to the properties of the driver (device
>> tree properties, swnode properties, platform_data*)?
>>
>
> The driver uses device_property_read_* helpers to fetch the
> infromation and the mikroBUS driver populates the table of properties
> fetching the information from manifest and combining with platform
> information.
>
>> On a side note, does the manifest files use the (linux) kernel
>> module name for the driver-string-id?
>>
>
> The spi_device_id is used for the driver-string-id :
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fbtft/fbtft.h#n361
>
>
> Thanks and Regards,
> Vaishnav
>
>> -michael
>>
>> [1] https://github.com/MikroElektronika/click_id/blob/main/README.md
>> [2]
>> https://github.com/MikroElektronika/click_id/blob/main/manifests/OLEDB-CLICK.mnfs
>>
>>> Link: https://lwn.net/Articles/715955/ Greybus
>>> Link https://elinux.org/Mikrobus eLinux article
>>

I apologize if I gave the wrong impression, but the ability to enable
mikroBUS over Greybus is extremely important here. I left out mikroBUS
over greybus support from this patchset to allow for easier upstreaming
of a new driver. However, that does not mean it is a secondary goal of
this patch.

The current mikroBUS manifest is compatible with Greybus manifest, and
thus is easy to integrate into standard Greybus. Anything suggested here
still needs to enable support of mikroBUS over Greybus.

Ayush Singh

2024-03-19 17:42:11

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Hi Andrew,

On 19/03/24 17:55, Andrew Lunn wrote:
>> The device tree defines the SPI controller associated with mikroBUS SPI
>> pins. The driver on match queries and takes a reference to the SPI
>> controller but does nothing with it. Once a mikroBUS add-on board is
>> detected (by passing manifest using sysfs or reading from 1-wire EEPROM),
>> the driver parses the manifest, and if it detects an SPI device in manifest,
>> it registers SPI device along with setting properties such as `chip_select`,
>> `max_speed_hz`, `mode`, etc.,
>
> How complex can the description of the hardware be in the manifest?
>
> Could i describe an SPI to I2C converter? And then a few temperature
> sensors, a fan controller, and a GPIO controller on that I2C bus? And
> the GPIO controller is then used for LEDs and a push button? DT
> overlays could describe that. Can the manifest?

No, it cannot describe such complex hardware, it can only describe
simple devices (sensors/displays .etc) on a standard mikroBUS add-on
board, we did a analysis on what mikroBUS add-on boards have driver
support in Linux and then noticed that most devices does not need this
kind of complex description to work:
https://elinux.org/MikroEClicks_with_Linux_Support

The greybus manifest already is being used in the greybus susbystem for
describing an interface and there are already greybus controllers
(SPI/I2C .etc) being created according to the manifest contents, all
this driver does is to extend that format to be able to instantiate
devices on these buses. The primary goals for introducing the driver for
mikroBUS add-on boards are:

1) A way to isolate platform specific information from add-on board
specific information - so that each permutation of connecting the add-on
board on different ports on different board does not require a new overlay.
2) A way to instantiate add-on boards on greybus created virtual
mikroBUS ports.
3) Both 1 and 2 should use the same add-on board description format.

Standard device tree overlays did not help to achieve this and that is
why the standard interface discovery mechanism in greybus, the manifest
was extended even though it is not the most optimal way to describe
hardware.

The greybus manifest extensions were made with the following things in
mind and three new descriptor were introduced:
1) mikrobus descriptor - pinmux/port state
2) device descriptor - contains information which is a superset of
struct i2c_board_info , struct spi_board_info .etc
3) property descriptor - to describe named properties of the types
defined under
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/property.h#n22

With these we were able to test around 150 add-on boards with
corresponding drivers in Linux :
https://github.com/MikroElektronika/click_id/tree/main/manifests

The mechanism is not as robust a device tree and should not be compared,
the intent was not to create a new hardware description format, but
extend the existing greybus manifest format to be able to instantiate
devices on the greybus SPI/I2C/GPIO/ (mikroBUS)

Thanks and Regards,
Vaishnav

>
> Andrew

2024-03-19 18:20:06

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On Tue, Mar 19, 2024 at 11:05:37PM +0530, Vaishnav Achath wrote:
> Hi Andrew,
>
> On 19/03/24 17:55, Andrew Lunn wrote:
> > > The device tree defines the SPI controller associated with mikroBUS SPI
> > > pins. The driver on match queries and takes a reference to the SPI
> > > controller but does nothing with it. Once a mikroBUS add-on board is
> > > detected (by passing manifest using sysfs or reading from 1-wire EEPROM),
> > > the driver parses the manifest, and if it detects an SPI device in manifest,
> > > it registers SPI device along with setting properties such as `chip_select`,
> > > `max_speed_hz`, `mode`, etc.,
> >
> > How complex can the description of the hardware be in the manifest?
> >
> > Could i describe an SPI to I2C converter? And then a few temperature
> > sensors, a fan controller, and a GPIO controller on that I2C bus? And
> > the GPIO controller is then used for LEDs and a push button? DT
> > overlays could describe that. Can the manifest?
>
> No, it cannot describe such complex hardware, it can only describe simple
> devices (sensors/displays .etc) on a standard mikroBUS add-on board, we did
> a analysis on what mikroBUS add-on boards have driver support in Linux and
> then noticed that most devices does not need this kind of complex
> description to work:
> https://elinux.org/MikroEClicks_with_Linux_Support

What happens to the devices that fall outside of the "do not need a
complex description" category? Do you expect that those would be
described by a dt overlay?

> The greybus manifest already is being used in the greybus susbystem for
> describing an interface and there are already greybus controllers (SPI/I2C
> .etc) being created according to the manifest contents, all this driver does
> is to extend that format to be able to instantiate devices on these buses.
> The primary goals for introducing the driver for mikroBUS add-on boards are:
>
> 1) A way to isolate platform specific information from add-on board specific
> information - so that each permutation of connecting the add-on board on
> different ports on different board does not require a new overlay.
> 2) A way to instantiate add-on boards on greybus created virtual mikroBUS
> ports.
> 3) Both 1 and 2 should use the same add-on board description format.
>
> Standard device tree overlays did not help to achieve this and that is why
> the standard interface discovery mechanism in greybus, the manifest was
> extended even though it is not the most optimal way to describe hardware.
>
> The greybus manifest extensions were made with the following things in mind
> and three new descriptor were introduced:
> 1) mikrobus descriptor - pinmux/port state
> 2) device descriptor - contains information which is a superset of struct
> i2c_board_info , struct spi_board_info .etc
> 3) property descriptor - to describe named properties of the types defined
> under https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/property.h#n22
>
> With these we were able to test around 150 add-on boards with corresponding
> drivers in Linux :
> https://github.com/MikroElektronika/click_id/tree/main/manifests
>
> The mechanism is not as robust a device tree and should not be compared, the

Why not? You're suggesting this as a method for describing devices and you
seem to have extended the manifest to support more complex properties, why
shouldn't someone question make that comparison?

> intent was not to create a new hardware description format, but extend the
> existing greybus manifest format to be able to instantiate devices on the
> greybus SPI/I2C/GPIO/ (mikroBUS)

Attachments:

(No filename) (3.62 kB)
signature.asc (235.00 B)
Download all attachments

2024-03-19 19:24:19

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On Tue, Mar 19, 2024 at 11:05:37PM +0530, Vaishnav Achath wrote:
> Hi Andrew,
>
> On 19/03/24 17:55, Andrew Lunn wrote:
> > > The device tree defines the SPI controller associated with mikroBUS SPI
> > > pins. The driver on match queries and takes a reference to the SPI
> > > controller but does nothing with it. Once a mikroBUS add-on board is
> > > detected (by passing manifest using sysfs or reading from 1-wire EEPROM),
> > > the driver parses the manifest, and if it detects an SPI device in manifest,
> > > it registers SPI device along with setting properties such as `chip_select`,
> > > `max_speed_hz`, `mode`, etc.,
> >
> > How complex can the description of the hardware be in the manifest?
> >
> > Could i describe an SPI to I2C converter? And then a few temperature
> > sensors, a fan controller, and a GPIO controller on that I2C bus? And
> > the GPIO controller is then used for LEDs and a push button? DT
> > overlays could describe that. Can the manifest?
>
> No, it cannot describe such complex hardware, it can only describe simple
> devices (sensors/displays .etc) on a standard mikroBUS add-on board, we did
> a analysis on what mikroBUS add-on boards have driver support in Linux and
> then noticed that most devices does not need this kind of complex
> description to work:
> https://elinux.org/MikroEClicks_with_Linux_Support

Is that because the current software support is too limited? Are there
manufactures who want to create more complex designed, but are limited
by what can be described in the manifest?

Do you have a list of boards without Linux support? Why do they not
have Linux support? Is there a "vendor crap" driver which makes them
work? Does it make them work by working around the manifest
limitations?

> The greybus manifest already is being used in the greybus susbystem for
> describing an interface and there are already greybus controllers (SPI/I2C
> .etc) being created according to the manifest contents, all this driver does
> is to extend that format to be able to instantiate devices on these buses.

I don't know anything about greybus, so let me ask a few background
questions. Are these SPI and I2C controller plain Linux SPI and I2C
controllers? They fit the usual device model, they appear in
/sys/class/bus etc? Are the GPIO controllers also just plain Linux
GPIO controllers? All the drivers have a bottom interface which uses
greybus to perform some sort of RPC, but the top interface is standard
Linux. So in fact they are not so different to I2C over USB, SPI over
USB, GPIO over USB?

Andrew

2024-03-19 19:32:56

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

> Yes, after discussion with Vaishnav and trying to brainstorm some way to do
> the same thing with dt overlays, it seems that trying to use dt overlays
> will mean need to have completely separate implementation of mikroBUS for
> local ports and mikroBUS over greybus.

Could you explain why please?

Are greybus I2C bus masters different from physical I2C bus masters?
Are greybus SPI bus masters different from physical SPI bus masters?

> Additionally, trying to put dt overlays in EEPROM would mean they
> will be incompatible with use in local ports and vice versa.

I don't think you need to put the DT overlay in the EEPROM. All you
need to do is translate the manifest into DT for those simple devices
which can be described by the limited manifest format. For more
complex devices, you use the ID to go find a DT fragment which
describes the board, and skip the manifest to DT transformation.

Andrew

2024-03-19 19:38:06

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On Mon, Mar 18, 2024 at 01:07:09AM +0530, Ayush Singh wrote:
> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
> developed by MikroElektronika for connecting add-on boards to
> microcontrollers or microprocessors.
>
> mikroBUS is a connector and does not have a controller. Instead the
> software is responsible for identification of board and setting up /
> registering uart, spi, i2c, pwm and other buses. Thus it needs a way to
> get uart, spi, i2c, pwm and gpio controllers / adapters.
>
> A mikroBUS addon board is free to leave some of the pins unused which
> are marked as NC or Not Connected.

But your binding makes everything required. Why would I need to
instantiate and connect an i2c controller to the mikroebus header if
I don't intend connecting any mikroebus devices that need one?

> Some of the pins might need to be configured as GPIOs deviating from their
> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.

> For some add-on boards the driver may not take care of some additional
> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
> (RST pin on the mikrobus port) needs to be pulled high.

What drivers do is not relevant to the binding. Describe the hardware.

> Here's the list of pins in mikroBUS connector:
> Analog - AN
> Reset - RST
> SPI Chip Select - CS
> SPI Clock - SCK
> SPI Master Input Slave Output - MISO
> SPI Master Output Slave Input - MOSI
> VCC-3.3V power - +3.3V
> Reference Ground - GND
> PWM - PWM output
> INT - Hardware Interrupt
> RX - UART Receive
> TX - UART Transmit
> SCL - I2C Clock
> SDA - I2C Data
> +5V - VCC-5V power
> GND - Reference Ground
>
> Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains
> a manifest to describe the addon board to provide plug and play
> capabilities.

My problem with this is that it purports to be some generic description
of the connector but it appears to be very centred on a particular use
case (this beagle product) with simple add-on boards or those that
support the auto-detection eeprom. For other use cases I'm at a loss for
why I'd not omit this node from my DT and treat the mikroebus connector
like any other header on my boards, where I just apply an overlay that
hooks up the device to the relevant spi/pwm/i2c/etc controllers.

I'd almost go as far as to say that this binding is misleading, because
it's worded like a complete description of the port but actually only
seems to describe a (set of) use case(s). What am I missing?

> Link: https://www.mikroe.com/mikrobus
> Link:
> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
> mikroBUS specification
> Link: https://www.mikroe.com/sht1x-click SHT15 Click
> Link: https://www.mikroe.com/eth-click ENC28J60 Click
> Link: https://www.mikroe.com/clickid ClickID
>
> Co-developed-by: Vaishnav M A <[email protected]>
> Signed-off-by: Vaishnav M A <[email protected]>
> Signed-off-by: Ayush Singh <[email protected]>

> +required:
> + - compatible
> + - pinctrl-0
> + - pinctrl-1
> + - pinctrl-2
> + - pinctrl-3
> + - pinctrl-4
> + - pinctrl-5
> + - pinctrl-6
> + - pinctrl-7
> + - pinctrl-8
> + - i2c-adapter
> + - spi-controller
> + - spi-cs
> + - uart
> + - pwms
> + - mikrobus-gpios

Oh also, you're missing properties for the supplies.

Attachments:

(No filename) (3.45 kB)
signature.asc (235.00 B)
Download all attachments

2024-03-20 07:31:44

[permalink] [raw]

Subject: Re: [PATCH v4 5/5] dts: ti: k3-am625-beagleplay: Add mikroBUS

On 19/03/2024 07:34, Ayush Singh wrote:
>
> On 3/19/24 11:29, Krzysztof Kozlowski wrote:
>> On 17/03/2024 20:37, Ayush Singh wrote:
>>> DONOTMERGE
>> Why? Explain then the purpose of this patch.
>
> Well, it was suggested to have DONOTMERGE by Vaishnav in the patches
> until dt bindings have been approved, since this patch touches different
> subsystems. Here is the full context from v3:
>
>> The reasoning behind this is that these patches go in to separate maintainer trees and without the bindings merged first the device tree changes cannot be validated, thus it is a usual practice to get the bindings and driver merged first and the device tree changes to go in the next cycle. Another alternative is you can point to your fork with all the changes together.

This is some odd style of work. Please don't follow such advise.

>
>>> this patch depends on patch 1
>> How? I don't see any dependency.
>
> I think it is not allowed to have code in device tree unless a
> corresponding dt-binding exists for the device. And thus every time the

And you provided the binding.

> dt-binding changes, this patch also needs to change.So I thought it is
> dependent on patch 1.

But it is not a dependency. Dependency means something does not work
without another. Or something must be applied in the same branch as
another. None of the cases are here. Drop the statements.

This is no different than all of our regular works. Do you see any of
such comments ("dont merge", "dependency")? No.

Best regards,
Krzysztof

2024-03-20 07:33:47

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] mikrobus: Add mikroBUS driver

On 19/03/2024 07:47, Ayush Singh wrote:
> On 3/19/24 11:34, Krzysztof Kozlowski wrote:
>
>> On 17/03/2024 20:37, Ayush Singh wrote:
>>> DONOTMERGE
>>>
>>> this patch depends on Patch 1, 2, 3
>> So none of your work should be reviewed? I don't understand this, but in
>> such case I am not going to review it.
>>
>> Best regards,
>> Krzysztof
>>
> I am a bit lost here. It was mentioned in the patch v3 that I should
> specify the interdependence of patches in v3. And now you are saying I
> should not?
>
> Here is the rationale for the dependence:
>
> 1. Any changes to the property names in dt-bindings patch 1 will need an
> appropriate change here.
>
> 2. This patch will fail to build without patch 2.
>
> 3. This patch will fail to build without patch 3.

This is a natural ordering of patches... but the point is that it makes
ZERO sense once applied to Git repo. Your commit *MUST* make sense in
the Git. Now it does not.

Explain in cover letter what is the merging strategy. You can also
mention in patch changelog (---) that one patch must be applied
toogether with another.

Best regards,
Krzysztof

2024-03-20 16:39:38

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 3/20/24 01:02, Andrew Lunn wrote:

>> Yes, after discussion with Vaishnav and trying to brainstorm some way to do
>> the same thing with dt overlays, it seems that trying to use dt overlays
>> will mean need to have completely separate implementation of mikroBUS for
>> local ports and mikroBUS over greybus.
> Could you explain why please?
>
> Are greybus I2C bus masters different from physical I2C bus masters?
> Are greybus SPI bus masters different from physical SPI bus masters?

Well, they are virtual, so they are not declared in the device tree. I
have linked the greybus i2c implementation. It basically allocates an
i2c_adpater and then adds it using `i2c_add_adapter` method. This
adapter can then be passed to say mikroBUS driver where it can be used
as a normal i2c_adapter, and we can register the device to it.

>> Additionally, trying to put dt overlays in EEPROM would mean they
>> will be incompatible with use in local ports and vice versa.
> I don't think you need to put the DT overlay in the EEPROM. All you
> need to do is translate the manifest into DT for those simple devices
> which can be described by the limited manifest format. For more
> complex devices, you use the ID to go find a DT fragment which
> describes the board, and skip the manifest to DT transformation.
>
> Andrew

I am not familiar enough to know if the device tree can work with
virtual devices created by greybus subsystem.

Maybe the problem stems from the fact that mikroBUS does not have a
physical controller (and my inability to explain the patch properly).
However, the purpose of this patchset is to in fact provide a virtual
mikroBUS controller to allow us to register a mikroBUS addon board
described by board_info struct similar to how it is possible to create
and register an i2c device on an i2c adapter using
`i2c_new_client_device` or spi device using `spi_new_device`. The
manifest is used to populate this board_info struct, but it will be
possible to use something other than mikroBUS manifest if someone wants
to. I can make the necessary adjustments by moving manifest support to
its own config option.

Link:
https://elixir.bootlin.com/linux/latest/source/drivers/staging/greybus/i2c.c#L230
Greybus i2c

Ayush Singh

2024-03-20 18:44:42

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On Wed, Mar 20, 2024 at 10:09:05PM +0530, Ayush Singh wrote:
> On 3/20/24 01:02, Andrew Lunn wrote:
>
> > > Yes, after discussion with Vaishnav and trying to brainstorm some way to do
> > > the same thing with dt overlays, it seems that trying to use dt overlays
> > > will mean need to have completely separate implementation of mikroBUS for
> > > local ports and mikroBUS over greybus.
> > Could you explain why please?
> >
> > Are greybus I2C bus masters different from physical I2C bus masters?
> > Are greybus SPI bus masters different from physical SPI bus masters?
>
> Well, they are virtual, so they are not declared in the device tree. I have
> linked the greybus i2c implementation. It basically allocates an i2c_adpater
> and then adds it using `i2c_add_adapter` method. This adapter can then be
> passed to say mikroBUS driver where it can be used as a normal i2c_adapter,
> and we can register the device to it.

Being virtual does not really stop it being added to the DT.

I'm making this all up, but i assume it will look something like this:

greybus@42 {
compatible = "acme,greybus";
reg = <0x42 0x100>;

This would represent the greybus host controller.

module@0 {
reg = <0>;

This would represent a module discovered on the bus. I assume there is
some sort of addressing? The greybus core code dynamically creates the
node in DT to describe the modules it has discovered. This is not too
different to USB. You can already describe USB devices in DT, but the
assumption is you know they exists, e.g. because they are hard wired,
not hot-plugable. The USB core will associate the USB device with the
node in DT. But actually creating a node in DT is not too big a jump.

interface@0 {
compatible = "greybus,i2c";
reg = <0>;
}
interface@1 {
compatible = "greybus,spi";
reg = <1>;
}
interface@10 {
compatible = "greybus,gpio";
reg = <10>;
}

It can then enumerate the interfaces on the module, and create the I2C
node, SPI bus node, the gpio controller etc. Again, the greybus core
can add nodes to DT to described the discovered hardware, and
associate them to the linux devices which are created.

That gives you what you need to load a DT overlay to make use of these
devices. That overlay would contain one of your virtual mikroBUS
controllers. This virtual controller is basically a phandle-proxy. The
virtual mikroBUS controllers is a consumer of phandles to an I2C bus,
an SPI bus, GPIO bus which makes up the pins routed to the mikroBUS
connector. The virtual mikroBUS controllers is also a provider of an
I2C bus, an SPI bus, GPIO controller. The mikroBUS device consumes
these I2C bus, SPI bus etc. The virtual mikroBUS controllers makes it
simpler for the device to find the resources it needs, since they are
all in one place. For a physical mikroBUS you have a DT node with
phandles to the physical devices. For greybus you create a virtual
device with phandles to the virtual devices added to the DT bus.

You then have everything you need to describe the mikroBUS
devices. For very simple devices you convert the manifest to a DT
overlay and load it. For complex devices you directly use a DT
overlay.

I also don't see any need to do the manifest to DT overlay conversion
on the fly. You have a database of manifests. They could be converted
to DT and then added to the linux-firmware repo, for example. If
device with an unknown manifest is found, it should be possible to
read the manifest in userspace via its eeprom in /sys/class/. An tool
could create DT blob and add it to /lib/firmware to get it working
locally, and provide suggestions how to contribute it to the linux
firmware project?

Andrew

2024-03-21 06:31:22

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Hi Conor,

On 19/03/24 23:49, Conor Dooley wrote:
> On Tue, Mar 19, 2024 at 11:05:37PM +0530, Vaishnav Achath wrote:
>> Hi Andrew,
>>
>> On 19/03/24 17:55, Andrew Lunn wrote:
>>>> The device tree defines the SPI controller associated with mikroBUS SPI
>>>> pins. The driver on match queries and takes a reference to the SPI
>>>> controller but does nothing with it. Once a mikroBUS add-on board is
>>>> detected (by passing manifest using sysfs or reading from 1-wire EEPROM),
>>>> the driver parses the manifest, and if it detects an SPI device in manifest,
>>>> it registers SPI device along with setting properties such as `chip_select`,
>>>> `max_speed_hz`, `mode`, etc.,
>>>
>>> How complex can the description of the hardware be in the manifest?
>>>
>>> Could i describe an SPI to I2C converter? And then a few temperature
>>> sensors, a fan controller, and a GPIO controller on that I2C bus? And
>>> the GPIO controller is then used for LEDs and a push button? DT
>>> overlays could describe that. Can the manifest?
>>
>> No, it cannot describe such complex hardware, it can only describe simple
>> devices (sensors/displays .etc) on a standard mikroBUS add-on board, we did
>> a analysis on what mikroBUS add-on boards have driver support in Linux and
>> then noticed that most devices does not need this kind of complex
>> description to work:
>> https://elinux.org/MikroEClicks_with_Linux_Support
>
> What happens to the devices that fall outside of the "do not need a
> complex description" category? Do you expect that those would be
> described by a dt overlay?
>

Yes, those would need a device tree overlay, but most mikroBUS add-on
boards does not need this, almost all the boards need the standard bus
properties (SPI/I2C properties), IRQ, named-gpios, named properties,
regulators, clocks and the current implementation supports this.

Looking at the example Andrew provided above (SPI-I2C converter with
sensors .etc on the I2C bus and GPIO controller) - usually you will not
find such a mikroBUS add-on board, because if there are I2C devices they
would directly be on the mikroBUS I2C bus rather than on the converter,
now someone can do this in their custom solution but then it is no
different than an I2C adapter on USB/PCIe, does the standard discovery
mechanism on those buses help instantiate devices on the I2C? my
understanding is NO and you will need to write a custom device tree
overlay for the same and same will be the case here.

>> The greybus manifest already is being used in the greybus susbystem for
>> describing an interface and there are already greybus controllers (SPI/I2C
>> .etc) being created according to the manifest contents, all this driver does
>> is to extend that format to be able to instantiate devices on these buses.
>> The primary goals for introducing the driver for mikroBUS add-on boards are:
>>
>> 1) A way to isolate platform specific information from add-on board specific
>> information - so that each permutation of connecting the add-on board on
>> different ports on different board does not require a new overlay.
>> 2) A way to instantiate add-on boards on greybus created virtual mikroBUS
>> ports.
>> 3) Both 1 and 2 should use the same add-on board description format.
>>
>> Standard device tree overlays did not help to achieve this and that is why
>> the standard interface discovery mechanism in greybus, the manifest was
>> extended even though it is not the most optimal way to describe hardware.
>>
>> The greybus manifest extensions were made with the following things in mind
>> and three new descriptor were introduced:
>> 1) mikrobus descriptor - pinmux/port state
>> 2) device descriptor - contains information which is a superset of struct
>> i2c_board_info , struct spi_board_info .etc
>> 3) property descriptor - to describe named properties of the types defined
>> under https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/property.h#n22
>>
>> With these we were able to test around 150 add-on boards with corresponding
>> drivers in Linux :
>> https://github.com/MikroElektronika/click_id/tree/main/manifests
>>
>> The mechanism is not as robust a device tree and should not be compared, the
>
> Why not? You're suggesting this as a method for describing devices and you
> seem to have extended the manifest to support more complex properties, why
> shouldn't someone question make that comparison?
>

Agreed, but the comparison need to limited to the expansion interface
(mikroBUS) under discussion as the idea is not to create an alternate
interface for describing generic devices, the class of add-on boards
that can fit in the mikroBUS add-on board form factor and on the buses
exposed by mikroBUS requires only simple descriptions - namely standard
bus properties (SPI/I2C properties), IRQ, named-gpios, named properties,
regulators, clocks and the extensions to manifest were made for those
properties only. Also the extensions were done to support the properties
under unified device property interface under include/linux/property.h

Thanks and Regards,
Vaishnav

>> intent was not to create a new hardware description format, but extend the
>> existing greybus manifest format to be able to instantiate devices on the
>> greybus SPI/I2C/GPIO/ (mikroBUS)
>

2024-03-21 07:17:47

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Hi Andrew,

On 20/03/24 00:53, Andrew Lunn wrote:
> On Tue, Mar 19, 2024 at 11:05:37PM +0530, Vaishnav Achath wrote:
>> Hi Andrew,
>>
>> On 19/03/24 17:55, Andrew Lunn wrote:
>>>> The device tree defines the SPI controller associated with mikroBUS SPI
>>>> pins. The driver on match queries and takes a reference to the SPI
>>>> controller but does nothing with it. Once a mikroBUS add-on board is
>>>> detected (by passing manifest using sysfs or reading from 1-wire EEPROM),
>>>> the driver parses the manifest, and if it detects an SPI device in manifest,
>>>> it registers SPI device along with setting properties such as `chip_select`,
>>>> `max_speed_hz`, `mode`, etc.,
>>>
>>> How complex can the description of the hardware be in the manifest?
>>>
>>> Could i describe an SPI to I2C converter? And then a few temperature
>>> sensors, a fan controller, and a GPIO controller on that I2C bus? And
>>> the GPIO controller is then used for LEDs and a push button? DT
>>> overlays could describe that. Can the manifest?
>>
>> No, it cannot describe such complex hardware, it can only describe simple
>> devices (sensors/displays .etc) on a standard mikroBUS add-on board, we did
>> a analysis on what mikroBUS add-on boards have driver support in Linux and
>> then noticed that most devices does not need this kind of complex
>> description to work:
>> https://elinux.org/MikroEClicks_with_Linux_Support
>
> Is that because the current software support is too limited? Are there
> manufactures who want to create more complex designed, but are limited
> by what can be described in the manifest?
>

most mikroBUS add-on boards in production lies in the category of
sensors, displays, connectivity, mixed signal (ADC/DAC .etc) and if you
look at the existing bindings under bindings/iio/ , most devices need
only simple descriptions and the properties are mainly standard bus
properties (SPI/I2C properties), IRQ, named-gpios, named properties,
regulators, clocks the extension to manifest was made taking this into
account and the named property description interface just maps the
manifest entries to the unified device property interface under
include/linux/property.h

> Do you have a list of boards without Linux support? Why do they not
> have Linux support? Is there a "vendor crap" driver which makes them
> work? Does it make them work by working around the manifest
> limitations?
>

I did the survey in 2020, close to 600 board did not have Linux support
and 150 board had support then, the boards which did not have Linux
support was mostly because there was no corresponding driver present and
a lot of these were similar to the 150 that had support (IIO sensors,
ADC, DACs .etc), there is no vendor(Example MikroElektronika) drivers
being maintained, so I am not sure if there are drivers working around
limitations of manifests , but for the 150 boards that we have tested
support we never had to make any changes to the underlying device
drivers to be supported.

>> The greybus manifest already is being used in the greybus susbystem for
>> describing an interface and there are already greybus controllers (SPI/I2C
>> .etc) being created according to the manifest contents, all this driver does
>> is to extend that format to be able to instantiate devices on these buses.
>
> I don't know anything about greybus, so let me ask a few background
> questions. Are these SPI and I2C controller plain Linux SPI and I2C
> controllers? They fit the usual device model, they appear in
> /sys/class/bus etc? Are the GPIO controllers also just plain Linux
> GPIO controllers? All the drivers have a bottom interface which uses
> greybus to perform some sort of RPC, but the top interface is standard
> Linux. So in fact they are not so different to I2C over USB, SPI over
> USB, GPIO over USB?

They are very similar and all the details you mentioned are correct, I
will provide some comments on the DT proposal you made and why we could
not implement that approach initially, primarily it is because PCIe and
USB has OF device tree support and USB interface nodes are children of
USB device nodes and there is some hardware parent we can tie USB
interface to and share/derive the of_node, but in case of greybus we
could not find such mapping - looking at your proposal that is more
maintainable in the long term, have some doubts regarding the proposal
will post in the other thread.

Thanks and Regards,
Vaishnav

>
> Andrew

2024-03-21 07:36:09

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Hi Andrew,

On 21/03/24 00:14, Andrew Lunn wrote:
> On Wed, Mar 20, 2024 at 10:09:05PM +0530, Ayush Singh wrote:
>> On 3/20/24 01:02, Andrew Lunn wrote:
>>
>>>> Yes, after discussion with Vaishnav and trying to brainstorm some way to do
>>>> the same thing with dt overlays, it seems that trying to use dt overlays
>>>> will mean need to have completely separate implementation of mikroBUS for
>>>> local ports and mikroBUS over greybus.
>>> Could you explain why please?
>>>
>>> Are greybus I2C bus masters different from physical I2C bus masters?
>>> Are greybus SPI bus masters different from physical SPI bus masters?
>>
>> Well, they are virtual, so they are not declared in the device tree. I have
>> linked the greybus i2c implementation. It basically allocates an i2c_adpater
>> and then adds it using `i2c_add_adapter` method. This adapter can then be
>> passed to say mikroBUS driver where it can be used as a normal i2c_adapter,
>> and we can register the device to it.
>
> Being virtual does not really stop it being added to the DT.
>
> I'm making this all up, but i assume it will look something like this:
>
> greybus@42 {
> compatible = "acme,greybus";
> reg = <0x42 0x100>;
>
> This would represent the greybus host controller.
>
> module@0 {
> reg = <0>;
>
> This would represent a module discovered on the bus. I assume there is
> some sort of addressing? The greybus core code dynamically creates the
> node in DT to describe the modules it has discovered. This is not too
> different to USB. You can already describe USB devices in DT, but the
> assumption is you know they exists, e.g. because they are hard wired,
> not hot-plugable. The USB core will associate the USB device with the
> node in DT. But actually creating a node in DT is not too big a jump.
>
> interface@0 {
> compatible = "greybus,i2c";
> reg = <0>;
> }
> interface@1 {
> compatible = "greybus,spi";
> reg = <1>;
> }
> interface@10 {
> compatible = "greybus,gpio";
> reg = <10>;
> }
>
> It can then enumerate the interfaces on the module, and create the I2C
> node, SPI bus node, the gpio controller etc. Again, the greybus core
> can add nodes to DT to described the discovered hardware, and
> associate them to the linux devices which are created.
>

This proposal looks great and would be the ideal solution, but we met
with few challenges when initially trying to implement something like
this and had to drop and take the route with minimal development effort
to just instantiate mikroBUS devices.

From what we understand, you are recommending to change the manifest
description format used by greybus to device tree and also add of_bus
support for greybus - now this will not only solve instantiating
mikrobus devices on greybus but even complex devices on greybus making
it a robust solution and using standard tools and support DT offers.

However we have a few doubts:
* For USB or PCIe, to add OF device tree support the parent devices are
physically present, for example USB device is a child node of USB
controller (physically description available in a SoC DT) and USB
interfaces are child of USB devices, how would that hierarchy look for
greybus devices?
Would it be
USB/UART/transport controller -> AP Bridge host controller -> Module ->
interface -> bundle -> CPort ?

When this mikrobus driver was initially implemented we could not think
of such an approach as the SVC and Control functionality were
implemented in userspace with gbridge (
https://github.com/anobli/gbridge ) with a netlink interface to kernel
greybus, but today there are references to do it completely in kernel (
drivers/greybus/gb-beagleplay.c) and your proposal is implementable.

Also with this the manifesto tool which is not very well maintained is
not necessary : https://github.com/projectara/manifesto

> That gives you what you need to load a DT overlay to make use of these
> devices. That overlay would contain one of your virtual mikroBUS
> controllers. This virtual controller is basically a phandle-proxy. The
> virtual mikroBUS controllers is a consumer of phandles to an I2C bus,
> an SPI bus, GPIO bus which makes up the pins routed to the mikroBUS
> connector. The virtual mikroBUS controllers is also a provider of an
> I2C bus, an SPI bus, GPIO controller. The mikroBUS device consumes
> these I2C bus, SPI bus etc. The virtual mikroBUS controllers makes it
> simpler for the device to find the resources it needs, since they are
> all in one place. For a physical mikroBUS you have a DT node with
> phandles to the physical devices. For greybus you create a virtual
> device with phandles to the virtual devices added to the DT bus.
>
> You then have everything you need to describe the mikroBUS
> devices. For very simple devices you convert the manifest to a DT
> overlay and load it. For complex devices you directly use a DT
> overlay.
>
> I also don't see any need to do the manifest to DT overlay conversion
> on the fly. You have a database of manifests. They could be converted
> to DT and then added to the linux-firmware repo, for example. If
> device with an unknown manifest is found,

How do we know if we found a device with unknown manifest if we don't
read the EEPROM?

it should be possible to
> read the manifest in userspace via its eeprom in /sys/class/. An tool
> could create DT blob and add it to /lib/firmware to get it working
> locally, and provide suggestions how to contribute it to the linux
> firmware project?

Agreed, but on what basis will you load the particular manifest for a
add-on board if you are not reading the DT overlay (or manifest blob)
from the EEPROM?

Thanks and Regards,
Vaishnav

>
> Andrew

2024-03-21 09:44:49

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Hi,

> > Is that because the current software support is too limited? Are there
> > manufactures who want to create more complex designed, but are limited
> > by what can be described in the manifest?
> >
>
> most mikroBUS add-on boards in production lies in the category of
> sensors, displays, connectivity, mixed signal (ADC/DAC .etc) and if you
> look at the existing bindings under bindings/iio/ , most devices need
> only simple descriptions and the properties are mainly standard bus
> properties (SPI/I2C properties), IRQ, named-gpios, named properties,
> regulators, clocks the extension to manifest was made taking this into
> account and the named property description interface just maps the
> manifest entries to the unified device property interface under
> include/linux/property.h

How will the ethernet boards ([1], [2]) work? Where do they get
their MAC address from, for example. The DT has some nice properties
for that, but I doubt that will be possible with the manifest files.
I've looked at the manifest file for the w5500 board [3] and to me
it looks like that board will come up with a random MAC address on
each start. Thus, even today, you have some boards which require
a more complex description.

Apart from the discussion whether the manifest is a suitable or
sufficient mechanism to describe the hardware, I think the main
problem with the proposed binding, is that it doesn't follow the
binding Rob was proposing for a socket. If I want to use DT
overlays, how would you describe an add-on board?

The proposal was that the base board has something like

mikrobus: socket {
compatible = "mikrobus-socket";
i2c-parent = <&i2c0>;
spi-parent = <&spi0>;

i2c {};
spi {};
};

an add-on board can then have a DT snippet/overlay like the
following:

&mikrobus {
i2c {
eeprom@52: {
reg = <52>;
compatible = <atmel,at24..>;
}
};

spi {
sensor@0: {
reg = <0>;
compatible = <foobar>;
};
};
};

That should be possible with a binding for the mikrobus, which
in fact it is just a pin header with a standard pinout. Also as
Russell pointed out in v3, the EEPROM/manifest is not part of the
mikrobus standard. So maybe that deserves an own compatible, like

compatible = "mikroe,click", "mikrobus-socket";

Or maybe click-eeprom? Although click seems to be the brand name of
MikroElektronika.

-michael

[1] https://www.mikroe.com/eth-3-click
[2] https://www.mikroe.com/eth-wiz-click
[3] https://github.com/MikroElektronika/click_id/blob/main/manifests/ETH-WIZ-CLICK.mnfs

2024-03-21 11:56:09

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 21/03/24 15:08, Michael Walle wrote:
> Hi,
>
>>> Is that because the current software support is too limited? Are there
>>> manufactures who want to create more complex designed, but are limited
>>> by what can be described in the manifest?
>>>
>>
>> most mikroBUS add-on boards in production lies in the category of
>> sensors, displays, connectivity, mixed signal (ADC/DAC .etc) and if you
>> look at the existing bindings under bindings/iio/ , most devices need
>> only simple descriptions and the properties are mainly standard bus
>> properties (SPI/I2C properties), IRQ, named-gpios, named properties,
>> regulators, clocks the extension to manifest was made taking this into
>> account and the named property description interface just maps the
>> manifest entries to the unified device property interface under
>> include/linux/property.h
>
> How will the ethernet boards ([1], [2]) work? Where do they get
> their MAC address from, for example. The DT has some nice properties
> for that, but I doubt that will be possible with the manifest files.
> I've looked at the manifest file for the w5500 board [3] and to me
> it looks like that board will come up with a random MAC address on
> each start. Thus, even today, you have some boards which require
> a more complex description.
>

Agreed, this is a limitation, unless the corresponding
drivers/subsystems use device_property_read_* helper to fetch
properties, it will not work and net/core/of_net.c only implements
of_get_* helpers even though the underlying functions can be implemented
with equivalent device_property_read_* equivalent as well.

> Apart from the discussion whether the manifest is a suitable or
> sufficient mechanism to describe the hardware, I think the main
> problem with the proposed binding, is that it doesn't follow the
> binding Rob was proposing for a socket. If I want to use DT
> overlays, how would you describe an add-on board?
>
> The proposal was that the base board has something like
>
> mikrobus: socket {
> compatible = "mikrobus-socket";
> i2c-parent = <&i2c0>;
> spi-parent = <&spi0>;
>
> i2c {};
> spi {};
> };
>
> an add-on board can then have a DT snippet/overlay like the
> following:
>
> &mikrobus {
> i2c {
> eeprom@52: {
> reg = <52>;
> compatible = <atmel,at24..>;
> }
> };
>
> spi {
> sensor@0: {
> reg = <0>;
> compatible = <foobar>;
> };
> };
> };
>
> That should be possible with a binding for the mikrobus, which
> in fact it is just a pin header with a standard pinout. Also as
> Russell pointed out in v3, the EEPROM/manifest is not part of the
> mikrobus standard. So maybe that deserves an own compatible, like
>
> compatible = "mikroe,click", "mikrobus-socket";
>
> Or maybe click-eeprom? Although click seems to be the brand name of
> MikroElektronika.

Agreed, there is nothing preventing us to convert the binding and update
the driver to follow the above proposed format and will be done in next
revision. Click is brand name of MikroElektronika and they don't allow
custom boards to use that branding, however clickid can be used in the
case where EEPROM is present/enable the socket to be probeable.

Thanks and Regards,
Vaishnav

>
> -michael
>
> [1] https://www.mikroe.com/eth-3-click
> [2] https://www.mikroe.com/eth-wiz-click
> [3] https://github.com/MikroElektronika/click_id/blob/main/manifests/ETH-WIZ-CLICK.mnfs

2024-03-21 12:32:02

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On Thu, Mar 21, 2024 at 01:05:14PM +0530, Vaishnav Achath wrote:
> Hi Andrew,
>
> On 21/03/24 00:14, Andrew Lunn wrote:
> > On Wed, Mar 20, 2024 at 10:09:05PM +0530, Ayush Singh wrote:
> > > On 3/20/24 01:02, Andrew Lunn wrote:
> > >
> > > > > Yes, after discussion with Vaishnav and trying to brainstorm some way to do
> > > > > the same thing with dt overlays, it seems that trying to use dt overlays
> > > > > will mean need to have completely separate implementation of mikroBUS for
> > > > > local ports and mikroBUS over greybus.
> > > > Could you explain why please?
> > > >
> > > > Are greybus I2C bus masters different from physical I2C bus masters?
> > > > Are greybus SPI bus masters different from physical SPI bus masters?
> > >
> > > Well, they are virtual, so they are not declared in the device tree. I have
> > > linked the greybus i2c implementation. It basically allocates an i2c_adpater
> > > and then adds it using `i2c_add_adapter` method. This adapter can then be
> > > passed to say mikroBUS driver where it can be used as a normal i2c_adapter,
> > > and we can register the device to it.
> >
> > Being virtual does not really stop it being added to the DT.
> >
> > I'm making this all up, but i assume it will look something like this:
> >
> > greybus@42 {
> > compatible = "acme,greybus";
> > reg = <0x42 0x100>;
> >
> > This would represent the greybus host controller.
> >
> > module@0 {
> > reg = <0>;
> >
> > This would represent a module discovered on the bus. I assume there is
> > some sort of addressing? The greybus core code dynamically creates the
> > node in DT to describe the modules it has discovered. This is not too
> > different to USB. You can already describe USB devices in DT, but the
> > assumption is you know they exists, e.g. because they are hard wired,
> > not hot-plugable. The USB core will associate the USB device with the
> > node in DT. But actually creating a node in DT is not too big a jump.
> >
> > interface@0 {
> > compatible = "greybus,i2c";
> > reg = <0>;
> > }
> > interface@1 {
> > compatible = "greybus,spi";
> > reg = <1>;
> > }
> > interface@10 {
> > compatible = "greybus,gpio";
> > reg = <10>;
> > }
> >
> > It can then enumerate the interfaces on the module, and create the I2C
> > node, SPI bus node, the gpio controller etc. Again, the greybus core
> > can add nodes to DT to described the discovered hardware, and
> > associate them to the linux devices which are created.
> >
>
> This proposal looks great and would be the ideal solution, but we met with
> few challenges when initially trying to implement something like this and
> had to drop and take the route with minimal development effort to just
> instantiate mikroBUS devices.
>
> From what we understand, you are recommending to change the manifest
> description format used by greybus to device tree and also add of_bus
> support for greybus - now this will not only solve instantiating mikrobus
> devices on greybus but even complex devices on greybus making it a robust
> solution and using standard tools and support DT offers.

I would not discard the manifest. It exists, it appears to be used by
a lot of devices. So use it. But consider it an 'intermediate
representation'. Take it and transform it to DT.

Looking at:

https://libstock.mikroe.com/projects/view/5435/clickid

there appears to be a name, and hardware revision in the
manifest. Convert that to a string. Run a checksum over the rest of
the manifest, and append that to the string. You can then look in
/lib/firmware for a DT representation which matches.

> However we have a few doubts:
> * For USB or PCIe, to add OF device tree support the parent devices are
> physically present, for example USB device is a child node of USB controller
> (physically description available in a SoC DT) and USB interfaces are child
> of USB devices, how would that hierarchy look for greybus devices?

So DT support for USB, serial and PCIe devices already exists. When
the core enumerates a PCIe or USB bus, it looks in the DT blob for the
vendor:product ID, and associates any node it finds with the
device. It is not used very often, but if you search the .dts files,
you can find examples.

> Would it be
> USB/UART/transport controller -> AP Bridge host controller -> Module ->
> interface -> bundle -> CPort ?

That is for you to decide. I don't know the architecture well enough.

It is rather old, but:

https://lwn.net/Articles/715955/

There is a diagram of the sysfs tree, which looks pretty similar to
what you say above. What is however missing from the diagram is the
Linux devices themselves. Where do the I2C bus, the SPI bus, the GPIO
controller etc appear in the tree?

Maybe look at PCI and USB. Does the device tree representation include
all the bridges and hubs etc. Or does it simply the representation?

You need something to represent the controller. You need modules. Do
you need the details of interface & bundle and cport? Could you
represent them as an address tuple, and just have the devices under
module?

> When this mikrobus driver was initially implemented we could not think of
> such an approach as the SVC and Control functionality were implemented in
> userspace with gbridge ( https://github.com/anobli/gbridge ) with a netlink
> interface to kernel greybus, but today there are references to do it
> completely in kernel ( drivers/greybus/gb-beagleplay.c) and your proposal is
> implementable.

Does gb-beagleplay.c work together with the code in staging? It looks
like the GPIO controller, I2C controller, SPI controller, etc are
still in GregKH "Tree of crap". I don't think it is wise to build on
top of something in staging. So you probably need to spend time to
cleanup that code and moving it into the mainline proper. As you do
that, you could add the code needed to dynamically add nodes to device
tree.

Andrew

2024-03-21 12:44:44

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

Hi,

> >>> Is that because the current software support is too limited? Are there
> >>> manufactures who want to create more complex designed, but are limited
> >>> by what can be described in the manifest?
> >>>
> >>
> >> most mikroBUS add-on boards in production lies in the category of
> >> sensors, displays, connectivity, mixed signal (ADC/DAC .etc) and if you
> >> look at the existing bindings under bindings/iio/ , most devices need
> >> only simple descriptions and the properties are mainly standard bus
> >> properties (SPI/I2C properties), IRQ, named-gpios, named properties,
> >> regulators, clocks the extension to manifest was made taking this into
> >> account and the named property description interface just maps the
> >> manifest entries to the unified device property interface under
> >> include/linux/property.h
> >
> > How will the ethernet boards ([1], [2]) work? Where do they get
> > their MAC address from, for example. The DT has some nice properties
> > for that, but I doubt that will be possible with the manifest files.
> > I've looked at the manifest file for the w5500 board [3] and to me
> > it looks like that board will come up with a random MAC address on
> > each start. Thus, even today, you have some boards which require
> > a more complex description.
> >
>
> Agreed, this is a limitation, unless the corresponding
> drivers/subsystems use device_property_read_* helper to fetch
> properties, it will not work and net/core/of_net.c only implements
> of_get_* helpers even though the underlying functions can be implemented
> with equivalent device_property_read_* equivalent as well.

And I don't think this is a good idea given that the alternative is
just working.

> > Apart from the discussion whether the manifest is a suitable or
> > sufficient mechanism to describe the hardware, I think the main
> > problem with the proposed binding, is that it doesn't follow the
> > binding Rob was proposing for a socket. If I want to use DT
> > overlays, how would you describe an add-on board?
> >
> > The proposal was that the base board has something like
> >
> > mikrobus: socket {
> > compatible = "mikrobus-socket";
> > i2c-parent = <&i2c0>;
> > spi-parent = <&spi0>;
> >
> > i2c {};
> > spi {};
> > };
> >
> > an add-on board can then have a DT snippet/overlay like the
> > following:
> >
> > &mikrobus {
> > i2c {
> > eeprom@52: {
> > reg = <52>;
> > compatible = <atmel,at24..>;
> > }
> > };
> >
> > spi {
> > sensor@0: {
> > reg = <0>;
> > compatible = <foobar>;
> > };
> > };
> > };
> >
> > That should be possible with a binding for the mikrobus, which
> > in fact it is just a pin header with a standard pinout. Also as
> > Russell pointed out in v3, the EEPROM/manifest is not part of the
> > mikrobus standard. So maybe that deserves an own compatible, like
> >
> > compatible = "mikroe,click", "mikrobus-socket";
> >
> > Or maybe click-eeprom? Although click seems to be the brand name of
> > MikroElektronika.
>
> Agreed, there is nothing preventing us to convert the binding and update
> the driver to follow the above proposed format and will be done in next
> revision. Click is brand name of MikroElektronika and they don't allow
> custom boards to use that branding, however clickid can be used in the
> case where EEPROM is present/enable the socket to be probeable.

Thinking about this again. I'm not sure this additional compatible
really helps the discovery. It's a chicken egg problem. Only the
modules knows if it has an EEPROM, but then, we already have to
know it's in the socket. So while it might help for a static
configuration, it does not for the discovery.

-michael

> > [1] https://www.mikroe.com/eth-3-click
> > [2] https://www.mikroe.com/eth-wiz-click
> > [3] https://github.com/MikroElektronika/click_id/blob/main/manifests/ETH-WIZ-CLICK.mnfs

2024-03-21 12:56:33

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

> > How will the ethernet boards ([1], [2]) work? Where do they get
> > their MAC address from, for example. The DT has some nice properties
> > for that, but I doubt that will be possible with the manifest files.
> > I've looked at the manifest file for the w5500 board [3] and to me
> > it looks like that board will come up with a random MAC address on
> > each start. Thus, even today, you have some boards which require
> > a more complex description.
> >
>
> Agreed, this is a limitation, unless the corresponding drivers/subsystems
> use device_property_read_* helper to fetch properties, it will not work and
> net/core/of_net.c only implements of_get_* helpers even though the
> underlying functions can be implemented with equivalent
> device_property_read_* equivalent as well.

I think this is a good example of the limitations. For an Ethernet
NIC, you often want to describe properties of both the MAC and the
PHY. What phy-mode should be used, what delays on the RGMII bus, what
LEDs are there etc. This is a pretty much solved problem with DT, we
have a well defined sub tree to represent the MAC, the MDIO bus and
the PHY on the bus.

But we do have two classes of properties here. The MAC address is
unique to a board. So that does need to be stored in the EEPROM, and
cannot be in a one time converted manifest to DT file stored in
/lib/firmware. However, to some extent, this is a solved problem. We
have a DT representation of how to look in an EEPROM to find the MAC
address. So the DT just needs to point to some bytes in the manifest
in the EEPROM.

Andrew

2024-03-22 18:29:36

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: misc: Add mikrobus-connector

On 3/18/24 01:07, Ayush Singh wrote:

> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
> developed by MikroElektronika for connecting add-on boards to
> microcontrollers or microprocessors.
>
> mikroBUS is a connector and does not have a controller. Instead the
> software is responsible for identification of board and setting up /
> registering uart, spi, i2c, pwm and other buses. Thus it needs a way to
> get uart, spi, i2c, pwm and gpio controllers / adapters.
>
> A mikroBUS addon board is free to leave some of the pins unused which
> are marked as NC or Not Connected.
>
> Some of the pins might need to be configured as GPIOs deviating from their
> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.
>
> For some add-on boards the driver may not take care of some additional
> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
> (RST pin on the mikrobus port) needs to be pulled high.
>
> Here's the list of pins in mikroBUS connector:
> Analog - AN
> Reset - RST
> SPI Chip Select - CS
> SPI Clock - SCK
> SPI Master Input Slave Output - MISO
> SPI Master Output Slave Input - MOSI
> VCC-3.3V power - +3.3V
> Reference Ground - GND
> PWM - PWM output
> INT - Hardware Interrupt
> RX - UART Receive
> TX - UART Transmit
> SCL - I2C Clock
> SDA - I2C Data
> +5V - VCC-5V power
> GND - Reference Ground
>
> Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains
> a manifest to describe the addon board to provide plug and play
> capabilities.
>
> Link: https://www.mikroe.com/mikrobus
> Link:
> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
> mikroBUS specification
> Link: https://www.mikroe.com/sht1x-click SHT15 Click
> Link: https://www.mikroe.com/eth-click ENC28J60 Click
> Link: https://www.mikroe.com/clickid ClickID
>
> Co-developed-by: Vaishnav M A <[email protected]>
> Signed-off-by: Vaishnav M A <[email protected]>
> Signed-off-by: Ayush Singh <[email protected]>
> ---
> .../connector/mikrobus-connector.yaml | 113 ++++++++++++++++++
> MAINTAINERS | 6 +
> 2 files changed, 119 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
>
> diff --git a/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
> new file mode 100644
> index 000000000000..ee3736add41c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
> @@ -0,0 +1,113 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/connector/mikrobus-connector.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: mikroBUS add-on board socket
> +
> +maintainers:
> + - Ayush Singh <[email protected]>
> +
> +properties:
> + compatible:
> + const: mikrobus-connector
> +
> + pinctrl-0: true
> + pinctrl-1: true
> + pinctrl-2: true
> + pinctrl-3: true
> + pinctrl-4: true
> + pinctrl-5: true
> + pinctrl-6: true
> + pinctrl-7: true
> + pinctrl-8: true
> +
> + pinctrl-names:
> + items:
> + - const: default
> + - const: pwm_default
> + - const: pwm_gpio
> + - const: uart_default
> + - const: uart_gpio
> + - const: i2c_default
> + - const: i2c_gpio
> + - const: spi_default
> + - const: spi_gpio
> +
> + mikrobus-gpios:
> + minItems: 11
> + maxItems: 12
> +
> + i2c-adapter:
> + description: i2c adapter attached to the mikrobus socket.
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + spi-controller:
> + description: spi bus number of the spi-master attached to the mikrobus socket.
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + uart:
> + description: uart port attached to the mikrobus socket
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + pwms:
> + description: the pwm-controller corresponding to the mikroBUS PWM pin.
> + maxItems: 1
> +
> + spi-cs:
> + description: spi chip-select numbers corresponding to the chip-selects on the mikrobus socket.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - description: chip select corresponding to CS pin
> + - description: chip select corresponding to RST pin
> +
> +required:
> + - compatible
> + - pinctrl-0
> + - pinctrl-1
> + - pinctrl-2
> + - pinctrl-3
> + - pinctrl-4
> + - pinctrl-5
> + - pinctrl-6
> + - pinctrl-7
> + - pinctrl-8
> + - i2c-adapter
> + - spi-controller
> + - spi-cs
> + - uart
> + - pwms
> + - mikrobus-gpios
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + mikrobus {
> + compatible = "mikrobus-connector";
> + pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default",
> + "i2c_gpio", "spi_default", "spi_gpio";
> + pinctrl-0 = <&P2_03_gpio_input_pin &P1_04_gpio_pin &P1_02_gpio_pin>;
> + pinctrl-1 = <&P2_01_pwm_pin>;
> + pinctrl-2 = <&P2_01_gpio_pin>;
> + pinctrl-3 = <&P2_05_uart_pin &P2_07_uart_pin>;
> + pinctrl-4 = <&P2_05_gpio_pin &P2_07_gpio_pin>;
> + pinctrl-5 = <&P2_09_i2c_pin &P2_11_i2c_pin>;
> + pinctrl-6 = <&P2_09_gpio_pin &P2_11_gpio_pin>;
> + pinctrl-7 = <&P1_12_spi_pin &P1_10_spi_pin &P1_08_spi_sclk_pin &P1_06_spi_cs_pin>;
> + pinctrl-8 = <&P1_12_gpio_pin &P1_10_gpio_pin &P1_08_gpio_pin &P1_06_gpio_pin>;
> + pwms = <&ehrpwm1 0 500000 0>;
> + i2c-adapter = <&i2c1>;
> + spi-controller = <&spi1>;
> + spi-cs = <0 1>;
> + uart = <&uart1>;
> + mikrobus-gpios = <&gpio1 18 GPIO_ACTIVE_HIGH>, <&gpio0 23 GPIO_ACTIVE_HIGH>,
> + <&gpio0 30 GPIO_ACTIVE_HIGH>, <&gpio0 31 GPIO_ACTIVE_HIGH>,
> + <&gpio0 15 GPIO_ACTIVE_HIGH>, <&gpio0 14 GPIO_ACTIVE_HIGH>,
> + <&gpio0 4 GPIO_ACTIVE_HIGH>, <&gpio0 3 GPIO_ACTIVE_HIGH>,
> + <&gpio0 2 GPIO_ACTIVE_HIGH>, <&gpio0 5 GPIO_ACTIVE_HIGH>,
> + <&gpio2 25 GPIO_ACTIVE_HIGH>, <&gpio2 3 GPIO_ACTIVE_HIGH>;
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 375d34363777..69418a058c6b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14767,6 +14767,12 @@ M: Oliver Neukum <[email protected]>
> S: Maintained
> F: drivers/usb/image/microtek.*
>
> +MIKROBUS
> +M: Ayush Singh <[email protected]>
> +M: Vaishnav M A <[email protected]>
> +S: Maintained
> +F: Documentation/devicetree/bindings/misc/mikrobus-connector.yaml
> +
> MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
> M: Luka Kovacic <[email protected]>
> M: Luka Perkov <[email protected]>

After going through all the discussions here, I have a few questions:

1. Is the old `*_register_device(controller, board_info)` style
discouraged in favor of using device tree, at least for drivers using
multiple fundamental buses (i2c, spi, etc)? Or is the problem just that
these bindings do not leave open the possibility of using device tree
overlays? Will it be fine if the dt bindings allow for dt overlays, but
the driver still uses imperative registering of board?

2. Is the preferred way to handle virtual devices (like those created by
greybus subsystem) now device tree? Is that one of the blockers for
greybus i2c, spi etc to still be in staging?

3. How are virtual devices created in device tree? If I register an i2c
adapter using `i2c_add_adapter`, is the device tree entry is dynamically
created, which can then be used by a device tree overlay?

Ayush Singh

2024-03-22 18:51:49