Re: [PATCH 04/18] HID: haptic: introduce hid_haptic_device

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Dec 21, 2021 at 07:17:29PM +0000, Angela Czubak wrote:
> Define a new structure that contains simple haptic device configuration
> as well as current state.
> 
> Signed-off-by: Angela Czubak <acz@semihalf.com>
> ---
>  drivers/hid/Kconfig      |  4 +++
>  drivers/hid/Makefile     |  1 +
>  drivers/hid/hid-haptic.c | 10 ++++++
>  drivers/hid/hid-haptic.h | 68 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 83 insertions(+)
>  create mode 100644 drivers/hid/hid-haptic.c
>  create mode 100644 drivers/hid/hid-haptic.h
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index a7c78ac96270..8d1eb4491a7f 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -89,6 +89,10 @@ config HID_GENERIC
>  
>  	If unsure, say Y.
>  
> +config HID_HAPTIC
> +	bool
> +	default n

'n' is the default, no need to have it explicit.

> +
>  menu "Special HID drivers"
>  	depends on HID
>  
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 55a6fa3eca5a..65d54ccd4574 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -4,6 +4,7 @@
>  #
>  hid-y			:= hid-core.o hid-input.o hid-quirks.o
>  hid-$(CONFIG_DEBUG_FS)		+= hid-debug.o
> +hid-$(CONFIG_HID_HAPTIC)	+= hid-haptic.o
>  
>  obj-$(CONFIG_HID)		+= hid.o
>  obj-$(CONFIG_UHID)		+= uhid.o
> diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> new file mode 100644
> index 000000000000..0910d8af9f38
> --- /dev/null
> +++ b/drivers/hid/hid-haptic.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  HID Haptic support for Linux
> + *
> + *  Copyright (c) 2021 Angela Czubak
> + */
> +
> +/*
> + */

What is this comment block for? Actually I do not see why this needs to
be a separate patch.

> +#include "hid-haptic.h"
> diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
> new file mode 100644
> index 000000000000..41f19cd22f75
> --- /dev/null
> +++ b/drivers/hid/hid-haptic.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + *  HID Haptic support for Linux
> + *
> + *  Copyright (c) 2021 Angela Czubak
> + */
> +
> +/*
> + */
> +
> +
> +#include <linux/hid.h>
> +
> +#define HID_HAPTIC_ORDINAL_WAVEFORMNONE 1
> +#define HID_HAPTIC_ORDINAL_WAVEFORMSTOP 2
> +
> +#define HID_HAPTIC_PRESS_THRESH 200
> +#define HID_HAPTIC_RELEASE_THRESH 180
> +
> +#define HID_HAPTIC_MODE_DEVICE 0
> +#define HID_HAPTIC_MODE_KERNEL 1
> +
> +struct hid_haptic_effect {
> +	__u8 *report_buf;

This is a matter of preference, but in kernel we normally use u8, s16,
etc, and underscored versions are for headers that are part of uapi.

> +	struct input_dev *input_dev;
> +	struct work_struct work;
> +	struct list_head control;
> +	struct mutex control_mutex;
> +};
> +
> +struct hid_haptic_effect_node {
> +	struct list_head node;
> +	struct file *file;
> +};
> +
> +struct hid_haptic_device {
> +	struct input_dev *input_dev;
> +	struct hid_device *hdev;
> +	struct hid_report *auto_trigger_report;
> +	struct mutex auto_trigger_mutex;
> +	struct workqueue_struct *wq;
> +	struct hid_report *manual_trigger_report;
> +	struct mutex manual_trigger_mutex;
> +	size_t manual_trigger_report_len;
> +	int pressed_state;
> +	__s32 pressure_sum;
> +	__s32 force_logical_minimum;
> +	__s32 force_physical_minimum;
> +	__s32 force_resolution;
> +	__u32 press_threshold;
> +	__u32 release_threshold;
> +	__u32 mode;
> +	__u32 default_auto_trigger;
> +	__u32 vendor_page;
> +	__u32 vendor_id;
> +	__u32 max_waveform_id;
> +	__u32 max_duration_id;
> +	__u16 *hid_usage_map;
> +	__u32 *duration_map;
> +	__u16 press_ordinal_orig;
> +	__u16 press_ordinal_cur;
> +	__u16 release_ordinal_orig;
> +	__u16 release_ordinal_cur;
> +#define HID_HAPTIC_RELEASE_EFFECT_ID 0
> +#define HID_HAPTIC_PRESS_EFFECT_ID 1

Why these definitions are here?

> +	struct hid_haptic_effect *effect;
> +	struct hid_haptic_effect stop_effect;
> +};
> -- 
> 2.34.1.307.g9b7440fafd-goog
> 

Thanks.

-- 
Dmitry