Re: [PATCH v30 05/16] leds: multicolor: Introduce a multicolor class definition

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux