1. Patchew
  2. linux
  3. View series

[PATCH v2] leds: cat9532: support cat9532 in pca955x

Delphine CC Chiu posted 1 patch 9 months, 3 weeks ago 
drivers/leds/leds-pca955x.c | 41 +++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
[PATCH v2] leds: cat9532: support cat9532 in pca955x
Posted by Delphine CC Chiu 9 months, 3 weeks ago 
From: MarshallZhan-wiwynn <marshall_zhan@wiwynn.com>

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

Compare the state at INPUT with the state of LSn and dynamically
adjust how you program LSn

Signed-off-by: MarshallZhan <marshall_zhan@wiwynn.com>
Signed-off-by: Delphine CC Chiu <delphine_cc_chiu@wiwynn.com>
---
 drivers/leds/leds-pca955x.c | 41 +++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 94a9f8a54b35..447da4a2d089 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -4,7 +4,7 @@
  *
  * Author: Nate Case <ncase@xes-inc.com>
  *
- * LED driver for various PCA955x I2C LED drivers
+ * LED driver for various PCA955x and CAT9532 I2C LED drivers
  *
  * Supported devices:
  *
@@ -145,6 +145,11 @@ static inline u8 pca955x_ledsel(u8 oldval, int led_num, int state)
 		((state & 0x3) << (led_num << 1));
 }
 
+static inline int pca955x_ledstate(u8 ls, int led_num)
+{
+	return (ls >> (led_num << 1)) & 0x3;
+}
+
 /*
  * Write to frequency prescaler register, used to program the
  * period of the PWM output.  period = (PSCx + 1) / 38
@@ -235,6 +240,21 @@ 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 input_state;
+
+	input_state= i2c_smbus_read_byte_data(pca955x->client, cmd);
+	if (input_state < 0) {
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", 
+				__func__, led_num, input_state);
+		return input_state;
+	}
+	return (input_state >> (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 +271,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;
+		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 +297,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 input_bit;
 	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 +315,18 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 	if (ret)
 		goto out;
 
+	ls_last_state = pca955x_ledstate(ls, ls_led);
+	input_bit = pca955x_read_input_bit(pca955x, pca955x_led->led_num);
 	switch (value) {
 	case LED_FULL:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
+		ls = pca955x_ledsel(ls, ls_led,
+				(ls_last_state == input_bit) ? 
+				PCA955X_LS_LED_ON : PCA955X_LS_LED_OFF);
 		break;
 	case LED_OFF:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_OFF);
+		ls = pca955x_ledsel(ls, ls_led,
+				(ls_last_state == input_bit) ? 
+				PCA955X_LS_LED_OFF : PCA955X_LS_LED_ON);
 		break;
 	case LED_HALF:
 		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK0);
-- 
2.25.1
Re: [PATCH v2] leds: cat9532: support cat9532 in pca955x
Posted by Lee Jones 9 months, 3 weeks ago 
On Mon, 24 Feb 2025, Delphine CC Chiu wrote:

> From: MarshallZhan-wiwynn <marshall_zhan@wiwynn.com>
> 
> The CAT9532 chips are almost 100% compatible with PCA9552, except that
> the CAT9532 uses the opposite polarity in register that sets on/off.
> 
> Compare the state at INPUT with the state of LSn and dynamically
> adjust how you program LSn
> 
> Signed-off-by: MarshallZhan <marshall_zhan@wiwynn.com>
> Signed-off-by: Delphine CC Chiu <delphine_cc_chiu@wiwynn.com>
> ---
>  drivers/leds/leds-pca955x.c | 41 +++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 94a9f8a54b35..447da4a2d089 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -4,7 +4,7 @@
>   *
>   * Author: Nate Case <ncase@xes-inc.com>
>   *
> - * LED driver for various PCA955x I2C LED drivers
> + * LED driver for various PCA955x and CAT9532 I2C LED drivers
>   *
>   * Supported devices:

You not adding this device to the list of supported devices now?

>   *
> @@ -145,6 +145,11 @@ static inline u8 pca955x_ledsel(u8 oldval, int led_num, int state)
>  		((state & 0x3) << (led_num << 1));
>  }
>  
> +static inline int pca955x_ledstate(u8 ls, int led_num)
> +{
> +	return (ls >> (led_num << 1)) & 0x3;

Please describe what it is you're doing here.

Maybe renaming 'ls' would help.

> +}
> +
>  /*
>   * Write to frequency prescaler register, used to program the
>   * period of the PWM output.  period = (PSCx + 1) / 38
> @@ -235,6 +240,21 @@ 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 input_state;
> +

Comment here.  What information does 'input state' provide?

Why was the 'input state' not required for the PCA9552?

> +	input_state= i2c_smbus_read_byte_data(pca955x->client, cmd);

' ' after input_state.

Doesn't checkpatch.pl usually catch these issues?  Did you run it?

Maybe a function header comment would be better to describe what's
happening here?

> +	if (input_state < 0) {
> +		dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", 
> +				__func__, led_num, input_state);
> +		return input_state;
> +	}
> +	return (input_state >> (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 +271,11 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
>  	ls = (ls >> ((pca955x_led->led_num % 4) << 1)) & 0x3;

Changing all of these magic MOD and AND values to shift / mask defines
would help readability greatly.

>  	switch (ls) {
>  	case PCA955X_LS_LED_ON:
> -		ret = LED_FULL;
> -		break;
>  	case PCA955X_LS_LED_OFF:
> -		ret = LED_OFF;

I requested a comment here in the last version.

> +		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 +297,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 input_bit;
>  	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 +315,18 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
>  	if (ret)
>  		goto out;
>  
> +	ls_last_state = pca955x_ledstate(ls, ls_led);
> +	input_bit = pca955x_read_input_bit(pca955x, pca955x_led->led_num);
>  	switch (value) {
>  	case LED_FULL:
> -		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
> +		ls = pca955x_ledsel(ls, ls_led,
> +				(ls_last_state == input_bit) ? 
> +				PCA955X_LS_LED_ON : PCA955X_LS_LED_OFF);
>  		break;
>  	case LED_OFF:
> -		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_OFF);
> +		ls = pca955x_ledsel(ls, ls_led,
> +				(ls_last_state == input_bit) ? 
> +				PCA955X_LS_LED_OFF : PCA955X_LS_LED_ON);
>  		break;
>  	case LED_HALF:
>  		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK0);
> -- 
> 2.25.1
> 

-- 
Lee Jones [李琼斯]

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.