[PATCH v1 1/1] led: led_pwm: fix active-low behavior

[Quentin Schulz]

Hi Svyatoslav,

On 3/3/26 8:13 AM, Svyatoslav Ryhel wrote:
> пн, 2 бер. 2026 р. о 15:07 Quentin Schulz <quentin.schulz at cherry.de> пише:
>>
>> Hi Svyatoslav, Jonas,
>>
>> On 2/9/26 7:07 PM, Svyatoslav Ryhel wrote:
>>> From: Jonas Schwöbel <jonasschwoebel at yahoo.de>
>>>
>>> In the case of active-low behavior the Duty Cycle needs to be set to 100%.
>>> The PWM driver takes care of this but the LED_PWM driver does not take
>>> this into account. Adjust LED_PWM to take into account polarity of PWM.
>>>
>>> Signed-off-by: Jonas Schwöbel <jonasschwoebel at yahoo.de>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95 at gmail.com>
>>> ---
>>>    drivers/led/led_pwm.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/led/led_pwm.c b/drivers/led/led_pwm.c
>>> index 15dd836509b..7edfa286468 100644
>>> --- a/drivers/led/led_pwm.c
>>> +++ b/drivers/led/led_pwm.c
>>> @@ -34,7 +34,7 @@ static int led_pwm_enable(struct udevice *dev)
>>>        if (ret)
>>>                return ret;
>>>
>>> -     ret = pwm_set_enable(priv->pwm, priv->channel, true);
>>> +     ret = pwm_set_enable(priv->pwm, priv->channel, priv->active_low);
>>>        if (ret)
>>>                return ret;
>>>
>>> @@ -52,7 +52,7 @@ static int led_pwm_disable(struct udevice *dev)
>>>        if (ret)
>>>                return ret;
>>>
>>> -     ret = pwm_set_enable(priv->pwm, priv->channel, false);
>>> +     ret = pwm_set_enable(priv->pwm, priv->channel, priv->active_low);
>>
>> This feels wrong. Why would both enable and disable paths use the exact
>> same value for pwm_set_enable()?
>>
> 
> Hello Quentin!
> 
> You are mixing up the LED with the PWM that controls it.
> pwm_set_enable() triggers a PWM configuration update; therefore, the
> process is the same for both enabling and disabling.
> 

Does it? All drivers in drivers/pwm actually write to their registers in 
the set_config callback, some even re-enable the PWM controller if it 
was running before entering the set_config function.

The sunxi driver notably does not do anything on set_invert callback, 
it's only applied on next call to set_enable.

pwm_set_enable() clearly states it's for enabling or disabling the PWM. 
So why would we call pwm_set_enable(,,false) in led_pwm_enable() if 
active-low property isn't set in the DT? Why would we call 
pwm_set_enable(,,true) in led_pwm_disable() if the active-low property 
is set?

>> I can also see that the disable path doesn't actually call
>> pwm_set_invert() unlike in the enable path. I'm assuming this means we
>> cannot call disable before enable and have consistent behavior?
>>
> 
> The inversion setting is only relevant when the PWM is enabled. Since
> the disable operation forces the duty cycle to 0, the inversion state
> is inconsequential.
> 

If you have inverted polarity, I'm assuming you need 100% duty cycle to 
turn the LED off.. So we definitely need to have the PWM running and the 
inversion setting configured, no?

>> The kernel seems to use active_low as a way to invert the duty cycle if
>> I'm understanding the code properly. In U-Boot, it seems we don't handle
>> that at all and always pass the "normal" duty cycle. Maybe there's
>> something we need to do there.
>>
> 
> In U-Boot PWM should handle this in the set_invert operation.
> 
>> I'm also assuming there are setups where PWM should be enabled but with
>> a duty cycle of 0?
>>
> 
> A duty cycle of 0 is equivalent to pwm being turned off. In general

I don't think that's necessarily true? What happens to the PWM pin when 
the PWM controller is disabled? You're not driving the line low (or high 
for inverted polarity) so who's to say the state of the LED is off?

> most devices won't work on very low duty cycles. Fan needs at least
> 30%, LEDs make no sense below 10%, also most Display are not usable
> below 10%
> 
>> I'm also unsure what's the difference between active-low and
>> PWM_POLARITY_INVERTED (I've seen them both mentioned or mixed in the
>> kernel DTSes).
>>
> 
> These are properties of two different device classes: active-low is
> used with PWM-LEDs, while PWM_POLARITY_INVERTED is strictly a PWM
> property.
> 

Sure... but don't they mean the same thing? It seems we are not reading 
the flag property in the PWM cell in the PWM LED driver and only read 
the active-low property to decide on the polarity. What are we supposed 
to do if we only have one of both enabled? Assume it's inverted 
polarity? Then what do we do if both are enabled, is it doubly-inverted, 
meaning normal polarity? Or is it a simple binary OR?

I unfortunately don't have HW to test this.

Cheers,
Quentin