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:03:56 -0500	[thread overview]
Message-ID: <b298158e-327f-cf97-2723-75a7d832931e@ti.com> (raw)
In-Reply-To: <20200716083104.GA30361@amd>

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

> Best regards,
> 									Pavel