[PATCH] leds: pca963x: fix blink with hw acceleration

[PATCH] leds: pca963x: fix blink with hw acceleration

[bernardocrodrigues]

From: Bernardo Rodrigues <bernardocrodrigues@live.com>

LEDs would behave differently depending on the blink hardware
acceleration configuration. This commit will make LEDs respond exactly
the same independently of the hardware acceleration status.

In other words, if you had two pca963x, side by side, one with blink
hardware acceleration "ON" and the other "OFF; and performed some
arbitrary sequence of API calls (e.g. turn on/off, change brightness,
change blink mode, etc.) you probably would end with not matching LED
states.

'pca963x software blink' and 'leds-gpio' behavior were used as
reference.

Actual chip used to validate this change: pca9634

Some of the unmatched behaviors being fixed are (when hw blink was "ON")
    - Leds would stop blinking when the brightness was changed.
    - Leds would persist their blinking mode even after being
      turned off (brightness = 0).
    - Leds would only blink if another led was solid (pca963x will be
      forced out of low power)

Signed-off-by: Bernardo Rodrigues <bernardocrodrigues@live.com>
---
 drivers/leds/leds-pca963x.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
index 00aecd67e348..d8d866bcda19 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -101,6 +101,7 @@ struct pca963x_led {
 	struct pca963x *chip;
 	struct led_classdev led_cdev;
 	int led_num; /* 0 .. 15 potentially */
+	bool blinking;
 	u8 gdc;
 	u8 gfrq;
 };
@@ -129,12 +130,21 @@ static int pca963x_brightness(struct pca963x_led *led,
 
 	switch (brightness) {
 	case LED_FULL:
-		val = (ledout & ~mask) | (PCA963X_LED_ON << shift);
+		if (led->blinking) {
+			val = (ledout & ~mask) | (PCA963X_LED_GRP_PWM << shift);
+			ret = i2c_smbus_write_byte_data(client,
+						PCA963X_PWM_BASE +
+						led->led_num,
+						LED_FULL);
+		} else {
+			val = (ledout & ~mask) | (PCA963X_LED_ON << shift);
+		}
 		ret = i2c_smbus_write_byte_data(client, ledout_addr, val);
 		break;
 	case LED_OFF:
 		val = ledout & ~mask;
 		ret = i2c_smbus_write_byte_data(client, ledout_addr, val);
+		led->blinking = false;
 		break;
 	default:
 		ret = i2c_smbus_write_byte_data(client,
@@ -144,7 +154,11 @@ static int pca963x_brightness(struct pca963x_led *led,
 		if (ret < 0)
 			return ret;
 
-		val = (ledout & ~mask) | (PCA963X_LED_PWM << shift);
+		if (led->blinking)
+			val = (ledout & ~mask) | (PCA963X_LED_GRP_PWM << shift);
+		else
+			val = (ledout & ~mask) | (PCA963X_LED_PWM << shift);
+
 		ret = i2c_smbus_write_byte_data(client, ledout_addr, val);
 		break;
 	}
@@ -181,6 +195,7 @@ static void pca963x_blink(struct pca963x_led *led)
 	}
 
 	mutex_unlock(&led->chip->mutex);
+	led->blinking = true;
 }
 
 static int pca963x_power_state(struct pca963x_led *led)
@@ -275,6 +290,8 @@ static int pca963x_blink_set(struct led_classdev *led_cdev,
 	led->gfrq = gfrq;
 
 	pca963x_blink(led);
+	led->led_cdev.brightness = LED_FULL;
+	pca963x_led_set(led_cdev, LED_FULL);
 
 	*delay_on = time_on;
 	*delay_off = time_off;
@@ -337,6 +354,7 @@ static int pca963x_register_leds(struct i2c_client *client,
 		led->led_cdev.brightness_set_blocking = pca963x_led_set;
 		if (hw_blink)
 			led->led_cdev.blink_set = pca963x_blink_set;
+			led->blinking = false;
 
 		init_data.fwnode = child;
 		/* for backwards compatibility */
-- 
2.32.0

Re: [PATCH] leds: pca963x: fix blink with hw acceleration

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Sun 2021-12-05 18:00:49, bernardocrodrigues@live.com wrote:
> From: Bernardo Rodrigues <bernardocrodrigues@live.com>
> 
> LEDs would behave differently depending on the blink hardware
> acceleration configuration. This commit will make LEDs respond exactly
> the same independently of the hardware acceleration status.
> 
> In other words, if you had two pca963x, side by side, one with blink
> hardware acceleration "ON" and the other "OFF; and performed some
> arbitrary sequence of API calls (e.g. turn on/off, change brightness,
> change blink mode, etc.) you probably would end with not matching LED
> states.
> 
> 'pca963x software blink' and 'leds-gpio' behavior were used as
> reference.
> 
> Actual chip used to validate this change: pca9634

Thanks, applied.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]