Dan,
On 07.06.19 23:13, Dan Murphy wrote:
> Oleh
>
> On 6/7/19 2:03 PM, Oleh Kravchenko wrote:
>> +
>> +static int el15203000_set_sync(struct led_classdev *ldev,
>> + enum led_brightness brightness)
>> +{
>> + int ret;
>> + u8 cmd[2];
>
> I am wondering if you should #define this as well.
It's used only in one place, should I really wrap it to define?
>
> Then the #define can be used here and then in the for loop.
>
> There is no reason to do ARRAY_SIZE if it will always be 2.
Result of ARRAY_SIZE() will be always constant and actually it can be avoided by define.
But I prefer ARRAY_SIZE() :-)
>
>>
>> + device_for_each_child_node(priv->dev, child) {
>> + led = &priv->leds[i];
>> +
>> + ret = fwnode_property_read_u32(child, "reg", ®);
>
> Why didn't you use fwnode_property_read_u8 then you don't need to check the size of the value
Because fwnode_property_read_u8() return 0 for "reg" property and by DT standard reg has u32 type.
>
> below and you can remove the header?
>
> And then you can store the value of reg right into led->reg as that is a u8
>
>> + if (ret) {
>> + dev_err(priv->dev, "LED without ID number");
>> + fwnode_handle_put(child);
>> +
>> + return ret;
>> + }
> New line
Done
>> + if (reg > U8_MAX) {
>> + dev_err(priv->dev, "LED value %d is invalid", reg);
>> + fwnode_handle_put(child);
>> +
>> + return -EINVAL;
>> + }
> New line
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;
>> + led->ldev.brightness_set_blocking = el15203000_set_sync;
>> +
>> + ret = fwnode_property_read_u32(child, "max-brightness",
>> + &led->ldev.max_brightness);
>
> The value led->ldev.max_brightness will be over written here by this call.
>
> So setting it above will have no affect
It will have effect, because it initialized value which can be over written by fwnode_property_read_u32
> You should move the led->ldev.max_brightness = LED_ON into a ret check as this is an optional property
>
> if (ret)
>
> led->ldev.max_brightness = LED_ON;
Good point, thank you
>
> Then you can check the bounds below.
>
> As pointed out your max_brightness is a binary and the 0x32 is an effect you technically don't even need max_brightness if you
>
> expose the effects as a file.
>
> Does 0x32 turn the LED on or off? Or does it blink the LED?
It depends on LED board,
it can blink.
It can play scene on LED array.
It can blink smoothly.
But it depend on the board, not a protocol.
>
> Dan
>
Attachment:
signature.asc
Description: OpenPGP digital signature
