lkml.org 
[lkml]   [2020]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v6 1/2] leds: pwm: add support for default-state device property
Date
Hi Pavel,

Am Mittwoch, den 22.07.2020, 16:04 +0200 schrieb Pavel Machek:
> > This patch adds support for "default-state" devicetree property, which
> > allows to defer pwm init to first use of led.
> >
> > This allows to configure the PWM early in bootloader to let the LED
> > blink until an application in Linux userspace sets something different.
> >
> > Signed-off-by: Denis Osterland-Heim <Denis.Osterland@diehl.com>
> > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > +#define LEDS_PWM_DEFSTATE_OFF 0
> > +#define LEDS_PWM_DEFSTATE_ON 1
> > +#define LEDS_PWM_DEFSTATE_KEEP 2
>
> Turn this into enum; no need for prefix as this is private to the driver.
>
> > struct led_pwm {
> > const char *name;
> > const char *default_trigger;
> > u8 active_low;
> > + u8 default_state;
> > unsigned int max_brightness;
> > };
> >
> > @@ -88,7 +93,30 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> >
> > led_data->cdev.brightness_set_blocking = led_pwm_set;
> >
> > - pwm_init_state(led_data->pwm, &led_data->pwmstate);
> > + /* init PWM state */
> > + if (led->default_state == LEDS_PWM_DEFSTATE_KEEP) {
> > + pwm_get_state(led_data->pwm, &led_data->pwmstate);
> > + if (!led_data->pwmstate.period) {
> > + led->default_state = LEDS_PWM_DEFSTATE_OFF;
> > + dev_warn(dev,
> > + "failed to read period for %s, default to off",
> > + led->name);
> > + }
> > + }
> > + if (led->default_state != LEDS_PWM_DEFSTATE_KEEP)
> > + pwm_init_state(led_data->pwm, &led_data->pwmstate);
> > +
> > + /* set brightness */
> > + if (led->default_state == LEDS_PWM_DEFSTATE_ON)
> > + led_data->cdev.brightness = led->max_brightness;
> > + else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP) {
> > + uint64_t brightness;
> > +
> > + brightness = led->max_brightness;
> > + brightness *= led_data->pwmstate.duty_cycle;
> > + do_div(brightness, led_data->pwmstate.period);
> > + led_data->cdev.brightness = brightness;
> > + }
>
> Try to clean this up... switch() might help. Maybe two of them.
looks possible, lets see if it improves readability

>
> > @@ -134,6 +166,16 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
> > fwnode_property_read_u32(fwnode, "max-brightness",
> > &led.max_brightness);
> >
> > + if (!fwnode_property_read_string(fwnode, "default-state",
> > + &state)) {
> > + if (!strcmp(state, "keep"))
> > + led.default_state = LEDS_PWM_DEFSTATE_KEEP;
> > + else if (!strcmp(state, "on"))
> > + led.default_state = LEDS_PWM_DEFSTATE_ON;
> > + else
> > + led.default_state = LEDS_PWM_DEFSTATE_OFF;
> > + }
>
> Actually... Move the enum to core, and add helper for this. We don't
> want to see this duplicated.
I think I should put the refactoring into a separate patch.

This code duplicates leds-gpio, which uses the LEDS_GPIO_DEFSTATE_* defines,
defined in include/linux/leds.h.
The I idea was to let gpio its defines and add dt compatible defines for leds-pwm.
But it would be useful to have a common function, yes.

It looks like a enum leds_default_state with the define for leds-gpio changed
to the new enum in linux/leds.h and a new function declaration leds_defstate_get()
in leds/leds.h, with the implementation led-core.c. And leds-gpio and leds-pwm
will then include leds/leds.h to use leds_defstate_get().

>
> > 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/
>
> Get rid of this.
my force is not strong enough...

Regards, Denis

> Pavel
> +----------------------------------------------------------------------+
> > Z1 SecureMail Gateway Processing Info |
>
> +----------------------------------------------------------------------+
> > - The message was signed by |
> > [No Info available] |
> > Signature not verifiable |
> > - Message content not verifiable |
> > - Certificate not verifiable |
>
> +----------------------------------------------------------------------+


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/
\
 
 \ /
  Last update: 2020-07-22 18:50    [W:0.043 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site