| Message ID | 517dcdda21ea0b0df884bc6adcba1dadb78b66b1.1551966077.git.jan.kundrat@cesnet.cz |
|---|---|
| State | New |
| Headers | show |
| Series | mcp23s08 fixes | expand |
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. Also, this DT binding patch is not Cc'ed to the DT binding maintainers and mailing list. Best regards, Thomas
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; >
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
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;