All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/4] leds: pca955x: Add HW blink support
@ 2022-04-11 16:20 ` Eddie James
  0 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: linux-kernel, pavel, patrick, andy.shevchenko, openbmc, joel,
	Eddie James

This series adds support for blinking using the PCA955x chip, falling
back to software blinking if another LED on the chip is already blinking
at a different rate, or if the requested rate isn't representable with
the PCA955x.
Also included are some minor clean up and optimization changes that make
the HW blinking a bit easier.

Changes since v2:
 - Split the cleanup patch
 - Prettier dev_err calls
 - Include units for blink period and use defined unit translations
   rather than just a number.
 - Use positive conditionals.

Changes since v1:
 - Rework the blink function to fallback to software blinking if the
   period is out of range of the chip's capabilities or if another LED
   on the chip is already blinking at a different rate.
 - Add the cleanup patch

Eddie James (4):
  leds: pca955x: Refactor with helper functions and renaming
  leds: pca955x: Use pointers to driver data rather than I2C client
  leds: pca955x: Optimize probe led selection
  leds: pca955x: Add HW blink support

 drivers/leds/leds-pca955x.c | 341 ++++++++++++++++++++++++------------
 1 file changed, 232 insertions(+), 109 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 0/4] leds: pca955x: Add HW blink support
@ 2022-04-11 16:20 ` Eddie James
  0 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: openbmc, Eddie James, linux-kernel, andy.shevchenko, joel, pavel

This series adds support for blinking using the PCA955x chip, falling
back to software blinking if another LED on the chip is already blinking
at a different rate, or if the requested rate isn't representable with
the PCA955x.
Also included are some minor clean up and optimization changes that make
the HW blinking a bit easier.

Changes since v2:
 - Split the cleanup patch
 - Prettier dev_err calls
 - Include units for blink period and use defined unit translations
   rather than just a number.
 - Use positive conditionals.

Changes since v1:
 - Rework the blink function to fallback to software blinking if the
   period is out of range of the chip's capabilities or if another LED
   on the chip is already blinking at a different rate.
 - Add the cleanup patch

Eddie James (4):
  leds: pca955x: Refactor with helper functions and renaming
  leds: pca955x: Use pointers to driver data rather than I2C client
  leds: pca955x: Optimize probe led selection
  leds: pca955x: Add HW blink support

 drivers/leds/leds-pca955x.c | 341 ++++++++++++++++++++++++------------
 1 file changed, 232 insertions(+), 109 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 1/4] leds: pca955x: Refactor with helper functions and renaming
  2022-04-11 16:20 ` Eddie James
@ 2022-04-11 16:20   ` Eddie James
  -1 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: linux-kernel, pavel, patrick, andy.shevchenko, openbmc, joel,
	Eddie James

Add helper functions to clean up the code, and rename a few
oddly named functions and variables.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/leds/leds-pca955x.c | 50 ++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 81aaf21212d7..a683c872e1ff 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -134,19 +134,21 @@ struct pca955x_led {
 	struct fwnode_handle	*fwnode;
 };
 
+#define led_to_pca955x(l)	container_of(l, struct pca955x_led, led_cdev)
+
 struct pca955x_platform_data {
 	struct pca955x_led	*leds;
 	int			num_leds;
 };
 
 /* 8 bits per input register */
-static inline int pca95xx_num_input_regs(int bits)
+static inline int pca955x_num_input_regs(int bits)
 {
 	return (bits + 7) / 8;
 }
 
 /* 4 bits per LED selector register */
-static inline int pca95xx_num_led_regs(int bits)
+static inline int pca955x_num_led_regs(int bits)
 {
 	return (bits + 3)  / 4;
 }
@@ -161,6 +163,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
@@ -168,7 +175,7 @@ static inline u8 pca955x_ledsel(u8 oldval, int led_num, int state)
 static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + (2 * n);
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + (2 * n);
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, cmd, val);
@@ -188,7 +195,7 @@ static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
 static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, cmd, val);
@@ -205,7 +212,7 @@ static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
 static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n;
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 4 + n;
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, cmd, val);
@@ -222,7 +229,7 @@ static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
 static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n;
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 4 + n;
 	int ret;
 
 	ret = i2c_smbus_read_byte_data(client, cmd);
@@ -238,7 +245,7 @@ static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
 static int pca955x_read_pwm(struct i2c_client *client, int n, u8 *val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
 	int ret;
 
 	ret = i2c_smbus_read_byte_data(client, cmd);
@@ -253,9 +260,7 @@ static int pca955x_read_pwm(struct i2c_client *client, int n, u8 *val)
 
 static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 {
-	struct pca955x_led *pca955x_led = container_of(led_cdev,
-						       struct pca955x_led,
-						       led_cdev);
+	struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev);
 	struct pca955x *pca955x = pca955x_led->pca955x;
 	u8 ls, pwm;
 	int ret;
@@ -264,8 +269,7 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 	if (ret)
 		return ret;
 
-	ls = (ls >> ((pca955x_led->led_num % 4) << 1)) & 0x3;
-	switch (ls) {
+	switch (pca955x_ledstate(ls, pca955x_led->led_num % 4)) {
 	case PCA955X_LS_LED_ON:
 		ret = LED_FULL;
 		break;
@@ -289,19 +293,13 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 static int pca955x_led_set(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {
-	struct pca955x_led *pca955x_led;
-	struct pca955x *pca955x;
+	struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev);
+	struct pca955x *pca955x = pca955x_led->pca955x;
+	int reg = pca955x_led->led_num / 4;
+	int bit = pca955x_led->led_num % 4;
 	u8 ls;
-	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;
 
-	pca955x_led = container_of(led_cdev, struct pca955x_led, led_cdev);
-	pca955x = pca955x_led->pca955x;
-
-	chip_ls = pca955x_led->led_num / 4;
-	ls_led = pca955x_led->led_num % 4;
-
 	mutex_lock(&pca955x->lock);
 
 	ret = pca955x_read_ls(pca955x->client, chip_ls, &ls);
@@ -310,13 +308,13 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 
 	switch (value) {
 	case LED_FULL:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
+		ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON);
 		break;
 	case LED_OFF:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_OFF);
+		ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF);
 		break;
 	case LED_HALF:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK0);
+		ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0);
 		break;
 	default:
 		/*
@@ -329,7 +327,7 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 		ret = pca955x_write_pwm(pca955x->client, 1, 255 - value);
 		if (ret)
 			goto out;
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK1);
+		ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1);
 		break;
 	}
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 1/4] leds: pca955x: Refactor with helper functions and renaming
@ 2022-04-11 16:20   ` Eddie James
  0 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: openbmc, Eddie James, linux-kernel, andy.shevchenko, joel, pavel

Add helper functions to clean up the code, and rename a few
oddly named functions and variables.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/leds/leds-pca955x.c | 50 ++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 81aaf21212d7..a683c872e1ff 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -134,19 +134,21 @@ struct pca955x_led {
 	struct fwnode_handle	*fwnode;
 };
 
+#define led_to_pca955x(l)	container_of(l, struct pca955x_led, led_cdev)
+
 struct pca955x_platform_data {
 	struct pca955x_led	*leds;
 	int			num_leds;
 };
 
 /* 8 bits per input register */
-static inline int pca95xx_num_input_regs(int bits)
+static inline int pca955x_num_input_regs(int bits)
 {
 	return (bits + 7) / 8;
 }
 
 /* 4 bits per LED selector register */
-static inline int pca95xx_num_led_regs(int bits)
+static inline int pca955x_num_led_regs(int bits)
 {
 	return (bits + 3)  / 4;
 }
@@ -161,6 +163,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
@@ -168,7 +175,7 @@ static inline u8 pca955x_ledsel(u8 oldval, int led_num, int state)
 static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + (2 * n);
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + (2 * n);
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, cmd, val);
@@ -188,7 +195,7 @@ static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
 static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, cmd, val);
@@ -205,7 +212,7 @@ static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
 static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n;
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 4 + n;
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, cmd, val);
@@ -222,7 +229,7 @@ static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
 static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n;
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 4 + n;
 	int ret;
 
 	ret = i2c_smbus_read_byte_data(client, cmd);
@@ -238,7 +245,7 @@ static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
 static int pca955x_read_pwm(struct i2c_client *client, int n, u8 *val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
-	u8 cmd = pca95xx_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
+	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
 	int ret;
 
 	ret = i2c_smbus_read_byte_data(client, cmd);
@@ -253,9 +260,7 @@ static int pca955x_read_pwm(struct i2c_client *client, int n, u8 *val)
 
 static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 {
-	struct pca955x_led *pca955x_led = container_of(led_cdev,
-						       struct pca955x_led,
-						       led_cdev);
+	struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev);
 	struct pca955x *pca955x = pca955x_led->pca955x;
 	u8 ls, pwm;
 	int ret;
@@ -264,8 +269,7 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 	if (ret)
 		return ret;
 
-	ls = (ls >> ((pca955x_led->led_num % 4) << 1)) & 0x3;
-	switch (ls) {
+	switch (pca955x_ledstate(ls, pca955x_led->led_num % 4)) {
 	case PCA955X_LS_LED_ON:
 		ret = LED_FULL;
 		break;
@@ -289,19 +293,13 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 static int pca955x_led_set(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {
-	struct pca955x_led *pca955x_led;
-	struct pca955x *pca955x;
+	struct pca955x_led *pca955x_led = led_to_pca955x(led_cdev);
+	struct pca955x *pca955x = pca955x_led->pca955x;
+	int reg = pca955x_led->led_num / 4;
+	int bit = pca955x_led->led_num % 4;
 	u8 ls;
-	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;
 
-	pca955x_led = container_of(led_cdev, struct pca955x_led, led_cdev);
-	pca955x = pca955x_led->pca955x;
-
-	chip_ls = pca955x_led->led_num / 4;
-	ls_led = pca955x_led->led_num % 4;
-
 	mutex_lock(&pca955x->lock);
 
 	ret = pca955x_read_ls(pca955x->client, chip_ls, &ls);
@@ -310,13 +308,13 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 
 	switch (value) {
 	case LED_FULL:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
+		ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_ON);
 		break;
 	case LED_OFF:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_OFF);
+		ls = pca955x_ledsel(ls, bit, PCA955X_LS_LED_OFF);
 		break;
 	case LED_HALF:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK0);
+		ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK0);
 		break;
 	default:
 		/*
@@ -329,7 +327,7 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 		ret = pca955x_write_pwm(pca955x->client, 1, 255 - value);
 		if (ret)
 			goto out;
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK1);
+		ls = pca955x_ledsel(ls, bit, PCA955X_LS_BLINK1);
 		break;
 	}
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 2/4] leds: pca955x: Use pointers to driver data rather than I2C client
  2022-04-11 16:20 ` Eddie James
@ 2022-04-11 16:20   ` Eddie James
  -1 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: linux-kernel, pavel, patrick, andy.shevchenko, openbmc, joel,
	Eddie James

As a minor clean up item, pass the driver data pointer instead of the
I2C client to the reader and writer helper functions. Now the PCA
driver data doesn't have to be looked up again in the I2C client data

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/leds/leds-pca955x.c | 61 ++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index a683c872e1ff..cb1895b79eab 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -172,16 +172,15 @@ 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
  */
-static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_psc(struct pca955x *pca955x, int n, u8 val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + (2 * n);
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client, cmd, val);
+	ret = i2c_smbus_write_byte_data(pca955x->client, cmd, val);
 	if (ret < 0)
-		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
-			__func__, n, val, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n,
+			val, ret);
 	return ret;
 }
 
@@ -192,16 +191,15 @@ static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
  *
  * Duty cycle is (256 - PWMx) / 256
  */
-static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_pwm(struct pca955x *pca955x, int n, u8 val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client, cmd, val);
+	ret = i2c_smbus_write_byte_data(pca955x->client, cmd, val);
 	if (ret < 0)
-		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
-			__func__, n, val, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n,
+			val, ret);
 	return ret;
 }
 
@@ -209,16 +207,15 @@ static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
  * Write to LED selector register, which determines the source that
  * drives the LED output.
  */
-static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_ls(struct pca955x *pca955x, int n, u8 val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 4 + n;
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client, cmd, val);
+	ret = i2c_smbus_write_byte_data(pca955x->client, cmd, val);
 	if (ret < 0)
-		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
-			__func__, n, val, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n,
+			val, ret);
 	return ret;
 }
 
@@ -226,32 +223,28 @@ static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
  * Read the LED selector register, which determines the source that
  * drives the LED output.
  */
-static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
+static int pca955x_read_ls(struct pca955x *pca955x, int n, u8 *val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 4 + n;
 	int ret;
 
-	ret = i2c_smbus_read_byte_data(client, cmd);
+	ret = i2c_smbus_read_byte_data(pca955x->client, cmd);
 	if (ret < 0) {
-		dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
-			__func__, n, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, n, ret);
 		return ret;
 	}
 	*val = (u8)ret;
 	return 0;
 }
 
-static int pca955x_read_pwm(struct i2c_client *client, int n, u8 *val)
+static int pca955x_read_pwm(struct pca955x *pca955x, int n, u8 *val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
 	int ret;
 
-	ret = i2c_smbus_read_byte_data(client, cmd);
+	ret = i2c_smbus_read_byte_data(pca955x->client, cmd);
 	if (ret < 0) {
-		dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
-			__func__, n, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, n, ret);
 		return ret;
 	}
 	*val = (u8)ret;
@@ -265,7 +258,7 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 	u8 ls, pwm;
 	int ret;
 
-	ret = pca955x_read_ls(pca955x->client, pca955x_led->led_num / 4, &ls);
+	ret = pca955x_read_ls(pca955x, pca955x_led->led_num / 4, &ls);
 	if (ret)
 		return ret;
 
@@ -280,7 +273,7 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 		ret = LED_HALF;
 		break;
 	case PCA955X_LS_BLINK1:
-		ret = pca955x_read_pwm(pca955x->client, 1, &pwm);
+		ret = pca955x_read_pwm(pca955x, 1, &pwm);
 		if (ret)
 			return ret;
 		ret = 255 - pwm;
@@ -302,7 +295,7 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 
 	mutex_lock(&pca955x->lock);
 
-	ret = pca955x_read_ls(pca955x->client, chip_ls, &ls);
+	ret = pca955x_read_ls(pca955x, reg, &ls);
 	if (ret)
 		goto out;
 
@@ -324,14 +317,14 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 		 * OFF, HALF, or FULL.  But, this is probably better than
 		 * just turning off for all other values.
 		 */
-		ret = pca955x_write_pwm(pca955x->client, 1, 255 - value);
+		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->client, chip_ls, ls);
+	ret = pca955x_write_ls(pca955x, reg, ls);
 
 out:
 	mutex_unlock(&pca955x->lock);
@@ -632,22 +625,22 @@ static int pca955x_probe(struct i2c_client *client)
 	}
 
 	/* PWM0 is used for half brightness or 50% duty cycle */
-	err = pca955x_write_pwm(client, 0, 255 - LED_HALF);
+	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(client, 1, 0);
+		err = pca955x_write_pwm(pca955x, 1, 0);
 		if (err)
 			return err;
 	}
 
 	/* Set to fast frequency so we do not see flashing */
-	err = pca955x_write_psc(client, 0, 0);
+	err = pca955x_write_psc(pca955x, 0, 0);
 	if (err)
 		return err;
-	err = pca955x_write_psc(client, 1, 0);
+	err = pca955x_write_psc(pca955x, 1, 0);
 	if (err)
 		return err;
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 2/4] leds: pca955x: Use pointers to driver data rather than I2C client
@ 2022-04-11 16:20   ` Eddie James
  0 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: openbmc, Eddie James, linux-kernel, andy.shevchenko, joel, pavel

As a minor clean up item, pass the driver data pointer instead of the
I2C client to the reader and writer helper functions. Now the PCA
driver data doesn't have to be looked up again in the I2C client data

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/leds/leds-pca955x.c | 61 ++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index a683c872e1ff..cb1895b79eab 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -172,16 +172,15 @@ 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
  */
-static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_psc(struct pca955x *pca955x, int n, u8 val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + (2 * n);
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client, cmd, val);
+	ret = i2c_smbus_write_byte_data(pca955x->client, cmd, val);
 	if (ret < 0)
-		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
-			__func__, n, val, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n,
+			val, ret);
 	return ret;
 }
 
@@ -192,16 +191,15 @@ static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
  *
  * Duty cycle is (256 - PWMx) / 256
  */
-static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_pwm(struct pca955x *pca955x, int n, u8 val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client, cmd, val);
+	ret = i2c_smbus_write_byte_data(pca955x->client, cmd, val);
 	if (ret < 0)
-		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
-			__func__, n, val, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n,
+			val, ret);
 	return ret;
 }
 
@@ -209,16 +207,15 @@ static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
  * Write to LED selector register, which determines the source that
  * drives the LED output.
  */
-static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_ls(struct pca955x *pca955x, int n, u8 val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 4 + n;
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client, cmd, val);
+	ret = i2c_smbus_write_byte_data(pca955x->client, cmd, val);
 	if (ret < 0)
-		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
-			__func__, n, val, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n,
+			val, ret);
 	return ret;
 }
 
@@ -226,32 +223,28 @@ static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
  * Read the LED selector register, which determines the source that
  * drives the LED output.
  */
-static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
+static int pca955x_read_ls(struct pca955x *pca955x, int n, u8 *val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 4 + n;
 	int ret;
 
-	ret = i2c_smbus_read_byte_data(client, cmd);
+	ret = i2c_smbus_read_byte_data(pca955x->client, cmd);
 	if (ret < 0) {
-		dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
-			__func__, n, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, n, ret);
 		return ret;
 	}
 	*val = (u8)ret;
 	return 0;
 }
 
-static int pca955x_read_pwm(struct i2c_client *client, int n, u8 *val)
+static int pca955x_read_pwm(struct pca955x *pca955x, int n, u8 *val)
 {
-	struct pca955x *pca955x = i2c_get_clientdata(client);
 	u8 cmd = pca955x_num_input_regs(pca955x->chipdef->bits) + 1 + (2 * n);
 	int ret;
 
-	ret = i2c_smbus_read_byte_data(client, cmd);
+	ret = i2c_smbus_read_byte_data(pca955x->client, cmd);
 	if (ret < 0) {
-		dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
-			__func__, n, ret);
+		dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", __func__, n, ret);
 		return ret;
 	}
 	*val = (u8)ret;
@@ -265,7 +258,7 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 	u8 ls, pwm;
 	int ret;
 
-	ret = pca955x_read_ls(pca955x->client, pca955x_led->led_num / 4, &ls);
+	ret = pca955x_read_ls(pca955x, pca955x_led->led_num / 4, &ls);
 	if (ret)
 		return ret;
 
@@ -280,7 +273,7 @@ static enum led_brightness pca955x_led_get(struct led_classdev *led_cdev)
 		ret = LED_HALF;
 		break;
 	case PCA955X_LS_BLINK1:
-		ret = pca955x_read_pwm(pca955x->client, 1, &pwm);
+		ret = pca955x_read_pwm(pca955x, 1, &pwm);
 		if (ret)
 			return ret;
 		ret = 255 - pwm;
@@ -302,7 +295,7 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 
 	mutex_lock(&pca955x->lock);
 
-	ret = pca955x_read_ls(pca955x->client, chip_ls, &ls);
+	ret = pca955x_read_ls(pca955x, reg, &ls);
 	if (ret)
 		goto out;
 
@@ -324,14 +317,14 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 		 * OFF, HALF, or FULL.  But, this is probably better than
 		 * just turning off for all other values.
 		 */
-		ret = pca955x_write_pwm(pca955x->client, 1, 255 - value);
+		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->client, chip_ls, ls);
+	ret = pca955x_write_ls(pca955x, reg, ls);
 
 out:
 	mutex_unlock(&pca955x->lock);
@@ -632,22 +625,22 @@ static int pca955x_probe(struct i2c_client *client)
 	}
 
 	/* PWM0 is used for half brightness or 50% duty cycle */
-	err = pca955x_write_pwm(client, 0, 255 - LED_HALF);
+	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(client, 1, 0);
+		err = pca955x_write_pwm(pca955x, 1, 0);
 		if (err)
 			return err;
 	}
 
 	/* Set to fast frequency so we do not see flashing */
-	err = pca955x_write_psc(client, 0, 0);
+	err = pca955x_write_psc(pca955x, 0, 0);
 	if (err)
 		return err;
-	err = pca955x_write_psc(client, 1, 0);
+	err = pca955x_write_psc(pca955x, 1, 0);
 	if (err)
 		return err;
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 3/4] leds: pca955x: Optimize probe led selection
  2022-04-11 16:20 ` Eddie James
@ 2022-04-11 16:20   ` Eddie James
  -1 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: linux-kernel, pavel, patrick, andy.shevchenko, openbmc, joel,
	Eddie James

Previously, the probe function might do up to 32 reads and writes
to the same 4 registers to program the led selection. Reduce this to
a maximum of 8 operations by accumulating the changes to the led
selection and comparing with the previous value to write the
selection if different.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/leds/leds-pca955x.c | 40 +++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index cb1895b79eab..61f3cb84a945 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -483,7 +483,9 @@ static int pca955x_probe(struct i2c_client *client)
 	struct led_classdev *led;
 	struct led_init_data init_data;
 	struct i2c_adapter *adapter;
-	int i, err;
+	int i, bit, err, nls, reg;
+	u8 ls1[4];
+	u8 ls2[4];
 	struct pca955x_platform_data *pdata;
 	bool set_default_label = false;
 	bool keep_pwm = false;
@@ -554,6 +556,15 @@ static int pca955x_probe(struct i2c_client *client)
 	init_data.devname_mandatory = false;
 	init_data.devicename = "pca955x";
 
+	nls = pca955x_num_led_regs(chip->bits);
+	for (i = 0; i < nls; ++i) {
+		err = pca955x_read_ls(pca955x, i, &ls1[i]);
+		if (err)
+			return err;
+
+		ls2[i] = ls1[i];
+	}
+
 	for (i = 0; i < chip->bits; i++) {
 		pca955x_led = &pca955x->leds[i];
 		pca955x_led->led_num = i;
@@ -565,21 +576,20 @@ static int pca955x_probe(struct i2c_client *client)
 		case PCA955X_TYPE_GPIO:
 			break;
 		case PCA955X_TYPE_LED:
+			bit = i % 4;
+			reg = i / 4;
 			led = &pca955x_led->led_cdev;
 			led->brightness_set_blocking = pca955x_led_set;
 			led->brightness_get = pca955x_led_get;
 
 			if (pdata->leds[i].default_state ==
-			    LEDS_GPIO_DEFSTATE_OFF) {
-				err = pca955x_led_set(led, LED_OFF);
-				if (err)
-					return err;
-			} else if (pdata->leds[i].default_state ==
-				   LEDS_GPIO_DEFSTATE_ON) {
-				err = pca955x_led_set(led, LED_FULL);
-				if (err)
-					return err;
-			}
+			    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)
+				ls2[reg] = pca955x_ledsel(ls2[reg], bit,
+							  PCA955X_LS_LED_ON);
 
 			init_data.fwnode = pdata->leds[i].fwnode;
 
@@ -624,6 +634,14 @@ static int pca955x_probe(struct i2c_client *client)
 		}
 	}
 
+	for (i = 0; i < nls; ++i) {
+		if (ls1[i] != ls2[i]) {
+			err = pca955x_write_ls(pca955x, i, ls2[i]);
+			if (err)
+				return err;
+		}
+	}
+
 	/* PWM0 is used for half brightness or 50% duty cycle */
 	err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF);
 	if (err)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 3/4] leds: pca955x: Optimize probe led selection
@ 2022-04-11 16:20   ` Eddie James
  0 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: openbmc, Eddie James, linux-kernel, andy.shevchenko, joel, pavel

Previously, the probe function might do up to 32 reads and writes
to the same 4 registers to program the led selection. Reduce this to
a maximum of 8 operations by accumulating the changes to the led
selection and comparing with the previous value to write the
selection if different.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/leds/leds-pca955x.c | 40 +++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index cb1895b79eab..61f3cb84a945 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -483,7 +483,9 @@ static int pca955x_probe(struct i2c_client *client)
 	struct led_classdev *led;
 	struct led_init_data init_data;
 	struct i2c_adapter *adapter;
-	int i, err;
+	int i, bit, err, nls, reg;
+	u8 ls1[4];
+	u8 ls2[4];
 	struct pca955x_platform_data *pdata;
 	bool set_default_label = false;
 	bool keep_pwm = false;
@@ -554,6 +556,15 @@ static int pca955x_probe(struct i2c_client *client)
 	init_data.devname_mandatory = false;
 	init_data.devicename = "pca955x";
 
+	nls = pca955x_num_led_regs(chip->bits);
+	for (i = 0; i < nls; ++i) {
+		err = pca955x_read_ls(pca955x, i, &ls1[i]);
+		if (err)
+			return err;
+
+		ls2[i] = ls1[i];
+	}
+
 	for (i = 0; i < chip->bits; i++) {
 		pca955x_led = &pca955x->leds[i];
 		pca955x_led->led_num = i;
@@ -565,21 +576,20 @@ static int pca955x_probe(struct i2c_client *client)
 		case PCA955X_TYPE_GPIO:
 			break;
 		case PCA955X_TYPE_LED:
+			bit = i % 4;
+			reg = i / 4;
 			led = &pca955x_led->led_cdev;
 			led->brightness_set_blocking = pca955x_led_set;
 			led->brightness_get = pca955x_led_get;
 
 			if (pdata->leds[i].default_state ==
-			    LEDS_GPIO_DEFSTATE_OFF) {
-				err = pca955x_led_set(led, LED_OFF);
-				if (err)
-					return err;
-			} else if (pdata->leds[i].default_state ==
-				   LEDS_GPIO_DEFSTATE_ON) {
-				err = pca955x_led_set(led, LED_FULL);
-				if (err)
-					return err;
-			}
+			    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)
+				ls2[reg] = pca955x_ledsel(ls2[reg], bit,
+							  PCA955X_LS_LED_ON);
 
 			init_data.fwnode = pdata->leds[i].fwnode;
 
@@ -624,6 +634,14 @@ static int pca955x_probe(struct i2c_client *client)
 		}
 	}
 
+	for (i = 0; i < nls; ++i) {
+		if (ls1[i] != ls2[i]) {
+			err = pca955x_write_ls(pca955x, i, ls2[i]);
+			if (err)
+				return err;
+		}
+	}
+
 	/* PWM0 is used for half brightness or 50% duty cycle */
 	err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF);
 	if (err)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 4/4] leds: pca955x: Add HW blink support
  2022-04-11 16:20 ` Eddie James
@ 2022-04-11 16:20   ` Eddie James
  -1 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: linux-kernel, pavel, patrick, andy.shevchenko, openbmc, joel,
	Eddie James

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(-)

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


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 4/4] leds: pca955x: Add HW blink support
@ 2022-04-11 16:20   ` Eddie James
  0 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-04-11 16:20 UTC (permalink / raw)
  To: linux-leds
  Cc: openbmc, Eddie James, linux-kernel, andy.shevchenko, joel, pavel

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(-)

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


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/4] leds: pca955x: Add HW blink support
  2022-04-11 16:20 ` Eddie James
@ 2022-04-11 17:06   ` Andy Shevchenko
  -1 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2022-04-11 17:06 UTC (permalink / raw)
  To: Eddie James
  Cc: Linux LED Subsystem, Linux Kernel Mailing List, Pavel Machek,
	patrick, OpenBMC Maillist, Joel Stanley

On Mon, Apr 11, 2022 at 7:20 PM Eddie James <eajames@linux.ibm.com> wrote:
>
> This series adds support for blinking using the PCA955x chip, falling
> back to software blinking if another LED on the chip is already blinking
> at a different rate, or if the requested rate isn't representable with
> the PCA955x.
> Also included are some minor clean up and optimization changes that make
> the HW blinking a bit easier.

Don't see any big issues here, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

But please consider two things for the future improvements:
1) switching to regmap API;
2) switching to bitmap API.

You may find some code in drivers/gpio/gpio-pca953x.c. It might be
that some common code can be split to the generic bitmap/regmap
operations.

> Changes since v2:
>  - Split the cleanup patch
>  - Prettier dev_err calls
>  - Include units for blink period and use defined unit translations
>    rather than just a number.
>  - Use positive conditionals.
>
> Changes since v1:
>  - Rework the blink function to fallback to software blinking if the
>    period is out of range of the chip's capabilities or if another LED
>    on the chip is already blinking at a different rate.
>  - Add the cleanup patch
>
> Eddie James (4):
>   leds: pca955x: Refactor with helper functions and renaming
>   leds: pca955x: Use pointers to driver data rather than I2C client
>   leds: pca955x: Optimize probe led selection
>   leds: pca955x: Add HW blink support
>
>  drivers/leds/leds-pca955x.c | 341 ++++++++++++++++++++++++------------
>  1 file changed, 232 insertions(+), 109 deletions(-)
>
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/4] leds: pca955x: Add HW blink support
@ 2022-04-11 17:06   ` Andy Shevchenko
  0 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2022-04-11 17:06 UTC (permalink / raw)
  To: Eddie James
  Cc: OpenBMC Maillist, Linux Kernel Mailing List, Joel Stanley,
	Pavel Machek, Linux LED Subsystem

On Mon, Apr 11, 2022 at 7:20 PM Eddie James <eajames@linux.ibm.com> wrote:
>
> This series adds support for blinking using the PCA955x chip, falling
> back to software blinking if another LED on the chip is already blinking
> at a different rate, or if the requested rate isn't representable with
> the PCA955x.
> Also included are some minor clean up and optimization changes that make
> the HW blinking a bit easier.

Don't see any big issues here, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

But please consider two things for the future improvements:
1) switching to regmap API;
2) switching to bitmap API.

You may find some code in drivers/gpio/gpio-pca953x.c. It might be
that some common code can be split to the generic bitmap/regmap
operations.

> Changes since v2:
>  - Split the cleanup patch
>  - Prettier dev_err calls
>  - Include units for blink period and use defined unit translations
>    rather than just a number.
>  - Use positive conditionals.
>
> Changes since v1:
>  - Rework the blink function to fallback to software blinking if the
>    period is out of range of the chip's capabilities or if another LED
>    on the chip is already blinking at a different rate.
>  - Add the cleanup patch
>
> Eddie James (4):
>   leds: pca955x: Refactor with helper functions and renaming
>   leds: pca955x: Use pointers to driver data rather than I2C client
>   leds: pca955x: Optimize probe led selection
>   leds: pca955x: Add HW blink support
>
>  drivers/leds/leds-pca955x.c | 341 ++++++++++++++++++++++++------------
>  1 file changed, 232 insertions(+), 109 deletions(-)
>
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
  2022-04-11 16:20   ` Eddie James
  (?)
  (?)
@ 2022-04-12 11:33 ` Dan Carpenter
  -1 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2022-04-12 11:22 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 6627 bytes --]

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220411162033.39613-5-eajames@linux.ibm.com>
References: <20220411162033.39613-5-eajames@linux.ibm.com>
TO: Eddie James <eajames@linux.ibm.com>
TO: linux-leds(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org
CC: pavel(a)ucw.cz
CC: patrick(a)stwcx.xyz
CC: andy.shevchenko(a)gmail.com
CC: openbmc(a)lists.ozlabs.org
CC: joel(a)jms.id.au
CC: Eddie James <eajames@linux.ibm.com>

Hi Eddie,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pavel-leds/for-next]
[also build test WARNING on linus/master v5.18-rc2 next-20220412]
[cannot apply to linux/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

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
:::::: branch date: 19 hours ago
:::::: commit date: 19 hours ago
config: i386-randconfig-m021-20220411 (https://download.01.org/0day-ci/archive/20220412/202204121953.zHZcX6EV-lkp(a)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  389  
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  		}
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  }
f46e9203d9a100 Nate Case        2008-07-16  457  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
@ 2022-04-12 11:33 ` Dan Carpenter
  0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2022-04-12 11:33 UTC (permalink / raw)
  To: kbuild, Eddie James, linux-leds
  Cc: lkp, kbuild-all, linux-kernel, pavel, patrick, andy.shevchenko,
	openbmc, joel, Eddie James

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  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
@ 2022-04-12 11:33 ` Dan Carpenter
  0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2022-04-12 11:33 UTC (permalink / raw)
  To: kbuild, Eddie James, linux-leds
  Cc: kbuild-all, lkp, openbmc, Eddie James, linux-kernel,
	andy.shevchenko, joel, pavel

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  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
@ 2022-04-12 11:33 ` Dan Carpenter
  0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2022-04-12 11:33 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 5734 bytes --]

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(a)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  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/4] leds: pca955x: Optimize probe led selection
  2022-04-11 16:20   ` Eddie James
@ 2022-05-04 17:24     ` Pavel Machek
  -1 siblings, 0 replies; 24+ messages in thread
From: Pavel Machek @ 2022-05-04 17:24 UTC (permalink / raw)
  To: Eddie James; +Cc: openbmc, linux-kernel, andy.shevchenko, joel, linux-leds

Hi!

> Previously, the probe function might do up to 32 reads and writes
> to the same 4 registers to program the led selection. Reduce this to
> a maximum of 8 operations by accumulating the changes to the led
> selection and comparing with the previous value to write the
> selection if different.

We have regmap APIs. You are free to use them if you really care about
those few reads. Reimplementing them by hand is not acceptable. How big is 
the seedup here?

Best regards,
								Pavel

> @@ -554,6 +556,15 @@ static int pca955x_probe(struct i2c_client *client)
>  	init_data.devname_mandatory = false;
>  	init_data.devicename = "pca955x";
>  
> +	nls = pca955x_num_led_regs(chip->bits);
> +	for (i = 0; i < nls; ++i) {
> +		err = pca955x_read_ls(pca955x, i, &ls1[i]);
> +		if (err)
> +			return err;
> +
> +		ls2[i] = ls1[i];
> +	}
> +
>  	for (i = 0; i < chip->bits; i++) {
>  		pca955x_led = &pca955x->leds[i];
>  		pca955x_led->led_num = i;
> @@ -624,6 +634,14 @@ static int pca955x_probe(struct i2c_client *client)
>  		}
>  	}
>  
> +	for (i = 0; i < nls; ++i) {
> +		if (ls1[i] != ls2[i]) {
> +			err = pca955x_write_ls(pca955x, i, ls2[i]);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
>  	/* PWM0 is used for half brightness or 50% duty cycle */
>  	err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF);
>  	if (err)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/4] leds: pca955x: Optimize probe led selection
@ 2022-05-04 17:24     ` Pavel Machek
  0 siblings, 0 replies; 24+ messages in thread
From: Pavel Machek @ 2022-05-04 17:24 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-leds, linux-kernel, patrick, andy.shevchenko, openbmc, joel

Hi!

> Previously, the probe function might do up to 32 reads and writes
> to the same 4 registers to program the led selection. Reduce this to
> a maximum of 8 operations by accumulating the changes to the led
> selection and comparing with the previous value to write the
> selection if different.

We have regmap APIs. You are free to use them if you really care about
those few reads. Reimplementing them by hand is not acceptable. How big is 
the seedup here?

Best regards,
								Pavel

> @@ -554,6 +556,15 @@ static int pca955x_probe(struct i2c_client *client)
>  	init_data.devname_mandatory = false;
>  	init_data.devicename = "pca955x";
>  
> +	nls = pca955x_num_led_regs(chip->bits);
> +	for (i = 0; i < nls; ++i) {
> +		err = pca955x_read_ls(pca955x, i, &ls1[i]);
> +		if (err)
> +			return err;
> +
> +		ls2[i] = ls1[i];
> +	}
> +
>  	for (i = 0; i < chip->bits; i++) {
>  		pca955x_led = &pca955x->leds[i];
>  		pca955x_led->led_num = i;
> @@ -624,6 +634,14 @@ static int pca955x_probe(struct i2c_client *client)
>  		}
>  	}
>  
> +	for (i = 0; i < nls; ++i) {
> +		if (ls1[i] != ls2[i]) {
> +			err = pca955x_write_ls(pca955x, i, ls2[i]);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
>  	/* PWM0 is used for half brightness or 50% duty cycle */
>  	err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF);
>  	if (err)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
  2022-04-11 16:20   ` Eddie James
@ 2022-05-04 17:24     ` Pavel Machek
  -1 siblings, 0 replies; 24+ messages in thread
From: Pavel Machek @ 2022-05-04 17:24 UTC (permalink / raw)
  To: Eddie James; +Cc: openbmc, linux-kernel, andy.shevchenko, joel, linux-leds

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
@ 2022-05-04 17:24     ` Pavel Machek
  0 siblings, 0 replies; 24+ messages in thread
From: Pavel Machek @ 2022-05-04 17:24 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-leds, linux-kernel, patrick, andy.shevchenko, openbmc, joel

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/4] leds: pca955x: Optimize probe led selection
  2022-05-04 17:24     ` Pavel Machek
@ 2022-05-11 19:43       ` Eddie James
  -1 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-05-11 19:43 UTC (permalink / raw)
  To: Pavel Machek
  Cc: linux-leds, linux-kernel, patrick, andy.shevchenko, openbmc, joel


On 5/4/22 12:24, Pavel Machek wrote:
> Hi!
>
>> Previously, the probe function might do up to 32 reads and writes
>> to the same 4 registers to program the led selection. Reduce this to
>> a maximum of 8 operations by accumulating the changes to the led
>> selection and comparing with the previous value to write the
>> selection if different.
> We have regmap APIs. You are free to use them if you really care about
> those few reads. Reimplementing them by hand is not acceptable. How big is
> the seedup here?


Hi,

To be honest this was not about speed, it's about a problem I've 
observed where many operations in quick succession caused the PCA chip 
to NAK the i2c transfer. I'm not sure of the root cause yet. So reducing 
the number of operations helped. We are also probably going to carry a 
throttling patch, unless I can prove the PCA chip itself is at fault and 
needs slower transfers.

I can try the regmap API and see if it helps though.

Thanks,

Eddie


>
> Best regards,
> 								Pavel
>
>> @@ -554,6 +556,15 @@ static int pca955x_probe(struct i2c_client *client)
>>   	init_data.devname_mandatory = false;
>>   	init_data.devicename = "pca955x";
>>   
>> +	nls = pca955x_num_led_regs(chip->bits);
>> +	for (i = 0; i < nls; ++i) {
>> +		err = pca955x_read_ls(pca955x, i, &ls1[i]);
>> +		if (err)
>> +			return err;
>> +
>> +		ls2[i] = ls1[i];
>> +	}
>> +
>>   	for (i = 0; i < chip->bits; i++) {
>>   		pca955x_led = &pca955x->leds[i];
>>   		pca955x_led->led_num = i;
>> @@ -624,6 +634,14 @@ static int pca955x_probe(struct i2c_client *client)
>>   		}
>>   	}
>>   
>> +	for (i = 0; i < nls; ++i) {
>> +		if (ls1[i] != ls2[i]) {
>> +			err = pca955x_write_ls(pca955x, i, ls2[i]);
>> +			if (err)
>> +				return err;
>> +		}
>> +	}
>> +
>>   	/* PWM0 is used for half brightness or 50% duty cycle */
>>   	err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF);
>>   	if (err)

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 3/4] leds: pca955x: Optimize probe led selection
@ 2022-05-11 19:43       ` Eddie James
  0 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-05-11 19:43 UTC (permalink / raw)
  To: Pavel Machek; +Cc: openbmc, linux-kernel, andy.shevchenko, joel, linux-leds


On 5/4/22 12:24, Pavel Machek wrote:
> Hi!
>
>> Previously, the probe function might do up to 32 reads and writes
>> to the same 4 registers to program the led selection. Reduce this to
>> a maximum of 8 operations by accumulating the changes to the led
>> selection and comparing with the previous value to write the
>> selection if different.
> We have regmap APIs. You are free to use them if you really care about
> those few reads. Reimplementing them by hand is not acceptable. How big is
> the seedup here?


Hi,

To be honest this was not about speed, it's about a problem I've 
observed where many operations in quick succession caused the PCA chip 
to NAK the i2c transfer. I'm not sure of the root cause yet. So reducing 
the number of operations helped. We are also probably going to carry a 
throttling patch, unless I can prove the PCA chip itself is at fault and 
needs slower transfers.

I can try the regmap API and see if it helps though.

Thanks,

Eddie


>
> Best regards,
> 								Pavel
>
>> @@ -554,6 +556,15 @@ static int pca955x_probe(struct i2c_client *client)
>>   	init_data.devname_mandatory = false;
>>   	init_data.devicename = "pca955x";
>>   
>> +	nls = pca955x_num_led_regs(chip->bits);
>> +	for (i = 0; i < nls; ++i) {
>> +		err = pca955x_read_ls(pca955x, i, &ls1[i]);
>> +		if (err)
>> +			return err;
>> +
>> +		ls2[i] = ls1[i];
>> +	}
>> +
>>   	for (i = 0; i < chip->bits; i++) {
>>   		pca955x_led = &pca955x->leds[i];
>>   		pca955x_led->led_num = i;
>> @@ -624,6 +634,14 @@ static int pca955x_probe(struct i2c_client *client)
>>   		}
>>   	}
>>   
>> +	for (i = 0; i < nls; ++i) {
>> +		if (ls1[i] != ls2[i]) {
>> +			err = pca955x_write_ls(pca955x, i, ls2[i]);
>> +			if (err)
>> +				return err;
>> +		}
>> +	}
>> +
>>   	/* PWM0 is used for half brightness or 50% duty cycle */
>>   	err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF);
>>   	if (err)

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
  2022-05-04 17:24     ` Pavel Machek
@ 2022-05-11 19:54       ` Eddie James
  -1 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-05-11 19:54 UTC (permalink / raw)
  To: Pavel Machek
  Cc: linux-leds, linux-kernel, patrick, andy.shevchenko, openbmc, joel


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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/4] leds: pca955x: Add HW blink support
@ 2022-05-11 19:54       ` Eddie James
  0 siblings, 0 replies; 24+ messages in thread
From: Eddie James @ 2022-05-11 19:54 UTC (permalink / raw)
  To: Pavel Machek; +Cc: openbmc, linux-kernel, andy.shevchenko, joel, linux-leds


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

^ permalink raw reply	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-05-11 19:55 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-11 16:20 [PATCH v3 0/4] leds: pca955x: Add HW blink support Eddie James
2022-04-11 16:20 ` Eddie James
2022-04-11 16:20 ` [PATCH v3 1/4] leds: pca955x: Refactor with helper functions and renaming Eddie James
2022-04-11 16:20   ` Eddie James
2022-04-11 16:20 ` [PATCH v3 2/4] leds: pca955x: Use pointers to driver data rather than I2C client Eddie James
2022-04-11 16:20   ` Eddie James
2022-04-11 16:20 ` [PATCH v3 3/4] leds: pca955x: Optimize probe led selection Eddie James
2022-04-11 16:20   ` Eddie James
2022-05-04 17:24   ` Pavel Machek
2022-05-04 17:24     ` Pavel Machek
2022-05-11 19:43     ` Eddie James
2022-05-11 19:43       ` Eddie James
2022-04-11 16:20 ` [PATCH v3 4/4] leds: pca955x: Add HW blink support Eddie James
2022-04-11 16:20   ` Eddie James
2022-05-04 17:24   ` Pavel Machek
2022-05-04 17:24     ` Pavel Machek
2022-05-11 19:54     ` Eddie James
2022-05-11 19:54       ` Eddie James
2022-04-11 17:06 ` [PATCH v3 0/4] " Andy Shevchenko
2022-04-11 17:06   ` Andy Shevchenko
2022-04-12 11:22 [PATCH v3 4/4] " kernel test robot
2022-04-12 11:33 ` Dan Carpenter
2022-04-12 11:33 ` Dan Carpenter
2022-04-12 11:33 ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.