Re: [PATCH v10 1/1] leds: LED driver for TI LP3952 6-Channel Color LED

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Thank you for the comments Jacek.

On 11/07/16 13:08, Jacek Anaszewski wrote:

@@ -228,6 +228,20 @@ 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
+    depends on GPIOLIB

Any specific reason for choosing "depends" instead of "select"?

select causes "recursive dependency error".


drivers/gpio/Kconfig:34:error: recursive dependency detected!
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpio/Kconfig:34:    symbol GPIOLIB is selected by LEDS_LP3952
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/leds/Kconfig:231:    symbol LEDS_LP3952 depends on NEW_LEDS
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/leds/Kconfig:8:    symbol NEW_LEDS is selected by
BCMA_DRIVER_GPIO
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/bcma/Kconfig:97:    symbol BCMA_DRIVER_GPIO depends on GPIOLIB

I didn't get this with my configuration, but it seems that it is
likely to happen. Let's abide by "depends on" then.

Besides, some redundant brackets are left, see below.


+    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..c0eb2f4
--- /dev/null
+++ b/drivers/leds/leds-lp3952.c
@@ -0,0 +1,306 @@
+/*
+ *    LED driver for TI lp3952 controller
+ *
+ *    Copyright (C) 2016, DAQRI, LLC.
+ *    Author: Tony Makkiel <tony.makkiel@xxxxxxxxx>
+ *
+ * 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, bool 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, false);
+        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, true);
+
+    return regmap_update_bits(priv->regmap, reg, 3 << shift_val,
+                  --value << shift_val);
+}
+
+static int lp3952_get_label(struct device *dev, const char *label,
char *dest)
+{
+    int ret;
+    const char *str;
+
+    ret = device_property_read_string(dev, label, &str);
+    if (!ret)
+        strncpy(dest, str, LP3952_LABEL_MAX_LEN);
+
+    return ret;
+}
+
+static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
+{
+    int i, acpi_ret, 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++) {
+        acpi_ret = lp3952_get_label(&priv->client->dev,
led_name_hdl[i],
+                        priv->leds[i].name);
+        if (acpi_ret)
+            continue;
+
+        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));

These brackets are redundant.

+}
+
+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));

Ditto.

If you agree I can remove them and take the patch.

Yes please :). Thank you for your help.

+}
+
+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, false);
+    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[] = {
+    {"TXNW3952", 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@xxxxxxxxx>");
+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..edd5ed6
--- /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@xxxxxxxxx>
+ *
+ * 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_CMD_REG_COUNT                8
+#define LP3952_BRIGHT_MAX                   4
+#define LP3952_LABEL_MAX_LEN                15
+
+#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[LP3952_LABEL_MAX_LEN];
+    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_ */



--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux