From: Pavel Machek <pavel@ucw.cz> To: Dan Murphy <dmurphy@ti.com> Cc: jacek.anaszewski@gmail.com, robh@kernel.org, marek.behun@nic.cz, devicetree@vger.kernel.org, linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v30 05/16] leds: multicolor: Introduce a multicolor class definition Date: Thu, 16 Jul 2020 10:31:04 +0200 [thread overview] Message-ID: <20200716083104.GA30361@amd> (raw) In-Reply-To: <20200713154544.1683-6-dmurphy@ti.com> [-- Attachment #1: Type: text/plain, Size: 1875 bytes --] Hi! First, let's substitute multi.color -> multicolor globally, LEDS_CLASS_MULTI_COLOR is most visible example of this. Please also decide whether it is MultiColor or multicolor, and make it consistent. > Introduce a multicolor class that groups colored LEDs > within a LED node. > > The multi color class groups monochrome LEDs and allows controlling two For example here. Plus, the LEDs are not neccessarily monochrome, we support white LEDs, too. Let's use "simple LEDs"? > aspects of the final combined color: hue and lightness. The former is > controlled via the intensity file and the latter is controlled > via brightness file. > + depends on LEDS_CLASS > + help > + This option enables the multicolor LED sysfs class in /sys/class/leds. > + It wraps LED class and adds multicolor LED specific sysfs attributes > + and kernel internal API to it. You'll need this to provide support > + for multicolor LEDs that are grouped together. This class is not > + intended for single color LEDs. It can be built as a module. "single color" -> "simple"? > + /* account for the new line at the end of the buffer */ > + offset++; > + if (offset < size) { > + ret = -EINVAL; > + goto err_out; > + } "new line" -> "newline", and actually check that character you are skipping is newline. Someone could put '%' in there... > + if (i < mcled_cdev->num_colors - 1) > + len += sprintf(buf + len, " "); > + len += sprintf(buf + len, "\n"); Using sprintf for single character has... quite a lot of overhead. Something like buf[len++] = '\n' would be simpler/shorter/better. Please fix all relevant places. Note I already applied patches 1-4. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Pavel Machek <pavel@ucw.cz> To: Dan Murphy <dmurphy@ti.com> Cc: linux-arm-kernel@lists.infradead.org, robh@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, marek.behun@nic.cz, jacek.anaszewski@gmail.com, linux-leds@vger.kernel.org Subject: Re: [PATCH v30 05/16] leds: multicolor: Introduce a multicolor class definition Date: Thu, 16 Jul 2020 10:31:04 +0200 [thread overview] Message-ID: <20200716083104.GA30361@amd> (raw) In-Reply-To: <20200713154544.1683-6-dmurphy@ti.com> [-- Attachment #1.1: Type: text/plain, Size: 1875 bytes --] Hi! First, let's substitute multi.color -> multicolor globally, LEDS_CLASS_MULTI_COLOR is most visible example of this. Please also decide whether it is MultiColor or multicolor, and make it consistent. > Introduce a multicolor class that groups colored LEDs > within a LED node. > > The multi color class groups monochrome LEDs and allows controlling two For example here. Plus, the LEDs are not neccessarily monochrome, we support white LEDs, too. Let's use "simple LEDs"? > aspects of the final combined color: hue and lightness. The former is > controlled via the intensity file and the latter is controlled > via brightness file. > + depends on LEDS_CLASS > + help > + This option enables the multicolor LED sysfs class in /sys/class/leds. > + It wraps LED class and adds multicolor LED specific sysfs attributes > + and kernel internal API to it. You'll need this to provide support > + for multicolor LEDs that are grouped together. This class is not > + intended for single color LEDs. It can be built as a module. "single color" -> "simple"? > + /* account for the new line at the end of the buffer */ > + offset++; > + if (offset < size) { > + ret = -EINVAL; > + goto err_out; > + } "new line" -> "newline", and actually check that character you are skipping is newline. Someone could put '%' in there... > + if (i < mcled_cdev->num_colors - 1) > + len += sprintf(buf + len, " "); > + len += sprintf(buf + len, "\n"); Using sprintf for single character has... quite a lot of overhead. Something like buf[len++] = '\n' would be simpler/shorter/better. Please fix all relevant places. Note I already applied patches 1-4. 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 --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-07-16 8:31 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-13 15:45 [PATCH v30 00/16] Multicolor Framework v30 Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 01/16] leds: lp55xx: Fix file permissions to use DEVICE_ATTR macros Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-15 13:19 ` Marek Behún 2020-07-15 13:19 ` Marek Behún 2020-07-13 15:45 ` [PATCH v30 02/16] leds: lp5523: Fix various formatting issues in the code Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-15 13:20 ` Marek Behún 2020-07-15 13:20 ` Marek Behún 2020-07-13 15:45 ` [PATCH v30 03/16] dt: bindings: Add multicolor class dt bindings documention Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-15 13:20 ` Marek Behún 2020-07-15 13:20 ` Marek Behún 2020-07-13 15:45 ` [PATCH v30 04/16] leds: Add multicolor ID to the color ID list Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-15 13:20 ` Marek Behún 2020-07-15 13:20 ` Marek Behún 2020-07-15 17:36 ` Pavel Machek 2020-07-15 17:36 ` Pavel Machek 2020-07-15 17:59 ` Marek Behún 2020-07-15 17:59 ` Marek Behún 2020-07-13 15:45 ` [PATCH v30 05/16] leds: multicolor: Introduce a multicolor class definition Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-15 13:21 ` Marek Behún 2020-07-15 13:21 ` Marek Behún 2020-07-16 8:31 ` Pavel Machek [this message] 2020-07-16 8:31 ` Pavel Machek 2020-07-16 15:03 ` Dan Murphy 2020-07-16 15:03 ` Dan Murphy 2020-07-16 15:06 ` Dan Murphy 2020-07-16 15:06 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 06/16] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 07/16] leds: lp50xx: Add the LP50XX family of the RGB LED driver Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 08/16] dt-bindings: leds: Convert leds-lp55xx to yaml Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 09/16] leds: lp55xx: Convert LED class registration to devm_* Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 10/16] leds: lp55xx: Add multicolor framework support to lp55xx Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 11/16] ARM: defconfig: u8500: Add LP55XX_COMMON config flag Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 12/16] leds: lp5523: Update the lp5523 code to add multicolor brightness function Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 13/16] leds: lp5521: Add multicolor framework multicolor brightness support Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 14/16] ARM: dts: n900: Add reg property to the LP5523 channel node Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 15/16] ARM: dts: imx6dl-yapp4: Add reg property to the lp5562 " Dan Murphy 2020-07-13 15:45 ` Dan Murphy 2020-07-13 15:45 ` [PATCH v30 16/16] ARM: dts: ste-href: Add reg property to the LP5521 channel nodes Dan Murphy 2020-07-13 15:45 ` Dan Murphy
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=20200716083104.GA30361@amd \ --to=pavel@ucw.cz \ --cc=devicetree@vger.kernel.org \ --cc=dmurphy@ti.com \ --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=marek.behun@nic.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.