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

Hi Jacek,

Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski j.anaszewski@samsung.com:

Hi Nikolaus,

Thanks for the updated version. Some comments below.

On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:

...

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

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

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

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

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

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

Tested-on: OMAP5 based Pyra handheld prototype. Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 363 insertions(+) create mode 100644 drivers/leds/leds-is31fl319x.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 5ae2834..65b1045 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -570,6 +570,14 @@ config LEDS_SEAD3 This driver can also be built as a module. If so the module will be called leds-sead3.

+config LEDS_IS31FL319X

• tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
• depends on LEDS_CLASS && I2C && OF
• help

•  This option enables support for LEDs connected to ISSI IS31FL319x
  

•  fancy LED driver chips accessed via the I2C bus.
  

•  Driver supports individual PWM brightness control for each channel.
  

config LEDS_IS31FL32XX tristate "LED support for ISSI IS31FL32XX I2C LED controller family" depends on LEDS_CLASS && I2C && OF diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index cb2013d..b6ce9f9 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o obj-$(CONFIG_LEDS_SEAD3) += leds-sead3.o +obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o

# LED SPI Drivers diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c new file mode 100644 index 0000000..bbf8850 --- /dev/null +++ b/drivers/leds/leds-is31fl319x.c @@ -0,0 +1,354 @@ +/*

• • Copyright 2015-16 Golden Delicious Computers
• • Author: Nikolaus Schaller hns@goldelico.com
• • Based on leds-tca6507.c
• • This file is subject to the terms and conditions of version 2 of
• • the GNU General Public License. See the file COPYING in the main
• • directory of this archive for more details.
• • LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
• • effect LEDs.
• */

+#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regmap.h> +#include <linux/slab.h>

+/* register numbers */ +#define IS31FL319X_SHUTDOWN 0x00 +#define IS31FL319X_CTRL1 0x01 +#define IS31FL319X_CTRL2 0x02 +#define IS31FL319X_CONFIG1 0x03 +#define IS31FL319X_CONFIG2 0x04 +#define IS31FL319X_RAMP_MODE 0x05 +#define IS31FL319X_BREATH_MASK 0x06 +#define IS31FL319X_PWM1 0x07 +#define IS31FL319X_PWM2 0x08 +#define IS31FL319X_PWM3 0x09 +#define IS31FL319X_PWM4 0x0a +#define IS31FL319X_PWM5 0x0b +#define IS31FL319X_PWM6 0x0c +#define IS31FL319X_PWM7 0x0d +#define IS31FL319X_PWM8 0x0e +#define IS31FL319X_PWM9 0x0f +#define IS31FL319X_DATA_UPDATE 0x10 +#define IS31FL319X_T0_1 0x11 +#define IS31FL319X_T0_2 0x12 +#define IS31FL319X_T0_3 0x13 +#define IS31FL319X_T0_4 0x14 +#define IS31FL319X_T0_5 0x15 +#define IS31FL319X_T0_6 0x16 +#define IS31FL319X_T0_7 0x17 +#define IS31FL319X_T0_8 0x18 +#define IS31FL319X_T0_9 0x19 +#define IS31FL319X_T123_1 0x1a +#define IS31FL319X_T123_2 0x1b +#define IS31FL319X_T123_3 0x1c +#define IS31FL319X_T4_1 0x1d +#define IS31FL319X_T4_2 0x1e +#define IS31FL319X_T4_3 0x1f +#define IS31FL319X_T4_4 0x20 +#define IS31FL319X_T4_5 0x21 +#define IS31FL319X_T4_6 0x22 +#define IS31FL319X_T4_7 0x23 +#define IS31FL319X_T4_8 0x24 +#define IS31FL319X_T4_9 0x25 +#define IS31FL319X_TIME_UPDATE 0x26 +#define IS31FL319X_RESET 0xff

+#define IS31FL319X_REG_CNT (IS31FL319X_RESET + 1)

+#define NUM_LEDS 9 /* max for 3199 chip */

+struct is31fl319x_chip {

• struct i2c_client *client;
• struct regmap *regmap;
• struct is31fl319x_led {

• struct is31fl319x_chip	*chip;
  

• struct led_classdev	led_cdev;
  

• } leds[NUM_LEDS];

+};

+static const struct i2c_device_id is31fl319x_id[] = {

• { "is31fl3190", 1 },
• { "is31fl3191", 1 },
• { "is31fl3193", 3 },
• { "is31fl3196", 6 },
• { "is31fl3199", 9 },
• { "sn3199", 9 },
• { }

+}; +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);

+static int is31fl319x_brightness_set(struct led_classdev *led_cdev,

• 		   enum led_brightness brightness)
  

+{

• struct is31fl319x_led *led = container_of(led_cdev,

• 				  struct is31fl319x_led,
  

• 				  led_cdev);
  

• struct is31fl319x_chip *is31 = led->chip;
• int ret;
• int i;
• u8 ctrl1, ctrl2;

You can initialize these variables here.

Ok.

...

• dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),

• brightness);
  

• /* update PWM register */
• ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),

• 	   brightness);
  

• if (ret < 0)

• return ret;
  

• ctrl1 = 0;
• ctrl2 = 0;

And remove above two lines.

...

• /* read current brightness of all PWM channels */
• for (i = 0; i < NUM_LEDS; i++) {

• unsigned int pwm_value;
  

• bool on;
  

• /*
  

•  * since neither led_cdev nor the chip can provide
  

•  * the current setting, we read from the regmap cache
  

•  */
  

• ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
  

• 		  &pwm_value);
  

• dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
  

• 	__func__, i, ret, pwm_value);
  

• on = ret >= 0 && pwm_value > LED_OFF;
  

• if (i < 3)
  

• 	ctrl1 |= on << i;	/* 0..2 => bit 0..2 */
  

• else if (i < 6)
  

• 	ctrl1 |= on << (i+1);	/* 3..5 => bit 4..6 */
  

• else
  

• 	ctrl2 |= on << (i-6);	/* 6..8 => bit 0..2 */
  

• }
• if (ctrl1 > 0 || ctrl2 > 0) {

• dev_dbg(&is31->client->dev, "power up %02x %02x\n",
  

• 	ctrl1, ctrl2);
  

• regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
  

• regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
  

• /* update PWMs */
  

• regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
  

• /* enable chip from shut down */
  

• ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
  

• } else {

• dev_dbg(&is31->client->dev, "power down\n");
  

• /* shut down (no need to clear CTRL1/2) */
  

• ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
  

• }

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

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

...

• return ret;

+}

+static struct led_info * +is31fl319x_parse_dt(struct i2c_client *client, int num_leds) +{

• struct device_node *np = client->dev.of_node, *child;
• struct led_info *is31_leds;
• int count;
• if (!np)

• return ERR_PTR(-ENODEV);
  

• count = of_get_child_count(np);
• dev_dbg(&client->dev, "child count %d\n", count);
• if (!count || count > NUM_LEDS)

• return ERR_PTR(-ENODEV);
  

• is31_leds = devm_kzalloc(&client->dev,

• 	sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
  

• if (!is31_leds)

• return ERR_PTR(-ENOMEM);
  

• for_each_child_of_node(np, child) {

• struct led_info led;
  

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

...

• u32 reg;
  

• int ret;
  

• led.name = NULL;
  

• ret = of_property_read_string(child, "label", &led.name);
  

• if (ret < 0 && ret != -EINVAL)	/* is optional */
  

• 	return ERR_PTR(ret);
  

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

Ok.

...

• led.default_trigger = NULL;
  

• ret = of_property_read_string(child, "linux,default-trigger",
  

• 	&led.default_trigger);
  

Here also please assign the trigger directly to struct led_classdev.

Ok.

...

• if (ret < 0 && ret != -EINVAL)	/* is optional */
  

• 	return ERR_PTR(ret);
  

• led.flags = 0;
  

• ret = of_property_read_u32(child, "reg", &reg);
  

• dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
  

• reg -= 1;	/* index 0 is at reg = 1 */
  

• if (ret != 0 || reg < 0 || reg >= num_leds)
  

• 	continue;
  

• if (is31_leds[reg].name)
  

• 	dev_err(&client->dev, "duplicate led line %d for %s -> %s\n",
  

• 		reg, is31_leds[reg].name, led.name);
  

• is31_leds[reg] = led;
  

• }
• return is31_leds;

+}

+static const struct of_device_id of_is31fl319x_leds_match[] = {

• { .compatible = "issi,is31fl3190", (void *) 1 },
• { .compatible = "issi,is31fl3191", (void *) 1 },
• { .compatible = "issi,is31fl3193", (void *) 3 },
• { .compatible = "issi,is31fl3196", (void *) 6 },
• { .compatible = "issi,is31fl3199", (void *) 9 },
• { .compatible = "si-en,sn3199", (void *) 9 },
• {},

+}; +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);

+static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg) +{ /* we have no readable registers */

• return false;

+}

+static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg) +{ /* volatile registers are not cached */

• switch (reg) {
• case IS31FL319X_DATA_UPDATE:
• case IS31FL319X_TIME_UPDATE:
• case IS31FL319X_RESET:

• return true;	/* always write-through */
  

• default:

• return false;
  

• }

+}

+struct regmap_config regmap_config = {

• .reg_bits = 8,
• .val_bits = 8,
• .max_register = IS31FL319X_REG_CNT,
• .cache_type = REGCACHE_FLAT,
• .readable_reg = is31fl319x_readable_reg,
• .volatile_reg = is31fl319x_volatile_reg,

+};

+static int is31fl319x_probe(struct i2c_client *client,

• const struct i2c_device_id *id)
  

+{

• struct is31fl319x_chip *is31;
• struct i2c_adapter *adapter;
• struct led_info *leds;
• int err;
• int i = 0;
• adapter = to_i2c_adapter(client->dev.parent);
• dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",

• (int) id->driver_data);
  

• if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))

• return -EIO;
  

• leds = is31fl319x_parse_dt(client, (int) id->driver_data);
• if (IS_ERR(leds)) {

• dev_err(&client->dev, "DT parsing error %d\n",
  

• 	(int) PTR_ERR(leds));
  

• return PTR_ERR(leds);
  

• }
• is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
• if (!is31)

• return -ENOMEM;
  

• is31->client = client;
• is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
• if (IS_ERR(is31->regmap)) {

• dev_err(&client->dev, "failed to allocate register map\n");
  

• return PTR_ERR(is31->regmap);
  

• }
• i2c_set_clientdata(client, is31);
• /* check for write-reply from chip (we can't read any registers) */
• err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);

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

Yes, but not a reboot.

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

So what would writing a different register change?

...

• if (err < 0) {

• dev_err(&client->dev, "no response from chip write: err = %d\n",
  

• 	err);
  

• return -EIO;	/* does not answer */
  

• }
• /* initialize chip and regmap so that we never try to read from i2c */

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

Hm,

...

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

• regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
  

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

...

• if (client->dev.of_node) {

• u32 val;
  

• u8 config2 = 0;
  

• if (of_property_read_u32(client->dev.of_node,
  

• 			 "led-max-microamp", &val)) {
  

of_property_read_u32 returns 0 on success.

Ok.

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

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

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

...

• 	if (val > 40000)
  

• 		val = 40000;
  

• 	if (val < 5000)
  

• 		val = 5000;
  

• 	config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
  

• }
  

• if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
  

• 			 &val)) {
  

Please move this to is31fl319x_parse_dt().

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

Would it be acceptable to inline all DT parsing?

...

• 	if (val > 21)
  

• 		val = 21;
  

• 	config2 |= val / 3; /* AGS */
  

• }
  

• regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
  

• }
• for (i = 0; i < NUM_LEDS; i++) {

• struct is31fl319x_led *l = is31->leds + i;
  

• l->chip = is31;
  

• if (leds[i].name && !leds[i].flags) {
  

• 	l->led_cdev.name = leds[i].name;
  

• 	l->led_cdev.default_trigger
  

• 		= leds[i].default_trigger;
  

• 	l->led_cdev.brightness_set_blocking
  

• 		= is31fl319x_brightness_set;
  

• 	/* NOTE: is31fl319x_brightness_set will be called
  

• 	 * immediately after register() before we return
  

• 	 */
  

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

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

...

• 	err = devm_led_classdev_register(&client->dev,
  

• 				    &l->led_cdev);
  

• 	if (err < 0)
  

• 		return err;
  

• }
  

• }
• dev_dbg(&client->dev, "probed\n");

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

Ok.

...

• return 0;

+}

+static struct i2c_driver is31fl319x_driver = {

• .driver = {

• .name    = "leds-is31fl319x",
  

• .of_match_table = of_match_ptr(of_is31fl319x_leds_match),
  

• },
• .probe = is31fl319x_probe,
• .id_table = is31fl319x_id,

+};

+module_i2c_driver(is31fl319x_driver);

+MODULE_AUTHOR("H. Nikolaus Schaller hns@goldelico.com"); +MODULE_DESCRIPTION("IS31FL319X LED driver"); +MODULE_LICENSE("GPL v2");

-- Best regards, Jacek Anaszewski

Will fix asap.

Thanks and BR, Nikolaus Schaller