Re: [V3,1/1] Input/misc: add support for Advantech software defined button

[Arnd Bergmann]

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