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

[Mikhail Kshevetskiy]

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


>
> Regards,
> Simon