[PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Andrew Lunn]

This patchset is a driver for the TI tlc59116 16 Channel i2c LED
driver and tlc59108 8 Channel i2c LED driver. This driver is used on
the Belkin WRT1900AC access point and the C code is derived from code
Belkin contributed to OpenWRT.  However it has been extensively
re-written, and a device tree binding added to replace platform data.

Cc: Matthew.Fatheree-REUqjI8E1xrQT0dZR+AlfA@public.gmane.org

Since v5:
      Hard code number of LEDs, rather than #define
      Moved common fields from led into priv
      Moved reg_ledout into priv, removed helper functions
      Cache brightness to avoid race conditions
      tlc591xx_led_set -> tlc591xx_set_brightness
      Be paranoid with of_match_device() and client->dev.of_node

Since v4:
      Fix Oops on module unload reported by Vignesh R

Since v3:
      Generalized and added support for tlc59108
      brightness == 0 and brightness == LED_FULL disable PWM and used
        fixed OFF/ON mode

Since v2:
      Remove incorrect /* Mode register ? */ comment
      Parenthesis around the macro arguments
      Converted many signed variables into unsigned
      Saved an initialization


Since v1:
      s/uint8_t/u8/g
      Remove empty line
      Removed #gpio-cells
      Added select REGMAP_I2C
      Sorted #includes into alphabetic order
      Added missing MODULE_DEVICE_TABLE(of, ...)
      Check return value of regmap_write()
      Simplified tlc59116_set_mode()

Andrew Lunn (2):
  leds: tlc59116: Document binding for the TI 16 Channel i2c LED driver
  leds: tlc59116: Driver for the TI 16 Channel i2c LED driver

 .../devicetree/bindings/leds/leds-tlc59116.txt     |  41 ++++
 drivers/leds/Kconfig                               |   7 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-tlc59116.c                       | 253 +++++++++++++++++++++
 4 files changed, 302 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc59116.txt
 create mode 100644 drivers/leds/leds-tlc59116.c

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

[PATCH RESEND v6 1/2] leds: tlc591xx: Document binding for the TI 8/16 Channel i2c LED driver

[Andrew Lunn]

Document the binding for the TLC591xx LED driver.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Imre Kaloz <kaloz@openwrt.org>
Cc: Matthew.Fatheree@belkin.com
---
 .../devicetree/bindings/leds/leds-tlc591xx.txt     | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc591xx.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt b/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
new file mode 100644
index 000000000000..3bbbf7024411
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
@@ -0,0 +1,40 @@
+LEDs connected to tlc59116 or tlc59108
+
+Required properties
+- compatible: should be "ti,tlc59116" or "ti,tlc59108"
+- #address-cells: must be 1
+- #size-cells: must be 0
+- reg: typically 0x68
+
+Each led is represented as a sub-node of the ti,tlc59116.
+See Documentation/devicetree/bindings/leds/common.txt
+
+LED sub-node properties:
+- reg: number of LED line, 0 to 15 or 0 to 7
+- label: (optional) name of LED
+- linux,default-trigger : (optional)
+
+Examples:
+
+tlc59116@68 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "ti,tlc59116";
+	reg = <0x68>;
+
+	wan@0 {
+		label = "wrt1900ac:amber:wan";
+		reg = <0x0>;
+	};
+
+	2g@2 {
+		label = "wrt1900ac:white:2g";
+		reg = <0x2>;
+	};
+
+	alive@9 {
+		label = "wrt1900ac:green:alive";
+		reg = <0x9>;
+		linux,default_trigger = "heartbeat";
+	};
+};
-- 
2.1.4

[PATCH RESEND v6 2/2] leds: tlc591xx: Driver for the TI 8/16 Channel i2c LED driver

[Andrew Lunn]

The TLC59116 is an I2C bus controlled 16-channel LED driver.  The
TLC59108 is an I2C bus controlled 8-channel LED driver, which is very
similar to the TLC59116. Each LED output has its own 8-bit
fixed-frequency PWM controller to control the brightness of the LED.
The LEDs can also be fixed off and on, making them suitable for use as
GPOs.

This is based on a driver from Belkin, but has been extensively
rewritten and extended to support both 08 and 16 versions.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Imre Kaloz <kaloz@openwrt.org>
Cc: Matthew.Fatheree@belkin.com
---
 drivers/leds/Kconfig         |   8 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-tlc591xx.c | 300 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 309 insertions(+)
 create mode 100644 drivers/leds/leds-tlc591xx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 966b9605f5f0..a38b17a10bd2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -467,6 +467,14 @@ config LEDS_TCA6507
 	  LED driver chips accessed via the I2C bus.
 	  Driver support brightness control and hardware-assisted blinking.
 
+config LEDS_TLC591XX
+	tristate "LED driver for TLC59108 and TLC59116 controllers"
+	depends on LEDS_CLASS && I2C
+	select REGMAP_I2C
+	help
+	  This option enables support for Texas Instruments TLC59108
+	  and TLC59116 LED controllers.
+
 config LEDS_MAX8997
 	tristate "LED support for MAX8997 PMIC"
 	depends on LEDS_CLASS && MFD_MAX8997
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index bf4609338e10..749dbe38ab27 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
 obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
 obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
 obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
+obj-$(CONFIG_LEDS_TLC591XX)		+= leds-tlc591xx.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
 obj-$(CONFIG_LEDS_IPAQ_MICRO)		+= leds-ipaq-micro.o
 obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
new file mode 100644
index 000000000000..de16c29d7895
--- /dev/null
+++ b/drivers/leds/leds-tlc591xx.c
@@ -0,0 +1,300 @@
+/*
+ * Copyright 2014 Belkin Inc.
+ * Copyright 2015 Andrew Lunn <andrew@lunn.ch>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#define TLC591XX_MAX_LEDS	16
+
+#define TLC591XX_REG_MODE1	0x00
+#define MODE1_RESPON_ADDR_MASK	0xF0
+#define MODE1_NORMAL_MODE	(0 << 4)
+#define MODE1_SPEED_MODE	(1 << 4)
+
+#define TLC591XX_REG_MODE2	0x01
+#define MODE2_DIM		(0 << 5)
+#define MODE2_BLINK		(1 << 5)
+#define MODE2_OCH_STOP		(0 << 3)
+#define MODE2_OCH_ACK		(1 << 3)
+
+#define TLC591XX_REG_PWM(x)	(0x02 + (x))
+
+#define TLC591XX_REG_GRPPWM	0x12
+#define TLC591XX_REG_GRPFREQ	0x13
+
+/* LED Driver Output State, determine the source that drives LED outputs */
+#define LEDOUT_OFF		0x0	/* Output LOW */
+#define LEDOUT_ON		0x1	/* Output HI-Z */
+#define LEDOUT_DIM		0x2	/* Dimming */
+#define LEDOUT_BLINK		0x3	/* Blinking */
+#define LEDOUT_MASK		0x3
+
+#define ldev_to_led(c)		container_of(c, struct tlc591xx_led, ldev)
+#define work_to_led(work)	container_of(work, struct tlc591xx_led, work)
+
+struct tlc591xx_led {
+	bool active;
+	unsigned int led_no;
+	struct led_classdev ldev;
+	struct work_struct work;
+	struct tlc591xx_priv *priv;
+};
+
+struct tlc591xx_priv {
+	struct tlc591xx_led leds[TLC591XX_MAX_LEDS];
+	struct regmap *regmap;
+	unsigned int reg_ledout_offset;
+};
+
+struct tlc591xx {
+	unsigned int max_leds;
+	unsigned int reg_ledout_offset;
+};
+
+static const struct tlc591xx tlc59116 = {
+	.max_leds = 16,
+	.reg_ledout_offset = 0x14,
+};
+
+static const struct tlc591xx tlc59108 = {
+	.max_leds = 8,
+	.reg_ledout_offset = 0x0c,
+};
+
+static int
+tlc591xx_set_mode(struct regmap *regmap, u8 mode)
+{
+	int err;
+	u8 val;
+
+	err = regmap_write(regmap, TLC591XX_REG_MODE1, MODE1_NORMAL_MODE);
+	if (err)
+		return err;
+
+	val = MODE2_OCH_STOP | mode;
+
+	return regmap_write(regmap, TLC591XX_REG_MODE2, val);
+}
+
+static int
+tlc591xx_set_ledout(struct tlc591xx_priv *priv, struct tlc591xx_led *led,
+		    u8 val)
+{
+	unsigned int i = (led->led_no % 4) * 2;
+	unsigned int mask = LEDOUT_MASK << i;
+	unsigned int addr = priv->reg_ledout_offset + (led->led_no >> 2);
+
+	val = val << i;
+
+	return regmap_update_bits(priv->regmap, addr, mask, val);
+}
+
+static int
+tlc591xx_set_pwm(struct tlc591xx_priv *priv, struct tlc591xx_led *led,
+		 u8 brightness)
+{
+	u8 pwm = TLC591XX_REG_PWM(led->led_no);
+
+	return regmap_write(priv->regmap, pwm, brightness);
+}
+
+static void
+tlc591xx_led_work(struct work_struct *work)
+{
+	struct tlc591xx_led *led = work_to_led(work);
+	struct tlc591xx_priv *priv = led->priv;
+	enum led_brightness brightness = led->ldev.brightness;
+	int err;
+
+	switch (brightness) {
+	case 0:
+		err = tlc591xx_set_ledout(priv, led, LEDOUT_OFF);
+		break;
+	case LED_FULL:
+		err = tlc591xx_set_ledout(priv, led, LEDOUT_ON);
+		break;
+	default:
+		err = tlc591xx_set_ledout(priv, led, LEDOUT_DIM);
+		if (!err)
+			err = tlc591xx_set_pwm(priv, led, brightness);
+	}
+
+	if (err)
+		dev_err(led->ldev.dev, "Failed setting brightness\n");
+}
+
+static void
+tlc591xx_brightness_set(struct led_classdev *led_cdev,
+			enum led_brightness brightness)
+{
+	struct tlc591xx_led *led = ldev_to_led(led_cdev);
+
+	led->ldev.brightness = brightness;
+	schedule_work(&led->work);
+}
+
+static void
+tlc591xx_destroy_devices(struct tlc591xx_priv *priv, unsigned int j)
+{
+	int i = j;
+
+	while (--i >= 0) {
+		if (priv->leds[i].active) {
+			led_classdev_unregister(&priv->leds[i].ldev);
+			cancel_work_sync(&priv->leds[i].work);
+		}
+	}
+}
+
+static int
+tlc591xx_configure(struct device *dev,
+		   struct tlc591xx_priv *priv,
+		   const struct tlc591xx *tlc591xx)
+{
+	unsigned int i;
+	int err = 0;
+
+	tlc591xx_set_mode(priv->regmap, MODE2_DIM);
+	for (i = 0; i < TLC591XX_MAX_LEDS; i++) {
+		struct tlc591xx_led *led = &priv->leds[i];
+
+		if (!led->active)
+			continue;
+
+		led->priv = priv;
+		led->led_no = i;
+		led->ldev.brightness_set = tlc591xx_brightness_set;
+		led->ldev.max_brightness = LED_FULL;
+		INIT_WORK(&led->work, tlc591xx_led_work);
+		err = led_classdev_register(dev, &led->ldev);
+		if (err < 0) {
+			dev_err(dev, "couldn't register LED %s\n",
+				led->ldev.name);
+			goto exit;
+		}
+	}
+
+	return 0;
+
+exit:
+	tlc591xx_destroy_devices(priv, i);
+	return err;
+}
+
+static const struct regmap_config tlc591xx_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x1e,
+};
+
+static const struct of_device_id of_tlc591xx_leds_match[] = {
+	{ .compatible = "ti,tlc59116",
+	  .data = &tlc59116 },
+	{ .compatible = "ti,tlc59108",
+	  .data = &tlc59108 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_tlc591xx_leds_match);
+
+static int
+tlc591xx_probe(struct i2c_client *client,
+	       const struct i2c_device_id *id)
+{
+	struct device_node *np = client->dev.of_node, *child;
+	struct device *dev = &client->dev;
+	const struct of_device_id *match;
+	const struct tlc591xx *tlc591xx;
+	struct tlc591xx_priv *priv;
+	int err, count, reg;
+
+	match = of_match_device(of_tlc591xx_leds_match, dev);
+	if (!match)
+		return -ENODEV;
+
+	tlc591xx = match->data;
+	if (!np)
+		return -ENODEV;
+
+	count = of_get_child_count(np);
+	if (!count || count > tlc591xx->max_leds)
+		return -EINVAL;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA))
+		return -EIO;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = devm_regmap_init_i2c(client, &tlc591xx_regmap);
+	if (IS_ERR(priv->regmap)) {
+		err = PTR_ERR(priv->regmap);
+		dev_err(dev, "Failed to allocate register map: %d\n", err);
+		return err;
+	}
+	priv->reg_ledout_offset = tlc591xx->reg_ledout_offset;
+
+	i2c_set_clientdata(client, priv);
+
+	for_each_child_of_node(np, child) {
+		err = of_property_read_u32(child, "reg", &reg);
+		if (err)
+			return err;
+		if (reg < 0 || reg >= tlc591xx->max_leds)
+			return -EINVAL;
+		if (priv->leds[reg].active)
+			return -EINVAL;
+		priv->leds[reg].active = true;
+		priv->leds[reg].ldev.name =
+			of_get_property(child, "label", NULL) ? : child->name;
+		priv->leds[reg].ldev.default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);
+	}
+	return tlc591xx_configure(dev, priv, tlc591xx);
+}
+
+static int
+tlc591xx_remove(struct i2c_client *client)
+{
+	struct tlc591xx_priv *priv = i2c_get_clientdata(client);
+
+	tlc591xx_destroy_devices(priv, TLC591XX_MAX_LEDS);
+
+	return 0;
+}
+
+static const struct i2c_device_id tlc591xx_id[] = {
+	{ "tlc59116" },
+	{ "tlc59108" },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, tlc591xx_id);
+
+static struct i2c_driver tlc591xx_driver = {
+	.driver = {
+		.name = "tlc591xx",
+		.of_match_table = of_match_ptr(of_tlc591xx_leds_match),
+	},
+	.probe = tlc591xx_probe,
+	.remove = tlc591xx_remove,
+	.id_table = tlc591xx_id,
+};
+
+module_i2c_driver(tlc591xx_driver);
+
+MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("TLC591XX LED driver");
-- 
2.1.4

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Andrew Lunn]

On Tue, Mar 17, 2015 at 11:08:25PM +0100, Andrew Lunn wrote:
> This patchset is a driver for the TI tlc59116 16 Channel i2c LED
> driver and tlc59108 8 Channel i2c LED driver. This driver is used on
> the Belkin WRT1900AC access point and the C code is derived from code
> Belkin contributed to OpenWRT.  However it has been extensively
> re-written, and a device tree binding added to replace platform data.

Hi Bryan

Do you have any comments on this driver? Anything you would like me to
do to help get it accepted?

Thanks
	Andrew


> 
> Cc: Matthew.Fatheree@belkin.com
> 
> Since v5:
>       Hard code number of LEDs, rather than #define
>       Moved common fields from led into priv
>       Moved reg_ledout into priv, removed helper functions
>       Cache brightness to avoid race conditions
>       tlc591xx_led_set -> tlc591xx_set_brightness
>       Be paranoid with of_match_device() and client->dev.of_node
> 
> Since v4:
>       Fix Oops on module unload reported by Vignesh R
> 
> Since v3:
>       Generalized and added support for tlc59108
>       brightness == 0 and brightness == LED_FULL disable PWM and used
>         fixed OFF/ON mode
> 
> Since v2:
>       Remove incorrect /* Mode register ? */ comment
>       Parenthesis around the macro arguments
>       Converted many signed variables into unsigned
>       Saved an initialization
> 
> 
> Since v1:
>       s/uint8_t/u8/g
>       Remove empty line
>       Removed #gpio-cells
>       Added select REGMAP_I2C
>       Sorted #includes into alphabetic order
>       Added missing MODULE_DEVICE_TABLE(of, ...)
>       Check return value of regmap_write()
>       Simplified tlc59116_set_mode()
> 
> Andrew Lunn (2):
>   leds: tlc59116: Document binding for the TI 16 Channel i2c LED driver
>   leds: tlc59116: Driver for the TI 16 Channel i2c LED driver
> 
>  .../devicetree/bindings/leds/leds-tlc59116.txt     |  41 ++++
>  drivers/leds/Kconfig                               |   7 +
>  drivers/leds/Makefile                              |   1 +
>  drivers/leds/leds-tlc59116.c                       | 253 +++++++++++++++++++++
>  4 files changed, 302 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc59116.txt
>  create mode 100644 drivers/leds/leds-tlc59116.c
> 
> -- 
> 2.1.3

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Bryan Wu]

On Sun, Mar 29, 2015 at 12:28 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Mar 17, 2015 at 11:08:25PM +0100, Andrew Lunn wrote:
>> This patchset is a driver for the TI tlc59116 16 Channel i2c LED
>> driver and tlc59108 8 Channel i2c LED driver. This driver is used on
>> the Belkin WRT1900AC access point and the C code is derived from code
>> Belkin contributed to OpenWRT.  However it has been extensively
>> re-written, and a device tree binding added to replace platform data.
>
> Hi Bryan
>
> Do you have any comments on this driver? Anything you would like me to
> do to help get it accepted?
>

Sorry for the delay. I'm reviewing several other patches as well as
working on my internal work.
I will review this soon.

Thanks,
-Bryan

>
>>
>> Cc: Matthew.Fatheree@belkin.com
>>
>> Since v5:
>>       Hard code number of LEDs, rather than #define
>>       Moved common fields from led into priv
>>       Moved reg_ledout into priv, removed helper functions
>>       Cache brightness to avoid race conditions
>>       tlc591xx_led_set -> tlc591xx_set_brightness
>>       Be paranoid with of_match_device() and client->dev.of_node
>>
>> Since v4:
>>       Fix Oops on module unload reported by Vignesh R
>>
>> Since v3:
>>       Generalized and added support for tlc59108
>>       brightness == 0 and brightness == LED_FULL disable PWM and used
>>         fixed OFF/ON mode
>>
>> Since v2:
>>       Remove incorrect /* Mode register ? */ comment
>>       Parenthesis around the macro arguments
>>       Converted many signed variables into unsigned
>>       Saved an initialization
>>
>>
>> Since v1:
>>       s/uint8_t/u8/g
>>       Remove empty line
>>       Removed #gpio-cells
>>       Added select REGMAP_I2C
>>       Sorted #includes into alphabetic order
>>       Added missing MODULE_DEVICE_TABLE(of, ...)
>>       Check return value of regmap_write()
>>       Simplified tlc59116_set_mode()
>>
>> Andrew Lunn (2):
>>   leds: tlc59116: Document binding for the TI 16 Channel i2c LED driver
>>   leds: tlc59116: Driver for the TI 16 Channel i2c LED driver
>>
>>  .../devicetree/bindings/leds/leds-tlc59116.txt     |  41 ++++
>>  drivers/leds/Kconfig                               |   7 +
>>  drivers/leds/Makefile                              |   1 +
>>  drivers/leds/leds-tlc59116.c                       | 253 +++++++++++++++++++++
>>  4 files changed, 302 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc59116.txt
>>  create mode 100644 drivers/leds/leds-tlc59116.c
>>
>> --
>> 2.1.3

Re: [PATCH RESEND v6 2/2] leds: tlc591xx: Driver for the TI 8/16 Channel i2c LED driver

[Jacek Anaszewski]

Hi Andrew,

Very nice driver. I have one question below.

On 03/17/2015 11:08 PM, Andrew Lunn wrote:
> The TLC59116 is an I2C bus controlled 16-channel LED driver.  The
> TLC59108 is an I2C bus controlled 8-channel LED driver, which is very
> similar to the TLC59116. Each LED output has its own 8-bit
> fixed-frequency PWM controller to control the brightness of the LED.
> The LEDs can also be fixed off and on, making them suitable for use as
> GPOs.
>
> This is based on a driver from Belkin, but has been extensively
> rewritten and extended to support both 08 and 16 versions.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Imre Kaloz <kaloz@openwrt.org>
> Cc: Matthew.Fatheree@belkin.comG
> ---
>   drivers/leds/Kconfig         |   8 ++
>   drivers/leds/Makefile        |   1 +
>   drivers/leds/leds-tlc591xx.c | 300 +++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 309 insertions(+)
>   create mode 100644 drivers/leds/leds-tlc591xx.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 966b9605f5f0..a38b17a10bd2 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -467,6 +467,14 @@ config LEDS_TCA6507
>   	  LED driver chips accessed via the I2C bus.
>   	  Driver support brightness control and hardware-assisted blinking.
>
> +config LEDS_TLC591XX
> +	tristate "LED driver for TLC59108 and TLC59116 controllers"
> +	depends on LEDS_CLASS && I2C
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for Texas Instruments TLC59108
> +	  and TLC59116 LED controllers.
> +
>   config LEDS_MAX8997
>   	tristate "LED support for MAX8997 PMIC"
>   	depends on LEDS_CLASS && MFD_MAX8997
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index bf4609338e10..749dbe38ab27 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
>   obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
>   obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
>   obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
> +obj-$(CONFIG_LEDS_TLC591XX)		+= leds-tlc591xx.o
>   obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
>   obj-$(CONFIG_LEDS_IPAQ_MICRO)		+= leds-ipaq-micro.o
>   obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
> diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
> new file mode 100644
> index 000000000000..de16c29d7895
> --- /dev/null
> +++ b/drivers/leds/leds-tlc591xx.c
> @@ -0,0 +1,300 @@
> +/*
> + * Copyright 2014 Belkin Inc.
> + * Copyright 2015 Andrew Lunn <andrew@lunn.ch>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +#define TLC591XX_MAX_LEDS	16
> +
> +#define TLC591XX_REG_MODE1	0x00
> +#define MODE1_RESPON_ADDR_MASK	0xF0
> +#define MODE1_NORMAL_MODE	(0 << 4)
> +#define MODE1_SPEED_MODE	(1 << 4)
> +
> +#define TLC591XX_REG_MODE2	0x01
> +#define MODE2_DIM		(0 << 5)
> +#define MODE2_BLINK		(1 << 5)
> +#define MODE2_OCH_STOP		(0 << 3)
> +#define MODE2_OCH_ACK		(1 << 3)
> +
> +#define TLC591XX_REG_PWM(x)	(0x02 + (x))
> +
> +#define TLC591XX_REG_GRPPWM	0x12
> +#define TLC591XX_REG_GRPFREQ	0x13
> +
> +/* LED Driver Output State, determine the source that drives LED outputs */
> +#define LEDOUT_OFF		0x0	/* Output LOW */
> +#define LEDOUT_ON		0x1	/* Output HI-Z */
> +#define LEDOUT_DIM		0x2	/* Dimming */
> +#define LEDOUT_BLINK		0x3	/* Blinking */
> +#define LEDOUT_MASK		0x3
> +
> +#define ldev_to_led(c)		container_of(c, struct tlc591xx_led, ldev)
> +#define work_to_led(work)	container_of(work, struct tlc591xx_led, work)
> +
> +struct tlc591xx_led {
> +	bool active;
> +	unsigned int led_no;
> +	struct led_classdev ldev;
> +	struct work_struct work;
> +	struct tlc591xx_priv *priv;
> +};
> +
> +struct tlc591xx_priv {
> +	struct tlc591xx_led leds[TLC591XX_MAX_LEDS];
> +	struct regmap *regmap;
> +	unsigned int reg_ledout_offset;
> +};
> +
> +struct tlc591xx {
> +	unsigned int max_leds;
> +	unsigned int reg_ledout_offset;
> +};
> +
> +static const struct tlc591xx tlc59116 = {
> +	.max_leds = 16,
> +	.reg_ledout_offset = 0x14,
> +};
> +
> +static const struct tlc591xx tlc59108 = {
> +	.max_leds = 8,
> +	.reg_ledout_offset = 0x0c,
> +};
> +
> +static int
> +tlc591xx_set_mode(struct regmap *regmap, u8 mode)
> +{
> +	int err;
> +	u8 val;
> +
> +	err = regmap_write(regmap, TLC591XX_REG_MODE1, MODE1_NORMAL_MODE);
> +	if (err)
> +		return err;
> +
> +	val = MODE2_OCH_STOP | mode;
> +
> +	return regmap_write(regmap, TLC591XX_REG_MODE2, val);
> +}
> +
> +static int
> +tlc591xx_set_ledout(struct tlc591xx_priv *priv, struct tlc591xx_led *led,
> +		    u8 val)
> +{
> +	unsigned int i = (led->led_no % 4) * 2;
> +	unsigned int mask = LEDOUT_MASK << i;
> +	unsigned int addr = priv->reg_ledout_offset + (led->led_no >> 2);
> +
> +	val = val << i;
> +
> +	return regmap_update_bits(priv->regmap, addr, mask, val);
> +}
> +
> +static int
> +tlc591xx_set_pwm(struct tlc591xx_priv *priv, struct tlc591xx_led *led,
> +		 u8 brightness)
> +{
> +	u8 pwm = TLC591XX_REG_PWM(led->led_no);
> +
> +	return regmap_write(priv->regmap, pwm, brightness);
> +}
> +
> +static void
> +tlc591xx_led_work(struct work_struct *work)
> +{
> +	struct tlc591xx_led *led = work_to_led(work);
> +	struct tlc591xx_priv *priv = led->priv;
> +	enum led_brightness brightness = led->ldev.brightness;
> +	int err;
> +
> +	switch (brightness) {
> +	case 0:
> +		err = tlc591xx_set_ledout(priv, led, LEDOUT_OFF);
> +		break;
> +	case LED_FULL:
> +		err = tlc591xx_set_ledout(priv, led, LEDOUT_ON);
> +		break;
> +	default:
> +		err = tlc591xx_set_ledout(priv, led, LEDOUT_DIM);
> +		if (!err)
> +			err = tlc591xx_set_pwm(priv, led, brightness);
> +	}
> +
> +	if (err)
> +		dev_err(led->ldev.dev, "Failed setting brightness\n");
> +}
> +
> +static void
> +tlc591xx_brightness_set(struct led_classdev *led_cdev,
> +			enum led_brightness brightness)
> +{
> +	struct tlc591xx_led *led = ldev_to_led(led_cdev);
> +
> +	led->ldev.brightness = brightness;
> +	schedule_work(&led->work);
> +}
> +
> +static void
> +tlc591xx_destroy_devices(struct tlc591xx_priv *priv, unsigned int j)
> +{
> +	int i = j;
> +
> +	while (--i >= 0) {
> +		if (priv->leds[i].active) {
> +			led_classdev_unregister(&priv->leds[i].ldev);
> +			cancel_work_sync(&priv->leds[i].work);
> +		}
> +	}
> +}
> +
> +static int
> +tlc591xx_configure(struct device *dev,
> +		   struct tlc591xx_priv *priv,
> +		   const struct tlc591xx *tlc591xx)
> +{
> +	unsigned int i;
> +	int err = 0;
> +
> +	tlc591xx_set_mode(priv->regmap, MODE2_DIM);

It seems that all leds will be initially turned on, in dim mode.
This shouldn't be fixed and probably an optional 'led-mode' DT node
property should be provided for defining the initial state. It would
default to OFF if not present.

> +	for (i = 0; i < TLC591XX_MAX_LEDS; i++) {
> +		struct tlc591xx_led *led = &priv->leds[i];
> +
> +		if (!led->active)
> +			continue;
> +
> +		led->priv = priv;
> +		led->led_no = i;
> +		led->ldev.brightness_set = tlc591xx_brightness_set;
> +		led->ldev.max_brightness = LED_FULL;
> +		INIT_WORK(&led->work, tlc591xx_led_work);
> +		err = led_classdev_register(dev, &led->ldev);
> +		if (err < 0) {
> +			dev_err(dev, "couldn't register LED %s\n",
> +				led->ldev.name);
> +			goto exit;
> +		}
> +	}
> +
> +	return 0;
> +
> +exit:
> +	tlc591xx_destroy_devices(priv, i);
> +	return err;
> +}
> +
> +static const struct regmap_config tlc591xx_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x1e,
> +};
> +
> +static const struct of_device_id of_tlc591xx_leds_match[] = {
> +	{ .compatible = "ti,tlc59116",
> +	  .data = &tlc59116 },
> +	{ .compatible = "ti,tlc59108",
> +	  .data = &tlc59108 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_tlc591xx_leds_match);
> +
> +static int
> +tlc591xx_probe(struct i2c_client *client,
> +	       const struct i2c_device_id *id)
> +{
> +	struct device_node *np = client->dev.of_node, *child;
> +	struct device *dev = &client->dev;
> +	const struct of_device_id *match;
> +	const struct tlc591xx *tlc591xx;
> +	struct tlc591xx_priv *priv;
> +	int err, count, reg;
> +
> +	match = of_match_device(of_tlc591xx_leds_match, dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	tlc591xx = match->data;
> +	if (!np)
> +		return -ENODEV;
> +
> +	count = of_get_child_count(np);
> +	if (!count || count > tlc591xx->max_leds)
> +		return -EINVAL;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -EIO;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = devm_regmap_init_i2c(client, &tlc591xx_regmap);
> +	if (IS_ERR(priv->regmap)) {
> +		err = PTR_ERR(priv->regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", err);
> +		return err;
> +	}
> +	priv->reg_ledout_offset = tlc591xx->reg_ledout_offset;
> +
> +	i2c_set_clientdata(client, priv);
> +
> +	for_each_child_of_node(np, child) {
> +		err = of_property_read_u32(child, "reg", &reg);
> +		if (err)
> +			return err;
> +		if (reg < 0 || reg >= tlc591xx->max_leds)
> +			return -EINVAL;
> +		if (priv->leds[reg].active)
> +			return -EINVAL;
> +		priv->leds[reg].active = true;
> +		priv->leds[reg].ldev.name =
> +			of_get_property(child, "label", NULL) ? : child->name;
> +		priv->leds[reg].ldev.default_trigger =
> +			of_get_property(child, "linux,default-trigger", NULL);
> +	}
> +	return tlc591xx_configure(dev, priv, tlc591xx);
> +}
> +
> +static int
> +tlc591xx_remove(struct i2c_client *client)
> +{
> +	struct tlc591xx_priv *priv = i2c_get_clientdata(client);
> +
> +	tlc591xx_destroy_devices(priv, TLC591XX_MAX_LEDS);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id tlc591xx_id[] = {
> +	{ "tlc59116" },
> +	{ "tlc59108" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, tlc591xx_id);
> +
> +static struct i2c_driver tlc591xx_driver = {
> +	.driver = {
> +		.name = "tlc591xx",
> +		.of_match_table = of_match_ptr(of_tlc591xx_leds_match),
> +	},
> +	.probe = tlc591xx_probe,
> +	.remove = tlc591xx_remove,
> +	.id_table = tlc591xx_id,
> +};
> +
> +module_i2c_driver(tlc591xx_driver);
> +
> +MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("TLC591XX LED driver");
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH RESEND v6 2/2] leds: tlc591xx: Driver for the TI 8/16 Channel i2c LED driver

[Andrew Lunn]

On Mon, Apr 20, 2015 at 11:06:09AM +0200, Jacek Anaszewski wrote:
> Hi Andrew,
> 
> Very nice driver.

Thanks. I just hope it gets accepted into this merge window. 

> I have one question below.



> 
> On 03/17/2015 11:08 PM, Andrew Lunn wrote:
> >The TLC59116 is an I2C bus controlled 16-channel LED driver.  The
> >TLC59108 is an I2C bus controlled 8-channel LED driver, which is very
> >similar to the TLC59116. Each LED output has its own 8-bit
> >fixed-frequency PWM controller to control the brightness of the LED.
> >The LEDs can also be fixed off and on, making them suitable for use as
> >GPOs.
> >
> >This is based on a driver from Belkin, but has been extensively
> >rewritten and extended to support both 08 and 16 versions.
> >
> >Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> >Tested-by: Imre Kaloz <kaloz@openwrt.org>
> >Cc: Matthew.Fatheree@belkin.comG
> >---
> >  drivers/leds/Kconfig         |   8 ++
> >  drivers/leds/Makefile        |   1 +
> >  drivers/leds/leds-tlc591xx.c | 300 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 309 insertions(+)
> >  create mode 100644 drivers/leds/leds-tlc591xx.c
> >
> >diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> >index 966b9605f5f0..a38b17a10bd2 100644
> >--- a/drivers/leds/Kconfig
> >+++ b/drivers/leds/Kconfig
> >@@ -467,6 +467,14 @@ config LEDS_TCA6507
> >  	  LED driver chips accessed via the I2C bus.
> >  	  Driver support brightness control and hardware-assisted blinking.
> >
> >+config LEDS_TLC591XX
> >+	tristate "LED driver for TLC59108 and TLC59116 controllers"
> >+	depends on LEDS_CLASS && I2C
> >+	select REGMAP_I2C
> >+	help
> >+	  This option enables support for Texas Instruments TLC59108
> >+	  and TLC59116 LED controllers.
> >+
> >  config LEDS_MAX8997
> >  	tristate "LED support for MAX8997 PMIC"
> >  	depends on LEDS_CLASS && MFD_MAX8997
> >diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> >index bf4609338e10..749dbe38ab27 100644
> >--- a/drivers/leds/Makefile
> >+++ b/drivers/leds/Makefile
> >@@ -31,6 +31,7 @@ obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
> >  obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
> >  obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
> >  obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
> >+obj-$(CONFIG_LEDS_TLC591XX)		+= leds-tlc591xx.o
> >  obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
> >  obj-$(CONFIG_LEDS_IPAQ_MICRO)		+= leds-ipaq-micro.o
> >  obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
> >diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
> >new file mode 100644
> >index 000000000000..de16c29d7895
> >--- /dev/null
> >+++ b/drivers/leds/leds-tlc591xx.c
> >@@ -0,0 +1,300 @@
> >+/*
> >+ * Copyright 2014 Belkin Inc.
> >+ * Copyright 2015 Andrew Lunn <andrew@lunn.ch>
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify
> >+ * it under the terms of the GNU General Public License as published by
> >+ * the Free Software Foundation; version 2 of the License.
> >+ */
> >+
> >+#include <linux/i2c.h>
> >+#include <linux/leds.h>
> >+#include <linux/module.h>
> >+#include <linux/of.h>
> >+#include <linux/of_device.h>
> >+#include <linux/regmap.h>
> >+#include <linux/slab.h>
> >+#include <linux/workqueue.h>
> >+
> >+#define TLC591XX_MAX_LEDS	16
> >+
> >+#define TLC591XX_REG_MODE1	0x00
> >+#define MODE1_RESPON_ADDR_MASK	0xF0
> >+#define MODE1_NORMAL_MODE	(0 << 4)
> >+#define MODE1_SPEED_MODE	(1 << 4)
> >+
> >+#define TLC591XX_REG_MODE2	0x01
> >+#define MODE2_DIM		(0 << 5)
> >+#define MODE2_BLINK		(1 << 5)
> >+#define MODE2_OCH_STOP		(0 << 3)
> >+#define MODE2_OCH_ACK		(1 << 3)
> >+
> >+#define TLC591XX_REG_PWM(x)	(0x02 + (x))
> >+
> >+#define TLC591XX_REG_GRPPWM	0x12
> >+#define TLC591XX_REG_GRPFREQ	0x13
> >+
> >+/* LED Driver Output State, determine the source that drives LED outputs */
> >+#define LEDOUT_OFF		0x0	/* Output LOW */
> >+#define LEDOUT_ON		0x1	/* Output HI-Z */
> >+#define LEDOUT_DIM		0x2	/* Dimming */
> >+#define LEDOUT_BLINK		0x3	/* Blinking */
> >+#define LEDOUT_MASK		0x3
> >+
> >+#define ldev_to_led(c)		container_of(c, struct tlc591xx_led, ldev)
> >+#define work_to_led(work)	container_of(work, struct tlc591xx_led, work)
> >+
> >+struct tlc591xx_led {
> >+	bool active;
> >+	unsigned int led_no;
> >+	struct led_classdev ldev;
> >+	struct work_struct work;
> >+	struct tlc591xx_priv *priv;
> >+};
> >+
> >+struct tlc591xx_priv {
> >+	struct tlc591xx_led leds[TLC591XX_MAX_LEDS];
> >+	struct regmap *regmap;
> >+	unsigned int reg_ledout_offset;
> >+};
> >+
> >+struct tlc591xx {
> >+	unsigned int max_leds;
> >+	unsigned int reg_ledout_offset;
> >+};
> >+
> >+static const struct tlc591xx tlc59116 = {
> >+	.max_leds = 16,
> >+	.reg_ledout_offset = 0x14,
> >+};
> >+
> >+static const struct tlc591xx tlc59108 = {
> >+	.max_leds = 8,
> >+	.reg_ledout_offset = 0x0c,
> >+};
> >+
> >+static int
> >+tlc591xx_set_mode(struct regmap *regmap, u8 mode)
> >+{
> >+	int err;
> >+	u8 val;
> >+
> >+	err = regmap_write(regmap, TLC591XX_REG_MODE1, MODE1_NORMAL_MODE);
> >+	if (err)
> >+		return err;
> >+
> >+	val = MODE2_OCH_STOP | mode;
> >+
> >+	return regmap_write(regmap, TLC591XX_REG_MODE2, val);
> >+}
> >+
> >+static int
> >+tlc591xx_set_ledout(struct tlc591xx_priv *priv, struct tlc591xx_led *led,
> >+		    u8 val)
> >+{
> >+	unsigned int i = (led->led_no % 4) * 2;
> >+	unsigned int mask = LEDOUT_MASK << i;
> >+	unsigned int addr = priv->reg_ledout_offset + (led->led_no >> 2);
> >+
> >+	val = val << i;
> >+
> >+	return regmap_update_bits(priv->regmap, addr, mask, val);
> >+}
> >+
> >+static int
> >+tlc591xx_set_pwm(struct tlc591xx_priv *priv, struct tlc591xx_led *led,
> >+		 u8 brightness)
> >+{
> >+	u8 pwm = TLC591XX_REG_PWM(led->led_no);
> >+
> >+	return regmap_write(priv->regmap, pwm, brightness);
> >+}
> >+
> >+static void
> >+tlc591xx_led_work(struct work_struct *work)
> >+{
> >+	struct tlc591xx_led *led = work_to_led(work);
> >+	struct tlc591xx_priv *priv = led->priv;
> >+	enum led_brightness brightness = led->ldev.brightness;
> >+	int err;
> >+
> >+	switch (brightness) {
> >+	case 0:
> >+		err = tlc591xx_set_ledout(priv, led, LEDOUT_OFF);
> >+		break;
> >+	case LED_FULL:
> >+		err = tlc591xx_set_ledout(priv, led, LEDOUT_ON);
> >+		break;
> >+	default:
> >+		err = tlc591xx_set_ledout(priv, led, LEDOUT_DIM);
> >+		if (!err)
> >+			err = tlc591xx_set_pwm(priv, led, brightness);
> >+	}
> >+
> >+	if (err)
> >+		dev_err(led->ldev.dev, "Failed setting brightness\n");
> >+}
> >+
> >+static void
> >+tlc591xx_brightness_set(struct led_classdev *led_cdev,
> >+			enum led_brightness brightness)
> >+{
> >+	struct tlc591xx_led *led = ldev_to_led(led_cdev);
> >+
> >+	led->ldev.brightness = brightness;
> >+	schedule_work(&led->work);
> >+}
> >+
> >+static void
> >+tlc591xx_destroy_devices(struct tlc591xx_priv *priv, unsigned int j)
> >+{
> >+	int i = j;
> >+
> >+	while (--i >= 0) {
> >+		if (priv->leds[i].active) {
> >+			led_classdev_unregister(&priv->leds[i].ldev);
> >+			cancel_work_sync(&priv->leds[i].work);
> >+		}
> >+	}
> >+}
> >+
> >+static int
> >+tlc591xx_configure(struct device *dev,
> >+		   struct tlc591xx_priv *priv,
> >+		   const struct tlc591xx *tlc591xx)
> >+{
> >+	unsigned int i;
> >+	int err = 0;
> >+
> >+	tlc591xx_set_mode(priv->regmap, MODE2_DIM);
> 
> It seems that all leds will be initially turned on, in dim mode.
> This shouldn't be fixed and probably an optional 'led-mode' DT node
> property should be provided for defining the initial state. It would
> default to OFF if not present.

If you look further down, you will find 

> >+		priv->leds[reg].ldev.default_trigger =
> >+			of_get_property(child, "linux,default-trigger", NULL);

This is the normal way in DT to specify the default on/off/keep
current value/heartbeat etc.

	Andrew

> 
> >+	for (i = 0; i < TLC591XX_MAX_LEDS; i++) {
> >+		struct tlc591xx_led *led = &priv->leds[i];
> >+
> >+		if (!led->active)
> >+			continue;
> >+
> >+		led->priv = priv;
> >+		led->led_no = i;
> >+		led->ldev.brightness_set = tlc591xx_brightness_set;
> >+		led->ldev.max_brightness = LED_FULL;
> >+		INIT_WORK(&led->work, tlc591xx_led_work);
> >+		err = led_classdev_register(dev, &led->ldev);
> >+		if (err < 0) {
> >+			dev_err(dev, "couldn't register LED %s\n",
> >+				led->ldev.name);
> >+			goto exit;
> >+		}
> >+	}
> >+
> >+	return 0;
> >+
> >+exit:
> >+	tlc591xx_destroy_devices(priv, i);
> >+	return err;
> >+}
> >+
> >+static const struct regmap_config tlc591xx_regmap = {
> >+	.reg_bits = 8,
> >+	.val_bits = 8,
> >+	.max_register = 0x1e,
> >+};
> >+
> >+static const struct of_device_id of_tlc591xx_leds_match[] = {
> >+	{ .compatible = "ti,tlc59116",
> >+	  .data = &tlc59116 },
> >+	{ .compatible = "ti,tlc59108",
> >+	  .data = &tlc59108 },
> >+	{},
> >+};
> >+MODULE_DEVICE_TABLE(of, of_tlc591xx_leds_match);
> >+
> >+static int
> >+tlc591xx_probe(struct i2c_client *client,
> >+	       const struct i2c_device_id *id)
> >+{
> >+	struct device_node *np = client->dev.of_node, *child;
> >+	struct device *dev = &client->dev;
> >+	const struct of_device_id *match;
> >+	const struct tlc591xx *tlc591xx;
> >+	struct tlc591xx_priv *priv;
> >+	int err, count, reg;
> >+
> >+	match = of_match_device(of_tlc591xx_leds_match, dev);
> >+	if (!match)
> >+		return -ENODEV;
> >+
> >+	tlc591xx = match->data;
> >+	if (!np)
> >+		return -ENODEV;
> >+
> >+	count = of_get_child_count(np);
> >+	if (!count || count > tlc591xx->max_leds)
> >+		return -EINVAL;
> >+
> >+	if (!i2c_check_functionality(client->adapter,
> >+				     I2C_FUNC_SMBUS_BYTE_DATA))
> >+		return -EIO;
> >+
> >+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >+	if (!priv)
> >+		return -ENOMEM;
> >+
> >+	priv->regmap = devm_regmap_init_i2c(client, &tlc591xx_regmap);
> >+	if (IS_ERR(priv->regmap)) {
> >+		err = PTR_ERR(priv->regmap);
> >+		dev_err(dev, "Failed to allocate register map: %d\n", err);
> >+		return err;
> >+	}
> >+	priv->reg_ledout_offset = tlc591xx->reg_ledout_offset;
> >+
> >+	i2c_set_clientdata(client, priv);
> >+
> >+	for_each_child_of_node(np, child) {
> >+		err = of_property_read_u32(child, "reg", &reg);
> >+		if (err)
> >+			return err;
> >+		if (reg < 0 || reg >= tlc591xx->max_leds)
> >+			return -EINVAL;
> >+		if (priv->leds[reg].active)
> >+			return -EINVAL;
> >+		priv->leds[reg].active = true;
> >+		priv->leds[reg].ldev.name =
> >+			of_get_property(child, "label", NULL) ? : child->name;
> >+		priv->leds[reg].ldev.default_trigger =
> >+			of_get_property(child, "linux,default-trigger", NULL);
> >+	}
> >+	return tlc591xx_configure(dev, priv, tlc591xx);
> >+}
> >+
> >+static int
> >+tlc591xx_remove(struct i2c_client *client)
> >+{
> >+	struct tlc591xx_priv *priv = i2c_get_clientdata(client);
> >+
> >+	tlc591xx_destroy_devices(priv, TLC591XX_MAX_LEDS);
> >+
> >+	return 0;
> >+}
> >+
> >+static const struct i2c_device_id tlc591xx_id[] = {
> >+	{ "tlc59116" },
> >+	{ "tlc59108" },
> >+	{},
> >+};
> >+MODULE_DEVICE_TABLE(i2c, tlc591xx_id);
> >+
> >+static struct i2c_driver tlc591xx_driver = {
> >+	.driver = {
> >+		.name = "tlc591xx",
> >+		.of_match_table = of_match_ptr(of_tlc591xx_leds_match),
> >+	},
> >+	.probe = tlc591xx_probe,
> >+	.remove = tlc591xx_remove,
> >+	.id_table = tlc591xx_id,
> >+};
> >+
> >+module_i2c_driver(tlc591xx_driver);
> >+
> >+MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
> >+MODULE_LICENSE("GPL");
> >+MODULE_DESCRIPTION("TLC591XX LED driver");
> >
> 
> 
> -- 
> Best Regards,
> Jacek Anaszewski

Re: [PATCH RESEND v6 2/2] leds: tlc591xx: Driver for the TI 8/16 Channel i2c LED driver

[Jacek Anaszewski]

On 04/20/2015 01:59 PM, Andrew Lunn wrote:
> On Mon, Apr 20, 2015 at 11:06:09AM +0200, Jacek Anaszewski wrote:
>> Hi Andrew,
>>
>> Very nice driver.
>
> Thanks. I just hope it gets accepted into this merge window.
>
>> I have one question below.
>
[...]
>>> +static int
>>> +tlc591xx_configure(struct device *dev,
>>> +		   struct tlc591xx_priv *priv,
>>> +		   const struct tlc591xx *tlc591xx)
>>> +{
>>> +	unsigned int i;
>>> +	int err = 0;
>>> +
>>> +	tlc591xx_set_mode(priv->regmap, MODE2_DIM);
>>
>> It seems that all leds will be initially turned on, in dim mode.
>> This shouldn't be fixed and probably an optional 'led-mode' DT node
>> property should be provided for defining the initial state. It would
>> default to OFF if not present.
>
> If you look further down, you will find
>
>>> +		priv->leds[reg].ldev.default_trigger =
>>> +			of_get_property(child, "linux,default-trigger", NULL);
>
> This is the normal way in DT to specify the default on/off/keep
> current value/heartbeat etc.

OK, I was initially thinking that initializing LED to MODE2_DIM
turns the LED on.

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH RESEND v6 1/2] leds: tlc591xx: Document binding for the TI 8/16 Channel i2c LED driver

[Jacek Anaszewski]

On 03/17/2015 11:08 PM, Andrew Lunn wrote:
> Document the binding for the TLC591xx LED driver.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Imre Kaloz <kaloz@openwrt.org>
> Cc: Matthew.Fatheree@belkin.com
> ---
>   .../devicetree/bindings/leds/leds-tlc591xx.txt     | 40 ++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt b/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
> new file mode 100644
> index 000000000000..3bbbf7024411
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
> @@ -0,0 +1,40 @@
> +LEDs connected to tlc59116 or tlc59108
> +
> +Required properties
> +- compatible: should be "ti,tlc59116" or "ti,tlc59108"
> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: typically 0x68
> +
> +Each led is represented as a sub-node of the ti,tlc59116.
> +See Documentation/devicetree/bindings/leds/common.txt
> +
> +LED sub-node properties:
> +- reg: number of LED line, 0 to 15 or 0 to 7
> +- label: (optional) name of LED
> +- linux,default-trigger : (optional)
> +
> +Examples:
> +
> +tlc59116@68 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "ti,tlc59116";
> +	reg = <0x68>;
> +
> +	wan@0 {
> +		label = "wrt1900ac:amber:wan";
> +		reg = <0x0>;
> +	};
> +
> +	2g@2 {
> +		label = "wrt1900ac:white:2g";
> +		reg = <0x2>;
> +	};
> +
> +	alive@9 {
> +		label = "wrt1900ac:green:alive";
> +		reg = <0x9>;
> +		linux,default_trigger = "heartbeat";
> +	};
> +};
>

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH RESEND v6 1/2] leds: tlc591xx: Document binding for the TI 8/16 Channel i2c LED driver

[Bryan Wu]

On Mon, Apr 20, 2015 at 6:09 AM, Jacek Anaszewski
<j.anaszewski@samsung.com> wrote:
> On 03/17/2015 11:08 PM, Andrew Lunn wrote:
>>
>> Document the binding for the TLC591xx LED driver.
>>
>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> Tested-by: Imre Kaloz <kaloz@openwrt.org>
>> Cc: Matthew.Fatheree@belkin.com
>> ---
>>   .../devicetree/bindings/leds/leds-tlc591xx.txt     | 40
>> ++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
>> b/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
>> new file mode 100644
>> index 000000000000..3bbbf7024411
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-tlc591xx.txt
>> @@ -0,0 +1,40 @@
>> +LEDs connected to tlc59116 or tlc59108
>> +
>> +Required properties
>> +- compatible: should be "ti,tlc59116" or "ti,tlc59108"
>> +- #address-cells: must be 1
>> +- #size-cells: must be 0
>> +- reg: typically 0x68
>> +
>> +Each led is represented as a sub-node of the ti,tlc59116.
>> +See Documentation/devicetree/bindings/leds/common.txt
>> +
>> +LED sub-node properties:
>> +- reg: number of LED line, 0 to 15 or 0 to 7
>> +- label: (optional) name of LED
>> +- linux,default-trigger : (optional)
>> +
>> +Examples:
>> +
>> +tlc59116@68 {
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +       compatible = "ti,tlc59116";
>> +       reg = <0x68>;
>> +
>> +       wan@0 {
>> +               label = "wrt1900ac:amber:wan";
>> +               reg = <0x0>;
>> +       };
>> +
>> +       2g@2 {
>> +               label = "wrt1900ac:white:2g";
>> +               reg = <0x2>;
>> +       };
>> +
>> +       alive@9 {
>> +               label = "wrt1900ac:green:alive";
>> +               reg = <0x9>;
>> +               linux,default_trigger = "heartbeat";
>> +       };
>> +};
>>
>
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>

Merged into my tree.
-Bryan

Re: [PATCH RESEND v6 2/2] leds: tlc591xx: Driver for the TI 8/16 Channel i2c LED driver

[Bryan Wu]

On Mon, Apr 20, 2015 at 6:07 AM, Jacek Anaszewski
<j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> On 04/20/2015 01:59 PM, Andrew Lunn wrote:
>>
>> On Mon, Apr 20, 2015 at 11:06:09AM +0200, Jacek Anaszewski wrote:
>>>
>>> Hi Andrew,
>>>
>>> Very nice driver.
>>
>>
>> Thanks. I just hope it gets accepted into this merge window.
>>
>>> I have one question below.
>>
>>
> [...]
>>>>
>>>> +static int
>>>> +tlc591xx_configure(struct device *dev,
>>>> +                  struct tlc591xx_priv *priv,
>>>> +                  const struct tlc591xx *tlc591xx)
>>>> +{
>>>> +       unsigned int i;
>>>> +       int err = 0;
>>>> +
>>>> +       tlc591xx_set_mode(priv->regmap, MODE2_DIM);
>>>
>>>
>>> It seems that all leds will be initially turned on, in dim mode.
>>> This shouldn't be fixed and probably an optional 'led-mode' DT node
>>> property should be provided for defining the initial state. It would
>>> default to OFF if not present.
>>
>>
>> If you look further down, you will find
>>
>>>> +               priv->leds[reg].ldev.default_trigger =
>>>> +                       of_get_property(child, "linux,default-trigger",
>>>> NULL);
>>
>>
>> This is the normal way in DT to specify the default on/off/keep
>> current value/heartbeat etc.
>
>
> OK, I was initially thinking that initializing LED to MODE2_DIM
> turns the LED on.
>
> Acked-by: Jacek Anaszewski <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>

Thanks, I merged it into my tree but it will target for next merge window.

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

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Tomi Valkeinen]

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

Hi Andrew, (and Brian),

On 18/03/15 00:08, Andrew Lunn wrote:
> This patchset is a driver for the TI tlc59116 16 Channel i2c LED
> driver and tlc59108 8 Channel i2c LED driver. This driver is used on
> the Belkin WRT1900AC access point and the C code is derived from code
> Belkin contributed to OpenWRT.  However it has been extensively
> re-written, and a device tree binding added to replace platform data.

I see this is now in 4.2-rc... And I see you dropped me from Cc in this
resend series. Because of that, I missed this resend, and couldn't
object to merging.

I'm not very happy about this getting merged. I think I had valid
comments to the driver, to which I did get no answers. You knew my
objections, but you did not even bother mentioning those in the resend
intro.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Andrew Lunn]

On Mon, Aug 17, 2015 at 03:11:48PM +0300, Tomi Valkeinen wrote:
> Hi Andrew, (and Brian),
> 
> On 18/03/15 00:08, Andrew Lunn wrote:
> > This patchset is a driver for the TI tlc59116 16 Channel i2c LED
> > driver and tlc59108 8 Channel i2c LED driver. This driver is used on
> > the Belkin WRT1900AC access point and the C code is derived from code
> > Belkin contributed to OpenWRT.  However it has been extensively
> > re-written, and a device tree binding added to replace platform data.
> 
> I see this is now in 4.2-rc... And I see you dropped me from Cc in this
> resend series. Because of that, I missed this resend, and couldn't
> object to merging.
> 
> I'm not very happy about this getting merged. I think I had valid
> comments to the driver, to which I did get no answers. You knew my
> objections, but you did not even bother mentioning those in the resend
> intro.

Hi Tomi

Our discussions were going around in circles, no progress being made.
The subsystem maintainer is ultimately the one who needs to decide,
bar Linus himself. Bryan Wu has seen all the discussions, and
ultimately decided the driver was O.K, despite any unresolved issues
you might have.

If you think your PWM code is so much better, please submit a revert
patch plus your PWM LED driver. We can find somebody to do a side by
side review.

Thanks
	Andrew

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Tomi Valkeinen]

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



On 17/08/15 16:27, Andrew Lunn wrote:
> On Mon, Aug 17, 2015 at 03:11:48PM +0300, Tomi Valkeinen wrote:
>> Hi Andrew, (and Brian),

(Sorry for typoing your name, Bryan =).

>> On 18/03/15 00:08, Andrew Lunn wrote:
>>> This patchset is a driver for the TI tlc59116 16 Channel i2c LED
>>> driver and tlc59108 8 Channel i2c LED driver. This driver is used on
>>> the Belkin WRT1900AC access point and the C code is derived from code
>>> Belkin contributed to OpenWRT.  However it has been extensively
>>> re-written, and a device tree binding added to replace platform data.
>>
>> I see this is now in 4.2-rc... And I see you dropped me from Cc in this
>> resend series. Because of that, I missed this resend, and couldn't
>> object to merging.
>>
>> I'm not very happy about this getting merged. I think I had valid
>> comments to the driver, to which I did get no answers. You knew my
>> objections, but you did not even bother mentioning those in the resend
>> intro.
> 
> Hi Tomi
> 
> Our discussions were going around in circles, no progress being made.
> The subsystem maintainer is ultimately the one who needs to decide,
> bar Linus himself. Bryan Wu has seen all the discussions, and
> ultimately decided the driver was O.K, despite any unresolved issues
> you might have.

If so, Bryan did not comment to the unanswered questions in any way.
More probable is that he missed them.

It's not easy as a maintainer to follow all the discussions on the
various threads. It's your responsibility as the sender of the patches
to make sure all the relevant information is available for the maintainer.

> If you think your PWM code is so much better, please submit a revert
> patch plus your PWM LED driver. We can find somebody to do a side by
> side review.

It's not about the driver code. That can be cleaned up if needed. The
question is whether the driver should be a PWM driver or a LED driver.

My question to you was that if this driver would be a PWM driver, and
you would use pwm-leds to implement the led functionality, would that
cover your use case?

We use the chip as a PWM device, for backlight, and also as a GPIO
expander. A PWM driver makes sense in our case. If it also works fine
for your use case, then isn't a PWM driver better option?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Andrew Lunn]

> > Hi Tomi
> > 
> > Our discussions were going around in circles, no progress being made.
> > The subsystem maintainer is ultimately the one who needs to decide,
> > bar Linus himself. Bryan Wu has seen all the discussions, and
> > ultimately decided the driver was O.K, despite any unresolved issues
> > you might have.
> 
> If so, Bryan did not comment to the unanswered questions in any way.
> More probable is that he missed them.

I would not jump to that conclusion. As one of the maintainers of
Marvell SoCs, i don't feel the need to address every open question
going back and forward between two developers, when i decide to end
the discussion by just accepting the patch.

> > If you think your PWM code is so much better, please submit a revert
> > patch plus your PWM LED driver. We can find somebody to do a side by
> > side review.
> 
> It's not about the driver code. That can be cleaned up if needed. The
> question is whether the driver should be a PWM driver or a LED driver.

There i disagree. It is all about the code. Release early, release
often.  Show us the code. Get it discussed, reviewed, tested, and then
ultimately merged.

You keep saying PWM is the way to solve this problem, but where is
your code showing your solution is superior to mine, yet still solves
my use case?

So please stop talking and show us the code.

   Thanks
	Andrew

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Tomi Valkeinen]

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


On 17/08/15 17:21, Andrew Lunn wrote:

>>> If you think your PWM code is so much better, please submit a revert
>>> patch plus your PWM LED driver. We can find somebody to do a side by
>>> side review.
>>
>> It's not about the driver code. That can be cleaned up if needed. The
>> question is whether the driver should be a PWM driver or a LED driver.
> 
> There i disagree. It is all about the code. Release early, release
> often.  Show us the code. Get it discussed, reviewed, tested, and then
> ultimately merged.
> 
> You keep saying PWM is the way to solve this problem, but where is
> your code showing your solution is superior to mine, yet still solves
> my use case?

No, I have not said PWM is the way to solve this problem. I have said it
sounds to me that a PWM driver better matches the HW and our use case. I
have asked you if PWM + pwd-leds would solve your use case as well as a
led driver.

If you don't have an answer, it's fine to say "I don't know". But you
have just ignored the question. And even worse, you dropped me (and
Vignesh) from the latest mail thread.

We have a pwm driver for the chip, but it only supports TLC59108. It
should be trivial to extend it to support TLC59116, but it's pointless
to spend time on that if a PWM driver doesn't handle your use cases.

> So please stop talking and show us the code.

TLC591xx is quite simple HW, and the code doing the HW programming
should be identical for a pwm and for a led driver. I don't see either a
pwm or a led driver being somehow superior. They are very similar simple
drivers, implementing a different interface. Whether the interface works
for you should be clear from the interface itself. The code is just details.

On the other hand, if you want something you can use to test, it's a
different case, but you haven't indicated any interest in that.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Andrew Lunn]

> TLC591xx is quite simple HW, and the code doing the HW programming
> should be identical for a pwm and for a led driver. I don't see either a
> pwm or a led driver being somehow superior. They are very similar simple
> drivers, implementing a different interface. Whether the interface works
> for you should be clear from the interface itself. The code is just details.
> 
> On the other hand, if you want something you can use to test, it's a
> different case, but you haven't indicated any interest in that.

What i want is code in the kernel to driver the LEDs on my Linksys
WRT1900ac. You say, "the code is just details", is just plain
wrong. Code is the Linux kernel and is the process of getting stuff
into the kernel. I've followed the process, submitted code for review,
fixed up comments, got it tested and ACKed by people, and merged.

Where as all you have done is emailed.

So i'm done.

   Andrew

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Bryan Wu]

I apologize to ignore this email, since Jacek is taking over the
maintainership of LED subsystem. Please include him in the email
thread.

Thanks,
-Bryan

On Mon, Aug 17, 2015 at 9:48 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> TLC591xx is quite simple HW, and the code doing the HW programming
>> should be identical for a pwm and for a led driver. I don't see either a
>> pwm or a led driver being somehow superior. They are very similar simple
>> drivers, implementing a different interface. Whether the interface works
>> for you should be clear from the interface itself. The code is just details.
>>
>> On the other hand, if you want something you can use to test, it's a
>> different case, but you haven't indicated any interest in that.
>
> What i want is code in the kernel to driver the LEDs on my Linksys
> WRT1900ac. You say, "the code is just details", is just plain
> wrong. Code is the Linux kernel and is the process of getting stuff
> into the kernel. I've followed the process, submitted code for review,
> fixed up comments, got it tested and ACKed by people, and merged.
>
> Where as all you have done is emailed.
>
> So i'm done.
>
>    Andrew
>

Re: [PATCH RESEND v6 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[Tomi Valkeinen]

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



On 17/08/15 19:48, Andrew Lunn wrote:
>> TLC591xx is quite simple HW, and the code doing the HW programming
>> should be identical for a pwm and for a led driver. I don't see either a
>> pwm or a led driver being somehow superior. They are very similar simple
>> drivers, implementing a different interface. Whether the interface works
>> for you should be clear from the interface itself. The code is just details.
>>
>> On the other hand, if you want something you can use to test, it's a
>> different case, but you haven't indicated any interest in that.
> 
> What i want is code in the kernel to driver the LEDs on my Linksys
> WRT1900ac. You say, "the code is just details", is just plain
> wrong. Code is the Linux kernel and is the process of getting stuff
> into the kernel. I've followed the process, submitted code for review,
> fixed up comments, got it tested and ACKed by people, and merged.

Your process also included in ignoring my questions, and dropping me
from cc so you'd get the driver merged. My idea of the Linux development
process differs greatly from yours.

> Where as all you have done is emailed.

Yes, as you were writing the driver for the HW, and I was expecting
answers from you. I would find it rather rude if I'm working on a
driver, getting comments on it and fixing things, and suddenly someone
else goes and writes his own version of the driver for the HW.

I presumed that you would either answer to my comments, or you would cc
me when you want to ping the maintainers.

Sure, if the driver is complex or somehow controversial it's good to
have different implementations of the driver so the best one can be
picked. This is not the case here. The HW is simple, and both PWM and
LED interfaces are simple. The code is just details in such a case.

I can post a pwm based driver tomorrow. I could've done that long ago if
you would have just said that you're not interested in finding answers
to my questions or trying a pwm driver. Or if you'd have bothered to cc
me in the last series, so I would've realized you are done with the driver.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]