LinuxLists.cc - [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

On Wed, 6 Dec 2006, Li Yu wrote:

> 1. Make hidinput_disconnect_core() be more robust, it can not
> break anything even failed to allocate device struct.
> 2. Thanks new input device driver API, we need not the extra code
> for support force-feed device yet, so say bye to
> CONFIG_HID_SIMPLE_FF.
> Is this ready to merge? or What still is problem in them? Thanks.

Hi,

actually, I have prepared patches to split the USBHID code in two parts -
generic HID, which could be hooked by transport-specific HID layers (USB,
Bluetooth).

I did not send them to lkml/linux-usb, as they are quite big (mainly
because a lot of code is being moved around). I am currently trying to
setup a git repository on kernel.org, hopefully kernel.org people will
react, so that the patches could be easily put into git repository and be
available for rewiew and easy merge. After that, they are planned to be
merged either into Greg's or Andrew's tree. I can send them to you if you
want.

Do you think that you could wait a little bit more, after the split has
been done? (it's currently planned approximately after 2.6.20-rc1). It
seems to me that your patches will apply almost cleanly on top of the
split patches (you will have to change the pathnames, of course).

Thanks,

--
Jiri Kosina
SUSE Labs

2006-12-06 14:24:06

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

Hi,

On 12/6/06, Jiri Kosina <[email protected]> wrote:
> On Wed, 6 Dec 2006, Li Yu wrote:
>
> > 1. Make hidinput_disconnect_core() be more robust, it can not
> > break anything even failed to allocate device struct.
> > 2. Thanks new input device driver API, we need not the extra code
> > for support force-feed device yet, so say bye to
> > CONFIG_HID_SIMPLE_FF.
> > Is this ready to merge? or What still is problem in them? Thanks.
>
> Hi,
>
> actually, I have prepared patches to split the USBHID code in two parts -
> generic HID, which could be hooked by transport-specific HID layers (USB,
> Bluetooth).
>
> I did not send them to lkml/linux-usb, as they are quite big (mainly
> because a lot of code is being moved around). I am currently trying to
> setup a git repository on kernel.org, hopefully kernel.org people will
> react, so that the patches could be easily put into git repository and be
> available for rewiew and easy merge. After that, they are planned to be
> merged either into Greg's or Andrew's tree. I can send them to you if you
> want.
>
> Do you think that you could wait a little bit more, after the split has
> been done? (it's currently planned approximately after 2.6.20-rc1). It
> seems to me that your patches will apply almost cleanly on top of the
> split patches (you will have to change the pathnames, of course).
>

I still have the same objection - the "simple'" code will have to be
compiled into the driver instead of being a separate module and
eventyally will lead to a monster-size HID module. We have this issue
with psmouse to a degree but with HID the growth potential is much
bigger IMO.

Jiri, I have not looked at your patches yet (I need to do that) but
what I was hoping to do (or have someone to do ;) ) is to provide
ability to define HID transport drivers (we would have USB transport
and bluetooth transport) and then say:

device = hid_create_device(&my_transport);
device->event = my_event_handler;
....
hid_start_device(device);

hid-create_device would parse all reports and create "standard" hid
device. Then you have a chance to override and tweak it as you see
fit.

This way we could have several small drivers implementing quirks to
the generic HID driver. Most of the code is still in hid core module
but every individual driver is complete USB (or bluetooth) driver, has
its own device table and is loaded via standard driver code bust
matching/hotplug modalias mechanism.

Btw, I saw you moving it into drivers/hid, would not
drivers/input/hid/ suit better?

--
Dmitry

2006-12-06 14:38:51

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

Hi Dmitry,

> > > 1. Make hidinput_disconnect_core() be more robust, it can not
> > > break anything even failed to allocate device struct.
> > > 2. Thanks new input device driver API, we need not the extra code
> > > for support force-feed device yet, so say bye to
> > > CONFIG_HID_SIMPLE_FF.
> > > Is this ready to merge? or What still is problem in them? Thanks.
> >
> > Hi,
> >
> > actually, I have prepared patches to split the USBHID code in two parts -
> > generic HID, which could be hooked by transport-specific HID layers (USB,
> > Bluetooth).
> >
> > I did not send them to lkml/linux-usb, as they are quite big (mainly
> > because a lot of code is being moved around). I am currently trying to
> > setup a git repository on kernel.org, hopefully kernel.org people will
> > react, so that the patches could be easily put into git repository and be
> > available for rewiew and easy merge. After that, they are planned to be
> > merged either into Greg's or Andrew's tree. I can send them to you if you
> > want.
> >
> > Do you think that you could wait a little bit more, after the split has
> > been done? (it's currently planned approximately after 2.6.20-rc1). It
> > seems to me that your patches will apply almost cleanly on top of the
> > split patches (you will have to change the pathnames, of course).
> >
>
> I still have the same objection - the "simple'" code will have to be
> compiled into the driver instead of being a separate module and
> eventyally will lead to a monster-size HID module. We have this issue
> with psmouse to a degree but with HID the growth potential is much
> bigger IMO.
>
> Jiri, I have not looked at your patches yet (I need to do that) but
> what I was hoping to do (or have someone to do ;) ) is to provide
> ability to define HID transport drivers (we would have USB transport
> and bluetooth transport) and then say:
>
> device = hid_create_device(&my_transport);
> device->event = my_event_handler;
> ....
> hid_start_device(device);
>
> hid-create_device would parse all reports and create "standard" hid
> device. Then you have a chance to override and tweak it as you see
> fit.

I actually prefer that we go the way hid_{register|unregister}_device
and have a hid_alloc_device(descriptor) to actually create the new HID
device and allocate its descriptor tables.

The transport actually doesn't care on how you interpret or tweak the
events. This is not its job. It is a simple plain stupid transport.
Every additional driver that you put on top of the HID parser will look
at the VID/PID and then decide which input event to send or what to do.
So from my understanding the generic one should work just fine, but in
case we need some special handling you can load an extra driver to
handle this. And this could be done as HID driver. One driver could be
the new hidraw driver to allow raw access to the HID reports.

> This way we could have several small drivers implementing quirks to
> the generic HID driver. Most of the code is still in hid core module
> but every individual driver is complete USB (or bluetooth) driver, has
> its own device table and is loaded via standard driver code bust
> matching/hotplug modalias mechanism.

Again. The driver doesn't have to know if it is Bluetooth or USB that
the reports are send over. For example a company can make the same
keyboard and one version is using Bluetooth and the other USB. You don't
wanna have two different drivers for it. You only need one driver that
knows how to speak HID.

> Btw, I saw you moving it into drivers/hid, would not
> drivers/input/hid/ suit better?

This would stick HID to the input subsystem and from my understanding
the HID parser can even work without input. The input subsystem is only
one user of it. In case of hidraw access you don't need the input
subsystem at all.

Regards

Marcel

2006-12-06 14:56:13

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

Hi Marcel,

On 12/6/06, Marcel Holtmann <[email protected]> wrote:
> Hi Dmitry,
>
> > > > 1. Make hidinput_disconnect_core() be more robust, it can not
> > > > break anything even failed to allocate device struct.
> > > > 2. Thanks new input device driver API, we need not the extra code
> > > > for support force-feed device yet, so say bye to
> > > > CONFIG_HID_SIMPLE_FF.
> > > > Is this ready to merge? or What still is problem in them? Thanks.
> > >
> > > Hi,
> > >
> > > actually, I have prepared patches to split the USBHID code in two parts -
> > > generic HID, which could be hooked by transport-specific HID layers (USB,
> > > Bluetooth).
> > >
> > > I did not send them to lkml/linux-usb, as they are quite big (mainly
> > > because a lot of code is being moved around). I am currently trying to
> > > setup a git repository on kernel.org, hopefully kernel.org people will
> > > react, so that the patches could be easily put into git repository and be
> > > available for rewiew and easy merge. After that, they are planned to be
> > > merged either into Greg's or Andrew's tree. I can send them to you if you
> > > want.
> > >
> > > Do you think that you could wait a little bit more, after the split has
> > > been done? (it's currently planned approximately after 2.6.20-rc1). It
> > > seems to me that your patches will apply almost cleanly on top of the
> > > split patches (you will have to change the pathnames, of course).
> > >
> >
> > I still have the same objection - the "simple'" code will have to be
> > compiled into the driver instead of being a separate module and
> > eventyally will lead to a monster-size HID module. We have this issue
> > with psmouse to a degree but with HID the growth potential is much
> > bigger IMO.
> >
> > Jiri, I have not looked at your patches yet (I need to do that) but
> > what I was hoping to do (or have someone to do ;) ) is to provide
> > ability to define HID transport drivers (we would have USB transport
> > and bluetooth transport) and then say:
> >
> > device = hid_create_device(&my_transport);
> > device->event = my_event_handler;
> > ....
> > hid_start_device(device);
> >
> > hid-create_device would parse all reports and create "standard" hid
> > device. Then you have a chance to override and tweak it as you see
> > fit.
>
> I actually prefer that we go the way hid_{register|unregister}_device
> and have a hid_alloc_device(descriptor) to actually create the new HID
> device and allocate its descriptor tables.
>
> The transport actually doesn't care on how you interpret or tweak the
> events. This is not its job. It is a simple plain stupid transport.
> Every additional driver that you put on top of the HID parser will look
> at the VID/PID and then decide which input event to send or what to do.
> So from my understanding the generic one should work just fine, but in
> case we need some special handling you can load an extra driver to
> handle this. And this could be done as HID driver. One driver could be
> the new hidraw driver to allow raw access to the HID reports.

Well, it seems we have 2 different kinds of HID drivers. Hidraw is an
alternate interface driver - it provides different interface to HID
device and therefore can be implemented as "extra" driver. Li Yu's
"simple" drivers extend existing input interfaces. I do not think
you'd want to have /dev/input/event0 implementing partially incorrect
keyboard support (for a particular keyboard) and /dev/input/event1
(the one cretaed by a simple drver if impleented as extra interface)
implementing better support.

>
> > This way we could have several small drivers implementing quirks to
> > the generic HID driver. Most of the code is still in hid core module
> > but every individual driver is complete USB (or bluetooth) driver, has
> > its own device table and is loaded via standard driver code bust
> > matching/hotplug modalias mechanism.
>
> Again. The driver doesn't have to know if it is Bluetooth or USB that
> the reports are send over. For example a company can make the same
> keyboard and one version is using Bluetooth and the other USB. You don't
> wanna have two different drivers for it. You only need one driver that
> knows how to speak HID.
>

But you need to know when to attach your driver. How would pure HID
driver know that? USB driver can do that via manufacturer/product IDs.
And we have all that infrastructure of loading proper modules. Or do
you want to create a hid bus?

> > Btw, I saw you moving it into drivers/hid, would not
> > drivers/input/hid/ suit better?
>
> This would stick HID to the input subsystem and from my understanding
> the HID parser can even work without input. The input subsystem is only
> one user of it. In case of hidraw access you don't need the input
> subsystem at all.
>

OK.

--
Dmitry

2006-12-06 15:00:04

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

On Wed, 6 Dec 2006, Marcel Holtmann wrote:

> > I still have the same objection - the "simple'" code will have to be
> > compiled into the driver instead of being a separate module and
> > eventyally will lead to a monster-size HID module. We have this issue
> > with psmouse to a degree but with HID the growth potential is much
> > bigger IMO.

I guess that this paragraph wasn't for me, but rather for the author of
the HID Simple Driver proposal, am I right?

> > Jiri, I have not looked at your patches yet (I need to do that) but

Please do, when you will have time. Any comments are very welcome, before
I start actually heading for the merge.

> The transport actually doesn't care on how you interpret or tweak the
> events. This is not its job. It is a simple plain stupid transport.
> Every additional driver that you put on top of the HID parser will look
> at the VID/PID and then decide which input event to send or what to do.
> So from my understanding the generic one should work just fine, but in
> case we need some special handling you can load an extra driver to
> handle this. And this could be done as HID driver. One driver could be
> the new hidraw driver to allow raw access to the HID reports.

Yes, the hidraw driver is one of the things that I am planning to develop
on top of the generic HID layer, after the HID split gets merged.

The current patches mainly take the contents of drivers/usb/input, and
separate the code to transport-specific and the rest, which is then put
into drivers/hid (of course some code changes are needed) and USB-specific
HID code is then hooked into this new HID layer.
I have tried to verify that hooking bluetooth, in the state in which it
currently is (plus the patches which are not in mainline, also because of
non-existence of common HID code), would stay relatively easy task,
hopefully.

This split is quite painful, as there are many things happening in USB all
the time, so the best way seem to be just to perform big split (with
needed changes) at once, and then develop other things on top of it (like
hidraw).

--
Jiri Kosina
SUSE Labs

2006-12-06 15:06:56

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

Hi Dmitry,

> > > I still have the same objection - the "simple'" code will have to be
> > > compiled into the driver instead of being a separate module and
> > > eventyally will lead to a monster-size HID module. We have this issue
> > > with psmouse to a degree but with HID the growth potential is much
> > > bigger IMO.
> > >
> > > Jiri, I have not looked at your patches yet (I need to do that) but
> > > what I was hoping to do (or have someone to do ;) ) is to provide
> > > ability to define HID transport drivers (we would have USB transport
> > > and bluetooth transport) and then say:
> > >
> > > device = hid_create_device(&my_transport);
> > > device->event = my_event_handler;
> > > ....
> > > hid_start_device(device);
> > >
> > > hid-create_device would parse all reports and create "standard" hid
> > > device. Then you have a chance to override and tweak it as you see
> > > fit.
> >
> > I actually prefer that we go the way hid_{register|unregister}_device
> > and have a hid_alloc_device(descriptor) to actually create the new HID
> > device and allocate its descriptor tables.
> >
> > The transport actually doesn't care on how you interpret or tweak the
> > events. This is not its job. It is a simple plain stupid transport.
> > Every additional driver that you put on top of the HID parser will look
> > at the VID/PID and then decide which input event to send or what to do.
> > So from my understanding the generic one should work just fine, but in
> > case we need some special handling you can load an extra driver to
> > handle this. And this could be done as HID driver. One driver could be
> > the new hidraw driver to allow raw access to the HID reports.
>
> Well, it seems we have 2 different kinds of HID drivers. Hidraw is an
> alternate interface driver - it provides different interface to HID
> device and therefore can be implemented as "extra" driver. Li Yu's
> "simple" drivers extend existing input interfaces. I do not think
> you'd want to have /dev/input/event0 implementing partially incorrect
> keyboard support (for a particular keyboard) and /dev/input/event1
> (the one cretaed by a simple drver if impleented as extra interface)
> implementing better support.

I think for every HID report id, you should be able to attach a special
driver that handles the reports of this id.

> > > This way we could have several small drivers implementing quirks to
> > > the generic HID driver. Most of the code is still in hid core module
> > > but every individual driver is complete USB (or bluetooth) driver, has
> > > its own device table and is loaded via standard driver code bust
> > > matching/hotplug modalias mechanism.
> >
> > Again. The driver doesn't have to know if it is Bluetooth or USB that
> > the reports are send over. For example a company can make the same
> > keyboard and one version is using Bluetooth and the other USB. You don't
> > wanna have two different drivers for it. You only need one driver that
> > knows how to speak HID.
> >
>
> But you need to know when to attach your driver. How would pure HID
> driver know that? USB driver can do that via manufacturer/product IDs.
> And we have all that infrastructure of loading proper modules. Or do
> you want to create a hid bus?

Actually creating a HID bus is not a bad idea. And even Bluetooth
provides the VID/PID values for every device. So the transports only
have to give these to the HID parser. Then every driver on top of the
common HID layer can do whatever they think is best and they don't have
to care about the actual transport. Actually the report descriptor is a
good example which both (USB and Bluetooth) have in common, but they are
retrieved totally different. The driver on-top of HID doesn't need to
have to deal with that.

However separating out the transports and creating a common HID layer
should be done sooner than later. Once we have done that, we can talk
about the HID simple driver or device specific drivers on top of the
common HID layer.

Regards

Marcel

2006-12-06 15:13:18

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

On 12/6/06, Jiri Kosina <[email protected]> wrote:
> On Wed, 6 Dec 2006, Marcel Holtmann wrote:
>
> > > I still have the same objection - the "simple'" code will have to be
> > > compiled into the driver instead of being a separate module and
> > > eventyally will lead to a monster-size HID module. We have this issue
> > > with psmouse to a degree but with HID the growth potential is much
> > > bigger IMO.
>
> I guess that this paragraph wasn't for me, but rather for the author of
> the HID Simple Driver proposal, am I right?

Yes, mainly for him but also for you because we need to be able to do
what Li Yu is trying to do and be able to tweak HID interfaces.

...

> This split is quite painful, as there are many things happening in USB all
> the time, so the best way seem to be just to perform big split (with
> needed changes) at once, and then develop other things on top of it (like
> hidraw).

Is there any reason why we can't mecanically move everything into
drivers/hid right now? Then Greg could simply forward all patches he
gets for HID your way and you won't have hard time merging your work
with others...

--
Dmitry

2006-12-06 15:18:56

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

On Wed, 6 Dec 2006, Dmitry Torokhov wrote:

> > I guess that this paragraph wasn't for me, but rather for the author of
> > the HID Simple Driver proposal, am I right?
> Yes, mainly for him but also for you because we need to be able to do
> what Li Yu is trying to do and be able to tweak HID interfaces.

I see. Will look at Li Yu's code to lear better what the Simple Driver
Interface idea is about. Thanks.

> > This split is quite painful, as there are many things happening in USB all
> > the time, so the best way seem to be just to perform big split (with
> > needed changes) at once, and then develop other things on top of it (like
> > hidraw).
> Is there any reason why we can't mecanically move everything into
> drivers/hid right now? Then Greg could simply forward all patches he
> gets for HID your way and you won't have hard time merging your work
> with others...

That definitely would be a possible solution.

In fact, the patches for the split I sent to you and a few other people
two weeks ago or so, do exactly this - in some sense "mechanical" split of
the generic parts laying currently in USB hid, from the USB-specific ones,
and moving them around (sure, some changes are done, like introducing data
structures specific to usbhid, etc., but no rocket science yet).

This would be nice to merge, if noone has any major objections, and do
other development on top of that.
I am currently trying to set up an account and git tree for this at
kernel.org ... request sent, waiting for reply :)

Thanks,

--
Jiri Kosina
SUSE Labs

2006-12-06 15:18:54

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

Hi Dmitry,

> > > > I still have the same objection - the "simple'" code will have to be
> > > > compiled into the driver instead of being a separate module and
> > > > eventyally will lead to a monster-size HID module. We have this issue
> > > > with psmouse to a degree but with HID the growth potential is much
> > > > bigger IMO.
> >
> > I guess that this paragraph wasn't for me, but rather for the author of
> > the HID Simple Driver proposal, am I right?
>
> Yes, mainly for him but also for you because we need to be able to do
> what Li Yu is trying to do and be able to tweak HID interfaces.
>
> ...
>
> > This split is quite painful, as there are many things happening in USB all
> > the time, so the best way seem to be just to perform big split (with
> > needed changes) at once, and then develop other things on top of it (like
> > hidraw).
>
> Is there any reason why we can't mecanically move everything into
> drivers/hid right now? Then Greg could simply forward all patches he
> gets for HID your way and you won't have hard time merging your work
> with others...

I fully agree. Lets move and split the transports now and start the work
on top of it. My only concern is to have a clean Git tree to the full
history of HID changes stay intact and will trackable. Some quirk
decisions and other stuff is not obvious and I assume will never be when
it comes to broken HID devices.

So do you have pending HID patches. If yes, please sync them with Linus
and Jiri can setup a clean tree for the move.

Regards

Marcel

2006-12-06 15:25:43

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

Hi Jiri,

> This would be nice to merge, if noone has any major objections, and do
> other development on top of that.
> I am currently trying to set up an account and git tree for this at
> kernel.org ... request sent, waiting for reply :)

I can setup a tree for the merge. You simply have to create the patches
with cg-mkpatch and then I can apply them.

Regards

Marcel

2006-12-06 15:31:48

[permalink] [raw]

Subject: Re: [PATCH] usb/hid: The HID Simple Driver Interface 0.4.1 (core)

On 12/6/06, Jiri Kosina <[email protected]> wrote:
> On Wed, 6 Dec 2006, Dmitry Torokhov wrote:
> > Is there any reason why we can't mecanically move everything into
> > drivers/hid right now? Then Greg could simply forward all patches he
> > gets for HID your way and you won't have hard time merging your work
> > with others...
>
> That definitely would be a possible solution.
>
> In fact, the patches for the split I sent to you and a few other people
> two weeks ago or so, do exactly this - in some sense "mechanical" split of
> the generic parts laying currently in USB hid, from the USB-specific ones,
> and moving them around (sure, some changes are done, like introducing data
> structures specific to usbhid, etc., but no rocket science yet).
>

If Greg is OK with that I would start with truly mechanical merge (no
now data structures, just move the files around) and merge this ASAP,
before we hit -rc1 or -rc2 at the latest. Then you can start puling up
your changesin the separate git tree.

> This would be nice to merge, if noone has any major objections, and do
> other development on top of that.
> I am currently trying to set up an account and git tree for this at
> kernel.org ... request sent, waiting for reply :)
>

Take up Marcel on his suggestion ;) I could set up a tree too but I
am afraid I won't have enought time at the moment.

--
Dmitry

2006-12-06 15:41:14