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#L...
""" 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