2009-03-16 10:55:11

by Bastien Nocera

[permalink] [raw]
Subject: Re: RFC: Wacom Bluetooth HID driver, first pass

On Fri, 2009-03-13 at 23:59 +0000, Bastien Nocera wrote:
> Heya,
>
> This is a first pass at a driver for the Wacom Graphire Bluetooth
> tablet, based on work by Andrew Zabolotny[1]. It's mildly tested (eg. it
> works in my very few tests).
>
> I was looking for guidance for the code itself, possibly making it
> easier to merge in the input/tablet/wacom* drivers into it in the
> future, as well as some explanations as to how I'm supposed to reset
> tools, etc. so the user-space drivers (the X.org linuxwacom driver)
> doesn't need special-casing for the device.
>
> Note that it requires a user-space activation to switch to Mode 2, using
> bluetoothd (as the Bluetooth HID doesn't support hid_output_raw_event).
> Patch at:
> http://cvs.fedoraproject.org/viewvc/rpms/bluez/devel/bluez-activate-wacom-mode2.patch?view=markup

A couple of notes:
- the wheel action is reversed, it's a simple fix, done locally
- hidp and the Bluetooth sub-system says it can't probe the device with
error "-14" when hid-wacom.ko isn't already loaded
- I'm getting oopses in hci_conn_del() when the device goes away (eg. I
turn it off).

I'd appreciate any help making this just a tad more stable.

Any comments about the code itself?

Cheers

PS: Original mail at
http://thread.gmane.org/gmane.linux.kernel.input/7078 for the Bluetooth
hackers



2009-03-16 23:58:54

by Bastien Nocera

[permalink] [raw]
Subject: Re: RFC: Wacom Bluetooth HID driver, first pass

On Mon, 2009-03-16 at 14:29 +0000, Bastien Nocera wrote:
> On Mon, 2009-03-16 at 15:00 +0100, Marcel Holtmann wrote:
<snip>
> > > A couple of notes:
> > > - the wheel action is reversed, it's a simple fix, done locally
> > > - hidp and the Bluetooth sub-system says it can't probe the device
> > > with
> > > error "-14" when hid-wacom.ko isn't already loaded

Bluetooth: HIDP (Human Interface Emulation) ver 1.2
wacom 0005:056A:0081.0001: parse failed
wacom: probe of 0005:056A:0081.0001 failed with error -14
wacom driver registered

> > what was the reason for not forcing HID to load the generic driver?
>
> What generic driver? If you're talking about the default Wacom driver,
> it isn't an HID driver, and requires a USB device.
>
> > I
> > lost track on how HID is suppose to handle these. You could also
> > introduce a phony export like we do with L2CAP to ensure it is loaded
> > even if userspace isn't ready to handle this dependency.
>
> Well, the problem is that the driver _does_ load, but I'm not certain
> about the sequence of events between the Bluetooth sub-system, the
> user-space enabling (see the linked patch to bluetoothd), and the HID
> sub-system.

Turning the device off and on again works though. No idea what's
happening.

> > > - I'm getting oopses in hci_conn_del() when the device goes away
> > > (eg. I
> > > turn it off).
> >
> > Try to run a net-next-2.6 or bluetooth-next-2.6 kernel. Kyle should
> > have merged these patches into the latest rawhide kernel.
>
> I was hoping this would be fixed in the latest linus-2.6 tree, guess
> not.

No more crashes. Great stuff.

I sent a fixed up patch to the list earlier.

Cheers

2009-03-16 14:29:59

by Bastien Nocera

[permalink] [raw]
Subject: Re: RFC: Wacom Bluetooth HID driver, first pass

On Mon, 2009-03-16 at 15:00 +0100, Marcel Holtmann wrote:
> Hi Bastien,
>
> >> This is a first pass at a driver for the Wacom Graphire Bluetooth
> >> tablet, based on work by Andrew Zabolotny[1]. It's mildly tested
> >> (eg. it
> >> works in my very few tests).
> >>
> >> I was looking for guidance for the code itself, possibly making it
> >> easier to merge in the input/tablet/wacom* drivers into it in the
> >> future, as well as some explanations as to how I'm supposed to reset
> >> tools, etc. so the user-space drivers (the X.org linuxwacom driver)
> >> doesn't need special-casing for the device.
> >>
> >> Note that it requires a user-space activation to switch to Mode 2,
> >> using
> >> bluetoothd (as the Bluetooth HID doesn't support
> >> hid_output_raw_event).
> >> Patch at:
> >> http://cvs.fedoraproject.org/viewvc/rpms/bluez/devel/bluez-activate-wacom-mode2.patch?view=markup
> >
> > A couple of notes:
> > - the wheel action is reversed, it's a simple fix, done locally
> > - hidp and the Bluetooth sub-system says it can't probe the device
> > with
> > error "-14" when hid-wacom.ko isn't already loaded
>
> what was the reason for not forcing HID to load the generic driver?

What generic driver? If you're talking about the default Wacom driver,
it isn't an HID driver, and requires a USB device.

> I
> lost track on how HID is suppose to handle these. You could also
> introduce a phony export like we do with L2CAP to ensure it is loaded
> even if userspace isn't ready to handle this dependency.

Well, the problem is that the driver _does_ load, but I'm not certain
about the sequence of events between the Bluetooth sub-system, the
user-space enabling (see the linked patch to bluetoothd), and the HID
sub-system.

> > - I'm getting oopses in hci_conn_del() when the device goes away
> > (eg. I
> > turn it off).
>
> Try to run a net-next-2.6 or bluetooth-next-2.6 kernel. Kyle should
> have merged these patches into the latest rawhide kernel.

I was hoping this would be fixed in the latest linus-2.6 tree, guess
not.

> > I'd appreciate any help making this just a tad more stable.
> >
> > Any comments about the code itself?
>
> The main parsing function looks highly complicated and could either
> use more comments or break into two.

The parsing is adapted from the code Andrew wrote, so I don't have any
great insights into it. Right it's in "it works well enough to do
things" mode, clean-ups and bug fixes will probably follow when my
machine stops crashing when using that driver :)

> if ((wdata->tool = tool)) {
> input_report_key(input, tool, 1);
> need_sync = 1;
> }
>
> Don't do the assignment in the if clause test.

As I mentioned, copy/paste code this is.

> Did you run
> checkpatch.pl on it?

Nope, I didn't, but will do.

Cheers

2009-03-16 14:00:55

by Marcel Holtmann

[permalink] [raw]
Subject: Re: RFC: Wacom Bluetooth HID driver, first pass

Hi Bastien,

>> This is a first pass at a driver for the Wacom Graphire Bluetooth
>> tablet, based on work by Andrew Zabolotny[1]. It's mildly tested
>> (eg. it
>> works in my very few tests).
>>
>> I was looking for guidance for the code itself, possibly making it
>> easier to merge in the input/tablet/wacom* drivers into it in the
>> future, as well as some explanations as to how I'm supposed to reset
>> tools, etc. so the user-space drivers (the X.org linuxwacom driver)
>> doesn't need special-casing for the device.
>>
>> Note that it requires a user-space activation to switch to Mode 2,
>> using
>> bluetoothd (as the Bluetooth HID doesn't support
>> hid_output_raw_event).
>> Patch at:
>> http://cvs.fedoraproject.org/viewvc/rpms/bluez/devel/bluez-activate-wacom-mode2.patch?view=markup
>
> A couple of notes:
> - the wheel action is reversed, it's a simple fix, done locally
> - hidp and the Bluetooth sub-system says it can't probe the device
> with
> error "-14" when hid-wacom.ko isn't already loaded

what was the reason for not forcing HID to load the generic driver? I
lost track on how HID is suppose to handle these. You could also
introduce a phony export like we do with L2CAP to ensure it is loaded
even if userspace isn't ready to handle this dependency.

> - I'm getting oopses in hci_conn_del() when the device goes away
> (eg. I
> turn it off).

Try to run a net-next-2.6 or bluetooth-next-2.6 kernel. Kyle should
have merged these patches into the latest rawhide kernel.

> I'd appreciate any help making this just a tad more stable.
>
> Any comments about the code itself?

The main parsing function looks highly complicated and could either
use more comments or break into two.

if ((wdata->tool = tool)) {
input_report_key(input, tool, 1);
need_sync = 1;
}

Don't do the assignment in the if clause test. Did you run
checkpatch.pl on it?

Regards

Marcel