Re: [PATCH v2 3/4] pinctrl: mcp23s08: work around GPIO line naming

Re: [PATCH v2 3/4] pinctrl: mcp23s08: work around GPIO line naming

[Jan Kundrát]

Sorry for missing the DT maintainers on this one.

Jan

On čtvrtek 7. března 2019 14:31:02 CET, 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.
>
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> Reviewed-by: Phil Reid <preid@electromag.com.au>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> ---
> v2: Depend on exported devprop_gpiochip_set_names
> ---
>  .../bindings/pinctrl/pinctrl-mcp23s08.txt     | 35 +++++++++++++++++++
>  drivers/pinctrl/pinctrl-mcp23s08.c            | 11 ++++++
>  2 files changed, 46 insertions(+)
>
> diff --git 
> a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt 
> b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> index 625a22e2f211..a0b1eb9aedad 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> @@ -144,3 +144,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 fd9d6f026d70..7810f56f8dd1 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -1102,6 +1102,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)
> @@ -1170,6 +1171,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;
>  

Re: [PATCH v2 3/4] pinctrl: mcp23s08: work around GPIO line naming

[Jan Kundrát]

On čtvrtek 7. března 2019 22:35:15 CET, Thomas Petazzoni wrote:
> Hello Jan,
>
> On Thu, 7 Mar 2019 14:31:02 +0100
> Jan Kundrát <jan.kundrat@cesnet.cz> wrote:
>
>> +Line naming
>> +===========
>> +
>> +Because several gpio_chip instances are hidden below a single device tree
>
> Mentioning a Linux specific data structure (gpio_chip here) in a Device
> Tree binding documentation does not seem like a good idea. Bindings are
> supposed to be OS-agnostic.

Hi Thomas, that's a fair comment. On the other hand, this driver's DT 
bindings are already quite Linux specific with their 
"microchip,spi-present-mask" workaround. The way I understand this, Linux 
does not support multiple child devices "within" one SPI slave. If there 
was proper support for this, this patch would be superfluous.

I, too, would love to see this driver changed so that the bindings look, 
e.g., like this:

  gpio_spi_chips: gpio@1 {
    compatible = "microchip,mcp23s17-multiple";

    gpio-bank@1 {
      compatible = "microchip,mcp23s17";
      reg = <1>;
      interrupt-parent = <&gpio1>;
      interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
      interrupt-controller;
      #interrupt-cells = <2>;
      gpio-controller;
      #gpio-cells = <2>;
      microchip,irq-mirror;
      drive-open-drain;
      spi-max-frequency = <10000000>;
      gpio-line-names =
        "foo",
        //...
        ;
       random_pin {
         gpio-hog;
         gpios = <6 GPIO_ACTIVE_HIGH>;
         input;
         line-name = "this hogs just one pin";
       };
    };

    gpio-bank@2 {
      compatible = "microchip,mcp23s17";
      reg = <2>;
      interrupt-parent = <&gpio1>;
      interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
      interrupt-controller;
      #interrupt-cells = <2>;
      gpio-controller;
      #gpio-cells = <2>;
      microchip,irq-mirror;
      drive-open-drain;
      spi-max-frequency = <10000000>;
      gpio-line-names =
        "bar",
        //...
        ;
    };

However, I am not volunteering to do this work -- that's quite out of my 
league and available time.

With kind regards,
Jan