| Message ID | 1438048204-632-1-git-send-email-bjorn.andersson@sonymobile.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Bjorn, On Mon, Jul 27, 2015 at 06:50:04PM -0700, Bjorn Andersson wrote: > In the cases where the gpio chip fails to acquire the current state an > error is reported back to gpio_keys. This is currently interpreted as if > the line went high, which just confuses the developer. > > This patch introduces an error print in this case and skipps the > reporting of a input event; to aid in debugging this issue. > > Reported-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > --- > drivers/input/keyboard/gpio_keys.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c > index ddf4045de084..3ce3298ac09e 100644 > --- a/drivers/input/keyboard/gpio_keys.c > +++ b/drivers/input/keyboard/gpio_keys.c > @@ -336,8 +336,14 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata) > const struct gpio_keys_button *button = bdata->button; > struct input_dev *input = bdata->input; > unsigned int type = button->type ?: EV_KEY; > - int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0) ^ button->active_low; > + int state = gpio_get_value_cansleep(button->gpio); > > + if (state < 0) { > + dev_err(input->dev.parent, "failed to get gpio state\n"); As far as I can see: static inline int gpio_get_value_cansleep(unsigned gpio) { return gpiod_get_raw_value_cansleep(gpio_to_desc(gpio)); } int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc) { might_sleep_if(extra_checks); if (!desc) return 0; return _gpiod_get_raw_value(desc); } static bool _gpiod_get_raw_value(const struct gpio_desc *desc) { ... } So how exactly do we get negative here? Thanks.
On Tue 28 Jul 14:00 PDT 2015, Dmitry Torokhov wrote: > Hi Bjorn, > > On Mon, Jul 27, 2015 at 06:50:04PM -0700, Bjorn Andersson wrote: > > In the cases where the gpio chip fails to acquire the current state an > > error is reported back to gpio_keys. This is currently interpreted as if > > the line went high, which just confuses the developer. > > > > This patch introduces an error print in this case and skipps the > > reporting of a input event; to aid in debugging this issue. > > > > Reported-by: John Stultz <john.stultz@linaro.org> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > > --- > > drivers/input/keyboard/gpio_keys.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c > > index ddf4045de084..3ce3298ac09e 100644 > > --- a/drivers/input/keyboard/gpio_keys.c > > +++ b/drivers/input/keyboard/gpio_keys.c > > @@ -336,8 +336,14 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata) > > const struct gpio_keys_button *button = bdata->button; > > struct input_dev *input = bdata->input; > > unsigned int type = button->type ?: EV_KEY; > > - int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0) ^ button->active_low; > > + int state = gpio_get_value_cansleep(button->gpio); > > > > + if (state < 0) { > > + dev_err(input->dev.parent, "failed to get gpio state\n"); > > As far as I can see: > > static inline int gpio_get_value_cansleep(unsigned gpio) > { > return gpiod_get_raw_value_cansleep(gpio_to_desc(gpio)); > } > > int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc) > { > might_sleep_if(extra_checks); > if (!desc) > return 0; > return _gpiod_get_raw_value(desc); > } > > static bool _gpiod_get_raw_value(const struct gpio_desc *desc) > { > ... > } > > So how exactly do we get negative here? I'm sorry, I obviously didn't pay enough attention when running through that callstack... But then the question first goes to Linus & co. gpio_chip->get() can return a negative value to indicate errors (and did so in this case), all parts of the API seems indicates that we can get an error (int vs bool). Should we change _gpiod_get_raw_value() to propagate this error? Or should we just ignore this issue and propagate an error as GPIO high reading? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote: > But then the question first goes to Linus & co. > > gpio_chip->get() can return a negative value to indicate errors (and did > so in this case), all parts of the API seems indicates that we can get > an error (int vs bool). Ooops. > Should we change _gpiod_get_raw_value() to propagate this error? Yes for now. Can you patch it? :) > Or > should we just ignore this issue and propagate an error as GPIO high > reading? I don't know about the future. In some sense GPIOs are so smallish resources that errorhandling every call to read/write them seem to be a royal PITA. That is why I wanted to switch them to bool and get rid of the problem, but now I also see that maybe that was not such a smart idea, if errors do occur on the set/get_value path. Alexandre? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson > <bjorn.andersson@sonymobile.com> wrote: > >> But then the question first goes to Linus & co. >> >> gpio_chip->get() can return a negative value to indicate errors (and did >> so in this case), all parts of the API seems indicates that we can get >> an error (int vs bool). > > Ooops. > >> Should we change _gpiod_get_raw_value() to propagate this error? > > Yes for now. Can you patch it? :) > >> Or >> should we just ignore this issue and propagate an error as GPIO high >> reading? > > I don't know about the future. In some sense GPIOs are so smallish > resources that errorhandling every call to read/write them seem to > be a royal PITA. That is why I wanted to switch them to bool and get > rid of the problem, but now I also see that maybe that was not such a > smart idea, if errors do occur on the set/get_value path. Nowadays GPIOs may reside at the other end of an i2c bus, which means that even the simplest operation like reading a GPIO value can potentially fail. And it will probably not get better - wait until we implement GPIO-over-IP! :) So I'd say it makes sense to propagate errors returned by the driver's get() hook. This might contradict some of our earlier statements about simplifying the GPIO API, but is preferrable to having to make a decision as to which valid value to return if the driver fails... It should then be made very clear in the documentation that the only positive values ever returned by the GPIO API will be 0 and 1 (we already have a clamping mechanism for that IIRC), and that negative values are propagated as-is. Linus, does that seem reasonable to you? Does anyone has the intention to address that one or should I add it to my short-term TODO list? -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun 16 Aug 23:59 PDT 2015, Alexandre Courbot wrote: > On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij > <linus.walleij@linaro.org> wrote: > > On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson > > <bjorn.andersson@sonymobile.com> wrote: > > > >> But then the question first goes to Linus & co. > >> > >> gpio_chip->get() can return a negative value to indicate errors (and did > >> so in this case), all parts of the API seems indicates that we can get > >> an error (int vs bool). > > > > Ooops. > > > >> Should we change _gpiod_get_raw_value() to propagate this error? > > > > Yes for now. Can you patch it? :) > > > >> Or > >> should we just ignore this issue and propagate an error as GPIO high > >> reading? > > > > I don't know about the future. In some sense GPIOs are so smallish > > resources that errorhandling every call to read/write them seem to > > be a royal PITA. That is why I wanted to switch them to bool and get > > rid of the problem, but now I also see that maybe that was not such a > > smart idea, if errors do occur on the set/get_value path. > > Nowadays GPIOs may reside at the other end of an i2c bus, which means > that even the simplest operation like reading a GPIO value can > potentially fail. And it will probably not get better - wait until we > implement GPIO-over-IP! :) > Now that's progress! I can't wait ;) > So I'd say it makes sense to propagate errors returned by the driver's > get() hook. This might contradict some of our earlier statements about > simplifying the GPIO API, but is preferrable to having to make a > decision as to which valid value to return if the driver fails... > Sounds good. As we're patching up _gpiod_get_raw_value(), is the lack of a get() implementation the same as a LOW or is that -ENOTSUPP? > It should then be made very clear in the documentation that the only > positive values ever returned by the GPIO API will be 0 and 1 (we > already have a clamping mechanism for that IIRC), and that negative > values are propagated as-is. > That makes sense. I'm however not able to find such clamping macro/mechanism and it would be very beneficial here... > Linus, does that seem reasonable to you? Does anyone has the intention > to address that one or should I add it to my short-term TODO list? If you have some input on above (is lack of get() an error) I can hack up the patch. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 17, 2015 at 8:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > So I'd say it makes sense to propagate errors returned by the driver's > get() hook. This might contradict some of our earlier statements about > simplifying the GPIO API, but is preferrable to having to make a > decision as to which valid value to return if the driver fails... > > It should then be made very clear in the documentation that the only > positive values ever returned by the GPIO API will be 0 and 1 (we > already have a clamping mechanism for that IIRC), and that negative > values are propagated as-is. > > Linus, does that seem reasonable to you? Does anyone has the intention > to address that one or should I add it to my short-term TODO list? I'm aligned with this. Go ahead on this path. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 18, 2015 at 4:34 AM, Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote: > On Sun 16 Aug 23:59 PDT 2015, Alexandre Courbot wrote: > >> On Thu, Aug 13, 2015 at 10:06 PM, Linus Walleij >> <linus.walleij@linaro.org> wrote: >> > On Tue, Aug 11, 2015 at 12:41 AM, Bjorn Andersson >> > <bjorn.andersson@sonymobile.com> wrote: >> > >> >> But then the question first goes to Linus & co. >> >> >> >> gpio_chip->get() can return a negative value to indicate errors (and did >> >> so in this case), all parts of the API seems indicates that we can get >> >> an error (int vs bool). >> > >> > Ooops. >> > >> >> Should we change _gpiod_get_raw_value() to propagate this error? >> > >> > Yes for now. Can you patch it? :) >> > >> >> Or >> >> should we just ignore this issue and propagate an error as GPIO high >> >> reading? >> > >> > I don't know about the future. In some sense GPIOs are so smallish >> > resources that errorhandling every call to read/write them seem to >> > be a royal PITA. That is why I wanted to switch them to bool and get >> > rid of the problem, but now I also see that maybe that was not such a >> > smart idea, if errors do occur on the set/get_value path. >> >> Nowadays GPIOs may reside at the other end of an i2c bus, which means >> that even the simplest operation like reading a GPIO value can >> potentially fail. And it will probably not get better - wait until we >> implement GPIO-over-IP! :) >> > > Now that's progress! I can't wait ;) > >> So I'd say it makes sense to propagate errors returned by the driver's >> get() hook. This might contradict some of our earlier statements about >> simplifying the GPIO API, but is preferrable to having to make a >> decision as to which valid value to return if the driver fails... >> > > Sounds good. > > As we're patching up _gpiod_get_raw_value(), is the lack of a get() > implementation the same as a LOW or is that -ENOTSUPP? I don't see any reason why it should not be -ENOTSUPP if we start to manage errors properly. > >> It should then be made very clear in the documentation that the only >> positive values ever returned by the GPIO API will be 0 and 1 (we >> already have a clamping mechanism for that IIRC), and that negative >> values are propagated as-is. >> > > That makes sense. I'm however not able to find such clamping > macro/mechanism and it would be very beneficial here... > >> Linus, does that seem reasonable to you? Does anyone has the intention >> to address that one or should I add it to my short-term TODO list? > > If you have some input on above (is lack of get() an error) I can hack > up the patch. Excellent - since Linus gave his thumb up, I think you can go ahead. Looking forward to seeing this finally fixed. Thanks! Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 28 Jul 14:00 PDT 2015, Dmitry Torokhov wrote: > Hi Bjorn, > > On Mon, Jul 27, 2015 at 06:50:04PM -0700, Bjorn Andersson wrote: > > In the cases where the gpio chip fails to acquire the current state an > > error is reported back to gpio_keys. This is currently interpreted as if > > the line went high, which just confuses the developer. > > > > This patch introduces an error print in this case and skipps the > > reporting of a input event; to aid in debugging this issue. > > > > Reported-by: John Stultz <john.stultz@linaro.org> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > > --- > > drivers/input/keyboard/gpio_keys.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c > > index ddf4045de084..3ce3298ac09e 100644 > > --- a/drivers/input/keyboard/gpio_keys.c > > +++ b/drivers/input/keyboard/gpio_keys.c > > @@ -336,8 +336,14 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata) > > const struct gpio_keys_button *button = bdata->button; > > struct input_dev *input = bdata->input; > > unsigned int type = button->type ?: EV_KEY; > > - int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0) ^ button->active_low; > > + int state = gpio_get_value_cansleep(button->gpio); > > > > + if (state < 0) { > > + dev_err(input->dev.parent, "failed to get gpio state\n"); > [..] > > So how exactly do we get negative here? > Linus merged my change in gpiolib to propagate the error value, so as of v4.3-rc2 this behaviour is now changed. So please have a look at this patch again. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 27, 2015 at 6:50 PM, Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote: > In the cases where the gpio chip fails to acquire the current state an > error is reported back to gpio_keys. This is currently interpreted as if > the line went high, which just confuses the developer. > > This patch introduces an error print in this case and skipps the > reporting of a input event; to aid in debugging this issue. > > Reported-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 02, 2015 at 03:34:45AM -0700, Linus Walleij wrote: > On Mon, Jul 27, 2015 at 6:50 PM, Bjorn Andersson > <bjorn.andersson@sonymobile.com> wrote: > > > In the cases where the gpio chip fails to acquire the current state an > > error is reported back to gpio_keys. This is currently interpreted as if > > the line went high, which just confuses the developer. > > > > This patch introduces an error print in this case and skipps the > > reporting of a input event; to aid in debugging this issue. > > > > Reported-by: John Stultz <john.stultz@linaro.org> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Applied, thank you.
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index ddf4045de084..3ce3298ac09e 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -336,8 +336,14 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata) const struct gpio_keys_button *button = bdata->button; struct input_dev *input = bdata->input; unsigned int type = button->type ?: EV_KEY; - int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0) ^ button->active_low; + int state = gpio_get_value_cansleep(button->gpio); + if (state < 0) { + dev_err(input->dev.parent, "failed to get gpio state\n"); + return; + } + + state = (state ? 1 : 0) ^ button->active_low; if (type == EV_ABS) { if (state) input_event(input, type, button->code, button->value);
In the cases where the gpio chip fails to acquire the current state an error is reported back to gpio_keys. This is currently interpreted as if the line went high, which just confuses the developer. This patch introduces an error print in this case and skipps the reporting of a input event; to aid in debugging this issue. Reported-by: John Stultz <john.stultz@linaro.org> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- drivers/input/keyboard/gpio_keys.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)