Re: [PATCH] HID: Add Support for Setting and Getting Feature Reports from hidraw

[Antonio Ospite]

On Tue, 08 Jun 2010 09:42:55 -0400
Alan Ott <alan@xxxxxxxxxxx> wrote:

> On 06/08/2010 02:32 AM, Antonio Ospite wrote:
> > On Mon,  7 Jun 2010 23:51:48 -0400
> > Alan Ott<alan@xxxxxxxxxxx>  wrote:
> >
> >    
> >> Per the HID Specification, Feature reports must be sent and received on
> >> the Configuration endpoint (EP 0) through the Set_Report/Get_Report
> >> interfaces.  This patch adds two ioctls to hidraw to set and get feature
> >> reports to and from the device.  Modifications were made to hidraw and
> >> usbhid.
> >>
> >> New hidraw ioctls:
> >>    HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
> >>    HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.
> >>
> >> Signed-off-by: Alan Ott<alan@xxxxxxxxxxx>
> >> ---
> >>      
> > Thanks Alan, I am going to test this quite soon.
> >
> > TBH, when I was thinking about how to extend hidraw I thought we could
> > have added a new report_type field to struct hidraw_report_descriptor,
> > in order to re-use the HIDIOCGRDESC ioctl handler itself, adding then a
> > HIDIOCSRDESC for setting the report. This looked cleaner to my eyes,
> >    
> Thanks for the feedback, Antonio. The HIDIOCGRDESC ioctl copies the 
> existing descriptor from the hid_device structure. Since it does not 
> initiate a Get_Report transfer, I'm not sure how much re-use there could 
> have been using that method. In my estimation, a Set_Report/Get_Report 
> was more similar to the call to write().
>

I was only thinking about the interface to userspace,
HIDIOCGRDESC/HIDIOCSRDESC sounded more general to me (and look like
HIDIOCGREPORT/HIDIOCSREPORT from hiddev) if they could be made to work
with different report types, but as I said I didn't look at the
current code very well, so my remark are surely quite naive.

> > but I didn't actually implement this, so I don't know if it was
> > feasible, for instance one problem I didn't investigate further was
> > about the default value of the aforementioned report_type field in
> > order to keep the current behavior of HIDIOCGRDESC.
> >    
> I'm not sure what you mean here, as the report_type field is not part of 
> hidraw_report_descriptor.
>

I was thinking about _adding_ that field, but again, pretty arbitrarily
thought.

> Thanks for testing my patch. Please let me know if you have problems 
> with it.
>

It works basically ok for my needs, thanks again, waiting for comments
from usb/HID people.

Note that there are some checkpatch.pl errors in the current patch, and
also a style fix mixed with functional ones (@@ -315,7 +411,7 @@), you
may want to sort these out in a v2.

After this gets in, some more style fixes to hidraw.c could be made,
I'll do these. Maybe some naming cleanup can be made too,
hid_output_raw_report could become hid_set_raw_report for instance, but
I am waiting for the topic to settle first.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

Attachment: pgp00000.pgp
Description: PGP signature