* [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid
@ 2019-08-21 17:17 Andy Shevchenko
2019-08-21 17:17 ` [PATCH v1 2/2] leds: trigger: gpio: Convert to use kstrtox() Andy Shevchenko
2019-08-24 16:47 ` [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid Jacek Anaszewski
0 siblings, 2 replies; 7+ messages in thread
From: Andy Shevchenko @ 2019-08-21 17:17 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek, Dan Murphy, linux-leds; +Cc: Andy Shevchenko
Allow all valid GPIOs to be used in the driver.
Fixes: 17354bfe8527 ("leds: Add gpio-led trigger")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/leds/trigger/ledtrig-gpio.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index 33cc99a1a16a..31f456dd4417 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -131,10 +131,10 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
if (gpio_data->gpio == gpio)
return n;
- if (!gpio) {
- if (gpio_data->gpio != 0)
+ if (!gpio_is_valid(gpio)) {
+ if (gpio_is_valid(gpio_data->gpio))
free_irq(gpio_to_irq(gpio_data->gpio), led);
- gpio_data->gpio = 0;
+ gpio_data->gpio = gpio;
return n;
}
@@ -144,7 +144,7 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
if (ret) {
dev_err(dev, "request_irq failed with error %d\n", ret);
} else {
- if (gpio_data->gpio != 0)
+ if (gpio_is_valid(gpio_data->gpio))
free_irq(gpio_to_irq(gpio_data->gpio), led);
gpio_data->gpio = gpio;
/* After changing the GPIO, we need to update the LED. */
@@ -181,7 +181,7 @@ static void gpio_trig_deactivate(struct led_classdev *led)
{
struct gpio_trig_data *gpio_data = led_get_trigger_data(led);
- if (gpio_data->gpio != 0)
+ if (gpio_is_valid(gpio_data->gpio))
free_irq(gpio_to_irq(gpio_data->gpio), led);
kfree(gpio_data);
}
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v1 2/2] leds: trigger: gpio: Convert to use kstrtox() 2019-08-21 17:17 [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid Andy Shevchenko @ 2019-08-21 17:17 ` Andy Shevchenko 2019-08-24 16:51 ` Jacek Anaszewski 2019-08-24 16:47 ` [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid Jacek Anaszewski 1 sibling, 1 reply; 7+ messages in thread From: Andy Shevchenko @ 2019-08-21 17:17 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek, Dan Murphy, linux-leds; +Cc: Andy Shevchenko sscanf() is a heavy one and moreover requires additional boundary checks. Convert driver to use kstrtox() and replace kstrtoul() by kstrtobool() in gpio_trig_inverted_store() Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/leds/trigger/ledtrig-gpio.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c index 31f456dd4417..b01862b94c99 100644 --- a/drivers/leds/trigger/ledtrig-gpio.c +++ b/drivers/leds/trigger/ledtrig-gpio.c @@ -57,13 +57,13 @@ static ssize_t gpio_trig_brightness_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t n) { struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev); - unsigned desired_brightness; + u8 desired_brightness; int ret; - ret = sscanf(buf, "%u", &desired_brightness); - if (ret < 1 || desired_brightness > 255) { + ret = kstrtou8(buf, 10, &desired_brightness); + if (ret) { dev_err(dev, "invalid value\n"); - return -EINVAL; + return ret; } gpio_data->desired_brightness = desired_brightness; @@ -86,16 +86,13 @@ static ssize_t gpio_trig_inverted_store(struct device *dev, { struct led_classdev *led = led_trigger_get_led(dev); struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev); - unsigned long inverted; + bool inverted; int ret; - ret = kstrtoul(buf, 10, &inverted); - if (ret < 0) + ret = kstrtobool(buf, &inverted); + if (ret) return ret; - if (inverted > 1) - return -EINVAL; - gpio_data->inverted = inverted; /* After inverting, we need to update the LED. */ @@ -122,10 +119,10 @@ static ssize_t gpio_trig_gpio_store(struct device *dev, unsigned gpio; int ret; - ret = sscanf(buf, "%u", &gpio); - if (ret < 1) { + ret = kstrtouint(buf, 10, &gpio); + if (ret) { dev_err(dev, "couldn't read gpio number\n"); - return -EINVAL; + return ret; } if (gpio_data->gpio == gpio) -- 2.23.0.rc1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] leds: trigger: gpio: Convert to use kstrtox() 2019-08-21 17:17 ` [PATCH v1 2/2] leds: trigger: gpio: Convert to use kstrtox() Andy Shevchenko @ 2019-08-24 16:51 ` Jacek Anaszewski 0 siblings, 0 replies; 7+ messages in thread From: Jacek Anaszewski @ 2019-08-24 16:51 UTC (permalink / raw) To: Andy Shevchenko, Pavel Machek, Dan Murphy, linux-leds Hi Andy, Thank you for the patch. On 8/21/19 7:17 PM, Andy Shevchenko wrote: > sscanf() is a heavy one and moreover requires additional boundary checks. > Convert driver to use kstrtox() and replace kstrtoul() by kstrtobool() in > gpio_trig_inverted_store() > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/leds/trigger/ledtrig-gpio.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c > index 31f456dd4417..b01862b94c99 100644 > --- a/drivers/leds/trigger/ledtrig-gpio.c > +++ b/drivers/leds/trigger/ledtrig-gpio.c > @@ -57,13 +57,13 @@ static ssize_t gpio_trig_brightness_store(struct device *dev, > struct device_attribute *attr, const char *buf, size_t n) > { > struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev); > - unsigned desired_brightness; > + u8 desired_brightness; > int ret; > > - ret = sscanf(buf, "%u", &desired_brightness); > - if (ret < 1 || desired_brightness > 255) { LED_FULL (255) is already not a hard limit for quite a long time. While we are at it we could change that to led->max_brightness. > + ret = kstrtou8(buf, 10, &desired_brightness); > + if (ret) { > dev_err(dev, "invalid value\n"); > - return -EINVAL; > + return ret; > } > > gpio_data->desired_brightness = desired_brightness; > @@ -86,16 +86,13 @@ static ssize_t gpio_trig_inverted_store(struct device *dev, > { > struct led_classdev *led = led_trigger_get_led(dev); > struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev); > - unsigned long inverted; > + bool inverted; > int ret; > > - ret = kstrtoul(buf, 10, &inverted); > - if (ret < 0) > + ret = kstrtobool(buf, &inverted); > + if (ret) > return ret; > > - if (inverted > 1) > - return -EINVAL; > - > gpio_data->inverted = inverted; > > /* After inverting, we need to update the LED. */ > @@ -122,10 +119,10 @@ static ssize_t gpio_trig_gpio_store(struct device *dev, > unsigned gpio; > int ret; > > - ret = sscanf(buf, "%u", &gpio); > - if (ret < 1) { > + ret = kstrtouint(buf, 10, &gpio); > + if (ret) { > dev_err(dev, "couldn't read gpio number\n"); > - return -EINVAL; > + return ret; > } > > if (gpio_data->gpio == gpio) > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid 2019-08-21 17:17 [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid Andy Shevchenko 2019-08-21 17:17 ` [PATCH v1 2/2] leds: trigger: gpio: Convert to use kstrtox() Andy Shevchenko @ 2019-08-24 16:47 ` Jacek Anaszewski 2019-08-26 9:57 ` Andy Shevchenko 1 sibling, 1 reply; 7+ messages in thread From: Jacek Anaszewski @ 2019-08-24 16:47 UTC (permalink / raw) To: Andy Shevchenko, Pavel Machek, Dan Murphy, linux-leds Hi Andy, Thank you for the patch. On 8/21/19 7:17 PM, Andy Shevchenko wrote: > Allow all valid GPIOs to be used in the driver. > > Fixes: 17354bfe8527 ("leds: Add gpio-led trigger") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/leds/trigger/ledtrig-gpio.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c > index 33cc99a1a16a..31f456dd4417 100644 > --- a/drivers/leds/trigger/ledtrig-gpio.c > +++ b/drivers/leds/trigger/ledtrig-gpio.c > @@ -131,10 +131,10 @@ static ssize_t gpio_trig_gpio_store(struct device *dev, > if (gpio_data->gpio == gpio) > return n; > > - if (!gpio) { > - if (gpio_data->gpio != 0) > + if (!gpio_is_valid(gpio)) { > + if (gpio_is_valid(gpio_data->gpio)) > free_irq(gpio_to_irq(gpio_data->gpio), led); > - gpio_data->gpio = 0; > + gpio_data->gpio = gpio; It looks odd to me. I'd just assign invalid constant gpio number e.g. -1. Note that we should also do that in gpio_trig_activate(), where gpio_data->gpio is initialized to 0 by kzalloc(). That later can have nasty side effect in gpio_trig_gpio_store() when gpio to set is 0. Then the condition "if (gpio_data->gpio == gpio)" will evaluate to true and gpio_trig_irq() handler will not be registered. > return n; > } > > @@ -144,7 +144,7 @@ static ssize_t gpio_trig_gpio_store(struct device *dev, > if (ret) { > dev_err(dev, "request_irq failed with error %d\n", ret); > } else { > - if (gpio_data->gpio != 0) > + if (gpio_is_valid(gpio_data->gpio)) > free_irq(gpio_to_irq(gpio_data->gpio), led); > gpio_data->gpio = gpio; > /* After changing the GPIO, we need to update the LED. */ > @@ -181,7 +181,7 @@ static void gpio_trig_deactivate(struct led_classdev *led) > { > struct gpio_trig_data *gpio_data = led_get_trigger_data(led); > > - if (gpio_data->gpio != 0) > + if (gpio_is_valid(gpio_data->gpio)) > free_irq(gpio_to_irq(gpio_data->gpio), led); > kfree(gpio_data); > } > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid 2019-08-24 16:47 ` [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid Jacek Anaszewski @ 2019-08-26 9:57 ` Andy Shevchenko 2019-08-26 18:36 ` Jacek Anaszewski 0 siblings, 1 reply; 7+ messages in thread From: Andy Shevchenko @ 2019-08-26 9:57 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: Pavel Machek, Dan Murphy, linux-leds On Sat, Aug 24, 2019 at 06:47:54PM +0200, Jacek Anaszewski wrote: > On 8/21/19 7:17 PM, Andy Shevchenko wrote: > > Allow all valid GPIOs to be used in the driver. > > > > Fixes: 17354bfe8527 ("leds: Add gpio-led trigger") > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > - if (!gpio) { > > - if (gpio_data->gpio != 0) > > + if (!gpio_is_valid(gpio)) { > > + if (gpio_is_valid(gpio_data->gpio)) > > free_irq(gpio_to_irq(gpio_data->gpio), led); > > - gpio_data->gpio = 0; > > + gpio_data->gpio = gpio; > > It looks odd to me. I'd just assign invalid constant gpio number > e.g. -1. Current ABI (unsigned) doesn't allow us to do this. Internally we can redefine invalid GPIO line number to -1 or so, but does it worth it? And actually I would prefer -EINVAL or -ENOENT in such cases. > Note that we should also do that in gpio_trig_activate(), where > gpio_data->gpio is initialized to 0 by kzalloc(). That later can > have nasty side effect in gpio_trig_gpio_store() when gpio to set > is 0. Then the condition "if (gpio_data->gpio == gpio)" will evaluate > to true and gpio_trig_irq() handler will not be registered. Thanks for spotting this! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid 2019-08-26 9:57 ` Andy Shevchenko @ 2019-08-26 18:36 ` Jacek Anaszewski 2019-08-30 15:01 ` Andy Shevchenko 0 siblings, 1 reply; 7+ messages in thread From: Jacek Anaszewski @ 2019-08-26 18:36 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Pavel Machek, Dan Murphy, linux-leds On 8/26/19 11:57 AM, Andy Shevchenko wrote: > On Sat, Aug 24, 2019 at 06:47:54PM +0200, Jacek Anaszewski wrote: >> On 8/21/19 7:17 PM, Andy Shevchenko wrote: >>> Allow all valid GPIOs to be used in the driver. >>> >>> Fixes: 17354bfe8527 ("leds: Add gpio-led trigger") >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >>> - if (!gpio) { >>> - if (gpio_data->gpio != 0) >>> + if (!gpio_is_valid(gpio)) { >>> + if (gpio_is_valid(gpio_data->gpio)) >>> free_irq(gpio_to_irq(gpio_data->gpio), led); >>> - gpio_data->gpio = 0; >>> + gpio_data->gpio = gpio; >> >> It looks odd to me. I'd just assign invalid constant gpio number >> e.g. -1. > > Current ABI (unsigned) doesn't allow us to do this. Internally we can redefine Ah, right, missed that. > invalid GPIO line number to -1 or so, but does it worth it? And actually I > would prefer -EINVAL or -ENOENT in such cases. OK, we can keep your "= gpio" assignment in view of the above, but need to return error instead of "n". >> Note that we should also do that in gpio_trig_activate(), where >> gpio_data->gpio is initialized to 0 by kzalloc(). That later can >> have nasty side effect in gpio_trig_gpio_store() when gpio to set >> is 0. Then the condition "if (gpio_data->gpio == gpio)" will evaluate >> to true and gpio_trig_irq() handler will not be registered. > > Thanks for spotting this! > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid 2019-08-26 18:36 ` Jacek Anaszewski @ 2019-08-30 15:01 ` Andy Shevchenko 0 siblings, 0 replies; 7+ messages in thread From: Andy Shevchenko @ 2019-08-30 15:01 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: Pavel Machek, Dan Murphy, linux-leds On Mon, Aug 26, 2019 at 08:36:04PM +0200, Jacek Anaszewski wrote: > On 8/26/19 11:57 AM, Andy Shevchenko wrote: > > On Sat, Aug 24, 2019 at 06:47:54PM +0200, Jacek Anaszewski wrote: > >> On 8/21/19 7:17 PM, Andy Shevchenko wrote: > >>> Allow all valid GPIOs to be used in the driver. > >>> > >>> Fixes: 17354bfe8527 ("leds: Add gpio-led trigger") > >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > >>> - if (!gpio) { > >>> - if (gpio_data->gpio != 0) > >>> + if (!gpio_is_valid(gpio)) { > >>> + if (gpio_is_valid(gpio_data->gpio)) > >>> free_irq(gpio_to_irq(gpio_data->gpio), led); > >>> - gpio_data->gpio = 0; > >>> + gpio_data->gpio = gpio; > >> > >> It looks odd to me. I'd just assign invalid constant gpio number > >> e.g. -1. > > > > Current ABI (unsigned) doesn't allow us to do this. Internally we can redefine > > Ah, right, missed that. > > > invalid GPIO line number to -1 or so, but does it worth it? And actually I > > would prefer -EINVAL or -ENOENT in such cases. > > OK, we can keep your "= gpio" assignment in view of the above, but need > to return error instead of "n". Then we will break an ABI, where user expects no error it suddenly will be one. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-30 15:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-21 17:17 [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid Andy Shevchenko 2019-08-21 17:17 ` [PATCH v1 2/2] leds: trigger: gpio: Convert to use kstrtox() Andy Shevchenko 2019-08-24 16:51 ` Jacek Anaszewski 2019-08-24 16:47 ` [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid Jacek Anaszewski 2019-08-26 9:57 ` Andy Shevchenko 2019-08-26 18:36 ` Jacek Anaszewski 2019-08-30 15:01 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).