diff mbox series

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

Message ID 20240703010131.1735100-3-mikhail.kshevetskiy@iopsys.eu
State Changes Requested
Delegated to: Simon Glass
Headers show
Series [1/2] led: Implement software led blinking | expand

Commit Message

Mikhail Kshevetskiy July 3, 2024, 1:01 a.m. UTC 
From: Michael Polyntsov <michael.polyntsov@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@iopsys.eu>
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
---
 drivers/led/led-uclass.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

Comments

Simon Glass July 3, 2024, 8:08 a.m. UTC | #1 
Hi Mikhail,

On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy
<mikhail.kshevetskiy@iopsys.eu> wrote:
>
> From: Michael Polyntsov <michael.polyntsov@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@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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
Mikhail Kshevetskiy July 5, 2024, 2:20 a.m. UTC | #2 
On 7/3/24 12:08, Simon Glass wrote:
> Hi Mikhail,
>
> On Wed, 3 Jul 2024 at 02:02, Mikhail Kshevetskiy
> <mikhail.kshevetskiy@iopsys.eu> wrote:
>> From: Michael Polyntsov <michael.polyntsov@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@iopsys.eu>
>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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
diff mbox series

Patch

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;
+#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
+	trigger = dev_read_string(dev, "linux,default-trigger");
+	if (trigger && !strncmp(trigger, "pattern", 7)) {
+		uc_plat->default_state = LEDST_BLINK;
+	}
+#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: {
+		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) = {