Messages in this thread Patch in this message | | | From | Lars Povlsen <> | Subject | Re: [RESEND PATCH v3 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO | Date | Fri, 9 Oct 2020 13:14:15 +0200 |
| |
Linus Walleij writes:
> Hi Lars, > > I'm overall mostly happy with the latest posting (not the one I respond to here)
I'm glad we're getting there :-)
> > On Thu, Oct 8, 2020 at 12:57 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: >> > On Tue, Oct 6, 2020 at 4:25 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > >> >> + gc->of_xlate = microchip_sgpio_of_xlate; >> >> + gc->of_gpio_n_cells = 3; >> > >> > So I'm sceptical to this. >> > >> > Why can't you just use the pin index in cell 0 directly >> > and avoid cell 1? >> > >> >> You scepticism has surfaced before :-). The (now) 2 indices relates to >> how the hardware address signals. >> >> Each signal/pin is addressed by port, bit number and direction. We now >> have the direction encoded in the bank/phandle. > > I'm sorry but I just don't get it, I suppose. To me it is pretty > straight-forward > that the cells indicate the pin and then the flags. I do understand you > need the port at all, since this is implicit from the reg property > of the DT node. Are these two different things?
I responded to this in your comments to the DT bindings.
I just for got to offer to add a description for "#gpio-cells", I see that's missing. That should make it "crystal clear" - I hope!
Something like:
--- a/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml +++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml @@ -86,10 +86,17 @@ patternProperties: gpio-controller: true
'#gpio-cells': + description: | + Specifies the pin (port and bit) and flags. Note that the + SGIO pin is defined by *2* numbers, a port number between 0 + and 31, and a bit index, 0 to 3. The maximum bit number is + controlled indirectly by the "ngpios" property: (ngpios/32). const: 3
ngpios: - minimum: 1 + description: The numbers of GPIO's exposed. This must be a + multiple of 32. + minimum: 32 maximum: 128
required: Would that be adequate, or should this also be added as a comment in microchip_sgpio_of_xlate()?
Like:
+ /* Note that the SGIO pin is defined by *2* numbers, a port + * number between 0 and 31, and a bit index, 0 to 3. + */ if (gpiospec->args[0] > SGPIO_BITS_PER_WORD || gpiospec->args[1] > priv->bitcount) return -EINVAL; I hope we can put this one to bed...
---Lars > > Yours, > Linus Walleij
-- Lars Povlsen, Microchip
| |