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
