From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 57EC1C2D0E5 for ; Sat, 28 Mar 2020 14:03:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 19CEF206E6 for ; Sat, 28 Mar 2020 14:03:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="T51wljFK" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726403AbgC1ODi (ORCPT ); Sat, 28 Mar 2020 10:03:38 -0400 Received: from mail-wm1-f66.google.com ([209.85.128.66]:54231 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726045AbgC1ODh (ORCPT ); Sat, 28 Mar 2020 10:03:37 -0400 Received: by mail-wm1-f66.google.com with SMTP id b12so14620043wmj.3; Sat, 28 Mar 2020 07:03:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:autocrypt:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=ib9hdHx7aw2wBAK621ZCWbG4MrIOtgk82DQaixuaIvc=; b=T51wljFKcTd3W4VtPn9B9GybI6f888qOSpmBxHPkF5u5GhqadlGMT2x1PUBg89b3AF YBXhTe+6pXDxDf8wfIhUmuz0l8nzYEUQ2pMUg6cqgre/G5a4nLJJ4m9LtP3fCEEv73Dd nyCscvKbx3GJ6Le2qYeMSsw4RbIKgFBK231MlTP4e0GnySEt4zkwuSnBoCuXVcQkmSsi mcHUfkgV78uXsCARDYexWqkp3PxJ5A0pHPAZJ47o5A8Y13tuRZIAuK/xZVDPpWq19eLX TigemXq0Ufhexg9S+HutPFbrz80hyp+9Yx5Mcmc4pz3f+DqpwN5VuS6V7Cfu9h5Eaqrd Q9xw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:autocrypt :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=ib9hdHx7aw2wBAK621ZCWbG4MrIOtgk82DQaixuaIvc=; b=mnmXkHDTBMdYXODvrnvGZiJWjXiT0YZ0rn0FTZ25J/Euoymrf+gOBv5rbcpCaA3C4Y mMhMxLZManZPIkbzuRywyO6eDxQj7FPDh5rHpWd+oX/qNOXO5Ft2CMvsGzJQD0fA0EHA qY6PuIZSQ9UyButPBCxY15lBBzJjPCqnhuNmrHgN3h8a8KpojUCgiePFTER+Tw9YBxOP O8gMi7g1E3Hs05Pytc7UdP2Qt6/XnPZTSDLT2hPIyEUXGYMSZWj3ed/WHBwdb4fAI9DM CAZA1L67G9UOwOPUfIYQsqFKN2mgfIZjQqElTY2Ky8cC5VwUdaLUwHjIJnvIWMyAsdvr 9j7w== X-Gm-Message-State: ANhLgQ0cJQ3zTuGKVdDE0q+zsOLUOdEKDO+Epjp/OgAsoHN3CKxDZMUq 0Gm5cbrLc0dO9riUlMvEASUSIiSf X-Google-Smtp-Source: ADFU+vtF2Vett1yhauMr4PkO9KSwKH76GOsE3LfddbObyefYS9tw27b77UboSbXZwZ9aczqXLANjaw== X-Received: by 2002:a1c:998b:: with SMTP id b133mr4068525wme.130.1585404213328; Sat, 28 Mar 2020 07:03:33 -0700 (PDT) Received: from [192.168.1.23] (afgh70.neoplus.adsl.tpnet.pl. [95.49.163.70]) by smtp.gmail.com with ESMTPSA id 71sm13834963wrc.53.2020.03.28.07.03.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 28 Mar 2020 07:03:32 -0700 (PDT) Subject: Re: [PATCH v18 4/4] leds: multicolor: Introduce a multicolor class definition To: Dan Murphy , pavel@ucw.cz Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org, Greg KH References: <20200324181434.24721-1-dmurphy@ti.com> <20200324181434.24721-5-dmurphy@ti.com> From: Jacek Anaszewski Autocrypt: addr=jacek.anaszewski@gmail.com; prefer-encrypt=mutual; keydata= xsFNBFWjfaEBEADd66EQbd6yd8YjG0kbEDT2QIkx8C7BqMXR8AdmA1OMApbfSvEZFT1D/ECR eWFBS8XtApKQx1xAs1j5z70k3zebk2eeNs5ahxi6vM4Qh89vBM46biSKeeX5fLcv7asmGb/a FnHPAfQaKFyG/Bj9V+//ef67hpjJWR3s74C6LZCFLcbZM0z/wTH+baA5Jwcnqr4h/ygosvhP X3gkRzJLSFYekmEv+WHieeKXLrJdsUPUvPJTZtvi3ELUxHNOZwX2oRJStWpmL2QGMwPokRNQ 29GvnueQdQrIl2ylhul6TSrClMrKZqOajDFng7TLgvNfyVZE8WQwmrkTrdzBLfu3kScjE14Q Volq8OtQpTsw5570D4plVKh2ahlhrwXdneSot0STk9Dh1grEB/Jfw8dknvqkdjALUrrM45eF FM4FSMxIlNV8WxueHDss9vXRbCUxzGw37Ck9JWYo0EpcpcvwPf33yntYCbnt+RQRjv7vy3w5 osVwRR4hpbL/fWt1AnZ+RvbP4kYSptOCPQ+Pp1tCw16BOaPjtlqSTcrlD2fo2IbaB5D21SUa IsdZ/XkD+V2S9jCrN1yyK2iKgxtDoUkWiqlfRgH2Ep1tZtb4NLF/S0oCr7rNLO7WbqLZQh1q ShfZR16h7YW//1/NFwnyCVaG1CP/L/io719dPWgEd/sVSKT2TwARAQABzS1KYWNlayBBbmFz emV3c2tpIDxqYWNlay5hbmFzemV3c2tpQGdtYWlsLmNvbT7Cwa8EEwEIAEICGwMHCwkIBwMC AQYVCAIJCgsDFgIBAh4BAheAAhkBFiEEvx38ClaPBfeVdXCQvWpQHLeLfCYFAl5O5twFCRIR arsAIQkQvWpQHLeLfCYWIQS/HfwKVo8F95V1cJC9alAct4t8JhIgEACtWz3zR5uxaU/GozHh iZfiyUTomQpGNvAtjjZE6UKO/cKusCcvOv0FZbfGDajcMIU8f3FUxJdybrY86KJ9a3tOddal KtB2of3/Ot/EIQjpQb28iLoY8AWnf9G4LQZtoXHiUcOAVPkKgCFnz1IENK3uvyCB9c9//KhE cRZkeAIE2sTmcI4k7/dNHpRI4nha/ZytPwTdM3BjAfxxQI5nMLptm1ksEBI7W1SDOnY3dG2J QWmqpxIefjgyiy0aU+jAw1x3RdZrokVD8OCJiJM8+Z36imarEzqIRQLh+sDNLfV3wEaBn/HU 0Vj6VrRyW2K0jAYToRFD3Ay/eGSfOOAEr/LoMr3NBTDkRLEWdOozllOwADEY9wH0BLHMp2WI hXGOStNiroIEhW2/E0udFJo9b3VoOWKWl+zcUP/keLxVUCXhpmeS7VpSkqsrCVqTVkEc8AXq xhJXeIQJC/XRpCYFc3pFUlVCFViF1ZU2OzE8TndRzzD8e/9ETrJ1GAYa78tNopYhY6AbGlv4 U01nIC93bK07O4IhtBAKsiUz3JPX/KA/dXJOC86qP373cVWVYPvZW+KOya9/7rz0MGR1az9G HqJB7q7DVcCQKt9Egae/goznnXbET6ivCNKbqkH3n/JpiPIxkaXVrbn3QlVtzYpROsS/pCOp 5Evig7kql5L0aYJIZs4zBFsKioYWCSsGAQQB2kcPAQEHQFCKEG5pCgebryz66pTa9eAo+r8y TkMEEnG8UR5oWFt3wsIbBBgBCAAgFiEEvx38ClaPBfeVdXCQvWpQHLeLfCYFAlsKioYCGwIA rwkQvWpQHLeLfCaNIAQZFggAHRYhBBTDHErITmX+em3wBGIQbFEb9KXbBQJbCoqGACEJEGIQ bFEb9KXbFiEEFMMcSshOZf56bfAEYhBsURv0pdvELgD/U+y3/hsz0bIjMQJY0LLxM/rFY9Vz 1L43+lQHXjL3MPsA/1lNm5sailsY7aFBVJxAzTa8ZAGWBdVaGo6KCvimDB8GFiEEvx38ClaP BfeVdXCQvWpQHLeLfCbuOg/+PH6gY6Z1GiCzuYb/8f7D0NOcF8+md+R6KKiQZij/6G5Y7lXQ Bz21Opl4Vz/+39i5gmfBa9LRHH4ovR9Pd6H0FCjju4XjIOJkiJYs2HgCCm6nUxRJWzPgyMPS VbqCG2ctwaUiChUdbS+09bWb2MBNjIlI4b8wLWIOtxhyn25Vifm0p+QR5A2ym4bqJJ9LSre1 qM8qdPWcnExPFU4PZFYQgZ9pX1Jyui73ZUP94L7/wg1GyJZL3ePeE4ogBXldE0g0Wq3ORqA9 gA/yvrCSyNKOHTV9JMGnnPGN+wjBYMPMOuqDPC/zcK+stdFXc6UbUM1QNgDnaomvjuloflAx aYdblM26gFfypvpFb8czcPM+BP6X6vWk+Mw9+8vW3tyK9lSg+43OjIWlBGPpO9aLZsYYxAqv J5iSxcbbOLb5q8wWct6U7EZ1RnuOfVInoBttrlYvdWtcI/5NQTptkuB/DyRhrxBJc/fKzJ4w jS2ikcWe0FnxrQpcE2yqoUIFaZMdd/Cx9bRWAGZG087t5dUHJuMnVVcpHZFnHBKr8ag1eH/K tFdDFtyln5A/f9O22xsV0pyJni7e2z7lTBitrQFG69vnVGJlHbBE2dR4GddZqAlVOUbtEcE7 /aMk4TrCtx0IyOzQiLA81aaJWhkD3fRO8cDlR4YQ3F0aqjYy8x1EnnhhohHOwU0EVaN9oQEQ AMPNymBNoCWc13U6qOztXrIKBVsLGZXq/yOaR2n7gFbFACD0TU7XuH2UcnwvNR+uQFwSrRqa EczX2V6iIy2CITXKg5Yvg12yn09gTmafuoIyKoU16XvC3aZQQ2Bn3LO2sRP0j/NuMD9GlO37 pHCVRpI2DPxFE39TMm1PLbHnDG8+lZql+dpNwWw8dDaRgyXx2Le542CcTBT52VCeeWDtqd2M wOr4LioYlfGfAqmwcwucBdTEBUxklQaOR3VbJQx6ntI2oDOBlNGvjnVDzZe+iREd5l40l+Oj TaiWvBGXkv6OI+wx5TFPp+BM6ATU+6UzFRTUWbj+LqVA/JMqYHQp04Y4H5GtjbHCa8abRvBw IKEvpwTyWZlfXPtp8gRlNmxYn6gQlTyEZAWodXwE7CE+KxNnq7bPHeLvrSn8bLNK682PoTGr 0Y00bguYLfyvEwuDYek1/h9YSXtHaCR3CEj4LU1B561G1j7FVaeYbX9bKBAoy/GxAW8J5O1n mmw7FnkSHuwO/QDe0COoO0QZ620Cf9IBWYHW4m2M2yh5981lUaiMcNM2kPgsJFYloFo2XGn6 lWU9BrWjEoNDhHZtF+yaPEuwjZo6x/3E2Tu3E5Jj0VpVcE9U1Zq/fquDY79l2RJn5ENogOs5 +Pi0GjVpEYQVWfm0PTCxNPOzOzGR4QB3BNFvABEBAAHCwZMEGAEIACYCGwwWIQS/HfwKVo8F 95V1cJC9alAct4t8JgUCXk7nGAUJEhFq9wAhCRC9alAct4t8JhYhBL8d/ApWjwX3lXVwkL1q UBy3i3wmVBwP/RNNux3dC513quZ0hFyU6ZDTxbiafprLN2PXhmLslxPktJgW/xO5xp16OXkW YgNI/TKxj3+oSu+MhEAhAFA2urFWHyqedfqdndQTzbv4yqNuyhGupzPBWNSqqJ2NwKJc9f2R wqYTXVYIO+6KLa32rpl7xvJISkx06s70lItFJjyOf6Hn1y5RBMwQN9hP2YxLhYNO3rmlNSVy 7Z/r95lZTDnnUCuxBZxnjx/pMHJ8LZtKY0t7D0esA+zYGUrmoAGUpNWEBP+uSL+f8rhjSAL0 HgoRL39ixg5Bm0MzJn9z3or++Pl5bRnSvHy6OKh7rzTjCwaGoZD+6LHBwPFPlmInX1H+yHrX lu1uPAdqG5xcsZAZFTxBRMEnYu1yYebDSA9x+iulggMZQcWC2GvHCaKIpKcFY8XCxk7Hbl5c 8hcPKWOy16NLO6Y66Ws4kMedXuNUHe4zBLVlRbcYUdgT9Brw8nxmxu3KhEVsJkwOpXLUDuzo hQNfg9em95lpAK+VOTocke8PSESy3GbEtmoMueW3caSeDHb5dRP6WrndaYhEOzAA/KjuPU7J LMXOABOMIq+R38y7e2B3TnVDCrccdZDseFPUWmH0cGCGihH/j2UZG+PImrSDCh3h5MedVHGo sI62tmWm0q6lrljwSZmMZ30w1QaGmdFpI3Q6V+nZ7TZldI3x Message-ID: Date: Sat, 28 Mar 2020 15:03:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <20200324181434.24721-5-dmurphy@ti.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Dan, Thanks for the update. The picture would be more complete if the patch set contained a client though. Anyway, please find my review remarks below. On 3/24/20 7:14 PM, Dan Murphy wrote: > Introduce a multicolor class that groups colored LEDs > within a LED node. > > The multi color class groups monochrome LEDs and allows controlling two > aspects of the final combined color: hue and lightness. The former is > controlled via color_intensity files and the latter is controlled s/files/file/ > via brightness file. > > CC: Greg KH > Signed-off-by: Dan Murphy > --- > A design alternative to having files that having multiple values written to a > single file is here: > > https://lore.kernel.org/patchwork/patch/1186194/ > > .../ABI/testing/sysfs-class-led-multicolor | 51 ++++ > Documentation/leds/index.rst | 1 + > Documentation/leds/leds-class-multicolor.rst | 110 +++++++++ > drivers/leds/Kconfig | 10 + > drivers/leds/Makefile | 1 + > drivers/leds/led-class-multicolor.c | 224 ++++++++++++++++++ > include/linux/led-class-multicolor.h | 124 ++++++++++ > 7 files changed, 521 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor > create mode 100644 Documentation/leds/leds-class-multicolor.rst > create mode 100644 drivers/leds/led-class-multicolor.c > create mode 100644 include/linux/led-class-multicolor.h > > diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor > new file mode 100644 > index 000000000000..a93772212c21 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor > @@ -0,0 +1,51 @@ > +What: /sys/class/leds//brightness > +Date: March 2020 > +KernelVersion: 5.8 > +Contact: Dan Murphy > +Description: read/write > + Writing to this file will update all LEDs within the group to a > + calculated percentage of what each color LED intensity is set > + to. The percentage is calculated for each grouped LED via the > + equation below: > + > + led_brightness = brightness * _intensity/_max_intensity We now have a single color_intensity file, so this needs to be rephrased. > + > + For additional details please refer to > + Documentation/leds/leds-class-multicolor.rst. > + > + The value of the color is from 0 to > + /sys/class/leds//max_brightness. > + > +What: /sys/class/leds//color_index > +Date: March 2020 > +KernelVersion: 5.8 > +Contact: Dan Murphy > +Description: read > + The color_index array, when read, will output the LED colors > + by name as they are indexed in the color_intensity array. > + > +What: /sys/class/leds//num_color_leds I would go for just num_colors. > +Date: March 2020 > +KernelVersion: 5.8 > +Contact: Dan Murphy > +Description: read > + The num_color_leds indicates the number of LEDs defined in the > + color_intensity, color_max_intensity and color_index arrays. > + > +What: /sys/class/leds//color_max_intensity > +Date: March 2020 > +KernelVersion: 5.8 > +Contact: Dan Murphy > +Description: read > + Maximum intensity level for the LED color within the array. > + The max intensities for each color must be entered based on the > + color_index array. I wonder if we should mention here that each LED within a cluster should have the same maximum intensity for linear color lightness calculation via brightness file. > + > +What: /sys/class/leds//color_intensity > +Date: March 2020 > +KernelVersion: 5.8 > +Contact: Dan Murphy > +Description: read/write > + Intensity level for the LED color within the array. > + The intensities for each color must be entered based on the > + color_index array. > diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst > index 060f4e485897..bc70c6aa7138 100644 > --- a/Documentation/leds/index.rst > +++ b/Documentation/leds/index.rst > @@ -9,6 +9,7 @@ LEDs > > leds-class > leds-class-flash > + leds-class-multicolor > ledtrig-oneshot > ledtrig-transient > ledtrig-usbport > diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst > new file mode 100644 > index 000000000000..902a281a44d4 > --- /dev/null > +++ b/Documentation/leds/leds-class-multicolor.rst > @@ -0,0 +1,110 @@ > +==================================== > +Multi Color LED handling under Linux > +==================================== > + > +Description > +=========== > +The multi color class groups monochrome LEDs and allows controlling two > +aspects of the final combined color: hue and lightness. The former is > +controlled via the color_intensity array file and the latter is controlled > +via brightness file. > + > +For more details on hue and lightness notions please refer to > +https://en.wikipedia.org/wiki/CIECAM02. > + > +Note that intensity file only caches the written values and the actual s/intensity/color_intensity/ > +change of hardware state occurs upon writing brightness file. This > +allows for changing many factors of the perceived color in a virtually > +unnoticeable way for the human observer. It will have to be changed after we agree upon exact semantics and interface of color_intensity, as I raised an issue below. > + > +Multicolor Class Control > +======================== > +The multicolor class presents files that groups the colors as indexes in an > +array. These files are children under the LED parent node created by the > +led_class framework. The led_class framework is documented in led-class.rst > +within this documentation directory. > + > +Each colored LED will be indexed under the color_* files. The order of the > +colors are arbitrary the color_index file can be read to determine the color > +to index value. This paragraph seems to be a leftover from some older version of the patch set. > + > +The color_index file is an array that contains the string list of the colors as > +they are defined in each color_* array file. > + > +The color_intensity is an array that can be read or written to for the > +individual color intensities. All elements within this array must be written in > +order for the color LED intensities to be updated. > + > +The color_max_intensity is an array that can be read to indicate each color LED > +maximum intensity value. > + > +The num_color_leds file returns the total number of color LEDs that are > +presented in each color_* array. > + > +Directory Layout Example > +======================== > +root:/sys/class/leds/multicolor:grouped_leds# ls -lR > +-rw-r--r-- 1 root root 4096 Oct 19 16:16 brightness > +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_index > +-rw-r--r-- 1 root root 4096 Oct 19 16:16 color_intensity > +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_max_intensity > +-r--r--r-- 1 root root 4096 Oct 19 16:16 num_color_leds > + > +Multicolor Class Brightness Control > +=================================== > +The multiclor class framework will calculate each monochrome LEDs intensity. > + > +The brightness level for each LED is calculated based on the color LED > +intensity setting divided by the color LED max intensity setting multiplied by > +the requested brightness. > + > +led_brightness = brightness * color_intensity/color_max_intensity Maybe some pseudo code would allow for better understanding here: for color in color_intensity led[color].brightness = brightness * led[color].intensity / led[color].max_intensity > + > +It is important to note that for each LED within the cluster the > +color_max_intensity value should be the same. This will help maintain a > +consistent hue across brightness levels. > + > +Example: > +A user first writes the color LEDs intensity file with the brightness levels s/color LEDs intensity/color_intensity/ > +that for each LED that is necessary to achieve a blueish violet output from a > +RGB LED group. > + > +cat /sys/class/leds/multicolor:grouped_leds/color_index We don't have "grouped_leds" LED function so let's better use something available, like e.g "status". > +green blue red > + > +echo 43 226 138 > /sys/class/leds/multicolor:grouped_leds/color_intensity > + > +red - > + intensity = 138 > + max_intensity = 255 > +green - > + intensity = 43 > + max_intensity = 255 > +blue - > + intensity = 226 > + max_intensity = 255 > + > +The user can control the brightness of that RGB group by writing the parent > +'brightness' control. Assuming a parent max_brightness of 255 the user may want > +to dim the LED color group to half. The user would write a value of 128 to the > +parent brightness file then the values written to each LED will be adjusted > +base on this value > + > +cat /sys/class/leds/multicolor:grouped_leds/max_brightness > +255 > +echo 128 > /sys/class/leds/multicolor:grouped_leds/brightness > + > +adjusted_red_value = 128 * 138/255 = 69 > +adjusted_green_value = 128 * 43/255 = 21 > +adjusted_blue_value = 128 * 226/255 = 113 > + > +Reading the parent brightness file will return the current brightness value of > +the color LED group. > + > +cat /sys/class/leds/multicolor:grouped_leds/max_brightness > +255 > + > +echo 128 > /sys/class/leds/multicolor:grouped_leds/brightness It was already written once above, no need to repeat. > + > +cat /sys/class/leds/multicolor:grouped_leds/brightness > +128 > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index d82f1dea3711..37dcdb06a29b 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -30,6 +30,16 @@ config LEDS_CLASS_FLASH > for the flash related features of a LED device. It can be built > as a module. > > +config LEDS_CLASS_MULTI_COLOR > + tristate "LED Mulit Color LED Class Support" > + 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. > + > config LEDS_BRIGHTNESS_HW_CHANGED > bool "LED Class brightness_hw_changed attribute support" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index d7e1107753fb..310b5518783a 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -4,6 +4,7 @@ > obj-$(CONFIG_NEW_LEDS) += led-core.o > obj-$(CONFIG_LEDS_CLASS) += led-class.o > obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o > +obj-$(CONFIG_LEDS_CLASS_MULTI_COLOR) += led-class-multicolor.o > obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o > > # LED Platform Drivers > diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c > new file mode 100644 > index 000000000000..0dbafede6e58 > --- /dev/null > +++ b/drivers/leds/led-class-multicolor.c > @@ -0,0 +1,224 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// LED Multi Color class interface > +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ I think that you deserve mentioning yourself as an author too. > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "leds.h" > + > +int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev, > + enum led_brightness brightness) > +{ > + int i; > + > + for (i = 0; i < mcled_cdev->num_colors; i++) > + mcled_cdev->color_brightness[i] = brightness * > + mcled_cdev->color_led_intensity[i] / > + mcled_cdev->color_led_max_intensity[i]; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(led_mc_calc_color_components); > + > +static ssize_t color_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 *priv = 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 < priv->num_colors; i++) { > + ret = sscanf(buf + offset, "%i%n", > + &intensity_value[i], &nrchars); > + if (ret != 1) { > + dev_err(led_cdev->dev, > + "Incorrect number of LEDs expected %i values intensity was not applied\n", > + priv->num_colors); > + goto err_out; > + } > + offset += nrchars; > + } I've just realized that moving to single color_intensity file doesn't allow setting all colors together with new brightness atomically. In effect, we will need to pass brightness to this file too, if we want to avoid "interesting" latching via brightenss file. Then we would need to call led_set_brightness() from here as well. > + > + memcpy(&priv->color_led_intensity, intensity_value, > + sizeof(intensity_value)); > +err_out: > + ret = size; > + mutex_unlock(&led_cdev->led_access); > + return ret; > +} > + > +static ssize_t color_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 *priv = lcdev_to_mccdev(led_cdev); > + int len = 0; > + int i; > + > + for (i = 0; i < priv->num_colors; i++) > + len += sprintf(buf + len, "%d ", priv->color_led_intensity[i]); > + > + len += sprintf(buf + len, "%s", "\n"); > + > + return len; > +} > +static DEVICE_ATTR_RW(color_intensity); > + > +static ssize_t color_index_show(struct device *dev, > + struct device_attribute *color_index_attr, > + char *buf) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev); > + int len = 0; > + int i; > + > + for (i = 0; i < priv->num_colors; i++) > + len += sprintf(buf + len, "%s ", priv->color_index[i]); > + > + len += sprintf(buf + len, "%s", "\n"); > + > + return len; > +} > +static DEVICE_ATTR_RO(color_index); > + > +static ssize_t color_max_intensity_show(struct device *dev, > + struct device_attribute *max_intensity_attr, > + char *buf) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev); > + int len = 0; > + int i; > + > + for (i = 0; i < priv->num_colors; i++) > + len += sprintf(buf + len, "%d ", > + priv->color_led_max_intensity[i]); > + > + len += sprintf(buf + len, "%s", "\n"); > + > + return len; > +} > +static DEVICE_ATTR_RO(color_max_intensity); > + > +static ssize_t num_color_leds_show(struct device *dev, > + struct device_attribute *max_intensity_attr, > + char *buf) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev); > + > + return sprintf(buf, "%d\n", priv->num_colors); > +} > +static DEVICE_ATTR_RO(num_color_leds); > + > +static struct attribute *led_multicolor_attrs[] = { > + &dev_attr_color_intensity.attr, > + &dev_attr_color_index.attr, > + &dev_attr_color_max_intensity.attr, > + &dev_attr_num_color_leds.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(led_multicolor); > + > +int led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data) > +{ > + struct led_classdev *led_cdev; > + int i; > + > + if (!mcled_cdev) > + return -EINVAL; > + > + if (!mcled_cdev->num_colors) > + return -EINVAL; > + > + led_cdev = &mcled_cdev->led_cdev; > + > + mcled_cdev->led_cdev.groups = led_multicolor_groups; > + > + for (i = 0; i <= mcled_cdev->num_colors; i++) > + if (!mcled_cdev->color_led_max_intensity[i]) > + mcled_cdev->color_led_max_intensity[i] = LED_FULL; > + > + return led_classdev_register_ext(parent, &mcled_cdev->led_cdev, > + init_data); > +} > +EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext); > + > +void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) > +{ > + if (!mcled_cdev) > + return; > + > + led_classdev_unregister(&mcled_cdev->led_cdev); > +} > +EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister); > + > +static void devm_led_classdev_multicolor_release(struct device *dev, void *res) > +{ > + led_classdev_multicolor_unregister(*(struct led_classdev_mc **)res); > +} > + > +int devm_led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data) > +{ > + struct led_classdev_mc **dr; > + int ret; > + > + dr = devres_alloc(devm_led_classdev_multicolor_release, > + sizeof(*dr), GFP_KERNEL); > + if (!dr) > + return -ENOMEM; > + > + ret = led_classdev_multicolor_register_ext(parent, mcled_cdev, > + init_data); > + if (ret) { > + devres_free(dr); > + return ret; > + } > + > + *dr = mcled_cdev; > + devres_add(parent, dr); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_register_ext); > + > +static int devm_led_classdev_multicolor_match(struct device *dev, > + void *res, void *data) > +{ > + struct led_classdev_mc **p = res; > + > + if (WARN_ON(!p || !*p)) > + return 0; > + > + return *p == data; > +} > + > +void devm_led_classdev_multicolor_unregister(struct device *dev, > + struct led_classdev_mc *mcled_cdev) > +{ > + WARN_ON(devres_release(dev, > + devm_led_classdev_multicolor_release, > + devm_led_classdev_multicolor_match, mcled_cdev)); > +} > +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_unregister); > + > +MODULE_AUTHOR("Dan Murphy "); > +MODULE_DESCRIPTION("Multi Color LED class interface"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h > new file mode 100644 > index 000000000000..bfbde2e98340 > --- /dev/null > +++ b/include/linux/led-class-multicolor.h > @@ -0,0 +1,124 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* LED Multicolor class interface > + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ > + */ > + > +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED > +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED > + > +#include > +#include > + > +struct led_classdev_mc { > + /* led class device */ > + struct led_classdev led_cdev; > + > + struct device_attribute color_max_intensity_attr; > + struct device_attribute color_intensity_attr; > + struct device_attribute color_index_attr; These are no longer needed as you define attrs statically. > + > + int color_brightness[LED_COLOR_ID_MAX]; > + > + int color_led_intensity[LED_COLOR_ID_MAX]; > + int color_led_max_intensity[LED_COLOR_ID_MAX]; > + const char *color_index[LED_COLOR_ID_MAX]; I think that we should get back to the available_colors bitmask and allow the framework to allocate arrays by itself. And yes, all the above should be pointers. Driver would only need to set led_mcdev->available_colors bits. > + > + int num_colors; > +}; > + > +static inline struct led_classdev_mc *lcdev_to_mccdev( > + struct led_classdev *led_cdev) > +{ > + return container_of(led_cdev, struct led_classdev_mc, led_cdev); > +} > + > +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR) > +/** > + * led_classdev_multicolor_register_ext - register a new object of led_classdev > + * class with support for multicolor LEDs > + * @parent: the multicolor LED to register > + * @mcled_cdev: the led_classdev_mc structure for this device > + * @init_data: the LED class Multi color device initialization data > + * > + * Returns: 0 on success or negative error value on failure > + */ > +int led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data); > + > +static inline int led_classdev_multicolor_register(struct device *parent, > + struct led_classdev_mc *mcled_cdev) > +{ > + return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL); > +} > + > +/** > + * led_classdev_multicolor_unregister - unregisters an object of led_classdev > + * class with support for multicolor LEDs > + * @mcled_cdev: the multicolor LED to unregister > + * > + * Unregister a previously registered via led_classdev_multicolor_register > + * object > + */ > +void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev); > + > +/* Calculate brightness for the monochrome LED cluster */ > +int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev, > + enum led_brightness brightness); > + > +int devm_led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data); > + > +static inline int devm_led_classdev_multicolor_register(struct device *parent, > + struct led_classdev_mc *mcled_cdev) > +{ > + return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev, > + NULL); > +} > + > +void devm_led_classdev_multicolor_unregister(struct device *parent, > + struct led_classdev_mc *mcled_cdev); > +#else > + > +static inline int led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data) > +{ > + return -EINVAL; > +} > + > +static inline int led_classdev_multicolor_register(struct device *parent, > + struct led_classdev_mc *mcled_cdev) > +{ > + return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL); > +} > + > +static inline void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) {}; > +static inline int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev, > + enum led_brightness brightness, > + struct led_mc_color_conversion color_components[]) > +{ > + return -EINVAL; > +} > + > +static inline int devm_led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data) > +{ > + return -EINVAL; > +} > + > +static inline int devm_led_classdev_multicolor_register(struct device *parent, > + struct led_classdev_mc *mcled_cdev) > +{ > + return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev, > + NULL); > +} > + > +static inline void devm_led_classdev_multicolor_unregister(struct device *parent, > + struct led_classdev_mc *mcled_cdev) > +{}; > + > +#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR) */ > +#endif /* __LINUX_MULTICOLOR_LEDS_H_INCLUDED */ > -- Best regards, Jacek Anaszewski