Oleh
On 6/7/19 3:05 PM, Oleh Kravchenko wrote:
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
I understand it but all I am saying is that 0x30 should have a #define
we try not to use magic numbers.
And per the DT 0x32 is not a brightness but a feature so technically
your max_brightness is 1. And you should expose a different
file for the effects. Because 0x32 is not a brightness level as it does
not affect brightness but I guess it does something special.
Dan
+ 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
