linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] RFC : mikroBUS driver for add-on boards
@ 2020-07-24 12:06 Vaishnav M A
  2020-07-24 20:02 ` Christopher Friedt
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Vaishnav M A @ 2020-07-24 12:06 UTC (permalink / raw)
  To: greybus-dev, linux-kernel, gregkh, arnd, johan, elder
  Cc: robh, mchehab+huawei, davem, jkridner, drew, robertcnelson,
	rajkovic, chrisfriedt, zoran.stojsavljevic, vaishnav

Attached is a patch for the mikroBUS driver which helps to
instantiate an add-on board device on a mikrobus port by fetching
the device identifier manifest binary from an EEPROM on-board
the device. mikroBUS is an add-on board socket standard by
MikroElektronika that can be freely used by anyone
following the guidelines, more details and discussions on
the status of the driver can be found here in this eLinux wiki:
https://elinux.org/Mikrobus

In the current state of the driver, more than 80 different
add-on boards have been tested on the BeagleBoard.org
PocketBeagle and the manifest binary is generated using the same
manifesto tool used to generate Greybus manifest binaries,
The pull request to manifesto to add new descriptors specific
to mikrobus is here : https://github.com/projectara/manifesto/pull/2
The utilization of Greybus manifest binaries here is not entirely
coincidental, We are evaluating ways to add mikroBUS sockets and
devices via Greybus expansion.

The mikroBUS standard includes SPI, I2C, UART, PWM, ADC, GPIO
and power (3.3V and 5V) connections to interface common embedded
peripherals, There are more than 750 add-on boards ranging from
wireless connectivity boards to human-machine interface sensors
which conform to the mikroBUS standard, out of which more than 140
boards already have support in the Linux kernel.Today, there is no
mainlinesolution for enabling mikroBUS add-on boards at run-time, the
most straight forward method for loading drivers is to provide
device-tree overlay fragments at boot time, this method suffers
from the need to maintain a large out-of-tree database for which there
is need to maintain an overlay for every mikroBUS add-on board for every
Linux system and for every mikroBUS socket on that system.

The mikroBUS driver tries to solve the problem by using extended version
of the greybus manifest to describe the add-on board device specific
information required by the device driver and uses it along with the fixed
port specific information to probe the specific device driver.The manifest
binary is now fetched from an I2C EEPROM over the I2C bus on the mikroBUS
port(subject to change in mikroBUS v3 specification) and enumerate drivers
for the add-on devices.There is also ongoing work to define a mikroBUS
v3 standard in which the addon board includes a non-volatile storage to
store the device identifier manifest binary, once the mikroBUS v3 standard
is released, the mikroBUS can be seen as a probeable bus such that the
kernel can discover the device on the bus at boot time.

The driver also has a few debug SysFS interfaces for testing on add-on
boards without an EEPROM, these can be used in the following manner:
(example for mikroBUS port 1 on BeagleBoard.org PocketBeagle):

printf "%b" '\x01\x00\x00\x59\x32\x17' > /sys/bus/mikrobus/add_port

The bytes in the byte array sequence are (in order):

	* i2c_adap_nr
	* spi_master_nr
	* serdev_ctlr_nr
	* rst_gpio_nr
	* pwm_gpio_nr
	* int_gpio_nr
* add_port sysFS entry is purely for debug and in the actual state
of the driver, port configuration will be loaded from a suitable
device tree overlay fragment.

echo 0 > /sys/bus/mikrobus/del_port (to delete the attached port)
echo 1 >  /sys/class/mikrobus-port/mikrobus-0/rescan (to rescan the EEPROM
contents on the I2C bus on the mikroBUS port).

cat board_manifest.mnfb >  /sys/class/mikrobus-port/mikrobus-0/new_device
* debug interface to pass the manifest binary in case an EEPROM is absent
echo 0 >  /sys/class/mikrobus-port/mikrobus-0/delete_device
* to unload the loaded board on the mikrobus port

These sysFS interfaces are only implemented for debug purposes and
in the actual implementation of the driver these will be removed and
the manifest binary will be fetched from the non volatile storage on-board
the device.

The mikroBUS driver enable the mikroBUS to be a probeable bus such that
the kernel can discover the device and automatically load the drivers.
There are already several Linux platforms with mikroBUS sockets and the
mikroBUS driver helps to reduce the time to develop and debug
support for various mikroBUS add-on boards. Further, it opens up
the possibility for support under dynamically instantiated buses
such as with Greybus.

Please let know the feedback you have on this patch or the approach used.

Thanks,

Vaishnav M A

Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org>
---
 MAINTAINERS                               |   6 +
 drivers/misc/Kconfig                      |   1 +
 drivers/misc/Makefile                     |   1 +
 drivers/misc/mikrobus/Kconfig             |  16 +
 drivers/misc/mikrobus/Makefile            |   5 +
 drivers/misc/mikrobus/mikrobus_core.c     | 649 ++++++++++++++++++++++
 drivers/misc/mikrobus/mikrobus_core.h     | 130 +++++
 drivers/misc/mikrobus/mikrobus_manifest.c | 390 +++++++++++++
 drivers/misc/mikrobus/mikrobus_manifest.h |  21 +
 include/linux/greybus/greybus_manifest.h  |  53 ++
 10 files changed, 1272 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 d53db30d1365..9a049746203f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11402,6 +11402,12 @@ M:	Oliver Neukum <oliver@neukum.org>
 S:	Maintained
 F:	drivers/usb/image/microtek.*
 
+MIKROBUS ADDON BOARD DRIVER
+M:	Vaishnav M A <vaishnav@beagleboard.org>
+S:	Maintained
+W:	https://elinux.org/Mikrobus
+F:	drivers/misc/mikrobus/
+
 MIPS
 M:	Thomas Bogendoerfer <tsbogend@alpha.franken.de>
 L:	linux-mips@vger.kernel.org
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index e1b1ba5e2b92..334f0c39d56b 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -472,4 +472,5 @@ source "drivers/misc/ocxl/Kconfig"
 source "drivers/misc/cardreader/Kconfig"
 source "drivers/misc/habanalabs/Kconfig"
 source "drivers/misc/uacce/Kconfig"
+source "drivers/misc/mikrobus/Kconfig"
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c7bd01ac6291..45486dd77da5 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_VMWARE_BALLOON)	+= vmw_balloon.o
 obj-$(CONFIG_PCH_PHUB)		+= pch_phub.o
 obj-y				+= ti-st/
 obj-y				+= lis3lv02d/
+obj-y				+= mikrobus/
 obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
 obj-$(CONFIG_INTEL_MEI)		+= mei/
 obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig
new file mode 100644
index 000000000000..c3b93e12daad
--- /dev/null
+++ b/drivers/misc/mikrobus/Kconfig
@@ -0,0 +1,16 @@
+menuconfig MIKROBUS
+	tristate "Module for instantiating devices on mikroBUS ports"
+	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 helps in
+	  instantiating devices on the mikroBUS port with identifier
+	  data fetched from an EEPROM on the device, more details on
+	  the mikroBUS driver support and discussion can be found in
+	  this eLinux wiki : elinux.org/Mikrobus
+
+
+	  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..1f80ed4064d8
--- /dev/null
+++ b/drivers/misc/mikrobus/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+# mikroBUS Core
+
+mikrobus-y :=	mikrobus_core.o	mikrobus_manifest.o
+obj-$(CONFIG_MIKROBUS) += mikrobus.o
\ No newline at end of file
diff --git a/drivers/misc/mikrobus/mikrobus_core.c b/drivers/misc/mikrobus/mikrobus_core.c
new file mode 100644
index 000000000000..78c96c637632
--- /dev/null
+++ b/drivers/misc/mikrobus/mikrobus_core.c
@@ -0,0 +1,649 @@
+// 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.
+ */
+
+#define pr_fmt(fmt) "mikrobus: " fmt
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/idr.h>
+#include <linux/init.h>
+#include <linux/jump_label.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/gpio/machine.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-provider.h>
+#include <linux/interrupt.h>
+#include <linux/spi/spi.h>
+#include <linux/serdev.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+#include "mikrobus_core.h"
+#include "mikrobus_manifest.h"
+
+#define ATMEL_24C32_I2C_ADDR 0x57
+
+static DEFINE_IDR(mikrobus_port_idr);
+static struct class_compat *mikrobus_port_compat_class;
+static bool is_registered;
+
+static ssize_t add_port_store(struct bus_type *bt, const char *buf,
+			      size_t count)
+{
+	struct mikrobus_port_config *cfg;
+
+	if (count < sizeof(*cfg)) {
+		pr_err("add_port: incorrect config data received: %s\n", buf);
+		return -EINVAL;
+	}
+	mikrobus_register_port_config((void *)buf);
+	return count;
+}
+BUS_ATTR_WO(add_port);
+
+static ssize_t del_port_store(struct bus_type *bt, const char *buf,
+							  size_t count)
+{
+	int id;
+	char end;
+	int res;
+
+	res = sscanf(buf, "%d%c", &id, &end);
+	if (res < 1) {
+		pr_err("delete_port: cannot parse mikrobus port ID\n");
+		return -EINVAL;
+	}
+	if (!idr_find(&mikrobus_port_idr, id)) {
+		pr_err("attempting to delete unregistered port [%d]\n", id);
+		return -EINVAL;
+	}
+	mikrobus_del_port(idr_find(&mikrobus_port_idr, id));
+	return count;
+}
+BUS_ATTR_WO(del_port);
+
+static struct attribute *mikrobus_attrs[] = {
+	&bus_attr_add_port.attr,
+	&bus_attr_del_port.attr,
+	 NULL
+};
+ATTRIBUTE_GROUPS(mikrobus);
+
+struct bus_type mikrobus_bus_type = {
+	.name = "mikrobus",
+	.bus_groups = mikrobus_groups,
+};
+EXPORT_SYMBOL_GPL(mikrobus_bus_type);
+
+static int mikrobus_port_scan_eeprom(struct mikrobus_port *port)
+{
+	char header[12];
+	struct addon_board_info *board;
+	int manifest_size;
+	int retval;
+	char *buf;
+
+	nvmem_device_read(port->eeprom, 0, 12, header);
+	manifest_size = mikrobus_manifest_header_validate(header, 12);
+	if (manifest_size > 0) {
+		buf = kzalloc(manifest_size, GFP_KERNEL);
+		nvmem_device_read(port->eeprom, 0, manifest_size, buf);
+		board = kzalloc(sizeof(*board), GFP_KERNEL);
+		if (!board)
+			return -ENOMEM;
+		INIT_LIST_HEAD(&board->manifest_descs);
+		INIT_LIST_HEAD(&board->devices);
+		retval = mikrobus_manifest_parse(board, (void *)buf, manifest_size);
+		if (!retval) {
+			pr_err("failed to parse manifest, size: %d", manifest_size);
+			return -EINVAL;
+		}
+		retval = mikrobus_register_board(port, board);
+		if (retval) {
+			pr_err("failed to register board: %s", board->name);
+			return -EINVAL;
+		}
+		kfree(buf);
+	} else {
+		pr_err("inavlide manifest port %d", port->id);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static ssize_t name_show(struct device *dev, struct device_attribute *attr,
+						 char *buf)
+{
+	return sprintf(buf, "%s\n", to_mikrobus_port(dev)->name);
+}
+static DEVICE_ATTR_RO(name);
+
+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 retval;
+
+	if (port->board == NULL) {
+		board = kzalloc(sizeof(*board), GFP_KERNEL);
+		if (!board)
+			return -EINVAL;
+		INIT_LIST_HEAD(&board->manifest_descs);
+		INIT_LIST_HEAD(&board->devices);
+	} else {
+		pr_err("port %d already has board registered", port->id);
+		return -EINVAL;
+	}
+	retval = mikrobus_manifest_parse(board, (void *)buf, count);
+	if (!retval) {
+		pr_err("failed to parse manifest");
+		return -EINVAL;
+	}
+	retval = mikrobus_register_board(port, board);
+	if (retval) {
+		pr_err("failed to register board: %s", board->name);
+		return -EINVAL;
+	}
+	return count;
+}
+static DEVICE_ATTR_WO(new_device);
+
+static ssize_t rescan_store(struct device *dev, struct device_attribute *attr,
+							const char *buf, size_t count)
+{
+	struct mikrobus_port *port = to_mikrobus_port(dev);
+	int id;
+	char end;
+	int res;
+	int retval;
+
+	res = sscanf(buf, "%d%c", &id, &end);
+	if (res < 1) {
+		pr_err("rescan: Can't parse trigger\n");
+		return -EINVAL;
+	}
+	if (port->board != NULL) {
+		pr_err("port %d already has board registered", port->id);
+		return -EINVAL;
+	}
+	retval = mikrobus_port_scan_eeprom(port);
+	if (retval) {
+		pr_err("port %d board register from manifest failed", port->id);
+		return -EINVAL;
+	}
+	return count;
+}
+static DEVICE_ATTR_WO(rescan);
+
+static ssize_t delete_device_store(struct device *dev, struct device_attribute *attr,
+							const char *buf, size_t count)
+{
+	int id;
+	char end;
+	int res;
+	struct mikrobus_port *port = to_mikrobus_port(dev);
+
+	res = sscanf(buf, "%d%c", &id, &end);
+	if (res < 1) {
+		pr_err("delete_board: Can't parse board ID\n");
+		return -EINVAL;
+	}
+	if (port->board == NULL) {
+		pr_err("delete_board: port does not have any boards registered\n");
+		return -EINVAL;
+	}
+	mikrobus_unregister_board(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_rescan.attr,
+	&dev_attr_delete_device.attr, &dev_attr_name.attr, NULL};
+ATTRIBUTE_GROUPS(mikrobus_port);
+
+static void mikrobus_dev_release(struct device *dev)
+{
+	struct mikrobus_port *port = to_mikrobus_port(dev);
+
+	idr_remove(&mikrobus_port_idr, port->id);
+	kfree(port);
+}
+
+struct device_type mikrobus_port_type = {
+	.groups = mikrobus_port_groups,
+	.release = mikrobus_dev_release,
+};
+EXPORT_SYMBOL_GPL(mikrobus_port_type);
+
+static int mikrobus_get_irq(struct mikrobus_port *port, int irqno,
+							int irq_type)
+{
+	int irq;
+
+	switch (irqno) {
+	case MIKROBUS_GPIO_INT:
+	irq = gpiod_to_irq(port->int_gpio);
+	break;
+	case MIKROBUS_GPIO_RST:
+	irq = gpiod_to_irq(port->rst_gpio);
+	break;
+	case MIKROBUS_GPIO_PWM:
+	irq = gpiod_to_irq(port->pwm_gpio);
+	break;
+	default:
+	return -EINVAL;
+	}
+	if (irq < 0) {
+		pr_err("Could not get irq for irq type: %d", irqno);
+		return -EINVAL;
+	}
+	irq_set_irq_type(irq, irq_type);
+	return irq;
+}
+
+static int mikrobus_setup_gpio(struct gpio_desc *gpio, int gpio_state)
+{
+	int retval;
+
+	if (gpio_state == MIKROBUS_GPIO_UNUSED)
+		return 0;
+	switch (gpio_state) {
+	case MIKROBUS_GPIO_INPUT:
+	retval = gpiod_direction_input(gpio);
+	break;
+	case MIKROBUS_GPIO_OUTPUT_HIGH:
+	retval = gpiod_direction_output(gpio, 1);
+	gpiod_set_value_cansleep(gpio, 1);
+	break;
+	case MIKROBUS_GPIO_OUTPUT_LOW:
+	retval = gpiod_direction_output(gpio, 0);
+	gpiod_set_value_cansleep(gpio, 0);
+	break;
+	default:
+	return -EINVAL;
+	}
+	return retval;
+}
+
+static void mikrobus_spi_device_delete(struct spi_master *master, unsigned int cs)
+{
+	struct device *dev;
+	char str[32];
+
+	snprintf(str, sizeof(str), "%s.%u", dev_name(&master->dev), cs);
+	dev = bus_find_device_by_name(&spi_bus_type, NULL, str);
+	if (dev != NULL) {
+		spi_unregister_device(to_spi_device(dev));
+		put_device(dev);
+	}
+}
+
+static char *mikrobus_get_gpio_chip_name(struct mikrobus_port *port, int gpio)
+{
+	char *name;
+	struct gpio_chip *gpiochip;
+
+	switch (gpio) {
+	case MIKROBUS_GPIO_INT:
+	gpiochip = gpiod_to_chip(port->int_gpio);
+	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
+	break;
+	case MIKROBUS_GPIO_RST:
+	gpiochip = gpiod_to_chip(port->rst_gpio);
+	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
+	break;
+	case MIKROBUS_GPIO_PWM:
+	gpiochip = gpiod_to_chip(port->pwm_gpio);
+	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
+	break;
+	}
+	return name;
+}
+
+static int mikrobus_get_gpio_hwnum(struct mikrobus_port *port, int gpio)
+{
+	int hwnum;
+	struct gpio_chip *gpiochip;
+
+	switch (gpio) {
+	case MIKROBUS_GPIO_INT:
+	gpiochip = gpiod_to_chip(port->int_gpio);
+	hwnum = desc_to_gpio(port->int_gpio) - gpiochip->base;
+	break;
+	case MIKROBUS_GPIO_RST:
+	gpiochip = gpiod_to_chip(port->rst_gpio);
+	hwnum = desc_to_gpio(port->rst_gpio) - gpiochip->base;
+	break;
+	case MIKROBUS_GPIO_PWM:
+	gpiochip = gpiod_to_chip(port->pwm_gpio);
+	hwnum = desc_to_gpio(port->pwm_gpio) - gpiochip->base;
+	break;
+	}
+	return hwnum;
+}
+
+static void release_mikrobus_device(struct board_device_info *dev)
+{
+	list_del(&dev->links);
+	kfree(dev);
+}
+
+static void release_board_devices(struct addon_board_info *info)
+{
+	struct board_device_info *dev;
+	struct board_device_info *next;
+
+	list_for_each_entry_safe(dev, next, &info->devices, links)
+		release_mikrobus_device(dev);
+}
+
+static int mikrobus_register_device(struct mikrobus_port *port,
+					struct board_device_info *dev, char *board_name)
+{
+	struct i2c_board_info *i2c;
+	struct spi_board_info *spi;
+	struct gpiod_lookup_table *lookup;
+	char devname[40];
+	int i;
+
+	pr_info(" registering device : %s\n", dev->drv_name);
+
+	if (dev->gpio_lookup != NULL) {
+		lookup = dev->gpio_lookup;
+		if (dev->protocol == MIKROBUS_PROTOCOL_SPI) {
+			snprintf(devname, sizeof(devname), "%s.%u",
+				dev_name(&port->spi_mstr->dev), dev->reg);
+			lookup->dev_id = kmemdup(devname, 40, GFP_KERNEL);
+		} else if (dev->protocol == MIKROBUS_PROTOCOL_I2C)
+			lookup->dev_id = dev->drv_name;
+		pr_info(" adding lookup table : %s\n", lookup->dev_id);
+		for (i = 0; i < dev->num_gpio_resources; i++) {
+			lookup->table[i].key =
+			mikrobus_get_gpio_chip_name(port, lookup->table[i].chip_hwnum);
+			lookup->table[i].chip_hwnum =
+			mikrobus_get_gpio_hwnum(port, lookup->table[i].chip_hwnum);
+			lookup->table[i].flags = GPIO_ACTIVE_HIGH;
+		}
+		gpiod_add_lookup_table(lookup);
+	}
+	switch (dev->protocol) {
+	case MIKROBUS_PROTOCOL_SPI:
+	spi = kzalloc(sizeof(*spi), GFP_KERNEL);
+	if (!spi)
+		return -ENOMEM;
+	strncpy(spi->modalias, dev->drv_name, sizeof(spi->modalias) - 1);
+	if (dev->irq)
+		spi->irq = mikrobus_get_irq(port, dev->irq, dev->irq_type);
+	if (dev->properties)
+		spi->properties = dev->properties;
+	spi->chip_select = dev->reg;
+	spi->max_speed_hz = dev->max_speed_hz;
+	spi->mode = dev->mode;
+	mikrobus_spi_device_delete(port->spi_mstr, dev->reg);
+	dev->dev_client = (void *)spi_new_device(port->spi_mstr, spi);
+	break;
+	case MIKROBUS_PROTOCOL_I2C:
+	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+	strncpy(i2c->type, dev->drv_name, sizeof(i2c->type) - 1);
+	if (dev->irq)
+		i2c->irq = mikrobus_get_irq(port, dev->irq, dev->irq_type);
+	if (dev->properties)
+		i2c->properties = dev->properties;
+	i2c->addr = dev->reg;
+	dev->dev_client = (void *)i2c_new_client_device(port->i2c_adap, i2c);
+	break;
+	case MIKROBUS_PROTOCOL_UART:
+	pr_info("SERDEV devices support not yet added");
+	break;
+	default:
+	return -EINVAL;
+	}
+	return 0;
+}
+
+static void mikrobus_unregister_device(struct mikrobus_port *port, struct board_device_info *dev,
+										char *board_name)
+{
+	pr_info(" removing device : %s\n", dev->drv_name);
+	if (dev->gpio_lookup != NULL) {
+		gpiod_remove_lookup_table(dev->gpio_lookup);
+		kfree(dev->gpio_lookup);
+	}
+	if (dev->properties != NULL)
+		kfree(dev->properties);
+	switch (dev->protocol) {
+	case MIKROBUS_PROTOCOL_SPI:
+	spi_unregister_device((struct spi_device *)dev->dev_client);
+	break;
+	case MIKROBUS_PROTOCOL_I2C:
+	i2c_unregister_device((struct i2c_client *)dev->dev_client);
+	break;
+	case MIKROBUS_PROTOCOL_UART:
+	pr_err("SERDEV devices support not yet added");
+	break;
+	}
+}
+
+int mikrobus_register_board(struct mikrobus_port *port,	struct addon_board_info *board)
+{
+	struct board_device_info *devinfo;
+	struct board_device_info *next;
+	int retval;
+
+	if (WARN_ON(list_empty(&board->devices)))
+		return false;
+
+	retval = mikrobus_setup_gpio(port->pwm_gpio, board->pwm_gpio_state);
+	if (retval) {
+		pr_err("mikrobus_setup_gpio : can't setup pwm gpio state: (%d)\n", retval);
+		return retval;
+	}
+	retval = mikrobus_setup_gpio(port->int_gpio, board->int_gpio_state);
+	if (retval) {
+		pr_err("mikrobus_setup_gpio : can't setup int gpio state: (%d)\n", retval);
+		return retval;
+	}
+	retval = mikrobus_setup_gpio(port->rst_gpio, board->rst_gpio_state);
+	if (retval) {
+		pr_err("mikrobus_setup_gpio : can't setup rst gpio state: (%d)\n", retval);
+		return retval;
+	}
+	list_for_each_entry_safe(devinfo, next, &board->devices, links)
+		mikrobus_register_device(port, devinfo, board->name);
+	port->board = board;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mikrobus_register_board);
+
+void mikrobus_unregister_board(struct mikrobus_port *port, struct addon_board_info *board)
+{
+	struct board_device_info *devinfo;
+	struct board_device_info *next;
+
+	if (WARN_ON(list_empty(&board->devices)))
+		return;
+	port->board = NULL;
+	list_for_each_entry_safe(devinfo, next, &board->devices, links)
+		mikrobus_unregister_device(port, devinfo, board->name);
+	release_board_devices(board);
+	kfree(board);
+	port->board = NULL;
+}
+EXPORT_SYMBOL_GPL(mikrobus_unregister_board);
+
+int mikrobus_register_port_config(struct mikrobus_port_config *cfg)
+{
+	struct mikrobus_port *port;
+	int retval;
+
+	if (WARN_ON(!is_registered))
+		return -EAGAIN;
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+	port->pwm_gpio = gpio_to_desc(cfg->pwm_gpio_nr);
+	port->int_gpio = gpio_to_desc(cfg->int_gpio_nr);
+	port->rst_gpio = gpio_to_desc(cfg->rst_gpio_nr);
+	port->spi_mstr = spi_busnum_to_master(cfg->spi_master_nr);
+	port->i2c_adap = i2c_get_adapter(cfg->i2c_adap_nr);
+	retval = mikrobus_register_port(port);
+	if (retval) {
+		pr_err("port : can't register port from config (%d)\n", retval);
+		return retval;
+	}
+	return retval;
+}
+EXPORT_SYMBOL_GPL(mikrobus_register_port_config);
+
+static struct i2c_board_info mikrobus_eeprom_info = {
+	I2C_BOARD_INFO("24c32", ATMEL_24C32_I2C_ADDR),
+};
+
+static int mikrobus_port_probe_eeprom(struct mikrobus_port *port)
+{
+	struct i2c_client *eeprom_client;
+	struct nvmem_device *eeprom;
+	char dev_name[40];
+
+	eeprom_client = i2c_new_client_device(port->i2c_adap, &mikrobus_eeprom_info);
+	if (!IS_ERR(eeprom_client)) {
+		pr_info(" mikrobus port %d default eeprom is probed at %02x\n", port->id,
+									eeprom_client->addr);
+		snprintf(dev_name, sizeof(dev_name), "%d-%04x0", port->i2c_adap->nr,
+				 eeprom_client->addr);
+		eeprom = nvmem_device_get(&eeprom_client->dev, dev_name);
+		if (IS_ERR(eeprom)) {
+			pr_err(" mikrobus port %d eeprom nvmem device probe failed\n", port->id);
+			i2c_unregister_device(eeprom_client);
+			port->eeprom = NULL;
+			return 0;
+		}
+	} else {
+		pr_info(" mikrobus port %d default eeprom probe failed\n", port->id);
+		return 0;
+	}
+	port->eeprom = eeprom;
+	port->eeprom_client = eeprom_client;
+	return 0;
+}
+
+int mikrobus_register_port(struct mikrobus_port *port)
+{
+	int retval;
+	int id;
+
+	if (WARN_ON(!is_registered))
+		return -EAGAIN;
+	id = idr_alloc(&mikrobus_port_idr, port, 0, 0, GFP_KERNEL);
+	if (id < 0)
+		return id;
+	port->id = id;
+	port->dev.bus = &mikrobus_bus_type;
+	port->dev.type = &mikrobus_port_type;
+	strncpy(port->name, "mikrobus-port", sizeof(port->name) - 1);
+	dev_set_name(&port->dev, "mikrobus-%d", port->id);
+	pr_info("registering port mikrobus-%d\n ", port->id);
+	retval = device_register(&port->dev);
+	if (retval) {
+		pr_err("port '%d': can't register device (%d)\n", port->id, retval);
+		put_device(&port->dev);
+		return retval;
+	}
+	retval = class_compat_create_link(mikrobus_port_compat_class, &port->dev,
+								port->dev.parent);
+	if (retval)
+		dev_warn(&port->dev, "failed to create compatibility class link\n");
+	if (!port->eeprom) {
+		pr_info("mikrobus port %d eeprom empty probing default eeprom\n", port->id);
+		retval = mikrobus_port_probe_eeprom(port);
+	}
+	if (port->eeprom) {
+		retval = mikrobus_port_scan_eeprom(port);
+		if (retval)
+			dev_warn(&port->dev, "failed to register board from manifest\n");
+	}
+	return retval;
+}
+EXPORT_SYMBOL_GPL(mikrobus_register_port);
+
+void mikrobus_del_port(struct mikrobus_port *port)
+{
+	struct mikrobus_port *found;
+
+	found = idr_find(&mikrobus_port_idr, port->id);
+	if (found != port) {
+		pr_err("attempting to delete unregistered port [%s]\n", port->name);
+		return;
+	}
+	if (port->board != NULL) {
+		pr_err("attempting to delete port with registered boards, port [%s]\n",
+									port->name);
+		return;
+	}
+
+	if (port->eeprom) {
+		nvmem_device_put(port->eeprom);
+		i2c_unregister_device(port->eeprom_client);
+	}
+
+	class_compat_remove_link(mikrobus_port_compat_class, &port->dev,
+							port->dev.parent);
+	device_unregister(&port->dev);
+	idr_remove(&mikrobus_port_idr, port->id);
+	memset(&port->dev, 0, sizeof(port->dev));
+}
+EXPORT_SYMBOL_GPL(mikrobus_del_port);
+
+static int __init mikrobus_init(void)
+{
+	int retval;
+
+	retval = bus_register(&mikrobus_bus_type);
+	if (retval) {
+		pr_err("bus_register failed (%d)\n", retval);
+		return retval;
+	}
+	mikrobus_port_compat_class = class_compat_register("mikrobus-port");
+	if (!mikrobus_port_compat_class) {
+		pr_err("class_compat register failed (%d)\n", retval);
+		retval = -ENOMEM;
+		goto class_err;
+	}
+	is_registered = true;
+	return 0;
+class_err:
+	bus_unregister(&mikrobus_bus_type);
+	idr_destroy(&mikrobus_port_idr);
+	is_registered = false;
+	return retval;
+}
+subsys_initcall(mikrobus_init);
+
+static void __exit mikrobus_exit(void)
+{
+	bus_unregister(&mikrobus_bus_type);
+	class_compat_unregister(mikrobus_port_compat_class);
+	idr_destroy(&mikrobus_port_idr);
+}
+module_exit(mikrobus_exit);
+
+MODULE_AUTHOR("Vaishnav M A <vaishnav@beagleboard.org>");
+MODULE_DESCRIPTION("mikroBUS main module");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/misc/mikrobus/mikrobus_core.h b/drivers/misc/mikrobus/mikrobus_core.h
new file mode 100644
index 000000000000..9684d315f564
--- /dev/null
+++ b/drivers/misc/mikrobus/mikrobus_core.h
@@ -0,0 +1,130 @@
+/* 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.
+ */
+
+#ifndef __MIKROBUS_H
+#define __MIKROBUS_H
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
+#include <linux/spi/spi.h>
+#include <linux/idr.h>
+#include <linux/init.h>
+#include <linux/jump_label.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/of_device.h>
+#include <linux/serdev.h>
+#include <linux/of.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+#define MIKROBUS_VERSION_MAJOR 0x00
+#define MIKROBUS_VERSION_MINOR 0x02
+
+extern struct bus_type mikrobus_bus_type;
+extern struct device_type mikrobus_port_type;
+
+enum mikrobus_property_type {
+	MIKROBUS_PROPERTY_TYPE_LINK = 0x00,
+	MIKROBUS_PROPERTY_TYPE_GPIO = 0x01,
+	MIKROBUS_PROPERTY_TYPE_U8 = 0x02,
+	MIKROBUS_PROPERTY_TYPE_U16 = 0x03,
+	MIKROBUS_PROPERTY_TYPE_U32 = 0x04,
+	MIKROBUS_PROPERTY_TYPE_U64 = 0x05,
+};
+
+enum mikrobus_gpio_pin {
+	MIKROBUS_GPIO_INVALID = 0x00,
+	MIKROBUS_GPIO_INT = 0x01,
+	MIKROBUS_GPIO_RST = 0x02,
+	MIKROBUS_GPIO_PWM = 0x03,
+};
+
+enum mikrobus_protocol {
+	MIKROBUS_PROTOCOL_SPI = 0x01,
+	MIKROBUS_PROTOCOL_I2C = 0x02,
+	MIKROBUS_PROTOCOL_UART = 0x03,
+	MIKROBUS_PROTOCOL_SPI_GPIOCS = 0x04,
+	MIKROBUS_PROTOCOL_I2C_MUX = 0x05
+};
+
+enum mikrobus_gpio_state {
+	MIKROBUS_GPIO_UNUSED = 0x00,
+	MIKROBUS_GPIO_INPUT = 0x01,
+	MIKROBUS_GPIO_OUTPUT_HIGH = 0x02,
+	MIKROBUS_GPIO_OUTPUT_LOW = 0x03,
+};
+
+struct mikrobus_port_config {
+	__u8 i2c_adap_nr;
+	__u8 spi_master_nr;
+	__u8 serdev_ctlr_nr;
+	__u8 rst_gpio_nr;
+	__u8 pwm_gpio_nr;
+	__u8 int_gpio_nr;
+} __packed;
+
+struct board_device_info {
+	struct list_head links;
+	int id;
+	char *drv_name;
+	unsigned short protocol;
+	unsigned short reg;
+	u32 max_speed_hz;
+	unsigned int mode;
+	int irq;
+	int irq_type;
+	int cs_gpio;
+	unsigned short num_gpio_resources;
+	unsigned short num_properties;
+	struct property_entry *properties;
+	struct gpiod_lookup_table *gpio_lookup;
+	void *dev_client;
+};
+
+struct addon_board_info {
+	char *name;
+	unsigned short num_devices;
+	unsigned short rst_gpio_state;
+	unsigned short pwm_gpio_state;
+	unsigned short int_gpio_state;
+	struct list_head manifest_descs;
+	struct list_head devices;
+};
+
+struct mikrobus_port {
+	char name[48];
+	struct module *owner;
+	struct device dev;
+	int id;
+	struct gpio_desc *pwm_gpio;
+	struct gpio_desc *int_gpio;
+	struct gpio_desc *rst_gpio;
+	struct spi_master *spi_mstr;
+	struct i2c_adapter *i2c_adap;
+	struct addon_board_info *board;
+	struct i2c_client *eeprom_client;
+	struct nvmem_device *eeprom;
+};
+#define to_mikrobus_port(d) container_of(d, struct mikrobus_port, dev)
+
+int mikrobus_register_board(struct mikrobus_port *port,
+							struct addon_board_info *board);
+void mikrobus_unregister_board(struct mikrobus_port *port,
+							   struct addon_board_info *board);
+int mikrobus_register_port_config(struct mikrobus_port_config *cfg);
+int mikrobus_register_port(struct mikrobus_port *port);
+void mikrobus_del_port(struct mikrobus_port *port);
+
+#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..60ebca560f0d
--- /dev/null
+++ b/drivers/misc/mikrobus/mikrobus_manifest.c
@@ -0,0 +1,390 @@
+// 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.
+ */
+
+#define pr_fmt(fmt) "mikrobus_manifest: " fmt
+
+#include <linux/bits.h>
+#include <linux/types.h>
+#include <linux/property.h>
+#include <linux/greybus/greybus_manifest.h>
+
+#include "mikrobus_manifest.h"
+
+struct manifest_desc {
+	struct list_head links;
+	size_t size;
+	void *data;
+	enum greybus_descriptor_type type;
+};
+
+static void release_manifest_descriptor(struct manifest_desc *descriptor)
+{
+	list_del(&descriptor->links);
+	kfree(descriptor);
+}
+
+static void release_manifest_descriptors(struct addon_board_info *info)
+{
+	struct manifest_desc *descriptor;
+	struct manifest_desc *next;
+
+	list_for_each_entry_safe(descriptor, next, &info->manifest_descs, links)
+		release_manifest_descriptor(descriptor);
+}
+
+static int identify_descriptor(struct addon_board_info *info, struct greybus_descriptor *desc,
+										 size_t size)
+{
+	struct greybus_descriptor_header *desc_header = &desc->header;
+	struct manifest_desc *descriptor;
+	size_t desc_size;
+	size_t expected_size;
+
+	if (size < sizeof(*desc_header))
+		return -EINVAL;
+	desc_size = le16_to_cpu(desc_header->size);
+	if (desc_size > 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:
+	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, &info->manifest_descs);
+	return desc_size;
+}
+
+static char *mikrobus_string_get(struct addon_board_info *info, 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, &info->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 *info)
+{
+	struct greybus_descriptor_mikrobus *mikrobus;
+	struct greybus_descriptor_interface *interface;
+	struct manifest_desc *descriptor;
+	bool found = false;
+
+	list_for_each_entry(descriptor, &info->manifest_descs, links) {
+		if (descriptor->type == GREYBUS_TYPE_MIKROBUS) {
+			mikrobus = descriptor->data;
+			found = true;
+			break;
+		}
+	}
+
+	if (found) {
+		info->num_devices = mikrobus->num_devices;
+		info->rst_gpio_state = mikrobus->rst_gpio_state;
+		info->pwm_gpio_state = mikrobus->pwm_gpio_state;
+		info->int_gpio_state = mikrobus->int_gpio_state;
+	} else {
+		info->num_devices = 1;
+		info->rst_gpio_state = MIKROBUS_GPIO_UNUSED;
+		info->pwm_gpio_state = MIKROBUS_GPIO_UNUSED;
+		info->int_gpio_state = MIKROBUS_GPIO_UNUSED;
+	}
+
+	list_for_each_entry(descriptor, &info->manifest_descs, links) {
+		if (descriptor->type == GREYBUS_TYPE_INTERFACE) {
+			interface = descriptor->data;
+			break;
+		}
+	}
+	info->name = mikrobus_string_get(info, interface->product_stringid);
+}
+
+static struct property_entry *
+mikrobus_property_entry_get(struct addon_board_info *info, u8 *prop_link,
+							int num_properties)
+{
+	struct greybus_descriptor_property *desc_property;
+	struct manifest_desc *descriptor;
+	struct property_entry *properties;
+	int i;
+	char *prop_name;
+	bool found = false;
+	u8 *val_u8;
+	u16 *val_u16;
+	u32 *val_u32;
+	u64 *val_u64;
+
+	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, &info->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)
+			return ERR_PTR(-ENOENT);
+		prop_name = mikrobus_string_get(info, desc_property->propname_stringid);
+		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:
+		return ERR_PTR(-EINVAL);
+		}
+	}
+	return properties;
+}
+
+static u8 *mikrobus_property_link_get(struct addon_board_info *info, u8 prop_id,
+									  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, &info->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);
+	return val_u8;
+}
+
+static int mikrobus_manifest_attach_device(struct addon_board_info *info,
+						struct greybus_descriptor_device *dev_desc)
+{
+	struct board_device_info *dev;
+	struct gpiod_lookup_table *lookup;
+	struct greybus_descriptor_property *desc_property;
+	struct manifest_desc *descriptor;
+	int i;
+	u8 *prop_link;
+	u8 *gpio_desc_link;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+	dev->id = dev_desc->id;
+	dev->drv_name = mikrobus_string_get(info, dev_desc->driver_stringid);
+	dev->protocol = dev_desc->protocol;
+	dev->reg = dev_desc->reg;
+	dev->irq = dev_desc->irq;
+	dev->irq_type = dev_desc->irq_type;
+	dev->max_speed_hz = le32_to_cpu(dev_desc->max_speed_hz);
+	dev->mode = dev_desc->mode;
+	dev->cs_gpio = dev_desc->cs_gpio;
+	dev->num_gpio_resources = dev_desc->num_gpio_resources;
+	dev->num_properties = dev_desc->num_properties;
+	pr_info("device %d , number of properties=%d , number of gpio resources=%d\n",
+	dev->id, dev->num_properties, dev->num_gpio_resources);
+	if (dev->num_properties > 0) {
+		prop_link = mikrobus_property_link_get(info, dev_desc->prop_link,
+								MIKROBUS_PROPERTY_TYPE_LINK);
+		dev->properties = mikrobus_property_entry_get(info, prop_link, dev->num_properties);
+	}
+	if (dev->num_gpio_resources > 0) {
+		lookup = kzalloc(struct_size(lookup, table, dev->num_gpio_resources),
+					GFP_KERNEL);
+	if (!lookup)
+		return -ENOMEM;
+	gpio_desc_link = mikrobus_property_link_get(info, dev_desc->gpio_link,
+						MIKROBUS_PROPERTY_TYPE_GPIO);
+	for (i = 0; i < dev->num_gpio_resources; i++) {
+		list_for_each_entry(descriptor, &info->manifest_descs, links) {
+			if (descriptor->type != GREYBUS_TYPE_PROPERTY)
+				continue;
+			desc_property = descriptor->data;
+			if (desc_property->id == gpio_desc_link[i]) {
+				lookup->table[i].chip_hwnum = *desc_property->value;
+				lookup->table[i].con_id =
+				mikrobus_string_get(info, desc_property->propname_stringid);
+				break;
+				}
+		}
+	}
+	dev->gpio_lookup = lookup;
+	}
+	list_add_tail(&dev->links, &info->devices);
+	return 0;
+}
+
+static int mikrobus_manifest_parse_devices(struct addon_board_info *info)
+{
+	struct greybus_descriptor_device *desc_device;
+	struct manifest_desc *desc, *next;
+	int devcount = 0;
+
+	if (WARN_ON(!list_empty(&info->devices)))
+		return false;
+	list_for_each_entry_safe(desc, next, &info->manifest_descs, links) {
+		if (desc->type != GREYBUS_TYPE_DEVICE)
+			continue;
+		desc_device = desc->data;
+		mikrobus_manifest_attach_device(info, desc_device);
+		devcount++;
+	}
+	return devcount;
+}
+
+bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
+							 size_t size)
+{
+	struct greybus_manifest *manifest;
+	struct greybus_manifest_header *header;
+	struct greybus_descriptor *desc;
+	u16 manifest_size;
+	int dev_count;
+	int desc_size;
+
+	if (WARN_ON(!list_empty(&info->manifest_descs)))
+		return false;
+	if (size < sizeof(*header))
+		return false;
+	manifest = data;
+	header = &manifest->header;
+	manifest_size = le16_to_cpu(header->size);
+	if (manifest_size != size)
+		return false;
+	if (header->version_major > MIKROBUS_VERSION_MAJOR)
+		return false;
+	desc = manifest->descriptors;
+	size -= sizeof(*header);
+	while (size) {
+		desc_size = identify_descriptor(info, desc, size);
+		if (desc_size < 0) {
+			pr_err("invalid manifest descriptor");
+		return -EINVAL;
+		}
+		desc = (struct greybus_descriptor *)((char *)desc + desc_size);
+		size -= desc_size;
+	}
+	mikrobus_state_get(info);
+	dev_count = mikrobus_manifest_parse_devices(info);
+	pr_info(" %s manifest parsed with %d device(s)\n", info->name,
+		info->num_devices);
+	release_manifest_descriptors(info);
+	return true;
+}
+
+size_t mikrobus_manifest_header_validate(void *data, size_t size)
+{
+	struct greybus_manifest_header *header;
+	u16 manifest_size;
+
+	if (size < sizeof(*header))
+		return 0;
+
+	header = data;
+	manifest_size = le16_to_cpu(header->size);
+	if (header->version_major > MIKROBUS_VERSION_MAJOR)
+		return 0;
+	return manifest_size;
+}
diff --git a/drivers/misc/mikrobus/mikrobus_manifest.h b/drivers/misc/mikrobus/mikrobus_manifest.h
new file mode 100644
index 000000000000..a195d1c26493
--- /dev/null
+++ b/drivers/misc/mikrobus/mikrobus_manifest.h
@@ -0,0 +1,21 @@
+/* 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"
+
+bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
+							 size_t size);
+size_t mikrobus_manifest_header_validate(void *data, size_t size);
+
+#endif /* __MIKROBUS_MANIFEST_H */
diff --git a/include/linux/greybus/greybus_manifest.h b/include/linux/greybus/greybus_manifest.h
index 6e62fe478712..79c8cab9ef96 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,53 @@ struct greybus_descriptor_cport {
 	__u8	protocol_id;	/* enum greybus_protocol */
 } __packed;
 
+/*
+ * A mikrobus descriptor is used to describe the details
+ * about the add-on board connected to the mikrobus port.
+ * It describes the number of indivdual devices on the
+ * add-on board and the default states of the GPIOs
+ */
+struct greybus_descriptor_mikrobus {
+	__u8 num_devices;
+	__u8 rst_gpio_state;
+	__u8 pwm_gpio_state;
+	__u8 int_gpio_state;
+} __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[0];
+} __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 num_properties;
+	__u8 protocol;
+	__le32 max_speed_hz;
+	__u8 reg;
+	__u8 mode;
+	__u8 num_gpio_resources;
+	__u8 cs_gpio;
+	__u8 irq;
+	__u8 irq_type;
+	__u8 prop_link;
+	__u8 gpio_link;
+} __packed;
+
 struct greybus_descriptor_header {
 	__le16	size;
 	__u8	type;		/* enum greybus_descriptor_type */
@@ -164,6 +214,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.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] RFC : mikroBUS driver for add-on boards
  2020-07-24 12:06 [PATCH] RFC : mikroBUS driver for add-on boards Vaishnav M A
@ 2020-07-24 20:02 ` Christopher Friedt
  2020-07-26 12:48 ` [greybus-dev] " Alex Elder
  2020-07-26 15:04 ` Alex Elder
  2 siblings, 0 replies; 7+ messages in thread
From: Christopher Friedt @ 2020-07-24 20:02 UTC (permalink / raw)
  To: Vaishnav M A
  Cc: greybus-dev, linux-kernel, Greg KH, arnd, johan, elder, robh,
	mchehab+huawei, davem, Jason Kridner, Drew Fustini,
	robertcnelson, rajkovic, zoran.stojsavljevic

Hi Vaishnav!

On Fri, Jul 24, 2020 at 8:06 AM Vaishnav M A <vaishnav@beagleboard.org> wrote:
> +enum mikrobus_protocol {
> +       MIKROBUS_PROTOCOL_SPI = 0x01,
> +       MIKROBUS_PROTOCOL_I2C = 0x02,
> +       MIKROBUS_PROTOCOL_UART = 0x03,
> +       MIKROBUS_PROTOCOL_SPI_GPIOCS = 0x04,
> +       MIKROBUS_PROTOCOL_I2C_MUX = 0x05
> +};

We've already chatted about this off-list, but I just wanted to post
here as well so other readers are aware.

Given that MikroBus will be layered on top of Greybus and will use
Greybus Manifests, and that there is a related change [1] for the
Greybus Manifest tool (manifesto), and given that other, non-mikroBUS,
form factors may want to re-use the "[device-descriptor N]" tag, I
think it would make sense to use

[device-descriptor];
protocol = [cport protocol]

instead of

[device-descriptor];
protocol = [mikrobus protocol].

As you mentioned, there is no specific mikrobus i2c protocol, and the
protocol field is really just an enumeration.

If there are no other technical issues aside from that, I think it
would make sense to use the Greybus CPort protocol enumerations
instead of introducing a new one that is less generic.

That's the only significant critique I have.

Otherwise, great work and thank you for your contribution!

[1] https://github.com/projectara/manifesto/pull/2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [greybus-dev] [PATCH] RFC : mikroBUS driver for add-on boards
  2020-07-24 12:06 [PATCH] RFC : mikroBUS driver for add-on boards Vaishnav M A
  2020-07-24 20:02 ` Christopher Friedt
@ 2020-07-26 12:48 ` Alex Elder
  2020-07-26 17:37   ` Vaishnav M A
  2020-07-26 15:04 ` Alex Elder
  2 siblings, 1 reply; 7+ messages in thread
From: Alex Elder @ 2020-07-26 12:48 UTC (permalink / raw)
  To: Vaishnav M A, greybus-dev, linux-kernel, gregkh, arnd, johan, elder
  Cc: rajkovic, robh, mchehab+huawei, robertcnelson, drew, davem,
	zoran.stojsavljevic

On 7/24/20 7:06 AM, Vaishnav M A wrote:
> Attached is a patch for the mikroBUS driver which helps to
> instantiate an add-on board device on a mikrobus port by fetching
> the device identifier manifest binary from an EEPROM on-board
> the device. mikroBUS is an add-on board socket standard by
> MikroElektronika that can be freely used by anyone
> following the guidelines, more details and discussions on
> the status of the driver can be found here in this eLinux wiki:
> https://elinux.org/Mikrobus

Vaishnav, I am finally looking at this a little more closely
today.  More than anything I want to compliment you on all
this work.  I think it looks like a great use of Greybus for
essentially its intended purpose, and I would love to see
it extended as needed to support what you're doing here.

At first glance your patch looks good, but I want to take the
time to give it a thorough review.  Unfortunately I did not
follow your progress on the GSoC project (which you posted
about last year here), and have not followed any discussion
since then, so it's taking me a little time to come up to speed
on it.  I'm hoping you might help me (and others) do this more
quickly.

I am scanning through some of the materials online and I find
there is quite a lot.  That includes information about both
your project and about mikroBUS.  Zeroing in on things that
fairly concisely describe the way things really work would be
very valuable.  Can you point me directly at something that
gives an overview of both the hardware and software
architecture (specifically as it's used with Greybus)?  If
not, I'm open to finding other ways to get in synch.  I'll
expand on this a little more below.

This isn't strictly necessary, but if I wanted to reproduce the
hardware setup you use to validate this code, what hardware
is required?  For example, is it possible to use a BeagleBone Black
with a MikroBus Cape for testing, or must a PocktBeagle be used?
Is there one or several MikroBus clickboards that would be the
best for basic testing?

Can you provide a short summary of why the Greybus manifest
format needed to be extended?  Can you summarize how a mikroBUS
add-on board differs from an Ara module?  And what they have in
common?

This patch is an RFC, and you say that v3 of the mikroBUS spec
is being developed.  Is your plan to have the "real" code (when
you submit it) adhere to the newer version of the spec?  Are
there specific things that you expect will be included in v3
that will how the driver works (compared to this RFC)?

Please realize I'm asking these questions so I can be more
effective in evaluating what you're doing here.  I'd like to
provide feedback not just on the code, but on the design that
underlies it, and for that I need to get better informed.  My
hope is that you can help me find or gather that information
as quickly as possible.

And now I'll go give a quick initial review of the code...

Thanks.

					-Alex

> In the current state of the driver, more than 80 different
> add-on boards have been tested on the BeagleBoard.org
> PocketBeagle and the manifest binary is generated using the same
> manifesto tool used to generate Greybus manifest binaries,
> The pull request to manifesto to add new descriptors specific
> to mikrobus is here : https://github.com/projectara/manifesto/pull/2
> The utilization of Greybus manifest binaries here is not entirely
> coincidental, We are evaluating ways to add mikroBUS sockets and
> devices via Greybus expansion.
> 
> The mikroBUS standard includes SPI, I2C, UART, PWM, ADC, GPIO
> and power (3.3V and 5V) connections to interface common embedded
> peripherals, There are more than 750 add-on boards ranging from
> wireless connectivity boards to human-machine interface sensors
> which conform to the mikroBUS standard, out of which more than 140
> boards already have support in the Linux kernel.Today, there is no
> mainlinesolution for enabling mikroBUS add-on boards at run-time, the
> most straight forward method for loading drivers is to provide
> device-tree overlay fragments at boot time, this method suffers
> from the need to maintain a large out-of-tree database for which there
> is need to maintain an overlay for every mikroBUS add-on board for every
> Linux system and for every mikroBUS socket on that system.
> 
> The mikroBUS driver tries to solve the problem by using extended version
> of the greybus manifest to describe the add-on board device specific
> information required by the device driver and uses it along with the fixed
> port specific information to probe the specific device driver.The manifest
> binary is now fetched from an I2C EEPROM over the I2C bus on the mikroBUS
> port(subject to change in mikroBUS v3 specification) and enumerate drivers
> for the add-on devices.There is also ongoing work to define a mikroBUS
> v3 standard in which the addon board includes a non-volatile storage to
> store the device identifier manifest binary, once the mikroBUS v3 standard
> is released, the mikroBUS can be seen as a probeable bus such that the
> kernel can discover the device on the bus at boot time.
> 
> The driver also has a few debug SysFS interfaces for testing on add-on
> boards without an EEPROM, these can be used in the following manner:
> (example for mikroBUS port 1 on BeagleBoard.org PocketBeagle):
> 
> printf "%b" '\x01\x00\x00\x59\x32\x17' > /sys/bus/mikrobus/add_port
> 
> The bytes in the byte array sequence are (in order):
> 
> 	* i2c_adap_nr
> 	* spi_master_nr
> 	* serdev_ctlr_nr
> 	* rst_gpio_nr
> 	* pwm_gpio_nr
> 	* int_gpio_nr
> * add_port sysFS entry is purely for debug and in the actual state
> of the driver, port configuration will be loaded from a suitable
> device tree overlay fragment.
> 
> echo 0 > /sys/bus/mikrobus/del_port (to delete the attached port)
> echo 1 >  /sys/class/mikrobus-port/mikrobus-0/rescan (to rescan the EEPROM
> contents on the I2C bus on the mikroBUS port).
> 
> cat board_manifest.mnfb >  /sys/class/mikrobus-port/mikrobus-0/new_device
> * debug interface to pass the manifest binary in case an EEPROM is absent
> echo 0 >  /sys/class/mikrobus-port/mikrobus-0/delete_device
> * to unload the loaded board on the mikrobus port
> 
> These sysFS interfaces are only implemented for debug purposes and
> in the actual implementation of the driver these will be removed and
> the manifest binary will be fetched from the non volatile storage on-board
> the device.
> 
> The mikroBUS driver enable the mikroBUS to be a probeable bus such that
> the kernel can discover the device and automatically load the drivers.
> There are already several Linux platforms with mikroBUS sockets and the
> mikroBUS driver helps to reduce the time to develop and debug
> support for various mikroBUS add-on boards. Further, it opens up
> the possibility for support under dynamically instantiated buses
> such as with Greybus.
> 
> Please let know the feedback you have on this patch or the approach used.
> 
> Thanks,
> 
> Vaishnav M A
> 
> Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org>
> ---
>  MAINTAINERS                               |   6 +
>  drivers/misc/Kconfig                      |   1 +
>  drivers/misc/Makefile                     |   1 +
>  drivers/misc/mikrobus/Kconfig             |  16 +
>  drivers/misc/mikrobus/Makefile            |   5 +
>  drivers/misc/mikrobus/mikrobus_core.c     | 649 ++++++++++++++++++++++
>  drivers/misc/mikrobus/mikrobus_core.h     | 130 +++++
>  drivers/misc/mikrobus/mikrobus_manifest.c | 390 +++++++++++++
>  drivers/misc/mikrobus/mikrobus_manifest.h |  21 +
>  include/linux/greybus/greybus_manifest.h  |  53 ++
>  10 files changed, 1272 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 d53db30d1365..9a049746203f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11402,6 +11402,12 @@ M:	Oliver Neukum <oliver@neukum.org>
>  S:	Maintained
>  F:	drivers/usb/image/microtek.*
>  
> +MIKROBUS ADDON BOARD DRIVER
> +M:	Vaishnav M A <vaishnav@beagleboard.org>
> +S:	Maintained
> +W:	https://elinux.org/Mikrobus
> +F:	drivers/misc/mikrobus/
> +
>  MIPS
>  M:	Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>  L:	linux-mips@vger.kernel.org
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index e1b1ba5e2b92..334f0c39d56b 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -472,4 +472,5 @@ source "drivers/misc/ocxl/Kconfig"
>  source "drivers/misc/cardreader/Kconfig"
>  source "drivers/misc/habanalabs/Kconfig"
>  source "drivers/misc/uacce/Kconfig"
> +source "drivers/misc/mikrobus/Kconfig"
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index c7bd01ac6291..45486dd77da5 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_VMWARE_BALLOON)	+= vmw_balloon.o
>  obj-$(CONFIG_PCH_PHUB)		+= pch_phub.o
>  obj-y				+= ti-st/
>  obj-y				+= lis3lv02d/
> +obj-y				+= mikrobus/
>  obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
>  obj-$(CONFIG_INTEL_MEI)		+= mei/
>  obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
> diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig
> new file mode 100644
> index 000000000000..c3b93e12daad
> --- /dev/null
> +++ b/drivers/misc/mikrobus/Kconfig
> @@ -0,0 +1,16 @@
> +menuconfig MIKROBUS
> +	tristate "Module for instantiating devices on mikroBUS ports"
> +	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 helps in
> +	  instantiating devices on the mikroBUS port with identifier
> +	  data fetched from an EEPROM on the device, more details on
> +	  the mikroBUS driver support and discussion can be found in
> +	  this eLinux wiki : elinux.org/Mikrobus
> +
> +
> +	  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..1f80ed4064d8
> --- /dev/null
> +++ b/drivers/misc/mikrobus/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# mikroBUS Core
> +
> +mikrobus-y :=	mikrobus_core.o	mikrobus_manifest.o
> +obj-$(CONFIG_MIKROBUS) += mikrobus.o
> \ No newline at end of file
> diff --git a/drivers/misc/mikrobus/mikrobus_core.c b/drivers/misc/mikrobus/mikrobus_core.c
> new file mode 100644
> index 000000000000..78c96c637632
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_core.c
> @@ -0,0 +1,649 @@
> +// 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.
> + */
> +
> +#define pr_fmt(fmt) "mikrobus: " fmt
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/jump_label.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mutex.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/interrupt.h>
> +#include <linux/spi/spi.h>
> +#include <linux/serdev.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +#include "mikrobus_core.h"
> +#include "mikrobus_manifest.h"
> +
> +#define ATMEL_24C32_I2C_ADDR 0x57
> +
> +static DEFINE_IDR(mikrobus_port_idr);
> +static struct class_compat *mikrobus_port_compat_class;
> +static bool is_registered;
> +
> +static ssize_t add_port_store(struct bus_type *bt, const char *buf,
> +			      size_t count)
> +{
> +	struct mikrobus_port_config *cfg;
> +
> +	if (count < sizeof(*cfg)) {
> +		pr_err("add_port: incorrect config data received: %s\n", buf);
> +		return -EINVAL;
> +	}
> +	mikrobus_register_port_config((void *)buf);
> +	return count;
> +}
> +BUS_ATTR_WO(add_port);
> +
> +static ssize_t del_port_store(struct bus_type *bt, const char *buf,
> +							  size_t count)
> +{
> +	int id;
> +	char end;
> +	int res;
> +
> +	res = sscanf(buf, "%d%c", &id, &end);
> +	if (res < 1) {
> +		pr_err("delete_port: cannot parse mikrobus port ID\n");
> +		return -EINVAL;
> +	}
> +	if (!idr_find(&mikrobus_port_idr, id)) {
> +		pr_err("attempting to delete unregistered port [%d]\n", id);
> +		return -EINVAL;
> +	}
> +	mikrobus_del_port(idr_find(&mikrobus_port_idr, id));
> +	return count;
> +}
> +BUS_ATTR_WO(del_port);
> +
> +static struct attribute *mikrobus_attrs[] = {
> +	&bus_attr_add_port.attr,
> +	&bus_attr_del_port.attr,
> +	 NULL
> +};
> +ATTRIBUTE_GROUPS(mikrobus);
> +
> +struct bus_type mikrobus_bus_type = {
> +	.name = "mikrobus",
> +	.bus_groups = mikrobus_groups,
> +};
> +EXPORT_SYMBOL_GPL(mikrobus_bus_type);
> +
> +static int mikrobus_port_scan_eeprom(struct mikrobus_port *port)
> +{
> +	char header[12];
> +	struct addon_board_info *board;
> +	int manifest_size;
> +	int retval;
> +	char *buf;
> +
> +	nvmem_device_read(port->eeprom, 0, 12, header);
> +	manifest_size = mikrobus_manifest_header_validate(header, 12);
> +	if (manifest_size > 0) {
> +		buf = kzalloc(manifest_size, GFP_KERNEL);
> +		nvmem_device_read(port->eeprom, 0, manifest_size, buf);
> +		board = kzalloc(sizeof(*board), GFP_KERNEL);
> +		if (!board)
> +			return -ENOMEM;
> +		INIT_LIST_HEAD(&board->manifest_descs);
> +		INIT_LIST_HEAD(&board->devices);
> +		retval = mikrobus_manifest_parse(board, (void *)buf, manifest_size);
> +		if (!retval) {
> +			pr_err("failed to parse manifest, size: %d", manifest_size);
> +			return -EINVAL;
> +		}
> +		retval = mikrobus_register_board(port, board);
> +		if (retval) {
> +			pr_err("failed to register board: %s", board->name);
> +			return -EINVAL;
> +		}
> +		kfree(buf);
> +	} else {
> +		pr_err("inavlide manifest port %d", port->id);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> +						 char *buf)
> +{
> +	return sprintf(buf, "%s\n", to_mikrobus_port(dev)->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +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 retval;
> +
> +	if (port->board == NULL) {
> +		board = kzalloc(sizeof(*board), GFP_KERNEL);
> +		if (!board)
> +			return -EINVAL;
> +		INIT_LIST_HEAD(&board->manifest_descs);
> +		INIT_LIST_HEAD(&board->devices);
> +	} else {
> +		pr_err("port %d already has board registered", port->id);
> +		return -EINVAL;
> +	}
> +	retval = mikrobus_manifest_parse(board, (void *)buf, count);
> +	if (!retval) {
> +		pr_err("failed to parse manifest");
> +		return -EINVAL;
> +	}
> +	retval = mikrobus_register_board(port, board);
> +	if (retval) {
> +		pr_err("failed to register board: %s", board->name);
> +		return -EINVAL;
> +	}
> +	return count;
> +}
> +static DEVICE_ATTR_WO(new_device);
> +
> +static ssize_t rescan_store(struct device *dev, struct device_attribute *attr,
> +							const char *buf, size_t count)
> +{
> +	struct mikrobus_port *port = to_mikrobus_port(dev);
> +	int id;
> +	char end;
> +	int res;
> +	int retval;
> +
> +	res = sscanf(buf, "%d%c", &id, &end);
> +	if (res < 1) {
> +		pr_err("rescan: Can't parse trigger\n");
> +		return -EINVAL;
> +	}
> +	if (port->board != NULL) {
> +		pr_err("port %d already has board registered", port->id);
> +		return -EINVAL;
> +	}
> +	retval = mikrobus_port_scan_eeprom(port);
> +	if (retval) {
> +		pr_err("port %d board register from manifest failed", port->id);
> +		return -EINVAL;
> +	}
> +	return count;
> +}
> +static DEVICE_ATTR_WO(rescan);
> +
> +static ssize_t delete_device_store(struct device *dev, struct device_attribute *attr,
> +							const char *buf, size_t count)
> +{
> +	int id;
> +	char end;
> +	int res;
> +	struct mikrobus_port *port = to_mikrobus_port(dev);
> +
> +	res = sscanf(buf, "%d%c", &id, &end);
> +	if (res < 1) {
> +		pr_err("delete_board: Can't parse board ID\n");
> +		return -EINVAL;
> +	}
> +	if (port->board == NULL) {
> +		pr_err("delete_board: port does not have any boards registered\n");
> +		return -EINVAL;
> +	}
> +	mikrobus_unregister_board(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_rescan.attr,
> +	&dev_attr_delete_device.attr, &dev_attr_name.attr, NULL};
> +ATTRIBUTE_GROUPS(mikrobus_port);
> +
> +static void mikrobus_dev_release(struct device *dev)
> +{
> +	struct mikrobus_port *port = to_mikrobus_port(dev);
> +
> +	idr_remove(&mikrobus_port_idr, port->id);
> +	kfree(port);
> +}
> +
> +struct device_type mikrobus_port_type = {
> +	.groups = mikrobus_port_groups,
> +	.release = mikrobus_dev_release,
> +};
> +EXPORT_SYMBOL_GPL(mikrobus_port_type);
> +
> +static int mikrobus_get_irq(struct mikrobus_port *port, int irqno,
> +							int irq_type)
> +{
> +	int irq;
> +
> +	switch (irqno) {
> +	case MIKROBUS_GPIO_INT:
> +	irq = gpiod_to_irq(port->int_gpio);
> +	break;
> +	case MIKROBUS_GPIO_RST:
> +	irq = gpiod_to_irq(port->rst_gpio);
> +	break;
> +	case MIKROBUS_GPIO_PWM:
> +	irq = gpiod_to_irq(port->pwm_gpio);
> +	break;
> +	default:
> +	return -EINVAL;
> +	}
> +	if (irq < 0) {
> +		pr_err("Could not get irq for irq type: %d", irqno);
> +		return -EINVAL;
> +	}
> +	irq_set_irq_type(irq, irq_type);
> +	return irq;
> +}
> +
> +static int mikrobus_setup_gpio(struct gpio_desc *gpio, int gpio_state)
> +{
> +	int retval;
> +
> +	if (gpio_state == MIKROBUS_GPIO_UNUSED)
> +		return 0;
> +	switch (gpio_state) {
> +	case MIKROBUS_GPIO_INPUT:
> +	retval = gpiod_direction_input(gpio);
> +	break;
> +	case MIKROBUS_GPIO_OUTPUT_HIGH:
> +	retval = gpiod_direction_output(gpio, 1);
> +	gpiod_set_value_cansleep(gpio, 1);
> +	break;
> +	case MIKROBUS_GPIO_OUTPUT_LOW:
> +	retval = gpiod_direction_output(gpio, 0);
> +	gpiod_set_value_cansleep(gpio, 0);
> +	break;
> +	default:
> +	return -EINVAL;
> +	}
> +	return retval;
> +}
> +
> +static void mikrobus_spi_device_delete(struct spi_master *master, unsigned int cs)
> +{
> +	struct device *dev;
> +	char str[32];
> +
> +	snprintf(str, sizeof(str), "%s.%u", dev_name(&master->dev), cs);
> +	dev = bus_find_device_by_name(&spi_bus_type, NULL, str);
> +	if (dev != NULL) {
> +		spi_unregister_device(to_spi_device(dev));
> +		put_device(dev);
> +	}
> +}
> +
> +static char *mikrobus_get_gpio_chip_name(struct mikrobus_port *port, int gpio)
> +{
> +	char *name;
> +	struct gpio_chip *gpiochip;
> +
> +	switch (gpio) {
> +	case MIKROBUS_GPIO_INT:
> +	gpiochip = gpiod_to_chip(port->int_gpio);
> +	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> +	break;
> +	case MIKROBUS_GPIO_RST:
> +	gpiochip = gpiod_to_chip(port->rst_gpio);
> +	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> +	break;
> +	case MIKROBUS_GPIO_PWM:
> +	gpiochip = gpiod_to_chip(port->pwm_gpio);
> +	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> +	break;
> +	}
> +	return name;
> +}
> +
> +static int mikrobus_get_gpio_hwnum(struct mikrobus_port *port, int gpio)
> +{
> +	int hwnum;
> +	struct gpio_chip *gpiochip;
> +
> +	switch (gpio) {
> +	case MIKROBUS_GPIO_INT:
> +	gpiochip = gpiod_to_chip(port->int_gpio);
> +	hwnum = desc_to_gpio(port->int_gpio) - gpiochip->base;
> +	break;
> +	case MIKROBUS_GPIO_RST:
> +	gpiochip = gpiod_to_chip(port->rst_gpio);
> +	hwnum = desc_to_gpio(port->rst_gpio) - gpiochip->base;
> +	break;
> +	case MIKROBUS_GPIO_PWM:
> +	gpiochip = gpiod_to_chip(port->pwm_gpio);
> +	hwnum = desc_to_gpio(port->pwm_gpio) - gpiochip->base;
> +	break;
> +	}
> +	return hwnum;
> +}
> +
> +static void release_mikrobus_device(struct board_device_info *dev)
> +{
> +	list_del(&dev->links);
> +	kfree(dev);
> +}
> +
> +static void release_board_devices(struct addon_board_info *info)
> +{
> +	struct board_device_info *dev;
> +	struct board_device_info *next;
> +
> +	list_for_each_entry_safe(dev, next, &info->devices, links)
> +		release_mikrobus_device(dev);
> +}
> +
> +static int mikrobus_register_device(struct mikrobus_port *port,
> +					struct board_device_info *dev, char *board_name)
> +{
> +	struct i2c_board_info *i2c;
> +	struct spi_board_info *spi;
> +	struct gpiod_lookup_table *lookup;
> +	char devname[40];
> +	int i;
> +
> +	pr_info(" registering device : %s\n", dev->drv_name);
> +
> +	if (dev->gpio_lookup != NULL) {
> +		lookup = dev->gpio_lookup;
> +		if (dev->protocol == MIKROBUS_PROTOCOL_SPI) {
> +			snprintf(devname, sizeof(devname), "%s.%u",
> +				dev_name(&port->spi_mstr->dev), dev->reg);
> +			lookup->dev_id = kmemdup(devname, 40, GFP_KERNEL);
> +		} else if (dev->protocol == MIKROBUS_PROTOCOL_I2C)
> +			lookup->dev_id = dev->drv_name;
> +		pr_info(" adding lookup table : %s\n", lookup->dev_id);
> +		for (i = 0; i < dev->num_gpio_resources; i++) {
> +			lookup->table[i].key =
> +			mikrobus_get_gpio_chip_name(port, lookup->table[i].chip_hwnum);
> +			lookup->table[i].chip_hwnum =
> +			mikrobus_get_gpio_hwnum(port, lookup->table[i].chip_hwnum);
> +			lookup->table[i].flags = GPIO_ACTIVE_HIGH;
> +		}
> +		gpiod_add_lookup_table(lookup);
> +	}
> +	switch (dev->protocol) {
> +	case MIKROBUS_PROTOCOL_SPI:
> +	spi = kzalloc(sizeof(*spi), GFP_KERNEL);
> +	if (!spi)
> +		return -ENOMEM;
> +	strncpy(spi->modalias, dev->drv_name, sizeof(spi->modalias) - 1);
> +	if (dev->irq)
> +		spi->irq = mikrobus_get_irq(port, dev->irq, dev->irq_type);
> +	if (dev->properties)
> +		spi->properties = dev->properties;
> +	spi->chip_select = dev->reg;
> +	spi->max_speed_hz = dev->max_speed_hz;
> +	spi->mode = dev->mode;
> +	mikrobus_spi_device_delete(port->spi_mstr, dev->reg);
> +	dev->dev_client = (void *)spi_new_device(port->spi_mstr, spi);
> +	break;
> +	case MIKROBUS_PROTOCOL_I2C:
> +	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +	strncpy(i2c->type, dev->drv_name, sizeof(i2c->type) - 1);
> +	if (dev->irq)
> +		i2c->irq = mikrobus_get_irq(port, dev->irq, dev->irq_type);
> +	if (dev->properties)
> +		i2c->properties = dev->properties;
> +	i2c->addr = dev->reg;
> +	dev->dev_client = (void *)i2c_new_client_device(port->i2c_adap, i2c);
> +	break;
> +	case MIKROBUS_PROTOCOL_UART:
> +	pr_info("SERDEV devices support not yet added");
> +	break;
> +	default:
> +	return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static void mikrobus_unregister_device(struct mikrobus_port *port, struct board_device_info *dev,
> +										char *board_name)
> +{
> +	pr_info(" removing device : %s\n", dev->drv_name);
> +	if (dev->gpio_lookup != NULL) {
> +		gpiod_remove_lookup_table(dev->gpio_lookup);
> +		kfree(dev->gpio_lookup);
> +	}
> +	if (dev->properties != NULL)
> +		kfree(dev->properties);
> +	switch (dev->protocol) {
> +	case MIKROBUS_PROTOCOL_SPI:
> +	spi_unregister_device((struct spi_device *)dev->dev_client);
> +	break;
> +	case MIKROBUS_PROTOCOL_I2C:
> +	i2c_unregister_device((struct i2c_client *)dev->dev_client);
> +	break;
> +	case MIKROBUS_PROTOCOL_UART:
> +	pr_err("SERDEV devices support not yet added");
> +	break;
> +	}
> +}
> +
> +int mikrobus_register_board(struct mikrobus_port *port,	struct addon_board_info *board)
> +{
> +	struct board_device_info *devinfo;
> +	struct board_device_info *next;
> +	int retval;
> +
> +	if (WARN_ON(list_empty(&board->devices)))
> +		return false;
> +
> +	retval = mikrobus_setup_gpio(port->pwm_gpio, board->pwm_gpio_state);
> +	if (retval) {
> +		pr_err("mikrobus_setup_gpio : can't setup pwm gpio state: (%d)\n", retval);
> +		return retval;
> +	}
> +	retval = mikrobus_setup_gpio(port->int_gpio, board->int_gpio_state);
> +	if (retval) {
> +		pr_err("mikrobus_setup_gpio : can't setup int gpio state: (%d)\n", retval);
> +		return retval;
> +	}
> +	retval = mikrobus_setup_gpio(port->rst_gpio, board->rst_gpio_state);
> +	if (retval) {
> +		pr_err("mikrobus_setup_gpio : can't setup rst gpio state: (%d)\n", retval);
> +		return retval;
> +	}
> +	list_for_each_entry_safe(devinfo, next, &board->devices, links)
> +		mikrobus_register_device(port, devinfo, board->name);
> +	port->board = board;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mikrobus_register_board);
> +
> +void mikrobus_unregister_board(struct mikrobus_port *port, struct addon_board_info *board)
> +{
> +	struct board_device_info *devinfo;
> +	struct board_device_info *next;
> +
> +	if (WARN_ON(list_empty(&board->devices)))
> +		return;
> +	port->board = NULL;
> +	list_for_each_entry_safe(devinfo, next, &board->devices, links)
> +		mikrobus_unregister_device(port, devinfo, board->name);
> +	release_board_devices(board);
> +	kfree(board);
> +	port->board = NULL;
> +}
> +EXPORT_SYMBOL_GPL(mikrobus_unregister_board);
> +
> +int mikrobus_register_port_config(struct mikrobus_port_config *cfg)
> +{
> +	struct mikrobus_port *port;
> +	int retval;
> +
> +	if (WARN_ON(!is_registered))
> +		return -EAGAIN;
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +	port->pwm_gpio = gpio_to_desc(cfg->pwm_gpio_nr);
> +	port->int_gpio = gpio_to_desc(cfg->int_gpio_nr);
> +	port->rst_gpio = gpio_to_desc(cfg->rst_gpio_nr);
> +	port->spi_mstr = spi_busnum_to_master(cfg->spi_master_nr);
> +	port->i2c_adap = i2c_get_adapter(cfg->i2c_adap_nr);
> +	retval = mikrobus_register_port(port);
> +	if (retval) {
> +		pr_err("port : can't register port from config (%d)\n", retval);
> +		return retval;
> +	}
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(mikrobus_register_port_config);
> +
> +static struct i2c_board_info mikrobus_eeprom_info = {
> +	I2C_BOARD_INFO("24c32", ATMEL_24C32_I2C_ADDR),
> +};
> +
> +static int mikrobus_port_probe_eeprom(struct mikrobus_port *port)
> +{
> +	struct i2c_client *eeprom_client;
> +	struct nvmem_device *eeprom;
> +	char dev_name[40];
> +
> +	eeprom_client = i2c_new_client_device(port->i2c_adap, &mikrobus_eeprom_info);
> +	if (!IS_ERR(eeprom_client)) {
> +		pr_info(" mikrobus port %d default eeprom is probed at %02x\n", port->id,
> +									eeprom_client->addr);
> +		snprintf(dev_name, sizeof(dev_name), "%d-%04x0", port->i2c_adap->nr,
> +				 eeprom_client->addr);
> +		eeprom = nvmem_device_get(&eeprom_client->dev, dev_name);
> +		if (IS_ERR(eeprom)) {
> +			pr_err(" mikrobus port %d eeprom nvmem device probe failed\n", port->id);
> +			i2c_unregister_device(eeprom_client);
> +			port->eeprom = NULL;
> +			return 0;
> +		}
> +	} else {
> +		pr_info(" mikrobus port %d default eeprom probe failed\n", port->id);
> +		return 0;
> +	}
> +	port->eeprom = eeprom;
> +	port->eeprom_client = eeprom_client;
> +	return 0;
> +}
> +
> +int mikrobus_register_port(struct mikrobus_port *port)
> +{
> +	int retval;
> +	int id;
> +
> +	if (WARN_ON(!is_registered))
> +		return -EAGAIN;
> +	id = idr_alloc(&mikrobus_port_idr, port, 0, 0, GFP_KERNEL);
> +	if (id < 0)
> +		return id;
> +	port->id = id;
> +	port->dev.bus = &mikrobus_bus_type;
> +	port->dev.type = &mikrobus_port_type;
> +	strncpy(port->name, "mikrobus-port", sizeof(port->name) - 1);
> +	dev_set_name(&port->dev, "mikrobus-%d", port->id);
> +	pr_info("registering port mikrobus-%d\n ", port->id);
> +	retval = device_register(&port->dev);
> +	if (retval) {
> +		pr_err("port '%d': can't register device (%d)\n", port->id, retval);
> +		put_device(&port->dev);
> +		return retval;
> +	}
> +	retval = class_compat_create_link(mikrobus_port_compat_class, &port->dev,
> +								port->dev.parent);
> +	if (retval)
> +		dev_warn(&port->dev, "failed to create compatibility class link\n");
> +	if (!port->eeprom) {
> +		pr_info("mikrobus port %d eeprom empty probing default eeprom\n", port->id);
> +		retval = mikrobus_port_probe_eeprom(port);
> +	}
> +	if (port->eeprom) {
> +		retval = mikrobus_port_scan_eeprom(port);
> +		if (retval)
> +			dev_warn(&port->dev, "failed to register board from manifest\n");
> +	}
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(mikrobus_register_port);
> +
> +void mikrobus_del_port(struct mikrobus_port *port)
> +{
> +	struct mikrobus_port *found;
> +
> +	found = idr_find(&mikrobus_port_idr, port->id);
> +	if (found != port) {
> +		pr_err("attempting to delete unregistered port [%s]\n", port->name);
> +		return;
> +	}
> +	if (port->board != NULL) {
> +		pr_err("attempting to delete port with registered boards, port [%s]\n",
> +									port->name);
> +		return;
> +	}
> +
> +	if (port->eeprom) {
> +		nvmem_device_put(port->eeprom);
> +		i2c_unregister_device(port->eeprom_client);
> +	}
> +
> +	class_compat_remove_link(mikrobus_port_compat_class, &port->dev,
> +							port->dev.parent);
> +	device_unregister(&port->dev);
> +	idr_remove(&mikrobus_port_idr, port->id);
> +	memset(&port->dev, 0, sizeof(port->dev));
> +}
> +EXPORT_SYMBOL_GPL(mikrobus_del_port);
> +
> +static int __init mikrobus_init(void)
> +{
> +	int retval;
> +
> +	retval = bus_register(&mikrobus_bus_type);
> +	if (retval) {
> +		pr_err("bus_register failed (%d)\n", retval);
> +		return retval;
> +	}
> +	mikrobus_port_compat_class = class_compat_register("mikrobus-port");
> +	if (!mikrobus_port_compat_class) {
> +		pr_err("class_compat register failed (%d)\n", retval);
> +		retval = -ENOMEM;
> +		goto class_err;
> +	}
> +	is_registered = true;
> +	return 0;
> +class_err:
> +	bus_unregister(&mikrobus_bus_type);
> +	idr_destroy(&mikrobus_port_idr);
> +	is_registered = false;
> +	return retval;
> +}
> +subsys_initcall(mikrobus_init);
> +
> +static void __exit mikrobus_exit(void)
> +{
> +	bus_unregister(&mikrobus_bus_type);
> +	class_compat_unregister(mikrobus_port_compat_class);
> +	idr_destroy(&mikrobus_port_idr);
> +}
> +module_exit(mikrobus_exit);
> +
> +MODULE_AUTHOR("Vaishnav M A <vaishnav@beagleboard.org>");
> +MODULE_DESCRIPTION("mikroBUS main module");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/misc/mikrobus/mikrobus_core.h b/drivers/misc/mikrobus/mikrobus_core.h
> new file mode 100644
> index 000000000000..9684d315f564
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_core.h
> @@ -0,0 +1,130 @@
> +/* 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.
> + */
> +
> +#ifndef __MIKROBUS_H
> +#define __MIKROBUS_H
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/spi/spi.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/jump_label.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/device.h>
> +#include <linux/of_device.h>
> +#include <linux/serdev.h>
> +#include <linux/of.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +#define MIKROBUS_VERSION_MAJOR 0x00
> +#define MIKROBUS_VERSION_MINOR 0x02
> +
> +extern struct bus_type mikrobus_bus_type;
> +extern struct device_type mikrobus_port_type;
> +
> +enum mikrobus_property_type {
> +	MIKROBUS_PROPERTY_TYPE_LINK = 0x00,
> +	MIKROBUS_PROPERTY_TYPE_GPIO = 0x01,
> +	MIKROBUS_PROPERTY_TYPE_U8 = 0x02,
> +	MIKROBUS_PROPERTY_TYPE_U16 = 0x03,
> +	MIKROBUS_PROPERTY_TYPE_U32 = 0x04,
> +	MIKROBUS_PROPERTY_TYPE_U64 = 0x05,
> +};
> +
> +enum mikrobus_gpio_pin {
> +	MIKROBUS_GPIO_INVALID = 0x00,
> +	MIKROBUS_GPIO_INT = 0x01,
> +	MIKROBUS_GPIO_RST = 0x02,
> +	MIKROBUS_GPIO_PWM = 0x03,
> +};
> +
> +enum mikrobus_protocol {
> +	MIKROBUS_PROTOCOL_SPI = 0x01,
> +	MIKROBUS_PROTOCOL_I2C = 0x02,
> +	MIKROBUS_PROTOCOL_UART = 0x03,
> +	MIKROBUS_PROTOCOL_SPI_GPIOCS = 0x04,
> +	MIKROBUS_PROTOCOL_I2C_MUX = 0x05
> +};
> +
> +enum mikrobus_gpio_state {
> +	MIKROBUS_GPIO_UNUSED = 0x00,
> +	MIKROBUS_GPIO_INPUT = 0x01,
> +	MIKROBUS_GPIO_OUTPUT_HIGH = 0x02,
> +	MIKROBUS_GPIO_OUTPUT_LOW = 0x03,
> +};
> +
> +struct mikrobus_port_config {
> +	__u8 i2c_adap_nr;
> +	__u8 spi_master_nr;
> +	__u8 serdev_ctlr_nr;
> +	__u8 rst_gpio_nr;
> +	__u8 pwm_gpio_nr;
> +	__u8 int_gpio_nr;
> +} __packed;
> +
> +struct board_device_info {
> +	struct list_head links;
> +	int id;
> +	char *drv_name;
> +	unsigned short protocol;
> +	unsigned short reg;
> +	u32 max_speed_hz;
> +	unsigned int mode;
> +	int irq;
> +	int irq_type;
> +	int cs_gpio;
> +	unsigned short num_gpio_resources;
> +	unsigned short num_properties;
> +	struct property_entry *properties;
> +	struct gpiod_lookup_table *gpio_lookup;
> +	void *dev_client;
> +};
> +
> +struct addon_board_info {
> +	char *name;
> +	unsigned short num_devices;
> +	unsigned short rst_gpio_state;
> +	unsigned short pwm_gpio_state;
> +	unsigned short int_gpio_state;
> +	struct list_head manifest_descs;
> +	struct list_head devices;
> +};
> +
> +struct mikrobus_port {
> +	char name[48];
> +	struct module *owner;
> +	struct device dev;
> +	int id;
> +	struct gpio_desc *pwm_gpio;
> +	struct gpio_desc *int_gpio;
> +	struct gpio_desc *rst_gpio;
> +	struct spi_master *spi_mstr;
> +	struct i2c_adapter *i2c_adap;
> +	struct addon_board_info *board;
> +	struct i2c_client *eeprom_client;
> +	struct nvmem_device *eeprom;
> +};
> +#define to_mikrobus_port(d) container_of(d, struct mikrobus_port, dev)
> +
> +int mikrobus_register_board(struct mikrobus_port *port,
> +							struct addon_board_info *board);
> +void mikrobus_unregister_board(struct mikrobus_port *port,
> +							   struct addon_board_info *board);
> +int mikrobus_register_port_config(struct mikrobus_port_config *cfg);
> +int mikrobus_register_port(struct mikrobus_port *port);
> +void mikrobus_del_port(struct mikrobus_port *port);
> +
> +#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..60ebca560f0d
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_manifest.c
> @@ -0,0 +1,390 @@
> +// 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.
> + */
> +
> +#define pr_fmt(fmt) "mikrobus_manifest: " fmt
> +
> +#include <linux/bits.h>
> +#include <linux/types.h>
> +#include <linux/property.h>
> +#include <linux/greybus/greybus_manifest.h>
> +
> +#include "mikrobus_manifest.h"
> +
> +struct manifest_desc {
> +	struct list_head links;
> +	size_t size;
> +	void *data;
> +	enum greybus_descriptor_type type;
> +};
> +
> +static void release_manifest_descriptor(struct manifest_desc *descriptor)
> +{
> +	list_del(&descriptor->links);
> +	kfree(descriptor);
> +}
> +
> +static void release_manifest_descriptors(struct addon_board_info *info)
> +{
> +	struct manifest_desc *descriptor;
> +	struct manifest_desc *next;
> +
> +	list_for_each_entry_safe(descriptor, next, &info->manifest_descs, links)
> +		release_manifest_descriptor(descriptor);
> +}
> +
> +static int identify_descriptor(struct addon_board_info *info, struct greybus_descriptor *desc,
> +										 size_t size)
> +{
> +	struct greybus_descriptor_header *desc_header = &desc->header;
> +	struct manifest_desc *descriptor;
> +	size_t desc_size;
> +	size_t expected_size;
> +
> +	if (size < sizeof(*desc_header))
> +		return -EINVAL;
> +	desc_size = le16_to_cpu(desc_header->size);
> +	if (desc_size > 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:
> +	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, &info->manifest_descs);
> +	return desc_size;
> +}
> +
> +static char *mikrobus_string_get(struct addon_board_info *info, 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, &info->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 *info)
> +{
> +	struct greybus_descriptor_mikrobus *mikrobus;
> +	struct greybus_descriptor_interface *interface;
> +	struct manifest_desc *descriptor;
> +	bool found = false;
> +
> +	list_for_each_entry(descriptor, &info->manifest_descs, links) {
> +		if (descriptor->type == GREYBUS_TYPE_MIKROBUS) {
> +			mikrobus = descriptor->data;
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (found) {
> +		info->num_devices = mikrobus->num_devices;
> +		info->rst_gpio_state = mikrobus->rst_gpio_state;
> +		info->pwm_gpio_state = mikrobus->pwm_gpio_state;
> +		info->int_gpio_state = mikrobus->int_gpio_state;
> +	} else {
> +		info->num_devices = 1;
> +		info->rst_gpio_state = MIKROBUS_GPIO_UNUSED;
> +		info->pwm_gpio_state = MIKROBUS_GPIO_UNUSED;
> +		info->int_gpio_state = MIKROBUS_GPIO_UNUSED;
> +	}
> +
> +	list_for_each_entry(descriptor, &info->manifest_descs, links) {
> +		if (descriptor->type == GREYBUS_TYPE_INTERFACE) {
> +			interface = descriptor->data;
> +			break;
> +		}
> +	}
> +	info->name = mikrobus_string_get(info, interface->product_stringid);
> +}
> +
> +static struct property_entry *
> +mikrobus_property_entry_get(struct addon_board_info *info, u8 *prop_link,
> +							int num_properties)
> +{
> +	struct greybus_descriptor_property *desc_property;
> +	struct manifest_desc *descriptor;
> +	struct property_entry *properties;
> +	int i;
> +	char *prop_name;
> +	bool found = false;
> +	u8 *val_u8;
> +	u16 *val_u16;
> +	u32 *val_u32;
> +	u64 *val_u64;
> +
> +	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, &info->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)
> +			return ERR_PTR(-ENOENT);
> +		prop_name = mikrobus_string_get(info, desc_property->propname_stringid);
> +		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:
> +		return ERR_PTR(-EINVAL);
> +		}
> +	}
> +	return properties;
> +}
> +
> +static u8 *mikrobus_property_link_get(struct addon_board_info *info, u8 prop_id,
> +									  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, &info->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);
> +	return val_u8;
> +}
> +
> +static int mikrobus_manifest_attach_device(struct addon_board_info *info,
> +						struct greybus_descriptor_device *dev_desc)
> +{
> +	struct board_device_info *dev;
> +	struct gpiod_lookup_table *lookup;
> +	struct greybus_descriptor_property *desc_property;
> +	struct manifest_desc *descriptor;
> +	int i;
> +	u8 *prop_link;
> +	u8 *gpio_desc_link;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +	dev->id = dev_desc->id;
> +	dev->drv_name = mikrobus_string_get(info, dev_desc->driver_stringid);
> +	dev->protocol = dev_desc->protocol;
> +	dev->reg = dev_desc->reg;
> +	dev->irq = dev_desc->irq;
> +	dev->irq_type = dev_desc->irq_type;
> +	dev->max_speed_hz = le32_to_cpu(dev_desc->max_speed_hz);
> +	dev->mode = dev_desc->mode;
> +	dev->cs_gpio = dev_desc->cs_gpio;
> +	dev->num_gpio_resources = dev_desc->num_gpio_resources;
> +	dev->num_properties = dev_desc->num_properties;
> +	pr_info("device %d , number of properties=%d , number of gpio resources=%d\n",
> +	dev->id, dev->num_properties, dev->num_gpio_resources);
> +	if (dev->num_properties > 0) {
> +		prop_link = mikrobus_property_link_get(info, dev_desc->prop_link,
> +								MIKROBUS_PROPERTY_TYPE_LINK);
> +		dev->properties = mikrobus_property_entry_get(info, prop_link, dev->num_properties);
> +	}
> +	if (dev->num_gpio_resources > 0) {
> +		lookup = kzalloc(struct_size(lookup, table, dev->num_gpio_resources),
> +					GFP_KERNEL);
> +	if (!lookup)
> +		return -ENOMEM;
> +	gpio_desc_link = mikrobus_property_link_get(info, dev_desc->gpio_link,
> +						MIKROBUS_PROPERTY_TYPE_GPIO);
> +	for (i = 0; i < dev->num_gpio_resources; i++) {
> +		list_for_each_entry(descriptor, &info->manifest_descs, links) {
> +			if (descriptor->type != GREYBUS_TYPE_PROPERTY)
> +				continue;
> +			desc_property = descriptor->data;
> +			if (desc_property->id == gpio_desc_link[i]) {
> +				lookup->table[i].chip_hwnum = *desc_property->value;
> +				lookup->table[i].con_id =
> +				mikrobus_string_get(info, desc_property->propname_stringid);
> +				break;
> +				}
> +		}
> +	}
> +	dev->gpio_lookup = lookup;
> +	}
> +	list_add_tail(&dev->links, &info->devices);
> +	return 0;
> +}
> +
> +static int mikrobus_manifest_parse_devices(struct addon_board_info *info)
> +{
> +	struct greybus_descriptor_device *desc_device;
> +	struct manifest_desc *desc, *next;
> +	int devcount = 0;
> +
> +	if (WARN_ON(!list_empty(&info->devices)))
> +		return false;
> +	list_for_each_entry_safe(desc, next, &info->manifest_descs, links) {
> +		if (desc->type != GREYBUS_TYPE_DEVICE)
> +			continue;
> +		desc_device = desc->data;
> +		mikrobus_manifest_attach_device(info, desc_device);
> +		devcount++;
> +	}
> +	return devcount;
> +}
> +
> +bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> +							 size_t size)
> +{
> +	struct greybus_manifest *manifest;
> +	struct greybus_manifest_header *header;
> +	struct greybus_descriptor *desc;
> +	u16 manifest_size;
> +	int dev_count;
> +	int desc_size;
> +
> +	if (WARN_ON(!list_empty(&info->manifest_descs)))
> +		return false;
> +	if (size < sizeof(*header))
> +		return false;
> +	manifest = data;
> +	header = &manifest->header;
> +	manifest_size = le16_to_cpu(header->size);
> +	if (manifest_size != size)
> +		return false;
> +	if (header->version_major > MIKROBUS_VERSION_MAJOR)
> +		return false;
> +	desc = manifest->descriptors;
> +	size -= sizeof(*header);
> +	while (size) {
> +		desc_size = identify_descriptor(info, desc, size);
> +		if (desc_size < 0) {
> +			pr_err("invalid manifest descriptor");
> +		return -EINVAL;
> +		}
> +		desc = (struct greybus_descriptor *)((char *)desc + desc_size);
> +		size -= desc_size;
> +	}
> +	mikrobus_state_get(info);
> +	dev_count = mikrobus_manifest_parse_devices(info);
> +	pr_info(" %s manifest parsed with %d device(s)\n", info->name,
> +		info->num_devices);
> +	release_manifest_descriptors(info);
> +	return true;
> +}
> +
> +size_t mikrobus_manifest_header_validate(void *data, size_t size)
> +{
> +	struct greybus_manifest_header *header;
> +	u16 manifest_size;
> +
> +	if (size < sizeof(*header))
> +		return 0;
> +
> +	header = data;
> +	manifest_size = le16_to_cpu(header->size);
> +	if (header->version_major > MIKROBUS_VERSION_MAJOR)
> +		return 0;
> +	return manifest_size;
> +}
> diff --git a/drivers/misc/mikrobus/mikrobus_manifest.h b/drivers/misc/mikrobus/mikrobus_manifest.h
> new file mode 100644
> index 000000000000..a195d1c26493
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_manifest.h
> @@ -0,0 +1,21 @@
> +/* 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"
> +
> +bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> +							 size_t size);
> +size_t mikrobus_manifest_header_validate(void *data, size_t size);
> +
> +#endif /* __MIKROBUS_MANIFEST_H */
> diff --git a/include/linux/greybus/greybus_manifest.h b/include/linux/greybus/greybus_manifest.h
> index 6e62fe478712..79c8cab9ef96 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,53 @@ struct greybus_descriptor_cport {
>  	__u8	protocol_id;	/* enum greybus_protocol */
>  } __packed;
>  
> +/*
> + * A mikrobus descriptor is used to describe the details
> + * about the add-on board connected to the mikrobus port.
> + * It describes the number of indivdual devices on the
> + * add-on board and the default states of the GPIOs
> + */
> +struct greybus_descriptor_mikrobus {
> +	__u8 num_devices;
> +	__u8 rst_gpio_state;
> +	__u8 pwm_gpio_state;
> +	__u8 int_gpio_state;
> +} __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[0];
> +} __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 num_properties;
> +	__u8 protocol;
> +	__le32 max_speed_hz;
> +	__u8 reg;
> +	__u8 mode;
> +	__u8 num_gpio_resources;
> +	__u8 cs_gpio;
> +	__u8 irq;
> +	__u8 irq_type;
> +	__u8 prop_link;
> +	__u8 gpio_link;
> +} __packed;
> +
>  struct greybus_descriptor_header {
>  	__le16	size;
>  	__u8	type;		/* enum greybus_descriptor_type */
> @@ -164,6 +214,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;
>  
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [greybus-dev] [PATCH] RFC : mikroBUS driver for add-on boards
  2020-07-24 12:06 [PATCH] RFC : mikroBUS driver for add-on boards Vaishnav M A
  2020-07-24 20:02 ` Christopher Friedt
  2020-07-26 12:48 ` [greybus-dev] " Alex Elder
@ 2020-07-26 15:04 ` Alex Elder
  2020-07-26 19:12   ` Vaishnav M A
  2 siblings, 1 reply; 7+ messages in thread
From: Alex Elder @ 2020-07-26 15:04 UTC (permalink / raw)
  To: Vaishnav M A, greybus-dev, linux-kernel, gregkh, arnd, johan, elder
  Cc: rajkovic, robh, mchehab+huawei, robertcnelson, drew, davem,
	zoran.stojsavljevic

On 7/24/20 7:06 AM, Vaishnav M A wrote:
> Attached is a patch for the mikroBUS driver which helps to
> instantiate an add-on board device on a mikrobus port by fetching
> the device identifier manifest binary from an EEPROM on-board
> the device. mikroBUS is an add-on board socket standard by
> MikroElektronika that can be freely used by anyone
> following the guidelines, more details and discussions on
> the status of the driver can be found here in this eLinux wiki:
> https://elinux.org/Mikrobus

I had some other things I would comment on in this code review,
but there are a lot of somewhat superficial things you should
fix.  I looked at much--but not all--of this, and I'll want to
review this again after you've fixed the simple things.  I also
can provide more substantive feedback after I've had more time
to digest the bigger picture of microBUS.

You should assume it will take a few iterations to get to the
point where things are shaping up for acceptance.

All that said, your basic foundation looks good.

> In the current state of the driver, more than 80 different
> add-on boards have been tested on the BeagleBoard.org
> PocketBeagle and the manifest binary is generated using the same
> manifesto tool used to generate Greybus manifest binaries,
> The pull request to manifesto to add new descriptors specific
> to mikrobus is here : https://github.com/projectara/manifesto/pull/2
> The utilization of Greybus manifest binaries here is not entirely
> coincidental, We are evaluating ways to add mikroBUS sockets and
> devices via Greybus expansion.
> 
> The mikroBUS standard includes SPI, I2C, UART, PWM, ADC, GPIO
> and power (3.3V and 5V) connections to interface common embedded
> peripherals, There are more than 750 add-on boards ranging from
> wireless connectivity boards to human-machine interface sensors
> which conform to the mikroBUS standard, out of which more than 140
> boards already have support in the Linux kernel.Today, there is no
> mainlinesolution for enabling mikroBUS add-on boards at run-time, the
> most straight forward method for loading drivers is to provide
> device-tree overlay fragments at boot time, this method suffers
> from the need to maintain a large out-of-tree database for which there
> is need to maintain an overlay for every mikroBUS add-on board for every
> Linux system and for every mikroBUS socket on that system.
> 
> The mikroBUS driver tries to solve the problem by using extended version
> of the greybus manifest to describe the add-on board device specific
> information required by the device driver and uses it along with the fixed
> port specific information to probe the specific device driver.The manifest
> binary is now fetched from an I2C EEPROM over the I2C bus on the mikroBUS
> port(subject to change in mikroBUS v3 specification) and enumerate drivers
> for the add-on devices.There is also ongoing work to define a mikroBUS
> v3 standard in which the addon board includes a non-volatile storage to
> store the device identifier manifest binary, once the mikroBUS v3 standard
> is released, the mikroBUS can be seen as a probeable bus such that the
> kernel can discover the device on the bus at boot time.
> 
> The driver also has a few debug SysFS interfaces for testing on add-on
> boards without an EEPROM, these can be used in the following manner:
> (example for mikroBUS port 1 on BeagleBoard.org PocketBeagle):

You should probably use debugfs, since this is a temporary thing.
But I guess if it's not actually upstream (and it sounds like you
might be avoiding the need for this with EEPROM anyway) maybe
that doesn't matter much.

> printf "%b" '\x01\x00\x00\x59\x32\x17' > /sys/bus/mikrobus/add_port
> 
> The bytes in the byte array sequence are (in order):
> 
> 	* i2c_adap_nr
> 	* spi_master_nr
> 	* serdev_ctlr_nr
> 	* rst_gpio_nr
> 	* pwm_gpio_nr
> 	* int_gpio_nr
> * add_port sysFS entry is purely for debug and in the actual state
> of the driver, port configuration will be loaded from a suitable
> device tree overlay fragment.
> 
> echo 0 > /sys/bus/mikrobus/del_port (to delete the attached port)
> echo 1 >  /sys/class/mikrobus-port/mikrobus-0/rescan (to rescan the EEPROM
> contents on the I2C bus on the mikroBUS port).
> 
> cat board_manifest.mnfb >  /sys/class/mikrobus-port/mikrobus-0/new_device
> * debug interface to pass the manifest binary in case an EEPROM is absent
> echo 0 >  /sys/class/mikrobus-port/mikrobus-0/delete_device
> * to unload the loaded board on the mikrobus port
> 
> These sysFS interfaces are only implemented for debug purposes and
> in the actual implementation of the driver these will be removed and
> the manifest binary will be fetched from the non volatile storage on-board
> the device.
> 
> The mikroBUS driver enable the mikroBUS to be a probeable bus such that
> the kernel can discover the device and automatically load the drivers.
> There are already several Linux platforms with mikroBUS sockets and the
> mikroBUS driver helps to reduce the time to develop and debug
> support for various mikroBUS add-on boards. Further, it opens up
> the possibility for support under dynamically instantiated buses
> such as with Greybus.
> 
> Please let know the feedback you have on this patch or the approach used.
> 
> Thanks,
> 
> Vaishnav M A
> 
> Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org>
> ---
>  MAINTAINERS                               |   6 +
>  drivers/misc/Kconfig                      |   1 +
>  drivers/misc/Makefile                     |   1 +
>  drivers/misc/mikrobus/Kconfig             |  16 +
>  drivers/misc/mikrobus/Makefile            |   5 +
>  drivers/misc/mikrobus/mikrobus_core.c     | 649 ++++++++++++++++++++++
>  drivers/misc/mikrobus/mikrobus_core.h     | 130 +++++
>  drivers/misc/mikrobus/mikrobus_manifest.c | 390 +++++++++++++
>  drivers/misc/mikrobus/mikrobus_manifest.h |  21 +
>  include/linux/greybus/greybus_manifest.h  |  53 ++
>  10 files changed, 1272 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 d53db30d1365..9a049746203f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11402,6 +11402,12 @@ M:	Oliver Neukum <oliver@neukum.org>
>  S:	Maintained
>  F:	drivers/usb/image/microtek.*
>  
> +MIKROBUS ADDON BOARD DRIVER
> +M:	Vaishnav M A <vaishnav@beagleboard.org>
> +S:	Maintained
> +W:	https://elinux.org/Mikrobus
> +F:	drivers/misc/mikrobus/
> +
>  MIPS
>  M:	Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>  L:	linux-mips@vger.kernel.org
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index e1b1ba5e2b92..334f0c39d56b 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -472,4 +472,5 @@ source "drivers/misc/ocxl/Kconfig"
>  source "drivers/misc/cardreader/Kconfig"
>  source "drivers/misc/habanalabs/Kconfig"
>  source "drivers/misc/uacce/Kconfig"
> +source "drivers/misc/mikrobus/Kconfig"
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index c7bd01ac6291..45486dd77da5 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_VMWARE_BALLOON)	+= vmw_balloon.o
>  obj-$(CONFIG_PCH_PHUB)		+= pch_phub.o
>  obj-y				+= ti-st/
>  obj-y				+= lis3lv02d/
> +obj-y				+= mikrobus/
>  obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
>  obj-$(CONFIG_INTEL_MEI)		+= mei/
>  obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
> diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig
> new file mode 100644
> index 000000000000..c3b93e12daad
> --- /dev/null
> +++ b/drivers/misc/mikrobus/Kconfig
> @@ -0,0 +1,16 @@
> +menuconfig MIKROBUS
> +	tristate "Module for instantiating devices on mikroBUS ports"
> +	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 helps in
> +	  instantiating devices on the mikroBUS port with identifier
> +	  data fetched from an EEPROM on the device, more details on
> +	  the mikroBUS driver support and discussion can be found in
> +	  this eLinux wiki : elinux.org/Mikrobus

This text could be cleaned up a bit.  For example:
	The mikroBUS driver instantiates devices on a mikroBUS port
	described by identifying data present in device-resident EEPROM.

Using well-defined terms can help a lot.  Is a physical thing that
plugs into a microbus port called a "board"?  Can a "board" present
more than one device to the system?  Is the EEPROM associated with
the board, or a device?  My point isn't that those answers belong
here, but that having meaningful terms can help you describe things
concisely.

> +	  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..1f80ed4064d8
> --- /dev/null
> +++ b/drivers/misc/mikrobus/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# mikroBUS Core
> +
> +mikrobus-y :=	mikrobus_core.o	mikrobus_manifest.o
> +obj-$(CONFIG_MIKROBUS) += mikrobus.o
> \ No newline at end of file
> diff --git a/drivers/misc/mikrobus/mikrobus_core.c b/drivers/misc/mikrobus/mikrobus_core.c
> new file mode 100644
> index 000000000000..78c96c637632
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_core.c
> @@ -0,0 +1,649 @@
> +// 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.
> + */
> +
> +#define pr_fmt(fmt) "mikrobus: " fmt
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/jump_label.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mutex.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/interrupt.h>
> +#include <linux/spi/spi.h>
> +#include <linux/serdev.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +#include "mikrobus_core.h"
> +#include "mikrobus_manifest.h"
> +
> +#define ATMEL_24C32_I2C_ADDR 0x57

I'm just getting started looking through this, so maybe I'll find
out soon. But why is this ATMEL I2C address needed in the core code?

> +static DEFINE_IDR(mikrobus_port_idr);
> +static struct class_compat *mikrobus_port_compat_class;
> +static bool is_registered;
> +
> +static ssize_t add_port_store(struct bus_type *bt, const char *buf,
> +			      size_t count)
> +{
> +	struct mikrobus_port_config *cfg;
> +
> +	if (count < sizeof(*cfg)) {

Perhaps this should be:
	if (count != sizeof(*cfg)) {

> +		pr_err("add_port: incorrect config data received: %s\n", buf);

I don't think you need to include "add_port: ".

This is binary data you are writing to this sysfs file, correct?
Don't try to interpret it as a string.  You could avoid the problem
with something more like:
    pr_err("bad port config size (%zu, should be %zu)", count, sizeof(*cfg));

> +		return -EINVAL;
> +	}
> +	mikrobus_register_port_config((void *)buf);

mikrobus_register_port_config() returns an error value, and
that should be returned from this function if it's non-zero.

Don't cast the buffer to a void pointer; cast it to the actual
type represents (struct mikrobus_port_config *).

> +	return count;
> +}
> +BUS_ATTR_WO(add_port);
> +
> +static ssize_t del_port_store(struct bus_type *bt, const char *buf,
> +							  size_t count)
> +{
> +	int id;
> +	char end;
> +	int res;
> +
> +	res = sscanf(buf, "%d%c", &id, &end);

The id value you pass to idr_find() is an unsigned long.
You might as well define "id" with that type and scan that
type here.  Make sure it's in the valid range for your
identifier as a separate step.  (There might be a good
reason you use a signed int as an identifier, but I would
recommend unsigned, with a well-defined number of bits,
such as u32).

Is "end" intended to just consume an optional trailing newline?

> +	if (res < 1) {
> +		pr_err("delete_port: cannot parse mikrobus port ID\n");

I don't think "delete_port: " here is necessary.  (Take this
comment to apply in all similar cases; I won't mention it
again.)

> +		return -EINVAL;
> +	}
> +	if (!idr_find(&mikrobus_port_idr, id)) {
> +		pr_err("attempting to delete unregistered port [%d]\n", id);

Maybe just "mikrobus port %lu not registered".

> +		return -EINVAL;
> +	}
> +	mikrobus_del_port(idr_find(&mikrobus_port_idr, id));
> +	return count;
> +}
> +BUS_ATTR_WO(del_port);
> +
> +static struct attribute *mikrobus_attrs[] = {
> +	&bus_attr_add_port.attr,
> +	&bus_attr_del_port.attr,
> +	 NULL
> +};
> +ATTRIBUTE_GROUPS(mikrobus);
> +
> +struct bus_type mikrobus_bus_type = {
> +	.name = "mikrobus",
> +	.bus_groups = mikrobus_groups,
> +};
> +EXPORT_SYMBOL_GPL(mikrobus_bus_type);
> +
> +static int mikrobus_manifest_header_validate(struct mikrobus_port *port)
> +{
> +	char header[12];
> +	struct addon_board_info *board;
> +	int manifest_size;
> +	int retval;
> +	char *buf;
> +
> +	nvmem_device_read(port->eeprom, 0, 12, header);

This function returns a value, and if it's less than zero you
should return it as the result of mikrobus_manifest_header_validate()
(possibly after reporting an error).

> +	manifest_size = mikrobus_manifest_header_validate(header, 12);
> +	if (manifest_size > 0) {
> +		buf = kzalloc(manifest_size, GFP_KERNEL);

Check whether kzalloc() returns NULL, and if so, return -ENOMEM.

> +		nvmem_device_read(port->eeprom, 0, manifest_size, buf);

Check the return value here, and if negative, free your buffer
and return the error.  (I won't make this comment any more if
I see other instances of ignoring the nvmem_device_read()
return value.)

> +		board = kzalloc(sizeof(*board), GFP_KERNEL);
> +		if (!board)
> +			return -ENOMEM;

You need to free the buffer you allocated before you return here.
It is helpful to use the "goto <error path>" pattern.  I.e.:

	if (!board) {
		retval = -ENOMEM;
		goto err_free_buf;
	}
...

err_free_buf:
	kfree(buf);

	return retval;
}

> +		INIT_LIST_HEAD(&board->manifest_descs);
> +		INIT_LIST_HEAD(&board->devices);
> +		retval = mikrobus_manifest_parse(board, (void *)buf, manifest_size);

No need to cast buf to (void *).

I have more comments on mikrobus_manifest_parse() below.  But it
might be useful to have it return an integer (0 or error code)
rather than Boolean.  Assuming you did that...

> +		if (!retval) {
> +			pr_err("failed to parse manifest, size: %d", manifest_size);

	if (retval)
		goto err_free_board;
...

err_free_board:
	free_board(board);
err_free_buf:
	free_buf(buf);

	return retval;
}

> +			return -EINVAL;
> +		}
> +		retval = mikrobus_register_board(port, board);
> +		if (retval) {
> +			pr_err("failed to register board: %s", board->name);

	goto err_free_board;

> +			return -EINVAL;
> +		}
> +		kfree(buf);
> +	} else {
> +		pr_err("inavlide manifest port %d", port->id);

s/inavlide/invalid/

> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> +						 char *buf)
> +{
> +	return sprintf(buf, "%s\n", to_mikrobus_port(dev)->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +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 retval;
> +
> +	if (port->board == NULL) {

This is just a style suggestion, but I would prefer this, because it
reduces the indentation of the normal path:

	if (port->board) {
		pr_err("mikrobus port %d already has a board registered\n",
			port->id);
		return -EBUSY;
	}

	port->board = kzalloc(...);
	if (!port->board)
		return -ENOMEM;

Also note the return values I suggest here.

> +		board = kzalloc(sizeof(*board), GFP_KERNEL);
> +		if (!board)
> +			return -EINVAL;
> +		INIT_LIST_HEAD(&board->manifest_descs);
> +		INIT_LIST_HEAD(&board->devices);
> +	} else {
> +		pr_err("port %d already has board registered", port->id);
> +		return -EINVAL;
> +	}
> +	retval = mikrobus_manifest_parse(board, (void *)buf, count);
> +	if (!retval) {
> +		pr_err("failed to parse manifest");
> +		return -EINVAL;
> +	}
> +	retval = mikrobus_register_board(port, board);
> +	if (retval) {
> +		pr_err("failed to register board: %s", board->name);
> +		return -EINVAL;
> +	}
> +	return count;
> +}
> +static DEVICE_ATTR_WO(new_device);
> +
> +static ssize_t rescan_store(struct device *dev, struct device_attribute *attr,
> +							const char *buf, size_t count)
> +{
> +	struct mikrobus_port *port = to_mikrobus_port(dev);
> +	int id;
> +	char end;
> +	int res;
> +	int retval;
> +
> +	res = sscanf(buf, "%d%c", &id, &end);
> +	if (res < 1) {
> +		pr_err("rescan: Can't parse trigger\n");
> +		return -EINVAL;
> +	}
> +	if (port->board != NULL) {
> +		pr_err("port %d already has board registered", port->id);
> +		return -EINVAL;
> +	}
> +	retval = mikrobus_port_scan_eeprom(port);
> +	if (retval) {
> +		pr_err("port %d board register from manifest failed", port->id);
> +		return -EINVAL;
> +	}
> +	return count;
> +}
> +static DEVICE_ATTR_WO(rescan);
> +
> +static ssize_t delete_device_store(struct device *dev, struct device_attribute *attr,
> +							const char *buf, size_t count)
> +{
> +	int id;
> +	char end;
> +	int res;
> +	struct mikrobus_port *port = to_mikrobus_port(dev);
> +
> +	res = sscanf(buf, "%d%c", &id, &end);
> +	if (res < 1) {
> +		pr_err("delete_board: Can't parse board ID\n");
> +		return -EINVAL;
> +	}
> +	if (port->board == NULL) {

Normally in kernel code this form is used:

	if (!port->board) {

> +		pr_err("delete_board: port does not have any boards registered\n");
> +		return -EINVAL;
> +	}
> +	mikrobus_unregister_board(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_rescan.attr,
> +	&dev_attr_delete_device.attr, &dev_attr_name.attr, NULL};
> +ATTRIBUTE_GROUPS(mikrobus_port);
> +
> +static void mikrobus_dev_release(struct device *dev)
> +{
> +	struct mikrobus_port *port = to_mikrobus_port(dev);
> +
> +	idr_remove(&mikrobus_port_idr, port->id);
> +	kfree(port);
> +}
> +
> +struct device_type mikrobus_port_type = {
> +	.groups = mikrobus_port_groups,
> +	.release = mikrobus_dev_release,
> +};
> +EXPORT_SYMBOL_GPL(mikrobus_port_type);
> +
> +static int mikrobus_get_irq(struct mikrobus_port *port, int irqno,
> +							int irq_type)
> +{
> +	int irq;
> +
> +	switch (irqno) {
> +	case MIKROBUS_GPIO_INT:
> +	irq = gpiod_to_irq(port->int_gpio);
> +	break;

Please fix your indentation here.  (And everywhere; I give
an example of the proper way to do it below.)

> +	case MIKROBUS_GPIO_RST:
> +	irq = gpiod_to_irq(port->rst_gpio);
> +	break;
> +	case MIKROBUS_GPIO_PWM:
> +	irq = gpiod_to_irq(port->pwm_gpio);
> +	break;
> +	default:
> +	return -EINVAL;
> +	}
> +	if (irq < 0) {
> +		pr_err("Could not get irq for irq type: %d", irqno);
> +		return -EINVAL;
> +	}
> +	irq_set_irq_type(irq, irq_type);

It shouldn't return an error, but please check the
return value here.

> +	return irq;
> +}
> +
> +static int mikrobus_setup_gpio(struct gpio_desc *gpio, int gpio_state)
> +{
> +	int retval;
> +
> +	if (gpio_state == MIKROBUS_GPIO_UNUSED)
> +		return 0;
> +	switch (gpio_state) {
> +	case MIKROBUS_GPIO_INPUT:
> +	retval = gpiod_direction_input(gpio);
> +	break;
> +	case MIKROBUS_GPIO_OUTPUT_HIGH:
> +	retval = gpiod_direction_output(gpio, 1);
> +	gpiod_set_value_cansleep(gpio, 1);
> +	break;
> +	case MIKROBUS_GPIO_OUTPUT_LOW:
> +	retval = gpiod_direction_output(gpio, 0);
> +	gpiod_set_value_cansleep(gpio, 0);
> +	break;
> +	default:
> +	return -EINVAL;
> +	}
> +	return retval;
> +}
> +
> +static void mikrobus_spi_device_delete(struct spi_master *master, unsigned int cs)
> +{
> +	struct device *dev;
> +	char str[32];
> +
> +	snprintf(str, sizeof(str), "%s.%u", dev_name(&master->dev), cs);
> +	dev = bus_find_device_by_name(&spi_bus_type, NULL, str);
> +	if (dev != NULL) {
> +		spi_unregister_device(to_spi_device(dev));
> +		put_device(dev);
> +	}
> +}
> +
> +static char *mikrobus_get_gpio_chip_name(struct mikrobus_port *port, int gpio)
> +{
> +	char *name;
> +	struct gpio_chip *gpiochip;
> +
> +	switch (gpio) {
> +	case MIKROBUS_GPIO_INT:
> +	gpiochip = gpiod_to_chip(port->int_gpio);
> +	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);

Why 40?  Please use a symbolic constant so you can
change it easily, and to give you a place to explain
why 40 is the limit used.

> +	break;
> +	case MIKROBUS_GPIO_RST:
> +	gpiochip = gpiod_to_chip(port->rst_gpio);
> +	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> +	break;
> +	case MIKROBUS_GPIO_PWM:
> +	gpiochip = gpiod_to_chip(port->pwm_gpio);
> +	name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> +	break;
> +	}
> +	return name;
> +}
> +
> +static int mikrobus_get_gpio_hwnum(struct mikrobus_port *port, int gpio)
> +{
> +	int hwnum;
> +	struct gpio_chip *gpiochip;
> +
> +	switch (gpio) {
> +	case MIKROBUS_GPIO_INT:
> +	gpiochip = gpiod_to_chip(port->int_gpio);
> +	hwnum = desc_to_gpio(port->int_gpio) - gpiochip->base;
> +	break;
> +	case MIKROBUS_GPIO_RST:
> +	gpiochip = gpiod_to_chip(port->rst_gpio);
> +	hwnum = desc_to_gpio(port->rst_gpio) - gpiochip->base;
> +	break;
> +	case MIKROBUS_GPIO_PWM:
> +	gpiochip = gpiod_to_chip(port->pwm_gpio);
> +	hwnum = desc_to_gpio(port->pwm_gpio) - gpiochip->base;
> +	break;
> +	}
> +	return hwnum;
> +}
> +
> +static void release_mikrobus_device(struct board_device_info *dev)

We tend to follow a convention throughout the Greybus code
that has the object name as the prefix for things you do
to the object.  I don't know how consistent that is, but
my suggestion would be that these functions would be named
something more like:
    mikrobus_gpio_chip_name_get()
    mikrobus_gpio_hwnum_get()
    mikrobus_board_release_device_all()
    mikrobus_device_register()
    mikrobus_device_unregister()
    mikrobus_board_register()
    mikrobus_board_unregister()
and so on.

> +{
> +	list_del(&dev->links);
> +	kfree(dev);
> +}
> +

(I skipped reviewing a lot here...)
. . .

> diff --git a/drivers/misc/mikrobus/mikrobus_core.h b/drivers/misc/mikrobus/mikrobus_core.h
> new file mode 100644
> index 000000000000..9684d315f564
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_core
. . .

> diff --git a/drivers/misc/mikrobus/mikrobus_manifest.c b/drivers/misc/mikrobus/mikrobus_manifest.c
> new file mode 100644
> index 000000000000..60ebca560f0d
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_manifest.c
> @@ -0,0 +1,390 @@
> +// 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.
> + */
> +
> +#define pr_fmt(fmt) "mikrobus_manifest: " fmt
> +
> +#include <linux/bits.h>
> +#include <linux/types.h>
> +#include <linux/property.h>
> +#include <linux/greybus/greybus_manifest.h>
> +
> +#include "mikrobus_manifest.h"
> +
> +struct manifest_desc {
> +	struct list_head links;
> +	size_t size;
> +	void *data;
> +	enum greybus_descriptor_type type;
> +};
> +
> +static void release_manifest_descriptor(struct manifest_desc *descriptor)
> +{
> +	list_del(&descriptor->links);
> +	kfree(descriptor);
> +}
> +
> +static void release_manifest_descriptors(struct addon_board_info *info)
> +{
> +	struct manifest_desc *descriptor;
> +	struct manifest_desc *next;
> +
> +	list_for_each_entry_safe(descriptor, next, &info->manifest_descs, links)
> +		release_manifest_descriptor(descriptor);
> +}
> +
> +static int identify_descriptor(struct addon_board_info *info, struct greybus_descriptor *desc,
> +										 size_t size)
> +{

I know it's technically acceptable now, but please limit your lines to
80 characters in the Greybus code if possible.

> +	struct greybus_descriptor_header *desc_header = &desc->header;
> +	struct manifest_desc *descriptor;
> +	size_t desc_size;
> +	size_t expected_size;
> +
> +	if (size < sizeof(*desc_header))
> +		return -EINVAL;
> +	desc_size = le16_to_cpu(desc_header->size);
> +	if (desc_size > size)
> +		return -EINVAL;

Probably check if (desc_size != size) here.

> +	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;

Your indentation is wrong here.  Please use:

	switch (desc_header->type) {
	case GREYBUS_TYPE_STRING:
		expected_size += ...;
		...
		break;
	case GREYBUS_TYPE_PROPERTY:
		...

> +	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:
> +	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, &info->manifest_descs);
> +	return desc_size;
> +}

. . .

> +static int mikrobus_manifest_attach_device(struct addon_board_info *info,
> +						struct greybus_descriptor_device *dev_desc)
> +{
> +	struct board_device_info *dev;

I would suggest something other than "dev" as the name of
a board_device.  The use of "dev" for (struct device *)
is pretty prevalent in the kernel, and so using it here
can be more confusing than it has to be.

> +	struct gpiod_lookup_table *lookup;
> +	struct greybus_descriptor_property *desc_property;
> +	struct manifest_desc *descriptor;
> +	int i;
> +	u8 *prop_link;
> +	u8 *gpio_desc_link;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +	dev->id = dev_desc->id;
> +	dev->drv_name = mikrobus_string_get(info, dev_desc->driver_stringid);

This can return NULL.  You need to check for that, and free
the board device you have already allocated.

> +	dev->protocol = dev_desc->protocol;
> +	dev->reg = dev_desc->reg;
> +	dev->irq = dev_desc->irq;
> +	dev->irq_type = dev_desc->irq_type;
> +	dev->max_speed_hz = le32_to_cpu(dev_desc->max_speed_hz);
> +	dev->mode = dev_desc->mode;
> +	dev->cs_gpio = dev_desc->cs_gpio;
> +	dev->num_gpio_resources = dev_desc->num_gpio_resources;
> +	dev->num_properties = dev_desc->num_properties;
> +	pr_info("device %d , number of properties=%d , number of gpio resources=%d\n",
> +	dev->id, dev->num_properties, dev->num_gpio_resources);
> +	if (dev->num_properties > 0) {
> +		prop_link = mikrobus_property_link_get(info, dev_desc->prop_link,
> +								MIKROBUS_PROPERTY_TYPE_LINK);
> +		dev->properties = mikrobus_property_entry_get(info, prop_link, dev->num_properties);
> +	}
> +	if (dev->num_gpio_resources > 0) {
> +		lookup = kzalloc(struct_size(lookup, table, dev->num_gpio_resources),
> +					GFP_KERNEL);
> +	if (!lookup)
> +		return -ENOMEM;

You can't return without freeing your previously-allocated resources.

> +	gpio_desc_link = mikrobus_property_link_get(info, dev_desc->gpio_link,
> +						MIKROBUS_PROPERTY_TYPE_GPIO);
> +	for (i = 0; i < dev->num_gpio_resources; i++) {
> +		list_for_each_entry(descriptor, &info->manifest_descs, links) {
> +			if (descriptor->type != GREYBUS_TYPE_PROPERTY)
> +				continue;
> +			desc_property = descriptor->data;
> +			if (desc_property->id == gpio_desc_link[i]) {
> +				lookup->table[i].chip_hwnum = *desc_property->value;
> +				lookup->table[i].con_id =
> +				mikrobus_string_get(info, desc_property->propname_stringid);
> +				break;
> +				}
> +		}
> +	}
> +	dev->gpio_lookup = lookup;
> +	}
> +	list_add_tail(&dev->links, &info->devices);
> +	return 0;
> +}
> +
> +static int mikrobus_manifest_parse_devices(struct addon_board_info *info)
> +{
> +	struct greybus_descriptor_device *desc_device;
> +	struct manifest_desc *desc, *next;
> +	int devcount = 0;
> +
> +	if (WARN_ON(!list_empty(&info->devices)))
> +		return false;

The manifest comes from outside the kernel  I might be misunderstanding
something, but you should not be using WARN_ON() if its content doesn't
match what you expect.

> +	list_for_each_entry_safe(desc, next, &info->manifest_descs, links) {
> +		if (desc->type != GREYBUS_TYPE_DEVICE)
> +			continue;
> +		desc_device = desc->data;
> +		mikrobus_manifest_attach_device(info, desc_device);

You are ignoring the return value of mikrobus_manifest_attach_device().

> +		devcount++;
> +	}
> +	return devcount;
> +}
> +
> +bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> +							 size_t size)

You use "board" as the name of a "board_info" variable elsewhere.
That is much more helpful than "info".  Please use a consistent
naming convention for your variables of given types if possible.
It makes it easier to follow the code.

> +{
> +	struct greybus_manifest *manifest;
> +	struct greybus_manifest_header *header;
> +	struct greybus_descriptor *desc;
> +	u16 manifest_size;
> +	int dev_count;
> +	int desc_size;
> +

Check the size before you bother checking anything else.

> +	if (WARN_ON(!list_empty(&info->manifest_descs)))
> +		return false;
> +	if (size < sizeof(*header))
> +		return false;
> +	manifest = data;
> +	header = &manifest->header;
> +	manifest_size = le16_to_cpu(header->size);
> +	if (manifest_size != size)
> +		return false;

It would be helpful to report what the problem with the
manifest is (here and in all cases).  Otherwise it's a
little mysterious why adding a board fails.

> +	if (header->version_major > MIKROBUS_VERSION_MAJOR)
> +		return false;

Version checks!!!  This is good!  But the topic is of great
interest to Greybus people!  Not sure we ever completely
resolved that.  That's my only comment on this for now.

> +	desc = manifest->descriptors;
> +	size -= sizeof(*header);

Why aren't you using mikrobus_manifest_header_validate() here?

> +	while (size) {
> +		desc_size = identify_descriptor(info, desc, size);

What you're really doing with identify_descriptor() is adding
a valid descriptor to a board's list of descriptors.  I think
the name of that function shoudl reflect that.  If it isn't
identified, that's an error--but that's not the purpose of
that function.  So maybe:
	desc_ = board_descriptor_add(board, desc, size);

> +		if (desc_size < 0) {
> +			pr_err("invalid manifest descriptor");
> +		return -EINVAL;
Your indentation of the return statement here is wrong.

> +		}
> +		desc = (struct greybus_descriptor *)((char *)desc + desc_size);

I don't know if this is better, but this could be:
		desc = (void *)desc + desc_size;

> +		size -= desc_size;
> +	}
> +	mikrobus_state_get(info);
> +	dev_count = mikrobus_manifest_parse_devices(info);
> +	pr_info(" %s manifest parsed with %d device(s)\n", info->name,
> +		info->num_devices);
> +	release_manifest_descriptors(info);
> +	return true;
> +}
> +
> +size_t mikrobus_manifest_header_validate(void *data, size_t size)
> +{
> +	struct greybus_manifest_header *header;
> +	u16 manifest_size;
> +
> +	if (size < sizeof(*header))
> +		return 0;
> +
> +	header = data;
> +	manifest_size = le16_to_cpu(header->size);
> +	if (header->version_major > MIKROBUS_VERSION_MAJOR)
> +		return 0;
> +	return manifest_size;
> +}
> diff --git a/drivers/misc/mikrobus/mikrobus_manifest.h b/drivers/misc/mikrobus/mikrobus_manifest.h
> new file mode 100644
> index 000000000000..a195d1c26493
> --- /dev/null
> +++ b/drivers/misc/mikrobus/mikrobus_manifest.h
> @@ -0,0 +1,21 @@
> +/* 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"
> +
> +bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> +							 size_t size);
> +size_t mikrobus_manifest_header_validate(void *data, size_t size);
> +
> +#endif /* __MIKROBUS_MANIFEST_H */
> diff --git a/include/linux/greybus/greybus_manifest.h b/include/linux/greybus/greybus_manifest.h
> index 6e62fe478712..79c8cab9ef96 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,

Please align your new values with the rest, for consistency.

>  };
>  
>  enum greybus_protocol {
> @@ -151,6 +154,53 @@ struct greybus_descriptor_cport {
>  	__u8	protocol_id;	/* enum greybus_protocol */
>  } __packed;
>  
> +/*
> + * A mikrobus descriptor is used to describe the details
> + * about the add-on board connected to the mikrobus port.
> + * It describes the number of indivdual devices on the
> + * add-on board and the default states of the GPIOs
> + */
> +struct greybus_descriptor_mikrobus {
> +	__u8 num_devices;
> +	__u8 rst_gpio_state;
> +	__u8 pwm_gpio_state;
> +	__u8 int_gpio_state;
> +} __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[0];
> +} __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 num_properties;
> +	__u8 protocol;
> +	__le32 max_speed_hz;
> +	__u8 reg;
> +	__u8 mode;
> +	__u8 num_gpio_resources;
> +	__u8 cs_gpio;
> +	__u8 irq;
> +	__u8 irq_type;
> +	__u8 prop_link;
> +	__u8 gpio_link;
> +} __packed;
> +
>  struct greybus_descriptor_header {
>  	__le16	size;
>  	__u8	type;		/* enum greybus_descriptor_type */
> @@ -164,6 +214,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;

We're going to need to talk about these things...  But I can't
comment much more without learning more about the broader
architecture.

					-Alex

>  	};
>  } __packed;
>  
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [greybus-dev] [PATCH] RFC : mikroBUS driver for add-on boards
  2020-07-26 12:48 ` [greybus-dev] " Alex Elder
@ 2020-07-26 17:37   ` Vaishnav M A
  0 siblings, 0 replies; 7+ messages in thread
From: Vaishnav M A @ 2020-07-26 17:37 UTC (permalink / raw)
  To: Alex Elder
  Cc: greybus-dev, linux-kernel, gregkh, arnd, johan, elder, rajkovic,
	robh, mchehab+huawei, Robert Nelson, Drew Fustini, davem,
	zoran.stojsavljevic

On Sun, Jul 26, 2020 at 6:18 PM Alex Elder <elder@linaro.org> wrote:
>
> On 7/24/20 7:06 AM, Vaishnav M A wrote:
> > Attached is a patch for the mikroBUS driver which helps to
> > instantiate an add-on board device on a mikrobus port by fetching
> > the device identifier manifest binary from an EEPROM on-board
> > the device. mikroBUS is an add-on board socket standard by
> > MikroElektronika that can be freely used by anyone
> > following the guidelines, more details and discussions on
> > the status of the driver can be found here in this eLinux wiki:
> > https://elinux.org/Mikrobus
>
> Vaishnav, I am finally looking at this a little more closely
> today.  More than anything I want to compliment you on all
> this work.  I think it looks like a great use of Greybus for
> essentially its intended purpose, and I would love to see
> it extended as needed to support what you're doing here.
>
Thank You, Alex, for your review and support, I have gone through
your initial code review and will make the changes as you
suggested.

> At first glance your patch looks good, but I want to take the
> time to give it a thorough review.  Unfortunately I did not
> follow your progress on the GSoC project (which you posted
> about last year here), and have not followed any discussion
> since then, so it's taking me a little time to come up to speed
> on it.  I'm hoping you might help me (and others) do this more
> quickly.
>
> I am scanning through some of the materials online and I find
> there is quite a lot.  That includes information about both
> your project and about mikroBUS.  Zeroing in on things that
> fairly concisely describe the way things really work would be
> very valuable.  Can you point me directly at something that
> gives an overview of both the hardware and software
> architecture (specifically as it's used with Greybus)?  If
> not, I'm open to finding other ways to get in synch.  I'll
> expand on this a little more below.
>
I am not exactly sure if there exists a single publicly available
document that gives an overview of mikrobus ports being
used over Greybus. under https://elinux.org/Mikrobus some
initial discussions and proposals are added.

> This isn't strictly necessary, but if I wanted to reproduce the
> hardware setup you use to validate this code, what hardware
> is required?  For example, is it possible to use a BeagleBone Black
> with a MikroBus Cape for testing, or must a PocktBeagle be used?
> Is there one or several MikroBus clickboards that would be the
> best for basic testing?
>
Yes, it should be possible to use a Beaglebone Black with a mikroBUS
cape for testing, the config array passed to the add_port will change,
this should be the values for the mikrobus port 1 on the mikroBUS cape
printf "%b" '\x02\x01\x00\x3c\x32\x30' > /sys/bus/mikrobus/add_port

I understand that this is not a proper way to pass the mikrobus-port
parameters, in the actual submission this information will be from
a corresponding device tree overlay fragment.

The status of different add-on boards supported is currently updated
here: https://github.com/vaishnav98/manifesto/wiki/click_info and the
corresponding manifests are available here :
https://github.com/vaishnav98/manifesto/tree/mikrobusv2

> Can you provide a short summary of why the Greybus manifest
> format needed to be extended?  Can you summarize how a mikroBUS
> add-on board differs from an Ara module?  And what they have in
> common?
>
The main reason for Greybus manifest to be extended is to describe
devices on I2C, SPI, UART behind a greybus device when there
is a need to bind an existing kernel driver for the device, the need for
this does not arise with the Project Ara modules, since they had
a firmware on the module, but a mikrobus add-on board is a
simple device which can contain one or more sensors, displays
or other communication modules and only the GBPHY class and
a subset of protocols (GPIO,I2C,SPI,UART,PWM)  are relevant to
the mikroBUS.


> This patch is an RFC, and you say that v3 of the mikroBUS spec
> is being developed.  Is your plan to have the "real" code (when
> you submit it) adhere to the newer version of the spec?  Are
> there specific things that you expect will be included in v3
> that will how the driver works (compared to this RFC)?
>
Yes, the plan is to have the real driver code to adhere to the
a newer version of the spec, the main thing that will change
is the mechanism of fetching the manifest binary from a
non-volatile storage on-board the add-on board. Currently
an I2C EEPROM on a default address(0x57) is used to fetch
the manifest binary, but this method will not work in cases like
the Beaglebone Black mikrobus cape where the I2C bus for
all the 4 different mikrobus port will be the same, this is the
major change expected.
> Please realize I'm asking these questions so I can be more
> effective in evaluating what you're doing here.  I'd like to
> provide feedback not just on the code, but on the design that
> underlies it, and for that I need to get better informed.  My
> hope is that you can help me find or gather that information
> as quickly as possible.
>
> And now I'll go give a quick initial review of the code...
>
> Thanks.
>
>                                         -Alex
>
> > In the current state of the driver, more than 80 different
> > add-on boards have been tested on the BeagleBoard.org
> > PocketBeagle and the manifest binary is generated using the same
> > manifesto tool used to generate Greybus manifest binaries,
> > The pull request to manifesto to add new descriptors specific
> > to mikrobus is here : https://github.com/projectara/manifesto/pull/2
> > The utilization of Greybus manifest binaries here is not entirely
> > coincidental, We are evaluating ways to add mikroBUS sockets and
> > devices via Greybus expansion.
> >
> > The mikroBUS standard includes SPI, I2C, UART, PWM, ADC, GPIO
> > and power (3.3V and 5V) connections to interface common embedded
> > peripherals, There are more than 750 add-on boards ranging from
> > wireless connectivity boards to human-machine interface sensors
> > which conform to the mikroBUS standard, out of which more than 140
> > boards already have support in the Linux kernel.Today, there is no
> > mainlinesolution for enabling mikroBUS add-on boards at run-time, the
> > most straight forward method for loading drivers is to provide
> > device-tree overlay fragments at boot time, this method suffers
> > from the need to maintain a large out-of-tree database for which there
> > is need to maintain an overlay for every mikroBUS add-on board for every
> > Linux system and for every mikroBUS socket on that system.
> >
> > The mikroBUS driver tries to solve the problem by using extended version
> > of the greybus manifest to describe the add-on board device specific
> > information required by the device driver and uses it along with the fixed
> > port specific information to probe the specific device driver.The manifest
> > binary is now fetched from an I2C EEPROM over the I2C bus on the mikroBUS
> > port(subject to change in mikroBUS v3 specification) and enumerate drivers
> > for the add-on devices.There is also ongoing work to define a mikroBUS
> > v3 standard in which the addon board includes a non-volatile storage to
> > store the device identifier manifest binary, once the mikroBUS v3 standard
> > is released, the mikroBUS can be seen as a probeable bus such that the
> > kernel can discover the device on the bus at boot time.
> >
> > The driver also has a few debug SysFS interfaces for testing on add-on
> > boards without an EEPROM, these can be used in the following manner:
> > (example for mikroBUS port 1 on BeagleBoard.org PocketBeagle):
> >
> > printf "%b" '\x01\x00\x00\x59\x32\x17' > /sys/bus/mikrobus/add_port
> >
> > The bytes in the byte array sequence are (in order):
> >
> >       * i2c_adap_nr
> >       * spi_master_nr
> >       * serdev_ctlr_nr
> >       * rst_gpio_nr
> >       * pwm_gpio_nr
> >       * int_gpio_nr
> > * add_port sysFS entry is purely for debug and in the actual state
> > of the driver, port configuration will be loaded from a suitable
> > device tree overlay fragment.
> >
> > echo 0 > /sys/bus/mikrobus/del_port (to delete the attached port)
> > echo 1 >  /sys/class/mikrobus-port/mikrobus-0/rescan (to rescan the EEPROM
> > contents on the I2C bus on the mikroBUS port).
> >
> > cat board_manifest.mnfb >  /sys/class/mikrobus-port/mikrobus-0/new_device
> > * debug interface to pass the manifest binary in case an EEPROM is absent
> > echo 0 >  /sys/class/mikrobus-port/mikrobus-0/delete_device
> > * to unload the loaded board on the mikrobus port
> >
> > These sysFS interfaces are only implemented for debug purposes and
> > in the actual implementation of the driver these will be removed and
> > the manifest binary will be fetched from the non volatile storage on-board
> > the device.
> >
> > The mikroBUS driver enable the mikroBUS to be a probeable bus such that
> > the kernel can discover the device and automatically load the drivers.
> > There are already several Linux platforms with mikroBUS sockets and the
> > mikroBUS driver helps to reduce the time to develop and debug
> > support for various mikroBUS add-on boards. Further, it opens up
> > the possibility for support under dynamically instantiated buses
> > such as with Greybus.
> >
> > Please let know the feedback you have on this patch or the approach used.
> >
> > Thanks,
> >
> > Vaishnav M A
> >
> > Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org>
> > ---
> >  MAINTAINERS                               |   6 +
> >  drivers/misc/Kconfig                      |   1 +
> >  drivers/misc/Makefile                     |   1 +
> >  drivers/misc/mikrobus/Kconfig             |  16 +
> >  drivers/misc/mikrobus/Makefile            |   5 +
> >  drivers/misc/mikrobus/mikrobus_core.c     | 649 ++++++++++++++++++++++
> >  drivers/misc/mikrobus/mikrobus_core.h     | 130 +++++
> >  drivers/misc/mikrobus/mikrobus_manifest.c | 390 +++++++++++++
> >  drivers/misc/mikrobus/mikrobus_manifest.h |  21 +
> >  include/linux/greybus/greybus_manifest.h  |  53 ++
> >  10 files changed, 1272 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 d53db30d1365..9a049746203f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11402,6 +11402,12 @@ M:   Oliver Neukum <oliver@neukum.org>
> >  S:   Maintained
> >  F:   drivers/usb/image/microtek.*
> >
> > +MIKROBUS ADDON BOARD DRIVER
> > +M:   Vaishnav M A <vaishnav@beagleboard.org>
> > +S:   Maintained
> > +W:   https://elinux.org/Mikrobus
> > +F:   drivers/misc/mikrobus/
> > +
> >  MIPS
> >  M:   Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> >  L:   linux-mips@vger.kernel.org
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index e1b1ba5e2b92..334f0c39d56b 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -472,4 +472,5 @@ source "drivers/misc/ocxl/Kconfig"
> >  source "drivers/misc/cardreader/Kconfig"
> >  source "drivers/misc/habanalabs/Kconfig"
> >  source "drivers/misc/uacce/Kconfig"
> > +source "drivers/misc/mikrobus/Kconfig"
> >  endmenu
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index c7bd01ac6291..45486dd77da5 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -40,6 +40,7 @@ obj-$(CONFIG_VMWARE_BALLOON)        += vmw_balloon.o
> >  obj-$(CONFIG_PCH_PHUB)               += pch_phub.o
> >  obj-y                                += ti-st/
> >  obj-y                                += lis3lv02d/
> > +obj-y                                += mikrobus/
> >  obj-$(CONFIG_ALTERA_STAPL)   +=altera-stapl/
> >  obj-$(CONFIG_INTEL_MEI)              += mei/
> >  obj-$(CONFIG_VMWARE_VMCI)    += vmw_vmci/
> > diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig
> > new file mode 100644
> > index 000000000000..c3b93e12daad
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/Kconfig
> > @@ -0,0 +1,16 @@
> > +menuconfig MIKROBUS
> > +     tristate "Module for instantiating devices on mikroBUS ports"
> > +     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 helps in
> > +       instantiating devices on the mikroBUS port with identifier
> > +       data fetched from an EEPROM on the device, more details on
> > +       the mikroBUS driver support and discussion can be found in
> > +       this eLinux wiki : elinux.org/Mikrobus
> > +
> > +
> > +       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..1f80ed4064d8
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# mikroBUS Core
> > +
> > +mikrobus-y :=        mikrobus_core.o mikrobus_manifest.o
> > +obj-$(CONFIG_MIKROBUS) += mikrobus.o
> > \ No newline at end of file
> > diff --git a/drivers/misc/mikrobus/mikrobus_core.c b/drivers/misc/mikrobus/mikrobus_core.c
> > new file mode 100644
> > index 000000000000..78c96c637632
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/mikrobus_core.c
> > @@ -0,0 +1,649 @@
> > +// 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.
> > + */
> > +
> > +#define pr_fmt(fmt) "mikrobus: " fmt
> > +
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/idr.h>
> > +#include <linux/init.h>
> > +#include <linux/jump_label.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/mutex.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/machine.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/nvmem-provider.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/serdev.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +
> > +#include "mikrobus_core.h"
> > +#include "mikrobus_manifest.h"
> > +
> > +#define ATMEL_24C32_I2C_ADDR 0x57
> > +
> > +static DEFINE_IDR(mikrobus_port_idr);
> > +static struct class_compat *mikrobus_port_compat_class;
> > +static bool is_registered;
> > +
> > +static ssize_t add_port_store(struct bus_type *bt, const char *buf,
> > +                           size_t count)
> > +{
> > +     struct mikrobus_port_config *cfg;
> > +
> > +     if (count < sizeof(*cfg)) {
> > +             pr_err("add_port: incorrect config data received: %s\n", buf);
> > +             return -EINVAL;
> > +     }
> > +     mikrobus_register_port_config((void *)buf);
> > +     return count;
> > +}
> > +BUS_ATTR_WO(add_port);
> > +
> > +static ssize_t del_port_store(struct bus_type *bt, const char *buf,
> > +                                                       size_t count)
> > +{
> > +     int id;
> > +     char end;
> > +     int res;
> > +
> > +     res = sscanf(buf, "%d%c", &id, &end);
> > +     if (res < 1) {
> > +             pr_err("delete_port: cannot parse mikrobus port ID\n");
> > +             return -EINVAL;
> > +     }
> > +     if (!idr_find(&mikrobus_port_idr, id)) {
> > +             pr_err("attempting to delete unregistered port [%d]\n", id);
> > +             return -EINVAL;
> > +     }
> > +     mikrobus_del_port(idr_find(&mikrobus_port_idr, id));
> > +     return count;
> > +}
> > +BUS_ATTR_WO(del_port);
> > +
> > +static struct attribute *mikrobus_attrs[] = {
> > +     &bus_attr_add_port.attr,
> > +     &bus_attr_del_port.attr,
> > +      NULL
> > +};
> > +ATTRIBUTE_GROUPS(mikrobus);
> > +
> > +struct bus_type mikrobus_bus_type = {
> > +     .name = "mikrobus",
> > +     .bus_groups = mikrobus_groups,
> > +};
> > +EXPORT_SYMBOL_GPL(mikrobus_bus_type);
> > +
> > +static int mikrobus_port_scan_eeprom(struct mikrobus_port *port)
> > +{
> > +     char header[12];
> > +     struct addon_board_info *board;
> > +     int manifest_size;
> > +     int retval;
> > +     char *buf;
> > +
> > +     nvmem_device_read(port->eeprom, 0, 12, header);
> > +     manifest_size = mikrobus_manifest_header_validate(header, 12);
> > +     if (manifest_size > 0) {
> > +             buf = kzalloc(manifest_size, GFP_KERNEL);
> > +             nvmem_device_read(port->eeprom, 0, manifest_size, buf);
> > +             board = kzalloc(sizeof(*board), GFP_KERNEL);
> > +             if (!board)
> > +                     return -ENOMEM;
> > +             INIT_LIST_HEAD(&board->manifest_descs);
> > +             INIT_LIST_HEAD(&board->devices);
> > +             retval = mikrobus_manifest_parse(board, (void *)buf, manifest_size);
> > +             if (!retval) {
> > +                     pr_err("failed to parse manifest, size: %d", manifest_size);
> > +                     return -EINVAL;
> > +             }
> > +             retval = mikrobus_register_board(port, board);
> > +             if (retval) {
> > +                     pr_err("failed to register board: %s", board->name);
> > +                     return -EINVAL;
> > +             }
> > +             kfree(buf);
> > +     } else {
> > +             pr_err("inavlide manifest port %d", port->id);
> > +             return -EINVAL;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> > +                                              char *buf)
> > +{
> > +     return sprintf(buf, "%s\n", to_mikrobus_port(dev)->name);
> > +}
> > +static DEVICE_ATTR_RO(name);
> > +
> > +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 retval;
> > +
> > +     if (port->board == NULL) {
> > +             board = kzalloc(sizeof(*board), GFP_KERNEL);
> > +             if (!board)
> > +                     return -EINVAL;
> > +             INIT_LIST_HEAD(&board->manifest_descs);
> > +             INIT_LIST_HEAD(&board->devices);
> > +     } else {
> > +             pr_err("port %d already has board registered", port->id);
> > +             return -EINVAL;
> > +     }
> > +     retval = mikrobus_manifest_parse(board, (void *)buf, count);
> > +     if (!retval) {
> > +             pr_err("failed to parse manifest");
> > +             return -EINVAL;
> > +     }
> > +     retval = mikrobus_register_board(port, board);
> > +     if (retval) {
> > +             pr_err("failed to register board: %s", board->name);
> > +             return -EINVAL;
> > +     }
> > +     return count;
> > +}
> > +static DEVICE_ATTR_WO(new_device);
> > +
> > +static ssize_t rescan_store(struct device *dev, struct device_attribute *attr,
> > +                                                     const char *buf, size_t count)
> > +{
> > +     struct mikrobus_port *port = to_mikrobus_port(dev);
> > +     int id;
> > +     char end;
> > +     int res;
> > +     int retval;
> > +
> > +     res = sscanf(buf, "%d%c", &id, &end);
> > +     if (res < 1) {
> > +             pr_err("rescan: Can't parse trigger\n");
> > +             return -EINVAL;
> > +     }
> > +     if (port->board != NULL) {
> > +             pr_err("port %d already has board registered", port->id);
> > +             return -EINVAL;
> > +     }
> > +     retval = mikrobus_port_scan_eeprom(port);
> > +     if (retval) {
> > +             pr_err("port %d board register from manifest failed", port->id);
> > +             return -EINVAL;
> > +     }
> > +     return count;
> > +}
> > +static DEVICE_ATTR_WO(rescan);
> > +
> > +static ssize_t delete_device_store(struct device *dev, struct device_attribute *attr,
> > +                                                     const char *buf, size_t count)
> > +{
> > +     int id;
> > +     char end;
> > +     int res;
> > +     struct mikrobus_port *port = to_mikrobus_port(dev);
> > +
> > +     res = sscanf(buf, "%d%c", &id, &end);
> > +     if (res < 1) {
> > +             pr_err("delete_board: Can't parse board ID\n");
> > +             return -EINVAL;
> > +     }
> > +     if (port->board == NULL) {
> > +             pr_err("delete_board: port does not have any boards registered\n");
> > +             return -EINVAL;
> > +     }
> > +     mikrobus_unregister_board(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_rescan.attr,
> > +     &dev_attr_delete_device.attr, &dev_attr_name.attr, NULL};
> > +ATTRIBUTE_GROUPS(mikrobus_port);
> > +
> > +static void mikrobus_dev_release(struct device *dev)
> > +{
> > +     struct mikrobus_port *port = to_mikrobus_port(dev);
> > +
> > +     idr_remove(&mikrobus_port_idr, port->id);
> > +     kfree(port);
> > +}
> > +
> > +struct device_type mikrobus_port_type = {
> > +     .groups = mikrobus_port_groups,
> > +     .release = mikrobus_dev_release,
> > +};
> > +EXPORT_SYMBOL_GPL(mikrobus_port_type);
> > +
> > +static int mikrobus_get_irq(struct mikrobus_port *port, int irqno,
> > +                                                     int irq_type)
> > +{
> > +     int irq;
> > +
> > +     switch (irqno) {
> > +     case MIKROBUS_GPIO_INT:
> > +     irq = gpiod_to_irq(port->int_gpio);
> > +     break;
> > +     case MIKROBUS_GPIO_RST:
> > +     irq = gpiod_to_irq(port->rst_gpio);
> > +     break;
> > +     case MIKROBUS_GPIO_PWM:
> > +     irq = gpiod_to_irq(port->pwm_gpio);
> > +     break;
> > +     default:
> > +     return -EINVAL;
> > +     }
> > +     if (irq < 0) {
> > +             pr_err("Could not get irq for irq type: %d", irqno);
> > +             return -EINVAL;
> > +     }
> > +     irq_set_irq_type(irq, irq_type);
> > +     return irq;
> > +}
> > +
> > +static int mikrobus_setup_gpio(struct gpio_desc *gpio, int gpio_state)
> > +{
> > +     int retval;
> > +
> > +     if (gpio_state == MIKROBUS_GPIO_UNUSED)
> > +             return 0;
> > +     switch (gpio_state) {
> > +     case MIKROBUS_GPIO_INPUT:
> > +     retval = gpiod_direction_input(gpio);
> > +     break;
> > +     case MIKROBUS_GPIO_OUTPUT_HIGH:
> > +     retval = gpiod_direction_output(gpio, 1);
> > +     gpiod_set_value_cansleep(gpio, 1);
> > +     break;
> > +     case MIKROBUS_GPIO_OUTPUT_LOW:
> > +     retval = gpiod_direction_output(gpio, 0);
> > +     gpiod_set_value_cansleep(gpio, 0);
> > +     break;
> > +     default:
> > +     return -EINVAL;
> > +     }
> > +     return retval;
> > +}
> > +
> > +static void mikrobus_spi_device_delete(struct spi_master *master, unsigned int cs)
> > +{
> > +     struct device *dev;
> > +     char str[32];
> > +
> > +     snprintf(str, sizeof(str), "%s.%u", dev_name(&master->dev), cs);
> > +     dev = bus_find_device_by_name(&spi_bus_type, NULL, str);
> > +     if (dev != NULL) {
> > +             spi_unregister_device(to_spi_device(dev));
> > +             put_device(dev);
> > +     }
> > +}
> > +
> > +static char *mikrobus_get_gpio_chip_name(struct mikrobus_port *port, int gpio)
> > +{
> > +     char *name;
> > +     struct gpio_chip *gpiochip;
> > +
> > +     switch (gpio) {
> > +     case MIKROBUS_GPIO_INT:
> > +     gpiochip = gpiod_to_chip(port->int_gpio);
> > +     name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> > +     break;
> > +     case MIKROBUS_GPIO_RST:
> > +     gpiochip = gpiod_to_chip(port->rst_gpio);
> > +     name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> > +     break;
> > +     case MIKROBUS_GPIO_PWM:
> > +     gpiochip = gpiod_to_chip(port->pwm_gpio);
> > +     name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> > +     break;
> > +     }
> > +     return name;
> > +}
> > +
> > +static int mikrobus_get_gpio_hwnum(struct mikrobus_port *port, int gpio)
> > +{
> > +     int hwnum;
> > +     struct gpio_chip *gpiochip;
> > +
> > +     switch (gpio) {
> > +     case MIKROBUS_GPIO_INT:
> > +     gpiochip = gpiod_to_chip(port->int_gpio);
> > +     hwnum = desc_to_gpio(port->int_gpio) - gpiochip->base;
> > +     break;
> > +     case MIKROBUS_GPIO_RST:
> > +     gpiochip = gpiod_to_chip(port->rst_gpio);
> > +     hwnum = desc_to_gpio(port->rst_gpio) - gpiochip->base;
> > +     break;
> > +     case MIKROBUS_GPIO_PWM:
> > +     gpiochip = gpiod_to_chip(port->pwm_gpio);
> > +     hwnum = desc_to_gpio(port->pwm_gpio) - gpiochip->base;
> > +     break;
> > +     }
> > +     return hwnum;
> > +}
> > +
> > +static void release_mikrobus_device(struct board_device_info *dev)
> > +{
> > +     list_del(&dev->links);
> > +     kfree(dev);
> > +}
> > +
> > +static void release_board_devices(struct addon_board_info *info)
> > +{
> > +     struct board_device_info *dev;
> > +     struct board_device_info *next;
> > +
> > +     list_for_each_entry_safe(dev, next, &info->devices, links)
> > +             release_mikrobus_device(dev);
> > +}
> > +
> > +static int mikrobus_register_device(struct mikrobus_port *port,
> > +                                     struct board_device_info *dev, char *board_name)
> > +{
> > +     struct i2c_board_info *i2c;
> > +     struct spi_board_info *spi;
> > +     struct gpiod_lookup_table *lookup;
> > +     char devname[40];
> > +     int i;
> > +
> > +     pr_info(" registering device : %s\n", dev->drv_name);
> > +
> > +     if (dev->gpio_lookup != NULL) {
> > +             lookup = dev->gpio_lookup;
> > +             if (dev->protocol == MIKROBUS_PROTOCOL_SPI) {
> > +                     snprintf(devname, sizeof(devname), "%s.%u",
> > +                             dev_name(&port->spi_mstr->dev), dev->reg);
> > +                     lookup->dev_id = kmemdup(devname, 40, GFP_KERNEL);
> > +             } else if (dev->protocol == MIKROBUS_PROTOCOL_I2C)
> > +                     lookup->dev_id = dev->drv_name;
> > +             pr_info(" adding lookup table : %s\n", lookup->dev_id);
> > +             for (i = 0; i < dev->num_gpio_resources; i++) {
> > +                     lookup->table[i].key =
> > +                     mikrobus_get_gpio_chip_name(port, lookup->table[i].chip_hwnum);
> > +                     lookup->table[i].chip_hwnum =
> > +                     mikrobus_get_gpio_hwnum(port, lookup->table[i].chip_hwnum);
> > +                     lookup->table[i].flags = GPIO_ACTIVE_HIGH;
> > +             }
> > +             gpiod_add_lookup_table(lookup);
> > +     }
> > +     switch (dev->protocol) {
> > +     case MIKROBUS_PROTOCOL_SPI:
> > +     spi = kzalloc(sizeof(*spi), GFP_KERNEL);
> > +     if (!spi)
> > +             return -ENOMEM;
> > +     strncpy(spi->modalias, dev->drv_name, sizeof(spi->modalias) - 1);
> > +     if (dev->irq)
> > +             spi->irq = mikrobus_get_irq(port, dev->irq, dev->irq_type);
> > +     if (dev->properties)
> > +             spi->properties = dev->properties;
> > +     spi->chip_select = dev->reg;
> > +     spi->max_speed_hz = dev->max_speed_hz;
> > +     spi->mode = dev->mode;
> > +     mikrobus_spi_device_delete(port->spi_mstr, dev->reg);
> > +     dev->dev_client = (void *)spi_new_device(port->spi_mstr, spi);
> > +     break;
> > +     case MIKROBUS_PROTOCOL_I2C:
> > +     i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> > +     if (!i2c)
> > +             return -ENOMEM;
> > +     strncpy(i2c->type, dev->drv_name, sizeof(i2c->type) - 1);
> > +     if (dev->irq)
> > +             i2c->irq = mikrobus_get_irq(port, dev->irq, dev->irq_type);
> > +     if (dev->properties)
> > +             i2c->properties = dev->properties;
> > +     i2c->addr = dev->reg;
> > +     dev->dev_client = (void *)i2c_new_client_device(port->i2c_adap, i2c);
> > +     break;
> > +     case MIKROBUS_PROTOCOL_UART:
> > +     pr_info("SERDEV devices support not yet added");
> > +     break;
> > +     default:
> > +     return -EINVAL;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static void mikrobus_unregister_device(struct mikrobus_port *port, struct board_device_info *dev,
> > +                                                                             char *board_name)
> > +{
> > +     pr_info(" removing device : %s\n", dev->drv_name);
> > +     if (dev->gpio_lookup != NULL) {
> > +             gpiod_remove_lookup_table(dev->gpio_lookup);
> > +             kfree(dev->gpio_lookup);
> > +     }
> > +     if (dev->properties != NULL)
> > +             kfree(dev->properties);
> > +     switch (dev->protocol) {
> > +     case MIKROBUS_PROTOCOL_SPI:
> > +     spi_unregister_device((struct spi_device *)dev->dev_client);
> > +     break;
> > +     case MIKROBUS_PROTOCOL_I2C:
> > +     i2c_unregister_device((struct i2c_client *)dev->dev_client);
> > +     break;
> > +     case MIKROBUS_PROTOCOL_UART:
> > +     pr_err("SERDEV devices support not yet added");
> > +     break;
> > +     }
> > +}
> > +
> > +int mikrobus_register_board(struct mikrobus_port *port,      struct addon_board_info *board)
> > +{
> > +     struct board_device_info *devinfo;
> > +     struct board_device_info *next;
> > +     int retval;
> > +
> > +     if (WARN_ON(list_empty(&board->devices)))
> > +             return false;
> > +
> > +     retval = mikrobus_setup_gpio(port->pwm_gpio, board->pwm_gpio_state);
> > +     if (retval) {
> > +             pr_err("mikrobus_setup_gpio : can't setup pwm gpio state: (%d)\n", retval);
> > +             return retval;
> > +     }
> > +     retval = mikrobus_setup_gpio(port->int_gpio, board->int_gpio_state);
> > +     if (retval) {
> > +             pr_err("mikrobus_setup_gpio : can't setup int gpio state: (%d)\n", retval);
> > +             return retval;
> > +     }
> > +     retval = mikrobus_setup_gpio(port->rst_gpio, board->rst_gpio_state);
> > +     if (retval) {
> > +             pr_err("mikrobus_setup_gpio : can't setup rst gpio state: (%d)\n", retval);
> > +             return retval;
> > +     }
> > +     list_for_each_entry_safe(devinfo, next, &board->devices, links)
> > +             mikrobus_register_device(port, devinfo, board->name);
> > +     port->board = board;
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mikrobus_register_board);
> > +
> > +void mikrobus_unregister_board(struct mikrobus_port *port, struct addon_board_info *board)
> > +{
> > +     struct board_device_info *devinfo;
> > +     struct board_device_info *next;
> > +
> > +     if (WARN_ON(list_empty(&board->devices)))
> > +             return;
> > +     port->board = NULL;
> > +     list_for_each_entry_safe(devinfo, next, &board->devices, links)
> > +             mikrobus_unregister_device(port, devinfo, board->name);
> > +     release_board_devices(board);
> > +     kfree(board);
> > +     port->board = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(mikrobus_unregister_board);
> > +
> > +int mikrobus_register_port_config(struct mikrobus_port_config *cfg)
> > +{
> > +     struct mikrobus_port *port;
> > +     int retval;
> > +
> > +     if (WARN_ON(!is_registered))
> > +             return -EAGAIN;
> > +     port = kzalloc(sizeof(*port), GFP_KERNEL);
> > +     if (!port)
> > +             return -ENOMEM;
> > +     port->pwm_gpio = gpio_to_desc(cfg->pwm_gpio_nr);
> > +     port->int_gpio = gpio_to_desc(cfg->int_gpio_nr);
> > +     port->rst_gpio = gpio_to_desc(cfg->rst_gpio_nr);
> > +     port->spi_mstr = spi_busnum_to_master(cfg->spi_master_nr);
> > +     port->i2c_adap = i2c_get_adapter(cfg->i2c_adap_nr);
> > +     retval = mikrobus_register_port(port);
> > +     if (retval) {
> > +             pr_err("port : can't register port from config (%d)\n", retval);
> > +             return retval;
> > +     }
> > +     return retval;
> > +}
> > +EXPORT_SYMBOL_GPL(mikrobus_register_port_config);
> > +
> > +static struct i2c_board_info mikrobus_eeprom_info = {
> > +     I2C_BOARD_INFO("24c32", ATMEL_24C32_I2C_ADDR),
> > +};
> > +
> > +static int mikrobus_port_probe_eeprom(struct mikrobus_port *port)
> > +{
> > +     struct i2c_client *eeprom_client;
> > +     struct nvmem_device *eeprom;
> > +     char dev_name[40];
> > +
> > +     eeprom_client = i2c_new_client_device(port->i2c_adap, &mikrobus_eeprom_info);
> > +     if (!IS_ERR(eeprom_client)) {
> > +             pr_info(" mikrobus port %d default eeprom is probed at %02x\n", port->id,
> > +                                                                     eeprom_client->addr);
> > +             snprintf(dev_name, sizeof(dev_name), "%d-%04x0", port->i2c_adap->nr,
> > +                              eeprom_client->addr);
> > +             eeprom = nvmem_device_get(&eeprom_client->dev, dev_name);
> > +             if (IS_ERR(eeprom)) {
> > +                     pr_err(" mikrobus port %d eeprom nvmem device probe failed\n", port->id);
> > +                     i2c_unregister_device(eeprom_client);
> > +                     port->eeprom = NULL;
> > +                     return 0;
> > +             }
> > +     } else {
> > +             pr_info(" mikrobus port %d default eeprom probe failed\n", port->id);
> > +             return 0;
> > +     }
> > +     port->eeprom = eeprom;
> > +     port->eeprom_client = eeprom_client;
> > +     return 0;
> > +}
> > +
> > +int mikrobus_register_port(struct mikrobus_port *port)
> > +{
> > +     int retval;
> > +     int id;
> > +
> > +     if (WARN_ON(!is_registered))
> > +             return -EAGAIN;
> > +     id = idr_alloc(&mikrobus_port_idr, port, 0, 0, GFP_KERNEL);
> > +     if (id < 0)
> > +             return id;
> > +     port->id = id;
> > +     port->dev.bus = &mikrobus_bus_type;
> > +     port->dev.type = &mikrobus_port_type;
> > +     strncpy(port->name, "mikrobus-port", sizeof(port->name) - 1);
> > +     dev_set_name(&port->dev, "mikrobus-%d", port->id);
> > +     pr_info("registering port mikrobus-%d\n ", port->id);
> > +     retval = device_register(&port->dev);
> > +     if (retval) {
> > +             pr_err("port '%d': can't register device (%d)\n", port->id, retval);
> > +             put_device(&port->dev);
> > +             return retval;
> > +     }
> > +     retval = class_compat_create_link(mikrobus_port_compat_class, &port->dev,
> > +                                                             port->dev.parent);
> > +     if (retval)
> > +             dev_warn(&port->dev, "failed to create compatibility class link\n");
> > +     if (!port->eeprom) {
> > +             pr_info("mikrobus port %d eeprom empty probing default eeprom\n", port->id);
> > +             retval = mikrobus_port_probe_eeprom(port);
> > +     }
> > +     if (port->eeprom) {
> > +             retval = mikrobus_port_scan_eeprom(port);
> > +             if (retval)
> > +                     dev_warn(&port->dev, "failed to register board from manifest\n");
> > +     }
> > +     return retval;
> > +}
> > +EXPORT_SYMBOL_GPL(mikrobus_register_port);
> > +
> > +void mikrobus_del_port(struct mikrobus_port *port)
> > +{
> > +     struct mikrobus_port *found;
> > +
> > +     found = idr_find(&mikrobus_port_idr, port->id);
> > +     if (found != port) {
> > +             pr_err("attempting to delete unregistered port [%s]\n", port->name);
> > +             return;
> > +     }
> > +     if (port->board != NULL) {
> > +             pr_err("attempting to delete port with registered boards, port [%s]\n",
> > +                                                                     port->name);
> > +             return;
> > +     }
> > +
> > +     if (port->eeprom) {
> > +             nvmem_device_put(port->eeprom);
> > +             i2c_unregister_device(port->eeprom_client);
> > +     }
> > +
> > +     class_compat_remove_link(mikrobus_port_compat_class, &port->dev,
> > +                                                     port->dev.parent);
> > +     device_unregister(&port->dev);
> > +     idr_remove(&mikrobus_port_idr, port->id);
> > +     memset(&port->dev, 0, sizeof(port->dev));
> > +}
> > +EXPORT_SYMBOL_GPL(mikrobus_del_port);
> > +
> > +static int __init mikrobus_init(void)
> > +{
> > +     int retval;
> > +
> > +     retval = bus_register(&mikrobus_bus_type);
> > +     if (retval) {
> > +             pr_err("bus_register failed (%d)\n", retval);
> > +             return retval;
> > +     }
> > +     mikrobus_port_compat_class = class_compat_register("mikrobus-port");
> > +     if (!mikrobus_port_compat_class) {
> > +             pr_err("class_compat register failed (%d)\n", retval);
> > +             retval = -ENOMEM;
> > +             goto class_err;
> > +     }
> > +     is_registered = true;
> > +     return 0;
> > +class_err:
> > +     bus_unregister(&mikrobus_bus_type);
> > +     idr_destroy(&mikrobus_port_idr);
> > +     is_registered = false;
> > +     return retval;
> > +}
> > +subsys_initcall(mikrobus_init);
> > +
> > +static void __exit mikrobus_exit(void)
> > +{
> > +     bus_unregister(&mikrobus_bus_type);
> > +     class_compat_unregister(mikrobus_port_compat_class);
> > +     idr_destroy(&mikrobus_port_idr);
> > +}
> > +module_exit(mikrobus_exit);
> > +
> > +MODULE_AUTHOR("Vaishnav M A <vaishnav@beagleboard.org>");
> > +MODULE_DESCRIPTION("mikroBUS main module");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/misc/mikrobus/mikrobus_core.h b/drivers/misc/mikrobus/mikrobus_core.h
> > new file mode 100644
> > index 000000000000..9684d315f564
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/mikrobus_core.h
> > @@ -0,0 +1,130 @@
> > +/* 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.
> > + */
> > +
> > +#ifndef __MIKROBUS_H
> > +#define __MIKROBUS_H
> > +
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/i2c.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/gpio/machine.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/idr.h>
> > +#include <linux/init.h>
> > +#include <linux/jump_label.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/device.h>
> > +#include <linux/of_device.h>
> > +#include <linux/serdev.h>
> > +#include <linux/of.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +
> > +#define MIKROBUS_VERSION_MAJOR 0x00
> > +#define MIKROBUS_VERSION_MINOR 0x02
> > +
> > +extern struct bus_type mikrobus_bus_type;
> > +extern struct device_type mikrobus_port_type;
> > +
> > +enum mikrobus_property_type {
> > +     MIKROBUS_PROPERTY_TYPE_LINK = 0x00,
> > +     MIKROBUS_PROPERTY_TYPE_GPIO = 0x01,
> > +     MIKROBUS_PROPERTY_TYPE_U8 = 0x02,
> > +     MIKROBUS_PROPERTY_TYPE_U16 = 0x03,
> > +     MIKROBUS_PROPERTY_TYPE_U32 = 0x04,
> > +     MIKROBUS_PROPERTY_TYPE_U64 = 0x05,
> > +};
> > +
> > +enum mikrobus_gpio_pin {
> > +     MIKROBUS_GPIO_INVALID = 0x00,
> > +     MIKROBUS_GPIO_INT = 0x01,
> > +     MIKROBUS_GPIO_RST = 0x02,
> > +     MIKROBUS_GPIO_PWM = 0x03,
> > +};
> > +
> > +enum mikrobus_protocol {
> > +     MIKROBUS_PROTOCOL_SPI = 0x01,
> > +     MIKROBUS_PROTOCOL_I2C = 0x02,
> > +     MIKROBUS_PROTOCOL_UART = 0x03,
> > +     MIKROBUS_PROTOCOL_SPI_GPIOCS = 0x04,
> > +     MIKROBUS_PROTOCOL_I2C_MUX = 0x05
> > +};
> > +
> > +enum mikrobus_gpio_state {
> > +     MIKROBUS_GPIO_UNUSED = 0x00,
> > +     MIKROBUS_GPIO_INPUT = 0x01,
> > +     MIKROBUS_GPIO_OUTPUT_HIGH = 0x02,
> > +     MIKROBUS_GPIO_OUTPUT_LOW = 0x03,
> > +};
> > +
> > +struct mikrobus_port_config {
> > +     __u8 i2c_adap_nr;
> > +     __u8 spi_master_nr;
> > +     __u8 serdev_ctlr_nr;
> > +     __u8 rst_gpio_nr;
> > +     __u8 pwm_gpio_nr;
> > +     __u8 int_gpio_nr;
> > +} __packed;
> > +
> > +struct board_device_info {
> > +     struct list_head links;
> > +     int id;
> > +     char *drv_name;
> > +     unsigned short protocol;
> > +     unsigned short reg;
> > +     u32 max_speed_hz;
> > +     unsigned int mode;
> > +     int irq;
> > +     int irq_type;
> > +     int cs_gpio;
> > +     unsigned short num_gpio_resources;
> > +     unsigned short num_properties;
> > +     struct property_entry *properties;
> > +     struct gpiod_lookup_table *gpio_lookup;
> > +     void *dev_client;
> > +};
> > +
> > +struct addon_board_info {
> > +     char *name;
> > +     unsigned short num_devices;
> > +     unsigned short rst_gpio_state;
> > +     unsigned short pwm_gpio_state;
> > +     unsigned short int_gpio_state;
> > +     struct list_head manifest_descs;
> > +     struct list_head devices;
> > +};
> > +
> > +struct mikrobus_port {
> > +     char name[48];
> > +     struct module *owner;
> > +     struct device dev;
> > +     int id;
> > +     struct gpio_desc *pwm_gpio;
> > +     struct gpio_desc *int_gpio;
> > +     struct gpio_desc *rst_gpio;
> > +     struct spi_master *spi_mstr;
> > +     struct i2c_adapter *i2c_adap;
> > +     struct addon_board_info *board;
> > +     struct i2c_client *eeprom_client;
> > +     struct nvmem_device *eeprom;
> > +};
> > +#define to_mikrobus_port(d) container_of(d, struct mikrobus_port, dev)
> > +
> > +int mikrobus_register_board(struct mikrobus_port *port,
> > +                                                     struct addon_board_info *board);
> > +void mikrobus_unregister_board(struct mikrobus_port *port,
> > +                                                        struct addon_board_info *board);
> > +int mikrobus_register_port_config(struct mikrobus_port_config *cfg);
> > +int mikrobus_register_port(struct mikrobus_port *port);
> > +void mikrobus_del_port(struct mikrobus_port *port);
> > +
> > +#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..60ebca560f0d
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/mikrobus_manifest.c
> > @@ -0,0 +1,390 @@
> > +// 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.
> > + */
> > +
> > +#define pr_fmt(fmt) "mikrobus_manifest: " fmt
> > +
> > +#include <linux/bits.h>
> > +#include <linux/types.h>
> > +#include <linux/property.h>
> > +#include <linux/greybus/greybus_manifest.h>
> > +
> > +#include "mikrobus_manifest.h"
> > +
> > +struct manifest_desc {
> > +     struct list_head links;
> > +     size_t size;
> > +     void *data;
> > +     enum greybus_descriptor_type type;
> > +};
> > +
> > +static void release_manifest_descriptor(struct manifest_desc *descriptor)
> > +{
> > +     list_del(&descriptor->links);
> > +     kfree(descriptor);
> > +}
> > +
> > +static void release_manifest_descriptors(struct addon_board_info *info)
> > +{
> > +     struct manifest_desc *descriptor;
> > +     struct manifest_desc *next;
> > +
> > +     list_for_each_entry_safe(descriptor, next, &info->manifest_descs, links)
> > +             release_manifest_descriptor(descriptor);
> > +}
> > +
> > +static int identify_descriptor(struct addon_board_info *info, struct greybus_descriptor *desc,
> > +                                                                              size_t size)
> > +{
> > +     struct greybus_descriptor_header *desc_header = &desc->header;
> > +     struct manifest_desc *descriptor;
> > +     size_t desc_size;
> > +     size_t expected_size;
> > +
> > +     if (size < sizeof(*desc_header))
> > +             return -EINVAL;
> > +     desc_size = le16_to_cpu(desc_header->size);
> > +     if (desc_size > 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:
> > +     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, &info->manifest_descs);
> > +     return desc_size;
> > +}
> > +
> > +static char *mikrobus_string_get(struct addon_board_info *info, 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, &info->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 *info)
> > +{
> > +     struct greybus_descriptor_mikrobus *mikrobus;
> > +     struct greybus_descriptor_interface *interface;
> > +     struct manifest_desc *descriptor;
> > +     bool found = false;
> > +
> > +     list_for_each_entry(descriptor, &info->manifest_descs, links) {
> > +             if (descriptor->type == GREYBUS_TYPE_MIKROBUS) {
> > +                     mikrobus = descriptor->data;
> > +                     found = true;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (found) {
> > +             info->num_devices = mikrobus->num_devices;
> > +             info->rst_gpio_state = mikrobus->rst_gpio_state;
> > +             info->pwm_gpio_state = mikrobus->pwm_gpio_state;
> > +             info->int_gpio_state = mikrobus->int_gpio_state;
> > +     } else {
> > +             info->num_devices = 1;
> > +             info->rst_gpio_state = MIKROBUS_GPIO_UNUSED;
> > +             info->pwm_gpio_state = MIKROBUS_GPIO_UNUSED;
> > +             info->int_gpio_state = MIKROBUS_GPIO_UNUSED;
> > +     }
> > +
> > +     list_for_each_entry(descriptor, &info->manifest_descs, links) {
> > +             if (descriptor->type == GREYBUS_TYPE_INTERFACE) {
> > +                     interface = descriptor->data;
> > +                     break;
> > +             }
> > +     }
> > +     info->name = mikrobus_string_get(info, interface->product_stringid);
> > +}
> > +
> > +static struct property_entry *
> > +mikrobus_property_entry_get(struct addon_board_info *info, u8 *prop_link,
> > +                                                     int num_properties)
> > +{
> > +     struct greybus_descriptor_property *desc_property;
> > +     struct manifest_desc *descriptor;
> > +     struct property_entry *properties;
> > +     int i;
> > +     char *prop_name;
> > +     bool found = false;
> > +     u8 *val_u8;
> > +     u16 *val_u16;
> > +     u32 *val_u32;
> > +     u64 *val_u64;
> > +
> > +     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, &info->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)
> > +                     return ERR_PTR(-ENOENT);
> > +             prop_name = mikrobus_string_get(info, desc_property->propname_stringid);
> > +             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:
> > +             return ERR_PTR(-EINVAL);
> > +             }
> > +     }
> > +     return properties;
> > +}
> > +
> > +static u8 *mikrobus_property_link_get(struct addon_board_info *info, u8 prop_id,
> > +                                                                       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, &info->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);
> > +     return val_u8;
> > +}
> > +
> > +static int mikrobus_manifest_attach_device(struct addon_board_info *info,
> > +                                             struct greybus_descriptor_device *dev_desc)
> > +{
> > +     struct board_device_info *dev;
> > +     struct gpiod_lookup_table *lookup;
> > +     struct greybus_descriptor_property *desc_property;
> > +     struct manifest_desc *descriptor;
> > +     int i;
> > +     u8 *prop_link;
> > +     u8 *gpio_desc_link;
> > +
> > +     dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +     if (!dev)
> > +             return -ENOMEM;
> > +     dev->id = dev_desc->id;
> > +     dev->drv_name = mikrobus_string_get(info, dev_desc->driver_stringid);
> > +     dev->protocol = dev_desc->protocol;
> > +     dev->reg = dev_desc->reg;
> > +     dev->irq = dev_desc->irq;
> > +     dev->irq_type = dev_desc->irq_type;
> > +     dev->max_speed_hz = le32_to_cpu(dev_desc->max_speed_hz);
> > +     dev->mode = dev_desc->mode;
> > +     dev->cs_gpio = dev_desc->cs_gpio;
> > +     dev->num_gpio_resources = dev_desc->num_gpio_resources;
> > +     dev->num_properties = dev_desc->num_properties;
> > +     pr_info("device %d , number of properties=%d , number of gpio resources=%d\n",
> > +     dev->id, dev->num_properties, dev->num_gpio_resources);
> > +     if (dev->num_properties > 0) {
> > +             prop_link = mikrobus_property_link_get(info, dev_desc->prop_link,
> > +                                                             MIKROBUS_PROPERTY_TYPE_LINK);
> > +             dev->properties = mikrobus_property_entry_get(info, prop_link, dev->num_properties);
> > +     }
> > +     if (dev->num_gpio_resources > 0) {
> > +             lookup = kzalloc(struct_size(lookup, table, dev->num_gpio_resources),
> > +                                     GFP_KERNEL);
> > +     if (!lookup)
> > +             return -ENOMEM;
> > +     gpio_desc_link = mikrobus_property_link_get(info, dev_desc->gpio_link,
> > +                                             MIKROBUS_PROPERTY_TYPE_GPIO);
> > +     for (i = 0; i < dev->num_gpio_resources; i++) {
> > +             list_for_each_entry(descriptor, &info->manifest_descs, links) {
> > +                     if (descriptor->type != GREYBUS_TYPE_PROPERTY)
> > +                             continue;
> > +                     desc_property = descriptor->data;
> > +                     if (desc_property->id == gpio_desc_link[i]) {
> > +                             lookup->table[i].chip_hwnum = *desc_property->value;
> > +                             lookup->table[i].con_id =
> > +                             mikrobus_string_get(info, desc_property->propname_stringid);
> > +                             break;
> > +                             }
> > +             }
> > +     }
> > +     dev->gpio_lookup = lookup;
> > +     }
> > +     list_add_tail(&dev->links, &info->devices);
> > +     return 0;
> > +}
> > +
> > +static int mikrobus_manifest_parse_devices(struct addon_board_info *info)
> > +{
> > +     struct greybus_descriptor_device *desc_device;
> > +     struct manifest_desc *desc, *next;
> > +     int devcount = 0;
> > +
> > +     if (WARN_ON(!list_empty(&info->devices)))
> > +             return false;
> > +     list_for_each_entry_safe(desc, next, &info->manifest_descs, links) {
> > +             if (desc->type != GREYBUS_TYPE_DEVICE)
> > +                     continue;
> > +             desc_device = desc->data;
> > +             mikrobus_manifest_attach_device(info, desc_device);
> > +             devcount++;
> > +     }
> > +     return devcount;
> > +}
> > +
> > +bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> > +                                                      size_t size)
> > +{
> > +     struct greybus_manifest *manifest;
> > +     struct greybus_manifest_header *header;
> > +     struct greybus_descriptor *desc;
> > +     u16 manifest_size;
> > +     int dev_count;
> > +     int desc_size;
> > +
> > +     if (WARN_ON(!list_empty(&info->manifest_descs)))
> > +             return false;
> > +     if (size < sizeof(*header))
> > +             return false;
> > +     manifest = data;
> > +     header = &manifest->header;
> > +     manifest_size = le16_to_cpu(header->size);
> > +     if (manifest_size != size)
> > +             return false;
> > +     if (header->version_major > MIKROBUS_VERSION_MAJOR)
> > +             return false;
> > +     desc = manifest->descriptors;
> > +     size -= sizeof(*header);
> > +     while (size) {
> > +             desc_size = identify_descriptor(info, desc, size);
> > +             if (desc_size < 0) {
> > +                     pr_err("invalid manifest descriptor");
> > +             return -EINVAL;
> > +             }
> > +             desc = (struct greybus_descriptor *)((char *)desc + desc_size);
> > +             size -= desc_size;
> > +     }
> > +     mikrobus_state_get(info);
> > +     dev_count = mikrobus_manifest_parse_devices(info);
> > +     pr_info(" %s manifest parsed with %d device(s)\n", info->name,
> > +             info->num_devices);
> > +     release_manifest_descriptors(info);
> > +     return true;
> > +}
> > +
> > +size_t mikrobus_manifest_header_validate(void *data, size_t size)
> > +{
> > +     struct greybus_manifest_header *header;
> > +     u16 manifest_size;
> > +
> > +     if (size < sizeof(*header))
> > +             return 0;
> > +
> > +     header = data;
> > +     manifest_size = le16_to_cpu(header->size);
> > +     if (header->version_major > MIKROBUS_VERSION_MAJOR)
> > +             return 0;
> > +     return manifest_size;
> > +}
> > diff --git a/drivers/misc/mikrobus/mikrobus_manifest.h b/drivers/misc/mikrobus/mikrobus_manifest.h
> > new file mode 100644
> > index 000000000000..a195d1c26493
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/mikrobus_manifest.h
> > @@ -0,0 +1,21 @@
> > +/* 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"
> > +
> > +bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> > +                                                      size_t size);
> > +size_t mikrobus_manifest_header_validate(void *data, size_t size);
> > +
> > +#endif /* __MIKROBUS_MANIFEST_H */
> > diff --git a/include/linux/greybus/greybus_manifest.h b/include/linux/greybus/greybus_manifest.h
> > index 6e62fe478712..79c8cab9ef96 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,53 @@ struct greybus_descriptor_cport {
> >       __u8    protocol_id;    /* enum greybus_protocol */
> >  } __packed;
> >
> > +/*
> > + * A mikrobus descriptor is used to describe the details
> > + * about the add-on board connected to the mikrobus port.
> > + * It describes the number of indivdual devices on the
> > + * add-on board and the default states of the GPIOs
> > + */
> > +struct greybus_descriptor_mikrobus {
> > +     __u8 num_devices;
> > +     __u8 rst_gpio_state;
> > +     __u8 pwm_gpio_state;
> > +     __u8 int_gpio_state;
> > +} __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[0];
> > +} __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 num_properties;
> > +     __u8 protocol;
> > +     __le32 max_speed_hz;
> > +     __u8 reg;
> > +     __u8 mode;
> > +     __u8 num_gpio_resources;
> > +     __u8 cs_gpio;
> > +     __u8 irq;
> > +     __u8 irq_type;
> > +     __u8 prop_link;
> > +     __u8 gpio_link;
> > +} __packed;
> > +
> >  struct greybus_descriptor_header {
> >       __le16  size;
> >       __u8    type;           /* enum greybus_descriptor_type */
> > @@ -164,6 +214,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;
> >
> >
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [greybus-dev] [PATCH] RFC : mikroBUS driver for add-on boards
  2020-07-26 15:04 ` Alex Elder
@ 2020-07-26 19:12   ` Vaishnav M A
  2020-07-26 21:54     ` Alex Elder
  0 siblings, 1 reply; 7+ messages in thread
From: Vaishnav M A @ 2020-07-26 19:12 UTC (permalink / raw)
  To: Alex Elder
  Cc: greybus-dev, linux-kernel, gregkh, arnd, johan, elder, rajkovic,
	robh, mchehab+huawei, Robert Nelson, Drew Fustini, davem,
	zoran.stojsavljevic

On Sun, Jul 26, 2020 at 8:34 PM Alex Elder <elder@linaro.org> wrote:
>
> On 7/24/20 7:06 AM, Vaishnav M A wrote:
> > Attached is a patch for the mikroBUS driver which helps to
> > instantiate an add-on board device on a mikrobus port by fetching
> > the device identifier manifest binary from an EEPROM on-board
> > the device. mikroBUS is an add-on board socket standard by
> > MikroElektronika that can be freely used by anyone
> > following the guidelines, more details and discussions on
> > the status of the driver can be found here in this eLinux wiki:
> > https://elinux.org/Mikrobus
>
> I had some other things I would comment on in this code review,
> but there are a lot of somewhat superficial things you should
> fix.  I looked at much--but not all--of this, and I'll want to
> review this again after you've fixed the simple things.  I also
> can provide more substantive feedback after I've had more time
> to digest the bigger picture of microBUS.
>
Thanks again Alex for the review and the suggestions, I have gone
through and will make the suggested changes wherever they
are applicable and get back. I hope the document Jason shared
will have enough details on the hardware and software
architecture to give you an overview.
> You should assume it will take a few iterations to get to the
> point where things are shaping up for acceptance.
>
Sure, this is understood
> All that said, your basic foundation looks good.
>
> > In the current state of the driver, more than 80 different
> > add-on boards have been tested on the BeagleBoard.org
> > PocketBeagle and the manifest binary is generated using the same
> > manifesto tool used to generate Greybus manifest binaries,
> > The pull request to manifesto to add new descriptors specific
> > to mikrobus is here : https://github.com/projectara/manifesto/pull/2
> > The utilization of Greybus manifest binaries here is not entirely
> > coincidental, We are evaluating ways to add mikroBUS sockets and
> > devices via Greybus expansion.
> >
> > The mikroBUS standard includes SPI, I2C, UART, PWM, ADC, GPIO
> > and power (3.3V and 5V) connections to interface common embedded
> > peripherals, There are more than 750 add-on boards ranging from
> > wireless connectivity boards to human-machine interface sensors
> > which conform to the mikroBUS standard, out of which more than 140
> > boards already have support in the Linux kernel.Today, there is no
> > mainlinesolution for enabling mikroBUS add-on boards at run-time, the
> > most straight forward method for loading drivers is to provide
> > device-tree overlay fragments at boot time, this method suffers
> > from the need to maintain a large out-of-tree database for which there
> > is need to maintain an overlay for every mikroBUS add-on board for every
> > Linux system and for every mikroBUS socket on that system.
> >
> > The mikroBUS driver tries to solve the problem by using extended version
> > of the greybus manifest to describe the add-on board device specific
> > information required by the device driver and uses it along with the fixed
> > port specific information to probe the specific device driver.The manifest
> > binary is now fetched from an I2C EEPROM over the I2C bus on the mikroBUS
> > port(subject to change in mikroBUS v3 specification) and enumerate drivers
> > for the add-on devices.There is also ongoing work to define a mikroBUS
> > v3 standard in which the addon board includes a non-volatile storage to
> > store the device identifier manifest binary, once the mikroBUS v3 standard
> > is released, the mikroBUS can be seen as a probeable bus such that the
> > kernel can discover the device on the bus at boot time.
> >
> > The driver also has a few debug SysFS interfaces for testing on add-on
> > boards without an EEPROM, these can be used in the following manner:
> > (example for mikroBUS port 1 on BeagleBoard.org PocketBeagle):
>
> You should probably use debugfs, since this is a temporary thing.
> But I guess if it's not actually upstream (and it sounds like you
> might be avoiding the need for this with EEPROM anyway) maybe
> that doesn't matter much.
>
> > printf "%b" '\x01\x00\x00\x59\x32\x17' > /sys/bus/mikrobus/add_port
> >
> > The bytes in the byte array sequence are (in order):
> >
> >       * i2c_adap_nr
> >       * spi_master_nr
> >       * serdev_ctlr_nr
> >       * rst_gpio_nr
> >       * pwm_gpio_nr
> >       * int_gpio_nr
> > * add_port sysFS entry is purely for debug and in the actual state
> > of the driver, port configuration will be loaded from a suitable
> > device tree overlay fragment.
> >
> > echo 0 > /sys/bus/mikrobus/del_port (to delete the attached port)
> > echo 1 >  /sys/class/mikrobus-port/mikrobus-0/rescan (to rescan the EEPROM
> > contents on the I2C bus on the mikroBUS port).
> >
> > cat board_manifest.mnfb >  /sys/class/mikrobus-port/mikrobus-0/new_device
> > * debug interface to pass the manifest binary in case an EEPROM is absent
> > echo 0 >  /sys/class/mikrobus-port/mikrobus-0/delete_device
> > * to unload the loaded board on the mikrobus port
> >
> > These sysFS interfaces are only implemented for debug purposes and
> > in the actual implementation of the driver these will be removed and
> > the manifest binary will be fetched from the non volatile storage on-board
> > the device.
> >
> > The mikroBUS driver enable the mikroBUS to be a probeable bus such that
> > the kernel can discover the device and automatically load the drivers.
> > There are already several Linux platforms with mikroBUS sockets and the
> > mikroBUS driver helps to reduce the time to develop and debug
> > support for various mikroBUS add-on boards. Further, it opens up
> > the possibility for support under dynamically instantiated buses
> > such as with Greybus.
> >
> > Please let know the feedback you have on this patch or the approach used.
> >
> > Thanks,
> >
> > Vaishnav M A
> >
> > Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org>
> > ---
> >  MAINTAINERS                               |   6 +
> >  drivers/misc/Kconfig                      |   1 +
> >  drivers/misc/Makefile                     |   1 +
> >  drivers/misc/mikrobus/Kconfig             |  16 +
> >  drivers/misc/mikrobus/Makefile            |   5 +
> >  drivers/misc/mikrobus/mikrobus_core.c     | 649 ++++++++++++++++++++++
> >  drivers/misc/mikrobus/mikrobus_core.h     | 130 +++++
> >  drivers/misc/mikrobus/mikrobus_manifest.c | 390 +++++++++++++
> >  drivers/misc/mikrobus/mikrobus_manifest.h |  21 +
> >  include/linux/greybus/greybus_manifest.h  |  53 ++
> >  10 files changed, 1272 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 d53db30d1365..9a049746203f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11402,6 +11402,12 @@ M:   Oliver Neukum <oliver@neukum.org>
> >  S:   Maintained
> >  F:   drivers/usb/image/microtek.*
> >
> > +MIKROBUS ADDON BOARD DRIVER
> > +M:   Vaishnav M A <vaishnav@beagleboard.org>
> > +S:   Maintained
> > +W:   https://elinux.org/Mikrobus
> > +F:   drivers/misc/mikrobus/
> > +
> >  MIPS
> >  M:   Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> >  L:   linux-mips@vger.kernel.org
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index e1b1ba5e2b92..334f0c39d56b 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -472,4 +472,5 @@ source "drivers/misc/ocxl/Kconfig"
> >  source "drivers/misc/cardreader/Kconfig"
> >  source "drivers/misc/habanalabs/Kconfig"
> >  source "drivers/misc/uacce/Kconfig"
> > +source "drivers/misc/mikrobus/Kconfig"
> >  endmenu
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index c7bd01ac6291..45486dd77da5 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -40,6 +40,7 @@ obj-$(CONFIG_VMWARE_BALLOON)        += vmw_balloon.o
> >  obj-$(CONFIG_PCH_PHUB)               += pch_phub.o
> >  obj-y                                += ti-st/
> >  obj-y                                += lis3lv02d/
> > +obj-y                                += mikrobus/
> >  obj-$(CONFIG_ALTERA_STAPL)   +=altera-stapl/
> >  obj-$(CONFIG_INTEL_MEI)              += mei/
> >  obj-$(CONFIG_VMWARE_VMCI)    += vmw_vmci/
> > diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig
> > new file mode 100644
> > index 000000000000..c3b93e12daad
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/Kconfig
> > @@ -0,0 +1,16 @@
> > +menuconfig MIKROBUS
> > +     tristate "Module for instantiating devices on mikroBUS ports"
> > +     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 helps in
> > +       instantiating devices on the mikroBUS port with identifier
> > +       data fetched from an EEPROM on the device, more details on
> > +       the mikroBUS driver support and discussion can be found in
> > +       this eLinux wiki : elinux.org/Mikrobus
>
> This text could be cleaned up a bit.  For example:
>         The mikroBUS driver instantiates devices on a mikroBUS port
>         described by identifying data present in device-resident EEPROM.
>
> Using well-defined terms can help a lot.  Is a physical thing that
> plugs into a microbus port called a "board"?  Can a "board" present
> more than one device to the system?  Is the EEPROM associated with
> the board, or a device?  My point isn't that those answers belong
> here, but that having meaningful terms can help you describe things
> concisely.
Will refine the description here, as Jason already replied to this,
I will add to that, only a very few add-on boards present more than
one device to the system, the mikrobus driver and additions to
manifest is made considering this fact and can support multiple
devices within a single add-on board.
>
> > +       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..1f80ed4064d8
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# mikroBUS Core
> > +
> > +mikrobus-y :=        mikrobus_core.o mikrobus_manifest.o
> > +obj-$(CONFIG_MIKROBUS) += mikrobus.o
> > \ No newline at end of file
> > diff --git a/drivers/misc/mikrobus/mikrobus_core.c b/drivers/misc/mikrobus/mikrobus_core.c
> > new file mode 100644
> > index 000000000000..78c96c637632
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/mikrobus_core.c
> > @@ -0,0 +1,649 @@
> > +// 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.
> > + */
> > +
> > +#define pr_fmt(fmt) "mikrobus: " fmt
> > +
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/idr.h>
> > +#include <linux/init.h>
> > +#include <linux/jump_label.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/mutex.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/machine.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/nvmem-provider.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/serdev.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +
> > +#include "mikrobus_core.h"
> > +#include "mikrobus_manifest.h"
> > +
> > +#define ATMEL_24C32_I2C_ADDR 0x57
>
> I'm just getting started looking through this, so maybe I'll find
> out soon. But why is this ATMEL I2C address needed in the core code?
>
This is the address for the EEPROM on the I2C  bus, this is temporary
and is not expected to exist in the actual driver code as the EEPROM
will most probably not be on the I2C bus and even if there is any
configuration required it will go with the device tree fragment that
contains the port configuration.

> > +static DEFINE_IDR(mikrobus_port_idr);
> > +static struct class_compat *mikrobus_port_compat_class;
> > +static bool is_registered;
> > +
> > +static ssize_t add_port_store(struct bus_type *bt, const char *buf,
> > +                           size_t count)
> > +{
> > +     struct mikrobus_port_config *cfg;
> > +
> > +     if (count < sizeof(*cfg)) {
>
> Perhaps this should be:
>         if (count != sizeof(*cfg)) {
>
> > +             pr_err("add_port: incorrect config data received: %s\n", buf);
>
> I don't think you need to include "add_port: ".
>
> This is binary data you are writing to this sysfs file, correct?
Yes, this is the port config binary data writtent to the sysfs file.
> Don't try to interpret it as a string.  You could avoid the problem
> with something more like:
>     pr_err("bad port config size (%zu, should be %zu)", count, sizeof(*cfg));
>
> > +             return -EINVAL;
> > +     }
> > +     mikrobus_register_port_config((void *)buf);
>
> mikrobus_register_port_config() returns an error value, and
> that should be returned from this function if it's non-zero.
>
> Don't cast the buffer to a void pointer; cast it to the actual
> type represents (struct mikrobus_port_config *).
>
> > +     return count;
> > +}
> > +BUS_ATTR_WO(add_port);
> > +
> > +static ssize_t del_port_store(struct bus_type *bt, const char *buf,
> > +                                                       size_t count)
> > +{
> > +     int id;
> > +     char end;
> > +     int res;
> > +
> > +     res = sscanf(buf, "%d%c", &id, &end);
>
> The id value you pass to idr_find() is an unsigned long.
> You might as well define "id" with that type and scan that
> type here.  Make sure it's in the valid range for your
> identifier as a separate step.  (There might be a good
> reason you use a signed int as an identifier, but I would
> recommend unsigned, with a well-defined number of bits,
> such as u32).
>
> Is "end" intended to just consume an optional trailing newline?
yes, end was intended to capture a trailing newline.
>
> > +     if (res < 1) {
> > +             pr_err("delete_port: cannot parse mikrobus port ID\n");
>
> I don't think "delete_port: " here is necessary.  (Take this
> comment to apply in all similar cases; I won't mention it
> again.)
>
> > +             return -EINVAL;
> > +     }
> > +     if (!idr_find(&mikrobus_port_idr, id)) {
> > +             pr_err("attempting to delete unregistered port [%d]\n", id);
>
> Maybe just "mikrobus port %lu not registered".
>
> > +             return -EINVAL;
> > +     }
> > +     mikrobus_del_port(idr_find(&mikrobus_port_idr, id));
> > +     return count;
> > +}
> > +BUS_ATTR_WO(del_port);
> > +
> > +static struct attribute *mikrobus_attrs[] = {
> > +     &bus_attr_add_port.attr,
> > +     &bus_attr_del_port.attr,
> > +      NULL
> > +};
> > +ATTRIBUTE_GROUPS(mikrobus);
> > +
> > +struct bus_type mikrobus_bus_type = {
> > +     .name = "mikrobus",
> > +     .bus_groups = mikrobus_groups,
> > +};
> > +EXPORT_SYMBOL_GPL(mikrobus_bus_type);
> > +
> > +static int mikrobus_manifest_header_validate(struct mikrobus_port *port)
> > +{
> > +     char header[12];
> > +     struct addon_board_info *board;
> > +     int manifest_size;
> > +     int retval;
> > +     char *buf;
> > +
> > +     nvmem_device_read(port->eeprom, 0, 12, header);
>
> This function returns a value, and if it's less than zero you
> should return it as the result of mikrobus_manifest_header_validate()
> (possibly after reporting an error).
>
> > +     manifest_size = mikrobus_manifest_header_validate(header, 12);
> > +     if (manifest_size > 0) {
> > +             buf = kzalloc(manifest_size, GFP_KERNEL);
>
> Check whether kzalloc() returns NULL, and if so, return -ENOMEM.
>
> > +             nvmem_device_read(port->eeprom, 0, manifest_size, buf);
>
> Check the return value here, and if negative, free your buffer
> and return the error.  (I won't make this comment any more if
> I see other instances of ignoring the nvmem_device_read()
> return value.)
>
> > +             board = kzalloc(sizeof(*board), GFP_KERNEL);
> > +             if (!board)
> > +                     return -ENOMEM;
>
> You need to free the buffer you allocated before you return here.
> It is helpful to use the "goto <error path>" pattern.  I.e.:
>
>         if (!board) {
>                 retval = -ENOMEM;
>                 goto err_free_buf;
>         }
> ...
>
> err_free_buf:
>         kfree(buf);
>
>         return retval;
> }
>
> > +             INIT_LIST_HEAD(&board->manifest_descs);
> > +             INIT_LIST_HEAD(&board->devices);
> > +             retval = mikrobus_manifest_parse(board, (void *)buf, manifest_size);
>
> No need to cast buf to (void *).
>
> I have more comments on mikrobus_manifest_parse() below.  But it
> might be useful to have it return an integer (0 or error code)
> rather than Boolean.  Assuming you did that...
>
> > +             if (!retval) {
> > +                     pr_err("failed to parse manifest, size: %d", manifest_size);
>
>         if (retval)
>                 goto err_free_board;
> ...
>
> err_free_board:
>         free_board(board);
> err_free_buf:
>         free_buf(buf);
>
>         return retval;
> }
>
> > +                     return -EINVAL;
> > +             }
> > +             retval = mikrobus_register_board(port, board);
> > +             if (retval) {
> > +                     pr_err("failed to register board: %s", board->name);
>
>         goto err_free_board;
>
> > +                     return -EINVAL;
> > +             }
> > +             kfree(buf);
> > +     } else {
> > +             pr_err("inavlide manifest port %d", port->id);
>
> s/inavlide/invalid/
>
> > +             return -EINVAL;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> > +                                              char *buf)
> > +{
> > +     return sprintf(buf, "%s\n", to_mikrobus_port(dev)->name);
> > +}
> > +static DEVICE_ATTR_RO(name);
> > +
> > +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 retval;
> > +
> > +     if (port->board == NULL) {
>
> This is just a style suggestion, but I would prefer this, because it
> reduces the indentation of the normal path:
>
>         if (port->board) {
>                 pr_err("mikrobus port %d already has a board registered\n",
>                         port->id);
>                 return -EBUSY;
>         }
>
>         port->board = kzalloc(...);
>         if (!port->board)
>                 return -ENOMEM;
>
> Also note the return values I suggest here.
>
> > +             board = kzalloc(sizeof(*board), GFP_KERNEL);
> > +             if (!board)
> > +                     return -EINVAL;
> > +             INIT_LIST_HEAD(&board->manifest_descs);
> > +             INIT_LIST_HEAD(&board->devices);
> > +     } else {
> > +             pr_err("port %d already has board registered", port->id);
> > +             return -EINVAL;
> > +     }
> > +     retval = mikrobus_manifest_parse(board, (void *)buf, count);
> > +     if (!retval) {
> > +             pr_err("failed to parse manifest");
> > +             return -EINVAL;
> > +     }
> > +     retval = mikrobus_register_board(port, board);
> > +     if (retval) {
> > +             pr_err("failed to register board: %s", board->name);
> > +             return -EINVAL;
> > +     }
> > +     return count;
> > +}
> > +static DEVICE_ATTR_WO(new_device);
> > +
> > +static ssize_t rescan_store(struct device *dev, struct device_attribute *attr,
> > +                                                     const char *buf, size_t count)
> > +{
> > +     struct mikrobus_port *port = to_mikrobus_port(dev);
> > +     int id;
> > +     char end;
> > +     int res;
> > +     int retval;
> > +
> > +     res = sscanf(buf, "%d%c", &id, &end);
> > +     if (res < 1) {
> > +             pr_err("rescan: Can't parse trigger\n");
> > +             return -EINVAL;
> > +     }
> > +     if (port->board != NULL) {
> > +             pr_err("port %d already has board registered", port->id);
> > +             return -EINVAL;
> > +     }
> > +     retval = mikrobus_port_scan_eeprom(port);
> > +     if (retval) {
> > +             pr_err("port %d board register from manifest failed", port->id);
> > +             return -EINVAL;
> > +     }
> > +     return count;
> > +}
> > +static DEVICE_ATTR_WO(rescan);
> > +
> > +static ssize_t delete_device_store(struct device *dev, struct device_attribute *attr,
> > +                                                     const char *buf, size_t count)
> > +{
> > +     int id;
> > +     char end;
> > +     int res;
> > +     struct mikrobus_port *port = to_mikrobus_port(dev);
> > +
> > +     res = sscanf(buf, "%d%c", &id, &end);
> > +     if (res < 1) {
> > +             pr_err("delete_board: Can't parse board ID\n");
> > +             return -EINVAL;
> > +     }
> > +     if (port->board == NULL) {
>
> Normally in kernel code this form is used:
>
>         if (!port->board) {
>
> > +             pr_err("delete_board: port does not have any boards registered\n");
> > +             return -EINVAL;
> > +     }
> > +     mikrobus_unregister_board(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_rescan.attr,
> > +     &dev_attr_delete_device.attr, &dev_attr_name.attr, NULL};
> > +ATTRIBUTE_GROUPS(mikrobus_port);
> > +
> > +static void mikrobus_dev_release(struct device *dev)
> > +{
> > +     struct mikrobus_port *port = to_mikrobus_port(dev);
> > +
> > +     idr_remove(&mikrobus_port_idr, port->id);
> > +     kfree(port);
> > +}
> > +
> > +struct device_type mikrobus_port_type = {
> > +     .groups = mikrobus_port_groups,
> > +     .release = mikrobus_dev_release,
> > +};
> > +EXPORT_SYMBOL_GPL(mikrobus_port_type);
> > +
> > +static int mikrobus_get_irq(struct mikrobus_port *port, int irqno,
> > +                                                     int irq_type)
> > +{
> > +     int irq;
> > +
> > +     switch (irqno) {
> > +     case MIKROBUS_GPIO_INT:
> > +     irq = gpiod_to_irq(port->int_gpio);
> > +     break;
>
> Please fix your indentation here.  (And everywhere; I give
> an example of the proper way to do it below.)
>
> > +     case MIKROBUS_GPIO_RST:
> > +     irq = gpiod_to_irq(port->rst_gpio);
> > +     break;
> > +     case MIKROBUS_GPIO_PWM:
> > +     irq = gpiod_to_irq(port->pwm_gpio);
> > +     break;
> > +     default:
> > +     return -EINVAL;
> > +     }
> > +     if (irq < 0) {
> > +             pr_err("Could not get irq for irq type: %d", irqno);
> > +             return -EINVAL;
> > +     }
> > +     irq_set_irq_type(irq, irq_type);
>
> It shouldn't return an error, but please check the
> return value here.
>
> > +     return irq;
> > +}
> > +
> > +static int mikrobus_setup_gpio(struct gpio_desc *gpio, int gpio_state)
> > +{
> > +     int retval;
> > +
> > +     if (gpio_state == MIKROBUS_GPIO_UNUSED)
> > +             return 0;
> > +     switch (gpio_state) {
> > +     case MIKROBUS_GPIO_INPUT:
> > +     retval = gpiod_direction_input(gpio);
> > +     break;
> > +     case MIKROBUS_GPIO_OUTPUT_HIGH:
> > +     retval = gpiod_direction_output(gpio, 1);
> > +     gpiod_set_value_cansleep(gpio, 1);
> > +     break;
> > +     case MIKROBUS_GPIO_OUTPUT_LOW:
> > +     retval = gpiod_direction_output(gpio, 0);
> > +     gpiod_set_value_cansleep(gpio, 0);
> > +     break;
> > +     default:
> > +     return -EINVAL;
> > +     }
> > +     return retval;
> > +}
> > +
> > +static void mikrobus_spi_device_delete(struct spi_master *master, unsigned int cs)
> > +{
> > +     struct device *dev;
> > +     char str[32];
> > +
> > +     snprintf(str, sizeof(str), "%s.%u", dev_name(&master->dev), cs);
> > +     dev = bus_find_device_by_name(&spi_bus_type, NULL, str);
> > +     if (dev != NULL) {
> > +             spi_unregister_device(to_spi_device(dev));
> > +             put_device(dev);
> > +     }
> > +}
> > +
> > +static char *mikrobus_get_gpio_chip_name(struct mikrobus_port *port, int gpio)
> > +{
> > +     char *name;
> > +     struct gpio_chip *gpiochip;
> > +
> > +     switch (gpio) {
> > +     case MIKROBUS_GPIO_INT:
> > +     gpiochip = gpiod_to_chip(port->int_gpio);
> > +     name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
>
> Why 40?  Please use a symbolic constant so you can
> change it easily, and to give you a place to explain
> why 40 is the limit used.
>
> > +     break;
> > +     case MIKROBUS_GPIO_RST:
> > +     gpiochip = gpiod_to_chip(port->rst_gpio);
> > +     name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> > +     break;
> > +     case MIKROBUS_GPIO_PWM:
> > +     gpiochip = gpiod_to_chip(port->pwm_gpio);
> > +     name = kmemdup(gpiochip->label, 40, GFP_KERNEL);
> > +     break;
> > +     }
> > +     return name;
> > +}
> > +
> > +static int mikrobus_get_gpio_hwnum(struct mikrobus_port *port, int gpio)
> > +{
> > +     int hwnum;
> > +     struct gpio_chip *gpiochip;
> > +
> > +     switch (gpio) {
> > +     case MIKROBUS_GPIO_INT:
> > +     gpiochip = gpiod_to_chip(port->int_gpio);
> > +     hwnum = desc_to_gpio(port->int_gpio) - gpiochip->base;
> > +     break;
> > +     case MIKROBUS_GPIO_RST:
> > +     gpiochip = gpiod_to_chip(port->rst_gpio);
> > +     hwnum = desc_to_gpio(port->rst_gpio) - gpiochip->base;
> > +     break;
> > +     case MIKROBUS_GPIO_PWM:
> > +     gpiochip = gpiod_to_chip(port->pwm_gpio);
> > +     hwnum = desc_to_gpio(port->pwm_gpio) - gpiochip->base;
> > +     break;
> > +     }
> > +     return hwnum;
> > +}
> > +
> > +static void release_mikrobus_device(struct board_device_info *dev)
>
> We tend to follow a convention throughout the Greybus code
> that has the object name as the prefix for things you do
> to the object.  I don't know how consistent that is, but
> my suggestion would be that these functions would be named
> something more like:
>     mikrobus_gpio_chip_name_get()
>     mikrobus_gpio_hwnum_get()
>     mikrobus_board_release_device_all()
>     mikrobus_device_register()
>     mikrobus_device_unregister()
>     mikrobus_board_register()
>     mikrobus_board_unregister()
> and so on.
>
> > +{
> > +     list_del(&dev->links);
> > +     kfree(dev);
> > +}
> > +
>
> (I skipped reviewing a lot here...)
> . . .
>
> > diff --git a/drivers/misc/mikrobus/mikrobus_core.h b/drivers/misc/mikrobus/mikrobus_core.h
> > new file mode 100644
> > index 000000000000..9684d315f564
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/mikrobus_core
> . . .
>
> > diff --git a/drivers/misc/mikrobus/mikrobus_manifest.c b/drivers/misc/mikrobus/mikrobus_manifest.c
> > new file mode 100644
> > index 000000000000..60ebca560f0d
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/mikrobus_manifest.c
> > @@ -0,0 +1,390 @@
> > +// 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.
> > + */
> > +
> > +#define pr_fmt(fmt) "mikrobus_manifest: " fmt
> > +
> > +#include <linux/bits.h>
> > +#include <linux/types.h>
> > +#include <linux/property.h>
> > +#include <linux/greybus/greybus_manifest.h>
> > +
> > +#include "mikrobus_manifest.h"
> > +
> > +struct manifest_desc {
> > +     struct list_head links;
> > +     size_t size;
> > +     void *data;
> > +     enum greybus_descriptor_type type;
> > +};
> > +
> > +static void release_manifest_descriptor(struct manifest_desc *descriptor)
> > +{
> > +     list_del(&descriptor->links);
> > +     kfree(descriptor);
> > +}
> > +
> > +static void release_manifest_descriptors(struct addon_board_info *info)
> > +{
> > +     struct manifest_desc *descriptor;
> > +     struct manifest_desc *next;
> > +
> > +     list_for_each_entry_safe(descriptor, next, &info->manifest_descs, links)
> > +             release_manifest_descriptor(descriptor);
> > +}
> > +
> > +static int identify_descriptor(struct addon_board_info *info, struct greybus_descriptor *desc,
> > +                                                                              size_t size)
> > +{
>
> I know it's technically acceptable now, but please limit your lines to
> 80 characters in the Greybus code if possible.
>
> > +     struct greybus_descriptor_header *desc_header = &desc->header;
> > +     struct manifest_desc *descriptor;
> > +     size_t desc_size;
> > +     size_t expected_size;
> > +
> > +     if (size < sizeof(*desc_header))
> > +             return -EINVAL;
> > +     desc_size = le16_to_cpu(desc_header->size);
> > +     if (desc_size > size)
> > +             return -EINVAL;
>
> Probably check if (desc_size != size) here.
>
> > +     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;
>
> Your indentation is wrong here.  Please use:
>
>         switch (desc_header->type) {
>         case GREYBUS_TYPE_STRING:
>                 expected_size += ...;
>                 ...
>                 break;
>         case GREYBUS_TYPE_PROPERTY:
>                 ...
>
> > +     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:
> > +     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, &info->manifest_descs);
> > +     return desc_size;
> > +}
>
> . . .
>
> > +static int mikrobus_manifest_attach_device(struct addon_board_info *info,
> > +                                             struct greybus_descriptor_device *dev_desc)
> > +{
> > +     struct board_device_info *dev;
>
> I would suggest something other than "dev" as the name of
> a board_device.  The use of "dev" for (struct device *)
> is pretty prevalent in the kernel, and so using it here
> can be more confusing than it has to be.
>
> > +     struct gpiod_lookup_table *lookup;
> > +     struct greybus_descriptor_property *desc_property;
> > +     struct manifest_desc *descriptor;
> > +     int i;
> > +     u8 *prop_link;
> > +     u8 *gpio_desc_link;
> > +
> > +     dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +     if (!dev)
> > +             return -ENOMEM;
> > +     dev->id = dev_desc->id;
> > +     dev->drv_name = mikrobus_string_get(info, dev_desc->driver_stringid);
>
> This can return NULL.  You need to check for that, and free
> the board device you have already allocated.
>
> > +     dev->protocol = dev_desc->protocol;
> > +     dev->reg = dev_desc->reg;
> > +     dev->irq = dev_desc->irq;
> > +     dev->irq_type = dev_desc->irq_type;
> > +     dev->max_speed_hz = le32_to_cpu(dev_desc->max_speed_hz);
> > +     dev->mode = dev_desc->mode;
> > +     dev->cs_gpio = dev_desc->cs_gpio;
> > +     dev->num_gpio_resources = dev_desc->num_gpio_resources;
> > +     dev->num_properties = dev_desc->num_properties;
> > +     pr_info("device %d , number of properties=%d , number of gpio resources=%d\n",
> > +     dev->id, dev->num_properties, dev->num_gpio_resources);
> > +     if (dev->num_properties > 0) {
> > +             prop_link = mikrobus_property_link_get(info, dev_desc->prop_link,
> > +                                                             MIKROBUS_PROPERTY_TYPE_LINK);
> > +             dev->properties = mikrobus_property_entry_get(info, prop_link, dev->num_properties);
> > +     }
> > +     if (dev->num_gpio_resources > 0) {
> > +             lookup = kzalloc(struct_size(lookup, table, dev->num_gpio_resources),
> > +                                     GFP_KERNEL);
> > +     if (!lookup)
> > +             return -ENOMEM;
>
> You can't return without freeing your previously-allocated resources.
>
> > +     gpio_desc_link = mikrobus_property_link_get(info, dev_desc->gpio_link,
> > +                                             MIKROBUS_PROPERTY_TYPE_GPIO);
> > +     for (i = 0; i < dev->num_gpio_resources; i++) {
> > +             list_for_each_entry(descriptor, &info->manifest_descs, links) {
> > +                     if (descriptor->type != GREYBUS_TYPE_PROPERTY)
> > +                             continue;
> > +                     desc_property = descriptor->data;
> > +                     if (desc_property->id == gpio_desc_link[i]) {
> > +                             lookup->table[i].chip_hwnum = *desc_property->value;
> > +                             lookup->table[i].con_id =
> > +                             mikrobus_string_get(info, desc_property->propname_stringid);
> > +                             break;
> > +                             }
> > +             }
> > +     }
> > +     dev->gpio_lookup = lookup;
> > +     }
> > +     list_add_tail(&dev->links, &info->devices);
> > +     return 0;
> > +}
> > +
> > +static int mikrobus_manifest_parse_devices(struct addon_board_info *info)
> > +{
> > +     struct greybus_descriptor_device *desc_device;
> > +     struct manifest_desc *desc, *next;
> > +     int devcount = 0;
> > +
> > +     if (WARN_ON(!list_empty(&info->devices)))
> > +             return false;
>
> The manifest comes from outside the kernel  I might be misunderstanding
> something, but you should not be using WARN_ON() if its content doesn't
> match what you expect.
>
> > +     list_for_each_entry_safe(desc, next, &info->manifest_descs, links) {
> > +             if (desc->type != GREYBUS_TYPE_DEVICE)
> > +                     continue;
> > +             desc_device = desc->data;
> > +             mikrobus_manifest_attach_device(info, desc_device);
>
> You are ignoring the return value of mikrobus_manifest_attach_device().
>
> > +             devcount++;
> > +     }
> > +     return devcount;
> > +}
> > +
> > +bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> > +                                                      size_t size)
>
> You use "board" as the name of a "board_info" variable elsewhere.
> That is much more helpful than "info".  Please use a consistent
> naming convention for your variables of given types if possible.
> It makes it easier to follow the code.
>
> > +{
> > +     struct greybus_manifest *manifest;
> > +     struct greybus_manifest_header *header;
> > +     struct greybus_descriptor *desc;
> > +     u16 manifest_size;
> > +     int dev_count;
> > +     int desc_size;
> > +
>
> Check the size before you bother checking anything else.
>
> > +     if (WARN_ON(!list_empty(&info->manifest_descs)))
> > +             return false;
> > +     if (size < sizeof(*header))
> > +             return false;
> > +     manifest = data;
> > +     header = &manifest->header;
> > +     manifest_size = le16_to_cpu(header->size);
> > +     if (manifest_size != size)
> > +             return false;
>
> It would be helpful to report what the problem with the
> manifest is (here and in all cases).  Otherwise it's a
> little mysterious why adding a board fails.
>
> > +     if (header->version_major > MIKROBUS_VERSION_MAJOR)
> > +             return false;
>
> Version checks!!!  This is good!  But the topic is of great
> interest to Greybus people!  Not sure we ever completely
> resolved that.  That's my only comment on this for now.
>
> > +     desc = manifest->descriptors;
> > +     size -= sizeof(*header);
>
> Why aren't you using mikrobus_manifest_header_validate() here?
>
the mikrobus_manifest_header_validate is actually used to validate
the manifest header and get the manifest size, this is applicable when
fetching the manifest from an EEPROM and the number of bytes to
read(manifest size) is not known so in the first pass the manifest
header will be read from the EEPROM and the size of actual manifest
binary will be found and in the next pass the entire manifest is read
and passed to the mikrobus_manifest_parse, while passing the
manifest binary through sysfs there is no need to parse the manifest
in two passes that is why the mikrobus_manifest_header_validate()
is not called here.
> > +     while (size) {
> > +             desc_size = identify_descriptor(info, desc, size);
>
> What you're really doing with identify_descriptor() is adding
> a valid descriptor to a board's list of descriptors.  I think
> the name of that function shoudl reflect that.  If it isn't
> identified, that's an error--but that's not the purpose of
> that function.  So maybe:
>         desc_ = board_descriptor_add(board, desc, size);
>
> > +             if (desc_size < 0) {
> > +                     pr_err("invalid manifest descriptor");
> > +             return -EINVAL;
> Your indentation of the return statement here is wrong.
>
> > +             }
> > +             desc = (struct greybus_descriptor *)((char *)desc + desc_size);
>
> I don't know if this is better, but this could be:
>                 desc = (void *)desc + desc_size;
>
> > +             size -= desc_size;
> > +     }
> > +     mikrobus_state_get(info);
> > +     dev_count = mikrobus_manifest_parse_devices(info);
> > +     pr_info(" %s manifest parsed with %d device(s)\n", info->name,
> > +             info->num_devices);
> > +     release_manifest_descriptors(info);
> > +     return true;
> > +}
> > +
> > +size_t mikrobus_manifest_header_validate(void *data, size_t size)
> > +{
> > +     struct greybus_manifest_header *header;
> > +     u16 manifest_size;
> > +
> > +     if (size < sizeof(*header))
> > +             return 0;
> > +
> > +     header = data;
> > +     manifest_size = le16_to_cpu(header->size);
> > +     if (header->version_major > MIKROBUS_VERSION_MAJOR)
> > +             return 0;
> > +     return manifest_size;
> > +}
> > diff --git a/drivers/misc/mikrobus/mikrobus_manifest.h b/drivers/misc/mikrobus/mikrobus_manifest.h
> > new file mode 100644
> > index 000000000000..a195d1c26493
> > --- /dev/null
> > +++ b/drivers/misc/mikrobus/mikrobus_manifest.h
> > @@ -0,0 +1,21 @@
> > +/* 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"
> > +
> > +bool mikrobus_manifest_parse(struct addon_board_info *info, void *data,
> > +                                                      size_t size);
> > +size_t mikrobus_manifest_header_validate(void *data, size_t size);
> > +
> > +#endif /* __MIKROBUS_MANIFEST_H */
> > diff --git a/include/linux/greybus/greybus_manifest.h b/include/linux/greybus/greybus_manifest.h
> > index 6e62fe478712..79c8cab9ef96 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,
>
> Please align your new values with the rest, for consistency.
>
> >  };
> >
> >  enum greybus_protocol {
> > @@ -151,6 +154,53 @@ struct greybus_descriptor_cport {
> >       __u8    protocol_id;    /* enum greybus_protocol */
> >  } __packed;
> >
> > +/*
> > + * A mikrobus descriptor is used to describe the details
> > + * about the add-on board connected to the mikrobus port.
> > + * It describes the number of indivdual devices on the
> > + * add-on board and the default states of the GPIOs
> > + */
> > +struct greybus_descriptor_mikrobus {
> > +     __u8 num_devices;
> > +     __u8 rst_gpio_state;
> > +     __u8 pwm_gpio_state;
> > +     __u8 int_gpio_state;
> > +} __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[0];
> > +} __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 num_properties;
> > +     __u8 protocol;
> > +     __le32 max_speed_hz;
> > +     __u8 reg;
> > +     __u8 mode;
> > +     __u8 num_gpio_resources;
> > +     __u8 cs_gpio;
> > +     __u8 irq;
> > +     __u8 irq_type;
> > +     __u8 prop_link;
> > +     __u8 gpio_link;
> > +} __packed;
> > +
> >  struct greybus_descriptor_header {
> >       __le16  size;
> >       __u8    type;           /* enum greybus_descriptor_type */
> > @@ -164,6 +214,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;
>
> We're going to need to talk about these things...  But I can't
> comment much more without learning more about the broader
> architecture.
>
>                                         -Alex
>
I have gone through all the suggestions and be back with an
updated version of the driver patch with the changes added.
> >       };
> >  } __packed;
> >
> >
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [greybus-dev] [PATCH] RFC : mikroBUS driver for add-on boards
  2020-07-26 19:12   ` Vaishnav M A
@ 2020-07-26 21:54     ` Alex Elder
  0 siblings, 0 replies; 7+ messages in thread
From: Alex Elder @ 2020-07-26 21:54 UTC (permalink / raw)
  To: Vaishnav M A
  Cc: greybus-dev, linux-kernel, gregkh, arnd, johan, elder, rajkovic,
	robh, mchehab+huawei, Robert Nelson, Drew Fustini, davem,
	zoran.stojsavljevic

On 7/26/20 2:12 PM, Vaishnav M A wrote:
> I have gone through all the suggestions and be back with an
> updated version of the driver patch with the changes added.

Great!  Take your time.  (Seriously, take your time, because
I have to carve out time in my own schedule for reviewing!)

					-Alex

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-07-26 21:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 12:06 [PATCH] RFC : mikroBUS driver for add-on boards Vaishnav M A
2020-07-24 20:02 ` Christopher Friedt
2020-07-26 12:48 ` [greybus-dev] " Alex Elder
2020-07-26 17:37   ` Vaishnav M A
2020-07-26 15:04 ` Alex Elder
2020-07-26 19:12   ` Vaishnav M A
2020-07-26 21:54     ` Alex Elder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).