On Mon, 15 Jul 2024 at 14:23, Mikhail Kshevetskiy
<mikhail.kshevets...@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.kshevets...@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.kshevets...@iopsys.eu> wrote:
> >>>> From: Michael Polyntsov <michael.polynt...@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.polynt...@iopsys.eu>
> >>>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevets...@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 <s...@chromium.org>
