On Tue, 5 Jul 2022 14:22:38 +0200 Pali Rohár <pali@xxxxxxxxxx> 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@xxxxxxxxxx> 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
