[RFC PATCH] pwm: TLC591xx PWM driver

[RFC PATCH] pwm: TLC591xx PWM driver

[Tomi Valkeinen]

Hi,

Here's an RFC for a driver for TI's TLC59108/TLC59116 chips. This is an
alternate implementation to the LED driver [1].

We (TI) use the chip for LCD backlight and also as a GPIO expander. Andrew
(author of the LED driver) uses it for plain LEDs.

This implementation works for backlight and plain LED use cases. The LED
version does not (afaik) work for backlight out of the box. Neither supports
GPIO API.

I'd like to hear feedback on whether this kind of HW should have a PWM or a LED
driver.

The LED driver offers APIs for things like blinking, which the PWM API doesn't.
On the other hand, there's pwm_bl.c which give us backlight device with PWM,
and a GPIO over PWM sounds more sane to me than GPIO over LED. But I really
don't have any hard arguments why one solution would be better than the other.

Of course, one option is to have both PWM and LED drivers in the kernel. This
is the reason I set the driver's compatible string to "tlc59108-pwm" so that it
doesn't clash with the LED driver.

Or, possibly a mfd is an option.

 Tomi

[1] https://www.marc.info/?l=devicetree&m=142663032707961&w=1

Tomi Valkeinen (1):
  pwm: add TLC59108/TLC59116 PWM driver

 .../devicetree/bindings/pwm/ti,tlc59108-pwm.txt    |  16 ++
 drivers/pwm/Kconfig                                |  10 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-tlc591xx.c                         | 273 +++++++++++++++++++++
 4 files changed, 300 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt
 create mode 100644 drivers/pwm/pwm-tlc591xx.c

-- 
2.1.4

[RFC PATCH] pwm: add TLC59108/TLC59116 PWM driver

[Tomi Valkeinen]

The TLC59108/TLC59116 is an I2C bus controlled 8-bit LED driver. Each
LED output has its own 8-bit resolution (256 steps) fixed-frequency
individual PWM controller that operates at 97 kHz, with a duty cycle
that is adjustable from 0% to 100%. The individual PWM controller allows
each LED to be set to a specific brightness value.

TLC59108 has 8 outputs, TLC59116 has 16 outputs.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 .../devicetree/bindings/pwm/ti,tlc59108-pwm.txt    |  16 ++
 drivers/pwm/Kconfig                                |  10 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-tlc591xx.c                         | 273 +++++++++++++++++++++
 4 files changed, 300 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt
 create mode 100644 drivers/pwm/pwm-tlc591xx.c

diff --git a/Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt b/Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt
new file mode 100644
index 000000000000..5b9580b6ac94
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt
@@ -0,0 +1,16 @@
+TI TLC59108/TLC59116 8/16-channel 8-bit PWM LED Sink Driver
+===========================================================
+
+Required properties:
+  - compatible: "ti,tlc59108-pwm" or "ti,tlc59116-pwm"
+  - #pwm-cells: should be 2. See pwm.txt in this directory for a description of
+    the cells format.
+  - reg: physical base address and size of the registers map.
+
+Example:
+
+tlc59108: tlc59108@40 {
+	compatible = "ti,tlc59108-pwm";
+	reg = <0x40>;
+	#pwm-cells = <2>;
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f40fd8d..0a9c8fcf993e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -373,4 +373,14 @@ config PWM_VT8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-vt8500.
 
+config PWM_TLC591XX
+	tristate "TLC59108/TLC59116 PWM driver"
+	depends on OF && I2C
+	select REGMAP_I2C
+	help
+	  Generic PWM framework driver for TI TLC59108/TLC59116 PWM controller.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-tlc591xx.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ec50eb5b5a8f..75896890d6ec 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -35,3 +35,4 @@ obj-$(CONFIG_PWM_TIPWMSS)	+= pwm-tipwmss.o
 obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
+obj-$(CONFIG_PWM_TLC591XX)	+= pwm-tlc591xx.o
diff --git a/drivers/pwm/pwm-tlc591xx.c b/drivers/pwm/pwm-tlc591xx.c
new file mode 100644
index 000000000000..cd49faa75eff
--- /dev/null
+++ b/drivers/pwm/pwm-tlc591xx.c
@@ -0,0 +1,273 @@
+/*
+ * Driver for TLC59108/TLC59116 I2C based PWM/LED chip
+ *
+ * Copyright 2014 Belkin Inc.
+ * Copyright 2015 Andrew Lunn
+ * Copyright 2015 Texas Instruments
+ *
+ * Authors:
+ * Andrew Lunn <andrew@lunn.ch>
+ * Vignesh R <vigneshr@ti.com>
+ * Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define TLC591XX_MAX_LEDS	16
+
+#define TLC591XX_MODE1		0x00
+#define TLC591XX_PWM(n)		(0x02 + (n))
+#define TLC591XX_MAXREG		0x1e
+
+#define LEDOUT_OFF		0x0
+#define LEDOUT_ON		0x1
+#define LEDOUT_DIM		0x2
+#define LEDOUT_MASK		0x3
+
+#define TLC591XX_CLK_PERIOD	10309 /* period in ns */
+#define TLC591XX_NO_STEPS	256
+#define TLC591XX_LEDS_PER_REG	4
+
+struct tlc591xx_led {
+	unsigned int led_no;
+	struct tlc591xx_priv *priv;
+};
+
+struct tlc591xx_priv {
+	struct pwm_chip chip;
+	struct regmap *regmap;
+	struct tlc591xx_led leds[TLC591XX_MAX_LEDS];
+	unsigned int reg_ledout_offset;
+};
+
+static struct regmap_config tlc591xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = TLC591XX_MAXREG,
+};
+
+struct tlc591xx_hw {
+	unsigned int max_leds;
+	unsigned int reg_ledout_offset;
+};
+
+static const struct tlc591xx_hw tlc59116 = {
+	.max_leds = 16,
+	.reg_ledout_offset = 0x14,
+};
+
+static const struct tlc591xx_hw tlc59108 = {
+	.max_leds = 8,
+	.reg_ledout_offset = 0x0c,
+};
+
+static inline struct tlc591xx_priv *to_tlc591xx_priv(struct pwm_chip *c)
+{
+	return container_of(c, struct tlc591xx_priv, chip);
+}
+
+static int tlc591xx_write_ledout(struct pwm_chip *chip, struct pwm_device *pwm,
+	unsigned val)
+{
+	struct tlc591xx_priv *priv = to_tlc591xx_priv(chip);
+	int led_no, led_grp;
+	unsigned int reg, mask;
+	unsigned shift;
+
+	led_no = (pwm->hwpwm) % TLC591XX_LEDS_PER_REG;
+	led_grp = (pwm->hwpwm) / TLC591XX_LEDS_PER_REG;
+
+	reg = priv->reg_ledout_offset + led_grp;
+	shift = led_no * 2;
+	mask = LEDOUT_MASK << shift;
+	val <<= shift;
+
+	return regmap_update_bits(priv->regmap, reg, mask, val);
+}
+
+static int tlc591xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			       int duty_ns, int period_ns)
+{
+	struct tlc591xx_priv *priv = to_tlc591xx_priv(chip);
+	unsigned val;
+	int r;
+
+	if (period_ns != TLC591XX_CLK_PERIOD) {
+		dev_err(chip->dev, "only period of 10309 ns is supported\n");
+		return -EINVAL;
+	}
+
+	val = DIV_ROUND_CLOSEST(duty_ns * (TLC591XX_NO_STEPS - 1), period_ns);
+
+	r = regmap_write(priv->regmap, TLC591XX_PWM(pwm->hwpwm), val);
+	if (r)
+		return r;
+
+	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+		if (duty_ns == period_ns)
+			r = tlc591xx_write_ledout(chip, pwm, LEDOUT_ON);
+		else if (duty_ns == 0)
+			r = tlc591xx_write_ledout(chip, pwm, LEDOUT_OFF);
+		else
+			r = tlc591xx_write_ledout(chip, pwm, LEDOUT_DIM);
+
+		if (r)
+			return r;
+	}
+
+	return 0;
+}
+
+static int tlc591xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	if (pwm->duty_cycle == pwm->period)
+		return tlc591xx_write_ledout(chip, pwm, LEDOUT_ON);
+	else
+		return tlc591xx_write_ledout(chip, pwm, LEDOUT_DIM);
+}
+
+static void tlc591xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	tlc591xx_write_ledout(chip, pwm, LEDOUT_OFF);
+}
+
+static int tlc591xx_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+	enum pwm_polarity polarity)
+{
+	return polarity != PWM_POLARITY_INVERSED ? -EINVAL : 0;
+}
+
+static const struct pwm_ops tlc591xx_pwm_ops = {
+	.config = tlc591xx_pwm_config,
+	.enable = tlc591xx_pwm_enable,
+	.disable = tlc591xx_pwm_disable,
+	.set_polarity = tlc591xx_set_polarity,
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id tlc591xx_pwm_dt_ids[] = {
+	{ .compatible = "ti,tlc59116-pwm", .data = &tlc59116 },
+	{ .compatible = "ti,tlc59108-pwm", .data = &tlc59108 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, tlc591xx_pwm_dt_ids);
+
+static int tlc591xx_pwm_probe(struct i2c_client *client,
+			      const struct i2c_device_id *id)
+{
+	struct device_node *np = client->dev.of_node;
+	struct device *dev = &client->dev;
+	struct tlc591xx_priv *priv;
+	const struct of_device_id *match;
+	const struct tlc591xx_hw *tlc591xx_hw;
+	struct regmap *regmap;
+	int err;
+	int i;
+
+	if (!np)
+		return -ENODEV;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA))
+		return -EIO;
+
+	match = of_match_device(tlc591xx_pwm_dt_ids, dev);
+	if (!match)
+		return -ENODEV;
+
+	tlc591xx_hw = match->data;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, priv);
+
+	regmap = devm_regmap_init_i2c(client, &tlc591xx_regmap_config);
+	if (IS_ERR(regmap)) {
+		err = PTR_ERR(regmap);
+		dev_err(dev, "Failed to allocate register map: %d\n", err);
+		return err;
+	}
+
+	priv->reg_ledout_offset = tlc591xx_hw->reg_ledout_offset;
+	priv->regmap = regmap;
+
+	priv->chip.dev = dev;
+	priv->chip.ops = &tlc591xx_pwm_ops;
+	priv->chip.base = -1;
+	priv->chip.npwm = tlc591xx_hw->max_leds;
+	priv->chip.can_sleep = true;
+
+	err = pwmchip_add(&priv->chip);
+	if (err < 0) {
+		dev_err(dev, "failed to add PWM chip %d\n", err);
+		return err;
+	}
+
+	for (i = 0; i < priv->chip.npwm; ++i) {
+		priv->chip.pwms[i].period = 10309;
+		priv->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
+	}
+
+	/* enable oscillator */
+	regmap_update_bits(priv->regmap, TLC591XX_MODE1, 1 << 4, 0 << 4);
+
+	/*
+	 * Delay of 500 micro second is required before accessing
+	 * the PWMx or LEDOUTx registers.
+	 */
+	usleep_range(500, 1000);
+
+	return 0;
+}
+
+static int tlc591xx_pwm_remove(struct i2c_client *client)
+{
+	struct tlc591xx_priv *priv = i2c_get_clientdata(client);
+	int err;
+
+	/* disable oscillator */
+	regmap_update_bits(priv->regmap, TLC591XX_MODE1, 1 << 4, 1 << 4);
+
+	err =  pwmchip_remove(&priv->chip);
+	if (err < 0)
+		dev_err(&client->dev,
+			"pwmchip_remove failed with error %d\n", err);
+
+	return err;
+}
+
+static const struct i2c_device_id tlc591xx_pwm_id[] = {
+	{ "tlc59108-pwm", },
+	{ "tlc59116-pwm", },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tlc591xx_pwm_id);
+
+static struct i2c_driver tlc591xx_pwm_driver = {
+	.driver = {
+		.name = "tlc591xx-pwm",
+		.of_match_table = tlc591xx_pwm_dt_ids,
+	},
+	.probe = tlc591xx_pwm_probe,
+	.remove = tlc591xx_pwm_remove,
+	.id_table = tlc591xx_pwm_id,
+};
+
+module_i2c_driver(tlc591xx_pwm_driver);
+
+MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>");
+MODULE_DESCRIPTION("PWM driver for TLC59108/TLC59116");
+MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [RFC PATCH] pwm: add TLC59108/TLC59116 PWM driver

[Andrew Lunn]

Hi Tomi

I know you want more high level comments, not a code review, but
anyway...

On Tue, Aug 18, 2015 at 04:52:07PM +0300, Tomi Valkeinen wrote:
> The TLC59108/TLC59116 is an I2C bus controlled 8-bit LED driver. Each
> LED output has its own 8-bit resolution (256 steps) fixed-frequency
> individual PWM controller that operates at 97 kHz, with a duty cycle
> that is adjustable from 0% to 100%. The individual PWM controller allows
> each LED to be set to a specific brightness value.
> 
> TLC59108 has 8 outputs, TLC59116 has 16 outputs.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  .../devicetree/bindings/pwm/ti,tlc59108-pwm.txt    |  16 ++
>  drivers/pwm/Kconfig                                |  10 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-tlc591xx.c                         | 273 +++++++++++++++++++++
>  4 files changed, 300 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt
>  create mode 100644 drivers/pwm/pwm-tlc591xx.c
> 
> diff --git a/Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt b/Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt
> new file mode 100644
> index 000000000000..5b9580b6ac94
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/ti,tlc59108-pwm.txt
> @@ -0,0 +1,16 @@
> +TI TLC59108/TLC59116 8/16-channel 8-bit PWM LED Sink Driver
> +===========================================================
> +
> +Required properties:
> +  - compatible: "ti,tlc59108-pwm" or "ti,tlc59116-pwm"
> +  - #pwm-cells: should be 2. See pwm.txt in this directory for a description of
> +    the cells format.
> +  - reg: physical base address and size of the registers map.

Cut and paste error. Reg is the i2c address.

> +
> +Example:
> +
> +tlc59108: tlc59108@40 {
> +	compatible = "ti,tlc59108-pwm";
> +	reg = <0x40>;
> +	#pwm-cells = <2>;
> +};
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f40fd8d..0a9c8fcf993e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -373,4 +373,14 @@ config PWM_VT8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-vt8500.
>  
> +config PWM_TLC591XX
> +	tristate "TLC59108/TLC59116 PWM driver"
> +	depends on OF && I2C
> +	select REGMAP_I2C
> +	help
> +	  Generic PWM framework driver for TI TLC59108/TLC59116 PWM controller.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-tlc591xx.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5b5a8f..75896890d6ec 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -35,3 +35,4 @@ obj-$(CONFIG_PWM_TIPWMSS)	+= pwm-tipwmss.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_TLC591XX)	+= pwm-tlc591xx.o
> diff --git a/drivers/pwm/pwm-tlc591xx.c b/drivers/pwm/pwm-tlc591xx.c
> new file mode 100644
> index 000000000000..cd49faa75eff
> --- /dev/null
> +++ b/drivers/pwm/pwm-tlc591xx.c
> @@ -0,0 +1,273 @@
> +/*
> + * Driver for TLC59108/TLC59116 I2C based PWM/LED chip
> + *
> + * Copyright 2014 Belkin Inc.
> + * Copyright 2015 Andrew Lunn
> + * Copyright 2015 Texas Instruments
> + *
> + * Authors:
> + * Andrew Lunn <andrew@lunn.ch>
> + * Vignesh R <vigneshr@ti.com>
> + * Tomi Valkeinen <tomi.valkeinen@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define TLC591XX_MAX_LEDS	16
> +
> +#define TLC591XX_MODE1		0x00
> +#define TLC591XX_PWM(n)		(0x02 + (n))
> +#define TLC591XX_MAXREG		0x1e
> +
> +#define LEDOUT_OFF		0x0
> +#define LEDOUT_ON		0x1
> +#define LEDOUT_DIM		0x2
> +#define LEDOUT_MASK		0x3
> +
> +#define TLC591XX_CLK_PERIOD	10309 /* period in ns */
> +#define TLC591XX_NO_STEPS	256
> +#define TLC591XX_LEDS_PER_REG	4
> +
> +struct tlc591xx_led {
> +	unsigned int led_no;
> +	struct tlc591xx_priv *priv;
> +};
> +
> +struct tlc591xx_priv {
> +	struct pwm_chip chip;
> +	struct regmap *regmap;
> +	struct tlc591xx_led leds[TLC591XX_MAX_LEDS];
> +	unsigned int reg_ledout_offset;
> +};
> +
> +static struct regmap_config tlc591xx_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = TLC591XX_MAXREG,
> +};
> +
> +struct tlc591xx_hw {
> +	unsigned int max_leds;
> +	unsigned int reg_ledout_offset;
> +};
> +
> +static const struct tlc591xx_hw tlc59116 = {
> +	.max_leds = 16,
> +	.reg_ledout_offset = 0x14,
> +};
> +
> +static const struct tlc591xx_hw tlc59108 = {
> +	.max_leds = 8,
> +	.reg_ledout_offset = 0x0c,
> +};
> +
> +static inline struct tlc591xx_priv *to_tlc591xx_priv(struct pwm_chip *c)
> +{
> +	return container_of(c, struct tlc591xx_priv, chip);
> +}
> +
> +static int tlc591xx_write_ledout(struct pwm_chip *chip, struct pwm_device *pwm,
> +	unsigned val)
> +{
> +	struct tlc591xx_priv *priv = to_tlc591xx_priv(chip);
> +	int led_no, led_grp;
> +	unsigned int reg, mask;
> +	unsigned shift;
> +
> +	led_no = (pwm->hwpwm) % TLC591XX_LEDS_PER_REG;
> +	led_grp = (pwm->hwpwm) / TLC591XX_LEDS_PER_REG;
> +
> +	reg = priv->reg_ledout_offset + led_grp;
> +	shift = led_no * 2;
> +	mask = LEDOUT_MASK << shift;
> +	val <<= shift;
> +
> +	return regmap_update_bits(priv->regmap, reg, mask, val);
> +}
> +
> +static int tlc591xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       int duty_ns, int period_ns)
> +{
> +	struct tlc591xx_priv *priv = to_tlc591xx_priv(chip);
> +	unsigned val;
> +	int r;
> +
> +	if (period_ns != TLC591XX_CLK_PERIOD) {
> +		dev_err(chip->dev, "only period of 10309 ns is supported\n");
> +		return -EINVAL;
> +	}

The hardware does allow you to change the period, but there is only
one clock generator for all the channels. If you are going to make the
effort to try to model the hardware as a PWM, you should try to
actually implement the PWM API. Let the first channel which calls
config set the period, and return an error for any other channel which
picks a different period.

      Andrew

Re: [RFC PATCH] pwm: add TLC59108/TLC59116 PWM driver

[Tomi Valkeinen]

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



On 18/08/15 17:19, Andrew Lunn wrote:

>> +Required properties:
>> +  - compatible: "ti,tlc59108-pwm" or "ti,tlc59116-pwm"
>> +  - #pwm-cells: should be 2. See pwm.txt in this directory for a description of
>> +    the cells format.
>> +  - reg: physical base address and size of the registers map.
> 
> Cut and paste error. Reg is the i2c address.

Oh, right. Thanks.

>> +static int tlc591xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			       int duty_ns, int period_ns)
>> +{
>> +	struct tlc591xx_priv *priv = to_tlc591xx_priv(chip);
>> +	unsigned val;
>> +	int r;
>> +
>> +	if (period_ns != TLC591XX_CLK_PERIOD) {
>> +		dev_err(chip->dev, "only period of 10309 ns is supported\n");
>> +		return -EINVAL;
>> +	}
> 
> The hardware does allow you to change the period, but there is only
> one clock generator for all the channels. If you are going to make the

Hmm... How does that work?

The per-led output is fixed 97-kHz. Then there's the group dimming
signal, which can be superimposed on all the individual outputs, with
fixed 190-Hz. Neither frequency can be tuned. And when using group
dimming, the output isn't exactly PWM anymore, but two PWM signals combined.

Is there some other setting I'm missing?

These group dimming features don't quite fit into PWM model, I guess.
Then again, I don't think LED framework offers any ways to utilize them
either.

> effort to try to model the hardware as a PWM, you should try to
> actually implement the PWM API. Let the first channel which calls
> config set the period, and return an error for any other channel which
> picks a different period.

If there is a way to set the period, I think it may be better to define
it in the TLC's DT data than by "whoever gets there first".

 Tomi


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

Re: [RFC PATCH] pwm: add TLC59108/TLC59116 PWM driver

[Andrew Lunn]

> > The hardware does allow you to change the period, but there is only
> > one clock generator for all the channels. If you are going to make the
> 
> Hmm... How does that work?

I does not :-(

It is something like 8 months since i rewrote the LED class driver,
and i've forgotten how the thing actually works.

Sorry
	Andrew

Re: [RFC PATCH] pwm: TLC591xx PWM driver

[Andrew Lunn]

> On the other hand, there's pwm_bl.c which give us backlight device
> with PWM,

Lets look at this. A backlight device seems to do most of its work in
the update_status callback. It is given a brightness in
bl->props.brightness, which takes a value between 0 and
props.max_brightness.

What pwm_bl.c does it then turn this brightness value into an
artificial PWM configuration. Your proposed PWM driver then turns this
back into a brightness, since you don't actually implement the period
part of the PWM interface.

>From an architecture point of view, doesn't an LED class device, which
takes a brightness value, seem much more naturally?

It seems like implementing a generic led_bl.c driver would make sense.
It would also allow some of the code in drivers/video/backlight/ to be
eliminated. There seems to be both an LED class driver for lp55xx and
a blacklight driver lp855x_bl.c. There are duplicate lp8788, 88pm860x,
adp5520, da903x, da9052, hp6xx, and lm3533 drivers which might all be
removed if a led_bl.c generic driver existed.

> and a GPIO over PWM sounds more sane to me than GPIO over LED.

Currently two LED class drivers are calling gpiochip_add:

~/linux/drivers/leds$ grep gpiochip_add *.c
leds-pca9532.c:	      	   err = gpiochip_add(&data->gpio);
leds-tca6507.c:		   err = gpiochip_add(&tca->gpio);

The pca9532 has full GPIO capabilities, in as well as out. But it
seems like tca6507 is output only. The TLC59108/TLC59116 is also
output only. So a generic GPO driver on top of LED would make sense
for these two, and save some code/bugs.

>From a stand back, lets take a look at the architecture point of view,
generic led_bl and gpio-led drivers seem to make sense.

       Andrew

Re: [RFC PATCH] pwm: TLC591xx PWM driver

[Thierry Reding]

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

On Tue, Aug 18, 2015 at 05:09:16PM +0200, Andrew Lunn wrote:
> > On the other hand, there's pwm_bl.c which give us backlight device
> > with PWM,
> 
> Lets look at this. A backlight device seems to do most of its work in
> the update_status callback. It is given a brightness in
> bl->props.brightness, which takes a value between 0 and
> props.max_brightness.
> 
> What pwm_bl.c does it then turn this brightness value into an
> artificial PWM configuration. Your proposed PWM driver then turns this
> back into a brightness, since you don't actually implement the period
> part of the PWM interface.
> 
> From an architecture point of view, doesn't an LED class device, which
> takes a brightness value, seem much more naturally?
> 
> It seems like implementing a generic led_bl.c driver would make sense.
> It would also allow some of the code in drivers/video/backlight/ to be
> eliminated. There seems to be both an LED class driver for lp55xx and
> a blacklight driver lp855x_bl.c. There are duplicate lp8788, 88pm860x,
> adp5520, da903x, da9052, hp6xx, and lm3533 drivers which might all be
> removed if a led_bl.c generic driver existed.
> 
> > and a GPIO over PWM sounds more sane to me than GPIO over LED.
> 
> Currently two LED class drivers are calling gpiochip_add:
> 
> ~/linux/drivers/leds$ grep gpiochip_add *.c
> leds-pca9532.c:	      	   err = gpiochip_add(&data->gpio);
> leds-tca6507.c:		   err = gpiochip_add(&tca->gpio);
> 
> The pca9532 has full GPIO capabilities, in as well as out. But it
> seems like tca6507 is output only. The TLC59108/TLC59116 is also
> output only. So a generic GPO driver on top of LED would make sense
> for these two, and save some code/bugs.
> 
> From a stand back, lets take a look at the architecture point of view,
> generic led_bl and gpio-led drivers seem to make sense.

I agree it's sensible for there to be a driver that can drive a
backlight from an LED. PWM is somewhat of a lower level API than the LED
API, so there are cases where LED fits better than PWM. This particular
case seems to be one of them.

Thierry

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

Re: [RFC PATCH] pwm: TLC591xx PWM driver

[Tomi Valkeinen]

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


On 18/08/15 18:09, Andrew Lunn wrote:
>> On the other hand, there's pwm_bl.c which give us backlight device
>> with PWM,
> 
> Lets look at this. A backlight device seems to do most of its work in
> the update_status callback. It is given a brightness in
> bl->props.brightness, which takes a value between 0 and
> props.max_brightness.
> 
> What pwm_bl.c does it then turn this brightness value into an
> artificial PWM configuration. Your proposed PWM driver then turns this
> back into a brightness, since you don't actually implement the period
> part of the PWM interface.

Hmm, I'm not sure I follow. It's still PWM, even if the period is fixed.
It is programming the PWM of TLC591xx.

> From an architecture point of view, doesn't an LED class device, which
> takes a brightness value, seem much more naturally?

It does the same thing, takes a brightness value, and programs TLC's PWM
for particular PWM duty cycle.

I guess I get your point. You're saying that as TLC has a fixed period,
we can just consider it as a brightness value.

But in the end, it is still PWM in the HW level, and also, the
brightness isn't linear, or even anything like it. At least on my
backlight, the visible values range from ~230 to 255.

So at least in this case I think it makes more sense to consider the
value as PWM duty cycle, which then causes the LED to glow at some
brightness.

If the value would be brightness, I'd presume that more or less the
whole 0-255 range would be usable, and 128 would be somewhere near
mid-brightness.

> It seems like implementing a generic led_bl.c driver would make sense.
> It would also allow some of the code in drivers/video/backlight/ to be
> eliminated. There seems to be both an LED class driver for lp55xx and
> a blacklight driver lp855x_bl.c. There are duplicate lp8788, 88pm860x,
> adp5520, da903x, da9052, hp6xx, and lm3533 drivers which might all be
> removed if a led_bl.c generic driver existed.

Yes, I think this should be looked at. I'll probably have a look at some
point if I find time.

>> and a GPIO over PWM sounds more sane to me than GPIO over LED.
> 
> Currently two LED class drivers are calling gpiochip_add:
> 
> ~/linux/drivers/leds$ grep gpiochip_add *.c
> leds-pca9532.c:	      	   err = gpiochip_add(&data->gpio);
> leds-tca6507.c:		   err = gpiochip_add(&tca->gpio);
> 
> The pca9532 has full GPIO capabilities, in as well as out. But it
> seems like tca6507 is output only. The TLC59108/TLC59116 is also
> output only. So a generic GPO driver on top of LED would make sense
> for these two, and save some code/bugs.
> 
> From a stand back, lets take a look at the architecture point of view,
> generic led_bl and gpio-led drivers seem to make sense.

Yes, led_bl makes sense. I don't think all LEDs are PWM/GPIO driven, so
in the minimum those LEDs might need a LED class driver.

For this particular chip I still think PWM driver would do just fine.
But as the LED framework offers features like blinking, and it's
possible to implement backlight and gpios on top LED (I hope), I do
agree that we should go with your driver.

That said, I feel that these frameworks, GPIO, PWM and LED are somewhat
overlapping. Not really my area of expertise, but I imagine a single
framework containing all those functionalities could be possible.

 Tomi


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

Re: [RFC PATCH] pwm: TLC591xx PWM driver

[Andrew Lunn]

> But in the end, it is still PWM in the HW level, and also, the
> brightness isn't linear, or even anything like it. At least on my
> backlight, the visible values range from ~230 to 255.

I suspect the narrow range for you is a property of your hardware. For
my WRT1900ac, 0-32 the LEDs look pretty much like off. Above that, i
get light. Should the LED class layer care about this? Probably
not. It seems like the generic backlight layer should have some method
to reduce the 0-255 down to a couple of dozen useful working points.

> That said, I feel that these frameworks, GPIO, PWM and LED are somewhat
> overlapping. Not really my area of expertise, but I imagine a single
> framework containing all those functionalities could be possible.

Humm, i don't think the frameworks overlap that much. I think it is
the hardware which overlaps. You have a device with an output which
you normally connect an LED to, but you could connect anything to. You
can control this pin using a very restricted PWM like scheme, or you
can have it plain off/on.

With hardware like this, you need to decide which it is most like, and
put that framework at the bottom. Then stack generic X->Y layers on
top to get the rest of the functionality.

    Andrew