[PATCH v3 0/4] leds: pca9532: Use hardware for blinking LEDs

[PATCH v3 0/4] leds: pca9532: Use hardware for blinking LEDs

[Bastien Curutchet]

Hi all,

This series aims to use hardware more often to blink LEDs.

The pca9532_set_blink() rejects asymmetric delays. So the core's software
fallback is almost always used when we want to blink a LED. Removing
this restriction revealed some conflicts between setting brightness and
blinking as the same PWM (PWM0) configuration is used by all LEDs for
both brightness and blinking.

Make use of the second available PWM (PWM1) to blink LEDs. This PWM1 was
reserved for beepers so hardware blinking is explicitly disabled if at
least one LED is used to drive a beeper to avoid conflicts.

Tested with PCA9532

Changes in v2:
 * Add defines to get rid of magic numbers
 * Replace every 'led' by 'LED'
 * Use dev_err() when returning errors
 * Remove unused struct pca9532_data from patch 2 to add it on patch 3
   where it's actually used

Changes in v3 (in PATCH 2/4):
 * Drop dev_err() messages for comments
 * Replace a -EINVAL with a -EBUSY

[v1] : https://lore.kernel.org/all/20240527125940.155260-1-bastien.curutchet@bootlin.com/
[v2] : https://lore.kernel.org/all/20240617143910.154546-1-bastien.curutchet@bootlin.com/

Bastien Curutchet (4):
  leds: pca9532: Use defines to select PWM instance
  leds: pca9532: Use PWM1 for hardware blinking
  leds: pca9532: Explicitly disable hardware blink when PWM1 is
    unavailable
  leds: pca9532: Change default blinking frequency to 1Hz

 drivers/leds/leds-pca9532.c | 80 ++++++++++++++++++++++++++++---------
 1 file changed, 62 insertions(+), 18 deletions(-)

-- 
2.45.0

[PATCH v3 1/4] leds: pca9532: Use defines to select PWM instance

[Bastien Curutchet]

Two tables are used to configure the PWM and PSC registers of the two
PWM available in the pca9532. Magic numbers are used to access this table
instead of defines.

Add defines PCA9532_PWM_ID_0 and PCA9532_PWM_ID_1 and use them in place of
these magic numbers.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/leds/leds-pca9532.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index bf8bb8fc007c..b6e5f48bffe7 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -45,6 +45,9 @@ struct pca9532_data {
 	struct gpio_chip gpio;
 #endif
 	const struct pca9532_chip_info *chip_info;
+
+#define PCA9532_PWM_ID_0	0
+#define PCA9532_PWM_ID_1	1
 	u8 pwm[2];
 	u8 psc[2];
 };
@@ -181,12 +184,12 @@ static int pca9532_set_brightness(struct led_classdev *led_cdev,
 		led->state = PCA9532_ON;
 	else {
 		led->state = PCA9532_PWM0; /* Thecus: hardcode one pwm */
-		err = pca9532_calcpwm(led->client, 0, 0, value);
+		err = pca9532_calcpwm(led->client, PCA9532_PWM_ID_0, 0, value);
 		if (err)
 			return err;
 	}
 	if (led->state == PCA9532_PWM0)
-		pca9532_setpwm(led->client, 0);
+		pca9532_setpwm(led->client, PCA9532_PWM_ID_0);
 	pca9532_setled(led);
 	return err;
 }
@@ -209,11 +212,11 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
 
 	/* Thecus specific: only use PSC/PWM 0 */
 	psc = (*delay_on * 152-1)/1000;
-	err = pca9532_calcpwm(client, 0, psc, led_cdev->brightness);
+	err = pca9532_calcpwm(client, PCA9532_PWM_ID_0, psc, led_cdev->brightness);
 	if (err)
 		return err;
 	if (led->state == PCA9532_PWM0)
-		pca9532_setpwm(led->client, 0);
+		pca9532_setpwm(led->client, PCA9532_PWM_ID_0);
 	pca9532_setled(led);
 
 	return 0;
@@ -229,9 +232,9 @@ static int pca9532_event(struct input_dev *dev, unsigned int type,
 
 	/* XXX: allow different kind of beeps with psc/pwm modifications */
 	if (value > 1 && value < 32767)
-		data->pwm[1] = 127;
+		data->pwm[PCA9532_PWM_ID_1] = 127;
 	else
-		data->pwm[1] = 0;
+		data->pwm[PCA9532_PWM_ID_1] = 0;
 
 	schedule_work(&data->work);
 
@@ -246,7 +249,7 @@ static void pca9532_input_work(struct work_struct *work)
 
 	mutex_lock(&data->update_lock);
 	i2c_smbus_write_byte_data(data->client, PCA9532_REG_PWM(maxleds, 1),
-		data->pwm[1]);
+		data->pwm[PCA9532_PWM_ID_1]);
 	mutex_unlock(&data->update_lock);
 }
 
@@ -475,9 +478,9 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
 
 	pdata->gpio_base = -1;
 
-	of_property_read_u8_array(np, "nxp,pwm", &pdata->pwm[0],
+	of_property_read_u8_array(np, "nxp,pwm", &pdata->pwm[PCA9532_PWM_ID_0],
 				  ARRAY_SIZE(pdata->pwm));
-	of_property_read_u8_array(np, "nxp,psc", &pdata->psc[0],
+	of_property_read_u8_array(np, "nxp,psc", &pdata->psc[PCA9532_PWM_ID_0],
 				  ARRAY_SIZE(pdata->psc));
 
 	for_each_available_child_of_node(np, child) {
-- 
2.45.0

[PATCH v3 2/4] leds: pca9532: Use PWM1 for hardware blinking

[Bastien Curutchet]

PSC0/PWM0 are used by all LEDs for brightness and blinking. This causes
conflicts when you set a brightness of a new LED while an other one is
already using PWM0 for blinking.

Use PSC1/PWM1 for blinking.
Check that no other LED is already blinking with a different PSC1/PWM1
configuration to avoid conflict.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/leds/leds-pca9532.c | 52 ++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index b6e5f48bffe7..098fd66ca43e 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -29,6 +29,9 @@
 #define LED_SHIFT(led)		(LED_NUM(led) * 2)
 #define LED_MASK(led)		(0x3 << LED_SHIFT(led))
 
+#define PCA9532_PWM_PERIOD_DIV	152
+#define PCA9532_PWM_DUTY_DIV	256
+
 #define ldev_to_led(c)       container_of(c, struct pca9532_led, ldev)
 
 struct pca9532_chip_info {
@@ -194,29 +197,58 @@ static int pca9532_set_brightness(struct led_classdev *led_cdev,
 	return err;
 }
 
+static int pca9532_update_hw_blink(struct pca9532_led *led,
+				   unsigned long delay_on, unsigned long delay_off)
+{
+	struct pca9532_data *data = i2c_get_clientdata(led->client);
+	unsigned int psc;
+	int i;
+
+	/* Look for others LEDs that already use PWM1 */
+	for (i = 0; i < data->chip_info->num_leds; i++) {
+		struct pca9532_led *other = &data->leds[i];
+
+		if (other == led)
+			continue;
+
+		if (other->state == PCA9532_PWM1) {
+			if (other->ldev.blink_delay_on != delay_on ||
+			    other->ldev.blink_delay_off != delay_off) {
+				/* HW can handle only one blink configuration at a time */
+				return -EBUSY;
+			}
+		}
+	}
+
+	psc = ((delay_on + delay_off) * PCA9532_PWM_PERIOD_DIV - 1) / 1000;
+	if (psc > U8_MAX) {
+		/* Blink period too long to be handled by hardware */
+		return -EINVAL;
+	}
+
+	led->state = PCA9532_PWM1;
+	data->psc[PCA9532_PWM_ID_1] = psc;
+	data->pwm[PCA9532_PWM_ID_1] = (delay_on * PCA9532_PWM_DUTY_DIV) / (delay_on + delay_off);
+
+	return pca9532_setpwm(data->client, PCA9532_PWM_ID_1);
+}
+
 static int pca9532_set_blink(struct led_classdev *led_cdev,
 	unsigned long *delay_on, unsigned long *delay_off)
 {
 	struct pca9532_led *led = ldev_to_led(led_cdev);
-	struct i2c_client *client = led->client;
-	int psc;
-	int err = 0;
+	int err;
 
 	if (*delay_on == 0 && *delay_off == 0) {
 		/* led subsystem ask us for a blink rate */
 		*delay_on = 1000;
 		*delay_off = 1000;
 	}
-	if (*delay_on != *delay_off || *delay_on > 1690 || *delay_on < 6)
-		return -EINVAL;
 
-	/* Thecus specific: only use PSC/PWM 0 */
-	psc = (*delay_on * 152-1)/1000;
-	err = pca9532_calcpwm(client, PCA9532_PWM_ID_0, psc, led_cdev->brightness);
+	err = pca9532_update_hw_blink(led, *delay_on, *delay_off);
 	if (err)
 		return err;
-	if (led->state == PCA9532_PWM0)
-		pca9532_setpwm(led->client, PCA9532_PWM_ID_0);
+
 	pca9532_setled(led);
 
 	return 0;
-- 
2.45.0

[PATCH v3 3/4] leds: pca9532: Explicitly disable hardware blink when PWM1 is unavailable

[Bastien Curutchet]

When a LED is used to drive a beeper, it uses PWM1. This can cause
conflicts if an other LED want to use PWM1 for blinking.

Disable use of hardware for blinking when one or more LEDs are used to
drive beepers.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/leds/leds-pca9532.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index 098fd66ca43e..673d4d5b62bb 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -53,6 +53,7 @@ struct pca9532_data {
 #define PCA9532_PWM_ID_1	1
 	u8 pwm[2];
 	u8 psc[2];
+	bool hw_blink;
 };
 
 static int pca9532_probe(struct i2c_client *client);
@@ -237,8 +238,13 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
 	unsigned long *delay_on, unsigned long *delay_off)
 {
 	struct pca9532_led *led = ldev_to_led(led_cdev);
+	struct i2c_client *client = led->client;
+	struct pca9532_data *data = i2c_get_clientdata(client);
 	int err;
 
+	if (!data->hw_blink)
+		return -EINVAL;
+
 	if (*delay_on == 0 && *delay_off == 0) {
 		/* led subsystem ask us for a blink rate */
 		*delay_on = 1000;
@@ -394,6 +400,7 @@ static int pca9532_configure(struct i2c_client *client,
 			data->psc[i]);
 	}
 
+	data->hw_blink = true;
 	for (i = 0; i < data->chip_info->num_leds; i++) {
 		struct pca9532_led *led = &data->leds[i];
 		struct pca9532_led *pled = &pdata->leds[i];
@@ -428,6 +435,8 @@ static int pca9532_configure(struct i2c_client *client,
 			pca9532_setled(led);
 			break;
 		case PCA9532_TYPE_N2100_BEEP:
+			/* PWM1 is reserved for beeper so blink will not use hardware */
+			data->hw_blink = false;
 			BUG_ON(data->idev);
 			led->state = PCA9532_PWM1;
 			pca9532_setled(led);
-- 
2.45.0

[PATCH v3 4/4] leds: pca9532: Change default blinking frequency to 1Hz

[Bastien Curutchet]

Default blinking period is set to 2s. This is too long to be handled by
the hardware (maximum is 1.69s).

Set the default blinking period to 1s to match what is done in the
other LED drivers.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/leds/leds-pca9532.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index 673d4d5b62bb..266f6e5001be 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -247,8 +247,8 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
 
 	if (*delay_on == 0 && *delay_off == 0) {
 		/* led subsystem ask us for a blink rate */
-		*delay_on = 1000;
-		*delay_off = 1000;
+		*delay_on = 500;
+		*delay_off = 500;
 	}
 
 	err = pca9532_update_hw_blink(led, *delay_on, *delay_off);
-- 
2.45.0

Re: [PATCH v3 0/4] leds: pca9532: Use hardware for blinking LEDs

[Lee Jones]

On Thu, 11 Jul 2024, Bastien Curutchet wrote:

> Hi all,
> 
> This series aims to use hardware more often to blink LEDs.
> 
> The pca9532_set_blink() rejects asymmetric delays. So the core's software
> fallback is almost always used when we want to blink a LED. Removing
> this restriction revealed some conflicts between setting brightness and
> blinking as the same PWM (PWM0) configuration is used by all LEDs for
> both brightness and blinking.
> 
> Make use of the second available PWM (PWM1) to blink LEDs. This PWM1 was
> reserved for beepers so hardware blinking is explicitly disabled if at
> least one LED is used to drive a beeper to avoid conflicts.
> 
> Tested with PCA9532
> 
> Changes in v2:
>  * Add defines to get rid of magic numbers
>  * Replace every 'led' by 'LED'
>  * Use dev_err() when returning errors
>  * Remove unused struct pca9532_data from patch 2 to add it on patch 3
>    where it's actually used
> 
> Changes in v3 (in PATCH 2/4):
>  * Drop dev_err() messages for comments
>  * Replace a -EINVAL with a -EBUSY
> 
> [v1] : https://lore.kernel.org/all/20240527125940.155260-1-bastien.curutchet@bootlin.com/
> [v2] : https://lore.kernel.org/all/20240617143910.154546-1-bastien.curutchet@bootlin.com/
> 
> Bastien Curutchet (4):
>   leds: pca9532: Use defines to select PWM instance
>   leds: pca9532: Use PWM1 for hardware blinking
>   leds: pca9532: Explicitly disable hardware blink when PWM1 is
>     unavailable
>   leds: pca9532: Change default blinking frequency to 1Hz

This set was applied on the 20th June.

Please rebase and send follow-ups.

-- 
Lee Jones [李琼斯]