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

[Denis Osterland-Heim]

Hi Jacek,

thanks for the hint.
Perfekt! One thing less to do :D

Regards Denis

Am Mittwoch, den 11.03.2020, 23:33 +0100 schrieb Jacek Anaszewski:
> 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
> 


Diehl Connectivity Solutions GmbH
GeschÃftsfÃhrung: Horst Leonberger
Sitz der Gesellschaft: NÃrnberg - Registergericht: Amtsgericht
NÃrnberg: HRB 32315
___________________________________________________________________________________________________

Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
- Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/

The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
- For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/