[PATCH v5 2/2] led: Add dts property to specify blinking of the led

[Simon Glass]

On Mon, 15 Jul 2024 at 14:23, Mikhail Kshevetskiy
<mikhail.kshevetskiy at genexis.eu> wrote:
>
>
> On 7/15/24 11:11, Simon Glass wrote:
> > Hi Mikhail,
> >
> > On Sat, 13 Jul 2024 at 17:32, Mikhail Kshevetskiy
> > <mikhail.kshevetskiy at genexis.eu> wrote:
> >>
> >> On 13.07.2024 18:13, Simon Glass wrote:
> >>> Hi Mikhail,
> >>>
> >>> On Fri, 12 Jul 2024 at 06:25, Mikhail Kshevetskiy
> >>> <mikhail.kshevetskiy at iopsys.eu> wrote:
> >>>> From: Michael Polyntsov <michael.polyntsov at 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 at iopsys.eu>
> >>>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at iopsys.eu>
> >>>> ---
> >>>>  drivers/led/led-uclass.c | 33 +++++++++++++++++++++++++++++----
> >>>>  1 file changed, 29 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
> >>>> index 4f5dd66765e..90047ad35ba 100644
> >>>> --- a/drivers/led/led-uclass.c
> >>>> +++ b/drivers/led/led-uclass.c
> >>>> @@ -100,6 +100,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");
> >>>> @@ -120,6 +123,12 @@ 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
> >>> I still wonder whether you can use IS_ENABLED() instead of adding #ifdefs?
> >> LEDST_BLINK depends on CONFIG_LED_BLINK, so the code will not compile if CONFIG_LED_BLINK is not defined.
> >> Defining a LEDST_BLINK unconditionally will produce an issue with cmd/led.c
> > We should not use #ifdefs around such declarations. What issue is
> > caused by defining it always?
>
> We need to patch cmd/led.c. Without it we will get:
> * something like out of boundary access
> * enumeration value ‘LEDST_BLINK’ not handled in switch
>
> can be fixed, but probably it should be a separate patch

Sounds good

Reviewed-by: Simon Glass <sjg at chromium.org>