Re: [PATCH v4 5/6] gpiolib: consolidate GPIO lookups

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Nov 11, 2022 at 02:19:07PM -0800, Dmitry Torokhov wrote:
> Ensure that all paths to obtain/look up GPIOD from generic
> consumer-visible APIs go through the new gpiod_find_and_request()
> helper, so that we can easily extend it with support for new firmware
> mechanisms.
> 
> The only exception is OF-specific [devm_]gpiod_get_from_of_node() API
> that is still being used by a couple of drivers and will be removed as
> soon as patches converting them to use generic fwnode/device APIs are
> accepted.

...

> +static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
> +						struct fwnode_handle *fwnode,
> +						const char *con_id,
> +						unsigned int idx,
> +						enum gpiod_flags flags,
> +						const char *label,
> +						bool platform_lookup_allowed)
> +{
> +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> +	unsigned long lookupflags;
> +	int ret;

> +	if (!IS_ERR_OR_NULL(fwnode))

Just for the record. I haven't given my tag to this patch, because I think that
the above check (and respective assignment above) are redundant. Even comment
inside the below condition clarifies the point of the meaning of descriptor
being not found. Besides that many of device property APIs designed the way
that input fwnode can be invalid.

Nevertheless, we agreed with Dmitry that this disagreement should be solved on
maintainer's level, while it doesn't affect code functionality.

The rest of the patch is fine.

> +		desc = gpiod_find_by_fwnode(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 did not
> +		 * return a result. In that case, use platform lookup as a
> +		 * fallback.
> +		 */
> +		dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
> +		desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> +	}
> +
> +	if (IS_ERR(desc)) {
> +		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
> +		return desc;
> +	}

-- 
With Best Regards,
Andy Shevchenko