[PATCH v2 0/2] driver: leds: is31fl319x dimmable LED driver

[PATCH v2 0/2] driver: leds: is31fl319x dimmable LED driver

[H. Nikolaus Schaller]

Changes V2:
* suggested by David Rifshin
	* add more "compatible" strings for other chip variants/brands
	* renumber output <reg> values to expect a range of 1..9
	* fixes for typos and DT example, Kconfig message
	* fix location in Makefile and Kconfig
	* remove some dead/not implemented code
	* use of_property_read_string for better error handling
	* coding style improvements
	* use devm_led_classdev_register and simplify error path
* suggested by Jacek Anaszewski
	* fix more typos & writing style
	* separate bindings document into a second patch
	* max current property renamed and define uA instead of mA
	* debugging message improvements
	* remove platform data and header file completely and therefore require DT
	* use regmap to handle caching and locking + i2c serialization
* suggested by Rob Herring
	* bindings documentation style improvements

V1: 2016-04-18 20:43:18:
This patch adds a driver for the is31fl3196/99 dimmable dual/triple rgb
controller chips from ISSI.

H. Nikolaus Schaller (2):
  drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver
  Bindings documentation for ISSI is31fl319x driver

 .../devicetree/bindings/leds/is31fl319x.txt        |  52 +++
 drivers/leds/Kconfig                               |   8 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-is31fl319x.c                     | 354 +++++++++++++++++++++
 4 files changed, 415 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
 create mode 100644 drivers/leds/leds-is31fl319x.c

-- 
2.7.3

[PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[H. Nikolaus Schaller]

This is a driver for the Integrated Silicon Solution Inc. LED driver
chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
LEDs.

Each LED is individually controllable in brightness (through pwm)
in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.

The maximum current of the LEDs can be programmed and limited to
5 .. 40mA through a device tree property.

The chip is connected through I2C and can have one of 4 addresses
in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
address is defined by the reg property as usual.

The chip also has a shutdown input which could be connected to a GPIO,
but this driver uses software shutdown if all LEDs are inactivated.

The chip also has breathing and audio features which are not fully
supported by this driver.

Tested-on: OMAP5 based Pyra handheld prototype.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/leds/Kconfig           |   8 +
 drivers/leds/Makefile          |   1 +
 drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 drivers/leds/leds-is31fl319x.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 5ae2834..65b1045 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -570,6 +570,14 @@ config LEDS_SEAD3
 	  This driver can also be built as a module. If so the module
 	  will be called leds-sead3.
 
+config LEDS_IS31FL319X
+	tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
+	depends on LEDS_CLASS && I2C && OF
+	help
+	  This option enables support for LEDs connected to ISSI IS31FL319x
+	  fancy LED driver chips accessed via the I2C bus.
+	  Driver supports individual PWM brightness control for each channel.
+
 config LEDS_IS31FL32XX
 	tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
 	depends on LEDS_CLASS && I2C && OF
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index cb2013d..b6ce9f9 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
 obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
 obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
 obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
+obj-$(CONFIG_LEDS_IS31FL319X)		+= leds-is31fl319x.o
 obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
 
 # LED SPI Drivers
diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
new file mode 100644
index 0000000..bbf8850
--- /dev/null
+++ b/drivers/leds/leds-is31fl319x.c
@@ -0,0 +1,354 @@
+/*
+ * Copyright 2015-16 Golden Delicious Computers
+ *
+ * Author: Nikolaus Schaller <hns@goldelico.com>
+ *
+ * Based on leds-tca6507.c
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License.  See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
+ * effect LEDs.
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/* register numbers */
+#define IS31FL319X_SHUTDOWN	0x00
+#define IS31FL319X_CTRL1	0x01
+#define IS31FL319X_CTRL2	0x02
+#define IS31FL319X_CONFIG1	0x03
+#define IS31FL319X_CONFIG2	0x04
+#define IS31FL319X_RAMP_MODE	0x05
+#define IS31FL319X_BREATH_MASK	0x06
+#define IS31FL319X_PWM1		0x07
+#define IS31FL319X_PWM2		0x08
+#define IS31FL319X_PWM3		0x09
+#define IS31FL319X_PWM4		0x0a
+#define IS31FL319X_PWM5		0x0b
+#define IS31FL319X_PWM6		0x0c
+#define IS31FL319X_PWM7		0x0d
+#define IS31FL319X_PWM8		0x0e
+#define IS31FL319X_PWM9		0x0f
+#define IS31FL319X_DATA_UPDATE	0x10
+#define IS31FL319X_T0_1		0x11
+#define IS31FL319X_T0_2		0x12
+#define IS31FL319X_T0_3		0x13
+#define IS31FL319X_T0_4		0x14
+#define IS31FL319X_T0_5		0x15
+#define IS31FL319X_T0_6		0x16
+#define IS31FL319X_T0_7		0x17
+#define IS31FL319X_T0_8		0x18
+#define IS31FL319X_T0_9		0x19
+#define IS31FL319X_T123_1	0x1a
+#define IS31FL319X_T123_2	0x1b
+#define IS31FL319X_T123_3	0x1c
+#define IS31FL319X_T4_1		0x1d
+#define IS31FL319X_T4_2		0x1e
+#define IS31FL319X_T4_3		0x1f
+#define IS31FL319X_T4_4		0x20
+#define IS31FL319X_T4_5		0x21
+#define IS31FL319X_T4_6		0x22
+#define IS31FL319X_T4_7		0x23
+#define IS31FL319X_T4_8		0x24
+#define IS31FL319X_T4_9		0x25
+#define IS31FL319X_TIME_UPDATE	0x26
+#define IS31FL319X_RESET	0xff
+
+#define IS31FL319X_REG_CNT	(IS31FL319X_RESET + 1)
+
+#define NUM_LEDS 9	/* max for 3199 chip */
+
+struct is31fl319x_chip {
+	struct i2c_client	*client;
+	struct regmap		*regmap;
+
+	struct is31fl319x_led {
+		struct is31fl319x_chip	*chip;
+		struct led_classdev	led_cdev;
+	} leds[NUM_LEDS];
+};
+
+static const struct i2c_device_id is31fl319x_id[] = {
+	{ "is31fl3190", 1 },
+	{ "is31fl3191", 1 },
+	{ "is31fl3193", 3 },
+	{ "is31fl3196", 6 },
+	{ "is31fl3199", 9 },
+	{ "sn3199", 9 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
+
+static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
+				   enum led_brightness brightness)
+{
+	struct is31fl319x_led *led = container_of(led_cdev,
+						  struct is31fl319x_led,
+						  led_cdev);
+	struct is31fl319x_chip *is31 = led->chip;
+	int ret;
+
+	int i;
+	u8 ctrl1, ctrl2;
+
+	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
+		brightness);
+
+	/* update PWM register */
+	ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
+			   brightness);
+	if (ret < 0)
+		return ret;
+
+	ctrl1 = 0;
+	ctrl2 = 0;
+
+	/* read current brightness of all PWM channels */
+	for (i = 0; i < NUM_LEDS; i++) {
+		unsigned int pwm_value;
+		bool on;
+
+		/*
+		 * since neither led_cdev nor the chip can provide
+		 * the current setting, we read from the regmap cache
+		 */
+
+		ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
+				  &pwm_value);
+		dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
+			__func__, i, ret, pwm_value);
+		on = ret >= 0 && pwm_value > LED_OFF;
+
+		if (i < 3)
+			ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
+		else if (i < 6)
+			ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
+		else
+			ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
+	}
+
+	if (ctrl1 > 0 || ctrl2 > 0) {
+		dev_dbg(&is31->client->dev, "power up %02x %02x\n",
+			ctrl1, ctrl2);
+		regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
+		regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
+		/* update PWMs */
+		regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
+		/* enable chip from shut down */
+		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
+	} else {
+		dev_dbg(&is31->client->dev, "power down\n");
+		/* shut down (no need to clear CTRL1/2) */
+		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
+	}
+
+	return ret;
+}
+
+static struct led_info *
+is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
+{
+	struct device_node *np = client->dev.of_node, *child;
+	struct led_info *is31_leds;
+	int count;
+
+	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	count = of_get_child_count(np);
+	dev_dbg(&client->dev, "child count %d\n", count);
+	if (!count || count > NUM_LEDS)
+		return ERR_PTR(-ENODEV);
+
+	is31_leds = devm_kzalloc(&client->dev,
+			sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
+	if (!is31_leds)
+		return ERR_PTR(-ENOMEM);
+
+	for_each_child_of_node(np, child) {
+		struct led_info led;
+		u32 reg;
+		int ret;
+
+		led.name = NULL;
+		ret = of_property_read_string(child, "label", &led.name);
+		if (ret < 0 && ret != -EINVAL)	/* is optional */
+			return ERR_PTR(ret);
+		led.default_trigger = NULL;
+		ret = of_property_read_string(child, "linux,default-trigger",
+			&led.default_trigger);
+		if (ret < 0 && ret != -EINVAL)	/* is optional */
+			return ERR_PTR(ret);
+		led.flags = 0;
+		ret = of_property_read_u32(child, "reg", &reg);
+		dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
+		reg -= 1;	/* index 0 is at reg = 1 */
+		if (ret != 0 || reg < 0 || reg >= num_leds)
+			continue;
+
+		if (is31_leds[reg].name)
+			dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
+				reg, is31_leds[reg].name, led.name);
+		is31_leds[reg] = led;
+	}
+
+	return is31_leds;
+}
+
+static const struct of_device_id of_is31fl319x_leds_match[] = {
+	{ .compatible = "issi,is31fl3190", (void *) 1 },
+	{ .compatible = "issi,is31fl3191", (void *) 1 },
+	{ .compatible = "issi,is31fl3193", (void *) 3 },
+	{ .compatible = "issi,is31fl3196", (void *) 6 },
+	{ .compatible = "issi,is31fl3199", (void *) 9 },
+	{ .compatible = "si-en,sn3199", (void *) 9 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
+
+static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
+{ /* we have no readable registers */
+	return false;
+}
+
+static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
+{ /* volatile registers are not cached */
+	switch (reg) {
+	case IS31FL319X_DATA_UPDATE:
+	case IS31FL319X_TIME_UPDATE:
+	case IS31FL319X_RESET:
+		return true;	/* always write-through */
+	default:
+		return false;
+	}
+}
+
+struct regmap_config regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = IS31FL319X_REG_CNT,
+	.cache_type = REGCACHE_FLAT,
+	.readable_reg = is31fl319x_readable_reg,
+	.volatile_reg = is31fl319x_volatile_reg,
+};
+
+static int is31fl319x_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct is31fl319x_chip *is31;
+	struct i2c_adapter *adapter;
+	struct led_info *leds;
+	int err;
+	int i = 0;
+
+	adapter = to_i2c_adapter(client->dev.parent);
+
+	dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
+		(int) id->driver_data);
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
+		return -EIO;
+
+	leds = is31fl319x_parse_dt(client, (int) id->driver_data);
+	if (IS_ERR(leds)) {
+		dev_err(&client->dev, "DT parsing error %d\n",
+			(int) PTR_ERR(leds));
+		return PTR_ERR(leds);
+	}
+
+	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
+	if (!is31)
+		return -ENOMEM;
+
+	is31->client = client;
+	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (IS_ERR(is31->regmap)) {
+		dev_err(&client->dev, "failed to allocate register map\n");
+		return PTR_ERR(is31->regmap);
+	}
+
+	i2c_set_clientdata(client, is31);
+
+	/* check for write-reply from chip (we can't read any registers) */
+	err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
+	if (err < 0) {
+		dev_err(&client->dev, "no response from chip write: err = %d\n",
+			err);
+		return -EIO;	/* does not answer */
+	}
+
+	/* initialize chip and regmap so that we never try to read from i2c */
+	regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
+	regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
+	for (i = 0; i < NUM_LEDS; i++)
+		regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
+
+	if (client->dev.of_node) {
+		u32 val;
+		u8 config2 = 0;
+
+		if (of_property_read_u32(client->dev.of_node,
+					 "led-max-microamp", &val)) {
+			if (val > 40000)
+				val = 40000;
+			if (val < 5000)
+				val = 5000;
+			config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
+		}
+		if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
+					 &val)) {
+			if (val > 21)
+				val = 21;
+			config2 |= val / 3; /* AGS */
+		}
+		regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
+	}
+
+	for (i = 0; i < NUM_LEDS; i++) {
+		struct is31fl319x_led *l = is31->leds + i;
+
+		l->chip = is31;
+		if (leds[i].name && !leds[i].flags) {
+			l->led_cdev.name = leds[i].name;
+			l->led_cdev.default_trigger
+				= leds[i].default_trigger;
+			l->led_cdev.brightness_set_blocking
+				= is31fl319x_brightness_set;
+			/* NOTE: is31fl319x_brightness_set will be called
+			 * immediately after register() before we return
+			 */
+			err = devm_led_classdev_register(&client->dev,
+						    &l->led_cdev);
+			if (err < 0)
+				return err;
+		}
+	}
+
+	dev_dbg(&client->dev, "probed\n");
+	return 0;
+}
+
+static struct i2c_driver is31fl319x_driver = {
+	.driver   = {
+		.name    = "leds-is31fl319x",
+		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),
+	},
+	.probe    = is31fl319x_probe,
+	.id_table = is31fl319x_id,
+};
+
+module_i2c_driver(is31fl319x_driver);
+
+MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
+MODULE_DESCRIPTION("IS31FL319X LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.3

[PATCH v2 2/2] Bindings documentation for ISSI is31fl319x driver

[H. Nikolaus Schaller]

Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
---
 .../devicetree/bindings/leds/is31fl319x.txt        | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt

diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt b/Documentation/devicetree/bindings/leds/is31fl319x.txt
new file mode 100644
index 0000000..065e78c
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
@@ -0,0 +1,52 @@
+LEDs connected to is31fl319x LED controller chip
+
+Required properties:
+- compatible : Should be any of
+	"issi,is31fl3190"
+	"issi,is31fl3191"
+	"issi,is31fl3193"
+	"issi,is31fl3196"
+	"issi,is31fl3199"
+	"si-en,sn3199".
+- #address-cells: Must be 1.
+- #size-cells: Must be 0.
+- reg: 0x64, 0x65, 0x66, 0x67.
+
+Optional properties:
+- led-max-microamp : See Documentation/devicetree/bindings/leds/common.txt.
+	Valid values: 5000 - 40000, step by 5000 (rounded down)
+	Default: 20000 (20 mA)
+- audio-gain-db : audio gain selection for external analog modulation input.
+	Valid values: 0 - 21, step by 3 (rounded down)
+	Default: 0
+
+Each led is represented as a sub-node of the issi,is31fl319x device.
+There can be less leds subnodes than the chip can support but not more.
+
+LED sub-node properties:
+- label : (optional) see Documentation/devicetree/bindings/leds/common.txt.
+- reg : number of LED line
+	Valid values: 1 - number of leds supported by the chip variant.
+- linux,default-trigger : (optional)
+	see Documentation/devicetree/bindings/leds/common.txt.
+
+Examples:
+
+fancy_leds: leds@65 {
+	compatible = "issi,is31fl3196";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x65>;
+
+	red_aux: led@1 {
+		label = "red:aux";
+		reg = <1>;
+	};
+
+	green_power: led@5 {
+		label = "green:power";
+		reg = <5>;
+		linux,default-trigger = "default-on";
+	};
+};
+
-- 
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] Bindings documentation for ISSI is31fl319x driver

[H. Nikolaus Schaller]

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../devicetree/bindings/leds/is31fl319x.txt        | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt

diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt b/Documentation/devicetree/bindings/leds/is31fl319x.txt
new file mode 100644
index 0000000..065e78c
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
@@ -0,0 +1,52 @@
+LEDs connected to is31fl319x LED controller chip
+
+Required properties:
+- compatible : Should be any of
+	"issi,is31fl3190"
+	"issi,is31fl3191"
+	"issi,is31fl3193"
+	"issi,is31fl3196"
+	"issi,is31fl3199"
+	"si-en,sn3199".
+- #address-cells: Must be 1.
+- #size-cells: Must be 0.
+- reg: 0x64, 0x65, 0x66, 0x67.
+
+Optional properties:
+- led-max-microamp : See Documentation/devicetree/bindings/leds/common.txt.
+	Valid values: 5000 - 40000, step by 5000 (rounded down)
+	Default: 20000 (20 mA)
+- audio-gain-db : audio gain selection for external analog modulation input.
+	Valid values: 0 - 21, step by 3 (rounded down)
+	Default: 0
+
+Each led is represented as a sub-node of the issi,is31fl319x device.
+There can be less leds subnodes than the chip can support but not more.
+
+LED sub-node properties:
+- label : (optional) see Documentation/devicetree/bindings/leds/common.txt.
+- reg : number of LED line
+	Valid values: 1 - number of leds supported by the chip variant.
+- linux,default-trigger : (optional)
+	see Documentation/devicetree/bindings/leds/common.txt.
+
+Examples:
+
+fancy_leds: leds@65 {
+	compatible = "issi,is31fl3196";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x65>;
+
+	red_aux: led@1 {
+		label = "red:aux";
+		reg = <1>;
+	};
+
+	green_power: led@5 {
+		label = "green:power";
+		reg = <5>;
+		linux,default-trigger = "default-on";
+	};
+};
+
-- 
2.7.3

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[kbuild test robot]

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

Hi,

[auto build test WARNING on j.anaszewski-leds/for-next]
[also build test WARNING on v4.7-rc6 next-20160706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/H-Nikolaus-Schaller/driver-leds-is31fl319x-dimmable-LED-driver/20160706-180838
base:   https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git for-next
config: mn10300-allmodconfig (attached as .config)
compiler: am33_2.0-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mn10300 

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:289:0,
                    from include/linux/kernel.h:13,
                    from include/linux/list.h:8,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from include/linux/i2c.h:30,
                    from drivers/leds/leds-is31fl319x.c:18:
   drivers/leds/leds-is31fl319x.c: In function 'is31fl319x_brightness_set':
>> include/linux/dynamic_debug.h:64:16: warning: format '%d' expects argument of type 'int', but argument 5 has type 'long int' [-Wformat=]
     static struct _ddebug  __aligned(8)   \
                   ^
   include/linux/dynamic_debug.h:84:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA'
     DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
     ^
   include/linux/device.h:1197:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
     ^
   drivers/leds/leds-is31fl319x.c:104:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
     ^

vim +64 include/linux/dynamic_debug.h

b48420c1 Jim Cromie  2012-04-27  48  					const char *modname);
b48420c1 Jim Cromie  2012-04-27  49  
cbc46635 Joe Perches 2011-08-11  50  struct device;
cbc46635 Joe Perches 2011-08-11  51  
b9075fa9 Joe Perches 2011-10-31  52  extern __printf(3, 4)
906d2015 Joe Perches 2014-09-24  53  void __dynamic_dev_dbg(struct _ddebug *descriptor, const struct device *dev,
b9075fa9 Joe Perches 2011-10-31  54  		       const char *fmt, ...);
cbc46635 Joe Perches 2011-08-11  55  
ffa10cb4 Jason Baron 2011-08-11  56  struct net_device;
ffa10cb4 Jason Baron 2011-08-11  57  
b9075fa9 Joe Perches 2011-10-31  58  extern __printf(3, 4)
906d2015 Joe Perches 2014-09-24  59  void __dynamic_netdev_dbg(struct _ddebug *descriptor,
ffa10cb4 Jason Baron 2011-08-11  60  			  const struct net_device *dev,
b9075fa9 Joe Perches 2011-10-31  61  			  const char *fmt, ...);
ffa10cb4 Jason Baron 2011-08-11  62  
07613b0b Jason Baron 2011-10-04  63  #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
c0d2af63 Joe Perches 2012-10-18 @64  	static struct _ddebug  __aligned(8)			\
07613b0b Jason Baron 2011-10-04  65  	__attribute__((section("__verbose"))) name = {		\
07613b0b Jason Baron 2011-10-04  66  		.modname = KBUILD_MODNAME,			\
07613b0b Jason Baron 2011-10-04  67  		.function = __func__,				\
07613b0b Jason Baron 2011-10-04  68  		.filename = __FILE__,				\
07613b0b Jason Baron 2011-10-04  69  		.format = (fmt),				\
07613b0b Jason Baron 2011-10-04  70  		.lineno = __LINE__,				\
07613b0b Jason Baron 2011-10-04  71  		.flags =  _DPRINTK_FLAGS_DEFAULT,		\
07613b0b Jason Baron 2011-10-04  72  	}

:::::: The code at line 64 was first introduced by commit
:::::: c0d2af637863940b1a4fb208224ca7acb905c39f dynamic_debug: Remove unnecessary __used

:::::: TO: Joe Perches <joe@perches.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 39061 bytes --]

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[kbuild test robot]

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

Hi,

[auto build test WARNING on j.anaszewski-leds/for-next]
[also build test WARNING on v4.7-rc6 next-20160706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/H-Nikolaus-Schaller/driver-leds-is31fl319x-dimmable-LED-driver/20160706-180838
base:   https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git for-next
config: mn10300-allmodconfig (attached as .config)
compiler: am33_2.0-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mn10300 

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:289:0,
                    from include/linux/kernel.h:13,
                    from include/linux/list.h:8,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from include/linux/i2c.h:30,
                    from drivers/leds/leds-is31fl319x.c:18:
   drivers/leds/leds-is31fl319x.c: In function 'is31fl319x_brightness_set':
>> include/linux/dynamic_debug.h:64:16: warning: format '%d' expects argument of type 'int', but argument 5 has type 'long int' [-Wformat=]
     static struct _ddebug  __aligned(8)   \
                   ^
   include/linux/dynamic_debug.h:84:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA'
     DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
     ^
   include/linux/device.h:1197:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
     ^
   drivers/leds/leds-is31fl319x.c:104:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
     ^

vim +64 include/linux/dynamic_debug.h

b48420c1 Jim Cromie  2012-04-27  48  					const char *modname);
b48420c1 Jim Cromie  2012-04-27  49  
cbc46635 Joe Perches 2011-08-11  50  struct device;
cbc46635 Joe Perches 2011-08-11  51  
b9075fa9 Joe Perches 2011-10-31  52  extern __printf(3, 4)
906d2015 Joe Perches 2014-09-24  53  void __dynamic_dev_dbg(struct _ddebug *descriptor, const struct device *dev,
b9075fa9 Joe Perches 2011-10-31  54  		       const char *fmt, ...);
cbc46635 Joe Perches 2011-08-11  55  
ffa10cb4 Jason Baron 2011-08-11  56  struct net_device;
ffa10cb4 Jason Baron 2011-08-11  57  
b9075fa9 Joe Perches 2011-10-31  58  extern __printf(3, 4)
906d2015 Joe Perches 2014-09-24  59  void __dynamic_netdev_dbg(struct _ddebug *descriptor,
ffa10cb4 Jason Baron 2011-08-11  60  			  const struct net_device *dev,
b9075fa9 Joe Perches 2011-10-31  61  			  const char *fmt, ...);
ffa10cb4 Jason Baron 2011-08-11  62  
07613b0b Jason Baron 2011-10-04  63  #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
c0d2af63 Joe Perches 2012-10-18 @64  	static struct _ddebug  __aligned(8)			\
07613b0b Jason Baron 2011-10-04  65  	__attribute__((section("__verbose"))) name = {		\
07613b0b Jason Baron 2011-10-04  66  		.modname = KBUILD_MODNAME,			\
07613b0b Jason Baron 2011-10-04  67  		.function = __func__,				\
07613b0b Jason Baron 2011-10-04  68  		.filename = __FILE__,				\
07613b0b Jason Baron 2011-10-04  69  		.format = (fmt),				\
07613b0b Jason Baron 2011-10-04  70  		.lineno = __LINE__,				\
07613b0b Jason Baron 2011-10-04  71  		.flags =  _DPRINTK_FLAGS_DEFAULT,		\
07613b0b Jason Baron 2011-10-04  72  	}

:::::: The code at line 64 was first introduced by commit
:::::: c0d2af637863940b1a4fb208224ca7acb905c39f dynamic_debug: Remove unnecessary __used

:::::: TO: Joe Perches <joe@perches.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 39061 bytes --]

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

Thanks for the updated version. Some comments below.

On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
> This is a driver for the Integrated Silicon Solution Inc. LED driver
> chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
> LEDs.
>
> Each LED is individually controllable in brightness (through pwm)
> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>
> The maximum current of the LEDs can be programmed and limited to
> 5 .. 40mA through a device tree property.
>
> The chip is connected through I2C and can have one of 4 addresses
> in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
> address is defined by the reg property as usual.
>
> The chip also has a shutdown input which could be connected to a GPIO,
> but this driver uses software shutdown if all LEDs are inactivated.
>
> The chip also has breathing and audio features which are not fully
> supported by this driver.
>
> Tested-on: OMAP5 based Pyra handheld prototype.
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>   drivers/leds/Kconfig           |   8 +
>   drivers/leds/Makefile          |   1 +
>   drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 363 insertions(+)
>   create mode 100644 drivers/leds/leds-is31fl319x.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 5ae2834..65b1045 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -570,6 +570,14 @@ config LEDS_SEAD3
>   	  This driver can also be built as a module. If so the module
>   	  will be called leds-sead3.
>
> +config LEDS_IS31FL319X
> +	tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
> +	depends on LEDS_CLASS && I2C && OF
> +	help
> +	  This option enables support for LEDs connected to ISSI IS31FL319x
> +	  fancy LED driver chips accessed via the I2C bus.
> +	  Driver supports individual PWM brightness control for each channel.
> +
>   config LEDS_IS31FL32XX
>   	tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
>   	depends on LEDS_CLASS && I2C && OF
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index cb2013d..b6ce9f9 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>   obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
>   obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>   obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
> +obj-$(CONFIG_LEDS_IS31FL319X)		+= leds-is31fl319x.o
>   obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
>
>   # LED SPI Drivers
> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
> new file mode 100644
> index 0000000..bbf8850
> --- /dev/null
> +++ b/drivers/leds/leds-is31fl319x.c
> @@ -0,0 +1,354 @@
> +/*
> + * Copyright 2015-16 Golden Delicious Computers
> + *
> + * Author: Nikolaus Schaller <hns@goldelico.com>
> + *
> + * Based on leds-tca6507.c
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License.  See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
> + * effect LEDs.
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* register numbers */
> +#define IS31FL319X_SHUTDOWN	0x00
> +#define IS31FL319X_CTRL1	0x01
> +#define IS31FL319X_CTRL2	0x02
> +#define IS31FL319X_CONFIG1	0x03
> +#define IS31FL319X_CONFIG2	0x04
> +#define IS31FL319X_RAMP_MODE	0x05
> +#define IS31FL319X_BREATH_MASK	0x06
> +#define IS31FL319X_PWM1		0x07
> +#define IS31FL319X_PWM2		0x08
> +#define IS31FL319X_PWM3		0x09
> +#define IS31FL319X_PWM4		0x0a
> +#define IS31FL319X_PWM5		0x0b
> +#define IS31FL319X_PWM6		0x0c
> +#define IS31FL319X_PWM7		0x0d
> +#define IS31FL319X_PWM8		0x0e
> +#define IS31FL319X_PWM9		0x0f
> +#define IS31FL319X_DATA_UPDATE	0x10
> +#define IS31FL319X_T0_1		0x11
> +#define IS31FL319X_T0_2		0x12
> +#define IS31FL319X_T0_3		0x13
> +#define IS31FL319X_T0_4		0x14
> +#define IS31FL319X_T0_5		0x15
> +#define IS31FL319X_T0_6		0x16
> +#define IS31FL319X_T0_7		0x17
> +#define IS31FL319X_T0_8		0x18
> +#define IS31FL319X_T0_9		0x19
> +#define IS31FL319X_T123_1	0x1a
> +#define IS31FL319X_T123_2	0x1b
> +#define IS31FL319X_T123_3	0x1c
> +#define IS31FL319X_T4_1		0x1d
> +#define IS31FL319X_T4_2		0x1e
> +#define IS31FL319X_T4_3		0x1f
> +#define IS31FL319X_T4_4		0x20
> +#define IS31FL319X_T4_5		0x21
> +#define IS31FL319X_T4_6		0x22
> +#define IS31FL319X_T4_7		0x23
> +#define IS31FL319X_T4_8		0x24
> +#define IS31FL319X_T4_9		0x25
> +#define IS31FL319X_TIME_UPDATE	0x26
> +#define IS31FL319X_RESET	0xff
> +
> +#define IS31FL319X_REG_CNT	(IS31FL319X_RESET + 1)
> +
> +#define NUM_LEDS 9	/* max for 3199 chip */
> +
> +struct is31fl319x_chip {
> +	struct i2c_client	*client;
> +	struct regmap		*regmap;
> +
> +	struct is31fl319x_led {
> +		struct is31fl319x_chip	*chip;
> +		struct led_classdev	led_cdev;
> +	} leds[NUM_LEDS];
> +};
> +
> +static const struct i2c_device_id is31fl319x_id[] = {
> +	{ "is31fl3190", 1 },
> +	{ "is31fl3191", 1 },
> +	{ "is31fl3193", 3 },
> +	{ "is31fl3196", 6 },
> +	{ "is31fl3199", 9 },
> +	{ "sn3199", 9 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
> +
> +static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
> +				   enum led_brightness brightness)
> +{
> +	struct is31fl319x_led *led = container_of(led_cdev,
> +						  struct is31fl319x_led,
> +						  led_cdev);
> +	struct is31fl319x_chip *is31 = led->chip;
> +	int ret;
> +
> +	int i;
> +	u8 ctrl1, ctrl2;

You can initialize these variables here.

> +
> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
> +		brightness);
> +
> +	/* update PWM register */
> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
> +			   brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	ctrl1 = 0;
> +	ctrl2 = 0;

And remove above two lines.

> +
> +	/* read current brightness of all PWM channels */
> +	for (i = 0; i < NUM_LEDS; i++) {
> +		unsigned int pwm_value;
> +		bool on;
> +
> +		/*
> +		 * since neither led_cdev nor the chip can provide
> +		 * the current setting, we read from the regmap cache
> +		 */
> +
> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
> +				  &pwm_value);
 > +
> +		dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
> +			__func__, i, ret, pwm_value);
> +		on = ret >= 0 && pwm_value > LED_OFF;
> +
> +		if (i < 3)
> +			ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
> +		else if (i < 6)
> +			ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
> +		else
> +			ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
> +	}
 > +
> +
> +	if (ctrl1 > 0 || ctrl2 > 0) {
> +		dev_dbg(&is31->client->dev, "power up %02x %02x\n",
> +			ctrl1, ctrl2);
> +		regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
> +		regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
> +		/* update PWMs */
> +		regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
> +		/* enable chip from shut down */
> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
> +	} else {
> +		dev_dbg(&is31->client->dev, "power down\n");
> +		/* shut down (no need to clear CTRL1/2) */
> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
> +	}

You need a mutex protection in this function, so as to prevent other
processes from jumping in in the middle of setting up a brightness,
which requires writing several registers.

> +	return ret;
> +}
> +
> +static struct led_info *
> +is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
> +{
> +	struct device_node *np = client->dev.of_node, *child;
> +	struct led_info *is31_leds;
> +	int count;
> +
> +	if (!np)
> +		return ERR_PTR(-ENODEV);
> +
> +	count = of_get_child_count(np);
> +	dev_dbg(&client->dev, "child count %d\n", count);
> +	if (!count || count > NUM_LEDS)
> +		return ERR_PTR(-ENODEV);
> +
> +	is31_leds = devm_kzalloc(&client->dev,
> +			sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
> +	if (!is31_leds)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for_each_child_of_node(np, child) {
> +		struct led_info led;

Using this local variable adds too much noise here.
I suggest avoiding the use of struct led_info. It is semantically
inconsistent in the context of DT parsing, since you don't have
any flags to parse.

> +		u32 reg;
> +		int ret;
> +
> +		led.name = NULL;
> +		ret = of_property_read_string(child, "label", &led.name);
> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
> +			return ERR_PTR(ret);

You could assign the name directly to related struct led_classdev
property. Moreover, if label is not present the child node name
should be used for LED class device name. Please refer to the other
drivers and common LED bindings.

> +		led.default_trigger = NULL;
> +		ret = of_property_read_string(child, "linux,default-trigger",
> +			&led.default_trigger);

Here also please assign the trigger directly to struct led_classdev.

> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
> +			return ERR_PTR(ret);
> +		led.flags = 0;
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
> +		reg -= 1;	/* index 0 is at reg = 1 */
> +		if (ret != 0 || reg < 0 || reg >= num_leds)
> +			continue;
> +
> +		if (is31_leds[reg].name)
> +			dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
> +				reg, is31_leds[reg].name, led.name);
> +		is31_leds[reg] = led;
> +	}
> +
> +	return is31_leds;
> +}
> +
> +static const struct of_device_id of_is31fl319x_leds_match[] = {
> +	{ .compatible = "issi,is31fl3190", (void *) 1 },
> +	{ .compatible = "issi,is31fl3191", (void *) 1 },
> +	{ .compatible = "issi,is31fl3193", (void *) 3 },
> +	{ .compatible = "issi,is31fl3196", (void *) 6 },
> +	{ .compatible = "issi,is31fl3199", (void *) 9 },
> +	{ .compatible = "si-en,sn3199", (void *) 9 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
> +
> +static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
> +{ /* we have no readable registers */
> +	return false;
> +}
> +
> +static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
> +{ /* volatile registers are not cached */
> +	switch (reg) {
> +	case IS31FL319X_DATA_UPDATE:
> +	case IS31FL319X_TIME_UPDATE:
> +	case IS31FL319X_RESET:
> +		return true;	/* always write-through */
> +	default:
> +		return false;
> +	}
> +}
> +
> +struct regmap_config regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = IS31FL319X_REG_CNT,
> +	.cache_type = REGCACHE_FLAT,
> +	.readable_reg = is31fl319x_readable_reg,
> +	.volatile_reg = is31fl319x_volatile_reg,
> +};
> +
> +static int is31fl319x_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	struct is31fl319x_chip *is31;
> +	struct i2c_adapter *adapter;
> +	struct led_info *leds;
> +	int err;
> +	int i = 0;
> +
> +	adapter = to_i2c_adapter(client->dev.parent);
> +
> +	dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
> +		(int) id->driver_data);
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
> +		return -EIO;
> +
> +	leds = is31fl319x_parse_dt(client, (int) id->driver_data);
> +	if (IS_ERR(leds)) {
> +		dev_err(&client->dev, "DT parsing error %d\n",
> +			(int) PTR_ERR(leds));
> +		return PTR_ERR(leds);
> +	}
> +
> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
> +	if (!is31)
> +		return -ENOMEM;
> +
> +	is31->client = client;
> +	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(is31->regmap)) {
> +		dev_err(&client->dev, "failed to allocate register map\n");
> +		return PTR_ERR(is31->regmap);
> +	}
> +
> +	i2c_set_clientdata(client, is31);
> +
> +	/* check for write-reply from chip (we can't read any registers) */
> +	err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);

Doesn't powering the device up result in loading registers with default
values?

> +	if (err < 0) {
> +		dev_err(&client->dev, "no response from chip write: err = %d\n",
> +			err);
> +		return -EIO;	/* does not answer */
> +	}
> +
> +	/* initialize chip and regmap so that we never try to read from i2c */

You can initialize default register values without writing them,
by using regmap's reg_defaults property.

> +	regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
> +	regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
> +	for (i = 0; i < NUM_LEDS; i++)
> +		regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
> +
> +	if (client->dev.of_node) {
> +		u32 val;
> +		u8 config2 = 0;
> +
> +		if (of_property_read_u32(client->dev.of_node,
> +					 "led-max-microamp", &val)) {

of_property_read_u32 returns 0 on success.

led-max-microamp needs to be defined per each sub-LED (child node).
The greatest value should be used for setting CS bit field of CTRL2
register, and max_brightness values of the LEDs with lower max microamp
value should be limited accordingly.

Moreover, this is still DT parsing and should find its place in
is31fl319x_parse_dt().

> +			if (val > 40000)
> +				val = 40000;
> +			if (val < 5000)
> +				val = 5000;
> +			config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
> +		}
> +		if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
> +					 &val)) {

Please move this to is31fl319x_parse_dt().

> +			if (val > 21)
> +				val = 21;
> +			config2 |= val / 3; /* AGS */
> +		}
> +		regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
> +	}
> +
> +	for (i = 0; i < NUM_LEDS; i++) {
> +		struct is31fl319x_led *l = is31->leds + i;
> +
> +		l->chip = is31;
> +		if (leds[i].name && !leds[i].flags) {
> +			l->led_cdev.name = leds[i].name;
> +			l->led_cdev.default_trigger
> +				= leds[i].default_trigger;
> +			l->led_cdev.brightness_set_blocking
> +				= is31fl319x_brightness_set;
> +			/* NOTE: is31fl319x_brightness_set will be called
> +			 * immediately after register() before we return
> +			 */

Who will call it? I believe that this is true only if default-on
trigger is registered.

> +			err = devm_led_classdev_register(&client->dev,
> +						    &l->led_cdev);
> +			if (err < 0)
> +				return err;
> +		}
> +	}
> +
> +	dev_dbg(&client->dev, "probed\n");

Please remove this debug log. You can verify it by checking presence of
the device in the sysfs.

> +	return 0;
> +}
> +
> +static struct i2c_driver is31fl319x_driver = {
> +	.driver   = {
> +		.name    = "leds-is31fl319x",
> +		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),
> +	},
> +	.probe    = is31fl319x_probe,
> +	.id_table = is31fl319x_id,
> +};
> +
> +module_i2c_driver(is31fl319x_driver);
> +
> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
> +MODULE_DESCRIPTION("IS31FL319X LED driver");
> +MODULE_LICENSE("GPL v2");
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi Jacek,

> Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
> 
> Hi Nikolaus,
> 
> Thanks for the updated version. Some comments below.
> 
> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>> chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
>> LEDs.
>> 
>> Each LED is individually controllable in brightness (through pwm)
>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>> 
>> The maximum current of the LEDs can be programmed and limited to
>> 5 .. 40mA through a device tree property.
>> 
>> The chip is connected through I2C and can have one of 4 addresses
>> in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
>> address is defined by the reg property as usual.
>> 
>> The chip also has a shutdown input which could be connected to a GPIO,
>> but this driver uses software shutdown if all LEDs are inactivated.
>> 
>> The chip also has breathing and audio features which are not fully
>> supported by this driver.
>> 
>> Tested-on: OMAP5 based Pyra handheld prototype.
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  drivers/leds/Kconfig           |   8 +
>>  drivers/leds/Makefile          |   1 +
>>  drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 363 insertions(+)
>>  create mode 100644 drivers/leds/leds-is31fl319x.c
>> 
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 5ae2834..65b1045 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -570,6 +570,14 @@ config LEDS_SEAD3
>>  	  This driver can also be built as a module. If so the module
>>  	  will be called leds-sead3.
>> 
>> +config LEDS_IS31FL319X
>> +	tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
>> +	depends on LEDS_CLASS && I2C && OF
>> +	help
>> +	  This option enables support for LEDs connected to ISSI IS31FL319x
>> +	  fancy LED driver chips accessed via the I2C bus.
>> +	  Driver supports individual PWM brightness control for each channel.
>> +
>>  config LEDS_IS31FL32XX
>>  	tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
>>  	depends on LEDS_CLASS && I2C && OF
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index cb2013d..b6ce9f9 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>>  obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
>>  obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>>  obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
>> +obj-$(CONFIG_LEDS_IS31FL319X)		+= leds-is31fl319x.o
>>  obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
>> 
>>  # LED SPI Drivers
>> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
>> new file mode 100644
>> index 0000000..bbf8850
>> --- /dev/null
>> +++ b/drivers/leds/leds-is31fl319x.c
>> @@ -0,0 +1,354 @@
>> +/*
>> + * Copyright 2015-16 Golden Delicious Computers
>> + *
>> + * Author: Nikolaus Schaller <hns@goldelico.com>
>> + *
>> + * Based on leds-tca6507.c
>> + *
>> + * This file is subject to the terms and conditions of version 2 of
>> + * the GNU General Public License.  See the file COPYING in the main
>> + * directory of this archive for more details.
>> + *
>> + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
>> + * effect LEDs.
>> + *
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +/* register numbers */
>> +#define IS31FL319X_SHUTDOWN	0x00
>> +#define IS31FL319X_CTRL1	0x01
>> +#define IS31FL319X_CTRL2	0x02
>> +#define IS31FL319X_CONFIG1	0x03
>> +#define IS31FL319X_CONFIG2	0x04
>> +#define IS31FL319X_RAMP_MODE	0x05
>> +#define IS31FL319X_BREATH_MASK	0x06
>> +#define IS31FL319X_PWM1		0x07
>> +#define IS31FL319X_PWM2		0x08
>> +#define IS31FL319X_PWM3		0x09
>> +#define IS31FL319X_PWM4		0x0a
>> +#define IS31FL319X_PWM5		0x0b
>> +#define IS31FL319X_PWM6		0x0c
>> +#define IS31FL319X_PWM7		0x0d
>> +#define IS31FL319X_PWM8		0x0e
>> +#define IS31FL319X_PWM9		0x0f
>> +#define IS31FL319X_DATA_UPDATE	0x10
>> +#define IS31FL319X_T0_1		0x11
>> +#define IS31FL319X_T0_2		0x12
>> +#define IS31FL319X_T0_3		0x13
>> +#define IS31FL319X_T0_4		0x14
>> +#define IS31FL319X_T0_5		0x15
>> +#define IS31FL319X_T0_6		0x16
>> +#define IS31FL319X_T0_7		0x17
>> +#define IS31FL319X_T0_8		0x18
>> +#define IS31FL319X_T0_9		0x19
>> +#define IS31FL319X_T123_1	0x1a
>> +#define IS31FL319X_T123_2	0x1b
>> +#define IS31FL319X_T123_3	0x1c
>> +#define IS31FL319X_T4_1		0x1d
>> +#define IS31FL319X_T4_2		0x1e
>> +#define IS31FL319X_T4_3		0x1f
>> +#define IS31FL319X_T4_4		0x20
>> +#define IS31FL319X_T4_5		0x21
>> +#define IS31FL319X_T4_6		0x22
>> +#define IS31FL319X_T4_7		0x23
>> +#define IS31FL319X_T4_8		0x24
>> +#define IS31FL319X_T4_9		0x25
>> +#define IS31FL319X_TIME_UPDATE	0x26
>> +#define IS31FL319X_RESET	0xff
>> +
>> +#define IS31FL319X_REG_CNT	(IS31FL319X_RESET + 1)
>> +
>> +#define NUM_LEDS 9	/* max for 3199 chip */
>> +
>> +struct is31fl319x_chip {
>> +	struct i2c_client	*client;
>> +	struct regmap		*regmap;
>> +
>> +	struct is31fl319x_led {
>> +		struct is31fl319x_chip	*chip;
>> +		struct led_classdev	led_cdev;
>> +	} leds[NUM_LEDS];
>> +};
>> +
>> +static const struct i2c_device_id is31fl319x_id[] = {
>> +	{ "is31fl3190", 1 },
>> +	{ "is31fl3191", 1 },
>> +	{ "is31fl3193", 3 },
>> +	{ "is31fl3196", 6 },
>> +	{ "is31fl3199", 9 },
>> +	{ "sn3199", 9 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
>> +
>> +static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
>> +				   enum led_brightness brightness)
>> +{
>> +	struct is31fl319x_led *led = container_of(led_cdev,
>> +						  struct is31fl319x_led,
>> +						  led_cdev);
>> +	struct is31fl319x_chip *is31 = led->chip;
>> +	int ret;
>> +
>> +	int i;
>> +	u8 ctrl1, ctrl2;
> 
> You can initialize these variables here.

Ok.

> 
>> +
>> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
>> +		brightness);
>> +
>> +	/* update PWM register */
>> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
>> +			   brightness);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ctrl1 = 0;
>> +	ctrl2 = 0;
> 
> And remove above two lines.
> 
>> +
>> +	/* read current brightness of all PWM channels */
>> +	for (i = 0; i < NUM_LEDS; i++) {
>> +		unsigned int pwm_value;
>> +		bool on;
>> +
>> +		/*
>> +		 * since neither led_cdev nor the chip can provide
>> +		 * the current setting, we read from the regmap cache
>> +		 */
>> +
>> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
>> +				  &pwm_value);
> > +
>> +		dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
>> +			__func__, i, ret, pwm_value);
>> +		on = ret >= 0 && pwm_value > LED_OFF;
>> +
>> +		if (i < 3)
>> +			ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
>> +		else if (i < 6)
>> +			ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
>> +		else
>> +			ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
>> +	}
> > +
>> +
>> +	if (ctrl1 > 0 || ctrl2 > 0) {
>> +		dev_dbg(&is31->client->dev, "power up %02x %02x\n",
>> +			ctrl1, ctrl2);
>> +		regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
>> +		regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
>> +		/* update PWMs */
>> +		regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
>> +		/* enable chip from shut down */
>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
>> +	} else {
>> +		dev_dbg(&is31->client->dev, "power down\n");
>> +		/* shut down (no need to clear CTRL1/2) */
>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
>> +	}
> 
> You need a mutex protection in this function, so as to prevent other
> processes from jumping in in the middle of setting up a brightness,
> which requires writing several registers.

Uh?
I thought that registering is31fl319x_brightness_set as 
led_cdev.brightness_set_blocking
does the trick that we don't need a private mutex here because it
can be called only once anyways. At least that was suggested during v1 review.

> 
>> +	return ret;
>> +}
>> +
>> +static struct led_info *
>> +is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
>> +{
>> +	struct device_node *np = client->dev.of_node, *child;
>> +	struct led_info *is31_leds;
>> +	int count;
>> +
>> +	if (!np)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	count = of_get_child_count(np);
>> +	dev_dbg(&client->dev, "child count %d\n", count);
>> +	if (!count || count > NUM_LEDS)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	is31_leds = devm_kzalloc(&client->dev,
>> +			sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
>> +	if (!is31_leds)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for_each_child_of_node(np, child) {
>> +		struct led_info led;
> 
> Using this local variable adds too much noise here.
> I suggest avoiding the use of struct led_info. It is semantically
> inconsistent in the context of DT parsing, since you don't have
> any flags to parse.
> 
>> +		u32 reg;
>> +		int ret;
>> +
>> +		led.name = NULL;
>> +		ret = of_property_read_string(child, "label", &led.name);
>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>> +			return ERR_PTR(ret);
> 
> You could assign the name directly to related struct led_classdev
> property. Moreover, if label is not present the child node name
> should be used for LED class device name. Please refer to the other
> drivers and common LED bindings.

Ok.

> 
>> +		led.default_trigger = NULL;
>> +		ret = of_property_read_string(child, "linux,default-trigger",
>> +			&led.default_trigger);
> 
> Here also please assign the trigger directly to struct led_classdev.

Ok.

> 
>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>> +			return ERR_PTR(ret);
>> +		led.flags = 0;
>> +		ret = of_property_read_u32(child, "reg", &reg);
>> +		dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
>> +		reg -= 1;	/* index 0 is at reg = 1 */
>> +		if (ret != 0 || reg < 0 || reg >= num_leds)
>> +			continue;
>> +
>> +		if (is31_leds[reg].name)
>> +			dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
>> +				reg, is31_leds[reg].name, led.name);
>> +		is31_leds[reg] = led;
>> +	}
>> +
>> +	return is31_leds;
>> +}
>> +
>> +static const struct of_device_id of_is31fl319x_leds_match[] = {
>> +	{ .compatible = "issi,is31fl3190", (void *) 1 },
>> +	{ .compatible = "issi,is31fl3191", (void *) 1 },
>> +	{ .compatible = "issi,is31fl3193", (void *) 3 },
>> +	{ .compatible = "issi,is31fl3196", (void *) 6 },
>> +	{ .compatible = "issi,is31fl3199", (void *) 9 },
>> +	{ .compatible = "si-en,sn3199", (void *) 9 },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
>> +
>> +static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
>> +{ /* we have no readable registers */
>> +	return false;
>> +}
>> +
>> +static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
>> +{ /* volatile registers are not cached */
>> +	switch (reg) {
>> +	case IS31FL319X_DATA_UPDATE:
>> +	case IS31FL319X_TIME_UPDATE:
>> +	case IS31FL319X_RESET:
>> +		return true;	/* always write-through */
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +struct regmap_config regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.max_register = IS31FL319X_REG_CNT,
>> +	.cache_type = REGCACHE_FLAT,
>> +	.readable_reg = is31fl319x_readable_reg,
>> +	.volatile_reg = is31fl319x_volatile_reg,
>> +};
>> +
>> +static int is31fl319x_probe(struct i2c_client *client,
>> +		const struct i2c_device_id *id)
>> +{
>> +	struct is31fl319x_chip *is31;
>> +	struct i2c_adapter *adapter;
>> +	struct led_info *leds;
>> +	int err;
>> +	int i = 0;
>> +
>> +	adapter = to_i2c_adapter(client->dev.parent);
>> +
>> +	dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
>> +		(int) id->driver_data);
>> +
>> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>> +		return -EIO;
>> +
>> +	leds = is31fl319x_parse_dt(client, (int) id->driver_data);
>> +	if (IS_ERR(leds)) {
>> +		dev_err(&client->dev, "DT parsing error %d\n",
>> +			(int) PTR_ERR(leds));
>> +		return PTR_ERR(leds);
>> +	}
>> +
>> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
>> +	if (!is31)
>> +		return -ENOMEM;
>> +
>> +	is31->client = client;
>> +	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
>> +	if (IS_ERR(is31->regmap)) {
>> +		dev_err(&client->dev, "failed to allocate register map\n");
>> +		return PTR_ERR(is31->regmap);
>> +	}
>> +
>> +	i2c_set_clientdata(client, is31);
>> +
>> +	/* check for write-reply from chip (we can't read any registers) */
>> +	err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
> 
> Doesn't powering the device up result in loading registers with default
> values?

Yes, but not a reboot.

And we have to write *some* register anyways here to be able to detect
if the chip is present or does not respond.

So what would writing a different register change?

> 
>> +	if (err < 0) {
>> +		dev_err(&client->dev, "no response from chip write: err = %d\n",
>> +			err);
>> +		return -EIO;	/* does not answer */
>> +	}
>> +
>> +	/* initialize chip and regmap so that we never try to read from i2c */
> 
> You can initialize default register values without writing them,
> by using regmap's reg_defaults property.

Hm,

> 
>> +	regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
>> +	regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
>> +	for (i = 0; i < NUM_LEDS; i++)
>> +		regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);

would replace a small loop with a bigger table. But if you prefer this I can change.

>> +
>> +	if (client->dev.of_node) {
>> +		u32 val;
>> +		u8 config2 = 0;
>> +
>> +		if (of_property_read_u32(client->dev.of_node,
>> +					 "led-max-microamp", &val)) {
> 
> of_property_read_u32 returns 0 on success.

Ok.

> 
> led-max-microamp needs to be defined per each sub-LED (child node).
> The greatest value should be used for setting CS bit field of CTRL2
> register, and max_brightness values of the LEDs with lower max microamp
> value should be limited accordingly.

Hm. The chip does not allow to limit individual LEDs. So we should take the
*minimum* of all subnodes.

> 
> Moreover, this is still DT parsing and should find its place in
> is31fl319x_parse_dt().
> 
>> +			if (val > 40000)
>> +				val = 40000;
>> +			if (val < 5000)
>> +				val = 5000;
>> +			config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
>> +		}
>> +		if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
>> +					 &val)) {
> 
> Please move this to is31fl319x_parse_dt().

Well, the main reason is that this needs to introduce another temporary struct
to pass values from parse_dt to probe and copy values there.

Would it be acceptable to inline all DT parsing?

> 
>> +			if (val > 21)
>> +				val = 21;
>> +			config2 |= val / 3; /* AGS */
>> +		}
>> +		regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
>> +	}
>> +
>> +	for (i = 0; i < NUM_LEDS; i++) {
>> +		struct is31fl319x_led *l = is31->leds + i;
>> +
>> +		l->chip = is31;
>> +		if (leds[i].name && !leds[i].flags) {
>> +			l->led_cdev.name = leds[i].name;
>> +			l->led_cdev.default_trigger
>> +				= leds[i].default_trigger;
>> +			l->led_cdev.brightness_set_blocking
>> +				= is31fl319x_brightness_set;
>> +			/* NOTE: is31fl319x_brightness_set will be called
>> +			 * immediately after register() before we return
>> +			 */
> 
> Who will call it? I believe that this is true only if default-on
> trigger is registered.

Yes, as soon as anyone can call it we must be aware.

> 
>> +			err = devm_led_classdev_register(&client->dev,
>> +						    &l->led_cdev);
>> +			if (err < 0)
>> +				return err;
>> +		}
>> +	}
>> +
>> +	dev_dbg(&client->dev, "probed\n");
> 
> Please remove this debug log. You can verify it by checking presence of
> the device in the sysfs.

Ok.

> 
>> +	return 0;
>> +}
>> +
>> +static struct i2c_driver is31fl319x_driver = {
>> +	.driver   = {
>> +		.name    = "leds-is31fl319x",
>> +		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),
>> +	},
>> +	.probe    = is31fl319x_probe,
>> +	.id_table = is31fl319x_id,
>> +};
>> +
>> +module_i2c_driver(is31fl319x_driver);
>> +
>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>> +MODULE_DESCRIPTION("IS31FL319X LED driver");
>> +MODULE_LICENSE("GPL v2");
>> 
> 
> 
> -- 
> Best regards,
> Jacek Anaszewski

Will fix asap.

Thanks and BR,
Nikolaus Schaller

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
> Hi Jacek,
>
>> Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>
>> Hi Nikolaus,
>>
>> Thanks for the updated version. Some comments below.
>>
>> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>>> chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
>>> LEDs.
>>>
>>> Each LED is individually controllable in brightness (through pwm)
>>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>>>
>>> The maximum current of the LEDs can be programmed and limited to
>>> 5 .. 40mA through a device tree property.
>>>
>>> The chip is connected through I2C and can have one of 4 addresses
>>> in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
>>> address is defined by the reg property as usual.
>>>
>>> The chip also has a shutdown input which could be connected to a GPIO,
>>> but this driver uses software shutdown if all LEDs are inactivated.
>>>
>>> The chip also has breathing and audio features which are not fully
>>> supported by this driver.
>>>
>>> Tested-on: OMAP5 based Pyra handheld prototype.
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>>   drivers/leds/Kconfig           |   8 +
>>>   drivers/leds/Makefile          |   1 +
>>>   drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 363 insertions(+)
>>>   create mode 100644 drivers/leds/leds-is31fl319x.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 5ae2834..65b1045 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -570,6 +570,14 @@ config LEDS_SEAD3
>>>   	  This driver can also be built as a module. If so the module
>>>   	  will be called leds-sead3.
>>>
>>> +config LEDS_IS31FL319X
>>> +	tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
>>> +	depends on LEDS_CLASS && I2C && OF
>>> +	help
>>> +	  This option enables support for LEDs connected to ISSI IS31FL319x
>>> +	  fancy LED driver chips accessed via the I2C bus.
>>> +	  Driver supports individual PWM brightness control for each channel.
>>> +
>>>   config LEDS_IS31FL32XX
>>>   	tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
>>>   	depends on LEDS_CLASS && I2C && OF
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index cb2013d..b6ce9f9 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>>>   obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
>>>   obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>>>   obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
>>> +obj-$(CONFIG_LEDS_IS31FL319X)		+= leds-is31fl319x.o
>>>   obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
>>>
>>>   # LED SPI Drivers
>>> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
>>> new file mode 100644
>>> index 0000000..bbf8850
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-is31fl319x.c
>>> @@ -0,0 +1,354 @@
>>> +/*
>>> + * Copyright 2015-16 Golden Delicious Computers
>>> + *
>>> + * Author: Nikolaus Schaller <hns@goldelico.com>
>>> + *
>>> + * Based on leds-tca6507.c
>>> + *
>>> + * This file is subject to the terms and conditions of version 2 of
>>> + * the GNU General Public License.  See the file COPYING in the main
>>> + * directory of this archive for more details.
>>> + *
>>> + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
>>> + * effect LEDs.
>>> + *
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +
>>> +/* register numbers */
>>> +#define IS31FL319X_SHUTDOWN	0x00
>>> +#define IS31FL319X_CTRL1	0x01
>>> +#define IS31FL319X_CTRL2	0x02
>>> +#define IS31FL319X_CONFIG1	0x03
>>> +#define IS31FL319X_CONFIG2	0x04
>>> +#define IS31FL319X_RAMP_MODE	0x05
>>> +#define IS31FL319X_BREATH_MASK	0x06
>>> +#define IS31FL319X_PWM1		0x07
>>> +#define IS31FL319X_PWM2		0x08
>>> +#define IS31FL319X_PWM3		0x09
>>> +#define IS31FL319X_PWM4		0x0a
>>> +#define IS31FL319X_PWM5		0x0b
>>> +#define IS31FL319X_PWM6		0x0c
>>> +#define IS31FL319X_PWM7		0x0d
>>> +#define IS31FL319X_PWM8		0x0e
>>> +#define IS31FL319X_PWM9		0x0f
>>> +#define IS31FL319X_DATA_UPDATE	0x10
>>> +#define IS31FL319X_T0_1		0x11
>>> +#define IS31FL319X_T0_2		0x12
>>> +#define IS31FL319X_T0_3		0x13
>>> +#define IS31FL319X_T0_4		0x14
>>> +#define IS31FL319X_T0_5		0x15
>>> +#define IS31FL319X_T0_6		0x16
>>> +#define IS31FL319X_T0_7		0x17
>>> +#define IS31FL319X_T0_8		0x18
>>> +#define IS31FL319X_T0_9		0x19
>>> +#define IS31FL319X_T123_1	0x1a
>>> +#define IS31FL319X_T123_2	0x1b
>>> +#define IS31FL319X_T123_3	0x1c
>>> +#define IS31FL319X_T4_1		0x1d
>>> +#define IS31FL319X_T4_2		0x1e
>>> +#define IS31FL319X_T4_3		0x1f
>>> +#define IS31FL319X_T4_4		0x20
>>> +#define IS31FL319X_T4_5		0x21
>>> +#define IS31FL319X_T4_6		0x22
>>> +#define IS31FL319X_T4_7		0x23
>>> +#define IS31FL319X_T4_8		0x24
>>> +#define IS31FL319X_T4_9		0x25
>>> +#define IS31FL319X_TIME_UPDATE	0x26
>>> +#define IS31FL319X_RESET	0xff
>>> +
>>> +#define IS31FL319X_REG_CNT	(IS31FL319X_RESET + 1)
>>> +
>>> +#define NUM_LEDS 9	/* max for 3199 chip */
>>> +
>>> +struct is31fl319x_chip {
>>> +	struct i2c_client	*client;
>>> +	struct regmap		*regmap;
>>> +
>>> +	struct is31fl319x_led {
>>> +		struct is31fl319x_chip	*chip;
>>> +		struct led_classdev	led_cdev;
>>> +	} leds[NUM_LEDS];
>>> +};
>>> +
>>> +static const struct i2c_device_id is31fl319x_id[] = {
>>> +	{ "is31fl3190", 1 },
>>> +	{ "is31fl3191", 1 },
>>> +	{ "is31fl3193", 3 },
>>> +	{ "is31fl3196", 6 },
>>> +	{ "is31fl3199", 9 },
>>> +	{ "sn3199", 9 },
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
>>> +
>>> +static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
>>> +				   enum led_brightness brightness)
>>> +{
>>> +	struct is31fl319x_led *led = container_of(led_cdev,
>>> +						  struct is31fl319x_led,
>>> +						  led_cdev);
>>> +	struct is31fl319x_chip *is31 = led->chip;
>>> +	int ret;
>>> +
>>> +	int i;
>>> +	u8 ctrl1, ctrl2;
>>
>> You can initialize these variables here.
>
> Ok.
>
>>
>>> +
>>> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
>>> +		brightness);
>>> +
>>> +	/* update PWM register */
>>> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
>>> +			   brightness);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	ctrl1 = 0;
>>> +	ctrl2 = 0;
>>
>> And remove above two lines.
>>
>>> +
>>> +	/* read current brightness of all PWM channels */
>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>> +		unsigned int pwm_value;
>>> +		bool on;
>>> +
>>> +		/*
>>> +		 * since neither led_cdev nor the chip can provide
>>> +		 * the current setting, we read from the regmap cache
>>> +		 */
>>> +
>>> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
>>> +				  &pwm_value);
>>> +
>>> +		dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
>>> +			__func__, i, ret, pwm_value);
>>> +		on = ret >= 0 && pwm_value > LED_OFF;
>>> +
>>> +		if (i < 3)
>>> +			ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
>>> +		else if (i < 6)
>>> +			ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
>>> +		else
>>> +			ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
>>> +	}
>>> +
>>> +
>>> +	if (ctrl1 > 0 || ctrl2 > 0) {
>>> +		dev_dbg(&is31->client->dev, "power up %02x %02x\n",
>>> +			ctrl1, ctrl2);
>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
>>> +		/* update PWMs */
>>> +		regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
>>> +		/* enable chip from shut down */
>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
>>> +	} else {
>>> +		dev_dbg(&is31->client->dev, "power down\n");
>>> +		/* shut down (no need to clear CTRL1/2) */
>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
>>> +	}
>>
>> You need a mutex protection in this function, so as to prevent other
>> processes from jumping in in the middle of setting up a brightness,
>> which requires writing several registers.
>
> Uh?
> I thought that registering is31fl319x_brightness_set as
> led_cdev.brightness_set_blocking
> does the trick that we don't need a private mutex here because it
> can be called only once anyways. At least that was suggested during v1 review.

Blocking doesn't mean that no one else can call the function in
the same time, but to the fact that the caller can be blocked
for the time needed for satisfying some condition. e.g. completion
of I2C transmission. It needs different treatment in the LED core,
as brightness setting can't sleep in general, due to the fact,
that it can be called from atomic context. In this case LED core
delegates brightness setting to a work queue task and
led_set_brightness() returns without blocking the caller.

Excerpt from linux/leds.h:

=================================================================

/* Set LED brightness level
  * Must not sleep. Use brightness_set_blocking for drivers
  * that can sleep while setting brightness.
  */
void            (*brightness_set)(struct led_classdev *led_cdev,
                                   enum led_brightness brightness);
/*
  * Set LED brightness level immediately - it can block the caller for
  * the time required for accessing a LED device register.
  */
int (*brightness_set_blocking)(struct led_classdev *led_cdev,
                                enum led_brightness brightness);

=================================================================


In the [1] I suggested the following :

"You can serialize the operations in brightness_set_blocking with
a mutex."

>>
>>> +	return ret;
>>> +}
>>> +
>>> +static struct led_info *
>>> +is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
>>> +{
>>> +	struct device_node *np = client->dev.of_node, *child;
>>> +	struct led_info *is31_leds;
>>> +	int count;
>>> +
>>> +	if (!np)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	count = of_get_child_count(np);
>>> +	dev_dbg(&client->dev, "child count %d\n", count);
>>> +	if (!count || count > NUM_LEDS)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	is31_leds = devm_kzalloc(&client->dev,
>>> +			sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
>>> +	if (!is31_leds)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	for_each_child_of_node(np, child) {
>>> +		struct led_info led;
>>
>> Using this local variable adds too much noise here.
>> I suggest avoiding the use of struct led_info. It is semantically
>> inconsistent in the context of DT parsing, since you don't have
>> any flags to parse.
>>
>>> +		u32 reg;
>>> +		int ret;
>>> +
>>> +		led.name = NULL;
>>> +		ret = of_property_read_string(child, "label", &led.name);
>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>> +			return ERR_PTR(ret);
>>
>> You could assign the name directly to related struct led_classdev
>> property. Moreover, if label is not present the child node name
>> should be used for LED class device name. Please refer to the other
>> drivers and common LED bindings.
>
> Ok.
>
>>
>>> +		led.default_trigger = NULL;
>>> +		ret = of_property_read_string(child, "linux,default-trigger",
>>> +			&led.default_trigger);
>>
>> Here also please assign the trigger directly to struct led_classdev.
>
> Ok.
>
>>
>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>> +			return ERR_PTR(ret);
>>> +		led.flags = 0;
>>> +		ret = of_property_read_u32(child, "reg", &reg);
>>> +		dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
>>> +		reg -= 1;	/* index 0 is at reg = 1 */
>>> +		if (ret != 0 || reg < 0 || reg >= num_leds)
>>> +			continue;
>>> +
>>> +		if (is31_leds[reg].name)
>>> +			dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
>>> +				reg, is31_leds[reg].name, led.name);
>>> +		is31_leds[reg] = led;
>>> +	}
>>> +
>>> +	return is31_leds;
>>> +}
>>> +
>>> +static const struct of_device_id of_is31fl319x_leds_match[] = {
>>> +	{ .compatible = "issi,is31fl3190", (void *) 1 },
>>> +	{ .compatible = "issi,is31fl3191", (void *) 1 },
>>> +	{ .compatible = "issi,is31fl3193", (void *) 3 },
>>> +	{ .compatible = "issi,is31fl3196", (void *) 6 },
>>> +	{ .compatible = "issi,is31fl3199", (void *) 9 },
>>> +	{ .compatible = "si-en,sn3199", (void *) 9 },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
>>> +
>>> +static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
>>> +{ /* we have no readable registers */
>>> +	return false;
>>> +}
>>> +
>>> +static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
>>> +{ /* volatile registers are not cached */
>>> +	switch (reg) {
>>> +	case IS31FL319X_DATA_UPDATE:
>>> +	case IS31FL319X_TIME_UPDATE:
>>> +	case IS31FL319X_RESET:
>>> +		return true;	/* always write-through */
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>> +
>>> +struct regmap_config regmap_config = {
>>> +	.reg_bits = 8,
>>> +	.val_bits = 8,
>>> +	.max_register = IS31FL319X_REG_CNT,
>>> +	.cache_type = REGCACHE_FLAT,
>>> +	.readable_reg = is31fl319x_readable_reg,
>>> +	.volatile_reg = is31fl319x_volatile_reg,
>>> +};
>>> +
>>> +static int is31fl319x_probe(struct i2c_client *client,
>>> +		const struct i2c_device_id *id)
>>> +{
>>> +	struct is31fl319x_chip *is31;
>>> +	struct i2c_adapter *adapter;
>>> +	struct led_info *leds;
>>> +	int err;
>>> +	int i = 0;
>>> +
>>> +	adapter = to_i2c_adapter(client->dev.parent);
>>> +
>>> +	dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
>>> +		(int) id->driver_data);
>>> +
>>> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>>> +		return -EIO;
>>> +
>>> +	leds = is31fl319x_parse_dt(client, (int) id->driver_data);
>>> +	if (IS_ERR(leds)) {
>>> +		dev_err(&client->dev, "DT parsing error %d\n",
>>> +			(int) PTR_ERR(leds));
>>> +		return PTR_ERR(leds);
>>> +	}
>>> +
>>> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
>>> +	if (!is31)
>>> +		return -ENOMEM;
>>> +
>>> +	is31->client = client;
>>> +	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
>>> +	if (IS_ERR(is31->regmap)) {
>>> +		dev_err(&client->dev, "failed to allocate register map\n");
>>> +		return PTR_ERR(is31->regmap);
>>> +	}
>>> +
>>> +	i2c_set_clientdata(client, is31);
>>> +
>>> +	/* check for write-reply from chip (we can't read any registers) */
>>> +	err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
>>
>> Doesn't powering the device up result in loading registers with default
>> values?
>
> Yes, but not a reboot.
>
> And we have to write *some* register anyways here to be able to detect
> if the chip is present or does not respond.
>
> So what would writing a different register change?

OK, let's leave that as is.

>>
>>> +	if (err < 0) {
>>> +		dev_err(&client->dev, "no response from chip write: err = %d\n",
>>> +			err);
>>> +		return -EIO;	/* does not answer */
>>> +	}
>>> +
>>> +	/* initialize chip and regmap so that we never try to read from i2c */
>>
>> You can initialize default register values without writing them,
>> by using regmap's reg_defaults property.
>
> Hm,
>
>>
>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
>>> +	for (i = 0; i < NUM_LEDS; i++)
>>> +		regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
>
> would replace a small loop with a bigger table. But if you prefer this I can change.

Writing registers only to initialize regmap cache looks odd.
Moreover it unnecessarily extends driver probing time.

>>> +
>>> +	if (client->dev.of_node) {
>>> +		u32 val;
>>> +		u8 config2 = 0;
>>> +
>>> +		if (of_property_read_u32(client->dev.of_node,
>>> +					 "led-max-microamp", &val)) {
>>
>> of_property_read_u32 returns 0 on success.
>
> Ok.
>
>>
>> led-max-microamp needs to be defined per each sub-LED (child node).
>> The greatest value should be used for setting CS bit field of CTRL2
>> register, and max_brightness values of the LEDs with lower max microamp
>> value should be limited accordingly.
>
> Hm. The chip does not allow to limit individual LEDs. So we should take the
> *minimum* of all subnodes.

But the LED core does. That's a max_brightness property is for.

>>
>> Moreover, this is still DT parsing and should find its place in
>> is31fl319x_parse_dt().
>>
>>> +			if (val > 40000)
>>> +				val = 40000;
>>> +			if (val < 5000)
>>> +				val = 5000;
>>> +			config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
>>> +		}
>>> +		if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
>>> +					 &val)) {
>>
>> Please move this to is31fl319x_parse_dt().
>
> Well, the main reason is that this needs to introduce another temporary struct
> to pass values from parse_dt to probe and copy values there.
 >
> Would it be acceptable to inline all DT parsing?

Don't reinvent the wheel. Please compare how other LED class drivers
perform DT parsing.

>>
>>> +			if (val > 21)
>>> +				val = 21;
>>> +			config2 |= val / 3; /* AGS */
>>> +		}
>>> +		regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
>>> +	}
>>> +
>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>> +		struct is31fl319x_led *l = is31->leds + i;
>>> +
>>> +		l->chip = is31;
>>> +		if (leds[i].name && !leds[i].flags) {
>>> +			l->led_cdev.name = leds[i].name;
>>> +			l->led_cdev.default_trigger
>>> +				= leds[i].default_trigger;
>>> +			l->led_cdev.brightness_set_blocking
>>> +				= is31fl319x_brightness_set;
>>> +			/* NOTE: is31fl319x_brightness_set will be called
>>> +			 * immediately after register() before we return
>>> +			 */
>>
>> Who will call it? I believe that this is true only if default-on
>> trigger is registered.
>
> Yes, as soon as anyone can call it we must be aware.

This is common for all LED class devices. Please drop the comment.

>>
>>> +			err = devm_led_classdev_register(&client->dev,
>>> +						    &l->led_cdev);
>>> +			if (err < 0)
>>> +				return err;
>>> +		}
>>> +	}
>>> +
>>> +	dev_dbg(&client->dev, "probed\n");
>>
>> Please remove this debug log. You can verify it by checking presence of
>> the device in the sysfs.
>
> Ok.
>
>>
>>> +	return 0;
>>> +}
>>> +
>>> +static struct i2c_driver is31fl319x_driver = {
>>> +	.driver   = {
>>> +		.name    = "leds-is31fl319x",
>>> +		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),
>>> +	},
>>> +	.probe    = is31fl319x_probe,
>>> +	.id_table = is31fl319x_id,
>>> +};
>>> +
>>> +module_i2c_driver(is31fl319x_driver);
>>> +
>>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>>> +MODULE_DESCRIPTION("IS31FL319X LED driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
>> --
>> Best regards,
>> Jacek Anaszewski
>
> Will fix asap.
>
> Thanks and BR,
> Nikolaus Schaller
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

[1] https://lkml.org/lkml/2016/4/20/754

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi Jacek,

> Am 07.07.2016 um 13:32 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
> 
> Hi Nikolaus,
> 
> On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
>> Hi Jacek,
>> 
>>> Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>> 
>>> Hi Nikolaus,
>>> 
>>> Thanks for the updated version. Some comments below.
>>> 
>>> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>>>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>>>> chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
>>>> LEDs.
>>>> 
>>>> Each LED is individually controllable in brightness (through pwm)
>>>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>>>> 
>>>> The maximum current of the LEDs can be programmed and limited to
>>>> 5 .. 40mA through a device tree property.
>>>> 
>>>> The chip is connected through I2C and can have one of 4 addresses
>>>> in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
>>>> address is defined by the reg property as usual.
>>>> 
>>>> The chip also has a shutdown input which could be connected to a GPIO,
>>>> but this driver uses software shutdown if all LEDs are inactivated.
>>>> 
>>>> The chip also has breathing and audio features which are not fully
>>>> supported by this driver.
>>>> 
>>>> Tested-on: OMAP5 based Pyra handheld prototype.
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>>  drivers/leds/Kconfig           |   8 +
>>>>  drivers/leds/Makefile          |   1 +
>>>>  drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 363 insertions(+)
>>>>  create mode 100644 drivers/leds/leds-is31fl319x.c
>>>> 
>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>> index 5ae2834..65b1045 100644
>>>> --- a/drivers/leds/Kconfig
>>>> +++ b/drivers/leds/Kconfig
>>>> @@ -570,6 +570,14 @@ config LEDS_SEAD3
>>>>  	  This driver can also be built as a module. If so the module
>>>>  	  will be called leds-sead3.
>>>> 
>>>> +config LEDS_IS31FL319X
>>>> +	tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
>>>> +	depends on LEDS_CLASS && I2C && OF
>>>> +	help
>>>> +	  This option enables support for LEDs connected to ISSI IS31FL319x
>>>> +	  fancy LED driver chips accessed via the I2C bus.
>>>> +	  Driver supports individual PWM brightness control for each channel.
>>>> +
>>>>  config LEDS_IS31FL32XX
>>>>  	tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
>>>>  	depends on LEDS_CLASS && I2C && OF
>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>> index cb2013d..b6ce9f9 100644
>>>> --- a/drivers/leds/Makefile
>>>> +++ b/drivers/leds/Makefile
>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>>>>  obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
>>>>  obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>>>>  obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
>>>> +obj-$(CONFIG_LEDS_IS31FL319X)		+= leds-is31fl319x.o
>>>>  obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
>>>> 
>>>>  # LED SPI Drivers
>>>> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
>>>> new file mode 100644
>>>> index 0000000..bbf8850
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-is31fl319x.c
>>>> @@ -0,0 +1,354 @@
>>>> +/*
>>>> + * Copyright 2015-16 Golden Delicious Computers
>>>> + *
>>>> + * Author: Nikolaus Schaller <hns@goldelico.com>
>>>> + *
>>>> + * Based on leds-tca6507.c
>>>> + *
>>>> + * This file is subject to the terms and conditions of version 2 of
>>>> + * the GNU General Public License.  See the file COPYING in the main
>>>> + * directory of this archive for more details.
>>>> + *
>>>> + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
>>>> + * effect LEDs.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/leds.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +/* register numbers */
>>>> +#define IS31FL319X_SHUTDOWN	0x00
>>>> +#define IS31FL319X_CTRL1	0x01
>>>> +#define IS31FL319X_CTRL2	0x02
>>>> +#define IS31FL319X_CONFIG1	0x03
>>>> +#define IS31FL319X_CONFIG2	0x04
>>>> +#define IS31FL319X_RAMP_MODE	0x05
>>>> +#define IS31FL319X_BREATH_MASK	0x06
>>>> +#define IS31FL319X_PWM1		0x07
>>>> +#define IS31FL319X_PWM2		0x08
>>>> +#define IS31FL319X_PWM3		0x09
>>>> +#define IS31FL319X_PWM4		0x0a
>>>> +#define IS31FL319X_PWM5		0x0b
>>>> +#define IS31FL319X_PWM6		0x0c
>>>> +#define IS31FL319X_PWM7		0x0d
>>>> +#define IS31FL319X_PWM8		0x0e
>>>> +#define IS31FL319X_PWM9		0x0f
>>>> +#define IS31FL319X_DATA_UPDATE	0x10
>>>> +#define IS31FL319X_T0_1		0x11
>>>> +#define IS31FL319X_T0_2		0x12
>>>> +#define IS31FL319X_T0_3		0x13
>>>> +#define IS31FL319X_T0_4		0x14
>>>> +#define IS31FL319X_T0_5		0x15
>>>> +#define IS31FL319X_T0_6		0x16
>>>> +#define IS31FL319X_T0_7		0x17
>>>> +#define IS31FL319X_T0_8		0x18
>>>> +#define IS31FL319X_T0_9		0x19
>>>> +#define IS31FL319X_T123_1	0x1a
>>>> +#define IS31FL319X_T123_2	0x1b
>>>> +#define IS31FL319X_T123_3	0x1c
>>>> +#define IS31FL319X_T4_1		0x1d
>>>> +#define IS31FL319X_T4_2		0x1e
>>>> +#define IS31FL319X_T4_3		0x1f
>>>> +#define IS31FL319X_T4_4		0x20
>>>> +#define IS31FL319X_T4_5		0x21
>>>> +#define IS31FL319X_T4_6		0x22
>>>> +#define IS31FL319X_T4_7		0x23
>>>> +#define IS31FL319X_T4_8		0x24
>>>> +#define IS31FL319X_T4_9		0x25
>>>> +#define IS31FL319X_TIME_UPDATE	0x26
>>>> +#define IS31FL319X_RESET	0xff
>>>> +
>>>> +#define IS31FL319X_REG_CNT	(IS31FL319X_RESET + 1)
>>>> +
>>>> +#define NUM_LEDS 9	/* max for 3199 chip */
>>>> +
>>>> +struct is31fl319x_chip {
>>>> +	struct i2c_client	*client;
>>>> +	struct regmap		*regmap;
>>>> +
>>>> +	struct is31fl319x_led {
>>>> +		struct is31fl319x_chip	*chip;
>>>> +		struct led_classdev	led_cdev;
>>>> +	} leds[NUM_LEDS];
>>>> +};
>>>> +
>>>> +static const struct i2c_device_id is31fl319x_id[] = {
>>>> +	{ "is31fl3190", 1 },
>>>> +	{ "is31fl3191", 1 },
>>>> +	{ "is31fl3193", 3 },
>>>> +	{ "is31fl3196", 6 },
>>>> +	{ "is31fl3199", 9 },
>>>> +	{ "sn3199", 9 },
>>>> +	{ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
>>>> +
>>>> +static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
>>>> +				   enum led_brightness brightness)
>>>> +{
>>>> +	struct is31fl319x_led *led = container_of(led_cdev,
>>>> +						  struct is31fl319x_led,
>>>> +						  led_cdev);
>>>> +	struct is31fl319x_chip *is31 = led->chip;
>>>> +	int ret;
>>>> +
>>>> +	int i;
>>>> +	u8 ctrl1, ctrl2;
>>> 
>>> You can initialize these variables here.
>> 
>> Ok.
>> 
>>> 
>>>> +
>>>> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
>>>> +		brightness);
>>>> +
>>>> +	/* update PWM register */
>>>> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
>>>> +			   brightness);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	ctrl1 = 0;
>>>> +	ctrl2 = 0;
>>> 
>>> And remove above two lines.
>>> 
>>>> +
>>>> +	/* read current brightness of all PWM channels */
>>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>>> +		unsigned int pwm_value;
>>>> +		bool on;
>>>> +
>>>> +		/*
>>>> +		 * since neither led_cdev nor the chip can provide
>>>> +		 * the current setting, we read from the regmap cache
>>>> +		 */
>>>> +
>>>> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
>>>> +				  &pwm_value);
>>>> +
>>>> +		dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
>>>> +			__func__, i, ret, pwm_value);
>>>> +		on = ret >= 0 && pwm_value > LED_OFF;
>>>> +
>>>> +		if (i < 3)
>>>> +			ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
>>>> +		else if (i < 6)
>>>> +			ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
>>>> +		else
>>>> +			ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
>>>> +	}
>>>> +
>>>> +
>>>> +	if (ctrl1 > 0 || ctrl2 > 0) {
>>>> +		dev_dbg(&is31->client->dev, "power up %02x %02x\n",
>>>> +			ctrl1, ctrl2);
>>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
>>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
>>>> +		/* update PWMs */
>>>> +		regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
>>>> +		/* enable chip from shut down */
>>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
>>>> +	} else {
>>>> +		dev_dbg(&is31->client->dev, "power down\n");
>>>> +		/* shut down (no need to clear CTRL1/2) */
>>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
>>>> +	}
>>> 
>>> You need a mutex protection in this function, so as to prevent other
>>> processes from jumping in in the middle of setting up a brightness,
>>> which requires writing several registers.
>> 
>> Uh?
>> I thought that registering is31fl319x_brightness_set as
>> led_cdev.brightness_set_blocking
>> does the trick that we don't need a private mutex here because it
>> can be called only once anyways. At least that was suggested during v1 review.
> 
> Blocking doesn't mean that no one else can call the function in
> the same time, but to the fact that the caller can be blocked
> for the time needed for satisfying some condition. e.g. completion
> of I2C transmission. It needs different treatment in the LED core,
> as brightness setting can't sleep in general, due to the fact,
> that it can be called from atomic context. In this case LED core
> delegates brightness setting to a work queue task and
> led_set_brightness() returns without blocking the caller.

Ok. That is what I didn't think about: multiple LED triggers running
in parallel.

And with the new structure each one is independently calling set_brightness.
i2c traffic has its mutex but not the step between writing and reading the PWM
values to control chip shut down. The risk is that it is shut down in the wrong
moment by a race.

Thanks for spotting this.

> 
> Excerpt from linux/leds.h:
> 
> =================================================================
> 
> /* Set LED brightness level
> * Must not sleep. Use brightness_set_blocking for drivers
> * that can sleep while setting brightness.
> */
> void            (*brightness_set)(struct led_classdev *led_cdev,
>                                  enum led_brightness brightness);
> /*
> * Set LED brightness level immediately - it can block the caller for
> * the time required for accessing a LED device register.
> */
> int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>                               enum led_brightness brightness);
> 
> =================================================================
> 
> 
> In the [1] I suggested the following :
> 
> "You can serialize the operations in brightness_set_blocking with
> a mutex."
> 
>>> 
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static struct led_info *
>>>> +is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
>>>> +{
>>>> +	struct device_node *np = client->dev.of_node, *child;
>>>> +	struct led_info *is31_leds;
>>>> +	int count;
>>>> +
>>>> +	if (!np)
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	count = of_get_child_count(np);
>>>> +	dev_dbg(&client->dev, "child count %d\n", count);
>>>> +	if (!count || count > NUM_LEDS)
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	is31_leds = devm_kzalloc(&client->dev,
>>>> +			sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
>>>> +	if (!is31_leds)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	for_each_child_of_node(np, child) {
>>>> +		struct led_info led;
>>> 
>>> Using this local variable adds too much noise here.
>>> I suggest avoiding the use of struct led_info. It is semantically
>>> inconsistent in the context of DT parsing, since you don't have
>>> any flags to parse.
>>> 
>>>> +		u32 reg;
>>>> +		int ret;
>>>> +
>>>> +		led.name = NULL;
>>>> +		ret = of_property_read_string(child, "label", &led.name);
>>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>>> +			return ERR_PTR(ret);
>>> 
>>> You could assign the name directly to related struct led_classdev
>>> property. Moreover, if label is not present the child node name
>>> should be used for LED class device name. Please refer to the other
>>> drivers and common LED bindings.
>> 
>> Ok.
>> 
>>> 
>>>> +		led.default_trigger = NULL;
>>>> +		ret = of_property_read_string(child, "linux,default-trigger",
>>>> +			&led.default_trigger);
>>> 
>>> Here also please assign the trigger directly to struct led_classdev.
>> 
>> Ok.
>> 
>>> 
>>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>>> +			return ERR_PTR(ret);
>>>> +		led.flags = 0;
>>>> +		ret = of_property_read_u32(child, "reg", &reg);
>>>> +		dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
>>>> +		reg -= 1;	/* index 0 is at reg = 1 */
>>>> +		if (ret != 0 || reg < 0 || reg >= num_leds)
>>>> +			continue;
>>>> +
>>>> +		if (is31_leds[reg].name)
>>>> +			dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
>>>> +				reg, is31_leds[reg].name, led.name);
>>>> +		is31_leds[reg] = led;
>>>> +	}
>>>> +
>>>> +	return is31_leds;
>>>> +}
>>>> +
>>>> +static const struct of_device_id of_is31fl319x_leds_match[] = {
>>>> +	{ .compatible = "issi,is31fl3190", (void *) 1 },
>>>> +	{ .compatible = "issi,is31fl3191", (void *) 1 },
>>>> +	{ .compatible = "issi,is31fl3193", (void *) 3 },
>>>> +	{ .compatible = "issi,is31fl3196", (void *) 6 },
>>>> +	{ .compatible = "issi,is31fl3199", (void *) 9 },
>>>> +	{ .compatible = "si-en,sn3199", (void *) 9 },
>>>> +	{},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
>>>> +
>>>> +static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
>>>> +{ /* we have no readable registers */
>>>> +	return false;
>>>> +}
>>>> +
>>>> +static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
>>>> +{ /* volatile registers are not cached */
>>>> +	switch (reg) {
>>>> +	case IS31FL319X_DATA_UPDATE:
>>>> +	case IS31FL319X_TIME_UPDATE:
>>>> +	case IS31FL319X_RESET:
>>>> +		return true;	/* always write-through */
>>>> +	default:
>>>> +		return false;
>>>> +	}
>>>> +}
>>>> +
>>>> +struct regmap_config regmap_config = {
>>>> +	.reg_bits = 8,
>>>> +	.val_bits = 8,
>>>> +	.max_register = IS31FL319X_REG_CNT,
>>>> +	.cache_type = REGCACHE_FLAT,
>>>> +	.readable_reg = is31fl319x_readable_reg,
>>>> +	.volatile_reg = is31fl319x_volatile_reg,
>>>> +};
>>>> +
>>>> +static int is31fl319x_probe(struct i2c_client *client,
>>>> +		const struct i2c_device_id *id)
>>>> +{
>>>> +	struct is31fl319x_chip *is31;
>>>> +	struct i2c_adapter *adapter;
>>>> +	struct led_info *leds;
>>>> +	int err;
>>>> +	int i = 0;
>>>> +
>>>> +	adapter = to_i2c_adapter(client->dev.parent);
>>>> +
>>>> +	dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
>>>> +		(int) id->driver_data);
>>>> +
>>>> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>>>> +		return -EIO;
>>>> +
>>>> +	leds = is31fl319x_parse_dt(client, (int) id->driver_data);
>>>> +	if (IS_ERR(leds)) {
>>>> +		dev_err(&client->dev, "DT parsing error %d\n",
>>>> +			(int) PTR_ERR(leds));
>>>> +		return PTR_ERR(leds);
>>>> +	}
>>>> +
>>>> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
>>>> +	if (!is31)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	is31->client = client;
>>>> +	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
>>>> +	if (IS_ERR(is31->regmap)) {
>>>> +		dev_err(&client->dev, "failed to allocate register map\n");
>>>> +		return PTR_ERR(is31->regmap);
>>>> +	}
>>>> +
>>>> +	i2c_set_clientdata(client, is31);
>>>> +
>>>> +	/* check for write-reply from chip (we can't read any registers) */
>>>> +	err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
>>> 
>>> Doesn't powering the device up result in loading registers with default
>>> values?
>> 
>> Yes, but not a reboot.
>> 
>> And we have to write *some* register anyways here to be able to detect
>> if the chip is present or does not respond.
>> 
>> So what would writing a different register change?
> 
> OK, let's leave that as is.
> 
>>> 
>>>> +	if (err < 0) {
>>>> +		dev_err(&client->dev, "no response from chip write: err = %d\n",
>>>> +			err);
>>>> +		return -EIO;	/* does not answer */
>>>> +	}
>>>> +
>>>> +	/* initialize chip and regmap so that we never try to read from i2c */
>>> 
>>> You can initialize default register values without writing them,
>>> by using regmap's reg_defaults property.
>> 
>> Hm,
>> 
>>> 
>>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
>>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
>>>> +	for (i = 0; i < NUM_LEDS; i++)
>>>> +		regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
>> 
>> would replace a small loop with a bigger table. But if you prefer this I can change.
> 
> Writing registers only to initialize regmap cache looks odd.
> Moreover it unnecessarily extends driver probing time.

Ok, we will change.

> 
>>>> +
>>>> +	if (client->dev.of_node) {
>>>> +		u32 val;
>>>> +		u8 config2 = 0;
>>>> +
>>>> +		if (of_property_read_u32(client->dev.of_node,
>>>> +					 "led-max-microamp", &val)) {
>>> 
>>> of_property_read_u32 returns 0 on success.
>> 
>> Ok.
>> 
>>> 
>>> led-max-microamp needs to be defined per each sub-LED (child node).
>>> The greatest value should be used for setting CS bit field of CTRL2
>>> register, and max_brightness values of the LEDs with lower max microamp
>>> value should be limited accordingly.
>> 
>> Hm. The chip does not allow to limit individual LEDs. So we should take the
>> *minimum* of all subnodes.
> 
> But the LED core does. That's a max_brightness property is for.

Ok, that seems to be a new feature we have no use case for since all our
LEDs have the same current specification. But we will check how it can be done.

> 
>>> 
>>> Moreover, this is still DT parsing and should find its place in
>>> is31fl319x_parse_dt().
>>> 
>>>> +			if (val > 40000)
>>>> +				val = 40000;
>>>> +			if (val < 5000)
>>>> +				val = 5000;
>>>> +			config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
>>>> +		}
>>>> +		if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
>>>> +					 &val)) {
>>> 
>>> Please move this to is31fl319x_parse_dt().
>> 
>> Well, the main reason is that this needs to introduce another temporary struct
>> to pass values from parse_dt to probe and copy values there.
> >
>> Would it be acceptable to inline all DT parsing?
> 
> Don't reinvent the wheel. Please compare how other LED class drivers
> perform DT parsing.

I have looked into other drivers but have not seen a common pattern. Some
have no DT (e.g. 88pm860x), some have some struct led_config_data (e.g. aat1290)
and some do it inlined in the probe function (e.g. bcm6328) and some do it
in a separate parse_dt (is31fl32).

So which driver would you recommend as best practice?

> 
>>> 
>>>> +			if (val > 21)
>>>> +				val = 21;
>>>> +			config2 |= val / 3; /* AGS */
>>>> +		}
>>>> +		regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>>> +		struct is31fl319x_led *l = is31->leds + i;
>>>> +
>>>> +		l->chip = is31;
>>>> +		if (leds[i].name && !leds[i].flags) {
>>>> +			l->led_cdev.name = leds[i].name;
>>>> +			l->led_cdev.default_trigger
>>>> +				= leds[i].default_trigger;
>>>> +			l->led_cdev.brightness_set_blocking
>>>> +				= is31fl319x_brightness_set;
>>>> +			/* NOTE: is31fl319x_brightness_set will be called
>>>> +			 * immediately after register() before we return
>>>> +			 */
>>> 
>>> Who will call it? I believe that this is true only if default-on
>>> trigger is registered.
>> 
>> Yes, as soon as anyone can call it we must be aware.
> 
> This is common for all LED class devices. Please drop the comment.

Ok. Might belong somewhere else (e.g. to devm_led_classdev_register
documentation).

> 
>>> 
>>>> +			err = devm_led_classdev_register(&client->dev,
>>>> +						    &l->led_cdev);
>>>> +			if (err < 0)
>>>> +				return err;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	dev_dbg(&client->dev, "probed\n");
>>> 
>>> Please remove this debug log. You can verify it by checking presence of
>>> the device in the sysfs.
>> 
>> Ok.
>> 
>>> 
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct i2c_driver is31fl319x_driver = {
>>>> +	.driver   = {
>>>> +		.name    = "leds-is31fl319x",
>>>> +		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),
>>>> +	},
>>>> +	.probe    = is31fl319x_probe,
>>>> +	.id_table = is31fl319x_id,
>>>> +};
>>>> +
>>>> +module_i2c_driver(is31fl319x_driver);
>>>> +
>>>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>>>> +MODULE_DESCRIPTION("IS31FL319X LED driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> 
>>> 
>>> 
>>> --
>>> Best regards,
>>> Jacek Anaszewski
>> 
>> Will fix asap.
>> 
>> Thanks and BR,
>> Nikolaus Schaller
>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> 
> 
> [1] https://lkml.org/lkml/2016/4/20/754
> 
> -- 
> Best regards,
> Jacek Anaszewski

BR and thanks,
Nikolaus

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi Jacek,

> Am 07.07.2016 um 13:32 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
> 
> Hi Nikolaus,
> 
> On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
>> Hi Jacek,
>> 
>>> Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>> 
>>> Hi Nikolaus,
>>> 
>>> Thanks for the updated version. Some comments below.
>>> 
>>> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>>>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>>>> chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
>>>> LEDs.
>>>> 
>>>> Each LED is individually controllable in brightness (through pwm)
>>>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>>>> 
>>>> The maximum current of the LEDs can be programmed and limited to
>>>> 5 .. 40mA through a device tree property.
>>>> 
>>>> The chip is connected through I2C and can have one of 4 addresses
>>>> in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
>>>> address is defined by the reg property as usual.
>>>> 
>>>> The chip also has a shutdown input which could be connected to a GPIO,
>>>> but this driver uses software shutdown if all LEDs are inactivated.
>>>> 
>>>> The chip also has breathing and audio features which are not fully
>>>> supported by this driver.
>>>> 
>>>> Tested-on: OMAP5 based Pyra handheld prototype.
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>>  drivers/leds/Kconfig           |   8 +
>>>>  drivers/leds/Makefile          |   1 +
>>>>  drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 363 insertions(+)
>>>>  create mode 100644 drivers/leds/leds-is31fl319x.c
>>>> 
>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>> index 5ae2834..65b1045 100644
>>>> --- a/drivers/leds/Kconfig
>>>> +++ b/drivers/leds/Kconfig
>>>> @@ -570,6 +570,14 @@ config LEDS_SEAD3
>>>>  	  This driver can also be built as a module. If so the module
>>>>  	  will be called leds-sead3.
>>>> 
>>>> +config LEDS_IS31FL319X
>>>> +	tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
>>>> +	depends on LEDS_CLASS && I2C && OF
>>>> +	help
>>>> +	  This option enables support for LEDs connected to ISSI IS31FL319x
>>>> +	  fancy LED driver chips accessed via the I2C bus.
>>>> +	  Driver supports individual PWM brightness control for each channel.
>>>> +
>>>>  config LEDS_IS31FL32XX
>>>>  	tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
>>>>  	depends on LEDS_CLASS && I2C && OF
>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>> index cb2013d..b6ce9f9 100644
>>>> --- a/drivers/leds/Makefile
>>>> +++ b/drivers/leds/Makefile
>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>>>>  obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
>>>>  obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>>>>  obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
>>>> +obj-$(CONFIG_LEDS_IS31FL319X)		+= leds-is31fl319x.o
>>>>  obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
>>>> 
>>>>  # LED SPI Drivers
>>>> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
>>>> new file mode 100644
>>>> index 0000000..bbf8850
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-is31fl319x.c
>>>> @@ -0,0 +1,354 @@
>>>> +/*
>>>> + * Copyright 2015-16 Golden Delicious Computers
>>>> + *
>>>> + * Author: Nikolaus Schaller <hns@goldelico.com>
>>>> + *
>>>> + * Based on leds-tca6507.c
>>>> + *
>>>> + * This file is subject to the terms and conditions of version 2 of
>>>> + * the GNU General Public License.  See the file COPYING in the main
>>>> + * directory of this archive for more details.
>>>> + *
>>>> + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
>>>> + * effect LEDs.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/leds.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +/* register numbers */
>>>> +#define IS31FL319X_SHUTDOWN	0x00
>>>> +#define IS31FL319X_CTRL1	0x01
>>>> +#define IS31FL319X_CTRL2	0x02
>>>> +#define IS31FL319X_CONFIG1	0x03
>>>> +#define IS31FL319X_CONFIG2	0x04
>>>> +#define IS31FL319X_RAMP_MODE	0x05
>>>> +#define IS31FL319X_BREATH_MASK	0x06
>>>> +#define IS31FL319X_PWM1		0x07
>>>> +#define IS31FL319X_PWM2		0x08
>>>> +#define IS31FL319X_PWM3		0x09
>>>> +#define IS31FL319X_PWM4		0x0a
>>>> +#define IS31FL319X_PWM5		0x0b
>>>> +#define IS31FL319X_PWM6		0x0c
>>>> +#define IS31FL319X_PWM7		0x0d
>>>> +#define IS31FL319X_PWM8		0x0e
>>>> +#define IS31FL319X_PWM9		0x0f
>>>> +#define IS31FL319X_DATA_UPDATE	0x10
>>>> +#define IS31FL319X_T0_1		0x11
>>>> +#define IS31FL319X_T0_2		0x12
>>>> +#define IS31FL319X_T0_3		0x13
>>>> +#define IS31FL319X_T0_4		0x14
>>>> +#define IS31FL319X_T0_5		0x15
>>>> +#define IS31FL319X_T0_6		0x16
>>>> +#define IS31FL319X_T0_7		0x17
>>>> +#define IS31FL319X_T0_8		0x18
>>>> +#define IS31FL319X_T0_9		0x19
>>>> +#define IS31FL319X_T123_1	0x1a
>>>> +#define IS31FL319X_T123_2	0x1b
>>>> +#define IS31FL319X_T123_3	0x1c
>>>> +#define IS31FL319X_T4_1		0x1d
>>>> +#define IS31FL319X_T4_2		0x1e
>>>> +#define IS31FL319X_T4_3		0x1f
>>>> +#define IS31FL319X_T4_4		0x20
>>>> +#define IS31FL319X_T4_5		0x21
>>>> +#define IS31FL319X_T4_6		0x22
>>>> +#define IS31FL319X_T4_7		0x23
>>>> +#define IS31FL319X_T4_8		0x24
>>>> +#define IS31FL319X_T4_9		0x25
>>>> +#define IS31FL319X_TIME_UPDATE	0x26
>>>> +#define IS31FL319X_RESET	0xff
>>>> +
>>>> +#define IS31FL319X_REG_CNT	(IS31FL319X_RESET + 1)
>>>> +
>>>> +#define NUM_LEDS 9	/* max for 3199 chip */
>>>> +
>>>> +struct is31fl319x_chip {
>>>> +	struct i2c_client	*client;
>>>> +	struct regmap		*regmap;
>>>> +
>>>> +	struct is31fl319x_led {
>>>> +		struct is31fl319x_chip	*chip;
>>>> +		struct led_classdev	led_cdev;
>>>> +	} leds[NUM_LEDS];
>>>> +};
>>>> +
>>>> +static const struct i2c_device_id is31fl319x_id[] = {
>>>> +	{ "is31fl3190", 1 },
>>>> +	{ "is31fl3191", 1 },
>>>> +	{ "is31fl3193", 3 },
>>>> +	{ "is31fl3196", 6 },
>>>> +	{ "is31fl3199", 9 },
>>>> +	{ "sn3199", 9 },
>>>> +	{ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
>>>> +
>>>> +static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
>>>> +				   enum led_brightness brightness)
>>>> +{
>>>> +	struct is31fl319x_led *led = container_of(led_cdev,
>>>> +						  struct is31fl319x_led,
>>>> +						  led_cdev);
>>>> +	struct is31fl319x_chip *is31 = led->chip;
>>>> +	int ret;
>>>> +
>>>> +	int i;
>>>> +	u8 ctrl1, ctrl2;
>>> 
>>> You can initialize these variables here.
>> 
>> Ok.
>> 
>>> 
>>>> +
>>>> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
>>>> +		brightness);
>>>> +
>>>> +	/* update PWM register */
>>>> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
>>>> +			   brightness);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	ctrl1 = 0;
>>>> +	ctrl2 = 0;
>>> 
>>> And remove above two lines.
>>> 
>>>> +
>>>> +	/* read current brightness of all PWM channels */
>>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>>> +		unsigned int pwm_value;
>>>> +		bool on;
>>>> +
>>>> +		/*
>>>> +		 * since neither led_cdev nor the chip can provide
>>>> +		 * the current setting, we read from the regmap cache
>>>> +		 */
>>>> +
>>>> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
>>>> +				  &pwm_value);
>>>> +
>>>> +		dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
>>>> +			__func__, i, ret, pwm_value);
>>>> +		on = ret >= 0 && pwm_value > LED_OFF;
>>>> +
>>>> +		if (i < 3)
>>>> +			ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
>>>> +		else if (i < 6)
>>>> +			ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
>>>> +		else
>>>> +			ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
>>>> +	}
>>>> +
>>>> +
>>>> +	if (ctrl1 > 0 || ctrl2 > 0) {
>>>> +		dev_dbg(&is31->client->dev, "power up %02x %02x\n",
>>>> +			ctrl1, ctrl2);
>>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
>>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
>>>> +		/* update PWMs */
>>>> +		regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
>>>> +		/* enable chip from shut down */
>>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
>>>> +	} else {
>>>> +		dev_dbg(&is31->client->dev, "power down\n");
>>>> +		/* shut down (no need to clear CTRL1/2) */
>>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
>>>> +	}
>>> 
>>> You need a mutex protection in this function, so as to prevent other
>>> processes from jumping in in the middle of setting up a brightness,
>>> which requires writing several registers.
>> 
>> Uh?
>> I thought that registering is31fl319x_brightness_set as
>> led_cdev.brightness_set_blocking
>> does the trick that we don't need a private mutex here because it
>> can be called only once anyways. At least that was suggested during v1 review.
> 
> Blocking doesn't mean that no one else can call the function in
> the same time, but to the fact that the caller can be blocked
> for the time needed for satisfying some condition. e.g. completion
> of I2C transmission. It needs different treatment in the LED core,
> as brightness setting can't sleep in general, due to the fact,
> that it can be called from atomic context. In this case LED core
> delegates brightness setting to a work queue task and
> led_set_brightness() returns without blocking the caller.

Ok. That is what I didn't think about: multiple LED triggers running
in parallel.

And with the new structure each one is independently calling set_brightness.
i2c traffic has its mutex but not the step between writing and reading the PWM
values to control chip shut down. The risk is that it is shut down in the wrong
moment by a race.

Thanks for spotting this.

> 
> Excerpt from linux/leds.h:
> 
> =================================================================
> 
> /* Set LED brightness level
> * Must not sleep. Use brightness_set_blocking for drivers
> * that can sleep while setting brightness.
> */
> void            (*brightness_set)(struct led_classdev *led_cdev,
>                                  enum led_brightness brightness);
> /*
> * Set LED brightness level immediately - it can block the caller for
> * the time required for accessing a LED device register.
> */
> int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>                               enum led_brightness brightness);
> 
> =================================================================
> 
> 
> In the [1] I suggested the following :
> 
> "You can serialize the operations in brightness_set_blocking with
> a mutex."
> 
>>> 
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static struct led_info *
>>>> +is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
>>>> +{
>>>> +	struct device_node *np = client->dev.of_node, *child;
>>>> +	struct led_info *is31_leds;
>>>> +	int count;
>>>> +
>>>> +	if (!np)
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	count = of_get_child_count(np);
>>>> +	dev_dbg(&client->dev, "child count %d\n", count);
>>>> +	if (!count || count > NUM_LEDS)
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	is31_leds = devm_kzalloc(&client->dev,
>>>> +			sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
>>>> +	if (!is31_leds)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	for_each_child_of_node(np, child) {
>>>> +		struct led_info led;
>>> 
>>> Using this local variable adds too much noise here.
>>> I suggest avoiding the use of struct led_info. It is semantically
>>> inconsistent in the context of DT parsing, since you don't have
>>> any flags to parse.
>>> 
>>>> +		u32 reg;
>>>> +		int ret;
>>>> +
>>>> +		led.name = NULL;
>>>> +		ret = of_property_read_string(child, "label", &led.name);
>>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>>> +			return ERR_PTR(ret);
>>> 
>>> You could assign the name directly to related struct led_classdev
>>> property. Moreover, if label is not present the child node name
>>> should be used for LED class device name. Please refer to the other
>>> drivers and common LED bindings.
>> 
>> Ok.
>> 
>>> 
>>>> +		led.default_trigger = NULL;
>>>> +		ret = of_property_read_string(child, "linux,default-trigger",
>>>> +			&led.default_trigger);
>>> 
>>> Here also please assign the trigger directly to struct led_classdev.
>> 
>> Ok.
>> 
>>> 
>>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>>> +			return ERR_PTR(ret);
>>>> +		led.flags = 0;
>>>> +		ret = of_property_read_u32(child, "reg", &reg);
>>>> +		dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
>>>> +		reg -= 1;	/* index 0 is at reg = 1 */
>>>> +		if (ret != 0 || reg < 0 || reg >= num_leds)
>>>> +			continue;
>>>> +
>>>> +		if (is31_leds[reg].name)
>>>> +			dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
>>>> +				reg, is31_leds[reg].name, led.name);
>>>> +		is31_leds[reg] = led;
>>>> +	}
>>>> +
>>>> +	return is31_leds;
>>>> +}
>>>> +
>>>> +static const struct of_device_id of_is31fl319x_leds_match[] = {
>>>> +	{ .compatible = "issi,is31fl3190", (void *) 1 },
>>>> +	{ .compatible = "issi,is31fl3191", (void *) 1 },
>>>> +	{ .compatible = "issi,is31fl3193", (void *) 3 },
>>>> +	{ .compatible = "issi,is31fl3196", (void *) 6 },
>>>> +	{ .compatible = "issi,is31fl3199", (void *) 9 },
>>>> +	{ .compatible = "si-en,sn3199", (void *) 9 },
>>>> +	{},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
>>>> +
>>>> +static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
>>>> +{ /* we have no readable registers */
>>>> +	return false;
>>>> +}
>>>> +
>>>> +static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
>>>> +{ /* volatile registers are not cached */
>>>> +	switch (reg) {
>>>> +	case IS31FL319X_DATA_UPDATE:
>>>> +	case IS31FL319X_TIME_UPDATE:
>>>> +	case IS31FL319X_RESET:
>>>> +		return true;	/* always write-through */
>>>> +	default:
>>>> +		return false;
>>>> +	}
>>>> +}
>>>> +
>>>> +struct regmap_config regmap_config = {
>>>> +	.reg_bits = 8,
>>>> +	.val_bits = 8,
>>>> +	.max_register = IS31FL319X_REG_CNT,
>>>> +	.cache_type = REGCACHE_FLAT,
>>>> +	.readable_reg = is31fl319x_readable_reg,
>>>> +	.volatile_reg = is31fl319x_volatile_reg,
>>>> +};
>>>> +
>>>> +static int is31fl319x_probe(struct i2c_client *client,
>>>> +		const struct i2c_device_id *id)
>>>> +{
>>>> +	struct is31fl319x_chip *is31;
>>>> +	struct i2c_adapter *adapter;
>>>> +	struct led_info *leds;
>>>> +	int err;
>>>> +	int i = 0;
>>>> +
>>>> +	adapter = to_i2c_adapter(client->dev.parent);
>>>> +
>>>> +	dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
>>>> +		(int) id->driver_data);
>>>> +
>>>> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>>>> +		return -EIO;
>>>> +
>>>> +	leds = is31fl319x_parse_dt(client, (int) id->driver_data);
>>>> +	if (IS_ERR(leds)) {
>>>> +		dev_err(&client->dev, "DT parsing error %d\n",
>>>> +			(int) PTR_ERR(leds));
>>>> +		return PTR_ERR(leds);
>>>> +	}
>>>> +
>>>> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
>>>> +	if (!is31)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	is31->client = client;
>>>> +	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
>>>> +	if (IS_ERR(is31->regmap)) {
>>>> +		dev_err(&client->dev, "failed to allocate register map\n");
>>>> +		return PTR_ERR(is31->regmap);
>>>> +	}
>>>> +
>>>> +	i2c_set_clientdata(client, is31);
>>>> +
>>>> +	/* check for write-reply from chip (we can't read any registers) */
>>>> +	err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
>>> 
>>> Doesn't powering the device up result in loading registers with default
>>> values?
>> 
>> Yes, but not a reboot.
>> 
>> And we have to write *some* register anyways here to be able to detect
>> if the chip is present or does not respond.
>> 
>> So what would writing a different register change?
> 
> OK, let's leave that as is.
> 
>>> 
>>>> +	if (err < 0) {
>>>> +		dev_err(&client->dev, "no response from chip write: err = %d\n",
>>>> +			err);
>>>> +		return -EIO;	/* does not answer */
>>>> +	}
>>>> +
>>>> +	/* initialize chip and regmap so that we never try to read from i2c */
>>> 
>>> You can initialize default register values without writing them,
>>> by using regmap's reg_defaults property.
>> 
>> Hm,
>> 
>>> 
>>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
>>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
>>>> +	for (i = 0; i < NUM_LEDS; i++)
>>>> +		regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
>> 
>> would replace a small loop with a bigger table. But if you prefer this I can change.
> 
> Writing registers only to initialize regmap cache looks odd.
> Moreover it unnecessarily extends driver probing time.

Ok, we will change.

> 
>>>> +
>>>> +	if (client->dev.of_node) {
>>>> +		u32 val;
>>>> +		u8 config2 = 0;
>>>> +
>>>> +		if (of_property_read_u32(client->dev.of_node,
>>>> +					 "led-max-microamp", &val)) {
>>> 
>>> of_property_read_u32 returns 0 on success.
>> 
>> Ok.
>> 
>>> 
>>> led-max-microamp needs to be defined per each sub-LED (child node).
>>> The greatest value should be used for setting CS bit field of CTRL2
>>> register, and max_brightness values of the LEDs with lower max microamp
>>> value should be limited accordingly.
>> 
>> Hm. The chip does not allow to limit individual LEDs. So we should take the
>> *minimum* of all subnodes.
> 
> But the LED core does. That's a max_brightness property is for.

Ok, that seems to be a new feature we have no use case for since all our
LEDs have the same current specification. But we will check how it can be done.

> 
>>> 
>>> Moreover, this is still DT parsing and should find its place in
>>> is31fl319x_parse_dt().
>>> 
>>>> +			if (val > 40000)
>>>> +				val = 40000;
>>>> +			if (val < 5000)
>>>> +				val = 5000;
>>>> +			config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
>>>> +		}
>>>> +		if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
>>>> +					 &val)) {
>>> 
>>> Please move this to is31fl319x_parse_dt().
>> 
>> Well, the main reason is that this needs to introduce another temporary struct
>> to pass values from parse_dt to probe and copy values there.
> >
>> Would it be acceptable to inline all DT parsing?
> 
> Don't reinvent the wheel. Please compare how other LED class drivers
> perform DT parsing.

I have looked into other drivers but have not seen a common pattern. Some
have no DT (e.g. 88pm860x), some have some struct led_config_data (e.g. aat1290)
and some do it inlined in the probe function (e.g. bcm6328) and some do it
in a separate parse_dt (is31fl32).

So which driver would you recommend as best practice?

> 
>>> 
>>>> +			if (val > 21)
>>>> +				val = 21;
>>>> +			config2 |= val / 3; /* AGS */
>>>> +		}
>>>> +		regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>>> +		struct is31fl319x_led *l = is31->leds + i;
>>>> +
>>>> +		l->chip = is31;
>>>> +		if (leds[i].name && !leds[i].flags) {
>>>> +			l->led_cdev.name = leds[i].name;
>>>> +			l->led_cdev.default_trigger
>>>> +				= leds[i].default_trigger;
>>>> +			l->led_cdev.brightness_set_blocking
>>>> +				= is31fl319x_brightness_set;
>>>> +			/* NOTE: is31fl319x_brightness_set will be called
>>>> +			 * immediately after register() before we return
>>>> +			 */
>>> 
>>> Who will call it? I believe that this is true only if default-on
>>> trigger is registered.
>> 
>> Yes, as soon as anyone can call it we must be aware.
> 
> This is common for all LED class devices. Please drop the comment.

Ok. Might belong somewhere else (e.g. to devm_led_classdev_register
documentation).

> 
>>> 
>>>> +			err = devm_led_classdev_register(&client->dev,
>>>> +						    &l->led_cdev);
>>>> +			if (err < 0)
>>>> +				return err;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	dev_dbg(&client->dev, "probed\n");
>>> 
>>> Please remove this debug log. You can verify it by checking presence of
>>> the device in the sysfs.
>> 
>> Ok.
>> 
>>> 
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct i2c_driver is31fl319x_driver = {
>>>> +	.driver   = {
>>>> +		.name    = "leds-is31fl319x",
>>>> +		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),
>>>> +	},
>>>> +	.probe    = is31fl319x_probe,
>>>> +	.id_table = is31fl319x_id,
>>>> +};
>>>> +
>>>> +module_i2c_driver(is31fl319x_driver);
>>>> +
>>>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>>>> +MODULE_DESCRIPTION("IS31FL319X LED driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> 
>>> 
>>> 
>>> --
>>> Best regards,
>>> Jacek Anaszewski
>> 
>> Will fix asap.
>> 
>> Thanks and BR,
>> Nikolaus Schaller
>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> 
> 
> [1] https://lkml.org/lkml/2016/4/20/754
> 
> -- 
> Best regards,
> Jacek Anaszewski

BR and thanks,
Nikolaus

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[Jacek Anaszewski]

On 07/07/2016 03:00 PM, H. Nikolaus Schaller wrote:
> Hi Jacek,
>
>> Am 07.07.2016 um 13:32 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>
>> Hi Nikolaus,
>>
>> On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
>>> Hi Jacek,
>>>
>>>> Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>>>
>>>> Hi Nikolaus,
>>>>
>>>> Thanks for the updated version. Some comments below.
>>>>
>>>> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>>>>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>>>>> chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
>>>>> LEDs.
>>>>>
>>>>> Each LED is individually controllable in brightness (through pwm)
>>>>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>>>>>
>>>>> The maximum current of the LEDs can be programmed and limited to
>>>>> 5 .. 40mA through a device tree property.
>>>>>
>>>>> The chip is connected through I2C and can have one of 4 addresses
>>>>> in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
>>>>> address is defined by the reg property as usual.
>>>>>
>>>>> The chip also has a shutdown input which could be connected to a GPIO,
>>>>> but this driver uses software shutdown if all LEDs are inactivated.
>>>>>
>>>>> The chip also has breathing and audio features which are not fully
>>>>> supported by this driver.
>>>>>
>>>>> Tested-on: OMAP5 based Pyra handheld prototype.
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>> ---
>>>>>   drivers/leds/Kconfig           |   8 +
>>>>>   drivers/leds/Makefile          |   1 +
>>>>>   drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++
>>>>>   3 files changed, 363 insertions(+)
>>>>>   create mode 100644 drivers/leds/leds-is31fl319x.c
>>>>>
>>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>>> index 5ae2834..65b1045 100644
>>>>> --- a/drivers/leds/Kconfig
>>>>> +++ b/drivers/leds/Kconfig
>>>>> @@ -570,6 +570,14 @@ config LEDS_SEAD3
>>>>>   	  This driver can also be built as a module. If so the module
>>>>>   	  will be called leds-sead3.
>>>>>
>>>>> +config LEDS_IS31FL319X
>>>>> +	tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
>>>>> +	depends on LEDS_CLASS && I2C && OF
>>>>> +	help
>>>>> +	  This option enables support for LEDs connected to ISSI IS31FL319x
>>>>> +	  fancy LED driver chips accessed via the I2C bus.
>>>>> +	  Driver supports individual PWM brightness control for each channel.
>>>>> +
>>>>>   config LEDS_IS31FL32XX
>>>>>   	tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
>>>>>   	depends on LEDS_CLASS && I2C && OF
>>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>>> index cb2013d..b6ce9f9 100644
>>>>> --- a/drivers/leds/Makefile
>>>>> +++ b/drivers/leds/Makefile
>>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>>>>>   obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
>>>>>   obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>>>>>   obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
>>>>> +obj-$(CONFIG_LEDS_IS31FL319X)		+= leds-is31fl319x.o
>>>>>   obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
>>>>>
>>>>>   # LED SPI Drivers
>>>>> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
>>>>> new file mode 100644
>>>>> index 0000000..bbf8850
>>>>> --- /dev/null
>>>>> +++ b/drivers/leds/leds-is31fl319x.c
>>>>> @@ -0,0 +1,354 @@
>>>>> +/*
>>>>> + * Copyright 2015-16 Golden Delicious Computers
>>>>> + *
>>>>> + * Author: Nikolaus Schaller <hns@goldelico.com>
>>>>> + *
>>>>> + * Based on leds-tca6507.c
>>>>> + *
>>>>> + * This file is subject to the terms and conditions of version 2 of
>>>>> + * the GNU General Public License.  See the file COPYING in the main
>>>>> + * directory of this archive for more details.
>>>>> + *
>>>>> + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
>>>>> + * effect LEDs.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/i2c.h>
>>>>> +#include <linux/leds.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/regmap.h>
>>>>> +#include <linux/slab.h>
>>>>> +
>>>>> +/* register numbers */
>>>>> +#define IS31FL319X_SHUTDOWN	0x00
>>>>> +#define IS31FL319X_CTRL1	0x01
>>>>> +#define IS31FL319X_CTRL2	0x02
>>>>> +#define IS31FL319X_CONFIG1	0x03
>>>>> +#define IS31FL319X_CONFIG2	0x04
>>>>> +#define IS31FL319X_RAMP_MODE	0x05
>>>>> +#define IS31FL319X_BREATH_MASK	0x06
>>>>> +#define IS31FL319X_PWM1		0x07
>>>>> +#define IS31FL319X_PWM2		0x08
>>>>> +#define IS31FL319X_PWM3		0x09
>>>>> +#define IS31FL319X_PWM4		0x0a
>>>>> +#define IS31FL319X_PWM5		0x0b
>>>>> +#define IS31FL319X_PWM6		0x0c
>>>>> +#define IS31FL319X_PWM7		0x0d
>>>>> +#define IS31FL319X_PWM8		0x0e
>>>>> +#define IS31FL319X_PWM9		0x0f
>>>>> +#define IS31FL319X_DATA_UPDATE	0x10
>>>>> +#define IS31FL319X_T0_1		0x11
>>>>> +#define IS31FL319X_T0_2		0x12
>>>>> +#define IS31FL319X_T0_3		0x13
>>>>> +#define IS31FL319X_T0_4		0x14
>>>>> +#define IS31FL319X_T0_5		0x15
>>>>> +#define IS31FL319X_T0_6		0x16
>>>>> +#define IS31FL319X_T0_7		0x17
>>>>> +#define IS31FL319X_T0_8		0x18
>>>>> +#define IS31FL319X_T0_9		0x19
>>>>> +#define IS31FL319X_T123_1	0x1a
>>>>> +#define IS31FL319X_T123_2	0x1b
>>>>> +#define IS31FL319X_T123_3	0x1c
>>>>> +#define IS31FL319X_T4_1		0x1d
>>>>> +#define IS31FL319X_T4_2		0x1e
>>>>> +#define IS31FL319X_T4_3		0x1f
>>>>> +#define IS31FL319X_T4_4		0x20
>>>>> +#define IS31FL319X_T4_5		0x21
>>>>> +#define IS31FL319X_T4_6		0x22
>>>>> +#define IS31FL319X_T4_7		0x23
>>>>> +#define IS31FL319X_T4_8		0x24
>>>>> +#define IS31FL319X_T4_9		0x25
>>>>> +#define IS31FL319X_TIME_UPDATE	0x26
>>>>> +#define IS31FL319X_RESET	0xff
>>>>> +
>>>>> +#define IS31FL319X_REG_CNT	(IS31FL319X_RESET + 1)
>>>>> +
>>>>> +#define NUM_LEDS 9	/* max for 3199 chip */
>>>>> +
>>>>> +struct is31fl319x_chip {
>>>>> +	struct i2c_client	*client;
>>>>> +	struct regmap		*regmap;
>>>>> +
>>>>> +	struct is31fl319x_led {
>>>>> +		struct is31fl319x_chip	*chip;
>>>>> +		struct led_classdev	led_cdev;
>>>>> +	} leds[NUM_LEDS];
>>>>> +};
>>>>> +
>>>>> +static const struct i2c_device_id is31fl319x_id[] = {
>>>>> +	{ "is31fl3190", 1 },
>>>>> +	{ "is31fl3191", 1 },
>>>>> +	{ "is31fl3193", 3 },
>>>>> +	{ "is31fl3196", 6 },
>>>>> +	{ "is31fl3199", 9 },
>>>>> +	{ "sn3199", 9 },
>>>>> +	{ }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
>>>>> +
>>>>> +static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
>>>>> +				   enum led_brightness brightness)
>>>>> +{
>>>>> +	struct is31fl319x_led *led = container_of(led_cdev,
>>>>> +						  struct is31fl319x_led,
>>>>> +						  led_cdev);
>>>>> +	struct is31fl319x_chip *is31 = led->chip;
>>>>> +	int ret;
>>>>> +
>>>>> +	int i;
>>>>> +	u8 ctrl1, ctrl2;
>>>>
>>>> You can initialize these variables here.
>>>
>>> Ok.
>>>
>>>>
>>>>> +
>>>>> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
>>>>> +		brightness);
>>>>> +
>>>>> +	/* update PWM register */
>>>>> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
>>>>> +			   brightness);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	ctrl1 = 0;
>>>>> +	ctrl2 = 0;
>>>>
>>>> And remove above two lines.
>>>>
>>>>> +
>>>>> +	/* read current brightness of all PWM channels */
>>>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>>>> +		unsigned int pwm_value;
>>>>> +		bool on;
>>>>> +
>>>>> +		/*
>>>>> +		 * since neither led_cdev nor the chip can provide
>>>>> +		 * the current setting, we read from the regmap cache
>>>>> +		 */
>>>>> +
>>>>> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
>>>>> +				  &pwm_value);
>>>>> +
>>>>> +		dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
>>>>> +			__func__, i, ret, pwm_value);
>>>>> +		on = ret >= 0 && pwm_value > LED_OFF;
>>>>> +
>>>>> +		if (i < 3)
>>>>> +			ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
>>>>> +		else if (i < 6)
>>>>> +			ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
>>>>> +		else
>>>>> +			ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
>>>>> +	}
>>>>> +
>>>>> +
>>>>> +	if (ctrl1 > 0 || ctrl2 > 0) {
>>>>> +		dev_dbg(&is31->client->dev, "power up %02x %02x\n",
>>>>> +			ctrl1, ctrl2);
>>>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
>>>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
>>>>> +		/* update PWMs */
>>>>> +		regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
>>>>> +		/* enable chip from shut down */
>>>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
>>>>> +	} else {
>>>>> +		dev_dbg(&is31->client->dev, "power down\n");
>>>>> +		/* shut down (no need to clear CTRL1/2) */
>>>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
>>>>> +	}
>>>>
>>>> You need a mutex protection in this function, so as to prevent other
>>>> processes from jumping in in the middle of setting up a brightness,
>>>> which requires writing several registers.
>>>
>>> Uh?
>>> I thought that registering is31fl319x_brightness_set as
>>> led_cdev.brightness_set_blocking
>>> does the trick that we don't need a private mutex here because it
>>> can be called only once anyways. At least that was suggested during v1 review.
>>
>> Blocking doesn't mean that no one else can call the function in
>> the same time, but to the fact that the caller can be blocked
>> for the time needed for satisfying some condition. e.g. completion
>> of I2C transmission. It needs different treatment in the LED core,
>> as brightness setting can't sleep in general, due to the fact,
>> that it can be called from atomic context. In this case LED core
>> delegates brightness setting to a work queue task and
>> led_set_brightness() returns without blocking the caller.
>
> Ok. That is what I didn't think about: multiple LED triggers running
> in parallel.
>
> And with the new structure each one is independently calling set_brightness.
> i2c traffic has its mutex but not the step between writing and reading the PWM
> values to control chip shut down. The risk is that it is shut down in the wrong
> moment by a race.

Exactly.

>
> Thanks for spotting this.
>
>>
>> Excerpt from linux/leds.h:
>>
>> =================================================================
>>
>> /* Set LED brightness level
>> * Must not sleep. Use brightness_set_blocking for drivers
>> * that can sleep while setting brightness.
>> */
>> void            (*brightness_set)(struct led_classdev *led_cdev,
>>                                   enum led_brightness brightness);
>> /*
>> * Set LED brightness level immediately - it can block the caller for
>> * the time required for accessing a LED device register.
>> */
>> int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>>                                enum led_brightness brightness);
>>
>> =================================================================
>>
>>
>> In the [1] I suggested the following :
>>
>> "You can serialize the operations in brightness_set_blocking with
>> a mutex."
>>
>>>>
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static struct led_info *
>>>>> +is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
>>>>> +{
>>>>> +	struct device_node *np = client->dev.of_node, *child;
>>>>> +	struct led_info *is31_leds;
>>>>> +	int count;
>>>>> +
>>>>> +	if (!np)
>>>>> +		return ERR_PTR(-ENODEV);
>>>>> +
>>>>> +	count = of_get_child_count(np);
>>>>> +	dev_dbg(&client->dev, "child count %d\n", count);
>>>>> +	if (!count || count > NUM_LEDS)
>>>>> +		return ERR_PTR(-ENODEV);
>>>>> +
>>>>> +	is31_leds = devm_kzalloc(&client->dev,
>>>>> +			sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
>>>>> +	if (!is31_leds)
>>>>> +		return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> +	for_each_child_of_node(np, child) {
>>>>> +		struct led_info led;
>>>>
>>>> Using this local variable adds too much noise here.
>>>> I suggest avoiding the use of struct led_info. It is semantically
>>>> inconsistent in the context of DT parsing, since you don't have
>>>> any flags to parse.
>>>>
>>>>> +		u32 reg;
>>>>> +		int ret;
>>>>> +
>>>>> +		led.name = NULL;
>>>>> +		ret = of_property_read_string(child, "label", &led.name);
>>>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>>>> +			return ERR_PTR(ret);
>>>>
>>>> You could assign the name directly to related struct led_classdev
>>>> property. Moreover, if label is not present the child node name
>>>> should be used for LED class device name. Please refer to the other
>>>> drivers and common LED bindings.
>>>
>>> Ok.
>>>
>>>>
>>>>> +		led.default_trigger = NULL;
>>>>> +		ret = of_property_read_string(child, "linux,default-trigger",
>>>>> +			&led.default_trigger);
>>>>
>>>> Here also please assign the trigger directly to struct led_classdev.
>>>
>>> Ok.
>>>
>>>>
>>>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>>>> +			return ERR_PTR(ret);
>>>>> +		led.flags = 0;
>>>>> +		ret = of_property_read_u32(child, "reg", &reg);
>>>>> +		dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
>>>>> +		reg -= 1;	/* index 0 is at reg = 1 */
>>>>> +		if (ret != 0 || reg < 0 || reg >= num_leds)
>>>>> +			continue;
>>>>> +
>>>>> +		if (is31_leds[reg].name)
>>>>> +			dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
>>>>> +				reg, is31_leds[reg].name, led.name);
>>>>> +		is31_leds[reg] = led;
>>>>> +	}
>>>>> +
>>>>> +	return is31_leds;
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id of_is31fl319x_leds_match[] = {
>>>>> +	{ .compatible = "issi,is31fl3190", (void *) 1 },
>>>>> +	{ .compatible = "issi,is31fl3191", (void *) 1 },
>>>>> +	{ .compatible = "issi,is31fl3193", (void *) 3 },
>>>>> +	{ .compatible = "issi,is31fl3196", (void *) 6 },
>>>>> +	{ .compatible = "issi,is31fl3199", (void *) 9 },
>>>>> +	{ .compatible = "si-en,sn3199", (void *) 9 },
>>>>> +	{},
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
>>>>> +
>>>>> +static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
>>>>> +{ /* we have no readable registers */
>>>>> +	return false;
>>>>> +}
>>>>> +
>>>>> +static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
>>>>> +{ /* volatile registers are not cached */
>>>>> +	switch (reg) {
>>>>> +	case IS31FL319X_DATA_UPDATE:
>>>>> +	case IS31FL319X_TIME_UPDATE:
>>>>> +	case IS31FL319X_RESET:
>>>>> +		return true;	/* always write-through */
>>>>> +	default:
>>>>> +		return false;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +struct regmap_config regmap_config = {
>>>>> +	.reg_bits = 8,
>>>>> +	.val_bits = 8,
>>>>> +	.max_register = IS31FL319X_REG_CNT,
>>>>> +	.cache_type = REGCACHE_FLAT,
>>>>> +	.readable_reg = is31fl319x_readable_reg,
>>>>> +	.volatile_reg = is31fl319x_volatile_reg,
>>>>> +};
>>>>> +
>>>>> +static int is31fl319x_probe(struct i2c_client *client,
>>>>> +		const struct i2c_device_id *id)
>>>>> +{
>>>>> +	struct is31fl319x_chip *is31;
>>>>> +	struct i2c_adapter *adapter;
>>>>> +	struct led_info *leds;
>>>>> +	int err;
>>>>> +	int i = 0;
>>>>> +
>>>>> +	adapter = to_i2c_adapter(client->dev.parent);
>>>>> +
>>>>> +	dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
>>>>> +		(int) id->driver_data);
>>>>> +
>>>>> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>>>>> +		return -EIO;
>>>>> +
>>>>> +	leds = is31fl319x_parse_dt(client, (int) id->driver_data);
>>>>> +	if (IS_ERR(leds)) {
>>>>> +		dev_err(&client->dev, "DT parsing error %d\n",
>>>>> +			(int) PTR_ERR(leds));
>>>>> +		return PTR_ERR(leds);
>>>>> +	}
>>>>> +
>>>>> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
>>>>> +	if (!is31)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	is31->client = client;
>>>>> +	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
>>>>> +	if (IS_ERR(is31->regmap)) {
>>>>> +		dev_err(&client->dev, "failed to allocate register map\n");
>>>>> +		return PTR_ERR(is31->regmap);
>>>>> +	}
>>>>> +
>>>>> +	i2c_set_clientdata(client, is31);
>>>>> +
>>>>> +	/* check for write-reply from chip (we can't read any registers) */
>>>>> +	err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
>>>>
>>>> Doesn't powering the device up result in loading registers with default
>>>> values?
>>>
>>> Yes, but not a reboot.
>>>
>>> And we have to write *some* register anyways here to be able to detect
>>> if the chip is present or does not respond.
>>>
>>> So what would writing a different register change?
>>
>> OK, let's leave that as is.
>>
>>>>
>>>>> +	if (err < 0) {
>>>>> +		dev_err(&client->dev, "no response from chip write: err = %d\n",
>>>>> +			err);
>>>>> +		return -EIO;	/* does not answer */
>>>>> +	}
>>>>> +
>>>>> +	/* initialize chip and regmap so that we never try to read from i2c */
>>>>
>>>> You can initialize default register values without writing them,
>>>> by using regmap's reg_defaults property.
>>>
>>> Hm,
>>>
>>>>
>>>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
>>>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
>>>>> +	for (i = 0; i < NUM_LEDS; i++)
>>>>> +		regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
>>>
>>> would replace a small loop with a bigger table. But if you prefer this I can change.
>>
>> Writing registers only to initialize regmap cache looks odd.
>> Moreover it unnecessarily extends driver probing time.
>
> Ok, we will change.
>
>>
>>>>> +
>>>>> +	if (client->dev.of_node) {
>>>>> +		u32 val;
>>>>> +		u8 config2 = 0;
>>>>> +
>>>>> +		if (of_property_read_u32(client->dev.of_node,
>>>>> +					 "led-max-microamp", &val)) {
>>>>
>>>> of_property_read_u32 returns 0 on success.
>>>
>>> Ok.
>>>
>>>>
>>>> led-max-microamp needs to be defined per each sub-LED (child node).
>>>> The greatest value should be used for setting CS bit field of CTRL2
>>>> register, and max_brightness values of the LEDs with lower max microamp
>>>> value should be limited accordingly.
>>>
>>> Hm. The chip does not allow to limit individual LEDs. So we should take the
>>> *minimum* of all subnodes.
>>
>> But the LED core does. That's a max_brightness property is for.
>
> Ok, that seems to be a new feature we have no use case for since all our
> LEDs have the same current specification. But we will check how it can be done.

max_brightness property and corresponding sysfs attribute were added
in 2009. See:

1bd465e6b0 ("leds: allow led-drivers to use a variable range of 
brightness values")

>>
>>>>
>>>> Moreover, this is still DT parsing and should find its place in
>>>> is31fl319x_parse_dt().
>>>>
>>>>> +			if (val > 40000)
>>>>> +				val = 40000;
>>>>> +			if (val < 5000)
>>>>> +				val = 5000;
>>>>> +			config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
>>>>> +		}
>>>>> +		if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
>>>>> +					 &val)) {
>>>>
>>>> Please move this to is31fl319x_parse_dt().
>>>
>>> Well, the main reason is that this needs to introduce another temporary struct
>>> to pass values from parse_dt to probe and copy values there.
>>>
>>> Would it be acceptable to inline all DT parsing?
>>
>> Don't reinvent the wheel. Please compare how other LED class drivers
>> perform DT parsing.
>
> I have looked into other drivers but have not seen a common pattern. Some
> have no DT (e.g. 88pm860x), some have some struct led_config_data (e.g. aat1290)
> and some do it inlined in the probe function (e.g. bcm6328) and some do it
> in a separate parse_dt (is31fl32).
>
> So which driver would you recommend as best practice?

E.g. drivers/leds/leds-is31fl32xx.c

>>
>>>>
>>>>> +			if (val > 21)
>>>>> +				val = 21;
>>>>> +			config2 |= val / 3; /* AGS */
>>>>> +		}
>>>>> +		regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>>>> +		struct is31fl319x_led *l = is31->leds + i;
>>>>> +
>>>>> +		l->chip = is31;
>>>>> +		if (leds[i].name && !leds[i].flags) {
>>>>> +			l->led_cdev.name = leds[i].name;
>>>>> +			l->led_cdev.default_trigger
>>>>> +				= leds[i].default_trigger;
>>>>> +			l->led_cdev.brightness_set_blocking
>>>>> +				= is31fl319x_brightness_set;
>>>>> +			/* NOTE: is31fl319x_brightness_set will be called
>>>>> +			 * immediately after register() before we return
>>>>> +			 */
>>>>
>>>> Who will call it? I believe that this is true only if default-on
>>>> trigger is registered.
>>>
>>> Yes, as soon as anyone can call it we must be aware.
>>
>> This is common for all LED class devices. Please drop the comment.
>
> Ok. Might belong somewhere else (e.g. to devm_led_classdev_register
> documentation).
>
>>
>>>>
>>>>> +			err = devm_led_classdev_register(&client->dev,
>>>>> +						    &l->led_cdev);
>>>>> +			if (err < 0)
>>>>> +				return err;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	dev_dbg(&client->dev, "probed\n");
>>>>
>>>> Please remove this debug log. You can verify it by checking presence of
>>>> the device in the sysfs.
>>>
>>> Ok.
>>>
>>>>
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static struct i2c_driver is31fl319x_driver = {
>>>>> +	.driver   = {
>>>>> +		.name    = "leds-is31fl319x",
>>>>> +		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),
>>>>> +	},
>>>>> +	.probe    = is31fl319x_probe,
>>>>> +	.id_table = is31fl319x_id,
>>>>> +};
>>>>> +
>>>>> +module_i2c_driver(is31fl319x_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>>>>> +MODULE_DESCRIPTION("IS31FL319X LED driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>>
>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Jacek Anaszewski
>>>
>>> Will fix asap.
>>>
>>> Thanks and BR,
>>> Nikolaus Schaller
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>> [1] https://lkml.org/lkml/2016/4/20/754
>>
>> --
>> Best regards,
>> Jacek Anaszewski
>
> BR and thanks,
> Nikolaus
>
>
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[Jacek Anaszewski]

On 07/07/2016 03:00 PM, H. Nikolaus Schaller wrote:
> Hi Jacek,
>
>> Am 07.07.2016 um 13:32 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>
>> Hi Nikolaus,
>>
>> On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
>>> Hi Jacek,
>>>
>>>> Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>>>
>>>> Hi Nikolaus,
>>>>
>>>> Thanks for the updated version. Some comments below.
>>>>
>>>> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>>>>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>>>>> chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
>>>>> LEDs.
>>>>>
>>>>> Each LED is individually controllable in brightness (through pwm)
>>>>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>>>>>
>>>>> The maximum current of the LEDs can be programmed and limited to
>>>>> 5 .. 40mA through a device tree property.
>>>>>
>>>>> The chip is connected through I2C and can have one of 4 addresses
>>>>> in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
>>>>> address is defined by the reg property as usual.
>>>>>
>>>>> The chip also has a shutdown input which could be connected to a GPIO,
>>>>> but this driver uses software shutdown if all LEDs are inactivated.
>>>>>
>>>>> The chip also has breathing and audio features which are not fully
>>>>> supported by this driver.
>>>>>
>>>>> Tested-on: OMAP5 based Pyra handheld prototype.
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>> ---
>>>>>   drivers/leds/Kconfig           |   8 +
>>>>>   drivers/leds/Makefile          |   1 +
>>>>>   drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++
>>>>>   3 files changed, 363 insertions(+)
>>>>>   create mode 100644 drivers/leds/leds-is31fl319x.c
>>>>>
>>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>>> index 5ae2834..65b1045 100644
>>>>> --- a/drivers/leds/Kconfig
>>>>> +++ b/drivers/leds/Kconfig
>>>>> @@ -570,6 +570,14 @@ config LEDS_SEAD3
>>>>>   	  This driver can also be built as a module. If so the module
>>>>>   	  will be called leds-sead3.
>>>>>
>>>>> +config LEDS_IS31FL319X
>>>>> +	tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
>>>>> +	depends on LEDS_CLASS && I2C && OF
>>>>> +	help
>>>>> +	  This option enables support for LEDs connected to ISSI IS31FL319x
>>>>> +	  fancy LED driver chips accessed via the I2C bus.
>>>>> +	  Driver supports individual PWM brightness control for each channel.
>>>>> +
>>>>>   config LEDS_IS31FL32XX
>>>>>   	tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
>>>>>   	depends on LEDS_CLASS && I2C && OF
>>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>>> index cb2013d..b6ce9f9 100644
>>>>> --- a/drivers/leds/Makefile
>>>>> +++ b/drivers/leds/Makefile
>>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>>>>>   obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
>>>>>   obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>>>>>   obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
>>>>> +obj-$(CONFIG_LEDS_IS31FL319X)		+= leds-is31fl319x.o
>>>>>   obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
>>>>>
>>>>>   # LED SPI Drivers
>>>>> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
>>>>> new file mode 100644
>>>>> index 0000000..bbf8850
>>>>> --- /dev/null
>>>>> +++ b/drivers/leds/leds-is31fl319x.c
>>>>> @@ -0,0 +1,354 @@
>>>>> +/*
>>>>> + * Copyright 2015-16 Golden Delicious Computers
>>>>> + *
>>>>> + * Author: Nikolaus Schaller <hns@goldelico.com>
>>>>> + *
>>>>> + * Based on leds-tca6507.c
>>>>> + *
>>>>> + * This file is subject to the terms and conditions of version 2 of
>>>>> + * the GNU General Public License.  See the file COPYING in the main
>>>>> + * directory of this archive for more details.
>>>>> + *
>>>>> + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
>>>>> + * effect LEDs.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/i2c.h>
>>>>> +#include <linux/leds.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/regmap.h>
>>>>> +#include <linux/slab.h>
>>>>> +
>>>>> +/* register numbers */
>>>>> +#define IS31FL319X_SHUTDOWN	0x00
>>>>> +#define IS31FL319X_CTRL1	0x01
>>>>> +#define IS31FL319X_CTRL2	0x02
>>>>> +#define IS31FL319X_CONFIG1	0x03
>>>>> +#define IS31FL319X_CONFIG2	0x04
>>>>> +#define IS31FL319X_RAMP_MODE	0x05
>>>>> +#define IS31FL319X_BREATH_MASK	0x06
>>>>> +#define IS31FL319X_PWM1		0x07
>>>>> +#define IS31FL319X_PWM2		0x08
>>>>> +#define IS31FL319X_PWM3		0x09
>>>>> +#define IS31FL319X_PWM4		0x0a
>>>>> +#define IS31FL319X_PWM5		0x0b
>>>>> +#define IS31FL319X_PWM6		0x0c
>>>>> +#define IS31FL319X_PWM7		0x0d
>>>>> +#define IS31FL319X_PWM8		0x0e
>>>>> +#define IS31FL319X_PWM9		0x0f
>>>>> +#define IS31FL319X_DATA_UPDATE	0x10
>>>>> +#define IS31FL319X_T0_1		0x11
>>>>> +#define IS31FL319X_T0_2		0x12
>>>>> +#define IS31FL319X_T0_3		0x13
>>>>> +#define IS31FL319X_T0_4		0x14
>>>>> +#define IS31FL319X_T0_5		0x15
>>>>> +#define IS31FL319X_T0_6		0x16
>>>>> +#define IS31FL319X_T0_7		0x17
>>>>> +#define IS31FL319X_T0_8		0x18
>>>>> +#define IS31FL319X_T0_9		0x19
>>>>> +#define IS31FL319X_T123_1	0x1a
>>>>> +#define IS31FL319X_T123_2	0x1b
>>>>> +#define IS31FL319X_T123_3	0x1c
>>>>> +#define IS31FL319X_T4_1		0x1d
>>>>> +#define IS31FL319X_T4_2		0x1e
>>>>> +#define IS31FL319X_T4_3		0x1f
>>>>> +#define IS31FL319X_T4_4		0x20
>>>>> +#define IS31FL319X_T4_5		0x21
>>>>> +#define IS31FL319X_T4_6		0x22
>>>>> +#define IS31FL319X_T4_7		0x23
>>>>> +#define IS31FL319X_T4_8		0x24
>>>>> +#define IS31FL319X_T4_9		0x25
>>>>> +#define IS31FL319X_TIME_UPDATE	0x26
>>>>> +#define IS31FL319X_RESET	0xff
>>>>> +
>>>>> +#define IS31FL319X_REG_CNT	(IS31FL319X_RESET + 1)
>>>>> +
>>>>> +#define NUM_LEDS 9	/* max for 3199 chip */
>>>>> +
>>>>> +struct is31fl319x_chip {
>>>>> +	struct i2c_client	*client;
>>>>> +	struct regmap		*regmap;
>>>>> +
>>>>> +	struct is31fl319x_led {
>>>>> +		struct is31fl319x_chip	*chip;
>>>>> +		struct led_classdev	led_cdev;
>>>>> +	} leds[NUM_LEDS];
>>>>> +};
>>>>> +
>>>>> +static const struct i2c_device_id is31fl319x_id[] = {
>>>>> +	{ "is31fl3190", 1 },
>>>>> +	{ "is31fl3191", 1 },
>>>>> +	{ "is31fl3193", 3 },
>>>>> +	{ "is31fl3196", 6 },
>>>>> +	{ "is31fl3199", 9 },
>>>>> +	{ "sn3199", 9 },
>>>>> +	{ }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
>>>>> +
>>>>> +static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
>>>>> +				   enum led_brightness brightness)
>>>>> +{
>>>>> +	struct is31fl319x_led *led = container_of(led_cdev,
>>>>> +						  struct is31fl319x_led,
>>>>> +						  led_cdev);
>>>>> +	struct is31fl319x_chip *is31 = led->chip;
>>>>> +	int ret;
>>>>> +
>>>>> +	int i;
>>>>> +	u8 ctrl1, ctrl2;
>>>>
>>>> You can initialize these variables here.
>>>
>>> Ok.
>>>
>>>>
>>>>> +
>>>>> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
>>>>> +		brightness);
>>>>> +
>>>>> +	/* update PWM register */
>>>>> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
>>>>> +			   brightness);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	ctrl1 = 0;
>>>>> +	ctrl2 = 0;
>>>>
>>>> And remove above two lines.
>>>>
>>>>> +
>>>>> +	/* read current brightness of all PWM channels */
>>>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>>>> +		unsigned int pwm_value;
>>>>> +		bool on;
>>>>> +
>>>>> +		/*
>>>>> +		 * since neither led_cdev nor the chip can provide
>>>>> +		 * the current setting, we read from the regmap cache
>>>>> +		 */
>>>>> +
>>>>> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
>>>>> +				  &pwm_value);
>>>>> +
>>>>> +		dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
>>>>> +			__func__, i, ret, pwm_value);
>>>>> +		on = ret >= 0 && pwm_value > LED_OFF;
>>>>> +
>>>>> +		if (i < 3)
>>>>> +			ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
>>>>> +		else if (i < 6)
>>>>> +			ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
>>>>> +		else
>>>>> +			ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
>>>>> +	}
>>>>> +
>>>>> +
>>>>> +	if (ctrl1 > 0 || ctrl2 > 0) {
>>>>> +		dev_dbg(&is31->client->dev, "power up %02x %02x\n",
>>>>> +			ctrl1, ctrl2);
>>>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
>>>>> +		regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
>>>>> +		/* update PWMs */
>>>>> +		regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
>>>>> +		/* enable chip from shut down */
>>>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
>>>>> +	} else {
>>>>> +		dev_dbg(&is31->client->dev, "power down\n");
>>>>> +		/* shut down (no need to clear CTRL1/2) */
>>>>> +		ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
>>>>> +	}
>>>>
>>>> You need a mutex protection in this function, so as to prevent other
>>>> processes from jumping in in the middle of setting up a brightness,
>>>> which requires writing several registers.
>>>
>>> Uh?
>>> I thought that registering is31fl319x_brightness_set as
>>> led_cdev.brightness_set_blocking
>>> does the trick that we don't need a private mutex here because it
>>> can be called only once anyways. At least that was suggested during v1 review.
>>
>> Blocking doesn't mean that no one else can call the function in
>> the same time, but to the fact that the caller can be blocked
>> for the time needed for satisfying some condition. e.g. completion
>> of I2C transmission. It needs different treatment in the LED core,
>> as brightness setting can't sleep in general, due to the fact,
>> that it can be called from atomic context. In this case LED core
>> delegates brightness setting to a work queue task and
>> led_set_brightness() returns without blocking the caller.
>
> Ok. That is what I didn't think about: multiple LED triggers running
> in parallel.
>
> And with the new structure each one is independently calling set_brightness.
> i2c traffic has its mutex but not the step between writing and reading the PWM
> values to control chip shut down. The risk is that it is shut down in the wrong
> moment by a race.

Exactly.

>
> Thanks for spotting this.
>
>>
>> Excerpt from linux/leds.h:
>>
>> =================================================================
>>
>> /* Set LED brightness level
>> * Must not sleep. Use brightness_set_blocking for drivers
>> * that can sleep while setting brightness.
>> */
>> void            (*brightness_set)(struct led_classdev *led_cdev,
>>                                   enum led_brightness brightness);
>> /*
>> * Set LED brightness level immediately - it can block the caller for
>> * the time required for accessing a LED device register.
>> */
>> int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>>                                enum led_brightness brightness);
>>
>> =================================================================
>>
>>
>> In the [1] I suggested the following :
>>
>> "You can serialize the operations in brightness_set_blocking with
>> a mutex."
>>
>>>>
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static struct led_info *
>>>>> +is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
>>>>> +{
>>>>> +	struct device_node *np = client->dev.of_node, *child;
>>>>> +	struct led_info *is31_leds;
>>>>> +	int count;
>>>>> +
>>>>> +	if (!np)
>>>>> +		return ERR_PTR(-ENODEV);
>>>>> +
>>>>> +	count = of_get_child_count(np);
>>>>> +	dev_dbg(&client->dev, "child count %d\n", count);
>>>>> +	if (!count || count > NUM_LEDS)
>>>>> +		return ERR_PTR(-ENODEV);
>>>>> +
>>>>> +	is31_leds = devm_kzalloc(&client->dev,
>>>>> +			sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
>>>>> +	if (!is31_leds)
>>>>> +		return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> +	for_each_child_of_node(np, child) {
>>>>> +		struct led_info led;
>>>>
>>>> Using this local variable adds too much noise here.
>>>> I suggest avoiding the use of struct led_info. It is semantically
>>>> inconsistent in the context of DT parsing, since you don't have
>>>> any flags to parse.
>>>>
>>>>> +		u32 reg;
>>>>> +		int ret;
>>>>> +
>>>>> +		led.name = NULL;
>>>>> +		ret = of_property_read_string(child, "label", &led.name);
>>>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>>>> +			return ERR_PTR(ret);
>>>>
>>>> You could assign the name directly to related struct led_classdev
>>>> property. Moreover, if label is not present the child node name
>>>> should be used for LED class device name. Please refer to the other
>>>> drivers and common LED bindings.
>>>
>>> Ok.
>>>
>>>>
>>>>> +		led.default_trigger = NULL;
>>>>> +		ret = of_property_read_string(child, "linux,default-trigger",
>>>>> +			&led.default_trigger);
>>>>
>>>> Here also please assign the trigger directly to struct led_classdev.
>>>
>>> Ok.
>>>
>>>>
>>>>> +		if (ret < 0 && ret != -EINVAL)	/* is optional */
>>>>> +			return ERR_PTR(ret);
>>>>> +		led.flags = 0;
>>>>> +		ret = of_property_read_u32(child, "reg", &reg);
>>>>> +		dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
>>>>> +		reg -= 1;	/* index 0 is at reg = 1 */
>>>>> +		if (ret != 0 || reg < 0 || reg >= num_leds)
>>>>> +			continue;
>>>>> +
>>>>> +		if (is31_leds[reg].name)
>>>>> +			dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
>>>>> +				reg, is31_leds[reg].name, led.name);
>>>>> +		is31_leds[reg] = led;
>>>>> +	}
>>>>> +
>>>>> +	return is31_leds;
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id of_is31fl319x_leds_match[] = {
>>>>> +	{ .compatible = "issi,is31fl3190", (void *) 1 },
>>>>> +	{ .compatible = "issi,is31fl3191", (void *) 1 },
>>>>> +	{ .compatible = "issi,is31fl3193", (void *) 3 },
>>>>> +	{ .compatible = "issi,is31fl3196", (void *) 6 },
>>>>> +	{ .compatible = "issi,is31fl3199", (void *) 9 },
>>>>> +	{ .compatible = "si-en,sn3199", (void *) 9 },
>>>>> +	{},
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
>>>>> +
>>>>> +static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
>>>>> +{ /* we have no readable registers */
>>>>> +	return false;
>>>>> +}
>>>>> +
>>>>> +static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
>>>>> +{ /* volatile registers are not cached */
>>>>> +	switch (reg) {
>>>>> +	case IS31FL319X_DATA_UPDATE:
>>>>> +	case IS31FL319X_TIME_UPDATE:
>>>>> +	case IS31FL319X_RESET:
>>>>> +		return true;	/* always write-through */
>>>>> +	default:
>>>>> +		return false;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +struct regmap_config regmap_config = {
>>>>> +	.reg_bits = 8,
>>>>> +	.val_bits = 8,
>>>>> +	.max_register = IS31FL319X_REG_CNT,
>>>>> +	.cache_type = REGCACHE_FLAT,
>>>>> +	.readable_reg = is31fl319x_readable_reg,
>>>>> +	.volatile_reg = is31fl319x_volatile_reg,
>>>>> +};
>>>>> +
>>>>> +static int is31fl319x_probe(struct i2c_client *client,
>>>>> +		const struct i2c_device_id *id)
>>>>> +{
>>>>> +	struct is31fl319x_chip *is31;
>>>>> +	struct i2c_adapter *adapter;
>>>>> +	struct led_info *leds;
>>>>> +	int err;
>>>>> +	int i = 0;
>>>>> +
>>>>> +	adapter = to_i2c_adapter(client->dev.parent);
>>>>> +
>>>>> +	dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
>>>>> +		(int) id->driver_data);
>>>>> +
>>>>> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>>>>> +		return -EIO;
>>>>> +
>>>>> +	leds = is31fl319x_parse_dt(client, (int) id->driver_data);
>>>>> +	if (IS_ERR(leds)) {
>>>>> +		dev_err(&client->dev, "DT parsing error %d\n",
>>>>> +			(int) PTR_ERR(leds));
>>>>> +		return PTR_ERR(leds);
>>>>> +	}
>>>>> +
>>>>> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
>>>>> +	if (!is31)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	is31->client = client;
>>>>> +	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
>>>>> +	if (IS_ERR(is31->regmap)) {
>>>>> +		dev_err(&client->dev, "failed to allocate register map\n");
>>>>> +		return PTR_ERR(is31->regmap);
>>>>> +	}
>>>>> +
>>>>> +	i2c_set_clientdata(client, is31);
>>>>> +
>>>>> +	/* check for write-reply from chip (we can't read any registers) */
>>>>> +	err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
>>>>
>>>> Doesn't powering the device up result in loading registers with default
>>>> values?
>>>
>>> Yes, but not a reboot.
>>>
>>> And we have to write *some* register anyways here to be able to detect
>>> if the chip is present or does not respond.
>>>
>>> So what would writing a different register change?
>>
>> OK, let's leave that as is.
>>
>>>>
>>>>> +	if (err < 0) {
>>>>> +		dev_err(&client->dev, "no response from chip write: err = %d\n",
>>>>> +			err);
>>>>> +		return -EIO;	/* does not answer */
>>>>> +	}
>>>>> +
>>>>> +	/* initialize chip and regmap so that we never try to read from i2c */
>>>>
>>>> You can initialize default register values without writing them,
>>>> by using regmap's reg_defaults property.
>>>
>>> Hm,
>>>
>>>>
>>>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
>>>>> +	regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
>>>>> +	for (i = 0; i < NUM_LEDS; i++)
>>>>> +		regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
>>>
>>> would replace a small loop with a bigger table. But if you prefer this I can change.
>>
>> Writing registers only to initialize regmap cache looks odd.
>> Moreover it unnecessarily extends driver probing time.
>
> Ok, we will change.
>
>>
>>>>> +
>>>>> +	if (client->dev.of_node) {
>>>>> +		u32 val;
>>>>> +		u8 config2 = 0;
>>>>> +
>>>>> +		if (of_property_read_u32(client->dev.of_node,
>>>>> +					 "led-max-microamp", &val)) {
>>>>
>>>> of_property_read_u32 returns 0 on success.
>>>
>>> Ok.
>>>
>>>>
>>>> led-max-microamp needs to be defined per each sub-LED (child node).
>>>> The greatest value should be used for setting CS bit field of CTRL2
>>>> register, and max_brightness values of the LEDs with lower max microamp
>>>> value should be limited accordingly.
>>>
>>> Hm. The chip does not allow to limit individual LEDs. So we should take the
>>> *minimum* of all subnodes.
>>
>> But the LED core does. That's a max_brightness property is for.
>
> Ok, that seems to be a new feature we have no use case for since all our
> LEDs have the same current specification. But we will check how it can be done.

max_brightness property and corresponding sysfs attribute were added
in 2009. See:

1bd465e6b0 ("leds: allow led-drivers to use a variable range of 
brightness values")

>>
>>>>
>>>> Moreover, this is still DT parsing and should find its place in
>>>> is31fl319x_parse_dt().
>>>>
>>>>> +			if (val > 40000)
>>>>> +				val = 40000;
>>>>> +			if (val < 5000)
>>>>> +				val = 5000;
>>>>> +			config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
>>>>> +		}
>>>>> +		if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
>>>>> +					 &val)) {
>>>>
>>>> Please move this to is31fl319x_parse_dt().
>>>
>>> Well, the main reason is that this needs to introduce another temporary struct
>>> to pass values from parse_dt to probe and copy values there.
>>>
>>> Would it be acceptable to inline all DT parsing?
>>
>> Don't reinvent the wheel. Please compare how other LED class drivers
>> perform DT parsing.
>
> I have looked into other drivers but have not seen a common pattern. Some
> have no DT (e.g. 88pm860x), some have some struct led_config_data (e.g. aat1290)
> and some do it inlined in the probe function (e.g. bcm6328) and some do it
> in a separate parse_dt (is31fl32).
>
> So which driver would you recommend as best practice?

E.g. drivers/leds/leds-is31fl32xx.c

>>
>>>>
>>>>> +			if (val > 21)
>>>>> +				val = 21;
>>>>> +			config2 |= val / 3; /* AGS */
>>>>> +		}
>>>>> +		regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < NUM_LEDS; i++) {
>>>>> +		struct is31fl319x_led *l = is31->leds + i;
>>>>> +
>>>>> +		l->chip = is31;
>>>>> +		if (leds[i].name && !leds[i].flags) {
>>>>> +			l->led_cdev.name = leds[i].name;
>>>>> +			l->led_cdev.default_trigger
>>>>> +				= leds[i].default_trigger;
>>>>> +			l->led_cdev.brightness_set_blocking
>>>>> +				= is31fl319x_brightness_set;
>>>>> +			/* NOTE: is31fl319x_brightness_set will be called
>>>>> +			 * immediately after register() before we return
>>>>> +			 */
>>>>
>>>> Who will call it? I believe that this is true only if default-on
>>>> trigger is registered.
>>>
>>> Yes, as soon as anyone can call it we must be aware.
>>
>> This is common for all LED class devices. Please drop the comment.
>
> Ok. Might belong somewhere else (e.g. to devm_led_classdev_register
> documentation).
>
>>
>>>>
>>>>> +			err = devm_led_classdev_register(&client->dev,
>>>>> +						    &l->led_cdev);
>>>>> +			if (err < 0)
>>>>> +				return err;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	dev_dbg(&client->dev, "probed\n");
>>>>
>>>> Please remove this debug log. You can verify it by checking presence of
>>>> the device in the sysfs.
>>>
>>> Ok.
>>>
>>>>
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static struct i2c_driver is31fl319x_driver = {
>>>>> +	.driver   = {
>>>>> +		.name    = "leds-is31fl319x",
>>>>> +		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),
>>>>> +	},
>>>>> +	.probe    = is31fl319x_probe,
>>>>> +	.id_table = is31fl319x_id,
>>>>> +};
>>>>> +
>>>>> +module_i2c_driver(is31fl319x_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>>>>> +MODULE_DESCRIPTION("IS31FL319X LED driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>>
>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Jacek Anaszewski
>>>
>>> Will fix asap.
>>>
>>> Thanks and BR,
>>> Nikolaus Schaller
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>> [1] https://lkml.org/lkml/2016/4/20/754
>>
>> --
>> Best regards,
>> Jacek Anaszewski
>
> BR and thanks,
> Nikolaus
>
>
>
>


-- 
Best regards,
Jacek Anaszewski