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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx>
+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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux