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

[PATCH v2 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

[v1] : https://lore.kernel.org/all/20240527125940.155260-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 | 81 ++++++++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 18 deletions(-)

-- 
2.45.0

[PATCH v2 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 v2 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 | 53 ++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index b6e5f48bffe7..244ae3ff79b5 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,59 @@ 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) {
+				dev_err(&led->client->dev,
+					"HW can handle only one blink configuration at a time\n");
+				return -EINVAL;
+			}
+		}
+	}
+
+	psc = ((delay_on + delay_off) * PCA9532_PWM_PERIOD_DIV - 1) / 1000;
+	if (psc > U8_MAX) {
+		dev_err(&led->client->dev, "Blink period too long to be handled by hardware\n");
+		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

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

[Bastien Curutchet]

Hi Lee,

On 6/17/24 16:39, Bastien Curutchet wrote:

> +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) {
> +				dev_err(&led->client->dev,
> +					"HW can handle only one blink configuration at a time\n");

I'm having some second thoughts about this dev_err().

It was dev_dbg() in V1, but based on your suggestion, I changed it to 
dev_err() because an error was returned after.

I've worked more with this patch since it got applied, these error 
messages appear frequently, though they don’t seem to be 'real' errors 
to me as the software callback is used afterwards and the LED blinks at 
the expected period.

Don't you think a dev_dbg() would be more appropriate in this case ? Or 
perhaps a comment instead of a message ?

> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	psc = ((delay_on + delay_off) * PCA9532_PWM_PERIOD_DIV - 1) / 1000;
> +	if (psc > U8_MAX) {
> +		dev_err(&led->client->dev, "Blink period too long to be handled by hardware\n");

Same comments for this dev_err()

> +		return -EINVAL;
> +	}


Best regards,
Bastien

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

[Lee Jones]

On Wed, 10 Jul 2024, Bastien Curutchet wrote:

> Hi Lee,
> 
> On 6/17/24 16:39, Bastien Curutchet wrote:
> 
> > +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) {
> > +				dev_err(&led->client->dev,
> > +					"HW can handle only one blink configuration at a time\n");
> 
> I'm having some second thoughts about this dev_err().
> 
> It was dev_dbg() in V1, but based on your suggestion, I changed it to
> dev_err() because an error was returned after.
> 
> I've worked more with this patch since it got applied, these error messages
> appear frequently, though they don’t seem to be 'real' errors to me as the
> software callback is used afterwards and the LED blinks at the expected
> period.
> 
> Don't you think a dev_dbg() would be more appropriate in this case ? Or
> perhaps a comment instead of a message ?

If it's not an error, then don't return an error message.

Maybe drop the message for a comment and return -EBUSY instead?

> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> > +
> > +	psc = ((delay_on + delay_off) * PCA9532_PWM_PERIOD_DIV - 1) / 1000;
> > +	if (psc > U8_MAX) {
> > +		dev_err(&led->client->dev, "Blink period too long to be handled by hardware\n");
> 
> Same comments for this dev_err()
> 
> > +		return -EINVAL;
> > +	}
> 
> 
> Best regards,
> Bastien

-- 
Lee Jones [李琼斯]

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

[Bastien Curutchet]

Hi Lee,

On 7/11/24 10:30, Lee Jones wrote:
> On Wed, 10 Jul 2024, Bastien Curutchet wrote:
> 
>> Hi Lee,
>>
>> On 6/17/24 16:39, Bastien Curutchet wrote:
>>
>>> +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) {
>>> +				dev_err(&led->client->dev,
>>> +					"HW can handle only one blink configuration at a time\n");
>>
>> I'm having some second thoughts about this dev_err().
>>
>> It was dev_dbg() in V1, but based on your suggestion, I changed it to
>> dev_err() because an error was returned after.
>>
>> I've worked more with this patch since it got applied, these error messages
>> appear frequently, though they don’t seem to be 'real' errors to me as the
>> software callback is used afterwards and the LED blinks at the expected
>> period.
>>
>> Don't you think a dev_dbg() would be more appropriate in this case ? Or
>> perhaps a comment instead of a message ?
> 
> If it's not an error, then don't return an error message.
> 
> Maybe drop the message for a comment and return -EBUSY instead?
> 

OK I'll do this, thank you.

>>> +				return -EINVAL;
>>> +			}
>>> +		}
>>> +	}
>>> +

Best regards,
Bastien

[PATCH v2 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 244ae3ff79b5..c7a4f677ed4d 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);
@@ -238,8 +239,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;
@@ -395,6 +401,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];
@@ -429,6 +436,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 v2 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 c7a4f677ed4d..9f3fac66a11c 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -248,8 +248,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 v2 0/4] leds: pca9532: Use hardware for blinking LEDs

[Lee Jones]

On Mon, 17 Jun 2024 16:39:06 +0200, Bastien Curutchet wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/4] leds: pca9532: Use defines to select PWM instance
      commit: 8df8c664cd6f1a58201f03849faf06115deaa928
[2/4] leds: pca9532: Use PWM1 for hardware blinking
      commit: 8a8f75fbfde181fbba6d67ba8606fcc4ef7fe5fa
[3/4] leds: pca9532: Explicitly disable hardware blink when PWM1 is unavailable
      commit: 5122d8e29cb64cd8c49bdae61236f68f265b6bb4
[4/4] leds: pca9532: Change default blinking frequency to 1Hz
      commit: cfe05eae52d4d60a53a97ce9d5e6b29bef6bfd60

--
Lee Jones [李琼斯]