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