Messages in this thread | | | Subject | Re: [PATCH v30 05/16] leds: multicolor: Introduce a multicolor class definition | From | Dan Murphy <> | Date | Thu, 16 Jul 2020 10:06:04 -0500 |
| |
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
| |