Re: [PATCH v5 6/7] mikrobus: Add mikroBUS driver

[Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Thu, Jun 27, 2024 at 09:56:16PM +0530, Ayush Singh wrote:
> Adds support for SPI mikroBUS addon boards with configuration based on
> device tree. The goal is to get a minimal version in mainline to sort
> out the device tree structure that should be used.
> 
> A mikroBUS board can use any combination of the following based protocols:
> I2C, SPI, UART, PWM, Analog, GPIO with possibility of all pins being used
> as GPIO instead of their original purpose. This requires the driver to be
> flexible and identify the type of board based on the compatible string.

So this has nothing to do with greybus?  Or am I thinking of something
else?

> +menuconfig MIKROBUS
> +	tristate "Module for instantiating devices on mikroBUS ports"
> +	depends on GPIOLIB
> +	help
> +	  This option enables the mikroBUS driver. mikroBUS is an add-on
> +	  board socket standard that offers maximum expandability with
> +	  the smallest number of pins. The mikroBUS driver instantiates
> +	  devices on a mikroBUS port described mikroBUS manifest which is
> +	  passed using a sysfs interface.
> +
> +

Remove extra blank line.

> +	  Say Y here to enable support for this driver.

This isn't needed.

> +
> +	  To compile this code as a module, chose M here: the module
> +	  will be called mikrobus.ko
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 153a3f4837e8..f10f1414270b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -69,3 +69,4 @@ obj-$(CONFIG_TMR_INJECT)	+= xilinx_tmr_inject.o
>  obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
>  obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
>  obj-$(CONFIG_NSM)		+= nsm.o
> +obj-$(CONFIG_MIKROBUS)		+= mikrobus.o
> diff --git a/drivers/misc/mikrobus.c b/drivers/misc/mikrobus.c
> new file mode 100644
> index 000000000000..bf160a0e8903
> --- /dev/null
> +++ b/drivers/misc/mikrobus.c
> @@ -0,0 +1,361 @@
> +// SPDX-License-Identifier: GPL-2.0:
> +/*
> + * Copyright 2024 Ayush Singh <ayush@xxxxxxxxxxxxxxx>
> + */
> +
> +#define pr_fmt(fmt) "mikrobus:%s: " fmt, __func__

KBUILD_MODNAME?  Also, why is this needed at all, as you are a
bus/subsystem you should never need a pr_*() call, but instead just use
dev_*() ones instead.

> +
> +#include <linux/device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/spi/spi.h>
> +
> +struct mikrobus_spi_cs_item {
> +	const char *cs_name;
> +	u32 cs;

Documentation?  What is "cs"?  More vowels please...

> +};
> +
> +/**
> + * struct mikrobus_port - MikroBUS Driver
> + *
> + * @dev: underlying platform_device

Why must this be a platform device?  What requires that?

> + * @board_ocs: board device tree changeset
> + * @pinctrl: mikroBUS pinctrl
> + * @mikrobus_spi_cs: list of supported chipselect address and name
> + * @mikrobus_spi_cs_count: length of mikrobus_spi_cs
> + * @spi_ctrl: spi controller of mikroBUS connector
> + * @spi_dev: spi mikroBUS board
> + */
> +struct mikrobus_port {
> +	struct platform_device *dev;
> +	struct of_changeset board_ocs;
> +	struct pinctrl *pctrl;
> +
> +	struct mikrobus_spi_cs_item *spi_cs;
> +	int spi_cs_count;
> +	struct spi_controller *spi_ctrl;
> +	struct spi_device *spi_dev;

What controls the lifespan of this object?  You have multiple devices
pointed to here with different lifecycles, what controls this one?

> +};
> +
> +/*
> + * mikrobus_pinctrl_select: Select pinctrl state for mikrobus pin

Either use kerneldoc or not, should be /** right?

> + *
> + * @port: mikrobus port
> + * @pinctrl_selected: pinctrl state to be selected
> + */
> +static int mikrobus_pinctrl_select(struct device *dev,
> +				   const char *pinctrl_selected)
> +{
> +	int ret;
> +	struct pinctrl_state *state;
> +	struct mikrobus_port *mb = dev_get_drvdata(dev);
> +
> +	state = pinctrl_lookup_state(mb->pctrl, pinctrl_selected);
> +	if (IS_ERR(state))
> +		return dev_err_probe(dev, PTR_ERR(state),
> +				     "failed to find state %s",
> +				     pinctrl_selected);
> +
> +	ret = pinctrl_select_state(mb->pctrl, state);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to select state %s",
> +				     pinctrl_selected);
> +
> +	dev_dbg_ratelimited(dev, "setting pinctrl %s", pinctrl_selected);

Why rate limited?  What is going to cause this to spam the log?

> +
> +	return 0;
> +}
> +
> +/*
> + * mikrobus_lookup_cs - lookup mikroBUS SPI chipselect by name
> + *
> + * @mb: mikroBUS port
> + * @cs_name: chipselect name

Use "chipselect" instead of "cs" everywhere please.

> + */
> +static int mikrobus_lookup_cs(const struct mikrobus_port *mb,
> +			      const char *cs_name)
> +{
> +	for (int i = 0; i < mb->spi_cs_count; ++i) {
> +		if (strcmp(cs_name, mb->spi_cs[i].cs_name) == 0)
> +			return mb->spi_cs[i].cs;
> +	}
> +
> +	return -1;

what does -1 mean?  Use real error numbers please.

> +}
> +
> +static int mikrobus_spi_set_cs(struct device *dev, struct device_node *np)
> +{
> +	struct mikrobus_port *mb = dev_get_drvdata(dev);
> +	const char *temp_str;
> +	int reg_len;
> +	int ret, i;
> +	u32 *reg = NULL;
> +
> +	reg_len = of_property_count_strings(np, "spi-cs");
> +	/* Use default cs if spi-cs property not present */
> +	if (reg_len <= 0) {
> +		reg_len = 1;
> +		reg = devm_kmalloc_array(dev, reg_len, sizeof(*reg),
> +					 GFP_KERNEL);
> +		if (!reg)
> +			return -ENOMEM;
> +
> +		ret = mikrobus_lookup_cs(mb, "default");
> +		if (ret < 0)
> +			goto free_reg;
> +
> +		reg[0] = ret;
> +	} else {
> +		reg = devm_kmalloc_array(dev, reg_len, sizeof(*reg),
> +					 GFP_KERNEL);
> +		if (!reg)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < reg_len; ++i) {
> +			ret = of_property_read_string_index(np, "spi-cs", i,
> +							    &temp_str);
> +			if (ret < 0)
> +				goto free_reg;
> +
> +			ret = mikrobus_lookup_cs(mb, temp_str);
> +			if (ret < 0)
> +				goto free_reg;
> +
> +			reg[i] = ret;
> +		}
> +	}
> +
> +	ret = of_changeset_add_prop_u32_array(&mb->board_ocs, np, "reg", reg,
> +					      reg_len);
> +	if (ret < 0)
> +		goto free_reg;
> +
> +	ret = of_changeset_apply(&mb->board_ocs);
> +	if (ret < 0)
> +		goto free_reg;
> +
> +	devm_kfree(dev, reg);
> +	return 0;
> +
> +free_reg:
> +	devm_kfree(dev, reg);
> +	return ret;
> +}
> +
> +static int of_register_mikrobus_device(struct mikrobus_port *mb,
> +				       struct device_node *np)
> +{
> +	const char *temp_str;
> +	int i, pinctrl_count, ret;
> +	struct spi_device *spi_dev;
> +	struct device *dev = &mb->dev->dev;

That's some pointer dereferencing without checking anything, what could
go wrong...

Why don't you have your own real device?  Why are you relying on a
platform device without actually showing your device anywhere in the
kernel's device topology?  Are you sure that is ok?


> +
> +	pinctrl_count = of_property_count_strings(np, "pinctrl-apply");
> +	if (pinctrl_count < 0)
> +		return dev_err_probe(dev, pinctrl_count,
> +				     "Missing required property pinctrl-apply");
> +
> +	for (i = 0; i < pinctrl_count; ++i) {
> +		ret = of_property_read_string_index(np, "pinctrl-apply", i,
> +						    &temp_str);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = mikrobus_pinctrl_select(dev, temp_str);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to set pinctrl");
> +	}
> +
> +	if (mb->spi_ctrl && !mb->spi_dev &&
> +	    of_device_is_compatible(np, "mikrobus-spi")) {
> +		ret = mikrobus_spi_set_cs(dev, np);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to set SPI chipselect");
> +
> +		spi_dev = of_register_spi_device(mb->spi_ctrl, np);
> +		if (IS_ERR(spi_dev))
> +			return dev_err_probe(dev, PTR_ERR(spi_dev),
> +					     "Failed to register SPI device");
> +		mb->spi_dev = spi_dev;
> +	}
> +
> +	return 0;
> +}
> +
> +static int of_register_mikrobus_board(struct mikrobus_port *mb)
> +{
> +	struct device *dev = &mb->dev->dev;
> +	int board_len, i, ret;
> +	struct device_node *np;
> +
> +	board_len = of_count_phandle_with_args(dev->of_node, "board", NULL);
> +	for (i = 0; i < board_len; ++i) {
> +		np = of_parse_phandle(dev->of_node, "board", i);
> +		if (!np) {
> +			ret = dev_err_probe(dev, -ENODEV, "Board not found");
> +			goto free_np;
> +		}
> +
> +		ret = of_register_mikrobus_device(mb, np);
> +		if (ret < 0)
> +			goto free_np;
> +
> +		of_node_put(np);
> +	}
> +
> +	return 0;
> +free_np:
> +	of_node_put(np);
> +	return ret;
> +}
> +
> +static int mikrobus_port_probe(struct platform_device *pdev)
> +{
> +	int ret, i;
> +	struct mikrobus_port *mb;
> +	struct device_node *np;
> +	struct device *dev = &pdev->dev;
> +
> +	mb = devm_kmalloc(dev, sizeof(*mb), GFP_KERNEL);
> +	if (!mb)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, mb);
> +
> +	of_changeset_init(&mb->board_ocs);
> +	mb->dev = pdev;
> +	mb->pctrl = NULL;
> +	mb->spi_ctrl = NULL;
> +	mb->spi_dev = NULL;
> +	mb->spi_cs = NULL;
> +	mb->spi_cs_count = 0;
> +
> +	mb->pctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(mb->pctrl))
> +		return dev_err_probe(dev, PTR_ERR(mb->pctrl),
> +				     "failed to get pinctrl [%ld]",
> +				     PTR_ERR(mb->pctrl));
> +
> +	np = of_parse_phandle(dev->of_node, "spi-controller", 0);
> +	if (np) {
> +		mb->spi_ctrl = of_find_spi_controller_by_node(np);
> +		if (mb->spi_ctrl) {
> +			ret = of_property_count_u32_elems(dev->of_node,
> +							  "spi-cs");
> +			if (ret < 0) {
> +				dev_err(dev, "Missing property spi-cs");
> +				goto free_np;
> +			}
> +
> +			mb->spi_cs_count = ret;
> +
> +			ret = of_property_count_strings(dev->of_node,
> +							"spi-cs-names");
> +			if (ret < 0) {
> +				dev_err(dev, "Missing property spi-cs-names");
> +				goto free_np;
> +			}
> +
> +			if (mb->spi_cs_count != ret) {
> +				ret = dev_err_probe(
> +					dev, -EINVAL,
> +					"spi-cs and spi-cs-names out of sync");
> +				goto free_np;
> +			}
> +
> +			mb->spi_cs = devm_kmalloc_array(dev, mb->spi_cs_count,
> +							sizeof(*mb->spi_cs),
> +							GFP_KERNEL);
> +			if (!mb->spi_cs) {
> +				ret = -ENOMEM;
> +				goto free_np;
> +			}
> +
> +			for (i = 0; i < mb->spi_cs_count; ++i) {
> +				of_property_read_u32_index(dev->of_node,
> +							   "spi-cs", i,
> +							   &mb->spi_cs->cs);
> +				of_property_read_string_index(
> +					dev->of_node, "spi-cs-names", i,
> +					&mb->spi_cs->cs_name);
> +			}
> +		}
> +	}
> +	of_node_put(np);
> +
> +	ret = of_register_mikrobus_board(mb);
> +	if (ret < 0)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Failed to register mikrobus board");
> +
> +	return 0;
> +
> +free_np:
> +	of_node_put(np);
> +	return ret;
> +}
> +
> +static void mikrobus_port_remove(struct platform_device *pdev)
> +{
> +	struct mikrobus_port *mb = dev_get_drvdata(&pdev->dev);
> +
> +	spi_unregister_device(mb->spi_dev);
> +
> +	of_changeset_revert(&mb->board_ocs);
> +}
> +
> +static const struct of_device_id mikrobus_port_of_match[] = {
> +	{ .compatible = "mikrobus-connector" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mikrobus_port_of_match);
> +
> +static struct platform_driver mikrobus_port_driver = {
> +	.probe = mikrobus_port_probe,
> +	.remove = mikrobus_port_remove,

Again, why is this a platform driver?  Why is a platform device used at
all here?

> +	.driver = {
> +		.name = "mikrobus",
> +		.of_match_table = mikrobus_port_of_match,
> +	},
> +};
> +
> +static const struct bus_type mikrobus_bus_type = {
> +	.name = "mikrobus",
> +};
> +
> +static int mikrobus_init(void)
> +{
> +	int ret;
> +
> +	ret = bus_register(&mikrobus_bus_type);
> +	if (ret) {
> +		pr_err("bus_register failed (%d)", ret);
> +		return ret;
> +	}
> +
> +	ret = platform_driver_register(&mikrobus_port_driver);
> +	if (ret)
> +		pr_err("driver register failed [%d]", ret);

It fails yet you leave your bus around?  Not good :(

thanks,

greg k-h