lkml.org 
[lkml]   [2020]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2020-05-12 12:09    [W:0.032 / U:2.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site