Re: [PATCH v29 03/16] leds: multicolor: Introduce a multicolor class definition

Dan Murphy
2020-07-13T13:34:58Z

Pavel

Thanks for the review

On 7/11/20 10:57 AM, Pavel Machek wrote:
> Hi!
>
>> Introduce a multicolor class that groups colored LEDs
>> within a LED node.
>> +What: /sys/class/leds/<led>/multi_intensity
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <dmurphy@ti.com>
>> +Description: read/write
>> + Intensity level for the LED color within an array of integers.
> ? "This file contains array of integers".
OK
>
>> + The intensities for each color must be entered based on the
>> + multi_index array.
> This does not make sense to me. "Order of components is described by
> the multi_index array"?
>
>> The max_intensity should not exceed
> "max_intensity" -> "maximum intensity"?
OK
>
>> + /sys/class/leds/<led>/max_brightness.
>> +Multicolor Class Brightness Control
>> +===================================
>> +The multicolor class framework will calculate each monochrome LEDs intensity.
> ?
This is redundant and will be removed.
>
>> +static ssize_t multi_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_dbg(led_cdev->dev,
>> + "Incorrect number of LEDs expected %i values intensity was not applied\n",
>> + mcled_cdev->num_colors);
>> + ret = -EINVAL;
>> + goto err_out;
>> + }
>> + offset += nrchars;
>> + }
>> +
>> + /* account for the space at the end of the buffer */
>> + offset++;
> space? I'd expect \n there. And it would be good to verify it is
> indeed \n, so that for example "0 0 0b" is not accepted.
It is a new line the comment is incorrect I can remove the comment or
update the comment to account for the new line
> Please remove the dev_dbg()s that can be triggered by userspace. We
> don't want users spamming the logs.
Removed
>
>> +static ssize_t multi_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->subled_info[i].intensity);
>> + len += sprintf(buf + len, " ");
> We should not really put " " before newline.

OK I will fix that.

>> +static ssize_t multi_index_show(struct device *dev,
>> + struct device_attribute *multi_index_attr,
>> + char *buf)
>> +{
>> + for (i = 0; i < mcled_cdev->num_colors; i++) {
>> + index = mcled_cdev->subled_info[i].color_index;
>> + len += sprintf(buf + len, "%s", led_colors[index]);
>> + len += sprintf(buf + len, " ");
>> + }
> We should not really put " " before newline.
>
>> +{
>> + struct led_classdev *led_cdev;
>> +
>> + if (!mcled_cdev)
>> + return -EINVAL;
>> +
>> + if (!mcled_cdev->num_colors)
>> + return -EINVAL;
> It is plain int, so you may want to check for <= 0? Or maybe make it
> unsigned?

ok

>> +MODULE_LICENSE("GPL v2");
> If your legal department allows that, GPL v2+ would be preffered
> (globally).

OK

>
>> +struct mc_subled {
>> + int color_index;
>> + int brightness;
>> + int intensity;
>> + int channel;
>> +};
>> +
>> +struct led_classdev_mc {
>> + /* led class device */
>> + struct led_classdev led_cdev;
>> + int num_colors;
>> +
>> + struct mc_subled *subled_info;
>> +};
> Would some "unsigned"s make sense here to cut number of corner cases?

I made these unsigned.

Dan

>
> Best regards,
> Pavel