leds-bcm6328: support second hw blinking interval

PATCH Superseded Álvaro Fernández Rojas 2 participants 14 messages Apr 24, 2020 – Jun 4, 2020 LED

lore

Share & tools

lore

Álvaro Fernández Rojas

author

2020-04-24 12:46 UTC #0 lore raw

PATCH 1/1 leds-bcm6328: support second hw blinking interval

Add support for both configurable HW blinking intervals.

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
 drivers/leds/leds-bcm6328.c | 54 ++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +38 ~~-16~~

index 42e1b7598c3a..beb1aaff68f6 100644
@@ -24,12 +24,16 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
 #define BCM6328_LED_INTERVAL_MS		20
 
 #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,7 +131,8 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);

@@ -176,20 +181,35 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		led->blink_leds[0] |= BIT(led->pin);
+		led->blink_delay[0] = delay;
 
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
+		unsigned long val;
+
+		led->blink_leds[1] |= BIT(led->pin);
+		led->blink_delay[1] = delay;
+
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +378,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.20.1

Álvaro Fernández Rojas

author

2020-04-24 13:32 UTC #1 lore raw

PATCH v2 1/1 leds-bcm6328: support second hw blinking interval

Add support for both configurable HW blinking intervals.

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v2: remove LED from the other interval

 drivers/leds/leds-bcm6328.c | 56 ++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +40 ~~-16~~

index 42e1b7598c3a..a5a57a8d2a1c 100644
@@ -24,12 +24,16 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
 #define BCM6328_LED_INTERVAL_MS		20
 
 #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,7 +131,8 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);

@@ -176,20 +181,37 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		led->blink_leds[0] |= BIT(led->pin);
+		led->blink_leds[1] &= ~BIT(led->pin);
+		led->blink_delay[0] = delay;
 
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
+		unsigned long val;
+
+		led->blink_leds[0] &= ~BIT(led->pin);
+		led->blink_leds[1] |= BIT(led->pin);
+		led->blink_delay[1] = delay;
+
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +380,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.20.1

Pavel Machek

2020-04-25 21:14 UTC #2 lore raw

Hi!

Quoted 1 line

Add support for both configurable HW blinking intervals.

The code could use ... better documentation and better changelog. What is the current blinking interval and what other interval does this add?

Best regards,
Pavel

Quoted 114 lines

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v2: remove LED from the other interval

 drivers/leds/leds-bcm6328.c | 56 ++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +40 ~~-16~~

index 42e1b7598c3a..a5a57a8d2a1c 100644
@@ -24,12 +24,16 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
 #define BCM6328_LED_INTERVAL_MS		20
 
 #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,7 +131,8 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);

@@ -176,20 +181,37 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		led->blink_leds[0] |= BIT(led->pin);
+		led->blink_leds[1] &= ~BIT(led->pin);
+		led->blink_delay[0] = delay;
 
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
+		unsigned long val;
+
+		led->blink_leds[0] &= ~BIT(led->pin);
+		led->blink_leds[1] |= BIT(led->pin);
+		led->blink_delay[1] = delay;
+
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +380,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Álvaro Fernández Rojas

author

2020-04-26 08:40 UTC #3 lore raw

Hi Pavel,

Two different HW blinking intervals can be configured in the controller, but we were only using one of them until now.

These blinking intervals are labeled as FAST_INTV and SLOW_INTV in the sources provided by Broadcom:
https://github.com/jameshilliard/gfiber-gflt100/blob/master/shared/opensource/include/bcm963xx/63268_map_part.h#L1077

However, there are no fast/slow blinking intervals, since the configurable delay range is exactly the same for both of them.

Therefore, a LED can have 4 states (https://github.com/jameshilliard/gfiber-gflt100/blob/master/shared/opensource/include/bcm963xx/63268_map_part.h#L1085): Off, Blinking Interval 1, Blinking Interval 2 and On.

P.S: sorry for the previous emails, but my email client wasn't properly configured... (It should be now :$)

Best regards,
Álvaro.

Quoted 126 lines · Pavel Machek

Hi!

Quoted 1 line

Add support for both configurable HW blinking intervals.

The code could use ... better documentation and better changelog. What is the current blinking interval and what other interval does this add?

Best regards,
Pavel

Quoted 114 lines

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v2: remove LED from the other interval

  drivers/leds/leds-bcm6328.c | 56 ++++++++++++++++++++++++++-----------
  1 file changed, 40 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +40 ~~-16~~

index 42e1b7598c3a..a5a57a8d2a1c 100644
@@ -24,12 +24,16 @@
 
  #define BCM6328_LED_MAX_COUNT		24
  #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
  #define BCM6328_LED_INTERVAL_MS		20
 
  #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
  #define BCM6328_SERIAL_LED_EN		BIT(12)
  #define BCM6328_SERIAL_LED_MUX		BIT(13)
  #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
  #define BCM6328_LED_MODE_MASK		3
  #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
  #define BCM6328_LED_MODE_OFF		3
  #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,7 +131,8 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
  	unsigned long flags;
 
  	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
  	if ((led->active_low && value == LED_OFF) ||
  	    (!led->active_low && value != LED_OFF))
  		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);

@@ -176,20 +181,37 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
  	}
 
  	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
  		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		led->blink_leds[0] |= BIT(led->pin);
+		led->blink_leds[1] &= ~BIT(led->pin);
+		led->blink_delay[0] = delay;
 
  		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
  		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
+		unsigned long val;
+
+		led->blink_leds[0] &= ~BIT(led->pin);
+		led->blink_leds[1] |= BIT(led->pin);
+		led->blink_delay[1] = delay;
+
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
  		rc = 0;
  	} else {
  		dev_dbg(led_cdev->dev,

@@ -358,11 +380,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
  	if (!lock)
  		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
  	if (!blink_leds)
  		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
  	if (!blink_delay)
  		return -ENOMEM;

Álvaro Fernández Rojas

author

2020-05-12 10:01 UTC #4 lore raw

PATCH v3 1/1 leds-bcm6328: support second hw blinking interval

In reply to Álvaro Fernández Rojas

Add support for both configurable HW blinking intervals.

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: add code documentation
v2: remove LED from the other interval

 drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +67 ~~-16~~

index 42e1b7598c3a..050a77591f0b 100644
@@ -24,12 +24,16 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
 #define BCM6328_LED_INTERVAL_MS		20
 
 #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+
+	/* Remove LED from cached HW blinking intervals */
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
+
+	/* Set LED on/off */
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
 	else
 		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+
 	spin_unlock_irqrestore(led->lock, flags);
 }

@@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	/*
+	 * Check if any of the two configurable HW blinking intervals is
+	 * available:
+	 *   1. No LEDs assigned to the HW blinking interval.
+	 *   2. LEDs with the same delay assigned.
+	 */
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
+		unsigned long val;
+
+		/* Add LED to the first HW blinking interval cache */
+		led->blink_leds[0] |= BIT(led->pin);
+
+		/* Remove LED from the second HW blinking interval cache */
+		led->blink_leds[1] &= ~BIT(led->pin);
+
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[0] = delay;
+
+		/* Update the delay for the first HW blinking interval */
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		/* Set the LED to first HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		/* Remove LED from the first HW blinking interval */
+		led->blink_leds[0] &= ~BIT(led->pin);
+
+		/* Add LED to the second HW blinking interval */
+		led->blink_leds[1] |= BIT(led->pin);
 
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[1] = delay;
+
+		/* Update the delay for the second HW blinking interval */
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		/* Set the LED to second HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
+
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.26.2

Pavel Machek

2020-06-04 13:24 UTC #5 lore raw

Quoted 2 lines · Álvaro Fernández Rojas

Add support for both configurable HW blinking intervals.

The code could use ... better documentation and better changelog. What is the current blinking interval and what other interval does this add?

Best regards,
Pavel

Quoted 149 lines

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: add code documentation
v2: remove LED from the other interval

 drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +67 ~~-16~~

index 42e1b7598c3a..050a77591f0b 100644
@@ -24,12 +24,16 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
 #define BCM6328_LED_INTERVAL_MS		20
 
 #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+
+	/* Remove LED from cached HW blinking intervals */
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
+
+	/* Set LED on/off */
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
 	else
 		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+
 	spin_unlock_irqrestore(led->lock, flags);
 }

@@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	/*
+	 * Check if any of the two configurable HW blinking intervals is
+	 * available:
+	 *   1. No LEDs assigned to the HW blinking interval.
+	 *   2. LEDs with the same delay assigned.
+	 */
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
+		unsigned long val;
+
+		/* Add LED to the first HW blinking interval cache */
+		led->blink_leds[0] |= BIT(led->pin);
+
+		/* Remove LED from the second HW blinking interval cache */
+		led->blink_leds[1] &= ~BIT(led->pin);
+
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[0] = delay;
+
+		/* Update the delay for the first HW blinking interval */
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		/* Set the LED to first HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		/* Remove LED from the first HW blinking interval */
+		led->blink_leds[0] &= ~BIT(led->pin);
+
+		/* Add LED to the second HW blinking interval */
+		led->blink_leds[1] |= BIT(led->pin);
 
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[1] = delay;
+
+		/* Update the delay for the second HW blinking interval */
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		/* Set the LED to second HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
+
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.26.2

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Álvaro Fernández Rojas

author

2020-06-04 13:35 UTC #6 lore raw

HI Pavel,

Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. This patch just adds support for the second hw blinking interval.

BTW, I will send v4 which should clarify that each LED can be configured in 4 modes:
0: On
1: HW Blinking (Interval 1)
2: HW Blinking (Interval 2)
3: Off

Until now we were only using 3 modes:
0: On
1: HW Blinking (Interval 1)
3: Off

Best regards,
Álvaro.

Quoted 166 lines · Pavel Machek

Quoted 2 lines · Álvaro Fernández Rojas

Add support for both configurable HW blinking intervals.

The code could use ... better documentation and better changelog. What is the current blinking interval and what other interval does this add?

Best regards,
Pavel

Quoted 149 lines

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: add code documentation
v2: remove LED from the other interval

drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++-------

1 file changed, 67 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +67 ~~-16~~

index 42e1b7598c3a..050a77591f0b 100644
@@ -24,12 +24,16 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
 #define BCM6328_LED_INTERVAL_MS		20
 
 #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+
+	/* Remove LED from cached HW blinking intervals */
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
+
+	/* Set LED on/off */
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
 	else
 		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+
 	spin_unlock_irqrestore(led->lock, flags);
 }

@@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	/*
+	 * Check if any of the two configurable HW blinking intervals is
+	 * available:
+	 *   1. No LEDs assigned to the HW blinking interval.
+	 *   2. LEDs with the same delay assigned.
+	 */
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
+		unsigned long val;
+
+		/* Add LED to the first HW blinking interval cache */
+		led->blink_leds[0] |= BIT(led->pin);
+
+		/* Remove LED from the second HW blinking interval cache */
+		led->blink_leds[1] &= ~BIT(led->pin);
+
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[0] = delay;
+
+		/* Update the delay for the first HW blinking interval */
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		/* Set the LED to first HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		/* Remove LED from the first HW blinking interval */
+		led->blink_leds[0] &= ~BIT(led->pin);
+
+		/* Add LED to the second HW blinking interval */
+		led->blink_leds[1] |= BIT(led->pin);
 
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[1] = delay;
+
+		/* Update the delay for the second HW blinking interval */
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		/* Set the LED to second HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
+
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.26.2

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Pavel Machek

2020-06-04 13:40 UTC #7 lore raw

Quoted 11 lines · Álvaro Fernández Rojas

HI Pavel,
Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. This patch just adds support for the second hw blinking interval.
BTW, I will send v4 which should clarify that each LED can be configured in 4 modes:
0: On
1: HW Blinking (Interval 1)
2: HW Blinking (Interval 2)
3: Off

Quoted 4 lines

Until now we were only using 3 modes:
0: On
1: HW Blinking (Interval 1)
3: Off

So the issue is .... there are like 5 LEDs and they can be on or off or blinking with at most 2 different intervals?
Pavel

Quoted 166 lines

Quoted 166 lines · Pavel Machek

Quoted 2 lines · Álvaro Fernández Rojas

Add support for both configurable HW blinking intervals.

The code could use ... better documentation and better changelog. What is the current blinking interval and what other interval does this add?

Best regards,
Pavel

Quoted 149 lines

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: add code documentation
v2: remove LED from the other interval

drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++-------

1 file changed, 67 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +67 ~~-16~~

index 42e1b7598c3a..050a77591f0b 100644
@@ -24,12 +24,16 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
 #define BCM6328_LED_INTERVAL_MS		20
 
 #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+
+	/* Remove LED from cached HW blinking intervals */
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
+
+	/* Set LED on/off */
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
 	else
 		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+
 	spin_unlock_irqrestore(led->lock, flags);
 }

@@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	/*
+	 * Check if any of the two configurable HW blinking intervals is
+	 * available:
+	 *   1. No LEDs assigned to the HW blinking interval.
+	 *   2. LEDs with the same delay assigned.
+	 */
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
+		unsigned long val;
+
+		/* Add LED to the first HW blinking interval cache */
+		led->blink_leds[0] |= BIT(led->pin);
+
+		/* Remove LED from the second HW blinking interval cache */
+		led->blink_leds[1] &= ~BIT(led->pin);
+
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[0] = delay;
+
+		/* Update the delay for the first HW blinking interval */
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		/* Set the LED to first HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		/* Remove LED from the first HW blinking interval */
+		led->blink_leds[0] &= ~BIT(led->pin);
+
+		/* Add LED to the second HW blinking interval */
+		led->blink_leds[1] |= BIT(led->pin);
 
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[1] = delay;
+
+		/* Update the delay for the second HW blinking interval */
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		/* Set the LED to second HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
+
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.26.2

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Álvaro Fernández Rojas

author

2020-06-04 13:43 UTC #8 lore raw

Hi Pavel,

Quoted 14 lines · Pavel Machek

Quoted 11 lines · Álvaro Fernández Rojas
HI Pavel,
Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. This patch just adds support for the second hw blinking interval.
BTW, I will send v4 which should clarify that each LED can be configured in 4 modes:
0: On
1: HW Blinking (Interval 1)
2: HW Blinking (Interval 2)
3: Off

Quoted 8 lines

Quoted 4 lines
Until now we were only using 3 modes:
0: On
1: HW Blinking (Interval 1)
3: Off
So the issue is .... there are like 5 LEDs and they can be on or off or blinking with at most 2 different intervals?
Pavel

Yes, the controller supports two different blinking delays (intervals), and LEDs can either be on, off or assigned to one of the two blinking delays. However, the current upstream controller only supports one blinking delay. This patch just adds support for both blinking delays instead of just one.

Quoted 171 lines

Quoted 167 lines

Quoted 167 lines · Pavel Machek

Quoted 2 lines · Álvaro Fernández Rojas

Add support for both configurable HW blinking intervals.

The code could use ... better documentation and better changelog. What is the current blinking interval and what other interval does this add?

Best regards,
Pavel

Quoted 149 lines

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: add code documentation
v2: remove LED from the other interval

drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++-------

1 file changed, 67 insertions(+), 16 deletions(-)

drivers/leds/leds-bcm6328.c +67 ~~-16~~

index 42e1b7598c3a..050a77591f0b 100644
@@ -24,12 +24,16 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_NUM	2
 #define BCM6328_LED_INTERVAL_MS		20
 
 #define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_INTV1_SHIFT		0
+#define BCM6328_LED_INTV1_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV1_SHIFT)
+#define BCM6328_LED_INTV2_SHIFT		6
+#define BCM6328_LED_INTV2_MASK		(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_INTV2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +49,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_INTV1		1
+#define BCM6328_LED_MODE_INTV2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+
+	/* Remove LED from cached HW blinking intervals */
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
+
+	/* Set LED on/off */
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
 	else
 		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+
 	spin_unlock_irqrestore(led->lock, flags);
 }

@@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	/*
+	 * Check if any of the two configurable HW blinking intervals is
+	 * available:
+	 *   1. No LEDs assigned to the HW blinking interval.
+	 *   2. LEDs with the same delay assigned.
+	 */
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
+		unsigned long val;
+
+		/* Add LED to the first HW blinking interval cache */
+		led->blink_leds[0] |= BIT(led->pin);
+
+		/* Remove LED from the second HW blinking interval cache */
+		led->blink_leds[1] &= ~BIT(led->pin);
+
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[0] = delay;
+
+		/* Update the delay for the first HW blinking interval */
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_INTV1_MASK;
+		val |= (delay << BCM6328_LED_INTV1_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		/* Set the LED to first HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1);
+
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		/* Remove LED from the first HW blinking interval */
+		led->blink_leds[0] &= ~BIT(led->pin);
+
+		/* Add LED to the second HW blinking interval */
+		led->blink_leds[1] |= BIT(led->pin);
 
+		/* Cache the LED in the first HW blinking interval delay */
+		led->blink_delay[1] = delay;
+
+		/* Update the delay for the second HW blinking interval */
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_INTV2_MASK;
+		val |= (delay << BCM6328_LED_INTV2_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		/* Set the LED to second HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2);
+
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.26.2

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Pavel Machek

2020-06-04 13:53 UTC #9 lore raw

Quoted 21 lines · Álvaro Fernández Rojas

Hi Pavel,
Quoted 19 lines · Pavel Machek
Quoted 15 lines · Álvaro Fernández Rojas
HI Pavel,
Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. This patch just adds support for the second hw blinking interval.
BTW, I will send v4 which should clarify that each LED can be configured in 4 modes:
0: On
1: HW Blinking (Interval 1)
2: HW Blinking (Interval 2)
3: Off
Until now we were only using 3 modes:
0: On
1: HW Blinking (Interval 1)
3: Off

Quoted 7 lines

Quoted 3 lines
So the issue is .... there are like 5 LEDs and they can be on or off or blinking with at most 2 different intervals?
Pavel
Yes, the controller supports two different blinking delays (intervals), and LEDs can either be on, off or assigned to one of the two blinking delays. However, the current upstream controller only supports one blinking delay. This patch just adds support for both blinking delays instead of just one.

Ok. Please put this kind of summary in comment somewhere.

Best regards,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Álvaro Fernández Rojas

author

2020-06-04 14:00 UTC #10 lore raw

Sorry, I had just sent v4 without it.
However, I’ve now sent v5 with the summary.

Best regards,
Álvaro.

Quoted 40 lines · Pavel Machek

Quoted 21 lines · Álvaro Fernández Rojas
Hi Pavel,
Quoted 19 lines · Pavel Machek
Quoted 15 lines · Álvaro Fernández Rojas
HI Pavel,
Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. This patch just adds support for the second hw blinking interval.
BTW, I will send v4 which should clarify that each LED can be configured in 4 modes:
0: On
1: HW Blinking (Interval 1)
2: HW Blinking (Interval 2)
3: Off
Until now we were only using 3 modes:
0: On
1: HW Blinking (Interval 1)
3: Off
Quoted 7 lines
Quoted 3 lines
So the issue is .... there are like 5 LEDs and they can be on or off or blinking with at most 2 different intervals?
Pavel
Yes, the controller supports two different blinking delays (intervals), and LEDs can either be on, off or assigned to one of the two blinking delays. However, the current upstream controller only supports one blinking delay. This patch just adds support for both blinking delays instead of just one.
Ok. Please put this kind of summary in comment somewhere.
Best regards,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Álvaro Fernández Rojas

author

2020-06-04 13:48 UTC #11 lore raw

PATCH v4 1/1 leds-bcm6328: support second hw blinking interval

In reply to Álvaro Fernández Rojas

Add support for both configurable HW blinking intervals.

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v4: improve code documentation and use blink instead of intervals, which
should make the code much more clear to understand
v3: add code documentation
v2: remove LED from the other blinking interval

 drivers/leds/leds-bcm6328.c | 97 ++++++++++++++++++++++++++++---------
 1 file changed, 75 insertions(+), 22 deletions(-)

drivers/leds/leds-bcm6328.c +75 ~~-22~~

index 42e1b7598c3a..bad7efb75112 100644
@@ -24,12 +24,17 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
-#define BCM6328_LED_INTERVAL_MS		20
 
-#define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_BLINK_DELAYS	2
+#define BCM6328_LED_BLINK_MS		20
+
+#define BCM6328_LED_BLINK_MASK		0x3f
+#define BCM6328_LED_BLINK1_SHIFT	0
+#define BCM6328_LED_BLINK1_MASK		(BCM6328_LED_BLINK_MASK << \
+					 BCM6328_LED_BLINK1_SHIFT)
+#define BCM6328_LED_BLINK2_SHIFT	6
+#define BCM6328_LED_BLINK2_MASK		(BCM6328_LED_BLINK_MASK << \
+					 BCM6328_LED_BLINK2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +50,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_BLINK1		1
+#define BCM6328_LED_MODE_BLINK2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,12 +132,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+
+	/* Remove LED from cached HW blinking intervals */
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
+
+	/* Set LED on/off */
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
 	else
 		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+
 	spin_unlock_irqrestore(led->lock, flags);
 }

@@ -140,8 +151,8 @@ static unsigned long bcm6328_blink_delay(unsigned long delay)
 {
 	unsigned long bcm6328_delay;
 
-	bcm6328_delay = delay + BCM6328_LED_INTERVAL_MS / 2;
-	bcm6328_delay = bcm6328_delay / BCM6328_LED_INTERVAL_MS;
+	bcm6328_delay = delay + BCM6328_LED_BLINK_MS / 2;
+	bcm6328_delay = bcm6328_delay / BCM6328_LED_BLINK_MS;
 	if (bcm6328_delay == 0)
 		bcm6328_delay = 1;

@@ -168,28 +179,68 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 		return -EINVAL;
 	}
 
-	if (delay > BCM6328_LED_INTV_MASK) {
+	if (delay > BCM6328_LED_BLINK_MASK) {
 		dev_dbg(led_cdev->dev,
 			"fallback to soft blinking (delay > %ums)\n",
-			BCM6328_LED_INTV_MASK * BCM6328_LED_INTERVAL_MS);
+			BCM6328_LED_BLINK_MASK * BCM6328_LED_BLINK_MS);
 		return -EINVAL;
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	/*
+	 * Check if any of the two configurable HW blinking intervals is
+	 * available:
+	 *   1. No LEDs assigned to the HW blinking interval.
+	 *   2. Only this LED is assigned to the HW blinking interval.
+	 *   3. LEDs with the same delay assigned.
+	 */
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		/* Add LED to the first HW blinking interval cache */
+		led->blink_leds[0] |= BIT(led->pin);
+
+		/* Remove LED from the second HW blinking interval cache */
+		led->blink_leds[1] &= ~BIT(led->pin);
 
+		/* Cache first HW blinking interval delay */
+		led->blink_delay[0] = delay;
+
+		/* Update the delay for the first HW blinking interval */
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_BLINK1_MASK;
+		val |= (delay << BCM6328_LED_BLINK1_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		/* Set the LED to first HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK1);
+
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
+		unsigned long val;
+
+		/* Remove LED from the first HW blinking interval */
+		led->blink_leds[0] &= ~BIT(led->pin);
+
+		/* Add LED to the second HW blinking interval */
+		led->blink_leds[1] |= BIT(led->pin);
+
+		/* Cache second HW blinking interval delay */
+		led->blink_delay[1] = delay;
+
+		/* Update the delay for the second HW blinking interval */
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_BLINK2_MASK;
+		val |= (delay << BCM6328_LED_BLINK2_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		/* Set the LED to second HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK2);
+
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +409,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_BLINK_DELAYS,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_BLINK_DELAYS,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.26.2

Álvaro Fernández Rojas

author

2020-06-04 13:59 UTC #12 lore raw

PATCH v5 1/1 leds-bcm6328: support second hw blinking interval

Right now the driver uses only 3 LED modes:
0: On
1: HW Blinking (Interval 1)
3: Off

However, the controller supports a second HW blinking interval, which results in 4 possible LED modes:
0: On
1: HW Blinking (Interval 1)
2: HW Blinking (Interval 2)
3: Off

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v5: add commit description including LED modes clarification
v4: improve code documentation and use blink instead of intervals, which
should make the code much more clear to understand
v3: add code documentation
v2: remove LED from the other blinking interval

 drivers/leds/leds-bcm6328.c | 97 ++++++++++++++++++++++++++++---------
 1 file changed, 75 insertions(+), 22 deletions(-)

drivers/leds/leds-bcm6328.c +75 ~~-22~~

index 42e1b7598c3a..bad7efb75112 100644
@@ -24,12 +24,17 @@
 
 #define BCM6328_LED_MAX_COUNT		24
 #define BCM6328_LED_DEF_DELAY		500
-#define BCM6328_LED_INTERVAL_MS		20
 
-#define BCM6328_LED_INTV_MASK		0x3f
-#define BCM6328_LED_FAST_INTV_SHIFT	6
-#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
-					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_LED_BLINK_DELAYS	2
+#define BCM6328_LED_BLINK_MS		20
+
+#define BCM6328_LED_BLINK_MASK		0x3f
+#define BCM6328_LED_BLINK1_SHIFT	0
+#define BCM6328_LED_BLINK1_MASK		(BCM6328_LED_BLINK_MASK << \
+					 BCM6328_LED_BLINK1_SHIFT)
+#define BCM6328_LED_BLINK2_SHIFT	6
+#define BCM6328_LED_BLINK2_MASK		(BCM6328_LED_BLINK_MASK << \
+					 BCM6328_LED_BLINK2_SHIFT)
 #define BCM6328_SERIAL_LED_EN		BIT(12)
 #define BCM6328_SERIAL_LED_MUX		BIT(13)
 #define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)

@@ -45,8 +50,8 @@
 
 #define BCM6328_LED_MODE_MASK		3
 #define BCM6328_LED_MODE_ON		0
-#define BCM6328_LED_MODE_FAST		1
-#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_BLINK1		1
+#define BCM6328_LED_MODE_BLINK2		2
 #define BCM6328_LED_MODE_OFF		3
 #define BCM6328_LED_SHIFT(X)		((X) << 1)

@@ -127,12 +132,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(led->lock, flags);
-	*(led->blink_leds) &= ~BIT(led->pin);
+
+	/* Remove LED from cached HW blinking intervals */
+	led->blink_leds[0] &= ~BIT(led->pin);
+	led->blink_leds[1] &= ~BIT(led->pin);
+
+	/* Set LED on/off */
 	if ((led->active_low && value == LED_OFF) ||
 	    (!led->active_low && value != LED_OFF))
 		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
 	else
 		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+
 	spin_unlock_irqrestore(led->lock, flags);
 }

@@ -140,8 +151,8 @@ static unsigned long bcm6328_blink_delay(unsigned long delay)
 {
 	unsigned long bcm6328_delay;
 
-	bcm6328_delay = delay + BCM6328_LED_INTERVAL_MS / 2;
-	bcm6328_delay = bcm6328_delay / BCM6328_LED_INTERVAL_MS;
+	bcm6328_delay = delay + BCM6328_LED_BLINK_MS / 2;
+	bcm6328_delay = bcm6328_delay / BCM6328_LED_BLINK_MS;
 	if (bcm6328_delay == 0)
 		bcm6328_delay = 1;

@@ -168,28 +179,68 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
 		return -EINVAL;
 	}
 
-	if (delay > BCM6328_LED_INTV_MASK) {
+	if (delay > BCM6328_LED_BLINK_MASK) {
 		dev_dbg(led_cdev->dev,
 			"fallback to soft blinking (delay > %ums)\n",
-			BCM6328_LED_INTV_MASK * BCM6328_LED_INTERVAL_MS);
+			BCM6328_LED_BLINK_MASK * BCM6328_LED_BLINK_MS);
 		return -EINVAL;
 	}
 
 	spin_lock_irqsave(led->lock, flags);
-	if (*(led->blink_leds) == 0 ||
-	    *(led->blink_leds) == BIT(led->pin) ||
-	    *(led->blink_delay) == delay) {
+	/*
+	 * Check if any of the two configurable HW blinking intervals is
+	 * available:
+	 *   1. No LEDs assigned to the HW blinking interval.
+	 *   2. Only this LED is assigned to the HW blinking interval.
+	 *   3. LEDs with the same delay assigned.
+	 */
+	if (led->blink_leds[0] == 0 ||
+	    led->blink_leds[0] == BIT(led->pin) ||
+	    led->blink_delay[0] == delay) {
 		unsigned long val;
 
-		*(led->blink_leds) |= BIT(led->pin);
-		*(led->blink_delay) = delay;
+		/* Add LED to the first HW blinking interval cache */
+		led->blink_leds[0] |= BIT(led->pin);
+
+		/* Remove LED from the second HW blinking interval cache */
+		led->blink_leds[1] &= ~BIT(led->pin);
 
+		/* Cache first HW blinking interval delay */
+		led->blink_delay[0] = delay;
+
+		/* Update the delay for the first HW blinking interval */
 		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
-		val &= ~BCM6328_LED_FAST_INTV_MASK;
-		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		val &= ~BCM6328_LED_BLINK1_MASK;
+		val |= (delay << BCM6328_LED_BLINK1_SHIFT);
 		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
 
-		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+		/* Set the LED to first HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK1);
+
+		rc = 0;
+	} else if (led->blink_leds[1] == 0 ||
+		   led->blink_leds[1] == BIT(led->pin) ||
+		   led->blink_delay[1] == delay) {
+		unsigned long val;
+
+		/* Remove LED from the first HW blinking interval */
+		led->blink_leds[0] &= ~BIT(led->pin);
+
+		/* Add LED to the second HW blinking interval */
+		led->blink_leds[1] |= BIT(led->pin);
+
+		/* Cache second HW blinking interval delay */
+		led->blink_delay[1] = delay;
+
+		/* Update the delay for the second HW blinking interval */
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_BLINK2_MASK;
+		val |= (delay << BCM6328_LED_BLINK2_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		/* Set the LED to second HW blinking interval */
+		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK2);
+
 		rc = 0;
 	} else {
 		dev_dbg(led_cdev->dev,

@@ -358,11 +409,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev)
 	if (!lock)
 		return -ENOMEM;
 
-	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	blink_leds = devm_kcalloc(dev, BCM6328_LED_BLINK_DELAYS,
+				  sizeof(*blink_leds), GFP_KERNEL);
 	if (!blink_leds)
 		return -ENOMEM;
 
-	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	blink_delay = devm_kcalloc(dev, BCM6328_LED_BLINK_DELAYS,
+				   sizeof(*blink_delay), GFP_KERNEL);
 	if (!blink_delay)
 		return -ENOMEM;

Signature · 1 line

2.26.2

Pavel Machek

Applied

2020-06-04 14:01 UTC #13 lore raw

Quoted 5 lines · Álvaro Fernández Rojas

Right now the driver uses only 3 LED modes:
0: On
1: HW Blinking (Interval 1)
3: Off

Quoted 8 lines

However, the controller supports a second HW blinking interval, which results in 4 possible LED modes:
0: On
1: HW Blinking (Interval 1)
2: HW Blinking (Interval 2)
3: Off
Signed-off-by Álvaro Fernández Rojas <[email protected]>

Thanks, applied.

Best regards,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html