Dan,
07.06.19 22:41, Dan Murphy пише:
> Oleh
>
> On 6/7/19 12:46 PM, Oleh Kravchenko wrote:
>>> These can be #defined which makes them desctiptive
>>>
>>> #define EL15203000_LED_OFF 0x30
>>>
>>> #define EL15203000_LED_ON 0x31
>>>
>>> and so on
>> Hm, but just adding 0x30 not will be more clear and faster?
>> I think switch .. case or if .. else, will be more hard to read :)
>
> Huh?
>
> You had to explain what the value 0x3X meant in a comment. So clarity is not there. Faster?
>
> The #define LED_?? makes sense without having to explain the protocol. What is 0x30?
>
> And I am not seeing any switch..case or if..else in the code using these values but if it was defined it would be easier to read.
>
> Why would this value be in a switch..case or if..else if it is just a protocol value?
>
> You have 1 line of code that uses the 0x30 so maybe you don't need to define LED_ON and LED_OFF but this value means something
>
> and that should be #defined as there is no understanding what the 0x30 is actually doing.
>
> cmd[1] = EL15203000_LED_???? + (u8)brightness;
I described it in top of the file. Looks likes it's not clear.
LED board has next brightness level:
'0' - 0x30
'1' - 0x31
'2' - 0x32
>>>> + if (reg > U8_MAX) {
>>>> + dev_err(priv->dev, "LED value %d is invalid", reg);
>>>> + fwnode_handle_put(child);
>>>> +
>>>> + return -ENODEV;
>>> -EINVAL
>> Done
>>
>>>> + }
>>>> + led->reg = reg;
>>>> +
>>>> + ret = fwnode_property_read_string(child, "label", &str);
>>>> + if (ret)
>>>> + snprintf(led->name, sizeof(led->name),
>>>> + "el15203000::");
>>>> + else
>>>> + snprintf(led->name, sizeof(led->name),
>>>> + "el15203000:%s", str);
>>>> +
>>>> + fwnode_property_read_string(child, "linux,default-trigger",
>>>> + &led->ldev.default_trigger);
>>>> +
>>>> + led->priv = priv;
>>>> + led->ldev.name = led->name;
>>>> + led->ldev.max_brightness = LED_ON;
>>> Do not need this as it should be stored when doing the fwnode read.
>> This is default value 1, by dtb we can override it to 2.
>
> This has code some other issues. I will comment in your v2.
Thanks
> Dan
>
--
Best regards,
Oleh Kravchenko
Attachment:
signature.asc
Description: OpenPGP digital signature
