Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid
@ 2019-08-30 15:08 Andy Shevchenko
  2019-08-30 15:08 ` [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox() Andy Shevchenko
  2019-08-30 21:11 ` [PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid Jacek Anaszewski
  0 siblings, 2 replies; 7+ messages in thread
From: Andy Shevchenko @ 2019-08-30 15:08 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>
---
- set initial GPIO value to -ENOENT (Jacek)
 drivers/leds/trigger/ledtrig-gpio.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index 33cc99a1a16a..dc64679b1a92 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. */
@@ -172,6 +172,8 @@ static int gpio_trig_activate(struct led_classdev *led)
 		return -ENOMEM;
 
 	gpio_data->led = led;
+	gpio_data->gpio = -ENOENT;
+
 	led_set_trigger_data(led, gpio_data);
 
 	return 0;
@@ -181,7 +183,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 v2 2/2] leds: trigger: gpio: Convert to use kstrtox()
  2019-08-30 15:08 [PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid Andy Shevchenko
@ 2019-08-30 15:08 ` Andy Shevchenko
  2019-09-01 10:23   ` Pavel Machek
  2019-08-30 21:11 ` [PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid Jacek Anaszewski
  1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2019-08-30 15:08 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().

While here, check the desired brightness against maximum defined for
a certain LED.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
- check against max_brightness (Jacek)
 drivers/leds/trigger/ledtrig-gpio.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index dc64679b1a92..2d70c35588cb 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -60,10 +60,10 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
 	unsigned desired_brightness;
 	int ret;
 
-	ret = sscanf(buf, "%u", &desired_brightness);
-	if (ret < 1 || desired_brightness > 255) {
+	ret = kstrtouint(buf, 10, &desired_brightness);
+	if (ret || desired_brightness > gpio_data->led->max_brightness) {
 		dev_err(dev, "invalid value\n");
-		return -EINVAL;
+		return ret ? ret : -EINVAL;
 	}
 
 	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 v2 1/2] leds: trigger: gpio: GPIO 0 is valid
  2019-08-30 15:08 [PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid Andy Shevchenko
  2019-08-30 15:08 ` [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox() Andy Shevchenko
@ 2019-08-30 21:11 ` Jacek Anaszewski
  1 sibling, 0 replies; 7+ messages in thread
From: Jacek Anaszewski @ 2019-08-30 21:11 UTC (permalink / raw)
  To: Andy Shevchenko, Pavel Machek, Dan Murphy, linux-leds

Hi Andy,

Thank you for the v2.

On 8/30/19 5:08 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>
> ---
> - set initial GPIO value to -ENOENT (Jacek)
>  drivers/leds/trigger/ledtrig-gpio.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
> index 33cc99a1a16a..dc64679b1a92 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. */
> @@ -172,6 +172,8 @@ static int gpio_trig_activate(struct led_classdev *led)
>  		return -ENOMEM;
>  
>  	gpio_data->led = led;
> +	gpio_data->gpio = -ENOENT;
> +
>  	led_set_trigger_data(led, gpio_data);
>  
>  	return 0;
> @@ -181,7 +183,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);
>  }
> 

Both 1/2 and 2/2 applied.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox()
  2019-08-30 15:08 ` [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox() Andy Shevchenko
@ 2019-09-01 10:23   ` Pavel Machek
  2019-09-01 11:36     ` Jacek Anaszewski
  2019-09-02 10:08     ` Andy Shevchenko
  0 siblings, 2 replies; 7+ messages in thread
From: Pavel Machek @ 2019-09-01 10:23 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Jacek Anaszewski, Dan Murphy, linux-leds

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

On Fri 2019-08-30 18:08:20, 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().
> 
> While here, check the desired brightness against maximum defined for
> a certain LED.

One change per patch, please.

Because this one will not end well.

> @@ -60,10 +60,10 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
>  	unsigned desired_brightness;
>  	int ret;
>  
> -	ret = sscanf(buf, "%u", &desired_brightness);
> -	if (ret < 1 || desired_brightness > 255) {
> +	ret = kstrtouint(buf, 10, &desired_brightness);
> +	if (ret || desired_brightness > gpio_data->led->max_brightness) {
>  		dev_err(dev, "invalid value\n");
> -		return -EINVAL;
> +		return ret ? ret : -EINVAL;
>  	}

We have people writing 255 into brightness, because that's what we
used to do even for on/off LEDS. It is expected to work even for leds
with max_brightness of 1.

So... we want to saturate here, not return -EINVAL. (And we will
eventually want to switch on/off leds to max_brightness = 1...)

> @@ -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. */

So, this accepted 0/1. Now it also accepts true false and many other pairs.

Which... might be ok. But probably should be separated.

Best regards,
									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox()
  2019-09-01 10:23   ` Pavel Machek
@ 2019-09-01 11:36     ` Jacek Anaszewski
  2019-09-01 11:41       ` Jacek Anaszewski
  2019-09-02 10:08     ` Andy Shevchenko
  1 sibling, 1 reply; 7+ messages in thread
From: Jacek Anaszewski @ 2019-09-01 11:36 UTC (permalink / raw)
  To: Pavel Machek, Andy Shevchenko; +Cc: Dan Murphy, linux-leds

Hi Pavel,

On 9/1/19 12:23 PM, Pavel Machek wrote:
> On Fri 2019-08-30 18:08:20, 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().
>>
>> While here, check the desired brightness against maximum defined for
>> a certain LED.
> 
> One change per patch, please.
> 
> Because this one will not end well.
> 
>> @@ -60,10 +60,10 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
>>  	unsigned desired_brightness;
>>  	int ret;
>>  
>> -	ret = sscanf(buf, "%u", &desired_brightness);
>> -	if (ret < 1 || desired_brightness > 255) {
>> +	ret = kstrtouint(buf, 10, &desired_brightness);
>> +	if (ret || desired_brightness > gpio_data->led->max_brightness) {
>>  		dev_err(dev, "invalid value\n");
>> -		return -EINVAL;
>> +		return ret ? ret : -EINVAL;
>>  	}
> 
> We have people writing 255 into brightness, because that's what we
> used to do even for on/off LEDS. It is expected to work even for leds
> with max_brightness of 1.
> 
> So... we want to saturate here, not return -EINVAL. (And we will
> eventually want to switch on/off leds to max_brightness = 1...)

Good point. We shouldn't fail here but proceed similarly as in case
of setting brightness for a LED in led_set_brightness_nosleep(), i.e.
here it should be:

desired_brightness = min(desired_brightness,
                         gpio_data->led->->max_brightness);

So the condition should be limited to checking error code.

>> @@ -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. */
> 
> So, this accepted 0/1. Now it also accepts true false and many other pairs.
> 
> Which... might be ok. But probably should be separated.
> 
> Best regards,
> 									Pavel
> 									
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox()
  2019-09-01 11:36     ` Jacek Anaszewski
@ 2019-09-01 11:41       ` Jacek Anaszewski
  0 siblings, 0 replies; 7+ messages in thread
From: Jacek Anaszewski @ 2019-09-01 11:41 UTC (permalink / raw)
  To: Pavel Machek, Andy Shevchenko; +Cc: Dan Murphy, linux-leds

On 9/1/19 1:36 PM, Jacek Anaszewski wrote:
> Hi Pavel,
> 
> On 9/1/19 12:23 PM, Pavel Machek wrote:
>> On Fri 2019-08-30 18:08:20, 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().
>>>
>>> While here, check the desired brightness against maximum defined for
>>> a certain LED.
>>
>> One change per patch, please.
>>
>> Because this one will not end well.
>>
>>> @@ -60,10 +60,10 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
>>>  	unsigned desired_brightness;
>>>  	int ret;
>>>  
>>> -	ret = sscanf(buf, "%u", &desired_brightness);
>>> -	if (ret < 1 || desired_brightness > 255) {
>>> +	ret = kstrtouint(buf, 10, &desired_brightness);
>>> +	if (ret || desired_brightness > gpio_data->led->max_brightness) {
>>>  		dev_err(dev, "invalid value\n");
>>> -		return -EINVAL;
>>> +		return ret ? ret : -EINVAL;
>>>  	}
>>
>> We have people writing 255 into brightness, because that's what we
>> used to do even for on/off LEDS. It is expected to work even for leds
>> with max_brightness of 1.
>>
>> So... we want to saturate here, not return -EINVAL. (And we will
>> eventually want to switch on/off leds to max_brightness = 1...)
> 
> Good point. We shouldn't fail here but proceed similarly as in case
> of setting brightness for a LED in led_set_brightness_nosleep(), i.e.
> here it should be:
> 
> desired_brightness = min(desired_brightness,
>                          gpio_data->led->->max_brightness);

PS. Dropped the patch.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox()
  2019-09-01 10:23   ` Pavel Machek
  2019-09-01 11:36     ` Jacek Anaszewski
@ 2019-09-02 10:08     ` Andy Shevchenko
  1 sibling, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2019-09-02 10:08 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Jacek Anaszewski, Dan Murphy, linux-leds

On Sun, Sep 01, 2019 at 12:23:17PM +0200, Pavel Machek wrote:
> On Fri 2019-08-30 18:08:20, 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().
> > 
> > While here, check the desired brightness against maximum defined for
> > a certain LED.
> 
> One change per patch, please.
> 
> Because this one will not end well.
> 
> > @@ -60,10 +60,10 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
> >  	unsigned desired_brightness;
> >  	int ret;
> >  
> > -	ret = sscanf(buf, "%u", &desired_brightness);
> > -	if (ret < 1 || desired_brightness > 255) {
> > +	ret = kstrtouint(buf, 10, &desired_brightness);
> > +	if (ret || desired_brightness > gpio_data->led->max_brightness) {
> >  		dev_err(dev, "invalid value\n");
> > -		return -EINVAL;
> > +		return ret ? ret : -EINVAL;
> >  	}
> 
> We have people writing 255 into brightness, because that's what we
> used to do even for on/off LEDS. It is expected to work even for leds
> with max_brightness of 1.
> 
> So... we want to saturate here, not return -EINVAL. (And we will
> eventually want to switch on/off leds to max_brightness = 1...)

Agree. Thank you for your review!

> 
> > @@ -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. */
> 
> So, this accepted 0/1. Now it also accepts true false and many other pairs.
> 
> Which... might be ok. But probably should be separated.



-- 
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-30 15:08 [PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid Andy Shevchenko
2019-08-30 15:08 ` [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox() Andy Shevchenko
2019-09-01 10:23   ` Pavel Machek
2019-09-01 11:36     ` Jacek Anaszewski
2019-09-01 11:41       ` Jacek Anaszewski
2019-09-02 10:08     ` Andy Shevchenko
2019-08-30 21:11 ` [PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid Jacek Anaszewski

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