[PATCH 2/2] usbhid: HID device simple driver interface

[PATCH 2/2] usbhid: HID device simple driver interface

[liyu]

[-- Attachment #1: Type: text/plain, Size: 302 bytes --]

==================================
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


[-- Attachment #2: usbhid-simple-driver-header.kernel-2.6.17.7.patch --]
[-- Type: text/x-patch, Size: 4008 bytes --]

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

    This patch include the header file for this API.
    
Signed-off-by:  Yu Li <liyu@ccoss.com.cn>

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
--- linux-2.6.17.6/drivers/usb/input.orig/hid-simple.h	1970-01-01 08:00:00.000000000 +0800
+++ linux-2.6.17.6/drivers/usb/input/hid-simple.h	2006-07-26 09:42:36.000000000 +0800
@@ -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 */

Re: [PATCH 2/2] usbhid: HID device simple driver interface

[Josef Sipek]

On Wed, Jul 26, 2006 at 06:41:53PM +0800, liyu wrote:
> ==================================
> 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
> 

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.

-- 
If I have trouble installing Linux, something is wrong. Very wrong.
		- Linus Torvalds

Re: [PATCH 2/2] usbhid: HID device simple driver interface

[Arnd Bergmann]

On Wednesday 26 July 2006 18:10, Josef Sipek wrote:
> 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 <><

Re: [PATCH 2/2] usbhid: HID device simple driver interface

[liyu]

    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.

Arnd Bergmann wrote:
> On Wednesday 26 July 2006 18:10, Josef Sipek wrote:
>   
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>