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

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Tony,

On 05/31/2016 05:31 PM, Tony Makkiel wrote:
>
> Thank you for the comments Jacek. I have done all the changes suggested,
> except some on which, needed some clarification please.
>
> On 30/05/16 13:51, Jacek Anaszewski wrote:
>> Hi Tony,
>>
>> Thanks for the patch. Please refer to my comments in the code.
>>
>> Please address also all issues filed by scripts/checkpatch.pl --strict.
>>
>> On 05/26/2016 12:26 PM, Tony Makkiel wrote:
>>> Datasheet: http://www.ti.com/lit/gpn/lp3952
>>
>> Please put this line at the end of the commit message.
>>
>>>
>>> The chip can drive 2 sets of RGB leds. Controller can
>>> be controlled via PWM, I2C and audio sychnronisation.
>>
>> s/sychnronisation/synchronisation/
>>
>>> This driver use I2C to communicate with chip.
>>
>> s/use/uses/
>> s/chip/the chip/
>>
>>>
>>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com>
>>> ---
>>>    drivers/leds/Kconfig        |  12 ++
>>>    drivers/leds/Makefile       |   1 +
>>>    drivers/leds/leds-lp3952.c  | 411 ++++++++++++++++++++++++++++++++++++++++++++
>>>    include/linux/leds-lp3952.h |  89 ++++++++++
>>>    4 files changed, 513 insertions(+)
>>>    create mode 100644 drivers/leds/leds-lp3952.c
>>>    create mode 100644 include/linux/leds-lp3952.h
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 5ae2834..305faf9 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -228,6 +228,18 @@ config LEDS_LP3944
>>>          To compile this driver as a module, choose M here: the
>>>          module will be called leds-lp3944.
>>>
>>> +config LEDS_LP3952
>>> +    tristate "LED Support for TI LP3952 2 channel LED driver"
>>> +    depends on LEDS_CLASS
>>> +    depends on I2C
>>> +    select REGMAP_I2C
>>> +    help
>>> +      This option enables support for LEDs connected to the Texas
>>> +      Instruments LP3952 LED driver.
>>> +
>>> +      To compile this driver as a module, choose M here: the
>>> +      module will be called leds-lp3952.
>>> +
>>>    config LEDS_LP55XX_COMMON
>>>        tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
>>>        depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index cb2013d..0684c86 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_PCA9532)        += leds-pca9532.o
>>>    obj-$(CONFIG_LEDS_GPIO_REGISTER)    += leds-gpio-register.o
>>>    obj-$(CONFIG_LEDS_GPIO)            += leds-gpio.o
>>>    obj-$(CONFIG_LEDS_LP3944)        += leds-lp3944.o
>>> +obj-$(CONFIG_LEDS_LP3952)        += leds-lp3952.o
>>>    obj-$(CONFIG_LEDS_LP55XX_COMMON)    += leds-lp55xx-common.o
>>>    obj-$(CONFIG_LEDS_LP5521)        += leds-lp5521.o
>>>    obj-$(CONFIG_LEDS_LP5523)        += leds-lp5523.o
>>> diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
>>> new file mode 100644
>>> index 0000000..8229d5a
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-lp3952.c
>>> @@ -0,0 +1,411 @@
>>> +/*
>>> + * Copyright (c) 2016, DAQRI, LLC.
>>> + *
>>> + * License Terms: GNU General Public License v2
>>> + *
>>> + * leds-lp3952 - LED class driver for TI lp3952 controller
>>> + *
>>> + * Based on:
>>> + * leds-tlc9516 - LED class driver for TLC95XX series
>>> + * of I2C driven LED controllers from NXP
>>> + *
>>> + * Derived from leds-atmel-pwm, leds-bd2802 and initial PWM led 3952
>>> + * driver written by Alex Feinman
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +/*#define DEBUG 1*/
>>
>> This seems to be stray debug definition. Let's drop it.
>>
>>> +
>>> +#include <linux/io.h>
>>> +#include <linux/pm.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/acpi.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/reboot.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/notifier.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/leds-lp3952.h>
>>
>> Please arrange include directives in the alphabetical order.
>>
> Have arranged them in alphabetical order. But have left
> <linux/leds-lp3952.h> at the end as it is specific to this file.
> Is that ok? Or should that also follow the order?

Let's treat all the includes in the same way.

>>> +
>>> +#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
>>> +
>>> +struct lp3952_ctrl_hdl {
>>> +    struct led_classdev cdev;
>>> +    char name[10];
>>> +    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;
>>> +    };
>>> +} __attribute__ ((packed));
>>> +
>>> +struct lp3952_led_array {
>>> +    u8 ndev;
>>> +    u32 enable_gpio;
>>> +    struct regmap *regmap;
>>> +    struct i2c_client *client;
>>> +    struct ptrn_gen_cmd pgm[8];
>>
>> Why 8? Please add a macro definition for this constant.
>>
>>> +    struct lp3952_ctrl_hdl *leds;
>>> +};
>>> +
>>> +int lp3952_register_write(struct i2c_client *client, u8 reg, u8 val)
>>> +{
>>> +    int ret;
>>> +    struct lp3952_led_array *priv = i2c_get_clientdata(client);
>>> +
>>> +    ret = regmap_write(priv->regmap, reg, val);
>>> +
>>> +    if (ret)
>>> +        dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
>>> +            __func__, reg, val, ret);
>>> +    return ret;
>>> +}
>>> +
>>> +int lp3952_register_read(struct i2c_client *client, u8 reg, u8 len,
>>> +             u8 *valarray)
>>> +{
>>> +    struct lp3952_led_array *priv = i2c_get_clientdata(client);
>>> +    int ret = regmap_bulk_read(priv->regmap, reg, valarray, len);
>>> +
>>> +    if (ret < 0)
>>> +        dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
>>> +            __func__, reg, valarray[0], ret);
>>> +    return ret;
>>> +}
>>
>> This function seems to be unused.
> Removed the function.
>>
>>> +
>>> +static void lp3952_on_off(struct lp3952_led_array *priv, enum lp3952_leds led,
>>> +              int on)
>>> +{
>>> +    int val = 1 << led;
>>> +
>>> +    dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led, on);
>>
>> Please add empty line here.
>>
>>> +    if (LP3952_LED_ALL == led)
>>> +        val = 0x3f;
>>> +
>>> +    regmap_update_bits(priv->regmap, LP3952_REG_LED_CTRL, val,
>>> +               on ? val : 0);
>>
>> Please check return value.
>>
>>> +}
>>> +
>>> +/*
>>> + * 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 lp3952_brightness b)
>>> +{
>>
>> I'd prefer something more meaningful than 'b'. brightness or value?
>>
>>> +    unsigned int reg, val, shiftVal;
>>> +    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", b, led->channel);
>>> +
>>> +    if (LP3952_OFF == b) {
>>> +        lp3952_on_off(priv, led->channel, LP3952_OFF);
>>> +        return 0;
>>> +    }
>>> +
>>> +    val = b - 1;
>>> +    reg = LP3952_REG_RGB1_MAX_I_CTRL;
>>> +    shiftVal = (led->channel - LP3952_BLUE_1) * 2;
>>> +
>>> +    if (led->channel > LP3952_RED_1) {
>>> +        dev_err(cdev->dev, " %s Invalid LED requested", __func__);
>>> +        return -EINVAL;
>>> +    } else if (led->channel < LP3952_BLUE_1) {
>>> +        shiftVal = led->channel * 2;
>>> +        reg = LP3952_REG_RGB2_MAX_I_CTRL;
>>> +    }
>>
>> Empty line here
>>
>>> +    /* Enable the LED in case it is not enabled already */
>>> +    lp3952_on_off(priv, led->channel, 1);
>>
>> and here would improve readability.
>>
>>> +    return (regmap_update_bits(priv->regmap, reg, 3 << shiftVal,
>>> +                   val << shiftVal));
>>> +}
>>> +
>>> +static void lp3952_brightness_ctrl(struct led_classdev *cdev,
>>> +                   enum led_brightness b)
>>
>> brightness_set_blocking op returns int.
>>
>>> +{
>>> +    int range = LED_HALF / 2;
>>> +    enum lp3952_brightness val = LP3952_FULL_BRIGHT;
>>> +
>>> +    dev_dbg(cdev->dev, "Requested Brightness %d\n", b);
>>> +
>>> +    if (LED_OFF == b)
>>> +        val = LP3952_OFF;
>>> +    else if (b < (LED_HALF - range))
>>> +        val = LP3952_QUARTER_BRIGHT;
>>> +    else if (b <= LED_HALF)
>>> +        val = LP3952_HALF_BRIGHT;
>>> +    else if (b <= LED_HALF + range)
>>> +        val = LP3952_THREE_QRTR_BRIGHT;
>>
>> You should operate directly on brightness levels, i.e. write the
>> brightness level passed from user space directly to the device.
>> enum lp3952_brightness is redundant IMO.
>
> The chip supports 5 brightness values, hence lp3952_brightness.
> The brightness register takes values between 0-3 (2 bits). This
> function should convert user requested legacy values 0-255 to a
> value within the register range.

You have four active brightness levels: 00, 01, 10, 11.
If you initialized max_brightness to 4, then you wouldn't need
any translation. There is max_brightness sysfs attribute that
says what is the maximum allowed brightness level. LED_FULL is
a legacy enum value.

Apart of that, 2-bit color intensity resolution is very poor.
Actually this is a resolution of max current for each LED.
I can see that pattern control registers allow to set 3-bit
intensity for each color. I assume that that the same can't
be applied for non-pattern use cases?

>>
>>> +
>>> +    lp3952_set_brightness(cdev, val);
>>> +}
>>> +
>>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
>>> +{
>>> +    int ret, i;
>>> +    const char *led_name[] = {
>>> +        "blue2",
>>> +        "green2",
>>> +        "red2",
>>> +        "blue1",
>>> +        "green1",
>>> +        "red1"
>>> +    };
>>
>> According to Documentation/leds/leds-class.txt LED class device name
>> should match following pattern: devicename:colour:function. Colour
>> can be omitted if not relevant.
>>
>>> +    for (i = 0; i < priv->ndev; i++) {
>>> +        sprintf(priv->leds[i].name, "%s", led_name[i]);
>>> +        priv->leds[i].cdev.name = priv->leds[i].name;
>>> +        priv->leds[i].cdev.brightness = LED_OFF;
>>> +        priv->leds[i].cdev.max_brightness = LED_FULL;
>>
>> max_brightness should reflect the maximum brightness level allowed for
>> given LED. LED_FULL is a legacy enum, that max_brightness property
>> overrides.
>>
>
> lp3952_brightness_ctrl should convert the legacy value to value
> understood by the chip. The function ( + lp3952_set_brightness)
> should convert users max request 255 to chips max supported value 3.

You don't need the conversion as I explained above.

>>> +        priv->leds[i].cdev.brightness_set_blocking =
>>> +            lp3952_brightness_ctrl;
>>> +        priv->leds[i].channel = i;
>>> +        priv->leds[i].priv = priv;
>>> +
>>> +        ret = led_classdev_register(&priv->client->dev,
>>> +                        &priv->leds[i].cdev);
>>
>> Please use devm prefixed version.
>>
>>> +        if (ret < 0) {
>>> +            dev_err(&priv->client->dev,
>>> +                "couldn't register LED %s\n",
>>> +                priv->leds[i].cdev.name);
>>> +            goto error;
>>> +        }
>>> +    }
>>> +    return 0;
>>> +
>>> +error:
>>> +    for (; i >= 0; i--)
>>> +        led_classdev_unregister(&priv->leds[i].cdev);
>>> +    return ret;
>>> +}
>>> +
>>> +static void lp3952_unregister_led_classdev(struct lp3952_led_array *priv)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < priv->ndev; i++)
>>> +        led_classdev_unregister(&priv->leds[i].cdev);
>>> +}
>>> +
>>> +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)
>>> +{
>>> +    struct ptrn_gen_cmd line = {
>>> +        .r = r,
>>> +        .g = g,
>>> +        .b = b,
>>> +        .cet = cet,
>>> +        .tt = tt
>>> +    };
>>> +    if (cmd_index >= 8)
>>> +        return -EINVAL;
>>> +
>>> +    priv->pgm[cmd_index] = line;
>>> +    lp3952_register_write(priv->client, LP3952_REG_CMD_0 + cmd_index * 2,
>>> +                  line.bytes.msb);
>>
>> Please check return value.
>>
>>> +    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;
>>
>> Empty line here please.
>>
>>> +    /* enable rgb patter, loop */
>>> +    lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, 0x06);
>>
>> Please check return value and add empty line after that.
>>
>>> +    /* Update Bit 6 (Active mode),1 and 0 for pattern Gen */
>>> +    ret = regmap_update_bits(priv->regmap, LP3952_REG_ENABLES, 0x43, 0xfc);
>>
>> Please add bit definitions for all the register bit fields.
>>
>>> +    if (ret)
>>> +        return ret;
>>
>> Empty line here please.
>>
>>> +    /* Set Cmd1 for RGB intensity,cmd and transition time */
>>> +    return (lp3952_set_pattern_gen_cmd(priv, 0, I46, I71, I100, TT0,
>>> +                       CET197));
>>> +}
>>> +
>>> +static const struct regmap_config lp3952_regmap = {
>>> +    .reg_bits = 8,
>>> +    .val_bits = 8,
>>> +    .max_register = REG_MAX,
>>> +};
>>> +
>>> +static int lp3952_probe(struct i2c_client *client,
>>> +            const struct i2c_device_id *id)
>>> +{
>>> +    int status;
>>> +    struct lp3952_ctrl_hdl *leds;
>>> +    struct lp3952_led_array *priv;
>>> +    struct lp3952_platform_data *pdata = dev_get_platdata(&client->dev);
>>> +
>>> +    dev_info(&client->dev, "lp3952 probe\n");
>>
>> I believe this is stray debug log. Please remove it.
>>
>>> +
>>> +    if (!pdata) {
>>> +        dev_err(&client->dev,
>>> +            "lp3952: failed to obtain platform_data\n");
>>> +        return -EINVAL;
>>> +    }
>>> +    priv = kzalloc(sizeof(struct lp3952_led_array), GFP_KERNEL);
>>
>> Please use devm prefixed version.
>>
>>> +    if (!priv)
>>> +        return -ENOMEM;
>>> +
>>> +    priv->ndev = 6;
>>> +
>>> +    leds = kcalloc(priv->ndev, sizeof(*leds), GFP_KERNEL);
>>
>> Please use devm prefixed version.
>>
>>> +    if (!leds) {
>>> +        kfree(priv);
>>> +        return -ENOMEM;
>>> +    }
>>> +    priv->leds = leds;
>>> +    priv->client = client;
>>> +    priv->enable_gpio = pdata->enable_gpio;
>>> +    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;
>>> +    }
>>
>> Empty line here please.
>>
>>> +    status = gpio_request(priv->enable_gpio, "lp3952_gpio");
>>> +    if (status)
>>> +        dev_warn(&client->dev, "Unable to disable reset gpio: %d\n",
>>> +             status);
>>
>> Empty line here please.
>>
>>> +    status = gpio_direction_output(priv->enable_gpio, 1);
>>> +    if (status)
>>> +        dev_warn(&client->dev, "Unable to disable reset gpio: %d\n",
>>> +             status);
>>
>> Empty line here please.
>>
>>> +    i2c_set_clientdata(client, priv);
>>> +
>>> +    status = lp3952_configure(priv);
>>> +    if (status) {
>>> +        dev_err(&client->dev, "Probe failed. Device not found (%d)\n",
>>> +            status);
>>> +        kfree(leds);
>>> +        kfree(priv);
>>> +        return status;
>>> +    }
>>
>> Empty line here please.
>>
>>> +    lp3952_register_led_classdev(priv);
>>
>> Please check return value.
>>
>>> +
>>> +    lp3952_on_off(priv, LP3952_LED_ALL, 1);
>>
>> If it is possible then device should remain in power down
>> mode if brightness equals 0. And this should be secured before
>> the LED class device is registered. Preferrably to be moved to the
>> lp3952_configure().
>>
>>> +    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);
>>> +
>>> +    lp3952_unregister_led_classdev(priv);
>>> +
>>> +    kfree(priv->leds);
>>> +    gpio_direction_input(priv->enable_gpio);
>>> +    gpio_free(priv->enable_gpio);
>>> +    kfree(priv);
>>> +    return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM
>>> +static int lp3952_suspend(struct device *dev)
>>> +{
>>> +    struct i2c_client *client = to_i2c_client(dev);
>>> +    struct lp3952_led_array *priv = i2c_get_clientdata(client);
>>> +
>>> +    lp3952_on_off(priv, LP3952_LED_ALL, 0);
>>> +    gpio_direction_input(priv->enable_gpio);
>>> +    return 0;
>>> +}
>>> +
>>> +static int lp3952_resume(struct device *dev)
>>> +{
>>> +    struct i2c_client *client = to_i2c_client(dev);
>>> +    struct lp3952_led_array *priv = i2c_get_clientdata(client);
>>> +
>>> +    gpio_direction_output(priv->enable_gpio, 1);
>>> +    return 0;
>>> +}
>>
>> Power management is already handled in led-class.c. Please
>> refer to led_suspend(). It sets brightness to 0, and thus the LED
>> class drivers are expected to turn the devices they control into
>> power down mode in case all LEDs controlled by them are set to LED_OFF.
>>
>> Therefore you should control the "enable_gpio" state in the
>> brightness_set_blocking() op.
>>
>
> Removed the suspend/resume routine within the driver. Power management
> in led-class.c should be enough.
>
>>> +
>>> +static SIMPLE_DEV_PM_OPS(lp3952_pm, lp3952_suspend, lp3952_resume);
>>> +#define LP3952_PM (&lp3952_pm)
>>> +#else /* CONFIG_PM */
>>> +#define LP3952_PM NULL
>>> +#endif
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +static const struct acpi_device_id lp3952_acpi_match[] = {
>>> +    {LP3952_NAME, 0},
>>> +    {}
>>> +};
>>>
>>> +
>>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
>>> +#endif
>>> +
>>> +static const struct i2c_device_id lp3952_id[] = {
>>> +    {LP3952_NAME, 0},
>>> +    {}
>>> +};
>>> +
>>> +static struct i2c_driver lp3952_i2c_driver = {
>>> +    .driver = {
>>> +           .name = LP3952_NAME,
>>> +           .owner = THIS_MODULE,
>>> +           .pm = LP3952_PM,
>>> +#ifdef CONFIG_ACPI
>>> +           .acpi_match_table = ACPI_PTR(lp3952_acpi_match),
>>> +#endif
>>
>> Did you test booting using ACPI? Would it be possible to switch
>> to using Device Tree? This device is controlled via I2C so this
>> would be a more suitable way.
>>
>
> My host platform runs intel chipset which prefers ACPI( dont have firmware code).
> I couldn't test it as ACPI device. But have followed instructions in
> Documentation/acpi/enumeration.txt.
>
> I tested the led controller registering it, from host board
> init file.
>
> #include <linux/leds-lp3952.h>
>
> static struct lp3952_platform_data lp3952_data = {
>         .enable_gpio = LP3952_NRST_GPIO
> };
>
> static struct i2c_board_info __initdata lp3952_i2c_board_info = {
>         I2C_BOARD_INFO(LP3952_NAME, LP3952_DEV_ADDR),
>         .platform_data = &lp3952_data,
> };
>
> then i2c_new_device(adap, &lp3952_i2c_board_info);
>
> from host board initialisation.
>
>
> Should I remove the ACPI registration?

I'd like to have ACPI maintainer's ack for this.

Cc linux-acpi list and maintainers.

>>> +           },
>>> +    .probe = lp3952_probe,
>>> +    .remove = __exit_p(lp3952_remove)
>>
>> If not building the driver as a module I am getting the following:
>>
>> drivers/leds/leds-lp3952.c:337:12: warning: ‘lp3952_remove’ defined but not used
>>
>> Is there some specific reason for which you don't want to have
>> the "remove" op initialized when driver is built into the kernel?
>>
>> What config are you using for building this driver?
>>
>
> I have been building it as module. I assumed, if it was built in, wont be using
> remove op.  I have removed the __exit_p so that it is built in all the time.
> I have tested the module as built in. The warning is gone, with the change.
>
>> ,
>>> +    .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..1b1e7c3
>>> --- /dev/null
>>> +++ b/include/linux/leds-lp3952.h
>>> @@ -0,0 +1,89 @@
>>> +/*
>>> + * Copyright (C) 2016, DAQRI LLC
>>> + *
>>> + * License Terms: GPL v2
>>> + *
>>> + * TI lp3952 Controller driver
>>> + *
>>> + * Author: Tony Makkiel <tony.makkiel@daqri.com>
>>> + *
>>> + */
>>> +
>>> +#ifndef LEDS_LP3952_H_
>>> +#define LEDS_LP3952_H_
>>> +
>>> +#define LP3952_NAME             "lp3952"
>>> +#define LP3952_DEV_ADDR         0x54
>>> +/* Following 2 MACRO are specific to Minnowboard Max
>>> + * Use the appropriate host specific values when
>>> + * instantiating device
>>> + */
>>> +#define LP3952_BUS_ADDR         7
>>> +#define LP3952_NRST_GPIO        464
>>> +
>>> +/* 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_brightness {
>>> +    LP3952_OFF = 0,
>>> +    LP3952_QUARTER_BRIGHT,
>>> +    LP3952_HALF_BRIGHT,
>>> +    LP3952_THREE_QRTR_BRIGHT,
>>> +    LP3952_FULL_BRIGHT
>>> +};
>>> +
>>> +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_platform_data {
>>> +    u32 enable_gpio;
>>> +};
>>> +
>>> +#endif /* LEDS_LP3952_H_ */
>>>
>>
>
>


-- 
Best regards,
Jacek Anaszewski