* [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
@ 2016-07-05 15:30 Tony Makkiel
2016-07-06 8:23 ` Mika Westerberg
2016-07-06 13:33 ` Jacek Anaszewski
0 siblings, 2 replies; 8+ messages in thread
From: Tony Makkiel @ 2016-07-05 15:30 UTC (permalink / raw)
To: linux-leds
Cc: Tony Makkiel, linux-acpi, j.anaszewski, rpurdie, rjw, lenb,
mika.westerberg
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 | 13 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-lp3952.c | 308 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/leds-lp3952.h | 125 ++++++++++++++++++
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..0f321f4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -228,6 +228,19 @@ 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
+ depends on ACPI
+ 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..a903bcd
--- /dev/null
+++ b/drivers/leds/leds-lp3952.c
@@ -0,0 +1,308 @@
+/*
+ * LED driver for TI lp3952 controller
+ *
+ * Copyright (C) 2016, DAQRI, LLC.
+ * Author: Tony Makkiel <tony.makkiel@daqri.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/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>
+
+static 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_get_label(char *dest, const char *label, struct device *dev)
+{
+ int ret;
+ const union acpi_object *obj;
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+
+ ret = acpi_dev_get_property(adev, label, ACPI_TYPE_STRING, &obj);
+ if (!ret)
+ strncpy(dest, obj->string.pointer, obj->string.length + 1);
+
+ return ret;
+}
+
+static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
+{
+ int i, ret = -ENODEV;
+ static const char *led_name_hdl[LP3952_LED_ALL] = {
+ "blue2",
+ "green2",
+ "red2",
+ "blue1",
+ "green1",
+ "red1"
+ };
+
+ for (i = 0; i < LP3952_LED_ALL; i++) {
+ 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 = lp3952_get_label(priv->leds[i].name, led_name_hdl[i],
+ &priv->client->dev);
+ if (ret)
+ break;
+
+ priv->leds[i].cdev.name = priv->leds[i].name;
+
+ 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;
+
+ 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 = lp3952_register_write(priv->client, LP3952_REG_ENABLES,
+ LP3952_ACTIVE_MODE | LP3952_INT_B00ST_LDR);
+ 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,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+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;
+
+ priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ leds = devm_kcalloc(&client->dev, LP3952_LED_ALL, 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, "nrst",
+ 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_ACPI_NAME, 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
+#endif
+
+static struct i2c_driver lp3952_i2c_driver = {
+ .driver = {
+ .name = LP3952_NAME,
+ .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
+ },
+ .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..9d072da
--- /dev/null
+++ b/include/linux/leds-lp3952.h
@@ -0,0 +1,125 @@
+/*
+ * LED driver for TI lp3952 controller
+ *
+ * Copyright (C) 2016, DAQRI, LLC.
+ * Author: Tony Makkiel <tony.makkiel@daqri.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef LEDS_LP3952_H_
+#define LEDS_LP3952_H_
+
+#define LP3952_NAME "lp3952"
+#define LP3952_ACPI_NAME "TXNW3952"
+#define LP3952_CMD_REG_COUNT 8
+#define LP3952_BRIGHT_MAX 4
+
+#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_INT_B00ST_LDR BIT(2)
+#define LP3952_ACTIVE_MODE BIT(6)
+#define LP3952_LED_MASK_ALL 0x3f
+
+/* Transition Time in ms */
+enum lp3952_tt {
+ TT0,
+ TT55,
+ TT110,
+ TT221,
+ TT422,
+ TT885,
+ TT1770,
+ TT3539
+};
+
+/* Command Execution Time in ms */
+enum lp3952_cet {
+ CET197,
+ CET393,
+ CET590,
+ CET786,
+ CET1180,
+ CET1376,
+ CET1573,
+ CET1769,
+ CET1966,
+ CET2163,
+ CET2359,
+ CET2556,
+ CET2763,
+ CET2949,
+ CET3146
+};
+
+/* Max Current in % */
+enum lp3952_colour_I_log_0 {
+ I0,
+ I7,
+ I14,
+ I21,
+ I32,
+ I46,
+ I71,
+ I100
+};
+
+enum lp3952_leds {
+ LP3952_BLUE_2,
+ LP3952_GREEN_2,
+ LP3952_RED_2,
+ LP3952_BLUE_1,
+ LP3952_GREEN_1,
+ LP3952_RED_1,
+ LP3952_LED_ALL
+};
+
+struct lp3952_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 {
+ struct regmap *regmap;
+ struct i2c_client *client;
+ struct gpio_desc *enable_gpio;
+ struct lp3952_ctrl_hdl *leds;
+};
+
+#endif /* LEDS_LP3952_H_ */
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-05 15:30 [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel
@ 2016-07-06 8:23 ` Mika Westerberg
2016-07-06 13:33 ` Jacek Anaszewski
1 sibling, 0 replies; 8+ messages in thread
From: Mika Westerberg @ 2016-07-06 8:23 UTC (permalink / raw)
To: Tony Makkiel; +Cc: linux-leds, linux-acpi, j.anaszewski, rpurdie, rjw, lenb
On Tue, Jul 05, 2016 at 04:30:28PM +0100, Tony Makkiel wrote:
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id lp3952_acpi_match[] = {
> + {LP3952_ACPI_NAME, 0},
I would just drop the silly macro and use
{ "TXNW3952" },
instead.
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
> +#endif
> +
> +static struct i2c_driver lp3952_i2c_driver = {
> + .driver = {
> + .name = LP3952_NAME,
> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
> + },
> + .probe = lp3952_probe,
> + .remove = lp3952_remove,
> + .id_table = lp3952_id,
> +};
> +
Anyway ACPI parts look good enough,
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-05 15:30 [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel
2016-07-06 8:23 ` Mika Westerberg
@ 2016-07-06 13:33 ` Jacek Anaszewski
2016-07-06 15:05 ` Tony
1 sibling, 1 reply; 8+ messages in thread
From: Jacek Anaszewski @ 2016-07-06 13:33 UTC (permalink / raw)
To: Tony Makkiel; +Cc: linux-leds, linux-acpi, rpurdie, rjw, lenb, mika.westerberg
Hi Tony,
Thanks for the update.
On 07/05/2016 05:30 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 | 13 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-lp3952.c | 308 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/leds-lp3952.h | 125 ++++++++++++++++++
> 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..0f321f4 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -228,6 +228,19 @@ 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
> + depends on ACPI
> + 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..a903bcd
> --- /dev/null
> +++ b/drivers/leds/leds-lp3952.c
> @@ -0,0 +1,308 @@
> +/*
> + * LED driver for TI lp3952 controller
> + *
> + * Copyright (C) 2016, DAQRI, LLC.
> + * Author: Tony Makkiel <tony.makkiel@daqri.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/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>
> +
> +static 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);
Here you have two separate calls that change the device state.
I think that mutex protection is required here to assure that
the device will not be left in an inconsistent state, due to
the concurrent calls from other processes.
> +}
> +
> +static int lp3952_get_label(char *dest, const char *label, struct device *dev)
> +{
> + int ret;
> + const union acpi_object *obj;
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> + ret = acpi_dev_get_property(adev, label, ACPI_TYPE_STRING, &obj);
> + if (!ret)
> + strncpy(dest, obj->string.pointer, obj->string.length + 1);
> +
> + return ret;
> +}
> +
> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
> +{
> + int i, ret = -ENODEV;
> + static const char *led_name_hdl[LP3952_LED_ALL] = {
> + "blue2",
> + "green2",
> + "red2",
> + "blue1",
> + "green1",
> + "red1"
> + };
> +
> + for (i = 0; i < LP3952_LED_ALL; i++) {
> + 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 = lp3952_get_label(priv->leds[i].name, led_name_hdl[i],
> + &priv->client->dev);
> + if (ret)
> + break;
You're assuming here that all LEDs will always be present, which
doesn't necessarily have to be true. How about just skipping
the LED and moving to the next one if given label was not found?
You could move this check to the beginning of the loop then.
> +
> + priv->leds[i].cdev.name = priv->leds[i].name;
> +
> + 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;
> +
> + 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 = lp3952_register_write(priv->client, LP3952_REG_ENABLES,
> + LP3952_ACTIVE_MODE | LP3952_INT_B00ST_LDR);
> + 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,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +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;
> +
> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + leds = devm_kcalloc(&client->dev, LP3952_LED_ALL, 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, "nrst",
> + 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_ACPI_NAME, 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
> +#endif
> +
> +static struct i2c_driver lp3952_i2c_driver = {
> + .driver = {
> + .name = LP3952_NAME,
> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
> + },
> + .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..9d072da
> --- /dev/null
> +++ b/include/linux/leds-lp3952.h
> @@ -0,0 +1,125 @@
> +/*
> + * LED driver for TI lp3952 controller
> + *
> + * Copyright (C) 2016, DAQRI, LLC.
> + * Author: Tony Makkiel <tony.makkiel@daqri.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef LEDS_LP3952_H_
> +#define LEDS_LP3952_H_
> +
> +#define LP3952_NAME "lp3952"
> +#define LP3952_ACPI_NAME "TXNW3952"
> +#define LP3952_CMD_REG_COUNT 8
> +#define LP3952_BRIGHT_MAX 4
> +
> +#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_INT_B00ST_LDR BIT(2)
> +#define LP3952_ACTIVE_MODE BIT(6)
> +#define LP3952_LED_MASK_ALL 0x3f
> +
> +/* Transition Time in ms */
> +enum lp3952_tt {
> + TT0,
> + TT55,
> + TT110,
> + TT221,
> + TT422,
> + TT885,
> + TT1770,
> + TT3539
> +};
> +
> +/* Command Execution Time in ms */
> +enum lp3952_cet {
> + CET197,
> + CET393,
> + CET590,
> + CET786,
> + CET1180,
> + CET1376,
> + CET1573,
> + CET1769,
> + CET1966,
> + CET2163,
> + CET2359,
> + CET2556,
> + CET2763,
> + CET2949,
> + CET3146
> +};
> +
> +/* Max Current in % */
> +enum lp3952_colour_I_log_0 {
> + I0,
> + I7,
> + I14,
> + I21,
> + I32,
> + I46,
> + I71,
> + I100
> +};
> +
> +enum lp3952_leds {
> + LP3952_BLUE_2,
> + LP3952_GREEN_2,
> + LP3952_RED_2,
> + LP3952_BLUE_1,
> + LP3952_GREEN_1,
> + LP3952_RED_1,
> + LP3952_LED_ALL
> +};
> +
> +struct lp3952_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 {
> + struct regmap *regmap;
> + struct i2c_client *client;
> + struct gpio_desc *enable_gpio;
> + struct lp3952_ctrl_hdl *leds;
> +};
> +
> +#endif /* LEDS_LP3952_H_ */
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-06 13:33 ` Jacek Anaszewski
@ 2016-07-06 15:05 ` Tony
2016-07-07 7:13 ` Jacek Anaszewski
0 siblings, 1 reply; 8+ messages in thread
From: Tony @ 2016-07-06 15:05 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: linux-leds, linux-acpi, rpurdie, rjw, lenb, mika.westerberg
Thank you for the comments Jacek and Mika.
On 06/07/16 14:33, Jacek Anaszewski wrote:
>> +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);
>
> Here you have two separate calls that change the device state.
> I think that mutex protection is required here to assure that
> the device will not be left in an inconsistent state, due to
> the concurrent calls from other processes.
Sorry, I did not quite understand.
Did you mean 'regmap_update_bits' here and the one within
'lp3952_on_off'? If there isn't a lock assigned by driver, a mutex or
spinlock is assigned based on fast_io support by regmap during init.
>
>> +}
>> +
>> +static int lp3952_get_label(char *dest, const char *label, struct
>> device *dev)
>> +{
>> + int ret;
>> + const union acpi_object *obj;
>> + struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> + ret = acpi_dev_get_property(adev, label, ACPI_TYPE_STRING, &obj);
>> + if (!ret)
>> + strncpy(dest, obj->string.pointer, obj->string.length + 1);
>> +
>> + return ret;
>> +}
>> +
>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>> +{
>> + int i, ret = -ENODEV;
>> + static const char *led_name_hdl[LP3952_LED_ALL] = {
>> + "blue2",
>> + "green2",
>> + "red2",
>> + "blue1",
>> + "green1",
>> + "red1"
>> + };
>> +
>> + for (i = 0; i < LP3952_LED_ALL; i++) {
>> + 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 = lp3952_get_label(priv->leds[i].name, led_name_hdl[i],
>> + &priv->client->dev);
>> + if (ret)
>> + break;
>
> You're assuming here that all LEDs will always be present, which
> doesn't necessarily have to be true. How about just skipping
> the LED and moving to the next one if given label was not found?
> You could move this check to the beginning of the loop then.
If we skip leds, there could be problem, if all the leds were
skipped(For example, no ACPI name data present).
At present, if a label is not present, the probe will fail.
How about reverting to a default name (eg: "lp3952:blue2") if led name
read fails? This would also cover cases in which none of the led names
were read properly.
>
>> +
>> + priv->leds[i].cdev.name = priv->leds[i].name;
>> +
>> + 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;
>> +}
>> +
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-06 15:05 ` Tony
@ 2016-07-07 7:13 ` Jacek Anaszewski
2016-07-07 11:44 ` Tony
0 siblings, 1 reply; 8+ messages in thread
From: Jacek Anaszewski @ 2016-07-07 7:13 UTC (permalink / raw)
To: Tony; +Cc: linux-leds, linux-acpi, rpurdie, rjw, lenb, mika.westerberg
Hi Tony,
On 07/06/2016 05:05 PM, Tony wrote:
> Thank you for the comments Jacek and Mika.
>
> On 06/07/16 14:33, Jacek Anaszewski wrote:
>
>
>>> +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);
>>
>> Here you have two separate calls that change the device state.
>> I think that mutex protection is required here to assure that
>> the device will not be left in an inconsistent state, due to
>> the concurrent calls from other processes.
>
> Sorry, I did not quite understand.
>
> Did you mean 'regmap_update_bits' here and the one within
> 'lp3952_on_off'? If there isn't a lock assigned by driver, a mutex or
> spinlock is assigned based on fast_io support by regmap during init.
The lock protects an access to the I2C bus for a single transmission.
When there are two registers to set then current process can be
preempted after first write by the second process which overwrites
the register with a new value. The first one after being woken up
performs second register write, being unaware that the instruction it
completed in the previous step has been just overwritten.
It applies to the situations where there is a common register
setting affecting all LEDs, e.g. power down mode or so, but after
consulting the documentation I figured out that this is not the
case.
>>
>>> +}
>>> +
>>> +static int lp3952_get_label(char *dest, const char *label, struct
>>> device *dev)
>>> +{
>>> + int ret;
>>> + const union acpi_object *obj;
>>> + struct acpi_device *adev = ACPI_COMPANION(dev);
>>> +
>>> + ret = acpi_dev_get_property(adev, label, ACPI_TYPE_STRING, &obj);
>>> + if (!ret)
>>> + strncpy(dest, obj->string.pointer, obj->string.length + 1);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>>> +{
>>> + int i, ret = -ENODEV;
>>> + static const char *led_name_hdl[LP3952_LED_ALL] = {
>>> + "blue2",
>>> + "green2",
>>> + "red2",
>>> + "blue1",
>>> + "green1",
>>> + "red1"
>>> + };
Why actually do you need this array? Couldn't you provide the LED
identifier in the DSD entry corresponding to a LED? You can follow
the scheme used in Device Tree by introducing "reg" property.
>>> + for (i = 0; i < LP3952_LED_ALL; i++) {
>>> + 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 = lp3952_get_label(priv->leds[i].name, led_name_hdl[i],
>>> + &priv->client->dev);
>>> + if (ret)
>>> + break;
>>
>> You're assuming here that all LEDs will always be present, which
>> doesn't necessarily have to be true. How about just skipping
>> the LED and moving to the next one if given label was not found?
>> You could move this check to the beginning of the loop then.
>
> If we skip leds, there could be problem, if all the leds were
> skipped(For example, no ACPI name data present).
If all the LEDs were skipped, then no LED class device would be
registered. In such a case you could return an error from
lp3952_probe().
>
> At present, if a label is not present, the probe will fail.
> How about reverting to a default name (eg: "lp3952:blue2") if led name
> read fails?
There is no point in registering the LED if its DSD entry is not
valid. In case of Device Tree we fail LED registration in such
cases.
> This would also cover cases in which none of the led names
> were read properly.
Whole driver probing should fail then.
>
>>
>>> +
>>> + priv->leds[i].cdev.name = priv->leds[i].name;
>>> +
>>> + 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;
>>> +}
>>> +
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-07 7:13 ` Jacek Anaszewski
@ 2016-07-07 11:44 ` Tony
2016-07-07 14:34 ` Jacek Anaszewski
0 siblings, 1 reply; 8+ messages in thread
From: Tony @ 2016-07-07 11:44 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: linux-leds, linux-acpi, rpurdie, rjw, lenb, mika.westerberg
Hi Jacek,
Thank you for the comments.
On 07/07/16 08:13, Jacek Anaszewski wrote:
> Hi Tony,
>
> On 07/06/2016 05:05 PM, Tony wrote:
>> Thank you for the comments Jacek and Mika.
>>
>> On 06/07/16 14:33, Jacek Anaszewski wrote:
>>
>>
>>>> +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);
>>>
>>> Here you have two separate calls that change the device state.
>>> I think that mutex protection is required here to assure that
>>> the device will not be left in an inconsistent state, due to
>>> the concurrent calls from other processes.
>>
>> Sorry, I did not quite understand.
>>
>> Did you mean 'regmap_update_bits' here and the one within
>> 'lp3952_on_off'? If there isn't a lock assigned by driver, a mutex or
>> spinlock is assigned based on fast_io support by regmap during init.
>
> The lock protects an access to the I2C bus for a single transmission.
> When there are two registers to set then current process can be
> preempted after first write by the second process which overwrites
> the register with a new value. The first one after being woken up
> performs second register write, being unaware that the instruction it
> completed in the previous step has been just overwritten.
>
> It applies to the situations where there is a common register
> setting affecting all LEDs, e.g. power down mode or so, but after
> consulting the documentation I figured out that this is not the
> case.
>
>>>
>>>> +}
>>>> +
>>>> +static int lp3952_get_label(char *dest, const char *label, struct
>>>> device *dev)
>>>> +{
>>>> + int ret;
>>>> + const union acpi_object *obj;
>>>> + struct acpi_device *adev = ACPI_COMPANION(dev);
>>>> +
>>>> + ret = acpi_dev_get_property(adev, label, ACPI_TYPE_STRING, &obj);
>>>> + if (!ret)
>>>> + strncpy(dest, obj->string.pointer, obj->string.length + 1);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>>>> +{
>>>> + int i, ret = -ENODEV;
>>>> + static const char *led_name_hdl[LP3952_LED_ALL] = {
>>>> + "blue2",
>>>> + "green2",
>>>> + "red2",
>>>> + "blue1",
>>>> + "green1",
>>>> + "red1"
>>>> + };
>
> Why actually do you need this array? Couldn't you provide the LED
> identifier in the DSD entry corresponding to a LED? You can follow
> the scheme used in Device Tree by introducing "reg" property.
The names in the array are the 6 identifiers in the DSD entry. For example
the DSD property for blue2 led used is
"Package ()
{
Package (2) {"blue2", "led_blue2"},
"
acpi_dev_get_property will return "led_blue2" in obj->string.pointer.
There isn't separate DSD for individual leds. There is only 1 DSD for
the led controller identified by "Name (_HID, "TXNW3952")". The
individual led names are separate properties withing th DSD (eg:
led_blue2 identified by blue2).
>
>>>> + for (i = 0; i < LP3952_LED_ALL; i++) {
>>>> + 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 = lp3952_get_label(priv->leds[i].name, led_name_hdl[i],
>>>> + &priv->client->dev);
>>>> + if (ret)
>>>> + break;
>>>
>>> You're assuming here that all LEDs will always be present, which
>>> doesn't necessarily have to be true. How about just skipping
>>> the LED and moving to the next one if given label was not found?
>>> You could move this check to the beginning of the loop then.
>>
>> If we skip leds, there could be problem, if all the leds were
>> skipped(For example, no ACPI name data present).
>
> If all the LEDs were skipped, then no LED class device would be
> registered. In such a case you could return an error from
> lp3952_probe().
>
>>
>> At present, if a label is not present, the probe will fail.
>> How about reverting to a default name (eg: "lp3952:blue2") if led name
>> read fails?
>
> There is no point in registering the LED if its DSD entry is not
> valid. In case of Device Tree we fail LED registration in such
> cases.
Sorry, I thought we were skipping LEDs due to invalid DSD. If that is
not the case, isn't the assumption all LEDS will be present true?
I am guessing leds might be missing, if some of the leds are not
physically connected to controller output. But the controller would
have 6 led controls all the time.
>
>> This would also cover cases in which none of the led names
>> were read properly.
>
> Whole driver probing should fail then.
>
>>
>>>
>>>> +
>>>> + priv->leds[i].cdev.name = priv->leds[i].name;
>>>> +
>>>> + 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;
>>>> +}
>>>> +
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-07 11:44 ` Tony
@ 2016-07-07 14:34 ` Jacek Anaszewski
2016-07-07 14:59 ` Tony
0 siblings, 1 reply; 8+ messages in thread
From: Jacek Anaszewski @ 2016-07-07 14:34 UTC (permalink / raw)
To: Tony; +Cc: linux-leds, linux-acpi, rpurdie, rjw, lenb, mika.westerberg
On 07/07/2016 01:44 PM, Tony wrote:
> Hi Jacek,
> Thank you for the comments.
You're welcome.
> On 07/07/16 08:13, Jacek Anaszewski wrote:
>> Hi Tony,
>>
>> On 07/06/2016 05:05 PM, Tony wrote:
>>> Thank you for the comments Jacek and Mika.
>>>
>>> On 06/07/16 14:33, Jacek Anaszewski wrote:
>>>
>>>
>>>>> +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);
>>>>
>>>> Here you have two separate calls that change the device state.
>>>> I think that mutex protection is required here to assure that
>>>> the device will not be left in an inconsistent state, due to
>>>> the concurrent calls from other processes.
>>>
>>> Sorry, I did not quite understand.
>>>
>>> Did you mean 'regmap_update_bits' here and the one within
>>> 'lp3952_on_off'? If there isn't a lock assigned by driver, a mutex or
>>> spinlock is assigned based on fast_io support by regmap during init.
>>
>> The lock protects an access to the I2C bus for a single transmission.
>> When there are two registers to set then current process can be
>> preempted after first write by the second process which overwrites
>> the register with a new value. The first one after being woken up
>> performs second register write, being unaware that the instruction it
>> completed in the previous step has been just overwritten.
>>
>> It applies to the situations where there is a common register
>> setting affecting all LEDs, e.g. power down mode or so, but after
>> consulting the documentation I figured out that this is not the
>> case.
>>
>>>>
>>>>> +}
>>>>> +
>>>>> +static int lp3952_get_label(char *dest, const char *label, struct
>>>>> device *dev)
>>>>> +{
>>>>> + int ret;
>>>>> + const union acpi_object *obj;
>>>>> + struct acpi_device *adev = ACPI_COMPANION(dev);
>>>>> +
>>>>> + ret = acpi_dev_get_property(adev, label, ACPI_TYPE_STRING, &obj);
>>>>> + if (!ret)
>>>>> + strncpy(dest, obj->string.pointer, obj->string.length + 1);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int lp3952_register_led_classdev(struct lp3952_led_array
>>>>> *priv)
>>>>> +{
>>>>> + int i, ret = -ENODEV;
>>>>> + static const char *led_name_hdl[LP3952_LED_ALL] = {
>>>>> + "blue2",
>>>>> + "green2",
>>>>> + "red2",
>>>>> + "blue1",
>>>>> + "green1",
>>>>> + "red1"
>>>>> + };
>>
>> Why actually do you need this array? Couldn't you provide the LED
>> identifier in the DSD entry corresponding to a LED? You can follow
>> the scheme used in Device Tree by introducing "reg" property.
>
> The names in the array are the 6 identifiers in the DSD entry. For example
> the DSD property for blue2 led used is
>
> "Package ()
> {
> Package (2) {"blue2", "led_blue2"},
> "
> acpi_dev_get_property will return "led_blue2" in obj->string.pointer.
>
> There isn't separate DSD for individual leds. There is only 1 DSD for
> the led controller identified by "Name (_HID, "TXNW3952")". The
> individual led names are separate properties withing th DSD (eg:
> led_blue2 identified by blue2).
OK, thanks for the explanation. BTW LED name pattern is
devicename:colour:function.
>>
>>>>> + for (i = 0; i < LP3952_LED_ALL; i++) {
>>>>> + 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 = lp3952_get_label(priv->leds[i].name, led_name_hdl[i],
>>>>> + &priv->client->dev);
>>>>> + if (ret)
>>>>> + break;
>>>>
>>>> You're assuming here that all LEDs will always be present, which
>>>> doesn't necessarily have to be true. How about just skipping
>>>> the LED and moving to the next one if given label was not found?
>>>> You could move this check to the beginning of the loop then.
>>>
>>> If we skip leds, there could be problem, if all the leds were
>>> skipped(For example, no ACPI name data present).
>>
>> If all the LEDs were skipped, then no LED class device would be
>> registered. In such a case you could return an error from
>> lp3952_probe().
>>
>>>
>>> At present, if a label is not present, the probe will fail.
>>> How about reverting to a default name (eg: "lp3952:blue2") if led name
>>> read fails?
>>
>> There is no point in registering the LED if its DSD entry is not
>> valid. In case of Device Tree we fail LED registration in such
>> cases.
>
> Sorry, I thought we were skipping LEDs due to invalid DSD. If that is
> not the case, isn't the assumption all LEDS will be present true?
> I am guessing leds might be missing, if some of the leds are not
> physically connected to controller output. But the controller would
> have 6 led controls all the time.
Right, but there is no point in exposing corresponding
LED class devices to user space if the user will be unable
to notice any effect because the LED is not connected.
>>
>>> This would also cover cases in which none of the led names
>>> were read properly.
>>
>> Whole driver probing should fail then.
>>
>>>
>>>>
>>>>> +
>>>>> + priv->leds[i].cdev.name = priv->leds[i].name;
>>>>> +
>>>>> + 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;
>>>>> +}
>>>>> +
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-07 14:34 ` Jacek Anaszewski
@ 2016-07-07 14:59 ` Tony
0 siblings, 0 replies; 8+ messages in thread
From: Tony @ 2016-07-07 14:59 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds
On 07/07/16 15:34, Jacek Anaszewski wrote:
>>>
>>> Why actually do you need this array? Couldn't you provide the LED
>>> identifier in the DSD entry corresponding to a LED? You can follow
>>> the scheme used in Device Tree by introducing "reg" property.
>>
>> The names in the array are the 6 identifiers in the DSD entry. For
>> example
>> the DSD property for blue2 led used is
>>
>> "Package ()
>> {
>> Package (2) {"blue2", "led_blue2"},
>> "
>> acpi_dev_get_property will return "led_blue2" in obj->string.pointer.
>>
>> There isn't separate DSD for individual leds. There is only 1 DSD for
>> the led controller identified by "Name (_HID, "TXNW3952")". The
>> individual led names are separate properties withing th DSD (eg:
>> led_blue2 identified by blue2).
>
> OK, thanks for the explanation. BTW LED name pattern is
> devicename:colour:function.
>
>>>
>>>>>> + for (i = 0; i < LP3952_LED_ALL; i++) {
>>>>>> + 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 = lp3952_get_label(priv->leds[i].name, led_name_hdl[i],
>>>>>> + &priv->client->dev);
>>>>>> + if (ret)
>>>>>> + break;
>>>>>
>>>>> You're assuming here that all LEDs will always be present, which
>>>>> doesn't necessarily have to be true. How about just skipping
>>>>> the LED and moving to the next one if given label was not found?
>>>>> You could move this check to the beginning of the loop then.
>>>>
>>>> If we skip leds, there could be problem, if all the leds were
>>>> skipped(For example, no ACPI name data present).
>>>
>>> If all the LEDs were skipped, then no LED class device would be
>>> registered. In such a case you could return an error from
>>> lp3952_probe().
>>>
>>>>
>>>> At present, if a label is not present, the probe will fail.
>>>> How about reverting to a default name (eg: "lp3952:blue2") if led name
>>>> read fails?
>>>
>>> There is no point in registering the LED if its DSD entry is not
>>> valid. In case of Device Tree we fail LED registration in such
>>> cases.
>>
>> Sorry, I thought we were skipping LEDs due to invalid DSD. If that is
>> not the case, isn't the assumption all LEDS will be present true?
>> I am guessing leds might be missing, if some of the leds are not
>> physically connected to controller output. But the controller would
>> have 6 led controls all the time.
>
> Right, but there is no point in exposing corresponding
> LED class devices to user space if the user will be unable
> to notice any effect because the LED is not connected.
Got it, Thank you :) Will skip registration if led not found.
>
>>>
>>>> This would also cover cases in which none of the led names
>>>> were read properly.
>>>
>>> Whole driver probing should fail then.
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> + priv->leds[i].cdev.name = priv->leds[i].name;
>>>>>> +
>>>>>> + 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;
>>>>>> +}
>>>>>> +
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-acpi" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>
>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-07-07 15:00 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-05 15:30 [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel
2016-07-06 8:23 ` Mika Westerberg
2016-07-06 13:33 ` Jacek Anaszewski
2016-07-06 15:05 ` Tony
2016-07-07 7:13 ` Jacek Anaszewski
2016-07-07 11:44 ` Tony
2016-07-07 14:34 ` Jacek Anaszewski
2016-07-07 14:59 ` Tony
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.