Re: [PATCH v2 2/2] gpiolib: of: Handle threecell GPIO chips

[Rob Herring <robh@xxxxxxxxxx>]

On Tue, Feb 25, 2025 at 10:15 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> When describing GPIO controllers in the device tree, the ambition
> of device tree to describe the hardware may require a three-cell
> scheme:
>
> gpios = <&gpio instance offset flags>;

I think we have cases where bank/instance and offset are in 1 cell.
Not sure if you want to make it possible to split those into separate
gpio chips.

> This implements support for this scheme in the gpiolib OF core.
>
> Drivers that want to handle multiple gpiochip instances from one
> OF node need to implement a callback similar to this to
> determine if a certain gpio chip is a pointer to the right
> instance (pseudo-code):
>
> struct my_gpio {
>     struct gpio_chip gcs[MAX_CHIPS];
> };
>
> static bool my_of_node_instance_match(struct gpio_chip *gc
>                                       unsigned int instance)
> {
>     struct my_gpio *mg = gpiochip_get_data(gc);
>
>     if (instance >= MAX_CHIPS)
>         return false;
>     return (gc == &mg->gcs[instance]);
> }
>
> probe() {
>     struct my_gpio *mg;
>     struct gpio_chip *gc;
>     int i, ret;
>
>     for (i = 0; i++; i < MAX_CHIPS) {
>         gc = &mg->gcs[i];
>         /* This tells gpiolib we have several instances per node */
>         gc->of_gpio_n_cells = 3;
>         gc->of_node_instance_match = my_of_node_instance_match;
>         gc->base = -1;
>         ...
>
>         ret = devm_gpiochip_add_data(dev, gc, mg);
>         if (ret)
>             return ret;
>     }
> }
>
> Rename the "simple" of_xlate function to "twocell" which is closer
> to what it actually does.
>
> In the device tree bindings, the provide node needs
> to specify #gpio-cells = <3>; where the first cell is the instance
> number:
>
> gpios = <&gpio instance offset flags>;
>
> Conversely ranges need to have four cells:
>
> gpio-ranges = <&pinctrl instance gpio_offset pin_offset count>;
>
> Reviewed-by: Alex Elder <elder@xxxxxxxxxxxx>
> Tested-by: Yixun Lan <dlan@xxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/gpio/gpiolib-of.c   | 93 ++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/gpio/driver.h | 24 +++++++++++-
>  2 files changed, 106 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 86405218f4e2ddc951a1a9d168e886400652bf60..614590a5bcd10e5605ecb66ebd956250e4ea1fd8 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -929,7 +929,7 @@ struct notifier_block gpio_of_notifier = {
>  #endif /* CONFIG_OF_DYNAMIC */
>
>  /**
> - * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
> + * of_gpio_twocell_xlate - translate twocell gpiospec to the GPIO number and flags
>   * @gc:                pointer to the gpio_chip structure
>   * @gpiospec:  GPIO specifier as found in the device tree
>   * @flags:     a flags pointer to fill in
> @@ -941,9 +941,9 @@ struct notifier_block gpio_of_notifier = {
>   * Returns:
>   * GPIO number (>= 0) on success, negative errno on failure.
>   */
> -static int of_gpio_simple_xlate(struct gpio_chip *gc,
> -                               const struct of_phandle_args *gpiospec,
> -                               u32 *flags)
> +static int of_gpio_twocell_xlate(struct gpio_chip *gc,
> +                                const struct of_phandle_args *gpiospec,
> +                                u32 *flags)
>  {
>         /*
>          * We're discouraging gpio_cells < 2, since that way you'll have to

Note that this function only rejects <2, but I doubt 3 cells worked
for it. So it should probably check for !=2.

> @@ -968,6 +968,49 @@ static int of_gpio_simple_xlate(struct gpio_chip *gc,
>         return gpiospec->args[0];
>  }
>
> +/**
> + * of_gpio_threecell_xlate - translate threecell gpiospec to the GPIO number and flags
> + * @gc:                pointer to the gpio_chip structure
> + * @gpiospec:  GPIO specifier as found in the device tree
> + * @flags:     a flags pointer to fill in
> + *
> + * This is simple translation function, suitable for the most 1:n mapped
> + * GPIO chips, i.e. several GPIO chip instances from one device tree node.
> + * In this case the following binding is implied:
> + *
> + * foo-gpios = <&gpio instance offset flags>;
> + *
> + * Returns:
> + * GPIO number (>= 0) on success, negative errno on failure.
> + */
> +static int of_gpio_threecell_xlate(struct gpio_chip *gc,
> +                                  const struct of_phandle_args *gpiospec,
> +                                  u32 *flags)
> +{
> +       if (gc->of_gpio_n_cells != 3) {
> +               WARN_ON(1);
> +               return -EINVAL;
> +       }
> +
> +       if (WARN_ON(gpiospec->args_count != 3))
> +               return -EINVAL;
> +
> +       /*
> +        * Check chip instance number, the driver responds with true if
> +        * this is the chip we are looking for.
> +        */
> +       if (!gc->of_node_instance_match(gc, gpiospec->args[0]))

I would just pass gpiospec here. Then it can look at anything it wants
in the args.

Taking it a step further, if you made the function return the GPIO
line number, you could combine the 2 translate functions. You'd need a
default of_node_instance_match() to return args[0] for the 2 cell
case.

> +               return -EINVAL;
> +
> +       if (gpiospec->args[1] >= gc->ngpio)
> +               return -EINVAL;
> +
> +       if (flags)
> +               *flags = gpiospec->args[2];

With the above, this would work for both cases:

gpiospec->args[gpiospec->args_count - 1]


Just an idea. Keep it as-is if you prefer.

Rob