Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v20 03/17] leds: multicolor: Introduce a multicolor class
 definition
To:     Pavel Machek <pavel@ucw.cz>, Dan Murphy <dmurphy@ti.com>
Cc:     linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200423155524.13971-1-dmurphy@ti.com>
 <20200423155524.13971-4-dmurphy@ti.com> <20200425202306.GA23926@amd>
From:   Jacek Anaszewski <jacek.anaszewski@gmail.com>
Message-ID: <a19ff014-32f1-eb18-ceb6-581503821fcc@gmail.com>
Date:   Sun, 26 Apr 2020 18:24:17 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200425202306.GA23926@amd>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi all,

On 4/25/20 10:23 PM, Pavel Machek wrote:
> Hi!
> 
[...]
>> +	for (i = 0; i < mcled_cdev->num_colors; i++)
>> +		mcled_cdev->multicolor_info[i].color_brightness = brightness *
>> +					  mcled_cdev->multicolor_info[i].color_led_intensity /
>> +					  led_cdev->max_brightness;
> 
> It would be good to get this under ~80 characters. Perhaps shorter
> identifiers would help... shortening multicolor_ to mc_?


And color_led_intensity to led_intensity.

> 
>> +static ssize_t multi_led_intensity_store(struct device *dev,
>> +				struct device_attribute *intensity_attr,
>> +				const char *buf, size_t size)
>> +{
>> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
>> +	int nrchars, offset = 0;
>> +	int intensity_value[LED_COLOR_ID_MAX];
>> +	int i;
>> +	ssize_t ret;
>> +
>> +	mutex_lock(&led_cdev->led_access);
>> +
>> +	for (i = 0; i < mcled_cdev->num_colors; i++) {
>> +		ret = sscanf(buf + offset, "%i%n",
>> +			     &intensity_value[i], &nrchars);
>> +		if (ret != 1) {
>> +			dev_err(led_cdev->dev,
> 
> dev_dbg, at most. It is user-triggerable.
> 
>> +				"Incorrect number of LEDs expected %i values intensity was not applied\n",
>> +				mcled_cdev->num_colors);
>> +			goto err_out;
> 
> Should not we return -ERRNO to userspace on error?
> 
>> +		}
>> +		offset += nrchars;
>> +	}
> 
> This checks for "not enough" intensities. Do we need check for "too
> many" intensities?
> 
>> +static ssize_t multi_led_intensity_show(struct device *dev,
>> +			      struct device_attribute *intensity_attr,
>> +			      char *buf)
>> +{
>> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
>> +	int len = 0;
>> +	int i;
>> +
>> +	for (i = 0; i < mcled_cdev->num_colors; i++)
>> +		len += sprintf(buf + len, "%d ",
>> +			    mcled_cdev->multicolor_info[i].color_led_intensity);
>> +
>> +	len += sprintf(buf + len, "%s", "\n");
> 
> This will result in extra " " before end of line.
> 
> Please don't use "%s", "\n" to add single character. "\n" would be enough.
> 
> 
>> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
>> +	int len = 0;
>> +	int index;
>> +	int i;
>> +
>> +	for (i = 0; i < mcled_cdev->num_colors; i++) {
>> +		index = mcled_cdev->multicolor_info[i].color_index;
>> +		len += sprintf(buf + len, "%s ", led_colors[index]);
>> +	}
>> +
>> +	len += sprintf(buf + len, "%s", "\n");
> 
> Same here.
> 
>> +int led_classdev_multicolor_register_ext(struct device *parent,
>> +				     struct led_classdev_mc *mcled_cdev,
>> +				     struct led_init_data *init_data)
>> +{
>> +	struct led_classdev *led_cdev;
>> +
>> +	if (!mcled_cdev)
>> +		return -EINVAL;
>> +
>> +	if (!mcled_cdev->num_colors)
>> +		return -EINVAL;
> 
> if (num_colors > max)... ?
> 
>> +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED
>> +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED
> 
> Usual style is "_LINUX_MULTICOLOR_LEDS_H".
> 
>> +#else
>> +
>> +static inline  int led_classdev_multicolor_register_ext(struct device *parent,
> 
> double space after "inline".
> 
>> +					    struct led_classdev_mc *mcled_cdev,
>> +					    struct led_init_data *init_data)
>> +{
>> +	return -EINVAL;
>> +}
> 
> Do we need to include these stubs? I guess it is okay to have them,
> OTOH I'd expect drivers to depend on MULTICOLOR being available...

Single driver can support both monochrome and multicolor LED class,
which is chosen basing on DT. In this regard having no-ops for LED mc
class is justified since it is a superset of monochrome LED class.

-- 
Best regards,
Jacek Anaszewski