leds: pca995x: Minor fixes; reset, output-gain, error reporting and group brightness support

Each entry in led_fwnodes needs to be put as long as no LED device has been created for it yet - not just in the creation loop, but also in the first loop that iterates over the child nodes.

By clearing entries in led_fwnodes once they have been used, the same cleanup loop can be used to handle errors in both loops.

Fixes 82c5ada1f9d0 ("leds: pca995x: Fix device child node usage in pca995x_probe()")

Signed-off-by Nora Schiffer <[email protected]>

---
 drivers/leds/leds-pca995x.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

drivers/leds/leds-pca995x.c +6 -5

index 8b05b3f12cf38..079ce3d8f1e49 100644
@@ -123,7 +123,7 @@ static int pca995x_probe(struct i2c_client *client)
 	const struct pca995x_chipdef *chipdef;
 	struct pca995x_chip *chip;
 	struct pca995x_led *led;
-	int i, j, reg, ret;
+	int i, reg, ret;
 
 	chipdef = device_get_match_data(&client->dev);
 

@@ -144,10 +144,12 @@ static int pca995x_probe(struct i2c_client *client)
 	device_for_each_child_node_scoped(dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &reg);
 		if (ret)
-			return ret;
+			goto err_put_nodes;
 
-		if (reg < 0 || reg >= chipdef->num_leds || led_fwnodes[reg])
-			return -EINVAL;
+		if (reg < 0 || reg >= chipdef->num_leds || led_fwnodes[reg]) {
+			ret = -EINVAL;
+			goto err_put_nodes;
+		}
 
 		led = &chip->leds[reg];
 		led_fwnodes[reg] = fwnode_handle_get(child);

@@ -169,12 +171,12 @@ static int pca995x_probe(struct i2c_client *client)
 						     &chip->leds[i].ldev,
 						     &init_data);
 		if (ret < 0) {
-			for (j = i; j < chipdef->num_leds; j++)

drivers/leds/leds-pca995x.c +6 -0

 	}
 
 	/* Disable LED all-call address and set normal mode */

@@ -184,6 +186,12 @@ static int pca995x_probe(struct i2c_client *client)
 
 	/* IREF Output current value for all LEDn outputs */
 	return regmap_write(chip->regmap, chipdef->irefall, PCA995X_IREFALL_HALF_CFG);
+
+err_put_nodes:
+	for (i = 0; i < chipdef->num_leds; i++)
+		fwnode_handle_put(led_fwnodes[i]);
+
+	return ret;
 }
 
 static const struct i2c_device_id pca995x_id[] = {

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

--- checkpatch.pl: clean (0 issues) ---

Quoted 51 lines · Nora Schiffer

Each entry in led_fwnodes needs to be put as long as no LED device has been created for it yet - not just in the creation loop, but also in the first loop that iterates over the child nodes.

By clearing entries in led_fwnodes once they have been used, the same cleanup loop can be used to handle errors in both loops.

Fixes 82c5ada1f9d0 ("leds: pca995x: Fix device child node usage in pca995x_probe()")

Signed-off-by Nora Schiffer <[email protected]>

---
 drivers/leds/leds-pca995x.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

drivers/leds/leds-pca995x.c +6 -9

index 8b05b3f12cf38..079ce3d8f1e49 100644
@@ -123,7 +123,7 @@ static int pca995x_probe(struct i2c_client *client)
 	const struct pca995x_chipdef *chipdef;
 	struct pca995x_chip *chip;
 	struct pca995x_led *led;
-	int i, j, reg, ret;
+	int i, reg, ret;
 
 	chipdef = device_get_match_data(&client->dev);
 

@@ -144,10 +144,12 @@ static int pca995x_probe(struct i2c_client *client)
 	device_for_each_child_node_scoped(dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &reg);
 		if (ret)
-			return ret;
+			goto err_put_nodes;
 
-		if (reg < 0 || reg >= chipdef->num_leds || led_fwnodes[reg])
-			return -EINVAL;
+		if (reg < 0 || reg >= chipdef->num_leds || led_fwnodes[reg]) {
+			ret = -EINVAL;
+			goto err_put_nodes;
+		}
 
 		led = &chip->leds[reg];
 		led_fwnodes[reg] = fwnode_handle_get(child);

@@ -169,12 +171,12 @@ static int pca995x_probe(struct i2c_client *client)
 						     &chip->leds[i].ldev,
 						     &init_data);
 		if (ret < 0) {
-			for (j = i; j < chipdef->num_leds; j++)
-				fwnode_handle_put(led_fwnodes[j]);
-			return dev_err_probe(dev, ret,
-					     "Could not register LED %s\n",
-					     chip->leds[i].ldev.name);

drivers/leds/leds-pca995x.c static int pca995x_probe(struct i2c_client *client) · line 179

Quoted 6 lines

drivers/leds/leds-pca995x.c +5 -0

+			dev_err_probe(dev, ret, "Could not register LED %s\n",
+				      chip->leds[i].ldev.name);
+			goto err_put_nodes;
 		}
+
+		led_fwnodes[i] = NULL;

Shouldn't we put this before removing it?

Quoted 24 lines

drivers/leds/leds-pca995x.c +6 -0

 	}
 
 	/* Disable LED all-call address and set normal mode */

@@ -184,6 +186,12 @@ static int pca995x_probe(struct i2c_client *client)
 
 	/* IREF Output current value for all LEDn outputs */
 	return regmap_write(chip->regmap, chipdef->irefall, PCA995X_IREFALL_HALF_CFG);
+
+err_put_nodes:
+	for (i = 0; i < chipdef->num_leds; i++)
+		fwnode_handle_put(led_fwnodes[i]);
+
+	return ret;
 }
 
 static const struct i2c_device_id pca995x_id[] = {

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

The PCA9952/9955B/9956B have a reset pin. Use it to reset the LED driver if provided in the Device Tree.

Signed-off-by Nora Schiffer <[email protected]>

---
 drivers/leds/leds-pca995x.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/leds/leds-pca995x.c +3 -0

index e6122b33451ed..b528b2b4990a7 100644
@@ -8,6 +8,8 @@
  * Copyright 2023 Marek Vasut
  */
 
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
 #include <linux/module.h>

@@ -121,6 +123,7 @@ static int pca995x_probe(struct i2c_client *client)
 	struct fwnode_handle *led_fwnodes[PCA995X_MAX_OUTPUTS] = { 0 };
 	struct device *dev = &client->dev;
 	const struct pca995x_chipdef *chipdef;
+	struct gpio_desc *reset_gpio;
 	struct pca995x_chip *chip;
 	struct pca995x_led *led;
 	int i, reg, ret;

drivers/leds/leds-pca995x.c +4 -0

+		gpiod_set_value_cansleep(reset_gpio, 0);
+		usleep_range(1500, 1600);
+	}
+
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Quoted 33 lines · Nora Schiffer

The PCA9952/9955B/9956B have a reset pin. Use it to reset the LED driver if provided in the Device Tree.

Signed-off-by Nora Schiffer <[email protected]>

---
 drivers/leds/leds-pca995x.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/leds/leds-pca995x.c +3 -0

index e6122b33451ed..b528b2b4990a7 100644
@@ -8,6 +8,8 @@
  * Copyright 2023 Marek Vasut
  */
 
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
 #include <linux/module.h>

@@ -121,6 +123,7 @@ static int pca995x_probe(struct i2c_client *client)
 	struct fwnode_handle *led_fwnodes[PCA995X_MAX_OUTPUTS] = { 0 };
 	struct device *dev = &client->dev;
 	const struct pca995x_chipdef *chipdef;
+	struct gpio_desc *reset_gpio;
 	struct pca995x_chip *chip;
 	struct pca995x_led *led;
 	int i, reg, ret;

@@ -130,6 +133,16 @@ static int pca995x_probe(struct i2c_client *client)
 	if (!dev_fwnode(dev))
 		return -ENODEV;
 

drivers/leds/leds-pca995x.c static int pca995x_probe(struct i2c_client *client) · line 141

Quoted 6 lines

drivers/leds/leds-pca995x.c +6 -0

+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(reset_gpio),
+				     "failed to request reset GPIO\n");
+	if (reset_gpio) {
+		udelay(3);

It's strange to see delays _before_ actions.

What are you delaying for exactly?

Quoted 13 lines

drivers/leds/leds-pca995x.c +4 -0

+		gpiod_set_value_cansleep(reset_gpio, 0);
+		usleep_range(1500, 1600);
+	}
+
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

The PCA995x has builtin failure detection. Provide sysfs files for individual LED status (reporting "okay", "short-circuit" or "open-circuit") as well as a global "has_errors" flag. has_errors is sticky and must be cleared by writing "clear" to the sysfs file.

Signed-off-by Nora Schiffer <[email protected]>

---
 drivers/leds/leds-pca995x.c | 112 +++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

drivers/leds/leds-pca995x.c +47 -0

index 13cce0c3fdc19..57b5d2d6e035d 100644
@@ -8,6 +8,7 @@
  * Copyright 2023 Marek Vasut
  */
 
+#include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>

@@ -24,6 +25,12 @@
 /* Auto-increment disabled. Normal mode */
 #define PCA995X_MODE1_CFG		0x00
 
+#define PCA995X_MODE2_CLRERR		BIT(4)
+#define PCA995X_MODE2_ERROR		BIT(6)
+
+/* Clear errors on probe, group brightness control, linear adjustment */
+#define PCA995X_MODE2_CFG		PCA995X_MODE2_CLRERR
+
 /* LED select registers determine the source that drives LED outputs */
 #define PCA995X_LED_OFF			0x0
 #define PCA995X_LED_ON			0x1

@@ -37,30 +44,37 @@
 #define PCA995X_IREFALL_FULL_CFG	0xFF
 #define PCA995X_IREFALL_HALF_CFG	(PCA995X_IREFALL_FULL_CFG / 2)
 
+#define PCA995X_EFLAG_BITS		2
+#define PCA995X_EFLAG_MASK		GENMASK(1, 0)
+
 #define ldev_to_led(c)	container_of(c, struct pca995x_led, ldev)
 
 struct pca995x_chipdef {
 	unsigned int num_leds;
 	u8 pwm_base;
 	u8 irefall;
+	u8 eflag_base;
 };
 
 static const struct pca995x_chipdef pca9952_chipdef = {
 	.num_leds	= 16,
 	.pwm_base	= 0x0a,
 	.irefall	= 0x43,
+	.eflag_base	= 0x44,
 };
 
 static const struct pca995x_chipdef pca9955b_chipdef = {
 	.num_leds	= 16,
 	.pwm_base	= 0x08,
 	.irefall	= 0x45,
+	.eflag_base	= 0x46,
 };
 
 static const struct pca995x_chipdef pca9956b_chipdef = {
 	.num_leds	= 24,
 	.pwm_base	= 0x0a,
 	.irefall	= 0x40,
+	.eflag_base	= 0x41,
 };
 
 struct pca995x_led {

@@ -112,6 +126,83 @@ static int pca995x_brightness_set(struct led_classdev *led_cdev,
 	}
 }
 
+static ssize_t status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pca995x_led *led = ldev_to_led(led_cdev);
+	struct pca995x_chip *chip = led->chip;
+	const struct pca995x_chipdef *chipdef = chip->chipdef;
+	const char *status = "unknown";
+	unsigned int val;
+	int shift, ret;
+	u8 reg;
+
+	reg = chipdef->eflag_base + (led->led_no / PCA995X_OUTPUTS_PER_REG);
+	shift = PCA995X_EFLAG_BITS * (led->led_no % PCA995X_OUTPUTS_PER_REG);
+
+	ret = regmap_read(chip->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	switch ((val >> shift) & PCA995X_EFLAG_MASK) {
+	case 0:
+		status = "okay";
+		break;
+	case 1:
+		status = "short-circuit";
+		break;
+	case 2:
+		status = "open-circuit";
+	}
+
+	return sysfs_emit(buf, "%s\n", status);
+}
+
+static DEVICE_ATTR_RO(status);

drivers/leds/leds-pca995x.c +21 -0

+	int ret;
+
+	if (!sysfs_streq(buf, "clear"))
+		return -EINVAL;
+
+	ret = regmap_update_bits(chip->regmap, PCA995X_MODE2,
+				 PCA995X_MODE2_CLRERR, PCA995X_MODE2_CLRERR);
+
+	return ret ?: count;
+}
+
+static DEVICE_ATTR_RW(has_errors);
+
+static struct attribute *pca995x_attrs[] = {
+	&dev_attr_has_errors.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(pca995x);
+
 static const struct regmap_config pca995x_regmap = {
 	.reg_bits = 8,
 	.val_bits = 8,

@@ -176,6 +267,7 @@ static int pca995x_probe(struct i2c_client *client)
 		led->led_no = reg;
 		led->ldev.brightness_set_blocking = pca995x_brightness_set;
 		led->ldev.max_brightness = 255;
+		led->ldev.groups = pca995x_led_groups;
 	}
 
 	/* Disable LED all-call address and set normal mode */

@@ -183,11 +275,20 @@ static int pca995x_probe(struct i2c_client *client)
 	if (ret)
 		goto err_put_nodes;
 
+	/* Clear errors on probe */

drivers/leds/leds-pca995x.c +4 -1

+	if (ret)
+		goto err_put_nodes;
+
 	for (i = 0; i < chipdef->num_leds; i++) {
 		struct led_init_data init_data = {};
 

@@ -202,7 +303,7 @@ static int pca995x_probe(struct i2c_client *client)
 		if (ret < 0) {
 			dev_err_probe(dev, ret, "Could not register LED %s\n",
 				      chip->leds[i].ldev.name);
-			goto err_put_nodes;
+			goto err_remove_groups;
 		}
 
 		led_fwnodes[i] = NULL;

drivers/leds/leds-pca995x.c +1 -0

 static const struct i2c_device_id pca995x_id[] = {
 	{ .name = "pca9952", .driver_data = (kernel_ulong_t)&pca9952_chipdef },
 	{ .name = "pca9955b", .driver_data = (kernel_ulong_t)&pca9955b_chipdef },

@@ -239,6 +348,7 @@ static struct i2c_driver pca995x_driver = {
 		.of_match_table = pca995x_of_match,
 	},
 	.probe = pca995x_probe,
+	.remove = pca995x_remove,
 	.id_table = pca995x_id,
 };
 module_i2c_driver(pca995x_driver);

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Quoted 110 lines · Nora Schiffer

The PCA995x has builtin failure detection. Provide sysfs files for individual LED status (reporting "okay", "short-circuit" or "open-circuit") as well as a global "has_errors" flag. has_errors is sticky and must be cleared by writing "clear" to the sysfs file.

Signed-off-by Nora Schiffer <[email protected]>

---
 drivers/leds/leds-pca995x.c | 112 +++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

drivers/leds/leds-pca995x.c +47 -0

index 13cce0c3fdc19..57b5d2d6e035d 100644
@@ -8,6 +8,7 @@
  * Copyright 2023 Marek Vasut
  */
 
+#include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>

@@ -24,6 +25,12 @@
 /* Auto-increment disabled. Normal mode */
 #define PCA995X_MODE1_CFG		0x00
 
+#define PCA995X_MODE2_CLRERR		BIT(4)
+#define PCA995X_MODE2_ERROR		BIT(6)
+
+/* Clear errors on probe, group brightness control, linear adjustment */
+#define PCA995X_MODE2_CFG		PCA995X_MODE2_CLRERR
+
 /* LED select registers determine the source that drives LED outputs */
 #define PCA995X_LED_OFF			0x0
 #define PCA995X_LED_ON			0x1

@@ -37,30 +44,37 @@
 #define PCA995X_IREFALL_FULL_CFG	0xFF
 #define PCA995X_IREFALL_HALF_CFG	(PCA995X_IREFALL_FULL_CFG / 2)
 
+#define PCA995X_EFLAG_BITS		2
+#define PCA995X_EFLAG_MASK		GENMASK(1, 0)
+
 #define ldev_to_led(c)	container_of(c, struct pca995x_led, ldev)
 
 struct pca995x_chipdef {
 	unsigned int num_leds;
 	u8 pwm_base;
 	u8 irefall;
+	u8 eflag_base;
 };
 
 static const struct pca995x_chipdef pca9952_chipdef = {
 	.num_leds	= 16,
 	.pwm_base	= 0x0a,
 	.irefall	= 0x43,
+	.eflag_base	= 0x44,
 };
 
 static const struct pca995x_chipdef pca9955b_chipdef = {
 	.num_leds	= 16,
 	.pwm_base	= 0x08,
 	.irefall	= 0x45,
+	.eflag_base	= 0x46,
 };
 
 static const struct pca995x_chipdef pca9956b_chipdef = {
 	.num_leds	= 24,
 	.pwm_base	= 0x0a,
 	.irefall	= 0x40,
+	.eflag_base	= 0x41,
 };
 
 struct pca995x_led {

@@ -112,6 +126,83 @@ static int pca995x_brightness_set(struct led_classdev *led_cdev,
 	}
 }
 
+static ssize_t status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pca995x_led *led = ldev_to_led(led_cdev);
+	struct pca995x_chip *chip = led->chip;
+	const struct pca995x_chipdef *chipdef = chip->chipdef;
+	const char *status = "unknown";
+	unsigned int val;
+	int shift, ret;
+	u8 reg;
+
+	reg = chipdef->eflag_base + (led->led_no / PCA995X_OUTPUTS_PER_REG);
+	shift = PCA995X_EFLAG_BITS * (led->led_no % PCA995X_OUTPUTS_PER_REG);
+
+	ret = regmap_read(chip->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	switch ((val >> shift) & PCA995X_EFLAG_MASK) {
+	case 0:
+		status = "okay";
+		break;
+	case 1:
+		status = "short-circuit";
+		break;
+	case 2:
+		status = "open-circuit";
+	}
+
+	return sysfs_emit(buf, "%s\n", status);
+}
+
+static DEVICE_ATTR_RO(status);

drivers/leds/leds-pca995x.c static int pca995x_brightness_set(struct led_classdev *led_cdev, · line 171

Quoted 10 lines

drivers/leds/leds-pca995x.c +10 -0

+
+static struct attribute *pca995x_led_attrs[] = {
+	&dev_attr_status.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(pca995x_led);
+
+static ssize_t has_errors_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));

Why not 'dev_get_drvdata(dev)' instead of bouncing through the i2c_client?

drivers/leds/leds-pca995x.c static int pca995x_brightness_set(struct led_classdev *led_cdev, · line 179

Quoted 8 lines

drivers/leds/leds-pca995x.c +8 -0

+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(chip->regmap, PCA995X_MODE2, &val);
+	if (ret)
+		return ret;
+
+

Stray blank line.

drivers/leds/leds-pca995x.c static int pca995x_brightness_set(struct led_classdev *led_cdev, · line 186

Quoted 7 lines

drivers/leds/leds-pca995x.c +7 -0

+	return sysfs_emit(buf, "%d\n", !!(val & PCA995X_MODE2_ERROR));
+}
+
+static ssize_t has_errors_store(struct device *dev, struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));

As above.

Quoted 44 lines

drivers/leds/leds-pca995x.c +25 -0

+	int ret;
+
+	if (!sysfs_streq(buf, "clear"))
+		return -EINVAL;
+
+	ret = regmap_update_bits(chip->regmap, PCA995X_MODE2,
+				 PCA995X_MODE2_CLRERR, PCA995X_MODE2_CLRERR);
+
+	return ret ?: count;
+}
+
+static DEVICE_ATTR_RW(has_errors);
+
+static struct attribute *pca995x_attrs[] = {
+	&dev_attr_has_errors.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(pca995x);
+
 static const struct regmap_config pca995x_regmap = {
 	.reg_bits = 8,
 	.val_bits = 8,

@@ -176,6 +267,7 @@ static int pca995x_probe(struct i2c_client *client)
 		led->led_no = reg;
 		led->ldev.brightness_set_blocking = pca995x_brightness_set;
 		led->ldev.max_brightness = 255;
+		led->ldev.groups = pca995x_led_groups;
 	}
 
 	/* Disable LED all-call address and set normal mode */

@@ -183,11 +275,20 @@ static int pca995x_probe(struct i2c_client *client)
 	if (ret)
 		goto err_put_nodes;
 
+	/* Clear errors on probe */
+	ret = regmap_write(chip->regmap, PCA995X_MODE2, PCA995X_MODE2_CFG);
+	if (ret)
+		goto err_put_nodes;
+
 	/* IREF Output current value for all LEDn outputs */
 	ret = regmap_write(chip->regmap, chipdef->irefall, iref);
 	if (ret)
 		goto err_put_nodes;
 

drivers/leds/leds-pca995x.c static int pca995x_probe(struct i2c_client *client) · line 288

Quoted 1 line

drivers/leds/leds-pca995x.c +1 -0

+	ret = device_add_groups(dev, pca995x_groups);

Why not devm_* and remove the clean-up?

Quoted 17 lines

drivers/leds/leds-pca995x.c +4 -1

+	if (ret)
+		goto err_put_nodes;
+
 	for (i = 0; i < chipdef->num_leds; i++) {
 		struct led_init_data init_data = {};
 

@@ -202,7 +303,7 @@ static int pca995x_probe(struct i2c_client *client)
 		if (ret < 0) {
 			dev_err_probe(dev, ret, "Could not register LED %s\n",
 				      chip->leds[i].ldev.name);
-			goto err_put_nodes;
+			goto err_remove_groups;
 		}
 
 		led_fwnodes[i] = NULL;

drivers/leds/leds-pca995x.c static int pca995x_probe(struct i2c_client *client) · line 316

Quoted 6 lines

drivers/leds/leds-pca995x.c +3 -0

@@ -210,6 +311,9 @@ static int pca995x_probe(struct i2c_client *client)
 
 	return 0;
 
+err_remove_groups:
+	device_remove_groups(dev, pca995x_groups);
+

You can remove this if you move to devm_*.

drivers/leds/leds-pca995x.c static int pca995x_probe(struct i2c_client *client) · line 328

Quoted 12 lines

drivers/leds/leds-pca995x.c +5 -0

 err_put_nodes:
 	for (i = 0; i < chipdef->num_leds; i++)
 		fwnode_handle_put(led_fwnodes[i]);

@@ -217,6 +321,11 @@ static int pca995x_probe(struct i2c_client *client)
 	return ret;
 }
 
+static void pca995x_remove(struct i2c_client *client)
+{
+	device_remove_groups(&client->dev, pca995x_groups);
+}
+

As above.

Quoted 18 lines

drivers/leds/leds-pca995x.c +1 -0

 static const struct i2c_device_id pca995x_id[] = {
 	{ .name = "pca9952", .driver_data = (kernel_ulong_t)&pca9952_chipdef },
 	{ .name = "pca9955b", .driver_data = (kernel_ulong_t)&pca9955b_chipdef },

@@ -239,6 +348,7 @@ static struct i2c_driver pca995x_driver = {
 		.of_match_table = pca995x_of_match,
 	},
 	.probe = pca995x_probe,
+	.remove = pca995x_remove,
 	.id_table = pca995x_id,
 };
 module_i2c_driver(pca995x_driver);

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

When LEDs are set to PWM mode with group control enabled, their brightness can control using a global "group PWM" setting, modulating the individual LEDs' brightness with a second PWM running at a different frequency. This lowers the minimum duty cycle from 1/256 to 1/65536 (averaged over the modulated signal). Group brightness control is particularly useful to adjust for different levels of ambient light.

For simplicity, group PWM mode is always enabled, with the reset default of 255 as group brightness. This reduces the effective duty cycle by 1/256 at all individual brightness levels (the individual PWM signals are modulated with the 255/256 duty cycle group PWM), which should be imperceptible.

Signed-off-by Nora Schiffer <[email protected]>

---
 drivers/leds/leds-pca995x.c | 46 ++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

drivers/leds/leds-pca995x.c +5 -0

index 61d2581b976ce..d6438b576b186 100644
@@ -35,6 +35,7 @@
 #define PCA995X_LED_OFF			0x0
 #define PCA995X_LED_ON			0x1
 #define PCA995X_LED_PWM_MODE		0x2
+#define PCA995X_LED_PWM_MODE_GRP	0x3
 #define PCA995X_LDRX_MASK		0x3
 #define PCA995X_LDRX_BITS		2
 

@@ -52,6 +53,7 @@
 struct pca995x_chipdef {
 	unsigned int num_leds;
 	u8 pwm_base;
+	u8 grppwm;
 	u8 irefall;
 	u8 eflag_base;
 };

@@ -59,6 +61,7 @@ struct pca995x_chipdef {
 static const struct pca995x_chipdef pca9952_chipdef = {
 	.num_leds	= 16,
 	.pwm_base	= 0x0a,
+	.grppwm		= 0x08,
 	.irefall	= 0x43,
 	.eflag_base	= 0x44,
 };

@@ -66,6 +69,7 @@ static const struct pca995x_chipdef pca9952_chipdef = {
 static const struct pca995x_chipdef pca9955b_chipdef = {
 	.num_leds	= 16,
 	.pwm_base	= 0x08,
+	.grppwm		= 0x06,
 	.irefall	= 0x45,
 	.eflag_base	= 0x46,
 };

@@ -73,6 +77,7 @@ static const struct pca995x_chipdef pca9955b_chipdef = {
 static const struct pca995x_chipdef pca9956b_chipdef = {
 	.num_leds	= 24,
 	.pwm_base	= 0x0a,
+	.grppwm		= 0x08,
 	.irefall	= 0x40,
 	.eflag_base	= 0x41,
 };

drivers/leds/leds-pca995x.c +34 -1

+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(chip->regmap, chip->chipdef->grppwm, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", val);
+}
+
+static ssize_t group_brightness_store(struct device *dev, struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));
+	u8 val;
+	int ret;
+
+	ret = kstrtou8(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(chip->regmap, chip->chipdef->grppwm, val);
+
+	return ret ?: count;
+}
+
 static DEVICE_ATTR_RW(has_errors);
+static DEVICE_ATTR_RW(group_brightness);
 
 static struct attribute *pca995x_attrs[] = {
 	&dev_attr_has_errors.attr,
+	&dev_attr_group_brightness.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(pca995x);

@@ -270,11 +305,16 @@ static int pca995x_probe(struct i2c_client *client)
 	if (ret)
 		goto err_put_nodes;
 
-	/* Clear errors on probe */
+	/* Clear errors on probe, use GRPPWM register for group brightness control */
 	ret = regmap_write(chip->regmap, PCA995X_MODE2, PCA995X_MODE2_CFG);
 	if (ret)
 		goto err_put_nodes;
 
+	/* Full group brightness */
+	ret = regmap_write(chip->regmap, chipdef->grppwm, U8_MAX);
+	if (ret)
+		goto err_put_nodes;
+
 	/* IREF Output current value for all LEDn outputs */
 	ret = regmap_write(chip->regmap, chipdef->irefall, iref);
 	if (ret)

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

--- checkpatch.pl: clean (0 issues) ---

Quoted 64 lines · Nora Schiffer

When LEDs are set to PWM mode with group control enabled, their brightness can control using a global "group PWM" setting, modulating the individual LEDs' brightness with a second PWM running at a different frequency. This lowers the minimum duty cycle from 1/256 to 1/65536 (averaged over the modulated signal). Group brightness control is particularly useful to adjust for different levels of ambient light.

For simplicity, group PWM mode is always enabled, with the reset default of 255 as group brightness. This reduces the effective duty cycle by 1/256 at all individual brightness levels (the individual PWM signals are modulated with the 255/256 duty cycle group PWM), which should be imperceptible.

Signed-off-by Nora Schiffer <[email protected]>

---
 drivers/leds/leds-pca995x.c | 46 ++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

drivers/leds/leds-pca995x.c +5 -0

index 61d2581b976ce..d6438b576b186 100644
@@ -35,6 +35,7 @@
 #define PCA995X_LED_OFF			0x0
 #define PCA995X_LED_ON			0x1
 #define PCA995X_LED_PWM_MODE		0x2
+#define PCA995X_LED_PWM_MODE_GRP	0x3
 #define PCA995X_LDRX_MASK		0x3
 #define PCA995X_LDRX_BITS		2
 

@@ -52,6 +53,7 @@
 struct pca995x_chipdef {
 	unsigned int num_leds;
 	u8 pwm_base;
+	u8 grppwm;
 	u8 irefall;
 	u8 eflag_base;
 };

@@ -59,6 +61,7 @@ struct pca995x_chipdef {
 static const struct pca995x_chipdef pca9952_chipdef = {
 	.num_leds	= 16,
 	.pwm_base	= 0x0a,
+	.grppwm		= 0x08,
 	.irefall	= 0x43,
 	.eflag_base	= 0x44,
 };

@@ -66,6 +69,7 @@ static const struct pca995x_chipdef pca9952_chipdef = {
 static const struct pca995x_chipdef pca9955b_chipdef = {
 	.num_leds	= 16,
 	.pwm_base	= 0x08,
+	.grppwm		= 0x06,
 	.irefall	= 0x45,
 	.eflag_base	= 0x46,
 };

@@ -73,6 +77,7 @@ static const struct pca995x_chipdef pca9955b_chipdef = {
 static const struct pca995x_chipdef pca9956b_chipdef = {
 	.num_leds	= 24,
 	.pwm_base	= 0x0a,
+	.grppwm		= 0x08,
 	.irefall	= 0x40,
 	.eflag_base	= 0x41,
 };

@@ -114,11 +119,10 @@ static int pca995x_brightness_set(struct led_classdev *led_cdev,
 

drivers/leds/leds-pca995x.c static int pca995x_brightness_set(struct led_classdev *led_cdev, · line 125

Quoted 8 lines

drivers/leds/leds-pca995x.c +1 -2

 	/*
 	 * Change LDRx configuration to individual brightness via PWM.
-	 * LED will stop blinking if it's doing so.
 	 */
 	return regmap_update_bits(chip->regmap, ledout_addr,
 				  PCA995X_LDRX_MASK << shift,
-				  PCA995X_LED_PWM_MODE << shift);
+				  PCA995X_LED_PWM_MODE_GRP << shift);

If we use group PWM mode to scale brightness, what happens when 'brightness' is 'LED_FULL'? The 'switch' statement above sets 'PCA995X_LED_ON', which typically bypasses PWM entirely. Won't this cause an LED set to 255 to ignore the group brightness and be fully on, while an LED at 254 is scaled down?

drivers/leds/leds-pca995x.c static ssize_t has_errors_store(struct device *dev, struct device_attribute *att · line 199

Quoted 11 lines

drivers/leds/leds-pca995x.c +3 -0

 }
 
 static ssize_t status_show(struct device *dev, struct device_attribute *attr, char *buf)

@@ -190,10 +194,41 @@ static ssize_t has_errors_store(struct device *dev, struct device_attribute *att
 	return ret ?: count;
 }
 
+static ssize_t group_brightness_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));

'dev_get_drvdata(dev)' again?

Quoted 60 lines

drivers/leds/leds-pca995x.c +34 -1

+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(chip->regmap, chip->chipdef->grppwm, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", val);
+}
+
+static ssize_t group_brightness_store(struct device *dev, struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));
+	u8 val;
+	int ret;
+
+	ret = kstrtou8(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(chip->regmap, chip->chipdef->grppwm, val);
+
+	return ret ?: count;
+}
+
 static DEVICE_ATTR_RW(has_errors);
+static DEVICE_ATTR_RW(group_brightness);
 
 static struct attribute *pca995x_attrs[] = {
 	&dev_attr_has_errors.attr,
+	&dev_attr_group_brightness.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(pca995x);

@@ -270,11 +305,16 @@ static int pca995x_probe(struct i2c_client *client)
 	if (ret)
 		goto err_put_nodes;
 
-	/* Clear errors on probe */
+	/* Clear errors on probe, use GRPPWM register for group brightness control */
 	ret = regmap_write(chip->regmap, PCA995X_MODE2, PCA995X_MODE2_CFG);
 	if (ret)
 		goto err_put_nodes;
 
+	/* Full group brightness */
+	ret = regmap_write(chip->regmap, chipdef->grppwm, U8_MAX);
+	if (ret)
+		goto err_put_nodes;
+
 	/* IREF Output current value for all LEDn outputs */
 	ret = regmap_write(chip->regmap, chipdef->irefall, iref);
 	if (ret)

Signature · 4 lines

TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/