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

["Marek Behún" <kabel@kernel.org>]

On Tue, 5 Jul 2022 14:22:38 +0200
Pali Rohár <pali@kernel.org> wrote:

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

OK, that would be satisfactory. Something like
"selected_brightness_index".

Marek