Thank you for the comments Jacek.
On 07/06/16 15:46, Jacek Anaszewski wrote:
> 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/
>
Also thank you for the link Jacek, managed to build kernel with ACPI
overlay. Will send updated patch, with ACPI enumeration support and
all the other changes suggested.
--
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
