Message ID | 20240703010131.1735100-3-mikhail.kshevetskiy@iopsys.eu |
---|---|
State | Changes Requested |
Delegated to: | Simon Glass |
Headers | show |
Series | [1/2] led: Implement software led blinking | expand |
Hi Mikhail, On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu> wrote: > > From: Michael Polyntsov <michael.polyntsov@iopsys.eu> > > The standard property > > linux,default-trigger = "pattern"; > > used to get an effect. No blinking parameters can be set yet. > > Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu> > Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu> > --- > drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c > index d021c3bbf20..78d1a3d152b 100644 > --- a/drivers/led/led-uclass.c > +++ b/drivers/led/led-uclass.c > @@ -190,6 +190,9 @@ static int led_post_bind(struct udevice *dev) > { > struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); > const char *default_state; > +#ifdef CONFIG_LED_BLINK > + const char *trigger; It looks like this is not used, so you can drop it and use a local variable here? > +#endif > > if (!uc_plat->label) > uc_plat->label = dev_read_string(dev, "label"); > @@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev) > else > return 0; > > +#ifdef CONFIG_LED_BLINK if (IS_ENABLED(CONFIG_LED_BLINK) > + trigger = dev_read_string(dev, "linux,default-trigger"); > + if (trigger && !strncmp(trigger, "pattern", 7)) { > + uc_plat->default_state = LEDST_BLINK; > + } No {} around single lines. You can use patman to send patches and it will check this for you (I hope!). > +#endif > + > /* > * In case the LED has default-state DT property, trigger > * probe() to configure its default state during startup. > @@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev) > static int led_post_probe(struct udevice *dev) > { > struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); > + int rc = 0; > > - if (uc_plat->default_state == LEDST_ON || > - uc_plat->default_state == LEDST_OFF) > - led_set_state(dev, uc_plat->default_state); > + switch (uc_plat->default_state) { > + case LEDST_ON: > + case LEDST_OFF: > + rc = led_set_state(dev, uc_plat->default_state); > + break; > +#ifdef CONFIG_LED_BLINK > + case LEDST_BLINK: { Here again you can use if IS_ENABLED(CONFIG_LED_BLINK) inside the case which should produce very similar code size, hopefully the same. > + const int default_period_ms = 1000; > > - return 0; > + rc = led_set_period(dev, default_period_ms); > + if (rc == 0) > + rc = led_set_state(dev, uc_plat->default_state); > + break; > + } > +#endif > + default: > + break; > + } > + > + return rc; > } > > UCLASS_DRIVER(led) = { > -- > 2.39.2 > Regards, Simon
On 7/3/24 12:08, Simon Glass wrote: > Hi Mikhail, > > On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy > <mikhail.kshevetskiy@iopsys.eu> wrote: >> From: Michael Polyntsov <michael.polyntsov@iopsys.eu> >> >> The standard property >> >> linux,default-trigger = "pattern"; >> >> used to get an effect. No blinking parameters can be set yet. >> >> Signed-off-by: Michael Polyntsov <michael.polyntsov@iopsys.eu> >> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu> >> --- >> drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++---- >> 1 file changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c >> index d021c3bbf20..78d1a3d152b 100644 >> --- a/drivers/led/led-uclass.c >> +++ b/drivers/led/led-uclass.c >> @@ -190,6 +190,9 @@ static int led_post_bind(struct udevice *dev) >> { >> struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); >> const char *default_state; >> +#ifdef CONFIG_LED_BLINK >> + const char *trigger; > It looks like this is not used, so you can drop it and use a local > variable here? Unfortunately no, see below. > >> +#endif >> >> if (!uc_plat->label) >> uc_plat->label = dev_read_string(dev, "label"); >> @@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev) >> else >> return 0; >> >> +#ifdef CONFIG_LED_BLINK > if (IS_ENABLED(CONFIG_LED_BLINK) This is not so easy. The definition of LEDST_BLINK depends on CONFIG_LED_BLINK, thus this code will not compile if CONFIG_LED_BLINK is not defined. We can define LEDST_BLINK unconditionally, but it will cause an issue in the cmd/led.c. >> + trigger = dev_read_string(dev, "linux,default-trigger"); >> + if (trigger && !strncmp(trigger, "pattern", 7)) { >> + uc_plat->default_state = LEDST_BLINK; >> + } > No {} around single lines. You can use patman to send patches and it > will check this for you (I hope!). > >> +#endif >> + >> /* >> * In case the LED has default-state DT property, trigger >> * probe() to configure its default state during startup. >> @@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev) >> static int led_post_probe(struct udevice *dev) >> { >> struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); >> + int rc = 0; >> >> - if (uc_plat->default_state == LEDST_ON || >> - uc_plat->default_state == LEDST_OFF) >> - led_set_state(dev, uc_plat->default_state); >> + switch (uc_plat->default_state) { >> + case LEDST_ON: >> + case LEDST_OFF: >> + rc = led_set_state(dev, uc_plat->default_state); >> + break; >> +#ifdef CONFIG_LED_BLINK >> + case LEDST_BLINK: { > Here again you can use if IS_ENABLED(CONFIG_LED_BLINK) inside the case > which should produce very similar code size, hopefully the same. Do you suggest an if inside the handling of "default:" case? Also see above notes about dependency of LEDST_BLINK on CONFIG_LED_BLINK. >> + const int default_period_ms = 1000; >> >> - return 0; >> + rc = led_set_period(dev, default_period_ms); >> + if (rc == 0) >> + rc = led_set_state(dev, uc_plat->default_state); >> + break; >> + } >> +#endif >> + default: >> + break; >> + } >> + >> + return rc; >> } >> >> UCLASS_DRIVER(led) = { >> -- >> 2.39.2 >> > Regards, > Simon
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index d021c3bbf20..78d1a3d152b 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -190,6 +190,9 @@ static int led_post_bind(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); const char *default_state; +#ifdef CONFIG_LED_BLINK + const char *trigger; +#endif if (!uc_plat->label) uc_plat->label = dev_read_string(dev, "label"); @@ -210,6 +213,13 @@ static int led_post_bind(struct udevice *dev) else return 0; +#ifdef CONFIG_LED_BLINK + trigger = dev_read_string(dev, "linux,default-trigger"); + if (trigger && !strncmp(trigger, "pattern", 7)) { + uc_plat->default_state = LEDST_BLINK; + } +#endif + /* * In case the LED has default-state DT property, trigger * probe() to configure its default state during startup. @@ -222,12 +232,28 @@ static int led_post_bind(struct udevice *dev) static int led_post_probe(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); + int rc = 0; - if (uc_plat->default_state == LEDST_ON || - uc_plat->default_state == LEDST_OFF) - led_set_state(dev, uc_plat->default_state); + switch (uc_plat->default_state) { + case LEDST_ON: + case LEDST_OFF: + rc = led_set_state(dev, uc_plat->default_state); + break; +#ifdef CONFIG_LED_BLINK + case LEDST_BLINK: { + const int default_period_ms = 1000; - return 0; + rc = led_set_period(dev, default_period_ms); + if (rc == 0) + rc = led_set_state(dev, uc_plat->default_state); + break; + } +#endif + default: + break; + } + + return rc; } UCLASS_DRIVER(led) = {