Skip to contentSkip to conversation

usbhid: HID device simple driver interface

PATCH In review liyu 3 participants 4 messages Jul 26, 2006 – Jul 27, 2006 USB

lore
Share & tools
lore

liyu

author
#0 lore raw

PATCH 2/2 usbhid: HID device simple driver interface

==================================
HID device simple driver interface
==================================

This patch include the header file for this API.

I am sorry for sendding patches in the attachment, beacause of my
mail client always break TAB into some spaces.

Good luck.

-Liyu

==================================
HID device simple driver interface
==================================

This patch include the header file for this API.

drivers/usb/input/hid-simple.h renamed from drivers/usb/input.orig/hid-simple.h +106 -0
diff -Naurp linux-2.6.17.6/drivers/usb/input.orig/hid-simple.h linux-2.6.17.6/drivers/usb/input/hid-simple.h
@@ -0,0 +1,106 @@
+/*
+ *  NOTE:
+ *	For use me , you must include hid.h in your source first. 
+ */
+#ifndef __HID_SIMPLE_H
+#define __HID_SIMPLE_H
+
+/* list_*_entry like interface */
+/* FIXME: Would you like move them to include/linux/list.h ? */
+#define list_for_each_continue(pos, head) \
+	for ((pos) = (pos)->next; \
+	prefetch((pos)->next), (pos) != (head); \
+	(pos) = (pos)->next)
+#define list_prepare(pos, head) ((pos) ? : head)
+
+#include <linux/usb.h>
+/***** The private section for simple device implement only *****/
+
+/* 
+ *  matched_device record one device which hid-subsystem handle, it may 
+ *  be one simple device can not handle.
+ *
+ *  The element of matched_device list is inserted at hidinput_connect(), 
+ *  and is removed  at hidinput_disconnect().
+ */ 
+struct matched_device {
+	struct usb_interface *intf;
+	struct list_head node;
+};
+
+/* 
+ *  raw_simple_driver record one device which hid simple device handle.
+ *  It used as one member of hid_simple_driver.
+ */ 
+
+struct raw_simple_device {
+	struct usb_interface *intf;
+	struct list_head node;
+};
+
+
+/* simple device internal flags */
+#define HIDINPUT_SIMPLE_SETUP_USAGE 0x1 /* the reverse is to call clear_usage */
+
+#define hidinput_simple_driver_setup_usage(hid) \
+do {\
+	if (hid->simple) {\
+		hid->simple->flags |= HIDINPUT_SIMPLE_SETUP_USAGE; \
+		hidinput_simple_driver_configure_usage(hid); \
+	}\
+} while (0)
+
+#define hidinput_simple_driver_clear_usage(hid) \
+do {\
+	if (hid->simple) {\
+		hid->simple->flags &= (~HIDINPUT_SIMPLE_SETUP_USAGE); \
+		hidinput_simple_driver_configure_usage(hid); \
+	}\
+} while (0)
+
+/* Note again here, you must include hid.h in your source first. */
+
+struct hidinput_simple_driver;
+void hidinput_simple_driver_configure_usage(struct hid_device *hid);
+int hidinput_simple_driver_init(struct hidinput_simple_driver *simple);
+int hidinput_simple_driver_push(struct hidinput_simple_driver *simple, 
+						struct matched_device *dev);
+void hidinput_simple_driver_pop(struct hidinput_simple_driver *simple, 
+						struct matched_device *dev);
+void hidinput_simple_driver_clear(struct hidinput_simple_driver *simple);
+int hidinput_simple_driver_connect(struct hidinput_simple_driver *simple, 
+							struct hid_device *hid);
+void hidinput_simple_driver_disconnect(struct hid_device *hid);
+
+
+/***** The private section end.  *****/
+
+
+/***** The public interface for simple device driver *****/
+struct hidinput_simple_driver {
+/* private */
+	struct list_head node; /* link with simple_drivers_list */
+	struct list_head raw_devices;
+	int flags;
+/* public */
+	char *name;
+	int (*connect)(struct hid_device *, struct hid_input *);	
+	void (*setup_usage)(struct hid_field *,   struct hid_usage *);
+	int (*pre_event)(const struct hid_device *, const struct hid_field *,
+					const struct hid_usage *, const __s32,
+					const struct pt_regs *regs);
+	int (*post_event)(const struct hid_device *, const struct hid_field *,
+					const struct hid_usage *, const __s32,
+					const struct pt_regs *regs);
+	void (*clear_usage)(struct hid_field *,   struct hid_usage *);
+	void (*disconnect)(struct hid_device *, struct hid_input *);
+	void *private;
+	struct usb_device_id *id_table;
+};
+
+
+int hidinput_register_simple_driver(struct hidinput_simple_driver *device);
+void hidinput_unregister_simple_driver(struct hidinput_simple_driver *device);
+
+/********************* The public section end ***********/
+#endif /* __HID_SIMPLE_H */
#1 lore raw
Quoted 4 lines · liyu

==================================
HID device simple driver interface
==================================

Quoted 9 lines

This patch include the header file for this API.

I am sorry for sendding patches in the attachment, beacause of my
mail client always break TAB into some spaces.

Good luck.

-Liyu

First of all, you should include your patches inline. That way one can easily comment on them. I only quickly glanced at it, and I am not sure about why you need the additional list_ macros. Also, several of your macros do something like this:

+#define hidinput_simple_driver_setup_usage(hid) \
+do {\
+ if (hid->simple) {\
+ hid->simple->flags |= HIDINPUT_SIMPLE_SETUP_USAGE; \
+ hidinput_simple_driver_configure_usage(hid); \
+ }\
+} while (0)

You should use (hid) instead of hid. Because of how the pre-processor works.

Josef "Jeff" Sipek.

Signature · 2 lines
If I have trouble installing Linux, something is wrong. Very wrong.
		- Linus Torvalds
#2 lore raw
Quoted 2 lines · Josef Sipek

You should use (hid) instead of hid. Because of how the pre-processor works.

Or, even better, use an inline function instead of a macro whereever possible.

One more thing, the description for patch 1 can probably be refined a bit more and put into Documentation/somewhere as a new file.

Regarding the split of the patch, it's usually a bad idea to put the header file into a separate patch from its users. E.g. if someone during debugging tries to revert patch 2 but not patch 1, he ends up with a broken build.

	Arnd <><
author
#3 lore raw

Well, That change from macro to inline function is more bettter idea.

Regarding the Documentation/newwhere, I also have such plan to
complete it, but need some time.
We will have it.

Regarding list_* marco, I really apologize, they wrote when I
implemented matched_lock/simple_lock
as spin lock. As I found this design have some restrictions, I replace spin lock with semaphore, but leave these macro in header. I will clear them.

For Patch splitting, I can not find more words in Documentation/,
however, It seem Arnd are right.
The next version of this patch will join core patch and header patch.

Last, I had tried again and again, but mail client still replace TAB
with spaces, I think I should replace it with another.

Thanks a lot.

Quoted 26 lines · Arnd Bergmann
Quoted 3 lines · Josef Sipek

You should use (hid) instead of hid. Because of how the pre-processor works.

Or, even better, use an inline function instead of a macro whereever possible.

One more thing, the description for patch 1 can probably be refined a bit more and put into Documentation/somewhere as a new file.

Regarding the split of the patch, it's usually a bad idea to put the header file into a separate patch from its users. E.g. if someone during debugging tries to revert patch 2 but not patch 1, he ends up with a broken build.

	Arnd <><

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/