Re: [PATCH 1/3] leds: pca9532: Use PWM1 for hardware blinking

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi Lee,

Thank you for the feedback, I'll send a V2 iteration that corrects all 
the points you mentioned.

Best regards,
Bastien

On 5/31/24 16:55, Lee Jones wrote:
> On Mon, 27 May 2024, Bastien Curutchet wrote:
> 
>> PSC0/PWM0 are used by all leds for brightness and blinking. This causes
> 
> LEDs everywhere please.
> 
>> 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 | 49 ++++++++++++++++++++++++++++++-------
>>   1 file changed, 40 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
>> index bf8bb8fc007c..c210608ad336 100644
>> --- a/drivers/leds/leds-pca9532.c
>> +++ b/drivers/leds/leds-pca9532.c
>> @@ -191,29 +191,60 @@ 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;
> 
> Don't bunch things up please - new line here.
> 
>> +		if (other->state == PCA9532_PWM1) {
>> +			if (other->ldev.blink_delay_on != delay_on ||
>> +			    other->ldev.blink_delay_off != delay_off) {
>> +				dev_dbg(&led->client->dev,
>> +					"HW can handle only one blink configuration at a time\n");
>> +				return -EINVAL;
>> +			}
>> +		}
>> +	}
>> +
>> +	psc = ((delay_on + delay_off) * 152 - 1) / 1000;
> 
> What are these funny magic numbers?  Define them please.
> 
>> +	if (psc > 255) {
> 
> And this.
> 
>> +		dev_dbg(&led->client->dev, "Blink period too long to be handled by hardware\n");
> 
> If you're returning an error, this should be dev_err().
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	data->psc[1] = psc;
> 
> Can we define these indexes please?
> 
>> +	data->pwm[1] = (delay_on * 255) / (delay_on + delay_off);
>> +
>> +	return pca9532_setpwm(data->client, 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;
>> +	struct pca9532_data *data = i2c_get_clientdata(client);
> 
> Did you build this with W=1?  This looks unused.
> 
>> +	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, 0, psc, led_cdev->brightness);
>> +	led->state = PCA9532_PWM1;
>> +	err = pca9532_update_hw_blink(led, *delay_on, *delay_off);
>>   	if (err)
>>   		return err;
>> -	if (led->state == PCA9532_PWM0)
>> -		pca9532_setpwm(led->client, 0);
>> +
>>   	pca9532_setled(led);
>>   
>>   	return 0;
>> -- 
>> 2.44.0
>>
>