Message-ID: <CACRpkdZYYZ5tUR4gJXuCrix0k56rPPB2TUGP3KpwqMgjs_Vd5w@mail.gmail.com>
Date: Thu, 13 Feb 2025 14:07:31 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Yixun Lan <dlan@...too.org>
Cc: Rob Herring <robh@...nel.org>, Olof Johansson <olof@...om.net>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Conor Dooley <conor@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Yangyu Chen <cyy@...self.name>, 
	Jisheng Zhang <jszhang@...nel.org>, Jesse Taube <mr.bossman075@...il.com>, 
	Inochi Amaoto <inochiama@...look.com>, Icenowy Zheng <uwu@...nowy.me>, 
	Meng Zhang <zhangmeng.kevin@...ux.spacemit.com>, linux-gpio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@...too.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];
}

...
gc->of_gpio_n_cells = 3;
gc->of_xlate = spacemit_gpio_xlate;

If it works as I hope, this will make the code in gpiolib-of.c in
of_find_gpio_device_by_xlate() calling gpio_device_find()
(which will iterate over all registered gpio_chips and then
of_gpiochip_match_node_and_xlate() will call this custom function
to see if it's the right one and return > 0 when we have the right
chip.

This should work for gpios *only*. When we then come to irqs,
these assume (see gpiolib.c) that we are using
irq_domain_xlate_twocell() when using GPIOLIB_IRQCHIP, so
you either need to roll your own irqchip code or we should fix
the core (I can help with this if the above works).

Several gpio chips use their own domain translation outside
of the gpiolib so you can use this as an intermediate step:
git grep irq_domain_ops drivers/gpio/
... but if you get here, let's patch the core to deal with custom
irqdomain xlate functions in the same manner as above.

I hope this isn't terribly unclear or complicated?
Otherwise tell me and I will try to ... explain more or give
up and say you can use a single 96-pin gpio_chip.

Yours,
Linus Walleij

Powered by blists - more mailing lists