Re: [PATCH v1] leds: cat9532: support cat9532 in pca955x

[Lee Jones <lee@kernel.org>]

On Wed, 12 Feb 2025, MarshallZhan-wiwynn wrote:

> The CAT9532 chips are almost 100% compatible with PCA9552, except that
> the CAT9532 used the opposite polarity in register that sets on/off.

"uses"

> Compare the state at INPUT with the state of LSn and dynamically
> adjust how you program LSn
> 
> Signed-off-by: MarshallZhan-wiwynn <marshall_zhan@wiwynn.com>
> ---
>  drivers/leds/leds-pca955x.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 94a9f8a54b35..c5bb81473b6a 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -13,6 +13,7 @@
>   *     PCA9550         2-bit driver            0x60 .. 0x61
>   *     PCA9551         8-bit driver            0x60 .. 0x67
>   *     PCA9552         16-bit driver           0x60 .. 0x67
> + *     CAT9532         16-bit driver           0x60 .. 0x67

Strange placement.  This list looks alphabetical.

>   *     PCA9553/01      4-bit driver            0x62
>   *     PCA9553/02      4-bit driver            0x63
>   *
> @@ -235,6 +236,20 @@ static int pca955x_read_pwm(struct i2c_client *client, int n, u8 *val)
>         return 0;
>  }
> 
> +static int pca955x_read_input_bit(struct pca955x *pca955x, int led_num)
> +{
> +       u8 cmd = led_num / 8;
> +       int ret;
> +
> +       ret = i2c_smbus_read_byte_data(pca955x->client, cmd);

Better to rename 'ret' to something more forthcoming.

> +       if (ret < 0) {
> +               dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, led_num, ret);
> +               return ret;
> +       }

'\n'
> +       return (ret >> (led_num % 8)) & 1;
> +
> +}
> +
>  static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
>  {
>         struct pca955x_led *pca955x_led = container_of(led_cdev,
> @@ -251,10 +266,11 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
>         ls = (ls >> ((pca955x_led->led_num % 4) << 1)) & 0x3;
>         switch (ls) {
>         case PCA955X_LS_LED_ON:
> -               ret = LED_FULL;
> -               break;
>         case PCA955X_LS_LED_OFF:
> -               ret = LED_OFF;

This deserves a comment.

> +               if (pca955x_read_input_bit(pca955x, pca955x_led->led_num))
> +                       ret = LED_FULL;
> +               else
> +                       ret = LED_OFF;
>                 break;
>         case PCA955X_LS_BLINK0:
>                 ret = LED_HALF;
> @@ -276,6 +292,8 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
>         struct pca955x_led *pca955x_led;
>         struct pca955x *pca955x;
>         u8 ls;
> +       u8 ls_last_state;
> +       int inupt_bit;

Did you even compile test this?

>         int chip_ls;    /* which LSx to use (0-3 potentially) */
>         int ls_led;     /* which set of bits within LSx to use (0-3) */
>         int ret;
> @@ -292,12 +310,21 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
>         if (ret)
>                 goto out;
> 
> +       ls_last_state = pca955x_ledstate(ls, bit);
>         switch (value) {
>         case LED_FULL:
> -               ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
> +               input_bit = pca955x_read_input_bit(pca955x, pca955x_led->led_num);
> +               if (ls_last_state == input_bit)
> +                       ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
> +               else
> +                       ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_OFF);
>                 break;
>         case LED_OFF:
> -               ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_OFF);
> +               input_bit = pca955x_read_input_bit(pca955x, pca955x_led->led_num);
> +               if (ls_last_state == input_bit)
> +                       ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_OFF);
> +               else
> +                       ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
>                 break;

These 2 sections are getting pretty messy and providing 6 calls to
pca955x_ledsel() doesn't sit right with me.  Can you think of a better
way to accomplish this?

>         case LED_HALF:
>                 ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK0);
> --
> 2.25.1
> 
> WIWYNN PROPRIETARY
> This email (and any attachments) contains proprietary or confidential information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, please notify the sender and delete this email immediately.

-- 
Lee Jones [李琼斯]