Three different LED brightnesses (was Re: PM regression with LED changes in next-20161109)

[Pavel Machek]

On Sat 2016-11-12 09:03:42, Hans de Goede wrote:
> Hi,
> 
> On 11-11-16 23:12, Pavel Machek wrote:
> >Hi!
> >
> >Reason #1:
> >
> >>>>Hmm. So userland can read the LED state, and it can get _some_ value
> >>>>back, but it can not know if it is current state or not.
> 
> That is not correct, the current behavior for eading the brightness
> atrribute is to always return the current state.

No. (Because some hardware can't get back current state of
hardware-controlled leds, and because of blinking).

> >>Why a dedicated file? Are we going to mirror brightness here
> >>wrt r/w (show/store) behavior ? If not userspace now needs
> >>2 open fds which is not really nice. If we are and we are
> >>not going to use poll for something else on brightness itself
> >>then why not just poll directly on brightness ?
> >
> >Reason #1 is above.
> 
> See my reply above.
> 
> >Reason #2 is "if userspace sees brightness file, it can not know if
> >the notifications on change actually work or not".
> 
> If it needs to know that it can simply check the kernel version.

No. Because in case of hardware blinking we can't provide poll()
functionality.

Plus, saying "simply check the kernel version" simply means you should
not be submitting patches to kernel... at all. (Hint... it also does
not work.)

> >Reason #3 is that you broke Tony's system. Polling does not make sense
> >when trigger such as "CPU in use" is active.
> 
> Have you seen v4 of my patch? It fixes this while keeping the
> polling on the brightness attribute itself, it basically goes
> back (more or less) to v1 of my patch which did not have this
> problem. I never wanted notification of trigger / blinking
> changes because I already feared Tony's problem would happen.

Have you seen v67123 of my latest facebook post? It explains why you
are completely wrong.

> >Reason #4 is that there are really two brightnesses:
> >
> >1) maximum brightness trigger is going to use
> >
> >2) current brightness
> >
> >Currently writing to "brightness" file changes 1), but reading returns
> >2) when available.
> 
> Right and Jacek has already said that we cannot change the
> reading behavior on the brightness file because of ABI concerns.

Until there's user that actually reads that, ABI can be fixed. Given
that it basically returns random value,

> >So, feel free to propose better interface. One that solves #1..#4
> >above.
> 
> Proposal 1:
> 
> v4 of my patch, see the list. It solves all but #4, which
> is out of scope for my patch, feel free to submit a patch to
> solve #4 (with a new sysfs attr).

NAK on that. (And it does not solve #1 and #2 at least.)

> Proposal 2:
> 
> Add a new "user_brightness" file, which shows the last brightness
> as set by the user, this would show the read behavior we really
> want of brightness: show the real brightness when not blinking /
> triggers are active, show the brightness used when on when
> blinking / triggers are active.

No, that's just adding more mess on the system.

Here's better proposal:

brightness (write): what we do today. (Mess, but too late to change it)
	   (read): -Esomething or what we do today (if someone
	   	   	       	       	  	   acutally uses it)
           (poll): -Esomething

current_brightness (write): -Esomething, or maybe change brightness
		   for triggers that can work with that
              (read, poll): if the current trigger can get current
	             state of led, do it, otherwise -Esomething...
		     or maybe file should be simply hidden from sysfs.

trigger_max_brightness (read,write): change the maximum brightness for
		       a trigger.
	       (poll): -Esomething

If you have hardware changing the brightness behind kernel's back,
that should be modelled as a trigger. Userspace should know
there's hardware changing it autonomously ... there should be
"hardware-keylight-brightness" trigger, probably impossible to change
(depends on hardware behaviour).

On thinkpad, for example, for many LEDs kernel can select either
"hardware drives the LED", but then current_brightness is unavailable,
or "kernel drives the LED", but then hardware does not touch the led
at all.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature