| Message ID | 1421879364-8573-3-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
> 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
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
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
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
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 > >
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", ®); + 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");
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