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

["Pali Rohár" <pali@kernel.org>]

On Tuesday 05 July 2022 13:52:27 Marek Behún wrote:
> 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.

I think that testing if some bit is set in 32-bit general purpose
processor register is something which really does not play role here.

Note that readb() is always needed to do because it is required to
modify just one bit and this cannot be done without read-modify-write
operations.

> 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...

Ok. I thought "brightness" == "brightness" too.

Anyway, as Omnia has this API it makes sense to use same API for other
devices, this allows userspace software to be compatible with more
devices.

> > 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.

And this is the issue. If you want to get current brightness, you need
to read two files and then do non-trivial logic to derive current
brightness.

> > 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

So what about exporting another sysfs file which controls current level (0-7)?