Changes V2: * suggested by David Rifshin * add more "compatible" strings for other chip variants/brands * renumber output <reg> values to expect a range of 1..9 * fixes for typos and DT example, Kconfig message * fix location in Makefile and Kconfig * remove some dead/not implemented code * use of_property_read_string for better error handling * coding style improvements * use devm_led_classdev_register and simplify error path * suggested by Jacek Anaszewski * fix more typos & writing style * separate bindings document into a second patch * max current property renamed and define uA instead of mA * debugging message improvements * remove platform data and header file completely and therefore require DT * use regmap to handle caching and locking + i2c serialization * suggested by Rob Herring * bindings documentation style improvements
V1: 2016-04-18 20:43:18: This patch adds a driver for the is31fl3196/99 dimmable dual/triple rgb controller chips from ISSI.
H. Nikolaus Schaller (2): drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver Bindings documentation for ISSI is31fl319x driver
.../devicetree/bindings/leds/is31fl319x.txt | 52 +++ drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++ 4 files changed, 415 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt create mode 100644 drivers/leds/leds-is31fl319x.c
This is a driver for the Integrated Silicon Solution Inc. LED driver chips series IS31FL319x. They can drive 1, 3, 6 or up to 9 LEDs.
Each LED is individually controllable in brightness (through pwm) in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
The maximum current of the LEDs can be programmed and limited to 5 .. 40mA through a device tree property.
The chip is connected through I2C and can have one of 4 addresses in the range 0x64 .. 0x67 depending on how the AD pin is connected. The address is defined by the reg property as usual.
The chip also has a shutdown input which could be connected to a GPIO, but this driver uses software shutdown if all LEDs are inactivated.
The chip also has breathing and audio features which are not fully supported by this driver.
Tested-on: OMAP5 based Pyra handheld prototype. Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 363 insertions(+) create mode 100644 drivers/leds/leds-is31fl319x.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 5ae2834..65b1045 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -570,6 +570,14 @@ config LEDS_SEAD3 This driver can also be built as a module. If so the module will be called leds-sead3.
+config LEDS_IS31FL319X + tristate "LED Support for ISSI IS31FL319x I2C LED controller family" + depends on LEDS_CLASS && I2C && OF + help + This option enables support for LEDs connected to ISSI IS31FL319x + fancy LED driver chips accessed via the I2C bus. + Driver supports individual PWM brightness control for each channel. + config LEDS_IS31FL32XX tristate "LED support for ISSI IS31FL32XX I2C LED controller family" depends on LEDS_CLASS && I2C && OF diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index cb2013d..b6ce9f9 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o obj-$(CONFIG_LEDS_SEAD3) += leds-sead3.o +obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
# LED SPI Drivers diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c new file mode 100644 index 0000000..bbf8850 --- /dev/null +++ b/drivers/leds/leds-is31fl319x.c @@ -0,0 +1,354 @@ +/* + * Copyright 2015-16 Golden Delicious Computers + * + * Author: Nikolaus Schaller hns@goldelico.com + * + * Based on leds-tca6507.c + * + * This file is subject to the terms and conditions of version 2 of + * the GNU General Public License. See the file COPYING in the main + * directory of this archive for more details. + * + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light + * effect LEDs. + * + */ + +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +/* register numbers */ +#define IS31FL319X_SHUTDOWN 0x00 +#define IS31FL319X_CTRL1 0x01 +#define IS31FL319X_CTRL2 0x02 +#define IS31FL319X_CONFIG1 0x03 +#define IS31FL319X_CONFIG2 0x04 +#define IS31FL319X_RAMP_MODE 0x05 +#define IS31FL319X_BREATH_MASK 0x06 +#define IS31FL319X_PWM1 0x07 +#define IS31FL319X_PWM2 0x08 +#define IS31FL319X_PWM3 0x09 +#define IS31FL319X_PWM4 0x0a +#define IS31FL319X_PWM5 0x0b +#define IS31FL319X_PWM6 0x0c +#define IS31FL319X_PWM7 0x0d +#define IS31FL319X_PWM8 0x0e +#define IS31FL319X_PWM9 0x0f +#define IS31FL319X_DATA_UPDATE 0x10 +#define IS31FL319X_T0_1 0x11 +#define IS31FL319X_T0_2 0x12 +#define IS31FL319X_T0_3 0x13 +#define IS31FL319X_T0_4 0x14 +#define IS31FL319X_T0_5 0x15 +#define IS31FL319X_T0_6 0x16 +#define IS31FL319X_T0_7 0x17 +#define IS31FL319X_T0_8 0x18 +#define IS31FL319X_T0_9 0x19 +#define IS31FL319X_T123_1 0x1a +#define IS31FL319X_T123_2 0x1b +#define IS31FL319X_T123_3 0x1c +#define IS31FL319X_T4_1 0x1d +#define IS31FL319X_T4_2 0x1e +#define IS31FL319X_T4_3 0x1f +#define IS31FL319X_T4_4 0x20 +#define IS31FL319X_T4_5 0x21 +#define IS31FL319X_T4_6 0x22 +#define IS31FL319X_T4_7 0x23 +#define IS31FL319X_T4_8 0x24 +#define IS31FL319X_T4_9 0x25 +#define IS31FL319X_TIME_UPDATE 0x26 +#define IS31FL319X_RESET 0xff + +#define IS31FL319X_REG_CNT (IS31FL319X_RESET + 1) + +#define NUM_LEDS 9 /* max for 3199 chip */ + +struct is31fl319x_chip { + struct i2c_client *client; + struct regmap *regmap; + + struct is31fl319x_led { + struct is31fl319x_chip *chip; + struct led_classdev led_cdev; + } leds[NUM_LEDS]; +}; + +static const struct i2c_device_id is31fl319x_id[] = { + { "is31fl3190", 1 }, + { "is31fl3191", 1 }, + { "is31fl3193", 3 }, + { "is31fl3196", 6 }, + { "is31fl3199", 9 }, + { "sn3199", 9 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, is31fl319x_id); + +static int is31fl319x_brightness_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct is31fl319x_led *led = container_of(led_cdev, + struct is31fl319x_led, + led_cdev); + struct is31fl319x_chip *is31 = led->chip; + int ret; + + int i; + u8 ctrl1, ctrl2; + + dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds), + brightness); + + /* update PWM register */ + ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds), + brightness); + if (ret < 0) + return ret; + + ctrl1 = 0; + ctrl2 = 0; + + /* read current brightness of all PWM channels */ + for (i = 0; i < NUM_LEDS; i++) { + unsigned int pwm_value; + bool on; + + /* + * since neither led_cdev nor the chip can provide + * the current setting, we read from the regmap cache + */ + + ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i, + &pwm_value); + dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n", + __func__, i, ret, pwm_value); + on = ret >= 0 && pwm_value > LED_OFF; + + if (i < 3) + ctrl1 |= on << i; /* 0..2 => bit 0..2 */ + else if (i < 6) + ctrl1 |= on << (i+1); /* 3..5 => bit 4..6 */ + else + ctrl2 |= on << (i-6); /* 6..8 => bit 0..2 */ + } + + if (ctrl1 > 0 || ctrl2 > 0) { + dev_dbg(&is31->client->dev, "power up %02x %02x\n", + ctrl1, ctrl2); + regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1); + regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2); + /* update PWMs */ + regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00); + /* enable chip from shut down */ + ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01); + } else { + dev_dbg(&is31->client->dev, "power down\n"); + /* shut down (no need to clear CTRL1/2) */ + ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00); + } + + return ret; +} + +static struct led_info * +is31fl319x_parse_dt(struct i2c_client *client, int num_leds) +{ + struct device_node *np = client->dev.of_node, *child; + struct led_info *is31_leds; + int count; + + if (!np) + return ERR_PTR(-ENODEV); + + count = of_get_child_count(np); + dev_dbg(&client->dev, "child count %d\n", count); + if (!count || count > NUM_LEDS) + return ERR_PTR(-ENODEV); + + is31_leds = devm_kzalloc(&client->dev, + sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL); + if (!is31_leds) + return ERR_PTR(-ENOMEM); + + for_each_child_of_node(np, child) { + struct led_info led; + u32 reg; + int ret; + + led.name = NULL; + ret = of_property_read_string(child, "label", &led.name); + if (ret < 0 && ret != -EINVAL) /* is optional */ + return ERR_PTR(ret); + led.default_trigger = NULL; + ret = of_property_read_string(child, "linux,default-trigger", + &led.default_trigger); + if (ret < 0 && ret != -EINVAL) /* is optional */ + return ERR_PTR(ret); + led.flags = 0; + ret = of_property_read_u32(child, "reg", ®); + 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, ®map_config); + if (IS_ERR(is31->regmap)) { + dev_err(&client->dev, "failed to allocate register map\n"); + return PTR_ERR(is31->regmap); + } + + i2c_set_clientdata(client, is31); + + /* check for write-reply from chip (we can't read any registers) */ + err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00); + if (err < 0) { + dev_err(&client->dev, "no response from chip write: err = %d\n", + err); + return -EIO; /* does not answer */ + } + + /* initialize chip and regmap so that we never try to read from i2c */ + regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00); + regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00); + for (i = 0; i < NUM_LEDS; i++) + regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00); + + if (client->dev.of_node) { + u32 val; + u8 config2 = 0; + + if (of_property_read_u32(client->dev.of_node, + "led-max-microamp", &val)) { + if (val > 40000) + val = 40000; + if (val < 5000) + val = 5000; + config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */ + } + if (of_property_read_u32(client->dev.of_node, "audio-gain-db", + &val)) { + if (val > 21) + val = 21; + config2 |= val / 3; /* AGS */ + } + regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2); + } + + for (i = 0; i < NUM_LEDS; i++) { + struct is31fl319x_led *l = is31->leds + i; + + l->chip = is31; + if (leds[i].name && !leds[i].flags) { + l->led_cdev.name = leds[i].name; + l->led_cdev.default_trigger + = leds[i].default_trigger; + l->led_cdev.brightness_set_blocking + = is31fl319x_brightness_set; + /* NOTE: is31fl319x_brightness_set will be called + * immediately after register() before we return + */ + err = devm_led_classdev_register(&client->dev, + &l->led_cdev); + if (err < 0) + return err; + } + } + + dev_dbg(&client->dev, "probed\n"); + return 0; +} + +static struct i2c_driver is31fl319x_driver = { + .driver = { + .name = "leds-is31fl319x", + .of_match_table = of_match_ptr(of_is31fl319x_leds_match), + }, + .probe = is31fl319x_probe, + .id_table = is31fl319x_id, +}; + +module_i2c_driver(is31fl319x_driver); + +MODULE_AUTHOR("H. Nikolaus Schaller hns@goldelico.com"); +MODULE_DESCRIPTION("IS31FL319X LED driver"); +MODULE_LICENSE("GPL v2");
Hi,
[auto build test WARNING on j.anaszewski-leds/for-next] [also build test WARNING on v4.7-rc6 next-20160706] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/H-Nikolaus-Schaller/driver-leds-is3... base: https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git for-next config: mn10300-allmodconfig (attached as .config) compiler: am33_2.0-linux-gcc (GCC) 4.9.0 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/ma... -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mn10300
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:289:0, from include/linux/kernel.h:13, from include/linux/list.h:8, from include/linux/kobject.h:20, from include/linux/device.h:17, from include/linux/i2c.h:30, from drivers/leds/leds-is31fl319x.c:18: drivers/leds/leds-is31fl319x.c: In function 'is31fl319x_brightness_set':
include/linux/dynamic_debug.h:64:16: warning: format '%d' expects argument of type 'int', but argument 5 has type 'long int' [-Wformat=]
static struct _ddebug __aligned(8) \ ^ include/linux/dynamic_debug.h:84:2: note: in expansion of macro 'DEFINE_DYNAMIC_DEBUG_METADATA' DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \ ^ include/linux/device.h:1197:2: note: in expansion of macro 'dynamic_dev_dbg' dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \ ^ drivers/leds/leds-is31fl319x.c:104:2: note: in expansion of macro 'dev_dbg' dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds), ^
vim +64 include/linux/dynamic_debug.h
b48420c1 Jim Cromie 2012-04-27 48 const char *modname); b48420c1 Jim Cromie 2012-04-27 49 cbc46635 Joe Perches 2011-08-11 50 struct device; cbc46635 Joe Perches 2011-08-11 51 b9075fa9 Joe Perches 2011-10-31 52 extern __printf(3, 4) 906d2015 Joe Perches 2014-09-24 53 void __dynamic_dev_dbg(struct _ddebug *descriptor, const struct device *dev, b9075fa9 Joe Perches 2011-10-31 54 const char *fmt, ...); cbc46635 Joe Perches 2011-08-11 55 ffa10cb4 Jason Baron 2011-08-11 56 struct net_device; ffa10cb4 Jason Baron 2011-08-11 57 b9075fa9 Joe Perches 2011-10-31 58 extern __printf(3, 4) 906d2015 Joe Perches 2014-09-24 59 void __dynamic_netdev_dbg(struct _ddebug *descriptor, ffa10cb4 Jason Baron 2011-08-11 60 const struct net_device *dev, b9075fa9 Joe Perches 2011-10-31 61 const char *fmt, ...); ffa10cb4 Jason Baron 2011-08-11 62 07613b0b Jason Baron 2011-10-04 63 #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \ c0d2af63 Joe Perches 2012-10-18 @64 static struct _ddebug __aligned(8) \ 07613b0b Jason Baron 2011-10-04 65 __attribute__((section("__verbose"))) name = { \ 07613b0b Jason Baron 2011-10-04 66 .modname = KBUILD_MODNAME, \ 07613b0b Jason Baron 2011-10-04 67 .function = __func__, \ 07613b0b Jason Baron 2011-10-04 68 .filename = __FILE__, \ 07613b0b Jason Baron 2011-10-04 69 .format = (fmt), \ 07613b0b Jason Baron 2011-10-04 70 .lineno = __LINE__, \ 07613b0b Jason Baron 2011-10-04 71 .flags = _DPRINTK_FLAGS_DEFAULT, \ 07613b0b Jason Baron 2011-10-04 72 }
:::::: The code at line 64 was first introduced by commit :::::: c0d2af637863940b1a4fb208224ca7acb905c39f dynamic_debug: Remove unnecessary __used
:::::: TO: Joe Perches joe@perches.com :::::: CC: Greg Kroah-Hartman gregkh@linuxfoundation.org
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 IS31FL319xfancy LED driver chips accessed via the I2C bus.Driver supports individual PWM brightness control for each channel.- config LEDS_IS31FL32XX tristate "LED support for ISSI IS31FL32XX I2C LED controller family" depends on LEDS_CLASS && I2C && OF
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index cb2013d..b6ce9f9 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o obj-$(CONFIG_LEDS_SEAD3) += leds-sead3.o +obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
# LED SPI Drivers diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c new file mode 100644 index 0000000..bbf8850 --- /dev/null +++ b/drivers/leds/leds-is31fl319x.c @@ -0,0 +1,354 @@ +/*
- Copyright 2015-16 Golden Delicious Computers
- Author: Nikolaus Schaller hns@goldelico.com
- Based on leds-tca6507.c
- This file is subject to the terms and conditions of version 2 of
- the GNU General Public License. See the file COPYING in the main
- directory of this archive for more details.
- LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
- effect LEDs.
- */
+#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regmap.h> +#include <linux/slab.h>
+/* register numbers */ +#define IS31FL319X_SHUTDOWN 0x00 +#define IS31FL319X_CTRL1 0x01 +#define IS31FL319X_CTRL2 0x02 +#define IS31FL319X_CONFIG1 0x03 +#define IS31FL319X_CONFIG2 0x04 +#define IS31FL319X_RAMP_MODE 0x05 +#define IS31FL319X_BREATH_MASK 0x06 +#define IS31FL319X_PWM1 0x07 +#define IS31FL319X_PWM2 0x08 +#define IS31FL319X_PWM3 0x09 +#define IS31FL319X_PWM4 0x0a +#define IS31FL319X_PWM5 0x0b +#define IS31FL319X_PWM6 0x0c +#define IS31FL319X_PWM7 0x0d +#define IS31FL319X_PWM8 0x0e +#define IS31FL319X_PWM9 0x0f +#define IS31FL319X_DATA_UPDATE 0x10 +#define IS31FL319X_T0_1 0x11 +#define IS31FL319X_T0_2 0x12 +#define IS31FL319X_T0_3 0x13 +#define IS31FL319X_T0_4 0x14 +#define IS31FL319X_T0_5 0x15 +#define IS31FL319X_T0_6 0x16 +#define IS31FL319X_T0_7 0x17 +#define IS31FL319X_T0_8 0x18 +#define IS31FL319X_T0_9 0x19 +#define IS31FL319X_T123_1 0x1a +#define IS31FL319X_T123_2 0x1b +#define IS31FL319X_T123_3 0x1c +#define IS31FL319X_T4_1 0x1d +#define IS31FL319X_T4_2 0x1e +#define IS31FL319X_T4_3 0x1f +#define IS31FL319X_T4_4 0x20 +#define IS31FL319X_T4_5 0x21 +#define IS31FL319X_T4_6 0x22 +#define IS31FL319X_T4_7 0x23 +#define IS31FL319X_T4_8 0x24 +#define IS31FL319X_T4_9 0x25 +#define IS31FL319X_TIME_UPDATE 0x26 +#define IS31FL319X_RESET 0xff
+#define IS31FL319X_REG_CNT (IS31FL319X_RESET + 1)
+#define NUM_LEDS 9 /* max for 3199 chip */
+struct is31fl319x_chip {
- struct i2c_client *client;
- struct regmap *regmap;
- struct is31fl319x_led {
struct is31fl319x_chip *chip;struct led_classdev led_cdev;- } leds[NUM_LEDS];
+};
+static const struct i2c_device_id is31fl319x_id[] = {
- { "is31fl3190", 1 },
- { "is31fl3191", 1 },
- { "is31fl3193", 3 },
- { "is31fl3196", 6 },
- { "is31fl3199", 9 },
- { "sn3199", 9 },
- { }
+}; +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
+static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
enum led_brightness brightness)+{
- struct is31fl319x_led *led = container_of(led_cdev,
struct is31fl319x_led,led_cdev);- struct is31fl319x_chip *is31 = led->chip;
- int ret;
- int i;
- u8 ctrl1, ctrl2;
You can initialize these variables here.
- dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
brightness);- /* update PWM register */
- ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
brightness);- if (ret < 0)
return ret;- ctrl1 = 0;
- ctrl2 = 0;
And remove above two lines.
- /* read current brightness of all PWM channels */
- for (i = 0; i < NUM_LEDS; i++) {
unsigned int pwm_value;bool on;/** since neither led_cdev nor the chip can provide* the current setting, we read from the regmap cache*/ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,&pwm_value);dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",__func__, i, ret, pwm_value);on = ret >= 0 && pwm_value > LED_OFF;if (i < 3)ctrl1 |= on << i; /* 0..2 => bit 0..2 */else if (i < 6)ctrl1 |= on << (i+1); /* 3..5 => bit 4..6 */elsectrl2 |= on << (i-6); /* 6..8 => bit 0..2 */- }
- if (ctrl1 > 0 || ctrl2 > 0) {
dev_dbg(&is31->client->dev, "power up %02x %02x\n",ctrl1, ctrl2);regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);/* update PWMs */regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);/* enable chip from shut down */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);- } else {
dev_dbg(&is31->client->dev, "power down\n");/* shut down (no need to clear CTRL1/2) */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);- }
You need a mutex protection in this function, so as to prevent other processes from jumping in in the middle of setting up a brightness, which requires writing several registers.
- return ret;
+}
+static struct led_info * +is31fl319x_parse_dt(struct i2c_client *client, int num_leds) +{
- struct device_node *np = client->dev.of_node, *child;
- struct led_info *is31_leds;
- int count;
- if (!np)
return ERR_PTR(-ENODEV);- count = of_get_child_count(np);
- dev_dbg(&client->dev, "child count %d\n", count);
- if (!count || count > NUM_LEDS)
return ERR_PTR(-ENODEV);- is31_leds = devm_kzalloc(&client->dev,
sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);- if (!is31_leds)
return ERR_PTR(-ENOMEM);- for_each_child_of_node(np, child) {
struct led_info led;
Using this local variable adds too much noise here. I suggest avoiding the use of struct led_info. It is semantically inconsistent in the context of DT parsing, since you don't have any flags to parse.
u32 reg;int ret;led.name = NULL;ret = of_property_read_string(child, "label", &led.name);if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);
You could assign the name directly to related struct led_classdev property. Moreover, if label is not present the child node name should be used for LED class device name. Please refer to the other drivers and common LED bindings.
led.default_trigger = NULL;ret = of_property_read_string(child, "linux,default-trigger",&led.default_trigger);
Here also please assign the trigger directly to struct led_classdev.
if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);led.flags = 0;ret = of_property_read_u32(child, "reg", ®);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, ®map_config);
- if (IS_ERR(is31->regmap)) {
dev_err(&client->dev, "failed to allocate register map\n");return PTR_ERR(is31->regmap);- }
- i2c_set_clientdata(client, is31);
- /* check for write-reply from chip (we can't read any registers) */
- err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
Doesn't powering the device up result in loading registers with default values?
- if (err < 0) {
dev_err(&client->dev, "no response from chip write: err = %d\n",err);return -EIO; /* does not answer */- }
- /* initialize chip and regmap so that we never try to read from i2c */
You can initialize default register values without writing them, by using regmap's reg_defaults property.
- regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
- regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
- for (i = 0; i < NUM_LEDS; i++)
regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);- if (client->dev.of_node) {
u32 val;u8 config2 = 0;if (of_property_read_u32(client->dev.of_node,"led-max-microamp", &val)) {
of_property_read_u32 returns 0 on success.
led-max-microamp needs to be defined per each sub-LED (child node). The greatest value should be used for setting CS bit field of CTRL2 register, and max_brightness values of the LEDs with lower max microamp value should be limited accordingly.
Moreover, this is still DT parsing and should find its place in is31fl319x_parse_dt().
if (val > 40000)val = 40000;if (val < 5000)val = 5000;config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */}if (of_property_read_u32(client->dev.of_node, "audio-gain-db",&val)) {
Please move this to is31fl319x_parse_dt().
if (val > 21)val = 21;config2 |= val / 3; /* AGS */}regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);- }
- for (i = 0; i < NUM_LEDS; i++) {
struct is31fl319x_led *l = is31->leds + i;l->chip = is31;if (leds[i].name && !leds[i].flags) {l->led_cdev.name = leds[i].name;l->led_cdev.default_trigger= leds[i].default_trigger;l->led_cdev.brightness_set_blocking= is31fl319x_brightness_set;/* NOTE: is31fl319x_brightness_set will be called* immediately after register() before we return*/
Who will call it? I believe that this is true only if default-on trigger is registered.
err = devm_led_classdev_register(&client->dev,&l->led_cdev);if (err < 0)return err;}- }
- dev_dbg(&client->dev, "probed\n");
Please remove this debug log. You can verify it by checking presence of the device in the sysfs.
- return 0;
+}
+static struct i2c_driver is31fl319x_driver = {
- .driver = {
.name = "leds-is31fl319x",.of_match_table = of_match_ptr(of_is31fl319x_leds_match),- },
- .probe = is31fl319x_probe,
- .id_table = is31fl319x_id,
+};
+module_i2c_driver(is31fl319x_driver);
+MODULE_AUTHOR("H. Nikolaus Schaller hns@goldelico.com"); +MODULE_DESCRIPTION("IS31FL319X LED driver"); +MODULE_LICENSE("GPL v2");
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 IS31FL319xfancy 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 */elsectrl2 |= 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", ®);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, ®map_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
Hi Nikolaus,
On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
Hi Jacek,
Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski j.anaszewski@samsung.com:
Hi Nikolaus,
Thanks for the updated version. Some comments below.
On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
This is a driver for the Integrated Silicon Solution Inc. LED driver chips series IS31FL319x. They can drive 1, 3, 6 or up to 9 LEDs.
Each LED is individually controllable in brightness (through pwm) in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
The maximum current of the LEDs can be programmed and limited to 5 .. 40mA through a device tree property.
The chip is connected through I2C and can have one of 4 addresses in the range 0x64 .. 0x67 depending on how the AD pin is connected. The address is defined by the reg property as usual.
The chip also has a shutdown input which could be connected to a GPIO, but this driver uses software shutdown if all LEDs are inactivated.
The chip also has breathing and audio features which are not fully supported by this driver.
Tested-on: OMAP5 based Pyra handheld prototype. Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 363 insertions(+) create mode 100644 drivers/leds/leds-is31fl319x.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 5ae2834..65b1045 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -570,6 +570,14 @@ config LEDS_SEAD3 This driver can also be built as a module. If so the module will be called leds-sead3.
+config LEDS_IS31FL319X
- tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
- depends on LEDS_CLASS && I2C && OF
- help
This option enables support for LEDs connected to ISSI IS31FL319xfancy 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 */elsectrl2 |= on << (i-6); /* 6..8 => bit 0..2 */- }
- if (ctrl1 > 0 || ctrl2 > 0) {
dev_dbg(&is31->client->dev, "power up %02x %02x\n",ctrl1, ctrl2);regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);/* update PWMs */regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);/* enable chip from shut down */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);- } else {
dev_dbg(&is31->client->dev, "power down\n");/* shut down (no need to clear CTRL1/2) */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);- }
You need a mutex protection in this function, so as to prevent other processes from jumping in in the middle of setting up a brightness, which requires writing several registers.
Uh? I thought that registering is31fl319x_brightness_set as led_cdev.brightness_set_blocking does the trick that we don't need a private mutex here because it can be called only once anyways. At least that was suggested during v1 review.
Blocking doesn't mean that no one else can call the function in the same time, but to the fact that the caller can be blocked for the time needed for satisfying some condition. e.g. completion of I2C transmission. It needs different treatment in the LED core, as brightness setting can't sleep in general, due to the fact, that it can be called from atomic context. In this case LED core delegates brightness setting to a work queue task and led_set_brightness() returns without blocking the caller.
Excerpt from linux/leds.h:
=================================================================
/* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers * that can sleep while setting brightness. */ void (*brightness_set)(struct led_classdev *led_cdev, enum led_brightness brightness); /* * Set LED brightness level immediately - it can block the caller for * the time required for accessing a LED device register. */ int (*brightness_set_blocking)(struct led_classdev *led_cdev, enum led_brightness brightness);
=================================================================
In the [1] I suggested the following :
"You can serialize the operations in brightness_set_blocking with a mutex."
- return ret;
+}
+static struct led_info * +is31fl319x_parse_dt(struct i2c_client *client, int num_leds) +{
- struct device_node *np = client->dev.of_node, *child;
- struct led_info *is31_leds;
- int count;
- if (!np)
return ERR_PTR(-ENODEV);- count = of_get_child_count(np);
- dev_dbg(&client->dev, "child count %d\n", count);
- if (!count || count > NUM_LEDS)
return ERR_PTR(-ENODEV);- is31_leds = devm_kzalloc(&client->dev,
sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);- if (!is31_leds)
return ERR_PTR(-ENOMEM);- for_each_child_of_node(np, child) {
struct led_info led;Using this local variable adds too much noise here. I suggest avoiding the use of struct led_info. It is semantically inconsistent in the context of DT parsing, since you don't have any flags to parse.
u32 reg;int ret;led.name = NULL;ret = of_property_read_string(child, "label", &led.name);if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);You could assign the name directly to related struct led_classdev property. Moreover, if label is not present the child node name should be used for LED class device name. Please refer to the other drivers and common LED bindings.
Ok.
led.default_trigger = NULL;ret = of_property_read_string(child, "linux,default-trigger",&led.default_trigger);Here also please assign the trigger directly to struct led_classdev.
Ok.
if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);led.flags = 0;ret = of_property_read_u32(child, "reg", ®);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, ®map_config);
- if (IS_ERR(is31->regmap)) {
dev_err(&client->dev, "failed to allocate register map\n");return PTR_ERR(is31->regmap);- }
- i2c_set_clientdata(client, is31);
- /* check for write-reply from chip (we can't read any registers) */
- err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
Doesn't powering the device up result in loading registers with default values?
Yes, but not a reboot.
And we have to write *some* register anyways here to be able to detect if the chip is present or does not respond.
So what would writing a different register change?
OK, let's leave that as is.
- if (err < 0) {
dev_err(&client->dev, "no response from chip write: err = %d\n",err);return -EIO; /* does not answer */- }
- /* initialize chip and regmap so that we never try to read from i2c */
You can initialize default register values without writing them, by using regmap's reg_defaults property.
Hm,
- regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
- regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
- for (i = 0; i < NUM_LEDS; i++)
regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);would replace a small loop with a bigger table. But if you prefer this I can change.
Writing registers only to initialize regmap cache looks odd. Moreover it unnecessarily extends driver probing time.
- if (client->dev.of_node) {
u32 val;u8 config2 = 0;if (of_property_read_u32(client->dev.of_node,"led-max-microamp", &val)) {of_property_read_u32 returns 0 on success.
Ok.
led-max-microamp needs to be defined per each sub-LED (child node). The greatest value should be used for setting CS bit field of CTRL2 register, and max_brightness values of the LEDs with lower max microamp value should be limited accordingly.
Hm. The chip does not allow to limit individual LEDs. So we should take the *minimum* of all subnodes.
But the LED core does. That's a max_brightness property is for.
Moreover, this is still DT parsing and should find its place in is31fl319x_parse_dt().
if (val > 40000)val = 40000;if (val < 5000)val = 5000;config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */}if (of_property_read_u32(client->dev.of_node, "audio-gain-db",&val)) {Please move this to is31fl319x_parse_dt().
Well, the main reason is that this needs to introduce another temporary struct to pass values from parse_dt to probe and copy values there.
Would it be acceptable to inline all DT parsing?
Don't reinvent the wheel. Please compare how other LED class drivers perform DT parsing.
if (val > 21)val = 21;config2 |= val / 3; /* AGS */}regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);- }
- for (i = 0; i < NUM_LEDS; i++) {
struct is31fl319x_led *l = is31->leds + i;l->chip = is31;if (leds[i].name && !leds[i].flags) {l->led_cdev.name = leds[i].name;l->led_cdev.default_trigger= leds[i].default_trigger;l->led_cdev.brightness_set_blocking= is31fl319x_brightness_set;/* NOTE: is31fl319x_brightness_set will be called* immediately after register() before we return*/Who will call it? I believe that this is true only if default-on trigger is registered.
Yes, as soon as anyone can call it we must be aware.
This is common for all LED class devices. Please drop the comment.
err = devm_led_classdev_register(&client->dev,&l->led_cdev);if (err < 0)return err;}- }
- dev_dbg(&client->dev, "probed\n");
Please remove this debug log. You can verify it by checking presence of the device in the sysfs.
Ok.
- return 0;
+}
+static struct i2c_driver is31fl319x_driver = {
- .driver = {
.name = "leds-is31fl319x",.of_match_table = of_match_ptr(of_is31fl319x_leds_match),- },
- .probe = is31fl319x_probe,
- .id_table = is31fl319x_id,
+};
+module_i2c_driver(is31fl319x_driver);
+MODULE_AUTHOR("H. Nikolaus Schaller hns@goldelico.com"); +MODULE_DESCRIPTION("IS31FL319X LED driver"); +MODULE_LICENSE("GPL v2");
-- Best regards, Jacek Anaszewski
Will fix asap.
Thanks and BR, Nikolaus Schaller
-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[1] https://lkml.org/lkml/2016/4/20/754
Hi Andrey, I think this is a good point where you can jump in and help to edit the driver patch.
Would that be ok?
I have prepared a -v3 branch (I have a GUI tool where I just needed one click to do that):
http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hn...
A draft for the Changes note for [PATCH 0/n]:
Changes V3: * coding style improvements * added a mutex to properly serialize multiple calls to set_backlight * use regmap defaults * improve DT parsing
I think we can just pile up fixes and finally merge them into the first commit.
BR, Nikolaus
Anfang der weitergeleiteten Nachricht:
Von: Jacek Anaszewski j.anaszewski@samsung.com Betreff: Aw: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver Datum: 7. Juli 2016 um 13:32:41 MESZ An: "H. Nikolaus Schaller" hns@goldelico.com Kopie: Rob Herring robh+dt@kernel.org, Pawel Moll pawel.moll@arm.com, Mark Rutland mark.rutland@arm.com, Ian Campbell ijc+devicetree@hellion.org.uk, Kumar Gala galak@codeaurora.org, Richard Purdie rpurdie@rpsys.net, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org, kernel@pyra-handheld.com, marek@goldelico.com, letux-kernel@openphoenux.org
Hi Nikolaus,
On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
Hi Jacek,
Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski j.anaszewski@samsung.com:
Hi Nikolaus,
Thanks for the updated version. Some comments below.
On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
This is a driver for the Integrated Silicon Solution Inc. LED driver chips series IS31FL319x. They can drive 1, 3, 6 or up to 9 LEDs.
Each LED is individually controllable in brightness (through pwm) in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
The maximum current of the LEDs can be programmed and limited to 5 .. 40mA through a device tree property.
The chip is connected through I2C and can have one of 4 addresses in the range 0x64 .. 0x67 depending on how the AD pin is connected. The address is defined by the reg property as usual.
The chip also has a shutdown input which could be connected to a GPIO, but this driver uses software shutdown if all LEDs are inactivated.
The chip also has breathing and audio features which are not fully supported by this driver.
Tested-on: OMAP5 based Pyra handheld prototype. Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 363 insertions(+) create mode 100644 drivers/leds/leds-is31fl319x.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 5ae2834..65b1045 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -570,6 +570,14 @@ config LEDS_SEAD3 This driver can also be built as a module. If so the module will be called leds-sead3.
+config LEDS_IS31FL319X
- tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
- depends on LEDS_CLASS && I2C && OF
- help
This option enables support for LEDs connected to ISSI IS31FL319xfancy 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 */elsectrl2 |= on << (i-6); /* 6..8 => bit 0..2 */- }
- if (ctrl1 > 0 || ctrl2 > 0) {
dev_dbg(&is31->client->dev, "power up %02x %02x\n",ctrl1, ctrl2);regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);/* update PWMs */regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);/* enable chip from shut down */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);- } else {
dev_dbg(&is31->client->dev, "power down\n");/* shut down (no need to clear CTRL1/2) */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);- }
You need a mutex protection in this function, so as to prevent other processes from jumping in in the middle of setting up a brightness, which requires writing several registers.
Uh? I thought that registering is31fl319x_brightness_set as led_cdev.brightness_set_blocking does the trick that we don't need a private mutex here because it can be called only once anyways. At least that was suggested during v1 review.
Blocking doesn't mean that no one else can call the function in the same time, but to the fact that the caller can be blocked for the time needed for satisfying some condition. e.g. completion of I2C transmission. It needs different treatment in the LED core, as brightness setting can't sleep in general, due to the fact, that it can be called from atomic context. In this case LED core delegates brightness setting to a work queue task and led_set_brightness() returns without blocking the caller.
Excerpt from linux/leds.h:
=================================================================
/* Set LED brightness level
- Must not sleep. Use brightness_set_blocking for drivers
- that can sleep while setting brightness.
*/ void (*brightness_set)(struct led_classdev *led_cdev, enum led_brightness brightness); /*
- Set LED brightness level immediately - it can block the caller for
- the time required for accessing a LED device register.
*/ int (*brightness_set_blocking)(struct led_classdev *led_cdev, enum led_brightness brightness);
=================================================================
In the [1] I suggested the following :
"You can serialize the operations in brightness_set_blocking with a mutex."
- return ret;
+}
+static struct led_info * +is31fl319x_parse_dt(struct i2c_client *client, int num_leds) +{
- struct device_node *np = client->dev.of_node, *child;
- struct led_info *is31_leds;
- int count;
- if (!np)
return ERR_PTR(-ENODEV);- count = of_get_child_count(np);
- dev_dbg(&client->dev, "child count %d\n", count);
- if (!count || count > NUM_LEDS)
return ERR_PTR(-ENODEV);- is31_leds = devm_kzalloc(&client->dev,
sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);- if (!is31_leds)
return ERR_PTR(-ENOMEM);- for_each_child_of_node(np, child) {
struct led_info led;Using this local variable adds too much noise here. I suggest avoiding the use of struct led_info. It is semantically inconsistent in the context of DT parsing, since you don't have any flags to parse.
u32 reg;int ret;led.name = NULL;ret = of_property_read_string(child, "label", &led.name);if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);You could assign the name directly to related struct led_classdev property. Moreover, if label is not present the child node name should be used for LED class device name. Please refer to the other drivers and common LED bindings.
Ok.
led.default_trigger = NULL;ret = of_property_read_string(child, "linux,default-trigger",&led.default_trigger);Here also please assign the trigger directly to struct led_classdev.
Ok.
if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);led.flags = 0;ret = of_property_read_u32(child, "reg", ®);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, ®map_config);
- if (IS_ERR(is31->regmap)) {
dev_err(&client->dev, "failed to allocate register map\n");return PTR_ERR(is31->regmap);- }
- i2c_set_clientdata(client, is31);
- /* check for write-reply from chip (we can't read any registers) */
- err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
Doesn't powering the device up result in loading registers with default values?
Yes, but not a reboot.
And we have to write *some* register anyways here to be able to detect if the chip is present or does not respond.
So what would writing a different register change?
OK, let's leave that as is.
- if (err < 0) {
dev_err(&client->dev, "no response from chip write: err = %d\n",err);return -EIO; /* does not answer */- }
- /* initialize chip and regmap so that we never try to read from i2c */
You can initialize default register values without writing them, by using regmap's reg_defaults property.
Hm,
- regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
- regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
- for (i = 0; i < NUM_LEDS; i++)
regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);would replace a small loop with a bigger table. But if you prefer this I can change.
Writing registers only to initialize regmap cache looks odd. Moreover it unnecessarily extends driver probing time.
- if (client->dev.of_node) {
u32 val;u8 config2 = 0;if (of_property_read_u32(client->dev.of_node,"led-max-microamp", &val)) {of_property_read_u32 returns 0 on success.
Ok.
led-max-microamp needs to be defined per each sub-LED (child node). The greatest value should be used for setting CS bit field of CTRL2 register, and max_brightness values of the LEDs with lower max microamp value should be limited accordingly.
Hm. The chip does not allow to limit individual LEDs. So we should take the *minimum* of all subnodes.
But the LED core does. That's a max_brightness property is for.
Moreover, this is still DT parsing and should find its place in is31fl319x_parse_dt().
if (val > 40000)val = 40000;if (val < 5000)val = 5000;config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */}if (of_property_read_u32(client->dev.of_node, "audio-gain-db",&val)) {Please move this to is31fl319x_parse_dt().
Well, the main reason is that this needs to introduce another temporary struct to pass values from parse_dt to probe and copy values there.
Would it be acceptable to inline all DT parsing?
Don't reinvent the wheel. Please compare how other LED class drivers perform DT parsing.
if (val > 21)val = 21;config2 |= val / 3; /* AGS */}regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);- }
- for (i = 0; i < NUM_LEDS; i++) {
struct is31fl319x_led *l = is31->leds + i;l->chip = is31;if (leds[i].name && !leds[i].flags) {l->led_cdev.name = leds[i].name;l->led_cdev.default_trigger= leds[i].default_trigger;l->led_cdev.brightness_set_blocking= is31fl319x_brightness_set;/* NOTE: is31fl319x_brightness_set will be called* immediately after register() before we return*/Who will call it? I believe that this is true only if default-on trigger is registered.
Yes, as soon as anyone can call it we must be aware.
This is common for all LED class devices. Please drop the comment.
err = devm_led_classdev_register(&client->dev,&l->led_cdev);if (err < 0)return err;}- }
- dev_dbg(&client->dev, "probed\n");
Please remove this debug log. You can verify it by checking presence of the device in the sysfs.
Ok.
- return 0;
+}
+static struct i2c_driver is31fl319x_driver = {
- .driver = {
.name = "leds-is31fl319x",.of_match_table = of_match_ptr(of_is31fl319x_leds_match),- },
- .probe = is31fl319x_probe,
- .id_table = is31fl319x_id,
+};
+module_i2c_driver(is31fl319x_driver);
+MODULE_AUTHOR("H. Nikolaus Schaller hns@goldelico.com"); +MODULE_DESCRIPTION("IS31FL319X LED driver"); +MODULE_LICENSE("GPL v2");
-- Best regards, Jacek Anaszewski
Will fix asap.
Thanks and BR, Nikolaus Schaller
-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[1] https://lkml.org/lkml/2016/4/20/754
-- Best regards, Jacek Anaszewski
On Thu, Jul 07, 2016 at 02:39:49PM +0200, H. Nikolaus Schaller wrote:
Hi Andrey, I think this is a good point where you can jump in and help to edit the driver patch.
Would that be ok?
I have prepared a -v3 branch (I have a GUI tool where I just needed one click to do that):
http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hn...
Ok, jumping in. Thanks for turning to me for this. Expect some results in few hours.
A draft for the Changes note for [PATCH 0/n]:
Changes V3:
- coding style improvements
Done (something).
- added a mutex to properly serialize multiple calls to set_backlight
Done.
- use regmap defaults
TODO.
- improve DT parsing
In progress.
I think we can just pile up fixes and finally merge them into the first commit.
On Thu, Jul 07, 2016 at 08:47:34PM +0300, Andrey Utkin wrote:
On Thu, Jul 07, 2016 at 02:39:49PM +0200, H. Nikolaus Schaller wrote:
Hi Andrey, I think this is a good point where you can jump in and help to edit the driver patch.
Would that be ok?
I have prepared a -v3 branch (I have a GUI tool where I just needed one click to do that):
http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hn...
Ok, jumping in. Thanks for turning to me for this. Expect some results in few hours.
A draft for the Changes note for [PATCH 0/n]:
Changes V3:
- coding style improvements
Done (something).
- added a mutex to properly serialize multiple calls to set_backlight
Done.
- use regmap defaults
TODO.
- improve DT parsing
In progress.
I think we can just pile up fixes and finally merge them into the first commit.
I hope I've fixed all points of review.
Please check my changes in work/andrey/leds/is31fl319x branch. http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/an...
The last one, "led: is31fl319x: Replace all tabs in the middle of lines with spaces", may be controversal, but I believe it is right thing to do (i.e. to have tabs in the middle of lines is wrong thing to do). Honestly, I have an idea that indentation style currently enforced by checkpatch.pl is defective regarding abuse of tabs, and discouragement of spaces in exact places where they fit better than tabs. But that's unrelated; I think I'll raise this discussion in LKML some day :)
What is important is that checkpatch.pl likes how the driver looks after all of my changes.
Hi Andrey,
Am 07.07.2016 um 23:47 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Thu, Jul 07, 2016 at 08:47:34PM +0300, Andrey Utkin wrote:
On Thu, Jul 07, 2016 at 02:39:49PM +0200, H. Nikolaus Schaller wrote:
Hi Andrey, I think this is a good point where you can jump in and help to edit the driver patch.
Would that be ok?
I have prepared a -v3 branch (I have a GUI tool where I just needed one click to do that):
http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hn...
Ok, jumping in. Thanks for turning to me for this. Expect some results in few hours.
A draft for the Changes note for [PATCH 0/n]:
Changes V3:
- coding style improvements
Done (something).
- added a mutex to properly serialize multiple calls to set_backlight
Done.
- use regmap defaults
TODO.
- improve DT parsing
In progress.
I think we can just pile up fixes and finally merge them into the first commit.
I hope I've fixed all points of review.
Not only those, but 200% more :)
I will look through them and comment (if necessary).
Please check my changes in work/andrey/leds/is31fl319x branch. http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/an...
Interesting that your checkpatch did report more than mine...
The last one, "led: is31fl319x: Replace all tabs in the middle of lines with spaces", may be controversal, but I believe it is right thing to do (i.e. to have tabs in the middle of lines is wrong thing to do). Honestly, I have an idea that indentation style currently enforced by checkpatch.pl is defective regarding abuse of tabs, and discouragement of spaces in exact places where they fit better than tabs. But that's unrelated; I think I'll raise this discussion in LKML some day :)
Yes, even tools like checkpatch are not perfect. For example the warning about missing Kconfig description. I had simply ignored that warning...
What is important is that checkpatch.pl likes how the driver looks after all of my changes.
I will merge & compile & test asap and if ok resubmit.
Thanks and BR, Nikolaus
Hi Andrey,
Am 08.07.2016 um 08:43 schrieb H. Nikolaus Schaller hns@goldelico.com:
Hi Andrey,
Am 07.07.2016 um 23:47 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Thu, Jul 07, 2016 at 08:47:34PM +0300, Andrey Utkin wrote:
On Thu, Jul 07, 2016 at 02:39:49PM +0200, H. Nikolaus Schaller wrote:
Hi Andrey, I think this is a good point where you can jump in and help to edit the driver patch.
Would that be ok?
I have prepared a -v3 branch (I have a GUI tool where I just needed one click to do that):
http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hn...
Ok, jumping in. Thanks for turning to me for this. Expect some results in few hours.
A draft for the Changes note for [PATCH 0/n]:
Changes V3:
- coding style improvements
Done (something).
- added a mutex to properly serialize multiple calls to set_backlight
Done.
- use regmap defaults
TODO.
- improve DT parsing
In progress.
I think we can just pile up fixes and finally merge them into the first commit.
I hope I've fixed all points of review.
Not only those, but 200% more :)
I will look through them and comment (if necessary).
Please check my changes in work/andrey/leds/is31fl319x branch. http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/an...
Interesting that your checkpatch did report more than mine...
The last one, "led: is31fl319x: Replace all tabs in the middle of lines with spaces", may be controversal, but I believe it is right thing to do (i.e. to have tabs in the middle of lines is wrong thing to do). Honestly, I have an idea that indentation style currently enforced by checkpatch.pl is defective regarding abuse of tabs, and discouragement of spaces in exact places where they fit better than tabs. But that's unrelated; I think I'll raise this discussion in LKML some day :)
Yes, even tools like checkpatch are not perfect. For example the warning about missing Kconfig description. I had simply ignored that warning...
What is important is that checkpatch.pl likes how the driver looks after all of my changes.
I will merge & compile & test asap and if ok resubmit.
Seems to work with no noticeable differences. Maybe because we are not using microcampere settings at all. So I can't test bugs here...
Generally I like the approach to pass the struct is31fl319x_led into the parse_dt().
One thing I don't understand exactly is why you did replace the formula by a switch in is31fl319x_microamp_to_cs. The formula is designed to accept any value and never report "invalid microamperes" but simply round down to the next available value.
Since we don't use it for the Pyra (and maybe never will need it), I don't really care how this functionality will evolve in the future...
Another minor thing is that I think we should also add IS31FL319X_CTRL1 and IS31FL319X_CTRL2 to the reg_default setup.
I will add that myself, squeeze everything into the original patches, add a description of your changes to the patch 0/n message and resubmit.
BR and thanks!
Nikolaus
Thanks and BR, Nikolaus
Kernel mailing list Kernel@pyra-handheld.com http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel
On Fri, Jul 08, 2016 at 08:25:25PM +0200, H. Nikolaus Schaller wrote:
Seems to work with no noticeable differences. Maybe because we are not using microcampere settings at all. So I can't test bugs here...
Generally I like the approach to pass the struct is31fl319x_led into the parse_dt().
One thing I don't understand exactly is why you did replace the formula by a switch in is31fl319x_microamp_to_cs. The formula is designed to accept any value and never report "invalid microamperes" but simply round down to the next available value.
Well, now I see I have underestimated your implementation. The expression
(64000 - aggregated_led_microamp) / 5000 & 0x7
feels wrong in many ways to me. In fact, it was just misleading and counter-intuitive. It must be my lack of mathematical capabilities, but it was hard for me to figure out that your formula works correctly. I didn't even consider such outcome that the formula is right. I went straight to rewriting it. Ridiculous :)
Well, 64000 blows the mind. It is not dividable by 5000, it's outside of the working range.
Then, I assumed that & 0x7 (with subseq. << 4) is just a safety capping, so that when you write to config2 you declare that you touch only bits 4..6. Turns out I was wrong and this masking is a part of arithmetical formula.
Then, a quick look at datasheet reveals that CS values for microamperes don't grow linearly: 0 => 20000, 3 => 5000, 7 => 25000. I understood that this could be turned to arithmetic formula but since the working range is so short, I decided to go fully hardcoded to save some trial and error (which I do all the time and of which I'm tired of).
So let's tabulate your function:
(64000 - aggregated_led_microamp) / 5000
$ for microamp in `seq 5000 5000 40000`; do EXPR="(64000 - $microamp) / 5000 % 8"; echo "$EXPR = `echo $EXPR | bc`"; done (64000 - 5000) / 5000 % 8 = 3 (64000 - 10000) / 5000 % 8 = 2 (64000 - 15000) / 5000 % 8 = 1 (64000 - 20000) / 5000 % 8 = 0 (64000 - 25000) / 5000 % 8 = 7 (64000 - 30000) / 5000 % 8 = 6 (64000 - 35000) / 5000 % 8 = 5 (64000 - 40000) / 5000 % 8 = 4
So feel free to drop the patch which replaces it, and also drop the line added in previous patch "/* TODO FIXME calculation seems wrong */"
Since we don't use it for the Pyra (and maybe never will need it), I don't really care how this functionality will evolve in the future...
Then what's the device it's present in? GTA04?
Another minor thing is that I think we should also add IS31FL319X_CTRL1 and IS31FL319X_CTRL2 to the reg_default setup.
Fine, I just thought those need to be actually written to device at that stage, so didn't replace these occurances. It felt so from conversation at review.
I will add that myself, squeeze everything into the original patches, add a description of your changes to the patch 0/n message and resubmit.
Fine.
HI,
Am 08.07.2016 um 22:35 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Fri, Jul 08, 2016 at 08:25:25PM +0200, H. Nikolaus Schaller wrote:
Seems to work with no noticeable differences. Maybe because we are not using microcampere settings at all. So I can't test bugs here...
Generally I like the approach to pass the struct is31fl319x_led into the parse_dt().
One thing I don't understand exactly is why you did replace the formula by a switch in is31fl319x_microamp_to_cs. The formula is designed to accept any value and never report "invalid microamperes" but simply round down to the next available value.
Well, now I see I have underestimated your implementation. The expression
(64000 - aggregated_led_microamp) / 5000 & 0x7
feels wrong in many ways to me. In fact, it was just misleading and counter-intuitive. It must be my lack of mathematical capabilities, but it was hard for me to figure out that your formula works correctly. I didn't even consider such outcome that the formula is right. I went straight to rewriting it. Ridiculous :)
Well, 64000 blows the mind. It is not dividable by 5000, it's outside of the working range.
Then, I assumed that & 0x7 (with subseq. << 4) is just a safety capping, so that when you write to config2 you declare that you touch only bits 4..6. Turns out I was wrong and this masking is a part of arithmetical formula.
Then, a quick look at datasheet reveals that CS values for microamperes don't grow linearly: 0 => 20000, 3 => 5000, 7 => 25000. I understood that this could be turned to arithmetic formula but since the working range is so short, I decided to go fully hardcoded to save some trial and error (which I do all the time and of which I'm tired of).
So let's tabulate your function:
(64000 - aggregated_led_microamp) / 5000
$ for microamp in `seq 5000 5000 40000`; do EXPR="(64000 - $microamp) / 5000 % 8"; echo "$EXPR = `echo $EXPR | bc`"; done (64000 - 5000) / 5000 % 8 = 3 (64000 - 10000) / 5000 % 8 = 2 (64000 - 15000) / 5000 % 8 = 1 (64000 - 20000) / 5000 % 8 = 0 (64000 - 25000) / 5000 % 8 = 7 (64000 - 30000) / 5000 % 8 = 6 (64000 - 35000) / 5000 % 8 = 5 (64000 - 40000) / 5000 % 8 = 4
So feel free to drop the patch which replaces it, and also drop the line added in previous patch "/* TODO FIXME calculation seems wrong */"
Since we don't use it for the Pyra (and maybe never will need it), I don't really care how this functionality will evolve in the future...
Then what's the device it's present in? GTA04?
No, Pyra. But we don't use the microamps settings because the LEDs have series resistors so any (default) value is ok...
Another minor thing is that I think we should also add IS31FL319X_CTRL1 and IS31FL319X_CTRL2 to the reg_default setup.
Fine, I just thought those need to be actually written to device at that stage, so didn't replace these occurances. It felt so from conversation at review.
Yes, they talked about the loop, but these registers are also reset to 0x00.
I will add that myself, squeeze everything into the original patches, add a description of your changes to the patch 0/n message and resubmit.
Fine.
BR, Nikolaus
Hi Jacek,
Am 07.07.2016 um 13:32 schrieb Jacek Anaszewski j.anaszewski@samsung.com:
Hi Nikolaus,
On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
Hi Jacek,
Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski j.anaszewski@samsung.com:
Hi Nikolaus,
Thanks for the updated version. Some comments below.
On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
This is a driver for the Integrated Silicon Solution Inc. LED driver chips series IS31FL319x. They can drive 1, 3, 6 or up to 9 LEDs.
Each LED is individually controllable in brightness (through pwm) in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
The maximum current of the LEDs can be programmed and limited to 5 .. 40mA through a device tree property.
The chip is connected through I2C and can have one of 4 addresses in the range 0x64 .. 0x67 depending on how the AD pin is connected. The address is defined by the reg property as usual.
The chip also has a shutdown input which could be connected to a GPIO, but this driver uses software shutdown if all LEDs are inactivated.
The chip also has breathing and audio features which are not fully supported by this driver.
Tested-on: OMAP5 based Pyra handheld prototype. Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 363 insertions(+) create mode 100644 drivers/leds/leds-is31fl319x.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 5ae2834..65b1045 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -570,6 +570,14 @@ config LEDS_SEAD3 This driver can also be built as a module. If so the module will be called leds-sead3.
+config LEDS_IS31FL319X
- tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
- depends on LEDS_CLASS && I2C && OF
- help
This option enables support for LEDs connected to ISSI IS31FL319xfancy 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 */elsectrl2 |= on << (i-6); /* 6..8 => bit 0..2 */- }
- if (ctrl1 > 0 || ctrl2 > 0) {
dev_dbg(&is31->client->dev, "power up %02x %02x\n",ctrl1, ctrl2);regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);/* update PWMs */regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);/* enable chip from shut down */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);- } else {
dev_dbg(&is31->client->dev, "power down\n");/* shut down (no need to clear CTRL1/2) */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);- }
You need a mutex protection in this function, so as to prevent other processes from jumping in in the middle of setting up a brightness, which requires writing several registers.
Uh? I thought that registering is31fl319x_brightness_set as led_cdev.brightness_set_blocking does the trick that we don't need a private mutex here because it can be called only once anyways. At least that was suggested during v1 review.
Blocking doesn't mean that no one else can call the function in the same time, but to the fact that the caller can be blocked for the time needed for satisfying some condition. e.g. completion of I2C transmission. It needs different treatment in the LED core, as brightness setting can't sleep in general, due to the fact, that it can be called from atomic context. In this case LED core delegates brightness setting to a work queue task and led_set_brightness() returns without blocking the caller.
Ok. That is what I didn't think about: multiple LED triggers running in parallel.
And with the new structure each one is independently calling set_brightness. i2c traffic has its mutex but not the step between writing and reading the PWM values to control chip shut down. The risk is that it is shut down in the wrong moment by a race.
Thanks for spotting this.
Excerpt from linux/leds.h:
=================================================================
/* Set LED brightness level
- Must not sleep. Use brightness_set_blocking for drivers
- that can sleep while setting brightness.
*/ void (*brightness_set)(struct led_classdev *led_cdev, enum led_brightness brightness); /*
- Set LED brightness level immediately - it can block the caller for
- the time required for accessing a LED device register.
*/ int (*brightness_set_blocking)(struct led_classdev *led_cdev, enum led_brightness brightness);
=================================================================
In the [1] I suggested the following :
"You can serialize the operations in brightness_set_blocking with a mutex."
- return ret;
+}
+static struct led_info * +is31fl319x_parse_dt(struct i2c_client *client, int num_leds) +{
- struct device_node *np = client->dev.of_node, *child;
- struct led_info *is31_leds;
- int count;
- if (!np)
return ERR_PTR(-ENODEV);- count = of_get_child_count(np);
- dev_dbg(&client->dev, "child count %d\n", count);
- if (!count || count > NUM_LEDS)
return ERR_PTR(-ENODEV);- is31_leds = devm_kzalloc(&client->dev,
sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);- if (!is31_leds)
return ERR_PTR(-ENOMEM);- for_each_child_of_node(np, child) {
struct led_info led;Using this local variable adds too much noise here. I suggest avoiding the use of struct led_info. It is semantically inconsistent in the context of DT parsing, since you don't have any flags to parse.
u32 reg;int ret;led.name = NULL;ret = of_property_read_string(child, "label", &led.name);if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);You could assign the name directly to related struct led_classdev property. Moreover, if label is not present the child node name should be used for LED class device name. Please refer to the other drivers and common LED bindings.
Ok.
led.default_trigger = NULL;ret = of_property_read_string(child, "linux,default-trigger",&led.default_trigger);Here also please assign the trigger directly to struct led_classdev.
Ok.
if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);led.flags = 0;ret = of_property_read_u32(child, "reg", ®);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, ®map_config);
- if (IS_ERR(is31->regmap)) {
dev_err(&client->dev, "failed to allocate register map\n");return PTR_ERR(is31->regmap);- }
- i2c_set_clientdata(client, is31);
- /* check for write-reply from chip (we can't read any registers) */
- err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
Doesn't powering the device up result in loading registers with default values?
Yes, but not a reboot.
And we have to write *some* register anyways here to be able to detect if the chip is present or does not respond.
So what would writing a different register change?
OK, let's leave that as is.
- if (err < 0) {
dev_err(&client->dev, "no response from chip write: err = %d\n",err);return -EIO; /* does not answer */- }
- /* initialize chip and regmap so that we never try to read from i2c */
You can initialize default register values without writing them, by using regmap's reg_defaults property.
Hm,
- regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
- regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
- for (i = 0; i < NUM_LEDS; i++)
regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);would replace a small loop with a bigger table. But if you prefer this I can change.
Writing registers only to initialize regmap cache looks odd. Moreover it unnecessarily extends driver probing time.
Ok, we will change.
- if (client->dev.of_node) {
u32 val;u8 config2 = 0;if (of_property_read_u32(client->dev.of_node,"led-max-microamp", &val)) {of_property_read_u32 returns 0 on success.
Ok.
led-max-microamp needs to be defined per each sub-LED (child node). The greatest value should be used for setting CS bit field of CTRL2 register, and max_brightness values of the LEDs with lower max microamp value should be limited accordingly.
Hm. The chip does not allow to limit individual LEDs. So we should take the *minimum* of all subnodes.
But the LED core does. That's a max_brightness property is for.
Ok, that seems to be a new feature we have no use case for since all our LEDs have the same current specification. But we will check how it can be done.
Moreover, this is still DT parsing and should find its place in is31fl319x_parse_dt().
if (val > 40000)val = 40000;if (val < 5000)val = 5000;config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */}if (of_property_read_u32(client->dev.of_node, "audio-gain-db",&val)) {Please move this to is31fl319x_parse_dt().
Well, the main reason is that this needs to introduce another temporary struct to pass values from parse_dt to probe and copy values there.
Would it be acceptable to inline all DT parsing?
Don't reinvent the wheel. Please compare how other LED class drivers perform DT parsing.
I have looked into other drivers but have not seen a common pattern. Some have no DT (e.g. 88pm860x), some have some struct led_config_data (e.g. aat1290) and some do it inlined in the probe function (e.g. bcm6328) and some do it in a separate parse_dt (is31fl32).
So which driver would you recommend as best practice?
if (val > 21)val = 21;config2 |= val / 3; /* AGS */}regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);- }
- for (i = 0; i < NUM_LEDS; i++) {
struct is31fl319x_led *l = is31->leds + i;l->chip = is31;if (leds[i].name && !leds[i].flags) {l->led_cdev.name = leds[i].name;l->led_cdev.default_trigger= leds[i].default_trigger;l->led_cdev.brightness_set_blocking= is31fl319x_brightness_set;/* NOTE: is31fl319x_brightness_set will be called* immediately after register() before we return*/Who will call it? I believe that this is true only if default-on trigger is registered.
Yes, as soon as anyone can call it we must be aware.
This is common for all LED class devices. Please drop the comment.
Ok. Might belong somewhere else (e.g. to devm_led_classdev_register documentation).
err = devm_led_classdev_register(&client->dev,&l->led_cdev);if (err < 0)return err;}- }
- dev_dbg(&client->dev, "probed\n");
Please remove this debug log. You can verify it by checking presence of the device in the sysfs.
Ok.
- return 0;
+}
+static struct i2c_driver is31fl319x_driver = {
- .driver = {
.name = "leds-is31fl319x",.of_match_table = of_match_ptr(of_is31fl319x_leds_match),- },
- .probe = is31fl319x_probe,
- .id_table = is31fl319x_id,
+};
+module_i2c_driver(is31fl319x_driver);
+MODULE_AUTHOR("H. Nikolaus Schaller hns@goldelico.com"); +MODULE_DESCRIPTION("IS31FL319X LED driver"); +MODULE_LICENSE("GPL v2");
-- Best regards, Jacek Anaszewski
Will fix asap.
Thanks and BR, Nikolaus Schaller
-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[1] https://lkml.org/lkml/2016/4/20/754
-- Best regards, Jacek Anaszewski
BR and thanks, Nikolaus
On 07/07/2016 03:00 PM, H. Nikolaus Schaller wrote:
Hi Jacek,
Am 07.07.2016 um 13:32 schrieb Jacek Anaszewski j.anaszewski@samsung.com:
Hi Nikolaus,
On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
Hi Jacek,
Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski j.anaszewski@samsung.com:
Hi Nikolaus,
Thanks for the updated version. Some comments below.
On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
This is a driver for the Integrated Silicon Solution Inc. LED driver chips series IS31FL319x. They can drive 1, 3, 6 or up to 9 LEDs.
Each LED is individually controllable in brightness (through pwm) in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
The maximum current of the LEDs can be programmed and limited to 5 .. 40mA through a device tree property.
The chip is connected through I2C and can have one of 4 addresses in the range 0x64 .. 0x67 depending on how the AD pin is connected. The address is defined by the reg property as usual.
The chip also has a shutdown input which could be connected to a GPIO, but this driver uses software shutdown if all LEDs are inactivated.
The chip also has breathing and audio features which are not fully supported by this driver.
Tested-on: OMAP5 based Pyra handheld prototype. Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-is31fl319x.c | 354 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 363 insertions(+) create mode 100644 drivers/leds/leds-is31fl319x.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 5ae2834..65b1045 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -570,6 +570,14 @@ config LEDS_SEAD3 This driver can also be built as a module. If so the module will be called leds-sead3.
+config LEDS_IS31FL319X
- tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
- depends on LEDS_CLASS && I2C && OF
- help
This option enables support for LEDs connected to ISSI IS31FL319xfancy 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 */elsectrl2 |= on << (i-6); /* 6..8 => bit 0..2 */- }
- if (ctrl1 > 0 || ctrl2 > 0) {
dev_dbg(&is31->client->dev, "power up %02x %02x\n",ctrl1, ctrl2);regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);/* update PWMs */regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);/* enable chip from shut down */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);- } else {
dev_dbg(&is31->client->dev, "power down\n");/* shut down (no need to clear CTRL1/2) */ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);- }
You need a mutex protection in this function, so as to prevent other processes from jumping in in the middle of setting up a brightness, which requires writing several registers.
Uh? I thought that registering is31fl319x_brightness_set as led_cdev.brightness_set_blocking does the trick that we don't need a private mutex here because it can be called only once anyways. At least that was suggested during v1 review.
Blocking doesn't mean that no one else can call the function in the same time, but to the fact that the caller can be blocked for the time needed for satisfying some condition. e.g. completion of I2C transmission. It needs different treatment in the LED core, as brightness setting can't sleep in general, due to the fact, that it can be called from atomic context. In this case LED core delegates brightness setting to a work queue task and led_set_brightness() returns without blocking the caller.
Ok. That is what I didn't think about: multiple LED triggers running in parallel.
And with the new structure each one is independently calling set_brightness. i2c traffic has its mutex but not the step between writing and reading the PWM values to control chip shut down. The risk is that it is shut down in the wrong moment by a race.
Exactly.
Thanks for spotting this.
Excerpt from linux/leds.h:
=================================================================
/* Set LED brightness level
- Must not sleep. Use brightness_set_blocking for drivers
- that can sleep while setting brightness.
*/ void (*brightness_set)(struct led_classdev *led_cdev, enum led_brightness brightness); /*
- Set LED brightness level immediately - it can block the caller for
- the time required for accessing a LED device register.
*/ int (*brightness_set_blocking)(struct led_classdev *led_cdev, enum led_brightness brightness);
=================================================================
In the [1] I suggested the following :
"You can serialize the operations in brightness_set_blocking with a mutex."
- return ret;
+}
+static struct led_info * +is31fl319x_parse_dt(struct i2c_client *client, int num_leds) +{
- struct device_node *np = client->dev.of_node, *child;
- struct led_info *is31_leds;
- int count;
- if (!np)
return ERR_PTR(-ENODEV);- count = of_get_child_count(np);
- dev_dbg(&client->dev, "child count %d\n", count);
- if (!count || count > NUM_LEDS)
return ERR_PTR(-ENODEV);- is31_leds = devm_kzalloc(&client->dev,
sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);- if (!is31_leds)
return ERR_PTR(-ENOMEM);- for_each_child_of_node(np, child) {
struct led_info led;Using this local variable adds too much noise here. I suggest avoiding the use of struct led_info. It is semantically inconsistent in the context of DT parsing, since you don't have any flags to parse.
u32 reg;int ret;led.name = NULL;ret = of_property_read_string(child, "label", &led.name);if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);You could assign the name directly to related struct led_classdev property. Moreover, if label is not present the child node name should be used for LED class device name. Please refer to the other drivers and common LED bindings.
Ok.
led.default_trigger = NULL;ret = of_property_read_string(child, "linux,default-trigger",&led.default_trigger);Here also please assign the trigger directly to struct led_classdev.
Ok.
if (ret < 0 && ret != -EINVAL) /* is optional */return ERR_PTR(ret);led.flags = 0;ret = of_property_read_u32(child, "reg", ®);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, ®map_config);
- if (IS_ERR(is31->regmap)) {
dev_err(&client->dev, "failed to allocate register map\n");return PTR_ERR(is31->regmap);- }
- i2c_set_clientdata(client, is31);
- /* check for write-reply from chip (we can't read any registers) */
- err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
Doesn't powering the device up result in loading registers with default values?
Yes, but not a reboot.
And we have to write *some* register anyways here to be able to detect if the chip is present or does not respond.
So what would writing a different register change?
OK, let's leave that as is.
- if (err < 0) {
dev_err(&client->dev, "no response from chip write: err = %d\n",err);return -EIO; /* does not answer */- }
- /* initialize chip and regmap so that we never try to read from i2c */
You can initialize default register values without writing them, by using regmap's reg_defaults property.
Hm,
- regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
- regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
- for (i = 0; i < NUM_LEDS; i++)
regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);would replace a small loop with a bigger table. But if you prefer this I can change.
Writing registers only to initialize regmap cache looks odd. Moreover it unnecessarily extends driver probing time.
Ok, we will change.
- if (client->dev.of_node) {
u32 val;u8 config2 = 0;if (of_property_read_u32(client->dev.of_node,"led-max-microamp", &val)) {of_property_read_u32 returns 0 on success.
Ok.
led-max-microamp needs to be defined per each sub-LED (child node). The greatest value should be used for setting CS bit field of CTRL2 register, and max_brightness values of the LEDs with lower max microamp value should be limited accordingly.
Hm. The chip does not allow to limit individual LEDs. So we should take the *minimum* of all subnodes.
But the LED core does. That's a max_brightness property is for.
Ok, that seems to be a new feature we have no use case for since all our LEDs have the same current specification. But we will check how it can be done.
max_brightness property and corresponding sysfs attribute were added in 2009. See:
1bd465e6b0 ("leds: allow led-drivers to use a variable range of brightness values")
Moreover, this is still DT parsing and should find its place in is31fl319x_parse_dt().
if (val > 40000)val = 40000;if (val < 5000)val = 5000;config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */}if (of_property_read_u32(client->dev.of_node, "audio-gain-db",&val)) {Please move this to is31fl319x_parse_dt().
Well, the main reason is that this needs to introduce another temporary struct to pass values from parse_dt to probe and copy values there.
Would it be acceptable to inline all DT parsing?
Don't reinvent the wheel. Please compare how other LED class drivers perform DT parsing.
I have looked into other drivers but have not seen a common pattern. Some have no DT (e.g. 88pm860x), some have some struct led_config_data (e.g. aat1290) and some do it inlined in the probe function (e.g. bcm6328) and some do it in a separate parse_dt (is31fl32).
So which driver would you recommend as best practice?
E.g. drivers/leds/leds-is31fl32xx.c
if (val > 21)val = 21;config2 |= val / 3; /* AGS */}regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);- }
- for (i = 0; i < NUM_LEDS; i++) {
struct is31fl319x_led *l = is31->leds + i;l->chip = is31;if (leds[i].name && !leds[i].flags) {l->led_cdev.name = leds[i].name;l->led_cdev.default_trigger= leds[i].default_trigger;l->led_cdev.brightness_set_blocking= is31fl319x_brightness_set;/* NOTE: is31fl319x_brightness_set will be called* immediately after register() before we return*/Who will call it? I believe that this is true only if default-on trigger is registered.
Yes, as soon as anyone can call it we must be aware.
This is common for all LED class devices. Please drop the comment.
Ok. Might belong somewhere else (e.g. to devm_led_classdev_register documentation).
err = devm_led_classdev_register(&client->dev,&l->led_cdev);if (err < 0)return err;}- }
- dev_dbg(&client->dev, "probed\n");
Please remove this debug log. You can verify it by checking presence of the device in the sysfs.
Ok.
- return 0;
+}
+static struct i2c_driver is31fl319x_driver = {
- .driver = {
.name = "leds-is31fl319x",.of_match_table = of_match_ptr(of_is31fl319x_leds_match),- },
- .probe = is31fl319x_probe,
- .id_table = is31fl319x_id,
+};
+module_i2c_driver(is31fl319x_driver);
+MODULE_AUTHOR("H. Nikolaus Schaller hns@goldelico.com"); +MODULE_DESCRIPTION("IS31FL319X LED driver"); +MODULE_LICENSE("GPL v2");
-- Best regards, Jacek Anaszewski
Will fix asap.
Thanks and BR, Nikolaus Schaller
-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[1] https://lkml.org/lkml/2016/4/20/754
-- Best regards, Jacek Anaszewski
BR and thanks, Nikolaus
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"; + }; +}; +
-
Andrey Utkin -
H. Nikolaus Schaller -
Jacek Anaszewski -
kbuild test robot