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@xxxxxxxxxx> 於 2021年3月19日 週五 上午12:21寫道： > > Hi, > > On 3/12/21 9:11 AM, YingChieh Ho wrote: > > From: "Andrea.Ho" <Andrea.Ho@xxxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxx> > > +S: Maintained > > +F: drivers/platform/x86/adv_swbutton.c > > + > > ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346) > > M: Michael Hennerich <michael.hennerich@xxxxxxxxxx> > > 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 >
