[PATCH 0/2] Panasonic AN30259A support

[PATCH 0/2] Panasonic AN30259A support

[Simon Shields]

Hi,

This patch series adds DT bindings (patch #1) and the corresponding driver
(patch #2) for the Panasonic AN30259A 3-channel LED driver. AN30259A
uses an internal clock for controlling brightness/on-off cycles, but also supports
using an external PWM/clock input. This patch series only implements
support for the former.

The AN30259A is connected using I2C, and the datasheet is freely
available[0].

Cheers,
Simon

[0]: https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf

Simon Shields (2):
  dt-bindings: leds: document Panasonic AN30259A bindings
  leds: add Panasonic AN30259A support

 .../devicetree/bindings/leds/leds-an30259a.txt     |  41 +++
 drivers/leds/Kconfig                               |  10 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-an30259a.c                       | 338 +++++++++++++++++++++
 4 files changed, 390 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-an30259a.txt
 create mode 100644 drivers/leds/leds-an30259a.c

-- 
2.16.2

[PATCH 1/2] dt-bindings: leds: document Panasonic AN30259A bindings

[Simon Shields]

Signed-off-by: Simon Shields <simon@lineageos.org>
---
 .../devicetree/bindings/leds/leds-an30259a.txt     | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-an30259a.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-an30259a.txt b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
new file mode 100644
index 000000000000..d3586ce71eef
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
@@ -0,0 +1,41 @@
+* Panasonic AN30259A 3-channel LED driver
+
+The AN30259A is a LED controller capable of driving three LEDs independently. It supports
+constant current output and sloping current output modes. The chip is connected over I2C.
+
+Required properties:
+	- compatible : must be "panasonic,an30259a"
+	- reg -  I2C slave address
+
+Each led is represented as a sub-node of the panasonic,an30259a node.
+
+Optional sub-node properties:
+	- label: see Documentation/devicetree/bindings/leds/common.txt
+	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
+	- led-sources: 0 if LED is connected to LED1 pin,
+	  1 if LED is connected to LED2 pin, or 2 if LED is connected to LED3 pin.
+
+Example:
+an30259a@30 {
+	compatible = "panasonic,an30259a";
+	reg = <0x30>;
+
+	red {
+		led-sources = <0>;
+		linux,default-trigger = "heartbeat";
+		label = "red";
+	};
+
+	green {
+		led-sources = <1>;
+		label = "green";
+	};
+
+	blue {
+		led-sources = <2>;
+		label = "blue";
+	};
+};
+
+For more information please see the link below:
+https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
-- 
2.16.2

[PATCH 2/2] leds: add Panasonic AN30259A support

[Simon Shields]

AN30259A is a 3-channel LED driver which uses I2C. It supports timed
operation via an internal PWM clock, and variable brightness. This
driver offers support for basic hardware-based blinking and brightness
control.

The datasheet is freely available:
https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf

Signed-off-by: Simon Shields <simon@lineageos.org>
---
 drivers/leds/Kconfig         |  10 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-an30259a.c | 338 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 349 insertions(+)
 create mode 100644 drivers/leds/leds-an30259a.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 3e763d2a0cb3..80bed557cc6b 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -57,6 +57,16 @@ config LEDS_AAT1290
 	depends on PINCTRL
 	help
 	 This option enables support for the LEDs on the AAT1290.
+
+config LEDS_AN30259A
+    tristate "LED support for Panasonic AN30259A"
+    depends on OF
+    depends on I2C
+    depends on LEDS_CLASS
+    help
+     This option enables support for the AN30259A 3-channel
+     LED driver.
+
 config LEDS_APU
 	tristate "Front panel LED support for PC Engines APU/APU2 boards"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 987884a5b9a5..44f9b42d1600 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
 obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
 obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
 obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
+obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
new file mode 100644
index 000000000000..b51faf67e7de
--- /dev/null
+++ b/drivers/leds/leds-an30259a.c
@@ -0,0 +1,338 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#define MAX_LEDS 3
+
+#define REG_SRESET 0x00
+#define LED_SRESET (1 << 0)
+
+/* LED power registers */
+#define REG_LEDON 0x01
+#define LED_EN(x) (1 << (x))
+#define LED_SLOPE(x) (1 << ((x) + 4))
+
+#define REG_LEDCC(x) (0x03 + (x))
+
+/* slope control registers */
+#define REG_SLOPE(x) (0x06 + (x))
+#define LED_SLOPETIME1(x) (x)
+#define LED_SLOPETIME2(x) ((x) << 4)
+
+#define REG_LEDCNT1(x) (0x09 + (4 * (x)))
+#define LED_DUTYMAX(x) ((x) << 4)
+#define LED_DUTYMID(x) (x)
+
+#define REG_LEDCNT2(x) (0x0A + (4 * (x)))
+#define LED_DELAY(x) ((x) << 4)
+#define LED_DUTYMIN(x) (x)
+
+/* detention time control (length of each slope step) */
+#define REG_LEDCNT3(x) (0x0B + (4 * (x)))
+#define LED_DT1(x) (x)
+#define LED_DT2(x) ((x) << 4)
+
+#define REG_LEDCNT4(x) (0x0C + (4 * (x)))
+#define LED_DT3(x) (x)
+#define LED_DT4(x) ((x) << 4)
+
+#define REG_MAX 0x14
+
+#define BLINK_MAX_TIME 7500 /* ms */
+
+struct an30259a;
+
+struct an30259a_led {
+	struct an30259a *chip;
+	struct led_classdev cdev;
+	int num;
+	char name[32];
+};
+
+struct an30259a {
+	struct mutex mutex;
+	struct i2c_client *client;
+	struct an30259a_led leds[MAX_LEDS];
+	struct regmap *regmap;
+};
+
+static u8 an30259a_get_duty_max(u8 brightness)
+{
+	u8 duty_max, floor, ceil;
+
+	/* squash 8 bit number into 7-bit PWM range */
+	duty_max = brightness >> 1;
+
+	/* bottom 3 bits are always set for DUTYMAX
+	 * figure out the closest value
+	 */
+	ceil = duty_max | 0x7;
+	floor = ceil - 0x8;
+
+	if ((duty_max - floor) < (ceil - duty_max))
+		duty_max = floor >> 3;
+	else
+		duty_max = ceil >> 3;
+
+	return duty_max;
+}
+
+static int an30259a_brightness(struct an30259a_led *led,
+		enum led_brightness brightness)
+{
+	int ret;
+	unsigned int ledon;
+	u8 duty_max;
+
+	ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon);
+	switch (brightness) {
+	case LED_OFF:
+		ledon &= ~LED_EN(led->num);
+		ledon &= ~LED_SLOPE(led->num);
+		break;
+	default:
+		ledon |= LED_EN(led->num);
+		duty_max = an30259a_get_duty_max(brightness & 0xff);
+		ret = regmap_write(led->chip->regmap,
+				REG_LEDCNT1(led->num),
+				LED_DUTYMAX(duty_max) | LED_DUTYMID(duty_max));
+		if (ret)
+			return ret;
+		break;
+	}
+	ret = regmap_write(led->chip->regmap, REG_LEDON,
+				ledon);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(led->chip->regmap, REG_LEDCC(led->num),
+			brightness & 0xff);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static int an30259a_blink(struct an30259a_led *led,
+		unsigned long *poff, unsigned long *pon)
+{
+	int ret, num = led->num;
+	unsigned int ledon;
+	unsigned long off = *poff, on = *pon;
+
+	/* slope time - multiples of 500ms only */
+	off -= off % 500;
+	if (!off)
+		off += 500;
+	else if (off > BLINK_MAX_TIME)
+		off = BLINK_MAX_TIME;
+	*poff = off;
+	off /= 500;
+
+	on -= on % 500;
+	if (!on)
+		on += 500;
+	else if (on > BLINK_MAX_TIME)
+		on = BLINK_MAX_TIME;
+	*pon = on;
+	on /= 500;
+
+	/* duty min should be zero (=off), delay should be zero */
+	ret = regmap_write(led->chip->regmap, REG_LEDCNT2(num),
+			LED_DELAY(0) | LED_DUTYMIN(0));
+	if (ret)
+		return ret;
+
+	/* reset detention time (no "breathing" effect) */
+	ret = regmap_write(led->chip->regmap, REG_LEDCNT3(num),
+			LED_DT1(0) | LED_DT2(0));
+	if (ret)
+		return ret;
+	ret = regmap_write(led->chip->regmap, REG_LEDCNT4(num),
+			LED_DT3(0) | LED_DT4(0));
+	if (ret)
+		return ret;
+
+	/* slope time controls on/off cycle length */
+	ret = regmap_write(led->chip->regmap, REG_SLOPE(num),
+			LED_SLOPETIME1(off) | LED_SLOPETIME2(on));
+	if (ret)
+		return ret;
+
+	/* Finally, enable slope mode. */
+	ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon);
+	if (ret)
+		return ret;
+	ledon |= LED_SLOPE(num);
+	return regmap_write(led->chip->regmap, REG_LEDON,
+			ledon);
+}
+
+static int an30259a_led_set(struct led_classdev *cdev,
+		enum led_brightness value)
+{
+	struct an30259a_led *led;
+	int ret;
+
+	led = container_of(cdev, struct an30259a_led, cdev);
+
+	mutex_lock(&led->chip->mutex);
+
+	ret = an30259a_brightness(led, value);
+
+	mutex_unlock(&led->chip->mutex);
+	return ret;
+}
+
+static int an30259a_blink_set(struct led_classdev *cdev,
+		unsigned long *delay_off, unsigned long *delay_on)
+{
+	struct an30259a_led *led;
+	int ret;
+
+	led = container_of(cdev, struct an30259a_led, cdev);
+
+	mutex_lock(&led->chip->mutex);
+
+	ret = an30259a_blink(led, delay_off, delay_on);
+
+	mutex_unlock(&led->chip->mutex);
+	return ret;
+}
+
+static struct led_platform_data *
+an30259a_dt_init(struct i2c_client *client)
+{
+	struct led_platform_data *pdata;
+	struct device_node *np = client->dev.of_node, *child;
+	struct led_info *leds;
+	int count;
+	int res;
+
+	count = of_get_child_count(np);
+	if (!count || count > MAX_LEDS)
+		return ERR_PTR(-EINVAL);
+
+	leds = devm_kzalloc(&client->dev, sizeof(*leds) * count, GFP_KERNEL);
+	if (!leds)
+		return ERR_PTR(-ENOMEM);
+
+	for_each_child_of_node(np, child) {
+		u32 source;
+
+		res = of_property_read_u32(child, "led-sources", &source);
+		if ((res != 0) || source >= MAX_LEDS)
+			continue;
+
+		leds[source].name = of_get_property(child, "label", NULL);
+		leds[source].default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);
+	}
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->leds = leds;
+	pdata->num_leds = MAX_LEDS;
+
+	return pdata;
+}
+
+static const struct regmap_config an30259a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = REG_MAX,
+};
+
+static int an30259a_probe(struct i2c_client *client)
+{
+	struct led_platform_data *pdata;
+	struct an30259a *chip;
+	int i, err;
+
+	pdata = an30259a_dt_init(client);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	mutex_init(&chip->mutex);
+	chip->client = client;
+	chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config);
+
+	i2c_set_clientdata(client, chip);
+
+	/* Reset the IC */
+	regmap_write(chip->regmap, REG_SRESET, LED_SRESET);
+
+	for (i = 0; i < pdata->num_leds; i++) {
+		chip->leds[i].num = i;
+		chip->leds[i].chip = chip;
+
+		if (pdata->leds[i].name)
+			snprintf(chip->leds[i].name, sizeof(chip->leds[i].name),
+					"an30259a:%s", pdata->leds[i].name);
+		else
+			snprintf(chip->leds[i].name, sizeof(chip->leds[i].name),
+					"an30259a:%d:%.2x:%d",
+					client->adapter->nr, client->addr, i);
+		if (pdata->leds[i].default_trigger)
+			chip->leds[i].cdev.default_trigger =
+				pdata->leds[i].default_trigger;
+
+		chip->leds[i].cdev.name = chip->leds[i].name;
+
+		chip->leds[i].cdev.brightness_set_blocking = an30259a_led_set;
+		chip->leds[i].cdev.blink_set = an30259a_blink_set;
+
+		err = led_classdev_register(&client->dev, &chip->leds[i].cdev);
+		if (err < 0)
+			goto exit;
+	}
+	return 0;
+
+exit:
+	while (i--)
+		led_classdev_unregister(&chip->leds[i].cdev);
+	return err;
+}
+
+static int an30259a_remove(struct i2c_client *client)
+{
+	struct an30259a *chip = i2c_get_clientdata(client);
+	int i;
+
+	for (i = 0; i < MAX_LEDS; i++)
+		led_classdev_unregister(&chip->leds[i].cdev);
+
+	return 0;
+}
+
+static const struct of_device_id an30259a_match_table[] = {
+	{ .compatible = "panasonic,an30259a", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, an30259a_match_table);
+
+static struct i2c_driver an30259a_driver = {
+	.driver = {
+		.name = "leds-an32059a",
+		.of_match_table = of_match_ptr(an30259a_match_table),
+	},
+	.probe_new = an30259a_probe,
+	.remove = an30259a_remove,
+};
+
+module_i2c_driver(an30259a_driver);
+
+MODULE_AUTHOR("Simon Shields <simon@lineageos.org>");
+MODULE_DESCRIPTION("AN32059A LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.16.2

Re: [PATCH 1/2] dt-bindings: leds: document Panasonic AN30259A bindings

[Jacek Anaszewski]

Hi Simon,

Thank you for the patch. I have a few comments below.

On 02/20/2018 01:54 AM, Simon Shields wrote:
> Signed-off-by: Simon Shields <simon@lineageos.org>
> ---
>  .../devicetree/bindings/leds/leds-an30259a.txt     | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-an30259a.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-an30259a.txt b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> new file mode 100644
> index 000000000000..d3586ce71eef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> @@ -0,0 +1,41 @@
> +* Panasonic AN30259A 3-channel LED driver
> +
> +The AN30259A is a LED controller capable of driving three LEDs independently. It supports
> +constant current output and sloping current output modes. The chip is connected over I2C.
> +
> +Required properties:
> +	- compatible : must be "panasonic,an30259a"

s/must/Must/ and please put a dot in the end.

> +	- reg -  I2C slave address
> +
> +Each led is represented as a sub-node of the panasonic,an30259a node.

s/led/LED/

> +
> +Optional sub-node properties:
> +	- label: see Documentation/devicetree/bindings/leds/common.txt
> +	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
> +	- led-sources: 0 if LED is connected to LED1 pin,

led-sources is applicable in e bit different arrangements.
Here please use "reg" property instead. In this case you will
need also below in the "Required properties":

- #address-cells: Must be 1.
- #size-cells: Must be 0.


> +	  1 if LED is connected to LED2 pin, or 2 if LED is connected to LED3 pin.
> +
> +Example:
> +an30259a@30 {

According to the lesson from Rob [0] it should be:

led-controller@30 {
  #address-cells = <1>;
  #size-cells = <0>
  reg = <30>;

  led0 {
    reg = <0>;
    label = "red";	
  };

  green@1 {
    reg = <1>;
    label = "green";
  };

  blue@2 {
    reg = <2>;
    label = "blue";
  };
};


> +	compatible = "panasonic,an30259a";
> +	reg = <0x30>;
> +
> +	red {
> +		led-sources = <0>;
> +		linux,default-trigger = "heartbeat";
> +		label = "red";
> +	};
> +
> +	green {
> +		led-sources = <1>;
> +		label = "green";
> +	};
> +
> +	blue {
> +		led-sources = <2>;
> +		label = "blue";
> +	};
> +};
> +
> +For more information please see the link below:
> +https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
> 

[0] https://patchwork.kernel.org/patch/10093757/

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 2/2] leds: add Panasonic AN30259A support

[Jacek Anaszewski]

Hi Simon,

Thank you for the patch. Please refer to my comments below.

On 02/20/2018 01:54 AM, Simon Shields wrote:
> AN30259A is a 3-channel LED driver which uses I2C. It supports timed
> operation via an internal PWM clock, and variable brightness. This
> driver offers support for basic hardware-based blinking and brightness
> control.
> 
> The datasheet is freely available:
> https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
> 
> Signed-off-by: Simon Shields <simon@lineageos.org>
> ---
>  drivers/leds/Kconfig         |  10 ++
>  drivers/leds/Makefile        |   1 +
>  drivers/leds/leds-an30259a.c | 338 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 349 insertions(+)
>  create mode 100644 drivers/leds/leds-an30259a.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 3e763d2a0cb3..80bed557cc6b 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -57,6 +57,16 @@ config LEDS_AAT1290
>  	depends on PINCTRL
>  	help
>  	 This option enables support for the LEDs on the AAT1290.
> +
> +config LEDS_AN30259A
> +    tristate "LED support for Panasonic AN30259A"
> +    depends on OF
> +    depends on I2C
> +    depends on LEDS_CLASS
> +    help
> +     This option enables support for the AN30259A 3-channel
> +     LED driver.
> +
>  config LEDS_APU
>  	tristate "Front panel LED support for PC Engines APU/APU2 boards"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 987884a5b9a5..44f9b42d1600 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
>  obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
>  obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
>  obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
> +obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
>  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
>  obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
>  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
> diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
> new file mode 100644
> index 000000000000..b51faf67e7de
> --- /dev/null
> +++ b/drivers/leds/leds-an30259a.c
> @@ -0,0 +1,338 @@
> +// SPDX-License-Identifier: GPL-2.0

You could mention yourself as an Author here too.

> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +#define MAX_LEDS 3
> +
> +#define REG_SRESET 0x00
> +#define LED_SRESET (1 << 0)
> +
> +/* LED power registers */
> +#define REG_LEDON 0x01
> +#define LED_EN(x) (1 << (x))
> +#define LED_SLOPE(x) (1 << ((x) + 4))
> +
> +#define REG_LEDCC(x) (0x03 + (x))
> +
> +/* slope control registers */
> +#define REG_SLOPE(x) (0x06 + (x))
> +#define LED_SLOPETIME1(x) (x)
> +#define LED_SLOPETIME2(x) ((x) << 4)
> +
> +#define REG_LEDCNT1(x) (0x09 + (4 * (x)))
> +#define LED_DUTYMAX(x) ((x) << 4)
> +#define LED_DUTYMID(x) (x)
> +
> +#define REG_LEDCNT2(x) (0x0A + (4 * (x)))
> +#define LED_DELAY(x) ((x) << 4)
> +#define LED_DUTYMIN(x) (x)
> +
> +/* detention time control (length of each slope step) */
> +#define REG_LEDCNT3(x) (0x0B + (4 * (x)))
> +#define LED_DT1(x) (x)
> +#define LED_DT2(x) ((x) << 4)
> +
> +#define REG_LEDCNT4(x) (0x0C + (4 * (x)))
> +#define LED_DT3(x) (x)
> +#define LED_DT4(x) ((x) << 4)
> +
> +#define REG_MAX 0x14
> +
> +#define BLINK_MAX_TIME 7500 /* ms */
> +
> +struct an30259a;
> +
> +struct an30259a_led {
> +	struct an30259a *chip;
> +	struct led_classdev cdev;
> +	int num;
> +	char name[32];
> +};
> +
> +struct an30259a {
> +	struct mutex mutex;
> +	struct i2c_client *client;
> +	struct an30259a_led leds[MAX_LEDS];
> +	struct regmap *regmap;
> +};
> +
> +static u8 an30259a_get_duty_max(u8 brightness)
> +{
> +	u8 duty_max, floor, ceil;
> +
> +	/* squash 8 bit number into 7-bit PWM range */
> +	duty_max = brightness >> 1;
> +
> +	/* bottom 3 bits are always set for DUTYMAX
> +	 * figure out the closest value
> +	 */
> +	ceil = duty_max | 0x7;
> +	floor = ceil - 0x8;
> +
> +	if ((duty_max - floor) < (ceil - duty_max))
> +		duty_max = floor >> 3;
> +	else
> +		duty_max = ceil >> 3;
> +
> +	return duty_max;
> +}
> +
> +static int an30259a_brightness(struct an30259a_led *led,
> +		enum led_brightness brightness)
> +{
> +	int ret;
> +	unsigned int ledon;
> +	u8 duty_max;
> +
> +	ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon);

Please put an empty line here - it improves readability.

> +	switch (brightness) {
> +	case LED_OFF:
> +		ledon &= ~LED_EN(led->num);
> +		ledon &= ~LED_SLOPE(led->num);
> +		break;
> +	default:
> +		ledon |= LED_EN(led->num);
> +		duty_max = an30259a_get_duty_max(brightness & 0xff);
> +		ret = regmap_write(led->chip->regmap,
> +				REG_LEDCNT1(led->num),
> +				LED_DUTYMAX(duty_max) | LED_DUTYMID(duty_max));
> +		if (ret)
> +			return ret;
> +		break;
> +	}
> +	ret = regmap_write(led->chip->regmap, REG_LEDON,
> +				ledon);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(led->chip->regmap, REG_LEDCC(led->num),
> +			brightness & 0xff);
> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}
> +
> +static int an30259a_blink(struct an30259a_led *led,
> +		unsigned long *poff, unsigned long *pon)
> +{
> +	int ret, num = led->num;
> +	unsigned int ledon;
> +	unsigned long off = *poff, on = *pon;
> +
> +	/* slope time - multiples of 500ms only */
> +	off -= off % 500;
> +	if (!off)
> +		off += 500;
> +	else if (off > BLINK_MAX_TIME)
> +		off = BLINK_MAX_TIME;
> +	*poff = off;
> +	off /= 500;
> +
> +	on -= on % 500;
> +	if (!on)
> +		on += 500;
> +	else if (on > BLINK_MAX_TIME)
> +		on = BLINK_MAX_TIME;
> +	*pon = on;
> +	on /= 500;
> +
> +	/* duty min should be zero (=off), delay should be zero */
> +	ret = regmap_write(led->chip->regmap, REG_LEDCNT2(num),
> +			LED_DELAY(0) | LED_DUTYMIN(0));
> +	if (ret)
> +		return ret;
> +
> +	/* reset detention time (no "breathing" effect) */
> +	ret = regmap_write(led->chip->regmap, REG_LEDCNT3(num),
> +			LED_DT1(0) | LED_DT2(0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(led->chip->regmap, REG_LEDCNT4(num),
> +			LED_DT3(0) | LED_DT4(0));
> +	if (ret)
> +		return ret;
> +
> +	/* slope time controls on/off cycle length */
> +	ret = regmap_write(led->chip->regmap, REG_SLOPE(num),
> +			LED_SLOPETIME1(off) | LED_SLOPETIME2(on));
> +	if (ret)
> +		return ret;
> +
> +	/* Finally, enable slope mode. */
> +	ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon);
> +	if (ret)
> +		return ret;
> +	ledon |= LED_SLOPE(num);

Empty line here please.

> +	return regmap_write(led->chip->regmap, REG_LEDON,
> +			ledon);
> +}
> +
> +static int an30259a_led_set(struct led_classdev *cdev,
> +		enum led_brightness value)
> +{
> +	struct an30259a_led *led;
> +	int ret;
> +
> +	led = container_of(cdev, struct an30259a_led, cdev);
> +
> +	mutex_lock(&led->chip->mutex);
> +
> +	ret = an30259a_brightness(led, value);
> +
> +	mutex_unlock(&led->chip->mutex);
> +	return ret;
> +}
> +
> +static int an30259a_blink_set(struct led_classdev *cdev,
> +		unsigned long *delay_off, unsigned long *delay_on)
> +{
> +	struct an30259a_led *led;
> +	int ret;
> +
> +	led = container_of(cdev, struct an30259a_led, cdev);
> +
> +	mutex_lock(&led->chip->mutex);
> +
> +	ret = an30259a_blink(led, delay_off, delay_on);
> +
> +	mutex_unlock(&led->chip->mutex);
> +	return ret;
> +}
> +
> +static struct led_platform_data *
> +an30259a_dt_init(struct i2c_client *client)
> +{
> +	struct led_platform_data *pdata;
> +	struct device_node *np = client->dev.of_node, *child;
> +	struct led_info *leds;
> +	int count;
> +	int res;
> +
> +	count = of_get_child_count(np);
> +	if (!count || count > MAX_LEDS)
> +		return ERR_PTR(-EINVAL);
> +
> +	leds = devm_kzalloc(&client->dev, sizeof(*leds) * count, GFP_KERNEL);
> +	if (!leds)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for_each_child_of_node(np, child) {
> +		u32 source;
> +
> +		res = of_property_read_u32(child, "led-sources", &source);
> +		if ((res != 0) || source >= MAX_LEDS)
> +			continue;
> +
> +		leds[source].name = of_get_property(child, "label", NULL);
> +		leds[source].default_trigger =
> +			of_get_property(child, "linux,default-trigger", NULL);
> +	}
> +
> +	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdata->leds = leds;
> +	pdata->num_leds = MAX_LEDS;
> +
> +	return pdata;
> +}
> +
> +static const struct regmap_config an30259a_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = REG_MAX,
> +};
> +
> +static int an30259a_probe(struct i2c_client *client)
> +{
> +	struct led_platform_data *pdata;
> +	struct an30259a *chip;
> +	int i, err;
> +
> +	pdata = an30259a_dt_init(client);
> +	if (IS_ERR(pdata))
> +		return PTR_ERR(pdata);
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	mutex_init(&chip->mutex);
> +	chip->client = client;
> +	chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config);
> +
> +	i2c_set_clientdata(client, chip);
> +
> +	/* Reset the IC */
> +	regmap_write(chip->regmap, REG_SRESET, LED_SRESET);
> +
> +	for (i = 0; i < pdata->num_leds; i++) {
> +		chip->leds[i].num = i;
> +		chip->leds[i].chip = chip;
> +
> +		if (pdata->leds[i].name)
> +			snprintf(chip->leds[i].name, sizeof(chip->leds[i].name),
> +					"an30259a:%s", pdata->leds[i].name);
> +		else
> +			snprintf(chip->leds[i].name, sizeof(chip->leds[i].name),
> +					"an30259a:%d:%.2x:%d",

We should have color in the second segment of the LED class device name.
If label is absent, please just leave it blank "::".

> +					client->adapter->nr, client->addr, i);
> +		if (pdata->leds[i].default_trigger)
> +			chip->leds[i].cdev.default_trigger =
> +				pdata->leds[i].default_trigger;
> +
> +		chip->leds[i].cdev.name = chip->leds[i].name;
> +
> +		chip->leds[i].cdev.brightness_set_blocking = an30259a_led_set;
> +		chip->leds[i].cdev.blink_set = an30259a_blink_set;
> +
> +		err = led_classdev_register(&client->dev, &chip->leds[i].cdev);

Please use devm prefixed API here.

> +		if (err < 0)
> +			goto exit;
> +	}
> +	return 0;
> +
> +exit:
> +	while (i--)
> +		led_classdev_unregister(&chip->leds[i].cdev);
> +	return err;
> +}
> +
> +static int an30259a_remove(struct i2c_client *client)
> +{
> +	struct an30259a *chip = i2c_get_clientdata(client);
> +	int i;
> +
> +	for (i = 0; i < MAX_LEDS; i++)
> +		led_classdev_unregister(&chip->leds[i].cdev)

You won't need this loop then, but please add mutex_destroy().

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id an30259a_match_table[] = {
> +	{ .compatible = "panasonic,an30259a", },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, an30259a_match_table);
> +
> +static struct i2c_driver an30259a_driver = {
> +	.driver = {
> +		.name = "leds-an32059a",
> +		.of_match_table = of_match_ptr(an30259a_match_table),
> +	},
> +	.probe_new = an30259a_probe,
> +	.remove = an30259a_remove,
> +};
> +
> +module_i2c_driver(an30259a_driver);
> +
> +MODULE_AUTHOR("Simon Shields <simon@lineageos.org>");
> +MODULE_DESCRIPTION("AN32059A LED driver");
> +MODULE_LICENSE("GPL v2");
> 

You could consider also addressing some (all?) of problems
reported by scripts/checkpatch.pl --strict.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 2/2] leds: add Panasonic AN30259A support

[Pavel Machek]

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

Hi!

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 3e763d2a0cb3..80bed557cc6b 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -57,6 +57,16 @@ config LEDS_AAT1290
>  	depends on PINCTRL
>  	help
>  	 This option enables support for the LEDs on the AAT1290.
> +
> +config LEDS_AN30259A
> +    tristate "LED support for Panasonic AN30259A"
> +    depends on OF
> +    depends on I2C
> +    depends on LEDS_CLASS
> +    help
> +     This option enables support for the AN30259A 3-channel
> +     LED driver.

Something funny goes on with the indentation here. And normally we'd
do depends on A && B && ... and have note how the module will be
called.


> +++ b/drivers/leds/leds-an30259a.c
> @@ -0,0 +1,338 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/i2c.h>

This would be suitable place for author name, and very short hw
description. http link for datasheet would be preffered here over the
dts binding.

> +		duty_max = an30259a_get_duty_max(brightness & 0xff);
> +		ret = regmap_write(led->chip->regmap,
> +				REG_LEDCNT1(led->num),
> +				LED_DUTYMAX(duty_max) | LED_DUTYMID(duty_max));
> +		if (ret)
> +			return ret;
> +		break;
> +	}
> +	ret = regmap_write(led->chip->regmap, REG_LEDON,
> +				ledon);

You can fit this on one line.

> +		chip->leds[i].cdev.brightness_set_blocking = an30259a_led_set;
> +		chip->leds[i].cdev.blink_set = an30259a_blink_set;
> +
> +		err = led_classdev_register(&client->dev, &chip->leds[i].cdev);
> +		if (err < 0)
> +			goto exit;
> +	}

Do ve have devm version of led class register?

> +static int an30259a_remove(struct i2c_client *client)
> +{
> +	struct an30259a *chip = i2c_get_clientdata(client);
> +	int i;
> +
> +	for (i = 0; i < MAX_LEDS; i++)
> +		led_classdev_unregister(&chip->leds[i].cdev);

You always unregister 3 leds; what if only 1 is described in the
device tree?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/2] dt-bindings: leds: document Panasonic AN30259A bindings

[Pavel Machek]

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

Hi!

> Signed-off-by: Simon Shields <simon@lineageos.org>
> ---
>  .../devicetree/bindings/leds/leds-an30259a.txt     | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-an30259a.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-an30259a.txt b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> new file mode 100644
> index 000000000000..d3586ce71eef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> @@ -0,0 +1,41 @@
> +* Panasonic AN30259A 3-channel LED driver
> +
> +The AN30259A is a LED controller capable of driving three LEDs independently. It supports
> +constant current output and sloping current output modes. The chip is connected over I2C.
> +
> +Required properties:
> +	- compatible : must be "panasonic,an30259a"
> +	- reg -  I2C slave address

Might be nice to be consistent, you use 'label: explanation' below.

> +Each led is represented as a sub-node of the panasonic,an30259a node.

LED.

> +Optional sub-node properties:
> +	- label: see Documentation/devicetree/bindings/leds/common.txt
> +	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
> +	- led-sources: 0 if LED is connected to LED1 pin,
> +	  1 if LED is connected to LED2 pin, or 2 if LED is connected to LED3 pin.

Normally, we use 'reg' for that, no?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] leds: add Panasonic AN30259A support

[Simon Shields]

Hi Jacek,

Thanks for the review!

On Tue, Feb 20, 2018 at 10:51:40PM +0100, Jacek Anaszewski wrote:
> Hi Simon,
> 
> Thank you for the patch. Please refer to my comments below.
> 
> On 02/20/2018 01:54 AM, Simon Shields wrote:
> > AN30259A is a 3-channel LED driver which uses I2C. It supports timed
> > operation via an internal PWM clock, and variable brightness. This
> > driver offers support for basic hardware-based blinking and brightness
> > control.
> > 
> > The datasheet is freely available:
> > https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
> > 
> > Signed-off-by: Simon Shields <simon@lineageos.org>
> > ---
> >  drivers/leds/Kconfig         |  10 ++
> >  drivers/leds/Makefile        |   1 +
> >  drivers/leds/leds-an30259a.c | 338 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 349 insertions(+)
> >  create mode 100644 drivers/leds/leds-an30259a.c
> > 
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 3e763d2a0cb3..80bed557cc6b 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -57,6 +57,16 @@ config LEDS_AAT1290
> >  	depends on PINCTRL
> >  	help
> >  	 This option enables support for the LEDs on the AAT1290.
> > +
> > +config LEDS_AN30259A
> > +    tristate "LED support for Panasonic AN30259A"
> > +    depends on OF
> > +    depends on I2C
> > +    depends on LEDS_CLASS
> > +    help
> > +     This option enables support for the AN30259A 3-channel
> > +     LED driver.
> > +
> >  config LEDS_APU
> >  	tristate "Front panel LED support for PC Engines APU/APU2 boards"
> >  	depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 987884a5b9a5..44f9b42d1600 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
> >  obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
> >  obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
> >  obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
> > +obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
> >  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
> >  obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
> >  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
> > diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
> > new file mode 100644
> > index 000000000000..b51faf67e7de
> > --- /dev/null
> > +++ b/drivers/leds/leds-an30259a.c
> > @@ -0,0 +1,338 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> You could mention yourself as an Author here too.
> 
> > +#include <linux/i2c.h>
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +
> > +#define MAX_LEDS 3
> > +
> > +#define REG_SRESET 0x00
> > +#define LED_SRESET (1 << 0)
> > +
> > +/* LED power registers */
> > +#define REG_LEDON 0x01
> > +#define LED_EN(x) (1 << (x))
> > +#define LED_SLOPE(x) (1 << ((x) + 4))
> > +
> > +#define REG_LEDCC(x) (0x03 + (x))
> > +
> > +/* slope control registers */
> > +#define REG_SLOPE(x) (0x06 + (x))
> > +#define LED_SLOPETIME1(x) (x)
> > +#define LED_SLOPETIME2(x) ((x) << 4)
> > +
> > +#define REG_LEDCNT1(x) (0x09 + (4 * (x)))
> > +#define LED_DUTYMAX(x) ((x) << 4)
> > +#define LED_DUTYMID(x) (x)
> > +
> > +#define REG_LEDCNT2(x) (0x0A + (4 * (x)))
> > +#define LED_DELAY(x) ((x) << 4)
> > +#define LED_DUTYMIN(x) (x)
> > +
> > +/* detention time control (length of each slope step) */
> > +#define REG_LEDCNT3(x) (0x0B + (4 * (x)))
> > +#define LED_DT1(x) (x)
> > +#define LED_DT2(x) ((x) << 4)
> > +
> > +#define REG_LEDCNT4(x) (0x0C + (4 * (x)))
> > +#define LED_DT3(x) (x)
> > +#define LED_DT4(x) ((x) << 4)
> > +
> > +#define REG_MAX 0x14
> > +
> > +#define BLINK_MAX_TIME 7500 /* ms */
> > +
> > +struct an30259a;
> > +
> > +struct an30259a_led {
> > +	struct an30259a *chip;
> > +	struct led_classdev cdev;
> > +	int num;
> > +	char name[32];
> > +};
> > +
> > +struct an30259a {
> > +	struct mutex mutex;
> > +	struct i2c_client *client;
> > +	struct an30259a_led leds[MAX_LEDS];
> > +	struct regmap *regmap;
> > +};
> > +
> > +static u8 an30259a_get_duty_max(u8 brightness)
> > +{
> > +	u8 duty_max, floor, ceil;
> > +
> > +	/* squash 8 bit number into 7-bit PWM range */
> > +	duty_max = brightness >> 1;
> > +
> > +	/* bottom 3 bits are always set for DUTYMAX
> > +	 * figure out the closest value
> > +	 */
> > +	ceil = duty_max | 0x7;
> > +	floor = ceil - 0x8;
> > +
> > +	if ((duty_max - floor) < (ceil - duty_max))
> > +		duty_max = floor >> 3;
> > +	else
> > +		duty_max = ceil >> 3;
> > +
> > +	return duty_max;
> > +}
> > +
> > +static int an30259a_brightness(struct an30259a_led *led,
> > +		enum led_brightness brightness)
> > +{
> > +	int ret;
> > +	unsigned int ledon;
> > +	u8 duty_max;
> > +
> > +	ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon);
> 
> Please put an empty line here - it improves readability.
> 
> > +	switch (brightness) {
> > +	case LED_OFF:
> > +		ledon &= ~LED_EN(led->num);
> > +		ledon &= ~LED_SLOPE(led->num);
> > +		break;
> > +	default:
> > +		ledon |= LED_EN(led->num);
> > +		duty_max = an30259a_get_duty_max(brightness & 0xff);
> > +		ret = regmap_write(led->chip->regmap,
> > +				REG_LEDCNT1(led->num),
> > +				LED_DUTYMAX(duty_max) | LED_DUTYMID(duty_max));
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	}
> > +	ret = regmap_write(led->chip->regmap, REG_LEDON,
> > +				ledon);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(led->chip->regmap, REG_LEDCC(led->num),
> > +			brightness & 0xff);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ret;
> > +}
> > +
> > +static int an30259a_blink(struct an30259a_led *led,
> > +		unsigned long *poff, unsigned long *pon)
> > +{
> > +	int ret, num = led->num;
> > +	unsigned int ledon;
> > +	unsigned long off = *poff, on = *pon;
> > +
> > +	/* slope time - multiples of 500ms only */
> > +	off -= off % 500;
> > +	if (!off)
> > +		off += 500;
> > +	else if (off > BLINK_MAX_TIME)
> > +		off = BLINK_MAX_TIME;
> > +	*poff = off;
> > +	off /= 500;
> > +
> > +	on -= on % 500;
> > +	if (!on)
> > +		on += 500;
> > +	else if (on > BLINK_MAX_TIME)
> > +		on = BLINK_MAX_TIME;
> > +	*pon = on;
> > +	on /= 500;
> > +
> > +	/* duty min should be zero (=off), delay should be zero */
> > +	ret = regmap_write(led->chip->regmap, REG_LEDCNT2(num),
> > +			LED_DELAY(0) | LED_DUTYMIN(0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* reset detention time (no "breathing" effect) */
> > +	ret = regmap_write(led->chip->regmap, REG_LEDCNT3(num),
> > +			LED_DT1(0) | LED_DT2(0));
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_write(led->chip->regmap, REG_LEDCNT4(num),
> > +			LED_DT3(0) | LED_DT4(0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* slope time controls on/off cycle length */
> > +	ret = regmap_write(led->chip->regmap, REG_SLOPE(num),
> > +			LED_SLOPETIME1(off) | LED_SLOPETIME2(on));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Finally, enable slope mode. */
> > +	ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon);
> > +	if (ret)
> > +		return ret;
> > +	ledon |= LED_SLOPE(num);
> 
> Empty line here please.
> 
> > +	return regmap_write(led->chip->regmap, REG_LEDON,
> > +			ledon);
> > +}
> > +
> > +static int an30259a_led_set(struct led_classdev *cdev,
> > +		enum led_brightness value)
> > +{
> > +	struct an30259a_led *led;
> > +	int ret;
> > +
> > +	led = container_of(cdev, struct an30259a_led, cdev);
> > +
> > +	mutex_lock(&led->chip->mutex);
> > +
> > +	ret = an30259a_brightness(led, value);
> > +
> > +	mutex_unlock(&led->chip->mutex);
> > +	return ret;
> > +}
> > +
> > +static int an30259a_blink_set(struct led_classdev *cdev,
> > +		unsigned long *delay_off, unsigned long *delay_on)
> > +{
> > +	struct an30259a_led *led;
> > +	int ret;
> > +
> > +	led = container_of(cdev, struct an30259a_led, cdev);
> > +
> > +	mutex_lock(&led->chip->mutex);
> > +
> > +	ret = an30259a_blink(led, delay_off, delay_on);
> > +
> > +	mutex_unlock(&led->chip->mutex);
> > +	return ret;
> > +}
> > +
> > +static struct led_platform_data *
> > +an30259a_dt_init(struct i2c_client *client)
> > +{
> > +	struct led_platform_data *pdata;
> > +	struct device_node *np = client->dev.of_node, *child;
> > +	struct led_info *leds;
> > +	int count;
> > +	int res;
> > +
> > +	count = of_get_child_count(np);
> > +	if (!count || count > MAX_LEDS)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	leds = devm_kzalloc(&client->dev, sizeof(*leds) * count, GFP_KERNEL);
> > +	if (!leds)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	for_each_child_of_node(np, child) {
> > +		u32 source;
> > +
> > +		res = of_property_read_u32(child, "led-sources", &source);
> > +		if ((res != 0) || source >= MAX_LEDS)
> > +			continue;
> > +
> > +		leds[source].name = of_get_property(child, "label", NULL);
> > +		leds[source].default_trigger =
> > +			of_get_property(child, "linux,default-trigger", NULL);
> > +	}
> > +
> > +	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	pdata->leds = leds;
> > +	pdata->num_leds = MAX_LEDS;
> > +
> > +	return pdata;
> > +}
> > +
> > +static const struct regmap_config an30259a_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = REG_MAX,
> > +};
> > +
> > +static int an30259a_probe(struct i2c_client *client)
> > +{
> > +	struct led_platform_data *pdata;
> > +	struct an30259a *chip;
> > +	int i, err;
> > +
> > +	pdata = an30259a_dt_init(client);
> > +	if (IS_ERR(pdata))
> > +		return PTR_ERR(pdata);
> > +
> > +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > +	if (!chip)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&chip->mutex);
> > +	chip->client = client;
> > +	chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config);
> > +
> > +	i2c_set_clientdata(client, chip);
> > +
> > +	/* Reset the IC */
> > +	regmap_write(chip->regmap, REG_SRESET, LED_SRESET);
> > +
> > +	for (i = 0; i < pdata->num_leds; i++) {
> > +		chip->leds[i].num = i;
> > +		chip->leds[i].chip = chip;
> > +
> > +		if (pdata->leds[i].name)
> > +			snprintf(chip->leds[i].name, sizeof(chip->leds[i].name),
> > +					"an30259a:%s", pdata->leds[i].name);
> > +		else
> > +			snprintf(chip->leds[i].name, sizeof(chip->leds[i].name),
> > +					"an30259a:%d:%.2x:%d",
> 
> We should have color in the second segment of the LED class device name.
> If label is absent, please just leave it blank "::".

How should we handle the case where two LEDs have an empty name (or the
same name)?

> 
> > +					client->adapter->nr, client->addr, i);
> > +		if (pdata->leds[i].default_trigger)
> > +			chip->leds[i].cdev.default_trigger =
> > +				pdata->leds[i].default_trigger;
> > +
> > +		chip->leds[i].cdev.name = chip->leds[i].name;
> > +
> > +		chip->leds[i].cdev.brightness_set_blocking = an30259a_led_set;
> > +		chip->leds[i].cdev.blink_set = an30259a_blink_set;
> > +
> > +		err = led_classdev_register(&client->dev, &chip->leds[i].cdev);
> 
> Please use devm prefixed API here.
> 
> > +		if (err < 0)
> > +			goto exit;
> > +	}
> > +	return 0;
> > +
> > +exit:
> > +	while (i--)
> > +		led_classdev_unregister(&chip->leds[i].cdev);
> > +	return err;
> > +}
> > +
> > +static int an30259a_remove(struct i2c_client *client)
> > +{
> > +	struct an30259a *chip = i2c_get_clientdata(client);
> > +	int i;
> > +
> > +	for (i = 0; i < MAX_LEDS; i++)
> > +		led_classdev_unregister(&chip->leds[i].cdev)
> 
> You won't need this loop then, but please add mutex_destroy().
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id an30259a_match_table[] = {
> > +	{ .compatible = "panasonic,an30259a", },
> > +	{ /* sentinel */ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, an30259a_match_table);
> > +
> > +static struct i2c_driver an30259a_driver = {
> > +	.driver = {
> > +		.name = "leds-an32059a",
> > +		.of_match_table = of_match_ptr(an30259a_match_table),
> > +	},
> > +	.probe_new = an30259a_probe,
> > +	.remove = an30259a_remove,
> > +};
> > +
> > +module_i2c_driver(an30259a_driver);
> > +
> > +MODULE_AUTHOR("Simon Shields <simon@lineageos.org>");
> > +MODULE_DESCRIPTION("AN32059A LED driver");
> > +MODULE_LICENSE("GPL v2");
> > 
> 
> You could consider also addressing some (all?) of problems
> reported by scripts/checkpatch.pl --strict.

Ack, will do :)

> 
> -- 
> Best regards,
> Jacek Anaszewski

Cheers,
Simon

Re: [PATCH 1/2] dt-bindings: leds: document Panasonic AN30259A bindings

[Jacek Anaszewski]

On 02/20/2018 10:28 PM, Jacek Anaszewski wrote:
> Hi Simon,
> 
> Thank you for the patch. I have a few comments below.
> 
> On 02/20/2018 01:54 AM, Simon Shields wrote:
>> Signed-off-by: Simon Shields <simon@lineageos.org>
>> ---
>>  .../devicetree/bindings/leds/leds-an30259a.txt     | 41 ++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-an30259a.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-an30259a.txt b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
>> new file mode 100644
>> index 000000000000..d3586ce71eef
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
>> @@ -0,0 +1,41 @@
>> +* Panasonic AN30259A 3-channel LED driver
>> +
>> +The AN30259A is a LED controller capable of driving three LEDs independently. It supports
>> +constant current output and sloping current output modes. The chip is connected over I2C.
>> +
>> +Required properties:
>> +	- compatible : must be "panasonic,an30259a"
> 
> s/must/Must/ and please put a dot in the end.
> 
>> +	- reg -  I2C slave address
>> +
>> +Each led is represented as a sub-node of the panasonic,an30259a node.
> 
> s/led/LED/
> 
>> +
>> +Optional sub-node properties:
>> +	- label: see Documentation/devicetree/bindings/leds/common.txt
>> +	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>> +	- led-sources: 0 if LED is connected to LED1 pin,
> 
> led-sources is applicable in e bit different arrangements.
> Here please use "reg" property instead. In this case you will
> need also below in the "Required properties":
> 
> - #address-cells: Must be 1.
> - #size-cells: Must be 0.
> 
> 
>> +	  1 if LED is connected to LED2 pin, or 2 if LED is connected to LED3 pin.
>> +
>> +Example:
>> +an30259a@30 {
> 
> According to the lesson from Rob [0] it should be:
> 
> led-controller@30 {
>   #address-cells = <1>;
>   #size-cells = <0>
>   reg = <30>;
> 
>   led0 {
>     reg = <0>;
>     label = "red";	
>   };
> 
>   green@1 {
>     reg = <1>;
>     label = "green";
>   };
> 
>   blue@2 {
>     reg = <2>;
>     label = "blue";
>   };
> };

Of course s/green@1/led@1/ and s/blue@2/led@2/

Moreover, label should describe LED function, color
is only an extra info. Please refer to the other LED bindings
for a reference.

> 
>> +	compatible = "panasonic,an30259a";
>> +	reg = <0x30>;
>> +
>> +	red {
>> +		led-sources = <0>;
>> +		linux,default-trigger = "heartbeat";
>> +		label = "red";
>> +	};
>> +
>> +	green {
>> +		led-sources = <1>;
>> +		label = "green";
>> +	};
>> +
>> +	blue {
>> +		led-sources = <2>;
>> +		label = "blue";
>> +	};
>> +};
>> +
>> +For more information please see the link below:
>> +https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
>>
> 
> [0] https://patchwork.kernel.org/patch/10093757/
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 2/2] leds: add Panasonic AN30259A support

[Jacek Anaszewski]

On 02/21/2018 03:18 PM, Simon Shields wrote:
> Hi Jacek,
> 
> Thanks for the review!

You're welcome!

> On Tue, Feb 20, 2018 at 10:51:40PM +0100, Jacek Anaszewski wrote:
>> Hi Simon,
>>
>> Thank you for the patch. Please refer to my comments below.
>>
>> On 02/20/2018 01:54 AM, Simon Shields wrote:
>>> AN30259A is a 3-channel LED driver which uses I2C. It supports timed
>>> operation via an internal PWM clock, and variable brightness. This
>>> driver offers support for basic hardware-based blinking and brightness
>>> control.
>>>
>>> The datasheet is freely available:
>>> https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
>>>
>>> Signed-off-by: Simon Shields <simon@lineageos.org>
>>> ---
>>>  drivers/leds/Kconfig         |  10 ++
>>>  drivers/leds/Makefile        |   1 +
>>>  drivers/leds/leds-an30259a.c | 338 +++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 349 insertions(+)
>>>  create mode 100644 drivers/leds/leds-an30259a.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 3e763d2a0cb3..80bed557cc6b 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -57,6 +57,16 @@ config LEDS_AAT1290
>>>  	depends on PINCTRL
>>>  	help
>>>  	 This option enables support for the LEDs on the AAT1290.
>>> +
>>> +config LEDS_AN30259A
>>> +    tristate "LED support for Panasonic AN30259A"
>>> +    depends on OF
>>> +    depends on I2C
>>> +    depends on LEDS_CLASS
>>> +    help
>>> +     This option enables support for the AN30259A 3-channel
>>> +     LED driver.
>>> +
>>>  config LEDS_APU
>>>  	tristate "Front panel LED support for PC Engines APU/APU2 boards"
>>>  	depends on LEDS_CLASS
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index 987884a5b9a5..44f9b42d1600 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -11,6 +11,7 @@ obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
>>>  obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
>>>  obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
>>>  obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
>>> +obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
>>>  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
>>>  obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
>>>  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
>>> diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
>>> new file mode 100644
>>> index 000000000000..b51faf67e7de
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-an30259a.c
>>> @@ -0,0 +1,338 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> You could mention yourself as an Author here too.
>>
>>> +#include <linux/i2c.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +#define MAX_LEDS 3
>>> +
>>> +#define REG_SRESET 0x00
>>> +#define LED_SRESET (1 << 0)
>>> +
>>> +/* LED power registers */
>>> +#define REG_LEDON 0x01
>>> +#define LED_EN(x) (1 << (x))
>>> +#define LED_SLOPE(x) (1 << ((x) + 4))
>>> +
>>> +#define REG_LEDCC(x) (0x03 + (x))
>>> +
>>> +/* slope control registers */
>>> +#define REG_SLOPE(x) (0x06 + (x))
>>> +#define LED_SLOPETIME1(x) (x)
>>> +#define LED_SLOPETIME2(x) ((x) << 4)
>>> +
>>> +#define REG_LEDCNT1(x) (0x09 + (4 * (x)))
>>> +#define LED_DUTYMAX(x) ((x) << 4)
>>> +#define LED_DUTYMID(x) (x)
>>> +
>>> +#define REG_LEDCNT2(x) (0x0A + (4 * (x)))
>>> +#define LED_DELAY(x) ((x) << 4)
>>> +#define LED_DUTYMIN(x) (x)
>>> +
>>> +/* detention time control (length of each slope step) */
>>> +#define REG_LEDCNT3(x) (0x0B + (4 * (x)))
>>> +#define LED_DT1(x) (x)
>>> +#define LED_DT2(x) ((x) << 4)
>>> +
>>> +#define REG_LEDCNT4(x) (0x0C + (4 * (x)))
>>> +#define LED_DT3(x) (x)
>>> +#define LED_DT4(x) ((x) << 4)
>>> +
>>> +#define REG_MAX 0x14
>>> +
>>> +#define BLINK_MAX_TIME 7500 /* ms */
>>> +
>>> +struct an30259a;
>>> +
>>> +struct an30259a_led {
>>> +	struct an30259a *chip;
>>> +	struct led_classdev cdev;
>>> +	int num;
>>> +	char name[32];
>>> +};
>>> +
>>> +struct an30259a {
>>> +	struct mutex mutex;
>>> +	struct i2c_client *client;
>>> +	struct an30259a_led leds[MAX_LEDS];
>>> +	struct regmap *regmap;
>>> +};
>>> +
>>> +static u8 an30259a_get_duty_max(u8 brightness)
>>> +{
>>> +	u8 duty_max, floor, ceil;
>>> +
>>> +	/* squash 8 bit number into 7-bit PWM range */
>>> +	duty_max = brightness >> 1;
>>> +
>>> +	/* bottom 3 bits are always set for DUTYMAX
>>> +	 * figure out the closest value
>>> +	 */
>>> +	ceil = duty_max | 0x7;
>>> +	floor = ceil - 0x8;
>>> +
>>> +	if ((duty_max - floor) < (ceil - duty_max))
>>> +		duty_max = floor >> 3;
>>> +	else
>>> +		duty_max = ceil >> 3;
>>> +
>>> +	return duty_max;
>>> +}
>>> +
>>> +static int an30259a_brightness(struct an30259a_led *led,
>>> +		enum led_brightness brightness)
>>> +{
>>> +	int ret;
>>> +	unsigned int ledon;
>>> +	u8 duty_max;
>>> +
>>> +	ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon);
>>
>> Please put an empty line here - it improves readability.
>>
>>> +	switch (brightness) {
>>> +	case LED_OFF:
>>> +		ledon &= ~LED_EN(led->num);
>>> +		ledon &= ~LED_SLOPE(led->num);
>>> +		break;
>>> +	default:
>>> +		ledon |= LED_EN(led->num);
>>> +		duty_max = an30259a_get_duty_max(brightness & 0xff);
>>> +		ret = regmap_write(led->chip->regmap,
>>> +				REG_LEDCNT1(led->num),
>>> +				LED_DUTYMAX(duty_max) | LED_DUTYMID(duty_max));
>>> +		if (ret)
>>> +			return ret;
>>> +		break;
>>> +	}
>>> +	ret = regmap_write(led->chip->regmap, REG_LEDON,
>>> +				ledon);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = regmap_write(led->chip->regmap, REG_LEDCC(led->num),
>>> +			brightness & 0xff);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int an30259a_blink(struct an30259a_led *led,
>>> +		unsigned long *poff, unsigned long *pon)
>>> +{
>>> +	int ret, num = led->num;
>>> +	unsigned int ledon;
>>> +	unsigned long off = *poff, on = *pon;
>>> +
>>> +	/* slope time - multiples of 500ms only */
>>> +	off -= off % 500;
>>> +	if (!off)
>>> +		off += 500;
>>> +	else if (off > BLINK_MAX_TIME)
>>> +		off = BLINK_MAX_TIME;
>>> +	*poff = off;
>>> +	off /= 500;
>>> +
>>> +	on -= on % 500;
>>> +	if (!on)
>>> +		on += 500;
>>> +	else if (on > BLINK_MAX_TIME)
>>> +		on = BLINK_MAX_TIME;
>>> +	*pon = on;
>>> +	on /= 500;
>>> +
>>> +	/* duty min should be zero (=off), delay should be zero */
>>> +	ret = regmap_write(led->chip->regmap, REG_LEDCNT2(num),
>>> +			LED_DELAY(0) | LED_DUTYMIN(0));
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* reset detention time (no "breathing" effect) */
>>> +	ret = regmap_write(led->chip->regmap, REG_LEDCNT3(num),
>>> +			LED_DT1(0) | LED_DT2(0));
>>> +	if (ret)
>>> +		return ret;
>>> +	ret = regmap_write(led->chip->regmap, REG_LEDCNT4(num),
>>> +			LED_DT3(0) | LED_DT4(0));
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* slope time controls on/off cycle length */
>>> +	ret = regmap_write(led->chip->regmap, REG_SLOPE(num),
>>> +			LED_SLOPETIME1(off) | LED_SLOPETIME2(on));
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Finally, enable slope mode. */
>>> +	ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon);
>>> +	if (ret)
>>> +		return ret;
>>> +	ledon |= LED_SLOPE(num);
>>
>> Empty line here please.
>>
>>> +	return regmap_write(led->chip->regmap, REG_LEDON,
>>> +			ledon);
>>> +}
>>> +
>>> +static int an30259a_led_set(struct led_classdev *cdev,
>>> +		enum led_brightness value)
>>> +{
>>> +	struct an30259a_led *led;
>>> +	int ret;
>>> +
>>> +	led = container_of(cdev, struct an30259a_led, cdev);
>>> +
>>> +	mutex_lock(&led->chip->mutex);
>>> +
>>> +	ret = an30259a_brightness(led, value);
>>> +
>>> +	mutex_unlock(&led->chip->mutex);
>>> +	return ret;
>>> +}
>>> +
>>> +static int an30259a_blink_set(struct led_classdev *cdev,
>>> +		unsigned long *delay_off, unsigned long *delay_on)
>>> +{
>>> +	struct an30259a_led *led;
>>> +	int ret;
>>> +
>>> +	led = container_of(cdev, struct an30259a_led, cdev);
>>> +
>>> +	mutex_lock(&led->chip->mutex);
>>> +
>>> +	ret = an30259a_blink(led, delay_off, delay_on);
>>> +
>>> +	mutex_unlock(&led->chip->mutex);
>>> +	return ret;
>>> +}
>>> +
>>> +static struct led_platform_data *
>>> +an30259a_dt_init(struct i2c_client *client)
>>> +{
>>> +	struct led_platform_data *pdata;
>>> +	struct device_node *np = client->dev.of_node, *child;
>>> +	struct led_info *leds;
>>> +	int count;
>>> +	int res;
>>> +
>>> +	count = of_get_child_count(np);
>>> +	if (!count || count > MAX_LEDS)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	leds = devm_kzalloc(&client->dev, sizeof(*leds) * count, GFP_KERNEL);
>>> +	if (!leds)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	for_each_child_of_node(np, child) {
>>> +		u32 source;
>>> +
>>> +		res = of_property_read_u32(child, "led-sources", &source);
>>> +		if ((res != 0) || source >= MAX_LEDS)
>>> +			continue;
>>> +
>>> +		leds[source].name = of_get_property(child, "label", NULL);
>>> +		leds[source].default_trigger =
>>> +			of_get_property(child, "linux,default-trigger", NULL);
>>> +	}
>>> +
>>> +	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>> +	if (!pdata)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	pdata->leds = leds;
>>> +	pdata->num_leds = MAX_LEDS;
>>> +
>>> +	return pdata;
>>> +}
>>> +
>>> +static const struct regmap_config an30259a_regmap_config = {
>>> +	.reg_bits = 8,
>>> +	.val_bits = 8,
>>> +	.max_register = REG_MAX,
>>> +};
>>> +
>>> +static int an30259a_probe(struct i2c_client *client)
>>> +{
>>> +	struct led_platform_data *pdata;
>>> +	struct an30259a *chip;
>>> +	int i, err;
>>> +
>>> +	pdata = an30259a_dt_init(client);
>>> +	if (IS_ERR(pdata))
>>> +		return PTR_ERR(pdata);
>>> +
>>> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>> +	if (!chip)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&chip->mutex);
>>> +	chip->client = client;
>>> +	chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config);
>>> +
>>> +	i2c_set_clientdata(client, chip);
>>> +
>>> +	/* Reset the IC */
>>> +	regmap_write(chip->regmap, REG_SRESET, LED_SRESET);
>>> +
>>> +	for (i = 0; i < pdata->num_leds; i++) {
>>> +		chip->leds[i].num = i;
>>> +		chip->leds[i].chip = chip;
>>> +
>>> +		if (pdata->leds[i].name)
>>> +			snprintf(chip->leds[i].name, sizeof(chip->leds[i].name),
>>> +					"an30259a:%s", pdata->leds[i].name);
>>> +		else
>>> +			snprintf(chip->leds[i].name, sizeof(chip->leds[i].name),
>>> +					"an30259a:%d:%.2x:%d",
>>
>> We should have color in the second segment of the LED class device name.
>> If label is absent, please just leave it blank "::".
> 
> How should we handle the case where two LEDs have an empty name (or the
> same name)?

Generally, the LED function segment of the LED class device name should
be always provided, at least by convention. However, if the LED name
to be registered is already taken, the LED core will add a numerical
suffix to avoid a device name clash.


Best regards,
Jacek Anaszewski

>>> +					client->adapter->nr, client->addr, i);
>>> +		if (pdata->leds[i].default_trigger)
>>> +			chip->leds[i].cdev.default_trigger =
>>> +				pdata->leds[i].default_trigger;
>>> +
>>> +		chip->leds[i].cdev.name = chip->leds[i].name;
>>> +
>>> +		chip->leds[i].cdev.brightness_set_blocking = an30259a_led_set;
>>> +		chip->leds[i].cdev.blink_set = an30259a_blink_set;
>>> +
>>> +		err = led_classdev_register(&client->dev, &chip->leds[i].cdev);
>>
>> Please use devm prefixed API here.
>>
>>> +		if (err < 0)
>>> +			goto exit;
>>> +	}
>>> +	return 0;
>>> +
>>> +exit:
>>> +	while (i--)
>>> +		led_classdev_unregister(&chip->leds[i].cdev);
>>> +	return err;
>>> +}
>>> +
>>> +static int an30259a_remove(struct i2c_client *client)
>>> +{
>>> +	struct an30259a *chip = i2c_get_clientdata(client);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < MAX_LEDS; i++)
>>> +		led_classdev_unregister(&chip->leds[i].cdev)
>>
>> You won't need this loop then, but please add mutex_destroy().
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id an30259a_match_table[] = {
>>> +	{ .compatible = "panasonic,an30259a", },
>>> +	{ /* sentinel */ },
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, an30259a_match_table);
>>> +
>>> +static struct i2c_driver an30259a_driver = {
>>> +	.driver = {
>>> +		.name = "leds-an32059a",
>>> +		.of_match_table = of_match_ptr(an30259a_match_table),
>>> +	},
>>> +	.probe_new = an30259a_probe,
>>> +	.remove = an30259a_remove,
>>> +};
>>> +
>>> +module_i2c_driver(an30259a_driver);
>>> +
>>> +MODULE_AUTHOR("Simon Shields <simon@lineageos.org>");
>>> +MODULE_DESCRIPTION("AN32059A LED driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>> You could consider also addressing some (all?) of problems
>> reported by scripts/checkpatch.pl --strict.
> 
> Ack, will do :)
> 
>>
>> -- 
>> Best regards,
>> Jacek Anaszewski
> 
> Cheers,
> Simon
>