[PATCH 1/1] leds: call led_pwm_set() in leds-pwm to enforce default LED_OFF

[PATCH 1/1] leds: call led_pwm_set() in leds-pwm to enforce default LED_OFF

[Markus Hofstaetter]

Some PWMs are disabled by default or the default pin setting
does not match the LED_OFF state (e.g., active-low leds).
Hence, the driver may end up reporting 0 brightness, but
the leds are actually on using full brightness, because
it never enforces its default configuration.
So enforce it by calling led_pwm_set() after successfully
registering the device.

Tested on a Phytec phyFLEX i.MX6Q board based on kernel
v3.19.5.

Signed-off-by: Markus Hofstaetter <markus.hofstaetter@ait.ac.at>
Tested-by: Markus Hofstaetter <markus.hofstaetter@ait.ac.at>
---
 drivers/leds/leds-pwm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 1d07e3e..3149dbe 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -132,6 +132,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	ret = led_classdev_register(dev, &led_data->cdev);
 	if (ret == 0) {
 		priv->num_leds++;
+		led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
 	} else {
 		dev_err(dev, "failed to register PWM led for %s: %d\n",
 			led->name, ret);
-- 
1.9.1

Re: [PATCH 1/1] leds: call led_pwm_set() in leds-pwm to enforce default LED_OFF

[Jacek Anaszewski]

Hi Markus,

On 11/11/2015 12:40 PM, Markus Hofstaetter wrote:
> Some PWMs are disabled by default or the default pin setting
> does not match the LED_OFF state (e.g., active-low leds).
> Hence, the driver may end up reporting 0 brightness, but
> the leds are actually on using full brightness, because
> it never enforces its default configuration.

AFAICS default brightness is hard coded to LED_OFF, so this
will turn active-low LEDs on.

> So enforce it by calling led_pwm_set() after successfully
> registering the device.
>
> Tested on a Phytec phyFLEX i.MX6Q board based on kernel
> v3.19.5.
>
> Signed-off-by: Markus Hofstaetter <markus.hofstaetter@ait.ac.at>
> Tested-by: Markus Hofstaetter <markus.hofstaetter@ait.ac.at>
> ---
>   drivers/leds/leds-pwm.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index 1d07e3e..3149dbe 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -132,6 +132,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>   	ret = led_classdev_register(dev, &led_data->cdev);
>   	if (ret == 0) {
>   		priv->num_leds++;
> +		led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
>   	} else {
>   		dev_err(dev, "failed to register PWM led for %s: %d\n",
>   			led->name, ret);
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 1/1] leds: call led_pwm_set() in leds-pwm to enforce default LED_OFF

[Hofstätter Markus]

Hi Jacek,

thx for looking into the patch.

> AFAICS default brightness is hard coded to LED_OFF, so this
> will turn active-low LEDs on.

LED_OFF will turn a active led on only if you don't set active-low in the dt. In latter case  the behaviour is inverted to match the same brightness meanings (for pwms that don't support inversion in the driver).
As you saw the LED_OFF setting is hard coded and set it in the device structure by the probe/pwm add routine.

The issue here is that this will only report a brightness of 0 (e.g., cat ..led/brightness is LED_OFF), but it never configures the associated pwm to that state. So the PIN will be left at its default resp. previous state until you set a new brightness value.

E.g., in my case on the i.MX6Q, the default PIN setting outputs low --> led is at full brightness and cat brightness reports 0. Setting 0 brightness again, turns the light off.
Also, I had looked at the PWM register and saw that it has not been configured until I have set the brightness again.
Anyhow, this would also be an issue for active-high leds, if  the default/previous PIN state were active-high, as it would not be set to low. This however, is not the case in most scenarios.

Best regards,

Markus Hofstätter

________________________________________
Von: Jacek Anaszewski [j.anaszewski@samsung.com]
Gesendet: Montag, 16. November 2015 12:20
An: Hofstätter Markus
Cc: Bryan Wu; Richard Purdie; linux-leds@vger.kernel.org
Betreff: Re: [PATCH 1/1] leds: call led_pwm_set() in leds-pwm to enforce default LED_OFF

Hi Markus,

On 11/11/2015 12:40 PM, Markus Hofstaetter wrote:
> Some PWMs are disabled by default or the default pin setting
> does not match the LED_OFF state (e.g., active-low leds).
> Hence, the driver may end up reporting 0 brightness, but
> the leds are actually on using full brightness, because
> it never enforces its default configuration.

AFAICS default brightness is hard coded to LED_OFF, so this
will turn active-low LEDs on.

> So enforce it by calling led_pwm_set() after successfully
> registering the device.
>
> Tested on a Phytec phyFLEX i.MX6Q board based on kernel
> v3.19.5.
>
> Signed-off-by: Markus Hofstaetter <markus.hofstaetter@ait.ac.at>
> Tested-by: Markus Hofstaetter <markus.hofstaetter@ait.ac.at>
> ---
>   drivers/leds/leds-pwm.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index 1d07e3e..3149dbe 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -132,6 +132,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>       ret = led_classdev_register(dev, &led_data->cdev);
>       if (ret == 0) {
>               priv->num_leds++;
> +             led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
>       } else {
>               dev_err(dev, "failed to register PWM led for %s: %d\n",
>                       led->name, ret);
>


--
Best Regards,
Jacek Anaszewski

Re: [PATCH 1/1] leds: call led_pwm_set() in leds-pwm to enforce default LED_OFF

[Jacek Anaszewski]

Hi Markus,

Thanks for your explanation. I missed that led_pwm_set behaves
differently when active-low DT property is set.
I've just applied that patch to the LED tree, for-next branch.

Thanks,
Jacek Anaszewski

On 11/16/2015 02:48 PM, Hofstätter Markus wrote:
> Hi Jacek,
>
> thx for looking into the patch.
>
>> AFAICS default brightness is hard coded to LED_OFF, so this
>> will turn active-low LEDs on.
>
> LED_OFF will turn a active led on only if you don't set active-low in the dt. In latter case  the behaviour is inverted to match the same brightness meanings (for pwms that don't support inversion in the driver).
> As you saw the LED_OFF setting is hard coded and set it in the device structure by the probe/pwm add routine.
>
> The issue here is that this will only report a brightness of 0 (e.g., cat ..led/brightness is LED_OFF), but it never configures the associated pwm to that state. So the PIN will be left at its default resp. previous state until you set a new brightness value.
>
> E.g., in my case on the i.MX6Q, the default PIN setting outputs low --> led is at full brightness and cat brightness reports 0. Setting 0 brightness again, turns the light off.
> Also, I had looked at the PWM register and saw that it has not been configured until I have set the brightness again.
> Anyhow, this would also be an issue for active-high leds, if  the default/previous PIN state were active-high, as it would not be set to low. This however, is not the case in most scenarios.
>
> Best regards,
>
> Markus Hofstätter
>
> ________________________________________
> Von: Jacek Anaszewski [j.anaszewski@samsung.com]
> Gesendet: Montag, 16. November 2015 12:20
> An: Hofstätter Markus
> Cc: Bryan Wu; Richard Purdie; linux-leds@vger.kernel.org
> Betreff: Re: [PATCH 1/1] leds: call led_pwm_set() in leds-pwm to enforce default LED_OFF
>
> Hi Markus,
>
> On 11/11/2015 12:40 PM, Markus Hofstaetter wrote:
>> Some PWMs are disabled by default or the default pin setting
>> does not match the LED_OFF state (e.g., active-low leds).
>> Hence, the driver may end up reporting 0 brightness, but
>> the leds are actually on using full brightness, because
>> it never enforces its default configuration.
>
> AFAICS default brightness is hard coded to LED_OFF, so this
> will turn active-low LEDs on.
>
>> So enforce it by calling led_pwm_set() after successfully
>> registering the device.
>>
>> Tested on a Phytec phyFLEX i.MX6Q board based on kernel
>> v3.19.5.
>>
>> Signed-off-by: Markus Hofstaetter <markus.hofstaetter@ait.ac.at>
>> Tested-by: Markus Hofstaetter <markus.hofstaetter@ait.ac.at>
>> ---
>>    drivers/leds/leds-pwm.c | 1 +
>>    1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
>> index 1d07e3e..3149dbe 100644
>> --- a/drivers/leds/leds-pwm.c
>> +++ b/drivers/leds/leds-pwm.c
>> @@ -132,6 +132,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>>        ret = led_classdev_register(dev, &led_data->cdev);
>>        if (ret == 0) {
>>                priv->num_leds++;
>> +             led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
>>        } else {
>>                dev_err(dev, "failed to register PWM led for %s: %d\n",
>>                        led->name, ret);
>>
>
>
> --
> Best Regards,
> Jacek Anaszewski
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>