| Message ID | 1485415981-20487-1-git-send-email-alex.hung@canonical.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
> New firmwares include a feature called 5 button array that supports > super key, volume up/down, rotation lock and power button. Especially, Personally, I would drop the word "especially". It seems redundant. > support for this feature is required to fix power button on some recent > systems. > > This patch was tested on a Dell Latitude 7280. > > Signed-off-by: Alex Hung <alex.hung@canonical.com> > --- > drivers/platform/x86/intel-hid.c | 107 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 102 insertions(+), 5 deletions(-) > > diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c > index cb3ab2b..6e796a5 100644 > --- a/drivers/platform/x86/intel-hid.c > +++ b/drivers/platform/x86/intel-hid.c > @@ -1,5 +1,5 @@ > /* > - * Intel HID event driver for Windows 8 > + * Intel HID event & 5 button array driver > * > * Copyright (C) 2015 Alex Hung <alex.hung@canonical.com> > * Copyright (C) 2015 Andrew Lutomirski <luto@kernel.org> > @@ -57,8 +57,24 @@ static const struct key_entry intel_hid_keymap[] = { > { KE_END }, > }; > > +/* 5 button array notification value. */ > +static const struct key_entry intel_array_keymap[] = { > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ Comments in the last two lines are now misaligned with the rest. Other than these two nitpicks (which IMHO are not worth a v3): Reviewed-by: Michał Kępień <kernel@kempniu.pl> > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ > + { KE_END }, > +}; > + > struct intel_hid_priv { > struct input_dev *input_dev; > + struct input_dev *array; > }; > > static int intel_hid_set_enable(struct device *device, int enable) > @@ -78,15 +94,43 @@ static int intel_hid_set_enable(struct device *device, int enable) > return 0; > } > > +static void intel_button_array_enable(struct device *device, int enable) > +{ > + struct intel_hid_priv *priv = dev_get_drvdata(device); > + acpi_handle handle = ACPI_HANDLE(device); > + unsigned long long button_cap; > + acpi_status status; > + > + if (!priv->array) > + return; > + > + /* Query supported platform features */ > + status = acpi_evaluate_integer(handle, "BTNC", NULL, &button_cap); > + if (ACPI_FAILURE(status)) { > + dev_warn(device, "failed to get button capability\n"); > + return; > + } > + > + /* Enable|disable features - power button is always enabled */ > + status = acpi_execute_simple_method(handle, "BTNE", > + enable ? button_cap : 1); > + if (ACPI_FAILURE(status)) > + dev_warn(device, "failed to set button capability\n"); > +} > + > static int intel_hid_pl_suspend_handler(struct device *device) > { > intel_hid_set_enable(device, 0); > + intel_button_array_enable(device, 0); > + > return 0; > } > > static int intel_hid_pl_resume_handler(struct device *device) > { > intel_hid_set_enable(device, 1); > + intel_button_array_enable(device, 1); > + > return 0; > } > > @@ -126,11 +170,43 @@ static int intel_hid_input_setup(struct platform_device *device) > return ret; > } > > +static int intel_button_array_input_setup(struct platform_device *device) > +{ > + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); > + int ret; > + > + /* Setup input device for 5 button array */ > + priv->array = input_allocate_device(); > + if (!priv->array) > + return -ENOMEM; > + > + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL); > + if (ret) > + goto err_free_array_device; > + > + priv->array->dev.parent = &device->dev; > + priv->array->name = "Intel HID 5 button array"; > + priv->array->id.bustype = BUS_HOST; > + > + ret = input_register_device(priv->array); > + if (ret) > + goto err_free_array_device; > + > + return 0; > + > +err_free_array_device: > + input_free_device(priv->array); > + return ret; > +} > + > static void intel_hid_input_destroy(struct platform_device *device) > { > struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); > > input_unregister_device(priv->input_dev); > + > + if (priv->array) > + input_unregister_device(priv->array); > } > > static void notify_handler(acpi_handle handle, u32 event, void *context) > @@ -140,10 +216,11 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) > unsigned long long ev_index; > acpi_status status; > > - /* The platform spec only defines one event code: 0xC0. */ > + /* 0xC0 is for HID events, other values are for 5 button array */ > if (event != 0xc0) { > - dev_warn(&device->dev, "received unknown event (0x%x)\n", > - event); > + if (!priv->array || > + !sparse_keymap_report_event(priv->array, event, 1, true)) > + dev_info(&device->dev, "unknown event 0x%x\n", event); > return; > } > > @@ -161,8 +238,8 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) > static int intel_hid_probe(struct platform_device *device) > { > acpi_handle handle = ACPI_HANDLE(&device->dev); > + unsigned long long event_cap, mode; > struct intel_hid_priv *priv; > - unsigned long long mode; > acpi_status status; > int err; > > @@ -193,6 +270,15 @@ static int intel_hid_probe(struct platform_device *device) > return err; > } > > + /* Setup 5 button array */ > + status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap); > + if (ACPI_SUCCESS(status) && (event_cap & 0x20000)) { > + dev_info(&device->dev, "platform supports 5 button array\n"); > + err = intel_button_array_input_setup(device); > + if (err) > + pr_err("Failed to setup Intel 5 button array hotkeys\n"); > + } > + > status = acpi_install_notify_handler(handle, > ACPI_DEVICE_NOTIFY, > notify_handler, > @@ -206,6 +292,16 @@ static int intel_hid_probe(struct platform_device *device) > if (err) > goto err_remove_notify; > > + if (priv->array) { > + intel_button_array_enable(&device->dev, 1); > + > + /* Call button load method to enable HID power button */ > + status = acpi_evaluate_object(handle, "BTNL", NULL, NULL); > + if (ACPI_FAILURE(status)) > + dev_warn(&device->dev, > + "failed to enable HID power button\n"); > + } > + > return 0; > > err_remove_notify: > @@ -224,6 +320,7 @@ static int intel_hid_remove(struct platform_device *device) > acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, notify_handler); > intel_hid_input_destroy(device); > intel_hid_set_enable(&device->dev, 0); > + intel_button_array_enable(&device->dev, 0); > > /* > * Even if we failed to shut off the event stream, we can still > -- > 2.7.4 >
On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote: > New firmwares include a feature called 5 button array that supports > super key, volume up/down, rotation lock and power button. Especially, > support for this feature is required to fix power button on some recent > systems. > > This patch was tested on a Dell Latitude 7280. Hi Alex, Minor nit below (no need to resend, but a pair of follow-up cleanups would be nice). Queued to testing. > > Signed-off-by: Alex Hung <alex.hung@canonical.com> Pali, would you care to offer a review or some testing to verify no unexpected conflicts with the other dell drivers? > --- > drivers/platform/x86/intel-hid.c | 107 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 102 insertions(+), 5 deletions(-) > > diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c > index cb3ab2b..6e796a5 100644 > --- a/drivers/platform/x86/intel-hid.c > +++ b/drivers/platform/x86/intel-hid.c > @@ -1,5 +1,5 @@ > /* > - * Intel HID event driver for Windows 8 > + * Intel HID event & 5 button array driver > * > * Copyright (C) 2015 Alex Hung <alex.hung@canonical.com> > * Copyright (C) 2015 Andrew Lutomirski <luto@kernel.org> > @@ -57,8 +57,24 @@ static const struct key_entry intel_hid_keymap[] = { > { KE_END }, > }; > > +/* 5 button array notification value. */ > +static const struct key_entry intel_array_keymap[] = { > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ > + { KE_END }, > +}; > + > struct intel_hid_priv { > struct input_dev *input_dev; > + struct input_dev *array; > }; > > static int intel_hid_set_enable(struct device *device, int enable) > @@ -78,15 +94,43 @@ static int intel_hid_set_enable(struct device *device, int enable) > return 0; > } > > +static void intel_button_array_enable(struct device *device, int enable) > +{ As enable is always explicitly passed and is used solely as a boolean value, it would preferable for both this and the previous usage above to define it as a bool. Being self-consistent is important however, so please consider this for a cleanup as a separate patch. > + struct intel_hid_priv *priv = dev_get_drvdata(device); > + acpi_handle handle = ACPI_HANDLE(device); > + unsigned long long button_cap; > + acpi_status status; > + > + if (!priv->array) > + return; > + > + /* Query supported platform features */ > + status = acpi_evaluate_integer(handle, "BTNC", NULL, &button_cap); > + if (ACPI_FAILURE(status)) { > + dev_warn(device, "failed to get button capability\n"); > + return; > + } > + > + /* Enable|disable features - power button is always enabled */ > + status = acpi_execute_simple_method(handle, "BTNE", > + enable ? button_cap : 1); > + if (ACPI_FAILURE(status)) > + dev_warn(device, "failed to set button capability\n"); > +} > + > static int intel_hid_pl_suspend_handler(struct device *device) > { > intel_hid_set_enable(device, 0); > + intel_button_array_enable(device, 0); > + > return 0; > } > > static int intel_hid_pl_resume_handler(struct device *device) > { > intel_hid_set_enable(device, 1); > + intel_button_array_enable(device, 1); > + > return 0; > } > > @@ -126,11 +170,43 @@ static int intel_hid_input_setup(struct platform_device *device) > return ret; > } > > +static int intel_button_array_input_setup(struct platform_device *device) > +{ > + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); > + int ret; > + > + /* Setup input device for 5 button array */ > + priv->array = input_allocate_device(); > + if (!priv->array) > + return -ENOMEM; > + > + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL); > + if (ret) > + goto err_free_array_device; > + > + priv->array->dev.parent = &device->dev; > + priv->array->name = "Intel HID 5 button array"; > + priv->array->id.bustype = BUS_HOST; > + > + ret = input_register_device(priv->array); > + if (ret) > + goto err_free_array_device; > + > + return 0; > + > +err_free_array_device: > + input_free_device(priv->array); > + return ret; This return path is more complex than it could be, since you test for ret before return anyway: out: if (ret) input_free_device(priv->array); return ret; There is no need for a second return point that I can see. Same for the hid_input_setup function. We can remove 8 lines. This follows the previous nit - it's self consistent, but a follow-on cleanup patch would be worthwhile. Thanks Alex, I've queued this to testing and it will go to for-next unless the CI, Pali, or a user reports a problem. Appreciate all your effort on this one.
Apologies, this time with Pali's correct email address (aliases fail). > > On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote: > > New firmwares include a feature called 5 button array that supports > > super key, volume up/down, rotation lock and power button. Especially, > > support for this feature is required to fix power button on some recent > > systems. > > > > This patch was tested on a Dell Latitude 7280. > > > Hi Alex, > > Minor nit below (no need to resend, but a pair of follow-up cleanups would be > nice). > > Queued to testing. > > > > > Signed-off-by: Alex Hung <alex.hung@canonical.com> > > Pali, would you care to offer a review or some testing to verify no unexpected > conflicts with the other dell drivers? > > > --- > > drivers/platform/x86/intel-hid.c | 107 +++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 102 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c > > index cb3ab2b..6e796a5 100644 > > --- a/drivers/platform/x86/intel-hid.c > > +++ b/drivers/platform/x86/intel-hid.c > > @@ -1,5 +1,5 @@ > > /* > > - * Intel HID event driver for Windows 8 > > + * Intel HID event & 5 button array driver > > * > > * Copyright (C) 2015 Alex Hung <alex.hung@canonical.com> > > * Copyright (C) 2015 Andrew Lutomirski <luto@kernel.org> > > @@ -57,8 +57,24 @@ static const struct key_entry intel_hid_keymap[] = { > > { KE_END }, > > }; > > > > +/* 5 button array notification value. */ > > +static const struct key_entry intel_array_keymap[] = { > > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ > > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ > > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ > > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ > > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ > > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ > > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ > > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ > > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ > > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ > > + { KE_END }, > > +}; > > + > > struct intel_hid_priv { > > struct input_dev *input_dev; > > + struct input_dev *array; > > }; > > > > static int intel_hid_set_enable(struct device *device, int enable) > > @@ -78,15 +94,43 @@ static int intel_hid_set_enable(struct device *device, int enable) > > return 0; > > } > > > > +static void intel_button_array_enable(struct device *device, int enable) > > +{ > > As enable is always explicitly passed and is used solely as a boolean value, it > would preferable for both this and the previous usage above to define it as a > bool. Being self-consistent is important however, so please consider this for a > cleanup as a separate patch. > > > + struct intel_hid_priv *priv = dev_get_drvdata(device); > > + acpi_handle handle = ACPI_HANDLE(device); > > + unsigned long long button_cap; > > + acpi_status status; > > + > > + if (!priv->array) > > + return; > > + > > + /* Query supported platform features */ > > + status = acpi_evaluate_integer(handle, "BTNC", NULL, &button_cap); > > + if (ACPI_FAILURE(status)) { > > + dev_warn(device, "failed to get button capability\n"); > > + return; > > + } > > + > > + /* Enable|disable features - power button is always enabled */ > > + status = acpi_execute_simple_method(handle, "BTNE", > > + enable ? button_cap : 1); > > + if (ACPI_FAILURE(status)) > > + dev_warn(device, "failed to set button capability\n"); > > +} > > + > > static int intel_hid_pl_suspend_handler(struct device *device) > > { > > intel_hid_set_enable(device, 0); > > + intel_button_array_enable(device, 0); > > + > > return 0; > > } > > > > static int intel_hid_pl_resume_handler(struct device *device) > > { > > intel_hid_set_enable(device, 1); > > + intel_button_array_enable(device, 1); > > + > > return 0; > > } > > > > @@ -126,11 +170,43 @@ static int intel_hid_input_setup(struct platform_device *device) > > return ret; > > } > > > > +static int intel_button_array_input_setup(struct platform_device *device) > > +{ > > + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); > > + int ret; > > + > > + /* Setup input device for 5 button array */ > > + priv->array = input_allocate_device(); > > + if (!priv->array) > > + return -ENOMEM; > > + > > + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL); > > + if (ret) > > + goto err_free_array_device; > > + > > + priv->array->dev.parent = &device->dev; > > + priv->array->name = "Intel HID 5 button array"; > > + priv->array->id.bustype = BUS_HOST; > > + > > + ret = input_register_device(priv->array); > > + if (ret) > > + goto err_free_array_device; > > + > > + return 0; > > + > > +err_free_array_device: > > + input_free_device(priv->array); > > + return ret; > > This return path is more complex than it could be, since you test for ret before > return anyway: > > out: > if (ret) > input_free_device(priv->array); > return ret; > > There is no need for a second return point that I can see. Same for the > hid_input_setup function. We can remove 8 lines. > > This follows the previous nit - it's self consistent, but a follow-on cleanup > patch would be worthwhile. > > Thanks Alex, I've queued this to testing and it will go to for-next unless the > CI, Pali, or a user reports a problem. Appreciate all your effort on this one. > > -- > Darren Hart > Intel Open Source Technology Center
On Sat, Feb 4, 2017 at 3:14 AM, Darren Hart <dvhart@infradead.org> wrote: > On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote: >> New firmwares include a feature called 5 button array that supports >> super key, volume up/down, rotation lock and power button. Especially, >> support for this feature is required to fix power button on some recent >> systems. >> +static int intel_button_array_input_setup(struct platform_device *device) >> +{ >> + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); >> + int ret; >> + >> + /* Setup input device for 5 button array */ >> + priv->array = input_allocate_device(); >> + if (!priv->array) >> + return -ENOMEM; >> + >> + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL); >> + if (ret) >> + goto err_free_array_device; >> + >> + priv->array->dev.parent = &device->dev; >> + priv->array->name = "Intel HID 5 button array"; >> + priv->array->id.bustype = BUS_HOST; >> + >> + ret = input_register_device(priv->array); >> + if (ret) >> + goto err_free_array_device; >> + >> + return 0; >> + >> +err_free_array_device: >> + input_free_device(priv->array); >> + return ret; > > This return path is more complex than it could be, since you test for ret before > return anyway: > > out: > if (ret) > input_free_device(priv->array); > return ret; > > There is no need for a second return point that I can see. Same for the > hid_input_setup function. We can remove 8 lines. Darren, if I didn't miss anything the function is purely used in ->probe() path and thus devm_ version of input_allocate_device() would make this error path gone.
Hi! On Saturday 04 February 2017 02:26:05 Darren Hart wrote: > Apologies, this time with Pali's correct email address (aliases fail). > ... > > > > Pali, would you care to offer a review or some testing to verify no unexpected > > conflicts with the other dell drivers? I do not have Dell machine which uses intel-hid.ko so I cannot test this patch. And obviously as it is not loaded it cannot break machines which do not use intel-hid.ko. > > > +/* 5 button array notification value. */ > > > +static const struct key_entry intel_array_keymap[] = { > > > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ > > > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ > > > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ > > > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ > > > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ > > > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ > > > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ > > > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ > > > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ > > > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ > > > + { KE_END }, > > > +}; This looks suspicious. Why are all release events ignored?
> Hi! > > On Saturday 04 February 2017 02:26:05 Darren Hart wrote: > > Apologies, this time with Pali's correct email address (aliases fail). > > > ... > > > > > > Pali, would you care to offer a review or some testing to verify no unexpected > > > conflicts with the other dell drivers? > > I do not have Dell machine which uses intel-hid.ko so I cannot test this > patch. And obviously as it is not loaded it cannot break machines which > do not use intel-hid.ko. > > > > > +/* 5 button array notification value. */ > > > > +static const struct key_entry intel_array_keymap[] = { > > > > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ > > > > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ > > > > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ > > > > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ > > > > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ > > > > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ > > > > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ > > > > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ > > > > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ > > > > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ > > > > + { KE_END }, > > > > +}; > > This looks suspicious. Why are all release events ignored? I also do not have a Dell machine that makes use of this driver, but my understanding is that calling sparse_keymap_report_event() with autorelease set to true makes release events irrelevant and simplifies implementation. Though each release event still needs a KE_IGNORE entry in the keymap so that superfluous KEY_UNKNOWN events are suppressed.
On Wednesday 08 February 2017 08:21:46 Michał Kępień wrote: > > Hi! > > > > On Saturday 04 February 2017 02:26:05 Darren Hart wrote: > > > Apologies, this time with Pali's correct email address (aliases fail). > > > > > ... > > > > > > > > Pali, would you care to offer a review or some testing to verify no unexpected > > > > conflicts with the other dell drivers? > > > > I do not have Dell machine which uses intel-hid.ko so I cannot test this > > patch. And obviously as it is not loaded it cannot break machines which > > do not use intel-hid.ko. > > > > > > > +/* 5 button array notification value. */ > > > > > +static const struct key_entry intel_array_keymap[] = { > > > > > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ > > > > > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ > > > > > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ > > > > > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ > > > > > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ > > > > > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ > > > > > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ > > > > > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ > > > > > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ > > > > > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ > > > > > + { KE_END }, > > > > > +}; > > > > This looks suspicious. Why are all release events ignored? > > I also do not have a Dell machine that makes use of this driver, but my > understanding is that calling sparse_keymap_report_event() with > autorelease set to true makes release events irrelevant and simplifies > implementation. Though each release event still needs a KE_IGNORE entry > in the keymap so that superfluous KEY_UNKNOWN events are suppressed. Right, but that means if ACPI/WMI/firmware provides correct information when key was pressed and when released we should use it. It allow userspace e.g. measure how long was key pressed or similar thing...
On Sat, Feb 4, 2017 at 9:14 AM, Darren Hart <dvhart@infradead.org> wrote: > On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote: >> New firmwares include a feature called 5 button array that supports >> super key, volume up/down, rotation lock and power button. Especially, >> support for this feature is required to fix power button on some recent >> systems. >> >> This patch was tested on a Dell Latitude 7280. > > Hi Alex, > > Minor nit below (no need to resend, but a pair of follow-up cleanups would be > nice). > > Queued to testing. > >> >> Signed-off-by: Alex Hung <alex.hung@canonical.com> > > Pali, would you care to offer a review or some testing to verify no unexpected > conflicts with the other dell drivers? > >> --- >> drivers/platform/x86/intel-hid.c | 107 +++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 102 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c >> index cb3ab2b..6e796a5 100644 >> --- a/drivers/platform/x86/intel-hid.c >> +++ b/drivers/platform/x86/intel-hid.c >> @@ -1,5 +1,5 @@ >> /* >> - * Intel HID event driver for Windows 8 >> + * Intel HID event & 5 button array driver >> * >> * Copyright (C) 2015 Alex Hung <alex.hung@canonical.com> >> * Copyright (C) 2015 Andrew Lutomirski <luto@kernel.org> >> @@ -57,8 +57,24 @@ static const struct key_entry intel_hid_keymap[] = { >> { KE_END }, >> }; >> >> +/* 5 button array notification value. */ >> +static const struct key_entry intel_array_keymap[] = { >> + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ >> + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ >> + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ >> + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ >> + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ >> + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ >> + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ >> + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ >> + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ >> + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ >> + { KE_END }, >> +}; >> + >> struct intel_hid_priv { >> struct input_dev *input_dev; >> + struct input_dev *array; >> }; >> >> static int intel_hid_set_enable(struct device *device, int enable) >> @@ -78,15 +94,43 @@ static int intel_hid_set_enable(struct device *device, int enable) >> return 0; >> } >> >> +static void intel_button_array_enable(struct device *device, int enable) >> +{ > > As enable is always explicitly passed and is used solely as a boolean value, it > would preferable for both this and the previous usage above to define it as a > bool. Being self-consistent is important however, so please consider this for a > cleanup as a separate patch. > >> + struct intel_hid_priv *priv = dev_get_drvdata(device); >> + acpi_handle handle = ACPI_HANDLE(device); >> + unsigned long long button_cap; >> + acpi_status status; >> + >> + if (!priv->array) >> + return; >> + >> + /* Query supported platform features */ >> + status = acpi_evaluate_integer(handle, "BTNC", NULL, &button_cap); >> + if (ACPI_FAILURE(status)) { >> + dev_warn(device, "failed to get button capability\n"); >> + return; >> + } >> + >> + /* Enable|disable features - power button is always enabled */ >> + status = acpi_execute_simple_method(handle, "BTNE", >> + enable ? button_cap : 1); >> + if (ACPI_FAILURE(status)) >> + dev_warn(device, "failed to set button capability\n"); >> +} >> + >> static int intel_hid_pl_suspend_handler(struct device *device) >> { >> intel_hid_set_enable(device, 0); >> + intel_button_array_enable(device, 0); >> + >> return 0; >> } >> >> static int intel_hid_pl_resume_handler(struct device *device) >> { >> intel_hid_set_enable(device, 1); >> + intel_button_array_enable(device, 1); >> + >> return 0; >> } >> >> @@ -126,11 +170,43 @@ static int intel_hid_input_setup(struct platform_device *device) >> return ret; >> } >> >> +static int intel_button_array_input_setup(struct platform_device *device) >> +{ >> + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); >> + int ret; >> + >> + /* Setup input device for 5 button array */ >> + priv->array = input_allocate_device(); >> + if (!priv->array) >> + return -ENOMEM; >> + >> + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL); >> + if (ret) >> + goto err_free_array_device; >> + >> + priv->array->dev.parent = &device->dev; >> + priv->array->name = "Intel HID 5 button array"; >> + priv->array->id.bustype = BUS_HOST; >> + >> + ret = input_register_device(priv->array); >> + if (ret) >> + goto err_free_array_device; >> + >> + return 0; >> + >> +err_free_array_device: >> + input_free_device(priv->array); >> + return ret; > > This return path is more complex than it could be, since you test for ret before > return anyway: > > out: > if (ret) > input_free_device(priv->array); > return ret; > > There is no need for a second return point that I can see. Same for the > hid_input_setup function. We can remove 8 lines. > > This follows the previous nit - it's self consistent, but a follow-on cleanup > patch would be worthwhile. > > Thanks Alex, I've queued this to testing and it will go to for-next unless the > CI, Pali, or a user reports a problem. Appreciate all your effort on this one. Thanks for the review. I will send another patch to address all comments later. > > -- > Darren Hart > Intel Open Source Technology Center
On Wed, Feb 8, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Wednesday 08 February 2017 08:21:46 Michał Kępień wrote: >> > Hi! >> > >> > On Saturday 04 February 2017 02:26:05 Darren Hart wrote: >> > > Apologies, this time with Pali's correct email address (aliases fail). >> > > >> > ... >> > > > >> > > > Pali, would you care to offer a review or some testing to verify no unexpected >> > > > conflicts with the other dell drivers? >> > >> > I do not have Dell machine which uses intel-hid.ko so I cannot test this >> > patch. And obviously as it is not loaded it cannot break machines which >> > do not use intel-hid.ko. >> > >> > > > > +/* 5 button array notification value. */ >> > > > > +static const struct key_entry intel_array_keymap[] = { >> > > > > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ >> > > > > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ >> > > > > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ >> > > > > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ >> > > > > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ >> > > > > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ >> > > > > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ >> > > > > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ >> > > > > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ >> > > > > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ >> > > > > + { KE_END }, >> > > > > +}; >> > >> > This looks suspicious. Why are all release events ignored? >> >> I also do not have a Dell machine that makes use of this driver, but my >> understanding is that calling sparse_keymap_report_event() with >> autorelease set to true makes release events irrelevant and simplifies >> implementation. Though each release event still needs a KE_IGNORE entry >> in the keymap so that superfluous KEY_UNKNOWN events are suppressed. Thanks Michał, this was indeed my intention. > > Right, but that means if ACPI/WMI/firmware provides correct information > when key was pressed and when released we should use it. It allow > userspace e.g. measure how long was key pressed or similar thing... Pali, The systems I tested generate a press event followed by a release event immediately, so the results will be the same for them. However, future ACPI implementation may not be the same and I will fix this in a following patch. Thanks for pointing this out. > > -- > Pali Rohár > pali.rohar@gmail.com
On Wednesday 08 February 2017 17:05:13 Alex Hung wrote: > On Wed, Feb 8, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Wednesday 08 February 2017 08:21:46 Michał Kępień wrote: > >> > Hi! > >> > > >> > On Saturday 04 February 2017 02:26:05 Darren Hart wrote: > >> > > Apologies, this time with Pali's correct email address (aliases fail). > >> > > > >> > ... > >> > > > > >> > > > Pali, would you care to offer a review or some testing to verify no unexpected > >> > > > conflicts with the other dell drivers? > >> > > >> > I do not have Dell machine which uses intel-hid.ko so I cannot test this > >> > patch. And obviously as it is not loaded it cannot break machines which > >> > do not use intel-hid.ko. > >> > > >> > > > > +/* 5 button array notification value. */ > >> > > > > +static const struct key_entry intel_array_keymap[] = { > >> > > > > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ > >> > > > > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ > >> > > > > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ > >> > > > > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ > >> > > > > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ > >> > > > > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ > >> > > > > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ > >> > > > > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ > >> > > > > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ > >> > > > > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ > >> > > > > + { KE_END }, > >> > > > > +}; > >> > > >> > This looks suspicious. Why are all release events ignored? > >> > >> I also do not have a Dell machine that makes use of this driver, but my > >> understanding is that calling sparse_keymap_report_event() with > >> autorelease set to true makes release events irrelevant and simplifies > >> implementation. Though each release event still needs a KE_IGNORE entry > >> in the keymap so that superfluous KEY_UNKNOWN events are suppressed. > > Thanks Michał, this was indeed my intention. > > > > > Right, but that means if ACPI/WMI/firmware provides correct information > > when key was pressed and when released we should use it. It allow > > userspace e.g. measure how long was key pressed or similar thing... > > Pali, > > The systems I tested generate a press event followed by a release > event immediately, so the results will be the same for them. Ok, in this case release events can be "simulated" by kernel as your current code is doing it. That is fine. > However, future ACPI implementation may not be the same and I will fix > this in a following patch. Thanks for pointing this out. If you think that this will be really changes in ACPI/firmware for new machines (or upcoming BIOS/firmware update), then it make sense to use release events from firmware.
On Sat, Feb 04, 2017 at 04:58:33PM +0200, Andy Shevchenko wrote: > On Sat, Feb 4, 2017 at 3:14 AM, Darren Hart <dvhart@infradead.org> wrote: > > On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote: > >> New firmwares include a feature called 5 button array that supports > >> super key, volume up/down, rotation lock and power button. Especially, > >> support for this feature is required to fix power button on some recent > >> systems. > > >> +static int intel_button_array_input_setup(struct platform_device *device) > >> +{ > >> + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); > >> + int ret; > >> + > >> + /* Setup input device for 5 button array */ > >> + priv->array = input_allocate_device(); > >> + if (!priv->array) > >> + return -ENOMEM; > >> + > >> + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL); > >> + if (ret) > >> + goto err_free_array_device; > >> + > >> + priv->array->dev.parent = &device->dev; > >> + priv->array->name = "Intel HID 5 button array"; > >> + priv->array->id.bustype = BUS_HOST; > >> + > >> + ret = input_register_device(priv->array); > >> + if (ret) > >> + goto err_free_array_device; > >> + > >> + return 0; > >> + > >> +err_free_array_device: > >> + input_free_device(priv->array); > >> + return ret; > > > > This return path is more complex than it could be, since you test for ret before > > return anyway: > > > > out: > > if (ret) > > input_free_device(priv->array); > > return ret; > > > > There is no need for a second return point that I can see. Same for the > > hid_input_setup function. We can remove 8 lines. > > Darren, if I didn't miss anything the function is purely used in > ->probe() path and thus devm_ version of input_allocate_device() would > make this error path gone. Hi Andy, These are optional input devices for the driver, so if I understand the semantics of devm_ correctly, the input device would remain allocated until such time as the driver is unloaded or if it fails to bind - which for systems with intel-hid, but no 5 button array, the unused input device would remain allocated until system shutdown. Alex, is that correct?
On Thu, Feb 9, 2017 at 10:22 AM, Darren Hart <dvhart@infradead.org> wrote: > On Sat, Feb 04, 2017 at 04:58:33PM +0200, Andy Shevchenko wrote: >> On Sat, Feb 4, 2017 at 3:14 AM, Darren Hart <dvhart@infradead.org> wrote: >> > On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote: >> >> New firmwares include a feature called 5 button array that supports >> >> super key, volume up/down, rotation lock and power button. Especially, >> >> support for this feature is required to fix power button on some recent >> >> systems. >> >> >> +static int intel_button_array_input_setup(struct platform_device *device) >> >> +{ >> >> + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); >> >> + int ret; >> >> + >> >> + /* Setup input device for 5 button array */ >> >> + priv->array = input_allocate_device(); >> >> + if (!priv->array) >> >> + return -ENOMEM; >> >> + >> >> + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL); >> >> + if (ret) >> >> + goto err_free_array_device; >> >> + >> >> + priv->array->dev.parent = &device->dev; >> >> + priv->array->name = "Intel HID 5 button array"; >> >> + priv->array->id.bustype = BUS_HOST; >> >> + >> >> + ret = input_register_device(priv->array); >> >> + if (ret) >> >> + goto err_free_array_device; >> >> + >> >> + return 0; >> >> + >> >> +err_free_array_device: >> >> + input_free_device(priv->array); >> >> + return ret; >> > >> > This return path is more complex than it could be, since you test for ret before >> > return anyway: >> > >> > out: >> > if (ret) >> > input_free_device(priv->array); >> > return ret; >> > >> > There is no need for a second return point that I can see. Same for the >> > hid_input_setup function. We can remove 8 lines. >> >> Darren, if I didn't miss anything the function is purely used in >> ->probe() path and thus devm_ version of input_allocate_device() would >> make this error path gone. > > Hi Andy, > > These are optional input devices for the driver, so if I understand the > semantics of devm_ correctly, the input device would remain allocated until such > time as the driver is unloaded or if it fails to bind - which for systems with > intel-hid, but no 5 button array, the unused input device would remain allocated > until system shutdown. > > Alex, is that correct? Yes, the input device will be only allocated if 5 button array is supported. Previous firmware that does not support 5 button array won't be affected. > > -- > Darren Hart > Intel Open Source Technology Center
On Thu, Feb 9, 2017 at 5:09 AM, Alex Hung <alex.hung@canonical.com> wrote: > On Thu, Feb 9, 2017 at 10:22 AM, Darren Hart <dvhart@infradead.org> wrote: >> On Sat, Feb 04, 2017 at 04:58:33PM +0200, Andy Shevchenko wrote: >>> On Sat, Feb 4, 2017 at 3:14 AM, Darren Hart <dvhart@infradead.org> wrote: >>> > On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote: >>> >> New firmwares include a feature called 5 button array that supports >>> >> super key, volume up/down, rotation lock and power button. Especially, >>> >> support for this feature is required to fix power button on some recent >>> >> systems. >>> >> + ret = input_register_device(priv->array); >>> >> + if (ret) >>> >> + goto err_free_array_device; >>> >> + >>> >> + return 0; >>> >> + >>> >> +err_free_array_device: >>> >> + input_free_device(priv->array); >>> >> + return ret; >>> > >>> > This return path is more complex than it could be, since you test for ret before >>> > return anyway: >>> > >>> > out: >>> > if (ret) >>> > input_free_device(priv->array); >>> > return ret; >>> > >>> > There is no need for a second return point that I can see. Same for the >>> > hid_input_setup function. We can remove 8 lines. >>> >>> Darren, if I didn't miss anything the function is purely used in >>> ->probe() path and thus devm_ version of input_allocate_device() would >>> make this error path gone. >> These are optional input devices for the driver, so if I understand the >> semantics of devm_ correctly, the input device would remain allocated until such >> time as the driver is unloaded or if it fails to bind - which for systems with >> intel-hid, but no 5 button array, the unused input device would remain allocated >> until system shutdown. >> >> Alex, is that correct? > > Yes, the input device will be only allocated if 5 button array is > supported. Previous firmware that does not support 5 button array > won't be affected. I guess there is a misunderstanding. From code I see that 1. input_allocate_device() is called only at ->probe() and only for devices that *have* 5 button HID array. 2. Time of live of allocated device is until device driver unbound or unloaded. From the above I conclude that using devm_input_allocate_device() will reduce burden on error path w/o affecting functionality. If I;m correct, please, use devm_*() variant.
> > Thanks Alex, I've queued this to testing and it will go to for-next unless the > > CI, Pali, or a user reports a problem. Appreciate all your effort on this one. > > Thanks for the review. I will send another patch to address all comments later. Alex, I have only just now noticed one more really trivial issue: a space is missing in each keymap entry before the first right curly bracket: > +/* 5 button array notification value. */ > +static const struct key_entry intel_array_keymap[] = { > + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ { KE_KEY, 0xC2, { KEY_LEFTMETA } }, ^ etc. > + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ > + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ > + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ > + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ > + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ > + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ > + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ For these two lines, I would personally insert a space next to each of the innermost curly brackets, like this: { KE_SW, 0xC9, { .sw = { SW_ROTATE_LOCK, 0 } } }, > + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ > + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ > + { KE_END }, > +}; To avoid churn, perhaps the maintainers could do this for you while moving this patch to for-next? Or we can simply ignore this, as I said this is just a minor annoyance.
On Mon, Feb 13, 2017 at 1:43 PM, Michał Kępień <kernel@kempniu.pl> wrote: >> Thanks for the review. I will send another patch to address all comments later. > To avoid churn, perhaps the maintainers could do this for you while > moving this patch to for-next? Or we can simply ignore this, as I said > this is just a minor annoyance. I would like to see simplification due to use of devm_input_allocate_device(). Alex, can you send v3 addressing latest comments? If you feel follow up patch would be better, please tell us.
On Tue, Feb 14, 2017 at 7:23 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Feb 13, 2017 at 1:43 PM, Michał Kępień <kernel@kempniu.pl> wrote: >>> Thanks for the review. I will send another patch to address all comments later. > >> To avoid churn, perhaps the maintainers could do this for you while >> moving this patch to for-next? Or we can simply ignore this, as I said >> this is just a minor annoyance. > > I would like to see simplification due to use of devm_input_allocate_device(). > > Alex, can you send v3 addressing latest comments? > If you feel follow up patch would be better, please tell us. Okay, I will send V3 shortly. > > -- > With Best Regards, > Andy Shevchenko
On Thu, Feb 09, 2017 at 09:56:06PM +0200, Andy Shevchenko wrote: > On Thu, Feb 9, 2017 at 5:09 AM, Alex Hung <alex.hung@canonical.com> wrote: > > On Thu, Feb 9, 2017 at 10:22 AM, Darren Hart <dvhart@infradead.org> wrote: > >> On Sat, Feb 04, 2017 at 04:58:33PM +0200, Andy Shevchenko wrote: > >>> On Sat, Feb 4, 2017 at 3:14 AM, Darren Hart <dvhart@infradead.org> wrote: > >>> > On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote: > >>> >> New firmwares include a feature called 5 button array that supports > >>> >> super key, volume up/down, rotation lock and power button. Especially, > >>> >> support for this feature is required to fix power button on some recent > >>> >> systems. > > >>> >> + ret = input_register_device(priv->array); > >>> >> + if (ret) > >>> >> + goto err_free_array_device; > >>> >> + > >>> >> + return 0; > >>> >> + > >>> >> +err_free_array_device: > >>> >> + input_free_device(priv->array); > >>> >> + return ret; > >>> > > >>> > This return path is more complex than it could be, since you test for ret before > >>> > return anyway: > >>> > > >>> > out: > >>> > if (ret) > >>> > input_free_device(priv->array); > >>> > return ret; > >>> > > >>> > There is no need for a second return point that I can see. Same for the > >>> > hid_input_setup function. We can remove 8 lines. > >>> > >>> Darren, if I didn't miss anything the function is purely used in > >>> ->probe() path and thus devm_ version of input_allocate_device() would > >>> make this error path gone. > > >> These are optional input devices for the driver, so if I understand the > >> semantics of devm_ correctly, the input device would remain allocated until such > >> time as the driver is unloaded or if it fails to bind - which for systems with > >> intel-hid, but no 5 button array, the unused input device would remain allocated > >> until system shutdown. > >> > >> Alex, is that correct? > > > > Yes, the input device will be only allocated if 5 button array is > > supported. Previous firmware that does not support 5 button array > > won't be affected. > > I guess there is a misunderstanding. > > >From code I see that > 1. input_allocate_device() is called only at ->probe() and only for > devices that *have* 5 button HID array. > 2. Time of live of allocated device is until device driver unbound or unloaded. It's not critical, and I've applied v3. But, my point was that the driver will not become unbound or unloaded just because the 5 button array input_setup fails at some point, such as with the keymap_setup. In this case, if devm is used and keymap_setup fails, the driver will remain loaded without 5 button array support, and the input device will remain allocated, but unused. The error path is definitely cleaner using devm, but it can leave an unused input device allocated in error cases - although, perhaps such a situation is rare enough that the advantages of devm make it the better choice. So, I'm fine with the v3 Alex sent using devm.
diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c index cb3ab2b..6e796a5 100644 --- a/drivers/platform/x86/intel-hid.c +++ b/drivers/platform/x86/intel-hid.c @@ -1,5 +1,5 @@ /* - * Intel HID event driver for Windows 8 + * Intel HID event & 5 button array driver * * Copyright (C) 2015 Alex Hung <alex.hung@canonical.com> * Copyright (C) 2015 Andrew Lutomirski <luto@kernel.org> @@ -57,8 +57,24 @@ static const struct key_entry intel_hid_keymap[] = { { KE_END }, }; +/* 5 button array notification value. */ +static const struct key_entry intel_array_keymap[] = { + { KE_KEY, 0xC2, { KEY_LEFTMETA} }, /* Press */ + { KE_IGNORE, 0xC3, { KEY_LEFTMETA} }, /* Release */ + { KE_KEY, 0xC4, { KEY_VOLUMEUP} }, /* Press */ + { KE_IGNORE, 0xC5, { KEY_VOLUMEUP} }, /* Release */ + { KE_KEY, 0xC6, { KEY_VOLUMEDOWN} }, /* Press */ + { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN} }, /* Release */ + { KE_SW, 0xC8, { .sw = {SW_ROTATE_LOCK, 1} } }, /* Press */ + { KE_SW, 0xC9, { .sw = {SW_ROTATE_LOCK, 0} } }, /* Release */ + { KE_KEY, 0xCE, { KEY_POWER} }, /* Press */ + { KE_IGNORE, 0xCF, { KEY_POWER} }, /* Release */ + { KE_END }, +}; + struct intel_hid_priv { struct input_dev *input_dev; + struct input_dev *array; }; static int intel_hid_set_enable(struct device *device, int enable) @@ -78,15 +94,43 @@ static int intel_hid_set_enable(struct device *device, int enable) return 0; } +static void intel_button_array_enable(struct device *device, int enable) +{ + struct intel_hid_priv *priv = dev_get_drvdata(device); + acpi_handle handle = ACPI_HANDLE(device); + unsigned long long button_cap; + acpi_status status; + + if (!priv->array) + return; + + /* Query supported platform features */ + status = acpi_evaluate_integer(handle, "BTNC", NULL, &button_cap); + if (ACPI_FAILURE(status)) { + dev_warn(device, "failed to get button capability\n"); + return; + } + + /* Enable|disable features - power button is always enabled */ + status = acpi_execute_simple_method(handle, "BTNE", + enable ? button_cap : 1); + if (ACPI_FAILURE(status)) + dev_warn(device, "failed to set button capability\n"); +} + static int intel_hid_pl_suspend_handler(struct device *device) { intel_hid_set_enable(device, 0); + intel_button_array_enable(device, 0); + return 0; } static int intel_hid_pl_resume_handler(struct device *device) { intel_hid_set_enable(device, 1); + intel_button_array_enable(device, 1); + return 0; } @@ -126,11 +170,43 @@ static int intel_hid_input_setup(struct platform_device *device) return ret; } +static int intel_button_array_input_setup(struct platform_device *device) +{ + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); + int ret; + + /* Setup input device for 5 button array */ + priv->array = input_allocate_device(); + if (!priv->array) + return -ENOMEM; + + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL); + if (ret) + goto err_free_array_device; + + priv->array->dev.parent = &device->dev; + priv->array->name = "Intel HID 5 button array"; + priv->array->id.bustype = BUS_HOST; + + ret = input_register_device(priv->array); + if (ret) + goto err_free_array_device; + + return 0; + +err_free_array_device: + input_free_device(priv->array); + return ret; +} + static void intel_hid_input_destroy(struct platform_device *device) { struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); input_unregister_device(priv->input_dev); + + if (priv->array) + input_unregister_device(priv->array); } static void notify_handler(acpi_handle handle, u32 event, void *context) @@ -140,10 +216,11 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) unsigned long long ev_index; acpi_status status; - /* The platform spec only defines one event code: 0xC0. */ + /* 0xC0 is for HID events, other values are for 5 button array */ if (event != 0xc0) { - dev_warn(&device->dev, "received unknown event (0x%x)\n", - event); + if (!priv->array || + !sparse_keymap_report_event(priv->array, event, 1, true)) + dev_info(&device->dev, "unknown event 0x%x\n", event); return; } @@ -161,8 +238,8 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) static int intel_hid_probe(struct platform_device *device) { acpi_handle handle = ACPI_HANDLE(&device->dev); + unsigned long long event_cap, mode; struct intel_hid_priv *priv; - unsigned long long mode; acpi_status status; int err; @@ -193,6 +270,15 @@ static int intel_hid_probe(struct platform_device *device) return err; } + /* Setup 5 button array */ + status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap); + if (ACPI_SUCCESS(status) && (event_cap & 0x20000)) { + dev_info(&device->dev, "platform supports 5 button array\n"); + err = intel_button_array_input_setup(device); + if (err) + pr_err("Failed to setup Intel 5 button array hotkeys\n"); + } + status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY, notify_handler, @@ -206,6 +292,16 @@ static int intel_hid_probe(struct platform_device *device) if (err) goto err_remove_notify; + if (priv->array) { + intel_button_array_enable(&device->dev, 1); + + /* Call button load method to enable HID power button */ + status = acpi_evaluate_object(handle, "BTNL", NULL, NULL); + if (ACPI_FAILURE(status)) + dev_warn(&device->dev, + "failed to enable HID power button\n"); + } + return 0; err_remove_notify: @@ -224,6 +320,7 @@ static int intel_hid_remove(struct platform_device *device) acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, notify_handler); intel_hid_input_destroy(device); intel_hid_set_enable(&device->dev, 0); + intel_button_array_enable(&device->dev, 0); /* * Even if we failed to shut off the event stream, we can still
New firmwares include a feature called 5 button array that supports super key, volume up/down, rotation lock and power button. Especially, support for this feature is required to fix power button on some recent systems. This patch was tested on a Dell Latitude 7280. Signed-off-by: Alex Hung <alex.hung@canonical.com> --- drivers/platform/x86/intel-hid.c | 107 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 102 insertions(+), 5 deletions(-)