linux-next.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] i2c: Simplified CONFIG_I2C=n interface.
@ 2009-05-27  7:08 Paul Mundt
       [not found] ` <20090527070850.GA11221-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Mundt @ 2009-05-27  7:08 UTC (permalink / raw)
  To: Jean Delvare; +Cc: linux-next, linux-media, linux-i2c

Another day, another module-related failure due to the i2c interface
being used in code that optionally uses it:

ERROR: "i2c_new_device" [drivers/media/video/soc_camera.ko] undefined!
ERROR: "i2c_get_adapter" [drivers/media/video/soc_camera.ko] undefined!
ERROR: "i2c_put_adapter" [drivers/media/video/soc_camera.ko] undefined!
ERROR: "i2c_unregister_device" [drivers/media/video/soc_camera.ko] undefined!
make[2]: *** [__modpost] Error 1
make[1]: *** [modules] Error 2
make: *** [sub-make] Error 2

In the interest of not continually inserting i2c ifdefs in to every
driver that supports an optional i2c interface, this provides a stubbed
set of interfaces for the CONFIG_I2C=n case.

I've covered the obvious ones that cause the majority of the build
failures, anything more involved really ought to have its dependencies
fixed instead.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

---

 include/linux/i2c.h |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index ad25805..ba73cd0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -277,6 +277,8 @@ struct i2c_board_info {
 	.type = dev_type, .addr = (dev_addr)
 
 
+#ifdef CONFIG_I2C
+
 /* Add-on boards should register/unregister their devices; e.g. a board
  * with integrated I2C, a config eeprom, sensors, and a codec that's
  * used in conjunction with the primary hardware.
@@ -301,6 +303,33 @@ i2c_new_dummy(struct i2c_adapter *adap, u16 address);
 
 extern void i2c_unregister_device(struct i2c_client *);
 
+#else
+
+static inline struct i2c_client *
+i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
+{
+	return NULL;
+}
+
+static inline struct i2c_client *
+i2c_new_probed_device(struct i2c_adapter *adap,
+		      struct i2c_board_info *info,
+		      unsigned short const *addr_list)
+{
+	return NULL;
+}
+
+static inline struct i2c_client *
+i2c_new_dummy(struct i2c_adapter *adap, u16 address)
+{
+	return NULL;
+}
+
+static inline void i2c_unregister_device(struct i2c_client *client)
+{
+}
+#endif /* CONFIG_I2C */
+
 /* Mainboard arch_initcall() code should register all its I2C devices.
  * This is done at arch_initcall time, before declaring any i2c adapters.
  * Modules for add-on boards must use other calls.
@@ -415,6 +444,7 @@ struct i2c_client_address_data {
 
 /* ----- functions exported by i2c.o */
 
+#ifdef CONFIG_I2C
 /* administration...
  */
 extern int i2c_add_adapter(struct i2c_adapter *);
@@ -460,6 +490,85 @@ static inline u32 i2c_get_functionality(struct i2c_adapter *adap)
 	return adap->algo->functionality(adap);
 }
 
+#else
+
+static inline int i2c_add_adapter(struct i2c_adapter *adap)
+{
+	return -ENODEV;
+}
+
+static inline int i2c_del_adapter(struct i2c_adapter *adap)
+{
+	return -ENODEV;
+}
+
+static inline int i2c_add_numbered_adapter(struct i2c_adapter *adap)
+{
+	return -ENODEV;
+}
+
+static inline int i2c_register_driver(struct module *module,
+				      struct i2c_driver *driver)
+{
+	return -ENODEV;
+}
+
+static inline void i2c_del_driver(struct i2c_driver *driver)
+{
+}
+
+static inline int i2c_add_driver(struct i2c_driver *driver)
+{
+	return -ENODEV;
+}
+
+static inline int __deprecated i2c_attach_client(struct i2c_client *client)
+{
+	return -EINVAL;
+}
+
+static inline int __deprecated i2c_detach_client(struct i2c_client *client)
+{
+	return -EINVAL;
+}
+
+static inline struct i2c_client *i2c_use_client(struct i2c_client *client)
+{
+	return NULL;
+}
+
+static inline void i2c_release_client(struct i2c_client *client)
+{
+}
+
+static inline void i2c_clients_command(struct i2c_adapter *adap,
+				       unsigned int cmd, void *arg)
+{
+}
+
+static inline int i2c_probe(struct i2c_adapter *adapter,
+		const struct i2c_client_address_data *address_data,
+		int (*found_proc) (struct i2c_adapter *, int, int))
+{
+	return -ENODEV;
+}
+
+static inline struct i2c_adapter *i2c_get_adapter(int id)
+{
+	return NULL;
+}
+
+static inline void i2c_put_adapter(struct i2c_adapter *adap)
+{
+}
+
+/* Return the functionality mask */
+static inline u32 i2c_get_functionality(struct i2c_adapter *adap)
+{
+	return 0;
+}
+#endif /* CONFIG_I2C */
+
 /* Return 1 if adapter supports everything we need, 0 if not. */
 static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
 {

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: Simplified CONFIG_I2C=n interface.
       [not found] ` <20090527070850.GA11221-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
@ 2009-05-27  7:18   ` Jean Delvare
       [not found]     ` <20090527091831.26b60d6d-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
  2009-05-27 12:01     ` Mark Brown
  0 siblings, 2 replies; 8+ messages in thread
From: Jean Delvare @ 2009-05-27  7:18 UTC (permalink / raw)
  To: Paul Mundt
  Cc: linux-next-u79uwXL29TY76Z2rM5mHXA,
	linux-media-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA

On Wed, 27 May 2009 16:08:50 +0900, Paul Mundt wrote:
> Another day, another module-related failure due to the i2c interface
> being used in code that optionally uses it:
> 
> ERROR: "i2c_new_device" [drivers/media/video/soc_camera.ko] undefined!
> ERROR: "i2c_get_adapter" [drivers/media/video/soc_camera.ko] undefined!
> ERROR: "i2c_put_adapter" [drivers/media/video/soc_camera.ko] undefined!
> ERROR: "i2c_unregister_device" [drivers/media/video/soc_camera.ko] undefined!
> make[2]: *** [__modpost] Error 1
> make[1]: *** [modules] Error 2
> make: *** [sub-make] Error 2
> 
> In the interest of not continually inserting i2c ifdefs in to every
> driver that supports an optional i2c interface, this provides a stubbed
> set of interfaces for the CONFIG_I2C=n case.
> 
> I've covered the obvious ones that cause the majority of the build
> failures, anything more involved really ought to have its dependencies
> fixed instead.

Violent nack. Drivers which optionally use I2C are a minority.
Designing them in such a way that a single #ifdef CONFIG_I2C will make
them work can't be that hard, really. Not to mention that having a
dozen stubs in i2c.h in the CONFIG_I2C=n case won't save you much work
at the driver level anyway, because you certainly need to run different
code paths depending on how the device is connected, and you also have
to differentiate between the "I2C support is missing" case and the "I2C
device registration failed" case, etc.

-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: Simplified CONFIG_I2C=n interface.
       [not found]     ` <20090527091831.26b60d6d-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
@ 2009-05-27  7:28       ` Paul Mundt
  0 siblings, 0 replies; 8+ messages in thread
From: Paul Mundt @ 2009-05-27  7:28 UTC (permalink / raw)
  To: Jean Delvare
  Cc: linux-next-u79uwXL29TY76Z2rM5mHXA,
	linux-media-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA

On Wed, May 27, 2009 at 09:18:31AM +0200, Jean Delvare wrote:
> On Wed, 27 May 2009 16:08:50 +0900, Paul Mundt wrote:
> > Another day, another module-related failure due to the i2c interface
> > being used in code that optionally uses it:
> > 
> > ERROR: "i2c_new_device" [drivers/media/video/soc_camera.ko] undefined!
> > ERROR: "i2c_get_adapter" [drivers/media/video/soc_camera.ko] undefined!
> > ERROR: "i2c_put_adapter" [drivers/media/video/soc_camera.ko] undefined!
> > ERROR: "i2c_unregister_device" [drivers/media/video/soc_camera.ko] undefined!
> > make[2]: *** [__modpost] Error 1
> > make[1]: *** [modules] Error 2
> > make: *** [sub-make] Error 2
> > 
> > In the interest of not continually inserting i2c ifdefs in to every
> > driver that supports an optional i2c interface, this provides a stubbed
> > set of interfaces for the CONFIG_I2C=n case.
> > 
> > I've covered the obvious ones that cause the majority of the build
> > failures, anything more involved really ought to have its dependencies
> > fixed instead.
> 
> Violent nack. Drivers which optionally use I2C are a minority.

If they were a minority we wouldn't be hitting them on a weekly basis,
and other busses already do similar things for similar reasons. You may
not like it, but it's much less distasteful than littering random i2c
ifdefs around every driver that chooses to have an optional i2c
interface. If every multi-bus driver was forced to go through these sorts
of hoops, the drivers would be even less readable than they already are
today.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: Simplified CONFIG_I2C=n interface.
  2009-05-27  7:18   ` Jean Delvare
       [not found]     ` <20090527091831.26b60d6d-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
@ 2009-05-27 12:01     ` Mark Brown
  2009-06-02  7:12       ` Jean Delvare
  1 sibling, 1 reply; 8+ messages in thread
From: Mark Brown @ 2009-05-27 12:01 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Paul Mundt, linux-next, linux-media, linux-i2c

On Wed, May 27, 2009 at 09:18:31AM +0200, Jean Delvare wrote:

> Violent nack. Drivers which optionally use I2C are a minority.

It's extremely common for devices like the CODECs and PMICs used in
embedded systems to have both I2C and SPI interfaces, selectable via a
pin strap at power on.  It's less common to have the SPI option for
things like hardware monitoring chips found in PCs but for anything that
might be I/O bound the high speed interface is a very common option.

> Designing them in such a way that a single #ifdef CONFIG_I2C will make
> them work can't be that hard, really. Not to mention that having a
> dozen stubs in i2c.h in the CONFIG_I2C=n case won't save you much work
> at the driver level anyway, because you certainly need to run different
> code paths depending on how the device is connected, and you also have
> to differentiate between the "I2C support is missing" case and the "I2C
> device registration failed" case, etc.

For the devices I've dealt with there's very little work at the driver
level - the various interfaces that can be used each probe using the
normal device model, set some register read/write operations and
possibly some other things and then call into the bulk of the driver
which has all the I/O abstracted away from it.

The error handling is already an issue with the current situation since
people are just silently building out the I2C support when I2C is not
enabled.  At the minute the main problem is with people not remembering
to do the #ifdef (a lot of platforms really need I2C enabled to be
useful so people never think to do the build without).

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: Simplified CONFIG_I2C=n interface.
  2009-05-27 12:01     ` Mark Brown
@ 2009-06-02  7:12       ` Jean Delvare
  2009-06-02  9:34         ` Mark Brown
  0 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2009-06-02  7:12 UTC (permalink / raw)
  To: Mark Brown; +Cc: Paul Mundt, linux-next, linux-media, linux-i2c

Hi Mark,

On Wed, 27 May 2009 13:01:40 +0100, Mark Brown wrote:
> On Wed, May 27, 2009 at 09:18:31AM +0200, Jean Delvare wrote:
> 
> > Violent nack. Drivers which optionally use I2C are a minority.
> 
> It's extremely common for devices like the CODECs and PMICs used in
> embedded systems to have both I2C and SPI interfaces, selectable via a
> pin strap at power on.  It's less common to have the SPI option for
> things like hardware monitoring chips found in PCs but for anything that
> might be I/O bound the high speed interface is a very common option.

Can you please point me at a couple of affected drivers?

> > Designing them in such a way that a single #ifdef CONFIG_I2C will make
> > them work can't be that hard, really. Not to mention that having a
> > dozen stubs in i2c.h in the CONFIG_I2C=n case won't save you much work
> > at the driver level anyway, because you certainly need to run different
> > code paths depending on how the device is connected, and you also have
> > to differentiate between the "I2C support is missing" case and the "I2C
> > device registration failed" case, etc.
> 
> For the devices I've dealt with there's very little work at the driver
> level - the various interfaces that can be used each probe using the
> normal device model, set some register read/write operations and
> possibly some other things and then call into the bulk of the driver
> which has all the I/O abstracted away from it.

It might make sense to define stubs for the CONFIG_I2C=n case for a few
of the i2c API functions, which are called from common driver parts, in
particular i2c_add/del_driver(), and as a matter of fact we already do
this for i2c_register_board_info(). But for lower-level functions, this
sounds wrong. The lower-level functions will only ever be called in
functions which should be completely discarded if I2C support is
missing from the kernel, and I would not count on gcc to be smart
enough to really discard all the code thanks to the i2c API being all
stubs. Meaning you end up with drivers larger than they should be -
which is no good for embedded systems.

I would really expect all I2C-related code to be in one place of the
driver (or even in a separate source file) and same for SPI-related
code. Then surrounding one big block of code with an ifdef doesn't
sound that difficult to read.

> The error handling is already an issue with the current situation since
> people are just silently building out the I2C support when I2C is not
> enabled.  At the minute the main problem is with people not remembering
> to do the #ifdef (a lot of platforms really need I2C enabled to be
> useful so people never think to do the build without).

I can't think of a way to solve this, other than what you do today
(build without I2C support from times to times and fix what needs to
be.) At least today you have a link breakage that tells you a given
driver needs to be reviewed for the CONFIG_I2C=n case. If we add stubs
all around to workaround the link breakage, this means the review never
happens, so the code might as well build and link but not work properly
or at least not be optimal. I wouldn't call this progress.

What could be done, OTOH, is to surround all the function declarations
in <linux/i2c.h> with a simple #ifdef CONFIG_I2C, so that mistakes are
caught earlier (build time instead of link time.)

-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: Simplified CONFIG_I2C=n interface.
  2009-06-02  7:12       ` Jean Delvare
@ 2009-06-02  9:34         ` Mark Brown
  2009-06-05  8:13           ` [PATCH] i2c: Don't advertise i2c functions when not available Jean Delvare
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2009-06-02  9:34 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Paul Mundt, linux-next, linux-media, linux-i2c

On Tue, Jun 02, 2009 at 09:12:29AM +0200, Jean Delvare wrote:
> On Wed, 27 May 2009 13:01:40 +0100, Mark Brown wrote:

> > It's extremely common for devices like the CODECs and PMICs used in
> > embedded systems to have both I2C and SPI interfaces, selectable via a

> Can you please point me at a couple of affected drivers?

Most of the Wolfson CODECs in sound/soc/codecs are affected (more than
actually have the SPI code at the minute), probably a lot of the other
CODECs there too.  I'd expect most I2C devices in drivers/mfd will also
be affectd.  For anything with more than a few registers the tendency is
to have both options unless there's a hardware constraint.

> I would really expect all I2C-related code to be in one place of the
> driver (or even in a separate source file) and same for SPI-related
> code. Then surrounding one big block of code with an ifdef doesn't
> sound that difficult to read.

It's not a legibility issue, it's to do with people remembering to
handle all the cases.  It's a bit of a PITA but not the end of the
world - I'm mentioning this more because you were suggesting that a
driver that was still useful with I2C=n was unusual rather than anything
else.

> driver needs to be reviewed for the CONFIG_I2C=n case. If we add stubs
> all around to workaround the link breakage, this means the review never
> happens, so the code might as well build and link but not work properly
> or at least not be optimal. I wouldn't call this progress.

I can't really see a situation where things wouldn't work properly
beyond the current situation where I2C support can just be built out -
if nobody is running the code then that's a separate issue.

> What could be done, OTOH, is to surround all the function declarations
> in <linux/i2c.h> with a simple #ifdef CONFIG_I2C, so that mistakes are
> caught earlier (build time instead of link time.)

That'd be helpful, yes.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] i2c: Don't advertise i2c functions when not available
  2009-06-02  9:34         ` Mark Brown
@ 2009-06-05  8:13           ` Jean Delvare
       [not found]             ` <20090605101330.2f93e9ab-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2009-06-05  8:13 UTC (permalink / raw)
  To: Mark Brown; +Cc: Paul Mundt, linux-next, linux-media, linux-i2c

On Tue, 2 Jun 2009 10:34:32 +0100, Mark Brown wrote:
> On Tue, Jun 02, 2009 at 09:12:29AM +0200, Jean Delvare wrote:
> > What could be done, OTOH, is to surround all the function declarations
> > in <linux/i2c.h> with a simple #ifdef CONFIG_I2C, so that mistakes are
> > caught earlier (build time instead of link time.)
> 
> That'd be helpful, yes.

Here you go:

From: Jean Delvare <khali@linux-fr.org>
Subject: i2c: Don't advertise i2c functions when not available

Surround i2c function declarations with ifdefs, so that they aren't
advertised when i2c-core isn't actually built. That way, drivers using
these functions unconditionally will result in an immediate build
failure, rather than a late linking failure which is harder to figure
out.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Paul Mundt <lethal@linux-sh.org>
---
 include/linux/i2c.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--- linux-2.6.30-rc8.orig/include/linux/i2c.h	2009-06-05 08:43:48.000000000 +0200
+++ linux-2.6.30-rc8/include/linux/i2c.h	2009-06-05 09:34:27.000000000 +0200
@@ -47,6 +47,7 @@ struct i2c_driver;
 union i2c_smbus_data;
 struct i2c_board_info;
 
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
 /*
  * The master routines are the ones normally used to transmit data to devices
  * on a bus (or read from them). Apart from two basic transfer functions to
@@ -93,6 +94,7 @@ extern s32 i2c_smbus_read_i2c_block_data
 extern s32 i2c_smbus_write_i2c_block_data(struct i2c_client *client,
 					  u8 command, u8 length,
 					  const u8 *values);
+#endif /* I2C */
 
 /**
  * struct i2c_driver - represent an I2C device driver
@@ -260,6 +262,7 @@ struct i2c_board_info {
 	.type = dev_type, .addr = (dev_addr)
 
 
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
 /* Add-on boards should register/unregister their devices; e.g. a board
  * with integrated I2C, a config eeprom, sensors, and a codec that's
  * used in conjunction with the primary hardware.
@@ -283,6 +286,7 @@ extern struct i2c_client *
 i2c_new_dummy(struct i2c_adapter *adap, u16 address);
 
 extern void i2c_unregister_device(struct i2c_client *);
+#endif /* I2C */
 
 /* Mainboard arch_initcall() code should register all its I2C devices.
  * This is done at arch_initcall time, before declaring any i2c adapters.
@@ -299,7 +303,7 @@ i2c_register_board_info(int busnum, stru
 {
 	return 0;
 }
-#endif
+#endif /* I2C_BOARDINFO */
 
 /*
  * The following structs are for those who like to implement new bus drivers:
@@ -394,6 +398,7 @@ struct i2c_client_address_data {
 
 /* administration...
  */
+#if defined CONFIG_I2C || defined CONFIG_I2C_MODULE
 extern int i2c_add_adapter(struct i2c_adapter *);
 extern int i2c_del_adapter(struct i2c_adapter *);
 extern int i2c_add_numbered_adapter(struct i2c_adapter *);
@@ -435,6 +440,7 @@ static inline int i2c_adapter_id(struct
 {
 	return adap->nr;
 }
+#endif /* I2C */
 #endif /* __KERNEL__ */
 
 /**



-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: Don't advertise i2c functions when not available
       [not found]             ` <20090605101330.2f93e9ab-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
@ 2009-06-05  8:42               ` Mark Brown
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2009-06-05  8:42 UTC (permalink / raw)
  To: Jean Delvare
  Cc: Paul Mundt, linux-next-u79uwXL29TY76Z2rM5mHXA,
	linux-media-u79uwXL29TY76Z2rM5mHXA,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA

On Fri, Jun 05, 2009 at 10:13:30AM +0200, Jean Delvare wrote:

> Surround i2c function declarations with ifdefs, so that they aren't
> advertised when i2c-core isn't actually built. That way, drivers using
> these functions unconditionally will result in an immediate build
> failure, rather than a late linking failure which is harder to figure
> out.

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2009-06-05  8:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-27  7:08 [PATCH] i2c: Simplified CONFIG_I2C=n interface Paul Mundt
     [not found] ` <20090527070850.GA11221-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
2009-05-27  7:18   ` Jean Delvare
     [not found]     ` <20090527091831.26b60d6d-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2009-05-27  7:28       ` Paul Mundt
2009-05-27 12:01     ` Mark Brown
2009-06-02  7:12       ` Jean Delvare
2009-06-02  9:34         ` Mark Brown
2009-06-05  8:13           ` [PATCH] i2c: Don't advertise i2c functions when not available Jean Delvare
     [not found]             ` <20090605101330.2f93e9ab-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2009-06-05  8:42               ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).