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@xxxxxxxxx>
> > Acked-by: Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>
> > +#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/
