=====================
HID device simple driver interface
=====================
Goal:
----------------
Let us write HID device driver more easier.
Basic idea:
---------------
Under current HID device driver development technique, We need
write one new interrupt handler to report event to input subsystem as
long as one new HID device come. However, the most of them have only
some extended keys, I think it seem break a fly on the wheel, which
write one new interrupt handler for this reason,
My idea is reuse the interrupt handler in hid-core.c. so we
write driver for new simple HID device will be more easier, and need not
touch hid core.
In essence, this interface just are some hooks in HID core.
Limitation:
----------------
The driver use this simple interface only can work with one
device at same time. In most time, this just is not a problem. if you
are going to make your driver can work with a bundle of devices at same
time, the I am sorry, this simple interface can not help you, and I am
afraid that driver is not one simple driver. Of course, any improvement
on this patch is welcome :)
Testing:
--------------
Tested on i386.
Usage:
---------------
Although this simple driver have not direct relation with device
driver core, but I still make its interface like
it on purpose.
The simple device has five methods:
1. int (*connect)(struct hid_device *);
2. void (*disconnect)(struct hid_device *);
When you simple device is connect with one real HID device, we
will call connect() method. To return 0 flag it complete its job
successfully. Any other value is looked as one error.
When the HID device that your simple device connect with is
down, or you unregister this simple device, we will call disconnect()
method.
3. void (*setup_usage)(struct hid_field *, struct hid_usage *);
4. void (*clear_usage)(struct hid_field *, struct hid_usage *);
The setup_usage() method is like hidinput_configure_usage() in
hid_input.c. You also can setup input_dev here. In most time, I think
you should
be fill the pointer slot for this method, elsewise the event() method do
not work for you at all.
The clear_usage() method is used to clear side-effect that came
from setup_usage() method, if they are there. Of course, you can do same
job in disconnect() method, but this method let your life more
simpler.
5. void (*event)(const struct hid_device *, const struct
hid_field *, const struct hid_usage *, const __s32, const struct pt_regs
*regs);
Its behavior is same with hidinput_hid_event() exactly. Note
again, if you do not correctly configure usage in setup_usage(), this
method do not work as you want.
All these method are optional, but if they are all NULL
pointers, what are you want *-)
Other information
............................
I am sorry that these patches are included in attachment, my
mail client alway transform TAB to eight spaces.
The attatchment include:
1. HID device simple driver interface
2. One sample use this interface.
3. Microsoft Natural Ergonomic Keyboard 4000 Driver use this
interface.
Happy new year.
On Thu, Jan 05, 2006 at 10:59:22AM +0800, liyu wrote:
> =====================
> HID device simple driver interface
> =====================
>
> Goal:
> ----------------
>
> Let us write HID device driver more easier.
>
>
> Basic idea:
> ---------------
>
> Under current HID device driver development technique, We need
> write one new interrupt handler to report event to input subsystem as
> long as one new HID device come. However, the most of them have only
> some extended keys, I think it seem break a fly on the wheel, which
> write one new interrupt handler for this reason,
> My idea is reuse the interrupt handler in hid-core.c. so we
> write driver for new simple HID device will be more easier, and need not
> touch hid core.
> In essence, this interface just are some hooks in HID core.
Vojtech, what do you think?
> Limitation:
> ----------------
>
> The driver use this simple interface only can work with one
> device at same time. In most time, this just is not a problem. if you
> are going to make your driver can work with a bundle of devices at same
> time, the I am sorry, this simple interface can not help you, and I am
> afraid that driver is not one simple driver. Of course, any improvement
> on this patch is welcome :)
But there can be many different devices of the same type, right? And
many different drivers all using this same interface, right? It doesn't
look that hard to have any limitation for this.
> +static void
> +hidinput_simple_device_bind_foreach(void)
> +{
> + struct hidinput_simple_device *simple=0;
> + struct matched_device *matched=0;
> + struct list_head *simple_node;
> + struct list_head *matched_node;
> + struct hid_device *hid;
You have tabs and spaces mixed together in lots of places in your patch,
please fix them all to be just tabs.
> + hid = usb_get_intfdata(matched->intf);
You also have a lot of lines that add trailing spaces, please fix that.
> + if ((simple->connect && 0==simple->connect(hid)) || !simple->connect) {
Why compare to 0? !simple->connect(hid) is the same, right?
> + if (!simple || !simple->name || simple->intf)
> + return -1;
Return proper negative error numbers please.
> + device_busy:
> + spin_lock(&simple_lock);
> + list_add(&simple->node, &simple_devices_list);
> + spin_unlock(&simple_lock);
> + return 1;
Should be a negative error number, not 1.
> +EXPORT_SYMBOL(hidinput_register_simple_device);
EXPORT_SYMBOL_GPL() for this and the other exports?
> @@ -471,7 +475,6 @@ struct hid_descriptor {
> #define resolv_event(a,b) do { } while (0)
> #endif
>
> -#endif
>
> #ifdef CONFIG_USB_HIDINPUT
> /* Applications from HID Usage Tables 4/8/99 Version 1.1 */
> @@ -515,3 +518,5 @@ static inline int hid_ff_event(struct hi
> return hid->ff_event(hid, input, type, code, value);
> return -ENOSYS;
> }
> +
> +#endif
Why move the #endif?
> +/*
> + * To give one simple device a configure usage chance.
> + * The most code of this function is copied from hidinput_connect()
> + */
> +void hidinput_simple_device_configure_usage(struct hid_device *hid)
> +{
> + struct hid_report *report;
> + int i, j, k;
> + void (*do_usage)(struct hid_field *, struct hid_usage *);
> +
> + if (!hid->simple)
> + return;
> + do_usage = 0;
Please run this through sparse and fix up the errors it produces (hint,
that should be NULL...)
> +struct nek4k_device {
> + struct hidinput_simple_device device;
> +};
Why create your own structure? You should be able to just use the
hidinput_simple_device instead, right?
> +#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,15))
> + struct input_dev *input = hidinput->input;
> +#else
> + struct input_dev *input = &hidinput->input;
> +#endif
No #ifdefs for versions please.
thanks,
greg k-h