* [PATCH v2 1/1] leds: LED driver for TI LP3952 6-Channel Color LED @ 2016-06-03 15:20 Tony Makkiel 2016-06-07 8:42 ` Jacek Anaszewski 0 siblings, 1 reply; 5+ messages in thread From: Tony Makkiel @ 2016-06-03 15:20 UTC (permalink / raw) To: linux-leds; +Cc: Tony Makkiel, j.anaszewski, rpurdie, rjw, lenb The chip can drive 2 sets of RGB leds. Controller can be controlled via PWM, I2C and audio synchronisation. This driver uses I2C to communicate with the chip. Datasheet: http://www.ti.com/lit/gpn/lp3952 Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> --- drivers/leds/Kconfig | 12 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-lp3952.c | 368 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/leds-lp3952.h | 83 ++++++++++ 4 files changed, 464 insertions(+) create mode 100644 drivers/leds/leds-lp3952.c create mode 100644 include/linux/leds-lp3952.h diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 5ae2834..305faf9 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -228,6 +228,18 @@ config LEDS_LP3944 To compile this driver as a module, choose M here: the module will be called leds-lp3944. +config LEDS_LP3952 + tristate "LED Support for TI LP3952 2 channel LED driver" + depends on LEDS_CLASS + depends on I2C + select REGMAP_I2C + help + This option enables support for LEDs connected to the Texas + Instruments LP3952 LED driver. + + To compile this driver as a module, choose M here: the + module will be called leds-lp3952. + config LEDS_LP55XX_COMMON tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index cb2013d..0684c86 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o +obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c new file mode 100644 index 0000000..abdf23a --- /dev/null +++ b/drivers/leds/leds-lp3952.c @@ -0,0 +1,368 @@ +/* + * Copyright (c) 2016, DAQRI, LLC. + * + * License Terms: GNU General Public License v2 + * + * leds-lp3952 - LED class driver for TI lp3952 controller + * + * Based on: + * leds-tlc9516 - LED class driver for TLC95XX series + * of I2C driven LED controllers from NXP + * + * Derived from leds-atmel-pwm, leds-bd2802 and initial PWM led 3952 + * driver written by Alex Feinman + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/delay.h> +#include <linux/gpio.h> +#include <linux/i2c.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/leds-lp3952.h> +#include <linux/module.h> +#include <linux/notifier.h> +#include <linux/platform_device.h> +#include <linux/pm.h> +#include <linux/reboot.h> +#include <linux/regmap.h> + +#define LP3952_REG_LED_CTRL 0x00 +#define LP3952_REG_R1_BLNK_TIME_CTRL 0x01 +#define LP3952_REG_R1_BLNK_CYCLE_CTRL 0x02 +#define LP3952_REG_G1_BLNK_TIME_CTRL 0x03 +#define LP3952_REG_G1_BLNK_CYCLE_CTRL 0x04 +#define LP3952_REG_B1_BLNK_TIME_CTRL 0x05 +#define LP3952_REG_B1_BLNK_CYCLE_CTRL 0x06 +#define LP3952_REG_ENABLES 0x0B +#define LP3952_REG_PAT_GEN_CTRL 0x11 +#define LP3952_REG_RGB1_MAX_I_CTRL 0x12 +#define LP3952_REG_RGB2_MAX_I_CTRL 0x13 +#define LP3952_REG_CMD_0 0x50 +#define LP3952_REG_RESET 0x60 + +#define REG_MAX LP3952_REG_RESET + +struct lp3952_ctrl_hdl { + struct led_classdev cdev; + char name[15]; + enum lp3952_leds channel; + void *priv; +}; + +struct ptrn_gen_cmd { + union { + struct { + u16 tt:3; + u16 b:3; + u16 cet:4; + u16 g:3; + u16 r:3; + }; + struct { + u8 lsb; + u8 msb; + } bytes; + }; +} __packed; + +struct lp3952_led_array { + u8 ndev; + u32 enable_gpio; + struct regmap *regmap; + struct i2c_client *client; + struct ptrn_gen_cmd pgm[LP3952_CMD_REG_COUNT]; + struct lp3952_ctrl_hdl *leds; +}; + +int lp3952_register_write(struct i2c_client *client, u8 reg, u8 val) +{ + int ret; + struct lp3952_led_array *priv = i2c_get_clientdata(client); + + ret = regmap_write(priv->regmap, reg, val); + + if (ret) + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", + __func__, reg, val, ret); + return ret; +} + +static void lp3952_on_off(struct lp3952_led_array *priv, enum lp3952_leds led, + int on) +{ + int ret, val; + + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led, on); + + val = 1 << led; + if (led == LP3952_LED_ALL) + val = 0x3f; + + ret = regmap_update_bits(priv->regmap, LP3952_REG_LED_CTRL, val, + on ? val : 0); + if (ret) + dev_err(&priv->client->dev, "%s, Error %d\n", __func__, ret); +} + +/* + * Using Imax to control brightness. There are 4 possible + * setting 25, 50, 75 and 100 % of Imax. Possible values are + * values 0-4. 0 meaning turn off. + */ +static int lp3952_set_brightness(struct led_classdev *cdev, + enum led_brightness value) +{ + unsigned int reg, val, shift_val; + struct lp3952_ctrl_hdl *led = container_of(cdev, + struct lp3952_ctrl_hdl, + cdev); + struct lp3952_led_array *priv = (struct lp3952_led_array *)led->priv; + + dev_dbg(cdev->dev, "Brightness request: %d on %d\n", value, + led->channel); + + if (value == LED_OFF) { + lp3952_on_off(priv, led->channel, LED_OFF); + return 0; + } + + val = value - 1; + reg = LP3952_REG_RGB1_MAX_I_CTRL; + shift_val = (led->channel - LP3952_BLUE_1) * 2; + + if (led->channel > LP3952_RED_1) { + dev_err(cdev->dev, " %s Invalid LED requested", __func__); + return -EINVAL; + } else if (led->channel < LP3952_BLUE_1) { + shift_val = led->channel * 2; + reg = LP3952_REG_RGB2_MAX_I_CTRL; + } + + /* Enable the LED in case it is not enabled already */ + lp3952_on_off(priv, led->channel, 1); + + return (regmap_update_bits(priv->regmap, reg, 3 << shift_val, + val << shift_val)); +} + +static int lp3952_register_led_classdev(struct lp3952_led_array *priv) +{ + int ret, i; + const char *led_name[] = { + "lp3952:blue2", + "lp3952:green2", + "lp3952:red2", + "lp3952:blue1", + "lp3952:green1", + "lp3952:red1" + }; + + for (i = 0; i < priv->ndev; i++) { + sprintf(priv->leds[i].name, "%s", led_name[i]); + priv->leds[i].cdev.name = priv->leds[i].name; + priv->leds[i].cdev.brightness = LED_OFF; + priv->leds[i].cdev.max_brightness = LP3952_BRIGHT_MAX; + priv->leds[i].cdev.brightness_set_blocking = + lp3952_set_brightness; + priv->leds[i].channel = i; + priv->leds[i].priv = priv; + + ret = devm_led_classdev_register(&priv->client->dev, + &priv->leds[i].cdev); + if (ret < 0) { + dev_err(&priv->client->dev, + "couldn't register LED %s\n", + priv->leds[i].cdev.name); + goto error; + } + } + return 0; + +error: + for (; i >= 0; i--) + devm_led_classdev_unregister(&priv->client->dev, + &priv->leds[i].cdev); + return ret; +} + +static void lp3952_unregister_led_classdev(struct lp3952_led_array *priv) +{ + int i; + + for (i = 0; i < priv->ndev; i++) + devm_led_classdev_unregister(&priv->client->dev, + &priv->leds[i].cdev); +} + +static int lp3952_set_pattern_gen_cmd(struct lp3952_led_array *priv, + u8 cmd_index, u8 r, u8 g, u8 b, + enum lp3952_tt tt, enum lp3952_cet cet) +{ + int ret; + struct ptrn_gen_cmd line = { + .r = r, + .g = g, + .b = b, + .cet = cet, + .tt = tt + }; + if (cmd_index >= LP3952_CMD_REG_COUNT) + return -EINVAL; + + priv->pgm[cmd_index] = line; + ret = lp3952_register_write(priv->client, + LP3952_REG_CMD_0 + cmd_index * 2, + line.bytes.msb); + if (ret) + return ret; + + return (lp3952_register_write(priv->client, + LP3952_REG_CMD_0 + cmd_index * 2 + 1, + line.bytes.lsb)); +} + +static int lp3952_configure(struct lp3952_led_array *priv) +{ + int ret; + + /* Disable any LEDs on from any previous conf. */ + ret = lp3952_register_write(priv->client, LP3952_REG_LED_CTRL, 0); + if (ret) + return ret; + + /* enable rgb patter, loop */ + ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, + 0x06); + if (ret) + return ret; + + /* Update Bit 6 (Active mode), Select both Led sets, Bit [1:0] */ + ret = regmap_update_bits(priv->regmap, LP3952_REG_ENABLES, 0x43, 0xfc); + if (ret) + return ret; + + /* Set Cmd1 for RGB intensity,cmd and transition time */ + return (lp3952_set_pattern_gen_cmd(priv, 0, I46, I71, I100, TT0, + CET197)); +} + +static const struct regmap_config lp3952_regmap = { + .reg_bits = 8, + .val_bits = 8, + .max_register = REG_MAX, +}; + +static int lp3952_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int status; + struct lp3952_ctrl_hdl *leds; + struct lp3952_led_array *priv; + struct lp3952_platform_data *pdata = dev_get_platdata(&client->dev); + + if (!pdata) { + dev_err(&client->dev, + "lp3952: failed to obtain platform_data\n"); + return -EINVAL; + } + priv = devm_kzalloc(&client->dev, sizeof(struct lp3952_led_array), + GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->ndev = 6; + + leds = devm_kcalloc(&client->dev, priv->ndev, sizeof(*leds), + GFP_KERNEL); + if (!leds) { + devm_kfree(&client->dev, priv); + return -ENOMEM; + } + priv->leds = leds; + priv->client = client; + priv->enable_gpio = pdata->enable_gpio; + priv->regmap = devm_regmap_init_i2c(client, &lp3952_regmap); + if (IS_ERR(priv->regmap)) { + int err = PTR_ERR(priv->regmap); + + dev_err(&client->dev, "Failed to allocate register map: %d\n", + err); + return err; + } + + status = gpio_request(priv->enable_gpio, "lp3952_gpio"); + if (status) + dev_warn(&client->dev, "Unable to disable reset gpio: %d\n", + status); + + status = gpio_direction_output(priv->enable_gpio, 1); + if (status) + dev_warn(&client->dev, "Unable to disable reset gpio: %d\n", + status); + + i2c_set_clientdata(client, priv); + + status = lp3952_configure(priv); + if (status) { + dev_err(&client->dev, "Probe failed. Device not found (%d)\n", + status); + devm_kfree(&client->dev, leds); + devm_kfree(&client->dev, priv); + return status; + } + + status = lp3952_register_led_classdev(priv); + if (status) { + dev_err(&client->dev, "Unable to register led_classdev: %d\n", + status); + devm_kfree(&client->dev, leds); + devm_kfree(&client->dev, priv); + return status; + } + + return 0; +} + +static int lp3952_remove(struct i2c_client *client) +{ + struct lp3952_led_array *priv; + + priv = i2c_get_clientdata(client); + lp3952_on_off(priv, LP3952_LED_ALL, 0); + + lp3952_unregister_led_classdev(priv); + + devm_kfree(&client->dev, priv->leds); + gpio_direction_input(priv->enable_gpio); + gpio_free(priv->enable_gpio); + devm_kfree(&client->dev, priv); + return 0; +} + +static const struct i2c_device_id lp3952_id[] = { + {LP3952_NAME, 0}, + {} +}; + +static struct i2c_driver lp3952_i2c_driver = { + .driver = { + .name = LP3952_NAME, + .owner = THIS_MODULE, + }, + .probe = lp3952_probe, + .remove = lp3952_remove, + .id_table = lp3952_id, +}; + +module_i2c_driver(lp3952_i2c_driver); + +MODULE_AUTHOR("Tony Makkiel <tony.makkiel@daqri.com>"); +MODULE_DESCRIPTION("lp3952 I2C LED controller driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/leds-lp3952.h b/include/linux/leds-lp3952.h new file mode 100644 index 0000000..393f456 --- /dev/null +++ b/include/linux/leds-lp3952.h @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2016, DAQRI LLC + * + * License Terms: GPL v2 + * + * TI lp3952 Controller driver + * + * Author: Tony Makkiel <tony.makkiel@daqri.com> + * + */ + +#ifndef LEDS_LP3952_H_ +#define LEDS_LP3952_H_ + +#define LP3952_NAME "lp3952" +#define LP3952_DEV_ADDR 0x54 +#define LP3952_CMD_REG_COUNT 8 +#define LP3952_BRIGHT_MAX 4 +/* Following 2 MACRO are specific to Minnowboard Max + * Use the appropriate host specific values when + * instantiating device + */ +#define LP3952_BUS_ADDR 7 +#define LP3952_NRST_GPIO 464 + +/* Transition Time in ms */ +enum lp3952_tt { + TT0, + TT55, + TT110, + TT221, + TT422, + TT885, + TT1770, + TT3539 +}; + +/* Command Execution Time in ms */ +enum lp3952_cet { + CET197, + CET393, + CET590, + CET786, + CET1180, + CET1376, + CET1573, + CET1769, + CET1966, + CET2163, + CET2359, + CET2556, + CET2763, + CET2949, + CET3146 +}; + +/* Max Current in % */ +enum lp3952_colour_I_log_0 { + I0, + I7, + I14, + I21, + I32, + I46, + I71, + I100 +}; + +enum lp3952_leds { + LP3952_BLUE_2, + LP3952_GREEN_2, + LP3952_RED_2, + LP3952_BLUE_1, + LP3952_GREEN_1, + LP3952_RED_1, + LP3952_LED_ALL +}; + +struct lp3952_platform_data { + u32 enable_gpio; +}; + +#endif /* LEDS_LP3952_H_ */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] leds: LED driver for TI LP3952 6-Channel Color LED 2016-06-03 15:20 [PATCH v2 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel @ 2016-06-07 8:42 ` Jacek Anaszewski 2016-06-07 12:50 ` Tony Makkiel 0 siblings, 1 reply; 5+ messages in thread From: Jacek Anaszewski @ 2016-06-07 8:42 UTC (permalink / raw) To: Tony Makkiel; +Cc: linux-leds, rpurdie, rjw, lenb Hi Tony, Thanks for the update. I have few more comments below. On 06/03/2016 05:20 PM, Tony Makkiel wrote: > The chip can drive 2 sets of RGB leds. Controller can > be controlled via PWM, I2C and audio synchronisation. > This driver uses I2C to communicate with the chip. > > Datasheet: http://www.ti.com/lit/gpn/lp3952 > > Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> > --- > drivers/leds/Kconfig | 12 ++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-lp3952.c | 368 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/leds-lp3952.h | 83 ++++++++++ > 4 files changed, 464 insertions(+) > create mode 100644 drivers/leds/leds-lp3952.c > create mode 100644 include/linux/leds-lp3952.h > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 5ae2834..305faf9 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -228,6 +228,18 @@ config LEDS_LP3944 > To compile this driver as a module, choose M here: the > module will be called leds-lp3944. > > +config LEDS_LP3952 > + tristate "LED Support for TI LP3952 2 channel LED driver" > + depends on LEDS_CLASS > + depends on I2C > + select REGMAP_I2C > + help > + This option enables support for LEDs connected to the Texas > + Instruments LP3952 LED driver. > + > + To compile this driver as a module, choose M here: the > + module will be called leds-lp3952. > + > config LEDS_LP55XX_COMMON > tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" > depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index cb2013d..0684c86 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o > obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o > obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o > obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o > +obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o > obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o > obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o > obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o > diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c > new file mode 100644 > index 0000000..abdf23a > --- /dev/null > +++ b/drivers/leds/leds-lp3952.c > @@ -0,0 +1,368 @@ > +/* > + * Copyright (c) 2016, DAQRI, LLC. > + * > + * License Terms: GNU General Public License v2 > + * > + * leds-lp3952 - LED class driver for TI lp3952 controller > + * > + * Based on: > + * leds-tlc9516 - LED class driver for TLC95XX series > + * of I2C driven LED controllers from NXP > + * > + * Derived from leds-atmel-pwm, leds-bd2802 and initial PWM led 3952 > + * driver written by Alex Feinman > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include <linux/delay.h> > +#include <linux/gpio.h> > +#include <linux/i2c.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/leds.h> > +#include <linux/leds-lp3952.h> > +#include <linux/module.h> > +#include <linux/notifier.h> > +#include <linux/platform_device.h> > +#include <linux/pm.h> > +#include <linux/reboot.h> > +#include <linux/regmap.h> > + > +#define LP3952_REG_LED_CTRL 0x00 > +#define LP3952_REG_R1_BLNK_TIME_CTRL 0x01 > +#define LP3952_REG_R1_BLNK_CYCLE_CTRL 0x02 > +#define LP3952_REG_G1_BLNK_TIME_CTRL 0x03 > +#define LP3952_REG_G1_BLNK_CYCLE_CTRL 0x04 > +#define LP3952_REG_B1_BLNK_TIME_CTRL 0x05 > +#define LP3952_REG_B1_BLNK_CYCLE_CTRL 0x06 > +#define LP3952_REG_ENABLES 0x0B > +#define LP3952_REG_PAT_GEN_CTRL 0x11 > +#define LP3952_REG_RGB1_MAX_I_CTRL 0x12 > +#define LP3952_REG_RGB2_MAX_I_CTRL 0x13 > +#define LP3952_REG_CMD_0 0x50 > +#define LP3952_REG_RESET 0x60 > + > +#define REG_MAX LP3952_REG_RESET > + > +struct lp3952_ctrl_hdl { > + struct led_classdev cdev; > + char name[15]; > + enum lp3952_leds channel; > + void *priv; > +}; > + > +struct ptrn_gen_cmd { > + union { > + struct { > + u16 tt:3; > + u16 b:3; > + u16 cet:4; > + u16 g:3; > + u16 r:3; > + }; > + struct { > + u8 lsb; > + u8 msb; > + } bytes; > + }; > +} __packed; > + > +struct lp3952_led_array { > + u8 ndev; > + u32 enable_gpio; > + struct regmap *regmap; > + struct i2c_client *client; > + struct ptrn_gen_cmd pgm[LP3952_CMD_REG_COUNT]; > + struct lp3952_ctrl_hdl *leds; > +}; > + > +int lp3952_register_write(struct i2c_client *client, u8 reg, u8 val) > +{ > + int ret; > + struct lp3952_led_array *priv = i2c_get_clientdata(client); > + > + ret = regmap_write(priv->regmap, reg, val); > + > + if (ret) > + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", > + __func__, reg, val, ret); > + return ret; > +} > + > +static void lp3952_on_off(struct lp3952_led_array *priv, enum lp3952_leds led, s/led/led_id/ ? > + int on) > +{ > + int ret, val; > + > + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led, on); > + > + val = 1 << led; > + if (led == LP3952_LED_ALL) > + val = 0x3f; Please add macro definitions for bit fields instead of using numerical values directly. > + > + ret = regmap_update_bits(priv->regmap, LP3952_REG_LED_CTRL, val, > + on ? val : 0); > + if (ret) > + dev_err(&priv->client->dev, "%s, Error %d\n", __func__, ret); > +} > + > +/* > + * Using Imax to control brightness. There are 4 possible > + * setting 25, 50, 75 and 100 % of Imax. Possible values are > + * values 0-4. 0 meaning turn off. > + */ > +static int lp3952_set_brightness(struct led_classdev *cdev, > + enum led_brightness value) > +{ > + unsigned int reg, val, shift_val; > + struct lp3952_ctrl_hdl *led = container_of(cdev, > + struct lp3952_ctrl_hdl, > + cdev); > + struct lp3952_led_array *priv = (struct lp3952_led_array *)led->priv; > + > + dev_dbg(cdev->dev, "Brightness request: %d on %d\n", value, > + led->channel); > + > + if (value == LED_OFF) { > + lp3952_on_off(priv, led->channel, LED_OFF); > + return 0; > + } > + > + val = value - 1; val is redundant. How about value-- instead? > + reg = LP3952_REG_RGB1_MAX_I_CTRL; > + shift_val = (led->channel - LP3952_BLUE_1) * 2; > + > + if (led->channel > LP3952_RED_1) { > + dev_err(cdev->dev, " %s Invalid LED requested", __func__); > + return -EINVAL; > + } else if (led->channel < LP3952_BLUE_1) { > + shift_val = led->channel * 2; > + reg = LP3952_REG_RGB2_MAX_I_CTRL; > + } I'd rearrange this this way: if (led->channel > LP3952_RED_1) { dev_err(cdev->dev, " %s Invalid LED requested", __func__); return -EINVAL; } if (led->channel >= LP3952_BLUE_1) { reg = LP3952_REG_RGB1_MAX_I_CTRL; shift_val = (led->channel - LP3952_BLUE_1) * 2; } else reg = LP3952_REG_RGB2_MAX_I_CTRL; shift_val = led->channel * 2; } Also you should control here "enable_gpio", i.e. when all LEDs are turned off, it should be asserted low and raised high otherwise. > + /* Enable the LED in case it is not enabled already */ > + lp3952_on_off(priv, led->channel, 1); > + > + return (regmap_update_bits(priv->regmap, reg, 3 << shift_val, > + val << shift_val)); > +} > + > +static int lp3952_register_led_classdev(struct lp3952_led_array *priv) > +{ > + int ret, i; > + const char *led_name[] = { > + "lp3952:blue2", > + "lp3952:green2", > + "lp3952:red2", > + "lp3952:blue1", > + "lp3952:green1", > + "lp3952:red1" > + }; > + > + for (i = 0; i < priv->ndev; i++) { > + sprintf(priv->leds[i].name, "%s", led_name[i]); > + priv->leds[i].cdev.name = priv->leds[i].name; > + priv->leds[i].cdev.brightness = LED_OFF; > + priv->leds[i].cdev.max_brightness = LP3952_BRIGHT_MAX; > + priv->leds[i].cdev.brightness_set_blocking = > + lp3952_set_brightness; > + priv->leds[i].channel = i; > + priv->leds[i].priv = priv; > + > + ret = devm_led_classdev_register(&priv->client->dev, > + &priv->leds[i].cdev); > + if (ret < 0) { > + dev_err(&priv->client->dev, > + "couldn't register LED %s\n", > + priv->leds[i].cdev.name); > + goto error; > + } > + } > + return 0; > + > +error: > + for (; i >= 0; i--) > + devm_led_classdev_unregister(&priv->client->dev, > + &priv->leds[i].cdev); Resources acquired with devm prefixed API are released on driver removal. > + return ret; > +} > + > +static void lp3952_unregister_led_classdev(struct lp3952_led_array *priv) > +{ > + int i; > + > + for (i = 0; i < priv->ndev; i++) > + devm_led_classdev_unregister(&priv->client->dev, > + &priv->leds[i].cdev); > +} This is also not needed. > +static int lp3952_set_pattern_gen_cmd(struct lp3952_led_array *priv, > + u8 cmd_index, u8 r, u8 g, u8 b, > + enum lp3952_tt tt, enum lp3952_cet cet) > +{ > + int ret; > + struct ptrn_gen_cmd line = { > + .r = r, > + .g = g, > + .b = b, > + .cet = cet, > + .tt = tt > + }; Empty line here please. > + if (cmd_index >= LP3952_CMD_REG_COUNT) > + return -EINVAL; > + > + priv->pgm[cmd_index] = line; > + ret = lp3952_register_write(priv->client, > + LP3952_REG_CMD_0 + cmd_index * 2, > + line.bytes.msb); > + if (ret) > + return ret; > + > + return (lp3952_register_write(priv->client, > + LP3952_REG_CMD_0 + cmd_index * 2 + 1, > + line.bytes.lsb)); > +} > + > +static int lp3952_configure(struct lp3952_led_array *priv) > +{ > + int ret; > + > + /* Disable any LEDs on from any previous conf. */ > + ret = lp3952_register_write(priv->client, LP3952_REG_LED_CTRL, 0); > + if (ret) > + return ret; > + > + /* enable rgb patter, loop */ > + ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, > + 0x06); Please add bit field definitions and use them here, instead of 0x06 value. It makes code analysis easier. > + if (ret) > + return ret; > + > + /* Update Bit 6 (Active mode), Select both Led sets, Bit [1:0] */ > + ret = regmap_update_bits(priv->regmap, LP3952_REG_ENABLES, 0x43, 0xfc); The same applies to 0x43 and 0xfc. > + if (ret) > + return ret; > + > + /* Set Cmd1 for RGB intensity,cmd and transition time */ > + return (lp3952_set_pattern_gen_cmd(priv, 0, I46, I71, I100, TT0, > + CET197)); > +} > + > +static const struct regmap_config lp3952_regmap = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = REG_MAX, > +}; > + > +static int lp3952_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int status; > + struct lp3952_ctrl_hdl *leds; > + struct lp3952_led_array *priv; > + struct lp3952_platform_data *pdata = dev_get_platdata(&client->dev); > + > + if (!pdata) { > + dev_err(&client->dev, > + "lp3952: failed to obtain platform_data\n"); > + return -EINVAL; > + } Actually board files are deprecated. We use Device Tree instead now. Would it be possible to switch to using it for this driver? Please refer to Documentation/devicetree/bindings/leds/common.txt. > + priv = devm_kzalloc(&client->dev, sizeof(struct lp3952_led_array), Instead of using struct type name it is more preferred to pass dereferenced pointer as an argument to sizeof. Here it would be: sizeof(*priv) > + GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->ndev = 6; > + > + leds = devm_kcalloc(&client->dev, priv->ndev, sizeof(*leds), > + GFP_KERNEL); Like you're doing it here. > + if (!leds) { > + devm_kfree(&client->dev, priv); > + return -ENOMEM; > + } > + priv->leds = leds; > + priv->client = client; > + priv->enable_gpio = pdata->enable_gpio; > + priv->regmap = devm_regmap_init_i2c(client, &lp3952_regmap); > + if (IS_ERR(priv->regmap)) { > + int err = PTR_ERR(priv->regmap); > + > + dev_err(&client->dev, "Failed to allocate register map: %d\n", > + err); > + return err; > + } > + > + status = gpio_request(priv->enable_gpio, "lp3952_gpio"); > + if (status) > + dev_warn(&client->dev, "Unable to disable reset gpio: %d\n", > + status); > + > + status = gpio_direction_output(priv->enable_gpio, 1); > + if (status) > + dev_warn(&client->dev, "Unable to disable reset gpio: %d\n", > + status); Please switch over to using devm_gpiod API. > + > + i2c_set_clientdata(client, priv); > + > + status = lp3952_configure(priv); > + if (status) { > + dev_err(&client->dev, "Probe failed. Device not found (%d)\n", > + status); > + devm_kfree(&client->dev, leds); > + devm_kfree(&client->dev, priv); You don't need to call this expliciyly here. devm_kfree is useful in cases one wants to release devm_ prefixed resource allocation, but not necessarily tear down the driver. > + return status; > + } > + > + status = lp3952_register_led_classdev(priv); > + if (status) { > + dev_err(&client->dev, "Unable to register led_classdev: %d\n", > + status); > + devm_kfree(&client->dev, leds); > + devm_kfree(&client->dev, priv); Ditto. > + return status; > + } > + > + return 0; > +} > + > +static int lp3952_remove(struct i2c_client *client) > +{ > + struct lp3952_led_array *priv; > + > + priv = i2c_get_clientdata(client); > + lp3952_on_off(priv, LP3952_LED_ALL, 0); > + > + lp3952_unregister_led_classdev(priv); > + > + devm_kfree(&client->dev, priv->leds); Please remove the above line. > + gpio_direction_input(priv->enable_gpio); > + gpio_free(priv->enable_gpio); > + devm_kfree(&client->dev, priv); Please remove the above line. > + return 0; > +} > + > +static const struct i2c_device_id lp3952_id[] = { > + {LP3952_NAME, 0}, > + {} > +}; > + > +static struct i2c_driver lp3952_i2c_driver = { > + .driver = { > + .name = LP3952_NAME, > + .owner = THIS_MODULE, > + }, > + .probe = lp3952_probe, > + .remove = lp3952_remove, > + .id_table = lp3952_id, > +}; > + > +module_i2c_driver(lp3952_i2c_driver); > + > +MODULE_AUTHOR("Tony Makkiel <tony.makkiel@daqri.com>"); > +MODULE_DESCRIPTION("lp3952 I2C LED controller driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/leds-lp3952.h b/include/linux/leds-lp3952.h > new file mode 100644 > index 0000000..393f456 > --- /dev/null > +++ b/include/linux/leds-lp3952.h > @@ -0,0 +1,83 @@ > +/* > + * Copyright (C) 2016, DAQRI LLC > + * > + * License Terms: GPL v2 > + * > + * TI lp3952 Controller driver > + * > + * Author: Tony Makkiel <tony.makkiel@daqri.com> > + * > + */ > + > +#ifndef LEDS_LP3952_H_ > +#define LEDS_LP3952_H_ > + > +#define LP3952_NAME "lp3952" > +#define LP3952_DEV_ADDR 0x54 > +#define LP3952_CMD_REG_COUNT 8 > +#define LP3952_BRIGHT_MAX 4 > +/* Following 2 MACRO are specific to Minnowboard Max > + * Use the appropriate host specific values when > + * instantiating device > + */ > +#define LP3952_BUS_ADDR 7 > +#define LP3952_NRST_GPIO 464 > + > +/* Transition Time in ms */ > +enum lp3952_tt { > + TT0, > + TT55, > + TT110, > + TT221, > + TT422, > + TT885, > + TT1770, > + TT3539 > +}; > + > +/* Command Execution Time in ms */ > +enum lp3952_cet { > + CET197, > + CET393, > + CET590, > + CET786, > + CET1180, > + CET1376, > + CET1573, > + CET1769, > + CET1966, > + CET2163, > + CET2359, > + CET2556, > + CET2763, > + CET2949, > + CET3146 > +}; > + > +/* Max Current in % */ > +enum lp3952_colour_I_log_0 { > + I0, > + I7, > + I14, > + I21, > + I32, > + I46, > + I71, > + I100 > +}; > + > +enum lp3952_leds { > + LP3952_BLUE_2, > + LP3952_GREEN_2, > + LP3952_RED_2, > + LP3952_BLUE_1, > + LP3952_GREEN_1, > + LP3952_RED_1, > + LP3952_LED_ALL > +}; > + > +struct lp3952_platform_data { > + u32 enable_gpio; > +}; > + > +#endif /* LEDS_LP3952_H_ */ > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] leds: LED driver for TI LP3952 6-Channel Color LED 2016-06-07 8:42 ` Jacek Anaszewski @ 2016-06-07 12:50 ` Tony Makkiel 2016-06-07 14:46 ` Jacek Anaszewski 0 siblings, 1 reply; 5+ messages in thread From: Tony Makkiel @ 2016-06-07 12:50 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: linux-leds, rpurdie, rjw, lenb On 07/06/16 09:42, Jacek Anaszewski wrote: > Hi Tony, > > Thanks for the update. I have few more comments below. > > On 06/03/2016 05:20 PM, Tony Makkiel wrote: >> The chip can drive 2 sets of RGB leds. Controller can >> be controlled via PWM, I2C and audio synchronisation. >> This driver uses I2C to communicate with the chip. >> >> Datasheet: http://www.ti.com/lit/gpn/lp3952 >> >> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> >> --- >> drivers/leds/Kconfig | 12 ++ >> drivers/leds/Makefile | 1 + >> drivers/leds/leds-lp3952.c | 368 ++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/leds-lp3952.h | 83 ++++++++++ >> 4 files changed, 464 insertions(+) >> create mode 100644 drivers/leds/leds-lp3952.c >> create mode 100644 include/linux/leds-lp3952.h >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index 5ae2834..305faf9 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -228,6 +228,18 @@ config LEDS_LP3944 >> To compile this driver as a module, choose M here: the >> module will be called leds-lp3944. >> >> +config LEDS_LP3952 >> + tristate "LED Support for TI LP3952 2 channel LED driver" >> + depends on LEDS_CLASS >> + depends on I2C >> + select REGMAP_I2C >> + help >> + This option enables support for LEDs connected to the Texas >> + Instruments LP3952 LED driver. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called leds-lp3952. >> + >> config LEDS_LP55XX_COMMON >> tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" >> depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >> index cb2013d..0684c86 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o >> obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o >> obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o >> obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o >> +obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o >> obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o >> obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o >> obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o >> diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c >> new file mode 100644 >> index 0000000..abdf23a >> --- /dev/null >> +++ b/drivers/leds/leds-lp3952.c >> @@ -0,0 +1,368 @@ >> +/* >> + * Copyright (c) 2016, DAQRI, LLC. >> + * >> + * License Terms: GNU General Public License v2 >> + * >> + * leds-lp3952 - LED class driver for TI lp3952 controller >> + * >> + * Based on: >> + * leds-tlc9516 - LED class driver for TLC95XX series >> + * of I2C driven LED controllers from NXP >> + * >> + * Derived from leds-atmel-pwm, leds-bd2802 and initial PWM led 3952 >> + * driver written by Alex Feinman >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> + >> +#include <linux/delay.h> >> +#include <linux/gpio.h> >> +#include <linux/i2c.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/leds.h> >> +#include <linux/leds-lp3952.h> >> +#include <linux/module.h> >> +#include <linux/notifier.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm.h> >> +#include <linux/reboot.h> >> +#include <linux/regmap.h> >> + >> +#define LP3952_REG_LED_CTRL 0x00 >> +#define LP3952_REG_R1_BLNK_TIME_CTRL 0x01 >> +#define LP3952_REG_R1_BLNK_CYCLE_CTRL 0x02 >> +#define LP3952_REG_G1_BLNK_TIME_CTRL 0x03 >> +#define LP3952_REG_G1_BLNK_CYCLE_CTRL 0x04 >> +#define LP3952_REG_B1_BLNK_TIME_CTRL 0x05 >> +#define LP3952_REG_B1_BLNK_CYCLE_CTRL 0x06 >> +#define LP3952_REG_ENABLES 0x0B >> +#define LP3952_REG_PAT_GEN_CTRL 0x11 >> +#define LP3952_REG_RGB1_MAX_I_CTRL 0x12 >> +#define LP3952_REG_RGB2_MAX_I_CTRL 0x13 >> +#define LP3952_REG_CMD_0 0x50 >> +#define LP3952_REG_RESET 0x60 >> + >> +#define REG_MAX LP3952_REG_RESET >> + >> +struct lp3952_ctrl_hdl { >> + struct led_classdev cdev; >> + char name[15]; >> + enum lp3952_leds channel; >> + void *priv; >> +}; >> + >> +struct ptrn_gen_cmd { >> + union { >> + struct { >> + u16 tt:3; >> + u16 b:3; >> + u16 cet:4; >> + u16 g:3; >> + u16 r:3; >> + }; >> + struct { >> + u8 lsb; >> + u8 msb; >> + } bytes; >> + }; >> +} __packed; >> + >> +struct lp3952_led_array { >> + u8 ndev; >> + u32 enable_gpio; >> + struct regmap *regmap; >> + struct i2c_client *client; >> + struct ptrn_gen_cmd pgm[LP3952_CMD_REG_COUNT]; >> + struct lp3952_ctrl_hdl *leds; >> +}; >> + >> +int lp3952_register_write(struct i2c_client *client, u8 reg, u8 val) >> +{ >> + int ret; >> + struct lp3952_led_array *priv = i2c_get_clientdata(client); >> + >> + ret = regmap_write(priv->regmap, reg, val); >> + >> + if (ret) >> + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", >> + __func__, reg, val, ret); >> + return ret; >> +} >> + >> +static void lp3952_on_off(struct lp3952_led_array *priv, enum lp3952_leds led, > > s/led/led_id/ ? > >> + int on) >> +{ >> + int ret, val; >> + >> + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led, on); >> + >> + val = 1 << led; >> + if (led == LP3952_LED_ALL) >> + val = 0x3f; > > Please add macro definitions for bit fields instead of using numerical > values directly. > >> + >> + ret = regmap_update_bits(priv->regmap, LP3952_REG_LED_CTRL, val, >> + on ? val : 0); >> + if (ret) >> + dev_err(&priv->client->dev, "%s, Error %d\n", __func__, ret); >> +} >> + >> +/* >> + * Using Imax to control brightness. There are 4 possible >> + * setting 25, 50, 75 and 100 % of Imax. Possible values are >> + * values 0-4. 0 meaning turn off. >> + */ >> +static int lp3952_set_brightness(struct led_classdev *cdev, >> + enum led_brightness value) >> +{ >> + unsigned int reg, val, shift_val; >> + struct lp3952_ctrl_hdl *led = container_of(cdev, >> + struct lp3952_ctrl_hdl, >> + cdev); >> + struct lp3952_led_array *priv = (struct lp3952_led_array *)led->priv; >> + >> + dev_dbg(cdev->dev, "Brightness request: %d on %d\n", value, >> + led->channel); >> + >> + if (value == LED_OFF) { >> + lp3952_on_off(priv, led->channel, LED_OFF); >> + return 0; >> + } >> + >> + val = value - 1; > > val is redundant. How about value-- instead? > >> + reg = LP3952_REG_RGB1_MAX_I_CTRL; >> + shift_val = (led->channel - LP3952_BLUE_1) * 2; >> + >> + if (led->channel > LP3952_RED_1) { >> + dev_err(cdev->dev, " %s Invalid LED requested", __func__); >> + return -EINVAL; >> + } else if (led->channel < LP3952_BLUE_1) { >> + shift_val = led->channel * 2; >> + reg = LP3952_REG_RGB2_MAX_I_CTRL; >> + } > > I'd rearrange this this way: > > if (led->channel > LP3952_RED_1) { > dev_err(cdev->dev, " %s Invalid LED requested", __func__); > return -EINVAL; > } > > if (led->channel >= LP3952_BLUE_1) { > reg = LP3952_REG_RGB1_MAX_I_CTRL; > shift_val = (led->channel - LP3952_BLUE_1) * 2; > } else > reg = LP3952_REG_RGB2_MAX_I_CTRL; > shift_val = led->channel * 2; > } > > Also you should control here "enable_gpio", i.e. when > all LEDs are turned off, it should be asserted low and raised high > otherwise. Pulling the gpio low here could be a bad idea. The pin also act as a chip reset, which will revert all the registers to default values. So it might be better to toggle them only when the driver is loaded/removed. > >> + /* Enable the LED in case it is not enabled already */ >> + lp3952_on_off(priv, led->channel, 1); >> + >> + return (regmap_update_bits(priv->regmap, reg, 3 << shift_val, >> + val << shift_val)); >> +} >> + >> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv) >> +{ >> + int ret, i; >> + const char *led_name[] = { >> + "lp3952:blue2", >> + "lp3952:green2", >> + "lp3952:red2", >> + "lp3952:blue1", >> + "lp3952:green1", >> + "lp3952:red1" >> + }; >> + >> + for (i = 0; i < priv->ndev; i++) { >> + sprintf(priv->leds[i].name, "%s", led_name[i]); >> + priv->leds[i].cdev.name = priv->leds[i].name; >> + priv->leds[i].cdev.brightness = LED_OFF; >> + priv->leds[i].cdev.max_brightness = LP3952_BRIGHT_MAX; >> + priv->leds[i].cdev.brightness_set_blocking = >> + lp3952_set_brightness; >> + priv->leds[i].channel = i; >> + priv->leds[i].priv = priv; >> + >> + ret = devm_led_classdev_register(&priv->client->dev, >> + &priv->leds[i].cdev); >> + if (ret < 0) { >> + dev_err(&priv->client->dev, >> + "couldn't register LED %s\n", >> + priv->leds[i].cdev.name); >> + goto error; >> + } >> + } >> + return 0; >> + >> +error: >> + for (; i >= 0; i--) >> + devm_led_classdev_unregister(&priv->client->dev, >> + &priv->leds[i].cdev); > > Resources acquired with devm prefixed API are released on driver > removal. > >> + return ret; >> +} >> + >> +static void lp3952_unregister_led_classdev(struct lp3952_led_array *priv) >> +{ >> + int i; >> + >> + for (i = 0; i < priv->ndev; i++) >> + devm_led_classdev_unregister(&priv->client->dev, >> + &priv->leds[i].cdev); >> +} > > This is also not needed. > >> +static int lp3952_set_pattern_gen_cmd(struct lp3952_led_array *priv, >> + u8 cmd_index, u8 r, u8 g, u8 b, >> + enum lp3952_tt tt, enum lp3952_cet cet) >> +{ >> + int ret; >> + struct ptrn_gen_cmd line = { >> + .r = r, >> + .g = g, >> + .b = b, >> + .cet = cet, >> + .tt = tt >> + }; > > Empty line here please. > >> + if (cmd_index >= LP3952_CMD_REG_COUNT) >> + return -EINVAL; >> + >> + priv->pgm[cmd_index] = line; >> + ret = lp3952_register_write(priv->client, >> + LP3952_REG_CMD_0 + cmd_index * 2, >> + line.bytes.msb); >> + if (ret) >> + return ret; >> + >> + return (lp3952_register_write(priv->client, >> + LP3952_REG_CMD_0 + cmd_index * 2 + 1, >> + line.bytes.lsb)); >> +} >> + >> +static int lp3952_configure(struct lp3952_led_array *priv) >> +{ >> + int ret; >> + >> + /* Disable any LEDs on from any previous conf. */ >> + ret = lp3952_register_write(priv->client, LP3952_REG_LED_CTRL, 0); >> + if (ret) >> + return ret; >> + >> + /* enable rgb patter, loop */ >> + ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, >> + 0x06); > > Please add bit field definitions and use them here, > instead of 0x06 value. It makes code analysis easier. I did not quite understand. Bit 1 enables pattern loop, Bit 2, enables pattern gen. Did you mean something like value = 1 << 1 | 1 << 2; ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, value); ? > >> + if (ret) >> + return ret; >> + >> + /* Update Bit 6 (Active mode), Select both Led sets, Bit [1:0] */ >> + ret = regmap_update_bits(priv->regmap, LP3952_REG_ENABLES, 0x43, 0xfc); > > The same applies to 0x43 and 0xfc. > >> + if (ret) >> + return ret; >> + >> + /* Set Cmd1 for RGB intensity,cmd and transition time */ >> + return (lp3952_set_pattern_gen_cmd(priv, 0, I46, I71, I100, TT0, >> + CET197)); >> +} >> + >> +static const struct regmap_config lp3952_regmap = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + .max_register = REG_MAX, >> +}; >> + >> +static int lp3952_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + int status; >> + struct lp3952_ctrl_hdl *leds; >> + struct lp3952_led_array *priv; >> + struct lp3952_platform_data *pdata = dev_get_platdata(&client->dev); >> + >> + if (!pdata) { >> + dev_err(&client->dev, >> + "lp3952: failed to obtain platform_data\n"); >> + return -EINVAL; >> + } > > Actually board files are deprecated. We use Device Tree instead now. > Would it be possible to switch to using it for this driver? > Please refer to Documentation/devicetree/bindings/leds/common.txt. > I will remove platform driver approach. My host board use ACPI instead of Device tree. I do not have firmware code, for the board. But shall try to emulate ACPI entry for the LED from kernel. >> + priv = devm_kzalloc(&client->dev, sizeof(struct lp3952_led_array), > > Instead of using struct type name it is more preferred to pass > dereferenced pointer as an argument to sizeof. Here it would be: > > sizeof(*priv) > >> + GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->ndev = 6; >> + >> + leds = devm_kcalloc(&client->dev, priv->ndev, sizeof(*leds), >> + GFP_KERNEL); > > Like you're doing it here. > >> + if (!leds) { >> + devm_kfree(&client->dev, priv); >> + return -ENOMEM; >> + } >> + priv->leds = leds; >> + priv->client = client; >> + priv->enable_gpio = pdata->enable_gpio; >> + priv->regmap = devm_regmap_init_i2c(client, &lp3952_regmap); >> + if (IS_ERR(priv->regmap)) { >> + int err = PTR_ERR(priv->regmap); >> + >> + dev_err(&client->dev, "Failed to allocate register map: %d\n", >> + err); >> + return err; >> + } >> + >> + status = gpio_request(priv->enable_gpio, "lp3952_gpio"); >> + if (status) >> + dev_warn(&client->dev, "Unable to disable reset gpio: %d\n", >> + status); >> + >> + status = gpio_direction_output(priv->enable_gpio, 1); >> + if (status) >> + dev_warn(&client->dev, "Unable to disable reset gpio: %d\n", >> + status); > > Please switch over to using devm_gpiod API. > >> + >> + i2c_set_clientdata(client, priv); >> + >> + status = lp3952_configure(priv); >> + if (status) { >> + dev_err(&client->dev, "Probe failed. Device not found (%d)\n", >> + status); >> + devm_kfree(&client->dev, leds); >> + devm_kfree(&client->dev, priv); > > You don't need to call this expliciyly here. devm_kfree is useful > in cases one wants to release devm_ prefixed resource allocation, > but not necessarily tear down the driver. > >> + return status; >> + } >> + >> + status = lp3952_register_led_classdev(priv); >> + if (status) { >> + dev_err(&client->dev, "Unable to register led_classdev: %d\n", >> + status); >> + devm_kfree(&client->dev, leds); >> + devm_kfree(&client->dev, priv); > > Ditto. > >> + return status; >> + } >> + >> + return 0; >> +} >> + >> +static int lp3952_remove(struct i2c_client *client) >> +{ >> + struct lp3952_led_array *priv; >> + >> + priv = i2c_get_clientdata(client); >> + lp3952_on_off(priv, LP3952_LED_ALL, 0); >> + >> + lp3952_unregister_led_classdev(priv); >> + >> + devm_kfree(&client->dev, priv->leds); > > Please remove the above line. > >> + gpio_direction_input(priv->enable_gpio); >> + gpio_free(priv->enable_gpio); >> + devm_kfree(&client->dev, priv); > > Please remove the above line. > >> + return 0; >> +} >> + >> +static const struct i2c_device_id lp3952_id[] = { >> + {LP3952_NAME, 0}, >> + {} >> +}; >> + >> +static struct i2c_driver lp3952_i2c_driver = { >> + .driver = { >> + .name = LP3952_NAME, >> + .owner = THIS_MODULE, >> + }, >> + .probe = lp3952_probe, >> + .remove = lp3952_remove, >> + .id_table = lp3952_id, >> +}; >> + >> +module_i2c_driver(lp3952_i2c_driver); >> + >> +MODULE_AUTHOR("Tony Makkiel <tony.makkiel@daqri.com>"); >> +MODULE_DESCRIPTION("lp3952 I2C LED controller driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/linux/leds-lp3952.h b/include/linux/leds-lp3952.h >> new file mode 100644 >> index 0000000..393f456 >> --- /dev/null >> +++ b/include/linux/leds-lp3952.h >> @@ -0,0 +1,83 @@ >> +/* >> + * Copyright (C) 2016, DAQRI LLC >> + * >> + * License Terms: GPL v2 >> + * >> + * TI lp3952 Controller driver >> + * >> + * Author: Tony Makkiel <tony.makkiel@daqri.com> >> + * >> + */ >> + >> +#ifndef LEDS_LP3952_H_ >> +#define LEDS_LP3952_H_ >> + >> +#define LP3952_NAME "lp3952" >> +#define LP3952_DEV_ADDR 0x54 >> +#define LP3952_CMD_REG_COUNT 8 >> +#define LP3952_BRIGHT_MAX 4 >> +/* Following 2 MACRO are specific to Minnowboard Max >> + * Use the appropriate host specific values when >> + * instantiating device >> + */ >> +#define LP3952_BUS_ADDR 7 >> +#define LP3952_NRST_GPIO 464 >> + >> +/* Transition Time in ms */ >> +enum lp3952_tt { >> + TT0, >> + TT55, >> + TT110, >> + TT221, >> + TT422, >> + TT885, >> + TT1770, >> + TT3539 >> +}; >> + >> +/* Command Execution Time in ms */ >> +enum lp3952_cet { >> + CET197, >> + CET393, >> + CET590, >> + CET786, >> + CET1180, >> + CET1376, >> + CET1573, >> + CET1769, >> + CET1966, >> + CET2163, >> + CET2359, >> + CET2556, >> + CET2763, >> + CET2949, >> + CET3146 >> +}; >> + >> +/* Max Current in % */ >> +enum lp3952_colour_I_log_0 { >> + I0, >> + I7, >> + I14, >> + I21, >> + I32, >> + I46, >> + I71, >> + I100 >> +}; >> + >> +enum lp3952_leds { >> + LP3952_BLUE_2, >> + LP3952_GREEN_2, >> + LP3952_RED_2, >> + LP3952_BLUE_1, >> + LP3952_GREEN_1, >> + LP3952_RED_1, >> + LP3952_LED_ALL >> +}; >> + >> +struct lp3952_platform_data { >> + u32 enable_gpio; >> +}; >> + >> +#endif /* LEDS_LP3952_H_ */ >> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] leds: LED driver for TI LP3952 6-Channel Color LED 2016-06-07 12:50 ` Tony Makkiel @ 2016-06-07 14:46 ` Jacek Anaszewski 2016-06-08 15:30 ` Tony Makkiel 0 siblings, 1 reply; 5+ messages in thread From: Jacek Anaszewski @ 2016-06-07 14:46 UTC (permalink / raw) To: Tony Makkiel, rjw, lenb; +Cc: linux-leds, rpurdie On 06/07/2016 02:50 PM, Tony Makkiel wrote: > > > On 07/06/16 09:42, Jacek Anaszewski wrote: >> Hi Tony, >> >> Thanks for the update. I have few more comments below. >> >> I'd rearrange this this way: >> >> if (led->channel > LP3952_RED_1) { >> dev_err(cdev->dev, " %s Invalid LED requested", __func__); >> return -EINVAL; >> } >> >> if (led->channel >= LP3952_BLUE_1) { >> reg = LP3952_REG_RGB1_MAX_I_CTRL; >> shift_val = (led->channel - LP3952_BLUE_1) * 2; >> } else >> reg = LP3952_REG_RGB2_MAX_I_CTRL; >> shift_val = led->channel * 2; >> } >> >> Also you should control here "enable_gpio", i.e. when >> all LEDs are turned off, it should be asserted low and raised high >> otherwise. > Pulling the gpio low here could be a bad idea. The pin also act as a > chip reset, which will revert all the registers to default values. > So it might be better to toggle them only when the driver is > loaded/removed. Ack. [...] >>> +static int lp3952_configure(struct lp3952_led_array *priv) >>> +{ >>> + int ret; >>> + >>> + /* Disable any LEDs on from any previous conf. */ >>> + ret = lp3952_register_write(priv->client, LP3952_REG_LED_CTRL, 0); >>> + if (ret) >>> + return ret; >>> + >>> + /* enable rgb patter, loop */ >>> + ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, >>> + 0x06); >> >> Please add bit field definitions and use them here, >> instead of 0x06 value. It makes code analysis easier. > I did not quite understand. Bit 1 enables pattern loop, Bit 2, enables > pattern gen. Did you mean something like > > value = 1 << 1 | 1 << 2; > ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, > value); > > ? You could have macros that would define values for these bits: #define LP3952_PATTERN_LOOP BIT(1) #define LP3952_PATTERN_GEN BIT(2) and then: ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, LP3952_PATTERN_LOOP | LP3952_PATTERN_GEN) If in doubt, please refer to the other LED class drivers. [...] >>> +static int lp3952_probe(struct i2c_client *client, >>> + const struct i2c_device_id *id) >>> +{ >>> + int status; >>> + struct lp3952_ctrl_hdl *leds; >>> + struct lp3952_led_array *priv; >>> + struct lp3952_platform_data *pdata = dev_get_platdata(&client->dev); >>> + >>> + if (!pdata) { >>> + dev_err(&client->dev, >>> + "lp3952: failed to obtain platform_data\n"); >>> + return -EINVAL; >>> + } >> >> Actually board files are deprecated. We use Device Tree instead now. >> Would it be possible to switch to using it for this driver? >> Please refer to Documentation/devicetree/bindings/leds/common.txt. >> > I will remove platform driver approach. My host board use ACPI instead of > Device tree. I do not have firmware code, for the board. But shall > try to emulate ACPI entry for the LED from kernel. Len, Rafael - any advise on how to approach this case? This I2C device is present on x86 Minnowboard Max board. I've seen ACPI overlays RFC [1], would it apply to this driver? [1] https://lwn.net/Articles/682084/ -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] leds: LED driver for TI LP3952 6-Channel Color LED 2016-06-07 14:46 ` Jacek Anaszewski @ 2016-06-08 15:30 ` Tony Makkiel 0 siblings, 0 replies; 5+ messages in thread From: Tony Makkiel @ 2016-06-08 15:30 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: linux-leds Thank you for the comments Jacek. On 07/06/16 15:46, Jacek Anaszewski wrote: > On 06/07/2016 02:50 PM, Tony Makkiel wrote: >> >> >> On 07/06/16 09:42, Jacek Anaszewski wrote: >>> Hi Tony, >>> >>> Thanks for the update. I have few more comments below. >>> >>> I'd rearrange this this way: >>> >>> if (led->channel > LP3952_RED_1) { >>> dev_err(cdev->dev, " %s Invalid LED requested", __func__); >>> return -EINVAL; >>> } >>> >>> if (led->channel >= LP3952_BLUE_1) { >>> reg = LP3952_REG_RGB1_MAX_I_CTRL; >>> shift_val = (led->channel - LP3952_BLUE_1) * 2; >>> } else >>> reg = LP3952_REG_RGB2_MAX_I_CTRL; >>> shift_val = led->channel * 2; >>> } >>> >>> Also you should control here "enable_gpio", i.e. when >>> all LEDs are turned off, it should be asserted low and raised high >>> otherwise. >> Pulling the gpio low here could be a bad idea. The pin also act as a >> chip reset, which will revert all the registers to default values. >> So it might be better to toggle them only when the driver is >> loaded/removed. > > Ack. > > [...] > >>>> +static int lp3952_configure(struct lp3952_led_array *priv) >>>> +{ >>>> + int ret; >>>> + >>>> + /* Disable any LEDs on from any previous conf. */ >>>> + ret = lp3952_register_write(priv->client, LP3952_REG_LED_CTRL, 0); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + /* enable rgb patter, loop */ >>>> + ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, >>>> + 0x06); >>> >>> Please add bit field definitions and use them here, >>> instead of 0x06 value. It makes code analysis easier. >> I did not quite understand. Bit 1 enables pattern loop, Bit 2, enables >> pattern gen. Did you mean something like >> >> value = 1 << 1 | 1 << 2; >> ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, >> value); >> >> ? > > You could have macros that would define values for these bits: > > #define LP3952_PATTERN_LOOP BIT(1) > #define LP3952_PATTERN_GEN BIT(2) > > and then: > > ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, > LP3952_PATTERN_LOOP | LP3952_PATTERN_GEN) > > If in doubt, please refer to the other LED class drivers. > > [...] >>>> +static int lp3952_probe(struct i2c_client *client, >>>> + const struct i2c_device_id *id) >>>> +{ >>>> + int status; >>>> + struct lp3952_ctrl_hdl *leds; >>>> + struct lp3952_led_array *priv; >>>> + struct lp3952_platform_data *pdata = dev_get_platdata(&client->dev); >>>> + >>>> + if (!pdata) { >>>> + dev_err(&client->dev, >>>> + "lp3952: failed to obtain platform_data\n"); >>>> + return -EINVAL; >>>> + } >>> >>> Actually board files are deprecated. We use Device Tree instead now. >>> Would it be possible to switch to using it for this driver? >>> Please refer to Documentation/devicetree/bindings/leds/common.txt. >>> >> I will remove platform driver approach. My host board use ACPI instead of >> Device tree. I do not have firmware code, for the board. But shall >> try to emulate ACPI entry for the LED from kernel. > > Len, Rafael - any advise on how to approach this case? > This I2C device is present on x86 Minnowboard Max board. > I've seen ACPI overlays RFC [1], would it apply to this driver? > > [1] https://lwn.net/Articles/682084/ > Also thank you for the link Jacek, managed to build kernel with ACPI overlay. Will send updated patch, with ACPI enumeration support and all the other changes suggested. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-08 15:30 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-03 15:20 [PATCH v2 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel 2016-06-07 8:42 ` Jacek Anaszewski 2016-06-07 12:50 ` Tony Makkiel 2016-06-07 14:46 ` Jacek Anaszewski 2016-06-08 15:30 ` Tony Makkiel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox