[PATCH 1/3] leds: add aw2013 driver

[PATCH 1/3] leds: add aw2013 driver

[nikitos.tr]

From: Nikita Travkin <nikitos.tr@gmail.com>

This commit adds support for AWINIC AW2013 3-channel LED driver.
The chip supports 3 PWM channels and is controlled with I2C.

Signed-off-by: Nikita Travkin <nikitos.tr@gmail.com>
---
 drivers/leds/Kconfig       |  10 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-aw2013.c | 481 +++++++++++++++++++++++++++++++++++++
 3 files changed, 492 insertions(+)
 create mode 100644 drivers/leds/leds-aw2013.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9cdc4cfc5d11..ed943140e1fd 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -103,6 +103,16 @@ config LEDS_AS3645A
 	  controller. V4L2 flash API is provided as well if
 	  CONFIG_V4L2_FLASH_API is enabled.
 
+config LEDS_AW2013
+	tristate "LED support for Awinic AW2013"
+	depends on LEDS_CLASS && I2C && OF
+	help
+	  This option enables support for the AW2013 3-channel
+	  LED driver.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-aw2013.
+
 config LEDS_BCM6328
 	tristate "LED Support for Broadcom BCM6328"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d0dff504f108..d6b8a792c936 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
 obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
 obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
 obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
+obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.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-aw2013.c b/drivers/leds/leds-aw2013.c
new file mode 100644
index 000000000000..371f128ab562
--- /dev/null
+++ b/drivers/leds/leds-aw2013.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Driver for Awinic AW2013 3-channel LED driver
+
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#define AW2013_MAX_LEDS 3
+
+/* Reset and ID register */
+#define AW2013_RSTR 0x00
+#define AW2013_RSTR_RESET 0x55
+#define AW2013_RSTR_CHIP_ID 0x33
+
+/* Global control register */
+#define AW2013_GCR 0x01
+#define AW2013_GCR_ENABLE BIT(0)
+
+/* LED channel enable register */
+#define AW2013_LCTR 0x30
+#define AW2013_LCTR_LE(x) BIT((x))
+
+/* LED channel control registers */
+#define AW2013_LCFG(x) (0x31 + (x))
+#define AW2013_LCFG_IMAX_MASK (BIT(0) | BIT(1)) // Should be 0-3
+#define AW2013_LCFG_MD BIT(4)
+#define AW2013_LCFG_FI BIT(5)
+#define AW2013_LCFG_FO BIT(6)
+
+/* LED channel PWM registers */
+#define AW2013_REG_PWM(x) (0x34 + (x))
+
+/* LED channel timing registers */
+#define AW2013_LEDT0(x) (0x37 + (x) * 3)
+#define AW2013_LEDT0_T1(x) ((x) << 4) // Should be 0-7
+#define AW2013_LEDT0_T2(x) (x) // Should be 0-5
+
+#define AW2013_LEDT1(x) (0x38 + (x) * 3)
+#define AW2013_LEDT1_T3(x) ((x) << 4) // Should be 0-7
+#define AW2013_LEDT1_T4(x) (x) // Should be 0-7
+
+#define AW2013_LEDT2(x) (0x39 + (x) * 3)
+#define AW2013_LEDT2_T0(x) ((x) << 4) // Should be 0-8
+#define AW2013_LEDT2_REPEAT(x) (x) // Should be 0-15
+
+#define AW2013_REG_MAX 0x77
+
+#define AW2013_NAME "aw2013"
+
+#define AW2013_TIME_STEP 130
+
+#define STATE_OFF 0
+#define STATE_KEEP 1
+#define STATE_ON 2
+
+struct aw2013;
+
+struct aw2013_led {
+	struct aw2013 *chip;
+	struct fwnode_handle *fwnode;
+	struct led_classdev cdev;
+	u32 num;
+	unsigned int imax;
+	u32 default_state;
+};
+
+struct aw2013 {
+	struct mutex mutex; /* held when writing to registers */
+	struct regulator *vcc_regulator;
+	struct i2c_client *client;
+	struct aw2013_led leds[AW2013_MAX_LEDS];
+	struct regmap *regmap;
+	int num_leds;
+	bool enabled;
+};
+
+static int aw2013_chip_init(struct aw2013 *chip)
+{
+	int i, ret;
+
+	ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
+	if (ret) {
+		dev_err(&chip->client->dev, "Failed to enable the chip: %d\n",
+			ret);
+		goto error;
+	}
+
+	for (i = 0; i < chip->num_leds; i++) {
+		ret = regmap_update_bits(chip->regmap,
+					 AW2013_LCFG(chip->leds[i].num),
+					 AW2013_LCFG_IMAX_MASK,
+					 chip->leds[i].imax);
+		if (ret) {
+			dev_err(&chip->client->dev,
+				"Failed to set maximum current for led %d: %d\n",
+				chip->leds[i].num, ret);
+			goto error;
+		}
+	}
+
+error:
+	return ret;
+}
+
+static void aw2013_chip_disable(struct aw2013 *chip)
+{
+	int ret;
+
+	if (!chip->enabled)
+		return;
+
+	regmap_write(chip->regmap, AW2013_GCR, 0);
+
+	ret = regulator_disable(chip->vcc_regulator);
+	if (ret) {
+		dev_err(&chip->client->dev,
+			"Failed to disable regulator: %d\n", ret);
+		return;
+	}
+
+	chip->enabled = false;
+}
+
+static int aw2013_chip_enable(struct aw2013 *chip)
+{
+	int ret;
+
+	if (chip->enabled)
+		return 0;
+
+	ret = regulator_enable(chip->vcc_regulator);
+	if (ret) {
+		dev_err(&chip->client->dev,
+			"Failed to enable regulator: %d\n", ret);
+		return ret;
+	}
+	chip->enabled = true;
+
+	ret = aw2013_chip_init(chip);
+	if (ret)
+		aw2013_chip_disable(chip);
+
+	return ret;
+}
+
+static bool aw2013_chip_in_use(struct aw2013 *chip)
+{
+	int i;
+
+	for (i = 0; i < chip->num_leds; i++)
+		if (chip->leds[i].cdev.brightness)
+			return true;
+
+	return false;
+}
+
+static int aw2013_brightness_set(struct led_classdev *cdev,
+				 enum led_brightness brightness)
+{
+	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
+	int ret, num;
+
+	mutex_lock(&led->chip->mutex);
+
+	if (aw2013_chip_in_use(led->chip)) {
+		ret = aw2013_chip_enable(led->chip);
+		if (ret)
+			return ret;
+	}
+
+	num = led->num;
+
+	ret = regmap_write(led->chip->regmap, AW2013_REG_PWM(num), brightness);
+	if (ret)
+		goto error;
+
+	if (brightness) {
+		ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
+					 AW2013_LCTR_LE(num), 0xFF);
+	} else {
+		ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
+					 AW2013_LCTR_LE(num), 0);
+		if (ret)
+			goto error;
+		ret = regmap_update_bits(led->chip->regmap, AW2013_LCFG(num),
+					 AW2013_LCFG_MD, 0);
+	}
+	if (ret)
+		goto error;
+
+	if (!aw2013_chip_in_use(led->chip))
+		aw2013_chip_disable(led->chip);
+
+error:
+	mutex_unlock(&led->chip->mutex);
+
+	return ret;
+}
+
+static int aw2013_blink_set(struct led_classdev *cdev,
+			    unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
+	int ret, num = led->num;
+	unsigned long off = 0, on = 0;
+
+	/* If no blink specified, default to 1 Hz. */
+	if (!*delay_off && !*delay_on) {
+		*delay_off = 500;
+		*delay_on = 500;
+	}
+
+	/* Never on - just set to off */
+	if (!*delay_on)
+		return aw2013_brightness_set(&led->cdev, LED_OFF);
+
+	/* Never off - just set to brightness */
+	if (!*delay_off)
+		return aw2013_brightness_set(&led->cdev, led->cdev.brightness);
+
+	if (!led->cdev.brightness) {
+		led->cdev.brightness = LED_FULL;
+		ret = aw2013_brightness_set(&led->cdev, led->cdev.brightness);
+		if (ret)
+			return ret;
+	}
+
+	/* Convert into values the HW will understand. */
+	off = min(5, ilog2((*delay_off - 1) / AW2013_TIME_STEP) + 1);
+	on = min(7, ilog2((*delay_on - 1) / AW2013_TIME_STEP) + 1);
+
+	*delay_off = BIT(off) * AW2013_TIME_STEP;
+	*delay_on = BIT(on) * AW2013_TIME_STEP;
+
+	mutex_lock(&led->chip->mutex);
+
+	/* Set timings */
+	ret = regmap_write(led->chip->regmap,
+			   AW2013_LEDT0(num), AW2013_LEDT0_T2(on));
+	if (ret)
+		goto error;
+	ret = regmap_write(led->chip->regmap,
+			   AW2013_LEDT1(num), AW2013_LEDT1_T4(off));
+	if (ret)
+		goto error;
+
+	/* Finally, enable the LED */
+	ret = regmap_update_bits(led->chip->regmap, AW2013_LCFG(num),
+				 AW2013_LCFG_MD, 0xFF);
+	if (ret)
+		goto error;
+
+	ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
+				 AW2013_LCTR_LE(num), 0xFF);
+
+error:
+	mutex_unlock(&led->chip->mutex);
+
+	return ret;
+}
+
+static int aw2013_dt_init(struct i2c_client *client,
+			  struct aw2013 *chip)
+{
+	struct device_node *np = client->dev.of_node, *child;
+	int count, ret;
+	int i = 0;
+	const char *str;
+	struct aw2013_led *led;
+
+	count = of_get_child_count(np);
+	if (!count || count > AW2013_MAX_LEDS)
+		return -EINVAL;
+
+	for_each_available_child_of_node(np, child) {
+		u32 source;
+		u32 imax;
+
+		ret = of_property_read_u32(child, "reg", &source);
+		if (ret != 0 || source >= AW2013_MAX_LEDS) {
+			dev_err(&client->dev,
+				"Couldn't read LED address: %d\n", ret);
+			count--;
+			continue;
+		}
+
+		led = &chip->leds[i];
+
+		if (!of_property_read_u32(child, "led-max-microamp", &imax)) {
+			led->imax = min_t(u32, imax / 5000, 3);
+		} else {
+			led->imax = 1; // 5mA
+			dev_info(&client->dev,
+				 "DT property led-max-microamp is missing!\n");
+		}
+
+		led->num = source;
+		led->chip = chip;
+		led->fwnode = of_fwnode_handle(child);
+
+		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 aw2013_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AW2013_REG_MAX,
+};
+
+static void aw2013_read_current_state(struct aw2013 *chip)
+{
+	int i, led_on;
+
+	regmap_read(chip->regmap, AW2013_LCTR, &led_on);
+
+	for (i = 0; i < chip->num_leds; i++) {
+		if (!(led_on & AW2013_LCTR_LE(chip->leds[i].num))) {
+			chip->leds[i].cdev.brightness = LED_OFF;
+			continue;
+		}
+		regmap_read(chip->regmap, AW2013_REG_PWM(chip->leds[i].num),
+			    &chip->leds[i].cdev.brightness);
+	}
+}
+
+static void aw2013_init_default_state(struct aw2013_led *led)
+{
+	switch (led->default_state) {
+	case STATE_ON:
+		led->cdev.brightness = LED_FULL;
+		break;
+	case STATE_OFF:
+		led->cdev.brightness = LED_OFF;
+	} /* On keep - just set brightness that was retrieved previously */
+
+	aw2013_brightness_set(&led->cdev, led->cdev.brightness);
+}
+
+static int aw2013_probe(struct i2c_client *client)
+{
+	struct aw2013 *chip;
+	int i, ret;
+	unsigned int chipid;
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	ret = aw2013_dt_init(client, chip);
+	if (ret < 0)
+		return ret;
+
+	mutex_init(&chip->mutex);
+	chip->client = client;
+	i2c_set_clientdata(client, chip);
+
+	chip->regmap = devm_regmap_init_i2c(client, &aw2013_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		ret = PTR_ERR(chip->regmap);
+		dev_err(&client->dev, "Failed to allocate register map: %d\n",
+			ret);
+		goto error;
+	}
+
+	chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc");
+	ret = PTR_ERR_OR_ZERO(chip->vcc_regulator);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&client->dev,
+				"Failed to request regulator: %d\n", ret);
+		goto error;
+	}
+
+	ret = regulator_enable(chip->vcc_regulator);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to enable regulator: %d\n", ret);
+		goto error;
+	}
+	chip->enabled = true;
+
+	ret = regmap_read(chip->regmap, AW2013_RSTR, &chipid);
+	if (ret) {
+		dev_err(&client->dev, "Failed to read chip ID: %d\n",
+			ret);
+		goto error;
+	}
+
+	if (chipid != AW2013_RSTR_CHIP_ID) {
+		dev_err(&client->dev, "Chip reported wrong ID: %x\n",
+			chipid);
+		ret = -ENODEV;
+		goto error;
+	}
+
+	ret = aw2013_chip_init(chip);
+	if (ret)
+		goto error;
+
+	aw2013_read_current_state(chip);
+
+	for (i = 0; i < chip->num_leds; i++) {
+		struct led_init_data init_data = {};
+
+		aw2013_init_default_state(&chip->leds[i]);
+
+		chip->leds[i].cdev.brightness_set_blocking =
+			aw2013_brightness_set;
+		chip->leds[i].cdev.blink_set = aw2013_blink_set;
+
+		init_data.fwnode = chip->leds[i].fwnode;
+		init_data.devicename = AW2013_NAME;
+		init_data.default_label = ":";
+
+		ret = devm_led_classdev_register_ext(&client->dev,
+						     &chip->leds[i].cdev,
+						     &init_data);
+		if (ret < 0)
+			goto error;
+	}
+	return 0;
+
+error:
+	mutex_destroy(&chip->mutex);
+	return ret;
+}
+
+static int aw2013_remove(struct i2c_client *client)
+{
+	struct aw2013 *chip = i2c_get_clientdata(client);
+
+	mutex_destroy(&chip->mutex);
+
+	return 0;
+}
+
+static const struct of_device_id aw2013_match_table[] = {
+	{ .compatible = "awinic,aw2013", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, aw2013_match_table);
+
+static struct i2c_driver aw2013_driver = {
+	.driver = {
+		.name = "leds-aw2013",
+		.of_match_table = of_match_ptr(aw2013_match_table),
+	},
+	.probe_new = aw2013_probe,
+	.remove = aw2013_remove,
+};
+
+module_i2c_driver(aw2013_driver);
+
+MODULE_AUTHOR("Nikita Travkin <nikitos.tr@gmail.com>");
+MODULE_DESCRIPTION("AW2013 LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

[PATCH 2/3] dt-bindings: leds: Add binding for aw2013

[nikitos.tr]

From: Nikita Travkin <nikitos.tr@gmail.com>

Add YAML devicetree binding for AWINIC AW2013 3-channel led driver

Signed-off-by: Nikita Travkin <nikitos.tr@gmail.com>
---
 .../devicetree/bindings/leds/leds-aw2013.yaml | 91 +++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-aw2013.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
new file mode 100644
index 000000000000..f118721df1e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-aw2013.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AWINIC AW2013 3-channel LED Driver
+
+maintainers:
+  - Nikita Travkin <nikitos.tr@gmail.com>
+
+description: |
+  The AW2013 is a 3-channel LED driver with I2C interface. It can control
+  LED brightness with PWM output.
+
+properties:
+  compatible:
+    const: awinic,aw2013
+
+  reg:
+    maxItems: 1
+
+  vcc-supply:
+    description: Regulator providing power to the "VCC" pin.
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-2]$":
+    type: object
+    allOf:
+      - $ref: common.yaml#
+
+    properties:
+      reg:
+        description: Index of the LED.
+        minimum: 0
+        maximum: 2
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@45 {
+            compatible = "awinic,aw2013";
+            reg = <0x45>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            vcc-supply = <&pm8916_l17>;
+
+            led@0 {
+                reg = <0>;
+                led-max-microamp = <5000>;
+                function = LED_FUNCTION_INDICATOR;
+                color = <LED_COLOR_ID_RED>;
+            };
+
+            led@1 {
+                reg = <1>;
+                led-max-microamp = <5000>;
+                function = LED_FUNCTION_INDICATOR;
+                color = <LED_COLOR_ID_GREEN>;
+            };
+
+            led@2 {
+                reg = <2>;
+                led-max-microamp = <5000>;
+                function = LED_FUNCTION_INDICATOR;
+                color = <LED_COLOR_ID_BLUE>;
+            };
+        };
+    };
+...
-- 
2.20.1

[PATCH 3/3] dt-bindings: vendor-prefixes: Add Shanghai Awinic Technology Co., Ltd.

[nikitos.tr]

From: Nikita Travkin <nikitos.tr@gmail.com>

Add the "awinic" vendor prefix for Shanghai Awinic Technology Co., Ltd.
Website: https://www.awinic.com/

Signed-off-by: Nikita Travkin <nikitos.tr@gmail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 334393037861..6a77e14c216e 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -133,6 +133,8 @@ patternProperties:
     description: Shanghai AVIC Optoelectronics Co., Ltd.
   "^avnet,.*":
     description: Avnet, Inc.
+  "^awinic,.*":
+    description: Shanghai Awinic Technology Co., Ltd.
   "^axentia,.*":
     description: Axentia Technologies AB
   "^axis,.*":
-- 
2.20.1

Re: [PATCH 1/3] leds: add aw2013 driver

[Pavel Machek]

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

Hi!

> +#define AW2013_NAME "aw2013"

That's.... not really useful define. Make it NAME? Drop it?

> +#define AW2013_TIME_STEP 130

I'd add comment with /* units */.

> +#define STATE_OFF 0
> +#define STATE_KEEP 1
> +#define STATE_ON 2

We should add enum into core for this...

> +static int aw2013_chip_init(struct aw2013 *chip)
> +{
> +	int i, ret;
> +
> +	ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "Failed to enable the chip: %d\n",
> +			ret);
> +		goto error;
> +	}
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		ret = regmap_update_bits(chip->regmap,
> +					 AW2013_LCFG(chip->leds[i].num),
> +					 AW2013_LCFG_IMAX_MASK,
> +					 chip->leds[i].imax);
> +		if (ret) {
> +			dev_err(&chip->client->dev,
> +				"Failed to set maximum current for led %d: %d\n",
> +				chip->leds[i].num, ret);
> +			goto error;
> +		}
> +	}
> +
> +error:
> +	return ret;
> +}

No need for goto if you are just returning.

> +static bool aw2013_chip_in_use(struct aw2013 *chip)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->num_leds; i++)
> +		if (chip->leds[i].cdev.brightness)
> +			return true;
> +
> +	return false;
> +}

How is this going to interact with ledstate == KEEP?

> +static int aw2013_brightness_set(struct led_classdev *cdev,
> +				 enum led_brightness brightness)
> +{
> +	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
> +	int ret, num;
> +
> +	mutex_lock(&led->chip->mutex);
> +
> +	if (aw2013_chip_in_use(led->chip)) {
> +		ret = aw2013_chip_enable(led->chip);
> +		if (ret)
> +			return ret;
> +	}

You are returning with mutex held.

> +	/* Never on - just set to off */
> +	if (!*delay_on)
> +		return aw2013_brightness_set(&led->cdev, LED_OFF);
> +
> +	/* Never off - just set to brightness */
> +	if (!*delay_off)
> +		return aw2013_brightness_set(&led->cdev, led->cdev.brightness);

Is this dance neccessary? Should we do it in the core somewhere?

> +		} else {
> +			led->imax = 1; // 5mA
> +			dev_info(&client->dev,
> +				 "DT property led-max-microamp is missing!\n");
> +		}

Lets remove the exclamation mark.

> +		led->num = source;
> +		led->chip = chip;
> +		led->fwnode = of_fwnode_handle(child);
> +
> +		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;
> +		}

We should really have something in core for this. Should we support
arbitrary brightness there?

> +static void aw2013_read_current_state(struct aw2013 *chip)
> +{
> +	int i, led_on;
> +
> +	regmap_read(chip->regmap, AW2013_LCTR, &led_on);
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		if (!(led_on & AW2013_LCTR_LE(chip->leds[i].num))) {
> +			chip->leds[i].cdev.brightness = LED_OFF;
> +			continue;
> +		}
> +		regmap_read(chip->regmap, AW2013_REG_PWM(chip->leds[i].num),
> +			    &chip->leds[i].cdev.brightness);
> +	}
> +}
> +
> +static void aw2013_init_default_state(struct aw2013_led *led)
> +{
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		led->cdev.brightness = LED_FULL;
> +		break;
> +	case STATE_OFF:
> +		led->cdev.brightness = LED_OFF;
> +	} /* On keep - just set brightness that was retrieved previously */
> +
> +	aw2013_brightness_set(&led->cdev, led->cdev.brightness);
> +}

Aha; I guess this makes "keeping" the state to work. Do you really
need that functionality?

Pretty nice driver, thanks.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/3] leds: add aw2013 driver

[Jacek Anaszewski]

Hi Nikita,

On 5/4/20 6:29 PM, nikitos.tr@gmail.com wrote:
> From: Nikita Travkin <nikitos.tr@gmail.com>
> 
> This commit adds support for AWINIC AW2013 3-channel LED driver.
> The chip supports 3 PWM channels and is controlled with I2C.
> 
> Signed-off-by: Nikita Travkin <nikitos.tr@gmail.com>
> ---
>   drivers/leds/Kconfig       |  10 +
>   drivers/leds/Makefile      |   1 +
>   drivers/leds/leds-aw2013.c | 481 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 492 insertions(+)
>   create mode 100644 drivers/leds/leds-aw2013.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9cdc4cfc5d11..ed943140e1fd 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -103,6 +103,16 @@ config LEDS_AS3645A
>   	  controller. V4L2 flash API is provided as well if
>   	  CONFIG_V4L2_FLASH_API is enabled.
>   
> +config LEDS_AW2013
> +	tristate "LED support for Awinic AW2013"
> +	depends on LEDS_CLASS && I2C && OF
> +	help
> +	  This option enables support for the AW2013 3-channel
> +	  LED driver.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-aw2013.
> +
>   config LEDS_BCM6328
>   	tristate "LED Support for Broadcom BCM6328"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d0dff504f108..d6b8a792c936 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
>   obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
>   obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
>   obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
> +obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.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-aw2013.c b/drivers/leds/leds-aw2013.c
> new file mode 100644
> index 000000000000..371f128ab562
> --- /dev/null
> +++ b/drivers/leds/leds-aw2013.c
> @@ -0,0 +1,481 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Driver for Awinic AW2013 3-channel LED driver
> +
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +#define AW2013_MAX_LEDS 3
> +
> +/* Reset and ID register */
> +#define AW2013_RSTR 0x00
> +#define AW2013_RSTR_RESET 0x55
> +#define AW2013_RSTR_CHIP_ID 0x33
> +
> +/* Global control register */
> +#define AW2013_GCR 0x01
> +#define AW2013_GCR_ENABLE BIT(0)
> +
> +/* LED channel enable register */
> +#define AW2013_LCTR 0x30
> +#define AW2013_LCTR_LE(x) BIT((x))
> +
> +/* LED channel control registers */
> +#define AW2013_LCFG(x) (0x31 + (x))
> +#define AW2013_LCFG_IMAX_MASK (BIT(0) | BIT(1)) // Should be 0-3
> +#define AW2013_LCFG_MD BIT(4)
> +#define AW2013_LCFG_FI BIT(5)
> +#define AW2013_LCFG_FO BIT(6)
> +
> +/* LED channel PWM registers */
> +#define AW2013_REG_PWM(x) (0x34 + (x))
> +
> +/* LED channel timing registers */
> +#define AW2013_LEDT0(x) (0x37 + (x) * 3)
> +#define AW2013_LEDT0_T1(x) ((x) << 4) // Should be 0-7
> +#define AW2013_LEDT0_T2(x) (x) // Should be 0-5
> +
> +#define AW2013_LEDT1(x) (0x38 + (x) * 3)
> +#define AW2013_LEDT1_T3(x) ((x) << 4) // Should be 0-7
> +#define AW2013_LEDT1_T4(x) (x) // Should be 0-7
> +
> +#define AW2013_LEDT2(x) (0x39 + (x) * 3)
> +#define AW2013_LEDT2_T0(x) ((x) << 4) // Should be 0-8
> +#define AW2013_LEDT2_REPEAT(x) (x) // Should be 0-15
> +
> +#define AW2013_REG_MAX 0x77
> +
> +#define AW2013_NAME "aw2013"
> +
> +#define AW2013_TIME_STEP 130
> +
> +#define STATE_OFF 0
> +#define STATE_KEEP 1
> +#define STATE_ON 2
> +
> +struct aw2013;
> +
> +struct aw2013_led {
> +	struct aw2013 *chip;
> +	struct fwnode_handle *fwnode;
> +	struct led_classdev cdev;
> +	u32 num;
> +	unsigned int imax;
> +	u32 default_state;

fwnode and default_state are only needed in probe() so it is suboptimal
to keep them afterwards. You could avoid that if you did DT parsing
and LED registration in one function in the same iteration step.

You can compare other LED drivers following this convention, they
usually name related functions "*probe_dt()".

> +};
> +
> +struct aw2013 {
> +	struct mutex mutex; /* held when writing to registers */
> +	struct regulator *vcc_regulator;
> +	struct i2c_client *client;
> +	struct aw2013_led leds[AW2013_MAX_LEDS];
> +	struct regmap *regmap;
> +	int num_leds;
> +	bool enabled;
> +};
> +
> +static int aw2013_chip_init(struct aw2013 *chip)
> +{
> +	int i, ret;
> +
> +	ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "Failed to enable the chip: %d\n",
> +			ret);
> +		goto error;
> +	}
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		ret = regmap_update_bits(chip->regmap,
> +					 AW2013_LCFG(chip->leds[i].num),
> +					 AW2013_LCFG_IMAX_MASK,
> +					 chip->leds[i].imax);
> +		if (ret) {
> +			dev_err(&chip->client->dev,
> +				"Failed to set maximum current for led %d: %d\n",
> +				chip->leds[i].num, ret);
> +			goto error;
> +		}
> +	}
> +
> +error:
> +	return ret;
> +}
> +
> +static void aw2013_chip_disable(struct aw2013 *chip)
> +{
> +	int ret;
> +
> +	if (!chip->enabled)
> +		return;
> +
> +	regmap_write(chip->regmap, AW2013_GCR, 0);
> +
> +	ret = regulator_disable(chip->vcc_regulator);
> +	if (ret) {
> +		dev_err(&chip->client->dev,
> +			"Failed to disable regulator: %d\n", ret);
> +		return;
> +	}
> +
> +	chip->enabled = false;
> +}
> +
> +static int aw2013_chip_enable(struct aw2013 *chip)
> +{
> +	int ret;
> +
> +	if (chip->enabled)
> +		return 0;
> +
> +	ret = regulator_enable(chip->vcc_regulator);
> +	if (ret) {
> +		dev_err(&chip->client->dev,
> +			"Failed to enable regulator: %d\n", ret);
> +		return ret;
> +	}
> +	chip->enabled = true;
> +
> +	ret = aw2013_chip_init(chip);
> +	if (ret)
> +		aw2013_chip_disable(chip);
> +
> +	return ret;
> +}
> +
> +static bool aw2013_chip_in_use(struct aw2013 *chip)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->num_leds; i++)
> +		if (chip->leds[i].cdev.brightness)
> +			return true;
> +
> +	return false;
> +}
> +
> +static int aw2013_brightness_set(struct led_classdev *cdev,
> +				 enum led_brightness brightness)
> +{
> +	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
> +	int ret, num;
> +
> +	mutex_lock(&led->chip->mutex);
> +
> +	if (aw2013_chip_in_use(led->chip)) {
> +		ret = aw2013_chip_enable(led->chip);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	num = led->num;
> +
> +	ret = regmap_write(led->chip->regmap, AW2013_REG_PWM(num), brightness);
> +	if (ret)
> +		goto error;
> +
> +	if (brightness) {
> +		ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
> +					 AW2013_LCTR_LE(num), 0xFF);
> +	} else {
> +		ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
> +					 AW2013_LCTR_LE(num), 0);
> +		if (ret)
> +			goto error;
> +		ret = regmap_update_bits(led->chip->regmap, AW2013_LCFG(num),
> +					 AW2013_LCFG_MD, 0);
> +	}
> +	if (ret)
> +		goto error;
> +
> +	if (!aw2013_chip_in_use(led->chip))
> +		aw2013_chip_disable(led->chip);
> +
> +error:
> +	mutex_unlock(&led->chip->mutex);
> +
> +	return ret;
> +}
> +
> +static int aw2013_blink_set(struct led_classdev *cdev,
> +			    unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
> +	int ret, num = led->num;
> +	unsigned long off = 0, on = 0;
> +
> +	/* If no blink specified, default to 1 Hz. */
> +	if (!*delay_off && !*delay_on) {
> +		*delay_off = 500;
> +		*delay_on = 500;
> +	}
> +
> +	/* Never on - just set to off */
> +	if (!*delay_on)
> +		return aw2013_brightness_set(&led->cdev, LED_OFF);
> +
> +	/* Never off - just set to brightness */
> +	if (!*delay_off)
> +		return aw2013_brightness_set(&led->cdev, led->cdev.brightness);
> +
> +	if (!led->cdev.brightness) {
> +		led->cdev.brightness = LED_FULL;
> +		ret = aw2013_brightness_set(&led->cdev, led->cdev.brightness);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Convert into values the HW will understand. */
> +	off = min(5, ilog2((*delay_off - 1) / AW2013_TIME_STEP) + 1);
> +	on = min(7, ilog2((*delay_on - 1) / AW2013_TIME_STEP) + 1);
> +
> +	*delay_off = BIT(off) * AW2013_TIME_STEP;
> +	*delay_on = BIT(on) * AW2013_TIME_STEP;
> +
> +	mutex_lock(&led->chip->mutex);
> +
> +	/* Set timings */
> +	ret = regmap_write(led->chip->regmap,
> +			   AW2013_LEDT0(num), AW2013_LEDT0_T2(on));
> +	if (ret)
> +		goto error;
> +	ret = regmap_write(led->chip->regmap,
> +			   AW2013_LEDT1(num), AW2013_LEDT1_T4(off));
> +	if (ret)
> +		goto error;
> +
> +	/* Finally, enable the LED */
> +	ret = regmap_update_bits(led->chip->regmap, AW2013_LCFG(num),
> +				 AW2013_LCFG_MD, 0xFF);
> +	if (ret)
> +		goto error;
> +
> +	ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
> +				 AW2013_LCTR_LE(num), 0xFF);
> +
> +error:
> +	mutex_unlock(&led->chip->mutex);
> +
> +	return ret;
> +}
> +
> +static int aw2013_dt_init(struct i2c_client *client,
> +			  struct aw2013 *chip)
> +{
> +	struct device_node *np = client->dev.of_node, *child;
> +	int count, ret;
> +	int i = 0;
> +	const char *str;
> +	struct aw2013_led *led;
> +
> +	count = of_get_child_count(np);
> +	if (!count || count > AW2013_MAX_LEDS)
> +		return -EINVAL;
> +
> +	for_each_available_child_of_node(np, child) {
> +		u32 source;
> +		u32 imax;
> +
> +		ret = of_property_read_u32(child, "reg", &source);
> +		if (ret != 0 || source >= AW2013_MAX_LEDS) {
> +			dev_err(&client->dev,
> +				"Couldn't read LED address: %d\n", ret);
> +			count--;
> +			continue;
> +		}
> +
> +		led = &chip->leds[i];
> +
> +		if (!of_property_read_u32(child, "led-max-microamp", &imax)) {
> +			led->imax = min_t(u32, imax / 5000, 3);
> +		} else {
> +			led->imax = 1; // 5mA
> +			dev_info(&client->dev,
> +				 "DT property led-max-microamp is missing!\n");
> +		}
> +
> +		led->num = source;
> +		led->chip = chip;
> +		led->fwnode = of_fwnode_handle(child);
> +
> +		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 aw2013_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AW2013_REG_MAX,
> +};
> +
> +static void aw2013_read_current_state(struct aw2013 *chip)
> +{
> +	int i, led_on;
> +
> +	regmap_read(chip->regmap, AW2013_LCTR, &led_on);
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		if (!(led_on & AW2013_LCTR_LE(chip->leds[i].num))) {
> +			chip->leds[i].cdev.brightness = LED_OFF;
> +			continue;
> +		}
> +		regmap_read(chip->regmap, AW2013_REG_PWM(chip->leds[i].num),
> +			    &chip->leds[i].cdev.brightness);
> +	}
> +}
> +
> +static void aw2013_init_default_state(struct aw2013_led *led)
> +{
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		led->cdev.brightness = LED_FULL;
> +		break;
> +	case STATE_OFF:
> +		led->cdev.brightness = LED_OFF;
> +	} /* On keep - just set brightness that was retrieved previously */
> +
> +	aw2013_brightness_set(&led->cdev, led->cdev.brightness);
> +}
> +
> +static int aw2013_probe(struct i2c_client *client)
> +{
> +	struct aw2013 *chip;
> +	int i, ret;
> +	unsigned int chipid;
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	ret = aw2013_dt_init(client, chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_init(&chip->mutex);
> +	chip->client = client;
> +	i2c_set_clientdata(client, chip);
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &aw2013_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		ret = PTR_ERR(chip->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		goto error;
> +	}
> +
> +	chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc");
> +	ret = PTR_ERR_OR_ZERO(chip->vcc_regulator);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&client->dev,
> +				"Failed to request regulator: %d\n", ret);
> +		goto error;
> +	}
> +
> +	ret = regulator_enable(chip->vcc_regulator);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to enable regulator: %d\n", ret);
> +		goto error;
> +	}
> +	chip->enabled = true;
> +
> +	ret = regmap_read(chip->regmap, AW2013_RSTR, &chipid);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to read chip ID: %d\n",
> +			ret);
> +		goto error;
> +	}
> +
> +	if (chipid != AW2013_RSTR_CHIP_ID) {
> +		dev_err(&client->dev, "Chip reported wrong ID: %x\n",
> +			chipid);
> +		ret = -ENODEV;
> +		goto error;
> +	}
> +
> +	ret = aw2013_chip_init(chip);
> +	if (ret)
> +		goto error;
> +
> +	aw2013_read_current_state(chip);
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		struct led_init_data init_data = {};
> +
> +		aw2013_init_default_state(&chip->leds[i]);
> +
> +		chip->leds[i].cdev.brightness_set_blocking =
> +			aw2013_brightness_set;
> +		chip->leds[i].cdev.blink_set = aw2013_blink_set;
> +
> +		init_data.fwnode = chip->leds[i].fwnode;
> +		init_data.devicename = AW2013_NAME;
> +		init_data.default_label = ":";

Please drop two above lines, they are needed only when converting
old drivers to the new LED registration API.

> +
> +		ret = devm_led_classdev_register_ext(&client->dev,
> +						     &chip->leds[i].cdev,
> +						     &init_data);
> +		if (ret < 0)
> +			goto error;
> +	}
> +	return 0;
> +
> +error:
> +	mutex_destroy(&chip->mutex);
> +	return ret;
> +}
> +
> +static int aw2013_remove(struct i2c_client *client)
> +{
> +	struct aw2013 *chip = i2c_get_clientdata(client);
> +
> +	mutex_destroy(&chip->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id aw2013_match_table[] = {
> +	{ .compatible = "awinic,aw2013", },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, aw2013_match_table);
> +
> +static struct i2c_driver aw2013_driver = {
> +	.driver = {
> +		.name = "leds-aw2013",
> +		.of_match_table = of_match_ptr(aw2013_match_table),
> +	},
> +	.probe_new = aw2013_probe,
> +	.remove = aw2013_remove,
> +};
> +
> +module_i2c_driver(aw2013_driver);
> +
> +MODULE_AUTHOR("Nikita Travkin <nikitos.tr@gmail.com>");
> +MODULE_DESCRIPTION("AW2013 LED driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 2/3] dt-bindings: leds: Add binding for aw2013

[Dan Murphy]

Pavel

On 5/4/20 11:29 AM, nikitos.tr@gmail.com wrote:
> From: Nikita Travkin <nikitos.tr@gmail.com>
>
> Add YAML devicetree binding for AWINIC AW2013 3-channel led driver
>
> Signed-off-by: Nikita Travkin <nikitos.tr@gmail.com>
> ---
>   .../devicetree/bindings/leds/leds-aw2013.yaml | 91 +++++++++++++++++++
>   1 file changed, 91 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-aw2013.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> new file mode 100644
> index 000000000000..f118721df1e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-aw2013.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AWINIC AW2013 3-channel LED Driver
> +
> +maintainers:
> +  - Nikita Travkin <nikitos.tr@gmail.com>
> +
> +description: |
> +  The AW2013 is a 3-channel LED driver with I2C interface. It can control
> +  LED brightness with PWM output.
> +
> +properties:
> +  compatible:
> +    const: awinic,aw2013
> +
> +  reg:
> +    maxItems: 1
> +
> +  vcc-supply:
> +    description: Regulator providing power to the "VCC" pin.
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^led@[0-2]$":
> +    type: object
> +    allOf:
> +      - $ref: common.yaml#
> +
> +    properties:
> +      reg:
> +        description: Index of the LED.
> +        minimum: 0
> +        maximum: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
> +
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led-controller@45 {
> +            compatible = "awinic,aw2013";
> +            reg = <0x45>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            vcc-supply = <&pm8916_l17>;
> +
> +            led@0 {
> +                reg = <0>;
> +                led-max-microamp = <5000>;
> +                function = LED_FUNCTION_INDICATOR;
> +                color = <LED_COLOR_ID_RED>;
> +            };
> +
> +            led@1 {
> +                reg = <1>;
> +                led-max-microamp = <5000>;
> +                function = LED_FUNCTION_INDICATOR;
> +                color = <LED_COLOR_ID_GREEN>;
> +            };
> +
> +            led@2 {
> +                reg = <2>;
> +                led-max-microamp = <5000>;
> +                function = LED_FUNCTION_INDICATOR;
> +                color = <LED_COLOR_ID_BLUE>;
> +            };
> +        };
> +    };
> +...

Would this be a good candidate for the multicolor framework?

Dan

Re: [PATCH 1/3] leds: add aw2013 driver

[Nikita Travkin]

> Hi!
>
>> +#define AW2013_NAME "aw2013"
> That's.... not really useful define. Make it NAME? Drop it?
Will drop it as well as (unnecessary) lines it is used in.
>> +#define AW2013_TIME_STEP 130
> I'd add comment with /* units */.
Will add.
>> +#define STATE_OFF 0
>> +#define STATE_KEEP 1
>> +#define STATE_ON 2
> We should add enum into core for this...
>
>> +static int aw2013_chip_init(struct aw2013 *chip)
>> +{
>> +	int i, ret;
>> +
>> +	ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
>> +	if (ret) {
>> +		dev_err(&chip->client->dev, "Failed to enable the chip: %d\n",
>> +			ret);
>> +		goto error;
>> +	}
>> +
>> +	for (i = 0; i < chip->num_leds; i++) {
>> +		ret = regmap_update_bits(chip->regmap,
>> +					 AW2013_LCFG(chip->leds[i].num),
>> +					 AW2013_LCFG_IMAX_MASK,
>> +					 chip->leds[i].imax);
>> +		if (ret) {
>> +			dev_err(&chip->client->dev,
>> +				"Failed to set maximum current for led %d: %d\n",
>> +				chip->leds[i].num, ret);
>> +			goto error;
>> +		}
>> +	}
>> +
>> +error:
>> +	return ret;
>> +}
> No need for goto if you are just returning.
Will change it.
>> +static bool aw2013_chip_in_use(struct aw2013 *chip)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < chip->num_leds; i++)
>> +		if (chip->leds[i].cdev.brightness)
>> +			return true;
>> +
>> +	return false;
>> +}
> How is this going to interact with ledstate == KEEP?
>
>> +static int aw2013_brightness_set(struct led_classdev *cdev,
>> +				 enum led_brightness brightness)
>> +{
>> +	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
>> +	int ret, num;
>> +
>> +	mutex_lock(&led->chip->mutex);
>> +
>> +	if (aw2013_chip_in_use(led->chip)) {
>> +		ret = aw2013_chip_enable(led->chip);
>> +		if (ret)
>> +			return ret;
>> +	}
> You are returning with mutex held.
Will fix.
>> +	/* Never on - just set to off */
>> +	if (!*delay_on)
>> +		return aw2013_brightness_set(&led->cdev, LED_OFF);
>> +
>> +	/* Never off - just set to brightness */
>> +	if (!*delay_off)
>> +		return aw2013_brightness_set(&led->cdev, led->cdev.brightness);
> Is this dance neccessary? Should we do it in the core somewhere?
Right now blink_set() can be called with either delay_on or delay_off
being zero.

Passing zero into calculations I do later will result in garbage so
I'm trying to avoid it.

Core could probably handle situation where both are zero (This way
default values will be shared across all drivers) and if only
delay_on is zero it could disable led and the blink mode. (As if
brightness was set to 0)
In case where only delay_off is zero it's a bit more complicated
since driver should disable blinking but leave led on if it was
blinking already.

That also means that my current solution is a bit broken since changing
delay_off to zero while led is already blinking will call brightness_set
without clearing the mode bit so the led will still blink.

For now I will fix that and leave all those checks in place.
>> +		} else {
>> +			led->imax = 1; // 5mA
>> +			dev_info(&client->dev,
>> +				 "DT property led-max-microamp is missing!\n");
>> +		}
> Lets remove the exclamation mark.
Will do.
>> +		led->num = source;
>> +		led->chip = chip;
>> +		led->fwnode = of_fwnode_handle(child);
>> +
>> +		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;
>> +		}
> We should really have something in core for this. Should we support
> arbitrary brightness there?
Not sure if there is good dt property for that.
>> +static void aw2013_read_current_state(struct aw2013 *chip)
>> +{
>> +	int i, led_on;
>> +
>> +	regmap_read(chip->regmap, AW2013_LCTR, &led_on);
>> +
>> +	for (i = 0; i < chip->num_leds; i++) {
>> +		if (!(led_on & AW2013_LCTR_LE(chip->leds[i].num))) {
>> +			chip->leds[i].cdev.brightness = LED_OFF;
>> +			continue;
>> +		}
>> +		regmap_read(chip->regmap, AW2013_REG_PWM(chip->leds[i].num),
>> +			    &chip->leds[i].cdev.brightness);
>> +	}
>> +}
>> +
>> +static void aw2013_init_default_state(struct aw2013_led *led)
>> +{
>> +	switch (led->default_state) {
>> +	case STATE_ON:
>> +		led->cdev.brightness = LED_FULL;
>> +		break;
>> +	case STATE_OFF:
>> +		led->cdev.brightness = LED_OFF;
>> +	} /* On keep - just set brightness that was retrieved previously */
>> +
>> +	aw2013_brightness_set(&led->cdev, led->cdev.brightness);
>> +}
> Aha; I guess this makes "keeping" the state to work. Do you really
> need that functionality?
I don't need that. On some theoretical device the chip could be
enabled by bootloader but I consider that unlikely. I can drop
support for keeping state. It would be then easier to get rid of
"default_state" and "fwnode" in device struct. Should I?
>
> Pretty nice driver, thanks.
>
> 									Pavel