| Message ID | 20240410111108.84470-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | Needs Review / ACK |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | gpio: pca953x_gpio: support optional reset-gpios property | expand |
Hi Rasmus, On 4/10/24 13:11, Rasmus Villemoes wrote: > The DT bindings for the pca953x family has an optional reset-gpios > property. If present, ensure that the device is taken out of reset > before attempting to read from it. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > drivers/gpio/pca953x_gpio.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpio/pca953x_gpio.c b/drivers/gpio/pca953x_gpio.c > index b0c66d18317..24b0732f89a 100644 > --- a/drivers/gpio/pca953x_gpio.c > +++ b/drivers/gpio/pca953x_gpio.c > @@ -306,6 +306,7 @@ static int pca953x_probe(struct udevice *dev) > struct pca953x_info *info = dev_get_plat(dev); > struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); > char name[32], label[8], *str; > + struct gpio_desc reset; > int addr; > ulong driver_data; > int ret; > @@ -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) { 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. 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? Cheers, Quentin
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
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
diff --git a/drivers/gpio/pca953x_gpio.c b/drivers/gpio/pca953x_gpio.c index b0c66d18317..24b0732f89a 100644 --- a/drivers/gpio/pca953x_gpio.c +++ b/drivers/gpio/pca953x_gpio.c @@ -306,6 +306,7 @@ static int pca953x_probe(struct udevice *dev) struct pca953x_info *info = dev_get_plat(dev); struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); char name[32], label[8], *str; + struct gpio_desc reset; int addr; ulong driver_data; int ret; @@ -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) { + dev_err(dev, "requesting reset-gpios failed: %d\n", ret); + return ret; + } + info->gpio_count = driver_data & PCA_GPIO_MASK; if (info->gpio_count > MAX_BANK * BANK_SZ) { dev_err(dev, "Max support %d pins now\n", MAX_BANK * BANK_SZ);
The DT bindings for the pca953x family has an optional reset-gpios property. If present, ensure that the device is taken out of reset before attempting to read from it. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/gpio/pca953x_gpio.c | 8 ++++++++ 1 file changed, 8 insertions(+)