| Message ID | 20170213011355.GA13075@dtor-ws |
|---|---|
| State | New |
| Headers | show |
[ Forgot to add Uwe to the CC ] On Sun, Feb 12, 2017 at 05:13:55PM -0800, Dmitry Torokhov wrote: > 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. > > Note that there was objection that someone might forget to enable GPIOLIB > when dealing with a platform that has device that actually specifies > optional gpio and we'll break it. I find this unconvincing as that would > have to be the *only GPIO* in the system, which is extremely unlikely. > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > This is a resend of a similar patch from a couple years ago. > > V2: > > - added paragraph to Documentation/gpio/consumer.txt at request of > Alexandre Courbot > - added Suggested-by: Uwe Kleine-König at request of Alexandre Courbot > - hopefully addressed Uwe's concern about gpiolib being disabled in the > patch description: I find it extremely unlikely that the optional GPIO > would happen to be the only GPIO in the whole system. > - added handling for more _optional() calls appeared since V1 was > posted. > > Documentation/gpio/consumer.txt | 6 ++++++ > include/linux/gpio/consumer.h | 12 ++++++------ > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt > index 05676fdacfe3..912568baabb9 100644 > --- a/Documentation/gpio/consumer.txt > +++ b/Documentation/gpio/consumer.txt > @@ -70,6 +70,12 @@ instead of -ENOENT if no GPIO has been assigned to the requested function: > unsigned int index, > enum gpiod_flags flags) > > +Note that gpio_get*_optional() functions (and their managed variants), unlike > +the rest of gpiolib API, also return NULL when gpiolib support is disabled. > +This is helpful to driver authors, since they do not need to special case > +-ENOSYS return codes. System integrators should however be careful to enable > +gpiolib on systems that need it. > + > For a function using multiple GPIOs all of those can be obtained with one call: > > struct gpio_descs *gpiod_get_array(struct device *dev, > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > index fb0fde686cb1..a0431f4aa6cc 100644 > --- a/include/linux/gpio/consumer.h > +++ b/include/linux/gpio/consumer.h > @@ -165,14 +165,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 struct gpio_descs *__must_check > @@ -186,7 +186,7 @@ static inline struct gpio_descs *__must_check > gpiod_get_array_optional(struct device *dev, const char *con_id, > enum gpiod_flags flags) > { > - return ERR_PTR(-ENOSYS); > + return NULL; > } > > static inline void gpiod_put(struct gpio_desc *desc) > @@ -226,14 +226,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 struct gpio_descs *__must_check > @@ -247,7 +247,7 @@ static inline struct gpio_descs *__must_check > devm_gpiod_get_array_optional(struct device *dev, const char *con_id, > enum gpiod_flags flags) > { > - return ERR_PTR(-ENOSYS); > + return NULL; > } > > static inline void devm_gpiod_put(struct device *dev, struct gpio_desc *desc) > -- > 2.11.0.483.g087da7b7c-goog > > > -- > Dmitry
Hello, On Sun, Feb 12, 2017 at 05:15:01PM -0800, Dmitry Torokhov wrote: > On Sun, Feb 12, 2017 at 05:13:55PM -0800, Dmitry Torokhov wrote: > > 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. > > > > Note that there was objection that someone might forget to enable GPIOLIB > > when dealing with a platform that has device that actually specifies > > optional gpio and we'll break it. I find this unconvincing as that would > > have to be the *only GPIO* in the system, which is extremely unlikely. > > > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> I don't like this patch and so I wonder what I wrote that could be interpreted as suggesting this patch. For now I'd say only Nacked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> is justified. My concern is still there. This might break some setups. IMHO it's not ok to request that a device in a certain configuration only works when the (optional) Kconfig option GPIOLIB is enabled and silently breaks if it's not. And you cannot rely on the person who configured the kernel. When accepting this you will burn debug time of others who see their device breaking with no or unrelated error messages. The only reliable way out here is to enable enough of GPIOLIB to only return NULL in ..._optional when there is no gpio required. You can have a suggested-by for that. The semantic of gpiod_get_optional is: if there is a gpio: give it to me else: give me a dummy If the kernel is configured to be unable to answer the question "is there a gpio?" that is worth a -ENOSYS. Best regards Uwe
On Mon, Feb 13, 2017 at 08:45:06AM +0100, Uwe Kleine-König wrote: > Hello, > > On Sun, Feb 12, 2017 at 05:15:01PM -0800, Dmitry Torokhov wrote: > > On Sun, Feb 12, 2017 at 05:13:55PM -0800, Dmitry Torokhov wrote: > > > 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. > > > > > > Note that there was objection that someone might forget to enable GPIOLIB > > > when dealing with a platform that has device that actually specifies > > > optional gpio and we'll break it. I find this unconvincing as that would > > > have to be the *only GPIO* in the system, which is extremely unlikely. > > > > > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > I don't like this patch and so I wonder what I wrote that could be > interpreted as suggesting this patch. For now I'd say only > > Nacked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > is justified. Oh, it seems I really sent such a RFC patch some time ago. Still I think it's wrong to do that and that we need something like a lookup-only-GPIOLIB that implements: def gpio_get_optional(...): if a gpio is specified: return -ENOSYS else: return NULL if you really want save some bytes and disable the full-fledged GPIOLIB. Best regards Uwe
On Mon, Feb 13, 2017 at 08:45:06AM +0100, Uwe Kleine-König wrote: > Hello, > > On Sun, Feb 12, 2017 at 05:15:01PM -0800, Dmitry Torokhov wrote: > > On Sun, Feb 12, 2017 at 05:13:55PM -0800, Dmitry Torokhov wrote: > > > 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. > > > > > > Note that there was objection that someone might forget to enable GPIOLIB > > > when dealing with a platform that has device that actually specifies > > > optional gpio and we'll break it. I find this unconvincing as that would > > > have to be the *only GPIO* in the system, which is extremely unlikely. > > > > > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > I don't like this patch and so I wonder what I wrote that could be > interpreted as suggesting this patch. It came from my exchange with Alexandre: On Fri, Feb 20, 2015 at 01:59:43PM +0900, Alexandre Courbot wrote: > On Fri, Feb 20, 2015 at 9:30 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > 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. > > Interestingly Uwe sent a RFC for this one week ago: > > http://patchwork.ozlabs.org/patch/439135/ > > Maybe credit him with a Suggested-by.? If you check out that patchwork entry you indeed suggested doing exactly what this patch is doing. But if you insist I can certainly remove the "suggested-by". > For now I'd say only > > Nacked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > is justified. > > My concern is still there. This might break some setups. IMHO it's not > ok to request that a device in a certain configuration only works when > the (optional) Kconfig option GPIOLIB is enabled and silently breaks if > it's not. And you cannot rely on the person who configured the kernel. Really? So I guess we can tell people do just do "make randconfig" and start reporting bugs when that kernel would not work on their hardware? > > When accepting this you will burn debug time of others who see their > device breaking with no or unrelated error messages. > > The only reliable way out here is to enable enough of GPIOLIB to only > return NULL in ..._optional when there is no gpio required. You can have > a suggested-by for that. > > The semantic of gpiod_get_optional is: > > if there is a gpio: > give it to me > else: > give me a dummy > > If the kernel is configured to be unable to answer the question "is > there a gpio?" that is worth a -ENOSYS. You know what every single driver for peripherals that are used on variety of systems using has to do for gpios that are truly optional? gpio = gpio_get_optional(); if (IS_ERR(gpio)) { error = PTR_ERR(gpio); if (error == -ENOSYS) { /* * - It's OK, we said it's optional GPIO so * if GPIOLIB is not there we should not fail * to enable the device, because this gpio is * *OPTIONAL*. * - But what if it is actually there? What if the * kernel is misconfigured? * - And what if it is not? How would we know? * - Let's parse device tree by hand! And check * ACPI. And if ACPI is disabled reimplement it * because we should not silently break users! * - ... no, let's not. */ gpio = NULL; } else { return error; } } Really, the argument that there is a *single* GPIO in the whole system, that GPIO happens to be optional, but critical for the deice operation and a random person is confused when enabling random options and forgetting gpiolib? And to "save" this scenario you prefer to fail loading drivers on perfectly configured systems that do not need gpiolib and do not use gpios? Or to add -ENOSYS checks (that actually break the scenario you described and make the driver code ugly)? I think you have your priorities wrong. Thanks.
Hello Dmitry, On Mon, Feb 13, 2017 at 12:20:08AM -0800, Dmitry Torokhov wrote: > On Mon, Feb 13, 2017 at 08:45:06AM +0100, Uwe Kleine-König wrote: > > My concern is still there. This might break some setups. IMHO it's not > > ok to request that a device in a certain configuration only works when > > the (optional) Kconfig option GPIOLIB is enabled and silently breaks if > > it's not. And you cannot rely on the person who configured the kernel. > > Really? So I guess we can tell people do just do "make randconfig" > and start reporting bugs when that kernel would not work on their > hardware? No, but if randconfig enabled a driver for your temperature sensor and that driver manages to try binding to that device, it should fail if the temperature sensor might have a GPIO that must be taken into account for proper functioning if it's there and the driver cannot find out if it's there. If your driver can handle a GPIO that can also safely be ignored, then sure, this function's semantic is a nuisance. But then given that this semantic is what most drivers should use AFAICT the right thing is to introduce another function with the semantic: if there is a gpio and GPIOLIB is enabled: return gpio else: return NULL > > When accepting this you will burn debug time of others who see their > > device breaking with no or unrelated error messages. > > > > The only reliable way out here is to enable enough of GPIOLIB to only > > return NULL in ..._optional when there is no gpio required. You can have > > a suggested-by for that. > > > > The semantic of gpiod_get_optional is: > > > > if there is a gpio: > > give it to me > > else: > > give me a dummy > > > > If the kernel is configured to be unable to answer the question "is > > there a gpio?" that is worth a -ENOSYS. > > You know what every single driver for peripherals that are used on > variety of systems using has to do for gpios that are truly optional? What is the difference between "truly optional" and the meaning of optional in gpiod_get_optional? > gpio = gpio_get_optional(); > if (IS_ERR(gpio)) { > error = PTR_ERR(gpio); > if (error == -ENOSYS) { > /* > * - It's OK, we said it's optional GPIO so > * if GPIOLIB is not there we should not fail > * to enable the device, because this gpio is > * *OPTIONAL*. > * - But what if it is actually there? What if the > * kernel is misconfigured? > * - And what if it is not? How would we know? > * - Let's parse device tree by hand! And check > * ACPI. And if ACPI is disabled reimplement it > * because we should not silently break users! > * - ... no, let's not. > */ > gpio = NULL; > } else { > return error; > } > } I don't know a good name, but implementing a separate helper function with this semantic is great. It simplifies all these drivers, makes them easily greppable for, and lets the drivers that want the reliable semantic just continue to use gpiod_get_optional(). Can you list a few drivers that should make use of this new function instead of gpiod_get_optional()? Or alternatively (if "truly optional" means: I don't need to do anything even if it's available) just do the following instead: /* * There might be a gpio but to not make the driver depend * on GPIOLIB we don't make use of it. */ gpio = NULL; Best regards Uwe
Hi Uwe, On Mon, Feb 13, 2017 at 09:59:35AM +0100, Uwe Kleine-König wrote: > Hello Dmitry, > > On Mon, Feb 13, 2017 at 12:20:08AM -0800, Dmitry Torokhov wrote: > > On Mon, Feb 13, 2017 at 08:45:06AM +0100, Uwe Kleine-König wrote: > > > My concern is still there. This might break some setups. IMHO it's not > > > ok to request that a device in a certain configuration only works when > > > the (optional) Kconfig option GPIOLIB is enabled and silently breaks if > > > it's not. And you cannot rely on the person who configured the kernel. > > > > Really? So I guess we can tell people do just do "make randconfig" > > and start reporting bugs when that kernel would not work on their > > hardware? > > No, but if randconfig enabled a driver for your temperature sensor and > that driver manages to try binding to that device, it should fail if the > temperature sensor might have a GPIO that must be taken into account for > proper functioning if it's there and the driver cannot find out if it's > there. I disagree. The gpio might be there, the regulator might be there, they might be physically present, but not described in DT, ACPI might be disabled, another subsystem might be disabled, kernel parameters might be wrong, additional DT data might be wrong. We are not going to "fix" everything. > > If your driver can handle a GPIO that can also safely be ignored, then > sure, this function's semantic is a nuisance. But then given that this > semantic is what most drivers should use AFAICT the right thing is to > introduce another function with the semantic: > > if there is a gpio and GPIOLIB is enabled: > return gpio > else: > return NULL gpiod_get_optional_I_really_mean_it_scouts_honor()? > > > > When accepting this you will burn debug time of others who see their > > > device breaking with no or unrelated error messages. > > > > > > The only reliable way out here is to enable enough of GPIOLIB to only > > > return NULL in ..._optional when there is no gpio required. You can have > > > a suggested-by for that. > > > > > > The semantic of gpiod_get_optional is: > > > > > > if there is a gpio: > > > give it to me > > > else: > > > give me a dummy > > > > > > If the kernel is configured to be unable to answer the question "is > > > there a gpio?" that is worth a -ENOSYS. > > > > You know what every single driver for peripherals that are used on > > variety of systems using has to do for gpios that are truly optional? > > What is the difference between "truly optional" and the meaning of > optional in gpiod_get_optional? I do not see there is anything different, at least for me. The drivers that believe they need to know about "true" presence of optional GPIOs are free to add "depend GPIOLIB" to their Kconfig entries. > > > gpio = gpio_get_optional(); > > if (IS_ERR(gpio)) { > > error = PTR_ERR(gpio); > > if (error == -ENOSYS) { > > /* > > * - It's OK, we said it's optional GPIO so > > * if GPIOLIB is not there we should not fail > > * to enable the device, because this gpio is > > * *OPTIONAL*. > > * - But what if it is actually there? What if the > > * kernel is misconfigured? > > * - And what if it is not? How would we know? > > * - Let's parse device tree by hand! And check > > * ACPI. And if ACPI is disabled reimplement it > > * because we should not silently break users! > > * - ... no, let's not. > > */ > > gpio = NULL; > > } else { > > return error; > > } > > } > > I don't know a good name, but implementing a separate helper function > with this semantic is great. It simplifies all these drivers, makes them > easily greppable for, and lets the drivers that want the reliable > semantic just continue to use gpiod_get_optional(). Or they can say "depends on GPIOLIB" and get that reliable semantics without introducing new API. > > Can you list a few drivers that should make use of this new function > instead of gpiod_get_optional()? Instead? None. But I consider all of the following: drivers/input/misc/drv260x.c: haptics->enable_gpio = devm_gpiod_get_optional(dev, "enable", drivers/input/touchscreen/cyttsp_core.c: ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); drivers/input/touchscreen/edt-ft5x06.c: tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev, drivers/input/touchscreen/edt-ft5x06.c: tsdata->wake_gpio = devm_gpiod_get_optional(&client->dev, drivers/input/touchscreen/goodix.c: gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN); drivers/input/touchscreen/goodix.c: gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_RST_NAME, GPIOD_IN); drivers/input/touchscreen/melfas_mip4.c: ts->gpio_ce = devm_gpiod_get_optional(&client->dev, drivers/input/touchscreen/pixcir_i2c_ts.c: tsdata->gpio_reset = devm_gpiod_get_optional(dev, "reset", drivers/input/touchscreen/pixcir_i2c_ts.c: tsdata->gpio_wake = devm_gpiod_get_optional(dev, "wake", drivers/input/touchscreen/pixcir_i2c_ts.c: tsdata->gpio_enable = devm_gpiod_get_optional(dev, "enable", drivers/input/touchscreen/raydium_i2c_ts.c: ts->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", drivers/input/touchscreen/silead.c: data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW); drivers/input/touchscreen/sis_i2c.c: ts->attn_gpio = devm_gpiod_get_optional(&client->dev, drivers/input/touchscreen/sis_i2c.c: ts->reset_gpio = devm_gpiod_get_optional(&client->dev, drivers/input/touchscreen/tsc200x-core.c: ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); drivers/input/touchscreen/zforce_ts.c: ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "reset", drivers/input/touchscreen/zforce_ts.c: ts->gpio_int = devm_gpiod_get_optional(&client->dev, "irq", slightly broken right now. Some of them are working around not having to care about -ENOSYS by using "depends on GPIOLIB || COMPILE_TEST" and forcing enabling gpiolib even on systems where there are no gpios exposed to the drivers (let's say there is an ACPI system where all gpios are ACPI owned and not to be touched by the kernel). > > Or alternatively (if "truly optional" means: I don't need to do anything > even if it's available) just do the following instead: > > /* > * There might be a gpio but to not make the driver depend > * on GPIOLIB we don't make use of it. > */ > gpio = NULL; That is not really helpful. OK, I do not think we'll have a meeting of minds here, as I strongly believe that configuring kernel is something that is not done randomly and "randconfig" option is not something that we should try to make working while you advocating for some developer blindly enabling and disabling kernel options and the rest of the kernel developers taking pains as to not inconvenience said developer. So let's leave this to Linus and Alexandre to decide if they believe my (and your older) proposal makes sense or not. Thanks.
On Mon, Feb 13, 2017 at 2:13 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > 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. > > Note that there was objection that someone might forget to enable GPIOLIB > when dealing with a platform that has device that actually specifies > optional gpio and we'll break it. I find this unconvincing as that would > have to be the *only GPIO* in the system, which is extremely unlikely. > > (...) > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > This is a resend of a similar patch from a couple years ago. I want to get an indication from Mark Brown on how he see this semantic compared to how regulators work. It makes most sense to developers to have the same kind of semantics in optional GPIOs as in optional regulators. For optional regulators, if I understand correctly, these are *electrically optional* as in: a voltage input pin on a chip that the chip can very well live without. The regulator may be there, it may not, it may affect the function of the chip but it's still OK without it. For this reason regulators will return an error if an optional regulator is not present, and the code is expected to deal with it. It is a common misconception that the "optional" part of the regulator API call means "software optional". It is not the semantic of this call. It is also tagged __must_check and return an error when compiled out. They return -ENODEV, see <linux/regulator/consumer.h> I would be happy for a patch switching out return value to -ENODEV for sure :) Now second: why should the GPIO semantic be different from what regulators is using? Consistency is important with regards to Rusty Russell's API rules, so we should try to have the same semantic: http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html 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
Hi Linus, On Wed, Feb 22, 2017 at 05:06:35PM +0100, Linus Walleij wrote: > On Mon, Feb 13, 2017 at 2:13 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > 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. > > > > Note that there was objection that someone might forget to enable GPIOLIB > > when dealing with a platform that has device that actually specifies > > optional gpio and we'll break it. I find this unconvincing as that would > > have to be the *only GPIO* in the system, which is extremely unlikely. > > > > (...) > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > > > This is a resend of a similar patch from a couple years ago. > > I want to get an indication from Mark Brown on how he see this > semantic compared to how regulators work. This is completely orthogonal to this patch though. This patch tries to solve problem that API behaves differently when GPIOLIB is disabled, so the driver has to check for both NULL and ENOSYS. I would like to make gpiod_get_optional() return NULL when GPIOLIB is disabled. > > It makes most sense to developers to have the same kind of > semantics in optional GPIOs as in optional regulators. I agree, here. Unfortunately optional regulators return -ENOENT and it will take an effort to change it to NULL if we can persuade Mark that it makes sense. However, there are more optional GPIOs than regulators, so switching gpio to return -ENODEV even bigger task. Besides, I believe that doing: gpio = gpiod_get_optional(); if (IS_ERR(gpio)) return PTR_ERR(gpio); is better than: gpio = gpiod_get_optional(); if ((IS_ERR(gpio)) { error = PTR_ERR(gpio); if (error != -ENODEV) return error; gpio = NULL; } > > For optional regulators, if I understand correctly, these are > *electrically optional* as in: a voltage input pin on a chip that > the chip can very well live without. The regulator may be there, > it may not, it may affect the function of the chip but it's still OK > without it. > > For this reason regulators will return an error if an optional regulator > is not present, and the code is expected to deal with it. > > It is a common misconception that the "optional" part of the > regulator API call means "software optional". It is not the semantic > of this call. > > It is also tagged __must_check and return an error when compiled > out. They return -ENODEV, see <linux/regulator/consumer.h> The point is that they return the same value when regulator is not present and when regulator support is compiled out. GPIOLIB returns NULL and -ENOSYS respectively, and I contend that this is wrong and needs to be resolved first. > > I would be happy for a patch switching out return value to -ENODEV > for sure :) I as shown is the example above I think NULL is better for optionals. > > Now second: why should the GPIO semantic be different from what > regulators is using? > > Consistency is important with regards to Rusty Russell's API > rules, so we should try to have the same semantic: > http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html I agree that if we can converge on the similar behavior it would be best, but it is bigger (and separate) task. First I think we need to make API behave sane when support is enabled and when it is disabled. Thanks.
On Wed, Feb 22, 2017 at 05:06:35PM +0100, Linus Walleij wrote: > For optional regulators, if I understand correctly, these are > *electrically optional* as in: a voltage input pin on a chip that > the chip can very well live without. The regulator may be there, > it may not, it may affect the function of the chip but it's still OK > without it. > For this reason regulators will return an error if an optional regulator > is not present, and the code is expected to deal with it. Yes. > It is a common misconception that the "optional" part of the > regulator API call means "software optional". It is not the semantic > of this call. > It is also tagged __must_check and return an error when compiled > out. They return -ENODEV, see <linux/regulator/consumer.h> Right. Dmitry actually sent a patch a few weeks ago proposing a change in this behaviour for the regulator API which I didn't apply. I'm mainly worried that this would the already encourage common broken patterns with people just not writing error handling code properly and the incorrect software optional assumption you mention above. The expectation is that the majority of drivers with optional regulators will need to do some explicit handling whenever they would interact with the regulator.
On Wed, Feb 22, 2017 at 10:27:25AM -0800, Dmitry Torokhov wrote: > The point is that they return the same value when regulator is not > present and when regulator support is compiled out. GPIOLIB returns NULL > and -ENOSYS respectively, and I contend that this is wrong and needs to > be resolved first. FWIW this makes sense to me.
On Mon, Feb 13, 2017 at 2:13 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > 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. > > Note that there was objection that someone might forget to enable GPIOLIB > when dealing with a platform that has device that actually specifies > optional gpio and we'll break it. I find this unconvincing as that would > have to be the *only GPIO* in the system, which is extremely unlikely. > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > This is a resend of a similar patch from a couple years ago. > > V2: After a lot of decision angst I applied this patch, dropping Uwe's Suggested-by since he doesn't seem to like it. I think we should eventually return proper error codes, but as you say: the API should be consistent first, we need to change both the lib and the stubs to return error codes for that, this is about something else, just keeping it consistent. 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
diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt index 05676fdacfe3..912568baabb9 100644 --- a/Documentation/gpio/consumer.txt +++ b/Documentation/gpio/consumer.txt @@ -70,6 +70,12 @@ instead of -ENOENT if no GPIO has been assigned to the requested function: unsigned int index, enum gpiod_flags flags) +Note that gpio_get*_optional() functions (and their managed variants), unlike +the rest of gpiolib API, also return NULL when gpiolib support is disabled. +This is helpful to driver authors, since they do not need to special case +-ENOSYS return codes. System integrators should however be careful to enable +gpiolib on systems that need it. + For a function using multiple GPIOs all of those can be obtained with one call: struct gpio_descs *gpiod_get_array(struct device *dev, diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index fb0fde686cb1..a0431f4aa6cc 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -165,14 +165,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 struct gpio_descs *__must_check @@ -186,7 +186,7 @@ static inline struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev, const char *con_id, enum gpiod_flags flags) { - return ERR_PTR(-ENOSYS); + return NULL; } static inline void gpiod_put(struct gpio_desc *desc) @@ -226,14 +226,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 struct gpio_descs *__must_check @@ -247,7 +247,7 @@ static inline struct gpio_descs *__must_check devm_gpiod_get_array_optional(struct device *dev, const char *con_id, enum gpiod_flags flags) { - return ERR_PTR(-ENOSYS); + return NULL; } static inline void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
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. Note that there was objection that someone might forget to enable GPIOLIB when dealing with a platform that has device that actually specifies optional gpio and we'll break it. I find this unconvincing as that would have to be the *only GPIO* in the system, which is extremely unlikely. Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- This is a resend of a similar patch from a couple years ago. V2: - added paragraph to Documentation/gpio/consumer.txt at request of Alexandre Courbot - added Suggested-by: Uwe Kleine-König at request of Alexandre Courbot - hopefully addressed Uwe's concern about gpiolib being disabled in the patch description: I find it extremely unlikely that the optional GPIO would happen to be the only GPIO in the whole system. - added handling for more _optional() calls appeared since V1 was posted. Documentation/gpio/consumer.txt | 6 ++++++ include/linux/gpio/consumer.h | 12 ++++++------ 2 files changed, 12 insertions(+), 6 deletions(-)