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

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

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 [李琼斯]

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