[PATCH v3 0/2] Panasonic AN30259A support

[PATCH v3 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].

Changes since v3:
* Rebased on v4.18-rc3.
* Drop unnecessary DUTYMAX/MID calculations when blinking:
 previously I'd thought that the PWM duty values were expressed
 as a percentage of the maximum current output, but in reality
 they're a percentage of the current set in the LEDxCC registers. 
This simplifies the code quite a bit.
* Corrected MODULE_LICENSE declaration.
* Return -EINVAL from set_blink if the blink rate is unsupported in
 hardware.
* Fix more checkpatch --strict issues.

Changes since v2:
* Drop "an30259a:" prefix from bindings and add it in the device driver
 instead.
* Use led-controller instead of leds for sample DT binding.
* Use ":indicator" instead of ":notification" in sample DT binding.
* Merge an30259a_led_set and an30259a_brightness to
 an30259a_brightness_set (and same for blink functions).
* Explain the range limitations of the AN30259A's sloping mode
 in the code - the AN30259A only has a 7-bit PWM range in slope mode,
 and the bottom 3 bits are always set.

Changes since v1:
* Documentation formatting/grammar fixes.
* Use reg property instead of led-sources for leds.
* Add default-state support.
* Fix auto-probing when built as a module.
* Simplified DT parsing code.
* Use devm version of led_class_register().
* Fix LED naming scheme.
* Fixed checkpatch --strict issues.

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

 .../bindings/leds/leds-an30259a.txt           |  43 +++
 drivers/leds/Kconfig                          |  11 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-an30259a.c                  | 365 ++++++++++++++++++
 4 files changed, 420 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-an30259a.txt
 create mode 100644 drivers/leds/leds-an30259a.c

-- 
2.18.0

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

[Simon Shields]

Signed-off-by: Simon Shields <simon@lineageos.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/leds/leds-an30259a.txt           | 43 +++++++++++++++++++
 1 file changed, 43 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..6ffb861083c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
@@ -0,0 +1,43 @@
+* 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.
+	- #address-cells: Must be 1.
+	- #size-cells: Must be 0.
+
+Each LED is represented as a sub-node of the panasonic,an30259a node.
+
+Required sub-node properties:
+	- reg: Pin that the LED is connected to. Must be 1, 2, or 3.
+
+Optional sub-node properties:
+	- label: see Documentation/devicetree/bindings/leds/common.txt
+	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+led-controller@30 {
+	compatible = "panasonic,an30259a";
+	reg = <0x30>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	led@1 {
+		reg = <1>;
+		linux,default-trigger = "heartbeat";
+		label = "red:indicator";
+	};
+
+	led@2 {
+		reg = <2>;
+		label = "green:indicator";
+	};
+
+	led@3 {
+		reg = <3>;
+		label = "blue:indicator";
+	};
+};
-- 
2.18.0

[PATCH v3 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         |  11 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-an30259a.c | 365 +++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/leds/leds-an30259a.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6e3a998f3370..1567882fd039 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -57,6 +57,17 @@ 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 LEDS_CLASS && I2C && OF
+	help
+	  This option enables support for the AN30259A 3-channel
+	  LED driver.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-an30259a.
+
 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 420b5d2cfa62..4c1b0054f379 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..c7b900b388ac
--- /dev/null
+++ b/drivers/leds/leds-an30259a.c
@@ -0,0 +1,365 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Driver for Panasonic AN30259A 3-channel LED driver
+//
+// Copyright (c) 2018 Simon Shields <simon@lineageos.org>
+//
+// Datasheet:
+// https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
+
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <uapi/linux/uleds.h>
+
+#define MAX_LEDS 3
+
+#define REG_SRESET 0x00
+#define LED_SRESET BIT(0)
+
+/* LED power registers */
+#define REG_LED_ON 0x01
+#define LED_EN(x) BIT((x) - 1)
+#define LED_SLOPE(x) BIT(((x) - 1) + 4)
+
+#define REG_LEDCC(x) (0x03 + ((x) - 1))
+
+/* slope control registers */
+#define REG_SLOPE(x) (0x06 + ((x) - 1))
+#define LED_SLOPETIME1(x) (x)
+#define LED_SLOPETIME2(x) ((x) << 4)
+
+#define REG_LEDCNT1(x) (0x09 + (4 * ((x) - 1)))
+#define LED_DUTYMAX(x) ((x) << 4)
+#define LED_DUTYMID(x) (x)
+
+#define REG_LEDCNT2(x) (0x0A + (4 * ((x) - 1)))
+#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) - 1)))
+#define LED_DT1(x) (x)
+#define LED_DT2(x) ((x) << 4)
+
+#define REG_LEDCNT4(x) (0x0C + (4 * ((x) - 1)))
+#define LED_DT3(x) (x)
+#define LED_DT4(x) ((x) << 4)
+
+#define REG_MAX 0x14
+
+#define BLINK_MAX_TIME 7500 /* ms */
+#define SLOPE_RESOLUTION 500 /* ms */
+
+#define STATE_OFF 0
+#define STATE_KEEP 1
+#define STATE_ON 2
+
+struct an30259a;
+
+struct an30259a_led {
+	struct an30259a *chip;
+	struct led_classdev cdev;
+	u32 num;
+	u32 default_state;
+	bool sloping;
+	char label[LED_MAX_NAME_SIZE];
+};
+
+struct an30259a {
+	struct mutex mutex; /* held when writing to registers */
+	struct i2c_client *client;
+	struct an30259a_led leds[MAX_LEDS];
+	struct regmap *regmap;
+	int num_leds;
+};
+
+static int an30259a_brightness_set(struct led_classdev *cdev,
+				   enum led_brightness brightness)
+{
+	struct an30259a_led *led;
+	int ret;
+	unsigned int led_on;
+
+	led = container_of(cdev, struct an30259a_led, cdev);
+	mutex_lock(&led->chip->mutex);
+
+	ret = regmap_read(led->chip->regmap, REG_LED_ON, &led_on);
+	if (ret)
+		goto error;
+
+	switch (brightness) {
+	case LED_OFF:
+		led_on &= ~LED_EN(led->num);
+		led_on &= ~LED_SLOPE(led->num);
+		led->sloping = false;
+		break;
+	default:
+		led_on |= LED_EN(led->num);
+		if (led->sloping)
+			led_on |= LED_SLOPE(led->num);
+		ret = regmap_write(led->chip->regmap, REG_LEDCNT1(led->num),
+				   LED_DUTYMAX(0xf) | LED_DUTYMID(0xf));
+		if (ret)
+			goto error;
+		break;
+	}
+
+	ret = regmap_write(led->chip->regmap, REG_LED_ON, led_on);
+	if (ret)
+		goto error;
+
+	ret = regmap_write(led->chip->regmap, REG_LEDCC(led->num),
+			   brightness);
+
+error:
+	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, num;
+	unsigned int led_on, duty;
+	unsigned long off = *delay_off, on = *delay_on;
+
+	led = container_of(cdev, struct an30259a_led, cdev);
+
+	mutex_lock(&led->chip->mutex);
+	num = led->num;
+
+	/* slope time can only be a multiple of 500ms. */
+	if (off % SLOPE_RESOLUTION || on % SLOPE_RESOLUTION) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	/* up to a maximum of 7500ms. */
+	if (off > BLINK_MAX_TIME || on > BLINK_MAX_TIME) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	/* if no blink specified, default to 1 Hz. */
+	if (!off && !on) {
+		*delay_off = off = 500;
+		*delay_on = on = 500;
+	}
+
+	/* convert into values the HW will understand. */
+	off /= SLOPE_RESOLUTION;
+	on /= SLOPE_RESOLUTION;
+
+	/* 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)
+		goto error;
+
+	/* reset detention time (no "breathing" effect). */
+	ret = regmap_write(led->chip->regmap, REG_LEDCNT3(num),
+			   LED_DT1(0) | LED_DT2(0));
+	if (ret)
+		goto error;
+	ret = regmap_write(led->chip->regmap, REG_LEDCNT4(num),
+			   LED_DT3(0) | LED_DT4(0));
+	if (ret)
+		goto error;
+
+	/* slope time controls on/off cycle length. */
+	ret = regmap_write(led->chip->regmap, REG_SLOPE(num),
+			   LED_SLOPETIME1(off) | LED_SLOPETIME2(on));
+	if (ret)
+		goto error;
+
+	/* Finally, enable slope mode. */
+	ret = regmap_read(led->chip->regmap, REG_LED_ON, &led_on);
+	if (ret)
+		goto error;
+
+	led_on |= LED_SLOPE(num) | LED_EN(led->num);
+
+	ret = regmap_write(led->chip->regmap, REG_LED_ON, led_on);
+
+	if (!ret)
+		led->sloping = true;
+error:
+	mutex_unlock(&led->chip->mutex);
+
+	return ret;
+}
+
+static int an30259a_dt_init(struct i2c_client *client,
+			    struct an30259a *chip)
+{
+	struct device_node *np = client->dev.of_node, *child;
+	int count, ret;
+	int i = 0;
+	const char *str;
+	struct an30259a_led *led;
+
+	count = of_get_child_count(np);
+	if (!count || count > MAX_LEDS)
+		return -EINVAL;
+
+	for_each_available_child_of_node(np, child) {
+		u32 source;
+
+		ret = of_property_read_u32(child, "reg", &source);
+		if (ret != 0 || !source || source > MAX_LEDS) {
+			dev_err(&client->dev, "Couldn't read LED address: %d\n",
+				ret);
+			count--;
+			continue;
+		}
+
+		led = &chip->leds[i];
+
+		led->num = source;
+		led->chip = chip;
+
+		if (of_property_read_string(child, "label", &str))
+			snprintf(led->label, sizeof(led->label), "an30259a::");
+		else
+			snprintf(led->label, sizeof(led->label), "an30259a:%s",
+				 str);
+
+		led->cdev.name = led->label;
+
+		if (!of_property_read_string(child, "default-state", &str)) {
+			if (!strcmp(str, "on"))
+				led->default_state = STATE_ON;
+			else if (!strcmp(str, "keep"))
+				led->default_state = STATE_KEEP;
+			else
+				led->default_state = STATE_OFF;
+		}
+
+		of_property_read_string(child, "linux,default-trigger",
+					&led->cdev.default_trigger);
+
+		i++;
+	}
+
+	if (!count)
+		return -EINVAL;
+
+	chip->num_leds = i;
+
+	return 0;
+}
+
+static const struct regmap_config an30259a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = REG_MAX,
+};
+
+static void an30259a_init_default_state(struct an30259a_led *led)
+{
+	struct an30259a *chip = led->chip;
+	int led_on, err;
+
+	switch (led->default_state) {
+	case STATE_ON:
+		led->cdev.brightness = LED_FULL;
+		break;
+	case STATE_KEEP:
+		err = regmap_read(chip->regmap, REG_LED_ON, &led_on);
+		if (err)
+			break;
+
+		if (!(led_on & LED_EN(led->num))) {
+			led->cdev.brightness = LED_OFF;
+			break;
+		}
+		regmap_read(chip->regmap, REG_LEDCC(led->num),
+			    &led->cdev.brightness);
+		break;
+	default:
+		led->cdev.brightness = LED_OFF;
+	}
+
+	an30259a_brightness_set(&led->cdev, led->cdev.brightness);
+}
+
+static int an30259a_probe(struct i2c_client *client)
+{
+	struct an30259a *chip;
+	int i, err;
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	err = an30259a_dt_init(client, chip);
+	if (err < 0)
+		return err;
+
+	mutex_init(&chip->mutex);
+	chip->client = client;
+	i2c_set_clientdata(client, chip);
+
+	chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config);
+
+	for (i = 0; i < chip->num_leds; i++) {
+		an30259a_init_default_state(&chip->leds[i]);
+		chip->leds[i].cdev.brightness_set_blocking =
+			an30259a_brightness_set;
+		chip->leds[i].cdev.blink_set = an30259a_blink_set;
+
+		err = devm_led_classdev_register(&client->dev,
+						 &chip->leds[i].cdev);
+		if (err < 0)
+			goto exit;
+	}
+	return 0;
+
+exit:
+	mutex_destroy(&chip->mutex);
+	return err;
+}
+
+static int an30259a_remove(struct i2c_client *client)
+{
+	struct an30259a *chip = i2c_get_clientdata(client);
+
+	mutex_destroy(&chip->mutex);
+
+	return 0;
+}
+
+static const struct of_device_id an30259a_match_table[] = {
+	{ .compatible = "panasonic,an30259a", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, an30259a_match_table);
+
+static const struct i2c_device_id an30259a_id[] = {
+	{ "an30259a", 0 },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i2c, an30259a_id);
+
+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,
+	.id_table = an30259a_id,
+};
+
+module_i2c_driver(an30259a_driver);
+
+MODULE_AUTHOR("Simon Shields <simon@lineageos.org>");
+MODULE_DESCRIPTION("AN32059A LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0

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

[Pavel Machek]

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

On Sun 2018-07-22 00:12:27, 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>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(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 v3 2/2] leds: add Panasonic AN30259A support

[Jacek Anaszewski]

Hi Simon,

Thank you for the updated patch. It looks good in general, with
one reservation, please refer below.

On 07/21/2018 04:12 PM, 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         |  11 ++
>   drivers/leds/Makefile        |   1 +
>   drivers/leds/leds-an30259a.c | 365 +++++++++++++++++++++++++++++++++++
>   3 files changed, 377 insertions(+)
>   create mode 100644 drivers/leds/leds-an30259a.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6e3a998f3370..1567882fd039 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -57,6 +57,17 @@ 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 LEDS_CLASS && I2C && OF
> +	help
> +	  This option enables support for the AN30259A 3-channel
> +	  LED driver.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-an30259a.
> +
>   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 420b5d2cfa62..4c1b0054f379 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..c7b900b388ac
> --- /dev/null
> +++ b/drivers/leds/leds-an30259a.c
> @@ -0,0 +1,365 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Driver for Panasonic AN30259A 3-channel LED driver
> +//
> +// Copyright (c) 2018 Simon Shields <simon@lineageos.org>
> +//
> +// Datasheet:
> +// https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf
> +
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <uapi/linux/uleds.h>
> +
> +#define MAX_LEDS 3
> +
> +#define REG_SRESET 0x00
> +#define LED_SRESET BIT(0)
> +
> +/* LED power registers */
> +#define REG_LED_ON 0x01
> +#define LED_EN(x) BIT((x) - 1)
> +#define LED_SLOPE(x) BIT(((x) - 1) + 4)
> +
> +#define REG_LEDCC(x) (0x03 + ((x) - 1))
> +
> +/* slope control registers */
> +#define REG_SLOPE(x) (0x06 + ((x) - 1))
> +#define LED_SLOPETIME1(x) (x)
> +#define LED_SLOPETIME2(x) ((x) << 4)
> +
> +#define REG_LEDCNT1(x) (0x09 + (4 * ((x) - 1)))
> +#define LED_DUTYMAX(x) ((x) << 4)
> +#define LED_DUTYMID(x) (x)
> +
> +#define REG_LEDCNT2(x) (0x0A + (4 * ((x) - 1)))
> +#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) - 1)))
> +#define LED_DT1(x) (x)
> +#define LED_DT2(x) ((x) << 4)
> +
> +#define REG_LEDCNT4(x) (0x0C + (4 * ((x) - 1)))
> +#define LED_DT3(x) (x)
> +#define LED_DT4(x) ((x) << 4)
> +
> +#define REG_MAX 0x14
> +
> +#define BLINK_MAX_TIME 7500 /* ms */
> +#define SLOPE_RESOLUTION 500 /* ms */
> +
> +#define STATE_OFF 0
> +#define STATE_KEEP 1
> +#define STATE_ON 2

Please add AN30259A_ namespacing prefix to all the above macro
definitions.

> +
> +struct an30259a;
> +
> +struct an30259a_led {
> +	struct an30259a *chip;
> +	struct led_classdev cdev;
> +	u32 num;
> +	u32 default_state;
> +	bool sloping;
> +	char label[LED_MAX_NAME_SIZE];
> +};
> +
> +struct an30259a {
> +	struct mutex mutex; /* held when writing to registers */
> +	struct i2c_client *client;
> +	struct an30259a_led leds[MAX_LEDS];
> +	struct regmap *regmap;
> +	int num_leds;
> +};
> +
> +static int an30259a_brightness_set(struct led_classdev *cdev,
> +				   enum led_brightness brightness)
> +{
> +	struct an30259a_led *led;
> +	int ret;
> +	unsigned int led_on;
> +
> +	led = container_of(cdev, struct an30259a_led, cdev);
> +	mutex_lock(&led->chip->mutex);
> +
> +	ret = regmap_read(led->chip->regmap, REG_LED_ON, &led_on);
> +	if (ret)
> +		goto error;
> +
> +	switch (brightness) {
> +	case LED_OFF:
> +		led_on &= ~LED_EN(led->num);
> +		led_on &= ~LED_SLOPE(led->num);
> +		led->sloping = false;
> +		break;
> +	default:
> +		led_on |= LED_EN(led->num);
> +		if (led->sloping)
> +			led_on |= LED_SLOPE(led->num);
> +		ret = regmap_write(led->chip->regmap, REG_LEDCNT1(led->num),
> +				   LED_DUTYMAX(0xf) | LED_DUTYMID(0xf));
> +		if (ret)
> +			goto error;
> +		break;
> +	}
> +
> +	ret = regmap_write(led->chip->regmap, REG_LED_ON, led_on);
> +	if (ret)
> +		goto error;
> +
> +	ret = regmap_write(led->chip->regmap, REG_LEDCC(led->num),
> +			   brightness);
> +
> +error:
> +	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, num;
> +	unsigned int led_on, duty;
> +	unsigned long off = *delay_off, on = *delay_on;
> +
> +	led = container_of(cdev, struct an30259a_led, cdev);
> +
> +	mutex_lock(&led->chip->mutex);
> +	num = led->num;
> +
> +	/* slope time can only be a multiple of 500ms. */
> +	if (off % SLOPE_RESOLUTION || on % SLOPE_RESOLUTION) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
> +	/* up to a maximum of 7500ms. */
> +	if (off > BLINK_MAX_TIME || on > BLINK_MAX_TIME) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
> +	/* if no blink specified, default to 1 Hz. */
> +	if (!off && !on) {
> +		*delay_off = off = 500;
> +		*delay_on = on = 500;
> +	}
> +
> +	/* convert into values the HW will understand. */
> +	off /= SLOPE_RESOLUTION;
> +	on /= SLOPE_RESOLUTION;
> +
> +	/* 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)
> +		goto error;
> +
> +	/* reset detention time (no "breathing" effect). */
> +	ret = regmap_write(led->chip->regmap, REG_LEDCNT3(num),
> +			   LED_DT1(0) | LED_DT2(0));
> +	if (ret)
> +		goto error;
> +	ret = regmap_write(led->chip->regmap, REG_LEDCNT4(num),
> +			   LED_DT3(0) | LED_DT4(0));
> +	if (ret)
> +		goto error;
> +
> +	/* slope time controls on/off cycle length. */
> +	ret = regmap_write(led->chip->regmap, REG_SLOPE(num),
> +			   LED_SLOPETIME1(off) | LED_SLOPETIME2(on));
> +	if (ret)
> +		goto error;
> +
> +	/* Finally, enable slope mode. */
> +	ret = regmap_read(led->chip->regmap, REG_LED_ON, &led_on);
> +	if (ret)
> +		goto error;
> +
> +	led_on |= LED_SLOPE(num) | LED_EN(led->num);
> +
> +	ret = regmap_write(led->chip->regmap, REG_LED_ON, led_on);
> +
> +	if (!ret)
> +		led->sloping = true;
> +error:
> +	mutex_unlock(&led->chip->mutex);
> +
> +	return ret;
> +}
> +
> +static int an30259a_dt_init(struct i2c_client *client,
> +			    struct an30259a *chip)
> +{
> +	struct device_node *np = client->dev.of_node, *child;
> +	int count, ret;
> +	int i = 0;
> +	const char *str;
> +	struct an30259a_led *led;
> +
> +	count = of_get_child_count(np);
> +	if (!count || count > MAX_LEDS)
> +		return -EINVAL;
> +
> +	for_each_available_child_of_node(np, child) {
> +		u32 source;
> +
> +		ret = of_property_read_u32(child, "reg", &source);
> +		if (ret != 0 || !source || source > MAX_LEDS) {
> +			dev_err(&client->dev, "Couldn't read LED address: %d\n",
> +				ret);
> +			count--;
> +			continue;
> +		}
> +
> +		led = &chip->leds[i];
> +
> +		led->num = source;
> +		led->chip = chip;
> +
> +		if (of_property_read_string(child, "label", &str))
> +			snprintf(led->label, sizeof(led->label), "an30259a::");
> +		else
> +			snprintf(led->label, sizeof(led->label), "an30259a:%s",
> +				 str);
> +
> +		led->cdev.name = led->label;
> +
> +		if (!of_property_read_string(child, "default-state", &str)) {
> +			if (!strcmp(str, "on"))
> +				led->default_state = STATE_ON;
> +			else if (!strcmp(str, "keep"))
> +				led->default_state = STATE_KEEP;
> +			else
> +				led->default_state = STATE_OFF;
> +		}
> +
> +		of_property_read_string(child, "linux,default-trigger",
> +					&led->cdev.default_trigger);
> +
> +		i++;
> +	}
> +
> +	if (!count)
> +		return -EINVAL;
> +
> +	chip->num_leds = i;
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config an30259a_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = REG_MAX,
> +};
> +
> +static void an30259a_init_default_state(struct an30259a_led *led)
> +{
> +	struct an30259a *chip = led->chip;
> +	int led_on, err;
> +
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		led->cdev.brightness = LED_FULL;
> +		break;
> +	case STATE_KEEP:
> +		err = regmap_read(chip->regmap, REG_LED_ON, &led_on);
> +		if (err)
> +			break;
> +
> +		if (!(led_on & LED_EN(led->num))) {
> +			led->cdev.brightness = LED_OFF;
> +			break;
> +		}
> +		regmap_read(chip->regmap, REG_LEDCC(led->num),
> +			    &led->cdev.brightness);
> +		break;
> +	default:
> +		led->cdev.brightness = LED_OFF;
> +	}
> +
> +	an30259a_brightness_set(&led->cdev, led->cdev.brightness);
> +}
> +
> +static int an30259a_probe(struct i2c_client *client)
> +{
> +	struct an30259a *chip;
> +	int i, err;
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	err = an30259a_dt_init(client, chip);
> +	if (err < 0)
> +		return err;
> +
> +	mutex_init(&chip->mutex);
> +	chip->client = client;
> +	i2c_set_clientdata(client, chip);
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config);
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		an30259a_init_default_state(&chip->leds[i]);
> +		chip->leds[i].cdev.brightness_set_blocking =
> +			an30259a_brightness_set;
> +		chip->leds[i].cdev.blink_set = an30259a_blink_set;
> +
> +		err = devm_led_classdev_register(&client->dev,
> +						 &chip->leds[i].cdev);
> +		if (err < 0)
> +			goto exit;
> +	}
> +	return 0;
> +
> +exit:
> +	mutex_destroy(&chip->mutex);
> +	return err;
> +}
> +
> +static int an30259a_remove(struct i2c_client *client)
> +{
> +	struct an30259a *chip = i2c_get_clientdata(client);
> +
> +	mutex_destroy(&chip->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id an30259a_match_table[] = {
> +	{ .compatible = "panasonic,an30259a", },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, an30259a_match_table);
> +
> +static const struct i2c_device_id an30259a_id[] = {
> +	{ "an30259a", 0 },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i2c, an30259a_id);
> +
> +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,
> +	.id_table = an30259a_id,
> +};
> +
> +module_i2c_driver(an30259a_driver);
> +
> +MODULE_AUTHOR("Simon Shields <simon@lineageos.org>");
> +MODULE_DESCRIPTION("AN32059A LED driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

On 07/27/2018 10:08 PM, Jacek Anaszewski wrote:
> Hi Simon,
> 
> Thank you for the updated patch. It looks good in general, with
> one reservation, please refer below.

And one more issue:

drivers/leds/leds-an30259a.c: In function 'an30259a_blink_set':
drivers/leds/leds-an30259a.c:129:23: warning: unused variable 'duty' 
[-Wunused-variable]
   unsigned int led_on, duty;
                        ^
BTW, this patch set version should be v4, so please set the next
one to v5.

-- 
Best regards,
Jacek Anaszewski