Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: Dan Murphy <dmurphy@ti.com>
To: Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Pavel Machek <pavel@ucw.cz>
Cc: "linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [RESEND PATCH v17 00/17] Multi Color LED Framework
Date: Wed, 26 Feb 2020 16:10:20 -0600
Message-ID: <2f349232-327c-eb3a-4f3a-c22cdbe98c24@ti.com> (raw)
In-Reply-To: <20f6bdd5-e899-aead-8c35-1c3a3d09145f@gmail.com>

Hello

On 2/26/20 2:45 PM, Jacek Anaszewski wrote:
> Hi Greg,
>
> We have here long lasting discussion related to LED multicolor class
> sysfs interface design. We went through several iterations and worked
> out the solution with individual file per each color sub-LED in the
> color directory as shown below:
>
> /sys/class/leds/<led>/colors/<color>_intensity
>
> This is in line with one-value-per-file sysfs rule, that is being
> frequently highlighted, and we even had not so long ago a patch
> for led cpu trigger solving the problem caused by this rule not
> being adhered to.
>
> Now we have the voice below bringing to attention another caveat
> from sysfs documentation:
>
> "it is socially acceptable to express an array of values of the same
> type"
>
> and proposing the interface in the form of two files:
>
> channel_intensity (file containing array of u32's)
> channel_names (usually containing "red green blue")
>
> In order to have this clarified once and for all, could you please
> provide us with guidance on whether the fragment from
> Documentation/filesystems/sysfs.txt is still in force and we
> can benefit from it for LED multicolor framework, or not and then
> it should be removed from the doc then?

I spent some time trying to find the conversation on the array 
implementation for the multi color framework.

It is buried here

https://lore.kernel.org/patchwork/patch/1026515/

The response from GKH on array's was sited in the above here:

https://www.spinics.net/lists/devicetree/msg69730.html

Repeating my objections to using arrays.

Would this array of colors be fixed?  Well the answer is it would be 
fixed per DT entries.  So if a user populates 10 LEDs for color then we 
will have 10 entries in the array.  Another DT entry may only have 3.  
So the array size is variable but bounded for now assuming no additional 
colors are added.

Another issue with this implementation is we expect channel_names and 
channel_intensity arrays to be aligned.  Two different arrays need to 
line up so that each element value in channel_intensity matches the 
color in the channel_name.   This can be very easily mixed up in the 
HLOS if the proper array tracking is not done. Can we guarantee that the 
values we are getting are for the LED they are targeted for?

Would this array be bounded?  Bounded yes by the number of supported 
color IDs we have defined in the LED file.  But the size of the array is 
completely variable from device to device

Specific LEDs cannot be updated without re-sending the whole array.  If 
the user would like to change just 1 color and it is the nth position 
(possibly 10th from above) then all color intensities need to be 
passed.  There would be no way to say I want "red" at the 10th position 
to be 128 without passing in all the other color values as well.  So the 
caller needs to keep a cache of the current values or call the framework 
to get the current values just to pass them back in. This seems more 
expensive then writing 2 sysfs files as writing to the color/<color> 
intensity file does not perform any I/O it merely caches the values.  
With the array the framework would need to process all the elements in 
the array every time the array is passed.  And then still call the 
brightness file to update the color outputs.  So we still have at 
minimum two sysfs accesses.

Neither implementation is perfect.

Maybe we need to ask GKH to host this in the staging directory and 
actually get some feedback from users as opposed to imposing our will on 
people.

Not sure if the ABI's can change in staging.

Dan


  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-27 15:00 Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 01/17] dt-bindings: leds: Add multicolor ID to the color ID list Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 02/17] " Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 03/17] leds: multicolor: Introduce a multicolor class definition Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 04/17] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 05/17] leds: lp50xx: Add the LP50XX family of the RGB LED driver Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 06/17] dt: bindings: lp55xx: Be consistent in the document with LED acronym Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 07/17] dt: bindings: lp55xx: Update binding for Multicolor Framework Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 08/17] ARM: dts: n900: Add reg property to the LP5523 channel node Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 09/17] ARM: dts: imx6dl-yapp4: Add reg property to the lp5562 " Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 10/17] ARM: dts: ste-href: Add reg property to the LP5521 channel nodes Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 11/17] leds: lp55xx: Convert LED class registration to devm_* Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 12/17] leds: lp55xx: Add multicolor framework support to lp55xx Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 13/17] leds: lp5523: Update the lp5523 code to add multicolor brightness function Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 14/17] leds: lp5521: Add multicolor framework multicolor brightness support Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 15/17] leds: lp55xx: Fix checkpatch file permissions issues Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 16/17] leds: lp5523: Fix checkpatch issues in the code Dan Murphy
2020-01-27 15:00 ` [RESEND PATCH v17 17/17] dt: bindings: Update lp55xx binding to recommended LED naming Dan Murphy
2020-02-12 13:09 ` [RESEND PATCH v17 00/17] Multi Color LED Framework Dan Murphy
2020-02-25 10:19   ` Pavel Machek
2020-02-25 22:17     ` Jacek Anaszewski
2020-02-25 22:44       ` Dan Murphy
2020-02-26 12:59       ` Pavel Machek
2020-02-26 20:45         ` Jacek Anaszewski
2020-02-26 22:10           ` Dan Murphy [this message]
2020-02-27 10:58           ` Pavel Machek
2020-02-27 12:43             ` Greg KH
2020-02-27 13:07               ` Dan Murphy
2020-02-27 13:29                 ` Dan Murphy
2020-02-27 21:22                 ` Jacek Anaszewski
2020-02-28  7:42                   ` Greg KH
2020-02-28  9:34                     ` Pavel Machek
2020-02-28 12:30                     ` Dan Murphy
2020-02-28  9:39                   ` 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=2f349232-327c-eb3a-4f3a-c22cdbe98c24@ti.com \
    --to=dmurphy@ti.com \
    --cc=jacek.anaszewski@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    /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