On Tue, May 12, 2020 at 6:32 AM <Andrea.Ho@xxxxxxxxxxxxxxxx> 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
> (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@xxxxxxxxxxxxxxxx>
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
