[PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Andrew Lunn]

This patchset is a driver for the TI tlc59116 16 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@belkin.com

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

[PATCHv3 1/2] leds: tlc59116: Document binding for the TI 16 Channel i2c LED driver

[Andrew Lunn]

Document the binding for the TLC59116 LED driver.

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

diff --git a/Documentation/devicetree/bindings/leds/leds-tlc59116.txt b/Documentation/devicetree/bindings/leds/leds-tlc59116.txt
new file mode 100644
index 000000000000..45c2123774d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-tlc59116.txt
@@ -0,0 +1,40 @@
+LEDs connected to tcl59116
+
+Required properties
+- compatible: should be "ti,tlc59116"
+- #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
+- 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

[PATCHv3 2/2] leds: tlc59116: Driver for the TI 16 Channel i2c LED driver

[Andrew Lunn]

The TLC59116 is an I2C bus controlled 16-channel LED driver.  Each LED
output has its own 8-bit fixed-frequency PWM controller to control the
brightness of the LED.

This is based on a driver from Belkin, but has been extensively
rewritten.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Cc: Matthew.Fatheree@belkin.com
---
 drivers/leds/Kconfig         |   8 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-tlc59116.c | 252 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 261 insertions(+)
 create mode 100644 drivers/leds/leds-tlc59116.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a6c3d2f153f3..8fc283c01673 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -457,6 +457,14 @@ config LEDS_TCA6507
 	  LED driver chips accessed via the I2C bus.
 	  Driver support brightness control and hardware-assisted blinking.
 
+config LEDS_TLC59116
+	tristate "LED driver for TLC59116F controllers"
+	depends on LEDS_CLASS && I2C
+	select REGMAP_I2C
+	help
+	  This option enables support for Texas Instruments TLC59116F
+	  LED controller.
+
 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 1c65a191d907..8e7d20acfa7b 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -30,6 +30,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_TLC59116)		+= leds-tlc59116.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-tlc59116.c b/drivers/leds/leds-tlc59116.c
new file mode 100644
index 000000000000..fddad1f45c87
--- /dev/null
+++ b/drivers/leds/leds-tlc59116.c
@@ -0,0 +1,252 @@
+/*
+ * Copyright 2014 Belkin Inc.
+ * Copyright 2014 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/regmap.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#define TLC59116_LEDS		16
+
+#define TLC59116_REG_MODE1	0x00
+#define MODE1_RESPON_ADDR_MASK	0xF0
+#define MODE1_NORMAL_MODE	(0 << 4)
+#define MODE1_SPEED_MODE	(1 << 4)
+
+#define TLC59116_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 TLC59116_REG_PWM(x)	(0x02 + (x))
+
+#define TLC59116_REG_GRPPWM	0x12
+#define TLC59116_REG_GRPFREQ	0x13
+
+/* LED Driver Output State, determine the source that drives LED outputs */
+#define TLC59116_REG_LEDOUT(x)	(0x14 + ((x) >> 2))
+#define TLC59116_LED_OFF	0x0	/* Output LOW */
+#define TLC59116_LED_ON		0x1	/* Output HI-Z */
+#define TLC59116_DIM		0x2	/* Dimming */
+#define TLC59116_BLINK		0x3	/* Blinking */
+#define LED_MASK		0x3
+
+#define ldev_to_led(c)		container_of(c, struct tlc59116_led, ldev)
+#define work_to_led(work)	container_of(work, struct tlc59116_led, work)
+
+struct tlc59116_led {
+	bool active;
+	struct regmap *regmap;
+	unsigned int led_no;
+	struct led_classdev ldev;
+	struct work_struct work;
+};
+
+struct tlc59116_priv {
+	struct tlc59116_led leds[TLC59116_LEDS];
+};
+
+static int
+tlc59116_set_mode(struct regmap *regmap, u8 mode)
+{
+	int err;
+	u8 val;
+
+	if ((mode != MODE2_DIM) && (mode != MODE2_BLINK))
+		mode = MODE2_DIM;
+
+	/* Configure MODE1 register */
+	err = regmap_write(regmap, TLC59116_REG_MODE1, MODE1_NORMAL_MODE);
+	if (err)
+		return err;
+
+	/* Configure MODE2 Reg */
+	val = MODE2_OCH_STOP | mode;
+
+	return regmap_write(regmap, TLC59116_REG_MODE2, val);
+}
+
+static int
+tlc59116_set_led(struct tlc59116_led *led, u8 val)
+{
+	struct regmap *regmap = led->regmap;
+	unsigned int i = (led->led_no % 4) * 2;
+	unsigned int addr = TLC59116_REG_LEDOUT(led->led_no);
+	unsigned int mask = LED_MASK << i;
+
+	val = val << i;
+
+	return regmap_update_bits(regmap, addr, mask, val);
+}
+
+static void
+tlc59116_led_work(struct work_struct *work)
+{
+	struct tlc59116_led *led = work_to_led(work);
+	struct regmap *regmap = led->regmap;
+	int err;
+	u8 pwm;
+
+	pwm = TLC59116_REG_PWM(led->led_no);
+	err = regmap_write(regmap, pwm, led->ldev.brightness);
+	if (err)
+		dev_err(led->ldev.dev, "Failed setting brightness\n");
+}
+
+static void
+tlc59116_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	struct tlc59116_led *led = ldev_to_led(led_cdev);
+
+	led->ldev.brightness = value;
+	schedule_work(&led->work);
+}
+
+static void
+tlc59116_destroy_devices(struct tlc59116_priv *priv, unsigned int i)
+{
+	while (--i >= 0) {
+		if (priv->leds[i].active) {
+			led_classdev_unregister(&priv->leds[i].ldev);
+			cancel_work_sync(&priv->leds[i].work);
+		}
+	}
+}
+
+static int
+tlc59116_configure(struct device *dev,
+		   struct tlc59116_priv *priv,
+		   struct regmap *regmap)
+{
+	unsigned int i;
+	int err = 0;
+
+	tlc59116_set_mode(regmap, MODE2_DIM);
+	for (i = 0; i < TLC59116_LEDS; i++) {
+		struct tlc59116_led *led = &priv->leds[i];
+
+		if (!led->active)
+			continue;
+
+		led->regmap = regmap;
+		led->led_no = i;
+		led->ldev.brightness_set = tlc59116_led_set;
+		led->ldev.max_brightness = LED_FULL;
+		INIT_WORK(&led->work, tlc59116_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;
+		}
+		tlc59116_set_led(led, TLC59116_DIM);
+	}
+
+	return 0;
+
+exit:
+	tlc59116_destroy_devices(priv, i);
+	return err;
+}
+
+static const struct regmap_config tlc59116_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x1e,
+};
+
+static int
+tlc59116_probe(struct i2c_client *client,
+	       const struct i2c_device_id *id)
+{
+	struct tlc59116_priv *priv = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
+	struct device_node *np = client->dev.of_node, *child;
+	struct regmap *regmap;
+	int err, count, reg;
+
+	count = of_get_child_count(np);
+	if (!count || count > TLC59116_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;
+
+	regmap = devm_regmap_init_i2c(client, &tlc59116_regmap);
+	if (IS_ERR(regmap)) {
+		err = PTR_ERR(regmap);
+		dev_err(dev, "Failed to allocate register map: %d\n", err);
+		return err;
+	}
+
+	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 >= TLC59116_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 tlc59116_configure(dev, priv, regmap);
+}
+
+static int
+tlc59116_remove(struct i2c_client *client)
+{
+	struct tlc59116_priv *priv = i2c_get_clientdata(client);
+
+	tlc59116_destroy_devices(priv, TLC59116_LEDS);
+
+	return 0;
+}
+
+static const struct of_device_id of_tlc59116_leds_match[] = {
+	{ .compatible = "ti,tlc59116", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_tlc59116_leds_match);
+
+static const struct i2c_device_id tlc59116_id[] = {
+	{ "tlc59116" },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, tlc59116_id);
+
+static struct i2c_driver tlc59116_driver = {
+	.driver = {
+		.name = "tlc59116",
+		.of_match_table = of_match_ptr(of_tlc59116_leds_match),
+	},
+	.probe = tlc59116_probe,
+	.remove = tlc59116_remove,
+	.id_table = tlc59116_id,
+};
+
+module_i2c_driver(tlc59116_driver);
+
+MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("TLC59116 LED driver");
-- 
2.1.4

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 890 bytes --]

Hi,

On 15/01/15 01:15, Andrew Lunn wrote:
> This patchset is a driver for the TI tlc59116 16 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.

We have a TLC59108 on one of our boards, and with a quick glance it
looks about the same as TLC59116, except the amount of outputs. Vignesh
wrote a driver for it and was about to send it for review.

However, Vignesh implemented it as a PWM driver. We use it for LCD
backlight (via pwm-backlight).

I'm not very familiar with LED and PWM drivers, but doesn't implementing
this as a LED driver prevent us from using it as a backlight? Whereas it
looks like PWM can be used as a led via pwm-leds (I think).

 Tomi



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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Andrew Lunn]

On Fri, Jan 16, 2015 at 04:52:12PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 15/01/15 01:15, Andrew Lunn wrote:
> > This patchset is a driver for the TI tlc59116 16 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.
> 
> We have a TLC59108 on one of our boards, and with a quick glance it
> looks about the same as TLC59116, except the amount of outputs. Vignesh
> wrote a driver for it and was about to send it for review.
> 
> However, Vignesh implemented it as a PWM driver. We use it for LCD
> backlight (via pwm-backlight).
> 
> I'm not very familiar with LED and PWM drivers, but doesn't implementing
> this as a LED driver prevent us from using it as a backlight? Whereas it
> looks like PWM can be used as a led via pwm-leds (I think).
> 
Hi Tomi

I've no idea about backlighting....

But the driver does fully implement brightness. So on my board:

echo 128 > /sys/class/leds/wrt1900ac\:white\:wan/brightness

will set that LED to 128/256 brightness. You have 255 steps of
brightness.

The reason i did not model it as a PWM, is that it cannot fulfil the
PWM interface. The configure interface is:

int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);

However with this device, you have no control over the period. It is
fixed, from a 97-kHz clock. All you can change is the duty as a ratio
between 0 and 256. There is the group PWM, where you do have control
over the period. But as the name implies, this PWM is shared for all
outputs, which is not what the PWM interface expects, it wants to be
able to control them individually. Also, it only has a range of 24 Hz
to 10.73 s. Again, i have no idea about backlights, but i suspect if
it is driven at 24Hz, i'm going to get a headache. 

The LED interface can be fully implemented. So that is what i have
done.

So i think modeling this as an LED driver is correct, and maybe you
need to implemented an led_bl.c driver?

	Andrew




--
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: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[R, Vignesh]

On 1/16/2015 9:25 PM, Andrew Lunn wrote:
> On Fri, Jan 16, 2015 at 04:52:12PM +0200, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 15/01/15 01:15, Andrew Lunn wrote:
>>> This patchset is a driver for the TI tlc59116 16 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.
>>
>> We have a TLC59108 on one of our boards, and with a quick glance it
>> looks about the same as TLC59116, except the amount of outputs. Vignesh
>> wrote a driver for it and was about to send it for review.
>>
>> However, Vignesh implemented it as a PWM driver. We use it for LCD
>> backlight (via pwm-backlight).
>>
>> I'm not very familiar with LED and PWM drivers, but doesn't implementing
>> this as a LED driver prevent us from using it as a backlight? Whereas it
>> looks like PWM can be used as a led via pwm-leds (I think).
>>
> Hi Tomi
> 
> I've no idea about backlighting....
> 
> But the driver does fully implement brightness. So on my board:
> 
> echo 128 > /sys/class/leds/wrt1900ac\:white\:wan/brightness
> 
> will set that LED to 128/256 brightness. You have 255 steps of
> brightness.
> 
> The reason i did not model it as a PWM, is that it cannot fulfil the
> PWM interface. The configure interface is:
> 
> int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
> 
> However with this device, you have no control over the period. It is
> fixed, from a 97-kHz clock. All you can change is the duty as a ratio
> between 0 and 256. There is the group PWM, where you do have control
> over the period. But as the name implies, this PWM is shared for all
> outputs, which is not what the PWM interface expects, it wants to be
> able to control them individually. Also, it only has a range of 24 Hz
> to 10.73 s. Again, i have no idea about backlights, but i suspect if
> it is driven at 24Hz, i'm going to get a headache. 
> 
> The LED interface can be fully implemented. So that is what i have
> done.

I understand that period of TLC chip cannot be changed and hence cannot
fully implement PWM interface. But, suppose I want to control brightness
of an LCD screen, with your current design, my LCD driver can never can do
something like:
	pwm_get(chip);
	pwm_update_brightness();
	pwm_remove();
I will always have to rely on sysfs entries to control brightness.
 
If the driver is implemented as pwm-backlight, then brightness can
be controlled both via sysfs entries and pwm-core callbacks(in kernel).

> 
> So i think modeling this as an LED driver is correct, and maybe you
> need to implemented an led_bl.c driver?
> 

But, as far as I understand, leds-pwm.c does almost similar function.  

Regards
Vignesh R

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Andrew Lunn]

> I understand that period of TLC chip cannot be changed and hence cannot
> fully implement PWM interface.

O.K, so that is agreed.

> But, suppose I want to control brightness
> of an LCD screen, with your current design, my LCD driver can never can do
> something like:
> 	pwm_get(chip);
> 	pwm_update_brightness();
> 	pwm_remove();
> I will always have to rely on sysfs entries to control brightness.

Or you can use the kernel API for controlling leds. There is a hint in

Documentation/devicetree/bindings/leds/common.txt

- linux,default-trigger :  This parameter, if present, is a
    string defining the trigger assigned to the LED.  Current triggers are:
     "backlight" - LED will act as a back-light, controlled by the framebuffer
                   system

So somebody has at least thought about this, and there is

drivers/leds/trigger/ledtrig-backlight.c

What i also find interesting is:

drivers/video/backlight/adp8860_bl.c
drivers/video/backlight/adp8870_bl.c
drivers/video/backlight/lm3639_bl.c

all implement an led_class driver. So it does seem some people think
backlight LEDs can be models using the led class.

	  Andrew

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Andrew Lunn]

On Fri, Jan 16, 2015 at 04:52:12PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 15/01/15 01:15, Andrew Lunn wrote:
> > This patchset is a driver for the TI tlc59116 16 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.
> 
> We have a TLC59108 on one of our boards, and with a quick glance it
> looks about the same as TLC59116, except the amount of outputs.

Hi Tomi

I took a look at the TLC59108. The hardware designs have not made it
easy, but it does look similar enough that my driver can be extended
to cover both. These extensions It will not change the DT binding, it
will just gain a new compatibility string. So i would prefer my driver
is first fully reviewed and accepted, and then i will take a stab at
extending it for the TLC59108. I will need help testing it, but i hope
you can help there.

    Andrew

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Tomi Valkeinen]

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

On 16/01/15 21:17, Andrew Lunn wrote:
> On Fri, Jan 16, 2015 at 04:52:12PM +0200, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 15/01/15 01:15, Andrew Lunn wrote:
>>> This patchset is a driver for the TI tlc59116 16 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.
>>
>> We have a TLC59108 on one of our boards, and with a quick glance it
>> looks about the same as TLC59116, except the amount of outputs.
> 
> Hi Tomi
> 
> I took a look at the TLC59108. The hardware designs have not made it
> easy, but it does look similar enough that my driver can be extended
> to cover both. These extensions It will not change the DT binding, it
> will just gain a new compatibility string. So i would prefer my driver
> is first fully reviewed and accepted, and then i will take a stab at
> extending it for the TLC59108. I will need help testing it, but i hope
> you can help there.

Ok.

I'm not familiar with the LED/PWM frameworks, so I want to summarize our
use case and how I guess this should be done:

We use the TLC59108 to provide backlight to a LCD, but in addition to
that, it is used as a GPIO expander (or GPO, as it cannot do input). In
your earlier patch versions there were some 'gpio' leftovers. Does your
board have such GPIO use also?

So my current thinking is that TLC591xx should be a LED driver (as it
sounded to me that PWM is not quite suitable for it). On top of that, we
need generic 'led-backlight' and 'led-gpio' drivers, each of which uses
the given LED driver to do the hardware manipulation, and they expose a
backlight device and gpio device, respectively.

Another option would be an mfd device, but that doesn't sound good as I
think the backlight and gpio functionality here is not related to
TLC591xx as such, but to a LED device in general.

Does this sound sane?

 Tomi



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

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Andrew Lunn]

> I'm not familiar with the LED/PWM frameworks, so I want to summarize our
> use case and how I guess this should be done:
> 
> We use the TLC59108 to provide backlight to a LCD, but in addition to
> that, it is used as a GPIO expander (or GPO, as it cannot do input). In
> your earlier patch versions there were some 'gpio' leftovers. Does your
> board have such GPIO use also?

No, only LEDs.
 
> So my current thinking is that TLC591xx should be a LED driver (as it
> sounded to me that PWM is not quite suitable for it). On top of that, we
> need generic 'led-backlight' and 'led-gpio' drivers, each of which uses
> the given LED driver to do the hardware manipulation, and they expose a
> backlight device and gpio device, respectively.

That sounds sensible. led-backlight seems to be mostly implemented
already via the led trigger code. Adding a minimal backlight_ops does
not look too hard. You might also be able to do some cleanup of the
other led based backlight drivers. 

led-gpio looks like more work, but i don't see why it should not be
possible. One thing i do need to check is that if the brightness is
set to 255 the output is not set to 255/256 on and you have a regular
glitch. For an LED that does not really matter, but for GPIO it would
not be good.

    Andrew

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Tomi Valkeinen]

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

On 20/01/15 15:26, Andrew Lunn wrote:
>> I'm not familiar with the LED/PWM frameworks, so I want to summarize our
>> use case and how I guess this should be done:
>>
>> We use the TLC59108 to provide backlight to a LCD, but in addition to
>> that, it is used as a GPIO expander (or GPO, as it cannot do input). In
>> your earlier patch versions there were some 'gpio' leftovers. Does your
>> board have such GPIO use also?
> 
> No, only LEDs.
>  
>> So my current thinking is that TLC591xx should be a LED driver (as it
>> sounded to me that PWM is not quite suitable for it). On top of that, we
>> need generic 'led-backlight' and 'led-gpio' drivers, each of which uses
>> the given LED driver to do the hardware manipulation, and they expose a
>> backlight device and gpio device, respectively.
> 
> That sounds sensible. led-backlight seems to be mostly implemented
> already via the led trigger code. Adding a minimal backlight_ops does
> not look too hard. You might also be able to do some cleanup of the
> other led based backlight drivers. 
> 
> led-gpio looks like more work, but i don't see why it should not be
> possible. One thing i do need to check is that if the brightness is
> set to 255 the output is not set to 255/256 on and you have a regular
> glitch. For an LED that does not really matter, but for GPIO it would
> not be good.

Right. TLC591xx has the constant output mode which should be used for
GPIO. Perhaps that mode could be always used when the brightness is
turned to maximum. I haven't read the spec carefully enough to know if
there are some downsides.

 Tomi



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

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Andrew Lunn]

> Right. TLC591xx has the constant output mode which should be used for
> GPIO. Perhaps that mode could be always used when the brightness is
> turned to maximum. I haven't read the spec carefully enough to know if
> there are some downsides.

The original Belkin code did use that mode for full brightness. But it
added complexity to the code which i did not see a need for. Now you
need gpio support, i can add it back.

     Andrew

Re: [PATCHv3 0/2] Driver for TI tlc59116 16 Channel i2c LED driver

[Geert Uytterhoeven]

On Tue, Jan 20, 2015 at 2:26 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> led-gpio looks like more work, but i don't see why it should not be
> possible. One thing i do need to check is that if the brightness is
> set to 255 the output is not set to 255/256 on and you have a regular

is set to 255/256?

> glitch. For an LED that does not really matter, but for GPIO it would
> not be good.

Indeed.

Gr{oetje,eeting}s,

                        Geert, who had a quick look at the data sheet recently

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds