Brightness change on notebook that follow ACPI specification (par. B.7)

[John Baldwin]

On Tuesday 30 March 2010 3:30:37 pm Dan Lukes wrote:
> On 03/30/10 16:11, John Baldwin:
> >> Most notebooks have special keys (mostly Fn+something) to change
> >> brightness of LCD display.
> >>
> >> Some of them (my notebok, for example) follows the ACPI specification
> >> (paragraph B.7) how to announce the user request for brightness change
> >> to OS.
> >>
> >> I implemented such handling as part of acpi-video module.
> >>
> >> It's verified to work on my HP Mini 5101&  FreeBSD 8.0.
> >>
> >> No test on other notebook done as I have no other notebook.
> 
> > A patch to implement the brightness controls was already committed to HEAD a
> > while ago and have already been merged to 8-stable and 7-stable.  Can you test
> > the a kernel from 8-stable to ensure it works ok on your machine?  The patches
> > in HEAD work fine on an HP Mini 2140 that I have here.
> 
> I'm sure there has been a regular PR before commitment assigned to ACPI 
> group for review, but I missed them. It resulted in waste of time. My fault.

I'm not sure if there was a PR.  I do recall a thread on the acpi@ mailing
list when the patch was first submitted.

> I will test it later (I have no 8-STABLE here), I'm almost sure it will 
> work (based on code review).
> 
> There are several notices related to coding of the committed patch:
> 
>   --- 1 ---------------
> /* events */
> #define	VID_NOTIFY_SWITCHED	0x80
> #define	VID_NOTIFY_REPROBE	0x81
> #define	VID_NOTIFY_CYCLE_BRN	0x85
> #define	VID_NOTIFY_INC_BRN	0x86
> #define	VID_NOTIFY_DEC_BRN	0x87
> #define	VID_NOTIFY_ZERO_BRN	0x88
> 
> The events 0x80 and 0x81 are "Display Devices" device specific 
> notifications (according ACPI specification B.5), but events 0x85-0x88 
> are "Output devices" device specific notification (ACPI spec. B.7). The 
> common name VID_NOTIFY_* for values that come from different domains is 
> confusing. Note that future versions of ACPI specification may overlaps 
> (there may be defined 0x80 event for Output device or event 0x85 event 
> for Display device).
> 
>   --- 2 ---------------
> The code of acpi_video_vo_notify_handler():
> 
> Handling of event 0x85 refuse do anything when there are less than four 
> levels. I see no reason for such limitation - we can safely cycle 
> trougth three or even trougth two levels as well.
> 
> It advance, the 0x85 handling assume the _BCL list is sorted and ignores 
> vo_levels[0] and vo_levels[1] (note that handling of 0x86/0x87 just few 
> lines bellow doesn't assume sorted levels nor ignore levels [0] and [1]).
> 
> The 0x88 handling is duplicate implementation of 
> acpi_video_vo_check_level instead just calling the already implemented 
> function.
>   ------------------------
> 
> Such notices should not be considered offence against the autor nor 
> committer in any way. Just my $0.02 related to the code.

These all sound like good fixes to me.  If you can code up a patch that
implements them I would be happy to test it.  (My netbook only has keys
for up and down, so I can't test changes for 0x85 and 0x88.)

-- 
John Baldwin