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

[Joe Perches]

On Thu, 2020-02-27 at 03:15 +0000, ycho1399@xxxxxxxxx wrote:
> From: "Andrea.Ho" <Andrea.Ho@xxxxxxxxxxxxxxxx>
> 
> 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?