[4/4] pinctrl: mcp23s08: work around GPIO line naming

This driver is a bit weird because it can hide several gpio_chip
instances underneath a single SPI slave. One cannot put the
gpio-line-names DT stanza directly to the SPI slave when the
spi-present-mask has more than one bit set.

I'm making up the `gpio-bank` DT child name as well as its `address`
property. We need something to match the DT entries with the SPI
address.

Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
---
 .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 35 +++++++++++++++++++
 drivers/pinctrl/pinctrl-mcp23s08.c            | 12 +++++++
 2 files changed, 47 insertions(+)

G'day Jan,


On 5/06/2018 01:05, Jan Kundrát wrote:
> This driver is a bit weird because it can hide several gpio_chip
> instances underneath a single SPI slave. One cannot put the
> gpio-line-names DT stanza directly to the SPI slave when the
> spi-present-mask has more than one bit set.
> 
> I'm making up the `gpio-bank` DT child name as well as its `address`
> property. We need something to match the DT entries with the SPI
> address.
> 

Reviewed-by: Phil Reid <preid@electromag.com.au>
One comment below

> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> ---
>   .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 35 +++++++++++++++++++
>   drivers/pinctrl/pinctrl-mcp23s08.c            | 12 +++++++
>   2 files changed, 47 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> index a5a8322a31bd..7573df69ccea 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> @@ -142,3 +142,38 @@ gpio21: gpio@21 {
>   		bias-pull-up;
>   	};
>   };
> +
> +Line naming
> +===========
> +
> +Because several gpio_chip instances are hidden below a single device tree
> +node, it is necessary to split the names into several child nodes. Ensure
> +that the configured addresses match those in the microchip,spi-present-mask:
> +
> +gpio@0 {
> +        compatible = "microchip,mcp23s17";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +	/* this bitmask has bits #0 (0x01) and #2 (0x04) set */
> +        spi-present-mask = <0x05>;
> +        reg = <0>;
> +        spi-max-frequency = <1000000>;
> +
> +	gpio-bank@1 {
> +		address = <0>;
> +		gpio-line-names =
> +			"GPA0",
> +			"GPA1",
> +			...
> +			"GPA7",
> +			"GPB0",
> +			"GPB1",
> +			...
> +			"GPB7";
> +	};
> +
> +	gpio-bank@2 {
> +		address = <2>;
> +		gpio-line-names = ...
> +	};
> +};
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index bbef9833edb8..c4fac4e1de71 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -16,6 +16,7 @@
>   #include <linux/pinctrl/pinctrl.h>
>   #include <linux/pinctrl/pinconf.h>
>   #include <linux/pinctrl/pinconf-generic.h>
> +#include "../gpio/gpiolib.h"

Perhaps the declaration for devprop_gpiochip_set_names() needs to be moved
to linux/gpio/driver.h

Someone more knowledgeable than me may be able to comment.


>   
>   /*
>    * MCP types supported by driver
> @@ -1086,6 +1087,7 @@ static int mcp23s08_probe(struct spi_device *spi)
>   	int				status, type;
>   	unsigned			ngpio = 0;
>   	const struct			of_device_id *match;
> +	struct device_node		*np;
>   
>   	match = of_match_device(of_match_ptr(mcp23s08_spi_of_match), &spi->dev);
>   	if (match)
> @@ -1148,6 +1150,16 @@ static int mcp23s08_probe(struct spi_device *spi)
>   		if (pdata->base != -1)
>   			pdata->base += data->mcp[addr]->chip.ngpio;
>   		ngpio += data->mcp[addr]->chip.ngpio;
> +
> +		for_each_available_child_of_node(spi->dev.of_node, np) {
> +			u32 chip_addr;
> +			status = of_property_read_u32(np, "address", &chip_addr);
> +			if (status)
> +				continue;
> +			if (chip_addr != addr)
> +				continue;
> +			devprop_gpiochip_set_names(&data->mcp[addr]->chip, of_fwnode_handle(np));
> +		}
>   	}
>   	data->ngpio = ngpio;
>   
>

On Mon, Jun 4, 2018 at 7:05 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote:

> This driver is a bit weird because it can hide several gpio_chip
> instances underneath a single SPI slave. One cannot put the
> gpio-line-names DT stanza directly to the SPI slave when the
> spi-present-mask has more than one bit set.
>
> I'm making up the `gpio-bank` DT child name as well as its `address`
> property. We need something to match the DT entries with the SPI
> address.
>
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>

Ah that was a tricky one.

+ #include "../gpio/gpiolib.h"
(...)
> +                       devprop_gpiochip_set_names(&data->mcp[addr]->chip, of_fwnode_handle(np));

I'm not very happy about reaching into the gpiolib internal API like
this.

As Phil says the function should be moved to <linux/gpio/driver.h>
if we need to use it from drivers.

Also, this symbol is not exported so you would get compile errors
from the test farm quickly becuase a lot of module building
starts failing. This function needs EXPORT_SYMBOL_GPL()
to be used from the outside in drivers.

Mika added this function, I don't know how he feels about
having it externalized?

I also think it would be OK with a device-tree only function,
but maybe that is not as reusable?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Jun 13, 2018 at 11:07:00AM +0200, Linus Walleij wrote:
> > +                       devprop_gpiochip_set_names(&data->mcp[addr]->chip, of_fwnode_handle(np));
> 
> I'm not very happy about reaching into the gpiolib internal API like
> this.
> 
> As Phil says the function should be moved to <linux/gpio/driver.h>
> if we need to use it from drivers.
> 
> Also, this symbol is not exported so you would get compile errors
> from the test farm quickly becuase a lot of module building
> starts failing. This function needs EXPORT_SYMBOL_GPL()
> to be used from the outside in drivers.
> 
> Mika added this function, I don't know how he feels about
> having it externalized?

No objections from my side.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html