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

[Angela Czubak <acz@semihalf.com>]

On Fri, Jan 7, 2022 at 11:18 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> 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.
>
Ack.
> > +
> >  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.
>
I have seen this kind of comment block in both hid-multitouch.c and
hid-core.c, though I can remove it.
Just to be sure: is it OK to introduce all fields/structures at once
with path no 8 ("HID: haptic: initialize haptic device")? Or would you
prefer if I extend the structures as necessary?

> > +#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?
>
I use them as indices in the effect array of effects below, though
they are actually mentioned in Sean O'Brien's kernel design proposal.
Please let me know if you would rather move them above. Perhaps it
should be even somehow exported via uapi (so that userspace does not
hardcode it separately).


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