diff mbox series

leds: pca955x: Add HW blink support

Message ID 20220330203318.19225-1-eajames@linux.ibm.com
State New
Headers show
Series leds: pca955x: Add HW blink support | expand

Commit Message

Eddie James March 30, 2022, 8:33 p.m. UTC 
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, all blinked LEDs on
the chip will have the same frequency and brightness.

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

Comments

Patrick Williams March 31, 2022, 3:39 p.m. UTC | #1 
On Wed, Mar 30, 2022 at 03:33:18PM -0500, Eddie James wrote:
> Support blinking using the PCA955x chip. Use PWM0 for blinking
> instead of LED_HALF brightness. Since there is only one frequency
> and brightness register for any blinking LED, all blinked LEDs on
> the chip will have the same frequency and brightness.

The current implementation uses the PWM to control the "brightness"
with PWM0 being assigned 50% and PWM1 being configured as a single value
that isn't ON, OFF, or 50%.  I suspect that most users of these 955x
chips care either about brightness or blinking but not both, but it is
possible I am wrong.  It would be nice if we could use PWM1 as another
hardware blink control when it hasn't been used for brightness, but that
would require some additional state tracking I think.

I like that we can now use the hardware to control blink rate, rather
than doing it in software, and, I really like that in theory if N LEDs on
the device are all blinking at the same rate they will actually turn on and
off at the same exact moment because it is done in hardware.  I am really
concerned about this proposed change and the way it will change current
behavior though.

It is not uncommon in a BMC design to use one of these 955x chips to control
8 or 16 different LEDs reflecting the state of the system and at
different blink rates.  An example LED policy might be that you have 1 LED
for "power status" and another LED for "system identify + health status".
When the system is powered off the "power status" LED flashes at a slow rate
and when the system is powered on it goes on solid.  When the system is healthy
the "health status" is on, when it is unhealthy it blinks slowly, and when the
system is "identified" it blinks fast.

My point of the above is that there are certainly system policies where
you'd want to flash two different LEDs at two different rates.  In
today's implementation of this driver those both turn into
software-emulated blinking by the kernel.  With your proposal we lose
this ability and instead whichever LED is configured second will affect
all other blinking LEDs.

It looks like in led-core.c led_blink_setup that if the device
`blink_set` returns an error then software blinking is the fallback.  Is
it possible for us to have this driver keep track of how many LEDs are
in blink state (and which speeds are allocated) and get led-core to
fallback to software blinking if we are unable to satisfy the new blink
rate without affecting an existing LED blink rate?

Looking at the tree it seems bcm6328 does what I am suggesting already
but I don't see any other drivers that obviously do.  The PCA955x is
pretty widely used in BMC implementations:

    $ git grep -l pca955 arch/arm/boot/dts/aspeed* | wc -l
    13
Eddie James April 1, 2022, 2:17 p.m. UTC | #2 
On 3/31/22 10:39, Patrick Williams wrote:
> On Wed, Mar 30, 2022 at 03:33:18PM -0500, Eddie James wrote:
>> Support blinking using the PCA955x chip. Use PWM0 for blinking
>> instead of LED_HALF brightness. Since there is only one frequency
>> and brightness register for any blinking LED, all blinked LEDs on
>> the chip will have the same frequency and brightness.
> The current implementation uses the PWM to control the "brightness"
> with PWM0 being assigned 50% and PWM1 being configured as a single value
> that isn't ON, OFF, or 50%.  I suspect that most users of these 955x
> chips care either about brightness or blinking but not both, but it is
> possible I am wrong.  It would be nice if we could use PWM1 as another
> hardware blink control when it hasn't been used for brightness, but that
> would require some additional state tracking I think.
>
> I like that we can now use the hardware to control blink rate, rather
> than doing it in software, and, I really like that in theory if N LEDs on
> the device are all blinking at the same rate they will actually turn on and
> off at the same exact moment because it is done in hardware.  I am really
> concerned about this proposed change and the way it will change current
> behavior though.
>
> It is not uncommon in a BMC design to use one of these 955x chips to control
> 8 or 16 different LEDs reflecting the state of the system and at
> different blink rates.  An example LED policy might be that you have 1 LED
> for "power status" and another LED for "system identify + health status".
> When the system is powered off the "power status" LED flashes at a slow rate
> and when the system is powered on it goes on solid.  When the system is healthy
> the "health status" is on, when it is unhealthy it blinks slowly, and when the
> system is "identified" it blinks fast.
>
> My point of the above is that there are certainly system policies where
> you'd want to flash two different LEDs at two different rates.  In
> today's implementation of this driver those both turn into
> software-emulated blinking by the kernel.  With your proposal we lose
> this ability and instead whichever LED is configured second will affect
> all other blinking LEDs.


Yep. I see your point, it could be problematic.


>
> It looks like in led-core.c led_blink_setup that if the device
> `blink_set` returns an error then software blinking is the fallback.  Is
> it possible for us to have this driver keep track of how many LEDs are
> in blink state (and which speeds are allocated) and get led-core to
> fallback to software blinking if we are unable to satisfy the new blink
> rate without affecting an existing LED blink rate?


OK, I like this idea, I'll go ahead and implement it. Thanks for the 
suggestion!


Eddie


>
> Looking at the tree it seems bcm6328 does what I am suggesting already
> but I don't see any other drivers that obviously do.  The PCA955x is
> pretty widely used in BMC implementations:
>
>      $ git grep -l pca955 arch/arm/boot/dts/aspeed* | wc -l
>      13
>
diff mbox series

Patch

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 81aaf21212d7..aeddc64e8ecf 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -74,6 +74,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 +82,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,
 	},
 };
 
@@ -163,7 +169,7 @@  static inline u8 pca955x_ledsel(u8 oldval, int led_num, int state)
 
 /*
  * 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) / <38 or 44, chip dependent>
  */
 static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
 {
@@ -273,13 +279,16 @@  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->client, 0, &pwm);
+		if (ret)
+			return ret;
+		ret = 256 - pwm;
 		break;
 	case PCA955X_LS_BLINK1:
 		ret = pca955x_read_pwm(pca955x->client, 1, &pwm);
 		if (ret)
 			return ret;
-		ret = 255 - pwm;
+		ret = 256 - pwm;
 		break;
 	}
 
@@ -308,32 +317,98 @@  static int pca955x_led_set(struct led_classdev *led_cdev,
 	if (ret)
 		goto out;
 
-	switch (value) {
-	case LED_FULL:
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
-		break;
-	case LED_OFF:
+	if (value == LED_OFF) {
 		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_OFF);
-		break;
-	case LED_HALF:
+	} else {
+		u8 tls = (ls >> (ls_led << 1)) & 0x3;
+
+		if (tls == PCA955X_LS_BLINK0) {
+			ret = pca955x_write_pwm(pca955x->client, 0,
+						256 - value);
+			goto out;
+		} else {
+			if (value == LED_FULL) {
+				ls = pca955x_ledsel(ls, ls_led,
+						    PCA955X_LS_LED_ON);
+			} else {
+				/*
+				 * 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->client, 1,
+							256 - value);
+				if (ret || tls == PCA955X_LS_BLINK1)
+					goto out;
+				ls = pca955x_ledsel(ls, ls_led,
+						    PCA955X_LS_BLINK1);
+			}
+		}
+	}
+
+	ret = pca955x_write_ls(pca955x->client, chip_ls, ls);
+
+out:
+	mutex_unlock(&pca955x->lock);
+
+	return ret;
+}
+
+static int pca955x_led_blink(struct led_classdev *led_cdev,
+			     unsigned long *delay_on, unsigned long *delay_off)
+{
+	int chip_ls;
+	int ls_led;
+	int ret;
+	u8 ls;
+	struct pca955x_led *pca955x_led = container_of(led_cdev,
+						      struct pca955x_led,
+						      led_cdev);
+	struct pca955x *pca955x = pca955x_led->pca955x;
+	unsigned long p = *delay_on + *delay_off;
+
+	/* 1 Hz default */
+	if (!p)
+		p = 1000;
+
+	p *= (unsigned long)pca955x->chipdef->blink_div;
+	p /= 1000;
+	p -= 1;
+
+	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);
+	if (ret)
+		goto out;
+
+	/*
+	 * All blinking leds on the PCA955x chip will use the same period and
+	 * brightness.
+	 */
+	ret = pca955x_write_psc(pca955x->client, 0, (u8)p);
+	if (ret)
+		goto out;
+
+	if (((ls >> (ls_led << 1)) & 0x3) != PCA955X_LS_BLINK0) {
 		ls = pca955x_ledsel(ls, ls_led, 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->client, 1, 255 - value);
+		ret = pca955x_write_ls(pca955x->client, chip_ls, ls);
 		if (ret)
 			goto out;
-		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK1);
-		break;
 	}
 
-	ret = pca955x_write_ls(pca955x->client, chip_ls, ls);
+	p += 1;
+	p *= 1000;
+	p /= (unsigned long)pca955x->chipdef->blink_div;
+	p /= 2;
+
+	*delay_on = p;
+	*delay_off = p;
 
 out:
 	mutex_unlock(&pca955x->lock);
@@ -495,7 +570,6 @@  static int pca955x_probe(struct i2c_client *client)
 	int i, err;
 	struct pca955x_platform_data *pdata;
 	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);
@@ -577,6 +651,7 @@  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) {
@@ -585,9 +660,28 @@  static int pca955x_probe(struct i2c_client *client)
 					return err;
 			} else if (pdata->leds[i].default_state ==
 				   LEDS_GPIO_DEFSTATE_ON) {
-				err = pca955x_led_set(led, LED_FULL);
+				/*
+				 * handle this case specially in order to turn
+				 * off blinking, which pca955x_led_set won't do
+				 */
+				u8 ls;
+				int chip_ls = i / 4;
+				int ls_led = i % 4;
+
+				err = pca955x_read_ls(pca955x->client, chip_ls,
+						      &ls);
 				if (err)
 					return err;
+
+				if (((ls >> (ls_led << 1)) & 0x3) !=
+				    PCA955X_LS_LED_ON) {
+					ls = pca955x_ledsel(ls, ls_led,
+							    PCA955X_LS_LED_ON);
+					err = pca955x_write_ls(pca955x->client,
+							       chip_ls, ls);
+					if (err)
+						return err;
+				}
 			}
 
 			init_data.fwnode = pdata->leds[i].fwnode;
@@ -616,39 +710,10 @@  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;
-			}
 		}
 	}
 
-	/* PWM0 is used for half brightness or 50% duty cycle */
-	err = pca955x_write_pwm(client, 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);
-		if (err)
-			return err;
-	}
-
-	/* Set to fast frequency so we do not see flashing */
-	err = pca955x_write_psc(client, 0, 0);
-	if (err)
-		return err;
+	/* Set PWM1 to fast frequency so we do not see flashing */
 	err = pca955x_write_psc(client, 1, 0);
 	if (err)
 		return err;