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?
> +#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
