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

[Tony <tony.makkiel@daqri.com>]

On 11/07/16 12:25, Jacek Anaszewski wrote:
> On 07/11/2016 12:48 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>
>> ---
>>
>> Changes from v9:
>>   - Add dependancy on GPIOLIB.
>>   - make on/off variable of lp3952_on_off function bool.
>>
>> Changes from v8:
>>   - Removed macro LP3952_ACPI_NAME.
>>   - Replaced acpi_dev_get_property with device_property_read_string
>>   - Skip led class registration if led data couldn't be read from
>>     ACPI table.
>>
>>
>>   drivers/leds/Kconfig        |  14 ++
>>   drivers/leds/Makefile       |   1 +
>>   drivers/leds/leds-lp3952.c  | 306
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/leds-lp3952.h | 125 ++++++++++++++++++
>>   4 files changed, 446 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..9dcc9b1 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -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

>
>> +    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@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, 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));
>> +}
>> +
>> +static int lp3952_configure(struct lp3952_led_array *priv)
>> +{
>> +    int ret;
>> +
>> +    /* Disable any LEDs on from any previous conf. */
>> +    ret = lp3952_register_write(priv->client, LP3952_REG_LED_CTRL, 0);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* enable rgb patter, loop */
>> +    ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL,
>> +                    LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Update Bit 6 (Active mode), Select both Led sets, Bit [1:0] */
>> +    ret = lp3952_register_write(priv->client, LP3952_REG_ENABLES,
>> +                    LP3952_ACTIVE_MODE | LP3952_INT_B00ST_LDR);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Set Cmd1 for RGB intensity,cmd and transition time */
>> +    return (lp3952_set_pattern_gen_cmd(priv, 0, I46, I71, I100, TT0,
>> +                       CET197));
>> +}
>> +
>> +static const struct regmap_config lp3952_regmap = {
>> +    .reg_bits = 8,
>> +    .val_bits = 8,
>> +    .max_register = REG_MAX,
>> +    .cache_type = REGCACHE_RBTREE,
>> +};
>> +
>> +static int lp3952_probe(struct i2c_client *client,
>> +            const struct i2c_device_id *id)
>> +{
>> +    int status;
>> +    struct lp3952_ctrl_hdl *leds;
>> +    struct lp3952_led_array *priv;
>> +
>> +    priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>> +    if (!priv)
>> +        return -ENOMEM;
>> +
>> +    leds = devm_kcalloc(&client->dev, LP3952_LED_ALL, sizeof(*leds),
>> +                GFP_KERNEL);
>> +    if (!leds) {
>> +        devm_kfree(&client->dev, priv);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    priv->leds = leds;
>> +    priv->client = client;
>> +
>> +    priv->enable_gpio = devm_gpiod_get(&client->dev, "nrst",
>> +                       GPIOD_OUT_HIGH);
>> +    if (IS_ERR(priv->enable_gpio)) {
>> +        status = PTR_ERR(priv->enable_gpio);
>> +        dev_err(&client->dev, "Failed to enable gpio: %d\n", status);
>> +        return status;
>> +    }
>> +
>> +    priv->regmap = devm_regmap_init_i2c(client, &lp3952_regmap);
>> +    if (IS_ERR(priv->regmap)) {
>> +        int err = PTR_ERR(priv->regmap);
>> +
>> +        dev_err(&client->dev, "Failed to allocate register map: %d\n",
>> +            err);
>> +        return err;
>> +    }
>> +
>> +    i2c_set_clientdata(client, priv);
>> +
>> +    status = lp3952_configure(priv);
>> +    if (status) {
>> +        dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
>> +            status);
>> +        return status;
>> +    }
>> +
>> +    status = lp3952_register_led_classdev(priv);
>> +    if (status) {
>> +        dev_err(&client->dev, "Unable to register led_classdev: %d\n",
>> +            status);
>> +        return status;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int lp3952_remove(struct i2c_client *client)
>> +{
>> +    struct lp3952_led_array *priv;
>> +
>> +    priv = i2c_get_clientdata(client);
>> +    lp3952_on_off(priv, LP3952_LED_ALL, 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@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..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@daqri.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#ifndef LEDS_LP3952_H_
>> +#define LEDS_LP3952_H_
>> +
>> +#define LP3952_NAME                         "lp3952"
>> +#define LP3952_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_ */
>>
>
>