Thank you for the review, Filipe!
I'm still new to the kernel patch submission process, so I'm a little
perplexed by the next steps. Who will decide if it is to be applied, and
is there any further action I need to take?
I am obviously keen, but not impatient, to get this feature in. I'm
happy to follow whatever process is required, I just haven't found
anything beyond the initial "submitting your patch" documentation.
Cheers!
- Dean
On 27/11/2020 8:42 am, Filipe Laíns wrote:
On Fri, 2020-11-27 at 08:30 +1100, Dean Camera wrote:
Hi Filipe,
Comments inline.
- Dean
On 27/11/2020 6:05 am, Filipe Laíns wrote:
Hi,
What is the difference between V1, V2 and V3? I think generally you would
add a
small summary.
Sorry, that's my fault -- the contents are identical. I am more used to
modern tooling with code review platforms, pull-requests or even emailed
attached patches, so the old tooling took me a few goes to get right.
V1 was mangled by Thunderbird, while V2 was missing the cover letter
when I submitted it via git send-email from my test machine. V3 is where
I (think) I beat the tooling into submission.
No worries, I was just wondering what the differences were, given that the
patches were identical.
*snip*
I am not sure using the same approach as HIDIOCGFEATURE is a good design
choice.
The first byte of the supplied buffer is the report ID, but you can set is
to 0
if you don't want to use numbered reports. From my understanding, this makes
it
impossible to use the ioctl with report ID 0, which valid per the HID spec.
Report ID 0 is reserved by the HID specification and may not be used in
a device with multiple reports (see "Device Class Definition for HID
1.11", section "6.2.2.7 Global Items" where it states "Report ID zero is
reserved and should not be used.").
Yup, you are right! I missed that.
I think the designers of HID forsaw a sane future where in userspace
everyone just assumed the report ID was present at all times, and the
HID driver would just omit it on the wire if it was zero. Unfortunatly
every platform seems to handle that differently now, with some always
requring it, and others selectively omitting it in their APIs.
Yes, it's unfortunate. Perhaps if the HID designers made the spec easier to read
we wouldn't have this issue.
My suggestion would be to automatically use numbered reports or not
depending if
the device uses them. A HID endpoint either uses numbered reports or not, it
doesn't make much sense to me to let users try to use numbered reports on
devices that do not use them or the other way round.
But I guess this is a question for Benjamin.
I'm *strongly* in favour of always having them at least in the
`ioctl()`, with a (reserved) zero value indicating it is unused - like
it is now. That makes userspace easier to deal with, and covers the
quirk case where a device does not list report IDs in its HID report
descriptor properly, but requires them anyway.
It also makes the new requests consistent with the existing request, so
there's no extra cognitive load from working with one then switching to
the others.
Yeah, it makes sense given that report 0 is reserved :)
I tried to track down the discussion about the addition of the
HIDIOCGFEATURE
ioctl but from what I saw there was no mention of this design flaw.
Am I missing something here?
I was :D
You can have my
Signed-off-by: Filipe Laíns <lains@xxxxxxxxxxxxx>
Perhaps just split the hid-example.c change to another patch.
*snip*
Cheers,
Filipe Laíns
