Hi Dan, On Wed, 15 Jul 2020 14:10:34 -0500 Dan Murphy <dmurphy@xxxxxx> wrote: > >>> + cdev->max_brightness = 255; > >> This is not needed. It is defaulted to LED_FULL in led_class > > This was discussed last year and resulted in LED_FULL being > > declared obsolete in the header file. > > No I am referring to setting the max_brightness to 255 the LED class > sets this to 255 if the value is not set. I'll rather have this value here, since this is a controller specific constant. The LED class sets this to LED_FULL, which is obsolete. It could be changed to 255 there, but but I think that having this value here says to the reader that it is controller specific. > >>> +/* > >>> + * On the front panel of the Turris Omnia router there is also a > >>> button which can be used to control > >>> + * the intensity of all the LEDs at once, so that if they are too > >>> bright, user can dim them. > >>> + * The microcontroller cycles between 8 levels of this global > >>> brightness (from 100% to 0%), but this > >>> + * setting can have any integer value between 0 and 100. > >>> + * It is usable to be able to change this value from software, so > >>> that it does not start at 100% > >> This does not make sense. > > It does. The user changes the brightness of all 12 LEDs with the > > button to his liking and wants to have the same setting after > > powering the router on again. > > No the english does not make sense > > " It is usable to be able to change this value from software, so > that it does not start at 100%" > > "It is usable" is not really clear. OK I'll change it to "It is convenient to be able to change this setting from software." > >>> + * after every power on and annoy the user. > >>> + * We expose this setting via a sysfs attribute file called > >>> "brightness". This file lives in the > >>> + * device directory of the LED controller, not an individual LED, > >>> so it should not confuse users. > >>> + */ > >> Sorry if this has been discussed already > >> > >> This seems a bit wonky. You are overriding the brightness set by > >> the LED class. > > I am not. Pressing the button does not change the brightness read > > from the /sys/class/leds/<LED>/brightness file. This is different > > brightness, it is above the classic brightnes in the PWM hierarchy > > in the microcontroller. I discussed this with Pavel and he said we > > can call this file brightness as well (since it is brightness of > > the whole panel), and the file does not reside in > > /sys/class/leds/<LED> directory. > > OK then there needs to be some ABI documentation no? You are right, I'll add this for v5. Thx
