[PATCH] gpio: aggregator: restore the set_config operation

[PATCH] gpio: aggregator: restore the set_config operation

[Thomas Richard]

Restore the set_config operation, as it was lost during the refactoring of
the gpio-aggregator driver while creating the gpio forwarder library.

Fixes: b31c68fd851e7 ("gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202509281206.a7334ae8-lkp@intel.com
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-aggregator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 37600faf4a4b728ceb1937538b3f6b78ab3e90fa..416f265d09d070ee33e30bf6773e9d8fffc242bd 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -723,6 +723,7 @@ struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
 	chip->get_multiple = gpio_fwd_get_multiple_locked;
 	chip->set = gpio_fwd_set;
 	chip->set_multiple = gpio_fwd_set_multiple_locked;
+	chip->set_config = gpio_fwd_set_config;
 	chip->to_irq = gpio_fwd_to_irq;
 	chip->base = -1;
 	chip->ngpio = ngpios;

---
base-commit: bc061143637532c08d9fc657eec93fdc2588068e
change-id: 20250929-gpio-aggregator-fix-set-config-callback-6d2bff306023

Best regards,
-- 
Thomas Richard <thomas.richard@bootlin.com>

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Geert Uytterhoeven]

Hi Thomas,

On Mon, 29 Sept 2025 at 12:03, Thomas Richard
<thomas.richard@bootlin.com> wrote:
> Restore the set_config operation, as it was lost during the refactoring of
> the gpio-aggregator driver while creating the gpio forwarder library.
>
> Fixes: b31c68fd851e7 ("gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202509281206.a7334ae8-lkp@intel.com
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -723,6 +723,7 @@ struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
>         chip->get_multiple = gpio_fwd_get_multiple_locked;
>         chip->set = gpio_fwd_set;
>         chip->set_multiple = gpio_fwd_set_multiple_locked;
> +       chip->set_config = gpio_fwd_set_config;
>         chip->to_irq = gpio_fwd_to_irq;
>         chip->base = -1;
>         chip->ngpio = ngpios;
>

Is there any specific reason why you are doing this unconditionally,
instead of only when any of its parents support .set_config(), like
was done before?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Bartosz Golaszewski]

On Mon, Sep 29, 2025 at 12:03 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> Restore the set_config operation, as it was lost during the refactoring of
> the gpio-aggregator driver while creating the gpio forwarder library.
>
> Fixes: b31c68fd851e7 ("gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202509281206.a7334ae8-lkp@intel.com
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/gpio/gpio-aggregator.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index 37600faf4a4b728ceb1937538b3f6b78ab3e90fa..416f265d09d070ee33e30bf6773e9d8fffc242bd 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -723,6 +723,7 @@ struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
>         chip->get_multiple = gpio_fwd_get_multiple_locked;
>         chip->set = gpio_fwd_set;
>         chip->set_multiple = gpio_fwd_set_multiple_locked;
> +       chip->set_config = gpio_fwd_set_config;
>         chip->to_irq = gpio_fwd_to_irq;
>         chip->base = -1;
>         chip->ngpio = ngpios;
>
> ---

Thanks for fixing it. I saw the report but I had already prepared my
big PR for Linus. This will still make v6.18-rc1, I will send it later
into the merge window. Please address Geert's review.

Bartosz

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Thomas Richard]

> 
> Is there any specific reason why you are doing this unconditionally,
> instead of only when any of its parents support .set_config(), like
> was done before?
> 
My idea was: it will be handled by the core, so the if statement is not
needed. But if we conditionally add the operation we can save some time
in case there is no chip supporting set_config().

Thomas

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Thomas Richard]

On 10/3/25 3:59 PM, Thomas Richard wrote:
>>
>> Is there any specific reason why you are doing this unconditionally,
>> instead of only when any of its parents support .set_config(), like
>> was done before?
>>
> My idea was: it will be handled by the core, so the if statement is not
> needed. But if we conditionally add the operation we can save some time
> in case there is no chip supporting set_config().

I just remembered the true reason why I'm doing this unconditionally.

The user of the forwarder can override GPIO operations like I do in the
pinctrl-upboard driver [1].
And now we can add/remove GPIO desc at runtime, if set_config() is set
conditionally in gpiochip_fwd_desc_add() it will override the custom
set_config() operation.
So the only solution is to set the set_config() operation
unconditionally in devm_gpiochip_fwd_alloc().

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/pinctrl-upboard.c#n1044

Thomas

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Geert Uytterhoeven]

Hi Thomas,

On Fri, 3 Oct 2025 at 16:30, Thomas Richard <thomas.richard@bootlin.com> wrote:
> On 10/3/25 3:59 PM, Thomas Richard wrote:
> >> Is there any specific reason why you are doing this unconditionally,
> >> instead of only when any of its parents support .set_config(), like
> >> was done before?
> >>
> > My idea was: it will be handled by the core, so the if statement is not
> > needed. But if we conditionally add the operation we can save some time
> > in case there is no chip supporting set_config().
>
> I just remembered the true reason why I'm doing this unconditionally.
>
> The user of the forwarder can override GPIO operations like I do in the
> pinctrl-upboard driver [1].
> And now we can add/remove GPIO desc at runtime, if set_config() is set
> conditionally in gpiochip_fwd_desc_add() it will override the custom
> set_config() operation.
> So the only solution is to set the set_config() operation
> unconditionally in devm_gpiochip_fwd_alloc().

OK, that makes sense, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I do find this overriding a bit fragile.
And in theory, such a driver could override chip->can_sleep to false,
which might be overwritten again by gpiochip_fwd_desc_add()...

> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/pinctrl-upboard.c#n1044

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Thomas Richard]

On 10/6/25 9:42 AM, Geert Uytterhoeven wrote:
> Hi Thomas,
> 
> On Fri, 3 Oct 2025 at 16:30, Thomas Richard <thomas.richard@bootlin.com> wrote:
>> On 10/3/25 3:59 PM, Thomas Richard wrote:
>>>> Is there any specific reason why you are doing this unconditionally,
>>>> instead of only when any of its parents support .set_config(), like
>>>> was done before?
>>>>
>>> My idea was: it will be handled by the core, so the if statement is not
>>> needed. But if we conditionally add the operation we can save some time
>>> in case there is no chip supporting set_config().
>>
>> I just remembered the true reason why I'm doing this unconditionally.
>>
>> The user of the forwarder can override GPIO operations like I do in the
>> pinctrl-upboard driver [1].
>> And now we can add/remove GPIO desc at runtime, if set_config() is set
>> conditionally in gpiochip_fwd_desc_add() it will override the custom
>> set_config() operation.
>> So the only solution is to set the set_config() operation
>> unconditionally in devm_gpiochip_fwd_alloc().
> 
> OK, that makes sense, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> I do find this overriding a bit fragile.
> And in theory, such a driver could override chip->can_sleep to false,
> which might be overwritten again by gpiochip_fwd_desc_add()...

Yes, I agree.
Maybe we should not export gpiochip_fwd_get_gpiochip() so it will not be
possible to get the gpio_chip and override some properties. And we add
some helpers to override the GPIO operations. I can make a try.

Regards,

Thomas

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Thomas Richard]

Hi Bartosz,

On 9/29/25 2:45 PM, Bartosz Golaszewski wrote:
> On Mon, Sep 29, 2025 at 12:03 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>>
>> Restore the set_config operation, as it was lost during the refactoring of
>> the gpio-aggregator driver while creating the gpio forwarder library.
>>
>> Fixes: b31c68fd851e7 ("gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd")
>> Reported-by: kernel test robot <oliver.sang@intel.com>
>> Closes: https://lore.kernel.org/oe-lkp/202509281206.a7334ae8-lkp@intel.com
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
>>  drivers/gpio/gpio-aggregator.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
>> index 37600faf4a4b728ceb1937538b3f6b78ab3e90fa..416f265d09d070ee33e30bf6773e9d8fffc242bd 100644
>> --- a/drivers/gpio/gpio-aggregator.c
>> +++ b/drivers/gpio/gpio-aggregator.c
>> @@ -723,6 +723,7 @@ struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
>>         chip->get_multiple = gpio_fwd_get_multiple_locked;
>>         chip->set = gpio_fwd_set;
>>         chip->set_multiple = gpio_fwd_set_multiple_locked;
>> +       chip->set_config = gpio_fwd_set_config;
>>         chip->to_irq = gpio_fwd_to_irq;
>>         chip->base = -1;
>>         chip->ngpio = ngpios;
>>
>> ---
> 
> Thanks for fixing it. I saw the report but I had already prepared my
> big PR for Linus. This will still make v6.18-rc1, I will send it later
> into the merge window. Please address Geert's review.

Gentle ping, just in case of you forgot it :)
Geert sent his Reviewed-by tag.

Best Regards,
Thomas

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Mon, 29 Sep 2025 12:03:13 +0200, Thomas Richard wrote:
> Restore the set_config operation, as it was lost during the refactoring of
> the gpio-aggregator driver while creating the gpio forwarder library.
> 
> 

Applied, thanks!

[1/1] gpio: aggregator: restore the set_config operation
      https://git.kernel.org/brgl/linux/c/5232334baec371a3c9d9192ba7d2da2d88a85333

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH] gpio: aggregator: restore the set_config operation

[Bartosz Golaszewski]

On Wed, Nov 5, 2025 at 10:59 AM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> Hi Bartosz,
>
> On 9/29/25 2:45 PM, Bartosz Golaszewski wrote:
> > On Mon, Sep 29, 2025 at 12:03 PM Thomas Richard
> > <thomas.richard@bootlin.com> wrote:
> >>
> >> Restore the set_config operation, as it was lost during the refactoring of
> >> the gpio-aggregator driver while creating the gpio forwarder library.
> >>
> >> Fixes: b31c68fd851e7 ("gpio: aggregator: handle runtime registration of gpio_desc in gpiochip_fwd")
> >> Reported-by: kernel test robot <oliver.sang@intel.com>
> >> Closes: https://lore.kernel.org/oe-lkp/202509281206.a7334ae8-lkp@intel.com
> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> >> ---
> >>  drivers/gpio/gpio-aggregator.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> >> index 37600faf4a4b728ceb1937538b3f6b78ab3e90fa..416f265d09d070ee33e30bf6773e9d8fffc242bd 100644
> >> --- a/drivers/gpio/gpio-aggregator.c
> >> +++ b/drivers/gpio/gpio-aggregator.c
> >> @@ -723,6 +723,7 @@ struct gpiochip_fwd *devm_gpiochip_fwd_alloc(struct device *dev,
> >>         chip->get_multiple = gpio_fwd_get_multiple_locked;
> >>         chip->set = gpio_fwd_set;
> >>         chip->set_multiple = gpio_fwd_set_multiple_locked;
> >> +       chip->set_config = gpio_fwd_set_config;
> >>         chip->to_irq = gpio_fwd_to_irq;
> >>         chip->base = -1;
> >>         chip->ngpio = ngpios;
> >>
> >> ---
> >
> > Thanks for fixing it. I saw the report but I had already prepared my
> > big PR for Linus. This will still make v6.18-rc1, I will send it later
> > into the merge window. Please address Geert's review.
>
> Gentle ping, just in case of you forgot it :)
> Geert sent his Reviewed-by tag.
>
> Best Regards,
> Thomas
>

It did indeed escape my attention, thanks for the heads-up. Now queued.

Bartosz