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

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

[H. Nikolaus Schaller]

Changes V3 (mostly done by Andrey Utkin):
* general coding style improvements
* added a mutex to properly serialize multiple calls to set_backlight
* use regmap defaults (suggested by Jacek Anaszewski)
* improve DT parsing (suggested by Jacek Anaszewski)
* define dependency on REGMAP_I2C
* minor code improvements

2016-07-06 12:02:47: 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                               |  12 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-is31fl319x.c                     | 405 +++++++++++++++++++++
 4 files changed, 470 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
 create mode 100644 drivers/leds/leds-is31fl319x.c

-- 
2.7.3

[PATCH v3 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>
Signed-off-by: Andrey Utkin <andrey_utkin@fastmail.com>
---
 drivers/leds/Kconfig           |  12 ++
 drivers/leds/Makefile          |   1 +
 drivers/leds/leds-is31fl319x.c | 405 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 418 insertions(+)
 create mode 100644 drivers/leds/leds-is31fl319x.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 5ae2834..6439a7d 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -570,6 +570,18 @@ 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
+	select REGMAP_I2C
+	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.
+
+	  This driver can also be built as a module. If so the module will be
+	  called leds-is31fl319x.
+
 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..7fc5c6ab
--- /dev/null
+++ b/drivers/leds/leds-is31fl319x.c
@@ -0,0 +1,405 @@
+/*
+ * 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 IS31FL319{0,1,3,6,9} 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_PWM(channel) (0x07 + channel)
+#define IS31FL319X_DATA_UPDATE  0x10
+#define IS31FL319X_T0(channel)  (0x11 + channel)
+#define IS31FL319X_T123_1       0x1a
+#define IS31FL319X_T123_2       0x1b
+#define IS31FL319X_T123_3       0x1c
+#define IS31FL319X_T4(channel)  (0x1d + channel)
+#define IS31FL319X_TIME_UPDATE  0x26
+#define IS31FL319X_RESET        0xff
+
+#define IS31FL319X_REG_CNT      (IS31FL319X_RESET + 1)
+
+#define NUM_LEDS 9 /* max for 3199 chip */
+
+#define LED_MAX_MICROAMP_UPPER_LIMIT ((u32) 40000)
+#define LED_MAX_MICROAMP_LOWER_LIMIT ((u32) 5000)
+#define LED_MAX_MICROAMP_DEFAULT ((u32) 20000)
+#define LED_MAX_MICROAMP_STEP ((u32) 5000)
+
+#define AUDIO_GAIN_DB_MAX ((u32) 21)
+
+/*
+ * regmap is used as a cache of chip's register space,
+ * to avoid reading back brightness values from chip,
+ * which is known to hang.
+ */
+struct is31fl319x_chip {
+	struct i2c_client       *client;
+	struct regmap           *regmap;
+	struct mutex            lock;
+	u32                     audio_gain_db;
+
+	struct is31fl319x_led {
+		struct is31fl319x_chip  *chip;
+		struct led_classdev     cdev;
+		u32                     max_microamp;
+		bool                    configured;
+	} 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 *cdev,
+				   enum led_brightness brightness)
+{
+	struct is31fl319x_led *led = container_of(cdev, struct is31fl319x_led,
+						  cdev);
+	struct is31fl319x_chip *is31 = led->chip;
+	int chan = led - is31->leds;
+	int ret;
+	int i;
+	u8 ctrl1 = 0, ctrl2 = 0;
+
+	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, chan, brightness);
+
+	mutex_lock(&is31->lock);
+
+	/* update PWM register */
+	ret = regmap_write(is31->regmap, IS31FL319X_PWM(chan), brightness);
+	if (ret < 0)
+		goto out;
+
+	/* read current brightness of all PWM channels */
+	for (i = 0; i < NUM_LEDS; i++) {
+		unsigned int pwm_value;
+		bool on;
+
+		/*
+		 * since neither cdev nor the chip can provide
+		 * the current setting, we read from the regmap cache
+		 */
+
+		ret = regmap_read(is31->regmap, IS31FL319X_PWM(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);
+	}
+
+out:
+	mutex_unlock(&is31->lock);
+
+	return ret;
+}
+
+static int is31fl319x_parse_child_dt(const struct device *dev,
+				     const struct device_node *child,
+				     struct is31fl319x_led *led)
+{
+	struct led_classdev *cdev = &led->cdev;
+	int ret;
+
+	if (of_property_read_string(child, "label", &cdev->name))
+		cdev->name = child->name;
+
+	cdev->default_trigger = NULL;
+	ret = of_property_read_string(child, "linux,default-trigger",
+		&cdev->default_trigger);
+	if (ret < 0 && ret != -EINVAL) /* is optional */
+		return ret;
+
+	led->max_microamp = LED_MAX_MICROAMP_DEFAULT;
+	ret = of_property_read_u32(child, "led-max-microamp",
+				   &led->max_microamp);
+	if (!ret) {
+		led->max_microamp = clamp(led->max_microamp,
+					  LED_MAX_MICROAMP_LOWER_LIMIT,
+					  LED_MAX_MICROAMP_UPPER_LIMIT);
+		led->max_microamp -= led->max_microamp % LED_MAX_MICROAMP_STEP;
+	}
+
+	return 0;
+}
+
+static int is31fl319x_parse_dt(struct device *dev,
+			       struct is31fl319x_chip *is31)
+{
+	struct device_node *np = dev->of_node, *child;
+	struct led_info *is31_leds;
+	int count;
+	int ret;
+
+	if (!np)
+		return -ENODEV;
+
+	count = of_get_child_count(np);
+
+	dev_dbg(dev, "child count %d\n", count);
+	if (!count || count > NUM_LEDS)
+		return -ENODEV;
+
+	is31_leds = devm_kzalloc(dev, sizeof(struct led_info) * NUM_LEDS,
+				 GFP_KERNEL);
+	if (!is31_leds)
+		return -ENOMEM;
+
+	for_each_child_of_node(np, child) {
+		struct is31fl319x_led *led;
+		u32 reg;
+
+		ret = of_property_read_u32(child, "reg", &reg);
+		if (ret)
+			break;
+
+		if (reg < 1 || reg > NUM_LEDS) {
+			dev_err(dev, "invalid led reg %u\n", reg);
+			ret = -EINVAL;
+			break;
+		}
+
+		led = &is31->leds[reg - 1];
+
+		if (led->configured) {
+			dev_err(dev, "led %u is already configured\n", reg);
+			ret = -EINVAL;
+			break;
+		}
+
+		ret = is31fl319x_parse_child_dt(dev, child, led);
+		if (ret) {
+			dev_err(dev, "led %u DT parsing failed\n", reg);
+			break;
+		}
+
+		led->configured = true;
+	}
+	if (ret) {
+		dev_err(dev, "DT child nodes parsing failed, error %d\n", ret);
+		return ret;
+	}
+
+	is31->audio_gain_db = 0;
+	ret = of_property_read_u32(np, "audio-gain-db", &is31->audio_gain_db);
+	if (!ret) {
+		is31->audio_gain_db = min(is31->audio_gain_db,
+					  AUDIO_GAIN_DB_MAX);
+		is31->audio_gain_db -= is31->audio_gain_db % 3;
+	}
+
+	return 0;
+}
+
+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;
+	}
+}
+
+static const struct reg_default is31fl319x_reg_defaults[] = {
+	{ IS31FL319X_CONFIG1, 0x00},
+	{ IS31FL319X_CONFIG2, 0x00},
+	{ IS31FL319X_PWM(0), 0x00},
+	{ IS31FL319X_PWM(1), 0x00},
+	{ IS31FL319X_PWM(2), 0x00},
+	{ IS31FL319X_PWM(3), 0x00},
+	{ IS31FL319X_PWM(4), 0x00},
+	{ IS31FL319X_PWM(5), 0x00},
+	{ IS31FL319X_PWM(6), 0x00},
+	{ IS31FL319X_PWM(7), 0x00},
+	{ IS31FL319X_PWM(8), 0x00},
+};
+
+static 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,
+	.reg_defaults = is31fl319x_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(is31fl319x_reg_defaults),
+};
+
+static int is31fl319x_microamp_to_cs(u32 microamp)
+{
+	switch (microamp) {
+	default:
+		WARN(1, "Invalid microamp setting");
+	case 20000: return 0;
+	case 15000: return 1;
+	case 10000: return 2;
+	case 5000:  return 3;
+	case 40000: return 4;
+	case 35000: return 5;
+	case 30000: return 6;
+	case 25000: return 7;
+	}
+}
+
+static int is31fl319x_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct is31fl319x_chip *is31;
+	struct i2c_adapter *adapter;
+	int err;
+	int i = 0;
+	u32 aggregated_led_microamp = LED_MAX_MICROAMP_UPPER_LIMIT;
+
+	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;
+
+	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
+	if (!is31)
+		return -ENOMEM;
+
+	mutex_init(&is31->lock);
+
+	err = is31fl319x_parse_dt(&client->dev, is31);
+	if (err) {
+		dev_err(&client->dev, "DT parsing error %d\n", err);
+		return err;
+	}
+
+	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 */
+	}
+
+	/*
+	 * Kernel conventions require per-LED led-max-microamp property.
+	 * But the chip does not allow to limit individual LEDs.
+	 * So we take minimum from all subnodes.
+	 */
+	for (i = 0; i < NUM_LEDS; i++)
+		if (is31->leds[i].configured &&
+		    is31->leds[i].max_microamp < aggregated_led_microamp)
+			aggregated_led_microamp = is31->leds[i].max_microamp;
+
+	regmap_write(is31->regmap, IS31FL319X_CONFIG2,
+		     is31fl319x_microamp_to_cs(aggregated_led_microamp) << 4 |
+		     is31->audio_gain_db / 3);
+
+	for (i = 0; i < NUM_LEDS; i++) {
+		struct is31fl319x_led *led = &is31->leds[i];
+
+		if (!led->configured)
+			continue;
+
+		led->chip = is31;
+		led->cdev.brightness_set_blocking = is31fl319x_brightness_set;
+
+		err = devm_led_classdev_register(&client->dev, &led->cdev);
+		if (err < 0)
+			return err;
+	}
+
+	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_AUTHOR("Andrey Utkin <andrey_utkin@fastmail.com>");
+MODULE_DESCRIPTION("IS31FL319X LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.3

[PATCH v3 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 v3 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 v3 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[Andrey Utkin]

On Fri, Jul 08, 2016 at 09:49:37PM +0200, H. Nikolaus Schaller wrote:
> +static int is31fl319x_microamp_to_cs(u32 microamp)
> +{
> +	switch (microamp) {
> +	default:
> +		WARN(1, "Invalid microamp setting");
> +	case 20000: return 0;
> +	case 15000: return 1;
> +	case 10000: return 2;
> +	case 5000:  return 3;
> +	case 40000: return 4;
> +	case 35000: return 5;
> +	case 30000: return 6;
> +	case 25000: return 7;
> +	}
> +}
> +
...
> +	regmap_write(is31->regmap, IS31FL319X_CONFIG2,
> +		     is31fl319x_microamp_to_cs(aggregated_led_microamp) << 4 |
> +		     is31->audio_gain_db / 3);

As I figured out by now, previous implementation of current setting (by
inline expression) was fine, this new one is just more obvious. So if
you wonder why it was replaced, I can say - "because of
misunderstanding". So switching back to that implementation is possible.

> +static struct i2c_driver is31fl319x_driver = {
> +	.driver   = {
> +		.name    = "leds-is31fl319x",
> +		.of_match_table = of_match_ptr(of_is31fl319x_leds_match),

Broken alignment :/ If this gets another iteration, i'll fix it.

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

[Jacek Anaszewski]

Hi Nikolaus,

Thanks for the update.

On 07/08/2016 09:49 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-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> Signed-off-by: Andrey Utkin <andrey_utkin-97jfqw80gc5Wk0Htik3J/w@public.gmane.org>
> ---
>   drivers/leds/Kconfig           |  12 ++
>   drivers/leds/Makefile          |   1 +
>   drivers/leds/leds-is31fl319x.c | 405 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 418 insertions(+)
>   create mode 100644 drivers/leds/leds-is31fl319x.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 5ae2834..6439a7d 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -570,6 +570,18 @@ 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
> +	select REGMAP_I2C
> +	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.
> +
> +	  This driver can also be built as a module. If so the module will be
> +	  called leds-is31fl319x.
> +
>   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..7fc5c6ab
> --- /dev/null
> +++ b/drivers/leds/leds-is31fl319x.c
> @@ -0,0 +1,405 @@
> +/*
> + * Copyright 2015-16 Golden Delicious Computers
> + *
> + * Author: Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> + *
> + * 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 IS31FL319{0,1,3,6,9} 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_PWM(channel) (0x07 + channel)
> +#define IS31FL319X_DATA_UPDATE  0x10
> +#define IS31FL319X_T0(channel)  (0x11 + channel)
> +#define IS31FL319X_T123_1       0x1a
> +#define IS31FL319X_T123_2       0x1b
> +#define IS31FL319X_T123_3       0x1c
> +#define IS31FL319X_T4(channel)  (0x1d + channel)
> +#define IS31FL319X_TIME_UPDATE  0x26
> +#define IS31FL319X_RESET        0xff
> +
> +#define IS31FL319X_REG_CNT      (IS31FL319X_RESET + 1)
> +
> +#define NUM_LEDS 9 /* max for 3199 chip */

Add namespacing prefix also to this macro.

> +
> +#define LED_MAX_MICROAMP_UPPER_LIMIT ((u32) 40000)
> +#define LED_MAX_MICROAMP_LOWER_LIMIT ((u32) 5000)
> +#define LED_MAX_MICROAMP_DEFAULT ((u32) 20000)
> +#define LED_MAX_MICROAMP_STEP ((u32) 5000)

Ditto.

> +
> +#define AUDIO_GAIN_DB_MAX ((u32) 21)

Ditto.

> +
> +/*
> + * regmap is used as a cache of chip's register space,
> + * to avoid reading back brightness values from chip,
> + * which is known to hang.
> + */
> +struct is31fl319x_chip {
> +	struct i2c_client       *client;
> +	struct regmap           *regmap;
> +	struct mutex            lock;
> +	u32                     audio_gain_db;
> +
> +	struct is31fl319x_led {
> +		struct is31fl319x_chip  *chip;
> +		struct led_classdev     cdev;
> +		u32                     max_microamp;
> +		bool                    configured;
> +	} 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);

This is redundant - you have this info in of_is31fl319x_leds_match,
and you can obtain it with of_match_device().
Please compare drivers/leds/leds-is31fl32xx.c.

> +static int is31fl319x_brightness_set(struct led_classdev *cdev,
> +				   enum led_brightness brightness)
> +{
> +	struct is31fl319x_led *led = container_of(cdev, struct is31fl319x_led,
> +						  cdev);
> +	struct is31fl319x_chip *is31 = led->chip;
> +	int chan = led - is31->leds;
> +	int ret;
> +	int i;
> +	u8 ctrl1 = 0, ctrl2 = 0;
> +
> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, chan, brightness);

It would be more useful after mutex IMO.

> +
> +	mutex_lock(&is31->lock);
> +
> +	/* update PWM register */
> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM(chan), brightness);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* read current brightness of all PWM channels */
> +	for (i = 0; i < NUM_LEDS; i++) {
> +		unsigned int pwm_value;
> +		bool on;
> +
> +		/*
> +		 * since neither cdev nor the chip can provide
> +		 * the current setting, we read from the regmap cache
> +		 */
> +
> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM(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);
> +	}
> +
> +out:
> +	mutex_unlock(&is31->lock);
> +
> +	return ret;
> +}
> +
> +static int is31fl319x_parse_child_dt(const struct device *dev,
> +				     const struct device_node *child,
> +				     struct is31fl319x_led *led)
> +{
> +	struct led_classdev *cdev = &led->cdev;
> +	int ret;
> +
> +	if (of_property_read_string(child, "label", &cdev->name))
> +		cdev->name = child->name;
> +
> +	cdev->default_trigger = NULL;

It was already zeroed by devm_kzalloc.

> +	ret = of_property_read_string(child, "linux,default-trigger",
> +		&cdev->default_trigger);
> +	if (ret < 0 && ret != -EINVAL) /* is optional */
> +		return ret;
> +
> +	led->max_microamp = LED_MAX_MICROAMP_DEFAULT;
> +	ret = of_property_read_u32(child, "led-max-microamp",
> +				   &led->max_microamp);
> +	if (!ret) {
> +		led->max_microamp = clamp(led->max_microamp,
> +					  LED_MAX_MICROAMP_LOWER_LIMIT,
> +					  LED_MAX_MICROAMP_UPPER_LIMIT);
> +		led->max_microamp -= led->max_microamp % LED_MAX_MICROAMP_STEP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int is31fl319x_parse_dt(struct device *dev,
> +			       struct is31fl319x_chip *is31)
> +{
> +	struct device_node *np = dev->of_node, *child;
> +	struct led_info *is31_leds;

You don't need it anymore.

> +	int count;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	count = of_get_child_count(np);

count is not used anywhere.

> +
> +	dev_dbg(dev, "child count %d\n", count);
> +	if (!count || count > NUM_LEDS)
> +		return -ENODEV;
> +
> +	is31_leds = devm_kzalloc(dev, sizeof(struct led_info) * NUM_LEDS,
> +				 GFP_KERNEL);
> +	if (!is31_leds)
> +		return -ENOMEM;

is31_leds is not used anywhere.

> +
> +	for_each_child_of_node(np, child) {
> +		struct is31fl319x_led *led;
> +		u32 reg;
> +
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			break;
> +
> +		if (reg < 1 || reg > NUM_LEDS) {
> +			dev_err(dev, "invalid led reg %u\n", reg);
> +			ret = -EINVAL;

You need to call of_node_put(child) when interrupting this loop.
Please add err label at the end:

err:
         of_node_put(child);
         return ret;

and goto err from here.


> +			break;
> +		}
> +
> +		led = &is31->leds[reg - 1];
> +
> +		if (led->configured) {
> +			dev_err(dev, "led %u is already configured\n", reg);
> +			ret = -EINVAL;

Ditto.

> +			break;
> +		}
> +
> +		ret = is31fl319x_parse_child_dt(dev, child, led);
> +		if (ret) {
> +			dev_err(dev, "led %u DT parsing failed\n", reg);

Ditto.

> +			break;
> +		}
> +
> +		led->configured = true;
> +	}
> +	if (ret) {
> +		dev_err(dev, "DT child nodes parsing failed, error %d\n", ret);
> +		return ret;
> +	}

This will be redundant then.

> +	is31->audio_gain_db = 0;
> +	ret = of_property_read_u32(np, "audio-gain-db", &is31->audio_gain_db);
> +	if (!ret) {
> +		is31->audio_gain_db = min(is31->audio_gain_db,
> +					  AUDIO_GAIN_DB_MAX);
> +		is31->audio_gain_db -= is31->audio_gain_db % 3;
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +	}
> +}
> +
> +static const struct reg_default is31fl319x_reg_defaults[] = {
> +	{ IS31FL319X_CONFIG1, 0x00},
> +	{ IS31FL319X_CONFIG2, 0x00},
> +	{ IS31FL319X_PWM(0), 0x00},
> +	{ IS31FL319X_PWM(1), 0x00},
> +	{ IS31FL319X_PWM(2), 0x00},
> +	{ IS31FL319X_PWM(3), 0x00},
> +	{ IS31FL319X_PWM(4), 0x00},
> +	{ IS31FL319X_PWM(5), 0x00},
> +	{ IS31FL319X_PWM(6), 0x00},
> +	{ IS31FL319X_PWM(7), 0x00},
> +	{ IS31FL319X_PWM(8), 0x00},
> +};
> +
> +static 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,
> +	.reg_defaults = is31fl319x_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(is31fl319x_reg_defaults),
> +};
> +
> +static int is31fl319x_microamp_to_cs(u32 microamp)
> +{
> +	switch (microamp) {
> +	default:
> +		WARN(1, "Invalid microamp setting");

Please move "default" case to the bottom of the switch,
and replace WARN with dev_warn.

> +	case 20000: return 0;
> +	case 15000: return 1;
> +	case 10000: return 2;
> +	case 5000:  return 3;
> +	case 40000: return 4;
> +	case 35000: return 5;
> +	case 30000: return 6;
> +	case 25000: return 7;
> +	}
> +}
> +
> +static int is31fl319x_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	struct is31fl319x_chip *is31;
> +	struct i2c_adapter *adapter;
> +	int err;
> +	int i = 0;
> +	u32 aggregated_led_microamp = LED_MAX_MICROAMP_UPPER_LIMIT;
> +
> +	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;
> +
> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
> +	if (!is31)
> +		return -ENOMEM;
> +
> +	mutex_init(&is31->lock);

You will have to bring back "remove" op now to call mutex_destroy on
remove.

> +
> +	err = is31fl319x_parse_dt(&client->dev, is31);
> +	if (err) {
> +		dev_err(&client->dev, "DT parsing error %d\n", err);

You have similar logging in is31fl319x_parse_dt().

> +		return err;
> +	}
> +
> +	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 */
> +	}
> +
> +	/*
> +	 * Kernel conventions require per-LED led-max-microamp property.
> +	 * But the chip does not allow to limit individual LEDs.
> +	 * So we take minimum from all subnodes.

Why minimum? Choose maximum and reduce max_brightness properties
of the sub-LEDs with lesser led-max-microamp.

> +	 */
> +	for (i = 0; i < NUM_LEDS; i++)
> +		if (is31->leds[i].configured &&
> +		    is31->leds[i].max_microamp < aggregated_led_microamp)
> +			aggregated_led_microamp = is31->leds[i].max_microamp;
> +
> +	regmap_write(is31->regmap, IS31FL319X_CONFIG2,
> +		     is31fl319x_microamp_to_cs(aggregated_led_microamp) << 4 |

Please add *SHIFT* macro definition for 4.

> +		     is31->audio_gain_db / 3);
> +
> +	for (i = 0; i < NUM_LEDS; i++) {
> +		struct is31fl319x_led *led = &is31->leds[i];
> +
> +		if (!led->configured)
> +			continue;
> +
> +		led->chip = is31;
> +		led->cdev.brightness_set_blocking = is31fl319x_brightness_set;
> +
> +		err = devm_led_classdev_register(&client->dev, &led->cdev);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	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-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>");
> +MODULE_AUTHOR("Andrey Utkin <andrey_utkin-97jfqw80gc5Wk0Htik3J/w@public.gmane.org>");
> +MODULE_DESCRIPTION("IS31FL319X LED driver");
> +MODULE_LICENSE("GPL v2");
>

-- 
Best regards,
Jacek Anaszewski
--
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

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

[Jacek Anaszewski]

Hi Nikolaus,

Thanks for the update.

On 07/08/2016 09:49 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>
> Signed-off-by: Andrey Utkin <andrey_utkin@fastmail.com>
> ---
>   drivers/leds/Kconfig           |  12 ++
>   drivers/leds/Makefile          |   1 +
>   drivers/leds/leds-is31fl319x.c | 405 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 418 insertions(+)
>   create mode 100644 drivers/leds/leds-is31fl319x.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 5ae2834..6439a7d 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -570,6 +570,18 @@ 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
> +	select REGMAP_I2C
> +	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.
> +
> +	  This driver can also be built as a module. If so the module will be
> +	  called leds-is31fl319x.
> +
>   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..7fc5c6ab
> --- /dev/null
> +++ b/drivers/leds/leds-is31fl319x.c
> @@ -0,0 +1,405 @@
> +/*
> + * 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 IS31FL319{0,1,3,6,9} 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_PWM(channel) (0x07 + channel)
> +#define IS31FL319X_DATA_UPDATE  0x10
> +#define IS31FL319X_T0(channel)  (0x11 + channel)
> +#define IS31FL319X_T123_1       0x1a
> +#define IS31FL319X_T123_2       0x1b
> +#define IS31FL319X_T123_3       0x1c
> +#define IS31FL319X_T4(channel)  (0x1d + channel)
> +#define IS31FL319X_TIME_UPDATE  0x26
> +#define IS31FL319X_RESET        0xff
> +
> +#define IS31FL319X_REG_CNT      (IS31FL319X_RESET + 1)
> +
> +#define NUM_LEDS 9 /* max for 3199 chip */

Add namespacing prefix also to this macro.

> +
> +#define LED_MAX_MICROAMP_UPPER_LIMIT ((u32) 40000)
> +#define LED_MAX_MICROAMP_LOWER_LIMIT ((u32) 5000)
> +#define LED_MAX_MICROAMP_DEFAULT ((u32) 20000)
> +#define LED_MAX_MICROAMP_STEP ((u32) 5000)

Ditto.

> +
> +#define AUDIO_GAIN_DB_MAX ((u32) 21)

Ditto.

> +
> +/*
> + * regmap is used as a cache of chip's register space,
> + * to avoid reading back brightness values from chip,
> + * which is known to hang.
> + */
> +struct is31fl319x_chip {
> +	struct i2c_client       *client;
> +	struct regmap           *regmap;
> +	struct mutex            lock;
> +	u32                     audio_gain_db;
> +
> +	struct is31fl319x_led {
> +		struct is31fl319x_chip  *chip;
> +		struct led_classdev     cdev;
> +		u32                     max_microamp;
> +		bool                    configured;
> +	} 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);

This is redundant - you have this info in of_is31fl319x_leds_match,
and you can obtain it with of_match_device().
Please compare drivers/leds/leds-is31fl32xx.c.

> +static int is31fl319x_brightness_set(struct led_classdev *cdev,
> +				   enum led_brightness brightness)
> +{
> +	struct is31fl319x_led *led = container_of(cdev, struct is31fl319x_led,
> +						  cdev);
> +	struct is31fl319x_chip *is31 = led->chip;
> +	int chan = led - is31->leds;
> +	int ret;
> +	int i;
> +	u8 ctrl1 = 0, ctrl2 = 0;
> +
> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, chan, brightness);

It would be more useful after mutex IMO.

> +
> +	mutex_lock(&is31->lock);
> +
> +	/* update PWM register */
> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM(chan), brightness);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* read current brightness of all PWM channels */
> +	for (i = 0; i < NUM_LEDS; i++) {
> +		unsigned int pwm_value;
> +		bool on;
> +
> +		/*
> +		 * since neither cdev nor the chip can provide
> +		 * the current setting, we read from the regmap cache
> +		 */
> +
> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM(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);
> +	}
> +
> +out:
> +	mutex_unlock(&is31->lock);
> +
> +	return ret;
> +}
> +
> +static int is31fl319x_parse_child_dt(const struct device *dev,
> +				     const struct device_node *child,
> +				     struct is31fl319x_led *led)
> +{
> +	struct led_classdev *cdev = &led->cdev;
> +	int ret;
> +
> +	if (of_property_read_string(child, "label", &cdev->name))
> +		cdev->name = child->name;
> +
> +	cdev->default_trigger = NULL;

It was already zeroed by devm_kzalloc.

> +	ret = of_property_read_string(child, "linux,default-trigger",
> +		&cdev->default_trigger);
> +	if (ret < 0 && ret != -EINVAL) /* is optional */
> +		return ret;
> +
> +	led->max_microamp = LED_MAX_MICROAMP_DEFAULT;
> +	ret = of_property_read_u32(child, "led-max-microamp",
> +				   &led->max_microamp);
> +	if (!ret) {
> +		led->max_microamp = clamp(led->max_microamp,
> +					  LED_MAX_MICROAMP_LOWER_LIMIT,
> +					  LED_MAX_MICROAMP_UPPER_LIMIT);
> +		led->max_microamp -= led->max_microamp % LED_MAX_MICROAMP_STEP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int is31fl319x_parse_dt(struct device *dev,
> +			       struct is31fl319x_chip *is31)
> +{
> +	struct device_node *np = dev->of_node, *child;
> +	struct led_info *is31_leds;

You don't need it anymore.

> +	int count;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	count = of_get_child_count(np);

count is not used anywhere.

> +
> +	dev_dbg(dev, "child count %d\n", count);
> +	if (!count || count > NUM_LEDS)
> +		return -ENODEV;
> +
> +	is31_leds = devm_kzalloc(dev, sizeof(struct led_info) * NUM_LEDS,
> +				 GFP_KERNEL);
> +	if (!is31_leds)
> +		return -ENOMEM;

is31_leds is not used anywhere.

> +
> +	for_each_child_of_node(np, child) {
> +		struct is31fl319x_led *led;
> +		u32 reg;
> +
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			break;
> +
> +		if (reg < 1 || reg > NUM_LEDS) {
> +			dev_err(dev, "invalid led reg %u\n", reg);
> +			ret = -EINVAL;

You need to call of_node_put(child) when interrupting this loop.
Please add err label at the end:

err:
         of_node_put(child);
         return ret;

and goto err from here.


> +			break;
> +		}
> +
> +		led = &is31->leds[reg - 1];
> +
> +		if (led->configured) {
> +			dev_err(dev, "led %u is already configured\n", reg);
> +			ret = -EINVAL;

Ditto.

> +			break;
> +		}
> +
> +		ret = is31fl319x_parse_child_dt(dev, child, led);
> +		if (ret) {
> +			dev_err(dev, "led %u DT parsing failed\n", reg);

Ditto.

> +			break;
> +		}
> +
> +		led->configured = true;
> +	}
> +	if (ret) {
> +		dev_err(dev, "DT child nodes parsing failed, error %d\n", ret);
> +		return ret;
> +	}

This will be redundant then.

> +	is31->audio_gain_db = 0;
> +	ret = of_property_read_u32(np, "audio-gain-db", &is31->audio_gain_db);
> +	if (!ret) {
> +		is31->audio_gain_db = min(is31->audio_gain_db,
> +					  AUDIO_GAIN_DB_MAX);
> +		is31->audio_gain_db -= is31->audio_gain_db % 3;
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +	}
> +}
> +
> +static const struct reg_default is31fl319x_reg_defaults[] = {
> +	{ IS31FL319X_CONFIG1, 0x00},
> +	{ IS31FL319X_CONFIG2, 0x00},
> +	{ IS31FL319X_PWM(0), 0x00},
> +	{ IS31FL319X_PWM(1), 0x00},
> +	{ IS31FL319X_PWM(2), 0x00},
> +	{ IS31FL319X_PWM(3), 0x00},
> +	{ IS31FL319X_PWM(4), 0x00},
> +	{ IS31FL319X_PWM(5), 0x00},
> +	{ IS31FL319X_PWM(6), 0x00},
> +	{ IS31FL319X_PWM(7), 0x00},
> +	{ IS31FL319X_PWM(8), 0x00},
> +};
> +
> +static 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,
> +	.reg_defaults = is31fl319x_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(is31fl319x_reg_defaults),
> +};
> +
> +static int is31fl319x_microamp_to_cs(u32 microamp)
> +{
> +	switch (microamp) {
> +	default:
> +		WARN(1, "Invalid microamp setting");

Please move "default" case to the bottom of the switch,
and replace WARN with dev_warn.

> +	case 20000: return 0;
> +	case 15000: return 1;
> +	case 10000: return 2;
> +	case 5000:  return 3;
> +	case 40000: return 4;
> +	case 35000: return 5;
> +	case 30000: return 6;
> +	case 25000: return 7;
> +	}
> +}
> +
> +static int is31fl319x_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	struct is31fl319x_chip *is31;
> +	struct i2c_adapter *adapter;
> +	int err;
> +	int i = 0;
> +	u32 aggregated_led_microamp = LED_MAX_MICROAMP_UPPER_LIMIT;
> +
> +	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;
> +
> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
> +	if (!is31)
> +		return -ENOMEM;
> +
> +	mutex_init(&is31->lock);

You will have to bring back "remove" op now to call mutex_destroy on
remove.

> +
> +	err = is31fl319x_parse_dt(&client->dev, is31);
> +	if (err) {
> +		dev_err(&client->dev, "DT parsing error %d\n", err);

You have similar logging in is31fl319x_parse_dt().

> +		return err;
> +	}
> +
> +	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 */
> +	}
> +
> +	/*
> +	 * Kernel conventions require per-LED led-max-microamp property.
> +	 * But the chip does not allow to limit individual LEDs.
> +	 * So we take minimum from all subnodes.

Why minimum? Choose maximum and reduce max_brightness properties
of the sub-LEDs with lesser led-max-microamp.

> +	 */
> +	for (i = 0; i < NUM_LEDS; i++)
> +		if (is31->leds[i].configured &&
> +		    is31->leds[i].max_microamp < aggregated_led_microamp)
> +			aggregated_led_microamp = is31->leds[i].max_microamp;
> +
> +	regmap_write(is31->regmap, IS31FL319X_CONFIG2,
> +		     is31fl319x_microamp_to_cs(aggregated_led_microamp) << 4 |

Please add *SHIFT* macro definition for 4.

> +		     is31->audio_gain_db / 3);
> +
> +	for (i = 0; i < NUM_LEDS; i++) {
> +		struct is31fl319x_led *led = &is31->leds[i];
> +
> +		if (!led->configured)
> +			continue;
> +
> +		led->chip = is31;
> +		led->cdev.brightness_set_blocking = is31fl319x_brightness_set;
> +
> +		err = devm_led_classdev_register(&client->dev, &led->cdev);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	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_AUTHOR("Andrey Utkin <andrey_utkin@fastmail.com>");
> +MODULE_DESCRIPTION("IS31FL319X LED driver");
> +MODULE_LICENSE("GPL v2");
>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 2/2] Bindings documentation for ISSI is31fl319x driver

[Jacek Anaszewski]

Hi Nikolaus,

On 07/08/2016 09:49 PM, H. Nikolaus Schaller wrote:
> 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";
> +	};
> +};
> +

You have an extra empty line at the end.

-- 
Best regards,
Jacek Anaszewski
--
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

Re: [PATCH v3 2/2] Bindings documentation for ISSI is31fl319x driver

[Jacek Anaszewski]

Hi Nikolaus,

On 07/08/2016 09:49 PM, H. Nikolaus Schaller wrote:
> 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";
> +	};
> +};
> +

You have an extra empty line at the end.

-- 
Best regards,
Jacek Anaszewski

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

[H. Nikolaus Schaller]

Hi Jacek,
Andrey already has worked in your comments but there is one topic
for discussion (see below) before we submit v4.

> Am 12.07.2016 um 22:14 schrieb Jacek Anaszewski <jacek.anaszewski@gmail.com>:
> 
> Hi Nikolaus,
> 
> Thanks for the update.
> 
> On 07/08/2016 09:49 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>
>> Signed-off-by: Andrey Utkin <andrey_utkin@fastmail.com>
>> ---
>>  drivers/leds/Kconfig           |  12 ++
>>  drivers/leds/Makefile          |   1 +
>>  drivers/leds/leds-is31fl319x.c | 405 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 418 insertions(+)
>>  create mode 100644 drivers/leds/leds-is31fl319x.c
>> 
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 5ae2834..6439a7d 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -570,6 +570,18 @@ 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
>> +	select REGMAP_I2C
>> +	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.
>> +
>> +	  This driver can also be built as a module. If so the module will be
>> +	  called leds-is31fl319x.
>> +
>>  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..7fc5c6ab
>> --- /dev/null
>> +++ b/drivers/leds/leds-is31fl319x.c
>> @@ -0,0 +1,405 @@
>> +/*
>> + * 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 IS31FL319{0,1,3,6,9} 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_PWM(channel) (0x07 + channel)
>> +#define IS31FL319X_DATA_UPDATE  0x10
>> +#define IS31FL319X_T0(channel)  (0x11 + channel)
>> +#define IS31FL319X_T123_1       0x1a
>> +#define IS31FL319X_T123_2       0x1b
>> +#define IS31FL319X_T123_3       0x1c
>> +#define IS31FL319X_T4(channel)  (0x1d + channel)
>> +#define IS31FL319X_TIME_UPDATE  0x26
>> +#define IS31FL319X_RESET        0xff
>> +
>> +#define IS31FL319X_REG_CNT      (IS31FL319X_RESET + 1)
>> +
>> +#define NUM_LEDS 9 /* max for 3199 chip */
> 
> Add namespacing prefix also to this macro.
> 
>> +
>> +#define LED_MAX_MICROAMP_UPPER_LIMIT ((u32) 40000)
>> +#define LED_MAX_MICROAMP_LOWER_LIMIT ((u32) 5000)
>> +#define LED_MAX_MICROAMP_DEFAULT ((u32) 20000)
>> +#define LED_MAX_MICROAMP_STEP ((u32) 5000)
> 
> Ditto.
> 
>> +
>> +#define AUDIO_GAIN_DB_MAX ((u32) 21)
> 
> Ditto.
> 
>> +
>> +/*
>> + * regmap is used as a cache of chip's register space,
>> + * to avoid reading back brightness values from chip,
>> + * which is known to hang.
>> + */
>> +struct is31fl319x_chip {
>> +	struct i2c_client       *client;
>> +	struct regmap           *regmap;
>> +	struct mutex            lock;
>> +	u32                     audio_gain_db;
>> +
>> +	struct is31fl319x_led {
>> +		struct is31fl319x_chip  *chip;
>> +		struct led_classdev     cdev;
>> +		u32                     max_microamp;
>> +		bool                    configured;
>> +	} 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);
> 
> This is redundant - you have this info in of_is31fl319x_leds_match,
> and you can obtain it with of_match_device().
> Please compare drivers/leds/leds-is31fl32xx.c.

Ah, I see. We now have a DT only driver.

> 
>> +static int is31fl319x_brightness_set(struct led_classdev *cdev,
>> +				   enum led_brightness brightness)
>> +{
>> +	struct is31fl319x_led *led = container_of(cdev, struct is31fl319x_led,
>> +						  cdev);
>> +	struct is31fl319x_chip *is31 = led->chip;
>> +	int chan = led - is31->leds;
>> +	int ret;
>> +	int i;
>> +	u8 ctrl1 = 0, ctrl2 = 0;
>> +
>> +	dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, chan, brightness);
> 
> It would be more useful after mutex IMO.
> 
>> +
>> +	mutex_lock(&is31->lock);
>> +
>> +	/* update PWM register */
>> +	ret = regmap_write(is31->regmap, IS31FL319X_PWM(chan), brightness);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	/* read current brightness of all PWM channels */
>> +	for (i = 0; i < NUM_LEDS; i++) {
>> +		unsigned int pwm_value;
>> +		bool on;
>> +
>> +		/*
>> +		 * since neither cdev nor the chip can provide
>> +		 * the current setting, we read from the regmap cache
>> +		 */
>> +
>> +		ret = regmap_read(is31->regmap, IS31FL319X_PWM(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);
>> +	}
>> +
>> +out:
>> +	mutex_unlock(&is31->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int is31fl319x_parse_child_dt(const struct device *dev,
>> +				     const struct device_node *child,
>> +				     struct is31fl319x_led *led)
>> +{
>> +	struct led_classdev *cdev = &led->cdev;
>> +	int ret;
>> +
>> +	if (of_property_read_string(child, "label", &cdev->name))
>> +		cdev->name = child->name;
>> +
>> +	cdev->default_trigger = NULL;
> 
> It was already zeroed by devm_kzalloc.
> 
>> +	ret = of_property_read_string(child, "linux,default-trigger",
>> +		&cdev->default_trigger);
>> +	if (ret < 0 && ret != -EINVAL) /* is optional */
>> +		return ret;
>> +
>> +	led->max_microamp = LED_MAX_MICROAMP_DEFAULT;
>> +	ret = of_property_read_u32(child, "led-max-microamp",
>> +				   &led->max_microamp);
>> +	if (!ret) {
>> +		led->max_microamp = clamp(led->max_microamp,
>> +					  LED_MAX_MICROAMP_LOWER_LIMIT,
>> +					  LED_MAX_MICROAMP_UPPER_LIMIT);
>> +		led->max_microamp -= led->max_microamp % LED_MAX_MICROAMP_STEP;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int is31fl319x_parse_dt(struct device *dev,
>> +			       struct is31fl319x_chip *is31)
>> +{
>> +	struct device_node *np = dev->of_node, *child;
>> +	struct led_info *is31_leds;
> 
> You don't need it anymore.
> 
>> +	int count;
>> +	int ret;
>> +
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	count = of_get_child_count(np);
> 
> count is not used anywhere.
> 
>> +
>> +	dev_dbg(dev, "child count %d\n", count);
>> +	if (!count || count > NUM_LEDS)
>> +		return -ENODEV;
>> +
>> +	is31_leds = devm_kzalloc(dev, sizeof(struct led_info) * NUM_LEDS,
>> +				 GFP_KERNEL);
>> +	if (!is31_leds)
>> +		return -ENOMEM;
> 
> is31_leds is not used anywhere.
> 
>> +
>> +	for_each_child_of_node(np, child) {
>> +		struct is31fl319x_led *led;
>> +		u32 reg;
>> +
>> +		ret = of_property_read_u32(child, "reg", &reg);
>> +		if (ret)
>> +			break;
>> +
>> +		if (reg < 1 || reg > NUM_LEDS) {
>> +			dev_err(dev, "invalid led reg %u\n", reg);
>> +			ret = -EINVAL;
> 
> You need to call of_node_put(child) when interrupting this loop.
> Please add err label at the end:
> 
> err:
>        of_node_put(child);
>        return ret;
> 
> and goto err from here.
> 
> 
>> +			break;
>> +		}
>> +
>> +		led = &is31->leds[reg - 1];
>> +
>> +		if (led->configured) {
>> +			dev_err(dev, "led %u is already configured\n", reg);
>> +			ret = -EINVAL;
> 
> Ditto.
> 
>> +			break;
>> +		}
>> +
>> +		ret = is31fl319x_parse_child_dt(dev, child, led);
>> +		if (ret) {
>> +			dev_err(dev, "led %u DT parsing failed\n", reg);
> 
> Ditto.
> 
>> +			break;
>> +		}
>> +
>> +		led->configured = true;
>> +	}
>> +	if (ret) {
>> +		dev_err(dev, "DT child nodes parsing failed, error %d\n", ret);
>> +		return ret;
>> +	}
> 
> This will be redundant then.
> 
>> +	is31->audio_gain_db = 0;
>> +	ret = of_property_read_u32(np, "audio-gain-db", &is31->audio_gain_db);
>> +	if (!ret) {
>> +		is31->audio_gain_db = min(is31->audio_gain_db,
>> +					  AUDIO_GAIN_DB_MAX);
>> +		is31->audio_gain_db -= is31->audio_gain_db % 3;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +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;
>> +	}
>> +}
>> +
>> +static const struct reg_default is31fl319x_reg_defaults[] = {
>> +	{ IS31FL319X_CONFIG1, 0x00},
>> +	{ IS31FL319X_CONFIG2, 0x00},
>> +	{ IS31FL319X_PWM(0), 0x00},
>> +	{ IS31FL319X_PWM(1), 0x00},
>> +	{ IS31FL319X_PWM(2), 0x00},
>> +	{ IS31FL319X_PWM(3), 0x00},
>> +	{ IS31FL319X_PWM(4), 0x00},
>> +	{ IS31FL319X_PWM(5), 0x00},
>> +	{ IS31FL319X_PWM(6), 0x00},
>> +	{ IS31FL319X_PWM(7), 0x00},
>> +	{ IS31FL319X_PWM(8), 0x00},
>> +};
>> +
>> +static 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,
>> +	.reg_defaults = is31fl319x_reg_defaults,
>> +	.num_reg_defaults = ARRAY_SIZE(is31fl319x_reg_defaults),
>> +};
>> +
>> +static int is31fl319x_microamp_to_cs(u32 microamp)
>> +{
>> +	switch (microamp) {
>> +	default:
>> +		WARN(1, "Invalid microamp setting");
> 
> Please move "default" case to the bottom of the switch,
> and replace WARN with dev_warn.
> 
>> +	case 20000: return 0;
>> +	case 15000: return 1;
>> +	case 10000: return 2;
>> +	case 5000:  return 3;
>> +	case 40000: return 4;
>> +	case 35000: return 5;
>> +	case 30000: return 6;
>> +	case 25000: return 7;
>> +	}
>> +}
>> +
>> +static int is31fl319x_probe(struct i2c_client *client,
>> +		const struct i2c_device_id *id)
>> +{
>> +	struct is31fl319x_chip *is31;
>> +	struct i2c_adapter *adapter;
>> +	int err;
>> +	int i = 0;
>> +	u32 aggregated_led_microamp = LED_MAX_MICROAMP_UPPER_LIMIT;
>> +
>> +	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;
>> +
>> +	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
>> +	if (!is31)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&is31->lock);
> 
> You will have to bring back "remove" op now to call mutex_destroy on
> remove.
> 
>> +
>> +	err = is31fl319x_parse_dt(&client->dev, is31);
>> +	if (err) {
>> +		dev_err(&client->dev, "DT parsing error %d\n", err);
> 
> You have similar logging in is31fl319x_parse_dt().
> 
>> +		return err;
>> +	}
>> +
>> +	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 */
>> +	}
>> +
>> +	/*
>> +	 * Kernel conventions require per-LED led-max-microamp property.
>> +	 * But the chip does not allow to limit individual LEDs.
>> +	 * So we take minimum from all subnodes.
> 
> Why minimum? Choose maximum and reduce max_brightness properties
> of the sub-LEDs with lesser led-max-microamp.

Hm. Is this really the correct way to handle it?

Assume you connect an LED which is specified with 10mA peak current.
And another with 20mA peak current.

So you define led-max-microamp in the DT as 10 mA and 20 mA.

Firstly a user can set the brightness only to 50% of LED_FULL since it is limited
by a reduced max_brightness. And heshe finds that not all LEDs have the same
max_brightness. The first LED will have 127 and the second one 255 for reasons
not directly understandable.

This entangles "brightness" with "max-current" - which are IMHO two independent
things.

Next, this will set the hardware limit to 20 mA. So there will be current peaks
of 20 mA for an LED where the DT developer thinks to have specified a led-max-microamp
of 10 mA. So you run the LED outside of its specs although the DT seems to
tell that it is inside and user-space thinks it is ok. This will reduce lifetime of LEDs.

So either "led-max-microamp" is the wrong name for this property (could better
be "led-max-average-microamp") or the whole logic is broken.

This is why we hesitate to hide (or even disable because you can't set the limit
to 10mA by DT) the current limiting chip feature by such a difficult to understand
automatic feature.

Using the minimum of all led-max-microamp keeps everything on the safe
side, running some LEDs with less current than specified. But never outside
of the spec. And all LEDs have the same max_brightness which is IMHO
more intuitive for the user.

Our original proposal was to define led-max-microamp for the whole chip
instead of individual LEDs, which is IMHO much easier to understand,
because it corresponds one-to-one with the data sheet.

> 
>> +	 */
>> +	for (i = 0; i < NUM_LEDS; i++)
>> +		if (is31->leds[i].configured &&
>> +		    is31->leds[i].max_microamp < aggregated_led_microamp)
>> +			aggregated_led_microamp = is31->leds[i].max_microamp;
>> +
>> +	regmap_write(is31->regmap, IS31FL319X_CONFIG2,
>> +		     is31fl319x_microamp_to_cs(aggregated_led_microamp) << 4 |
> 
> Please add *SHIFT* macro definition for 4.
> 
>> +		     is31->audio_gain_db / 3);
>> +
>> +	for (i = 0; i < NUM_LEDS; i++) {
>> +		struct is31fl319x_led *led = &is31->leds[i];
>> +
>> +		if (!led->configured)
>> +			continue;
>> +
>> +		led->chip = is31;
>> +		led->cdev.brightness_set_blocking = is31fl319x_brightness_set;
>> +
>> +		err = devm_led_classdev_register(&client->dev, &led->cdev);
>> +		if (err < 0)
>> +			return err;
>> +	}
>> +
>> +	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_AUTHOR("Andrey Utkin <andrey_utkin@fastmail.com>");
>> +MODULE_DESCRIPTION("IS31FL319X LED driver");
>> +MODULE_LICENSE("GPL v2");
>> 
> 
> -- 
> Best regards,
> Jacek Anaszewski

BR and thanks,
Nikolaus

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

[Jacek Anaszewski]

Hi Nikolaus,

On 07/13/2016 08:09 AM, H. Nikolaus Schaller wrote:
[...]
>>> +	/*
>>> +	 * Kernel conventions require per-LED led-max-microamp property.
>>> +	 * But the chip does not allow to limit individual LEDs.
>>> +	 * So we take minimum from all subnodes.
>>
>> Why minimum? Choose maximum and reduce max_brightness properties
>> of the sub-LEDs with lesser led-max-microamp.
>
> Hm. Is this really the correct way to handle it?
>
> Assume you connect an LED which is specified with 10mA peak current.
> And another with 20mA peak current.
>
> So you define led-max-microamp in the DT as 10 mA and 20 mA.
>
> Firstly a user can set the brightness only to 50% of LED_FULL since it is limited
> by a reduced max_brightness. And heshe finds that not all LEDs have the same
> max_brightness. The first LED will have 127 and the second one 255 for reasons
> not directly understandable.

You have to know that led-max-microamp property was introduced
to standarize Device Tree definition of maximum brightness allowed for
a sub-LED.

Earlier people defined max-brightness property in DT, but at
some point we realized that brightness level is not a proper unit for
describing a hardware property.

It was not global max-microamp setting that appears in recent LED
controllers that drove the introduction of led-max-microamp property.

If you question the idea of having different maximum brightness per
sub-LEDs controlled by the same device, then it means that you have
objections to the entire idea of LED subsystem max_brightness property,
whereas it has been broadly accepted and successfully used for years.

> This entangles "brightness" with "max-current" - which are IMHO two independent
> things.

The fact that recent LED controllers use the same notion for one of its
settings shouldn't mislead us.

> Next, this will set the hardware limit to 20 mA. So there will be current peaks
> of 20 mA for an LED where the DT developer thinks to have specified a led-max-microamp
> of 10 mA. So you run the LED outside of its specs although the DT seems to
> tell that it is inside and user-space thinks it is ok. This will reduce lifetime of LEDs.

In what circumstances would such current peaks occur? On reset?
Shouldn't such a device be considered as broken by design?
What if all LEDs would have low amperage? Would there be some way
to protect them from being damaged by current peaks?

> So either "led-max-microamp" is the wrong name for this property (could better
> be "led-max-average-microamp") or the whole logic is broken.
>
> This is why we hesitate to hide (or even disable because you can't set the limit
> to 10mA by DT) the current limiting chip feature by such a difficult to understand
> automatic feature.
>
> Using the minimum of all led-max-microamp keeps everything on the safe
> side, running some LEDs with less current than specified. But never outside
> of the spec. And all LEDs have the same max_brightness which is IMHO
> more intuitive for the user.
>
> Our original proposal was to define led-max-microamp for the whole chip
> instead of individual LEDs, which is IMHO much easier to understand,
> because it corresponds one-to-one with the data sheet.



-- 
Best regards,
Jacek Anaszewski

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

[H. Nikolaus Schaller]

Hi Jacek,

> Am 13.07.2016 um 09:45 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
> 
> Hi Nikolaus,
> 
> On 07/13/2016 08:09 AM, H. Nikolaus Schaller wrote:
> [...]
>>>> +	/*
>>>> +	 * Kernel conventions require per-LED led-max-microamp property.
>>>> +	 * But the chip does not allow to limit individual LEDs.
>>>> +	 * So we take minimum from all subnodes.
>>> 
>>> Why minimum? Choose maximum and reduce max_brightness properties
>>> of the sub-LEDs with lesser led-max-microamp.
>> 
>> Hm. Is this really the correct way to handle it?
>> 
>> Assume you connect an LED which is specified with 10mA peak current.
>> And another with 20mA peak current.
>> 
>> So you define led-max-microamp in the DT as 10 mA and 20 mA.
>> 
>> Firstly a user can set the brightness only to 50% of LED_FULL since it is limited
>> by a reduced max_brightness. And heshe finds that not all LEDs have the same
>> max_brightness. The first LED will have 127 and the second one 255 for reasons
>> not directly understandable.
> 
> You have to know that led-max-microamp property was introduced
> to standarize Device Tree definition of maximum brightness allowed for
> a sub-LED.

> 
> Earlier people defined max-brightness property in DT, but at
> some point we realized that brightness level is not a proper unit for
> describing a hardware property.

Indeed it isn't. It could be if it were brightness in some physical unit, but
here it is a user-space range definition. 

> It was not global max-microamp setting that appears in recent LED
> controllers that drove the introduction of led-max-microamp property.

> 
> If you question the idea of having different maximum brightness per
> sub-LEDs controlled by the same device, then it means that you have
> objections to the entire idea of LED subsystem max_brightness property,
> whereas it has been broadly accepted and successfully used for years.

Perhaps the max_brightness visible in user space could be standardized
to be always identical to LED_FULL.

> 
>> This entangles "brightness" with "max-current" - which are IMHO two independent
>> things.
> 
> The fact that recent LED controllers use the same notion for one of its
> settings shouldn't mislead us.

BTW, some LEDs may have the same brightness (measured physical light
intensity) at very different currents, but that is not the key point.

> 
>> Next, this will set the hardware limit to 20 mA. So there will be current peaks
>> of 20 mA for an LED where the DT developer thinks to have specified a led-max-microamp
>> of 10 mA. So you run the LED outside of its specs although the DT seems to
>> tell that it is inside and user-space thinks it is ok. This will reduce lifetime of LEDs.
> 
> In what circumstances would such current peaks occur? On reset?

During PWM operation. PWM makes pulses with different duty cycle.

Some % of the time it is 0 mA and some it is some max current. In our chip
this max current is defined for all LEDs by some global control register.

If you limit to 50% of the max current of all LEDS (e.g. 20 mA) you have
10 mA average described in the DT.

But this means permanent pulses of 20 mA e.g. 50% of the time at LED_HALF.
50% of the time the current is above what is described in DT.

> Shouldn't such a device be considered as broken by design?

Yes but only if it would do it during reset.

The problem I refer to is during normal operation and occurs because
we try to pretend that we can limit the max current for each LED individually,
which the chip does not provide.

We can only limit the average current of each LED and not the peak current.

> What if all LEDs would have low amperage?

Yes, the chip can reduce the max current in steps of 5 mA up to 40 mA. So
you can reduce it to the lowest led-max-microamp of any of the LEDs. Which
is the minimum-rule we have in v3.

If all LEDs are the same, this will be like a global limit.

> Would there be some way
> to protect them from being damaged by current peaks?

Yes, there would be a hardware means to limit the current: series resistors.

But I think neither driver implementation nor DT considerations should drive
hardware design into a specific solution...

That all is why we initially proposed a new global led-max-microamp DT property.
It would IMHO better describe the chip in DT and not loose any user-space
capabilities.

Ah, writing this sentence opens my eyes to where we are not talking about the same
picture. It appears that I am thinking chip-focussed about describing the
current-limiting facility of the chip. Hence the global property.

Contrary to that, the subnode led-max-microamp describes the LEDs (and not a chip).
And leaves it to the driver to set up the chip in a way that the LEDs never get
more current than specified there. Right?

This means we have to take the minimum (not the maximum) and round it down
to the nearest value the chip can provide.

I.e. if you specify one LED with 10mA and one with 1000mA, the whole chip would
limit to 10mA. This means the 1000mA LED will never be visibly bright (even at
max_brightness). But that is not a fault of the chip or the driver but of the hardware
designer choosing such imbalanced LEDs.

> 
>> So either "led-max-microamp" is the wrong name for this property (could better
>> be "led-max-average-microamp") or the whole logic is broken.
>> 
>> This is why we hesitate to hide (or even disable because you can't set the limit
>> to 10mA by DT) the current limiting chip feature by such a difficult to understand
>> automatic feature.
>> 
>> Using the minimum of all led-max-microamp keeps everything on the safe
>> side, running some LEDs with less current than specified. But never outside
>> of the spec. And all LEDs have the same max_brightness which is IMHO
>> more intuitive for the user.
>> 
>> Our original proposal was to define led-max-microamp for the whole chip
>> instead of individual LEDs, which is IMHO much easier to understand,
>> because it corresponds one-to-one with the data sheet.
> 
> 

BR and thanks,
Nikolaus

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

[Jacek Anaszewski]

On 07/13/2016 10:52 AM, H. Nikolaus Schaller wrote:
> Hi Jacek,
>
>> Am 13.07.2016 um 09:45 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>
>> Hi Nikolaus,
>>
>> On 07/13/2016 08:09 AM, H. Nikolaus Schaller wrote:
>> [...]
>>>>> +	/*
>>>>> +	 * Kernel conventions require per-LED led-max-microamp property.
>>>>> +	 * But the chip does not allow to limit individual LEDs.
>>>>> +	 * So we take minimum from all subnodes.
>>>>
>>>> Why minimum? Choose maximum and reduce max_brightness properties
>>>> of the sub-LEDs with lesser led-max-microamp.
>>>
>>> Hm. Is this really the correct way to handle it?
>>>
>>> Assume you connect an LED which is specified with 10mA peak current.
>>> And another with 20mA peak current.
>>>
>>> So you define led-max-microamp in the DT as 10 mA and 20 mA.
>>>
>>> Firstly a user can set the brightness only to 50% of LED_FULL since it is limited
>>> by a reduced max_brightness. And heshe finds that not all LEDs have the same
>>> max_brightness. The first LED will have 127 and the second one 255 for reasons
>>> not directly understandable.
>>
>> You have to know that led-max-microamp property was introduced
>> to standarize Device Tree definition of maximum brightness allowed for
>> a sub-LED.
>
>>
>> Earlier people defined max-brightness property in DT, but at
>> some point we realized that brightness level is not a proper unit for
>> describing a hardware property.
>
> Indeed it isn't. It could be if it were brightness in some physical unit, but
> here it is a user-space range definition.
>
>> It was not global max-microamp setting that appears in recent LED
>> controllers that drove the introduction of led-max-microamp property.
>
>>
>> If you question the idea of having different maximum brightness per
>> sub-LEDs controlled by the same device, then it means that you have
>> objections to the entire idea of LED subsystem max_brightness property,
>> whereas it has been broadly accepted and successfully used for years.
>
> Perhaps the max_brightness visible in user space could be standardized
> to be always identical to LED_FULL.

The patch that introduced max_brightness property claims that it
overrides the legacy LED_FULL enum.

>>
>>> This entangles "brightness" with "max-current" - which are IMHO two independent
>>> things.
>>
>> The fact that recent LED controllers use the same notion for one of its
>> settings shouldn't mislead us.
>
> BTW, some LEDs may have the same brightness (measured physical light
> intensity) at very different currents, but that is not the key point.
>
>>
>>> Next, this will set the hardware limit to 20 mA. So there will be current peaks
>>> of 20 mA for an LED where the DT developer thinks to have specified a led-max-microamp
>>> of 10 mA. So you run the LED outside of its specs although the DT seems to
>>> tell that it is inside and user-space thinks it is ok. This will reduce lifetime of LEDs.
>>
>> In what circumstances would such current peaks occur? On reset?
>
> During PWM operation. PWM makes pulses with different duty cycle.
>
> Some % of the time it is 0 mA and some it is some max current. In our chip
> this max current is defined for all LEDs by some global control register.
>
> If you limit to 50% of the max current of all LEDS (e.g. 20 mA) you have
> 10 mA average described in the DT.
>
> But this means permanent pulses of 20 mA e.g. 50% of the time at LED_HALF.
> 50% of the time the current is above what is described in DT.
>
>> Shouldn't such a device be considered as broken by design?
>
> Yes but only if it would do it during reset.
>
> The problem I refer to is during normal operation and occurs because
> we try to pretend that we can limit the max current for each LED individually,
> which the chip does not provide.
>
> We can only limit the average current of each LED and not the peak current.
>
>> What if all LEDs would have low amperage?
>
> Yes, the chip can reduce the max current in steps of 5 mA up to 40 mA. So
> you can reduce it to the lowest led-max-microamp of any of the LEDs. Which
> is the minimum-rule we have in v3.
>
> If all LEDs are the same, this will be like a global limit.
>
>> Would there be some way
>> to protect them from being damaged by current peaks?
>
> Yes, there would be a hardware means to limit the current: series resistors.
>
> But I think neither driver implementation nor DT considerations should drive
> hardware design into a specific solution...
>
> That all is why we initially proposed a new global led-max-microamp DT property.
> It would IMHO better describe the chip in DT and not loose any user-space
> capabilities.
>
> Ah, writing this sentence opens my eyes to where we are not talking about the same
> picture. It appears that I am thinking chip-focussed about describing the
> current-limiting facility of the chip. Hence the global property.
>
> Contrary to that, the subnode led-max-microamp describes the LEDs (and not a chip).
> And leaves it to the driver to set up the chip in a way that the LEDs never get
> more current than specified there. Right?
>
> This means we have to take the minimum (not the maximum) and round it down
> to the nearest value the chip can provide.
>
> I.e. if you specify one LED with 10mA and one with 1000mA, the whole chip would
> limit to 10mA. This means the 1000mA LED will never be visibly bright (even at
> max_brightness). But that is not a fault of the chip or the driver but of the hardware
> designer choosing such imbalanced LEDs.

The only rationale standing behind the introduction of led-max-microamp
property was to provide a means for defining max_brightness property in
a Device Tree, that would fit for its requirements.

As you realized we are not considering global max microamp setting,
which is supported by some LED controllers and cannot be considered
generic feature of this type of devices.

It is the driver originator's responsibility to infer proper (safe)
global max-microamp setting basing on the led-max-microamp values
provided for sub-LEDs.

Since PWM devices are specific and max current cannot be defined for
them uniformly, then they need special treatment. I agree that in
case of your device choosing the lowest value would be the most
suitable approach. All LEDs could be left with max_brightness
uninitialized, which is the hint for the LED core to set it to LED_FULL.


>>> So either "led-max-microamp" is the wrong name for this property (could better
>>> be "led-max-average-microamp") or the whole logic is broken.
>>>
>>> This is why we hesitate to hide (or even disable because you can't set the limit
>>> to 10mA by DT) the current limiting chip feature by such a difficult to understand
>>> automatic feature.
>>>
>>> Using the minimum of all led-max-microamp keeps everything on the safe
>>> side, running some LEDs with less current than specified. But never outside
>>> of the spec. And all LEDs have the same max_brightness which is IMHO
>>> more intuitive for the user.
>>>
>>> Our original proposal was to define led-max-microamp for the whole chip
>>> instead of individual LEDs, which is IMHO much easier to understand,
>>> because it corresponds one-to-one with the data sheet.
>>
>>
>
> BR and thanks,
> Nikolaus
>
>
>


-- 
Best regards,
Jacek Anaszewski

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

[H. Nikolaus Schaller]

Hi Jacek,.

> Am 13.07.2016 um 11:26 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
> 
> On 07/13/2016 10:52 AM, H. Nikolaus Schaller wrote:
>> Hi Jacek,
>> 
>>> Am 13.07.2016 um 09:45 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
>>> 
>>> Hi Nikolaus,
>>> 
>>> On 07/13/2016 08:09 AM, H. Nikolaus Schaller wrote:
>>> [...]
>>>>>> +	/*
>>>>>> +	 * Kernel conventions require per-LED led-max-microamp property.
>>>>>> +	 * But the chip does not allow to limit individual LEDs.
>>>>>> +	 * So we take minimum from all subnodes.
>>>>> 
>>>>> Why minimum? Choose maximum and reduce max_brightness properties
>>>>> of the sub-LEDs with lesser led-max-microamp.
>>>> 
>>>> Hm. Is this really the correct way to handle it?
>>>> 
>>>> Assume you connect an LED which is specified with 10mA peak current.
>>>> And another with 20mA peak current.
>>>> 
>>>> So you define led-max-microamp in the DT as 10 mA and 20 mA.
>>>> 
>>>> Firstly a user can set the brightness only to 50% of LED_FULL since it is limited
>>>> by a reduced max_brightness. And heshe finds that not all LEDs have the same
>>>> max_brightness. The first LED will have 127 and the second one 255 for reasons
>>>> not directly understandable.
>>> 
>>> You have to know that led-max-microamp property was introduced
>>> to standarize Device Tree definition of maximum brightness allowed for
>>> a sub-LED.
>> 
>>> 
>>> Earlier people defined max-brightness property in DT, but at
>>> some point we realized that brightness level is not a proper unit for
>>> describing a hardware property.
>> 
>> Indeed it isn't. It could be if it were brightness in some physical unit, but
>> here it is a user-space range definition.
>> 
>>> It was not global max-microamp setting that appears in recent LED
>>> controllers that drove the introduction of led-max-microamp property.
>> 
>>> 
>>> If you question the idea of having different maximum brightness per
>>> sub-LEDs controlled by the same device, then it means that you have
>>> objections to the entire idea of LED subsystem max_brightness property,
>>> whereas it has been broadly accepted and successfully used for years.
>> 
>> Perhaps the max_brightness visible in user space could be standardized
>> to be always identical to LED_FULL.
> 
> The patch that introduced max_brightness property claims that it
> overrides the legacy LED_FULL enum.

Well, that is IMHO standard in Linux that changes and decisions are sometimes
made with some specific idea in mind. Some time later it turns out that the old
variant wasn't that bad.

But I don't want to question the status quo of max_brightness and LED_FULL
at all.

> 
>>> 
>>>> This entangles "brightness" with "max-current" - which are IMHO two independent
>>>> things.
>>> 
>>> The fact that recent LED controllers use the same notion for one of its
>>> settings shouldn't mislead us.
>> 
>> BTW, some LEDs may have the same brightness (measured physical light
>> intensity) at very different currents, but that is not the key point.
>> 
>>> 
>>>> Next, this will set the hardware limit to 20 mA. So there will be current peaks
>>>> of 20 mA for an LED where the DT developer thinks to have specified a led-max-microamp
>>>> of 10 mA. So you run the LED outside of its specs although the DT seems to
>>>> tell that it is inside and user-space thinks it is ok. This will reduce lifetime of LEDs.
>>> 
>>> In what circumstances would such current peaks occur? On reset?
>> 
>> During PWM operation. PWM makes pulses with different duty cycle.
>> 
>> Some % of the time it is 0 mA and some it is some max current. In our chip
>> this max current is defined for all LEDs by some global control register.
>> 
>> If you limit to 50% of the max current of all LEDS (e.g. 20 mA) you have
>> 10 mA average described in the DT.
>> 
>> But this means permanent pulses of 20 mA e.g. 50% of the time at LED_HALF.
>> 50% of the time the current is above what is described in DT.
>> 
>>> Shouldn't such a device be considered as broken by design?
>> 
>> Yes but only if it would do it during reset.
>> 
>> The problem I refer to is during normal operation and occurs because
>> we try to pretend that we can limit the max current for each LED individually,
>> which the chip does not provide.
>> 
>> We can only limit the average current of each LED and not the peak current.
>> 
>>> What if all LEDs would have low amperage?
>> 
>> Yes, the chip can reduce the max current in steps of 5 mA up to 40 mA. So
>> you can reduce it to the lowest led-max-microamp of any of the LEDs. Which
>> is the minimum-rule we have in v3.
>> 
>> If all LEDs are the same, this will be like a global limit.
>> 
>>> Would there be some way
>>> to protect them from being damaged by current peaks?
>> 
>> Yes, there would be a hardware means to limit the current: series resistors.
>> 
>> But I think neither driver implementation nor DT considerations should drive
>> hardware design into a specific solution...
>> 
>> That all is why we initially proposed a new global led-max-microamp DT property.
>> It would IMHO better describe the chip in DT and not loose any user-space
>> capabilities.
>> 
>> Ah, writing this sentence opens my eyes to where we are not talking about the same
>> picture. It appears that I am thinking chip-focussed about describing the
>> current-limiting facility of the chip. Hence the global property.
>> 
>> Contrary to that, the subnode led-max-microamp describes the LEDs (and not a chip).
>> And leaves it to the driver to set up the chip in a way that the LEDs never get
>> more current than specified there. Right?
>> 
>> This means we have to take the minimum (not the maximum) and round it down
>> to the nearest value the chip can provide.
>> 
>> I.e. if you specify one LED with 10mA and one with 1000mA, the whole chip would
>> limit to 10mA. This means the 1000mA LED will never be visibly bright (even at
>> max_brightness). But that is not a fault of the chip or the driver but of the hardware
>> designer choosing such imbalanced LEDs.
> 
> The only rationale standing behind the introduction of led-max-microamp
> property was to provide a means for defining max_brightness property in
> a Device Tree, that would fit for its requirements.

Ok. Let's take that as it is :)

> 
> As you realized we are not considering global max microamp setting,
> which is supported by some LED controllers and cannot be considered
> generic feature of this type of devices.
> 
> It is the driver originator's responsibility to infer proper (safe)
> global max-microamp setting basing on the led-max-microamp values
> provided for sub-LEDs.

Yes, that is what we want to provide.

> Since PWM devices are specific and max current cannot be defined for
> them uniformly, then they need special treatment. I agree that in
> case of your device choosing the lowest value would be the most
> suitable approach. All LEDs could be left with max_brightness
> uninitialized, which is the hint for the LED core to set it to LED_FULL.

Ok, then let's do it that way which is probably the best we can reach now.
Maybe in the future someone has an idea to do it even better.

> 
> 
>>>> So either "led-max-microamp" is the wrong name for this property (could better
>>>> be "led-max-average-microamp") or the whole logic is broken.
>>>> 
>>>> This is why we hesitate to hide (or even disable because you can't set the limit
>>>> to 10mA by DT) the current limiting chip feature by such a difficult to understand
>>>> automatic feature.
>>>> 
>>>> Using the minimum of all led-max-microamp keeps everything on the safe
>>>> side, running some LEDs with less current than specified. But never outside
>>>> of the spec. And all LEDs have the same max_brightness which is IMHO
>>>> more intuitive for the user.
>>>> 
>>>> Our original proposal was to define led-max-microamp for the whole chip
>>>> instead of individual LEDs, which is IMHO much easier to understand,
>>>> because it corresponds one-to-one with the data sheet.
>>> 
>>> 
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> 
>> 
> 
> 
> -- 
> Best regards,
> Jacek Anaszewski

BR and thanks,
Nikolaus

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

[H. Nikolaus Schaller]

Hi Jacek,

> Am 12.07.2016 um 22:14 schrieb Jacek Anaszewski <jacek.anaszewski@gmail.com>:
> 
>> 
>> +
>> +/*
>> + * regmap is used as a cache of chip's register space,
>> + * to avoid reading back brightness values from chip,
>> + * which is known to hang.
>> + */
>> +struct is31fl319x_chip {
>> +	struct i2c_client       *client;
>> +	struct regmap           *regmap;
>> +	struct mutex            lock;
>> +	u32                     audio_gain_db;
>> +
>> +	struct is31fl319x_led {
>> +		struct is31fl319x_chip  *chip;
>> +		struct led_classdev     cdev;
>> +		u32                     max_microamp;
>> +		bool                    configured;
>> +	} 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);
> 
> This is redundant - you have this info in of_is31fl319x_leds_match,
> and you can obtain it with of_match_device().
> Please compare drivers/leds/leds-is31fl32xx.c.

we have tried like is31fl32xx.c but it does not automatically load the driver
if compiled as module (like all other I2C clients do). Reason seems to
be that there is no i2c information in modalias database any more.

So perhaps the i31fl32 approach is also incomplete. Has it been tested
with loadable modules?

And, there is a bug/typo in the fl32 driver:

http://lxr.free-electrons.com/source/drivers/leds/leds-is31fl32xx.c#L425

static const struct i2c_device_id is31fl31xx_id[] = {

s/fl31/fl32/ is31fl32xx.c

BR and thanks,
Nikolaus

Re: [PATCH v3 2/2] Bindings documentation for ISSI is31fl319x driver

[Rob Herring]

On Fri, Jul 08, 2016 at 09:49:38PM +0200, H. Nikolaus Schaller wrote:
> 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

Acked-by: Rob Herring <robh@kernel.org>

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

[Jacek Anaszewski]

Hi Nikolaus,

On 07/15/2016 10:08 AM, H. Nikolaus Schaller wrote:
> Hi Jacek,
>
>> Am 12.07.2016 um 22:14 schrieb Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>>>
>>> +
>>> +/*
>>> + * regmap is used as a cache of chip's register space,
>>> + * to avoid reading back brightness values from chip,
>>> + * which is known to hang.
>>> + */
>>> +struct is31fl319x_chip {
>>> +	struct i2c_client       *client;
>>> +	struct regmap           *regmap;
>>> +	struct mutex            lock;
>>> +	u32                     audio_gain_db;
>>> +
>>> +	struct is31fl319x_led {
>>> +		struct is31fl319x_chip  *chip;
>>> +		struct led_classdev     cdev;
>>> +		u32                     max_microamp;
>>> +		bool                    configured;
>>> +	} 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);
>>
>> This is redundant - you have this info in of_is31fl319x_leds_match,
>> and you can obtain it with of_match_device().
>> Please compare drivers/leds/leds-is31fl32xx.c.
>
> we have tried like is31fl32xx.c but it does not automatically load the driver
> if compiled as module (like all other I2C clients do). Reason seems to
> be that there is no i2c information in modalias database any more.
>
> So perhaps the i31fl32 approach is also incomplete. Has it been tested
> with loadable modules?

Having the issue clarified, please proceed accordingly in case of
your driver.

-- 
Best regards,
Jacek Anaszewski