Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs

From: "Marek Behún" <kabel@kernel.org>
To: "Pali Rohár" <pali@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] leds: Add support for Turris 1.x LEDs
Date: Tue, 5 Jul 2022 13:52:27 +0200	[thread overview]
Message-ID: <20220705135227.6380d6d5@thinkpad> (raw)
In-Reply-To: <20220705105609.cpabhrwozyeejwqe@pali>

On Tue, 5 Jul 2022 12:56:09 +0200
Pali Rohár <pali@kernel.org> wrote:

> > 
> > I don't consider this a problem  
> 
> I think it is a problem, to ensure that 'cat multi_intensity' for every

Misunderstanding. I meant that I don't consider the eventual
inconsistency a problem, i.e. I agree with your code.

> > Or maybe just write the value?
> > Is the register write expensive on the CPLD or why are you trying to
> > avoid it if unnecessary?  
> 
> I just do not see any reason to do unnecessary writes.

But now you do an unnecessary check. Unless the writeb() is slower than
that check. Since this isn't i2c, I am wondering how fast that writeb()
is... But this is just me wondering, we can keep it the way you wrote
it...

> > 
> > Hmm. Wouldn't it make more sense to simply have the global brightness
> > accept values from 0 to 7, instead of mapping it to 256 values? And
> > call it something like selected_brightness_index?  
> 
> All other drivers have brightness entry which operates on monotone
> brightness property.
> Brightness levels do not have to be monotone and by default are
> decreasing: 0 = brightness with higher intensity; 7 = no intensity (off)

What do you mean all other drivers? AFAIK only one driver does this
global brightness thing, and that is Omnia. The global brightness is
something different from LED cdev brightness property, the same names
are just coincidental (in fact it caused confusion when Pavel was
first reviewing Turris Omnia driver). Maybe it should have been called
global_intensity, to avoid the confusion...

> I cannot image who would like or prefer usage of such API.

One file that represents the index of the selected global intensity (as
is stored internally in the CPLD) and another file that represents the
configured intensities between which the button switches makes sense,
IMO.

> Just stick with existing APIs. "brightness" entry takes intensity value
> which is monotone, 0 the lowest, MAX (=255) the highest.

Again, the name "brightness" does not imply that it is the same thing
as "brightness" of a LED cdev. And since it even doesn't live in
/sys/class/<led>/ directory, we are proposing new API and can use
whatever makes sense.

I am not saying that the way you did it doesn't make sense. I am just
wondering if it wouldn't make more sense to be able to read the index
of what the user selected by button pressing.

Marek