dt-bindings: leds: register-bit-led: Add value property

PATCH In review Pali Rohár 3 participants 7 messages Jul 6, 2022 – Jul 11, 2022

Subsystems (2)

Device tree bindings LED

lore

Share & tools

lore

Pali Rohár

author

2022-07-06 11:29 UTC #0 lore raw

PATCH 1/2 dt-bindings: leds: register-bit-led: Add value property

Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value property. This property name is already used by other syscon drivers, e.g. syscon-reboot.

Signed-off-by Pali Rohár <[email protected]>

---
 .../devicetree/bindings/leds/register-bit-led.yaml    | 11 +++++++++++
 1 file changed, 11 insertions(+)

Documentation/devicetree/bindings/leds/register-bit-led.yaml +11 -0

index 79b8fc0f9d23..d6054a3f9087 100644
@@ -43,6 +43,17 @@ properties:
         0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
         0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
 
+  value:
+    description:
+      bit value of ON state for the bit controlling this LED in the register
+      when not specified it is same as the mask
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      [ 0x0, 0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80, 0x100, 0x200, 0x400, 0x800,
+        0x1000, 0x2000, 0x4000, 0x8000, 0x10000, 0x20000, 0x40000, 0x80000,
+        0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
+        0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
+
   offset:
     description:
       register offset to the register controlling this LED

Signature · 1 line

2.20.1

Pali Rohár

author

2022-07-06 11:29 UTC #1 lore raw

PATCH 2/2 leds: syscon: Implement support for value property

This new value property specify when is LED enabled. By default its value is from the mask and therefore LED is enabled when bit is set. This change allows to define inverted logic (0 - enable LED, 1 - disable LED) by setting value property to zero.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/leds/leds-syscon.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

drivers/leds/leds-syscon.c +9 -5

index 7eddb8ecb44e..337a0bada967 100644
@@ -29,6 +29,7 @@ struct syscon_led {
 	struct regmap *map;
 	u32 offset;
 	u32 mask;
+	u32 value;
 	bool state;
 };

@@ -41,10 +42,10 @@ static void syscon_led_set(struct led_classdev *led_cdev,
 	int ret;
 
 	if (value == LED_OFF) {
-		val = 0;
+		val = ~sled->value;
 		sled->state = false;
 	} else {
-		val = sled->mask;
+		val = sled->value;
 		sled->state = true;
 	}

@@ -85,6 +86,8 @@ static int syscon_led_probe(struct platform_device *pdev)
 		return -EINVAL;
 	if (of_property_read_u32(np, "mask", &sled->mask))
 		return -EINVAL;
+	if (of_property_read_u32(np, "value", &sled->value))
+		sled->value = sled->mask;
 
 	state = of_get_property(np, "default-state", NULL);
 	if (state) {

@@ -94,18 +97,19 @@ static int syscon_led_probe(struct platform_device *pdev)
 			ret = regmap_read(map, sled->offset, &val);
 			if (ret < 0)
 				return ret;
-			sled->state = !!(val & sled->mask);
+			sled->state = (val & sled->mask) == sled->value;
 		} else if (!strcmp(state, "on")) {
 			sled->state = true;
 			ret = regmap_update_bits(map, sled->offset,
 						 sled->mask,
-						 sled->mask);
+						 sled->value);
 			if (ret < 0)
 				return ret;
 		} else {
 			sled->state = false;
 			ret = regmap_update_bits(map, sled->offset,
-						 sled->mask, 0);
+						 sled->mask,
+						 ~sled->value);
 			if (ret < 0)
 				return ret;
 		}

Signature · 1 line

2.20.1

Rob Herring

maintainer

2022-07-06 16:21 UTC #2 lore raw

In reply to Pali Rohár

Quoted 3 lines · Pali Rohár

Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value property. This property name is already used by other syscon drivers, e.g. syscon-reboot.

Yes, but those are potentially multi-bit values. This is a single bit value, and the only value that's ever needed is 0. Why not just use 'active-low' here?

Quoted 32 lines

Signed-off-by Pali Rohár <[email protected]>

---
 .../devicetree/bindings/leds/register-bit-led.yaml    | 11 +++++++++++
 1 file changed, 11 insertions(+)

Documentation/devicetree/bindings/leds/register-bit-led.yaml +11 -0

index 79b8fc0f9d23..d6054a3f9087 100644
@@ -43,6 +43,17 @@ properties:
         0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
         0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
 
+  value:
+    description:
+      bit value of ON state for the bit controlling this LED in the register
+      when not specified it is same as the mask
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      [ 0x0, 0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80, 0x100, 0x200, 0x400, 0x800,
+        0x1000, 0x2000, 0x4000, 0x8000, 0x10000, 0x20000, 0x40000, 0x80000,
+        0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
+        0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
+
   offset:
     description:
       register offset to the register controlling this LED

Signature · 1 line

2.20.1

Pali Rohár

author

2022-07-06 16:23 UTC #3 lore raw

Quoted 8 lines · On Wednesday 06 July 2022 10:21:11 Rob Herring wrote:

Quoted 3 lines · Pali Rohár
Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value property. This property name is already used by other syscon drivers, e.g. syscon-reboot.
Yes, but those are potentially multi-bit values. This is a single bit value, and the only value that's ever needed is 0. Why not just use 'active-low' here?

Just because to have uniform definitions across more syscon nodes.

Quoted 32 lines

Signed-off-by Pali Rohár <[email protected]>

---
 .../devicetree/bindings/leds/register-bit-led.yaml    | 11 +++++++++++
 1 file changed, 11 insertions(+)

Documentation/devicetree/bindings/leds/register-bit-led.yaml +11 -0

index 79b8fc0f9d23..d6054a3f9087 100644
@@ -43,6 +43,17 @@ properties:
         0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
         0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
 
+  value:
+    description:
+      bit value of ON state for the bit controlling this LED in the register
+      when not specified it is same as the mask
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      [ 0x0, 0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80, 0x100, 0x200, 0x400, 0x800,
+        0x1000, 0x2000, 0x4000, 0x8000, 0x10000, 0x20000, 0x40000, 0x80000,
+        0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000,
+        0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ]
+
   offset:
     description:
       register offset to the register controlling this LED

Signature · 1 line

2.20.1

Linus Walleij

2022-07-11 12:07 UTC #4 lore raw

Quoted 11 lines · Pali Rohár

Quoted 8 lines · On Wednesday 06 July 2022 10:21:11 Rob Herring wrote:
Quoted 3 lines · Pali Rohár
Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value property. This property name is already used by other syscon drivers, e.g. syscon-reboot.
Yes, but those are potentially multi-bit values. This is a single bit value, and the only value that's ever needed is 0. Why not just use 'active-low' here?
Just because to have uniform definitions across more syscon nodes.

But what happens if he mask and value don't line up?

mask = 0x10;
value = 0x08;

If you just state active-low; this kind of mistake is not possible to make.

So I'd rather go for active-low;

Yours,
Linus Walleij

Pali Rohár

author

2022-07-11 12:10 UTC #5 lore raw

Quoted 11 lines · On Monday 11 July 2022 14:06:50 Linus Walleij wrote:

Quoted 10 lines · Pali Rohár
Quoted 8 lines · On Wednesday 06 July 2022 10:21:11 Rob Herring wrote:
Quoted 3 lines · Pali Rohár
Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value property. This property name is already used by other syscon drivers, e.g. syscon-reboot.
Yes, but those are potentially multi-bit values. This is a single bit value, and the only value that's ever needed is 0. Why not just use 'active-low' here?

Quoted 6 lines

Quoted 1 line
Just because to have uniform definitions across more syscon nodes.
But what happens if he mask and value don't line up?
mask = 0x10;
value = 0x08;

Same what would happen in other drivers, no?

Only those value bits are take into account which are also sets in the mask.

Quoted 6 lines

If you just state active-low; this kind of mistake is not possible to make.
So I'd rather go for active-low;
Yours,
Linus Walleij

Linus Walleij

2022-07-11 12:21 UTC #6 lore raw

Quoted 14 lines · Pali Rohár

Quoted 13 lines · On Monday 11 July 2022 14:06:50 Linus Walleij wrote:
Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Wednesday 06 July 2022 10:21:11 Rob Herring wrote:
Quoted 3 lines · Pali Rohár
Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value property. This property name is already used by other syscon drivers, e.g. syscon-reboot.
Yes, but those are potentially multi-bit values. This is a single bit value, and the only value that's ever needed is 0. Why not just use 'active-low' here?
Just because to have uniform definitions across more syscon nodes.

Quoted 8 lines

Quoted 4 lines
But what happens if he mask and value don't line up?
mask = 0x10;
value = 0x08;
Same what would happen in other drivers, no?
Only those value bits are take into account which are also sets in the mask.

Two wrongs does not make one right. I.e. just because this bad pattern is used in other syscon drivers we do not need to repeat their mistakes. Also, in this case we can only specify one bit due to the way the enum is designed, other drivers may specify multiple bits.

This also becomes a bit confusing:

+ enum:
+ [ 0x0,

What you do is add 0 at the beginning of the enum but that represent a low bit of any from 0-31. I just get confused, active-low is crystal clear.

Yours,
Linus Walleij