[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 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.

I've kept Rob's Reviewed-By and Pavel's ack on the DT patch since
the changes to the binding are minor and inline with other LED
DT binding documentation.

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     |  43 +++
 drivers/leds/Kconfig                               |  11 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-an30259a.c                       | 391 +++++++++++++++++++++
 4 files changed, 446 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-an30259a.txt
 create mode 100644 drivers/leds/leds-an30259a.c

-- 
2.16.2

[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>
---
 .../devicetree/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.16.2

[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 | 391 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 403 insertions(+)
 create mode 100644 drivers/leds/leds-an30259a.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 3e763d2a0cb3..369bfb1227f8 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 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..61275eb8106d
--- /dev/null
+++ b/drivers/leds/leds-an30259a.c
@@ -0,0 +1,391 @@
+// 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;
+	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;
+};
+
+/*
+ * When doing sloping, AN30259A only allows us
+ * to set the first four bits of a 7-bit brightness
+ * value, as opposed to the full 8-bit range
+ * allowed in constant output mode.
+ *
+ * This function returns the best approximation
+ * of the 8-bit brightness value in the 7-bit range
+ * we have available.
+ */
+static u8 an30259a_get_dutymax(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,
+	 * so 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_set(struct led_classdev *cdev,
+				   enum led_brightness brightness)
+{
+	struct an30259a_led *led;
+	int ret;
+	unsigned int led_on;
+	u8 dutymax;
+
+	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);
+		break;
+	default:
+		led_on |= LED_EN(led->num);
+		dutymax = an30259a_get_dutymax(brightness & 0xff);
+		ret = regmap_write(led->chip->regmap, REG_LEDCNT1(led->num),
+				   LED_DUTYMAX(dutymax) | LED_DUTYMID(dutymax));
+		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 & 0xff);
+
+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;
+	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 - multiples of 500ms only, floored */
+	off -= off % SLOPE_RESOLUTION;
+	/* don't floor off time to zero if a non-zero time was requested */
+	if (!off && *delay_off)
+		off += SLOPE_RESOLUTION;
+	else if (off > BLINK_MAX_TIME)
+		off = BLINK_MAX_TIME;
+	*delay_off = off;
+
+	on -= on % SLOPE_RESOLUTION;
+	/* don't floor on time to zero if a non-zero time was requested */
+	if (!on && *delay_on)
+		on += SLOPE_RESOLUTION;
+	else if (on > BLINK_MAX_TIME)
+		on = BLINK_MAX_TIME;
+	*delay_on = on;
+
+	/* 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);
+
+	ret = regmap_write(led->chip->regmap, REG_LED_ON, led_on);
+
+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.16.2

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

[Jacek Anaszewski]

Hi Simon,

Thanks for the update.

I'd like to clarify some details about brightness setting range
and blink setting. Please refer to my comments in the code.

On 03/19/2018 02:59 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 | 391 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 403 insertions(+)
>  create mode 100644 drivers/leds/leds-an30259a.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 3e763d2a0cb3..369bfb1227f8 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 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..61275eb8106d
> --- /dev/null
> +++ b/drivers/leds/leds-an30259a.c
> @@ -0,0 +1,391 @@
> +// 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;
> +	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;
> +};
> +
> +/*
> + * When doing sloping, AN30259A only allows us
> + * to set the first four bits of a 7-bit brightness
> + * value, as opposed to the full 8-bit range
> + * allowed in constant output mode.
> + *
> + * This function returns the best approximation
> + * of the 8-bit brightness value in the 7-bit range
> + * we have available.
> + */
> +static u8 an30259a_get_dutymax(u8 brightness)
> +{
> +	u8 duty_max, floor, ceil;
> +
> +	/* squash 8 bit number into 7-bit PWM range. */
> +	duty_max = brightness >> 1;

You should set max_brightness accordingly if the device
doesn't support 255 brightness levels. Otherwise, if you
leave it initialized to 0, the LED core will adjust it to LED_FULL,
which will give misleading information to the userspace.

> +
> +	/*
> +	 * Bottom 3 bits are always set for DUTYMAX,
> +	 * so 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_set(struct led_classdev *cdev,
> +				   enum led_brightness brightness)
> +{
> +	struct an30259a_led *led;
> +	int ret;
> +	unsigned int led_on;
> +	u8 dutymax;
> +
> +	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);
> +		break;
> +	default:
> +		led_on |= LED_EN(led->num);
> +		dutymax = an30259a_get_dutymax(brightness & 0xff);
> +		ret = regmap_write(led->chip->regmap, REG_LEDCNT1(led->num),
> +				   LED_DUTYMAX(dutymax) | LED_DUTYMID(dutymax));
> +		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 & 0xff);
> +
> +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;
> +	unsigned long off = *delay_off, on = *delay_on;

Please keep in mind that blink_set should return -EINVAL in case
the device can't handle given delay_on/off constraints. LED core will
employ software blink fallback then.

> +
> +	led = container_of(cdev, struct an30259a_led, cdev);
> +
> +	mutex_lock(&led->chip->mutex);
> +	num = led->num;
> +
> +	/* slope time - multiples of 500ms only, floored */
> +	off -= off % SLOPE_RESOLUTION;
> +	/* don't floor off time to zero if a non-zero time was requested */
> +	if (!off && *delay_off)
> +		off += SLOPE_RESOLUTION;
> +	else if (off > BLINK_MAX_TIME)
> +		off = BLINK_MAX_TIME;
> +	*delay_off = off;
> +
> +	on -= on % SLOPE_RESOLUTION;
> +	/* don't floor on time to zero if a non-zero time was requested */
> +	if (!on && *delay_on)
> +		on += SLOPE_RESOLUTION;
> +	else if (on > BLINK_MAX_TIME)
> +		on = BLINK_MAX_TIME;
> +	*delay_on = on;
> +
> +	/* 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);
> +
> +	ret = regmap_write(led->chip->regmap, REG_LED_ON, led_on);
> +
> +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");

In case of SPDX GPL-2.0+ at the top we should have
MODULE_LICENSE("GPL") here.

-- 
Best regards,
Jacek Anaszewski

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

[Simon Shields]

Hi Jacek,

Thanks for the review.

On Tue, Mar 20, 2018 at 10:04:59PM +0100, Jacek Anaszewski wrote:
> Hi Simon,
> 
> Thanks for the update.
> 
> I'd like to clarify some details about brightness setting range
> and blink setting. Please refer to my comments in the code.
> 
> On 03/19/2018 02:59 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 | 391 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 403 insertions(+)
> >  create mode 100644 drivers/leds/leds-an30259a.c
> > 
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 3e763d2a0cb3..369bfb1227f8 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 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..61275eb8106d
> > --- /dev/null
> > +++ b/drivers/leds/leds-an30259a.c
> > @@ -0,0 +1,391 @@
> > +// 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;
> > +	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;
> > +};
> > +
> > +/*
> > + * When doing sloping, AN30259A only allows us
> > + * to set the first four bits of a 7-bit brightness
> > + * value, as opposed to the full 8-bit range
> > + * allowed in constant output mode.
> > + *
> > + * This function returns the best approximation
> > + * of the 8-bit brightness value in the 7-bit range
> > + * we have available.
> > + */
> > +static u8 an30259a_get_dutymax(u8 brightness)
> > +{
> > +	u8 duty_max, floor, ceil;
> > +
> > +	/* squash 8 bit number into 7-bit PWM range. */
> > +	duty_max = brightness >> 1;
> 
> You should set max_brightness accordingly if the device
> doesn't support 255 brightness levels. Otherwise, if you
> leave it initialized to 0, the LED core will adjust it to LED_FULL,
> which will give misleading information to the userspace.

The device supports 255 brightness levels, but when doing hardware
blinking only 16 levels are supported (the user controls the top four
bits of the brightness level, and the bottom three are always set).

How should this be handled? It doesn't make sense to me to restrict the
brightness range that much just for the HW blink usecase (or maybe
dropping HW blink code entirely would be best?)

> 
> > +
> > +	/*
> > +	 * Bottom 3 bits are always set for DUTYMAX,
> > +	 * so 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_set(struct led_classdev *cdev,
> > +				   enum led_brightness brightness)
> > +{
> > +	struct an30259a_led *led;
> > +	int ret;
> > +	unsigned int led_on;
> > +	u8 dutymax;
> > +
> > +	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);
> > +		break;
> > +	default:
> > +		led_on |= LED_EN(led->num);
> > +		dutymax = an30259a_get_dutymax(brightness & 0xff);
> > +		ret = regmap_write(led->chip->regmap, REG_LEDCNT1(led->num),
> > +				   LED_DUTYMAX(dutymax) | LED_DUTYMID(dutymax));
> > +		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 & 0xff);
> > +
> > +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;
> > +	unsigned long off = *delay_off, on = *delay_on;
> 
> Please keep in mind that blink_set should return -EINVAL in case
> the device can't handle given delay_on/off constraints. LED core will
> employ software blink fallback then.

The documentation in include/linux/leds.h says something a bit
different:

    "Activate hardware accelerated blink, delays are in milliseconds
    and if both are zero then a sensible default should be chosen.
    The call should adjust the timings in that case and if it can't
    match the values specified exactly."

So I guess that comment should be updated. I'll add support for
delays both being zero in v4, as well.

> 
> > +
> > +	led = container_of(cdev, struct an30259a_led, cdev);
> > +
> > +	mutex_lock(&led->chip->mutex);
> > +	num = led->num;
> > +
> > +	/* slope time - multiples of 500ms only, floored */
> > +	off -= off % SLOPE_RESOLUTION;
> > +	/* don't floor off time to zero if a non-zero time was requested */
> > +	if (!off && *delay_off)
> > +		off += SLOPE_RESOLUTION;
> > +	else if (off > BLINK_MAX_TIME)
> > +		off = BLINK_MAX_TIME;
> > +	*delay_off = off;
> > +
> > +	on -= on % SLOPE_RESOLUTION;
> > +	/* don't floor on time to zero if a non-zero time was requested */
> > +	if (!on && *delay_on)
> > +		on += SLOPE_RESOLUTION;
> > +	else if (on > BLINK_MAX_TIME)
> > +		on = BLINK_MAX_TIME;
> > +	*delay_on = on;
> > +
> > +	/* 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);
> > +
> > +	ret = regmap_write(led->chip->regmap, REG_LED_ON, led_on);
> > +
> > +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");
> 
> In case of SPDX GPL-2.0+ at the top we should have
> MODULE_LICENSE("GPL") here.

Ack.

> 
> -- 
> Best regards,
> Jacek Anaszewski

Cheers,
Simon

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

[Jacek Anaszewski]

Hi Simon,

On 03/21/2018 12:39 AM, Simon Shields wrote:
> Hi Jacek,
> 
> Thanks for the review.
> 
> On Tue, Mar 20, 2018 at 10:04:59PM +0100, Jacek Anaszewski wrote:
>> Hi Simon,
>>
>> Thanks for the update.
>>
>> I'd like to clarify some details about brightness setting range
>> and blink setting. Please refer to my comments in the code.
>>
>> On 03/19/2018 02:59 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 | 391 +++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 403 insertions(+)
>>>  create mode 100644 drivers/leds/leds-an30259a.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 3e763d2a0cb3..369bfb1227f8 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 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..61275eb8106d
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-an30259a.c
>>> @@ -0,0 +1,391 @@
>>> +// 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;
>>> +	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;
>>> +};
>>> +
>>> +/*
>>> + * When doing sloping, AN30259A only allows us
>>> + * to set the first four bits of a 7-bit brightness
>>> + * value, as opposed to the full 8-bit range
>>> + * allowed in constant output mode.
>>> + *
>>> + * This function returns the best approximation
>>> + * of the 8-bit brightness value in the 7-bit range
>>> + * we have available.
>>> + */
>>> +static u8 an30259a_get_dutymax(u8 brightness)
>>> +{
>>> +	u8 duty_max, floor, ceil;
>>> +
>>> +	/* squash 8 bit number into 7-bit PWM range. */
>>> +	duty_max = brightness >> 1;
>>
>> You should set max_brightness accordingly if the device
>> doesn't support 255 brightness levels. Otherwise, if you
>> leave it initialized to 0, the LED core will adjust it to LED_FULL,
>> which will give misleading information to the userspace.
> 
> The device supports 255 brightness levels, but when doing hardware
> blinking only 16 levels are supported (the user controls the top four
> bits of the brightness level, and the bottom three are always set).
> 
> How should this be handled? It doesn't make sense to me to restrict the
> brightness range that much just for the HW blink usecase (or maybe
> dropping HW blink code entirely would be best?)

brightness sysfs file should always report the actual brightness
set in the hardware, so when applying hardware blink the reported
brightness should be updated.

Apart from that, I noticed some problem in the calculations below,
please verify if I am not missing something.

>>
>>> +
>>> +	/*
>>> +	 * Bottom 3 bits are always set for DUTYMAX,
>>> +	 * so 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;
>>> +

Let's consider the case for brightness = 8:

brightness = 8
duty_max = brightness >> 1 // 4
ceil = duty_max | 0x7 // 7
floor = ceil - 0x8 // -1 but floor is unsigned

Did you take into account such a case?


>>> +	return duty_max;
>>> +}
>>> +
>>> +static int an30259a_brightness_set(struct led_classdev *cdev,
>>> +				   enum led_brightness brightness)
>>> +{
>>> +	struct an30259a_led *led;
>>> +	int ret;
>>> +	unsigned int led_on;
>>> +	u8 dutymax;
>>> +
>>> +	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);
>>> +		break;
>>> +	default:
>>> +		led_on |= LED_EN(led->num);
>>> +		dutymax = an30259a_get_dutymax(brightness & 0xff);

The "& 0xff" conjunction here is redundant - LED core will not allow to
pass here the value greater than max_brightness, which will be adjusted
to LED_FULL in the led_classdev_register() if left initialized to 0.

>>> +		ret = regmap_write(led->chip->regmap, REG_LEDCNT1(led->num),
>>> +				   LED_DUTYMAX(dutymax) | LED_DUTYMID(dutymax));
>>> +		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 & 0xff);
>>> +
>>> +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;
>>> +	unsigned long off = *delay_off, on = *delay_on;
>>
>> Please keep in mind that blink_set should return -EINVAL in case
>> the device can't handle given delay_on/off constraints. LED core will
>> employ software blink fallback then.
> 
> The documentation in include/linux/leds.h says something a bit
> different:
> 
>     "Activate hardware accelerated blink, delays are in milliseconds
>     and if both are zero then a sensible default should be chosen.
>     The call should adjust the timings in that case and if it can't
>     match the values specified exactly."
> 
> So I guess that comment should be updated. I'll add support for
> delays both being zero in v4, as well.

In fact, this should be updated. Most LED class drivers that
implement blink_set op return 0 from it only if they can handle
the requested delay_on/off values. Please consult those drivers
to get the flavor on how blink_set is being usually implemented.

>>> +
>>> +	led = container_of(cdev, struct an30259a_led, cdev);
>>> +
>>> +	mutex_lock(&led->chip->mutex);
>>> +	num = led->num;
>>> +
>>> +	/* slope time - multiples of 500ms only, floored */
>>> +	off -= off % SLOPE_RESOLUTION;
>>> +	/* don't floor off time to zero if a non-zero time was requested */
>>> +	if (!off && *delay_off)
>>> +		off += SLOPE_RESOLUTION;
>>> +	else if (off > BLINK_MAX_TIME)
>>> +		off = BLINK_MAX_TIME;
>>> +	*delay_off = off;
>>> +
>>> +	on -= on % SLOPE_RESOLUTION;
>>> +	/* don't floor on time to zero if a non-zero time was requested */
>>> +	if (!on && *delay_on)
>>> +		on += SLOPE_RESOLUTION;
>>> +	else if (on > BLINK_MAX_TIME)
>>> +		on = BLINK_MAX_TIME;
>>> +	*delay_on = on;
>>> +
>>> +	/* 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);
>>> +
>>> +	ret = regmap_write(led->chip->regmap, REG_LED_ON, led_on);
>>> +
>>> +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");
>>
>> In case of SPDX GPL-2.0+ at the top we should have
>> MODULE_LICENSE("GPL") here.
> 
> Ack.
> 
>>
>> -- 
>> Best regards,
>> Jacek Anaszewski
> 
> Cheers,
> Simon
> 

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

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


On Tue 2018-03-20 00:59:37, 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>

You still need device tree description AFAICT:

pavel@amd:/data/l/linux$ grep -ri an30259 Documentation/devicetree/
pavel@amd:/data/l/linux$

									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 --]

[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 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