Re: [PATCH] rockchip: derive GPIO bank from alias if available

[John Keeping <john@metanate.com>]

On Tue, Jan 17, 2023 at 08:52:22PM +0100, Johan Jonker wrote:
> 
> 
> On 1/17/23 19:15, John Keeping wrote:
> > Upstream device trees now use standard node names like "gpio@ff..." but
> > the rk_gpio driver expects a name like "gpio0@ff..." (note the index
> > before the @).
> > 
> > This is not a change that can be made in a -u-boot.dtsi file, so
> > updating to the latest upstream device trees requires updating the
> > driver.
> > 
> > Other GPIO drivers already use the sequence number to derive the bank.
> > This can be set explicitly using an alias in the device tree (which can
> > be added in a -u-boot.dtsi file) but Rockchip GPIO banks are defined in
> > order in the device tree anyway so when no aliases are supplied the
> > indexes should still be correct.
> > 
> > Note that the bank name is only used by the `gpio` command or
> > potentially by board code via dm_gpio_lookup_name() and related
> > functions; it is not used by driver code which will lookup GPIOs from
> > the device tree by phandle.  No Rockchip platforms make any use of
> > dm_gpio_lookup_name() or `gpio` in the boot scripts.
> > 
> > Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > Cc: Johan Jonker <jbx6244@gmail.com>
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> >  drivers/gpio/rk_gpio.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
> > index 68f30157a9..8568f10cd0 100644
> > --- a/drivers/gpio/rk_gpio.c
> > +++ b/drivers/gpio/rk_gpio.c
> > @@ -142,7 +142,6 @@ static int rockchip_gpio_probe(struct udevice *dev)
> >  {
> >  	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> >  	struct rockchip_gpio_priv *priv = dev_get_priv(dev);
> > -	char *end;
> >  	int ret;
> >  
> >  	priv->regs = dev_read_addr_ptr(dev);
> > @@ -151,9 +150,8 @@ static int rockchip_gpio_probe(struct udevice *dev)
> >  		return ret;
> >  
> >  	uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
> > -	end = strrchr(dev->name, '@');
> > -	priv->bank = trailing_strtoln(dev->name, end);
> > -	priv->name[0] = 'A' + priv->bank;
> 
> > +
> > +	priv->name[0] = 'A' + dev_seq(dev);
> 
> Some comments. Have a look if it's useful.
> ===
> 1:
> Tested with rk3066a mk808 in only full u-boot.
> 
> The gpio banks have a number gap.
> 
> 	aliases {
> 		gpio0 = &gpio0;
> 		gpio1 = &gpio1;
> 		gpio2 = &gpio2;
> 		gpio3 = &gpio3;
> 		gpio4 = &gpio4;
> 
> 		gpio6 = &gpio6;
> 	};
> 
> With aliases gpio6 is called G.
> Without gpio6 is called F.
> There's no name consistency for command scripts.

True, but those scripts are going to be board specific and already
depend on board data.  The name will be consistent per board.

> ===
> 2:
> For example: rk3288-evb-u-boot.dtsi
> 
> What happens in tpl or spl with only gpio3 and gpio8 in dt-plat.c.

Is the bank name used in tpl/spl at all?  There's no script support
there and it could only be used in board code.

> Or with SPL FDT blob:
> 
> [v1,01/17] rockchip: spl: fix reloc gd and FDT blob pointer
> http://patchwork.ozlabs.org/project/uboot/patch/20220508150825.21711-2-jbx6244@gmail.com/
> 
> Not in use in mainline. Just saying there's more possible in SPL/TPL.
> ===
> The use of aliases is more of something of an additional thing that we not always can count on that is there.

Again this is board specific - if a board wants this scheme of GPIO
naming it can enable aliases and rely on that.

I have been thinking of another option which is a bigger change to the
bank name - it doesn't have to be a letter, could we use the register
address as the bank name?  So the GPIO would be something like
ff750000.1 (although I don't know how the naming scheme works, so having
a separator may not work).

What do you think of that idea?


Regards,
John