linux-kernel - Re: [V1,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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Feb 2020 00:11:02 -0800
From:   Joe Perches <joe@...ches.com>
To:     ycho1399@...il.com, linux-input@...r.kernel.org
Cc:     voyandrea@...il.com, andrea.ho@...antech.com.tw,
        oakley.ding@...antech.com.tw,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Luca Weiss <luca@...tu.xyz>,
        Maximilian Luz <luzmaximilian@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Pavel Machek <pavel@....cz>, linux-kernel@...r.kernel.org
Subject: Re: [V1,1/1] Input/misc: add support for Advantech software defined
 button

On Thu, 2020-02-27 at 03:15 +0000, ycho1399@...il.com 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_EVENTs on the
> Advantech Tabletop Network Appliances products and it has been
> tested in FWA1112VC.
> 
> Add the software define button support to report KEY_EVENTs by
> different acts of pressing button (like double-click, long pressed
> and tick) that cloud be get on user interface and trigger the
> customized actions.
[]
> diff --git a/drivers/input/misc/adv_swbutton.c b/drivers/input/misc/adv_swbutton.c
> new file mode 100644

mostly trivia:

> +/*
> + * Switch two elements in array.
> + *
> + * @param xp, yp The array elements need to swap.
> + */
> +void array_swap(unsigned int *xp, unsigned int *yp)
> +{
> +	int temp = *xp;
> +	*xp = *yp;
> +	*yp = temp;
> +}

kernel.h has swap

> +/*
> + * Sorting an array in ascending order
> + *
> + * @param arr The array for sorting.
> + * @param n The array size
> + */
> +void sort_asc(unsigned int arr[], int n)
> +{
> +	int i, j, min_idx;
> +
> +	for (i = 0; i < n - 1; i++) {
> +		min_idx = i;
> +		for (j = i + 1; j < n; j++)
> +			if (arr[j] < arr[min_idx])
> +				min_idx = j;
> +
> +		array_swap(&arr[min_idx], &arr[i]);
> +	}
> +}

sort.h has a generic sort too

> +
> +/*
> + * initial software button timer to check tick or double click
> + *
> + * @param btn Struct of acpi_button that should be required.
> + */ 
> +static void swbtn_init_timer(struct acpi_button *btn)
> +{
> +	pr_info(PREFIX "swbtn timer start\n");

Many of these printks should be removed and ftrace used
when necessary.

> +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, i;
> +
> +	pr_info(PREFIX "%s\n",  __func__);
> +	button = kzalloc(sizeof(*button), GFP_KERNEL);
> +	if (!button) {
> +		pr_err(PREFIX "alloc acpi_button failed\n");

alloc failure messages aren't really necessary
as a dump_stack() is already done on failure.

[]

> +		for (i = (!swbtn_cfg.dclick_enabled);
> +		     i < (swbtn_cfg.lkey_number + 2); i++) {
> +			pr_info(PREFIX "%d. Enabled keycode[0x%x]\n",
> +				i, swbtn_keycodes[i]);

Is it really useful to print all enabled keycodes?