Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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	[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 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-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, back to index

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

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org linux-leds@archiver.kernel.org
	public-inbox-index linux-leds


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/ public-inbox