Re: [PATCH v2] gpiolib: Extend software-node support to support secondary software-nodes

[Hans de Goede <hansg@kernel.org>]

On 20-Sep-25 11:27 PM, Dmitry Torokhov wrote:
> On Sat, Sep 20, 2025 at 10:09:55PM +0200, Hans de Goede wrote:
>> When a software-node gets added to a device which already has another
>> fwnode as primary node it will become the secondary fwnode for that
>> device.
>>
>> Currently if a software-node with GPIO properties ends up as the secondary
>> fwnode then gpiod_find_by_fwnode() will fail to find the GPIOs.
>>
>> Add a new gpiod_fwnode_lookup() helper which falls back to calling
>> gpiod_find_by_fwnode() with the secondary fwnode if the GPIO was not
>> found in the primary fwnode.
>>
>> Fixes: e7f9ff5dc90c ("gpiolib: add support for software nodes")
>> Cc: stable@vger.kernel.org
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
> 
> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
>> ---
>> Changes in v2:
>> - Add a new gpiod_fwnode_lookup() helper instead of putting the secondary
>>   fwnode check inside gpiod_find_by_fwnode()
>> ---
>>  drivers/gpio/gpiolib.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 0d2b470a252e..74d54513730a 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -4604,6 +4604,23 @@ static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
>>  	return desc;
>>  }
>>  
>> +static struct gpio_desc *gpiod_fwnode_lookup(struct fwnode_handle *fwnode,
>> +					     struct device *consumer,
>> +					     const char *con_id,
>> +					     unsigned int idx,
>> +					     enum gpiod_flags *flags,
>> +					     unsigned long *lookupflags)
>> +{
>> +	struct gpio_desc *desc;
>> +
>> +	desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, flags, lookupflags);
>> +	if (gpiod_not_found(desc) && !IS_ERR_OR_NULL(fwnode))
>> +		desc = gpiod_find_by_fwnode(fwnode->secondary, consumer, con_id,
>> +					    idx, flags, lookupflags);
>> +
>> +	return desc;
> 
> Bikeshedding for later. Maybe do it like this in case we can have more
> than 2 nodes at some point?
> 
>         do {
> 		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, flags, lookupflags);
> 		if (!gpiod_not_found(desc))
> 			return desc;
> 
> 		fwnode = fwnode->secondary;
> 	} while (!IS_ERR_OR_NULL(fwnode));
> 
> 	return ERR_PTR(-ENOENT);

At a minimum this would need to a regular while () {} loop then,
the initial fwnode may also be NULL and we don't want to deref that.

Andy did mention turning the fwnode-s into a regular linked-list
in the future, but I think that would be using <linux/list.h> then,
replacing the secondary pointer with a list head ?

Regards,

Hans






> 
>> +}
>> +
>>  struct gpio_desc *gpiod_find_and_request(struct device *consumer,
>>  					 struct fwnode_handle *fwnode,
>>  					 const char *con_id,
>> @@ -4622,8 +4639,8 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
>>  	int ret = 0;
>>  
>>  	scoped_guard(srcu, &gpio_devices_srcu) {
>> -		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
>> -					    &flags, &lookupflags);
>> +		desc = gpiod_fwnode_lookup(fwnode, consumer, con_id, idx,
>> +					   &flags, &lookupflags);
>>  		if (gpiod_not_found(desc) && platform_lookup_allowed) {
>>  			/*
>>  			 * Either we are not using DT or ACPI, or their lookup
> 
> Thanks.
>