Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan <dlan@xxxxxxxxxx>]

Hi Linus:

On 11:54 Fri 14 Feb     , Yixun Lan wrote:
> Hi Linus:
> 
> On 14:07 Thu 13 Feb     , Linus Walleij wrote:
> > On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@xxxxxxxxxx> wrote:
> > 
> > > > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
> > >
> > > if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip",
> > >  which mean one gpio chip combine three banks..
> > 
> > Not really: the fact that there is just one gpio node in the device
> > tree does not
> > mean that it needs to correspond to one single gpio_chip instance inside the
> > Linux kernel.
> > 
> > It's just what the current existing bindings and the code in the GPIO subsystem
> > assumes. It does not have to assume that: we can change it.
> > 
> > I'm sorry if this is not entirely intuitive :(
> > 
> > One node can very well spawn three gpio_chip instances, but it requires
> > some core changes. But I think it's the most elegant.
> > 
> > > if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1,
> > > then, even the three gpio-cells model is unnecessary needed, as we can map gpio number
> > >  to the <bank, offset> array in the underlying gpio driver
> > >
> > > the v4 patch is very similar to drivers/gpio/gpio-dwapb.c
> > >
> > > If had to choose the direction between v1 and v4, I personally would favor the latter,
> > >  as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers,
> > >  merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage
> > >  lots underlying generic gpio APIs, result in much simplified/clean code base..
> > 
> > So what I would suggest is a combination of the two.
> > 
> > One gpio node in the device tree, like the DT maintainers want it.
> > 
> > Three struct gpio_chip instances inside the driver, all three spawn from
> > that single gpio device, and from that single platform_device.
> > 
> > What we are suggesting is a three-cell phandle in the device tree:
> > 
> > foo-gpios = <&gpio 0 7 GPIO_ACTIVE_HIGH>;
> > bar-gpios = <&gpio 2 31 GPIO_ACTIVE_HIGH>;
> > 
> > Notice the new first cell which is 0 or 2.
> > 
> > The first one is what was previously called gpio 7.
> > The second one is what was 2*32+31 = gpio 95.
> > 
> > So internally in the driver it is easy to use the first cell (0 or 2) to map to
> > the right struct gpio_chip if you have it in your driver something like this:
> > 
> > struct spacemit_gpio {
> >     struct gpio_chip gcs[3];
> > ...
> > };
> > 
> > struct spacemit_gpio *sg;
> > struct gpio_chip *gc;
> > int ret;
> > 
> > for (i = 0; i++; i < 3) {
> >      ret = devm_gpiochip_add_data(dev, &sg->gcs[i], sg);
> >      if (ret)
> >         return ret;
> >      gc = sg->gcs[i];
> >      .... do stuff with this instance ....
> > }
> > 
> > Callbacks etc should work as before.
> > 
> > Then these phandles needs to be properly translated, which is done with the
> > struct gpio_chip .of_xlate() callback. (If you look inside gpiolib-of.c
> > you will see that chip->of_xlate() is called to map the phandle cells
> > to a certain GPIO line).
> > 
> > In most cases, drivers do not assign the chip->of_xlate callback
> > (one exception is gpio-pxa.c) and then it is default-assigned to
> > of_gpio_simple_xlate() which you can find in gpiolib-of.c as well.
> > 
> > You need to copy this callback to your driver and augment it
> > properly.
> > 
> > The xlate callback is used to locate the struct gpio_chip and
> > struct gpio_device as well, by just calling the xlate callback, so if
> > you code up the right xlate callback, everything should just
> > work by itself.
> > 
> > this is a guess on what it would look like (just dry coding,
> > but hopefully the idea works!):
> > 
> > static int spacemit_gpio_xlate(struct gpio_chip *gc,
> >                                 const struct of_phandle_args *gpiospec,
> >                                 u32 *flags)
> > {
> >         struct spacemit_gpio *sg = gpiochip_get_data(gc);
> >         int i;
> > 
> >         if (gc->of_gpio_n_cells != 3)
> >                 return -EINVAL;
> > 
> >         if (gpiospec->args_count < gc->of_gpio_n_cells)
> >                 return -EINVAL;
> > 
> >         /* We support maximum 3 gpio_chip instances */
> >         i = gpiospec->args[0];
> >         if (i >= 3)
> >                 return -EINVAL;
> > 
> >         /* OK is this the right gpio_chip out of the three ? */
> >         if (gc != sg->gcs[i])
> >                 return -EINVAL;
> > 
> >         /* Are we in range for this GPIO chip */
> >         if (gpiospec->args[1] >= gc->ngpio)
> >                 return -EINVAL;
> > 
> >         if (flags)
> >                 *flags = gpiospec->args[2];
> > 
> >         /* Return the hw index */
> >         return gpiospec->args[1];
> > }
> > 
> thanks for this very detail prototype! it works mostly, with one problem:
> 
> how to map gpio correctly to the pin from pinctrl subsystem?
> 
> for example, I specify gpio-ranges in dts, then 
>                 gpio0: gpio@d4019000 {
>                         compatible = "spacemit,k1-gpio";
>                         reg = <0x0 0xd4019000 0x0 0x100>;
> 			...
>                         gpio-ranges = <&pinctrl 0 0 96>;
>                 };
> 
> 		foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>;
> 
> It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28
> 
> Probably there is something I missed...
to make the gpio part work, we need additional custom gpio-ranges parser,
which should similar to of_gpiochip_add_pin_range() in gpiolib-of.c
(at least gpio core need to adjust to call custom this function)

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55