| Message ID | 20220411162033.39613-5-eajames@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: pca955x: Add HW blink support | expand |
Hi Eddie,
url: https://github.com/intel-lab-lkp/linux/commits/Eddie-James/leds-pca955x-Add-HW-blink-support/20220412-002330
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: i386-randconfig-m021-20220411 (https://download.01.org/0day-ci/archive/20220412/202204121953.zHZcX6EV-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/leds/leds-pca955x.c:455 pca955x_led_blink() error: uninitialized symbol 'ret'.
vim +/ret +455 drivers/leds/leds-pca955x.c
3b7b1899f6cc6d Eddie James 2022-04-11 390 static int pca955x_led_blink(struct led_classdev *led_cdev,
3b7b1899f6cc6d Eddie James 2022-04-11 391 unsigned long *delay_on, unsigned long *delay_off)
3b7b1899f6cc6d Eddie James 2022-04-11 392 {
3b7b1899f6cc6d Eddie James 2022-04-11 393 struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev);
3b7b1899f6cc6d Eddie James 2022-04-11 394 struct pca955x *pca955x = pca955x_led->pca955x;
3b7b1899f6cc6d Eddie James 2022-04-11 395 unsigned long p = *delay_on + *delay_off;
3b7b1899f6cc6d Eddie James 2022-04-11 396 int ret;
3b7b1899f6cc6d Eddie James 2022-04-11 397
3b7b1899f6cc6d Eddie James 2022-04-11 398 mutex_lock(&pca955x->lock);
3b7b1899f6cc6d Eddie James 2022-04-11 399
3b7b1899f6cc6d Eddie James 2022-04-11 400 if (p) {
3b7b1899f6cc6d Eddie James 2022-04-11 401 if (*delay_on != *delay_off) {
3b7b1899f6cc6d Eddie James 2022-04-11 402 ret = -EINVAL;
3b7b1899f6cc6d Eddie James 2022-04-11 403 goto out;
3b7b1899f6cc6d Eddie James 2022-04-11 404 }
3b7b1899f6cc6d Eddie James 2022-04-11 405
3b7b1899f6cc6d Eddie James 2022-04-11 406 if (p < pca955x_psc_to_period(pca955x, 0) ||
3b7b1899f6cc6d Eddie James 2022-04-11 407 p > pca955x_psc_to_period(pca955x, 0xff)) {
3b7b1899f6cc6d Eddie James 2022-04-11 408 ret = -EINVAL;
3b7b1899f6cc6d Eddie James 2022-04-11 409 goto out;
3b7b1899f6cc6d Eddie James 2022-04-11 410 }
3b7b1899f6cc6d Eddie James 2022-04-11 411 } else {
3b7b1899f6cc6d Eddie James 2022-04-11 412 p = pca955x->active_blink ? pca955x->blink_period :
3b7b1899f6cc6d Eddie James 2022-04-11 413 PCA955X_BLINK_DEFAULT_MS;
3b7b1899f6cc6d Eddie James 2022-04-11 414 }
3b7b1899f6cc6d Eddie James 2022-04-11 415
3b7b1899f6cc6d Eddie James 2022-04-11 416 if (!pca955x->active_blink ||
3b7b1899f6cc6d Eddie James 2022-04-11 417 pca955x->active_blink == BIT(pca955x_led->led_num) ||
3b7b1899f6cc6d Eddie James 2022-04-11 418 pca955x->blink_period == p) {
3b7b1899f6cc6d Eddie James 2022-04-11 419 u8 psc = pca955x_period_to_psc(pca955x, p);
f46e9203d9a100 Nate Case 2008-07-16 420
3b7b1899f6cc6d Eddie James 2022-04-11 421 if (!test_and_set_bit(pca955x_led->led_num,
3b7b1899f6cc6d Eddie James 2022-04-11 422 &pca955x->active_blink)) {
3b7b1899f6cc6d Eddie James 2022-04-11 423 u8 ls;
3b7b1899f6cc6d Eddie James 2022-04-11 424 int reg = pca955x_led->led_num / 4;
3b7b1899f6cc6d Eddie James 2022-04-11 425 int bit = pca955x_led->led_num % 4;
3b7b1899f6cc6d Eddie James 2022-04-11 426
3b7b1899f6cc6d Eddie James 2022-04-11 427 ret = pca955x_read_ls(pca955x, reg, &ls);
3b7b1899f6cc6d Eddie James 2022-04-11 428 if (ret)
3b7b1899f6cc6d Eddie James 2022-04-11 429 goto out;
3b7b1899f6cc6d Eddie James 2022-04-11 430
3b7b1899f6cc6d Eddie James 2022-04-11 431 ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0);
9e58c2a7bb91f6 Eddie James 2022-04-11 432 ret = pca955x_write_ls(pca955x, reg, ls);
3b7b1899f6cc6d Eddie James 2022-04-11 433 if (ret)
3b7b1899f6cc6d Eddie James 2022-04-11 434 goto out;
3b7b1899f6cc6d Eddie James 2022-04-11 435 }
3b7b1899f6cc6d Eddie James 2022-04-11 436
3b7b1899f6cc6d Eddie James 2022-04-11 437 if (pca955x->blink_period != p) {
3b7b1899f6cc6d Eddie James 2022-04-11 438 pca955x->blink_period = p;
3b7b1899f6cc6d Eddie James 2022-04-11 439 ret = pca955x_write_psc(pca955x, 0, psc);
3b7b1899f6cc6d Eddie James 2022-04-11 440 if (ret)
3b7b1899f6cc6d Eddie James 2022-04-11 441 goto out;
3b7b1899f6cc6d Eddie James 2022-04-11 442 }
Can both the !test_and_set_bit() and pca955x->blink_period != p conditions
be false? If so then "ret" is uninitialized.
3b7b1899f6cc6d Eddie James 2022-04-11 443
3b7b1899f6cc6d Eddie James 2022-04-11 444 p = pca955x_psc_to_period(pca955x, psc);
3b7b1899f6cc6d Eddie James 2022-04-11 445 p /= 2;
3b7b1899f6cc6d Eddie James 2022-04-11 446 *delay_on = p;
3b7b1899f6cc6d Eddie James 2022-04-11 447 *delay_off = p;
3b7b1899f6cc6d Eddie James 2022-04-11 448 } else {
3b7b1899f6cc6d Eddie James 2022-04-11 449 ret = -EBUSY;
3b7b1899f6cc6d Eddie James 2022-04-11 450 }
e7e11d8ba807d4 Alexander Stein 2012-05-29 451
1591caf2d5eafd Cédric Le Goater 2017-08-30 452 out:
e7e11d8ba807d4 Alexander Stein 2012-05-29 453 mutex_unlock(&pca955x->lock);
f46e9203d9a100 Nate Case 2008-07-16 454
1591caf2d5eafd Cédric Le Goater 2017-08-30 @455 return ret;
f46e9203d9a100 Nate Case 2008-07-16 456 }
Hi! > Support blinking using the PCA955x chip. Use PWM0 for blinking > instead of LED_HALF brightness. Since there is only one frequency > and brightness register for any blinking LED, track the blink state > of each LED and only support one HW blinking frequency. If another > frequency is requested, fallback to software blinking. WHat happens to LED_HALF support in this case? Could we only do the accelerated blinking if HALF support is not needed? Best regards, Pavel > --- > drivers/leds/leds-pca955x.c | 222 +++++++++++++++++++++++++++--------- > 1 file changed, 168 insertions(+), 54 deletions(-) > > diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c > index 61f3cb84a945..7c156de215d7 100644 > --- a/drivers/leds/leds-pca955x.c > +++ b/drivers/leds/leds-pca955x.c > @@ -62,6 +62,8 @@ > #define PCA955X_GPIO_HIGH LED_OFF > #define PCA955X_GPIO_LOW LED_FULL > > +#define PCA955X_BLINK_DEFAULT_MS 1000 > + > enum pca955x_type { > pca9550, > pca9551, > @@ -74,6 +76,7 @@ struct pca955x_chipdef { > int bits; > u8 slv_addr; /* 7-bit slave address mask */ > int slv_addr_shift; /* Number of bits to ignore */ > + int blink_div; /* PSC divider */ > }; > > static struct pca955x_chipdef pca955x_chipdefs[] = { > @@ -81,26 +84,31 @@ static struct pca955x_chipdef pca955x_chipdefs[] = { > .bits = 2, > .slv_addr = /* 110000x */ 0x60, > .slv_addr_shift = 1, > + .blink_div = 44, > }, > [pca9551] = { > .bits = 8, > .slv_addr = /* 1100xxx */ 0x60, > .slv_addr_shift = 3, > + .blink_div = 38, > }, > [pca9552] = { > .bits = 16, > .slv_addr = /* 1100xxx */ 0x60, > .slv_addr_shift = 3, > + .blink_div = 44, > }, > [ibm_pca9552] = { > .bits = 16, > .slv_addr = /* 0110xxx */ 0x30, > .slv_addr_shift = 3, > + .blink_div = 44, > }, > [pca9553] = { > .bits = 4, > .slv_addr = /* 110001x */ 0x62, > .slv_addr_shift = 1, > + .blink_div = 44, > }, > }; > > @@ -119,7 +127,9 @@ struct pca955x { > struct pca955x_led *leds; > struct pca955x_chipdef *chipdef; > struct i2c_client *client; > + unsigned long active_blink; > unsigned long active_pins; > + unsigned long blink_period; > #ifdef CONFIG_LEDS_PCA955X_GPIO > struct gpio_chip gpio; > #endif > @@ -170,7 +180,8 @@ static inline int pca955x_ledstate(u8 ls, int led_num) > > /* > * Write to frequency prescaler register, used to program the > - * period of the PWM output. period = (PSCx + 1) / 38 > + * period of the PWM output. period = (PSCx + 1) / coeff > + * Where for pca9551 chips coeff = 38 and for all other chips coeff = 44 > */ > static int pca955x_write_psc(struct pca955x *pca955x, int n, u8 val) > { > @@ -251,6 +262,20 @@ static int pca955x_read_pwm(struct pca955x *pca955x, int n, u8 *val) > return 0; > } > > +static int pca955x_read_psc(struct pca955x *pca955x, int n, u8 *val) > +{ > + u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + (2 * n); > + int ret; > + > + ret = i2c_smbus_read_byte_data(pca955x->client, cmd); > + if (ret < 0) { > + dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, n, ret); > + return ret; > + } > + *val = (u8)ret; > + return 0; > +} > + > static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev) > { > struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev); > @@ -270,7 +295,10 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev) > ret = LED_OFF; > break; > case PCA955X_LS_BLINK0: > - ret = LED_HALF; > + ret = pca955x_read_pwm(pca955x, 0, &pwm); > + if (ret) > + return ret; > + ret = 256 - pwm; > break; > case PCA955X_LS_BLINK1: > ret = pca955x_read_pwm(pca955x, 1, &pwm); > @@ -299,29 +327,36 @@ static int pca955x_led_set(struct led_classdev *led_cdev, > if (ret) > goto out; > > - switch (value) { > - case LED_FULL: > - ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON); > - break; > - case LED_OFF: > - ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); > - break; > - case LED_HALF: > - ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0); > - break; > - default: > - /* > - * Use PWM1 for all other values. This has the unwanted > - * side effect of making all LEDs on the chip share the > - * same brightness level if set to a value other than > - * OFF, HALF, or FULL. But, this is probably better than > - * just turning off for all other values. > - */ > - ret = pca955x_write_pwm(pca955x, 1, 255 - value); > - if (ret) > + if (test_bit(pca955x_led->led_num, &pca955x->active_blink)) { > + if (value == LED_OFF) { > + clear_bit(pca955x_led->led_num, &pca955x->active_blink); > + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); > + } else { > + ret = pca955x_write_pwm(pca955x, 0, 256 - value); > goto out; > - ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1); > - break; > + } > + } else { > + switch (value) { > + case LED_FULL: > + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON); > + break; > + case LED_OFF: > + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); > + break; > + default: > + /* > + * Use PWM1 for all other values. This has the unwanted > + * side effect of making all LEDs on the chip share the > + * same brightness level if set to a value other than > + * OFF or FULL. But, this is probably better than just > + * turning off for all other values. > + */ > + ret = pca955x_write_pwm(pca955x, 1, 255 - value); > + if (ret) > + goto out; > + ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1); > + break; > + } > } > > ret = pca955x_write_ls(pca955x, reg, ls); > @@ -332,6 +367,94 @@ static int pca955x_led_set(struct led_classdev *led_cdev, > return ret; > } > > +static u8 pca955x_period_to_psc(struct pca955x *pca955x, unsigned long p) > +{ > + p *= pca955x->chipdef->blink_div; > + p /= MSEC_PER_SEC; > + p -= 1; > + > + return p; > +} > + > +static unsigned long pca955x_psc_to_period(struct pca955x *pca955x, u8 psc) > +{ > + unsigned long p = psc; > + > + p += 1; > + p *= MSEC_PER_SEC; > + p /= pca955x->chipdef->blink_div; > + > + return p; > +} > + > +static int pca955x_led_blink(struct led_classdev *led_cdev, > + unsigned long *delay_on, unsigned long *delay_off) > +{ > + struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev); > + struct pca955x *pca955x = pca955x_led->pca955x; > + unsigned long p = *delay_on + *delay_off; > + int ret; > + > + mutex_lock(&pca955x->lock); > + > + if (p) { > + if (*delay_on != *delay_off) { > + ret = -EINVAL; > + goto out; > + } > + > + if (p < pca955x_psc_to_period(pca955x, 0) || > + p > pca955x_psc_to_period(pca955x, 0xff)) { > + ret = -EINVAL; > + goto out; > + } > + } else { > + p = pca955x->active_blink ? pca955x->blink_period : > + PCA955X_BLINK_DEFAULT_MS; > + } > + > + if (!pca955x->active_blink || > + pca955x->active_blink == BIT(pca955x_led->led_num) || > + pca955x->blink_period == p) { > + u8 psc = pca955x_period_to_psc(pca955x, p); > + > + if (!test_and_set_bit(pca955x_led->led_num, > + &pca955x->active_blink)) { > + u8 ls; > + int reg = pca955x_led->led_num / 4; > + int bit = pca955x_led->led_num % 4; > + > + ret = pca955x_read_ls(pca955x, reg, &ls); > + if (ret) > + goto out; > + > + ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0); > + ret = pca955x_write_ls(pca955x, reg, ls); > + if (ret) > + goto out; > + } > + > + if (pca955x->blink_period != p) { > + pca955x->blink_period = p; > + ret = pca955x_write_psc(pca955x, 0, psc); > + if (ret) > + goto out; > + } > + > + p = pca955x_psc_to_period(pca955x, psc); > + p /= 2; > + *delay_on = p; > + *delay_off = p; > + } else { > + ret = -EBUSY; > + } > + > +out: > + mutex_unlock(&pca955x->lock); > + > + return ret; > +} > + > #ifdef CONFIG_LEDS_PCA955X_GPIO > /* > * Read the INPUT register, which contains the state of LEDs. > @@ -487,8 +610,9 @@ static int pca955x_probe(struct i2c_client *client) > u8 ls1[4]; > u8 ls2[4]; > struct pca955x_platform_data *pdata; > + u8 psc0; > + bool keep_psc0 = false; > bool set_default_label = false; > - bool keep_pwm = false; > char default_label[8]; > enum pca955x_type chip_type; > const void *md = device_get_match_data(&client->dev); > @@ -552,6 +676,7 @@ static int pca955x_probe(struct i2c_client *client) > mutex_init(&pca955x->lock); > pca955x->client = client; > pca955x->chipdef = chip; > + pca955x->blink_period = PCA955X_BLINK_DEFAULT_MS; > > init_data.devname_mandatory = false; > init_data.devicename = "pca955x"; > @@ -581,15 +706,21 @@ static int pca955x_probe(struct i2c_client *client) > led = &pca955x_led->led_cdev; > led->brightness_set_blocking = pca955x_led_set; > led->brightness_get = pca955x_led_get; > + led->blink_set = pca955x_led_blink; > > if (pdata->leds[i].default_state == > - LEDS_GPIO_DEFSTATE_OFF) > + LEDS_GPIO_DEFSTATE_OFF) { > ls2[reg] = pca955x_ledsel(ls2[reg], bit, > PCA955X_LS_LED_OFF); > - else if (pdata->leds[i].default_state == > - LEDS_GPIO_DEFSTATE_ON) > + } else if (pdata->leds[i].default_state == > + LEDS_GPIO_DEFSTATE_ON) { > ls2[reg] = pca955x_ledsel(ls2[reg], bit, > PCA955X_LS_LED_ON); > + } else if (pca955x_ledstate(ls2[reg], bit) == > + PCA955X_LS_BLINK0) { > + keep_psc0 = true; > + set_bit(i, &pca955x->active_blink); > + } > > init_data.fwnode = pdata->leds[i].fwnode; > > @@ -617,20 +748,6 @@ static int pca955x_probe(struct i2c_client *client) > return err; > > set_bit(i, &pca955x->active_pins); > - > - /* > - * For default-state == "keep", let the core update the > - * brightness from the hardware, then check the > - * brightness to see if it's using PWM1. If so, PWM1 > - * should not be written below. > - */ > - if (pdata->leds[i].default_state == > - LEDS_GPIO_DEFSTATE_KEEP) { > - if (led->brightness != LED_FULL && > - led->brightness != LED_OFF && > - led->brightness != LED_HALF) > - keep_pwm = true; > - } > } > } > > @@ -642,22 +759,19 @@ static int pca955x_probe(struct i2c_client *client) > } > } > > - /* PWM0 is used for half brightness or 50% duty cycle */ > - err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF); > - if (err) > - return err; > - > - if (!keep_pwm) { > - /* PWM1 is used for variable brightness, default to OFF */ > - err = pca955x_write_pwm(pca955x, 1, 0); > - if (err) > - return err; > + if (keep_psc0) { > + err = pca955x_read_psc(pca955x, 0, &psc0); > + } else { > + psc0 = pca955x_period_to_psc(pca955x, pca955x->blink_period); > + err = pca955x_write_psc(pca955x, 0, psc0); > } > > - /* Set to fast frequency so we do not see flashing */ > - err = pca955x_write_psc(pca955x, 0, 0); > if (err) > return err; > + > + pca955x->blink_period = pca955x_psc_to_period(pca955x, psc0); > + > + /* Set PWM1 to fast frequency so we do not see flashing */ > err = pca955x_write_psc(pca955x, 1, 0); > if (err) > return err; > -- > 2.27.0
On 5/4/22 12:24, Pavel Machek wrote: > Hi! > >> Support blinking using the PCA955x chip. Use PWM0 for blinking >> instead of LED_HALF brightness. Since there is only one frequency >> and brightness register for any blinking LED, track the blink state >> of each LED and only support one HW blinking frequency. If another >> frequency is requested, fallback to software blinking. > WHat happens to LED_HALF support in this case? Could we only do the accelerated blinking > if HALF support is not needed? Yes, we lose LED_HALF support in this case. Basically, only full brightness is supported for HW blinking leds. The other channel (non-blinking) can support any brightness, but of course only for 1 led or all leds. Thanks, Eddie > > Best regards, > Pavel > >> --- >> drivers/leds/leds-pca955x.c | 222 +++++++++++++++++++++++++++--------- >> 1 file changed, 168 insertions(+), 54 deletions(-) >> >> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c >> index 61f3cb84a945..7c156de215d7 100644 >> --- a/drivers/leds/leds-pca955x.c >> +++ b/drivers/leds/leds-pca955x.c >> @@ -62,6 +62,8 @@ >> #define PCA955X_GPIO_HIGH LED_OFF >> #define PCA955X_GPIO_LOW LED_FULL >> >> +#define PCA955X_BLINK_DEFAULT_MS 1000 >> + >> enum pca955x_type { >> pca9550, >> pca9551, >> @@ -74,6 +76,7 @@ struct pca955x_chipdef { >> int bits; >> u8 slv_addr; /* 7-bit slave address mask */ >> int slv_addr_shift; /* Number of bits to ignore */ >> + int blink_div; /* PSC divider */ >> }; >> >> static struct pca955x_chipdef pca955x_chipdefs[] = { >> @@ -81,26 +84,31 @@ static struct pca955x_chipdef pca955x_chipdefs[] = { >> .bits = 2, >> .slv_addr = /* 110000x */ 0x60, >> .slv_addr_shift = 1, >> + .blink_div = 44, >> }, >> [pca9551] = { >> .bits = 8, >> .slv_addr = /* 1100xxx */ 0x60, >> .slv_addr_shift = 3, >> + .blink_div = 38, >> }, >> [pca9552] = { >> .bits = 16, >> .slv_addr = /* 1100xxx */ 0x60, >> .slv_addr_shift = 3, >> + .blink_div = 44, >> }, >> [ibm_pca9552] = { >> .bits = 16, >> .slv_addr = /* 0110xxx */ 0x30, >> .slv_addr_shift = 3, >> + .blink_div = 44, >> }, >> [pca9553] = { >> .bits = 4, >> .slv_addr = /* 110001x */ 0x62, >> .slv_addr_shift = 1, >> + .blink_div = 44, >> }, >> }; >> >> @@ -119,7 +127,9 @@ struct pca955x { >> struct pca955x_led *leds; >> struct pca955x_chipdef *chipdef; >> struct i2c_client *client; >> + unsigned long active_blink; >> unsigned long active_pins; >> + unsigned long blink_period; >> #ifdef CONFIG_LEDS_PCA955X_GPIO >> struct gpio_chip gpio; >> #endif >> @@ -170,7 +180,8 @@ static inline int pca955x_ledstate(u8 ls, int led_num) >> >> /* >> * Write to frequency prescaler register, used to program the >> - * period of the PWM output. period = (PSCx + 1) / 38 >> + * period of the PWM output. period = (PSCx + 1) / coeff >> + * Where for pca9551 chips coeff = 38 and for all other chips coeff = 44 >> */ >> static int pca955x_write_psc(struct pca955x *pca955x, int n, u8 val) >> { >> @@ -251,6 +262,20 @@ static int pca955x_read_pwm(struct pca955x *pca955x, int n, u8 *val) >> return 0; >> } >> >> +static int pca955x_read_psc(struct pca955x *pca955x, int n, u8 *val) >> +{ >> + u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + (2 * n); >> + int ret; >> + >> + ret = i2c_smbus_read_byte_data(pca955x->client, cmd); >> + if (ret < 0) { >> + dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, n, ret); >> + return ret; >> + } >> + *val = (u8)ret; >> + return 0; >> +} >> + >> static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev) >> { >> struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev); >> @@ -270,7 +295,10 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev) >> ret = LED_OFF; >> break; >> case PCA955X_LS_BLINK0: >> - ret = LED_HALF; >> + ret = pca955x_read_pwm(pca955x, 0, &pwm); >> + if (ret) >> + return ret; >> + ret = 256 - pwm; >> break; >> case PCA955X_LS_BLINK1: >> ret = pca955x_read_pwm(pca955x, 1, &pwm); >> @@ -299,29 +327,36 @@ static int pca955x_led_set(struct led_classdev *led_cdev, >> if (ret) >> goto out; >> >> - switch (value) { >> - case LED_FULL: >> - ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON); >> - break; >> - case LED_OFF: >> - ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); >> - break; >> - case LED_HALF: >> - ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0); >> - break; >> - default: >> - /* >> - * Use PWM1 for all other values. This has the unwanted >> - * side effect of making all LEDs on the chip share the >> - * same brightness level if set to a value other than >> - * OFF, HALF, or FULL. But, this is probably better than >> - * just turning off for all other values. >> - */ >> - ret = pca955x_write_pwm(pca955x, 1, 255 - value); >> - if (ret) >> + if (test_bit(pca955x_led->led_num, &pca955x->active_blink)) { >> + if (value == LED_OFF) { >> + clear_bit(pca955x_led->led_num, &pca955x->active_blink); >> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); >> + } else { >> + ret = pca955x_write_pwm(pca955x, 0, 256 - value); >> goto out; >> - ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1); >> - break; >> + } >> + } else { >> + switch (value) { >> + case LED_FULL: >> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON); >> + break; >> + case LED_OFF: >> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); >> + break; >> + default: >> + /* >> + * Use PWM1 for all other values. This has the unwanted >> + * side effect of making all LEDs on the chip share the >> + * same brightness level if set to a value other than >> + * OFF or FULL. But, this is probably better than just >> + * turning off for all other values. >> + */ >> + ret = pca955x_write_pwm(pca955x, 1, 255 - value); >> + if (ret) >> + goto out; >> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1); >> + break; >> + } >> } >> >> ret = pca955x_write_ls(pca955x, reg, ls); >> @@ -332,6 +367,94 @@ static int pca955x_led_set(struct led_classdev *led_cdev, >> return ret; >> } >> >> +static u8 pca955x_period_to_psc(struct pca955x *pca955x, unsigned long p) >> +{ >> + p *= pca955x->chipdef->blink_div; >> + p /= MSEC_PER_SEC; >> + p -= 1; >> + >> + return p; >> +} >> + >> +static unsigned long pca955x_psc_to_period(struct pca955x *pca955x, u8 psc) >> +{ >> + unsigned long p = psc; >> + >> + p += 1; >> + p *= MSEC_PER_SEC; >> + p /= pca955x->chipdef->blink_div; >> + >> + return p; >> +} >> + >> +static int pca955x_led_blink(struct led_classdev *led_cdev, >> + unsigned long *delay_on, unsigned long *delay_off) >> +{ >> + struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev); >> + struct pca955x *pca955x = pca955x_led->pca955x; >> + unsigned long p = *delay_on + *delay_off; >> + int ret; >> + >> + mutex_lock(&pca955x->lock); >> + >> + if (p) { >> + if (*delay_on != *delay_off) { >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + if (p < pca955x_psc_to_period(pca955x, 0) || >> + p > pca955x_psc_to_period(pca955x, 0xff)) { >> + ret = -EINVAL; >> + goto out; >> + } >> + } else { >> + p = pca955x->active_blink ? pca955x->blink_period : >> + PCA955X_BLINK_DEFAULT_MS; >> + } >> + >> + if (!pca955x->active_blink || >> + pca955x->active_blink == BIT(pca955x_led->led_num) || >> + pca955x->blink_period == p) { >> + u8 psc = pca955x_period_to_psc(pca955x, p); >> + >> + if (!test_and_set_bit(pca955x_led->led_num, >> + &pca955x->active_blink)) { >> + u8 ls; >> + int reg = pca955x_led->led_num / 4; >> + int bit = pca955x_led->led_num % 4; >> + >> + ret = pca955x_read_ls(pca955x, reg, &ls); >> + if (ret) >> + goto out; >> + >> + ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0); >> + ret = pca955x_write_ls(pca955x, reg, ls); >> + if (ret) >> + goto out; >> + } >> + >> + if (pca955x->blink_period != p) { >> + pca955x->blink_period = p; >> + ret = pca955x_write_psc(pca955x, 0, psc); >> + if (ret) >> + goto out; >> + } >> + >> + p = pca955x_psc_to_period(pca955x, psc); >> + p /= 2; >> + *delay_on = p; >> + *delay_off = p; >> + } else { >> + ret = -EBUSY; >> + } >> + >> +out: >> + mutex_unlock(&pca955x->lock); >> + >> + return ret; >> +} >> + >> #ifdef CONFIG_LEDS_PCA955X_GPIO >> /* >> * Read the INPUT register, which contains the state of LEDs. >> @@ -487,8 +610,9 @@ static int pca955x_probe(struct i2c_client *client) >> u8 ls1[4]; >> u8 ls2[4]; >> struct pca955x_platform_data *pdata; >> + u8 psc0; >> + bool keep_psc0 = false; >> bool set_default_label = false; >> - bool keep_pwm = false; >> char default_label[8]; >> enum pca955x_type chip_type; >> const void *md = device_get_match_data(&client->dev); >> @@ -552,6 +676,7 @@ static int pca955x_probe(struct i2c_client *client) >> mutex_init(&pca955x->lock); >> pca955x->client = client; >> pca955x->chipdef = chip; >> + pca955x->blink_period = PCA955X_BLINK_DEFAULT_MS; >> >> init_data.devname_mandatory = false; >> init_data.devicename = "pca955x"; >> @@ -581,15 +706,21 @@ static int pca955x_probe(struct i2c_client *client) >> led = &pca955x_led->led_cdev; >> led->brightness_set_blocking = pca955x_led_set; >> led->brightness_get = pca955x_led_get; >> + led->blink_set = pca955x_led_blink; >> >> if (pdata->leds[i].default_state == >> - LEDS_GPIO_DEFSTATE_OFF) >> + LEDS_GPIO_DEFSTATE_OFF) { >> ls2[reg] = pca955x_ledsel(ls2[reg], bit, >> PCA955X_LS_LED_OFF); >> - else if (pdata->leds[i].default_state == >> - LEDS_GPIO_DEFSTATE_ON) >> + } else if (pdata->leds[i].default_state == >> + LEDS_GPIO_DEFSTATE_ON) { >> ls2[reg] = pca955x_ledsel(ls2[reg], bit, >> PCA955X_LS_LED_ON); >> + } else if (pca955x_ledstate(ls2[reg], bit) == >> + PCA955X_LS_BLINK0) { >> + keep_psc0 = true; >> + set_bit(i, &pca955x->active_blink); >> + } >> >> init_data.fwnode = pdata->leds[i].fwnode; >> >> @@ -617,20 +748,6 @@ static int pca955x_probe(struct i2c_client *client) >> return err; >> >> set_bit(i, &pca955x->active_pins); >> - >> - /* >> - * For default-state == "keep", let the core update the >> - * brightness from the hardware, then check the >> - * brightness to see if it's using PWM1. If so, PWM1 >> - * should not be written below. >> - */ >> - if (pdata->leds[i].default_state == >> - LEDS_GPIO_DEFSTATE_KEEP) { >> - if (led->brightness != LED_FULL && >> - led->brightness != LED_OFF && >> - led->brightness != LED_HALF) >> - keep_pwm = true; >> - } >> } >> } >> >> @@ -642,22 +759,19 @@ static int pca955x_probe(struct i2c_client *client) >> } >> } >> >> - /* PWM0 is used for half brightness or 50% duty cycle */ >> - err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF); >> - if (err) >> - return err; >> - >> - if (!keep_pwm) { >> - /* PWM1 is used for variable brightness, default to OFF */ >> - err = pca955x_write_pwm(pca955x, 1, 0); >> - if (err) >> - return err; >> + if (keep_psc0) { >> + err = pca955x_read_psc(pca955x, 0, &psc0); >> + } else { >> + psc0 = pca955x_period_to_psc(pca955x, pca955x->blink_period); >> + err = pca955x_write_psc(pca955x, 0, psc0); >> } >> >> - /* Set to fast frequency so we do not see flashing */ >> - err = pca955x_write_psc(pca955x, 0, 0); >> if (err) >> return err; >> + >> + pca955x->blink_period = pca955x_psc_to_period(pca955x, psc0); >> + >> + /* Set PWM1 to fast frequency so we do not see flashing */ >> err = pca955x_write_psc(pca955x, 1, 0); >> if (err) >> return err; >> -- >> 2.27.0
diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c index 61f3cb84a945..7c156de215d7 100644 --- a/drivers/leds/leds-pca955x.c +++ b/drivers/leds/leds-pca955x.c @@ -62,6 +62,8 @@ #define PCA955X_GPIO_HIGH LED_OFF #define PCA955X_GPIO_LOW LED_FULL +#define PCA955X_BLINK_DEFAULT_MS 1000 + enum pca955x_type { pca9550, pca9551, @@ -74,6 +76,7 @@ struct pca955x_chipdef { int bits; u8 slv_addr; /* 7-bit slave address mask */ int slv_addr_shift; /* Number of bits to ignore */ + int blink_div; /* PSC divider */ }; static struct pca955x_chipdef pca955x_chipdefs[] = { @@ -81,26 +84,31 @@ static struct pca955x_chipdef pca955x_chipdefs[] = { .bits = 2, .slv_addr = /* 110000x */ 0x60, .slv_addr_shift = 1, + .blink_div = 44, }, [pca9551] = { .bits = 8, .slv_addr = /* 1100xxx */ 0x60, .slv_addr_shift = 3, + .blink_div = 38, }, [pca9552] = { .bits = 16, .slv_addr = /* 1100xxx */ 0x60, .slv_addr_shift = 3, + .blink_div = 44, }, [ibm_pca9552] = { .bits = 16, .slv_addr = /* 0110xxx */ 0x30, .slv_addr_shift = 3, + .blink_div = 44, }, [pca9553] = { .bits = 4, .slv_addr = /* 110001x */ 0x62, .slv_addr_shift = 1, + .blink_div = 44, }, }; @@ -119,7 +127,9 @@ struct pca955x { struct pca955x_led *leds; struct pca955x_chipdef *chipdef; struct i2c_client *client; + unsigned long active_blink; unsigned long active_pins; + unsigned long blink_period; #ifdef CONFIG_LEDS_PCA955X_GPIO struct gpio_chip gpio; #endif @@ -170,7 +180,8 @@ static inline int pca955x_ledstate(u8 ls, int led_num) /* * Write to frequency prescaler register, used to program the - * period of the PWM output. period = (PSCx + 1) / 38 + * period of the PWM output. period = (PSCx + 1) / coeff + * Where for pca9551 chips coeff = 38 and for all other chips coeff = 44 */ static int pca955x_write_psc(struct pca955x *pca955x, int n, u8 val) { @@ -251,6 +262,20 @@ static int pca955x_read_pwm(struct pca955x *pca955x, int n, u8 *val) return 0; } +static int pca955x_read_psc(struct pca955x *pca955x, int n, u8 *val) +{ + u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + (2 * n); + int ret; + + ret = i2c_smbus_read_byte_data(pca955x->client, cmd); + if (ret < 0) { + dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, n, ret); + return ret; + } + *val = (u8)ret; + return 0; +} + static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev) { struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev); @@ -270,7 +295,10 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev) ret = LED_OFF; break; case PCA955X_LS_BLINK0: - ret = LED_HALF; + ret = pca955x_read_pwm(pca955x, 0, &pwm); + if (ret) + return ret; + ret = 256 - pwm; break; case PCA955X_LS_BLINK1: ret = pca955x_read_pwm(pca955x, 1, &pwm); @@ -299,29 +327,36 @@ static int pca955x_led_set(struct led_classdev *led_cdev, if (ret) goto out; - switch (value) { - case LED_FULL: - ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON); - break; - case LED_OFF: - ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); - break; - case LED_HALF: - ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0); - break; - default: - /* - * Use PWM1 for all other values. This has the unwanted - * side effect of making all LEDs on the chip share the - * same brightness level if set to a value other than - * OFF, HALF, or FULL. But, this is probably better than - * just turning off for all other values. - */ - ret = pca955x_write_pwm(pca955x, 1, 255 - value); - if (ret) + if (test_bit(pca955x_led->led_num, &pca955x->active_blink)) { + if (value == LED_OFF) { + clear_bit(pca955x_led->led_num, &pca955x->active_blink); + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); + } else { + ret = pca955x_write_pwm(pca955x, 0, 256 - value); goto out; - ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1); - break; + } + } else { + switch (value) { + case LED_FULL: + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON); + break; + case LED_OFF: + ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF); + break; + default: + /* + * Use PWM1 for all other values. This has the unwanted + * side effect of making all LEDs on the chip share the + * same brightness level if set to a value other than + * OFF or FULL. But, this is probably better than just + * turning off for all other values. + */ + ret = pca955x_write_pwm(pca955x, 1, 255 - value); + if (ret) + goto out; + ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1); + break; + } } ret = pca955x_write_ls(pca955x, reg, ls); @@ -332,6 +367,94 @@ static int pca955x_led_set(struct led_classdev *led_cdev, return ret; } +static u8 pca955x_period_to_psc(struct pca955x *pca955x, unsigned long p) +{ + p *= pca955x->chipdef->blink_div; + p /= MSEC_PER_SEC; + p -= 1; + + return p; +} + +static unsigned long pca955x_psc_to_period(struct pca955x *pca955x, u8 psc) +{ + unsigned long p = psc; + + p += 1; + p *= MSEC_PER_SEC; + p /= pca955x->chipdef->blink_div; + + return p; +} + +static int pca955x_led_blink(struct led_classdev *led_cdev, + unsigned long *delay_on, unsigned long *delay_off) +{ + struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev); + struct pca955x *pca955x = pca955x_led->pca955x; + unsigned long p = *delay_on + *delay_off; + int ret; + + mutex_lock(&pca955x->lock); + + if (p) { + if (*delay_on != *delay_off) { + ret = -EINVAL; + goto out; + } + + if (p < pca955x_psc_to_period(pca955x, 0) || + p > pca955x_psc_to_period(pca955x, 0xff)) { + ret = -EINVAL; + goto out; + } + } else { + p = pca955x->active_blink ? pca955x->blink_period : + PCA955X_BLINK_DEFAULT_MS; + } + + if (!pca955x->active_blink || + pca955x->active_blink == BIT(pca955x_led->led_num) || + pca955x->blink_period == p) { + u8 psc = pca955x_period_to_psc(pca955x, p); + + if (!test_and_set_bit(pca955x_led->led_num, + &pca955x->active_blink)) { + u8 ls; + int reg = pca955x_led->led_num / 4; + int bit = pca955x_led->led_num % 4; + + ret = pca955x_read_ls(pca955x, reg, &ls); + if (ret) + goto out; + + ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0); + ret = pca955x_write_ls(pca955x, reg, ls); + if (ret) + goto out; + } + + if (pca955x->blink_period != p) { + pca955x->blink_period = p; + ret = pca955x_write_psc(pca955x, 0, psc); + if (ret) + goto out; + } + + p = pca955x_psc_to_period(pca955x, psc); + p /= 2; + *delay_on = p; + *delay_off = p; + } else { + ret = -EBUSY; + } + +out: + mutex_unlock(&pca955x->lock); + + return ret; +} + #ifdef CONFIG_LEDS_PCA955X_GPIO /* * Read the INPUT register, which contains the state of LEDs. @@ -487,8 +610,9 @@ static int pca955x_probe(struct i2c_client *client) u8 ls1[4]; u8 ls2[4]; struct pca955x_platform_data *pdata; + u8 psc0; + bool keep_psc0 = false; bool set_default_label = false; - bool keep_pwm = false; char default_label[8]; enum pca955x_type chip_type; const void *md = device_get_match_data(&client->dev); @@ -552,6 +676,7 @@ static int pca955x_probe(struct i2c_client *client) mutex_init(&pca955x->lock); pca955x->client = client; pca955x->chipdef = chip; + pca955x->blink_period = PCA955X_BLINK_DEFAULT_MS; init_data.devname_mandatory = false; init_data.devicename = "pca955x"; @@ -581,15 +706,21 @@ static int pca955x_probe(struct i2c_client *client) led = &pca955x_led->led_cdev; led->brightness_set_blocking = pca955x_led_set; led->brightness_get = pca955x_led_get; + led->blink_set = pca955x_led_blink; if (pdata->leds[i].default_state == - LEDS_GPIO_DEFSTATE_OFF) + LEDS_GPIO_DEFSTATE_OFF) { ls2[reg] = pca955x_ledsel(ls2[reg], bit, PCA955X_LS_LED_OFF); - else if (pdata->leds[i].default_state == - LEDS_GPIO_DEFSTATE_ON) + } else if (pdata->leds[i].default_state == + LEDS_GPIO_DEFSTATE_ON) { ls2[reg] = pca955x_ledsel(ls2[reg], bit, PCA955X_LS_LED_ON); + } else if (pca955x_ledstate(ls2[reg], bit) == + PCA955X_LS_BLINK0) { + keep_psc0 = true; + set_bit(i, &pca955x->active_blink); + } init_data.fwnode = pdata->leds[i].fwnode; @@ -617,20 +748,6 @@ static int pca955x_probe(struct i2c_client *client) return err; set_bit(i, &pca955x->active_pins); - - /* - * For default-state == "keep", let the core update the - * brightness from the hardware, then check the - * brightness to see if it's using PWM1. If so, PWM1 - * should not be written below. - */ - if (pdata->leds[i].default_state == - LEDS_GPIO_DEFSTATE_KEEP) { - if (led->brightness != LED_FULL && - led->brightness != LED_OFF && - led->brightness != LED_HALF) - keep_pwm = true; - } } } @@ -642,22 +759,19 @@ static int pca955x_probe(struct i2c_client *client) } } - /* PWM0 is used for half brightness or 50% duty cycle */ - err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF); - if (err) - return err; - - if (!keep_pwm) { - /* PWM1 is used for variable brightness, default to OFF */ - err = pca955x_write_pwm(pca955x, 1, 0); - if (err) - return err; + if (keep_psc0) { + err = pca955x_read_psc(pca955x, 0, &psc0); + } else { + psc0 = pca955x_period_to_psc(pca955x, pca955x->blink_period); + err = pca955x_write_psc(pca955x, 0, psc0); } - /* Set to fast frequency so we do not see flashing */ - err = pca955x_write_psc(pca955x, 0, 0); if (err) return err; + + pca955x->blink_period = pca955x_psc_to_period(pca955x, psc0); + + /* Set PWM1 to fast frequency so we do not see flashing */ err = pca955x_write_psc(pca955x, 1, 0); if (err) return err;
Support blinking using the PCA955x chip. Use PWM0 for blinking instead of LED_HALF brightness. Since there is only one frequency and brightness register for any blinking LED, track the blink state of each LED and only support one HW blinking frequency. If another frequency is requested, fallback to software blinking. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/leds/leds-pca955x.c | 222 +++++++++++++++++++++++++++--------- 1 file changed, 168 insertions(+), 54 deletions(-)