Hi Alan,
Thanks for taking the time to review it,
On Sat, Dec 12, 2009 at 18:37, Alan Jenkins
<sourcejedi.lkml@xxxxxxxxxxxxxx> wrote:
> Hi Anisse
>
> I think there are a few theoretical issues remaining in this driver
> (although I've not read your followup patches, I just checked the
> subject lines).
>
> On 12/10/09, Anisse Astier <anisse@xxxxxxxxx> wrote:
>> +config MSI_WMI
>> + tristate "MSI WMI extras"
>> + depends on ACPI_WMI
>> + depends on INPUT
>
> I think this driver depends on BACKLIGHT_CLASS_DEVICE as well.
Indeed it depends on it..
>
>> + help
>> + Say Y here if you want to support WMI-based hotkeys on MSI laptops.
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called msi-wmi.
>> +
>
>
>> +static int bl_get(struct backlight_device *bd)
>> +{
>> + int level, err, ret = 0;
>> +
>> + /* Instance 1 is "get backlight", cmp with DSDT */
>> + err = msi_wmi_query_block(1, &ret);
>> + if (err)
>> + printk(KERN_ERR DRV_PFX "Could not query backlight: %d\n", err);
>
> It looks like we continue despite this error, and return 0? I think
> an error code would be more appropriate. It would definitely be
> better to use an explicit return statement here (and not set ret = 0
> beforehand).
>
> In reality the current backlight class doesn't support error codes...
> but I figure it's better to give userspace an obviously wrong number,
> rather than falsely claiming we know that the blacklight is set to 0.
>
Sure.
>
>
>> +static void msi_wmi_notify(u32 value, void *context)
>> +{
>> + struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>> + static struct key_entry *key;
>> + union acpi_object *obj;
>> + ktime_t cur;
>> +
>> + wmi_get_event_data(value, &response);
>> +
>> + obj = (union acpi_object *)response.pointer;
>> +
>> + if (obj && obj->type == ACPI_TYPE_INTEGER) {
>> + int eventcode = obj->integer.value;
>> + dprintk("Eventcode: 0x%x\n", eventcode);
>> + key = msi_wmi_get_entry_by_scancode(eventcode);
>> + if (key) {
>> + cur = ktime_get_real();
>> + /* Ignore event if the same event happened in a 50 ms
>> + timeframe -> Key press may result in 10-20 GPEs */
>> + if (ktime_to_us(ktime_sub(cur, key->last_pressed))
>> + < 1000 * 50) {
>> + dprintk("Suppressed key event 0x%X - "
>> + "Last press was %lld us ago\n",
>> + key->code,
>> + ktime_to_us(ktime_sub(cur,
>> + key->last_pressed)));
>> + return;
>> + }
>> + key->last_pressed = cur;
>> +
>> + switch (key->type) {
>> + case KE_KEY:
>> + /* Brightness is served via acpi video driver */
>> + if (!backlight &&
>> + (key->keycode == KEY_BRIGHTNESSUP ||
>> + key->keycode == KEY_BRIGHTNESSDOWN))
>> + break;
>
> Given backlight == NULL, this will mysteriously prevent users from
> remapping the volume keys to use as a brightness control. I think it
> would be better to check the original scancodes.
Agreed, this backlight == NULL should be replaced by the
acpi_video_backlight_support() test, which would be clearer, as it is
the only way that the driver could be fully initialized and backlight
still be NULL.
As for the remapping problem you are right, but please remember that
this code is theoretical and future-proofing, because on my
hardware(MSI Windtop AE1900-WT, all-in-one Atom-based computer),
backlight is always controlled through WMI. Can you confirm that on
yours Thomas?
>
> Also, can you please confirm that the ACPI BIOS does _not_ change the
> brightness level in response to these keys? If it does, the driver
> should really use backlight_force_update() instead of generating
> KEY_BRIGHTNESS* events.
I can confirm that the backlight is not controlled by the ACPI BIOS.
You must issue a WMI request to change it.
Regards,
Anisse
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
