* [V2,1/1] platform/x86: add support for Advantech software defined button
@ 2021-03-12 8:11 YingChieh Ho
2021-03-18 16:14 ` Hans de Goede
0 siblings, 1 reply; 4+ messages in thread
From: YingChieh Ho @ 2021-03-12 8:11 UTC (permalink / raw)
To: Hans de Goede, platform-driver-x86, Rafael J. Wysocki
Cc: Mark Gross, linux-kernel, Andrea Ho
From: "Andrea.Ho" <Andrea.Ho@advantech.com.tw>
Advantech sw_button is a ACPI event trigger button.
With this driver, we can report KEY_EVENT on the
Advantech Tabletop Network Appliances products and it has been
tested in FWA1112VC.
Add the software define button support to report EV_REP key_event
(KEY_PROG1) by pressing button that cloud be get on user
interface and trigger the customized actions.
Signed-off-by: Andrea.Ho <Andrea.Ho@advantech.com.tw>
v2:
- change evdev key-code from BTN_TRIGGER_HAPPY to KEY_PROG1
- to rewrite the driver to be a regular platform_driver
- use specific names instead of generic names,
"Software Button" to "Advantech Software Button",
"button" to "adv_swbutton"
- remove unused defines or use-once defines, ACPI_BUTTON_FILE_INFO,
ACPI_BUTTON_FILE_STATE, ACPI_BUTTON_TYPE_UNKNOWN, ACPI_SWBTN_NAME
---
MAINTAINERS | 5 +
drivers/platform/x86/Kconfig | 11 ++
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/adv_swbutton.c | 192 ++++++++++++++++++++++++++++
4 files changed, 211 insertions(+)
create mode 100644 drivers/platform/x86/adv_swbutton.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 68f21d46614c..e35c48e411b7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -562,6 +562,11 @@ S: Maintained
F: Documentation/scsi/advansys.rst
F: drivers/scsi/advansys.c
+ADVANTECH SWBTN DRIVER
+M: Andrea Ho <Andrea.Ho@advantech.com.tw>
+S: Maintained
+F: drivers/platform/x86/adv_swbutton.c
+
ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
M: Michael Hennerich <michael.hennerich@analog.com>
S: Supported
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0581a54cf562..b1340135c5e9 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -180,6 +180,17 @@ config ACER_WMI
If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M
here.
+config ADV_SWBUTTON
+ tristate "Advantech ACPI Software Button Driver"
+ depends on ACPI
+ help
+ Say Y here to enable support for Advantech software defined
+ button feature. More information can be fount at
+ <http://www.advantech.com.tw/products/>
+
+ To compile this driver as a module, choose M here. The module will
+ be called adv_swbutton.
+
config APPLE_GMUX
tristate "Apple Gmux Driver"
depends on ACPI && PCI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2b85852a1a87..76a321fc58ba 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -22,6 +22,9 @@ obj-$(CONFIG_ACERHDF) += acerhdf.o
obj-$(CONFIG_ACER_WIRELESS) += acer-wireless.o
obj-$(CONFIG_ACER_WMI) += acer-wmi.o
+# Advantech
+obj-$(CONFIG_ADV_SWBUTTON) += adv_swbutton.o
+
# Apple
obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o
diff --git a/drivers/platform/x86/adv_swbutton.c
b/drivers/platform/x86/adv_swbutton.c
new file mode 100644
index 000000000000..f4387220e8a0
--- /dev/null
+++ b/drivers/platform/x86/adv_swbutton.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * adv_swbutton.c - Software Button Interface Driver.
+ *
+ * (C) Copyright 2020 Advantech Corporation, Inc
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/version.h>
+#include <linux/types.h>
+#include <linux/proc_fs.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+#include <acpi/button.h>
+#include <acpi/acpi_drivers.h>
+
+#define MODULE_NAME "adv_swbutton"
+
+#define ACPI_BUTTON_HID_SWBTN "AHC0310"
+#define ACPI_BUTTON_TYPE_SOFTWARE 0x07
+
+#define ACPI_BUTTON_NOTIFY_SWBTN_RELEASE 0x86
+#define ACPI_BUTTON_NOTIFY_SWBTN_PRESSED 0x85
+
+#define SOFTWARE_BUTTON KEY_PROG1
+
+#define _COMPONENT ACPI_BUTTON_COMPONENT
+
+struct acpi_button {
+ unsigned int type;
+ struct input_dev *input;
+ char phys[32];
+ bool pressed;
+};
+
+/*-------------------------------------------------------------------------
+ * Driver Interface
+ *--------------------------------------------------------------------------
+ */
+static void acpi_button_notify(acpi_handle handle, u32 event, void *context)
+{
+ struct acpi_device *device = context;
+
+ struct acpi_button *button = dev_get_drvdata(&device->dev);
+ struct input_dev *input;
+
+ int keycode, BTN_KEYCODE = SOFTWARE_BUTTON;
+
+ switch (event) {
+ case ACPI_BUTTON_NOTIFY_SWBTN_RELEASE:
+ input = button->input;
+
+ if (!button->pressed)
+ return;
+
+ keycode = test_bit(BTN_KEYCODE, input->keybit) ?
+ BTN_KEYCODE : KEY_UNKNOWN;
+
+ button->pressed = false;
+
+ input_report_key(input, keycode, 0);
+ input_sync(input);
+ break;
+ case ACPI_BUTTON_NOTIFY_SWBTN_PRESSED:
+ input = button->input;
+ button->pressed = true;
+
+ keycode = test_bit(BTN_KEYCODE, input->keybit) ?
+ BTN_KEYCODE : KEY_UNKNOWN;
+
+ input_report_key(input, keycode, 1);
+ input_sync(input);
+ break;
+ default:
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "Unsupported event [0x%x]\n", event));
+ break;
+ }
+}
+
+static int acpi_button_probe(struct platform_device *device)
+{
+ struct acpi_device *acpi_device;
+ struct acpi_button *button;
+ struct input_dev *input;
+ acpi_status status;
+
+ int error;
+
+ acpi_device = ACPI_COMPANION(&device->dev);
+ if (!acpi_device) {
+ dev_err(&device->dev, "ACPI companion is missing\n");
+ return -ENOMEM;
+ }
+
+ status = acpi_install_notify_handler(acpi_device->handle,
+ ACPI_DEVICE_NOTIFY,
+ acpi_button_notify,
+ acpi_device);
+ if (ACPI_FAILURE(status)) {
+ dev_err(&device->dev, "Error installing notify handler\n");
+ error = -ENODEV;
+ }
+
+ button = devm_kzalloc(&acpi_device->dev, sizeof(*button), GFP_KERNEL);
+ if (!button)
+ return -ENOMEM;
+
+ dev_set_drvdata(&acpi_device->dev, button);
+
+ input = devm_input_allocate_device(&acpi_device->dev);
+ if (!input) {
+ error = -ENOMEM;
+ goto err_free_mem;
+ }
+
+ button->input = input;
+ button->type = ACPI_BUTTON_TYPE_SOFTWARE;
+ button->pressed = false;
+
+ snprintf(button->phys, sizeof(button->phys),
"%s/button/input0", ACPI_BUTTON_HID_SWBTN);
+
+ input->name = "Advantech Software Button";
+ input->phys = button->phys;
+ input->id.bustype = BUS_HOST;
+ input->id.product = button->type;
+ input->dev.parent = &acpi_device->dev;
+
+ switch (button->type) {
+ case ACPI_BUTTON_TYPE_SOFTWARE:
+ set_bit(EV_KEY, input->evbit);
+ set_bit(EV_REP, input->evbit);
+
+ input_set_capability(input, EV_KEY, SOFTWARE_BUTTON);
+ break;
+ }
+
+ input_set_drvdata(input, acpi_device);
+ error = input_register_device(input);
+ if (error)
+ return error;
+
+ device_init_wakeup(&acpi_device->dev, true);
+
+ return 0;
+
+err_free_mem:
+ devm_kfree(&device->dev, button);
+ return error;
+}
+
+static int acpi_button_remove(struct platform_device *device)
+{
+ struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev);
+ struct acpi_button *button = dev_get_drvdata(&acpi_device->dev);
+
+ acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
+ acpi_button_notify);
+
+ input_unregister_device(button->input);
+ devm_kfree(&acpi_device->dev, button);
+
+ return 0;
+}
+
+static const struct acpi_device_id button_device_ids[] = {
+ {ACPI_BUTTON_HID_SWBTN, 0},
+ {"", 0},
+};
+
+MODULE_DEVICE_TABLE(acpi, button_device_ids);
+
+static struct platform_driver acpi_button_driver = {
+ .driver = {
+ .name = MODULE_NAME,
+ .acpi_match_table = button_device_ids,
+ },
+ .probe = acpi_button_probe,
+ .remove = acpi_button_remove,
+};
+
+module_platform_driver(acpi_button_driver);
+
+ACPI_MODULE_NAME(MODULE_NAME);
+
+MODULE_AUTHOR("Andrea Ho");
+MODULE_DESCRIPTION("Advantech ACPI SW Button Driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [V2,1/1] platform/x86: add support for Advantech software defined button
2021-03-12 8:11 [V2,1/1] platform/x86: add support for Advantech software defined button YingChieh Ho
@ 2021-03-18 16:14 ` Hans de Goede
2021-03-19 4:00 ` YingChieh, Ho
0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2021-03-18 16:14 UTC (permalink / raw)
To: YingChieh Ho, platform-driver-x86, Rafael J. Wysocki
Cc: Mark Gross, linux-kernel, Andrea Ho
Hi,
On 3/12/21 9:11 AM, YingChieh Ho wrote:
> From: "Andrea.Ho" <Andrea.Ho@advantech.com.tw>
>
> Advantech sw_button is a ACPI event trigger button.
>
> With this driver, we can report KEY_EVENT on the
> Advantech Tabletop Network Appliances products and it has been
> tested in FWA1112VC.
>
> Add the software define button support to report EV_REP key_event
> (KEY_PROG1) by pressing button that cloud be get on user
> interface and trigger the customized actions.
>
> Signed-off-by: Andrea.Ho <Andrea.Ho@advantech.com.tw>
>
> v2:
> - change evdev key-code from BTN_TRIGGER_HAPPY to KEY_PROG1
> - to rewrite the driver to be a regular platform_driver
> - use specific names instead of generic names,
> "Software Button" to "Advantech Software Button",
> "button" to "adv_swbutton"
> - remove unused defines or use-once defines, ACPI_BUTTON_FILE_INFO,
> ACPI_BUTTON_FILE_STATE, ACPI_BUTTON_TYPE_UNKNOWN, ACPI_SWBTN_NAME
Thank you this version is much better, I have various review remarks below.
Please send a v3 with those fixed.
> ---
> MAINTAINERS | 5 +
> drivers/platform/x86/Kconfig | 11 ++
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/adv_swbutton.c | 192 ++++++++++++++++++++++++++++
> 4 files changed, 211 insertions(+)
> create mode 100644 drivers/platform/x86/adv_swbutton.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 68f21d46614c..e35c48e411b7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -562,6 +562,11 @@ S: Maintained
> F: Documentation/scsi/advansys.rst
> F: drivers/scsi/advansys.c
>
> +ADVANTECH SWBTN DRIVER
> +M: Andrea Ho <Andrea.Ho@advantech.com.tw>
> +S: Maintained
> +F: drivers/platform/x86/adv_swbutton.c
> +
> ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
> M: Michael Hennerich <michael.hennerich@analog.com>
> S: Supported
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 0581a54cf562..b1340135c5e9 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -180,6 +180,17 @@ config ACER_WMI
> If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M
> here.
This is supposed to be indented by a space (from the diff format) and then
a tab + 2 spaces, but in your patch this is now indented by 10 spaces.
In general your entire patch has all tabs replaced by spaces, I guess that
your mail-client or editor is mangling things. Please do the following:
1. Check your patch for checkpatch errors:
git format-patch HEAD~
scripts/checkpatch.pl 0001-platform...patch
And fix all reported issues, both whitespace issues and others.
2. Send the next version of the patch like this:
git format-patch -v3 HEAD~
git send-email v3-0001-platform...patch
Do NOT edit the v3-0001-platform...patch file between these 2 steps.
> +config ADV_SWBUTTON
> + tristate "Advantech ACPI Software Button Driver"
You are using indent steps of 4 spaces here, that should be a tab
> + depends on ACPI
> + help
> + Say Y here to enable support for Advantech software defined
> + button feature. More information can be fount at
> + <http://www.advantech.com.tw/products/>
And a tab and 2 spaces for the help text.
> +
> + To compile this driver as a module, choose M here. The module will
> + be called adv_swbutton.
> +
> config APPLE_GMUX
> tristate "Apple Gmux Driver"
> depends on ACPI && PCI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 2b85852a1a87..76a321fc58ba 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -22,6 +22,9 @@ obj-$(CONFIG_ACERHDF) += acerhdf.o
> obj-$(CONFIG_ACER_WIRELESS) += acer-wireless.o
> obj-$(CONFIG_ACER_WMI) += acer-wmi.o
>
> +# Advantech
> +obj-$(CONFIG_ADV_SWBUTTON) += adv_swbutton.o
> +
The indent before the += should use tabs not spaces.
> # Apple
> obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o
>
> diff --git a/drivers/platform/x86/adv_swbutton.c
> b/drivers/platform/x86/adv_swbutton.c
> new file mode 100644
> index 000000000000..f4387220e8a0
> --- /dev/null
> +++ b/drivers/platform/x86/adv_swbutton.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * adv_swbutton.c - Software Button Interface Driver.
> + *
> + * (C) Copyright 2020 Advantech Corporation, Inc
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/version.h>
> +#include <linux/types.h>
> +#include <linux/proc_fs.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <acpi/button.h>
> +#include <acpi/acpi_drivers.h>
Please drop the following unused defines:
#include <linux/init.h>
#include <linux/version.h>
#include <linux/types.h>
#include <linux/proc_fs.h>
#include <linux/slab.h>
#include <acpi/button.h>
#include <acpi/acpi_drivers.h>
And sort the remaining includes alphabetically.
> +
> +#define MODULE_NAME "adv_swbutton"
Please drop this define, instead just set the .driver.name part of the platform_driver struct directly to "adv_swbutton".
> +
> +#define ACPI_BUTTON_HID_SWBTN "AHC0310"
> +#define ACPI_BUTTON_TYPE_SOFTWARE 0x07
Please drop the ACPI_BUTTON_TYPE_SOFTWARE define.
> +
> +#define ACPI_BUTTON_NOTIFY_SWBTN_RELEASE 0x86
> +#define ACPI_BUTTON_NOTIFY_SWBTN_PRESSED 0x85
> +
> +#define SOFTWARE_BUTTON KEY_PROG1
Please drop this unnecessary define and just use KEY_PROG1 directly.
> +
> +#define _COMPONENT ACPI_BUTTON_COMPONENT
> +
> +struct acpi_button {
Please give this struct-type a different name, e.g. use:
struct adv_swbutton {
> + unsigned int type;
And drop the type here, it is always set to ACPI_BUTTON_TYPE_SOFTWARE, so it has no function.
> + struct input_dev *input;
> + char phys[32];
> + bool pressed;
Also please drop the pressed bool, see below.
> +};
> +
> +/*-------------------------------------------------------------------------
> + * Driver Interface
> + *--------------------------------------------------------------------------
> + */
> +static void acpi_button_notify(acpi_handle handle, u32 event, void *context)
> +{
> + struct acpi_device *device = context;
> +
> + struct acpi_button *button = dev_get_drvdata(&device->dev);
You must not call dev_set_drvdata on the acpi_device since you are not binding
to the acpi_device, you are binding to the platform_device, so set the drv_data
there and please don't touch the acpi_device.
> + struct input_dev *input;
> +
> + int keycode, BTN_KEYCODE = SOFTWARE_BUTTON;
Please drop these 3 variables, they are not necessary.
> +
> + switch (event) {
> + case ACPI_BUTTON_NOTIFY_SWBTN_RELEASE:
> + input = button->input;
Just use button->input below instead of having this unnecessary
helper variable.
> +
> + if (!button->pressed)
> + return;
This is not necessary, the input core takes care of not sending
events when the state does not change.
> +
> + keycode = test_bit(BTN_KEYCODE, input->keybit) ?
> + BTN_KEYCODE : KEY_UNKNOWN;
No idea why you are doing this, but it is not necessary,
also it is wrong, you should be using KEY_PROG1 here.
> +
> + button->pressed = false;
Keeping track of the pressed state is not necessary.
> +
> + input_report_key(input, keycode, 0);
> + input_sync(input);
Basically this entire case can be simplified to:
case ACPI_BUTTON_NOTIFY_SWBTN_RELEASE:
input_report_key(button->input, KEY_PROG1, 0);
input_sync(button->input);
break;
> + break;
This break is indented wrong.
> + case ACPI_BUTTON_NOTIFY_SWBTN_PRESSED:
> + input = button->input;
> + button->pressed = true;
> +
> + keycode = test_bit(BTN_KEYCODE, input->keybit) ?
> + BTN_KEYCODE : KEY_UNKNOWN;
> +
> + input_report_key(input, keycode, 1);
> + input_sync(input);
> + break;
And the same as above here, this can be simplified to:
case ACPI_BUTTON_NOTIFY_SWBTN_PRESSED:
input_report_key(button->input, KEY_PROG1, 1);
input_sync(button->input);
break;
> + default:
> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> + "Unsupported event [0x%x]\n", event));
Please don't use ACPI_DEBUG_PRINTF outside of drivers/acpi, instead
use:
dev_dbg(&platform_device->dev, ...);
> + break;
There is no need for a break at the end of a switch-case
> + }
> +}
> +
> +static int acpi_button_probe(struct platform_device *device)
> +{
> + struct acpi_device *acpi_device;
> + struct acpi_button *button;
> + struct input_dev *input;
> + acpi_status status;
> +
> + int error;
> +
> + acpi_device = ACPI_COMPANION(&device->dev);
> + if (!acpi_device) {
> + dev_err(&device->dev, "ACPI companion is missing\n");
> + return -ENOMEM;
> + }
> +
> + status = acpi_install_notify_handler(acpi_device->handle,
> + ACPI_DEVICE_NOTIFY,
> + acpi_button_notify,
> + acpi_device);
> + if (ACPI_FAILURE(status)) {
> + dev_err(&device->dev, "Error installing notify handler\n");
> + error = -ENODEV;
> + }
This needs to be done at the end of probe, if the notifier now runs, before
you have called dev_set_drvdata(), you will have a NULL pointer deref,
and even if the dev_set_drvdata() then the notifier may run before you have
set button->input = input and you still have a NULL pointer deref.
So first setup everything and only then call acpi_install_notify_handler().
> +
> + button = devm_kzalloc(&acpi_device->dev, sizeof(*button), GFP_KERNEL);
> + if (!button)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&acpi_device->dev, button);
Call this on the platform_device, so:
dev_set_drvdata(&device->dev, button);
Also see above.
> +
> + input = devm_input_allocate_device(&acpi_device->dev);
> + if (!input) {
> + error = -ENOMEM;
> + goto err_free_mem;
There is no need for the goto err_free_mem here (see below), so this
can be simplified to just:
input = devm_input_allocate_device(&acpi_device->dev);
if (!input)
return -ENOMEM;
> + }
> +
> + button->input = input;
> + button->type = ACPI_BUTTON_TYPE_SOFTWARE;
> + button->pressed = false;
Drop the setting of type/pressed, see above.
> +
> + snprintf(button->phys, sizeof(button->phys),
> "%s/button/input0", ACPI_BUTTON_HID_SWBTN);
> +
> + input->name = "Advantech Software Button";
> + input->phys = button->phys;
> + input->id.bustype = BUS_HOST;
> + input->id.product = button->type;
Don't set product to some semi-random value. Just leave it at 0.
> + input->dev.parent = &acpi_device->dev;
This should be:
input->dev.parent = &device->dev;
> +
> + switch (button->type) {
> + case ACPI_BUTTON_TYPE_SOFTWARE:
> + set_bit(EV_KEY, input->evbit);
> + set_bit(EV_REP, input->evbit);
> +
> + input_set_capability(input, EV_KEY, SOFTWARE_BUTTON);
> + break;
> + }
button->type always equals ACPI_BUTTON_TYPE_SOFTWARE, so you can drop the whole
switch-case here and input_set_capability(input, EV_KEY, ...) already does:
set_bit(EV_KEY, input->evbit);, so this entire block can be simplified to just:
set_bit(EV_REP, input->evbit);
input_set_capability(input, EV_KEY, KEY_PROG1);
> +
> + input_set_drvdata(input, acpi_device);
You are never using the drvdata associated with the input_dev, so
this can be dropped. Also please don't use acpi_device for this/
> + error = input_register_device(input);
> + if (error)
> + return error;
> +
> + device_init_wakeup(&acpi_device->dev, true);
Don't use acpi_device, instead do:
device_init_wakeup(&device->dev, true);
> +
> + return 0;
> +
> +err_free_mem:
> + devm_kfree(&device->dev, button);
devm_kzalloced mem will automatically be free-ed when the probe function
returns with an error, or when the driver is unbound (removed) so there
is no need to call devm_kfree() here.
> + return error;
> +}
> +
> +static int acpi_button_remove(struct platform_device *device)
> +{
> + struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev);
> + struct acpi_button *button = dev_get_drvdata(&acpi_device->dev);
> +
> + acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
> + acpi_button_notify);
> +
> + input_unregister_device(button->input);
> + devm_kfree(&acpi_device->dev, button);
The input_unregister_device() and devm_kfree() will be done automatically
when the remove function exits, so you can drop these 2 calls.
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id button_device_ids[] = {
> + {ACPI_BUTTON_HID_SWBTN, 0},
> + {"", 0},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, button_device_ids);
> +
> +static struct platform_driver acpi_button_driver = {
> + .driver = {
> + .name = MODULE_NAME,
> + .acpi_match_table = button_device_ids,
> + },
> + .probe = acpi_button_probe,
> + .remove = acpi_button_remove,
> +};
> +
> +module_platform_driver(acpi_button_driver);
> +
This is not an ACPI driver, please drop the ACPI_MODULE_NAME() call.
> +ACPI_MODULE_NAME(MODULE_NAME);
> +
> +MODULE_AUTHOR("Andrea Ho");
> +MODULE_DESCRIPTION("Advantech ACPI SW Button Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
Regards,
Hans
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [V2,1/1] platform/x86: add support for Advantech software defined button
2021-03-18 16:14 ` Hans de Goede
@ 2021-03-19 4:00 ` YingChieh, Ho
2021-03-23 20:03 ` Hans de Goede
0 siblings, 1 reply; 4+ messages in thread
From: YingChieh, Ho @ 2021-03-19 4:00 UTC (permalink / raw)
To: Hans de Goede
Cc: platform-driver-x86, Rafael J. Wysocki, Mark Gross, linux-kernel,
Andrea Ho
Hi Hans,
Thank you for your kindly review.
As a beginner in driver coding, I got gains much from your suggestions.
Path v3 is ready at patchwork.^^
Hans de Goede <hdegoede@redhat.com> 於 2021年3月19日 週五 上午12:21寫道：
>
> Hi,
>
> On 3/12/21 9:11 AM, YingChieh Ho wrote:
> > From: "Andrea.Ho" <Andrea.Ho@advantech.com.tw>
> >
> > Advantech sw_button is a ACPI event trigger button.
> >
> > With this driver, we can report KEY_EVENT on the
> > Advantech Tabletop Network Appliances products and it has been
> > tested in FWA1112VC.
> >
> > Add the software define button support to report EV_REP key_event
> > (KEY_PROG1) by pressing button that cloud be get on user
> > interface and trigger the customized actions.
> >
> > Signed-off-by: Andrea.Ho <Andrea.Ho@advantech.com.tw>
> >
> > v2:
> > - change evdev key-code from BTN_TRIGGER_HAPPY to KEY_PROG1
> > - to rewrite the driver to be a regular platform_driver
> > - use specific names instead of generic names,
> > "Software Button" to "Advantech Software Button",
> > "button" to "adv_swbutton"
> > - remove unused defines or use-once defines, ACPI_BUTTON_FILE_INFO,
> > ACPI_BUTTON_FILE_STATE, ACPI_BUTTON_TYPE_UNKNOWN, ACPI_SWBTN_NAME
>
> Thank you this version is much better, I have various review remarks below.
>
> Please send a v3 with those fixed.
>
>
> > ---
> > MAINTAINERS | 5 +
> > drivers/platform/x86/Kconfig | 11 ++
> > drivers/platform/x86/Makefile | 3 +
> > drivers/platform/x86/adv_swbutton.c | 192 ++++++++++++++++++++++++++++
> > 4 files changed, 211 insertions(+)
> > create mode 100644 drivers/platform/x86/adv_swbutton.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 68f21d46614c..e35c48e411b7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -562,6 +562,11 @@ S: Maintained
> > F: Documentation/scsi/advansys.rst
> > F: drivers/scsi/advansys.c
> >
> > +ADVANTECH SWBTN DRIVER
> > +M: Andrea Ho <Andrea.Ho@advantech.com.tw>
> > +S: Maintained
> > +F: drivers/platform/x86/adv_swbutton.c
> > +
> > ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
> > M: Michael Hennerich <michael.hennerich@analog.com>
> > S: Supported
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 0581a54cf562..b1340135c5e9 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -180,6 +180,17 @@ config ACER_WMI
> > If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M
> > here.
>
> This is supposed to be indented by a space (from the diff format) and then
> a tab + 2 spaces, but in your patch this is now indented by 10 spaces.
>
> In general your entire patch has all tabs replaced by spaces, I guess that
> your mail-client or editor is mangling things. Please do the following:
>
> 1. Check your patch for checkpatch errors:
>
> git format-patch HEAD~
> scripts/checkpatch.pl 0001-platform...patch
>
> And fix all reported issues, both whitespace issues and others.
>
> 2. Send the next version of the patch like this:
>
> git format-patch -v3 HEAD~
> git send-email v3-0001-platform...patch
>
> Do NOT edit the v3-0001-platform...patch file between these 2 steps.
>
>
> > +config ADV_SWBUTTON
> > + tristate "Advantech ACPI Software Button Driver"
>
> You are using indent steps of 4 spaces here, that should be a tab
> > + depends on ACPI
> > + help
> > + Say Y here to enable support for Advantech software defined
> > + button feature. More information can be fount at
> > + <http://www.advantech.com.tw/products/>
>
> And a tab and 2 spaces for the help text.
>
> > +
> > + To compile this driver as a module, choose M here. The module will
> > + be called adv_swbutton.
> > +
> > config APPLE_GMUX
> > tristate "Apple Gmux Driver"
> > depends on ACPI && PCI
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 2b85852a1a87..76a321fc58ba 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -22,6 +22,9 @@ obj-$(CONFIG_ACERHDF) += acerhdf.o
> > obj-$(CONFIG_ACER_WIRELESS) += acer-wireless.o
> > obj-$(CONFIG_ACER_WMI) += acer-wmi.o
> >
> > +# Advantech
> > +obj-$(CONFIG_ADV_SWBUTTON) += adv_swbutton.o
> > +
>
> The indent before the += should use tabs not spaces.
>
> > # Apple
> > obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o
> >
> > diff --git a/drivers/platform/x86/adv_swbutton.c
> > b/drivers/platform/x86/adv_swbutton.c
> > new file mode 100644
> > index 000000000000..f4387220e8a0
> > --- /dev/null
> > +++ b/drivers/platform/x86/adv_swbutton.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * adv_swbutton.c - Software Button Interface Driver.
> > + *
> > + * (C) Copyright 2020 Advantech Corporation, Inc
> > + *
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/version.h>
> > +#include <linux/types.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/input.h>
> > +#include <linux/slab.h>
> > +#include <linux/acpi.h>
> > +#include <linux/platform_device.h>
> > +#include <acpi/button.h>
> > +#include <acpi/acpi_drivers.h>
>
> Please drop the following unused defines:
>
> #include <linux/init.h>
> #include <linux/version.h>
> #include <linux/types.h>
> #include <linux/proc_fs.h>
> #include <linux/slab.h>
> #include <acpi/button.h>
> #include <acpi/acpi_drivers.h>
>
> And sort the remaining includes alphabetically.
>
> > +
> > +#define MODULE_NAME "adv_swbutton"
>
> Please drop this define, instead just set the .driver.name part of the platform_driver struct directly to "adv_swbutton".
>
> > +
> > +#define ACPI_BUTTON_HID_SWBTN "AHC0310"
> > +#define ACPI_BUTTON_TYPE_SOFTWARE 0x07
>
> Please drop the ACPI_BUTTON_TYPE_SOFTWARE define.
>
> > +
> > +#define ACPI_BUTTON_NOTIFY_SWBTN_RELEASE 0x86
> > +#define ACPI_BUTTON_NOTIFY_SWBTN_PRESSED 0x85
> > +
> > +#define SOFTWARE_BUTTON KEY_PROG1
>
> Please drop this unnecessary define and just use KEY_PROG1 directly.
>
> > +
> > +#define _COMPONENT ACPI_BUTTON_COMPONENT
> > +
> > +struct acpi_button {
>
> Please give this struct-type a different name, e.g. use:
>
> struct adv_swbutton {
>
> > + unsigned int type;
>
> And drop the type here, it is always set to ACPI_BUTTON_TYPE_SOFTWARE, so it has no function.
>
> > + struct input_dev *input;
> > + char phys[32];
> > + bool pressed;
>
> Also please drop the pressed bool, see below.
>
>
> > +};
> > +
> > +/*-------------------------------------------------------------------------
> > + * Driver Interface
> > + *--------------------------------------------------------------------------
> > + */
> > +static void acpi_button_notify(acpi_handle handle, u32 event, void *context)
> > +{
> > + struct acpi_device *device = context;
> > +
> > + struct acpi_button *button = dev_get_drvdata(&device->dev);
>
> You must not call dev_set_drvdata on the acpi_device since you are not binding
> to the acpi_device, you are binding to the platform_device, so set the drv_data
> there and please don't touch the acpi_device.
>
> > + struct input_dev *input;
> > +
> > + int keycode, BTN_KEYCODE = SOFTWARE_BUTTON;
>
> Please drop these 3 variables, they are not necessary.
>
> > +
> > + switch (event) {
> > + case ACPI_BUTTON_NOTIFY_SWBTN_RELEASE:
> > + input = button->input;
>
> Just use button->input below instead of having this unnecessary
> helper variable.
>
> > +
> > + if (!button->pressed)
> > + return;
>
> This is not necessary, the input core takes care of not sending
> events when the state does not change.
>
> > +
> > + keycode = test_bit(BTN_KEYCODE, input->keybit) ?
> > + BTN_KEYCODE : KEY_UNKNOWN;
>
> No idea why you are doing this, but it is not necessary,
> also it is wrong, you should be using KEY_PROG1 here.
>
> > +
> > + button->pressed = false;
>
> Keeping track of the pressed state is not necessary.
>
> > +
> > + input_report_key(input, keycode, 0);
> > + input_sync(input);
>
> Basically this entire case can be simplified to:
>
> case ACPI_BUTTON_NOTIFY_SWBTN_RELEASE:
> input_report_key(button->input, KEY_PROG1, 0);
> input_sync(button->input);
> break;
>
>
> > + break;
>
> This break is indented wrong.
>
> > + case ACPI_BUTTON_NOTIFY_SWBTN_PRESSED:
> > + input = button->input;
> > + button->pressed = true;
> > +
> > + keycode = test_bit(BTN_KEYCODE, input->keybit) ?
> > + BTN_KEYCODE : KEY_UNKNOWN;
> > +
> > + input_report_key(input, keycode, 1);
> > + input_sync(input);
> > + break;
>
> And the same as above here, this can be simplified to:
>
> case ACPI_BUTTON_NOTIFY_SWBTN_PRESSED:
> input_report_key(button->input, KEY_PROG1, 1);
> input_sync(button->input);
> break;
>
>
> > + default:
> > + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > + "Unsupported event [0x%x]\n", event));
>
> Please don't use ACPI_DEBUG_PRINTF outside of drivers/acpi, instead
> use:
>
> dev_dbg(&platform_device->dev, ...);
>
> > + break;
>
> There is no need for a break at the end of a switch-case
>
> > + }
> > +}
> > +
> > +static int acpi_button_probe(struct platform_device *device)
> > +{
> > + struct acpi_device *acpi_device;
> > + struct acpi_button *button;
> > + struct input_dev *input;
> > + acpi_status status;
> > +
> > + int error;
> > +
> > + acpi_device = ACPI_COMPANION(&device->dev);
> > + if (!acpi_device) {
> > + dev_err(&device->dev, "ACPI companion is missing\n");
> > + return -ENOMEM;
> > + }
> > +
> > + status = acpi_install_notify_handler(acpi_device->handle,
> > + ACPI_DEVICE_NOTIFY,
> > + acpi_button_notify,
> > + acpi_device);
> > + if (ACPI_FAILURE(status)) {
> > + dev_err(&device->dev, "Error installing notify handler\n");
> > + error = -ENODEV;
> > + }
>
> This needs to be done at the end of probe, if the notifier now runs, before
> you have called dev_set_drvdata(), you will have a NULL pointer deref,
> and even if the dev_set_drvdata() then the notifier may run before you have
> set button->input = input and you still have a NULL pointer deref.
>
> So first setup everything and only then call acpi_install_notify_handler().
>
> > +
> > + button = devm_kzalloc(&acpi_device->dev, sizeof(*button), GFP_KERNEL);
> > + if (!button)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(&acpi_device->dev, button);
>
> Call this on the platform_device, so:
>
> dev_set_drvdata(&device->dev, button);
>
> Also see above.
>
> > +
> > + input = devm_input_allocate_device(&acpi_device->dev);
> > + if (!input) {
> > + error = -ENOMEM;
> > + goto err_free_mem;
>
> There is no need for the goto err_free_mem here (see below), so this
> can be simplified to just:
>
> input = devm_input_allocate_device(&acpi_device->dev);
> if (!input)
> return -ENOMEM;
>
> > + }
> > +
> > + button->input = input;
> > + button->type = ACPI_BUTTON_TYPE_SOFTWARE;
> > + button->pressed = false;
>
> Drop the setting of type/pressed, see above.
>
>
> > +
> > + snprintf(button->phys, sizeof(button->phys),
> > "%s/button/input0", ACPI_BUTTON_HID_SWBTN);
> > +
> > + input->name = "Advantech Software Button";
> > + input->phys = button->phys;
> > + input->id.bustype = BUS_HOST;
> > + input->id.product = button->type;
>
> Don't set product to some semi-random value. Just leave it at 0.
>
> > + input->dev.parent = &acpi_device->dev;
>
> This should be:
>
> input->dev.parent = &device->dev;
>
> > +
> > + switch (button->type) {
> > + case ACPI_BUTTON_TYPE_SOFTWARE:
> > + set_bit(EV_KEY, input->evbit);
> > + set_bit(EV_REP, input->evbit);
> > +
> > + input_set_capability(input, EV_KEY, SOFTWARE_BUTTON);
> > + break;
> > + }
>
> button->type always equals ACPI_BUTTON_TYPE_SOFTWARE, so you can drop the whole
> switch-case here and input_set_capability(input, EV_KEY, ...) already does:
> set_bit(EV_KEY, input->evbit);, so this entire block can be simplified to just:
>
> set_bit(EV_REP, input->evbit);
> input_set_capability(input, EV_KEY, KEY_PROG1);
>
>
> > +
> > + input_set_drvdata(input, acpi_device);
>
> You are never using the drvdata associated with the input_dev, so
> this can be dropped. Also please don't use acpi_device for this/
>
> > + error = input_register_device(input);
> > + if (error)
> > + return error;
> > +
> > + device_init_wakeup(&acpi_device->dev, true);
>
> Don't use acpi_device, instead do:
>
> device_init_wakeup(&device->dev, true);
>
>
>
>
> > +
> > + return 0;
> > +
> > +err_free_mem:
> > + devm_kfree(&device->dev, button);
>
> devm_kzalloced mem will automatically be free-ed when the probe function
> returns with an error, or when the driver is unbound (removed) so there
> is no need to call devm_kfree() here.
>
> > + return error;
> > +}
> > +
> > +static int acpi_button_remove(struct platform_device *device)
> > +{
> > + struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev);
> > + struct acpi_button *button = dev_get_drvdata(&acpi_device->dev);
> > +
> > + acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
> > + acpi_button_notify);
> > +
> > + input_unregister_device(button->input);
> > + devm_kfree(&acpi_device->dev, button);
>
> The input_unregister_device() and devm_kfree() will be done automatically
> when the remove function exits, so you can drop these 2 calls.
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct acpi_device_id button_device_ids[] = {
> > + {ACPI_BUTTON_HID_SWBTN, 0},
> > + {"", 0},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(acpi, button_device_ids);
> > +
> > +static struct platform_driver acpi_button_driver = {
> > + .driver = {
> > + .name = MODULE_NAME,
> > + .acpi_match_table = button_device_ids,
> > + },
> > + .probe = acpi_button_probe,
> > + .remove = acpi_button_remove,
> > +};
> > +
> > +module_platform_driver(acpi_button_driver);
> > +
>
> This is not an ACPI driver, please drop the ACPI_MODULE_NAME() call.
>
> > +ACPI_MODULE_NAME(MODULE_NAME);
> > +
> > +MODULE_AUTHOR("Andrea Ho");
> > +MODULE_DESCRIPTION("Advantech ACPI SW Button Driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >
>
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [V2,1/1] platform/x86: add support for Advantech software defined button
2021-03-19 4:00 ` YingChieh, Ho
@ 2021-03-23 20:03 ` Hans de Goede
0 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2021-03-23 20:03 UTC (permalink / raw)
To: YingChieh, Ho
Cc: platform-driver-x86, Rafael J. Wysocki, Mark Gross, linux-kernel,
Andrea Ho
Hi,
On 3/19/21 5:00 AM, YingChieh, Ho wrote:
> Hi Hans,
>
> Thank you for your kindly review.
> As a beginner in driver coding, I got gains much from your suggestions.
I'm glad to hear that my reviews are helping you.
> Path v3 is ready at patchwork.^^
Thank you, I'll try to take a look at it soon.
Regards,
Hans
>
> Hans de Goede <hdegoede@redhat.com> 於 2021年3月19日 週五 上午12:21寫道：
>>
>> Hi,
>>
>> On 3/12/21 9:11 AM, YingChieh Ho wrote:
>>> From: "Andrea.Ho" <Andrea.Ho@advantech.com.tw>
>>>
>>> Advantech sw_button is a ACPI event trigger button.
>>>
>>> With this driver, we can report KEY_EVENT on the
>>> Advantech Tabletop Network Appliances products and it has been
>>> tested in FWA1112VC.
>>>
>>> Add the software define button support to report EV_REP key_event
>>> (KEY_PROG1) by pressing button that cloud be get on user
>>> interface and trigger the customized actions.
>>>
>>> Signed-off-by: Andrea.Ho <Andrea.Ho@advantech.com.tw>
>>>
>>> v2:
>>> - change evdev key-code from BTN_TRIGGER_HAPPY to KEY_PROG1
>>> - to rewrite the driver to be a regular platform_driver
>>> - use specific names instead of generic names,
>>> "Software Button" to "Advantech Software Button",
>>> "button" to "adv_swbutton"
>>> - remove unused defines or use-once defines, ACPI_BUTTON_FILE_INFO,
>>> ACPI_BUTTON_FILE_STATE, ACPI_BUTTON_TYPE_UNKNOWN, ACPI_SWBTN_NAME
>>
>> Thank you this version is much better, I have various review remarks below.
>>
>> Please send a v3 with those fixed.
>>
>>
>>> ---
>>> MAINTAINERS | 5 +
>>> drivers/platform/x86/Kconfig | 11 ++
>>> drivers/platform/x86/Makefile | 3 +
>>> drivers/platform/x86/adv_swbutton.c | 192 ++++++++++++++++++++++++++++
>>> 4 files changed, 211 insertions(+)
>>> create mode 100644 drivers/platform/x86/adv_swbutton.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 68f21d46614c..e35c48e411b7 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -562,6 +562,11 @@ S: Maintained
>>> F: Documentation/scsi/advansys.rst
>>> F: drivers/scsi/advansys.c
>>>
>>> +ADVANTECH SWBTN DRIVER
>>> +M: Andrea Ho <Andrea.Ho@advantech.com.tw>
>>> +S: Maintained
>>> +F: drivers/platform/x86/adv_swbutton.c
>>> +
>>> ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
>>> M: Michael Hennerich <michael.hennerich@analog.com>
>>> S: Supported
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 0581a54cf562..b1340135c5e9 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -180,6 +180,17 @@ config ACER_WMI
>>> If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M
>>> here.
>>
>> This is supposed to be indented by a space (from the diff format) and then
>> a tab + 2 spaces, but in your patch this is now indented by 10 spaces.
>>
>> In general your entire patch has all tabs replaced by spaces, I guess that
>> your mail-client or editor is mangling things. Please do the following:
>>
>> 1. Check your patch for checkpatch errors:
>>
>> git format-patch HEAD~
>> scripts/checkpatch.pl 0001-platform...patch
>>
>> And fix all reported issues, both whitespace issues and others.
>>
>> 2. Send the next version of the patch like this:
>>
>> git format-patch -v3 HEAD~
>> git send-email v3-0001-platform...patch
>>
>> Do NOT edit the v3-0001-platform...patch file between these 2 steps.
>>
>>
>>> +config ADV_SWBUTTON
>>> + tristate "Advantech ACPI Software Button Driver"
>>
>> You are using indent steps of 4 spaces here, that should be a tab
>>> + depends on ACPI
>>> + help
>>> + Say Y here to enable support for Advantech software defined
>>> + button feature. More information can be fount at
>>> + <http://www.advantech.com.tw/products/>
>>
>> And a tab and 2 spaces for the help text.
>>
>>> +
>>> + To compile this driver as a module, choose M here. The module will
>>> + be called adv_swbutton.
>>> +
>>> config APPLE_GMUX
>>> tristate "Apple Gmux Driver"
>>> depends on ACPI && PCI
>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>> index 2b85852a1a87..76a321fc58ba 100644
>>> --- a/drivers/platform/x86/Makefile
>>> +++ b/drivers/platform/x86/Makefile
>>> @@ -22,6 +22,9 @@ obj-$(CONFIG_ACERHDF) += acerhdf.o
>>> obj-$(CONFIG_ACER_WIRELESS) += acer-wireless.o
>>> obj-$(CONFIG_ACER_WMI) += acer-wmi.o
>>>
>>> +# Advantech
>>> +obj-$(CONFIG_ADV_SWBUTTON) += adv_swbutton.o
>>> +
>>
>> The indent before the += should use tabs not spaces.
>>
>>> # Apple
>>> obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o
>>>
>>> diff --git a/drivers/platform/x86/adv_swbutton.c
>>> b/drivers/platform/x86/adv_swbutton.c
>>> new file mode 100644
>>> index 000000000000..f4387220e8a0
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/adv_swbutton.c
>>> @@ -0,0 +1,192 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * adv_swbutton.c - Software Button Interface Driver.
>>> + *
>>> + * (C) Copyright 2020 Advantech Corporation, Inc
>>> + *
>>> + */
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/version.h>
>>> +#include <linux/types.h>
>>> +#include <linux/proc_fs.h>
>>> +#include <linux/input.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/acpi.h>
>>> +#include <linux/platform_device.h>
>>> +#include <acpi/button.h>
>>> +#include <acpi/acpi_drivers.h>
>>
>> Please drop the following unused defines:
>>
>> #include <linux/init.h>
>> #include <linux/version.h>
>> #include <linux/types.h>
>> #include <linux/proc_fs.h>
>> #include <linux/slab.h>
>> #include <acpi/button.h>
>> #include <acpi/acpi_drivers.h>
>>
>> And sort the remaining includes alphabetically.
>>
>>> +
>>> +#define MODULE_NAME "adv_swbutton"
>>
>> Please drop this define, instead just set the .driver.name part of the platform_driver struct directly to "adv_swbutton".
>>
>>> +
>>> +#define ACPI_BUTTON_HID_SWBTN "AHC0310"
>>> +#define ACPI_BUTTON_TYPE_SOFTWARE 0x07
>>
>> Please drop the ACPI_BUTTON_TYPE_SOFTWARE define.
>>
>>> +
>>> +#define ACPI_BUTTON_NOTIFY_SWBTN_RELEASE 0x86
>>> +#define ACPI_BUTTON_NOTIFY_SWBTN_PRESSED 0x85
>>> +
>>> +#define SOFTWARE_BUTTON KEY_PROG1
>>
>> Please drop this unnecessary define and just use KEY_PROG1 directly.
>>
>>> +
>>> +#define _COMPONENT ACPI_BUTTON_COMPONENT
>>> +
>>> +struct acpi_button {
>>
>> Please give this struct-type a different name, e.g. use:
>>
>> struct adv_swbutton {
>>
>>> + unsigned int type;
>>
>> And drop the type here, it is always set to ACPI_BUTTON_TYPE_SOFTWARE, so it has no function.
>>
>>> + struct input_dev *input;
>>> + char phys[32];
>>> + bool pressed;
>>
>> Also please drop the pressed bool, see below.
>>
>>
>>> +};
>>> +
>>> +/*-------------------------------------------------------------------------
>>> + * Driver Interface
>>> + *--------------------------------------------------------------------------
>>> + */
>>> +static void acpi_button_notify(acpi_handle handle, u32 event, void *context)
>>> +{
>>> + struct acpi_device *device = context;
>>> +
>>> + struct acpi_button *button = dev_get_drvdata(&device->dev);
>>
>> You must not call dev_set_drvdata on the acpi_device since you are not binding
>> to the acpi_device, you are binding to the platform_device, so set the drv_data
>> there and please don't touch the acpi_device.
>>
>>> + struct input_dev *input;
>>> +
>>> + int keycode, BTN_KEYCODE = SOFTWARE_BUTTON;
>>
>> Please drop these 3 variables, they are not necessary.
>>
>>> +
>>> + switch (event) {
>>> + case ACPI_BUTTON_NOTIFY_SWBTN_RELEASE:
>>> + input = button->input;
>>
>> Just use button->input below instead of having this unnecessary
>> helper variable.
>>
>>> +
>>> + if (!button->pressed)
>>> + return;
>>
>> This is not necessary, the input core takes care of not sending
>> events when the state does not change.
>>
>>> +
>>> + keycode = test_bit(BTN_KEYCODE, input->keybit) ?
>>> + BTN_KEYCODE : KEY_UNKNOWN;
>>
>> No idea why you are doing this, but it is not necessary,
>> also it is wrong, you should be using KEY_PROG1 here.
>>
>>> +
>>> + button->pressed = false;
>>
>> Keeping track of the pressed state is not necessary.
>>
>>> +
>>> + input_report_key(input, keycode, 0);
>>> + input_sync(input);
>>
>> Basically this entire case can be simplified to:
>>
>> case ACPI_BUTTON_NOTIFY_SWBTN_RELEASE:
>> input_report_key(button->input, KEY_PROG1, 0);
>> input_sync(button->input);
>> break;
>>
>>
>>> + break;
>>
>> This break is indented wrong.
>>
>>> + case ACPI_BUTTON_NOTIFY_SWBTN_PRESSED:
>>> + input = button->input;
>>> + button->pressed = true;
>>> +
>>> + keycode = test_bit(BTN_KEYCODE, input->keybit) ?
>>> + BTN_KEYCODE : KEY_UNKNOWN;
>>> +
>>> + input_report_key(input, keycode, 1);
>>> + input_sync(input);
>>> + break;
>>
>> And the same as above here, this can be simplified to:
>>
>> case ACPI_BUTTON_NOTIFY_SWBTN_PRESSED:
>> input_report_key(button->input, KEY_PROG1, 1);
>> input_sync(button->input);
>> break;
>>
>>
>>> + default:
>>> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>>> + "Unsupported event [0x%x]\n", event));
>>
>> Please don't use ACPI_DEBUG_PRINTF outside of drivers/acpi, instead
>> use:
>>
>> dev_dbg(&platform_device->dev, ...);
>>
>>> + break;
>>
>> There is no need for a break at the end of a switch-case
>>
>>> + }
>>> +}
>>> +
>>> +static int acpi_button_probe(struct platform_device *device)
>>> +{
>>> + struct acpi_device *acpi_device;
>>> + struct acpi_button *button;
>>> + struct input_dev *input;
>>> + acpi_status status;
>>> +
>>> + int error;
>>> +
>>> + acpi_device = ACPI_COMPANION(&device->dev);
>>> + if (!acpi_device) {
>>> + dev_err(&device->dev, "ACPI companion is missing\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + status = acpi_install_notify_handler(acpi_device->handle,
>>> + ACPI_DEVICE_NOTIFY,
>>> + acpi_button_notify,
>>> + acpi_device);
>>> + if (ACPI_FAILURE(status)) {
>>> + dev_err(&device->dev, "Error installing notify handler\n");
>>> + error = -ENODEV;
>>> + }
>>
>> This needs to be done at the end of probe, if the notifier now runs, before
>> you have called dev_set_drvdata(), you will have a NULL pointer deref,
>> and even if the dev_set_drvdata() then the notifier may run before you have
>> set button->input = input and you still have a NULL pointer deref.
>>
>> So first setup everything and only then call acpi_install_notify_handler().
>>
>>> +
>>> + button = devm_kzalloc(&acpi_device->dev, sizeof(*button), GFP_KERNEL);
>>> + if (!button)
>>> + return -ENOMEM;
>>> +
>>> + dev_set_drvdata(&acpi_device->dev, button);
>>
>> Call this on the platform_device, so:
>>
>> dev_set_drvdata(&device->dev, button);
>>
>> Also see above.
>>
>>> +
>>> + input = devm_input_allocate_device(&acpi_device->dev);
>>> + if (!input) {
>>> + error = -ENOMEM;
>>> + goto err_free_mem;
>>
>> There is no need for the goto err_free_mem here (see below), so this
>> can be simplified to just:
>>
>> input = devm_input_allocate_device(&acpi_device->dev);
>> if (!input)
>> return -ENOMEM;
>>
>>> + }
>>> +
>>> + button->input = input;
>>> + button->type = ACPI_BUTTON_TYPE_SOFTWARE;
>>> + button->pressed = false;
>>
>> Drop the setting of type/pressed, see above.
>>
>>
>>> +
>>> + snprintf(button->phys, sizeof(button->phys),
>>> "%s/button/input0", ACPI_BUTTON_HID_SWBTN);
>>> +
>>> + input->name = "Advantech Software Button";
>>> + input->phys = button->phys;
>>> + input->id.bustype = BUS_HOST;
>>> + input->id.product = button->type;
>>
>> Don't set product to some semi-random value. Just leave it at 0.
>>
>>> + input->dev.parent = &acpi_device->dev;
>>
>> This should be:
>>
>> input->dev.parent = &device->dev;
>>
>>> +
>>> + switch (button->type) {
>>> + case ACPI_BUTTON_TYPE_SOFTWARE:
>>> + set_bit(EV_KEY, input->evbit);
>>> + set_bit(EV_REP, input->evbit);
>>> +
>>> + input_set_capability(input, EV_KEY, SOFTWARE_BUTTON);
>>> + break;
>>> + }
>>
>> button->type always equals ACPI_BUTTON_TYPE_SOFTWARE, so you can drop the whole
>> switch-case here and input_set_capability(input, EV_KEY, ...) already does:
>> set_bit(EV_KEY, input->evbit);, so this entire block can be simplified to just:
>>
>> set_bit(EV_REP, input->evbit);
>> input_set_capability(input, EV_KEY, KEY_PROG1);
>>
>>
>>> +
>>> + input_set_drvdata(input, acpi_device);
>>
>> You are never using the drvdata associated with the input_dev, so
>> this can be dropped. Also please don't use acpi_device for this/
>>
>>> + error = input_register_device(input);
>>> + if (error)
>>> + return error;
>>> +
>>> + device_init_wakeup(&acpi_device->dev, true);
>>
>> Don't use acpi_device, instead do:
>>
>> device_init_wakeup(&device->dev, true);
>>
>>
>>
>>
>>> +
>>> + return 0;
>>> +
>>> +err_free_mem:
>>> + devm_kfree(&device->dev, button);
>>
>> devm_kzalloced mem will automatically be free-ed when the probe function
>> returns with an error, or when the driver is unbound (removed) so there
>> is no need to call devm_kfree() here.
>>
>>> + return error;
>>> +}
>>> +
>>> +static int acpi_button_remove(struct platform_device *device)
>>> +{
>>> + struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev);
>>> + struct acpi_button *button = dev_get_drvdata(&acpi_device->dev);
>>> +
>>> + acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
>>> + acpi_button_notify);
>>> +
>>> + input_unregister_device(button->input);
>>> + devm_kfree(&acpi_device->dev, button);
>>
>> The input_unregister_device() and devm_kfree() will be done automatically
>> when the remove function exits, so you can drop these 2 calls.
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct acpi_device_id button_device_ids[] = {
>>> + {ACPI_BUTTON_HID_SWBTN, 0},
>>> + {"", 0},
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(acpi, button_device_ids);
>>> +
>>> +static struct platform_driver acpi_button_driver = {
>>> + .driver = {
>>> + .name = MODULE_NAME,
>>> + .acpi_match_table = button_device_ids,
>>> + },
>>> + .probe = acpi_button_probe,
>>> + .remove = acpi_button_remove,
>>> +};
>>> +
>>> +module_platform_driver(acpi_button_driver);
>>> +
>>
>> This is not an ACPI driver, please drop the ACPI_MODULE_NAME() call.
>>
>>> +ACPI_MODULE_NAME(MODULE_NAME);
>>> +
>>> +MODULE_AUTHOR("Andrea Ho");
>>> +MODULE_DESCRIPTION("Advantech ACPI SW Button Driver");
>>> +MODULE_LICENSE("GPL v2");
>>> --
>>> 2.17.1
>>>
>>
>> Regards,
>>
>> Hans
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-23 20:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 8:11 [V2,1/1] platform/x86: add support for Advantech software defined button YingChieh Ho
2021-03-18 16:14 ` Hans de Goede
2021-03-19 4:00 ` YingChieh, Ho
2021-03-23 20:03 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).