* Re: Clarification about the hidraw documentation on HIDIOCGFEATURE
From: Xiaofan Chen @ 2023-06-19 23:28 UTC (permalink / raw)
To: linux-input; +Cc: Ihor Dutchak
In-Reply-To: <CAGjSPUCBPSXTHji-aSs64QHNYjBms9-WhohBYuc9Tiom5KaSow@mail.gmail.com>
On Mon, Jun 19, 2023 at 11:14 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
>
> On Mon, Jun 19, 2023 at 2:09 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> >
> > I know that thurrent documentation has been there since it was created by
> > Alan Ott many years ago. And he started the HIDAPI project around that
> > time as well. However, I am starting to doubt whether it is correct or not
> > based on the testing using HIDAPI.
> >
> > Please help to clarify. Thanks.
> >
> > https://docs.kernel.org/hid/hidraw.html
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > HIDIOCGFEATURE(len):
> >
> > Get a Feature Report
> >
> > This ioctl will request a feature report from the device using the
> > control endpoint. The first byte of the supplied buffer should be
> > set to the report number of the requested report. For devices
> > which do not use numbered reports, set the first byte to 0. The
> > returned report buffer will contain the report number in the first
> > byte, followed by the report data read from the device. For devices
> > which do not use numbered reports, the report data will begin at the
> > first byte of the returned buffer.
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >
> > I have doubts about the last sentence. It seems to me that for
> > devices which do not use numbered reports, the first byte will
> > be 0 and the report data begins in the second byte.
> >
> > This is based on testing results using hidapi and hidapitester, which
> > use the ioctl.
> > int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned
> > char *data, size_t length)
> > {
> > int res;
> >
> > register_device_error(dev, NULL);
> >
> > res = ioctl(dev->device_handle, HIDIOCGFEATURE(length), data);
> > if (res < 0)
> > register_device_error_format(dev, "ioctl (GFEATURE): %s",
> > strerror(errno));
> >
> > return res;
> > }
> >
> > Reference discussion:
> > https://github.com/libusb/hidapi/issues/589
> >
> > Test device is Jan Axelson's generic HID example which I have tested using
> > libusb and hidapi across platforms as well as using Windows HID API.
> > So I believe the FW is good.
> > http://janaxelson.com/hidpage.htm#MyExampleCode (USB PIC MCU)
> >
>
> Modified testing code from the following Linux kernel
> samples/hidraw/hid-example.c
> https://github.com/libusb/hidapi/issues/589#issuecomment-1597356054
>
> Results are shown here. We can clearly see that the "Fake Report ID" 0 is
> in the first byte of the returned buffer, matching the output from
> hidapi/hidapitester,
>
> mcuee@UbuntuSwift3:~/build/hid/hidraw$ gcc -o myhid-example myhid-example.c
>
> mcuee@UbuntuSwift3:~/build/hid/hidraw$ sudo ./myhid-example
> Report Descriptor Size: 47
> Report Descriptor:
> 6 a0 ff 9 1 a1 1 9 3 15 0 26 ff 0 75 8 95 3f 81 2 9 4 15 0 26 ff 0 75
> 8 95 3f 91 2 9 5 15 0 26 ff 0 75 8 95 3f b1 2 c0
>
> Raw Name: Microchip Technology Inc. Generic HID
> Raw Phys: usb-0000:00:14.0-1/input0
> Raw Info:
> bustype: 3 (USB)
> vendor: 0x0925
> product: 0x7001
> ioctl HIDIOCSFEATURE returned: 64
> ioctl HIDIOCGFEATURE returned: 64
> Report data:
> 0 41 42 43 44 45 46 47 48 14 4 18 4 4d 72 31 50 51 52 53 54 55 56 57
> 58 59 5a 5b 5c 5d 5e 5f 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e
> 6f 70 71 72 73 74 75 76 77 78 79 7a 7b 7c 7d 7e 7f
>
> write() wrote 64 bytes
> read() read 63 bytes:
> 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37
> 38 39 3a 3b 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e
> 4f 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f
>
Moreover the kernel codes here seem to prove that the documentation
is wrong for both HIDIOCGFEATURE and HIDIOCGINPUT.
https://github.com/torvalds/linux/blob/master/include/uapi/linux/hidraw.h
/* The first byte of SFEATURE and GFEATURE is the report number */
#define HIDIOCSFEATURE(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
#define HIDIOCGFEATURE(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)
#define HIDIOCGRAWUNIQ(len) _IOC(_IOC_READ, 'H', 0x08, len)
/* The first byte of SINPUT and GINPUT is the report number */
#define HIDIOCSINPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x09, len)
#define HIDIOCGINPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0A, len)
/* The first byte of SOUTPUT and GOUTPUT is the report number */
#define HIDIOCSOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0B, len)
#define HIDIOCGOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
--
Xiaofan
^ permalink raw reply
* Re: [PATCH] HID: Reorder fields in 'struct hid_field'
From: Dan Carpenter @ 2023-06-20 5:09 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Jiri Kosina, Benjamin Tissoires, linux-kernel, kernel-janitors,
linux-input
In-Reply-To: <ba1a74ba-e91a-bf12-2d4e-7157e4519478@wanadoo.fr>
On Mon, Jun 19, 2023 at 08:20:08PM +0200, Christophe JAILLET wrote:
> Le 19/06/2023 à 07:18, Dan Carpenter a écrit :
> > On Sun, Jun 18, 2023 at 10:08:07AM +0200, Christophe JAILLET wrote:
> > > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > > index 39e21e3815ad..5be5e671c263 100644
> > > --- a/include/linux/hid.h
> > > +++ b/include/linux/hid.h
> > > @@ -480,9 +480,9 @@ struct hid_field {
> > > __s32 physical_maximum;
> > > __s32 unit_exponent;
> > > unsigned unit;
> > > - bool ignored; /* this field is ignored in this event */
> > > struct hid_report *report; /* associated report */
> > > unsigned index; /* index into report->field[] */
> > > + bool ignored; /* this field is ignored in this event */
> > > /* hidinput data */
> > > struct hid_input *hidinput; /* associated input structure */
> > > __u16 dpad; /* dpad input code */
> >
> > You could move the dpad next to the ignored to save another 4 bytes.
> > I think it is still grouped logically that way but I don't really know
> > what dpad is so I might be wrong.
> >
> > struct hid_report *report; /* associated report */
> > unsigned index; /* index into report->field[] */
> > bool ignored; /* this field is ignored in this event */
> > /* hidinput data */
> > __u16 dpad; /* dpad input code */
> > struct hid_input *hidinput; /* associated input structure */
> >
> > regards,
> > dan carpenter
> >
> >
>
> In fact, not really,
> It would fill a hole better, but would generate some padding at the end of
> the struct:
>
> bool ignored; /* 108 1 */
>
> /* XXX 1 byte hole, try to pack */
>
> __u16 dpad; /* 110 2 */
> struct hid_input * hidinput; /* 112 8 */
> unsigned int slot_idx; /* 120 4 */
>
> /* size: 128, cachelines: 2, members: 25 */
> /* sum members: 119, holes: 2, sum holes: 5 */
> /* padding: 4 */
> };
Ah. Right. I should have looked at that more closely.
regards,
dan carpenter
^ permalink raw reply
* [PATCH] HID: logitech-hidpp: Rename HID++ "internal" error constant
From: Bastien Nocera @ 2023-06-20 8:50 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
As per the upstream "hidpp" helpers commit:
"
There has been some confusion about error value 5 but feature specs that
refer to it generally use NOT_ALLOWED.
"
Signed-off-by: Bastien Nocera <hadess@hadess.net>
Link: https://github.com/mrubli2/hidpp/commit/80c3fecfcd89c5efe0f16feabe90a55ddfd25aaa
---
drivers/hid/hid-logitech-hidpp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5e1a412fd28f..2dd86c23412b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -228,7 +228,7 @@ struct hidpp_device {
#define HIDPP20_ERROR_INVALID_ARGS 0x02
#define HIDPP20_ERROR_OUT_OF_RANGE 0x03
#define HIDPP20_ERROR_HW_ERROR 0x04
-#define HIDPP20_ERROR_LOGITECH_INTERNAL 0x05
+#define HIDPP20_ERROR_NOT_ALLOWED 0x05
#define HIDPP20_ERROR_INVALID_FEATURE_INDEX 0x06
#define HIDPP20_ERROR_INVALID_FUNCTION_ID 0x07
#define HIDPP20_ERROR_BUSY 0x08
--
2.40.1
^ permalink raw reply related
* [PATCH 0/1] HID: Add introduction about HID for non-kernel programmers
From: Marco Morandini @ 2023-06-20 12:32 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input, Jonathan Corbet,
linux-doc
This doc addition is meant to allow a random user
to understand what is going wrong with his
brand-new device, and possibly try to fix it on his own.
I've written it, being one of those random users,
in an effort to document what I think I've learned,
and with the hope to minimize the pain to guys like me.
The chapter is by no means complete, and for sure will require
careful checking and fixing from the HID maintainers.
In my opinion, it would also be great if they could add a brief
explanation for the different quirks (I've left a FIXME there).
I hope I've not misunderstood too many concepts (?events?) and
that the whole thing is not so screwed up that it's better to
throw it into the thrash can and run away without looking back.
Marco
^ permalink raw reply
* [PATCH 1/1] HID: Add introduction about HID for non-kernel programmers
From: Marco Morandini @ 2023-06-20 12:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input, Jonathan Corbet,
linux-doc
In-Reply-To: <d6d16821-2592-8210-475a-5388d7a79e82@polimi.it>
This patch adds an introduction about HID
meant for the casual programmers that is trying
either to fix his device or to understand what
is going wrong
---
Documentation/hid/hidintro.rst | 558 +++++++++++++++++++++++++++++++++
Documentation/hid/index.rst | 1 +
2 files changed, 559 insertions(+)
create mode 100644 Documentation/hid/hidintro.rst
diff --git a/Documentation/hid/hidintro.rst b/Documentation/hid/hidintro.rst
new file mode 100644
index 000000000000..96afb8d807a6
--- /dev/null
+++ b/Documentation/hid/hidintro.rst
@@ -0,0 +1,558 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+======================================
+Introduction to HID report descriptors
+======================================
+
+This chapter is meant to give a broad overview
+of what HID report descriptors are, and of how a casual (non kernel)
+programmer can deal with an HID device that
+is not working well with Linux.
+
+.. contents::
+ :local:
+ :depth: 2
+
+
+Introduction
+============
+
+HID stands for Human Interface Device, and can be whatever device
+you are using to interact with a computer, be it a mouse,
+a touchpad, a tablet, a microphone.
+
+Many HID work out the box, even if their hardware is different.
+For example, mouses can have a different number of buttons; they
+can have (or not) a wheel; their movement sensitivity can be
+significantly different, and so on. Nonetheless,
+most of the time everything just works, without the need
+to have specialized code in the kernel for any mouse model
+developed since 1970.
+
+This is because most (if not all) of the modern HIDs do advertise
+the number and type of signal that can be exchanged, and
+describe how such signal are exchanged with the computer
+by means of *HID events*.
+This is done through the *HID report descriptor*, basically a bunch of numbers
+that allow the operating system to understand that the mouse at hand
+has (say) five rather than three buttons.
+
+The HID subsystem is in charge of parsing the HID report descriptors,
+and of converts HID events into normal input
+device interfaces (see Documentation/hid/hiddev.rst).
+Whenever something does not work as it should this can be
+because the HID report descriptor provided by the device is wrong,
+or because it needs to be dealt with in a special way,
+or because the some special device or interaction mode
+is not handled by the default code.
+
+The format of HID report descriptors is described by two documents,
+available from the `USB Implementers Forum <https://www.usb.org/>`_
+at `this <https://www.usb.org/hid>`_ addresses:
+
+ * the `HID USB Device Class Definition <https://www.usb.org/document-library/device-class-definition-hid-111>`_ (HIDUDC from now on)
+ * the `HID Usage Tables <https://usb.org/document-library/hid-usage-tables-14>`_ (HIDUT from now on)
+
+This does not means that the HID subsystem can deal with USB devices only;
+rather, different transport drivers, such as I2C or Bluetooth, can be dealt
+with, see Documentation/hid/hid-transport.rst.
+
+Parsing an HID descriptor
+=========================
+
+The current list of HID devices can be found at ``/sys/bus/hid/devices/``.
+For each device, say ``/sys/bus/hid/devices/0003\:093A\:2510.0002/``,
+one can read the corresponding report descriptor::
+
+ marco@sun:~> hexdump -C /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
+ 00000000 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 |..............).|
+ 00000010 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 |..%.u.....u.....|
+ 00000020 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 |...0.1.8..%.u...|
+ 00000030 81 06 c0 c0 |....|
+ 00000034
+
+Alternatively, the HID report descriptor can be read by accessig the hidraw
+driver, see Documentation/output/hid/hidraw.rst and
+file samples/hidraw/hid-example.c for a simple example.
+The output of ``hid-example`` would be, for the same device::
+
+ marco@sun:~> sudo ./hid-example
+ Report Descriptor Size: 52
+ Report Descriptor:
+ 5 1 9 2 a1 1 9 1 a1 0 5 9 19 1 29 3 15 0 25 1 75 1 95 3 81 2 75 5 95 1 81 1 5 1 9 30 9 31 9 38 15 81 25 7f 75 8 95 3 81 6 c0 c0
+
+ Raw Name: PixArt USB Optical Mouse
+ Raw Phys: usb-0000:05:00.4-2.3/input0
+ Raw Info:
+ bustype: 3 (USB)
+ vendor: 0x093a
+ product: 0x2510
+ HIDIOCSFEATURE: Broken pipe
+ HIDIOCGFEATURE: Broken pipe
+ Error: 32
+ write: Broken pipe
+ read: Resource temporarily unavailable
+
+The basic structure of a HID report descriptor is defined in the HIDUDC manual, while
+HIDUT "defines constants that can be interpreted by an application to
+identify the purpose and meaning of a data
+field in a HID report". Each entry is defined by at least two bytes,
+where the first one defines what type of value is following,
+and is described in the HIDUDC manual,
+and the second one carries the actual value,
+and is described in the HIDUT manual.
+
+Let consider the first number, 0x05: according to
+HIDUDC, Sec. 6.2.2.2, "Short Items"
+
+ * the first least significant two bits
+ define the number of the following data bytes (either 0, 1, 2 or 4
+ for the values 0, 1, 2 or 3, respectively)
+ * the second least significant two bits identify the type of the item:
+
+ * 0: ``Main``
+ * 1: ``Global``
+ * 2: ``Local``
+ * 3: ``Reserved``
+ * the remaining four bits give a numeric expression specifying
+ the function of the item (see below);
+
+This means that ``0x05`` (i.e. ``00000101``) stands for
+1 byte of data to be read, and Global type.
+Since we are dealing with a Global item the meaning
+of the most significant four bits
+is defines in HIDUC manual, Sec. 6.2.2.7, "Global Items".
+The bits are ``0000``, thus we have a ``Usage Page``.
+
+
+The second number is the actual data, and its meaning can
+be found in the HICUT manual.
+We have an ``Usage Page``, thus we need to refer to HICUT Sec. 3,
+"Usage Pages"; from there, it is clear that the ``0x01``
+stands for a ``Generic Desktop Page``.
+
+Moving now to the second two bytes, and following the same scheme, ``0x09``
+(i.e. ``00001001``) will be followed by one byte (``01``)
+and is a ``Local`` item.
+Thus, the meaning of the remaining four bits (``0000``)
+is given in HIDUDC Sec. 6.2.2.8 "Local Items", so that we have an ``Usage``.
+
+In this way the HID report descriptor can be painstakingly
+parsed, byte by byte. In practice you need not to do this,
+and almost no one does
+this: everyone resorts to specialized parsers. Among all the available ones
+
+ * the online `USB Descriptor and Request Parser
+ <http://eleccelerator.com/usbdescreqparser/>`_;
+ * `hidrdd <https://github.com/abend0c1/hidrdd>`_,
+ that provides very detailed and somewhat verbose descriptions
+ (verbosity can be useful if you are not familiar with HID report descriptors);
+ * `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_,
+ a complete utility set that allows, among other things,
+ to record and replay the raw HID reports and to debug
+ and replay HID devices.
+ It is being actively developed by the Linux HID subsystem mantainers.
+
+.. Parsing the mouse HID report descriptor with `hidrdd <https://github.com/abend0c1/hidrdd>`_ one gets::
+
+ marco@sun:~> rexx ./rd.rex -x -d --hex 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
+
+ //--------------------------------------------------------------------------------
+ // Report descriptor data in hex (length 52 bytes)
+ //--------------------------------------------------------------------------------
+
+
+ // 05010902 A1010901 A1000509 19012903 15002501 75019503 81027505 95018101
+ // 05010930 09310938 1581257F 75089503 8106C0C0
+
+
+ //--------------------------------------------------------------------------------
+ // Decoded Application Collection
+ //--------------------------------------------------------------------------------
+
+ /*
+ 05 01 (GLOBAL) USAGE_PAGE 0x0001 Generic Desktop Page
+ 09 02 (LOCAL) USAGE 0x00010002 Mouse (Application Collection)
+ A1 01 (MAIN) COLLECTION 0x01 Application (Usage=0x00010002: Page=Generic Desktop Page, Usage=Mouse, Type=Application Collection)
+ 09 01 (LOCAL) USAGE 0x00010001 Pointer (Physical Collection)
+ A1 00 (MAIN) COLLECTION 0x00 Physical (Usage=0x00010001: Page=Generic Desktop Page, Usage=Pointer, Type=Physical Collection)
+ 05 09 (GLOBAL) USAGE_PAGE 0x0009 Button Page
+ 19 01 (LOCAL) USAGE_MINIMUM 0x00090001 Button 1 Primary/trigger (Selector, On/Off Control, Momentary Control, or One Shot Control)
+ 29 03 (LOCAL) USAGE_MAXIMUM 0x00090003 Button 3 Tertiary (Selector, On/Off Control, Momentary Control, or One Shot Control)
+ 15 00 (GLOBAL) LOGICAL_MINIMUM 0x00 (0) <-- Info: Consider replacing 15 00 with 14
+ 25 01 (GLOBAL) LOGICAL_MAXIMUM 0x01 (1)
+ 75 01 (GLOBAL) REPORT_SIZE 0x01 (1) Number of bits per field
+ 95 03 (GLOBAL) REPORT_COUNT 0x03 (3) Number of fields
+ 81 02 (MAIN) INPUT 0x00000002 (3 fields x 1 bit) 0=Data 1=Variable 0=Absolute 0=NoWrap 0=Linear 0=PrefState 0=NoNull 0=NonVolatile 0=Bitmap
+ 75 05 (GLOBAL) REPORT_SIZE 0x05 (5) Number of bits per field
+ 95 01 (GLOBAL) REPORT_COUNT 0x01 (1) Number of fields
+ 81 01 (MAIN) INPUT 0x00000001 (1 field x 5 bits) 1=Constant 0=Array 0=Absolute
+ 05 01 (GLOBAL) USAGE_PAGE 0x0001 Generic Desktop Page
+ 09 30 (LOCAL) USAGE 0x00010030 X (Dynamic Value)
+ 09 31 (LOCAL) USAGE 0x00010031 Y (Dynamic Value)
+ 09 38 (LOCAL) USAGE 0x00010038 Wheel (Dynamic Value)
+ 15 81 (GLOBAL) LOGICAL_MINIMUM 0x81 (-127)
+ 25 7F (GLOBAL) LOGICAL_MAXIMUM 0x7F (127)
+ 75 08 (GLOBAL) REPORT_SIZE 0x08 (8) Number of bits per field
+ 95 03 (GLOBAL) REPORT_COUNT 0x03 (3) Number of fields
+ 81 06 (MAIN) INPUT 0x00000006 (3 fields x 8 bits) 0=Data 1=Variable 1=Relative 0=NoWrap 0=Linear 0=PrefState 0=NoNull 0=NonVolatile 0=Bitmap
+ C0 (MAIN) END_COLLECTION Physical
+ C0 (MAIN) END_COLLECTION Application
+ */
+
+ // All structure fields should be byte-aligned...
+ #pragma pack(push,1)
+
+ //--------------------------------------------------------------------------------
+ // Button Page inputReport (Device --> Host)
+ //--------------------------------------------------------------------------------
+
+ typedef struct
+ {
+ // No REPORT ID byte
+ // Collection: CA:Mouse CP:Pointer
+ uint8_t BTN_MousePointerButton1 : 1; // Usage 0x00090001: Button 1 Primary/trigger, Value = 0 to 1
+ uint8_t BTN_MousePointerButton2 : 1; // Usage 0x00090002: Button 2 Secondary, Value = 0 to 1
+ uint8_t BTN_MousePointerButton3 : 1; // Usage 0x00090003: Button 3 Tertiary, Value = 0 to 1
+ uint8_t : 5; // Pad
+ int8_t GD_MousePointerX; // Usage 0x00010030: X, Value = -127 to 127
+ int8_t GD_MousePointerY; // Usage 0x00010031: Y, Value = -127 to 127
+ int8_t GD_MousePointerWheel; // Usage 0x00010038: Wheel, Value = -127 to 127
+ } inputReport_t;
+
+ #pragma pack(pop)
+
+Parsing the mouse HID report descriptor with `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_ leads to::
+
+ marco@sun:~/Programmi/linux/hid-tools (master =)> ./hid-decode /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
+ # device 0:0
+ # 0x05, 0x01, // Usage Page (Generic Desktop) 0
+ # 0x09, 0x02, // Usage (Mouse) 2
+ # 0xa1, 0x01, // Collection (Application) 4
+ # 0x09, 0x01, // Usage (Pointer) 6
+ # 0xa1, 0x00, // Collection (Physical) 8
+ # 0x05, 0x09, // Usage Page (Button) 10
+ # 0x19, 0x01, // Usage Minimum (1) 12
+ # 0x29, 0x03, // Usage Maximum (3) 14
+ # 0x15, 0x00, // Logical Minimum (0) 16
+ # 0x25, 0x01, // Logical Maximum (1) 18
+ # 0x75, 0x01, // Report Size (1) 20
+ # 0x95, 0x03, // Report Count (3) 22
+ # 0x81, 0x02, // Input (Data,Var,Abs) 24
+ # 0x75, 0x05, // Report Size (5) 26
+ # 0x95, 0x01, // Report Count (1) 28
+ # 0x81, 0x01, // Input (Cnst,Arr,Abs) 30
+ # 0x05, 0x01, // Usage Page (Generic Desktop) 32
+ # 0x09, 0x30, // Usage (X) 34
+ # 0x09, 0x31, // Usage (Y) 36
+ # 0x09, 0x38, // Usage (Wheel) 38
+ # 0x15, 0x81, // Logical Minimum (-127) 40
+ # 0x25, 0x7f, // Logical Maximum (127) 42
+ # 0x75, 0x08, // Report Size (8) 44
+ # 0x95, 0x03, // Report Count (3) 46
+ # 0x81, 0x06, // Input (Data,Var,Rel) 48
+ # 0xc0, // End Collection 50
+ # 0xc0, // End Collection 51
+ #
+ R: 52 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
+ N: device 0:0
+ I: 3 0001 0001
+
+From it we undesratnd that
+
+ * the mouse has three (from ``Usage Minimum (1)`` to
+ ``Usage Maximum (3)``) buttons (``Usage Page (Button)``);
+ * buttons can take values ranging from ``0`` to ``1``;
+ (from ``Logical Minimum (0)`` to ``Logical Maximum (1)``);
+ * information is encoded into three bits: one bit has
+ ``Report Size (1)``,
+ but there are three of them since ``Report Count (3)``;
+ * the value of these bits can change
+ (``Data`` in ``Input (Data,Var,Abs)``);
+ * each field represents data from a physical control;
+ * the number of bits reserved for each field is determined
+ by the preceding ``Report Size``/``Report Count``
+ items (``Var`` in ``Input (Data,Var,Abs)``);
+ * the data is *absolute* (i.e it does not represent the
+ change from the last report, ``Abs`` in ``Input (Data,Var,Abs)``).
+
+The meaning of the ``Input``
+items is explained in HIDUDC Sec. 6.2.2.5 "Input, Output, and Feature Items.
+
+There are five additional padding bits, that are needed
+to reach a byte: see ``Report Size (5)``, that is
+repeated only once (``Report Count (1)``).
+These bits take *constant* values (``Cnst`` in
+``Input (Cnst,Arr,Abs)``).
+
+The mouse has also two physical positions (``Usage (X)``, ``Usage (Y)``) and a wheel
+(``Usage (Wheel)``).
+
+Each of them take values ranging from ``-127`` to ``127``
+(from ``Logical Minimum (-127)`` to ``Logical Maximum (-127)``),
+it is represented by eight bits (``Report Size (8)``)
+and there are three of these set of bits (``Report Count (3)``).
+
+This time the data do represent the change from the previous configuration
+(``Rel`` in ``Input (Data,Var,Rel)``).
+
+All in all, the mouse input will be transmitted using four bytes:
+the first one for the buttons (three bits used, five for padding),
+the last three for the mouse X, Y and wheel changes, respectively.
+
+Indeed, for any event, the mouse will send a *report* of four bytes.
+We can easily check the values sent by resorting e.g.
+to the `hid-recorder` tool, from `hid-tools
+<https://gitlab.freedesktop.org/libevdev/hid-tools>`_:
+The sequence of bytes sent by clicking and releasing
+button 1, then button 2, then button 3 is::
+
+ marco@sun:~/> sudo ./hid-recorder /dev/hidraw1
+
+ ....
+ output of hid-decode
+ ....
+
+ # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000000.000000 4 01 00 00 00
+ # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000000.183949 4 00 00 00 00
+ # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000001.959698 4 02 00 00 00
+ # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000002.103899 4 00 00 00 00
+ # Button: 0 0 1 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000004.855799 4 04 00 00 00
+ # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000005.103864 4 00 00 00 00
+
+where it's clear that, for example, when button 2 is clicked
+the bytes ``02 00 00 00`` are sent, and the immediately subsequent
+event (``00 00 00 00``) is the release of button 2 (no buttons are pressed,
+remember that the data is *absolute*).
+
+If instead one clicks and holds button 1, then clicks and holds button 2,
+releases button 1, and finally releases button 2, the reports are::
+
+ # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000044.175830 4 01 00 00 00
+ # Button: 1 1 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000045.975997 4 03 00 00 00
+ # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000047.407930 4 02 00 00 00
+ # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000049.199919 4 00 00 00 00
+
+where with ``03 00 00 00`` both buttons are pressed, and with the
+subsequent ``02 00 00 00`` button 1 is released while button 2 is still
+active.
+
+Outputs and Inputs
+------------------
+
+An HID devices can have inputs, like
+in the mouse example, and outputs.
+"Output" means that the information is fed
+from the device to the human; for examples,
+a joystick with force feedback will have
+some output.
+
+
+Report IDs and Evdev events
+===========================
+
+A single device can logically group
+data into different, independent sets.
+It is *as if* the HID presents
+itself as different devices, each exchanging
+its own data. The HID report descriptor is unique,
+but the different reports are identified by means
+of different ``Report ID`` fields. Whenever a ``Report ID``
+is needed it is transmitted as the first byte of any report.
+
+Consider the following HID report descriptor::
+
+ 05 01 09 02 A1 01 85 01 05 09 19 01 29 05 15 00
+ 25 01 95 05 75 01 81 02 95 01 75 03 81 01 05 01
+ 09 30 09 31 16 00 F8 26 FF 07 75 0C 95 02 81 06
+ 09 38 15 80 25 7F 75 08 95 01 81 06 05 0C 0A 38
+ 02 15 80 25 7F 75 08 95 01 81 06 C0 05 01 09 02
+ A1 01 85 02 05 09 19 01 29 05 15 00 25 01 95 05
+ 75 01 81 02 95 01 75 03 81 01 05 01 09 30 09 31
+ 16 00 F8 26 FF 07 75 0C 95 02 81 06 09 38 15 80
+ 25 7F 75 08 95 01 81 06 05 0C 0A 38 02 15 80 25
+ 7F 75 08 95 01 81 06 C0 05 01 09 07 A1 01 85 05
+ 05 07 15 00 25 01 09 29 09 3E 09 4B 09 4E 09 E3
+ 09 E8 09 E8 09 E8 75 01 95 08 81 02 95 00 81 01
+ C0 05 0C 09 01 A1 01 85 06 15 00 25 01 75 01 95
+ 01 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
+ 06 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
+ 06 C0 05 0C 09 01 A1 01 85 03 09 05 15 00 26 FF
+ 00 75 08 95 02 B1 02 C0
+
+After parsing it (try to parse it on your own using
+the suggested tools!)
+one can see that the device presents two mouses
+(Reports IDs 1 and 2, respectively),
+a Keypad (Report ID 5) and two consumer controls
+(Report IDs 6 and 3).
+The data sent for each of these report ids
+will begin with the Report ID byte, and will be followed
+by the corresponding information. For example, the
+report defined for the last consumer
+control::
+
+ 0x05, 0x0C, // Usage Page (Consumer)
+ 0x09, 0x01, // Usage (Consumer Control)
+ 0xA1, 0x01, // Collection (Application)
+ 0x85, 0x03, // Report ID (3)
+ 0x09, 0x05, // Usage (Headphone)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x26, 0xFF, 0x00, // Logical Maximum (255)
+ 0x75, 0x08, // Report Size (8)
+ 0x95, 0x02, // Report Count (2)
+ 0xB1, 0x02, // Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
+ 0xC0, // End Collection
+
+will be of three bytes: the first for the Report ID (5), the next two
+for the headphone, with two (``Report Count (2)``) bytes
+(``Report Size (8)``) each ranging from 0 (``Logical Minimum (0)`` to 255
+(``Logical Maximum (255)``).
+
+
+Events
+======
+
+One can expect that different ``/dev/input/event*`` are created for different
+Report IDs. Going back to the mouse example, and repeating the sequence where
+one clicks and holds button 1, then clicks and holds button 2,
+releases button 1, and finally releases button 2, one gets::
+
+ marco@sun:~> sudo evtest /dev/input/event4
+ Input driver version is 1.0.1
+ Input device ID: bus 0x3 vendor 0x3f0 product 0x94a version 0x111
+ Input device name: "PixArt HP USB Optical Mouse"
+ Supported events:
+ Event type 0 (EV_SYN)
+ Event type 1 (EV_KEY)
+ Event code 272 (BTN_LEFT)
+ Event code 273 (BTN_RIGHT)
+ Event code 274 (BTN_MIDDLE)
+ Event type 2 (EV_REL)
+ Event code 0 (REL_X)
+ Event code 1 (REL_Y)
+ Event code 8 (REL_WHEEL)
+ Event code 11 (REL_WHEEL_HI_RES)
+ Event type 4 (EV_MSC)
+ Event code 4 (MSC_SCAN)
+ Properties:
+ Testing ... (interrupt to exit)
+ Event: time 1687254626.454252, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90001
+ Event: time 1687254626.454252, type 1 (EV_KEY), code 272 (BTN_LEFT), value 1
+ Event: time 1687254626.454252, -------------- SYN_REPORT ------------
+ Event: time 1687254627.342093, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90002
+ Event: time 1687254627.342093, type 1 (EV_KEY), code 273 (BTN_RIGHT), value 1
+ Event: time 1687254627.342093, -------------- SYN_REPORT ------------
+ Event: time 1687254627.974282, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90001
+ Event: time 1687254627.974282, type 1 (EV_KEY), code 272 (BTN_LEFT), value 0
+ Event: time 1687254627.974282, -------------- SYN_REPORT ------------
+ Event: time 1687254628.798240, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90002
+ Event: time 1687254628.798240, type 1 (EV_KEY), code 273 (BTN_RIGHT), value 0
+ Event: time 1687254628.798240, -------------- SYN_REPORT ------------
+
+
+When everything works well
+==========================
+
+* The HID report descriptor makes sense;
+* It is possible to verify, by reading the raw hid data, that
+ the HID report descriptor *does match* what is sent by the device;
+* The HID report descriptor does not need any "quirk"s (see later on)
+* For any Report ID a corresponding ``/dev/input/event*`` is created,
+ and the events there match what you would expect
+
+When something does not work
+============================
+
+Sometimes not everything does work as it should.
+
+Quirks
+------
+
+A possible reason is that the HID
+has some common quirk. Should this be the case,
+it sould be enough to add the required quirk,
+in the kernel, for the device at hand.
+This can be done in file drivers/hid/hid-quirks.c .
+How to do it should be straightforward after looking into the file.
+
+The list of currently defined quirks, from
+include/linux/hid.h , is
+
+ * ``HID_QUIRK_NOTOUCH``, defined as ``BIT(1)``:
+ * ``HID_QUIRK_IGNORE``, defined as ``BIT(2)``:
+ * ``HID_QUIRK_NOGET``, defined as ``BIT(3)``:
+ * ``HID_QUIRK_HIDDEV_FORCE``, defined as ``BIT(4)``:
+ * ``HID_QUIRK_BADPAD``, defined as ``BIT(5)``:
+ * ``HID_QUIRK_MULTI_INPUT``, defined as ``BIT(6)``:
+ * ``HID_QUIRK_HIDINPUT_FORCE``, defined as ``BIT(7)``:
+ * ``HID_QUIRK_ALWAYS_POLL``, defined as ``BIT(10)``:
+ * ``HID_QUIRK_INPUT_PER_APP``, defined as ``BIT(11)``:
+ * ``HID_QUIRK_X_INVERT``, defined as ``BIT(12)``:
+ * ``HID_QUIRK_Y_INVERT``, defined as ``BIT(13)``:
+ * ``HID_QUIRK_SKIP_OUTPUT_REPORTS``, defined as ``BIT(16)``:
+ * ``HID_QUIRK_SKIP_OUTPUT_REPORT_ID``, defined as ``BIT(17)``:
+ * ``HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP``, defined as ``BIT(18)``:
+ * ``HID_QUIRK_HAVE_SPECIAL_DRIVER``, defined as ``BIT(19)``:
+ * ``HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE``, defined as ``BIT(20)``:
+ * ``HID_QUIRK_FULLSPEED_INTERVAL``, defined as ``BIT(28)``:
+ * ``HID_QUIRK_NO_INIT_REPORTS``, defined as ``BIT(29)``:
+ * ``HID_QUIRK_NO_IGNORE``, defined as ``BIT(30)``:
+ * ``HID_QUIRK_NO_INPUT_SYNC``, defined as ``BIT(31)``:
+
+
+FIXME: ADD A SHORT EXPLANATION FOR EACH QUIRK
+
+Quirks for USB devices can be specified run-time,
+see ``modinfo usbhid``, although the proper fix
+should go into hid-quirks.c and be submitted upstream.
+Quirks for other busses need to go into hid-quirks.c
+
+Fixing the HID report descriptor
+--------------------------------
+
+Should you need to patch the HID report descriptor
+the easiest way is to resort to eBPF, as described
+in Documentation/output/hid/hid-bpf.rst.
+
+Basically, you can change any byte of the original report descriptor.
+The examples in samples/hid should be relatively straightforward,
+see e.g. samples/hid_mouse.bpf.c::
+
+ SEC("fmod_ret/hid_bpf_rdesc_fixup")
+ int BPF_PROG(hid_rdesc_fixup, struct hid_bpf_ctx *hctx)
+ {
+ ....
+ data[39] = 0x31;
+ data[41] = 0x30;
+ return 0;
+ }
+
+Of course this can be also done within the kernel source
+code, see e.g. drivers/hid/hid-aureal.c or
+drivers/hid/hid-samsung.c for a slightly more complex file.
+
+
+
+Modifying on the fly the transmitted data
+-----------------------------------------
+
+It is also possible, always using eBPF, to modify
+on the fly the data exchanged with the device.
+See, again, the examples is samples/hid.
+
+Writing a specialized driver
+----------------------------
+
+This should really be your last resort.
+
diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
index b2028f382f11..af02cf7cfa82 100644
--- a/Documentation/hid/index.rst
+++ b/Documentation/hid/index.rst
@@ -7,6 +7,7 @@ Human Interface Devices (HID)
.. toctree::
:maxdepth: 1
+ hidintro
hiddev
hidraw
hid-sensor
--
2.41.0
^ permalink raw reply related
* Re: [PATCH 0/1] HID: Add introduction about HID for non-kernel programmers
From: Benjamin Tissoires @ 2023-06-20 12:59 UTC (permalink / raw)
To: Marco Morandini; +Cc: Jiri Kosina, linux-input, Jonathan Corbet, linux-doc
In-Reply-To: <d6d16821-2592-8210-475a-5388d7a79e82@polimi.it>
Hi Marco,
On Jun 20 2023, Marco Morandini wrote:
>
> This doc addition is meant to allow a random user
> to understand what is going wrong with his
> brand-new device, and possibly try to fix it on his own.
> I've written it, being one of those random users,
> in an effort to document what I think I've learned,
> and with the hope to minimize the pain to guys like me.
And many thanks for that. This is well written IMO and addresses a lot
of the pain points. So thanks!
>
> The chapter is by no means complete, and for sure will require
> careful checking and fixing from the HID maintainers.
> In my opinion, it would also be great if they could add a brief
> explanation for the different quirks (I've left a FIXME there).
For that particular part (I'll reiterate in the review of the patch),
I'm not sure we are tackling the problem at the correct place. We should
document those in the code directly, and include/reference them in the
.rst file. Trying to duplicate this information is prone to
desynchronisation of the code and the doc and probably noone will keep
it up to date when there is a change.
> I hope I've not misunderstood too many concepts (?events?) and
> that the whole thing is not so screwed up that it's better to
> throw it into the thrash can and run away without looking back.
There are a few nitpicks here and there, but you got the whole thing
(almost) right IMO :)
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 1/1] HID: Add introduction about HID for non-kernel programmers
From: Benjamin Tissoires @ 2023-06-20 13:51 UTC (permalink / raw)
To: Marco Morandini
Cc: Jiri Kosina, linux-input, Peter Hutterer, Jonathan Corbet,
linux-doc
In-Reply-To: <99a679e8-6b01-6805-1e33-ce02485e0063@polimi.it>
Hi Marco,
[adding Peter Hutterer in Cc, he always has good insights]
On Jun 20 2023, Marco Morandini wrote:
>
> This patch adds an introduction about HID
> meant for the casual programmers that is trying
> either to fix his device or to understand what
> is going wrong
You are missing your signed-off-by line here :)
> ---
> Documentation/hid/hidintro.rst | 558 +++++++++++++++++++++++++++++++++
> Documentation/hid/index.rst | 1 +
> 2 files changed, 559 insertions(+)
> create mode 100644 Documentation/hid/hidintro.rst
>
> diff --git a/Documentation/hid/hidintro.rst b/Documentation/hid/hidintro.rst
> new file mode 100644
> index 000000000000..96afb8d807a6
> --- /dev/null
> +++ b/Documentation/hid/hidintro.rst
> @@ -0,0 +1,558 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +======================================
> +Introduction to HID report descriptors
> +======================================
> +
> +This chapter is meant to give a broad overview
> +of what HID report descriptors are, and of how a casual (non kernel)
> +programmer can deal with an HID device that
> +is not working well with Linux.
> +
> +.. contents::
> + :local:
> + :depth: 2
> +
> +
> +Introduction
> +============
> +
> +HID stands for Human Interface Device, and can be whatever device
> +you are using to interact with a computer, be it a mouse,
> +a touchpad, a tablet, a microphone.
> +
> +Many HID work out the box, even if their hardware is different.
FWIW, I always refer to a device following the HID standard to a "HID
device". It's 2 times "device"", but HID has become just an acronym in my
head and refers to the protocol. So not saying "HID device", makes my
reading stop on this, but it's technically correct?
> +For example, mouses can have a different number of buttons; they
s/mouses/mice/
> +can have (or not) a wheel; their movement sensitivity can be
> +significantly different, and so on. Nonetheless,
> +most of the time everything just works, without the need
> +to have specialized code in the kernel for any mouse model
> +developed since 1970.
> +
> +This is because most (if not all) of the modern HIDs do advertise
> +the number and type of signal that can be exchanged, and
> +describe how such signal are exchanged with the computer
> +by means of *HID events*.
> +This is done through the *HID report descriptor*, basically a bunch of numbers
> +that allow the operating system to understand that the mouse at hand
> +has (say) five rather than three buttons.
> +
> +The HID subsystem is in charge of parsing the HID report descriptors,
> +and of converts HID events into normal input
> +device interfaces (see Documentation/hid/hiddev.rst).
hiddev is deprecated I would say. There are still a few users, but I'm
not sure they are quite actively developping products. hidraw is what
you want when you want to talk to the HID devices at the raw level, and
evdev (/dev/input/event*) is what libinput and Xorg consume.
> +Whenever something does not work as it should this can be
> +because the HID report descriptor provided by the device is wrong,
> +or because it needs to be dealt with in a special way,
> +or because the some special device or interaction mode
> +is not handled by the default code.
> +
> +The format of HID report descriptors is described by two documents,
> +available from the `USB Implementers Forum <https://www.usb.org/>`_
> +at `this <https://www.usb.org/hid>`_ addresses:
> +
> + * the `HID USB Device Class Definition <https://www.usb.org/document-library/device-class-definition-hid-111>`_ (HIDUDC from now on)
> + * the `HID Usage Tables <https://usb.org/document-library/hid-usage-tables-14>`_ (HIDUT from now on)
> +
> +This does not means that the HID subsystem can deal with USB devices only;
> +rather, different transport drivers, such as I2C or Bluetooth, can be dealt
> +with, see Documentation/hid/hid-transport.rst.
> +
> +Parsing an HID descriptor
> +=========================
> +
> +The current list of HID devices can be found at ``/sys/bus/hid/devices/``.
> +For each device, say ``/sys/bus/hid/devices/0003\:093A\:2510.0002/``,
> +one can read the corresponding report descriptor::
> +
> + marco@sun:~> hexdump -C /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
> + 00000000 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 |..............).|
> + 00000010 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 |..%.u.....u.....|
> + 00000020 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 |...0.1.8..%.u...|
> + 00000030 81 06 c0 c0 |....|
> + 00000034
> +
> +Alternatively, the HID report descriptor can be read by accessig the hidraw
> +driver, see Documentation/output/hid/hidraw.rst and
> +file samples/hidraw/hid-example.c for a simple example.
> +The output of ``hid-example`` would be, for the same device::
> +
> + marco@sun:~> sudo ./hid-example
> + Report Descriptor Size: 52
> + Report Descriptor:
> + 5 1 9 2 a1 1 9 1 a1 0 5 9 19 1 29 3 15 0 25 1 75 1 95 3 81 2 75 5 95 1 81 1 5 1 9 30 9 31 9 38 15 81 25 7f 75 8 95 3 81 6 c0 c0
> +
> + Raw Name: PixArt USB Optical Mouse
> + Raw Phys: usb-0000:05:00.4-2.3/input0
> + Raw Info:
> + bustype: 3 (USB)
> + vendor: 0x093a
> + product: 0x2510
> + HIDIOCSFEATURE: Broken pipe
> + HIDIOCGFEATURE: Broken pipe
> + Error: 32
> + write: Broken pipe
> + read: Resource temporarily unavailable
> +
> +The basic structure of a HID report descriptor is defined in the HIDUDC manual, while
> +HIDUT "defines constants that can be interpreted by an application to
> +identify the purpose and meaning of a data
> +field in a HID report". Each entry is defined by at least two bytes,
> +where the first one defines what type of value is following,
> +and is described in the HIDUDC manual,
> +and the second one carries the actual value,
> +and is described in the HIDUT manual.
I think the next part up to the various tools that allow to decode the report descriptors
is interesting, but should probably be in a separate file, or in a footnote.
IMO, what matters here is:
- it's a "simple" language defined in those 2 documents
- it's working as a stack: each elements adds to the previous, and
summing everything gives you the overview (there are subtleties of
course)
- noone should ever try to read it by hand, there are tools :)
- in case there is something wrong in the report descriptor, then yes
you'll need that explanation, but it's probably too early in the doc
to explain this IMO
> +
> +Let consider the first number, 0x05: according to
> +HIDUDC, Sec. 6.2.2.2, "Short Items"
> +
> + * the first least significant two bits
> + define the number of the following data bytes (either 0, 1, 2 or 4
> + for the values 0, 1, 2 or 3, respectively)
> + * the second least significant two bits identify the type of the item:
> +
> + * 0: ``Main``
> + * 1: ``Global``
> + * 2: ``Local``
> + * 3: ``Reserved``
> + * the remaining four bits give a numeric expression specifying
> + the function of the item (see below);
> +
> +This means that ``0x05`` (i.e. ``00000101``) stands for
> +1 byte of data to be read, and Global type.
> +Since we are dealing with a Global item the meaning
> +of the most significant four bits
> +is defines in HIDUC manual, Sec. 6.2.2.7, "Global Items".
> +The bits are ``0000``, thus we have a ``Usage Page``.
> +
> +
> +The second number is the actual data, and its meaning can
> +be found in the HICUT manual.
> +We have an ``Usage Page``, thus we need to refer to HICUT Sec. 3,
> +"Usage Pages"; from there, it is clear that the ``0x01``
> +stands for a ``Generic Desktop Page``.
> +
> +Moving now to the second two bytes, and following the same scheme, ``0x09``
> +(i.e. ``00001001``) will be followed by one byte (``01``)
> +and is a ``Local`` item.
> +Thus, the meaning of the remaining four bits (``0000``)
> +is given in HIDUDC Sec. 6.2.2.8 "Local Items", so that we have an ``Usage``.
> +
> +In this way the HID report descriptor can be painstakingly
> +parsed, byte by byte. In practice you need not to do this,
> +and almost no one does
> +this: everyone resorts to specialized parsers. Among all the available ones
> +
> + * the online `USB Descriptor and Request Parser
> + <http://eleccelerator.com/usbdescreqparser/>`_;
> + * `hidrdd <https://github.com/abend0c1/hidrdd>`_,
> + that provides very detailed and somewhat verbose descriptions
> + (verbosity can be useful if you are not familiar with HID report descriptors);
> + * `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_,
> + a complete utility set that allows, among other things,
> + to record and replay the raw HID reports and to debug
> + and replay HID devices.
> + It is being actively developed by the Linux HID subsystem mantainers.
> +
> +.. Parsing the mouse HID report descriptor with `hidrdd <https://github.com/abend0c1/hidrdd>`_ one gets::
Maybe I'm too biased, but why adding the hidrdd output when you also have
the one from hid-tools?
> +
> + marco@sun:~> rexx ./rd.rex -x -d --hex 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
> +
> + //--------------------------------------------------------------------------------
> + // Report descriptor data in hex (length 52 bytes)
> + //--------------------------------------------------------------------------------
> +
> +
> + // 05010902 A1010901 A1000509 19012903 15002501 75019503 81027505 95018101
> + // 05010930 09310938 1581257F 75089503 8106C0C0
> +
> +
> + //--------------------------------------------------------------------------------
> + // Decoded Application Collection
> + //--------------------------------------------------------------------------------
> +
> + /*
> + 05 01 (GLOBAL) USAGE_PAGE 0x0001 Generic Desktop Page
> + 09 02 (LOCAL) USAGE 0x00010002 Mouse (Application Collection)
> + A1 01 (MAIN) COLLECTION 0x01 Application (Usage=0x00010002: Page=Generic Desktop Page, Usage=Mouse, Type=Application Collection)
> + 09 01 (LOCAL) USAGE 0x00010001 Pointer (Physical Collection)
> + A1 00 (MAIN) COLLECTION 0x00 Physical (Usage=0x00010001: Page=Generic Desktop Page, Usage=Pointer, Type=Physical Collection)
> + 05 09 (GLOBAL) USAGE_PAGE 0x0009 Button Page
> + 19 01 (LOCAL) USAGE_MINIMUM 0x00090001 Button 1 Primary/trigger (Selector, On/Off Control, Momentary Control, or One Shot Control)
> + 29 03 (LOCAL) USAGE_MAXIMUM 0x00090003 Button 3 Tertiary (Selector, On/Off Control, Momentary Control, or One Shot Control)
> + 15 00 (GLOBAL) LOGICAL_MINIMUM 0x00 (0) <-- Info: Consider replacing 15 00 with 14
> + 25 01 (GLOBAL) LOGICAL_MAXIMUM 0x01 (1)
> + 75 01 (GLOBAL) REPORT_SIZE 0x01 (1) Number of bits per field
> + 95 03 (GLOBAL) REPORT_COUNT 0x03 (3) Number of fields
> + 81 02 (MAIN) INPUT 0x00000002 (3 fields x 1 bit) 0=Data 1=Variable 0=Absolute 0=NoWrap 0=Linear 0=PrefState 0=NoNull 0=NonVolatile 0=Bitmap
> + 75 05 (GLOBAL) REPORT_SIZE 0x05 (5) Number of bits per field
> + 95 01 (GLOBAL) REPORT_COUNT 0x01 (1) Number of fields
> + 81 01 (MAIN) INPUT 0x00000001 (1 field x 5 bits) 1=Constant 0=Array 0=Absolute
> + 05 01 (GLOBAL) USAGE_PAGE 0x0001 Generic Desktop Page
> + 09 30 (LOCAL) USAGE 0x00010030 X (Dynamic Value)
> + 09 31 (LOCAL) USAGE 0x00010031 Y (Dynamic Value)
> + 09 38 (LOCAL) USAGE 0x00010038 Wheel (Dynamic Value)
> + 15 81 (GLOBAL) LOGICAL_MINIMUM 0x81 (-127)
> + 25 7F (GLOBAL) LOGICAL_MAXIMUM 0x7F (127)
> + 75 08 (GLOBAL) REPORT_SIZE 0x08 (8) Number of bits per field
> + 95 03 (GLOBAL) REPORT_COUNT 0x03 (3) Number of fields
> + 81 06 (MAIN) INPUT 0x00000006 (3 fields x 8 bits) 0=Data 1=Variable 1=Relative 0=NoWrap 0=Linear 0=PrefState 0=NoNull 0=NonVolatile 0=Bitmap
> + C0 (MAIN) END_COLLECTION Physical
> + C0 (MAIN) END_COLLECTION Application
> + */
> +
> + // All structure fields should be byte-aligned...
> + #pragma pack(push,1)
> +
> + //--------------------------------------------------------------------------------
> + // Button Page inputReport (Device --> Host)
> + //--------------------------------------------------------------------------------
> +
> + typedef struct
> + {
> + // No REPORT ID byte
> + // Collection: CA:Mouse CP:Pointer
> + uint8_t BTN_MousePointerButton1 : 1; // Usage 0x00090001: Button 1 Primary/trigger, Value = 0 to 1
> + uint8_t BTN_MousePointerButton2 : 1; // Usage 0x00090002: Button 2 Secondary, Value = 0 to 1
> + uint8_t BTN_MousePointerButton3 : 1; // Usage 0x00090003: Button 3 Tertiary, Value = 0 to 1
> + uint8_t : 5; // Pad
> + int8_t GD_MousePointerX; // Usage 0x00010030: X, Value = -127 to 127
> + int8_t GD_MousePointerY; // Usage 0x00010031: Y, Value = -127 to 127
> + int8_t GD_MousePointerWheel; // Usage 0x00010038: Wheel, Value = -127 to 127
> + } inputReport_t;
> +
> + #pragma pack(pop)
> +
> +Parsing the mouse HID report descriptor with `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_ leads to::
> +
> + marco@sun:~/Programmi/linux/hid-tools (master =)> ./hid-decode /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
> + # device 0:0
> + # 0x05, 0x01, // Usage Page (Generic Desktop) 0
> + # 0x09, 0x02, // Usage (Mouse) 2
> + # 0xa1, 0x01, // Collection (Application) 4
> + # 0x09, 0x01, // Usage (Pointer) 6
> + # 0xa1, 0x00, // Collection (Physical) 8
> + # 0x05, 0x09, // Usage Page (Button) 10
> + # 0x19, 0x01, // Usage Minimum (1) 12
> + # 0x29, 0x03, // Usage Maximum (3) 14
> + # 0x15, 0x00, // Logical Minimum (0) 16
> + # 0x25, 0x01, // Logical Maximum (1) 18
> + # 0x75, 0x01, // Report Size (1) 20
> + # 0x95, 0x03, // Report Count (3) 22
> + # 0x81, 0x02, // Input (Data,Var,Abs) 24
> + # 0x75, 0x05, // Report Size (5) 26
> + # 0x95, 0x01, // Report Count (1) 28
> + # 0x81, 0x01, // Input (Cnst,Arr,Abs) 30
> + # 0x05, 0x01, // Usage Page (Generic Desktop) 32
> + # 0x09, 0x30, // Usage (X) 34
> + # 0x09, 0x31, // Usage (Y) 36
> + # 0x09, 0x38, // Usage (Wheel) 38
> + # 0x15, 0x81, // Logical Minimum (-127) 40
> + # 0x25, 0x7f, // Logical Maximum (127) 42
> + # 0x75, 0x08, // Report Size (8) 44
> + # 0x95, 0x03, // Report Count (3) 46
> + # 0x81, 0x06, // Input (Data,Var,Rel) 48
> + # 0xc0, // End Collection 50
> + # 0xc0, // End Collection 51
> + #
> + R: 52 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
> + N: device 0:0
> + I: 3 0001 0001
> +
> +From it we undesratnd that
> +
> + * the mouse has three (from ``Usage Minimum (1)`` to
> + ``Usage Maximum (3)``) buttons (``Usage Page (Button)``);
> + * buttons can take values ranging from ``0`` to ``1``;
> + (from ``Logical Minimum (0)`` to ``Logical Maximum (1)``);
> + * information is encoded into three bits: one bit has
> + ``Report Size (1)``,
> + but there are three of them since ``Report Count (3)``;
> + * the value of these bits can change
> + (``Data`` in ``Input (Data,Var,Abs)``);
> + * each field represents data from a physical control;
> + * the number of bits reserved for each field is determined
> + by the preceding ``Report Size``/``Report Count``
> + items (``Var`` in ``Input (Data,Var,Abs)``);
> + * the data is *absolute* (i.e it does not represent the
> + change from the last report, ``Abs`` in ``Input (Data,Var,Abs)``).
> +
> +The meaning of the ``Input``
> +items is explained in HIDUDC Sec. 6.2.2.5 "Input, Output, and Feature Items.
> +
> +There are five additional padding bits, that are needed
> +to reach a byte: see ``Report Size (5)``, that is
> +repeated only once (``Report Count (1)``).
> +These bits take *constant* values (``Cnst`` in
> +``Input (Cnst,Arr,Abs)``).
> +
> +The mouse has also two physical positions (``Usage (X)``, ``Usage (Y)``) and a wheel
> +(``Usage (Wheel)``).
> +
> +Each of them take values ranging from ``-127`` to ``127``
> +(from ``Logical Minimum (-127)`` to ``Logical Maximum (-127)``),
> +it is represented by eight bits (``Report Size (8)``)
> +and there are three of these set of bits (``Report Count (3)``).
> +
> +This time the data do represent the change from the previous configuration
> +(``Rel`` in ``Input (Data,Var,Rel)``).
> +
> +All in all, the mouse input will be transmitted using four bytes:
> +the first one for the buttons (three bits used, five for padding),
> +the last three for the mouse X, Y and wheel changes, respectively.
> +
> +Indeed, for any event, the mouse will send a *report* of four bytes.
> +We can easily check the values sent by resorting e.g.
> +to the `hid-recorder` tool, from `hid-tools
> +<https://gitlab.freedesktop.org/libevdev/hid-tools>`_:
> +The sequence of bytes sent by clicking and releasing
> +button 1, then button 2, then button 3 is::
> +
> + marco@sun:~/> sudo ./hid-recorder /dev/hidraw1
> +
> + ....
> + output of hid-decode
> + ....
> +
> + # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000000.000000 4 01 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000000.183949 4 00 00 00 00
> + # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000001.959698 4 02 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000002.103899 4 00 00 00 00
> + # Button: 0 0 1 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000004.855799 4 04 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000005.103864 4 00 00 00 00
> +
> +where it's clear that, for example, when button 2 is clicked
> +the bytes ``02 00 00 00`` are sent, and the immediately subsequent
> +event (``00 00 00 00``) is the release of button 2 (no buttons are pressed,
> +remember that the data is *absolute*).
> +
> +If instead one clicks and holds button 1, then clicks and holds button 2,
> +releases button 1, and finally releases button 2, the reports are::
> +
> + # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000044.175830 4 01 00 00 00
> + # Button: 1 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000045.975997 4 03 00 00 00
> + # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000047.407930 4 02 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000049.199919 4 00 00 00 00
> +
> +where with ``03 00 00 00`` both buttons are pressed, and with the
> +subsequent ``02 00 00 00`` button 1 is released while button 2 is still
> +active.
> +
> +Outputs and Inputs
> +------------------
> +
> +An HID devices can have inputs, like
> +in the mouse example, and outputs.
> +"Output" means that the information is fed
> +from the device to the human; for examples,
The other way around: outputs are from the host (computer/human) to the
device, when input is from the device to the host.
> +a joystick with force feedback will have
> +some output.
There are also Features, which is a side channel configuration of the
device from the host to the device.
Most of the time Features have a state (are you using high reslution
wheel events?) and can be queried from the host. Most of the time :)
> +
> +
> +Report IDs and Evdev events
> +===========================
> +
> +A single device can logically group
> +data into different, independent sets.
> +It is *as if* the HID presents
> +itself as different devices, each exchanging
> +its own data. The HID report descriptor is unique,
> +but the different reports are identified by means
> +of different ``Report ID`` fields. Whenever a ``Report ID``
> +is needed it is transmitted as the first byte of any report.
I wouldn't say this like that.
The following is an attempt to explain to you the slight difference
between collections and report IDs, so it should not be taken verbatim
in the doc.
You can group data by using "Collections". Each collection has a type
and purpose. You have "application" collections, "physical" collections
and "logical" collections. For each you can assign a purpose: for
example a touchpanel that exports a touchscreen and a stylus would
export 2 application collections of "Touchscreen" and "Pen".
But then to be able to differentiate those collections, we have the
"Report ID", which is handled specifically in the HID standard as the
first byte (when defined) associated to a collection.
But given that collections can be stacked, there is not a 1-to-1
relation between Report IDs and all defined collection.
> +
> +Consider the following HID report descriptor::
> +
> + 05 01 09 02 A1 01 85 01 05 09 19 01 29 05 15 00
> + 25 01 95 05 75 01 81 02 95 01 75 03 81 01 05 01
> + 09 30 09 31 16 00 F8 26 FF 07 75 0C 95 02 81 06
> + 09 38 15 80 25 7F 75 08 95 01 81 06 05 0C 0A 38
> + 02 15 80 25 7F 75 08 95 01 81 06 C0 05 01 09 02
> + A1 01 85 02 05 09 19 01 29 05 15 00 25 01 95 05
> + 75 01 81 02 95 01 75 03 81 01 05 01 09 30 09 31
> + 16 00 F8 26 FF 07 75 0C 95 02 81 06 09 38 15 80
> + 25 7F 75 08 95 01 81 06 05 0C 0A 38 02 15 80 25
> + 7F 75 08 95 01 81 06 C0 05 01 09 07 A1 01 85 05
> + 05 07 15 00 25 01 09 29 09 3E 09 4B 09 4E 09 E3
> + 09 E8 09 E8 09 E8 75 01 95 08 81 02 95 00 81 01
> + C0 05 0C 09 01 A1 01 85 06 15 00 25 01 75 01 95
> + 01 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
> + 06 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
> + 06 C0 05 0C 09 01 A1 01 85 03 09 05 15 00 26 FF
> + 00 75 08 95 02 B1 02 C0
> +
> +After parsing it (try to parse it on your own using
> +the suggested tools!)
> +one can see that the device presents two mouses
s/mouses/mice/
> +(Reports IDs 1 and 2, respectively),
> +a Keypad (Report ID 5) and two consumer controls
> +(Report IDs 6 and 3).
> +The data sent for each of these report ids
> +will begin with the Report ID byte, and will be followed
> +by the corresponding information. For example, the
> +report defined for the last consumer
> +control::
> +
> + 0x05, 0x0C, // Usage Page (Consumer)
> + 0x09, 0x01, // Usage (Consumer Control)
> + 0xA1, 0x01, // Collection (Application)
> + 0x85, 0x03, // Report ID (3)
> + 0x09, 0x05, // Usage (Headphone)
> + 0x15, 0x00, // Logical Minimum (0)
> + 0x26, 0xFF, 0x00, // Logical Maximum (255)
> + 0x75, 0x08, // Report Size (8)
> + 0x95, 0x02, // Report Count (2)
> + 0xB1, 0x02, // Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> + 0xC0, // End Collection
> +
> +will be of three bytes: the first for the Report ID (5), the next two
> +for the headphone, with two (``Report Count (2)``) bytes
> +(``Report Size (8)``) each ranging from 0 (``Logical Minimum (0)`` to 255
> +(``Logical Maximum (255)``).
> +
> +
> +Events
> +======
> +
> +One can expect that different ``/dev/input/event*`` are created for different
> +Report IDs. Going back to the mouse example, and repeating the sequence where
> +one clicks and holds button 1, then clicks and holds button 2,
> +releases button 1, and finally releases button 2, one gets::
> +
> + marco@sun:~> sudo evtest /dev/input/event4
evtest has been deprecated for a while, and evemu too. Now, cool kids
are using "libinput record", but it's slightly more verbose.
Using evemu is probably better at least.
> + Input driver version is 1.0.1
> + Input device ID: bus 0x3 vendor 0x3f0 product 0x94a version 0x111
> + Input device name: "PixArt HP USB Optical Mouse"
> + Supported events:
> + Event type 0 (EV_SYN)
> + Event type 1 (EV_KEY)
> + Event code 272 (BTN_LEFT)
> + Event code 273 (BTN_RIGHT)
> + Event code 274 (BTN_MIDDLE)
> + Event type 2 (EV_REL)
> + Event code 0 (REL_X)
> + Event code 1 (REL_Y)
> + Event code 8 (REL_WHEEL)
> + Event code 11 (REL_WHEEL_HI_RES)
> + Event type 4 (EV_MSC)
> + Event code 4 (MSC_SCAN)
> + Properties:
> + Testing ... (interrupt to exit)
> + Event: time 1687254626.454252, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90001
> + Event: time 1687254626.454252, type 1 (EV_KEY), code 272 (BTN_LEFT), value 1
> + Event: time 1687254626.454252, -------------- SYN_REPORT ------------
> + Event: time 1687254627.342093, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90002
> + Event: time 1687254627.342093, type 1 (EV_KEY), code 273 (BTN_RIGHT), value 1
> + Event: time 1687254627.342093, -------------- SYN_REPORT ------------
> + Event: time 1687254627.974282, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90001
> + Event: time 1687254627.974282, type 1 (EV_KEY), code 272 (BTN_LEFT), value 0
> + Event: time 1687254627.974282, -------------- SYN_REPORT ------------
> + Event: time 1687254628.798240, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90002
> + Event: time 1687254628.798240, type 1 (EV_KEY), code 273 (BTN_RIGHT), value 0
> + Event: time 1687254628.798240, -------------- SYN_REPORT ------------
> +
> +
> +When everything works well
> +==========================
> +
> +* The HID report descriptor makes sense;
i.e. the current tools are capable of making some sense out of it :)
> +* It is possible to verify, by reading the raw hid data, that
> + the HID report descriptor *does match* what is sent by the device;
nitpick: extra space between "does" and "match"
> +* The HID report descriptor does not need any "quirk"s (see later on)
> +* For any Report ID a corresponding ``/dev/input/event*`` is created,
> + and the events there match what you would expect
That last point is not stricly correct. Currently, each application
collection gets its own input node. You can have a collection with
several report IDs because they are all using the same device but a
different language.
> +
> +When something does not work
> +============================
> +
> +Sometimes not everything does work as it should.
*not everything works
> +
> +Quirks
> +------
> +
> +A possible reason is that the HID
> +has some common quirk. Should this be the case,
> +it sould be enough to add the required quirk,
> +in the kernel, for the device at hand.
> +This can be done in file drivers/hid/hid-quirks.c .
> +How to do it should be straightforward after looking into the file.
> +
> +The list of currently defined quirks, from
> +include/linux/hid.h , is
> +
> + * ``HID_QUIRK_NOTOUCH``, defined as ``BIT(1)``:
> + * ``HID_QUIRK_IGNORE``, defined as ``BIT(2)``:
> + * ``HID_QUIRK_NOGET``, defined as ``BIT(3)``:
> + * ``HID_QUIRK_HIDDEV_FORCE``, defined as ``BIT(4)``:
> + * ``HID_QUIRK_BADPAD``, defined as ``BIT(5)``:
> + * ``HID_QUIRK_MULTI_INPUT``, defined as ``BIT(6)``:
> + * ``HID_QUIRK_HIDINPUT_FORCE``, defined as ``BIT(7)``:
> + * ``HID_QUIRK_ALWAYS_POLL``, defined as ``BIT(10)``:
> + * ``HID_QUIRK_INPUT_PER_APP``, defined as ``BIT(11)``:
> + * ``HID_QUIRK_X_INVERT``, defined as ``BIT(12)``:
> + * ``HID_QUIRK_Y_INVERT``, defined as ``BIT(13)``:
> + * ``HID_QUIRK_SKIP_OUTPUT_REPORTS``, defined as ``BIT(16)``:
> + * ``HID_QUIRK_SKIP_OUTPUT_REPORT_ID``, defined as ``BIT(17)``:
> + * ``HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP``, defined as ``BIT(18)``:
> + * ``HID_QUIRK_HAVE_SPECIAL_DRIVER``, defined as ``BIT(19)``:
> + * ``HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE``, defined as ``BIT(20)``:
> + * ``HID_QUIRK_FULLSPEED_INTERVAL``, defined as ``BIT(28)``:
> + * ``HID_QUIRK_NO_INIT_REPORTS``, defined as ``BIT(29)``:
> + * ``HID_QUIRK_NO_IGNORE``, defined as ``BIT(30)``:
> + * ``HID_QUIRK_NO_INPUT_SYNC``, defined as ``BIT(31)``:
We should definitely include the doc directly in hid.h and include it
there. I already explained why in the cover letter.
But moreover, quirks are supposed to be the exception. If we are adding
too many quirks, and that the same devices work on Windows without a
special handling, that means that the quirk should be analyzed, and we
should probably rethink the processing of the HID devices to not have
this quirk.
> +
> +
> +FIXME: ADD A SHORT EXPLANATION FOR EACH QUIRK
> +
> +Quirks for USB devices can be specified run-time,
> +see ``modinfo usbhid``, although the proper fix
> +should go into hid-quirks.c and be submitted upstream.
> +Quirks for other busses need to go into hid-quirks.c
> +
> +Fixing the HID report descriptor
> +--------------------------------
> +
> +Should you need to patch the HID report descriptor
> +the easiest way is to resort to eBPF, as described
> +in Documentation/output/hid/hid-bpf.rst.
> +
> +Basically, you can change any byte of the original report descriptor.
> +The examples in samples/hid should be relatively straightforward,
> +see e.g. samples/hid_mouse.bpf.c::
> +
> + SEC("fmod_ret/hid_bpf_rdesc_fixup")
> + int BPF_PROG(hid_rdesc_fixup, struct hid_bpf_ctx *hctx)
> + {
> + ....
> + data[39] = 0x31;
> + data[41] = 0x30;
> + return 0;
> + }
If you ever do that, please share your code on the LKML. The current
background work is to integrate those hid-bpf programs in the kernel
directly, so that we can share them with everyone without resorting to a
third party userspace program.
> +
> +Of course this can be also done within the kernel source
> +code, see e.g. drivers/hid/hid-aureal.c or
> +drivers/hid/hid-samsung.c for a slightly more complex file.
> +
> +
> +
> +Modifying on the fly the transmitted data
> +-----------------------------------------
> +
> +It is also possible, always using eBPF, to modify
> +on the fly the data exchanged with the device.
> +See, again, the examples is samples/hid.
> +
> +Writing a specialized driver
> +----------------------------
> +
> +This should really be your last resort.
YES, definitely YES :)
> +
> diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> index b2028f382f11..af02cf7cfa82 100644
> --- a/Documentation/hid/index.rst
> +++ b/Documentation/hid/index.rst
> @@ -7,6 +7,7 @@ Human Interface Devices (HID)
> .. toctree::
> :maxdepth: 1
>
> + hidintro
> hiddev
> hidraw
> hid-sensor
> --
> 2.41.0
>
>
>
Cheers,
Benjamin
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-06-20 13:20 UTC (permalink / raw)
To: Malte Starostik, Benjamin Tissoires,
Linux regressions mailing list, Limonciello, Mario
Cc: Bagas Sanjaya, basavaraj.natikar, linux-input, linux, stable
In-Reply-To: <5980752.YW5z2jdOID@zen>
Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
for once, to make this easily accessible to everyone.
What happens to this? From here it looks like there was no progress to
resolve the regression in the past two weeks, but maybe I just missed
something.
On 07.06.23 00:57, Malte Starostik wrote:
> Am Dienstag, 6. Juni 2023, 17:25:13 CEST schrieb Limonciello, Mario:
>> On 6/6/2023 3:08 AM, Benjamin Tissoires wrote:
>>> On Jun 06 2023, Linux regression tracking (Thorsten Leemhuis) wrote:
>>>>> On Mon, Jun 05, 2023 at 01:24:25PM +0200, Malte Starostik wrote:
>>>>>> Hello,
>>>>>>
>>>>>> chiming in here as I'm experiencing what looks like the exact same
>>>>>> issue, also on a Lenovo Z13 notebook, also on Arch:
>>>>>> Oops during startup in task udev-worker followed by udev-worker
>>>>>> blocking all attempts to suspend or cleanly shutdown/reboot the
>>>>>> machine
>
>>> I have a suspicion on commit 7bcfdab3f0c6 ("HID: amd_sfh: if no sensors
>>> are enabled, clean up") because the stack trace says that there is a bad
>>> list_add, which could happen if the object is not correctly initialized.
>>>
>>> However, that commit was present in v6.2, so it might not be that one.
>>>
>> If I'm not mistaken the Z13 doesn't actually have any
>> sensors connected to SFH. So I think the suspicion on
>> 7bcfdab3f0c6 and theory this is triggered by HID init makes
>> a lot of sense.
>>
>> Can you try this patch?
>>
>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>> b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>> index d9b7b01900b5..fa693a5224c6 100644
>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>> @@ -324,6 +324,7 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev
>> *privdata)
>> devm_kfree(dev, cl_data->report_descr[i]);
>> }
>> dev_warn(dev, "Failed to discover, sensors not enabled
>> is %d\n", cl_data->is_any_sensor_enabled);
>> + cl_data->num_hid_devices = 0;
>> return -EOPNOTSUPP;
>> }
>> schedule_delayed_work(&cl_data->work_buffer,
>> msecs_to_jiffies(AMD_SFH_IDLE_LOOP));
>
> I applied this to 9e87b63ed37e202c77aa17d4112da6ae0c7c097c now, which was the
> origin when I started the whole bisection. Clean rebuild, issue still
> persists.
>
> Out of 50 boots, I got:
>
> 25 clean
> 22 Oops as posted by the OP
> 1 same Oops, followed by a panic
> 1 lockup [1]
> 1 hanging with just a blank screen
>
> Not sure whether the lockups are related, but [1] mentions modprobe and udev-
> worker as well and all problems including the blank screen one appear roughly
> at the same time during boot. As this is before a graphics mode switch, I
> suspect the last mentioned case may be like [1] while the screen was blanked.
> To support the timing correlation: the UVC error for the IR cam shown in the
> photo (normal boot noise) also appears right before the BUG in the non-lockup
> bad case.
>
> I do see the dev_warn in dmesg, so the code path modified in your patch is
> indeed hit:
> [ 10.897521] pcie_mp2_amd 0000:63:00.7: Failed to discover, sensors not
> enabled is 1
> [ 10.897533] pcie_mp2_amd: probe of 0000:63:00.7 failed with error -95
>
> BR Malte
>
> [1] https://photos.app.goo.gl/2FAvQ7DqBsHEF6Bd8
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
#regzbot poke
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: input: document Goodix Berlin Touchscreen IC
From: Rob Herring @ 2023-06-20 16:34 UTC (permalink / raw)
To: Neil Armstrong
Cc: Bastien Nocera, Rob Herring, linux-input, Hans de Goede,
linux-arm-msm, Conor Dooley, devicetree, Krzysztof Kozlowski,
linux-kernel, Henrik Rydberg, Dmitry Torokhov
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v2-1-26bc8fe1e90e@linaro.org>
On Thu, 15 Jun 2023 12:27:00 +0200, Neil Armstrong wrote:
> Document the Goodix GT9916 wich is part of the "Berlin" serie
> of Touchscreen controllers IC from Goodix.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> .../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH 1/2] HID: roccat: make all 'class' structures const
From: Greg Kroah-Hartman @ 2023-06-20 18:31 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Ivan Orlov, Stefan Achatz, Jiri Kosina,
Benjamin Tissoires, Greg Kroah-Hartman
From: Ivan Orlov <ivan.orlov0322@gmail.com>
Now that the driver core allows for struct class to be in read-only
memory, making all 'class' structures to be declared at build time
placing them into read-only memory, instead of having to be dynamically
allocated at load time.
Cc: Stefan Achatz <erazor_de@users.sourceforge.net>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: linux-input@vger.kernel.org
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-roccat-arvo.c | 20 +++++++++++---------
drivers/hid/hid-roccat-isku.c | 21 ++++++++++++---------
drivers/hid/hid-roccat-kone.c | 24 +++++++++++++-----------
drivers/hid/hid-roccat-koneplus.c | 22 ++++++++++++----------
drivers/hid/hid-roccat-konepure.c | 22 ++++++++++++----------
drivers/hid/hid-roccat-kovaplus.c | 22 ++++++++++++----------
drivers/hid/hid-roccat-pyra.c | 22 ++++++++++++----------
drivers/hid/hid-roccat-ryos.c | 20 +++++++++++---------
drivers/hid/hid-roccat-savu.c | 20 +++++++++++---------
drivers/hid/hid-roccat.c | 2 +-
include/linux/hid-roccat.h | 2 +-
11 files changed, 108 insertions(+), 89 deletions(-)
diff --git a/drivers/hid/hid-roccat-arvo.c b/drivers/hid/hid-roccat-arvo.c
index ea6b79b3aeeb..d55aaabab1ed 100644
--- a/drivers/hid/hid-roccat-arvo.c
+++ b/drivers/hid/hid-roccat-arvo.c
@@ -23,8 +23,6 @@
#include "hid-roccat-common.h"
#include "hid-roccat-arvo.h"
-static struct class *arvo_class;
-
static ssize_t arvo_sysfs_show_mode_key(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -268,6 +266,11 @@ static const struct attribute_group *arvo_groups[] = {
NULL,
};
+static const struct class arvo_class = {
+ .name = "arvo",
+ .dev_groups = arvo_groups,
+};
+
static int arvo_init_arvo_device_struct(struct usb_device *usb_dev,
struct arvo_device *arvo)
{
@@ -309,7 +312,7 @@ static int arvo_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(arvo_class, hdev,
+ retval = roccat_connect(&arvo_class, hdev,
sizeof(struct arvo_roccat_report));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
@@ -433,21 +436,20 @@ static int __init arvo_init(void)
{
int retval;
- arvo_class = class_create("arvo");
- if (IS_ERR(arvo_class))
- return PTR_ERR(arvo_class);
- arvo_class->dev_groups = arvo_groups;
+ retval = class_register(&arvo_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&arvo_driver);
if (retval)
- class_destroy(arvo_class);
+ class_unregister(&arvo_class);
return retval;
}
static void __exit arvo_exit(void)
{
hid_unregister_driver(&arvo_driver);
- class_destroy(arvo_class);
+ class_unregister(&arvo_class);
}
module_init(arvo_init);
diff --git a/drivers/hid/hid-roccat-isku.c b/drivers/hid/hid-roccat-isku.c
index 3903a2cea00c..458060403397 100644
--- a/drivers/hid/hid-roccat-isku.c
+++ b/drivers/hid/hid-roccat-isku.c
@@ -23,8 +23,6 @@
#include "hid-roccat-common.h"
#include "hid-roccat-isku.h"
-static struct class *isku_class;
-
static void isku_profile_activated(struct isku_device *isku, uint new_profile)
{
isku->actual_profile = new_profile;
@@ -248,6 +246,11 @@ static const struct attribute_group *isku_groups[] = {
NULL,
};
+static const struct class isku_class = {
+ .name = "isku",
+ .dev_groups = isku_groups,
+};
+
static int isku_init_isku_device_struct(struct usb_device *usb_dev,
struct isku_device *isku)
{
@@ -289,7 +292,7 @@ static int isku_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(isku_class, hdev,
+ retval = roccat_connect(&isku_class, hdev,
sizeof(struct isku_roccat_report));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
@@ -435,21 +438,21 @@ static struct hid_driver isku_driver = {
static int __init isku_init(void)
{
int retval;
- isku_class = class_create("isku");
- if (IS_ERR(isku_class))
- return PTR_ERR(isku_class);
- isku_class->dev_groups = isku_groups;
+
+ retval = class_register(&isku_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&isku_driver);
if (retval)
- class_destroy(isku_class);
+ class_unregister(&isku_class);
return retval;
}
static void __exit isku_exit(void)
{
hid_unregister_driver(&isku_driver);
- class_destroy(isku_class);
+ class_unregister(&isku_class);
}
module_init(isku_init);
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 945ae236fb45..00a1abc7e839 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -89,9 +89,6 @@ static int kone_send(struct usb_device *usb_dev, uint usb_command,
return ((len < 0) ? len : ((len != size) ? -EIO : 0));
}
-/* kone_class is used for creating sysfs attributes via roccat char device */
-static struct class *kone_class;
-
static void kone_set_settings_checksum(struct kone_settings *settings)
{
uint16_t checksum = 0;
@@ -657,6 +654,12 @@ static const struct attribute_group *kone_groups[] = {
NULL,
};
+/* kone_class is used for creating sysfs attributes via roccat char device */
+static const struct class kone_class = {
+ .name = "kone",
+ .dev_groups = kone_groups,
+};
+
static int kone_init_kone_device_struct(struct usb_device *usb_dev,
struct kone_device *kone)
{
@@ -712,8 +715,8 @@ static int kone_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(kone_class, hdev,
- sizeof(struct kone_roccat_report));
+ retval = roccat_connect(&kone_class, hdev,
+ sizeof(struct kone_roccat_report));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
/* be tolerant about not getting chrdev */
@@ -890,21 +893,20 @@ static int __init kone_init(void)
int retval;
/* class name has to be same as driver name */
- kone_class = class_create("kone");
- if (IS_ERR(kone_class))
- return PTR_ERR(kone_class);
- kone_class->dev_groups = kone_groups;
+ retval = class_register(&kone_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&kone_driver);
if (retval)
- class_destroy(kone_class);
+ class_unregister(&kone_class);
return retval;
}
static void __exit kone_exit(void)
{
hid_unregister_driver(&kone_driver);
- class_destroy(kone_class);
+ class_unregister(&kone_class);
}
module_init(kone_init);
diff --git a/drivers/hid/hid-roccat-koneplus.c b/drivers/hid/hid-roccat-koneplus.c
index 97b83b6f53dd..22b895436a7c 100644
--- a/drivers/hid/hid-roccat-koneplus.c
+++ b/drivers/hid/hid-roccat-koneplus.c
@@ -26,8 +26,6 @@
static uint profile_numbers[5] = {0, 1, 2, 3, 4};
-static struct class *koneplus_class;
-
static void koneplus_profile_activated(struct koneplus_device *koneplus,
uint new_profile)
{
@@ -356,6 +354,11 @@ static const struct attribute_group *koneplus_groups[] = {
NULL,
};
+static const struct class koneplus_class = {
+ .name = "koneplus",
+ .dev_groups = koneplus_groups,
+};
+
static int koneplus_init_koneplus_device_struct(struct usb_device *usb_dev,
struct koneplus_device *koneplus)
{
@@ -394,8 +397,8 @@ static int koneplus_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(koneplus_class, hdev,
- sizeof(struct koneplus_roccat_report));
+ retval = roccat_connect(&koneplus_class, hdev,
+ sizeof(struct koneplus_roccat_report));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
} else {
@@ -549,21 +552,20 @@ static int __init koneplus_init(void)
int retval;
/* class name has to be same as driver name */
- koneplus_class = class_create("koneplus");
- if (IS_ERR(koneplus_class))
- return PTR_ERR(koneplus_class);
- koneplus_class->dev_groups = koneplus_groups;
+ retval = class_register(&koneplus_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&koneplus_driver);
if (retval)
- class_destroy(koneplus_class);
+ class_unregister(&koneplus_class);
return retval;
}
static void __exit koneplus_exit(void)
{
hid_unregister_driver(&koneplus_driver);
- class_destroy(koneplus_class);
+ class_unregister(&koneplus_class);
}
module_init(koneplus_init);
diff --git a/drivers/hid/hid-roccat-konepure.c b/drivers/hid/hid-roccat-konepure.c
index a297756f2410..beca8aef8bbb 100644
--- a/drivers/hid/hid-roccat-konepure.c
+++ b/drivers/hid/hid-roccat-konepure.c
@@ -36,8 +36,6 @@ struct konepure_mouse_report_button {
uint8_t unknown[2];
} __packed;
-static struct class *konepure_class;
-
ROCCAT_COMMON2_BIN_ATTRIBUTE_W(control, 0x04, 0x03);
ROCCAT_COMMON2_BIN_ATTRIBUTE_RW(actual_profile, 0x05, 0x03);
ROCCAT_COMMON2_BIN_ATTRIBUTE_RW(profile_settings, 0x06, 0x1f);
@@ -72,6 +70,11 @@ static const struct attribute_group *konepure_groups[] = {
NULL,
};
+static const struct class konepure_class = {
+ .name = "konepure",
+ .dev_groups = konepure_groups,
+};
+
static int konepure_init_specials(struct hid_device *hdev)
{
struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
@@ -98,8 +101,8 @@ static int konepure_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(konepure_class, hdev,
- sizeof(struct konepure_mouse_report_button));
+ retval = roccat_connect(&konepure_class, hdev,
+ sizeof(struct konepure_mouse_report_button));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
} else {
@@ -207,21 +210,20 @@ static int __init konepure_init(void)
{
int retval;
- konepure_class = class_create("konepure");
- if (IS_ERR(konepure_class))
- return PTR_ERR(konepure_class);
- konepure_class->dev_groups = konepure_groups;
+ retval = class_register(&konepure_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&konepure_driver);
if (retval)
- class_destroy(konepure_class);
+ class_unregister(&konepure_class);
return retval;
}
static void __exit konepure_exit(void)
{
hid_unregister_driver(&konepure_driver);
- class_destroy(konepure_class);
+ class_unregister(&konepure_class);
}
module_init(konepure_init);
diff --git a/drivers/hid/hid-roccat-kovaplus.c b/drivers/hid/hid-roccat-kovaplus.c
index 1a1d96e11683..86af538c10d6 100644
--- a/drivers/hid/hid-roccat-kovaplus.c
+++ b/drivers/hid/hid-roccat-kovaplus.c
@@ -24,8 +24,6 @@
static uint profile_numbers[5] = {0, 1, 2, 3, 4};
-static struct class *kovaplus_class;
-
static uint kovaplus_convert_event_cpi(uint value)
{
return (value == 7 ? 4 : (value == 4 ? 3 : value));
@@ -409,6 +407,11 @@ static const struct attribute_group *kovaplus_groups[] = {
NULL,
};
+static const struct class kovaplus_class = {
+ .name = "kovaplus",
+ .dev_groups = kovaplus_groups,
+};
+
static int kovaplus_init_kovaplus_device_struct(struct usb_device *usb_dev,
struct kovaplus_device *kovaplus)
{
@@ -463,8 +466,8 @@ static int kovaplus_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(kovaplus_class, hdev,
- sizeof(struct kovaplus_roccat_report));
+ retval = roccat_connect(&kovaplus_class, hdev,
+ sizeof(struct kovaplus_roccat_report));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
} else {
@@ -638,21 +641,20 @@ static int __init kovaplus_init(void)
{
int retval;
- kovaplus_class = class_create("kovaplus");
- if (IS_ERR(kovaplus_class))
- return PTR_ERR(kovaplus_class);
- kovaplus_class->dev_groups = kovaplus_groups;
+ retval = class_register(&kovaplus_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&kovaplus_driver);
if (retval)
- class_destroy(kovaplus_class);
+ class_unregister(&kovaplus_class);
return retval;
}
static void __exit kovaplus_exit(void)
{
hid_unregister_driver(&kovaplus_driver);
- class_destroy(kovaplus_class);
+ class_unregister(&kovaplus_class);
}
module_init(kovaplus_init);
diff --git a/drivers/hid/hid-roccat-pyra.c b/drivers/hid/hid-roccat-pyra.c
index 15528c3b013c..5663b9cd9c69 100644
--- a/drivers/hid/hid-roccat-pyra.c
+++ b/drivers/hid/hid-roccat-pyra.c
@@ -26,9 +26,6 @@
static uint profile_numbers[5] = {0, 1, 2, 3, 4};
-/* pyra_class is used for creating sysfs attributes via roccat char device */
-static struct class *pyra_class;
-
static void profile_activated(struct pyra_device *pyra,
unsigned int new_profile)
{
@@ -366,6 +363,12 @@ static const struct attribute_group *pyra_groups[] = {
NULL,
};
+/* pyra_class is used for creating sysfs attributes via roccat char device */
+static const struct class pyra_class = {
+ .name = "pyra",
+ .dev_groups = pyra_groups,
+};
+
static int pyra_init_pyra_device_struct(struct usb_device *usb_dev,
struct pyra_device *pyra)
{
@@ -413,7 +416,7 @@ static int pyra_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(pyra_class, hdev,
+ retval = roccat_connect(&pyra_class, hdev,
sizeof(struct pyra_roccat_report));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
@@ -585,21 +588,20 @@ static int __init pyra_init(void)
int retval;
/* class name has to be same as driver name */
- pyra_class = class_create("pyra");
- if (IS_ERR(pyra_class))
- return PTR_ERR(pyra_class);
- pyra_class->dev_groups = pyra_groups;
+ retval = class_register(&pyra_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&pyra_driver);
if (retval)
- class_destroy(pyra_class);
+ class_unregister(&pyra_class);
return retval;
}
static void __exit pyra_exit(void)
{
hid_unregister_driver(&pyra_driver);
- class_destroy(pyra_class);
+ class_unregister(&pyra_class);
}
module_init(pyra_init);
diff --git a/drivers/hid/hid-roccat-ryos.c b/drivers/hid/hid-roccat-ryos.c
index 0eb17a3b925d..57714a4525e2 100644
--- a/drivers/hid/hid-roccat-ryos.c
+++ b/drivers/hid/hid-roccat-ryos.c
@@ -28,8 +28,6 @@ struct ryos_report_special {
uint8_t data[4];
} __packed;
-static struct class *ryos_class;
-
ROCCAT_COMMON2_BIN_ATTRIBUTE_W(control, 0x04, 0x03);
ROCCAT_COMMON2_BIN_ATTRIBUTE_RW(profile, 0x05, 0x03);
ROCCAT_COMMON2_BIN_ATTRIBUTE_RW(keys_primary, 0x06, 0x7d);
@@ -80,6 +78,11 @@ static const struct attribute_group *ryos_groups[] = {
NULL,
};
+static const struct class ryos_class = {
+ .name = "ryos",
+ .dev_groups = ryos_groups,
+};
+
static int ryos_init_specials(struct hid_device *hdev)
{
struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
@@ -106,7 +109,7 @@ static int ryos_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(ryos_class, hdev,
+ retval = roccat_connect(&ryos_class, hdev,
sizeof(struct ryos_report_special));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
@@ -216,21 +219,20 @@ static int __init ryos_init(void)
{
int retval;
- ryos_class = class_create("ryos");
- if (IS_ERR(ryos_class))
- return PTR_ERR(ryos_class);
- ryos_class->dev_groups = ryos_groups;
+ retval = class_register(&ryos_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&ryos_driver);
if (retval)
- class_destroy(ryos_class);
+ class_unregister(&ryos_class);
return retval;
}
static void __exit ryos_exit(void)
{
hid_unregister_driver(&ryos_driver);
- class_destroy(ryos_class);
+ class_unregister(&ryos_class);
}
module_init(ryos_init);
diff --git a/drivers/hid/hid-roccat-savu.c b/drivers/hid/hid-roccat-savu.c
index 93be7acef673..2baa47a0efc5 100644
--- a/drivers/hid/hid-roccat-savu.c
+++ b/drivers/hid/hid-roccat-savu.c
@@ -22,8 +22,6 @@
#include "hid-roccat-common.h"
#include "hid-roccat-savu.h"
-static struct class *savu_class;
-
ROCCAT_COMMON2_BIN_ATTRIBUTE_W(control, 0x4, 0x03);
ROCCAT_COMMON2_BIN_ATTRIBUTE_RW(profile, 0x5, 0x03);
ROCCAT_COMMON2_BIN_ATTRIBUTE_RW(general, 0x6, 0x10);
@@ -52,6 +50,11 @@ static const struct attribute_group *savu_groups[] = {
NULL,
};
+static const struct class savu_class = {
+ .name = "savu",
+ .dev_groups = savu_groups,
+};
+
static int savu_init_specials(struct hid_device *hdev)
{
struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
@@ -78,7 +81,7 @@ static int savu_init_specials(struct hid_device *hdev)
goto exit_free;
}
- retval = roccat_connect(savu_class, hdev,
+ retval = roccat_connect(&savu_class, hdev,
sizeof(struct savu_roccat_report));
if (retval < 0) {
hid_err(hdev, "couldn't init char dev\n");
@@ -204,21 +207,20 @@ static int __init savu_init(void)
{
int retval;
- savu_class = class_create("savu");
- if (IS_ERR(savu_class))
- return PTR_ERR(savu_class);
- savu_class->dev_groups = savu_groups;
+ retval = class_register(&savu_class);
+ if (retval)
+ return retval;
retval = hid_register_driver(&savu_driver);
if (retval)
- class_destroy(savu_class);
+ class_unregister(&savu_class);
return retval;
}
static void __exit savu_exit(void)
{
hid_unregister_driver(&savu_driver);
- class_destroy(savu_class);
+ class_unregister(&savu_class);
}
module_init(savu_init);
diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 6da80e442fdd..c7f7562e22e5 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -295,7 +295,7 @@ EXPORT_SYMBOL_GPL(roccat_report_event);
* Return value is minor device number in Range [0, ROCCAT_MAX_DEVICES] on
* success, a negative error code on failure.
*/
-int roccat_connect(struct class *klass, struct hid_device *hid, int report_size)
+int roccat_connect(const struct class *klass, struct hid_device *hid, int report_size)
{
unsigned int minor;
struct roccat_device *device;
diff --git a/include/linux/hid-roccat.h b/include/linux/hid-roccat.h
index 3214fb0815fc..753654fff07f 100644
--- a/include/linux/hid-roccat.h
+++ b/include/linux/hid-roccat.h
@@ -16,7 +16,7 @@
#ifdef __KERNEL__
-int roccat_connect(struct class *klass, struct hid_device *hid,
+int roccat_connect(const struct class *klass, struct hid_device *hid,
int report_size);
void roccat_disconnect(int minor);
int roccat_report_event(int minor, u8 const *data);
--
2.41.0
^ permalink raw reply related
* [PATCH 2/2] HID: hidraw: make hidraw_class structure const
From: Greg Kroah-Hartman @ 2023-06-20 18:31 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Greg Kroah-Hartman, Jiri Kosina, Benjamin Tissoires,
Ivan Orlov
In-Reply-To: <20230620183141.681353-3-gregkh@linuxfoundation.org>
Now that the driver core allows for struct class to be in read-only
memory, making all 'class' structures to be declared at build time
placing them into read-only memory, instead of having to be dynamically
allocated at load time.
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: linux-input@vger.kernel.org
Cc: Ivan Orlov <ivan.orlov0322@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hidraw.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 93e62b161501..d44e7a052584 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -32,7 +32,9 @@
static int hidraw_major;
static struct cdev hidraw_cdev;
-static struct class *hidraw_class;
+static const struct class hidraw_class = {
+ .name = "hidraw",
+};
static struct hidraw *hidraw_table[HIDRAW_MAX_DEVICES];
static DECLARE_RWSEM(minors_rwsem);
@@ -324,7 +326,7 @@ static void drop_ref(struct hidraw *hidraw, int exists_bit)
hid_hw_close(hidraw->hid);
wake_up_interruptible(&hidraw->wait);
}
- device_destroy(hidraw_class,
+ device_destroy(&hidraw_class,
MKDEV(hidraw_major, hidraw->minor));
} else {
--hidraw->open;
@@ -564,7 +566,7 @@ int hidraw_connect(struct hid_device *hid)
goto out;
}
- dev->dev = device_create(hidraw_class, &hid->dev, MKDEV(hidraw_major, minor),
+ dev->dev = device_create(&hidraw_class, &hid->dev, MKDEV(hidraw_major, minor),
NULL, "%s%d", "hidraw", minor);
if (IS_ERR(dev->dev)) {
@@ -618,11 +620,9 @@ int __init hidraw_init(void)
hidraw_major = MAJOR(dev_id);
- hidraw_class = class_create("hidraw");
- if (IS_ERR(hidraw_class)) {
- result = PTR_ERR(hidraw_class);
+ result = class_register(&hidraw_class);
+ if (result)
goto error_cdev;
- }
cdev_init(&hidraw_cdev, &hidraw_ops);
result = cdev_add(&hidraw_cdev, dev_id, HIDRAW_MAX_DEVICES);
@@ -634,7 +634,7 @@ int __init hidraw_init(void)
return result;
error_class:
- class_destroy(hidraw_class);
+ class_unregister(&hidraw_class);
error_cdev:
unregister_chrdev_region(dev_id, HIDRAW_MAX_DEVICES);
goto out;
@@ -645,7 +645,7 @@ void hidraw_exit(void)
dev_t dev_id = MKDEV(hidraw_major, 0);
cdev_del(&hidraw_cdev);
- class_destroy(hidraw_class);
+ class_unregister(&hidraw_class);
unregister_chrdev_region(dev_id, HIDRAW_MAX_DEVICES);
}
--
2.41.0
^ permalink raw reply related
* Re: [PATCH v2 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-20 18:38 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZIvTfE5O31NKnywd@nixie71>
On 16/06/2023 05:14, Jeff LaBundy wrote:
> Hi Neil,
>
> Great work; this driver cleaned up quite nicely. With the previous
> noise out of the way, I have left some more detailed comments which
> are relatively minor for the most part.
>
> On Thu, Jun 15, 2023 at 12:27:01PM +0200, Neil Armstrong wrote:
>> Add initial support for the new Goodix "Berlin" touchscreen ICs.
>>
>> These touchscreen ICs support SPI, I2C and I3C interface, up to
>> 10 finger touch, stylus and gestures events.
>>
>> This initial driver is derived from the Goodix goodix_ts_berlin
>> available at [1] and [2] and only supports the GT9916 IC
>> present on the Qualcomm SM8550 MTP & QRD touch panel.
>>
>> The current implementation only supports BerlinD, aka GT9916.
>>
>> Support for advanced features like:
>> - Firmware & config update
>> - Stylus events
>> - Gestures events
>> - Previous revisions support (BerlinA or BerlinB)
>> is not included in current version.
>>
>> The current support will work with currently flashed firmware
>> and config, and bail out if firmware or config aren't flashed yet.
>>
>> [1] https://github.com/goodix/goodix_ts_berlin
>> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>> drivers/input/touchscreen/Kconfig | 5 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/goodix_berlin.h | 178 +++++++
>> drivers/input/touchscreen/goodix_berlin_core.c | 681 +++++++++++++++++++++++++
>> 4 files changed, 865 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index c2cbd332af1d..1a6f6f6da991 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -416,6 +416,11 @@ config TOUCHSCREEN_GOODIX
>> To compile this driver as a module, choose M here: the
>> module will be called goodix.
>>
>> +config TOUCHSCREEN_GOODIX_BERLIN_CORE
>> + depends on GPIOLIB || COMPILE_TEST
>
> No need to depend on GPIOLIB; all gpiod calls used in this driver define dummy
> functions for the !CONFIG_GPIOLIB case.
Ack
>
>> + depends on REGMAP
>> + tristate
>> +
>> config TOUCHSCREEN_HIDEEP
>> tristate "HiDeep Touch IC"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..29cdb042e104 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
>> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
>> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
>> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
>> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
>> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
>> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
>> diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
>> new file mode 100644
>> index 000000000000..56c110d94dff
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin.h
>> @@ -0,0 +1,178 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Goodix Touchscreen Driver
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_berlin_berlin driver.
>> + */
>> +
>> +#ifndef __GOODIX_BERLIN_H_
>> +#define __GOODIX_BERLIN_H_
>> +
>> +#include <linux/input.h>
>> +#include <linux/of_gpio.h>
>
> I believe this should have been <linux/gpio/consumer.h>.
Ack
>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#define GOODIX_MAX_TOUCH 10
>> +
>> +#define GOODIX_NORMAL_RESET_DELAY_MS 100
>> +
>> +#define MAX_SCAN_FREQ_NUM 8
>> +#define MAX_SCAN_RATE_NUM 8
>> +#define MAX_FREQ_NUM_STYLUS 8
>> +
>> +#define IRQ_EVENT_HEAD_LEN 8
>> +#define BYTES_PER_POINT 8
>> +#define COOR_DATA_CHECKSUM_SIZE 2
>> +
>> +#define GOODIX_TOUCH_EVENT BIT(7)
>> +#define GOODIX_REQUEST_EVENT BIT(6)
>> +
>> +#define GOODIX_REQUEST_CODE_RESET 3
>> +
>> +#define POINT_TYPE_STYLUS_HOVER 0x01
>> +#define POINT_TYPE_STYLUS 0x03
>> +
>> +#define DEV_CONFIRM_VAL 0xAA
>> +#define BOOTOPTION_ADDR 0x10000
>> +#define FW_VERSION_INFO_ADDR 0x10014
>> +
>> +#define GOODIX_IC_INFO_MAX_LEN 1024
>> +#define GOODIX_IC_INFO_ADDR 0x10070
>> +
>> +struct goodix_berlin_fw_version {
>> + u8 rom_pid[6];
>> + u8 rom_vid[3];
>> + u8 rom_vid_reserved;
>> + u8 patch_pid[8];
>> + u8 patch_vid[4];
>> + u8 patch_vid_reserved;
>> + u8 sensor_id;
>> + u8 reserved[2];
>> + u16 checksum;
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_version {
>> + u8 info_customer_id;
>> + u8 info_version_id;
>> + u8 ic_die_id;
>> + u8 ic_version_id;
>> + u32 config_id;
>> + u8 config_version;
>> + u8 frame_data_customer_id;
>> + u8 frame_data_version_id;
>> + u8 touch_data_customer_id;
>> + u8 touch_data_version_id;
>> + u8 reserved[3];
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_feature {
>> + u16 freqhop_feature;
>> + u16 calibration_feature;
>> + u16 gesture_feature;
>> + u16 side_touch_feature;
>> + u16 stylus_feature;
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_misc {
>> + u32 cmd_addr;
>> + u16 cmd_max_len;
>> + u32 cmd_reply_addr;
>> + u16 cmd_reply_len;
>> + u32 fw_state_addr;
>> + u16 fw_state_len;
>> + u32 fw_buffer_addr;
>> + u16 fw_buffer_max_len;
>> + u32 frame_data_addr;
>> + u16 frame_data_head_len;
>> + u16 fw_attr_len;
>> + u16 fw_log_len;
>> + u8 pack_max_num;
>> + u8 pack_compress_version;
>> + u16 stylus_struct_len;
>> + u16 mutual_struct_len;
>> + u16 self_struct_len;
>> + u16 noise_struct_len;
>> + u32 touch_data_addr;
>> + u16 touch_data_head_len;
>> + u16 point_struct_len;
>> + u16 reserved1;
>> + u16 reserved2;
>> + u32 mutual_rawdata_addr;
>> + u32 mutual_diffdata_addr;
>> + u32 mutual_refdata_addr;
>> + u32 self_rawdata_addr;
>> + u32 self_diffdata_addr;
>> + u32 self_refdata_addr;
>> + u32 iq_rawdata_addr;
>> + u32 iq_refdata_addr;
>> + u32 im_rawdata_addr;
>> + u16 im_readata_len;
>> + u32 noise_rawdata_addr;
>> + u16 noise_rawdata_len;
>> + u32 stylus_rawdata_addr;
>> + u16 stylus_rawdata_len;
>> + u32 noise_data_addr;
>> + u32 esd_addr;
>> +} __packed;
>> +
>> +enum goodix_berlin_ts_event_type {
>> + GOODIX_BERLIN_EVENT_INVALID,
>> + GOODIX_BERLIN_EVENT_TOUCH,
>> + GOODIX_BERLIN_EVENT_REQUEST,
>> +};
>> +
>> +enum goodix_berlin_ts_request_type {
>> + GOODIX_BERLIN_REQUEST_TYPE_INVALID,
>> + GOODIX_BERLIN_REQUEST_TYPE_RESET,
>> +};
>> +
>> +enum goodix_berlin_touch_point_status {
>> + GOODIX_BERLIN_TS_NONE,
>> + GOODIX_BERLIN_TS_TOUCH,
>> +};
>> +
>> +struct goodix_berlin_coords {
>> + enum goodix_berlin_touch_point_status status;
>> + unsigned int x, y, w, p;
>> +};
>> +
>> +struct goodix_berlin_touch_data {
>> + int touch_num;
>> + struct goodix_berlin_coords coords[GOODIX_MAX_TOUCH];
>> +};
>> +
>> +struct goodix_berlin_event {
>> + enum goodix_berlin_ts_event_type event_type;
>> + enum goodix_berlin_ts_request_type request_code;
>> + struct goodix_berlin_touch_data touch_data;
>> +};
>> +
>> +/* Runtime parameters extracted from goodix_berlin_ic_info */
>> +struct goodix_berlin_params {
>> + u32 touch_data_addr;
>> +};
>
> Is there any reason to wrap this single member in a struct? It seems like
> touch_data_addr can simply be a member of struct goodix_berlin_core; this
> would shorten references to it throughout as well.
I did it first but then moved it in a separate struct for future additions,
but I'll move it in the core struct.
>
>> +
>> +struct goodix_berlin_core {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct regulator *avdd;
>> + struct regulator *iovdd;
>> + struct gpio_desc *reset_gpio;
>> + struct touchscreen_properties props;
>> + struct goodix_berlin_fw_version fw_version;
>> + struct goodix_berlin_params params;
>> + struct input_dev *input_dev;
>> + struct goodix_berlin_event ts_event;
>> + int irq;
>> + struct dentry *debugfs_root;
>
> This last member appears to be unused.
Oops, forgot this one.
>
>> +};
>> +
>> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>> + struct regmap *regmap);
>> +
>> +extern const struct dev_pm_ops goodix_berlin_pm_ops;
>> +
>> +#endif
>> diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
>> new file mode 100644
>> index 000000000000..11788662722a
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin_core.c
>> @@ -0,0 +1,681 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Goodix Touchscreen Driver
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_ts_berlin driver.
>> + */
>> +#include <linux/input/mt.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "goodix_berlin.h"
>> +
>> +/*
>> + * Goodix "Berlin" Touchscreen ID driver
>> + *
>> + * Currently only handles Multitouch events with already
>> + * programmed firmware and "config" for "Revision D" Berlin IC.
>> + *
>> + * Support is missing for:
>> + * - ESD Management
>> + * - Firmware update/flashing
>> + * - "Config" update/flashing
>> + * - Stylus Events
>> + * - Gesture Events
>> + * - Support for older revisions (A, B & C)
>> + */
>> +
>> +static bool goodix_berlin_check_checksum(const u8 *data, int size)
>
> Based on how this function is used later, something along the lines of
> goodix_berlin_checksum_valid() may be more descriptive.
Ack
>
>> +{
>> + u32 cal_checksum = 0;
>> + u32 r_checksum = 0;
>
> This can probably just be:
>
> u16 r_checksum;
>
>> + u32 i;
>> +
>> + if (size < COOR_DATA_CHECKSUM_SIZE)
>> + return false;
>> +
>> + for (i = 0; i < size - COOR_DATA_CHECKSUM_SIZE; i++)
>> + cal_checksum += data[i];
>> +
>> + r_checksum += data[i++];
>> + r_checksum += (data[i] << 8);
>
> And then:
>
> r_checksum = put_unaligned_le16(data[i]);
>
> In which case you must #include <asm/unaligned.h>.
Ack
>
>> +
>> + return (cal_checksum & 0xFFFF) == r_checksum;
>> +}
>> +
>> +static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
>> + const u8 *data, int size)
>> +{
>> + int zero_count = 0;
>> + int ff_count = 0;
>
> 'zero_count' and 'ff_count' seem like odd variable names; the following
> seems cleaner:
>
> static bool goodix_berlin_is_dummy_data(...)
> {
> int i;
>
> /*
> * 0 and 0xff represent ____, so declare success the first time
> * we encounter neither.
> */
> for (i = 0; i < size; i++)
> if (data[i] > 0 && data[i] < 0xff)
> return false;
>
> return true;
> }
>
> ...with the comment filled in to clarify what is the significance of dummy
> data, if possible. Note that the caller already prints a message when this
> fails.
Ack
>
>> + int i;
>> +
>> + for (i = 0; i < size; i++) {
>> + if (data[i] == 0)
>> + zero_count++;
>> + else if (data[i] == 0xff)
>> + ff_count++;
>> + }
>> + if (zero_count == size || ff_count == size) {
>> + dev_warn(cd->dev, "warning data is all %s\n",
>> + zero_count == size ? "zero" : "0xff");
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
>> +{
>> + u8 tx_buf[8], rx_buf[8];
>> + int retry = 3;
>> + int error;
>> +
>> + memset(tx_buf, DEV_CONFIRM_VAL, sizeof(tx_buf));
>
> Please namespace this #define as well as the one below.
Ack
>
>> + while (retry--) {
>> + error = regmap_raw_write(cd->regmap, BOOTOPTION_ADDR, tx_buf,
>> + sizeof(tx_buf));
>> + if (error < 0)
>> + return error;
>
> This should just be:
>
> if (error)
> return error;
>
>> +
>> + error = regmap_raw_read(cd->regmap, BOOTOPTION_ADDR, rx_buf,
>> + sizeof(rx_buf));
>> + if (error < 0)
>> + return error;
>
> And here, as well as a few other places throughout.
Ack will do a complete check
>
>> +
>> + if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
>> + return 0;
>> +
>> + usleep_range(5000, 5100);
>> + }
>> +
>> + dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
>> +{
>> + int error = 0;
>
> No need to initialize 'error' here.
Th refactor I did needs it to be initialized at 0 because the if() always calls return,
but yeah it's kind of ugly.
>
>> +
>> + if (on) {
>> + error = regulator_enable(cd->iovdd);
>> + if (error < 0) {
>> + dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
>> + return error;
>> + }
>> +
>> + /* Vendor waits 3ms for IOVDD to settle */
>> + usleep_range(3000, 3100);
>> +
>> + error = regulator_enable(cd->avdd);
>> + if (error < 0) {
>> + dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
>> + goto power_off_iovdd;
>> + }
>> +
>> + /* Vendor waits 15ms for IOVDD to settle */
>> + usleep_range(15000, 15100);
>> +
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + /* Vendor waits 4ms for Firmware to initialize */
>> + usleep_range(4000, 4100);
>> +
>> + error = goodix_berlin_dev_confirm(cd);
>> + if (error < 0)
>> + goto power_off_gpio;
>
> All of this cleaned up nicely. The following comment is idiomatic, but I feel
> the goto can be easily eliminated as follows:
>
> error = goodix_berlin_dev_confirm(cd);
> if (error)
> break;
Break ? in an if ?
>
> If you feel strongly otherwise, please consider a different label name beside
> 'power_off_gpio' as it is a bit confusing.
Ack
>
>> +
>> + /* Vendor waits 100ms for Firmware to fully boot */
>> + msleep(GOODIX_NORMAL_RESET_DELAY_MS);
>> +
>> + return 0;
>> + }
>> +
>> +power_off_gpio:
>> + gpiod_set_value(cd->reset_gpio, 1);
>> +
>> + regulator_disable(cd->avdd);
>> +
>> +power_off_iovdd:
>> + regulator_disable(cd->iovdd);
>> +
>> + return error;
>> +}
>> +
>> +static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
>> +{
>> + u8 buf[sizeof(struct goodix_berlin_fw_version)];
>> + int retry = 2;
>> + int error;
>> +
>> + while (retry--) {
>> + error = regmap_raw_read(cd->regmap, FW_VERSION_INFO_ADDR, buf, sizeof(buf));
>> + if (error) {
>> + dev_dbg(cd->dev, "read fw version: %d, retry %d\n", error, retry);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + if (goodix_berlin_check_checksum(buf, sizeof(buf)))
>> + break;
>> +
>> + dev_dbg(cd->dev, "invalid fw version: checksum error\n");
>> +
>> + error = -EINVAL;
>> +
>> + /* Do not sleep on the last try */
>> + if (retry)
>> + usleep_range(10000, 11000);
>
> This works, but do you reasonably expect to continue if the checksum is bad?
> Perhaps the device can still respond over I2C/SPI, but returns garbage data
> if the communication happens in the process of the device waking up?
Honestly I personnaly wouldn't retry, but the vendor code did this.
I personally would remove all the retries since the FW should respond
correctly at first time.
> If so, this may be even cleaner:
>
> if (retry)
> usleep_range(...);
> else
> error = -EINVAL;
>
>> + }
>> +
>> + if (error) {
>> + dev_err(cd->dev, "failed to get fw version");
>> + return error;
>> + }
>
> Again the following comment is idiomatic, but this seems cleaner:
>
> if (error)
> dev_err(...);
> else
> memcpy(...);
>
> return error;
>
> I do not feel strongly about it if you prefer the existing method.
>
>> +
>> + memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
>> +
>> + return 0;
>> +}
>> +
>> +/* Only extract necessary data for runtime */
>> +static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
>> + const u8 *data, u16 length)
>
> Nit: these arguments do not seem aligned as below:
Forgot this one...
>
> +static int goodix_berlin_convert_ic_info(...,
> ...)
>
>> +{
>> + struct goodix_berlin_ic_info_misc misc;
>> + unsigned int offset = 0;
>> + u8 param_num;
>> +
>> + offset += sizeof(__le16); /* length */
>> + offset += sizeof(struct goodix_berlin_ic_info_version);
>> + offset += sizeof(struct goodix_berlin_ic_info_feature);
>> +
>> + /* goodix_berlin_ic_info_param, variable width structure */
>
> I don't see any need to make up this name 'goodix_berlin_ic_info_param' which
> is not defined anywhere else; a generic text description of what this area of
> memory represents seems fine.
Ack
>
>> + offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* active_scan_rate_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>
> Do you actually need this check after every operation? It seems the error
> would be cumulative such that you could perform one check at the end, and
> potentially avoid a goto statement like below:
>
> offset += ...; /* foo */
>
> offset += ---; /* bar */
>
> if (offset > length) {
> dev_err(...);
> return -EINVAL);
> }
>
> return 0;
>
> In case I have misunderstood, please let me know.
Yes because the data is a follow-up of size followed by the elements,
so I need to read the size, to know where is the next size... and each
size can be garbage and lead to a buffer overflow.
>
>> +
>> + param_num = data[offset++]; /* mutual_freq_num*/
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* self_tx_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* self_rx_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* stylus_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset + sizeof(misc) > length)
>> + goto invalid_offset;
>> +
>> + /* goodix_berlin_ic_info_misc */
>> + memcpy(&misc, &data[offset], sizeof(misc));
>> +
>> + cd->params.touch_data_addr = le32_to_cpu(misc.touch_data_addr);
>> +
>> + return 0;
>> +
>> +invalid_offset:
>> + dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
>> + offset, length);
>> + return -EINVAL;
>> +}
>> +
>> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
>> +{
>> + int error, i;
>> + u16 length = 0;
>> + u32 ic_addr;
>> + u8 afe_data[GOODIX_IC_INFO_MAX_LEN] = { 0 };
>
> No need to initialize this array.
Ack
>
>> +
>> + ic_addr = GOODIX_IC_INFO_ADDR;
>> +
>> + for (i = 0; i < 3; i++) {
>> + error = regmap_raw_read(cd->regmap, ic_addr, (u8 *)&length, sizeof(length));
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + length = le16_to_cpu(length);
>
> This seems incorrect; it seems like you mean to initialize the following:
>
> __le16 length_raw;
> u16 length;
>
> And then this becomes:
>
> length = le16_to_cpu(length_raw);
>
>> + if (length >= GOODIX_IC_INFO_MAX_LEN) {
>> + dev_info(cd->dev, "invalid ic info length %d, retry %d\n", length, i);
>> + continue;
>> + }
>> +
>> + error = regmap_raw_read(cd->regmap, ic_addr, afe_data, length);
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + /* check whether the data is valid (ex. bus default values) */
>> + if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
>> + dev_info(cd->dev, "fw info data invalid\n");
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + if (!goodix_berlin_check_checksum((const uint8_t *)afe_data, length)) {
>> + dev_info(cd->dev, "fw info checksum error\n");
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + break;
>> + }
>
> Even after the loop gives up, we will still sleep for 5 ms; it seems we can
> simplify this. One option may be to offload the function calls into a helper
> like below:
>
> static int __goodix_berlin_get_ic_info(...);
> {
> /* ... */
>
> error = regmap_raw_read(...);
> if (error)
> return error;
>
> length = le16_to_cpu(...);
> if (...)
> return -EINVAL;
>
> if (goodix_berlin_is_dummy_data(...))
> return -EAGAIN;
>
> if (!goodix_berlin_check_checksum(...));
> return -EAGAIN;
>
> return 0;
> }
>
> ...and then:
>
> static int goodix_berlin_get_ic_info(...)
> {
> /* ... */
>
> for (i = 0; i = GOODIX_BERLIN_INFO_RETRIES; i++) {
> error = __goodix_berlin_get_ic_info(...);
> if (!error)
> break;
> elseif (error == -EINVAL)
> return error;
>
> usleep_range(...);
> }
>
> if (i == GOODIX_BERLIN_INFO_RETRIES) {
> dev_err(...);
> return -ETIMEDOUT;
> }
>
> /* ... */
>
> }
This looks better, I'll see what inspire me the most
>
>> + if (i == 3) {
>> + dev_err(cd->dev, "failed get ic info\n");
>> + return -EINVAL;
>> + }
>> +
>> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
>> + if (error) {
>> + dev_err(cd->dev, "error converting ic info\n");
>> + return error;
>> + }
>> +
>> + /* check some key info */
>> + if (!cd->params.touch_data_addr) {
>> + dev_err(cd->dev, "touch_data_addr is null\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_after_event_handler(struct goodix_berlin_core *cd)
>> +{
>> + u8 sync_clean = 0;
>> +
>> + return regmap_raw_write(cd->regmap, cd->params.touch_data_addr, &sync_clean, 1);
>
> Since your register data width is 8 bits, can this not simply be:
>
> regmap_write(cd->regmap, params.touch_data_addr, 0);
Indeed, let me check
>
>> +}
>> +
>> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
>> + struct goodix_berlin_touch_data *touch_data,
>> + u8 *buf, int touch_num)
>> +{
>> + unsigned int id = 0, x = 0, y = 0, w = 0;
>
> No need to initialize these.
>
>> + u8 *coor_data;
>> + int i;
>> +
>> + coor_data = &buf[IRQ_EVENT_HEAD_LEN];
>> +
>> + for (i = 0; i < touch_num; i++) {
>> + id = (coor_data[0] >> 4) & 0x0F;
>> +
>> + if (id >= GOODIX_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid finger id %d\n", id);
>> +
>> + touch_data->touch_num = 0;
>> + return;
>> + }
>
> It does not seem this check needs to happen in the loop.
>
>> +
>> + x = le16_to_cpup((__le16 *)(coor_data + 2));
>> + y = le16_to_cpup((__le16 *)(coor_data + 4));
>> + w = le16_to_cpup((__le16 *)(coor_data + 6));
>
> I got a bit lost here; this almost seems like there is an opportunity
> to define the coordinate layout as a __packed struct.
>
> It's also a bit confusing that all of this information is stored in
> the driver's private data and then passed around multiple functions.
> Please consider whether this logic can be simplified by consolidating
> it into one homogenous interrupt handler.
Yep, the whole interrupt handling needs a refactoring to simplify it,
there's no need to convert those data and pass them back and forth.
>
>> +
>> + touch_data->coords[id].status = GOODIX_BERLIN_TS_TOUCH;
>> + touch_data->coords[id].x = x;
>> + touch_data->coords[id].y = y;
>> + touch_data->coords[id].w = w;
>> +
>> + coor_data += BYTES_PER_POINT;
>> + }
>> +
>> + touch_data->touch_num = touch_num;
>> +}
>> +
>> +static int goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
>> + u8 *pre_buf, u32 pre_buf_len)
>> +{
>> + static u8 buffer[IRQ_EVENT_HEAD_LEN + BYTES_PER_POINT * GOODIX_MAX_TOUCH + 2];
>> + struct goodix_berlin_touch_data *touch_data = &cd->ts_event.touch_data;
>> + u8 point_type = 0;
>> + u8 touch_num = 0;
>> + int error = 0;
>> +
>> + memset(&cd->ts_event, 0, sizeof(cd->ts_event));
>> +
>> + /* copy pre-data to buffer */
>> + memcpy(buffer, pre_buf, pre_buf_len);
>> +
>> + touch_num = buffer[2] & 0x0F;
>> +
>> + if (touch_num > GOODIX_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
>> + return -EINVAL;
>> + }
>> +
>> + /* read more data if more than 2 touch events */
>> + if (unlikely(touch_num > 2)) {
>> + error = regmap_raw_read(cd->regmap,
>> + cd->params.touch_data_addr + pre_buf_len,
>> + &buffer[pre_buf_len],
>> + (touch_num - 2) * BYTES_PER_POINT);
>> + if (error) {
>> + dev_err(cd->dev, "failed get touch data\n");
>> + return error;
>> + }
>> + }
>> +
>> + goodix_berlin_after_event_handler(cd);
>> +
>> + if (!touch_num)
>> + goto out_touch_handler;
>> +
>> + point_type = buffer[IRQ_EVENT_HEAD_LEN] & 0x0F;
>> +
>> + if (point_type == POINT_TYPE_STYLUS || point_type == POINT_TYPE_STYLUS_HOVER) {
>> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
>> + return 0;
>> + }
>> +
>> + if (!goodix_berlin_check_checksum(&buffer[IRQ_EVENT_HEAD_LEN],
>> + touch_num * BYTES_PER_POINT + 2)) {
>> + dev_dbg(cd->dev, "touch data checksum error\n");
>> + dev_dbg(cd->dev, "data: %*ph\n",
>> + touch_num * BYTES_PER_POINT + 2, &buffer[IRQ_EVENT_HEAD_LEN]);
>> + return -EINVAL;
>> + }
>> +
>> +out_touch_handler:
>> + cd->ts_event.event_type = GOODIX_BERLIN_EVENT_TOUCH;
>> + goodix_berlin_parse_finger(cd, touch_data, buffer, touch_num);
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_event_handler(struct goodix_berlin_core *cd)
>> +{
>> + int pre_read_len;
>> + u8 pre_buf[32];
>> + u8 event_status;
>> + int error;
>> +
>> + pre_read_len = IRQ_EVENT_HEAD_LEN + BYTES_PER_POINT * 2 +
>> + COOR_DATA_CHECKSUM_SIZE;
>> +
>> + error = regmap_raw_read(cd->regmap, cd->params.touch_data_addr, pre_buf,
>> + pre_read_len);
>> + if (error) {
>> + dev_err(cd->dev, "failed get event head data\n");
>> + return error;
>> + }
>> +
>> + if (pre_buf[0] == 0x00)
>> + return -EINVAL;
>> +
>> + if (!goodix_berlin_check_checksum(pre_buf, IRQ_EVENT_HEAD_LEN)) {
>> + dev_warn(cd->dev, "touch head checksum err : %*ph\n",
>> + IRQ_EVENT_HEAD_LEN, pre_buf);
>> + return -EINVAL;
>> + }
>> +
>> + event_status = pre_buf[0];
>> + if (event_status & GOODIX_TOUCH_EVENT)
>> + return goodix_berlin_touch_handler(cd, pre_buf, pre_read_len);
>> +
>> + if (event_status & GOODIX_REQUEST_EVENT) {
>> + cd->ts_event.event_type = GOODIX_BERLIN_EVENT_REQUEST;
>> + if (pre_buf[2] == GOODIX_REQUEST_CODE_RESET)
>> + cd->ts_event.request_code = GOODIX_BERLIN_REQUEST_TYPE_RESET;
>> + else
>> + dev_warn(cd->dev, "unsupported request code 0x%x\n", pre_buf[2]);
>> + }
>> +
>> + goodix_berlin_after_event_handler(cd);
>> +
>> + return 0;
>> +}
>> +
>> +static void goodix_berlin_report_finger(struct goodix_berlin_core *cd)
>> +{
>> + struct goodix_berlin_touch_data *touch_data = &cd->ts_event.touch_data;
>> + int i;
>> +
>> + mutex_lock(&cd->input_dev->mutex);
>
> I do not see any need for a mutex here; this function is only ever called from
> a threaded handler declared with IRQF_ONESHOT; it cannot become re-entrant. In
> case I have misunderstood, please let me know.
Ack
>
>> +
>> + for (i = 0; i < GOODIX_MAX_TOUCH; i++) {
>> + bool pressed = touch_data->coords[i].status == GOODIX_BERLIN_TS_TOUCH;
>> +
>> + input_mt_slot(cd->input_dev, i);
>> + input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, pressed);
>> +
>> + if (touch_data->coords[i].status == GOODIX_BERLIN_TS_TOUCH) {
>> + dev_dbg(cd->dev, "report: id[%d], x %d, y %d, w %d\n", i,
>> + touch_data->coords[i].x,
>> + touch_data->coords[i].y,
>> + touch_data->coords[i].w);
>> +
>> + touchscreen_report_pos(cd->input_dev, &cd->props,
>> + touch_data->coords[i].x,
>> + touch_data->coords[i].y, true);
>> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
>> + touch_data->coords[i].w);
>> + }
>> + }
>> +
>> + input_mt_sync_frame(cd->input_dev);
>> + input_sync(cd->input_dev);
>> +
>> + mutex_unlock(&cd->input_dev->mutex);
>> +}
>> +
>> +static int goodix_berlin_request_handle(struct goodix_berlin_core *cd)
>> +{
>> + /* TOFIX: Handle more request codes */
>> + if (cd->ts_event.request_code != GOODIX_BERLIN_REQUEST_TYPE_RESET) {
>> + dev_info(cd->dev, "can't handle request type 0x%x\n",
>> + cd->ts_event.request_code);
>> + return -EINVAL;
>> + }
>> +
>> + gpiod_set_value(cd->reset_gpio, 1);
>> + usleep_range(2000, 2100);
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + msleep(GOODIX_NORMAL_RESET_DELAY_MS);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> + int error;
>> +
>> + error = goodix_berlin_event_handler(cd);
>> + if (likely(!error)) {
>> + switch (cd->ts_event.event_type) {
>> + case GOODIX_BERLIN_EVENT_TOUCH:
>> + goodix_berlin_report_finger(cd);
>> + break;
>> +
>> + case GOODIX_BERLIN_EVENT_REQUEST:
>> + goodix_berlin_request_handle(cd);
>> + break;
>> +
>> + /* TOFIX: Handle more request types */
>> + case GOODIX_BERLIN_EVENT_INVALID:
>> + break;
>> + }
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
>> + const struct input_id *id)
>> +{
>> + struct input_dev *input_dev;
>> + int error;
>> +
>> + input_dev = devm_input_allocate_device(cd->dev);
>> + if (!input_dev)
>> + return -ENOMEM;
>> +
>> + cd->input_dev = input_dev;
>> + input_set_drvdata(input_dev, cd);
>> +
>> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
>> + input_dev->phys = "input/ts";
>> + input_dev->dev.parent = cd->dev;
>
> This last line is not necessary as devm_input_allocate_device() already
> does it for us.
>
>> +
>> + memcpy(&input_dev->id, id, sizeof(*id));
>
> I do not think deep copy is necessary here; please let me know in case I
> have misunderstood.
Hmm, ok yeah
>
>> +
>> + /* Set input parameters */
>
> This comment is not necessary.
>
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> +
>> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
>> +
>> + error = input_mt_init_slots(cd->input_dev, GOODIX_MAX_TOUCH, INPUT_MT_DIRECT);
>> + if (error)
>> + return error;
>> +
>> + return input_register_device(cd->input_dev);
>> +}
>> +
>> +static int goodix_berlin_pm_suspend(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> +
>> + disable_irq(cd->irq);
>> +
>> + return goodix_berlin_power_on(cd, false);
>> +}
>> +
>> +static int goodix_berlin_pm_resume(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> + int error;
>> +
>> + error = goodix_berlin_power_on(cd, true);
>> + if (error)
>> + return error;
>> +
>> + enable_irq(cd->irq);
>> +
>> + return 0;
>> +}
>> +
>> +EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
>> + goodix_berlin_pm_suspend,
>> + goodix_berlin_pm_resume);
>> +
>> +static void goodix_berlin_power_off(void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> +
>> + goodix_berlin_power_on(cd, false);
>> +}
>> +
>> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>> + struct regmap *regmap)
>> +{
>> + struct goodix_berlin_core *cd;
>> + int error;
>> +
>> + if (irq <= 0)
>> + return -EINVAL;
>
> Please include a dev_err() here.
Ack
>
>> +
>> + cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
>> + if (!cd)
>> + return -ENOMEM;
>> +
>> + cd->dev = dev;
>> + cd->regmap = regmap;
>> + cd->irq = irq;
>> +
>> + cd->reset_gpio = devm_gpiod_get_optional(cd->dev, "reset", GPIOF_OUT_INIT_HIGH);
>
> You can simply use 'dev' instead of 'cd->dev' here and throughout.
Ack
>
>> + if (IS_ERR(cd->reset_gpio))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->reset_gpio),
>> + "Failed to request reset gpio\n");
>> +
>> + cd->avdd = devm_regulator_get(cd->dev, "avdd");
>> + if (IS_ERR(cd->avdd))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->avdd),
>> + "Failed to request avdd regulator\n");
>> +
>> + cd->iovdd = devm_regulator_get(cd->dev, "iovdd");
>> + if (IS_ERR(cd->iovdd))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->iovdd),
>> + "Failed to request iovdd regulator\n");
>> +
>> + error = goodix_berlin_input_dev_config(cd, id);
>> + if (error < 0) {
>> + dev_err(cd->dev, "failed set input device");
>> + return error;
>> + }
>
> I recommend calling this after goodix_berlin_get_ic_info(); no need to go
> through all this trouble if the device is not there.
Ack
>
>> +
>> + error = devm_request_threaded_irq(dev, irq, NULL,
>> + goodix_berlin_threadirq_func,
>> + IRQF_ONESHOT, "goodix-berlin", cd);
>> + if (error) {
>> + dev_err(dev, "request threaded irq failed: %d\n", error);
>> + return error;
>> + }
>
> Typically, the interrupt should be the last resource to be registered. At
> this stage the device is not powered and the IRQ pin may be in an invalid
> state.
Ack
>
>> +
>> + dev_set_drvdata(dev, cd);
>> +
>> + error = goodix_berlin_power_on(cd, true);
>> + if (error) {
>> + dev_err(cd->dev, "failed power on");
>> + return error;
>> + }
>> +
>> + error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
>> + if (error)
>> + return error;
>> +
>> + error = goodix_berlin_read_version(cd);
>> + if (error < 0) {
>
> if (error)
>
>> + dev_err(cd->dev, "failed to get version info");
>> + return error;
>> + }
>> +
>> + error = goodix_berlin_get_ic_info(cd);
>> + if (error) {
>> + dev_err(cd->dev, "invalid ic info, abort");
>> + return error;
>> + }
>> +
>> + dev_dbg(cd->dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(goodix_berlin_probe);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>
>> --
>> 2.34.1
>>
>
> Kind regards,
> Jeff LaBundy
Thanks for the in-depth review !
Neil
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Limonciello, Mario @ 2023-06-20 18:50 UTC (permalink / raw)
To: Linux regressions mailing list, Malte Starostik,
Benjamin Tissoires
Cc: Bagas Sanjaya, basavaraj.natikar, linux-input, linux, stable
In-Reply-To: <1b3fd148-44d7-d476-e9e6-f9d8c8ec0ee6@leemhuis.info>
>>>> I have a suspicion on commit 7bcfdab3f0c6 ("HID: amd_sfh: if no sensors
>>>> are enabled, clean up") because the stack trace says that there is a bad
>>>> list_add, which could happen if the object is not correctly initialized.
>>>>
>>>> However, that commit was present in v6.2, so it might not be that one.
>>>>
>>> If I'm not mistaken the Z13 doesn't actually have any
>>> sensors connected to SFH. So I think the suspicion on
>>> 7bcfdab3f0c6 and theory this is triggered by HID init makes
>>> a lot of sense.
>>>
>>> Can you try this patch?
>>>
>>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>>> b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>>> index d9b7b01900b5..fa693a5224c6 100644
>>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>>> @@ -324,6 +324,7 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev
>>> *privdata)
>>> devm_kfree(dev, cl_data->report_descr[i]);
>>> }
>>> dev_warn(dev, "Failed to discover, sensors not enabled
>>> is %d\n", cl_data->is_any_sensor_enabled);
>>> + cl_data->num_hid_devices = 0;
>>> return -EOPNOTSUPP;
>>> }
>>> schedule_delayed_work(&cl_data->work_buffer,
>>> msecs_to_jiffies(AMD_SFH_IDLE_LOOP));
>> I applied this to 9e87b63ed37e202c77aa17d4112da6ae0c7c097c now, which was the
>> origin when I started the whole bisection. Clean rebuild, issue still
>> persists.
>>
>> Out of 50 boots, I got:
>>
>> 25 clean
>> 22 Oops as posted by the OP
>> 1 same Oops, followed by a panic
>> 1 lockup [1]
>> 1 hanging with just a blank screen
>>
>> Not sure whether the lockups are related, but [1] mentions modprobe and udev-
>> worker as well and all problems including the blank screen one appear roughly
>> at the same time during boot. As this is before a graphics mode switch, I
>> suspect the last mentioned case may be like [1] while the screen was blanked.
>> To support the timing correlation: the UVC error for the IR cam shown in the
>> photo (normal boot noise) also appears right before the BUG in the non-lockup
>> bad case.
>>
>> I do see the dev_warn in dmesg, so the code path modified in your patch is
>> indeed hit:
>> [ 10.897521] pcie_mp2_amd 0000:63:00.7: Failed to discover, sensors not
>> enabled is 1
>> [ 10.897533] pcie_mp2_amd: probe of 0000:63:00.7 failed with error -95
>>
>> BR Malte
>>
>> [1] https://photos.app.goo.gl/2FAvQ7DqBsHEF6Bd8
Apologies; for some reason I never got that above reply in my inbox,
some server along the way might have deemed it spam.
Anyways; I just double checked the Z13 I have on my hand. I don't
have the PCI device for SFH (1022:164a) present on the system.
Can you please double check you are on the latest BIOS?
I'm on the latest release from LVFS, 0.1.57 according to fwupdmgr.
^ permalink raw reply
* Re: [PATCH v2 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-20 19:35 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <a4f36bef-7c2c-08f4-bf93-fe1f0f1d315e@linaro.org>
Hi Neil,
On Tue, Jun 20, 2023 at 08:38:20PM +0200, Neil Armstrong wrote:
[...]
> > > +static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
> > > +{
> > > + int error = 0;
> >
> > No need to initialize 'error' here.
>
> Th refactor I did needs it to be initialized at 0 because the if() always calls return,
> but yeah it's kind of ugly.
Ah, you're correct; I see now.
>
> >
> > > +
> > > + if (on) {
> > > + error = regulator_enable(cd->iovdd);
> > > + if (error < 0) {
> > > + dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
> > > + return error;
> > > + }
> > > +
> > > + /* Vendor waits 3ms for IOVDD to settle */
> > > + usleep_range(3000, 3100);
> > > +
> > > + error = regulator_enable(cd->avdd);
> > > + if (error < 0) {
> > > + dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
> > > + goto power_off_iovdd;
> > > + }
> > > +
> > > + /* Vendor waits 15ms for IOVDD to settle */
> > > + usleep_range(15000, 15100);
> > > +
> > > + gpiod_set_value(cd->reset_gpio, 0);
> > > +
> > > + /* Vendor waits 4ms for Firmware to initialize */
> > > + usleep_range(4000, 4100);
> > > +
> > > + error = goodix_berlin_dev_confirm(cd);
> > > + if (error < 0)
> > > + goto power_off_gpio;
> >
> > All of this cleaned up nicely. The following comment is idiomatic, but I feel
> > the goto can be easily eliminated as follows:
> >
> > error = goodix_berlin_dev_confirm(cd);
> > if (error)
> > break;
>
> Break ? in an if ?
Ignore my comment; I lost my place and thought we were inside a loop :)
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH] HID: amd_sfh: Check that sensors are enabled before set/get report
From: Mario Limonciello @ 2023-06-20 20:01 UTC (permalink / raw)
To: Basavaraj Natikar, Benjamin Tissoires
Cc: Bagas Sanjaya, Malte Starostik, linux-input, linux-kernel,
Mario Limonciello, stable, Linux regression tracking,
Haochen Tong
A crash was reported in amd-sfh related to hid core initialization
before SFH initialization has run.
```
amdtp_hid_request+0x36/0x50 [amd_sfh
2e3095779aada9fdb1764f08ca578ccb14e41fe4]
sensor_hub_get_feature+0xad/0x170 [hid_sensor_hub
d6157999c9d260a1bfa6f27d4a0dc2c3e2c5654e]
hid_sensor_parse_common_attributes+0x217/0x310 [hid_sensor_iio_common
07a7935272aa9c7a28193b574580b3e953a64ec4]
hid_gyro_3d_probe+0x7f/0x2e0 [hid_sensor_gyro_3d
9f2eb51294a1f0c0315b365f335617cbaef01eab]
platform_probe+0x44/0xa0
really_probe+0x19e/0x3e0
```
Ensure that sensors have been set up before calling into
amd_sfh_get_report() or amd_sfh_set_report().
Cc: stable@vger.kernel.org
Cc: Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info>
Fixes: 7bcfdab3f0c6 ("HID: amd_sfh: if no sensors are enabled, clean up")
Reported-by: Haochen Tong <linux@hexchain.org>
Link: https://lore.kernel.org/all/3250319.ancTxkQ2z5@zen/T/
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/hid/amd-sfh-hid/amd_sfh_client.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
index d9b7b01900b5..88f3d913eaa1 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
@@ -25,6 +25,9 @@ void amd_sfh_set_report(struct hid_device *hid, int report_id,
struct amdtp_cl_data *cli_data = hid_data->cli_data;
int i;
+ if (!cli_data->is_any_sensor_enabled)
+ return;
+
for (i = 0; i < cli_data->num_hid_devices; i++) {
if (cli_data->hid_sensor_hubs[i] == hid) {
cli_data->cur_hid_dev = i;
@@ -41,6 +44,9 @@ int amd_sfh_get_report(struct hid_device *hid, int report_id, int report_type)
struct request_list *req_list = &cli_data->req_list;
int i;
+ if (!cli_data->is_any_sensor_enabled)
+ return -ENODEV;
+
for (i = 0; i < cli_data->num_hid_devices; i++) {
if (cli_data->hid_sensor_hubs[i] == hid) {
struct request_list *new = kzalloc(sizeof(*new), GFP_KERNEL);
--
2.34.1
^ permalink raw reply related
* Re: amd_sfh driver causes kernel oops during boot
From: Limonciello, Mario @ 2023-06-20 20:03 UTC (permalink / raw)
To: Linux regressions mailing list, Malte Starostik,
Benjamin Tissoires
Cc: Bagas Sanjaya, basavaraj.natikar, linux-input, linux, stable
In-Reply-To: <f677a16e-1fa2-3f45-2b44-5acbf67aad3c@amd.com>
On 6/20/2023 1:50 PM, Limonciello, Mario wrote:
>
>>>>> I have a suspicion on commit 7bcfdab3f0c6 ("HID: amd_sfh: if no
>>>>> sensors
>>>>> are enabled, clean up") because the stack trace says that there is
>>>>> a bad
>>>>> list_add, which could happen if the object is not correctly
>>>>> initialized.
>>>>>
>>>>> However, that commit was present in v6.2, so it might not be that
>>>>> one.
>>>>>
>>>> If I'm not mistaken the Z13 doesn't actually have any
>>>> sensors connected to SFH. So I think the suspicion on
>>>> 7bcfdab3f0c6 and theory this is triggered by HID init makes
>>>> a lot of sense.
>>>>
>>>> Can you try this patch?
>>>>
>>>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>>>> b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>>>> index d9b7b01900b5..fa693a5224c6 100644
>>>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>>>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
>>>> @@ -324,6 +324,7 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev
>>>> *privdata)
>>>> devm_kfree(dev, cl_data->report_descr[i]);
>>>> }
>>>> dev_warn(dev, "Failed to discover, sensors not
>>>> enabled
>>>> is %d\n", cl_data->is_any_sensor_enabled);
>>>> + cl_data->num_hid_devices = 0;
>>>> return -EOPNOTSUPP;
>>>> }
>>>> schedule_delayed_work(&cl_data->work_buffer,
>>>> msecs_to_jiffies(AMD_SFH_IDLE_LOOP));
>>> I applied this to 9e87b63ed37e202c77aa17d4112da6ae0c7c097c now,
>>> which was the
>>> origin when I started the whole bisection. Clean rebuild, issue still
>>> persists.
>>>
>>> Out of 50 boots, I got:
>>>
>>> 25 clean
>>> 22 Oops as posted by the OP
>>> 1 same Oops, followed by a panic
>>> 1 lockup [1]
>>> 1 hanging with just a blank screen
>>>
>>> Not sure whether the lockups are related, but [1] mentions modprobe
>>> and udev-
>>> worker as well and all problems including the blank screen one
>>> appear roughly
>>> at the same time during boot. As this is before a graphics mode
>>> switch, I
>>> suspect the last mentioned case may be like [1] while the screen was
>>> blanked.
>>> To support the timing correlation: the UVC error for the IR cam
>>> shown in the
>>> photo (normal boot noise) also appears right before the BUG in the
>>> non-lockup
>>> bad case.
>>>
>>> I do see the dev_warn in dmesg, so the code path modified in your
>>> patch is
>>> indeed hit:
>>> [ 10.897521] pcie_mp2_amd 0000:63:00.7: Failed to discover,
>>> sensors not
>>> enabled is 1
>>> [ 10.897533] pcie_mp2_amd: probe of 0000:63:00.7 failed with error
>>> -95
>>>
>>> BR Malte
>>>
>>> [1] https://photos.app.goo.gl/2FAvQ7DqBsHEF6Bd8
>
> Apologies; for some reason I never got that above reply in my inbox,
> some server along the way might have deemed it spam.
>
> Anyways; I just double checked the Z13 I have on my hand. I don't
> have the PCI device for SFH (1022:164a) present on the system.
>
> Can you please double check you are on the latest BIOS?
>
> I'm on the latest release from LVFS, 0.1.57 according to fwupdmgr.
>
Hopefully the newer BIOS fixes it for you, but if it doesn't I did come
up with another patch I've sent out that I guess could be another
solution.
https://lore.kernel.org/linux-input/20230620200117.22261-1-mario.limonciello@amd.com/T/#u
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Haochen Tong @ 2023-06-21 2:46 UTC (permalink / raw)
To: Linux regressions mailing list, Malte Starostik,
Benjamin Tissoires, Limonciello, Mario
Cc: Bagas Sanjaya, basavaraj.natikar, linux-input, linux, stable
In-Reply-To: <1b3fd148-44d7-d476-e9e6-f9d8c8ec0ee6@leemhuis.info>
On 6/20/23 21:20, Linux regression tracking (Thorsten Leemhuis) wrote:
> Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
> for once, to make this easily accessible to everyone.
>
> What happens to this? From here it looks like there was no progress to
> resolve the regression in the past two weeks, but maybe I just missed
> something.
Hi,
I just looked at the journal again and this problem seemed to go away
after upgrading from 6.3.3 to 6.3.5. At that time the BIOS version was
still 1.27. Now, on 1.57, the device 1022:164a is indeed no longer
present anymore.
^ permalink raw reply
* [PATCH v2 2/5] dt-bindings: timer: gpt: Add i.MX6UL support
From: Oleksij Rempel @ 2023-06-21 9:32 UTC (permalink / raw)
To: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson
Cc: Oleksij Rempel, kernel, Peng Fan, Fabio Estevam, NXP Linux Team,
Daniel Lezcano, Thomas Gleixner, Michael Trimarchi, Mark Brown,
Dario Binacchi, Marek Vasut, linux-clk, devicetree,
linux-arm-kernel, linux-kernel, linux-crypto, linux-input,
linux-mmc
In-Reply-To: <20230621093245.78130-1-o.rempel@pengutronix.de>
Add 'fsl,imx6ul-gpt' compatible to resolve the following dtbs_check
warning:
imx6ull-jozacp.dtb:0:0: /soc/bus@2000000/timer@2098000: failed to match any schema with compatible: ['fsl,imx6ul-gpt', 'fsl,imx6sx-gpt']
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml b/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
index 716c6afcca1fa..685137338ac99 100644
--- a/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
+++ b/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
@@ -34,6 +34,9 @@ properties:
- fsl,imxrt1050-gpt
- fsl,imxrt1170-gpt
- const: fsl,imx6dl-gpt
+ - items:
+ - const: fsl,imx6ul-gpt
+ - const: fsl,imx6sx-gpt
reg:
maxItems: 1
--
2.39.2
^ permalink raw reply related
* [PATCH v2 1/5] dt-bindings: mmc: fsl-imx-esdhc: Add imx6ul support
From: Oleksij Rempel @ 2023-06-21 9:32 UTC (permalink / raw)
To: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson
Cc: Oleksij Rempel, kernel, Peng Fan, Fabio Estevam, NXP Linux Team,
Daniel Lezcano, Thomas Gleixner, Michael Trimarchi, Mark Brown,
Dario Binacchi, Marek Vasut, linux-clk, devicetree,
linux-arm-kernel, linux-kernel, linux-crypto, linux-input,
linux-mmc
In-Reply-To: <20230621093245.78130-1-o.rempel@pengutronix.de>
Add the 'fsl,imx6ul-usdhc' value to the compatible properties list in
the fsl-imx-esdhc.yaml file. This is required to match the compatible
strings present in the 'mmc@2190000' node of 'imx6ul-prti6g.dtb'. This
commit addresses the following dtbs_check warning:
imx6ul-prti6g.dtb:0:0: /soc/bus@2100000/mmc@2190000: failed to match any schema with compatible: ['fsl,imx6ul-usdhc', 'fsl,imx6sx-usdhc']
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
index fbfd822b92707..82eb7a24c8578 100644
--- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
+++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
@@ -42,6 +42,7 @@ properties:
- enum:
- fsl,imx6sll-usdhc
- fsl,imx6ull-usdhc
+ - fsl,imx6ul-usdhc
- const: fsl,imx6sx-usdhc
- items:
- const: fsl,imx7d-usdhc
--
2.39.2
^ permalink raw reply related
* [PATCH v2 5/5] dt-bindings: input: touchscreen: edt-ft5x06: Add 'threshold' property
From: Oleksij Rempel @ 2023-06-21 9:32 UTC (permalink / raw)
To: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson
Cc: Oleksij Rempel, Rob Herring, kernel, Peng Fan, Fabio Estevam,
NXP Linux Team, Daniel Lezcano, Thomas Gleixner,
Michael Trimarchi, Mark Brown, Dario Binacchi, Marek Vasut,
linux-clk, devicetree, linux-arm-kernel, linux-kernel,
linux-crypto, linux-input, linux-mmc
In-Reply-To: <20230621093245.78130-1-o.rempel@pengutronix.de>
Add a new property 'threshold' to the edt-ft5x06 touchscreen binding.
This property allows setting the "click"-threshold in the range from 0
to 255. This change addresses the following dtbs_check warning:
imx6dl-lanmcu.dtb: touchscreen@38: 'threshold' does not match any of the regexes: 'pinctrl-[0-9]+'
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/input/touchscreen/edt-ft5x06.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
index ef4c841387bdd..f2808cb4d99df 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
@@ -93,6 +93,12 @@ properties:
minimum: 1
maximum: 255
+ threshold:
+ description: Allows setting the "click"-threshold in the range from 0 to 255.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 255
+
touchscreen-size-x: true
touchscreen-size-y: true
touchscreen-fuzz-x: true
--
2.39.2
^ permalink raw reply related
* [PATCH v1 0/7] Add support for various features to i.MX6 bindings
From: Oleksij Rempel @ 2023-06-21 9:32 UTC (permalink / raw)
To: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson
Cc: Oleksij Rempel, kernel, Peng Fan, Fabio Estevam, NXP Linux Team,
Daniel Lezcano, Thomas Gleixner, Michael Trimarchi, Mark Brown,
Dario Binacchi, Marek Vasut, linux-clk, devicetree,
linux-arm-kernel, linux-kernel, linux-crypto, linux-input,
linux-mmc
changes v2:
- fix warning samples in the commit messages
- drop patches which needs more work
- address requested changes
This patch series is aimed at addressing several dtbs_check warnings by
introducing additional support in the device tree bindings for i.MX6
series SoCs. The warnings surfaced while validating some i.MX6 boards.
The issues were predominantly around unrecognized compatibility strings
and missing properties in the device trees.
Oleksij Rempel (5):
dt-bindings: mmc: fsl-imx-esdhc: Add imx6ul support
dt-bindings: timer: gpt: Add i.MX6UL support
dt-bindings: timer: gpt: Support 3rd clock for i.MX6DL
dt-bindings: clock: imx6ul: Support optional enet*_ref_pad clocks
dt-bindings: input: touchscreen: edt-ft5x06: Add 'threshold' property
Documentation/devicetree/bindings/clock/imx6ul-clock.yaml | 6 ++++++
.../devicetree/bindings/input/touchscreen/edt-ft5x06.yaml | 6 ++++++
Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml | 1 +
Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml | 7 +++++++
4 files changed, 20 insertions(+)
--
2.39.2
^ permalink raw reply
* [PATCH v2 3/5] dt-bindings: timer: gpt: Support 3rd clock for i.MX6DL
From: Oleksij Rempel @ 2023-06-21 9:32 UTC (permalink / raw)
To: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson
Cc: Oleksij Rempel, kernel, Peng Fan, Fabio Estevam, NXP Linux Team,
Daniel Lezcano, Thomas Gleixner, Michael Trimarchi, Mark Brown,
Dario Binacchi, Marek Vasut, linux-clk, devicetree,
linux-arm-kernel, linux-kernel, linux-crypto, linux-input,
linux-mmc
In-Reply-To: <20230621093245.78130-1-o.rempel@pengutronix.de>
Add support for a 3rd clock, 'osc_per', for i.MX6DL to the 'fsl,imxgpt'
binding to resolve the following dtbs_check warning:
imx6dl-alti6p.dtb: timer@2098000: clocks: [[2, 119], [2, 120], [2, 237]] is too long
imx6dl-alti6p.dtb: timer@2098000: clock-names: ['ipg', 'per', 'osc_per'] is too long
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml b/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
index 685137338ac99..34c62d152be81 100644
--- a/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
+++ b/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
@@ -45,14 +45,18 @@ properties:
maxItems: 1
clocks:
+ minItems: 2
items:
- description: SoC GPT ipg clock
- description: SoC GPT per clock
+ - description: SoC GPT osc_per clock
clock-names:
+ minItems: 2
items:
- const: ipg
- const: per
+ - const: osc_per
required:
- compatible
--
2.39.2
^ permalink raw reply related
* [PATCH v2 4/5] dt-bindings: clock: imx6ul: Support optional enet*_ref_pad clocks
From: Oleksij Rempel @ 2023-06-21 9:32 UTC (permalink / raw)
To: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson
Cc: Oleksij Rempel, kernel, Peng Fan, Fabio Estevam, NXP Linux Team,
Daniel Lezcano, Thomas Gleixner, Michael Trimarchi, Mark Brown,
Dario Binacchi, Marek Vasut, linux-clk, devicetree,
linux-arm-kernel, linux-kernel, linux-crypto, linux-input,
linux-mmc
In-Reply-To: <20230621093245.78130-1-o.rempel@pengutronix.de>
Extend the 'clocks' and 'clock-names' properties to support optional
'enet1_ref_pad' and 'enet2_ref_pad' clocks to resolve the following
dtbs_check warning:
imx6ul-prti6g.dtb: clock-controller@20c4000: clocks: [[17], [18], [19], [20], [21]] is too long
imx6ul-prti6g.dtb: clock-controller@20c4000: clock-names: ['ckil', 'osc', 'ipp_di0', 'ipp_di1', 'enet1_ref_pad'] is too long
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
Documentation/devicetree/bindings/clock/imx6ul-clock.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/clock/imx6ul-clock.yaml b/Documentation/devicetree/bindings/clock/imx6ul-clock.yaml
index be54d4df5afa2..3b71ebc100bf6 100644
--- a/Documentation/devicetree/bindings/clock/imx6ul-clock.yaml
+++ b/Documentation/devicetree/bindings/clock/imx6ul-clock.yaml
@@ -28,18 +28,24 @@ properties:
const: 1
clocks:
+ minItems: 4
items:
- description: 32k osc
- description: 24m osc
- description: ipp_di0 clock input
- description: ipp_di1 clock input
+ - description: Optional lenet1_ref_pad or enet2_ref_pad clocks
+ - description: Optional lenet1_ref_pad or enet2_ref_pad clocks
clock-names:
+ minItems: 4
items:
- const: ckil
- const: osc
- const: ipp_di0
- const: ipp_di1
+ - pattern: '^enet[12]_ref_pad$'
+ - pattern: '^enet[12]_ref_pad$'
required:
- compatible
--
2.39.2
^ permalink raw reply related
* [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
From: Benjamin Tissoires @ 2023-06-21 9:42 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina
Cc: linux-input, linux-kernel, stable, Benjamin Tissoires
Make the code looks less like Pascal.
Extract the internal code inside a helper function, fix the
initialization of the parameters used in the helper function
(`hidpp->answer_available` was not reset and `*response` wasn't too),
and use a `do {...} while();` loop.
Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
as requested by https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
This is a rewrite of that particular piece of code.
---
drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++----------------
1 file changed, 61 insertions(+), 41 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index dfe8e09a18de..3d1ffe199f08 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -275,21 +275,20 @@ static int __hidpp_send_report(struct hid_device *hdev,
}
/*
- * hidpp_send_message_sync() returns 0 in case of success, and something else
- * in case of a failure.
- * - If ' something else' is positive, that means that an error has been raised
- * by the protocol itself.
- * - If ' something else' is negative, that means that we had a classic error
- * (-ENOMEM, -EPIPE, etc...)
+ * Effectively send the message to the device, waiting for its answer.
+ *
+ * Must be called with hidpp->send_mutex locked
+ *
+ * Same return protocol than hidpp_send_message_sync():
+ * - success on 0
+ * - negative error means transport error
+ * - positive value means protocol error
*/
-static int hidpp_send_message_sync(struct hidpp_device *hidpp,
+static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
struct hidpp_report *message,
struct hidpp_report *response)
{
- int ret = -1;
- int max_retries = 3;
-
- mutex_lock(&hidpp->send_mutex);
+ int ret;
hidpp->send_receive_buf = response;
hidpp->answer_available = false;
@@ -300,41 +299,62 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
*/
*response = *message;
- for (; max_retries != 0 && ret; max_retries--) {
- ret = __hidpp_send_report(hidpp->hid_dev, message);
+ ret = __hidpp_send_report(hidpp->hid_dev, message);
+ if (ret) {
+ dbg_hid("__hidpp_send_report returned err: %d\n", ret);
+ memset(response, 0, sizeof(struct hidpp_report));
+ return ret;
+ }
- if (ret) {
- dbg_hid("__hidpp_send_report returned err: %d\n", ret);
- memset(response, 0, sizeof(struct hidpp_report));
- break;
- }
+ if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
+ 5*HZ)) {
+ dbg_hid("%s:timeout waiting for response\n", __func__);
+ memset(response, 0, sizeof(struct hidpp_report));
+ return -ETIMEDOUT;
+ }
- if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
- 5*HZ)) {
- dbg_hid("%s:timeout waiting for response\n", __func__);
- memset(response, 0, sizeof(struct hidpp_report));
- ret = -ETIMEDOUT;
- break;
- }
+ if (response->report_id == REPORT_ID_HIDPP_SHORT &&
+ response->rap.sub_id == HIDPP_ERROR) {
+ ret = response->rap.params[1];
+ dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
+ return ret;
+ }
- if (response->report_id == REPORT_ID_HIDPP_SHORT &&
- response->rap.sub_id == HIDPP_ERROR) {
- ret = response->rap.params[1];
- dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
+ if ((response->report_id == REPORT_ID_HIDPP_LONG ||
+ response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
+ response->fap.feature_index == HIDPP20_ERROR) {
+ ret = response->fap.params[1];
+ dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+/*
+ * hidpp_send_message_sync() returns 0 in case of success, and something else
+ * in case of a failure.
+ * - If ' something else' is positive, that means that an error has been raised
+ * by the protocol itself.
+ * - If ' something else' is negative, that means that we had a classic error
+ * (-ENOMEM, -EPIPE, etc...)
+ */
+static int hidpp_send_message_sync(struct hidpp_device *hidpp,
+ struct hidpp_report *message,
+ struct hidpp_report *response)
+{
+ int ret;
+ int max_retries = 3;
+
+ mutex_lock(&hidpp->send_mutex);
+
+ do {
+ ret = __do_hidpp_send_message_sync(hidpp, message, response);
+ if (ret != HIDPP20_ERROR_BUSY)
break;
- }
- if ((response->report_id == REPORT_ID_HIDPP_LONG ||
- response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
- response->fap.feature_index == HIDPP20_ERROR) {
- ret = response->fap.params[1];
- if (ret != HIDPP20_ERROR_BUSY) {
- dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
- break;
- }
- dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
- }
- }
+ dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
+ } while (--max_retries);
mutex_unlock(&hidpp->send_mutex);
return ret;
---
base-commit: b98ec211af5508457e2b1c4cc99373630a83fa81
change-id: 20230621-logitech-fixes-a4c0e66ea2ad
Best regards,
--
Benjamin Tissoires <benjamin.tissoires@redhat.com>
^ permalink raw reply related
* Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
From: Greg KH @ 2023-06-21 10:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina, linux-input,
linux-kernel, stable
In-Reply-To: <20230621-logitech-fixes-v1-1-32e70933c0b0@redhat.com>
On Wed, Jun 21, 2023 at 11:42:30AM +0200, Benjamin Tissoires wrote:
> Make the code looks less like Pascal.
>
> Extract the internal code inside a helper function, fix the
> initialization of the parameters used in the helper function
> (`hidpp->answer_available` was not reset and `*response` wasn't too),
> and use a `do {...} while();` loop.
>
> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> as requested by https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> This is a rewrite of that particular piece of code.
> ---
> drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++----------------
> 1 file changed, 61 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index dfe8e09a18de..3d1ffe199f08 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -275,21 +275,20 @@ static int __hidpp_send_report(struct hid_device *hdev,
> }
>
> /*
> - * hidpp_send_message_sync() returns 0 in case of success, and something else
> - * in case of a failure.
> - * - If ' something else' is positive, that means that an error has been raised
> - * by the protocol itself.
> - * - If ' something else' is negative, that means that we had a classic error
> - * (-ENOMEM, -EPIPE, etc...)
> + * Effectively send the message to the device, waiting for its answer.
> + *
> + * Must be called with hidpp->send_mutex locked
> + *
> + * Same return protocol than hidpp_send_message_sync():
> + * - success on 0
> + * - negative error means transport error
> + * - positive value means protocol error
> */
> -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> +static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
> struct hidpp_report *message,
> struct hidpp_report *response)
__must_hold(&hidpp->send_mutex) ?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox