Pavel
On 7/16/20 10:03 AM, Dan Murphy wrote:
Pavel
On 7/16/20 3:31 AM, Pavel Machek wrote:
Hi!
First, let's substitute multi.color -> multicolor globally,
LEDS_CLASS_MULTI_COLOR is most visible example of this. Please also
decide whether it is MultiColor or multicolor, and make it consistent.
Dictionary definition is multicolor no space. Capitalization is
dependent on how it is use.
Basically no capital in the middle of a sentence
Introduce a multicolor class that groups colored LEDs
within a LED node.
The multi color class groups monochrome LEDs and allows controlling two
For example here. Plus, the LEDs are not neccessarily monochrome, we
support white LEDs, too. Let's use "simple LEDs"?
OK
aspects of the final combined color: hue and lightness. The former is
controlled via the intensity file and the latter is controlled
via brightness file.
+ depends on LEDS_CLASS
+ help
+ This option enables the multicolor LED sysfs class in
/sys/class/leds.
+ It wraps LED class and adds multicolor LED specific sysfs
attributes
+ and kernel internal API to it. You'll need this to provide
support
+ for multicolor LEDs that are grouped together. This class is not
+ intended for single color LEDs. It can be built as a module.
"single color" -> "simple"?
ok
+ /* account for the new line at the end of the buffer */
+ offset++;
+ if (offset < size) {
+ ret = -EINVAL;
+ goto err_out;
+ }
"new line" -> "newline", and actually check that character you are
skipping is newline. Someone could put '%' in there...
Actually we don't need to check for the character. Even if someone
put the '%' there there will still be a '\n' at the end of the buffer.
The for..loop above only processes the total number of available
colors so effectively the '%' will be ignored just like the '\n'.
If the buffer contains more entries then the number of colors an error
will be returned via the check below since size will be greater then
offset
if (offset < size) {
ret = -EINVAL;
goto err_out;
}
Maybe I should remove the comment as it is a bit confusing.
+ if (i < mcled_cdev->num_colors - 1)
+ len += sprintf(buf + len, " ");
+ len += sprintf(buf + len, "\n");
Using sprintf for single character has... quite a lot of
overhead. Something like buf[len++] = '\n' would be
simpler/shorter/better. Please fix all relevant places.
OK
Note I already applied patches 1-4.
I will rebase on top
One last change my legal team came back and said no to GPL v2+ so I
reverted that change
Dan
Best regards,
Pavel
