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

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

On 06/07/2016 02:50 PM, Tony Makkiel wrote:
>
>
> On 07/06/16 09:42, Jacek Anaszewski wrote:
>> Hi Tony,
>>
>> Thanks for the update. I have few more comments below.
>>
>> I'd rearrange this this way:
>>
>> 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;
>> }
>>
>> Also you should control here "enable_gpio", i.e. when
>> all LEDs are turned off, it should be asserted low and raised high
>> otherwise.
> Pulling the gpio low here could be a bad idea. The pin also act as a
> chip reset, which will revert all the registers to default values.
> So it might be better to toggle them only when the driver is
> loaded/removed.

Ack.

[...]

>>> +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,
>>> +                    0x06);
>>
>> Please add bit field definitions and use them here,
>> instead of 0x06 value. It makes code analysis easier.
> I did not quite understand. Bit 1 enables pattern loop, Bit 2, enables
> pattern gen. Did you mean something like
>
> value = 1 << 1 | 1 << 2;
> ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL,
>                             value);
>
> ?

You could have macros that would define values for these bits:

#define LP3952_PATTERN_LOOP BIT(1)
#define LP3952_PATTERN_GEN  BIT(2)

and then:

ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL,
                             LP3952_PATTERN_LOOP | LP3952_PATTERN_GEN)

If in doubt, please refer to the other LED class drivers.

[...]
>>> +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);
>>> +
>>> +    if (!pdata) {
>>> +        dev_err(&client->dev,
>>> +            "lp3952: failed to obtain platform_data\n");
>>> +        return -EINVAL;
>>> +    }
>>
>> Actually board files are deprecated. We use Device Tree instead now.
>> Would it be possible to switch to using it for this driver?
>> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>>
> I will remove platform driver approach. My host board use ACPI instead of
> Device tree. I do not have firmware code, for the board. But shall
> try to emulate ACPI entry for the LED from kernel.

Len, Rafael - any advise on how to approach this case?
This I2C device is present on x86 Minnowboard Max board.
I've seen ACPI overlays RFC [1], would it apply to this driver?

[1] https://lwn.net/Articles/682084/

-- 
Best regards,
Jacek Anaszewski