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
Bastien Curutchet (3):
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 | 60 ++++++++++++++++++++++++++++++-------
1 file changed, 49 insertions(+), 11 deletions(-)
--
2.44.0
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 <[email protected]>
---
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;
+ 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;
+ if (psc > 255) {
+ dev_dbg(&led->client->dev, "Blink period too long to be handled by hardware\n");
+ return -EINVAL;
+ }
+
+ data->psc[1] = psc;
+ 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);
+ 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
When a led is used to driver 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
driver beepers.
Signed-off-by: Bastien Curutchet <[email protected]>
---
drivers/leds/leds-pca9532.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index c210608ad336..356b71a4b7ac 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -47,6 +47,7 @@ struct pca9532_data {
const struct pca9532_chip_info *chip_info;
u8 pwm[2];
u8 psc[2];
+ bool hw_blink;
};
static int pca9532_probe(struct i2c_client *client);
@@ -234,6 +235,9 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
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;
@@ -390,6 +394,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];
@@ -424,6 +429,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.44.0
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 <[email protected]>
---
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 356b71a4b7ac..5fefcaae7006 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -240,8 +240,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;
}
led->state = PCA9532_PWM1;
--
2.44.0
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 <[email protected]>
> ---
> 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
>
--
Lee Jones [李琼斯]
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 <[email protected]>
>> ---
>> 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
>>
>