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

[Alex Elder <elder@riscstar.com>]

On 2/18/25 4:28 AM, Linus Walleij 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>;
> 
> 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];

This is pseudocode, but either "(" not needed, or ")" missing.

> }
> 
> 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>;

I haven't looked for it, but is a DT binding update forthcoming?

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

This looks good.

Reviewed-by: Alex Elder <elder@riscstar.com>

> ---
>   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
> @@ -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]))
> +		return -EINVAL;
> +
> +	if (gpiospec->args[1] >= gc->ngpio)
> +		return -EINVAL;
> +
> +	if (flags)
> +		*flags = gpiospec->args[2];
> +
> +	return gpiospec->args[1];
> +}
> +
>   #if IS_ENABLED(CONFIG_OF_GPIO_MM_GPIOCHIP)
>   #include <linux/gpio/legacy-of-mm-gpiochip.h>
>   /**
> @@ -1068,7 +1111,15 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
>   	has_group_names = of_property_present(np, group_names_propname);
>   
>   	for (;; index++) {
> -		ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> +		/*
> +		 * Ordinary phandles contain 2-3 cells:
> +		 * gpios = <&gpio [instance] offset flags>;
> +		 * Ranges always contain one more cell:
> +		 * gpio-ranges <&pinctrl [gpio_instance] gpio_offet pin_offet count>;
> +		 * This is why we parse chip->of_gpio_n_cells + 1 cells
> +		 */
> +		ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges",
> +				chip->of_gpio_n_cells + 1,
>   				index, &pinspec);
>   		if (ret)
>   			break;
> @@ -1078,9 +1129,25 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
>   		if (!pctldev)
>   			return -EPROBE_DEFER;
>   
> -		offset = pinspec.args[0];
> -		pin = pinspec.args[1];
> -		count = pinspec.args[2];
> +		if (chip->of_gpio_n_cells == 3) {
> +			/* First cell is the gpiochip instance number */
> +			offset = pinspec.args[1];
> +			pin = pinspec.args[2];
> +			count = pinspec.args[3];
> +		} else {
> +			offset = pinspec.args[0];
> +			pin = pinspec.args[1];
> +			count = pinspec.args[2];
> +		}
> +
> +		/*
> +		 * With multiple GPIO chips per node, check that this chip is the
> +		 * right instance.
> +		 */
> +		if (chip->of_node_instance_match &&
> +		    (chip->of_gpio_n_cells == 3) &&
> +		    !chip->of_node_instance_match(chip, pinspec.args[0]))
> +			continue;
>   
>   		/* Ignore ranges outside of this GPIO chip */
>   		if (offset >= (chip->offset + chip->ngpio))
> @@ -1170,8 +1237,14 @@ int of_gpiochip_add(struct gpio_chip *chip)
>   		return 0;
>   
>   	if (!chip->of_xlate) {
> -		chip->of_gpio_n_cells = 2;
> -		chip->of_xlate = of_gpio_simple_xlate;
> +		if (chip->of_gpio_n_cells == 3) {
> +			if (!chip->of_node_instance_match)
> +				return -EINVAL;
> +			chip->of_xlate = of_gpio_threecell_xlate;
> +		} else {
> +			chip->of_gpio_n_cells = 2;
> +			chip->of_xlate = of_gpio_twocell_xlate;
> +		}
>   	}
>   
>   	if (chip->of_gpio_n_cells > MAX_PHANDLE_ARGS)
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 2dd7cb9cc270a68ddedbcdd5d44e0d0f88dfa785..70a361f6aba06d4a11e5ca913ec79411d7a11b3c 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -516,10 +516,32 @@ struct gpio_chip {
>   	/**
>   	 * @of_gpio_n_cells:
>   	 *
> -	 * Number of cells used to form the GPIO specifier.
> +	 * Number of cells used to form the GPIO specifier. The standard i 2

s/standard i /standard is /

> +	 * cells:
> +	 *
> +	 * gpios = <&gpio offset flags>;
> +	 *
> +	 * some complex GPIO controllers instantiate more than one chip per
> +	 * device tree node and have 3 cells:
> +	 *
> +	 * gpios = <&gpio instance offset flags>;
> +	 *
> +	 * Legacy GPIO controllers may even have 1 cell:
> +	 *
> +	 * gpios = <&gpio offset>;
>   	 */
>   	unsigned int of_gpio_n_cells;
>   
> +	/**
> +	 * of_node_instance_match:
> +	 *
> +	 * Determine if a chip is the right instance. Must be implemented by
> +	 * any driver using more than one gpio_chip per device tree node.
> +	 * Returns true if gc is the instance indicated by i (which is the
> +	 * first cell in the phandles for GPIO lines and gpio-ranges).
> +	 */
> +	bool (*of_node_instance_match)(struct gpio_chip *gc, unsigned int i);
> +
>   	/**
>   	 * @of_xlate:
>   	 *
>