Message-ID: <CACRpkdZOnjW-mjQbTOUybNod8PyT+p66v8h2c4ejAfV7wo-8Tg@mail.gmail.com>
Date: Thu, 14 Aug 2025 10:39:23 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: AKASHI Takahiro <takahiro.akashi@...aro.org>, Michal Simek <michal.simek@....com>, 
	Sudeep Holla <sudeep.holla@....com>, Cristian Marussi <cristian.marussi@....com>, 
	Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	arm-scmi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH RFC v2 6/7] pinctrl-scmi: Add GPIO support

On Sun, Jul 20, 2025 at 9:39 PM Dan Carpenter <dan.carpenter@...aro.org> wrote:

> This adds GPIO support to the SCMI pin controller driver.  It's an RFC
> patch because I'm not really sure how these are used and so I don't
> know how they should be configured via devicetree.  I've labeled the
> places where I think devicetree configuration would go with a FIXME.
>
> This driver was based on work from Takahiro AKASHI.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>

(...)

> @@ -955,19 +954,16 @@ int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned
>         if (ret)
>                 return ret;
>
> -       ops = pctldev->desc->confops;
> -       if (!ops || !ops->pin_config_get)
> -               return -EINVAL;
> -
>         mutex_lock(&pctldev->mutex);
>         pin = gpio_to_pin(range, gc, offset);
> -       ret = ops->pin_config_get(pctldev, pin, config);
> +       ret = pin_config_get_for_pin(pctldev, pin, config);
>         mutex_unlock(&pctldev->mutex);
>
>         if (ret)
>                 return ret;
>
>         *config = pinconf_to_config_argument(*config);
> +
>         return 0;
>  }

Looks reasonable.

> @@ -42,6 +46,7 @@ struct scmi_pinctrl {
>         unsigned int nr_functions;
>         struct pinctrl_pin_desc *pins;
>         unsigned int nr_pins;
> +       struct gpio_chip *gc;

This being a pointer is slightly confusing. And looking below I see why:

> +       gpio = fwnode_get_named_child_node(dev->fwnode, "gpio");
> +       if (!gpio)
> +               return 0;
> +
> +       pmx->gc = devm_kcalloc(dev, pmx->nr_functions, sizeof(*pmx->gc), GFP_KERNEL);
> +       if (!pmx->gc)
> +               return -ENOMEM;

So this needs a comment to what is actually going on here.

To me it looks like the code is instantiating a struct gpio_chip for
each function on the pin mux.

That feels wrong: instead we should probably instantiate *one* gpio_chip for
the whole pinmux and then create individual gpio lines for each
pin that can work as a GPIO.

> +       for (i = 0; i < pmx->nr_functions; i++) {
> +               const char *fn_name;
> +
> +               ret = pinctrl_ops->is_gpio(pmx->ph, i, FUNCTION_TYPE);
> +               if (ret < 0)
> +                       return ret;

So we are only looking for functions that are of GPIO type.

> +               if (ret == false)
> +                       continue;
> +
> +               ret = pinctrl_ops->name_get(pmx->ph, i, FUNCTION_TYPE, &fn_name);
> +               if (ret)
> +                       return ret;
> +
> +               pmx->gc[i].label = devm_kasprintf(dev, GFP_KERNEL, "%s", fn_name);
> +               if (!pmx->gc[i].label)
> +                       return -ENOMEM;
> +
> +               pmx->gc[i].owner = THIS_MODULE;
> +               pmx->gc[i].get = pinctrl_gpio_get;
> +               pmx->gc[i].set = pinctrl_gpio_set;
> +               pmx->gc[i].get_direction = pinctrl_gpio_get_direction;
> +               pmx->gc[i].direction_input = pinctrl_gpio_direction_input;
> +               pmx->gc[i].direction_output = pinctrl_gpio_direction_output_wrapper;
> +               pmx->gc[i].add_pin_ranges = gpio_add_pin_ranges;
> +
> +               // FIXME: verify that this is correct
> +               pmx->gc[i].can_sleep = true;
> +
> +               ret = get_nr_pins_in_function(pmx, i);
> +               if (ret < 0)
> +                       return ret;
> +               pmx->gc[i].ngpio = ret;

Please put a print here and see how many pins there are usually in the
function here. In my mind it should always be 1 and if it is not 1 then
something is wrong.

We cannot instantiate n gpio_chips for a pin controller where n is the
number of functions for GPIO, intead we need to instantiate one gpio_chip
for the whole pin controller with ngpio set to the number of pins
that exist on the pin controller and mask of any lines that cannot be
used as GPIO from the chip using the gc->init_valid_mask() callback.

I think you want to look at Bartosz series that introduce a GPIO
function category and use this as a basis for your work, because
I plan to merge this for v6.18:
https://lore.kernel.org/linux-gpio/20250812-pinctrl-gpio-pinfuncs-v4-0-bb3906c55e64@linaro.org/T/#t

If you can first patch the SCMI pin control driver to use
pinmux_generic_add_pinfunction() based on Bartosz patch set
and get to this:

+ .get_functions_count = pinmux_generic_get_function_count,
+ .get_function_name = pinmux_generic_get_function_name,
+ .get_function_groups = pinmux_generic_get_function_groups,

Then it is also easier to figure out if a line is GPIO or not (the
core will help you).

I hope the conversion to Bartosz generics could be
straightforward... It looks straightforward but there may
be devil in the details :)

After this you can use pinmux_generic_function_is_gpio()
to check if a certain function is used for GPIO or not, I think
in the SCMI case this is done with a simple strcmp().

Then you can use this to instantiate a gpio_chip for SCMI
that will dynamically allow pins to be remuxed into GPIO and
made available on the chip as we go.

I'm sure Bartosz can help out a bit with some details!

Yours,
Linus Walleij

Powered by blists - more mailing lists