diff mbox series

leds: pca963x: fix blink with hw acceleration

Message ID BY5PR04MB6327FCAC33A75918EA3B65B9A56C9@BY5PR04MB6327.namprd04.prod.outlook.com
State New
Headers show
Series leds: pca963x: fix blink with hw acceleration | expand

Commit Message

bernardocrodrigues@live.com Dec. 5, 2021, 9 p.m. UTC 
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(-)

Comments

Pavel Machek Sept. 21, 2022, 10:51 a.m. UTC | #1 
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
diff mbox series

Patch

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 */