Skip to contentSkip to conversation

leds: Support retaining state for the PCA955x

PATCH Superseded Eddie James 4 participants 10 messages Apr 29, 2021 – May 13, 2021

Subsystems (2)
Device tree bindings LED

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Eddie James

author
#0 lore raw

cover leds: Support retaining state for the PCA955x

This series implements the ability to retain the state of the LEDs controlled by the PCA955x across system reboots. This includes a change to the LED core driver to respect the retain-state-shutdown device tree property. It also cleans up the PCA955x driver and adds the ability to query the hardware LED brightness.

Eddie James (5):
dt-bindings: leds: Add retain-state-shutdown boolean
leds: leds-core: Implement the retain-state-shutdown property
leds: pca955x: Clean up code formatting
leds: pca955x: Add brightness_get function
leds: pca955x: Implement the default-state property

 .../devicetree/bindings/leds/common.yaml      |   6 +
 drivers/leds/led-class.c                      |  10 +-
 drivers/leds/leds-pca955x.c                   | 169 +++++++++++++-----
 3 files changed, 142 insertions(+), 43 deletions(-)
Signature · 1 line
2.27.0
author
#1 lore raw

PATCH 1/5 dt-bindings: leds: Add retain-state-shutdown boolean

Document the retain-state-shutdown property that indicates that a LED should not be turned off or changed during system shutdown.

Signed-off-by Eddie James <[email protected]>
---
 Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/leds/common.yaml +6 -0
index b1f363747a62..697102707703 100644
@@ -128,6 +128,12 @@ properties:
       as a panic indicator.
     type: boolean
 
+  retain-state-shutdown:
+    description:
+      This property specifies that the LED should not be turned off or changed
+      when the system shuts down.
+    type: boolean
+
   trigger-sources:
     description: |
       List of devices which should be used as a source triggering this LED
Signature · 1 line
2.27.0
author
#5 lore raw

PATCH 2/5 leds: leds-core: Implement the retain-state-shutdown property

In reply to Eddie James

Read the retain-state-shutdown device tree property to set the existing LED_RETAIN_AT_SHUTDOWN flag. Then check the flag when unregistering, and if set, don't set the brightness to OFF. This is useful for systems that want to keep the HW state of the LED across reboots.

Signed-off-by Eddie James <[email protected]>
---
 drivers/leds/led-class.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/leds/led-class.c +8 -2
index 2e495ff67856..f2f29318d312 100644
@@ -354,10 +354,15 @@ int led_classdev_register_ext(struct device *parent,
 		if (ret < 0)
 			return ret;
 
-		if (init_data->fwnode)
+		if (init_data->fwnode) {
 			fwnode_property_read_string(init_data->fwnode,
 				"linux,default-trigger",
 				&led_cdev->default_trigger);
+
+			if (fwnode_property_present(init_data->fwnode,
+						    "retain-state-shutdown"))
+				led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
+		}
 	} else {
 		proposed_name = led_cdev->name;
 	}

@@ -448,7 +453,8 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	/* Stop blinking */
 	led_stop_software_blink(led_cdev);
 
-	led_set_brightness(led_cdev, LED_OFF);
+	if (!(led_cdev->flags & LED_RETAIN_AT_SHUTDOWN))
+		led_set_brightness(led_cdev, LED_OFF);
 
 	flush_work(&led_cdev->set_brightness_work);
Signature · 1 line
2.27.0
author
#7 lore raw

PATCH 3/5 leds: pca955x: Clean up code formatting

In reply to Eddie James

Format the code. Add some variables to help shorten lines.

Signed-off-by Eddie James <[email protected]>
---
 drivers/leds/leds-pca955x.c | 63 ++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 33 deletions(-)
drivers/leds/leds-pca955x.c +30 -33
index 7087ca4592fc..f0d841cb59fc 100644
@@ -166,11 +166,10 @@ 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);
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client,
-		pca95xx_num_input_regs(pca955x->chipdef->bits) + 2*n,
-		val);
+	ret = i2c_smbus_write_byte_data(client, cmd, val);
 	if (ret < 0)
 		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
 			__func__, n, val, ret);

@@ -187,11 +186,10 @@ 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);
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client,
-		pca95xx_num_input_regs(pca955x->chipdef->bits) + 1 + 2*n,
-		val);
+	ret = i2c_smbus_write_byte_data(client, cmd, val);
 	if (ret < 0)
 		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
 			__func__, n, val, ret);

@@ -205,11 +203,10 @@ 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;
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client,
-		pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n,
-		val);
+	ret = i2c_smbus_write_byte_data(client, cmd, val);
 	if (ret < 0)
 		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
 			__func__, n, val, ret);

@@ -223,10 +220,10 @@ 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;
 	int ret;
 
-	ret = i2c_smbus_read_byte_data(client,
-		pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n);
+	ret = i2c_smbus_read_byte_data(client, cmd);
 	if (ret < 0) {
 		dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
 			__func__, n, ret);

@@ -371,6 +368,7 @@ static struct pca955x_platform_data *
 pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip)
 {
 	struct pca955x_platform_data *pdata;
+	struct pca955x_led *led;
 	struct fwnode_handle *child;
 	int count;
 

@@ -401,13 +399,13 @@ pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip)
 		if ((res != 0) && is_of_node(child))
 			name = to_of_node(child)->name;
 
-		snprintf(pdata->leds[reg].name, sizeof(pdata->leds[reg].name),
-			 "%s", name);
+		led = &pdata->leds[reg];
+		snprintf(led->name, sizeof(led->name), "%s", name);
 
-		pdata->leds[reg].type = PCA955X_TYPE_LED;
-		fwnode_property_read_u32(child, "type", &pdata->leds[reg].type);
+		led->type = PCA955X_TYPE_LED;
+		fwnode_property_read_u32(child, "type", &led->type);
 		fwnode_property_read_string(child, "linux,default-trigger",
-					&pdata->leds[reg].default_trigger);
+					    &led->default_trigger);
 	}
 
 	pdata->num_leds = chip->bits;

@@ -426,11 +424,12 @@ static const struct of_device_id of_pca955x_match[] = {
 MODULE_DEVICE_TABLE(of, of_pca955x_match);
 
 static int pca955x_probe(struct i2c_client *client,
-					const struct i2c_device_id *id)
+			 const struct i2c_device_id *id)
 {
 	struct pca955x *pca955x;
 	struct pca955x_led *pca955x_led;
 	struct pca955x_chipdef *chip;
+	struct led_classdev *led;
 	struct i2c_adapter *adapter;
 	int i, err;
 	struct pca955x_platform_data *pdata;

@@ -449,13 +448,13 @@ static int pca955x_probe(struct i2c_client *client,
 	if ((client->addr & ~((1 << chip->slv_addr_shift) - 1)) !=
 	    chip->slv_addr) {
 		dev_err(&client->dev, "invalid slave address %02x\n",
-				client->addr);
+			client->addr);
 		return -ENODEV;
 	}
 
 	dev_info(&client->dev, "leds-pca955x: Using %s %d-bit LED driver at "
-			"slave address 0x%02x\n",
-			client->name, chip->bits, client->addr);
+		 "slave address 0x%02x\n", client->name, chip->bits,
+		 client->addr);
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -EIO;

@@ -471,8 +470,8 @@ static int pca955x_probe(struct i2c_client *client,
 	if (!pca955x)
 		return -ENOMEM;
 
-	pca955x->leds = devm_kcalloc(&client->dev,
-			chip->bits, sizeof(*pca955x_led), GFP_KERNEL);
+	pca955x->leds = devm_kcalloc(&client->dev, chip->bits,
+				     sizeof(*pca955x_led), GFP_KERNEL);
 	if (!pca955x->leds)
 		return -ENOMEM;
 

@@ -501,27 +500,25 @@ static int pca955x_probe(struct i2c_client *client,
 			 */
 			if (pdata->leds[i].name[0] == '\0')
 				snprintf(pdata->leds[i].name,
-					sizeof(pdata->leds[i].name), "%d", i);
+					 sizeof(pdata->leds[i].name), "%d", i);
 
-			snprintf(pca955x_led->name,
-				sizeof(pca955x_led->name), "pca955x:%s",
-				pdata->leds[i].name);
+			snprintf(pca955x_led->name, sizeof(pca955x_led->name),
+				 "pca955x:%s", pdata->leds[i].name);
 
+			led = &pca955x_led->led_cdev;
 			if (pdata->leds[i].default_trigger)
-				pca955x_led->led_cdev.default_trigger =
+				led->default_trigger =
 					pdata->leds[i].default_trigger;
 
-			pca955x_led->led_cdev.name = pca955x_led->name;
-			pca955x_led->led_cdev.brightness_set_blocking =
-				pca955x_led_set;
+			led->name = pca955x_led->name;
+			led->brightness_set_blocking = pca955x_led_set;
 
-			err = devm_led_classdev_register(&client->dev,
-							&pca955x_led->led_cdev);
+			err = devm_led_classdev_register(&client->dev, led);
 			if (err)
 				return err;
 
 			/* Turn off LED */
-			err = pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);
+			err = pca955x_led_set(led, LED_OFF);
 			if (err)
 				return err;
 		}
Signature · 1 line
2.27.0
author
#8 lore raw

PATCH 4/5 leds: pca955x: Add brightness_get function

In reply to Eddie James

Add a function to fetch the state of the hardware LED.

Signed-off-by Eddie James <[email protected]>
---
 drivers/leds/leds-pca955x.c | 52 +++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
drivers/leds/leds-pca955x.c +52 -0
index f0d841cb59fc..e47ba7c3b7c7 100644
@@ -233,6 +233,57 @@ static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
 	return 0;
 }
 
+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);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, cmd);
+	if (ret < 0) {
+		dev_err(&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 = container_of(led_cdev,
+						       struct pca955x_led,
+						       led_cdev);
+	struct pca955x *pca955x = pca955x_led->pca955x;
+	u8 ls, pwm;
+	int ret;
+
+	ret = pca955x_read_ls(pca955x->client, pca955x_led->led_num / 4, &ls);
+	if (ret)
+		return ret;
+
+	ls = (ls >> ((pca955x_led->led_num % 4) << 1)) & 0x3;
+	switch (ls) {
+	case PCA955X_LS_LED_ON:
+		ret = LED_FULL;
+		break;
+	case PCA955X_LS_LED_OFF:
+		ret = LED_OFF;
+		break;
+	case PCA955X_LS_BLINK0:
+		ret = LED_HALF;
+		break;
+	case PCA955X_LS_BLINK1:
+		ret = pca955x_read_pwm(pca955x->client, 1, &pwm);
+		if (ret)
+			return ret;
+		ret = 255 - pwm;
+		break;
+	}
+
+	return ret;
+}
+
 static int pca955x_led_set(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {

@@ -512,6 +563,7 @@ static int pca955x_probe(struct i2c_client *client,
 
 			led->name = pca955x_led->name;
 			led->brightness_set_blocking = pca955x_led_set;
+			led->brightness_get = pca955x_led_get;
 
 			err = devm_led_classdev_register(&client->dev, led);
 			if (err)
Signature · 1 line
2.27.0
author
#9 lore raw

PATCH 5/5 leds: pca955x: Implement the default-state property

In reply to Eddie James

In order to retain the LED state after a system reboot, check the documented default-state device tree property during initialization. Modify the behavior of the probe according to the property.

Signed-off-by Eddie James <[email protected]>
---
 drivers/leds/leds-pca955x.c | 54 +++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 8 deletions(-)
drivers/leds/leds-pca955x.c +46 -8
index e47ba7c3b7c7..fa1d77d86ef6 100644
@@ -129,6 +129,7 @@ struct pca955x_led {
 	int			led_num;	/* 0 .. 15 potentially */
 	char			name[32];
 	u32			type;
+	int			default_state;
 	const char		*default_trigger;
 };
 

@@ -439,6 +440,7 @@ pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip)
 
 	device_for_each_child_node(&client->dev, child) {
 		const char *name;
+		const char *state;
 		u32 reg;
 		int res;
 

@@ -457,6 +459,18 @@ pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip)
 		fwnode_property_read_u32(child, "type", &led->type);
 		fwnode_property_read_string(child, "linux,default-trigger",
 					    &led->default_trigger);
+
+		if (!fwnode_property_read_string(child, "default-state",
+						 &state)) {
+			if (!strcmp(state, "keep"))
+				led->default_state = LEDS_GPIO_DEFSTATE_KEEP;
+			else if (!strcmp(state, "on"))
+				led->default_state = LEDS_GPIO_DEFSTATE_ON;
+			else
+				led->default_state = LEDS_GPIO_DEFSTATE_OFF;
+		} else {
+			led->default_state = LEDS_GPIO_DEFSTATE_OFF;
+		}
 	}
 
 	pdata->num_leds = chip->bits;

@@ -485,6 +499,7 @@ static int pca955x_probe(struct i2c_client *client,
 	int i, err;
 	struct pca955x_platform_data *pdata;
 	int ngpios = 0;
+	bool keep_pwm = false;
 
 	chip = &pca955x_chipdefs[id->driver_data];
 	adapter = client->adapter;

@@ -565,14 +580,35 @@ static int pca955x_probe(struct i2c_client *client,
 			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;
+			}
+
 			err = devm_led_classdev_register(&client->dev, led);
 			if (err)
 				return err;
 
-			/* Turn off LED */
-			err = pca955x_led_set(led, LED_OFF);
-			if (err)
-				return err;
+			/*
+			 * 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;
+			}
 		}
 	}
 

@@ -581,10 +617,12 @@ static int pca955x_probe(struct i2c_client *client,
 	if (err)
 		return err;
 
-	/* PWM1 is used for variable brightness, default to OFF */
-	err = pca955x_write_pwm(client, 1, 0);
-	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);
Signature · 1 line
2.27.0
author
#2 lore raw

Re: [PATCH 1/5] dt-bindings: leds: Add retain-state-shutdown boolean

In reply to Eddie James
Quoted 2 lines · Eddie James

Document the retain-state-shutdown property that indicates that a LED should not be turned off or changed during system shutdown.

Lost a character of Rob's email, so bumping this one with the right address.

Sorry,
Eddie

Quoted 26 lines
Signed-off-by Eddie James <[email protected]>
---
 Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/leds/common.yaml +3 -0
index b1f363747a62..697102707703 100644
@@ -128,6 +128,12 @@ properties:
       as a panic indicator.
     type: boolean
 
+  retain-state-shutdown:
+    description:
+      This property specifies that the LED should not be turned off

or changed

Documentation/devicetree/bindings/leds/common.yaml +3 -0
+      when the system shuts down.
+    type: boolean
+
   trigger-sources:
     description: |
       List of devices which should be used as a source triggering

this LED

Reviewed-by
#3 lore raw

Re: [PATCH 1/5] dt-bindings: leds: Add retain-state-shutdown boolean

Quoted 15 lines · Eddie James
Quoted 2 lines · Eddie James

Document the retain-state-shutdown property that indicates that a LED should not be turned off or changed during system shutdown.

Lost a character of Rob's email, so bumping this one with the right address.

Sorry,
Eddie

Quoted 1 line
Reviewed-by Vishwanatha Subbanna <[email protected]>
Quoted 26 lines
Quoted 25 lines
Signed-off-by Eddie James <[email protected]>

---
Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++

1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/leds/common.yaml +3 -0
index b1f363747a62..697102707703 100644
@@ -128,6 +128,12 @@ properties:
      as a panic indicator.
    type: boolean
 
+  retain-state-shutdown:
+    description:
+      This property specifies that the LED should not be turned off

or changed

Documentation/devicetree/bindings/leds/common.yaml +3 -0
+      when the system shuts down.
+    type: boolean
+
  trigger-sources:
    description: |
      List of devices which should be used as a source triggering

this LED

maintainer Acked-by on v1 1/5
#4 lore raw

Re: [PATCH 1/5] dt-bindings: leds: Add retain-state-shutdown boolean

In reply to Eddie James
Quoted 8 lines · Eddie James

Document the retain-state-shutdown property that indicates that a LED should not be turned off or changed during system shutdown.

Signed-off-by Eddie James <[email protected]>
---
 Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Acked-by Rob Herring <[email protected]>
#6 lore raw

Re: [PATCH 2/5] leds: leds-core: Implement the retain-state-shutdown property

In reply to Eddie James
Quoted 18 lines · Eddie James

Read the retain-state-shutdown device tree property to set the existing LED_RETAIN_AT_SHUTDOWN flag. Then check the flag when unregistering, and if set, don't set the brightness to OFF. This is useful for systems that want to keep the HW state of the LED across reboots.

Signed-off-by Eddie James <[email protected]>
---
 drivers/leds/led-class.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/leds/led-class.c +0 -0
index 2e495ff67856..f2f29318d312 100644
@@ -354,10 +354,15 @@ int led_classdev_register_ext(struct device *parent,
 		if (ret < 0)
 			return ret;

drivers/leds/led-class.c int led_classdev_register_ext(struct device *parent, · line 364

Quoted 10 lines
drivers/leds/led-class.c +5 -1
 
-		if (init_data->fwnode)
+		if (init_data->fwnode) {
 			fwnode_property_read_string(init_data->fwnode,
 				"linux,default-trigger",
 				&led_cdev->default_trigger);
+
+			if (fwnode_property_present(init_data->fwnode,
+						    "retain-state-shutdown"))
+				led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN;

This is what we need, but I notice the pca955x driver is calling through devm_led_classdev_register() which passes NULL through init_data. So we won't get the result we want from this series as I understand it.

Andrew