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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a1gKHir-hVoX_mFzqcOF=9NfM1NqO96kC-=6ZHf6Lojdg@mail.gmail.com>
Date:   Tue, 12 May 2020 12:08:08 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Andrea.Ho@...antech.com.tw
Cc:     "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Maximilian Luz <luzmaximilian@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        voyandrea@...il.com, amy.shih@...antech.com.tw,
        oakley.ding@...antech.com.tw, HY.Lee@...antech.com.tw
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@...antech.com.tw> wrote:
>
> From: "Andrea.Ho" <Andrea.Ho@...antech.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@...antech.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