gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
PATCH 1/1 gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled
Given the intent behind gpiod_get_optional() and friends it does not make sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to work just fine without gpio so let's behave as if gpio was not found. Otherwise we have to special-case -ENOSYS in drivers.
--- include/linux/gpio/consumer.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
include/linux/gpio/consumer.h +4 -4
@@ -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)
Signature · 5 lines
2.2.0.rc0.207.ga3a616c -- Dmitry
Quoted 4 lines · Dmitry Torokhov
Given the intent behind
gpiod_get_optional()and friends it does not make sense to return-ENOSYSwhen GPIOLIB is disabled: the driver is expected to work just fine without gpio so let's behave as if gpio was not found. Otherwise we have to special-case-ENOSYSin drivers.
Interestingly Uwe sent a RFC for this one week ago:
http://patchwork.ozlabs.org/patch/439135/
Maybe credit him with a Suggested-by.?
I should have commented at that time, but let's do it now: I agree with the idea, but this leaves the door open to confusing situations in case gpiolib was unintentionally disabled. Could you also add a note in the documentation of this function to explain this behavior, to spare a few headaches to users of this function?
Quoted 50 lines
Signed-off-by Dmitry Torokhov <[email protected]>--- include/linux/gpio/consumer.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)include/linux/gpio/consumer.h +4
-5@@ -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) --2.2.0.rc0.207.ga3a616c
--
Dmitry
Quoted 7 lines · Alexandre Courbot
Quoted 4 lines · Dmitry Torokhov
Given the intent behind
gpiod_get_optional()and friends it does not make sense to return-ENOSYSwhen GPIOLIB is disabled: the driver is expected to work just fine without gpio so let's behave as if gpio was not found. Otherwise we have to special-case-ENOSYSin drivers.
Quoted 5 lines
Interestingly Uwe sent a RFC for this one week ago:
http://patchwork.ozlabs.org/patch/439135/
Maybe credit him with a Suggested-by.?
I certainly am fine with crediting him with Suggested-by even though I did not see that Uwe's e-mail but this patch was prompted by his other patch changing a few input drivers to use gpiod_get_optional() and me recalling that I explicitly did not use it as it made no difference from gpiod_get() since I had to handle -ENOSYS anyway.
Quoted 6 lines
I should have commented at that time, but let's do it now: I agree with the idea, but this leaves the door open to confusing situations in case gpiolib was unintentionally disabled. Could you also add a note in the documentation of this function to explain this behavior, to spare a few headaches to users of this function?
You mean the inline documentation in drivers/gpio/gpiolib.c and drivers/gpio/devres.c? I can certainly mention there that is GPIOLIB is disabled thy will return NULL unlike the non-optional variants.
Quoted 51 lines
Quoted 51 lines
Signed-off-by Dmitry Torokhov <[email protected]>--- include/linux/gpio/consumer.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)include/linux/gpio/consumer.h +4
-5@@ -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) --2.2.0.rc0.207.ga3a616c
--
Dmitry
Signature · 1 line
Dmitry
Quoted 9 lines · Dmitry Torokhov
Quoted 5 lines
I should have commented at that time, but let's do it now: I agree with the idea, but this leaves the door open to confusing situations in case gpiolib was unintentionally disabled. Could you also add a note in the documentation of this function to explain this behavior, to spare a few headaches to users of this function?
You mean the inline documentation in
drivers/gpio/gpiolib.canddrivers/gpio/devres.c? I can certainly mention there that is GPIOLIB is disabled thy will return NULL unlike the non-optional variants.
I was thinking about Documentation/gpio/consumer.txt, but having a note in the inline documentation as well certainly makes sense.
Hello,
Quoted 12 lines · Dmitry Torokhov
Quoted 11 lines · Alexandre Courbot
Quoted 4 lines · Dmitry Torokhov
Given the intent behind
gpiod_get_optional()and friends it does not make sense to return-ENOSYSwhen GPIOLIB is disabled: the driver is expected to work just fine without gpio so let's behave as if gpio was not found. Otherwise we have to special-case-ENOSYSin drivers.Interestingly Uwe sent a RFC for this one week ago:
Quoted 7 lines
Quoted 1 line
Maybe credit him with a Suggested-by.?
I certainly am fine with crediting him with Suggested-by even though I did not see that Uwe's e-mail but this patch was prompted by his other patch changing a few input drivers to use
gpiod_get_optional()and me recalling that I explicitly did not use it as it made no difference fromgpiod_get()since I had to handle-ENOSYSanyway.
Note that I'm not convinced any more this is a good idea. Consider you have a device tree entry specifying
reset-gpio = <&gpio5 4 0>;
for your device.
With
gpiod_get_optional(dev, "reset", GPIO_OUT_LOW);
the drivers tells that some of the devices it can handle have a reset gpio. If the device in question does have such a gpio the driver must know and do something with it. If the device doesn't have such a gpio that's fine, too.
But if GPIOLIB is off and the device has a reset-gpio specified you certainly want to error out, right?
So IMHO the right thing to do is to return NULL iff there is no reset-gpio specified. Otherwise -ENOSYS is the right thing to return.
Best regards
Uwe
Signature · 6 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html