[PATCH] gpio: pca953x_gpio: support optional reset-gpios property

[Quentin Schulz]

Hi Rasmus,

On 4/10/24 14:59, Rasmus Villemoes wrote:
> On 10/04/2024 14.24, Quentin Schulz wrote:
>> Hi Rasmus,
>>
>>> @@ -321,6 +322,13 @@ static int pca953x_probe(struct udevice *dev)
>>>          driver_data = dev_get_driver_data(dev);
>>>    +    /* If a reset-gpios property is present, take the device out of
>>> reset. */
>>> +    ret = gpio_request_by_name(dev, "reset-gpios", 0, &reset,
>>> GPIOD_IS_OUT);
>>> +    if (ret && ret != -ENOENT) {
>>
>> This seems to differ from the implementation we have for optionally
>> getting gpios by index, c.f.
>> https://elixir.bootlin.com/u-boot/latest/source/drivers/gpio/gpio-uclass.c#L1498
>>
>> """
>> struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id,
>>                         unsigned int index, int flags)
>> {
>> [...]
>>      rc = gpio_request_by_name(dev, propname, index, desc, flags);
>>
>> end:
>> [...]
>>      if (rc)
>>          return ERR_PTR(rc);
>> [...]
>>      return desc;
>> }
>>
>> struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
>>                          const char *id,
>>                          unsigned int index,
>>                          int flags)
> 
> Well, that doesn't seem to have a lot of users outside tests? We really
> have way too many APIs for doing the same thing.
> 

It is actually used outside of tests as well:

   #define devm_gpiod_get_optional(dev, id, flags) \
           devm_gpiod_get_index_optional(dev, id, 0, flags)

Then:

"""
$ git grep devm_gpiod_get_optional
drivers/phy/ti/phy-j721e-wiz.c:        wiz->gpio_typec_dir = 
devm_gpiod_get_optional(dev, "typec-dir",
drivers/power/pmic/pca9450.c:                priv->sd_vsel_gpio = 
devm_gpiod_get_optional(dev, "sd-vsel",
drivers/usb/dwc3/dwc3-generic.c:                priv->ulpi_reset = 
devm_gpiod_get_optional(dev->parent, "reset",
"""

It's not THAT many but at least there are a few *real* users :)

Also, I think you actually want to use devm_gpiod_get_optional since 
it's basically just wrapping

"""
gpio_request_by_name(dev, "reset-gpios", 0, &reset, GPIOD_IS_OUT);
"""

with devres?

-ENOENT seems to happen (at least) if the property reset-gpios isn't 
found, which very much would be the case if it's optional. Now it seems 
-EINVAL could also be returned in some cases... 
https://elixir.bootlin.com/u-boot/latest/source/drivers/core/of_access.c#L669

Now... reading what the kernel does:

https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-devres.c#L186

checks the return value with
https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.h#L189

Soooo... it seems they are doing what you're doing.

So, what I suggest is the following:

1) fix devm_gpiod_get_index_optional

to return desc (which may be a pointer or an ERR_PTR, so users should do 
an IS_ERR(desc) check after call) ONLY when NOT IS_ERR(desc) && 
PTR_ERR(desc) == -ENOENT.

So:

"""
struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
						const char *id,
						unsigned int index,
						int flags)
{
	struct gpio_desc *desc = devm_gpiod_get_index(dev, id, index, flags);

	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
		return NULL;

	return desc;
}
"""

Maybe some adding some new test also for this new part of the if check?

2) use devm_gpiod_get_optional in your driver

What do you think?

Cheers,
Quentin