Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: Dan Murphy <dmurphy@ti.com>
To: Pavel Machek <pavel@ucw.cz>
Cc: <jacek.anaszewski@gmail.com>, <robh@kernel.org>,
	<devicetree@vger.kernel.org>, <linux-leds@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v29 03/16] leds: multicolor: Introduce a multicolor class definition
Date: Mon, 13 Jul 2020 08:34:43 -0500
Message-ID: <ab537d8d-00e3-fcad-d530-264ffeccdf30@ti.com> (raw)
In-Reply-To: <20200711155734.GA21726@amd>

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

  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-22 18:59 [PATCH v29 00/16] Multicolor Framework v29 Dan Murphy
2020-06-22 18:59 ` [PATCH v29 01/16] dt: bindings: Add multicolor class dt bindings documention Dan Murphy
2020-07-04 12:48   ` Pavel Machek
2020-07-09 19:24   ` Rob Herring
2020-07-12 19:52     ` Pavel Machek
2020-06-22 18:59 ` [PATCH v29 02/16] leds: Add multicolor ID to the color ID list Dan Murphy
2020-06-22 18:59 ` [PATCH v29 03/16] leds: multicolor: Introduce a multicolor class definition Dan Murphy
2020-07-11 15:57   ` Pavel Machek
2020-07-13 13:34     ` Dan Murphy [this message]
2020-06-22 18:59 ` [PATCH v29 04/16] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers Dan Murphy
2020-07-09 19:25   ` Rob Herring
2020-06-22 18:59 ` [PATCH v29 05/16] leds: lp50xx: Add the LP50XX family of the RGB LED driver Dan Murphy
2020-07-12 17:21   ` Marek Behun
2020-07-13 12:36     ` Dan Murphy
2020-06-22 18:59 ` [PATCH v29 06/16] dt-bindings: leds: Convert leds-lp55xx to yaml Dan Murphy
2020-07-09 19:25   ` Rob Herring
2020-06-22 18:59 ` [PATCH v29 07/16] ARM: dts: n900: Add reg property to the LP5523 channel node Dan Murphy
2020-06-22 18:59 ` [PATCH v29 08/16] ARM: dts: imx6dl-yapp4: Add reg property to the lp5562 " Dan Murphy
2020-06-22 18:59 ` [PATCH v29 09/16] ARM: dts: ste-href: Add reg property to the LP5521 channel nodes Dan Murphy
2020-06-22 18:59 ` [PATCH v29 10/16] leds: lp55xx: Convert LED class registration to devm_* Dan Murphy
2020-06-22 18:59 ` [PATCH v29 11/16] leds: lp55xx: Add multicolor framework support to lp55xx Dan Murphy
2020-06-22 18:59 ` [PATCH v29 12/16] ARM: defconfig: u8500: Add LP55XX_COMMON config flag Dan Murphy
2020-07-11 15:57   ` Pavel Machek
2020-06-22 18:59 ` [PATCH v29 13/16] leds: lp5523: Update the lp5523 code to add multicolor brightness function Dan Murphy
2020-07-11 15:57   ` Pavel Machek
2020-07-11 17:19     ` Jacek Anaszewski
2020-07-11 20:24       ` Pavel Machek
2020-07-12 15:31         ` Jacek Anaszewski
2020-06-22 18:59 ` [PATCH v29 14/16] leds: lp5521: Add multicolor framework multicolor brightness support Dan Murphy
2020-06-22 18:59 ` [PATCH v29 15/16] leds: lp55xx: Fix file permissions to use DEVICE_ATTR macros Dan Murphy
2020-06-22 18:59 ` [PATCH v29 16/16] leds: lp5523: Fix various formatting issues in the code Dan Murphy
2020-07-04 12:47 ` [PATCH v29 00/16] Multicolor Framework v29 Pavel Machek
2020-07-06 12:31   ` Dan Murphy
2020-07-07 15:36     ` Dan Murphy
2020-07-11 20:29   ` Pavel Machek
2020-07-12 17:13   ` Marek Behun
2020-07-12 19:55     ` Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ab537d8d-00e3-fcad-d530-264ffeccdf30@ti.com \
    --to=dmurphy@ti.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jacek.anaszewski@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=robh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git