Re: [PATCH v2] leds: pwm: add support for default-state device property

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Denis,

On 3/11/20 7:45 AM, Denis Osterland-Heim wrote:
> Hi Jacek,
> 
> Am Dienstag, den 10.03.2020, 22:15 +0100 schrieb Jacek Anaszewski:
>> Hi Denis,
>>
>> Thank you for the update. Please find my remarks below.
>>
>> On 3/10/20 4:19 PM, Denis Osterland-Heim wrote:
>>> Hi,
>>>
> ...
>>>> --- a/drivers/leds/leds-pwm.c
>>>> +++ b/drivers/leds/leds-pwm.c
>>>> @@ -75,7 +75,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>>>>  	led_data->active_low = led->active_low;
>>>>  	led_data->cdev.name = led->name;
>>>>  	led_data->cdev.default_trigger = led->default_trigger;
>>>> -	led_data->cdev.brightness = LED_OFF;
>>>> +	ret = led->default_state == LEDS_GPIO_DEFSTATE_ON;
>>
>> ret is for return value and it should not be used for anything
>> else just because it is at hand. Also LEDS_GPIO* definitions have
>> nothing to do with pwm leds. This is legacy because default-state
>> property was primarily specific to leds-gpio bindings and only
>> later was made common.
>>
>> Please introduce corresponding LEDS_PWM definitions, but in leds-pwm.c.
> will change
> 
>>
>>>> +	led_data->cdev.brightness = ret ? led->max_brightness : LED_OFF;
>>
>> Instead of above two changes I'd add below:
>>
>> if (led->default_state == LEDS_PWM_DEFSTATE_ON) {
>> 	led_data->cdev.brightness = led->max_brightness;
>> } else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP)) {
>> 	// here put what you're adding below, but please use
>> 	// pwm_get_state() instead of accessing ops directly
>> }
>>
>> LED_OFF case is covered by kzalloc() in led_pwm_probe().
> Looks very elegant, I will apply this.
> pwm_get_state() is not sufficient here because it only returns the values from structure,
> which were not read read from registers at pwm_device_request().
> Something like pwm_get_state_uncached() would be required, which I have not found yet.
> 
> I looked at the atomic PWM API (5ec803edcb703fe379836f13560b79dfac79b01d),
> which claims to provide a smooth handover from bootloader to kernel.
> So it seems it would be better to fix the FIXME first, in a first patch.

I missed that it's been recently done. You've got to rebase onto Pavel's
for-next branch [0].

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/log/?h=for-next

-- 
Best regards,
Jacek Anaszewski