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

From: Dan Murphy <dmurphy@ti.com>
To: Pavel Machek <pavel@ucw.cz>
Cc: <jacek.anaszewski@gmail.com>, <robh@kernel.org>,
	<marek.behun@nic.cz>, <devicetree@vger.kernel.org>,
	<linux-leds@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v30 05/16] leds: multicolor: Introduce a multicolor class definition
Date: Thu, 16 Jul 2020 10:06:04 -0500	[thread overview]
Message-ID: <a7cf67e7-680b-8e67-a4ee-207eff316316@ti.com> (raw)
In-Reply-To: <b298158e-327f-cf97-2723-75a7d832931e@ti.com>

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