| Message ID | 1423731809-4800-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > I wonder if gpiod_get_optional et all should be changed to return NULL > instead. This is actually the norm in most subsystems returning cookie pointers, clk, regulator, pinctrl... a NULL pointer is a functional noop. But normally then NULL is returned from all stubs, not just optional. Alexandre, what do you say? > The obvious downside is that if the device tree specifies a > reset-gpio and the kernel just fails to use it because there is some > code missing, this should better be an error. (The adau1977 code has > this problem already know, but when changing devm_gpiod_get_optional all > callers are affected.) Device Tree-specific problems is not something we design subsystems for, we try to just accomodate them. I'm not sure I fully understand what you mean here. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, Mar 06, 2015 at 09:26:26AM +0100, Linus Walleij wrote: > On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > I wonder if gpiod_get_optional et all should be changed to return NULL > > instead. > > This is actually the norm in most subsystems returning cookie > pointers, clk, regulator, pinctrl... a NULL pointer is a functional > noop. But normally then NULL is returned from all stubs, not > just optional. > > Alexandre, what do you say? > > > The obvious downside is that if the device tree specifies a > > reset-gpio and the kernel just fails to use it because there is some > > code missing, this should better be an error. (The adau1977 code has > > this problem already know, but when changing devm_gpiod_get_optional all > > callers are affected.) > > Device Tree-specific problems is not something we design > subsystems for, we try to just accomodate them. I'm not > sure I fully understand what you mean here. Consider you have a device that has: enable-gpio = <&gpio3 12 0>; and you do in your driver: enablegpio = devm_gpiod_get_optional(... "enable" ...); . If GPIOLIB is off, enablegpio gets assigned NULL and the driver continues happily without enabling the device which most likely is a bug. So IMHO the logic in devm_gpiod_get_optional for the GPIOLIB=n case should be: if (device_has_gpio()) return ERR_PTR(-ENOSYS); else return NULL; . device_has_gpio should use similar logic like gpiod_get_index to check if there is a gpio. If this is considered to be too complicated for a disabled subsystem, returning -ENOSYS unconditionally is better than NULL. Best regards Uwe
On Fri, Mar 6, 2015 at 9:59 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Fri, Mar 06, 2015 at 09:26:26AM +0100, Linus Walleij wrote: >> On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> >> > I wonder if gpiod_get_optional et all should be changed to return NULL >> > instead. >> >> This is actually the norm in most subsystems returning cookie >> pointers, clk, regulator, pinctrl... a NULL pointer is a functional >> noop. But normally then NULL is returned from all stubs, not >> just optional. >> >> Alexandre, what do you say? >> >> > The obvious downside is that if the device tree specifies a >> > reset-gpio and the kernel just fails to use it because there is some >> > code missing, this should better be an error. (The adau1977 code has >> > this problem already know, but when changing devm_gpiod_get_optional all >> > callers are affected.) >> >> Device Tree-specific problems is not something we design >> subsystems for, we try to just accomodate them. I'm not >> sure I fully understand what you mean here. > Consider you have a device that has: > > enable-gpio = <&gpio3 12 0>; > > and you do in your driver: > > enablegpio = devm_gpiod_get_optional(... "enable" ...); > > . If GPIOLIB is off, enablegpio gets assigned NULL and the driver > continues happily without enabling the device which most likely is a > bug. > > So IMHO the logic in devm_gpiod_get_optional for the GPIOLIB=n case > should be: > > if (device_has_gpio()) > return ERR_PTR(-ENOSYS); > else > return NULL; > > . device_has_gpio should use similar logic like gpiod_get_index to check > if there is a gpio. If this is considered to be too complicated for a > disabled subsystem, returning -ENOSYS unconditionally is better than > NULL. Aha Oh that's complex. I cannot answer that myself, it needs feedback from the OF/DT maintainers before I even dare to think anything, as it is related to how hardware is described :/ Added devicetree-list and DT/OF maintainers to To: line to get some feedback on this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 6, 2015 at 5:59 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello, > > On Fri, Mar 06, 2015 at 09:26:26AM +0100, Linus Walleij wrote: >> On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> >> > I wonder if gpiod_get_optional et all should be changed to return NULL >> > instead. >> >> This is actually the norm in most subsystems returning cookie >> pointers, clk, regulator, pinctrl... a NULL pointer is a functional >> noop. But normally then NULL is returned from all stubs, not >> just optional. >> >> Alexandre, what do you say? >> >> > The obvious downside is that if the device tree specifies a >> > reset-gpio and the kernel just fails to use it because there is some >> > code missing, this should better be an error. (The adau1977 code has >> > this problem already know, but when changing devm_gpiod_get_optional all >> > callers are affected.) >> >> Device Tree-specific problems is not something we design >> subsystems for, we try to just accomodate them. I'm not >> sure I fully understand what you mean here. > Consider you have a device that has: > > enable-gpio = <&gpio3 12 0>; > > and you do in your driver: > > enablegpio = devm_gpiod_get_optional(... "enable" ...); > > . If GPIOLIB is off, enablegpio gets assigned NULL and the driver > continues happily without enabling the device which most likely is a > bug. > > So IMHO the logic in devm_gpiod_get_optional for the GPIOLIB=n case > should be: > > if (device_has_gpio()) > return ERR_PTR(-ENOSYS); > else > return NULL; > > . device_has_gpio should use similar logic like gpiod_get_index to check > if there is a gpio. If this is considered to be too complicated for a > disabled subsystem, returning -ENOSYS unconditionally is better than > NULL. I should have replied one month ago, but if gpiolib is disabled, how can we use gpiolib-like logic to check the existence of a GPIO? Having GPIO disabled means there is no GPIO support, including the ability to look for GPIOs. -ENOSYS is a well-documented error-code which meaning also applies to the gpio_*_optional functions (we don't have support for the operation you requested). If a driver or architecture really, really needs GPIO support they can require or depend on CONFIG_GPIOLIB, and the problem goes away. If they can work with and without gpiolib, then they should check for -ENOSYS when they request GPIOs and behave accordingly. Moving the interpretation of what the absence of gpiolib means down to the GPIO functions themselves is actually what might lead consumers to not know the result of their request. For this reason I would say that -ENOSYS is appropriate here. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 9, 2015 at 4:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > Having GPIO disabled means there is no GPIO support, including the > ability to look for GPIOs. -ENOSYS is a well-documented error-code > which meaning also applies to the gpio_*_optional functions (we don't > have support for the operation you requested). If a driver or > architecture really, really needs GPIO support they can require or > depend on CONFIG_GPIOLIB, and the problem goes away. If they can work > with and without gpiolib, then they should check for -ENOSYS when they > request GPIOs and behave accordingly. Makes sense. We have ARCH_WANT_OPTIONAL_GPIOLIB and ARCH_REQUIRE_GPIOLIB for this reason. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote: > I should have replied one month ago, but if gpiolib is disabled, how > can we use gpiolib-like logic to check the existence of a GPIO? > > Having GPIO disabled means there is no GPIO support, including the > ability to look for GPIOs. -ENOSYS is a well-documented error-code > which meaning also applies to the gpio_*_optional functions (we don't > have support for the operation you requested). If a driver or > architecture really, really needs GPIO support they can require or > depend on CONFIG_GPIOLIB, and the problem goes away. If they can work > with and without gpiolib, then they should check for -ENOSYS when they > request GPIOs and behave accordingly. What whould be the right behaviour in your eyes? I hope it's not if (ret != -ENOSYS) return ret; /* continue and ignore error */ > Moving the interpretation of what the absence of gpiolib means down to > the GPIO functions themselves is actually what might lead consumers to > not know the result of their request. For this reason I would say that > -ENOSYS is appropriate here. Yeah, I fully agree. Best regards Uwe
On Tue, Apr 28, 2015 at 12:21 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello, > > On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote: >> I should have replied one month ago, but if gpiolib is disabled, how >> can we use gpiolib-like logic to check the existence of a GPIO? >> >> Having GPIO disabled means there is no GPIO support, including the >> ability to look for GPIOs. -ENOSYS is a well-documented error-code >> which meaning also applies to the gpio_*_optional functions (we don't >> have support for the operation you requested). If a driver or >> architecture really, really needs GPIO support they can require or >> depend on CONFIG_GPIOLIB, and the problem goes away. If they can work >> with and without gpiolib, then they should check for -ENOSYS when they >> request GPIOs and behave accordingly. > What whould be the right behaviour in your eyes? I hope it's not > > if (ret != -ENOSYS) > return ret; > > /* continue and ignore error */ If a consumer absolutely needs a GPIO (most of the drivers out there I believe), then -ENOSYS can be handled like any other error. If it doesn't, and the driver is fine without GPIO support as well (meaning that it can somehow work even if a GPIO is declared, but GPIO support is not compiled in), then it will need to explicitly handle that particular error. That case should be rare though - most drivers will want to propagate -ENOSYS. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Tue, Apr 28, 2015 at 12:31:37PM +0900, Alexandre Courbot wrote: > On Tue, Apr 28, 2015 at 12:21 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote: > >> Having GPIO disabled means there is no GPIO support, including the > >> ability to look for GPIOs. -ENOSYS is a well-documented error-code > >> which meaning also applies to the gpio_*_optional functions (we don't > >> have support for the operation you requested). If a driver or > >> architecture really, really needs GPIO support they can require or > >> depend on CONFIG_GPIOLIB, and the problem goes away. If they can work > >> with and without gpiolib, then they should check for -ENOSYS when they > >> request GPIOs and behave accordingly. > > What whould be the right behaviour in your eyes? I hope it's not > > > > if (ret != -ENOSYS) > > return ret; > > > > /* continue and ignore error */ > > If a consumer absolutely needs a GPIO (most of the drivers out there I > believe), then -ENOSYS can be handled like any other error. If it > doesn't, and the driver is fine without GPIO support as well (meaning > that it can somehow work even if a GPIO is declared, but GPIO support > is not compiled in), then it will need to explicitly handle that > particular error. That case should be rare though - most drivers will > want to propagate -ENOSYS. Ack. Uwe
On Tue, Apr 28, 2015 at 3:45 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello, > > On Tue, Apr 28, 2015 at 12:31:37PM +0900, Alexandre Courbot wrote: >> On Tue, Apr 28, 2015 at 12:21 AM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> > On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote: >> >> Having GPIO disabled means there is no GPIO support, including the >> >> ability to look for GPIOs. -ENOSYS is a well-documented error-code >> >> which meaning also applies to the gpio_*_optional functions (we don't >> >> have support for the operation you requested). If a driver or >> >> architecture really, really needs GPIO support they can require or >> >> depend on CONFIG_GPIOLIB, and the problem goes away. If they can work >> >> with and without gpiolib, then they should check for -ENOSYS when they >> >> request GPIOs and behave accordingly. >> > What whould be the right behaviour in your eyes? I hope it's not >> > >> > if (ret != -ENOSYS) >> > return ret; >> > >> > /* continue and ignore error */ >> >> If a consumer absolutely needs a GPIO (most of the drivers out there I >> believe), then -ENOSYS can be handled like any other error. If it >> doesn't, and the driver is fine without GPIO support as well (meaning >> that it can somehow work even if a GPIO is declared, but GPIO support >> is not compiled in), then it will need to explicitly handle that >> particular error. That case should be rare though - most drivers will >> want to propagate -ENOSYS. > Ack. Of course if you can think of a more elegant way to handle this, by all means, please let us know. :) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index fd85cb120ee0..f68244ffd3a9 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -132,14 +132,14 @@ static inline struct gpio_desc *__must_check __gpiod_get_optional(struct device *dev, const char *con_id, enum gpiod_flags flags) { - return ERR_PTR(-ENOSYS); + return NULL; } static inline struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev, const char *con_id, unsigned int index, enum gpiod_flags flags) { - return ERR_PTR(-ENOSYS); + return NULL; } static inline void gpiod_put(struct gpio_desc *desc) @@ -171,14 +171,14 @@ static inline struct gpio_desc *__must_check __devm_gpiod_get_optional(struct device *dev, const char *con_id, enum gpiod_flags flags) { - return ERR_PTR(-ENOSYS); + return NULL; } static inline struct gpio_desc *__must_check __devm_gpiod_get_index_optional(struct device *dev, const char *con_id, unsigned int index, enum gpiod_flags flags) { - return ERR_PTR(-ENOSYS); + return NULL; } static inline void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
If you look for example at sound/soc/codecs/adau1977.c it has: adau1977->reset_gpio = devm_gpiod_get(dev, "reset"); if (IS_ERR(adau1977->reset_gpio)) { ret = PTR_ERR(adau1977->reset_gpio); if (ret != -ENOENT && ret != -ENOSYS) return PTR_ERR(adau1977->reset_gpio); adau1977->reset_gpio = NULL; } This code could better make use of devm_gpiod_get_optional like: adau1977->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(adau1977->reset_gpio)) return PTR_ERR(adau1977->reset_gpio); but this slightly changes semantics. I.e. if GPIOLIB is not enabled devm_gpiod_get_optional unconditionally returns -ENOSYS which is ignored explicitly above but an error in the changed code. I wonder if gpiod_get_optional et all should be changed to return NULL instead. The obvious downside is that if the device tree specifies a reset-gpio and the kernel just fails to use it because there is some code missing, this should better be an error. (The adau1977 code has this problem already know, but when changing devm_gpiod_get_optional all callers are affected.) What do you think? --- include/linux/gpio/consumer.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)