[PATCH][V2] intel-hid: support 5 array button

[PATCH][V2] intel-hid: support 5 array button

[Alex Hung]

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(-)

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
-- 
2.7.4

Re: [PATCH][V2] intel-hid: support 5 array button

[Michał Kępień]

> 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
> 

-- 
Best regards,
Michał Kępień

Re: [PATCH][V2] intel-hid: support 5 array button

[Darren Hart]

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

Re: [PATCH][V2] intel-hid: support 5 array button

[Darren Hart]

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

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH][V2] intel-hid: support 5 array button

[Pali Rohár]

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?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH][V2] intel-hid: support 5 array button

[Michał Kępień]

> 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.

-- 
Best regards,
Michał Kępień

Re: [PATCH][V2] intel-hid: support 5 array button

[Pali Rohár]

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...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH][V2] intel-hid: support 5 array button

[Alex Hung]

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



-- 
Cheers,
Alex Hung

Re: [PATCH][V2] intel-hid: support 5 array button

[Pali Rohár]

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.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH][V2] intel-hid: support 5 array button

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH][V2] intel-hid: support 5 array button

[Darren Hart]

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?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH][V2] intel-hid: support 5 array button

[Alex Hung]

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



-- 
Cheers,
Alex Hung

Re: [PATCH][V2] intel-hid: support 5 array button

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH][V2] intel-hid: support 5 array button

[Darren Hart]

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.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH][V2] intel-hid: support 5 array button

[Alex Hung]

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



-- 
Cheers,
Alex Hung

Re: [PATCH][V2] intel-hid: support 5 array button

[Michał Kępień]

> > 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.

-- 
Best regards,
Michał Kępień

Re: [PATCH][V2] intel-hid: support 5 array button

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH][V2] intel-hid: support 5 array button

[Alex Hung]

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



-- 
Cheers,
Alex Hung