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

Yixun Lan
2025-02-14T13:24:01Z

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@gentoo.org> 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