On Thu, 30 Jan 2025, Eddie James wrote:
> 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. In addition,
> blinked LEDs can only use full brightness in order to maintain 50%
> duty cycle, which is required for the specified blink rate.
>
> Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> ---
> drivers/leds/leds-pca955x.c | 221 +++++++++++++++++++++++++++---------
> 1 file changed, 169 insertions(+), 52 deletions(-)
Just a couple of nits.
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 8bdebc14ea2e6..6e0e9deecb4a6 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 const struct pca955x_chipdef pca955x_chipdefs[] = {
> @@ -81,26 +84,31 @@ static const 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,
> },
> };
>
> @@ -109,7 +117,9 @@ struct pca955x {
> struct pca955x_led *leds;
> const 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
> @@ -154,7 +164,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)
> {
> @@ -235,6 +246,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);
I'd rather you didn't initialise with a complex function call like this.
Please call that function after the allocation block in this case.
> + 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);
> @@ -248,14 +273,12 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
>
> switch (pca955x_ledstate(ls, pca955x_led->led_num % 4)) {
> case PCA955X_LS_LED_ON:
> + case PCA955X_LS_BLINK0:
> ret = LED_FULL;
> break;
> case PCA955X_LS_LED_OFF:
> ret = LED_OFF;
> break;
> - case PCA955X_LS_BLINK0:
> - ret = LED_HALF;
> - break;
> case PCA955X_LS_BLINK1:
> ret = pca955x_read_pwm(pca955x, 1, &pwm);
> if (ret)
> @@ -283,29 +306,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 {
> + /* No variable brightness for blinking LEDs */
> 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);
> @@ -316,6 +346,102 @@ 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;
These functions could do with a comment or two.
> +
> + 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;
'p' is not a good name for a variable.
If this is 'period' please call it that instead.
> + int ret = 0;
> +
> + 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;
> +
> + /*
> + * Force 50% duty cycle to maintain the specified
> + * blink rate.
> + */
> + ret = pca955x_write_pwm(pca955x, 0, 128);
> + 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.
> @@ -450,8 +576,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];
>
> chip = i2c_get_match_data(client);
> @@ -502,6 +629,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";
> @@ -533,11 +661,16 @@ 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_DEFSTATE_OFF)
> ls2[reg] = pca955x_ledsel(ls2[reg], bit, PCA955X_LS_LED_OFF);
> else if (pdata->leds[i].default_state == LEDS_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;
>
> @@ -565,19 +698,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_DEFSTATE_KEEP) {
> - if (led->brightness != LED_FULL &&
> - led->brightness != LED_OFF &&
> - led->brightness != LED_HALF)
> - keep_pwm = true;
> - }
> }
> }
>
> @@ -589,22 +709,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.43.5
>
--
Lee Jones [李琼斯]
