* [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
@ 2016-06-09 10:46 Tony Makkiel
2016-06-09 12:11 ` Jacek Anaszewski
0 siblings, 1 reply; 10+ messages in thread
From: Tony Makkiel @ 2016-06-09 10:46 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 | 359 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/leds-lp3952.h | 75 +++++++++
4 files changed, 447 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..a621bda
--- /dev/null
+++ b/drivers/leds/leds-lp3952.c
@@ -0,0 +1,359 @@
+/*
+ * 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/acpi.h>
+#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
+
+
+#define LP3952_PATRN_LOOP BIT(1)
+#define LP3952_PATRN_GEN_EN BIT(2)
+#define LP3952_ACTIVE_MODE BIT(6)
+#define LP3952_RGB_SEL_MASK (BIT(0) | BIT(1))
+#define LP3952_LED_MASK_ALL 0x3f
+
+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;
+ struct regmap *regmap;
+ struct i2c_client *client;
+ struct gpio_desc *enable_gpio;
+ 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_id, int on)
+{
+ int ret, val;
+
+ dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led_id, on);
+
+ val = 1 << led_id;
+ if (led_id == LP3952_LED_ALL)
+ val = LP3952_LED_MASK_ALL;
+
+ 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, 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;
+ }
+
+ 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;
+ }
+
+ /* 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,
+ --value << shift_val));
+}
+
+static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
+{
+ int i, ret = -ENODEV;
+ 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);
+ break;
+ }
+ }
+ return ret;
+}
+
+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,
+ LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN);
+ 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,
+ LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK,
+ LP3952_ACTIVE_MODE);
+ 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;
+
+ dev_dbg(&client->dev, "%s\n", __func__);
+
+ priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->ndev = LP3952_LED_ALL;
+
+ 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 = devm_gpiod_get(&client->dev, NULL, GPIOD_OUT_HIGH);
+ if (IS_ERR(priv->enable_gpio)) {
+ status = PTR_ERR(priv->enable_gpio);
+ dev_err(&client->dev, "Failed to enable gpio: %d\n", status);
+ return status;
+ }
+
+ 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;
+ }
+
+ i2c_set_clientdata(client, priv);
+
+ status = lp3952_configure(priv);
+ if (status) {
+ dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
+ status);
+ return status;
+ }
+
+ status = lp3952_register_led_classdev(priv);
+ if (status) {
+ dev_err(&client->dev, "Unable to register led_classdev: %d\n",
+ status);
+ 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);
+ gpiod_set_value(priv->enable_gpio, 0);
+
+ return 0;
+}
+
+static const struct i2c_device_id lp3952_id[] = {
+ {LP3952_NAME, 0},
+ {}
+};
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id lp3952_acpi_match[] = {
+ {LP3952_NAME, 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
+#endif
+
+static struct i2c_driver lp3952_i2c_driver = {
+ .driver = {
+ .name = LP3952_NAME,
+ .owner = THIS_MODULE,
+#ifdef CONFIG_ACPI
+ .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
+#endif
+ },
+ .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..3ea120a
--- /dev/null
+++ b/include/linux/leds-lp3952.h
@@ -0,0 +1,75 @@
+/*
+ * 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_
+
+/* ACPI iasl compiler wont take name LP3952,
+ * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
+ */
+#define LP3952_NAME "TLP3952"
+#define LP3952_CMD_REG_COUNT 8
+#define LP3952_BRIGHT_MAX 4
+
+/* 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
+};
+
+#endif /* LEDS_LP3952_H_ */
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-09 10:46 [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel
@ 2016-06-09 12:11 ` Jacek Anaszewski
2016-06-09 15:20 ` Tony Makkiel
0 siblings, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2016-06-09 12:11 UTC (permalink / raw)
To: Tony Makkiel; +Cc: linux-leds, rpurdie, rjw, lenb
Hi Tony,
Thanks for the updated patch. I have some comments in the code below.
On 06/09/2016 12:46 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 | 359 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/leds-lp3952.h | 75 +++++++++
> 4 files changed, 447 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..a621bda
> --- /dev/null
> +++ b/drivers/leds/leds-lp3952.c
> @@ -0,0 +1,359 @@
> +/*
> + * Copyright (c) 2016, DAQRI, LLC.
> + *
> + * License Terms: GNU General Public License v2
You didn't address my remarks regarding GPL license
boilerplate. Is it intentional?
> + *
> + * 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/acpi.h>
> +#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
> +
> +
> +#define LP3952_PATRN_LOOP BIT(1)
> +#define LP3952_PATRN_GEN_EN BIT(2)
> +#define LP3952_ACTIVE_MODE BIT(6)
> +#define LP3952_RGB_SEL_MASK (BIT(0) | BIT(1))
#define LP3952_RGB_SEL_MASK 0x03
> +#define LP3952_LED_MASK_ALL 0x3f
Please put all the macro values in the same column.
Even though you don't use all features of the device, it is a good
practice to provide definitions for all bits and in all registers.
Also, since you are providing header file, please move all macro
definitions and structures there.
> +
> +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;
> + struct regmap *regmap;
> + struct i2c_client *client;
> + struct gpio_desc *enable_gpio;
> + 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_id, int on)
> +{
> + int ret, val;
> +
> + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led_id, on);
> +
> + val = 1 << led_id;
> + if (led_id == LP3952_LED_ALL)
> + val = LP3952_LED_MASK_ALL;
> +
> + 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, 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;
> + }
> +
> + 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;
> + }
> +
> + /* 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,
> + --value << shift_val));
> +}
> +
> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
> +{
> + int i, ret = -ENODEV;
> + 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);
> + break;
> + }
> + }
> + return ret;
> +}
> +
> +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;
Why actually do you need pgm array? It is accessed only here during
assignment.
> + 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,
> + LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN);
> + 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,
> + LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK,
> + LP3952_ACTIVE_MODE);
Why LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK? Mask argument should
match the bit field to be overwritten with the new value.
> + 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));
> +}
Could you explain please, why the pattern has to be set while only
fixed brightness setting is supported by the driver?
Do you plan to add support for setting blinking patterns in the future?
> +static const struct regmap_config lp3952_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = REG_MAX,
> +};
You're not enabling regmap cache so regmap_update_bits(), will perform
I2C readout each time. Is it intentional?
> +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;
> +
> + dev_dbg(&client->dev, "%s\n", __func__);
> +
> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->ndev = LP3952_LED_ALL;
> +
> + 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 = devm_gpiod_get(&client->dev, NULL, GPIOD_OUT_HIGH);
How the driver knows which GPIO should it acquire? You're passing NULL
as the second argument.
> + if (IS_ERR(priv->enable_gpio)) {
> + status = PTR_ERR(priv->enable_gpio);
> + dev_err(&client->dev, "Failed to enable gpio: %d\n", status);
> + return status;
> + }
> +
> + 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;
> + }
> +
> + i2c_set_clientdata(client, priv);
> +
> + status = lp3952_configure(priv);
> + if (status) {
> + dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
> + status);
> + return status;
> + }
> +
> + status = lp3952_register_led_classdev(priv);
> + if (status) {
> + dev_err(&client->dev, "Unable to register led_classdev: %d\n",
> + status);
> + 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);
> + gpiod_set_value(priv->enable_gpio, 0);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id lp3952_id[] = {
> + {LP3952_NAME, 0},
> + {}
> +};
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id lp3952_acpi_match[] = {
> + {LP3952_NAME, 0},
> + {}
> +};
Could you please share how did you apply ACPI overlay?
> +
> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
> +#endif
> +
> +static struct i2c_driver lp3952_i2c_driver = {
> + .driver = {
> + .name = LP3952_NAME,
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_ACPI
> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
> +#endif
> + },
> + .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..3ea120a
> --- /dev/null
> +++ b/include/linux/leds-lp3952.h
> @@ -0,0 +1,75 @@
> +/*
> + * 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_
> +
> +/* ACPI iasl compiler wont take name LP3952,
> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
> + */
> +#define LP3952_NAME "TLP3952"
> +#define LP3952_CMD_REG_COUNT 8
> +#define LP3952_BRIGHT_MAX 4
> +
> +/* 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
> +};
> +
> +#endif /* LEDS_LP3952_H_ */
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-09 12:11 ` Jacek Anaszewski
@ 2016-06-09 15:20 ` Tony Makkiel
2016-06-10 8:26 ` Jacek Anaszewski
0 siblings, 1 reply; 10+ messages in thread
From: Tony Makkiel @ 2016-06-09 15:20 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, rpurdie, rjw, lenb
On 09/06/16 13:11, Jacek Anaszewski wrote:
> Hi Tony,
>
> Thanks for the updated patch. I have some comments in the code below.
>
> On 06/09/2016 12:46 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 | 359 ++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/leds-lp3952.h | 75 +++++++++
>> 4 files changed, 447 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..a621bda
>> --- /dev/null
>> +++ b/drivers/leds/leds-lp3952.c
>> @@ -0,0 +1,359 @@
>> +/*
>> + * Copyright (c) 2016, DAQRI, LLC.
>> + *
>> + * License Terms: GNU General Public License v2
>
> You didn't address my remarks regarding GPL license
> boilerplate. Is it intentional?
>
Sorry, I couldn't find the comment on license
boiler plate. Can you please remind me what I missed?
>> + *
>> + * 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/acpi.h>
>> +#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
>> +
>> +
>> +#define LP3952_PATRN_LOOP BIT(1)
>> +#define LP3952_PATRN_GEN_EN BIT(2)
>> +#define LP3952_ACTIVE_MODE BIT(6)
>> +#define LP3952_RGB_SEL_MASK (BIT(0) | BIT(1))
>
> #define LP3952_RGB_SEL_MASK 0x03
>
>> +#define LP3952_LED_MASK_ALL 0x3f
>
> Please put all the macro values in the same column.
Will do
>
> Even though you don't use all features of the device, it is a good
> practice to provide definitions for all bits and in all registers.
>
> Also, since you are providing header file, please move all macro
> definitions and structures there.
will do.
>
>> +
>> +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;
>> + struct regmap *regmap;
>> + struct i2c_client *client;
>> + struct gpio_desc *enable_gpio;
>> + 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_id, int on)
>> +{
>> + int ret, val;
>> +
>> + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led_id, on);
>> +
>> + val = 1 << led_id;
>> + if (led_id == LP3952_LED_ALL)
>> + val = LP3952_LED_MASK_ALL;
>> +
>> + 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, 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;
>> + }
>> +
>> + 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;
>> + }
>> +
>> + /* 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,
>> + --value << shift_val));
>> +}
>> +
>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>> +{
>> + int i, ret = -ENODEV;
>> + 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);
>> + break;
>> + }
>> + }
>> + return ret;
>> +}
>> +
>> +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;
>
> Why actually do you need pgm array? It is accessed only here during
> assignment.
The chip can support upto 8 commands. We are using only 1 command (Chip maintains
settings of last command). But if somebody in the future wants to utilize the
whole 8 command sets (say to, have custom lighting effects), they
can program so using the pgm array(with this function). But for normal operation
just 1 array/command would do.
>
>> + 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,
>> + LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN);
>> + 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,
>> + LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK,
>> + LP3952_ACTIVE_MODE);
>
> Why LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK? Mask argument should
> match the bit field to be overwritten with the new value.
We have to make sure 3 bits have specific values.
Bit 6(LP3952_ACTIVE_MODE) needs to be high.
Bit [1:0] decides which leds needs to be controlled. It needs to be set
0 for both LED sets to follow Pattern gen.
>
>> + 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));
>> +}
>
> Could you explain please, why the pattern has to be set while only
> fixed brightness setting is supported by the driver?
>
Note we are only setting Command 1 as mentioned in my comment above.
Pattern gen loops on this command. This command sets the intensity
and transition times. Without setting this register, the LEDs do
not turn on.
> Do you plan to add support for setting blinking patterns in the future?
Not sure, The driver works as it is for normal led operations.
This can be easily added using device_attribute with help of
"lp3952_set_pattern_gen_cmd".
>
>> +static const struct regmap_config lp3952_regmap = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + .max_register = REG_MAX,
>> +};
>
> You're not enabling regmap cache so regmap_update_bits(), will perform
> I2C readout each time. Is it intentional?
>
Thank you for the pointer. I did not look into it. Did not expect
lot of traffic unless there is software blink.
I will find out more about caching mechanism and add an
appropriate cache type.
>> +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;
>> +
>> + dev_dbg(&client->dev, "%s\n", __func__);
>> +
>> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->ndev = LP3952_LED_ALL;
>> +
>> + 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 = devm_gpiod_get(&client->dev, NULL, GPIOD_OUT_HIGH);
>
> How the driver knows which GPIO should it acquire? You're passing NULL
> as the second argument.
In the ACPI asl file, there is only one GPIO associated for lp3952 node.
GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
IoRestrictionOutputOnly, "\\_SB.GPO2",
0x00, ResourceConsumer, , )
>
>> + if (IS_ERR(priv->enable_gpio)) {
>> + status = PTR_ERR(priv->enable_gpio);
>> + dev_err(&client->dev, "Failed to enable gpio: %d\n", status);
>> + return status;
>> + }
>> +
>> + 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;
>> + }
>> +
>> + i2c_set_clientdata(client, priv);
>> +
>> + status = lp3952_configure(priv);
>> + if (status) {
>> + dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
>> + status);
>> + return status;
>> + }
>> +
>> + status = lp3952_register_led_classdev(priv);
>> + if (status) {
>> + dev_err(&client->dev, "Unable to register led_classdev: %d\n",
>> + status);
>> + 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);
>> + gpiod_set_value(priv->enable_gpio, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id lp3952_id[] = {
>> + {LP3952_NAME, 0},
>> + {}
>> +};
>> +
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>> + {LP3952_NAME, 0},
>> + {}
>> +};
>
> Could you please share how did you apply ACPI overlay?
I am using the initrd ACPI method of Octavian's patch.
https://lkml.org/lkml/2016/3/31/333
>
>> +
>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>> +#endif
>> +
>> +static struct i2c_driver lp3952_i2c_driver = {
>> + .driver = {
>> + .name = LP3952_NAME,
>> + .owner = THIS_MODULE,
>> +#ifdef CONFIG_ACPI
>> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
>> +#endif
>> + },
>> + .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..3ea120a
>> --- /dev/null
>> +++ b/include/linux/leds-lp3952.h
>> @@ -0,0 +1,75 @@
>> +/*
>> + * 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_
>> +
>> +/* ACPI iasl compiler wont take name LP3952,
>> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
>> + */
>> +#define LP3952_NAME "TLP3952"
>> +#define LP3952_CMD_REG_COUNT 8
>> +#define LP3952_BRIGHT_MAX 4
>> +
>> +/* 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
>> +};
>> +
>> +#endif /* LEDS_LP3952_H_ */
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-09 15:20 ` Tony Makkiel
@ 2016-06-10 8:26 ` Jacek Anaszewski
2016-06-10 11:39 ` Tony Makkiel
0 siblings, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2016-06-10 8:26 UTC (permalink / raw)
To: Tony Makkiel; +Cc: linux-leds, rpurdie, rjw, lenb
Hi Tony,
On 06/09/2016 05:20 PM, Tony Makkiel wrote:
>
>
> On 09/06/16 13:11, Jacek Anaszewski wrote:
>> Hi Tony,
>>
>> Thanks for the updated patch. I have some comments in the code below.
>>
>> On 06/09/2016 12:46 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 | 359 ++++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/leds-lp3952.h | 75 +++++++++
>>> 4 files changed, 447 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..a621bda
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-lp3952.c
>>> @@ -0,0 +1,359 @@
>>> +/*
>>> + * Copyright (c) 2016, DAQRI, LLC.
>>> + *
>>> + * License Terms: GNU General Public License v2
>>
>> You didn't address my remarks regarding GPL license
>> boilerplate. Is it intentional?
>>
>
> Sorry, I couldn't find the comment on license
> boiler plate. Can you please remind me what I missed?
Ah, sorry I intended to ask for that but eventually forgot.
Please use following boilerplate:
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.
>>> + *
>>> + * 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.
>>> + *
>>> + */
And drop this, because here you seem to mention GPL, and above
GPL v2.
>>> +#include <linux/acpi.h>
>>> +#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
>>> +
>>> +
>>> +#define LP3952_PATRN_LOOP BIT(1)
>>> +#define LP3952_PATRN_GEN_EN BIT(2)
>>> +#define LP3952_ACTIVE_MODE BIT(6)
>>> +#define LP3952_RGB_SEL_MASK (BIT(0) | BIT(1))
>>
>> #define LP3952_RGB_SEL_MASK 0x03
>>
>>> +#define LP3952_LED_MASK_ALL 0x3f
>>
>> Please put all the macro values in the same column.
> Will do
>>
>> Even though you don't use all features of the device, it is a good
>> practice to provide definitions for all bits and in all registers.
>>
>> Also, since you are providing header file, please move all macro
>> definitions and structures there.
> will do.
>>
>>> +
>>> +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;
>>> + struct regmap *regmap;
>>> + struct i2c_client *client;
>>> + struct gpio_desc *enable_gpio;
>>> + 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_id, int on)
>>> +{
>>> + int ret, val;
>>> +
>>> + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led_id, on);
>>> +
>>> + val = 1 << led_id;
>>> + if (led_id == LP3952_LED_ALL)
>>> + val = LP3952_LED_MASK_ALL;
>>> +
>>> + 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, 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;
>>> + }
>>> +
>>> + 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;
>>> + }
>>> +
>>> + /* 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,
>>> + --value << shift_val));
>>> +}
>>> +
>>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>>> +{
>>> + int i, ret = -ENODEV;
>>> + 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);
>>> + break;
>>> + }
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> +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;
>>
>> Why actually do you need pgm array? It is accessed only here during
>> assignment.
>
> The chip can support upto 8 commands. We are using only 1 command (Chip maintains
> settings of last command). But if somebody in the future wants to utilize the
> whole 8 command sets (say to, have custom lighting effects), they
> can program so using the pgm array(with this function). But for normal operation
> just 1 array/command would do.
But currently it is completely useless - you're only assigning the value
here, and it seems not to be accessed in any other place.
>>
>>> + 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,
>>> + LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN);
>>> + 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,
>>> + LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK,
>>> + LP3952_ACTIVE_MODE);
>>
>> Why LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK? Mask argument should
>> match the bit field to be overwritten with the new value.
>
> We have to make sure 3 bits have specific values.
>
> Bit 6(LP3952_ACTIVE_MODE) needs to be high.
> Bit [1:0] decides which leds needs to be controlled. It needs to be set
> 0 for both LED sets to follow Pattern gen.
regmap_update_bits() is useful if we have caching enabled, so as not to
be forced to remember the current register state, and update only
selected bits. In this case regmap_write() seems to be more suitable.
>>
>>> + 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));
>>> +}
>>
>> Could you explain please, why the pattern has to be set while only
>> fixed brightness setting is supported by the driver?
>>
> Note we are only setting Command 1 as mentioned in my comment above.
> Pattern gen loops on this command. This command sets the intensity
> and transition times. Without setting this register, the LEDs do
> not turn on.
What role does the transition time play? Why is it required to be set
while we are interested in continuous LED glowing?
>> Do you plan to add support for setting blinking patterns in the future?
>
> Not sure, The driver works as it is for normal led operations.
>
> This can be easily added using device_attribute with help of
> "lp3952_set_pattern_gen_cmd".
>
>>
>>> +static const struct regmap_config lp3952_regmap = {
>>> + .reg_bits = 8,
>>> + .val_bits = 8,
>>> + .max_register = REG_MAX,
>>> +};
>>
>> You're not enabling regmap cache so regmap_update_bits(), will perform
>> I2C readout each time. Is it intentional?
>>
> Thank you for the pointer. I did not look into it. Did not expect
> lot of traffic unless there is software blink.
> I will find out more about caching mechanism and add an
> appropriate cache type.
>
>>> +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;
>>> +
>>> + dev_dbg(&client->dev, "%s\n", __func__);
>>> +
>>> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>> +
>>> + priv->ndev = LP3952_LED_ALL;
>>> +
>>> + 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 = devm_gpiod_get(&client->dev, NULL, GPIOD_OUT_HIGH);
>>
>> How the driver knows which GPIO should it acquire? You're passing NULL
>> as the second argument.
>
> In the ACPI asl file, there is only one GPIO associated for lp3952 node.
>
> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
> IoRestrictionOutputOnly, "\\_SB.GPO2",
> 0x00, ResourceConsumer, , )
Doesn't devm_gpiod_get need to be passed some identifier if only one
GPIO is provided in the ACPI asl file? How would it look like in case
there was more then one GPIO provided in the file?
>>
>>> + if (IS_ERR(priv->enable_gpio)) {
>>> + status = PTR_ERR(priv->enable_gpio);
>>> + dev_err(&client->dev, "Failed to enable gpio: %d\n", status);
>>> + return status;
>>> + }
>>> +
>>> + 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;
>>> + }
>>> +
>>> + i2c_set_clientdata(client, priv);
>>> +
>>> + status = lp3952_configure(priv);
>>> + if (status) {
>>> + dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
>>> + status);
>>> + return status;
>>> + }
>>> +
>>> + status = lp3952_register_led_classdev(priv);
>>> + if (status) {
>>> + dev_err(&client->dev, "Unable to register led_classdev: %d\n",
>>> + status);
>>> + 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);
>>> + gpiod_set_value(priv->enable_gpio, 0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id lp3952_id[] = {
>>> + {LP3952_NAME, 0},
>>> + {}
>>> +};
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>> + {LP3952_NAME, 0},
>>> + {}
>>> +};
>>
>> Could you please share how did you apply ACPI overlay?
>
> I am using the initrd ACPI method of Octavian's patch.
>
> https://lkml.org/lkml/2016/3/31/333
Thanks. Would it be possible to define entries per each LED
connected to the LED controller, similarly as it is in case
of Device Tree bindings?:
Documentation/devicetree/bindings/leds/common.txt
>>> +
>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>> +#endif
>>> +
>>> +static struct i2c_driver lp3952_i2c_driver = {
>>> + .driver = {
>>> + .name = LP3952_NAME,
>>> + .owner = THIS_MODULE,
>>> +#ifdef CONFIG_ACPI
>>> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
>>> +#endif
>>> + },
>>> + .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..3ea120a
>>> --- /dev/null
>>> +++ b/include/linux/leds-lp3952.h
>>> @@ -0,0 +1,75 @@
>>> +/*
>>> + * 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_
>>> +
>>> +/* ACPI iasl compiler wont take name LP3952,
>>> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
>>> + */
>>> +#define LP3952_NAME "TLP3952"
>>> +#define LP3952_CMD_REG_COUNT 8
>>> +#define LP3952_BRIGHT_MAX 4
>>> +
>>> +/* 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
>>> +};
>>> +
>>> +#endif /* LEDS_LP3952_H_ */
>>>
>>
>>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-10 8:26 ` Jacek Anaszewski
@ 2016-06-10 11:39 ` Tony Makkiel
2016-06-10 12:56 ` Tony Makkiel
2016-06-10 14:36 ` Jacek Anaszewski
0 siblings, 2 replies; 10+ messages in thread
From: Tony Makkiel @ 2016-06-10 11:39 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, rpurdie, rjw, lenb
On 10/06/16 09:26, Jacek Anaszewski wrote:
> Hi Tony,
>
> On 06/09/2016 05:20 PM, Tony Makkiel wrote:
>>
>>
>> On 09/06/16 13:11, Jacek Anaszewski wrote:
>>> Hi Tony,
>>>
>>> Thanks for the updated patch. I have some comments in the code below.
>>>
>>> On 06/09/2016 12:46 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 | 359 ++++++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/leds-lp3952.h | 75 +++++++++
>>>> 4 files changed, 447 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..a621bda
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-lp3952.c
>>>> @@ -0,0 +1,359 @@
>>>> +/*
>>>> + * Copyright (c) 2016, DAQRI, LLC.
>>>> + *
>>>> + * License Terms: GNU General Public License v2
>>>
>>> You didn't address my remarks regarding GPL license
>>> boilerplate. Is it intentional?
>>>
>>
>> Sorry, I couldn't find the comment on license
>> boiler plate. Can you please remind me what I missed?
>
> Ah, sorry I intended to ask for that but eventually forgot.
>
> Please use following boilerplate:
>
> 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.
>
Thank you, updated.
>>>> + *
>>>> + * 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.
>>>> + *
>>>> + */
>
> And drop this, because here you seem to mention GPL, and above
> GPL v2.
>
>>>> +#include <linux/acpi.h>
>>>> +#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
>>>> +
>>>> +
>>>> +#define LP3952_PATRN_LOOP BIT(1)
>>>> +#define LP3952_PATRN_GEN_EN BIT(2)
>>>> +#define LP3952_ACTIVE_MODE BIT(6)
>>>> +#define LP3952_RGB_SEL_MASK (BIT(0) | BIT(1))
>>>
>>> #define LP3952_RGB_SEL_MASK 0x03
>>>
>>>> +#define LP3952_LED_MASK_ALL 0x3f
>>>
>>> Please put all the macro values in the same column.
>> Will do
>>>
>>> Even though you don't use all features of the device, it is a good
>>> practice to provide definitions for all bits and in all registers.
>>>
>>> Also, since you are providing header file, please move all macro
>>> definitions and structures there.
>> will do.
>>>
>>>> +
>>>> +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;
>>>> + struct regmap *regmap;
>>>> + struct i2c_client *client;
>>>> + struct gpio_desc *enable_gpio;
>>>> + 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_id, int on)
>>>> +{
>>>> + int ret, val;
>>>> +
>>>> + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led_id, on);
>>>> +
>>>> + val = 1 << led_id;
>>>> + if (led_id == LP3952_LED_ALL)
>>>> + val = LP3952_LED_MASK_ALL;
>>>> +
>>>> + 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, 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;
>>>> + }
>>>> +
>>>> + 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;
>>>> + }
>>>> +
>>>> + /* 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,
>>>> + --value << shift_val));
>>>> +}
>>>> +
>>>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>>>> +{
>>>> + int i, ret = -ENODEV;
>>>> + 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);
>>>> + break;
>>>> + }
>>>> + }
>>>> + return ret;
>>>> +}
>>>> +
>>>> +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;
>>>
>>> Why actually do you need pgm array? It is accessed only here during
>>> assignment.
>>
>> The chip can support upto 8 commands. We are using only 1 command (Chip maintains
>> settings of last command). But if somebody in the future wants to utilize the
>> whole 8 command sets (say to, have custom lighting effects), they
>> can program so using the pgm array(with this function). But for normal operation
>> just 1 array/command would do.
>
> But currently it is completely useless - you're only assigning the value
> here, and it seems not to be accessed in any other place.
Yes, at present it is only called from 'lp3952_configure' to configure command 1.
But it can be useful for any one who wants to update the other 7 commands(say
using device_attribute).
But if you prefer to remove the arrays please let me know.
>
>>>
>>>> + 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,
>>>> + LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN);
>>>> + 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,
>>>> + LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK,
>>>> + LP3952_ACTIVE_MODE);
>>>
>>> Why LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK? Mask argument should
>>> match the bit field to be overwritten with the new value.
>>
>> We have to make sure 3 bits have specific values.
>>
>> Bit 6(LP3952_ACTIVE_MODE) needs to be high.
>> Bit [1:0] decides which leds needs to be controlled. It needs to be set
>> 0 for both LED sets to follow Pattern gen.
>
> regmap_update_bits() is useful if we have caching enabled, so as not to
> be forced to remember the current register state, and update only
> selected bits. In this case regmap_write() seems to be more suitable.
>
As you suggested in previous comment, I have enabled REGCACHE_RBTREE,
for regmap now.
>>>
>>>> + 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));
>>>> +}
>>>
>>> Could you explain please, why the pattern has to be set while only
>>> fixed brightness setting is supported by the driver?
>>>
>> Note we are only setting Command 1 as mentioned in my comment above.
>> Pattern gen loops on this command. This command sets the intensity
>> and transition times. Without setting this register, the LEDs do
>> not turn on.
>
> What role does the transition time play? Why is it required to be set
> while we are interested in continuous LED glowing?
I am not sure. I am assuming it is the on/off transition times. Without
setting at least one of the commands, the chip does not respond to any
led controls.
>
>>> Do you plan to add support for setting blinking patterns in the future?
>>
>> Not sure, The driver works as it is for normal led operations.
>>
>> This can be easily added using device_attribute with help of
>> "lp3952_set_pattern_gen_cmd".
>>
>>>
>>>> +static const struct regmap_config lp3952_regmap = {
>>>> + .reg_bits = 8,
>>>> + .val_bits = 8,
>>>> + .max_register = REG_MAX,
>>>> +};
>>>
>>> You're not enabling regmap cache so regmap_update_bits(), will perform
>>> I2C readout each time. Is it intentional?
>>>
>> Thank you for the pointer. I did not look into it. Did not expect
>> lot of traffic unless there is software blink.
>> I will find out more about caching mechanism and add an
>> appropriate cache type.
>>
>>>> +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;
>>>> +
>>>> + dev_dbg(&client->dev, "%s\n", __func__);
>>>> +
>>>> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>>> + if (!priv)
>>>> + return -ENOMEM;
>>>> +
>>>> + priv->ndev = LP3952_LED_ALL;
>>>> +
>>>> + 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 = devm_gpiod_get(&client->dev, NULL, GPIOD_OUT_HIGH);
>>>
>>> How the driver knows which GPIO should it acquire? You're passing NULL
>>> as the second argument.
>>
>> In the ACPI asl file, there is only one GPIO associated for lp3952 node.
>>
>> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>> IoRestrictionOutputOnly, "\\_SB.GPO2",
>> 0x00, ResourceConsumer, , )
>
> Doesn't devm_gpiod_get need to be passed some identifier if only one
> GPIO is provided in the ACPI asl file? How would it look like in case
> there was more then one GPIO provided in the file?
>
It works without name, for 1 gpio.
If there were more, one of the 2 methods in
Documentation/acpi/gpio-properties.txt
should be used.
>>>
>>>> + if (IS_ERR(priv->enable_gpio)) {
>>>> + status = PTR_ERR(priv->enable_gpio);
>>>> + dev_err(&client->dev, "Failed to enable gpio: %d\n", status);
>>>> + return status;
>>>> + }
>>>> +
>>>> + 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;
>>>> + }
>>>> +
>>>> + i2c_set_clientdata(client, priv);
>>>> +
>>>> + status = lp3952_configure(priv);
>>>> + if (status) {
>>>> + dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
>>>> + status);
>>>> + return status;
>>>> + }
>>>> +
>>>> + status = lp3952_register_led_classdev(priv);
>>>> + if (status) {
>>>> + dev_err(&client->dev, "Unable to register led_classdev: %d\n",
>>>> + status);
>>>> + 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);
>>>> + gpiod_set_value(priv->enable_gpio, 0);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct i2c_device_id lp3952_id[] = {
>>>> + {LP3952_NAME, 0},
>>>> + {}
>>>> +};
>>>> +
>>>> +#ifdef CONFIG_ACPI
>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>> + {LP3952_NAME, 0},
>>>> + {}
>>>> +};
>>>
>>> Could you please share how did you apply ACPI overlay?
>>
>> I am using the initrd ACPI method of Octavian's patch.
>>
>> https://lkml.org/lkml/2016/3/31/333
>
> Thanks. Would it be possible to define entries per each LED
> connected to the LED controller, similarly as it is in case
> of Device Tree bindings?:
>
> Documentation/devicetree/bindings/leds/common.txt
>
I am not sure. I did a quick skim through ACPI Spec 5.0.
But couldn't find anything.
>>>> +
>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>> +#endif
>>>> +
>>>> +static struct i2c_driver lp3952_i2c_driver = {
>>>> + .driver = {
>>>> + .name = LP3952_NAME,
>>>> + .owner = THIS_MODULE,
>>>> +#ifdef CONFIG_ACPI
>>>> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
>>>> +#endif
>>>> + },
>>>> + .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..3ea120a
>>>> --- /dev/null
>>>> +++ b/include/linux/leds-lp3952.h
>>>> @@ -0,0 +1,75 @@
>>>> +/*
>>>> + * 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_
>>>> +
>>>> +/* ACPI iasl compiler wont take name LP3952,
>>>> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
>>>> + */
>>>> +#define LP3952_NAME "TLP3952"
>>>> +#define LP3952_CMD_REG_COUNT 8
>>>> +#define LP3952_BRIGHT_MAX 4
>>>> +
>>>> +/* 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
>>>> +};
>>>> +
>>>> +#endif /* LEDS_LP3952_H_ */
>>>>
>>>
>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-10 11:39 ` Tony Makkiel
@ 2016-06-10 12:56 ` Tony Makkiel
2016-06-10 14:36 ` Jacek Anaszewski
2016-06-10 14:36 ` Jacek Anaszewski
1 sibling, 1 reply; 10+ messages in thread
From: Tony Makkiel @ 2016-06-10 12:56 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, rpurdie, rjw, lenb
>>>>> +
>>>>> +#ifdef CONFIG_ACPI
>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>> + {LP3952_NAME, 0},
>>>>> + {}
>>>>> +};
>>>>
>>>> Could you please share how did you apply ACPI overlay?
>>>
>>> I am using the initrd ACPI method of Octavian's patch.
>>>
>>> https://lkml.org/lkml/2016/3/31/333
>>
>> Thanks. Would it be possible to define entries per each LED
>> connected to the LED controller, similarly as it is in case
>> of Device Tree bindings?:
>>
>> Documentation/devicetree/bindings/leds/common.txt
>>
>
> I am not sure. I did a quick skim through ACPI Spec 5.0.
> But couldn't find anything.
Found it!, although not relevant for this driver. Please
see last page of
http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
>
>>>>> +
>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>> +#endif
>>>>> +
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-10 11:39 ` Tony Makkiel
2016-06-10 12:56 ` Tony Makkiel
@ 2016-06-10 14:36 ` Jacek Anaszewski
2016-06-10 15:18 ` Tony Makkiel
1 sibling, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2016-06-10 14:36 UTC (permalink / raw)
To: Tony Makkiel; +Cc: linux-leds, rpurdie, rjw, lenb
On 06/10/2016 01:39 PM, Tony Makkiel wrote:
>
>
> On 10/06/16 09:26, Jacek Anaszewski wrote:
>> Hi Tony,
>>
>> On 06/09/2016 05:20 PM, Tony Makkiel wrote:
>>>
>>>
>>> On 09/06/16 13:11, Jacek Anaszewski wrote:
>>>> Hi Tony,
>>>>
>>>> Thanks for the updated patch. I have some comments in the code below.
>>>>
>>>> On 06/09/2016 12:46 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 | 359 ++++++++++++++++++++++++++++++++++++++++++++
>>>>> include/linux/leds-lp3952.h | 75 +++++++++
>>>>> 4 files changed, 447 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..a621bda
>>>>> --- /dev/null
>>>>> +++ b/drivers/leds/leds-lp3952.c
>>>>> @@ -0,0 +1,359 @@
>>>>> +/*
>>>>> + * Copyright (c) 2016, DAQRI, LLC.
>>>>> + *
>>>>> + * License Terms: GNU General Public License v2
>>>>
>>>> You didn't address my remarks regarding GPL license
>>>> boilerplate. Is it intentional?
>>>>
>>>
>>> Sorry, I couldn't find the comment on license
>>> boiler plate. Can you please remind me what I missed?
>>
>> Ah, sorry I intended to ask for that but eventually forgot.
>>
>> Please use following boilerplate:
>>
>> 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.
>>
> Thank you, updated.
>>>>> + *
>>>>> + * 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.
>>>>> + *
>>>>> + */
>>
>> And drop this, because here you seem to mention GPL, and above
>> GPL v2.
>>
>>>>> +#include <linux/acpi.h>
>>>>> +#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
>>>>> +
>>>>> +
>>>>> +#define LP3952_PATRN_LOOP BIT(1)
>>>>> +#define LP3952_PATRN_GEN_EN BIT(2)
>>>>> +#define LP3952_ACTIVE_MODE BIT(6)
>>>>> +#define LP3952_RGB_SEL_MASK (BIT(0) | BIT(1))
>>>>
>>>> #define LP3952_RGB_SEL_MASK 0x03
>>>>
>>>>> +#define LP3952_LED_MASK_ALL 0x3f
>>>>
>>>> Please put all the macro values in the same column.
>>> Will do
>>>>
>>>> Even though you don't use all features of the device, it is a good
>>>> practice to provide definitions for all bits and in all registers.
>>>>
>>>> Also, since you are providing header file, please move all macro
>>>> definitions and structures there.
>>> will do.
>>>>
>>>>> +
>>>>> +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;
>>>>> + struct regmap *regmap;
>>>>> + struct i2c_client *client;
>>>>> + struct gpio_desc *enable_gpio;
>>>>> + 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_id, int on)
>>>>> +{
>>>>> + int ret, val;
>>>>> +
>>>>> + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led_id, on);
>>>>> +
>>>>> + val = 1 << led_id;
>>>>> + if (led_id == LP3952_LED_ALL)
>>>>> + val = LP3952_LED_MASK_ALL;
>>>>> +
>>>>> + 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, 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;
>>>>> + }
>>>>> +
>>>>> + 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;
>>>>> + }
>>>>> +
>>>>> + /* 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,
>>>>> + --value << shift_val));
>>>>> +}
>>>>> +
>>>>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>>>>> +{
>>>>> + int i, ret = -ENODEV;
>>>>> + 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);
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +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;
>>>>
>>>> Why actually do you need pgm array? It is accessed only here during
>>>> assignment.
>>>
>>> The chip can support upto 8 commands. We are using only 1 command (Chip maintains
>>> settings of last command). But if somebody in the future wants to utilize the
>>> whole 8 command sets (say to, have custom lighting effects), they
>>> can program so using the pgm array(with this function). But for normal operation
>>> just 1 array/command would do.
>>
>> But currently it is completely useless - you're only assigning the value
>> here, and it seems not to be accessed in any other place.
>
> Yes, at present it is only called from 'lp3952_configure' to configure command 1.
> But it can be useful for any one who wants to update the other 7 commands(say
> using device_attribute).
>
> But if you prefer to remove the arrays please let me know.
From what you're saying the function itself seems to be useful,
contrarily to the array.
>>
>>>>
>>>>> + 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,
>>>>> + LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN);
>>>>> + 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,
>>>>> + LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK,
>>>>> + LP3952_ACTIVE_MODE);
>>>>
>>>> Why LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK? Mask argument should
>>>> match the bit field to be overwritten with the new value.
>>>
>>> We have to make sure 3 bits have specific values.
>>>
>>> Bit 6(LP3952_ACTIVE_MODE) needs to be high.
>>> Bit [1:0] decides which leds needs to be controlled. It needs to be set
>>> 0 for both LED sets to follow Pattern gen.
>>
>> regmap_update_bits() is useful if we have caching enabled, so as not to
>> be forced to remember the current register state, and update only
>> selected bits. In this case regmap_write() seems to be more suitable.
>>
> As you suggested in previous comment, I have enabled REGCACHE_RBTREE,
> for regmap now.
But still the regmap_update_bits() is of help only when updating
register state and not when initializing it. Please change it to
regmap_write().
>>>>> + 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));
>>>>> +}
>>>>
>>>> Could you explain please, why the pattern has to be set while only
>>>> fixed brightness setting is supported by the driver?
>>>>
>>> Note we are only setting Command 1 as mentioned in my comment above.
>>> Pattern gen loops on this command. This command sets the intensity
>>> and transition times. Without setting this register, the LEDs do
>>> not turn on.
>>
>> What role does the transition time play? Why is it required to be set
>> while we are interested in continuous LED glowing?
>
> I am not sure. I am assuming it is the on/off transition times. Without
> setting at least one of the commands, the chip does not respond to any
> led controls.
ack.
>>
>>>> Do you plan to add support for setting blinking patterns in the future?
>>>
>>> Not sure, The driver works as it is for normal led operations.
>>>
>>> This can be easily added using device_attribute with help of
>>> "lp3952_set_pattern_gen_cmd".
>>>
>>>>
>>>>> +static const struct regmap_config lp3952_regmap = {
>>>>> + .reg_bits = 8,
>>>>> + .val_bits = 8,
>>>>> + .max_register = REG_MAX,
>>>>> +};
>>>>
>>>> You're not enabling regmap cache so regmap_update_bits(), will perform
>>>> I2C readout each time. Is it intentional?
>>>>
>>> Thank you for the pointer. I did not look into it. Did not expect
>>> lot of traffic unless there is software blink.
>>> I will find out more about caching mechanism and add an
>>> appropriate cache type.
>>>
>>>>> +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;
>>>>> +
>>>>> + dev_dbg(&client->dev, "%s\n", __func__);
>>>>> +
>>>>> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>>>> + if (!priv)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + priv->ndev = LP3952_LED_ALL;
>>>>> +
>>>>> + 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 = devm_gpiod_get(&client->dev, NULL, GPIOD_OUT_HIGH);
>>>>
>>>> How the driver knows which GPIO should it acquire? You're passing NULL
>>>> as the second argument.
>>>
>>> In the ACPI asl file, there is only one GPIO associated for lp3952 node.
>>>
>>> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>>> IoRestrictionOutputOnly, "\\_SB.GPO2",
>>> 0x00, ResourceConsumer, , )
>>
>> Doesn't devm_gpiod_get need to be passed some identifier if only one
>> GPIO is provided in the ACPI asl file? How would it look like in case
>> there was more then one GPIO provided in the file?
>>
>
> It works without name, for 1 gpio.
>
> If there were more, one of the 2 methods in
> Documentation/acpi/gpio-properties.txt
> should be used.
Thanks for the reference.
>>>>
>>>>> + if (IS_ERR(priv->enable_gpio)) {
>>>>> + status = PTR_ERR(priv->enable_gpio);
>>>>> + dev_err(&client->dev, "Failed to enable gpio: %d\n", status);
>>>>> + return status;
>>>>> + }
>>>>> +
>>>>> + 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;
>>>>> + }
>>>>> +
>>>>> + i2c_set_clientdata(client, priv);
>>>>> +
>>>>> + status = lp3952_configure(priv);
>>>>> + if (status) {
>>>>> + dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
>>>>> + status);
>>>>> + return status;
>>>>> + }
>>>>> +
>>>>> + status = lp3952_register_led_classdev(priv);
>>>>> + if (status) {
>>>>> + dev_err(&client->dev, "Unable to register led_classdev: %d\n",
>>>>> + status);
>>>>> + 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);
>>>>> + gpiod_set_value(priv->enable_gpio, 0);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct i2c_device_id lp3952_id[] = {
>>>>> + {LP3952_NAME, 0},
>>>>> + {}
>>>>> +};
>>>>> +
>>>>> +#ifdef CONFIG_ACPI
>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>> + {LP3952_NAME, 0},
>>>>> + {}
>>>>> +};
>>>>
>>>> Could you please share how did you apply ACPI overlay?
>>>
>>> I am using the initrd ACPI method of Octavian's patch.
>>>
>>> https://lkml.org/lkml/2016/3/31/333
>>
>> Thanks. Would it be possible to define entries per each LED
>> connected to the LED controller, similarly as it is in case
>> of Device Tree bindings?:
>>
>> Documentation/devicetree/bindings/leds/common.txt
>>
>
> I am not sure. I did a quick skim through ACPI Spec 5.0.
> But couldn't find anything.
>
>>>>> +
>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>> +#endif
>>>>> +
>>>>> +static struct i2c_driver lp3952_i2c_driver = {
>>>>> + .driver = {
>>>>> + .name = LP3952_NAME,
>>>>> + .owner = THIS_MODULE,
>>>>> +#ifdef CONFIG_ACPI
>>>>> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
>>>>> +#endif
>>>>> + },
>>>>> + .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..3ea120a
>>>>> --- /dev/null
>>>>> +++ b/include/linux/leds-lp3952.h
>>>>> @@ -0,0 +1,75 @@
>>>>> +/*
>>>>> + * 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_
>>>>> +
>>>>> +/* ACPI iasl compiler wont take name LP3952,
>>>>> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
>>>>> + */
>>>>> +#define LP3952_NAME "TLP3952"
>>>>> +#define LP3952_CMD_REG_COUNT 8
>>>>> +#define LP3952_BRIGHT_MAX 4
>>>>> +
>>>>> +/* 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
>>>>> +};
>>>>> +
>>>>> +#endif /* LEDS_LP3952_H_ */
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-10 12:56 ` Tony Makkiel
@ 2016-06-10 14:36 ` Jacek Anaszewski
2016-06-10 15:22 ` Tony Makkiel
0 siblings, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2016-06-10 14:36 UTC (permalink / raw)
To: Tony Makkiel; +Cc: linux-leds, rpurdie, rjw, lenb
On 06/10/2016 02:56 PM, Tony Makkiel wrote:
>
>
>
>>>>>> +
>>>>>> +#ifdef CONFIG_ACPI
>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>> + {LP3952_NAME, 0},
>>>>>> + {}
>>>>>> +};
>>>>>
>>>>> Could you please share how did you apply ACPI overlay?
>>>>
>>>> I am using the initrd ACPI method of Octavian's patch.
>>>>
>>>> https://lkml.org/lkml/2016/3/31/333
>>>
>>> Thanks. Would it be possible to define entries per each LED
>>> connected to the LED controller, similarly as it is in case
>>> of Device Tree bindings?:
>>>
>>> Documentation/devicetree/bindings/leds/common.txt
>>>
>>
>> I am not sure. I did a quick skim through ACPI Spec 5.0.
>> But couldn't find anything.
>
> Found it!, although not relevant for this driver. Please
> see last page of
> http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
Great. The "Package" entries seem to use names compatible with the
Device Tree properties documented in
Documentation/devicetree/bindings/leds/common.txt.
Probably you could also provide LED name in the "Name" entry and obtain
them in the driver directly from ACPI, similarly as it is accomplished
in case of Device Tree.
led_name array from lp3952_register_led_classdev() could be removed
then.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-10 14:36 ` Jacek Anaszewski
@ 2016-06-10 15:18 ` Tony Makkiel
0 siblings, 0 replies; 10+ messages in thread
From: Tony Makkiel @ 2016-06-10 15:18 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds
On 10/06/16 15:36, Jacek Anaszewski wrote:
> On 06/10/2016 01:39 PM, Tony Makkiel wrote:
>>
>>
>> On 10/06/16 09:26, Jacek Anaszewski wrote:
>>> Hi Tony,
>>>
>>> On 06/09/2016 05:20 PM, Tony Makkiel wrote:
>>>>
>>>>
>>>> On 09/06/16 13:11, Jacek Anaszewski wrote:
>>>>> Hi Tony,
>>>>>
>>>>> Thanks for the updated patch. I have some comments in the code below.
>>>>>
>>>>> On 06/09/2016 12:46 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 | 359 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>> include/linux/leds-lp3952.h | 75 +++++++++
>>>>>> 4 files changed, 447 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..a621bda
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/leds/leds-lp3952.c
>>>>>> @@ -0,0 +1,359 @@
>>>>>> +/*
>>>>>> + * Copyright (c) 2016, DAQRI, LLC.
>>>>>> + *
>>>>>> + * License Terms: GNU General Public License v2
>>>>>
>>>>> You didn't address my remarks regarding GPL license
>>>>> boilerplate. Is it intentional?
>>>>>
>>>>
>>>> Sorry, I couldn't find the comment on license
>>>> boiler plate. Can you please remind me what I missed?
>>>
>>> Ah, sorry I intended to ask for that but eventually forgot.
>>>
>>> Please use following boilerplate:
>>>
>>> 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.
>>>
>> Thank you, updated.
>>>>>> + *
>>>>>> + * 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.
>>>>>> + *
>>>>>> + */
>>>
>>> And drop this, because here you seem to mention GPL, and above
>>> GPL v2.
>>>
>>>>>> +#include <linux/acpi.h>
>>>>>> +#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
>>>>>> +
>>>>>> +
>>>>>> +#define LP3952_PATRN_LOOP BIT(1)
>>>>>> +#define LP3952_PATRN_GEN_EN BIT(2)
>>>>>> +#define LP3952_ACTIVE_MODE BIT(6)
>>>>>> +#define LP3952_RGB_SEL_MASK (BIT(0) | BIT(1))
>>>>>
>>>>> #define LP3952_RGB_SEL_MASK 0x03
>>>>>
>>>>>> +#define LP3952_LED_MASK_ALL 0x3f
>>>>>
>>>>> Please put all the macro values in the same column.
>>>> Will do
>>>>>
>>>>> Even though you don't use all features of the device, it is a good
>>>>> practice to provide definitions for all bits and in all registers.
>>>>>
>>>>> Also, since you are providing header file, please move all macro
>>>>> definitions and structures there.
>>>> will do.
>>>>>
>>>>>> +
>>>>>> +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;
>>>>>> + struct regmap *regmap;
>>>>>> + struct i2c_client *client;
>>>>>> + struct gpio_desc *enable_gpio;
>>>>>> + 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_id, int on)
>>>>>> +{
>>>>>> + int ret, val;
>>>>>> +
>>>>>> + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led_id, on);
>>>>>> +
>>>>>> + val = 1 << led_id;
>>>>>> + if (led_id == LP3952_LED_ALL)
>>>>>> + val = LP3952_LED_MASK_ALL;
>>>>>> +
>>>>>> + 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, 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;
>>>>>> + }
>>>>>> +
>>>>>> + 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;
>>>>>> + }
>>>>>> +
>>>>>> + /* 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,
>>>>>> + --value << shift_val));
>>>>>> +}
>>>>>> +
>>>>>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>>>>>> +{
>>>>>> + int i, ret = -ENODEV;
>>>>>> + 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);
>>>>>> + break;
>>>>>> + }
>>>>>> + }
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +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;
>>>>>
>>>>> Why actually do you need pgm array? It is accessed only here during
>>>>> assignment.
>>>>
>>>> The chip can support upto 8 commands. We are using only 1 command (Chip maintains
>>>> settings of last command). But if somebody in the future wants to utilize the
>>>> whole 8 command sets (say to, have custom lighting effects), they
>>>> can program so using the pgm array(with this function). But for normal operation
>>>> just 1 array/command would do.
>>>
>>> But currently it is completely useless - you're only assigning the value
>>> here, and it seems not to be accessed in any other place.
>>
>> Yes, at present it is only called from 'lp3952_configure' to configure command 1.
>> But it can be useful for any one who wants to update the other 7 commands(say
>> using device_attribute).
>>
>> But if you prefer to remove the arrays please let me know.
>
> From what you're saying the function itself seems to be useful,
> contrarily to the array.
I have been missing the point all along, Sorry. The array was left for
the show function in device_attribute. But as, it is not there yet,
will remove the array, in next patch version.
>
>>>
>>>>>
>>>>>> + 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,
>>>>>> + LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN);
>>>>>> + 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,
>>>>>> + LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK,
>>>>>> + LP3952_ACTIVE_MODE);
>>>>>
>>>>> Why LP3952_ACTIVE_MODE | LP3952_RGB_SEL_MASK? Mask argument should
>>>>> match the bit field to be overwritten with the new value.
>>>>
>>>> We have to make sure 3 bits have specific values.
>>>>
>>>> Bit 6(LP3952_ACTIVE_MODE) needs to be high.
>>>> Bit [1:0] decides which leds needs to be controlled. It needs to be set
>>>> 0 for both LED sets to follow Pattern gen.
>>>
>>> regmap_update_bits() is useful if we have caching enabled, so as not to
>>> be forced to remember the current register state, and update only
>>> selected bits. In this case regmap_write() seems to be more suitable.
>>>
>> As you suggested in previous comment, I have enabled REGCACHE_RBTREE,
>> for regmap now.
>
> But still the regmap_update_bits() is of help only when updating
> register state and not when initializing it. Please change it to
> regmap_write().
>
Will do
>
>>>>>> + 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));
>>>>>> +}
>>>>>
>>>>> Could you explain please, why the pattern has to be set while only
>>>>> fixed brightness setting is supported by the driver?
>>>>>
>>>> Note we are only setting Command 1 as mentioned in my comment above.
>>>> Pattern gen loops on this command. This command sets the intensity
>>>> and transition times. Without setting this register, the LEDs do
>>>> not turn on.
>>>
>>> What role does the transition time play? Why is it required to be set
>>> while we are interested in continuous LED glowing?
>>
>> I am not sure. I am assuming it is the on/off transition times. Without
>> setting at least one of the commands, the chip does not respond to any
>> led controls.
>
> ack.
>
>>>
>>>>> Do you plan to add support for setting blinking patterns in the future?
>>>>
>>>> Not sure, The driver works as it is for normal led operations.
>>>>
>>>> This can be easily added using device_attribute with help of
>>>> "lp3952_set_pattern_gen_cmd".
>>>>
>>>>>
>>>>>> +static const struct regmap_config lp3952_regmap = {
>>>>>> + .reg_bits = 8,
>>>>>> + .val_bits = 8,
>>>>>> + .max_register = REG_MAX,
>>>>>> +};
>>>>>
>>>>> You're not enabling regmap cache so regmap_update_bits(), will perform
>>>>> I2C readout each time. Is it intentional?
>>>>>
>>>> Thank you for the pointer. I did not look into it. Did not expect
>>>> lot of traffic unless there is software blink.
>>>> I will find out more about caching mechanism and add an
>>>> appropriate cache type.
>>>>
>>>>>> +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;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "%s\n", __func__);
>>>>>> +
>>>>>> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>>>>> + if (!priv)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + priv->ndev = LP3952_LED_ALL;
>>>>>> +
>>>>>> + 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 = devm_gpiod_get(&client->dev, NULL, GPIOD_OUT_HIGH);
>>>>>
>>>>> How the driver knows which GPIO should it acquire? You're passing NULL
>>>>> as the second argument.
>>>>
>>>> In the ACPI asl file, there is only one GPIO associated for lp3952 node.
>>>>
>>>> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>>>> IoRestrictionOutputOnly, "\\_SB.GPO2",
>>>> 0x00, ResourceConsumer, , )
>>>
>>> Doesn't devm_gpiod_get need to be passed some identifier if only one
>>> GPIO is provided in the ACPI asl file? How would it look like in case
>>> there was more then one GPIO provided in the file?
>>>
>>
>> It works without name, for 1 gpio.
>>
>> If there were more, one of the 2 methods in
>> Documentation/acpi/gpio-properties.txt
>> should be used.
>
> Thanks for the reference.
>
>>>>>
>>>>>> + if (IS_ERR(priv->enable_gpio)) {
>>>>>> + status = PTR_ERR(priv->enable_gpio);
>>>>>> + dev_err(&client->dev, "Failed to enable gpio: %d\n", status);
>>>>>> + return status;
>>>>>> + }
>>>>>> +
>>>>>> + 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;
>>>>>> + }
>>>>>> +
>>>>>> + i2c_set_clientdata(client, priv);
>>>>>> +
>>>>>> + status = lp3952_configure(priv);
>>>>>> + if (status) {
>>>>>> + dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
>>>>>> + status);
>>>>>> + return status;
>>>>>> + }
>>>>>> +
>>>>>> + status = lp3952_register_led_classdev(priv);
>>>>>> + if (status) {
>>>>>> + dev_err(&client->dev, "Unable to register led_classdev: %d\n",
>>>>>> + status);
>>>>>> + 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);
>>>>>> + gpiod_set_value(priv->enable_gpio, 0);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct i2c_device_id lp3952_id[] = {
>>>>>> + {LP3952_NAME, 0},
>>>>>> + {}
>>>>>> +};
>>>>>> +
>>>>>> +#ifdef CONFIG_ACPI
>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>> + {LP3952_NAME, 0},
>>>>>> + {}
>>>>>> +};
>>>>>
>>>>> Could you please share how did you apply ACPI overlay?
>>>>
>>>> I am using the initrd ACPI method of Octavian's patch.
>>>>
>>>> https://lkml.org/lkml/2016/3/31/333
>>>
>>> Thanks. Would it be possible to define entries per each LED
>>> connected to the LED controller, similarly as it is in case
>>> of Device Tree bindings?:
>>>
>>> Documentation/devicetree/bindings/leds/common.txt
>>>
>>
>> I am not sure. I did a quick skim through ACPI Spec 5.0.
>> But couldn't find anything.
>>
>>>>>> +
>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>>> +#endif
>>>>>> +
>>>>>> +static struct i2c_driver lp3952_i2c_driver = {
>>>>>> + .driver = {
>>>>>> + .name = LP3952_NAME,
>>>>>> + .owner = THIS_MODULE,
>>>>>> +#ifdef CONFIG_ACPI
>>>>>> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
>>>>>> +#endif
>>>>>> + },
>>>>>> + .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..3ea120a
>>>>>> --- /dev/null
>>>>>> +++ b/include/linux/leds-lp3952.h
>>>>>> @@ -0,0 +1,75 @@
>>>>>> +/*
>>>>>> + * 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_
>>>>>> +
>>>>>> +/* ACPI iasl compiler wont take name LP3952,
>>>>>> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
>>>>>> + */
>>>>>> +#define LP3952_NAME "TLP3952"
>>>>>> +#define LP3952_CMD_REG_COUNT 8
>>>>>> +#define LP3952_BRIGHT_MAX 4
>>>>>> +
>>>>>> +/* 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
>>>>>> +};
>>>>>> +
>>>>>> +#endif /* LEDS_LP3952_H_ */
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-10 14:36 ` Jacek Anaszewski
@ 2016-06-10 15:22 ` Tony Makkiel
0 siblings, 0 replies; 10+ messages in thread
From: Tony Makkiel @ 2016-06-10 15:22 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, rpurdie, rjw, lenb
On 10/06/16 15:36, Jacek Anaszewski wrote:
> On 06/10/2016 02:56 PM, Tony Makkiel wrote:
>>
>>
>>
>>>>>>> +
>>>>>>> +#ifdef CONFIG_ACPI
>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>>> + {LP3952_NAME, 0},
>>>>>>> + {}
>>>>>>> +};
>>>>>>
>>>>>> Could you please share how did you apply ACPI overlay?
>>>>>
>>>>> I am using the initrd ACPI method of Octavian's patch.
>>>>>
>>>>> https://lkml.org/lkml/2016/3/31/333
>>>>
>>>> Thanks. Would it be possible to define entries per each LED
>>>> connected to the LED controller, similarly as it is in case
>>>> of Device Tree bindings?:
>>>>
>>>> Documentation/devicetree/bindings/leds/common.txt
>>>>
>>>
>>> I am not sure. I did a quick skim through ACPI Spec 5.0.
>>> But couldn't find anything.
>>
>> Found it!, although not relevant for this driver. Please
>> see last page of
>> http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
>
> Great. The "Package" entries seem to use names compatible with the
> Device Tree properties documented in
> Documentation/devicetree/bindings/leds/common.txt.
>
> Probably you could also provide LED name in the "Name" entry and obtain
> them in the driver directly from ACPI, similarly as it is accomplished
> in case of Device Tree.
>
> led_name array from lp3952_register_led_classdev() could be removed
> then.
I don't think it is possible yet, maybe once there is an updated iasl compiler.
The current version does not even recognize _DSD.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-10 15:22 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-09 10:46 [PATCH v3 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel
2016-06-09 12:11 ` Jacek Anaszewski
2016-06-09 15:20 ` Tony Makkiel
2016-06-10 8:26 ` Jacek Anaszewski
2016-06-10 11:39 ` Tony Makkiel
2016-06-10 12:56 ` Tony Makkiel
2016-06-10 14:36 ` Jacek Anaszewski
2016-06-10 15:22 ` Tony Makkiel
2016-06-10 14:36 ` Jacek Anaszewski
2016-06-10 15:18 ` Tony Makkiel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.