[PATCHv5 0/2] Driver for TI tlc591xx 8/16 Channel i2c LED driver

[PATCHv5 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@belkin.com

Sinve 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

[PATCHv5 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-g2DYL2Zd6BY@public.gmane.org>
Cc: Matthew.Fatheree-REUqjI8E1xrQT0dZR+AlfA@public.gmane.org
---
 .../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

--
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

[PATCHv5 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>
Cc: Matthew.Fatheree@belkin.com
---
 drivers/leds/Kconfig         |   8 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-tlc591xx.c | 309 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)
 create mode 100644 drivers/leds/leds-tlc591xx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a6c3d2f153f3..67cba2032543 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_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 1c65a191d907..0558117a9407 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_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..531e83f54465
--- /dev/null
+++ b/drivers/leds/leds-tlc591xx.c
@@ -0,0 +1,309 @@
+/*
+ * 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 TLC59116_LEDS		16
+#define TLC59108_LEDS		8
+
+#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;
+	struct regmap *regmap;
+	unsigned int led_no;
+	struct led_classdev ldev;
+	struct work_struct work;
+	const struct tlc591xx *tlc591xx;
+};
+
+struct tlc591xx_priv {
+	struct tlc591xx_led leds[TLC59116_LEDS];
+};
+
+static int
+tlc59116_reg_ledout(unsigned int led) {
+
+	return (0x14 + (led >> 2));
+}
+
+static int
+tlc59108_reg_ledout(unsigned int led) {
+
+	return (0x0c + (led >> 2));
+}
+
+struct tlc591xx {
+	unsigned int maxleds;
+	int (*reg_ledout)(unsigned int);
+};
+
+static const struct tlc591xx tlc59116 = {
+	.maxleds = TLC59116_LEDS,
+	.reg_ledout = tlc59116_reg_ledout,
+};
+
+static const struct tlc591xx tlc59108 = {
+	.maxleds = TLC59108_LEDS,
+	.reg_ledout = tlc59108_reg_ledout,
+};
+
+static int
+tlc591xx_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, TLC591XX_REG_MODE1, MODE1_NORMAL_MODE);
+	if (err)
+		return err;
+
+	/* Configure MODE2 Reg */
+	val = MODE2_OCH_STOP | mode;
+
+	return regmap_write(regmap, TLC591XX_REG_MODE2, val);
+}
+
+static int
+tlc591xx_set_ledout(struct tlc591xx_led *led, u8 val)
+{
+	struct regmap *regmap = led->regmap;
+	unsigned int i = (led->led_no % 4) * 2;
+	unsigned int addr = led->tlc591xx->reg_ledout(led->led_no);
+	unsigned int mask = LEDOUT_MASK << i;
+
+	val = val << i;
+
+	return regmap_update_bits(regmap, addr, mask, val);
+}
+
+static int
+tlc591xx_set_pwm(struct tlc591xx_led *led, u8 val)
+{
+	struct regmap *regmap = led->regmap;
+	u8 pwm = TLC591XX_REG_PWM(led->led_no);
+
+	return regmap_write(regmap, pwm, led->ldev.brightness);
+}
+
+static void
+tlc591xx_led_work(struct work_struct *work)
+{
+	struct tlc591xx_led *led = work_to_led(work);
+	int err;
+
+	switch (led->ldev.brightness) {
+	case 0:
+		err = tlc591xx_set_ledout(led, LEDOUT_OFF);
+		break;
+	case LED_FULL:
+		err = tlc591xx_set_ledout(led, LEDOUT_ON);
+		break;
+	default:
+		err = tlc591xx_set_ledout(led, LEDOUT_DIM);
+		if (!err)
+			err = tlc591xx_set_pwm(led, led->ldev.brightness);
+	}
+
+	if (err)
+		dev_err(led->ldev.dev, "Failed setting brightness\n");
+}
+
+static void
+tlc591xx_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	struct tlc591xx_led *led = ldev_to_led(led_cdev);
+
+	led->ldev.brightness = value;
+	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,
+		   struct regmap *regmap,
+		   const struct tlc591xx *tlc591xx)
+{
+	unsigned int i;
+	int err = 0;
+
+	tlc591xx_set_mode(regmap, MODE2_DIM);
+	for (i = 0; i < TLC59116_LEDS; i++) {
+		struct tlc591xx_led *led = &priv->leds[i];
+
+		if (!led->active)
+			continue;
+
+		led->regmap = regmap;
+		led->led_no = i;
+		led->tlc591xx = tlc591xx;
+		led->ldev.brightness_set = tlc591xx_led_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 tlc591xx *tlc591xx;
+	struct tlc591xx_priv *priv;
+	struct regmap *regmap;
+	int err, count, reg;
+
+	tlc591xx = of_match_device(of_tlc591xx_leds_match, dev)->data;
+
+	count = of_get_child_count(np);
+	if (!count || count > tlc591xx->maxleds)
+		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, &tlc591xx_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 >= tlc591xx->maxleds)
+			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, regmap, tlc591xx);
+}
+
+static int
+tlc591xx_remove(struct i2c_client *client)
+{
+	struct tlc591xx_priv *priv = i2c_get_clientdata(client);
+
+	tlc591xx_destroy_devices(priv, TLC59116_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: [PATCHv5 2/2] leds: tlc591xx: Driver for the TI 8/16 Channel i2c LED driver

[Tomi Valkeinen]

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

On 22/01/15 00:29, 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>
> Cc: Matthew.Fatheree@belkin.com
> ---
>  drivers/leds/Kconfig         |   8 ++
>  drivers/leds/Makefile        |   1 +
>  drivers/leds/leds-tlc591xx.c | 309 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 318 insertions(+)
>  create mode 100644 drivers/leds/leds-tlc591xx.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a6c3d2f153f3..67cba2032543 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_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 1c65a191d907..0558117a9407 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_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..531e83f54465
> --- /dev/null
> +++ b/drivers/leds/leds-tlc591xx.c
> @@ -0,0 +1,309 @@
> +/*
> + * 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 TLC59116_LEDS		16
> +#define TLC59108_LEDS		8

These two are quite confusing. In some places they are used as "number
of leds on TLC5910x device", and in some places they are used as "max
leds on any TLC device".

When defining the static values for struct tlc59116 and tlc59108 I think
it would be much cleaner to just use the integer there, instead of one
of the defines above. And if you needs a "max leds on any TLC device",
define it clearly, like TLC591XX_MAX_LEDS or such.

> +
> +#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;
> +	struct regmap *regmap;
> +	unsigned int led_no;
> +	struct led_classdev ldev;
> +	struct work_struct work;
> +	const struct tlc591xx *tlc591xx;
> +};

The struct above contains fields that are common to all led instances.
Maybe a matter of taste, but I'd rather have one struct representing the
whole device (struct tlc591xx_priv I guess), which would contains
'regmap' and 'tlc591xx' fields. And tlc591xx_led would contain a pointer
to the tlc591xx_priv.

Not a big difference, but having regmap in the struct above makes me
think that each leds has its own regmap.

> +
> +struct tlc591xx_priv {
> +	struct tlc591xx_led leds[TLC59116_LEDS];
> +};
> +
> +static int
> +tlc59116_reg_ledout(unsigned int led) {
> +
> +	return (0x14 + (led >> 2));
> +}
> +
> +static int
> +tlc59108_reg_ledout(unsigned int led) {
> +
> +	return (0x0c + (led >> 2));
> +}

You could just have the offset (0x14 or 0x0c) as a value in the struct
below, and then you would not need the functions above.

> +
> +struct tlc591xx {
> +	unsigned int maxleds;
> +	int (*reg_ledout)(unsigned int);
> +};
> +
> +static const struct tlc591xx tlc59116 = {
> +	.maxleds = TLC59116_LEDS,
> +	.reg_ledout = tlc59116_reg_ledout,
> +};
> +
> +static const struct tlc591xx tlc59108 = {
> +	.maxleds = TLC59108_LEDS,

So here using "8", and "16" above, would look much cleaner to me.

> +	.reg_ledout = tlc59108_reg_ledout,
> +};
> +
> +static int
> +tlc591xx_set_mode(struct regmap *regmap, u8 mode)
> +{
> +	int err;
> +	u8 val;
> +
> +	if ((mode != MODE2_DIM) && (mode != MODE2_BLINK))
> +		mode = MODE2_DIM;

If mode is not DIM or BLINK, should this function return an error?

> +	/* Configure MODE1 register */
> +	err = regmap_write(regmap, TLC591XX_REG_MODE1, MODE1_NORMAL_MODE);
> +	if (err)
> +		return err;
> +
> +	/* Configure MODE2 Reg */
> +	val = MODE2_OCH_STOP | mode;
> +
> +	return regmap_write(regmap, TLC591XX_REG_MODE2, val);
> +}
> +
> +static int
> +tlc591xx_set_ledout(struct tlc591xx_led *led, u8 val)
> +{
> +	struct regmap *regmap = led->regmap;
> +	unsigned int i = (led->led_no % 4) * 2;
> +	unsigned int addr = led->tlc591xx->reg_ledout(led->led_no);
> +	unsigned int mask = LEDOUT_MASK << i;
> +
> +	val = val << i;
> +
> +	return regmap_update_bits(regmap, addr, mask, val);
> +}
> +
> +static int
> +tlc591xx_set_pwm(struct tlc591xx_led *led, u8 val)
> +{
> +	struct regmap *regmap = led->regmap;
> +	u8 pwm = TLC591XX_REG_PWM(led->led_no);
> +
> +	return regmap_write(regmap, pwm, led->ldev.brightness);
> +}
> +
> +static void
> +tlc591xx_led_work(struct work_struct *work)
> +{

Maybe it's normal for LED drivers, but why is the workqueue needed? Why
not just do the work synchronously?

> +	struct tlc591xx_led *led = work_to_led(work);
> +	int err;
> +
> +	switch (led->ldev.brightness) {

Can the brightness here be < 0 or > LED_FULL?

> +	case 0:
> +		err = tlc591xx_set_ledout(led, LEDOUT_OFF);
> +		break;
> +	case LED_FULL:
> +		err = tlc591xx_set_ledout(led, LEDOUT_ON);
> +		break;
> +	default:
> +		err = tlc591xx_set_ledout(led, LEDOUT_DIM);
> +		if (!err)
> +			err = tlc591xx_set_pwm(led, led->ldev.brightness);
> +	}

There doesn't seem to be any locking used for the fields this function
accesses. Perhaps none is needed, but an async work without locks and
without comments about it makes me feel a bit nervous.

> +
> +	if (err)
> +		dev_err(led->ldev.dev, "Failed setting brightness\n");
> +}
> +
> +static void
> +tlc591xx_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> +{
> +	struct tlc591xx_led *led = ldev_to_led(led_cdev);
> +
> +	led->ldev.brightness = value;
> +	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,
> +		   struct regmap *regmap,
> +		   const struct tlc591xx *tlc591xx)
> +{
> +	unsigned int i;
> +	int err = 0;
> +
> +	tlc591xx_set_mode(regmap, MODE2_DIM);
> +	for (i = 0; i < TLC59116_LEDS; i++) {

Looping for tlc591xx->maxleds should be enough?

> +		struct tlc591xx_led *led = &priv->leds[i];
> +
> +		if (!led->active)
> +			continue;
> +
> +		led->regmap = regmap;
> +		led->led_no = i;
> +		led->tlc591xx = tlc591xx;
> +		led->ldev.brightness_set = tlc591xx_led_set;

A matter of taste, but I'd rather have '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 tlc591xx *tlc591xx;
> +	struct tlc591xx_priv *priv;
> +	struct regmap *regmap;
> +	int err, count, reg;
> +
> +	tlc591xx = of_match_device(of_tlc591xx_leds_match, dev)->data;

I presume of_match_device() can return NULL or an error, making the
above crash.

> +
> +	count = of_get_child_count(np);

'np' may be NULL. I'm not sure how of_get_child_count() likes that.

 Tomi



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

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

[Andrew Lunn]

On Mon, Jan 26, 2015 at 01:32:40PM +0200, Tomi Valkeinen wrote:
> On 22/01/15 00:29, 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>
> > Cc: Matthew.Fatheree@belkin.com
> > ---
> >  drivers/leds/Kconfig         |   8 ++
> >  drivers/leds/Makefile        |   1 +
> >  drivers/leds/leds-tlc591xx.c | 309 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 318 insertions(+)
> >  create mode 100644 drivers/leds/leds-tlc591xx.c
> > 
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index a6c3d2f153f3..67cba2032543 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_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 1c65a191d907..0558117a9407 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_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..531e83f54465
> > --- /dev/null
> > +++ b/drivers/leds/leds-tlc591xx.c
> > @@ -0,0 +1,309 @@
> > +/*
> > + * 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 TLC59116_LEDS		16
> > +#define TLC59108_LEDS		8
> 
> These two are quite confusing. In some places they are used as "number
> of leds on TLC5910x device", and in some places they are used as "max
> leds on any TLC device".
> 
> When defining the static values for struct tlc59116 and tlc59108 I think
> it would be much cleaner to just use the integer there, instead of one
> of the defines above. And if you needs a "max leds on any TLC device",
> define it clearly, like TLC591XX_MAX_LEDS or such.

Yes, i can change this, hard code tio 8/16 in the structure, and use
TLC591XX_MAX_LEDS.
 
> > +
> > +#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;
> > +	struct regmap *regmap;
> > +	unsigned int led_no;
> > +	struct led_classdev ldev;
> > +	struct work_struct work;
> > +	const struct tlc591xx *tlc591xx;
> > +};
> 
> The struct above contains fields that are common to all led instances.
> Maybe a matter of taste, but I'd rather have one struct representing the
> whole device (struct tlc591xx_priv I guess), which would contains
> 'regmap' and 'tlc591xx' fields. And tlc591xx_led would contain a pointer
> to the tlc591xx_priv.
> 
> Not a big difference, but having regmap in the struct above makes me
> think that each leds has its own regmap.

Adding the 08 support made this grow with more shared properties.  It
probably is now time to have a common part and a per LED part.
 
> > +static int
> > +tlc591xx_set_mode(struct regmap *regmap, u8 mode)
> > +{
> > +	int err;
> > +	u8 val;
> > +
> > +	if ((mode != MODE2_DIM) && (mode != MODE2_BLINK))
> > +		mode = MODE2_DIM;
> 
> If mode is not DIM or BLINK, should this function return an error?

Actually, i would remove this check altogether. This cannot be
influenced outside the driver, so if it is not _DIM or BLANK, it is an
internal driver bug.
 
> > +static void
> > +tlc591xx_led_work(struct work_struct *work)
> > +{
> 
> Maybe it's normal for LED drivers, but why is the workqueue needed? Why
> not just do the work synchronously?

include/linux/leds.h says:

        /* Must not sleep, use a workqueue if needed */
        void            (*brightness_set)(struct led_classdev *led_cdev,
                                          enum led_brightness brightness);

and regmap uses a lock to protect its structures, and so does i2c, etc.

> 
> > +	struct tlc591xx_led *led = work_to_led(work);
> > +	int err;
> > +
> > +	switch (led->ldev.brightness) {
> 
> Can the brightness here be < 0 or > LED_FULL?

Only if the core is broken. From include/linux/leds.h

enum led_brightness {
        LED_OFF         = 0,
        LED_HALF        = 127,
        LED_FULL        = 255,
};

> > +	case 0:
> > +		err = tlc591xx_set_ledout(led, LEDOUT_OFF);
> > +		break;
> > +	case LED_FULL:
> > +		err = tlc591xx_set_ledout(led, LEDOUT_ON);
> > +		break;
> > +	default:
> > +		err = tlc591xx_set_ledout(led, LEDOUT_DIM);
> > +		if (!err)
> > +			err = tlc591xx_set_pwm(led, led->ldev.brightness);
> > +	}
> 
> There doesn't seem to be any locking used for the fields this function
> accesses. Perhaps none is needed, but an async work without locks and
> without comments about it makes me feel a bit nervous.

I suppose led->ldev.brightness could change value between the switch
and the call to tlc591xx_set_pwm. I can take a local copy and use
that.

> > +static int
> > +tlc591xx_configure(struct device *dev,
> > +		   struct tlc591xx_priv *priv,
> > +		   struct regmap *regmap,
> > +		   const struct tlc591xx *tlc591xx)
> > +{
> > +	unsigned int i;
> > +	int err = 0;
> > +
> > +	tlc591xx_set_mode(regmap, MODE2_DIM);
> > +	for (i = 0; i < TLC59116_LEDS; i++) {
> 
> Looping for tlc591xx->maxleds should be enough?

Yes, it would, but i'm not sure that is any better. At the moment it
is a constant, so the compiler can optimise it. We might save 8
iterations for TLC59108, but how much do we add by having less well
optimized code? And this is during probe, not some hot path, so do we
really care?
 
> > +
> > +	tlc591xx = of_match_device(of_tlc591xx_leds_match, dev)->data;
> 
> I presume of_match_device() can return NULL or an error, making the
> above crash.

It would be very odd. The fact the probe function is being called
means there is a match. So return values like -ENODEV would mean a
core OF bug. There is no memory allocations needed, so -ENOMEM would
also not be expected.
 
> > +
> > +	count = of_get_child_count(np);
> 
> 'np' may be NULL. I'm not sure how of_get_child_count() likes that.
 
How can it be NULL? If the probe has been called, it means the
compatibility string must match. If it matches, there must be a np!

Anyway, of_get_child_count() looks to be happy with NULL and will
return 0.

       Andrew

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

[Tomi Valkeinen]

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

On 26/01/15 19:41, Andrew Lunn wrote:

>> Maybe it's normal for LED drivers, but why is the workqueue needed? Why
>> not just do the work synchronously?
> 
> include/linux/leds.h says:
> 
>         /* Must not sleep, use a workqueue if needed */
>         void            (*brightness_set)(struct led_classdev *led_cdev,
>                                           enum led_brightness brightness);
> 
> and regmap uses a lock to protect its structures, and so does i2c, etc.

Ah ok.

>>> +static int
>>> +tlc591xx_configure(struct device *dev,
>>> +		   struct tlc591xx_priv *priv,
>>> +		   struct regmap *regmap,
>>> +		   const struct tlc591xx *tlc591xx)
>>> +{
>>> +	unsigned int i;
>>> +	int err = 0;
>>> +
>>> +	tlc591xx_set_mode(regmap, MODE2_DIM);
>>> +	for (i = 0; i < TLC59116_LEDS; i++) {
>>
>> Looping for tlc591xx->maxleds should be enough?
> 
> Yes, it would, but i'm not sure that is any better. At the moment it
> is a constant, so the compiler can optimise it. We might save 8
> iterations for TLC59108, but how much do we add by having less well
> optimized code? And this is during probe, not some hot path, so do we
> really care?

True. And if the define is renamed to TLC591XX_MAX_LEDS or such, then
it's clear.

>>> +
>>> +	tlc591xx = of_match_device(of_tlc591xx_leds_match, dev)->data;
>>
>> I presume of_match_device() can return NULL or an error, making the
>> above crash.
> 
> It would be very odd. The fact the probe function is being called
> means there is a match. So return values like -ENODEV would mean a
> core OF bug. There is no memory allocations needed, so -ENOMEM would
> also not be expected.

The match could come from non-DT based matching. You don't support that
in the driver, but it would be good to bail out early if that is the case.

>>> +
>>> +	count = of_get_child_count(np);
>>
>> 'np' may be NULL. I'm not sure how of_get_child_count() likes that.
>  
> How can it be NULL? If the probe has been called, it means the
> compatibility string must match. If it matches, there must be a np!

Again with non-DT match, although if that's the case the driver should
have already returned an error at this point.

> Anyway, of_get_child_count() looks to be happy with NULL and will
> return 0.

Yep, then it's not an issue.

 Tomi



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

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

[Tomi Valkeinen]

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

Hi,

On 22/01/15 00:29, 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.

So as I've mentioned, our HW uses TLC59108 not only for LED (backlight)
but also as a GPO. And as we shortly discussed (in private mails, I
think), technically it's possible to write gpio and backligth drivers
that utilize a LED driver to do the actual work.

Some years ago there was a patch from Peter to implement GPO on top of PWM:

https://lkml.org/lkml/2012/11/22/108

That wasn't merged, but there was also no outright NAK, although it
seemed different people preferred different approach. That patch is
quite similar to what we'd need for GPO on top of LED.

However, I'm still not convinced that is the correct solution. A GPO
support for a PWM output makes sense, as it's just a PWM with full
on/off output. But a LED means a, well, LED, something that emits light.
GPO on LED sounds a bit silly.

So... To me it's still slightly unclear when should one write a PWM
driver and when a LED driver. But I would say that as the TLC591xx
outputs a PWM signal, it should be a PWM driver. Then the different
users of this PWM signal could be made on top of that (LED, backlight, GPO).

What would be the technical drawbacks with having the TLC591xx driver as
a PWM, instead of LED?

 Tomi



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

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

[Andrew Lunn]

> So... To me it's still slightly unclear when should one write a PWM
> driver and when a LED driver. But I would say that as the TLC591xx
> outputs a PWM signal, it should be a PWM driver. Then the different
> users of this PWM signal could be made on top of that (LED, backlight, GPO).
> 
> What would be the technical drawbacks with having the TLC591xx driver as
> a PWM, instead of LED?
 
Hi Tomi

We have been through this once, but the big technical drawback is that
this hardware cannot do what the Linux Kernel defines as PWM.

It cannot correctly implement the PMW call:

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

This hardware has a fixed period, since it is clocked at 97-kHz.  So
you cannot set the period. The duty is also somewhat restrictive, in
that it only allows 1/256 increments of the 97Khz.

This hardware does however perfectly fit the LED API:

enum led_brightness {
        LED_OFF         = 0,
        LED_HALF        = 127,
        LED_FULL        = 255,
};

        void            (*brightness_set)(struct led_classdev *led_cdev,
                                          enum led_brightness brightness);

So we can model it perfectly as an LED driver, or badly as a PWM
driver.

   Andrew

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

[Tomi Valkeinen]

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

Hi,

On 26/01/15 19:10, Andrew Lunn wrote:
>> So... To me it's still slightly unclear when should one write a PWM
>> driver and when a LED driver. But I would say that as the TLC591xx
>> outputs a PWM signal, it should be a PWM driver. Then the different
>> users of this PWM signal could be made on top of that (LED, backlight, GPO).
>>
>> What would be the technical drawbacks with having the TLC591xx driver as
>> a PWM, instead of LED?
>  
> Hi Tomi
> 
> We have been through this once, but the big technical drawback is that
> this hardware cannot do what the Linux Kernel defines as PWM.
> 
> It cannot correctly implement the PMW call:
> 
> int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
> 
> This hardware has a fixed period, since it is clocked at 97-kHz.  So
> you cannot set the period. The duty is also somewhat restrictive, in
> that it only allows 1/256 increments of the 97Khz.

Surely other PWM devices also have restrictions in the period or duty cycle?

> This hardware does however perfectly fit the LED API:
> 
> enum led_brightness {
>         LED_OFF         = 0,
>         LED_HALF        = 127,
>         LED_FULL        = 255,
> };
> 
>         void            (*brightness_set)(struct led_classdev *led_cdev,
>                                           enum led_brightness brightness);
> 
> So we can model it perfectly as an LED driver, or badly as a PWM
> driver.

Maybe so, but what does it mean in practice? If it's implemented as a
PWM driver, and the existing leds-pwm driver is used for the led
functionality, shall we miss some brightness values? Is the
configuration more difficult? Or what?

So my point here is that it outputs a PWM signal, so it makes sense to
have it as a PWM driver. It has restricted configurability compared to
more versatile PWM hardware, but I so far don't see why that would be an
issue.

If it is a PWM driver, we get backlight support for free via the
existing pwm_bl driver, and LED support via leds-pwm. And there has been
a clear acceptance on GPO functionality with PWM outputs (in the Peter's
mail thread), whereas I would bet that a LED based GPO functionality
would encounter resistance, because that doesn't quite make sense.

 Tomi



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

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

[Peter Ujfalusi]

On 01/27/2015 01:11 PM, Tomi Valkeinen wrote:
> Hi,
> 
> On 26/01/15 19:10, Andrew Lunn wrote:
>>> So... To me it's still slightly unclear when should one write a PWM
>>> driver and when a LED driver. But I would say that as the TLC591xx
>>> outputs a PWM signal, it should be a PWM driver. Then the different
>>> users of this PWM signal could be made on top of that (LED, backlight, GPO).
>>>
>>> What would be the technical drawbacks with having the TLC591xx driver as
>>> a PWM, instead of LED?
>>  
>> Hi Tomi
>>
>> We have been through this once, but the big technical drawback is that
>> this hardware cannot do what the Linux Kernel defines as PWM.
>>
>> It cannot correctly implement the PMW call:
>>
>> int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
>>
>> This hardware has a fixed period, since it is clocked at 97-kHz.  So
>> you cannot set the period. The duty is also somewhat restrictive, in
>> that it only allows 1/256 increments of the 97Khz.

The duty_ns and period_ns will give you the on and off time your user is
asking for. If you can not change frequency, then you don't (97KHz that is).
1/256 is the resolution you can configure the on/off times, nothing
exceptional about it.
The twl4030/5030's LED drivers have 128 clock cycle to play with while twl6030
has 256 clock cycles. These PMICs also have PWM drivers which has 128 steps.
drivers/pwm/pwm-twl.c and pwm-twl-led.c

and these just work fine when used via pwm-led or the pwm backlight or
whatever driver.

There's no issue to have PWM driver for tlc591xx IMHO.

> Surely other PWM devices also have restrictions in the period or duty cycle?
> 
>> This hardware does however perfectly fit the LED API:
>>
>> enum led_brightness {
>>         LED_OFF         = 0,
>>         LED_HALF        = 127,
>>         LED_FULL        = 255,
>> };
>>
>>         void            (*brightness_set)(struct led_classdev *led_cdev,
>>                                           enum led_brightness brightness);
>>
>> So we can model it perfectly as an LED driver, or badly as a PWM
>> driver.
> 
> Maybe so, but what does it mean in practice? If it's implemented as a
> PWM driver, and the existing leds-pwm driver is used for the led
> functionality, shall we miss some brightness values? Is the
> configuration more difficult? Or what?
> 
> So my point here is that it outputs a PWM signal, so it makes sense to
> have it as a PWM driver. It has restricted configurability compared to
> more versatile PWM hardware, but I so far don't see why that would be an
> issue.
> 
> If it is a PWM driver, we get backlight support for free via the
> existing pwm_bl driver, and LED support via leds-pwm. And there has been
> a clear acceptance on GPO functionality with PWM outputs (in the Peter's
> mail thread), whereas I would bet that a LED based GPO functionality
> would encounter resistance, because that doesn't quite make sense.
> 
>  Tomi
> 
> 


-- 
Péter