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

[Rasmus Villemoes]

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.

I copied the logic from some other driver that also had an optional
reset-gpios and checked for -ENOENT, so I assumed that was the right
thing to do.

> {
>     struct gpio_desc *desc = devm_gpiod_get_index(dev, id, index, flags);
> 
>     if (IS_ERR(desc))
>         return NULL;
> 
>     return desc;
> }
> """
> 
> It seems we only need to check whether rc is non-zero, but it doesn't
> check that it's not ENOENT. I think we would benefit from having the
> same logic here.

What are you proposing exactly? That devm_gpiod_get_index_optional()
starts propagating errors which are not -ENOENT? That would make sense,
but requires that callers check three-ways (NULL, IS_ERR or valid), not
just two-ways. Dunno.

> Also, maybe we need a devm_gpio_get_by_name_optional implementation in
> the subsystem so we don't have to reimplement it in drivers that want to
> use this?

Maybe, but as I said, we already have too many helpers implemented in
terms of each other with drivers using some random one even if there
happens to be another helper that "helpfully" plugs in a 0 for the index
or whatnot.

I'm unsure just exactly what I should do from here?

Rasmus