Messages in this thread | | | From | Arnd Bergmann <> | Date | Tue, 12 May 2020 12:08:08 +0200 | Subject | Re: [V3,1/1] Input/misc: add support for Advantech software defined button |
| |
On Tue, May 12, 2020 at 6:32 AM <Andrea.Ho@advantech.com.tw> 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 > (BTN_TRIGGER_HAPPY) 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>
I don't have any comments on how the driver works, just some things on coding style:
> +ACPI_MODULE_NAME("swbutton"); > + > +MODULE_AUTHOR("Andrea Ho"); > +MODULE_DESCRIPTION("Advantech ACPI SW Button Driver"); > +MODULE_LICENSE("GPL");
These generally go at the bottom of the file.
The license tag here does not match the one in the SPDX header, after you changed the other one to v2-only.
> +static const struct acpi_device_id button_device_ids[] = { > + {ACPI_BUTTON_HID_SWBTN, 0}, > + {"", 0}, > +}; > +MODULE_DEVICE_TABLE(acpi, button_device_ids); > + > +static int acpi_button_add(struct acpi_device *device); > +static int acpi_button_remove(struct acpi_device *device); > +static void acpi_button_notify(struct acpi_device *device, u32 event);
Remove the forward declarations by defining the functions in the natural order, with the driver registration last.
> +static struct acpi_driver acpi_button_driver = { > + .name = ACPI_BUTTON_DEVICE_NAME_SOFTWARE, > + .class = ACPI_BUTTON_CLASS,
Better open-code these macros here, to make it easier to grep for
> +static int __init acpi_button_init(void) > +{ > + return acpi_bus_register_driver(&acpi_button_driver); > +} > + > +static void __exit acpi_button_exit(void) > +{ > + acpi_bus_unregister_driver(&acpi_button_driver); > +}
Just use
module_acpi_driver(acpi_button_driver);
> +static int acpi_button_add(struct acpi_device *device) > +{ > + struct acpi_button *button; > + struct input_dev *input; > + const char *hid = acpi_device_hid(device); > + char *name, *class; > + int error; > + > + button = kzalloc(sizeof(struct acpi_button), GFP_KERNEL); > + if (!button) > + return -ENOMEM; > + > + device->driver_data = button; > + > + button->input = input_allocate_device(); > + input = button->input; > + if (!input) { > + error = -ENOMEM; > + goto err_free_button; > + }
You can simplify the cleanup by using devm_kzalloc() and devm_input_allocate_device().
Arnd
| |