Re: [PATCH] i2c: i2c-omap: fix interrupt flood during resume
pe, 2012-10-05 kello 15:07 -0700, Kevin Hilman kirjoitti:
> +Grygorii (who's been working on various I2C related suspend/resume
>issues also)
>
> Hi Kalle,
>
> Kalle Jokiniemi writes:
>
> > The resume_noirq enables interrupts one-by-one starting from
> > first one. Now if the wake up event for suspend came from i2c
> > device, the i2c bus irq gets enabled before the threaded
> > i2c device irq, causing a flood of i2c bus interrupts as the
> > threaded irq that should clear the event is not enabled yet.
>
> Ugh, another reason we need some sort of driver dependency tracking in the
> driver model.
>
> > Fixed the issue by adding suspend_late and resume_early
> > functions that keep i2c bus interrupts disabled until
> > resume_noirq has run completely.
>
> Hmm, any reason we couldn't put the disable in the .suspend_noirq
> callback?
I just liked the symmetry of late/early.
> The reason being is that other drivers might use I2C in their
> suspend or late_suspend callbacks. I'm not aware of any using I2C in
> their late_suspend callbacks in mainline, but in theory I2C should work
> all the way through the late callbacks.
Makes sense, I'll change that.
>
> I know it might look strange to have a disable_irq() in the noirq
> callback, since IRQs are already disabled at that point, but a comment
> stating that it's just there to balance the (re)enable in the
> early_resume callback should be clear.
>
> Some other minor comments below...
>
> > Issue was detected doing a wake up from autosleep with
> > twl4030 power key on N9. Patch tested on N9.
> >
> > Signed-off-by: Kalle Jokiniemi
> > ---
> > drivers/i2c/busses/i2c-omap.c | 37 +
> > 1 files changed, 37 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index 801df60..b77b0c2 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -1158,6 +1158,35 @@ omap_i2c_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_SUSPEND
>
> This should be CONFIG_PM_SLEEP in order to cover hibernation also.
Ok, I'll fix these, thanks.
>
> > +static int omap_i2c_suspend_late(struct device *dev)
> > +{
> > +
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> > +
> > + /*
> > +* The noirq_resume enables the interrupts one by one,
> > +* this causes a interrupt flood if the SW irq actually reading
> > +* event from i2c device is enabled only after i2c bus irq, as the
> > +* irq that should clear the event is still disabled. We have to
> > +* disable the bus irq until all other irqs have been enabled.
> > +*/
>
> This comment probably belongs in the resume handler.
Ok. I'll move it.
- Kalle
>
> > + disable_irq(_dev->irq);
> > + return 0;
> > +}
> > +
> > +static int omap_i2c_resume_early(struct device *dev)
> > +{
> > +
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> > +
> > + enable_irq(_dev->irq);
> > +
> > + return 0;
> > +}
> > +#endif
> > #ifdef CONFIG_PM_RUNTIME
> > static int omap_i2c_runtime_suspend(struct device *dev)
> > {
> > @@ -1178,10 +1207,18 @@ static int omap_i2c_runtime_resume(struct device
> > *dev)
> >
> > return 0;
> > }
> > +#endif
> >
> > +#if defined(CONFIG_SUSPEND) || defined(CONFIG_PM_RUNTIME)
>
> #ifdef CONFIG_PM will cover this
>
> > static struct dev_pm_ops omap_i2c_pm_ops = {
> > +#ifdef CONFIG_SUSPEND
>
> again, CONFIG_PM_SLEEP here
>
> > + .suspend_late = omap_i2c_suspend_late,
> > + .resume_early = omap_i2c_resume_early,
> > +#endif
> > +#ifdef CONFIG_PM_RUNTIME
> > .runtime_suspend = omap_i2c_runtime_suspend,
> > .runtime_resume = omap_i2c_runtime_resume,
> > +#endif
> > };
> > #define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
> > #else
>
> Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
On Sun, 7 Oct 2012 18:57:36 +0200, Benjamin Tissoires wrote: > On Sun, Oct 7, 2012 at 4:28 PM, Jean Delvare wrote: > > On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: > >> + u16 readRegister = ihid->hdesc.wDataRegister; > > > > This is missing le16_to_cpu(). > > I agree this is awful, but not putting it allows me to not have to > check the endianness when I'm using it. > But I may be totally wrong on this. I'm afraid I don't follow you. I want to see: u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister); If you don't do that, your driver is broken on bigendian systems. And there's no need to "check the endianness when you're using it", the above should be enough for things to work just fine. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
Hello Jean, many thanks for this detailed review. I agree to almost all of your comments, so I didn't add an 'ok' after each remark. This review will give me some work, but the driver will be much better. On Sun, Oct 7, 2012 at 4:28 PM, Jean Delvare wrote: > Salut Benjamin, > > On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: >> From: Benjamin Tissoires >> >> Microsoft published the protocol specification of HID over i2c: >> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx >> >> This patch introduces an implementation of this protocol. >> >> This implementation does not includes the ACPI part of the specification. >> This will come when ACPI 5.0 devices will be available. >> >> Once the ACPI part will be done, OEM will not have to declare HID over I2C >> devices in their platform specific driver. >> >> Signed-off-by: Benjamin Tissoires >> --- >> >> Hi, >> >> this is finally my first implementation of HID over I2C. >> >> This has been tested on an Elan Microelectronics HID over I2C device, with >> a Samsung Exynos 4412 board. >> >> Any comments are welcome. > > Code review follows. It is by no way meant to be complete, as I don't > know a thing about HID. I hope you'll find it useful nevertheless. > >> >> Cheers, >> Benjamin >> >> drivers/i2c/Kconfig |8 + >> drivers/i2c/Makefile|1 + >> drivers/i2c/i2c-hid.c | 1027 >> +++ >> include/linux/i2c/i2c-hid.h | 35 ++ >> 4 files changed, 1071 insertions(+) >> create mode 100644 drivers/i2c/i2c-hid.c >> create mode 100644 include/linux/i2c/i2c-hid.h >> >> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig >> index 5a3bb3d..5adf65a 100644 >> --- a/drivers/i2c/Kconfig >> +++ b/drivers/i2c/Kconfig >> @@ -47,6 +47,14 @@ config I2C_CHARDEV >> This support is also available as a module. If so, the module >> will be called i2c-dev. >> >> +config I2C_HID >> + tristate "HID over I2C bus" > > You are definitely missing dependencies here, CONFIG_HID at least. > >> + help >> + Say Y here to use the HID over i2c protocol implementation. >> + >> + This support is also available as a module. If so, the module >> + will be called i2c-hid. >> + >> config I2C_MUX >> tristate "I2C bus multiplexing support" >> help >> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile >> index beee6b2..8f38116 100644 >> --- a/drivers/i2c/Makefile >> +++ b/drivers/i2c/Makefile >> @@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o >> obj-$(CONFIG_I2C)+= i2c-core.o >> obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o >> obj-$(CONFIG_I2C_CHARDEV)+= i2c-dev.o >> +obj-$(CONFIG_I2C_HID)+= i2c-hid.o >> obj-$(CONFIG_I2C_MUX)+= i2c-mux.o >> obj-y+= algos/ busses/ muxes/ >> >> diff --git a/drivers/i2c/i2c-hid.c b/drivers/i2c/i2c-hid.c >> new file mode 100644 >> index 000..eb17d8c >> --- /dev/null >> +++ b/drivers/i2c/i2c-hid.c >> @@ -0,0 +1,1027 @@ >> +/* >> + * HID over I2C protocol implementation >> + * >> + * Copyright (c) 2012 Benjamin Tissoires >> + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France >> + * >> + * This code is partly based on "USB HID support for Linux": >> + * >> + * Copyright (c) 1999 Andreas Gal >> + * Copyright (c) 2000-2005 Vojtech Pavlik >> + * Copyright (c) 2005 Michael Haboustak for Concept2, >> Inc >> + * Copyright (c) 2007-2008 Oliver Neukum >> + * Copyright (c) 2006-2010 Jiri Kosina >> + * >> + * This file is subject to the terms and conditions of the GNU General >> Public >> + * License. See the file COPYING in the main directory of this archive for >> + * more details. >> + */ >> + >> +#include >> +#include >> +#include > > I don't think you need to include that one, everything irq-related you > need comes with below. The header comment in that > file actually says: "Please do not include this file in generic code." > >> +#include > > Your driver makes no use of GPIO. > >> +#include >> +#include >> +#include >> +#include >> +#include > > You are missing for > spin_lock_irq()/spin_unlock_irq(), for > device_may_wakeup(), for wait_event_timeout(), > for IS_ERR(), for strcat()/memcpy(), > for list_for_each_entry() and for > msecs_to_jiffies(). I'd suggest including as well, for > sprintf() if nothing else, and for WARN_ON(). It seems like I've been to lazy with the includes... Thanks! > >> + >> +#include >> + >> +#include >> +#include >> +#include > > I don't think you using anything from , do you? oops... > >> + >> +#define DRIVER_NAME "i2chid" >> +#define DRIVER_DESC "HID over I2C core driver" > > I see little interest in this define, as you're only using it once. > Even DRIVER_NAME isn't so useful, 2 of the 3 occurrences would be > replaced with client->name.& > >> + >> +#define I2C_HID_COMMAND_TRIES3 >> + >> +/* fla
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
Le 7 oct. 2012 à 18:07, Benjamin Tissoires a écrit : >>> >>> Basically, to me this all boils down to the question -- what is more >>> important: low-level transport being used, or the general function of the >>> device? >>> >>> To me, it's the latter, and as such, everything would belong under >>> drivers/hid. >> >> Then shouldn't is be drivers/input, rather? > > Ouch, it will introduce more and more complexity. Purely rhetorical question, I agree. But still. > > It seems that hid transport layers should go in drivers/hid. > However, I don't like mixing the transport layer and the final > drivers. Maybe this is the time to rework a little bit the tree. > To minimize the moves, we could introduce: > drivers/hid/busses/usb > drivers/hid/busses/i2c > drivers/hid/busses/bluetooth What about creating drivers/hid/core and move all generic stuff there? That is: drivers/hid/core drivers/hid/usb drivers/hid/i2c drivers/hid/bluetooth Cheers, St. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
On Sat, Oct 6, 2012 at 11:39 PM, Stéphane Chatty wrote: > > Le 6 oct. 2012 à 23:28, Jiri Kosina a écrit : > >> On Sat, 6 Oct 2012, Jiri Kosina wrote: >> My vote is a clear 3. It took me a few years to kick all users (as opposed to implementers) of i2c from drivers/i2c and finding them a proper home, I'm not going to accept new intruders. Grouping drivers according to what they implement makes it a lot easier to share code and ideas between related drivers. If you want to convince yourself, just imagine the mess it would be if all drivers for PCI devices lived under drivers/pci. >>> >>> This is more or less consistent with my original opinion when I was >>> refactoring the HID layer out of the individual drivers a few years ago. >>> >>> But Marcel objected that he wants to keep all the bluetooth-related >>> drivers under net/bluetooth, and I didn't really want to push hard against >>> this, because I don't have really super-strong personal preference either >>> way. >>> >>> But we definitely can use this oportunity to bring this up for discussion >>> again. >> >> Basically, to me this all boils down to the question -- what is more >> important: low-level transport being used, or the general function of the >> device? >> >> To me, it's the latter, and as such, everything would belong under >> drivers/hid. > > Then shouldn't is be drivers/input, rather? Ouch, it will introduce more and more complexity. It seems that hid transport layers should go in drivers/hid. However, I don't like mixing the transport layer and the final drivers. Maybe this is the time to rework a little bit the tree. To minimize the moves, we could introduce: drivers/hid/busses/usb drivers/hid/busses/i2c drivers/hid/busses/bluetooth Cheers, Benjamin > > St. > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
Hi Jean, Thanks for the comments, the tests and the review. I'm going to try to answer most of the remarks, so here is the first: On Sat, Oct 6, 2012 at 10:04 PM, Jean Delvare wrote: > Hi Benjamin, > [...] >a > ERROR: "hiddev_report_event" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hiddev_disconnect" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hiddev_connect" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hid_pidff_init" [drivers/i2c/i2c-hid.ko] undefined! > make[1]: *** [__modpost] Erreur 1 > make: *** [modules] Erreur 2 > > This is because these functions aren't exported and I tried to build > i2c-hid as a module.BTW I see that these functions are part of the > usbhid driver, which looks seriously wrong. If these functions are > transport layer-independent, they should be moved to the hid-code or > some sub-module. One should be able to enable HID-over-I2C without > HID-over-USB. It looks like I've been mislead by the generic names of these functions. By looking at the code, it appears that I can not use them in their current state as they are all usb related. I think that it will need some work to refactor the general part of hiddev so that I2C and bluetooth can use them. For the next version, I'll just drop hiddev and pidff support. I'm not sure we won't ever find a ff device connected through i2c, but the hiddev support will surely be needed. Cheers, Benjamin > > -- > Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/11] introduce macros for i2c_msg initialization
This patch set introduces some macros for describing how an i2c_msg is
being initialized. There are three macros: I2C_MSG_READ, for a read
message, I2C_MSG_WRITE, for a write message, and I2C_MSG_OP, for some other
kind of message, which is expected to be very rarely used.
Some i2c_msg initializations have been updated accordingly using the
following semantic patch:
//
@r@
field list[n] ds;
type T;
identifier i;
@@
struct i2c_msg {
ds
T i;
...
};
@@
initializer list[r.n] is;
expression a;
identifier nm;
identifier r.i;
@@
struct i2c_msg nm = {
is,
- a
+ .i = a
,...
};
@@
initializer list[r.n] is;
expression a;
identifier nm;
identifier r.i;
@@
struct i2c_msg nm[...] = { ..., {
is,
- a
+ .i = a
,...}, ...};
@@
initializer list[r.n] is;
expression a;
identifier nm;
identifier r.i;
@@
struct i2c_msg nm[] = { ..., {
is,
- a
+ .i = a
,...}, ...};
//
// ensure everyone has all fields, pointer case first
@rt@
type T;
identifier i;
@@
struct i2c_msg {
...
T *i;
...
};
@t1@
expression e;
identifier nm,rt.i;
position p;
@@
struct i2c_msg nm = {@p
.i = e,
};
@@
identifier nm,rt.i;
position p!= t1.p;
@@
struct i2c_msg nm = {@p
+ .i = NULL,
...
};
@t2@
expression e;
identifier nm,rt.i;
position p;
@@
struct i2c_msg nm[] = { ..., {@p
.i = e,
}, ...};
@@
identifier nm,rt.i;
position p!= t2.p;
@@
struct i2c_msg nm[] = { ..., {@p
+ .i = NULL,
...
}, ...};
@t3@
expression e;
identifier nm,rt.i;
position p;
@@
struct i2c_msg nm[...] = { ..., {@p
.i = e,
}, ...};
@@
identifier nm,rt.i;
position p!= t3.p;
@@
struct i2c_msg nm[...] = { ..., {@p
+ .i = NULL,
...
}, ...};
// -
@f1@
expression e;
identifier nm,r.i;
position p;
@@
struct i2c_msg nm = {@p
.i = e,
};
@@
identifier nm,r.i;
position p!= f1.p;
@@
struct i2c_msg nm = {@p
+ .i = 0,
...
};
@f2@
expression e;
identifier nm,r.i;
position p;
@@
struct i2c_msg nm[] = { ..., {@p
.i = e,
}, ...};
@@
identifier nm,r.i;
position p!= f2.p;
@@
struct i2c_msg nm[] = { ..., {@p
+ .i = 0,
...
}, ...};
@f3@
expression e;
identifier nm,r.i;
position p;
@@
struct i2c_msg nm[...] = { ..., {@p
.i = e,
}, ...};
@@
identifier nm,r.i;
position p!= f3.p;
@@
struct i2c_msg nm[...] = { ..., {@p
+ .i = 0,
...
}, ...};
//
@@
constant c;
identifier x;
type T,T1;
T[] b;
@@
struct i2c_msg x =
{ .buf = \((T1)b\|(T1)(&b)\|(T1)(&b[0])\), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
};
@@
constant c;
identifier x;
type T,T1;
T[] b;
@@
struct i2c_msg x[...] = {...,
{ .buf = \((T1)b\|(T1)(&b)\|(T1)(&b[0])\), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
} ,...};
@@
constant c;
identifier x;
type T,T1;
T[] b;
@@
struct i2c_msg x[] = {...,
{ .buf = \((T1)b\|(T1)(&b)\|(T1)(&b[0])\), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
} ,...};
@@
constant c;
identifier x;
type T1;
expression b;
@@
struct i2c_msg x =
{ .buf = (T1)(&b), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
};
@@
constant c;
identifier x;
type T1;
expression b;
@@
struct i2c_msg x[...] = {...,
{ .buf = (T1)(&b), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
} ,...};
@@
constant c;
identifier x;
type T1;
expression b;
@@
struct i2c_msg x[] = {...,
{ .buf = (T1)(&b), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
} ,...};
//
@@
expression a,b,c;
identifier x;
@@
struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;
@@
expression a,b,c;
identifier x;
@@
struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;
// has to come before the next rule, which matcher fewer fields
@@
expression a,b,c,d;
identifier x;
@@
struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
//
@@
expression a,b,c;
identifier x;
@@
struct i2c_msg x[] = {...,
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
,...};
@@
expression a,b,c;
identifier x;
@@
struct i2c_msg x[] = {...,
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
,...};
@@
expression a,b,c,d;
identifier x;
@@
struct i2c_msg x[] = {...,
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
,...};
//
@@
expression a,b,c;
identifier x;
@@
struct i2c_msg x[...] = {...,
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
,...};
@@
expression a,b,c;
identifier x;
@@
struct i2c_msg x[...] = {...,
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
,...};
@@
expression a,b,c,d;
identifier x;
@@
struct i2c_msg x[...] = {...,
- {.addr = a, .buf = b, .len = c, .flags = d}
+
[PATCH 1/13] include/linux/i2c.h: introduce macros for i2c_msg initialization
From: Julia Lawall
This patch introduces some macros for describing how an i2c_msg is being
initialized. There are three macros: I2C_MSG_READ, for a read message,
I2C_MSG_WRITE, for a write message, and I2C_MSG_OP, for some other kind of
message, which is expected to be very rarely used.
Signed-off-by: Julia Lawall
---
include/linux/i2c.h |6 ++
1 file changed, 6 insertions(+)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 94aed0c..885ebea 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -556,6 +556,12 @@ struct i2c_msg {
__u8 *buf; /* pointer to msg data */
};
+#define I2C_MSG_OP(_addr, _buf, _len, _flags) \
+ { .addr = _addr, .buf = _buf, .len = _len, .flags = _flags }
+
+#define I2C_MSG_WRITE(addr, buf, len) I2C_MSG_OP(addr, buf, len, 0)
+#define I2C_MSG_READ(addr, buf, len) I2C_MSG_OP(addr, buf, len, I2C_M_RD)
+
/* To determine what functionality is present */
#define I2C_FUNC_I2C 0x0001
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
Salut Benjamin, On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: > From: Benjamin Tissoires > > Microsoft published the protocol specification of HID over i2c: > http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx > > This patch introduces an implementation of this protocol. > > This implementation does not includes the ACPI part of the specification. > This will come when ACPI 5.0 devices will be available. > > Once the ACPI part will be done, OEM will not have to declare HID over I2C > devices in their platform specific driver. > > Signed-off-by: Benjamin Tissoires > --- > > Hi, > > this is finally my first implementation of HID over I2C. > > This has been tested on an Elan Microelectronics HID over I2C device, with > a Samsung Exynos 4412 board. > > Any comments are welcome. Code review follows. It is by no way meant to be complete, as I don't know a thing about HID. I hope you'll find it useful nevertheless. > > Cheers, > Benjamin > > drivers/i2c/Kconfig |8 + > drivers/i2c/Makefile|1 + > drivers/i2c/i2c-hid.c | 1027 > +++ > include/linux/i2c/i2c-hid.h | 35 ++ > 4 files changed, 1071 insertions(+) > create mode 100644 drivers/i2c/i2c-hid.c > create mode 100644 include/linux/i2c/i2c-hid.h > > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig > index 5a3bb3d..5adf65a 100644 > --- a/drivers/i2c/Kconfig > +++ b/drivers/i2c/Kconfig > @@ -47,6 +47,14 @@ config I2C_CHARDEV > This support is also available as a module. If so, the module > will be called i2c-dev. > > +config I2C_HID > + tristate "HID over I2C bus" You are definitely missing dependencies here, CONFIG_HID at least. > + help > + Say Y here to use the HID over i2c protocol implementation. > + > + This support is also available as a module. If so, the module > + will be called i2c-hid. > + > config I2C_MUX > tristate "I2C bus multiplexing support" > help > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile > index beee6b2..8f38116 100644 > --- a/drivers/i2c/Makefile > +++ b/drivers/i2c/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o > obj-$(CONFIG_I2C)+= i2c-core.o > obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o > obj-$(CONFIG_I2C_CHARDEV)+= i2c-dev.o > +obj-$(CONFIG_I2C_HID)+= i2c-hid.o > obj-$(CONFIG_I2C_MUX)+= i2c-mux.o > obj-y+= algos/ busses/ muxes/ > > diff --git a/drivers/i2c/i2c-hid.c b/drivers/i2c/i2c-hid.c > new file mode 100644 > index 000..eb17d8c > --- /dev/null > +++ b/drivers/i2c/i2c-hid.c > @@ -0,0 +1,1027 @@ > +/* > + * HID over I2C protocol implementation > + * > + * Copyright (c) 2012 Benjamin Tissoires > + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France > + * > + * This code is partly based on "USB HID support for Linux": > + * > + * Copyright (c) 1999 Andreas Gal > + * Copyright (c) 2000-2005 Vojtech Pavlik > + * Copyright (c) 2005 Michael Haboustak for Concept2, > Inc > + * Copyright (c) 2007-2008 Oliver Neukum > + * Copyright (c) 2006-2010 Jiri Kosina > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file COPYING in the main directory of this archive for > + * more details. > + */ > + > +#include > +#include > +#include I don't think you need to include that one, everything irq-related you need comes with below. The header comment in that file actually says: "Please do not include this file in generic code." > +#include Your driver makes no use of GPIO. > +#include > +#include > +#include > +#include > +#include You are missing for spin_lock_irq()/spin_unlock_irq(), for device_may_wakeup(), for wait_event_timeout(), for IS_ERR(), for strcat()/memcpy(), for list_for_each_entry() and for msecs_to_jiffies(). I'd suggest including as well, for sprintf() if nothing else, and for WARN_ON(). > + > +#include > + > +#include > +#include > +#include I don't think you using anything from , do you? > + > +#define DRIVER_NAME "i2chid" > +#define DRIVER_DESC "HID over I2C core driver" I see little interest in this define, as you're only using it once. Even DRIVER_NAME isn't so useful, 2 of the 3 occurrences would be replaced with client->name.& > + > +#define I2C_HID_COMMAND_TRIES3 > + > +/* flags */ > +#define I2C_HID_STARTED (1 << 0) > +#define I2C_HID_OUT_RUNNING (1 << 1) > +#define I2C_HID_IN_RUNNING (1 << 2) > +#define I2C_HID_RESET_PENDING(1 << 3) > +#define I2C_HID_SUSPENDED(1 << 4) 3 of these are never used, so why define them? > + > +#define I2C_HID_PWR_ON 0x00 > +#define I2C_HID_PWR_SLEEP0x01 > + > +/* debug option */ > +static bool debug = false; checkpatch.pl says: ERROR: do not initialise statics to 0 or NULL #206:
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
On Sat, 6 Oct 2012 23:27:47 +0200, Stéphane Chatty wrote: > Le 6 oct. 2012 à 23:11, Jean Delvare a écrit : > > On Sat, 6 Oct 2012 22:30:00 +0200, Stéphane Chatty wrote: > >> This is a question I asked a few months back, but apparently not all > >> points of views had been expressed at the time. Currently, HID-over-USB > >> lives in drivers/hid, but HID-over-BT lives in drivers/bluetooth. When I > >> asked, Jiri explained that he maintained HID-over-USB and Marcel > >> maintained HID-over-BT, which explained the choices made. Let's try to > >> summarize what we know now: > >> > >> The question is what drives the choice of where to put HID-over-XXX, among > >> the following > >> 1- who the maintainer is. Here, Benjamin will probably maintain this so it > >> does not help. > >> 2- dependencies. HID-over-XXX depends on HID as much as it depends on XXX, > >> so it does not help. > >> 3- data flow. Indeed, HID is a client of HID-over-XXX which is a client of > >> XXX. Are there other parts of the kernel where this drives the choice of > >> where YYY-over-XXX lives? > >> > >> Jiri, Marcel, Greg, others, any opinions? > > > > My vote is a clear 3. It took me a few years to kick all users (as > > opposed to implementers) of i2c from drivers/i2c and finding them a > > proper home, I'm not going to accept new intruders. Grouping drivers > > according to what they implement makes it a lot easier to share code > > and ideas between related drivers. If you want to convince yourself, > > just imagine the mess it would be if all drivers for PCI devices lived > > under drivers/pci. > > > Having no strong opinion myself, I'm trying to get to the bottom of this :-) > Here, I see two points that need clarification: > > - I'm under the impression that the situation is exactly opposite between i2c > and USB: drivers/usb contains lots of drivers for specific devices, but > HID-over-USB is in drivers/hid. I actually found this disturbing when reading > the HID code for the first time. Mmmm. Indeed I see a lot of drivers under drivers/usb. I'm glad I am not responsible for this subsystem ;) I think drivers/pci is a much cleaner example to follow. If nothing else, grouping drivers by functionality solves the problem of devices which can be accessed through multiple transport layers. For devices with multiple functions, we have drivers/mfd, specifically designed to make it possible to put support for each function into its dedicated location. > - given your explanation, I'd say that you would agree to 2 as well, if it > meant for instance that HID-over-I2C is neither in drivers/hid nor > drivers/i2c. Actually, you don't care whether it is 1, 2, or 3 that drives > the choice as long as HID-over-I2C is not in drivers/i2c, do you? :-) I do care that things are as consistent and logical as possible. I know sometimes there are borderline cases, or things done a certain way for historical reasons, but grouping by functionality seems the more logical and efficient as a rule of thumb. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
