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

Pavel Machek
2020-07-11T15:59:22Z

[-- Attachment #1.1: Type: text/plain, Size: 3805 bytes --]

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".

> + 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"?

> + /sys/class/leds/<led>/max_brightness.

> +Multicolor Class Brightness Control
> +===================================
> +The multicolor class framework will calculate each monochrome LEDs intensity.

?

> +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.

Please remove the dev_dbg()s that can be triggered by userspace. We
don't want users spamming the logs.

> +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.

> +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?

> +MODULE_LICENSE("GPL v2");

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

> +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?

Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]