* [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
@ 2016-06-28 16:05 Tony Makkiel
2016-06-29 0:45 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Tony Makkiel @ 2016-06-28 16:05 UTC (permalink / raw)
To: linux-leds
Cc: Tony Makkiel, j.anaszewski, rpurdie, rjw, lenb, mika.westerberg,
linux-acpi
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 | 289 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/leds-lp3952.h | 128 ++++++++++++++++++++
4 files changed, 431 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..cec7374
--- /dev/null
+++ b/drivers/leds/leds-lp3952.c
@@ -0,0 +1,289 @@
+/*
+ * 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_register_led_classdev(struct lp3952_led_array *priv)
+{
+ int i, ret = -ENODEV;
+ static const char *led_name[LP3952_LED_ALL] = {
+ "lp3952:blue2",
+ "lp3952:green2",
+ "lp3952:red2",
+ "lp3952:blue1",
+ "lp3952:green1",
+ "lp3952:red1"
+ };
+
+ for (i = 0; i < LP3952_LED_ALL; 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;
+
+ 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, 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_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..9808ab7
--- /dev/null
+++ b/include/linux/leds-lp3952.h
@@ -0,0 +1,128 @@
+/*
+ * 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_
+
+/* ACPI iasl compiler wont take name LP3952,
+ * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
+ */
+#define LP3952_NAME "lp3952"
+#define LP3952_ACPI_NAME "PRP0001"
+#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] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-28 16:05 [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel
@ 2016-06-29 0:45 ` Rafael J. Wysocki
2016-06-29 6:52 ` Jacek Anaszewski
2016-06-29 10:02 ` Tony
0 siblings, 2 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2016-06-29 0:45 UTC (permalink / raw)
To: Tony Makkiel
Cc: linux-leds, j.anaszewski, rpurdie, lenb, mika.westerberg, linux-acpi
On Tuesday, June 28, 2016 05:05:19 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 | 289 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/leds-lp3952.h | 128 ++++++++++++++++++++
> 4 files changed, 431 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..cec7374
> --- /dev/null
> +++ b/drivers/leds/leds-lp3952.c
> @@ -0,0 +1,289 @@
> +/*
> + * 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_register_led_classdev(struct lp3952_led_array *priv)
> +{
> + int i, ret = -ENODEV;
> + static const char *led_name[LP3952_LED_ALL] = {
> + "lp3952:blue2",
> + "lp3952:green2",
> + "lp3952:red2",
> + "lp3952:blue1",
> + "lp3952:green1",
> + "lp3952:red1"
> + };
> +
> + for (i = 0; i < LP3952_LED_ALL; 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;
> +
> + 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, 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_ACPI_NAME, 0},
No, you can't use "PRP0001" in this list.
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
And you don't need this for the "PRP0001" thing to work. The core will
take care of it for you then.
> +#endif
So the entire ACPI block can be dropped for now.
And the driver doesn't have to depend on CONFIG_ACPI any more, does it?
> +
> +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..9808ab7
> --- /dev/null
> +++ b/include/linux/leds-lp3952.h
> @@ -0,0 +1,128 @@
> +/*
> + * 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_
> +
> +/* ACPI iasl compiler wont take name LP3952,
> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
> + */
> +#define LP3952_NAME "lp3952"
> +#define LP3952_ACPI_NAME "PRP0001"
This is not necessary.
> +#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_ */
Thanks,
Rafael
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-29 0:45 ` Rafael J. Wysocki
@ 2016-06-29 6:52 ` Jacek Anaszewski
2016-06-29 10:07 ` Tony
2016-06-29 10:02 ` Tony
1 sibling, 1 reply; 15+ messages in thread
From: Jacek Anaszewski @ 2016-06-29 6:52 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tony Makkiel, linux-leds, rpurdie, lenb, mika.westerberg, linux-acpi
Hi Rafael,
On 06/29/2016 02:45 AM, Rafael J. Wysocki wrote:
> On Tuesday, June 28, 2016 05:05:19 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 | 289 ++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/leds-lp3952.h | 128 ++++++++++++++++++++
>> 4 files changed, 431 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..cec7374
>> --- /dev/null
>> +++ b/drivers/leds/leds-lp3952.c
>> @@ -0,0 +1,289 @@
>> +/*
>> + * 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_register_led_classdev(struct lp3952_led_array *priv)
>> +{
>> + int i, ret = -ENODEV;
>> + static const char *led_name[LP3952_LED_ALL] = {
>> + "lp3952:blue2",
>> + "lp3952:green2",
>> + "lp3952:red2",
>> + "lp3952:blue1",
>> + "lp3952:green1",
>> + "lp3952:red1"
>> + };
>> +
>> + for (i = 0; i < LP3952_LED_ALL; 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;
>> +
>> + 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, 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_ACPI_NAME, 0},
>
> No, you can't use "PRP0001" in this list.
>
>> + {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>
> And you don't need this for the "PRP0001" thing to work. The core will
> take care of it for you then.
>
>> +#endif
>
> So the entire ACPI block can be dropped for now.
>
> And the driver doesn't have to depend on CONFIG_ACPI any more, does it?
The driver currently supports probing only with ACPI.
I have one question BTW: isn't there anything similar to the device tree
bindings documentation required for ACPI overlays?
Pointer to the discussion which led us to this solution:
http://www.spinics.net/lists/linux-leds/msg06230.html
>> +
>> +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..9808ab7
>> --- /dev/null
>> +++ b/include/linux/leds-lp3952.h
>> @@ -0,0 +1,128 @@
>> +/*
>> + * 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_
>> +
>> +/* ACPI iasl compiler wont take name LP3952,
>> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
>> + */
>> +#define LP3952_NAME "lp3952"
>> +#define LP3952_ACPI_NAME "PRP0001"
>
> This is not necessary.
>
>> +#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_ */
>
> Thanks,
> Rafael
>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-29 0:45 ` Rafael J. Wysocki
2016-06-29 6:52 ` Jacek Anaszewski
@ 2016-06-29 10:02 ` Tony
1 sibling, 0 replies; 15+ messages in thread
From: Tony @ 2016-06-29 10:02 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-leds, j.anaszewski, rpurdie, lenb, mika.westerberg, linux-acpi
Thank you for the comments Rafael.
On 29/06/16 01:45, Rafael J. Wysocki wrote:
> On Tuesday, June 28, 2016 05:05:19 PM Tony Makkiel wrote:
>> +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, 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_ACPI_NAME, 0},
>
> No, you can't use "PRP0001" in this list.
>
>> + {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>
> And you don't need this for the "PRP0001" thing to work. The core will
> take care of it for you then.
>
>> +#endif
>
> So the entire ACPI block can be dropped for now.
>
> And the driver doesn't have to depend on CONFIG_ACPI any more, does it?
We need to pass the reset gpio to the driver.
"devm_gpiod_get(&client->dev, NULL, GPIOD_OUT_HIGH);"
in the "lp3952_probe". I shall remove the PRP0001 from the driver and
check whether the gpio can be still read from ACPI. If it can, removing
ACPI block shouldn't be a problem.
>
>> +
>> +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..9808ab7
>> --- /dev/null
>> +++ b/include/linux/leds-lp3952.h
>> @@ -0,0 +1,128 @@
>> +/*
>> + * 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_
>> +
>> +/* ACPI iasl compiler wont take name LP3952,
>> + * "ASL_MSG_HID_LENGTH". Hence the name TLP3952
>> + */
>> +#define LP3952_NAME "lp3952"
>> +#define LP3952_ACPI_NAME "PRP0001"
>
> This is not necessary.
>
>> +#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_ */
>
> Thanks,
> Rafael
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-29 6:52 ` Jacek Anaszewski
@ 2016-06-29 10:07 ` Tony
2016-06-29 10:54 ` Jacek Anaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Tony @ 2016-06-29 10:07 UTC (permalink / raw)
To: Jacek Anaszewski, Rafael J. Wysocki
Cc: linux-leds, rpurdie, lenb, mika.westerberg, linux-acpi
On 29/06/16 07:52, Jacek Anaszewski wrote:
> Hi Rafael,
>>> +#ifdef CONFIG_ACPI
>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>> + {LP3952_ACPI_NAME, 0},
>>
>> No, you can't use "PRP0001" in this list.
>>
>>> + {}
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>
>> And you don't need this for the "PRP0001" thing to work. The core will
>> take care of it for you then.
>>
>>> +#endif
>>
>> So the entire ACPI block can be dropped for now.
>>
>> And the driver doesn't have to depend on CONFIG_ACPI any more, does it?
>
> The driver currently supports probing only with ACPI.
> I have one question BTW: isn't there anything similar to the device tree
> bindings documentation required for ACPI overlays?
> Pointer to the discussion which led us to this solution:
>
> http://www.spinics.net/lists/linux-leds/msg06230.html
>
_DSD is working now. I managed to get "PRP0001" working as suggested by
Rafael in
http://marc.info/?l=linux-acpi&m=146711623115228&w=2
with _DSD
I will try adding names using _DSD. I am not sure why DSD didn't work
earlier. The only reason I could think of is, upgrading my OS recently
following a raid failure.
>
>>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-29 10:07 ` Tony
@ 2016-06-29 10:54 ` Jacek Anaszewski
2016-07-05 10:47 ` Jacek Anaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Jacek Anaszewski @ 2016-06-29 10:54 UTC (permalink / raw)
To: Tony
Cc: Rafael J. Wysocki, linux-leds, rpurdie, lenb, mika.westerberg,
linux-acpi
On 06/29/2016 12:07 PM, Tony wrote:
>
> On 29/06/16 07:52, Jacek Anaszewski wrote:
>> Hi Rafael,
>
>>>> +#ifdef CONFIG_ACPI
>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>> + {LP3952_ACPI_NAME, 0},
>>>
>>> No, you can't use "PRP0001" in this list.
>>>
>>>> + {}
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>
>>> And you don't need this for the "PRP0001" thing to work. The core will
>>> take care of it for you then.
>>>
>>>> +#endif
>>>
>>> So the entire ACPI block can be dropped for now.
>>>
>>> And the driver doesn't have to depend on CONFIG_ACPI any more, does it?
>>
>> The driver currently supports probing only with ACPI.
>> I have one question BTW: isn't there anything similar to the device tree
>> bindings documentation required for ACPI overlays?
>> Pointer to the discussion which led us to this solution:
>>
>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>
> _DSD is working now. I managed to get "PRP0001" working as suggested by
> Rafael in
> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
> with _DSD
Thanks for the link.
Rafael, "Package" entries seem to mimic Device Tree properties defined
in the common leds bindings. Would it be possible to make it even
more compatible and define every LED connected to the LED controller
in the form of a child node, similarly as in case of LED DT bindings?
See Documentation/devicetree/bindings/leds/common.txt and other
bindings in Documentation/devicetree/bindings/leds.
> I will try adding names using _DSD. I am not sure why DSD didn't work
> earlier. The only reason I could think of is, upgrading my OS recently
> following a raid failure.
>
>>
>>>>
> --
> 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] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-06-29 10:54 ` Jacek Anaszewski
@ 2016-07-05 10:47 ` Jacek Anaszewski
2016-07-05 12:12 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Jacek Anaszewski @ 2016-07-05 10:47 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tony, linux-leds, rpurdie, lenb, mika.westerberg, linux-acpi
PING
On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
> On 06/29/2016 12:07 PM, Tony wrote:
>>
>> On 29/06/16 07:52, Jacek Anaszewski wrote:
>>> Hi Rafael,
>>
>>>>> +#ifdef CONFIG_ACPI
>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>> + {LP3952_ACPI_NAME, 0},
>>>>
>>>> No, you can't use "PRP0001" in this list.
>>>>
>>>>> + {}
>>>>> +};
>>>>> +
>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>
>>>> And you don't need this for the "PRP0001" thing to work. The core will
>>>> take care of it for you then.
>>>>
>>>>> +#endif
>>>>
>>>> So the entire ACPI block can be dropped for now.
>>>>
>>>> And the driver doesn't have to depend on CONFIG_ACPI any more, does it?
>>>
>>> The driver currently supports probing only with ACPI.
>>> I have one question BTW: isn't there anything similar to the device tree
>>> bindings documentation required for ACPI overlays?
>>> Pointer to the discussion which led us to this solution:
>>>
>>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>>
>> _DSD is working now. I managed to get "PRP0001" working as suggested by
>> Rafael in
>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
>> with _DSD
>
> Thanks for the link.
>
> Rafael, "Package" entries seem to mimic Device Tree properties defined
> in the common leds bindings. Would it be possible to make it even
> more compatible and define every LED connected to the LED controller
> in the form of a child node, similarly as in case of LED DT bindings?
>
> See Documentation/devicetree/bindings/leds/common.txt and other
> bindings in Documentation/devicetree/bindings/leds.
>
>> I will try adding names using _DSD. I am not sure why DSD didn't work
>> earlier. The only reason I could think of is, upgrading my OS recently
>> following a raid failure.
>>
>>>
>>>>>
>> --
>> 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] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-05 10:47 ` Jacek Anaszewski
@ 2016-07-05 12:12 ` Rafael J. Wysocki
2016-07-05 12:33 ` Jacek Anaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2016-07-05 12:12 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Rafael J. Wysocki, Tony, linux-leds, rpurdie, Len Brown,
Mika Westerberg, ACPI Devel Maling List
On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski
<j.anaszewski@samsung.com> wrote:
> PING
>
>
> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
>>
>> On 06/29/2016 12:07 PM, Tony wrote:
>>>
>>>
>>> On 29/06/16 07:52, Jacek Anaszewski wrote:
>>>>
>>>> Hi Rafael,
>>>
>>>
>>>>>> +#ifdef CONFIG_ACPI
>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>> + {LP3952_ACPI_NAME, 0},
>>>>>
>>>>>
>>>>> No, you can't use "PRP0001" in this list.
>>>>>
>>>>>> + {}
>>>>>> +};
>>>>>> +
>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>>
>>>>>
>>>>> And you don't need this for the "PRP0001" thing to work. The core will
>>>>> take care of it for you then.
>>>>>
>>>>>> +#endif
>>>>>
>>>>>
>>>>> So the entire ACPI block can be dropped for now.
>>>>>
>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more, does it?
>>>>
>>>>
>>>> The driver currently supports probing only with ACPI.
>>>> I have one question BTW: isn't there anything similar to the device tree
>>>> bindings documentation required for ACPI overlays?
>>>> Pointer to the discussion which led us to this solution:
>>>>
>>>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>>>
>>> _DSD is working now. I managed to get "PRP0001" working as suggested by
>>> Rafael in
>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
>>> with _DSD
>>
>>
>> Thanks for the link.
>>
>> Rafael, "Package" entries seem to mimic Device Tree properties defined
>> in the common leds bindings. Would it be possible to make it even
>> more compatible and define every LED connected to the LED controller
>> in the form of a child node, similarly as in case of LED DT bindings?
>>
>> See Documentation/devicetree/bindings/leds/common.txt and other
>> bindings in Documentation/devicetree/bindings/leds.
I'm not sure what you mean.
If somebody decides to arrange the data in their ACPI tables to follow
that scheme, it will just work IMO.
Is there anything more that needs to be done in the kernel here?
Thanks,
Rafael
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-05 12:12 ` Rafael J. Wysocki
@ 2016-07-05 12:33 ` Jacek Anaszewski
2016-07-05 13:14 ` Tony
2016-07-06 13:32 ` Jacek Anaszewski
0 siblings, 2 replies; 15+ messages in thread
From: Jacek Anaszewski @ 2016-07-05 12:33 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Tony, linux-leds, rpurdie, Len Brown,
Mika Westerberg, ACPI Devel Maling List
On 07/05/2016 02:12 PM, Rafael J. Wysocki wrote:
> On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski
> <j.anaszewski@samsung.com> wrote:
>> PING
>>
>>
>> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
>>>
>>> On 06/29/2016 12:07 PM, Tony wrote:
>>>>
>>>>
>>>> On 29/06/16 07:52, Jacek Anaszewski wrote:
>>>>>
>>>>> Hi Rafael,
>>>>
>>>>
>>>>>>> +#ifdef CONFIG_ACPI
>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>>> + {LP3952_ACPI_NAME, 0},
>>>>>>
>>>>>>
>>>>>> No, you can't use "PRP0001" in this list.
>>>>>>
>>>>>>> + {}
>>>>>>> +};
>>>>>>> +
>>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>>>
>>>>>>
>>>>>> And you don't need this for the "PRP0001" thing to work. The core will
>>>>>> take care of it for you then.
>>>>>>
>>>>>>> +#endif
>>>>>>
>>>>>>
>>>>>> So the entire ACPI block can be dropped for now.
>>>>>>
>>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more, does it?
>>>>>
>>>>>
>>>>> The driver currently supports probing only with ACPI.
>>>>> I have one question BTW: isn't there anything similar to the device tree
>>>>> bindings documentation required for ACPI overlays?
>>>>> Pointer to the discussion which led us to this solution:
>>>>>
>>>>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>>>>
>>>> _DSD is working now. I managed to get "PRP0001" working as suggested by
>>>> Rafael in
>>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
>>>> with _DSD
>>>
>>>
>>> Thanks for the link.
>>>
>>> Rafael, "Package" entries seem to mimic Device Tree properties defined
>>> in the common leds bindings. Would it be possible to make it even
>>> more compatible and define every LED connected to the LED controller
>>> in the form of a child node, similarly as in case of LED DT bindings?
>>>
>>> See Documentation/devicetree/bindings/leds/common.txt and other
>>> bindings in Documentation/devicetree/bindings/leds.
>
> I'm not sure what you mean.
>
> If somebody decides to arrange the data in their ACPI tables to follow
> that scheme, it will just work IMO.
>
> Is there anything more that needs to be done in the kernel here?
In case of Device Tree based platforms it is customary to define each
LED as a child node of the LED controller node. LED names can then
be obtained from the 'label' property or DT node name.
Tony has encountered some issues [1] while trying to follow this
pattern. Tony, could you please double check that?
[1] http://www.spinics.net/lists/linux-leds/msg06230.html
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-05 12:33 ` Jacek Anaszewski
@ 2016-07-05 13:14 ` Tony
2016-07-06 13:32 ` Jacek Anaszewski
1 sibling, 0 replies; 15+ messages in thread
From: Tony @ 2016-07-05 13:14 UTC (permalink / raw)
To: Jacek Anaszewski, Rafael J. Wysocki
Cc: Rafael J. Wysocki, linux-leds, rpurdie, Len Brown,
Mika Westerberg, ACPI Devel Maling List
On 05/07/16 13:33, Jacek Anaszewski wrote:
> On 07/05/2016 02:12 PM, Rafael J. Wysocki wrote:
>> On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski
>> <j.anaszewski@samsung.com> wrote:
>>> PING
>>>
>>>
>>> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
>>>>
>>>> On 06/29/2016 12:07 PM, Tony wrote:
>>>>>
>>>>>
>>>>> On 29/06/16 07:52, Jacek Anaszewski wrote:
>>>>>>
>>>>>> Hi Rafael,
>>>>>
>>>>>
>>>>>>>> +#ifdef CONFIG_ACPI
>>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>>>> + {LP3952_ACPI_NAME, 0},
>>>>>>>
>>>>>>>
>>>>>>> No, you can't use "PRP0001" in this list.
>>>>>>>
>>>>>>>> + {}
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>>>>
>>>>>>>
>>>>>>> And you don't need this for the "PRP0001" thing to work. The
>>>>>>> core will
>>>>>>> take care of it for you then.
>>>>>>>
>>>>>>>> +#endif
>>>>>>>
>>>>>>>
>>>>>>> So the entire ACPI block can be dropped for now.
>>>>>>>
>>>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more,
>>>>>>> does it?
>>>>>>
>>>>>>
>>>>>> The driver currently supports probing only with ACPI.
>>>>>> I have one question BTW: isn't there anything similar to the
>>>>>> device tree
>>>>>> bindings documentation required for ACPI overlays?
>>>>>> Pointer to the discussion which led us to this solution:
>>>>>>
>>>>>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>>>>>
>>>>> _DSD is working now. I managed to get "PRP0001" working as
>>>>> suggested by
>>>>> Rafael in
>>>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
>>>>> with _DSD
>>>>
>>>>
>>>> Thanks for the link.
>>>>
>>>> Rafael, "Package" entries seem to mimic Device Tree properties defined
>>>> in the common leds bindings. Would it be possible to make it even
>>>> more compatible and define every LED connected to the LED controller
>>>> in the form of a child node, similarly as in case of LED DT bindings?
>>>>
>>>> See Documentation/devicetree/bindings/leds/common.txt and other
>>>> bindings in Documentation/devicetree/bindings/leds.
>>
>> I'm not sure what you mean.
>>
>> If somebody decides to arrange the data in their ACPI tables to follow
>> that scheme, it will just work IMO.
>>
>> Is there anything more that needs to be done in the kernel here?
>
> In case of Device Tree based platforms it is customary to define each
> LED as a child node of the LED controller node. LED names can then
> be obtained from the 'label' property or DT node name.
> Tony has encountered some issues [1] while trying to follow this
> pattern. Tony, could you please double check that?
>
> [1] http://www.spinics.net/lists/linux-leds/msg06230.html
Sorry, I was quite for a while. I have been trying to get the driver
probed using "compatible" property, with PRP0001. But has not worked yet.
I am told by TI to use TXNW3952 as ACPI id (waiting for final
confirmation). So hopefully won't have to use PRP0001.
I have managed to read one of the led names from ACPI table using the _DSD.
Will implement the same for other leds, and send another patch soon.
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-05 12:33 ` Jacek Anaszewski
2016-07-05 13:14 ` Tony
@ 2016-07-06 13:32 ` Jacek Anaszewski
2016-07-06 21:15 ` Rafael J. Wysocki
1 sibling, 1 reply; 15+ messages in thread
From: Jacek Anaszewski @ 2016-07-06 13:32 UTC (permalink / raw)
To: Rafael J. Wysocki, Rafael J. Wysocki
Cc: Tony, linux-leds, rpurdie, Len Brown, Mika Westerberg,
ACPI Devel Maling List
On 07/05/2016 02:33 PM, Jacek Anaszewski wrote:
> On 07/05/2016 02:12 PM, Rafael J. Wysocki wrote:
>> On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski
>> <j.anaszewski@samsung.com> wrote:
>>> PING
>>>
>>>
>>> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
>>>>
>>>> On 06/29/2016 12:07 PM, Tony wrote:
>>>>>
>>>>>
>>>>> On 29/06/16 07:52, Jacek Anaszewski wrote:
>>>>>>
>>>>>> Hi Rafael,
>>>>>
>>>>>
>>>>>>>> +#ifdef CONFIG_ACPI
>>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>>>> + {LP3952_ACPI_NAME, 0},
>>>>>>>
>>>>>>>
>>>>>>> No, you can't use "PRP0001" in this list.
>>>>>>>
>>>>>>>> + {}
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>>>>
>>>>>>>
>>>>>>> And you don't need this for the "PRP0001" thing to work. The
>>>>>>> core will
>>>>>>> take care of it for you then.
>>>>>>>
>>>>>>>> +#endif
>>>>>>>
>>>>>>>
>>>>>>> So the entire ACPI block can be dropped for now.
>>>>>>>
>>>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more,
>>>>>>> does it?
>>>>>>
>>>>>>
>>>>>> The driver currently supports probing only with ACPI.
>>>>>> I have one question BTW: isn't there anything similar to the
>>>>>> device tree
>>>>>> bindings documentation required for ACPI overlays?
>>>>>> Pointer to the discussion which led us to this solution:
>>>>>>
>>>>>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>>>>>
>>>>> _DSD is working now. I managed to get "PRP0001" working as
>>>>> suggested by
>>>>> Rafael in
>>>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
>>>>> with _DSD
>>>>
>>>>
>>>> Thanks for the link.
>>>>
>>>> Rafael, "Package" entries seem to mimic Device Tree properties defined
>>>> in the common leds bindings. Would it be possible to make it even
>>>> more compatible and define every LED connected to the LED controller
>>>> in the form of a child node, similarly as in case of LED DT bindings?
>>>>
>>>> See Documentation/devicetree/bindings/leds/common.txt and other
>>>> bindings in Documentation/devicetree/bindings/leds.
>>
>> I'm not sure what you mean.
>>
>> If somebody decides to arrange the data in their ACPI tables to follow
>> that scheme, it will just work IMO.
>>
>> Is there anything more that needs to be done in the kernel here?
>
> In case of Device Tree based platforms it is customary to define each
> LED as a child node of the LED controller node. LED names can then
> be obtained from the 'label' property or DT node name.
> Tony has encountered some issues [1] while trying to follow this
> pattern. Tony, could you please double check that?
>
> [1] http://www.spinics.net/lists/linux-leds/msg06230.html
One more question: now anyone wanting to use the driver would have
to investigate the code to infer what DSD properties need to be
provided for it. Isn't there anything similar to
Documentation/devicetree/bindings/* documentation required in case of
DSD?
I've also come across your presentation [1], which describes the
device_property* API for parsing unified DT/ACPI device properties.
IMO it would be good solution for this driver as the device seems not
to be tightly coupled with only ACPI platforms.
[1]
http://events.linuxfoundation.org/sites/events/files/slides/ACPI_vs_DT.pdf
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-06 13:32 ` Jacek Anaszewski
@ 2016-07-06 21:15 ` Rafael J. Wysocki
2016-07-07 7:13 ` Jacek Anaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2016-07-06 21:15 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Rafael J. Wysocki, Tony, linux-leds, rpurdie, Len Brown,
Mika Westerberg, ACPI Devel Maling List, Al Stone
On Wednesday, July 06, 2016 03:32:25 PM Jacek Anaszewski wrote:
> On 07/05/2016 02:33 PM, Jacek Anaszewski wrote:
> > On 07/05/2016 02:12 PM, Rafael J. Wysocki wrote:
> >> On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski
> >> <j.anaszewski@samsung.com> wrote:
> >>> PING
> >>>
> >>>
> >>> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
> >>>>
> >>>> On 06/29/2016 12:07 PM, Tony wrote:
> >>>>>
> >>>>>
> >>>>> On 29/06/16 07:52, Jacek Anaszewski wrote:
> >>>>>>
> >>>>>> Hi Rafael,
> >>>>>
> >>>>>
> >>>>>>>> +#ifdef CONFIG_ACPI
> >>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
> >>>>>>>> + {LP3952_ACPI_NAME, 0},
> >>>>>>>
> >>>>>>>
> >>>>>>> No, you can't use "PRP0001" in this list.
> >>>>>>>
> >>>>>>>> + {}
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
> >>>>>>>
> >>>>>>>
> >>>>>>> And you don't need this for the "PRP0001" thing to work. The
> >>>>>>> core will
> >>>>>>> take care of it for you then.
> >>>>>>>
> >>>>>>>> +#endif
> >>>>>>>
> >>>>>>>
> >>>>>>> So the entire ACPI block can be dropped for now.
> >>>>>>>
> >>>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more,
> >>>>>>> does it?
> >>>>>>
> >>>>>>
> >>>>>> The driver currently supports probing only with ACPI.
> >>>>>> I have one question BTW: isn't there anything similar to the
> >>>>>> device tree
> >>>>>> bindings documentation required for ACPI overlays?
> >>>>>> Pointer to the discussion which led us to this solution:
> >>>>>>
> >>>>>> http://www.spinics.net/lists/linux-leds/msg06230.html
> >>>>>>
> >>>>> _DSD is working now. I managed to get "PRP0001" working as
> >>>>> suggested by
> >>>>> Rafael in
> >>>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
> >>>>> with _DSD
> >>>>
> >>>>
> >>>> Thanks for the link.
> >>>>
> >>>> Rafael, "Package" entries seem to mimic Device Tree properties defined
> >>>> in the common leds bindings. Would it be possible to make it even
> >>>> more compatible and define every LED connected to the LED controller
> >>>> in the form of a child node, similarly as in case of LED DT bindings?
> >>>>
> >>>> See Documentation/devicetree/bindings/leds/common.txt and other
> >>>> bindings in Documentation/devicetree/bindings/leds.
> >>
> >> I'm not sure what you mean.
> >>
> >> If somebody decides to arrange the data in their ACPI tables to follow
> >> that scheme, it will just work IMO.
> >>
> >> Is there anything more that needs to be done in the kernel here?
> >
> > In case of Device Tree based platforms it is customary to define each
> > LED as a child node of the LED controller node. LED names can then
> > be obtained from the 'label' property or DT node name.
> > Tony has encountered some issues [1] while trying to follow this
> > pattern. Tony, could you please double check that?
> >
> > [1] http://www.spinics.net/lists/linux-leds/msg06230.html
>
> One more question: now anyone wanting to use the driver would have
> to investigate the code to infer what DSD properties need to be
> provided for it. Isn't there anything similar to
> Documentation/devicetree/bindings/* documentation required in case of
> DSD?
There is a plan to set up a database for these, but that's still
under discussion (Al Stone has posted some related documentation for
comments recently).
> I've also come across your presentation [1], which describes the
> device_property* API for parsing unified DT/ACPI device properties.
> IMO it would be good solution for this driver as the device seems not
> to be tightly coupled with only ACPI platforms.
>
>
> [1]
> http://events.linuxfoundation.org/sites/events/files/slides/ACPI_vs_DT.pdf
The device_property* API should be used everywhere where (a) properties are
used and (b) the code has no specific need to use a particular platform
firmware interface (ACPI or DT), so I agree.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-06 21:15 ` Rafael J. Wysocki
@ 2016-07-07 7:13 ` Jacek Anaszewski
2016-07-07 10:35 ` Tony
0 siblings, 1 reply; 15+ messages in thread
From: Jacek Anaszewski @ 2016-07-07 7:13 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, linux-leds, rpurdie, Len Brown,
Mika Westerberg, ACPI Devel Maling List, Al Stone
On 07/06/2016 11:15 PM, Rafael J. Wysocki wrote:
> On Wednesday, July 06, 2016 03:32:25 PM Jacek Anaszewski wrote:
>> On 07/05/2016 02:33 PM, Jacek Anaszewski wrote:
>>> On 07/05/2016 02:12 PM, Rafael J. Wysocki wrote:
>>>> On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski
>>>> <j.anaszewski@samsung.com> wrote:
>>>>> PING
>>>>>
>>>>>
>>>>> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
>>>>>>
>>>>>> On 06/29/2016 12:07 PM, Tony wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 29/06/16 07:52, Jacek Anaszewski wrote:
>>>>>>>>
>>>>>>>> Hi Rafael,
>>>>>>>
>>>>>>>
>>>>>>>>>> +#ifdef CONFIG_ACPI
>>>>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>>>>>> + {LP3952_ACPI_NAME, 0},
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No, you can't use "PRP0001" in this list.
>>>>>>>>>
>>>>>>>>>> + {}
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> And you don't need this for the "PRP0001" thing to work. The
>>>>>>>>> core will
>>>>>>>>> take care of it for you then.
>>>>>>>>>
>>>>>>>>>> +#endif
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> So the entire ACPI block can be dropped for now.
>>>>>>>>>
>>>>>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more,
>>>>>>>>> does it?
>>>>>>>>
>>>>>>>>
>>>>>>>> The driver currently supports probing only with ACPI.
>>>>>>>> I have one question BTW: isn't there anything similar to the
>>>>>>>> device tree
>>>>>>>> bindings documentation required for ACPI overlays?
>>>>>>>> Pointer to the discussion which led us to this solution:
>>>>>>>>
>>>>>>>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>>>>>>>
>>>>>>> _DSD is working now. I managed to get "PRP0001" working as
>>>>>>> suggested by
>>>>>>> Rafael in
>>>>>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
>>>>>>> with _DSD
>>>>>>
>>>>>>
>>>>>> Thanks for the link.
>>>>>>
>>>>>> Rafael, "Package" entries seem to mimic Device Tree properties defined
>>>>>> in the common leds bindings. Would it be possible to make it even
>>>>>> more compatible and define every LED connected to the LED controller
>>>>>> in the form of a child node, similarly as in case of LED DT bindings?
>>>>>>
>>>>>> See Documentation/devicetree/bindings/leds/common.txt and other
>>>>>> bindings in Documentation/devicetree/bindings/leds.
>>>>
>>>> I'm not sure what you mean.
>>>>
>>>> If somebody decides to arrange the data in their ACPI tables to follow
>>>> that scheme, it will just work IMO.
>>>>
>>>> Is there anything more that needs to be done in the kernel here?
>>>
>>> In case of Device Tree based platforms it is customary to define each
>>> LED as a child node of the LED controller node. LED names can then
>>> be obtained from the 'label' property or DT node name.
>>> Tony has encountered some issues [1] while trying to follow this
>>> pattern. Tony, could you please double check that?
>>>
>>> [1] http://www.spinics.net/lists/linux-leds/msg06230.html
>>
>> One more question: now anyone wanting to use the driver would have
>> to investigate the code to infer what DSD properties need to be
>> provided for it. Isn't there anything similar to
>> Documentation/devicetree/bindings/* documentation required in case of
>> DSD?
>
> There is a plan to set up a database for these, but that's still
> under discussion (Al Stone has posted some related documentation for
> comments recently).
>
>> I've also come across your presentation [1], which describes the
>> device_property* API for parsing unified DT/ACPI device properties.
>> IMO it would be good solution for this driver as the device seems not
>> to be tightly coupled with only ACPI platforms.
>>
>>
>> [1]
>> http://events.linuxfoundation.org/sites/events/files/slides/ACPI_vs_DT.pdf
>
> The device_property* API should be used everywhere where (a) properties are
> used and (b) the code has no specific need to use a particular platform
> firmware interface (ACPI or DT), so I agree.
Tony, could you try switching to device_property* API and make
the DSD properties as much compatible with Device Tree ones as possible?
We haven't tried it in the LED subsystem so far but it is a good
occasion to do so.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-07 7:13 ` Jacek Anaszewski
@ 2016-07-07 10:35 ` Tony
2016-07-07 11:09 ` Jacek Anaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Tony @ 2016-07-07 10:35 UTC (permalink / raw)
To: Jacek Anaszewski, Rafael J. Wysocki
Cc: Rafael J. Wysocki, linux-leds, rpurdie, Len Brown,
Mika Westerberg, ACPI Devel Maling List, Al Stone
On 07/07/16 08:13, Jacek Anaszewski wrote:
> On 07/06/2016 11:15 PM, Rafael J. Wysocki wrote:
>> On Wednesday, July 06, 2016 03:32:25 PM Jacek Anaszewski wrote:
>>> On 07/05/2016 02:33 PM, Jacek Anaszewski wrote:
>>>> On 07/05/2016 02:12 PM, Rafael J. Wysocki wrote:
>>>>> On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski
>>>>> <j.anaszewski@samsung.com> wrote:
>>>>>> PING
>>>>>>
>>>>>>
>>>>>> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
>>>>>>>
>>>>>>> On 06/29/2016 12:07 PM, Tony wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 29/06/16 07:52, Jacek Anaszewski wrote:
>>>>>>>>>
>>>>>>>>> Hi Rafael,
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> +#ifdef CONFIG_ACPI
>>>>>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>>>>>>> + {LP3952_ACPI_NAME, 0},
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No, you can't use "PRP0001" in this list.
>>>>>>>>>>
>>>>>>>>>>> + {}
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> And you don't need this for the "PRP0001" thing to work. The
>>>>>>>>>> core will
>>>>>>>>>> take care of it for you then.
>>>>>>>>>>
>>>>>>>>>>> +#endif
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So the entire ACPI block can be dropped for now.
>>>>>>>>>>
>>>>>>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more,
>>>>>>>>>> does it?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The driver currently supports probing only with ACPI.
>>>>>>>>> I have one question BTW: isn't there anything similar to the
>>>>>>>>> device tree
>>>>>>>>> bindings documentation required for ACPI overlays?
>>>>>>>>> Pointer to the discussion which led us to this solution:
>>>>>>>>>
>>>>>>>>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>>>>>>>>
>>>>>>>> _DSD is working now. I managed to get "PRP0001" working as
>>>>>>>> suggested by
>>>>>>>> Rafael in
>>>>>>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
>>>>>>>> with _DSD
>>>>>>>
>>>>>>>
>>>>>>> Thanks for the link.
>>>>>>>
>>>>>>> Rafael, "Package" entries seem to mimic Device Tree properties
>>>>>>> defined
>>>>>>> in the common leds bindings. Would it be possible to make it even
>>>>>>> more compatible and define every LED connected to the LED controller
>>>>>>> in the form of a child node, similarly as in case of LED DT
>>>>>>> bindings?
>>>>>>>
>>>>>>> See Documentation/devicetree/bindings/leds/common.txt and other
>>>>>>> bindings in Documentation/devicetree/bindings/leds.
>>>>>
>>>>> I'm not sure what you mean.
>>>>>
>>>>> If somebody decides to arrange the data in their ACPI tables to follow
>>>>> that scheme, it will just work IMO.
>>>>>
>>>>> Is there anything more that needs to be done in the kernel here?
>>>>
>>>> In case of Device Tree based platforms it is customary to define each
>>>> LED as a child node of the LED controller node. LED names can then
>>>> be obtained from the 'label' property or DT node name.
>>>> Tony has encountered some issues [1] while trying to follow this
>>>> pattern. Tony, could you please double check that?
>>>>
>>>> [1] http://www.spinics.net/lists/linux-leds/msg06230.html
>>>
>>> One more question: now anyone wanting to use the driver would have
>>> to investigate the code to infer what DSD properties need to be
>>> provided for it. Isn't there anything similar to
>>> Documentation/devicetree/bindings/* documentation required in case of
>>> DSD?
>>
>> There is a plan to set up a database for these, but that's still
>> under discussion (Al Stone has posted some related documentation for
>> comments recently).
>>
>>> I've also come across your presentation [1], which describes the
>>> device_property* API for parsing unified DT/ACPI device properties.
>>> IMO it would be good solution for this driver as the device seems not
>>> to be tightly coupled with only ACPI platforms.
>>>
>>>
>>> [1]
>>> http://events.linuxfoundation.org/sites/events/files/slides/ACPI_vs_DT.pdf
>>>
>>
>> The device_property* API should be used everywhere where (a)
>> properties are
>> used and (b) the code has no specific need to use a particular platform
>> firmware interface (ACPI or DT), so I agree.
>
> Tony, could you try switching to device_property* API and make
> the DSD properties as much compatible with Device Tree ones as possible?
>
Sure will replace 'acpi_dev_get_property' with
'device_property_read_string'.
I am new to both ACPI and Device Tree. So not sure whether there is any
naming convention to follow for led name handles in device tree.
review comments for v8 patch, it was suggested to use "reg" property.
We shall continue the discussion on led name handles, on that
thread to avoid duplication.
> We haven't tried it in the LED subsystem so far but it is a good
> occasion to do so.
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED
2016-07-07 10:35 ` Tony
@ 2016-07-07 11:09 ` Jacek Anaszewski
0 siblings, 0 replies; 15+ messages in thread
From: Jacek Anaszewski @ 2016-07-07 11:09 UTC (permalink / raw)
To: Tony
Cc: Rafael J. Wysocki, Rafael J. Wysocki, linux-leds, rpurdie,
Len Brown, Mika Westerberg, ACPI Devel Maling List, Al Stone
On 07/07/2016 12:35 PM, Tony wrote:
>
>
> On 07/07/16 08:13, Jacek Anaszewski wrote:
>> On 07/06/2016 11:15 PM, Rafael J. Wysocki wrote:
>>> On Wednesday, July 06, 2016 03:32:25 PM Jacek Anaszewski wrote:
>>>> On 07/05/2016 02:33 PM, Jacek Anaszewski wrote:
>>>>> On 07/05/2016 02:12 PM, Rafael J. Wysocki wrote:
>>>>>> On Tue, Jul 5, 2016 at 12:47 PM, Jacek Anaszewski
>>>>>> <j.anaszewski@samsung.com> wrote:
>>>>>>> PING
>>>>>>>
>>>>>>>
>>>>>>> On 06/29/2016 12:54 PM, Jacek Anaszewski wrote:
>>>>>>>>
>>>>>>>> On 06/29/2016 12:07 PM, Tony wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 29/06/16 07:52, Jacek Anaszewski wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Rafael,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>> +#ifdef CONFIG_ACPI
>>>>>>>>>>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>>>>>>>>>>> + {LP3952_ACPI_NAME, 0},
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> No, you can't use "PRP0001" in this list.
>>>>>>>>>>>
>>>>>>>>>>>> + {}
>>>>>>>>>>>> +};
>>>>>>>>>>>> +
>>>>>>>>>>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> And you don't need this for the "PRP0001" thing to work. The
>>>>>>>>>>> core will
>>>>>>>>>>> take care of it for you then.
>>>>>>>>>>>
>>>>>>>>>>>> +#endif
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So the entire ACPI block can be dropped for now.
>>>>>>>>>>>
>>>>>>>>>>> And the driver doesn't have to depend on CONFIG_ACPI any more,
>>>>>>>>>>> does it?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The driver currently supports probing only with ACPI.
>>>>>>>>>> I have one question BTW: isn't there anything similar to the
>>>>>>>>>> device tree
>>>>>>>>>> bindings documentation required for ACPI overlays?
>>>>>>>>>> Pointer to the discussion which led us to this solution:
>>>>>>>>>>
>>>>>>>>>> http://www.spinics.net/lists/linux-leds/msg06230.html
>>>>>>>>>>
>>>>>>>>> _DSD is working now. I managed to get "PRP0001" working as
>>>>>>>>> suggested by
>>>>>>>>> Rafael in
>>>>>>>>> http://marc.info/?l=linux-acpi&m=146711623115228&w=2
>>>>>>>>> with _DSD
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for the link.
>>>>>>>>
>>>>>>>> Rafael, "Package" entries seem to mimic Device Tree properties
>>>>>>>> defined
>>>>>>>> in the common leds bindings. Would it be possible to make it even
>>>>>>>> more compatible and define every LED connected to the LED
>>>>>>>> controller
>>>>>>>> in the form of a child node, similarly as in case of LED DT
>>>>>>>> bindings?
>>>>>>>>
>>>>>>>> See Documentation/devicetree/bindings/leds/common.txt and other
>>>>>>>> bindings in Documentation/devicetree/bindings/leds.
>>>>>>
>>>>>> I'm not sure what you mean.
>>>>>>
>>>>>> If somebody decides to arrange the data in their ACPI tables to
>>>>>> follow
>>>>>> that scheme, it will just work IMO.
>>>>>>
>>>>>> Is there anything more that needs to be done in the kernel here?
>>>>>
>>>>> In case of Device Tree based platforms it is customary to define each
>>>>> LED as a child node of the LED controller node. LED names can then
>>>>> be obtained from the 'label' property or DT node name.
>>>>> Tony has encountered some issues [1] while trying to follow this
>>>>> pattern. Tony, could you please double check that?
>>>>>
>>>>> [1] http://www.spinics.net/lists/linux-leds/msg06230.html
>>>>
>>>> One more question: now anyone wanting to use the driver would have
>>>> to investigate the code to infer what DSD properties need to be
>>>> provided for it. Isn't there anything similar to
>>>> Documentation/devicetree/bindings/* documentation required in case of
>>>> DSD?
>>>
>>> There is a plan to set up a database for these, but that's still
>>> under discussion (Al Stone has posted some related documentation for
>>> comments recently).
>>>
>>>> I've also come across your presentation [1], which describes the
>>>> device_property* API for parsing unified DT/ACPI device properties.
>>>> IMO it would be good solution for this driver as the device seems not
>>>> to be tightly coupled with only ACPI platforms.
>>>>
>>>>
>>>> [1]
>>>> http://events.linuxfoundation.org/sites/events/files/slides/ACPI_vs_DT.pdf
>>>>
>>>>
>>>
>>> The device_property* API should be used everywhere where (a)
>>> properties are
>>> used and (b) the code has no specific need to use a particular platform
>>> firmware interface (ACPI or DT), so I agree.
>>
>> Tony, could you try switching to device_property* API and make
>> the DSD properties as much compatible with Device Tree ones as possible?
>>
> Sure will replace 'acpi_dev_get_property' with
> 'device_property_read_string'.
>
> I am new to both ACPI and Device Tree. So not sure whether there is any
> naming convention to follow for led name handles in device tree.
You can follow existing bindings in Documentation/devicetree/bindings
/leds.
> review comments for v8 patch, it was suggested to use "reg" property.
> We shall continue the discussion on led name handles, on that
> thread to avoid duplication.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-07-07 11:09 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-28 16:05 [PATCH v7 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Tony Makkiel
2016-06-29 0:45 ` Rafael J. Wysocki
2016-06-29 6:52 ` Jacek Anaszewski
2016-06-29 10:07 ` Tony
2016-06-29 10:54 ` Jacek Anaszewski
2016-07-05 10:47 ` Jacek Anaszewski
2016-07-05 12:12 ` Rafael J. Wysocki
2016-07-05 12:33 ` Jacek Anaszewski
2016-07-05 13:14 ` Tony
2016-07-06 13:32 ` Jacek Anaszewski
2016-07-06 21:15 ` Rafael J. Wysocki
2016-07-07 7:13 ` Jacek Anaszewski
2016-07-07 10:35 ` Tony
2016-07-07 11:09 ` Jacek Anaszewski
2016-06-29 10:02 ` 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.