Re: [PATCH 2/2 v4] asus-wmi: store backlight power status for AIO machine

[Corentin Chary <corentin.chary@xxxxxxxxx>]

On Mon, Mar 19, 2012 at 1:22 PM, AceLan Kao <acelan.kao@xxxxxxxxxxxxx> wrote:
> Dear Corentin,
>
> 2012/3/19 Corentin Chary <corentin.chary@xxxxxxxxx>:
>> On Mon, Mar 19, 2012 at 10:46 AM, AceLan Kao <acelan.kao@xxxxxxxxxxxxx> wrote:
>>> Due to some implementation reasons, ASUS ET2012 All-in-One machines
>>> can't report the correct backlight power status, it will always return
>>> 1. To track the backlight power status correctly, we have to store the
>>> status by ourselves.
>>>
>>> BTW, by the BIOS design, the backlight power will be turn on/off
>>> sequently, no matter what the value of the parameter will be.
>>> More over, the brightness adjustment command will turn on the backlight
>>> power. Those behaviors will make us fail to track the backlight power
>>> status.
>>> For example, While we are trying to turn on the backlight power,
>>> we will send out the brightness adjustment command and then trying to
>>> figure out if we have to turn on the backlight power, then send out
>>> the command. But, the real case is that, the backlight power turns on
>>> while sending the brightness adjustment command, and then we send out
>>> the command to turn on the backlight power, it actually will turn off
>>> the backlight power and the backlight power status we recorded becomes
>>> wrong. So, we have to seperate these two commands by a if statement.
>>>
>>> Signed-off-by: AceLan Kao <acelan.kao@xxxxxxxxxxxxx>
>>> ---
>>>  drivers/platform/x86/asus-wmi.c  |   33 ++++++++++++++++++++-------------
>>>  drivers/platform/x86/asus-wmi.h  |    2 ++
>>>  drivers/platform/x86/eeepc-wmi.c |   15 ++++++++++++---
>>>  3 files changed, 34 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>> index ea80fbe..9cd459a 100644
>>> --- a/drivers/platform/x86/asus-wmi.c
>>> +++ b/drivers/platform/x86/asus-wmi.c
>>> @@ -1076,7 +1076,12 @@ static int asus_wmi_hwmon_init(struct asus_wmi *asus)
>>>  */
>>>  static int read_backlight_power(struct asus_wmi *asus)
>>>  {
>>> -       int ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_BACKLIGHT);
>>> +       int ret;
>>> +       if (asus->driver->quirks->store_backlight_power)
>>> +               ret = !asus->driver->panel_power;
>>> +       else
>>> +               ret = asus_wmi_get_devstate_simple(asus,
>>> +                                                  ASUS_WMI_DEVID_BACKLIGHT);
>>>
>>>        if (ret < 0)
>>>                return ret;
>>> @@ -1138,24 +1143,23 @@ static int update_bl_status(struct backlight_device *bd)
>>>  {
>>>        struct asus_wmi *asus = bl_get_data(bd);
>>>        u32 ctrl_param;
>>> -       int power, err;
>>> -
>>> -       if (asus->driver->quirks->scalar_panel_brightness)
>>> -               ctrl_param = get_scalar_command(bd);
>>> -       else
>>> -               ctrl_param = bd->props.brightness;
>>> -
>>> -       err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BRIGHTNESS,
>>> -                                   ctrl_param, NULL);
>>> -
>>> -       if (err < 0)
>>> -               return err;
>>> +       int power, err = 0;
>>>
>>>        power = read_backlight_power(asus);
>>>        if (power != -ENODEV && bd->props.power != power) {
>>>                ctrl_param = !!(bd->props.power == FB_BLANK_UNBLANK);
>>>                err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT,
>>>                                            ctrl_param, NULL);
>>> +               if (asus->driver->quirks->store_backlight_power)
>>> +                       asus->driver->panel_power = bd->props.power;
>>> +       } else {
>>> +               if (asus->driver->quirks->scalar_panel_brightness)
>>> +                       ctrl_param = get_scalar_command(bd);
>>> +               else
>>> +                       ctrl_param = bd->props.brightness;
>>> +
>>> +               err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BRIGHTNESS,
>>> +                                           ctrl_param, NULL);
>>>        }
>>
>> Something I didn't notice, why is this moved in an "else" ?
>> Does this break something on your hardware ?
> Yes, in the original sequence, it will send out a brightness command through
> WMI anyway, even if we hit this function because of changing the
> backlight power.
>
> And in the ET2012 series machines, it will turn on the backlight power
> automatically
> when you send out the brightness command.
> And the backlight power command actually is a backlight toggle command
> in ET2012.
> That is no matter what the parameter is while sending the backlight
> power command,
> it will turn on and off the backlight power in a row.
> So, when we're trying to turn on the backlight power, the panel will
> be turned on
> by the brightness command and the turn on backlight command will actually
> turn off the backlight.
>
> To avoid this, I use an if else statement to distinguish these 2 cases,
> brightness or backlight power, and this should do no harm to normal machines,
> hope I don't miss anything.

Ok I see. I pushed some patches on top of these two, they are
available at http://git.iksaif.net/?p=acpi4asus-dkms.git;a=summary
These are mostly cleanups, fix for asus-nb-wmi, and I restored the
ability to update both brightness and power (still disabled for
et2012).
Could you test these patches ?
Thanks,

-- 
Corentin Chary
http://xf.iksaif.net
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html