Re: [PATCH RESEND 1/2] arm64: dts: rockchip: rk3308: Add gpio-ranges properties

[Matthijs Kooijman <matthijs@stdin.nl>]

[-- Attachment #1.1: Type: text/plain, Size: 2585 bytes --]

Hi Jonas,

Thanks for your review.

> > This does not immediately change functionality, because the
> > gpio-rockchip.c driver has a workaround that defines ranges when they
> > are not present in DT, but that relies on global gpio numbering (so
> > AFAICS only works when the rockchip gpio banks are initialized first and
> > in-order).
> 
> This is strictly not correct, the driver use the gpioX alias id as
> defined in the device tree and does not depend on the initialization
> order.

Ah, I had not realized these aliases existed. However, it seems they are
not actually relevant in this case. My assumption was that gpio
numbering was based on initalizating order, but I see in the code that
GPIO drivers decide themselves (by setting gc->base statically or maybe
based on DT). For the gpio-rockchip driver, these base numbers are
hardcoded to start from 0 in rockchip_pinctrl_get_soc_data(). Dynamic
initialization-order based numbering is also done for drivers that do
not set gc->base, but that starts at GPIO number 512 to prevent
conflicts.

In any case, I will update my commit message to better reflect what is
happening.


> > --- a/arch/arm64/boot/dts/rockchip/rk3308.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
> > @@ -889,6 +889,7 @@ gpio0: gpio@ff220000 {
> >  			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> >  			clocks = <&cru PCLK_GPIO0>;
> >  			gpio-controller;
> > +			gpio-ranges = <&pinctrl 0 0 32>;
> 
> This matches the driver, but not the hardware according to datasheet, it
> only lists gpio0 A0-C5 used, and not all 32 pins supported by the gpio0
> controller.

Indeed, this seems to be the case, but I wonder if this is a problem?

Isee other rockchip devicetree definitions (rk3528, rk3562, rk356x,
rk3576, rk3588) do not care about this and just map all 32 pins. See

	git grep -C 20 rockchip,gpio-bank | grep gpio-ranges

I also think there is no other provision for these missing GPIOs
anywhere - both pinctrl and gpiod seem to expose all 32 pins, even the
one that do not exist.

Limiting the gpio-ranges definitions would prevent writing to reserved
pinctrl registers via the userspace gpio API, which might be useful, but
you would still be able to write to them via a custom devicetree (that
uses non-existing pinctrl pins) and you would still be able to write to
non-existent gpio registers via the userspace API.

Given the above, do you still think it would be good to limit the
gpio-ranges to the existing GPIOs only (I have no strong opinion)?

Gr.

Matthijs

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip