On 7/3/24 12:08, Simon Glass wrote:
> Hi Mikhail,
>
> On Wed, 3 Jul 2024 at 02:02, 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 | 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
