[PATCH] i2c-i801: Don't depend on other kernel driver config options

[PATCH] i2c-i801: Don't depend on other kernel driver config options

[Jean Delvare]

Don't let other driver config options influence us, as it makes the
code more complex and fragile for a small benefit. There's nothing
wrong with instantiating I2C devices even if they don't have a driver.
And we're talking about 835 extra bytes in the binary on x86-64,
that's hardly worth arguing about.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: David Woodhouse <david.woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
David, would this make you feel better? Hans, any objection?

 drivers/i2c/busses/i2c-i801.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

--- linux-2.6.39-rc7.orig/drivers/i2c/busses/i2c-i801.c	2011-05-12 10:17:45.000000000 +0200
+++ linux-2.6.39-rc7/drivers/i2c/busses/i2c-i801.c	2011-05-12 16:41:33.000000000 +0200
@@ -638,7 +638,7 @@ static const struct pci_device_id i801_i
 
 MODULE_DEVICE_TABLE(pci, i801_ids);
 
-#if defined CONFIG_INPUT_APANEL || defined CONFIG_INPUT_APANEL_MODULE
+#ifdef CONFIG_X86
 static unsigned char apanel_addr;
 
 /* Scan the system ROM for the signature "FJKEYINF" */
@@ -668,11 +668,7 @@ static void __init input_apanel_init(voi
 	}
 	iounmap(bios);
 }
-#else
-static void __init input_apanel_init(void) {}
-#endif
 
-#if defined CONFIG_SENSORS_FSCHMD || defined CONFIG_SENSORS_FSCHMD_MODULE
 struct dmi_onboard_device_info {
 	const char *name;
 	u8 type;
@@ -738,6 +734,8 @@ static void __devinit dmi_check_onboard_
 		dmi_check_onboard_device(type, name, adap);
 	}
 }
+#else
+static void __init input_apanel_init(void) {}
 #endif
 
 /* Register optional slaves */
@@ -747,7 +745,7 @@ static void __devinit i801_probe_optiona
 	if (priv->features & FEATURE_IDF)
 		return;
 
-#if defined CONFIG_INPUT_APANEL || defined CONFIG_INPUT_APANEL_MODULE
+#ifdef CONFIG_X86
 	if (apanel_addr) {
 		struct i2c_board_info info;
 
@@ -756,8 +754,7 @@ static void __devinit i801_probe_optiona
 		strlcpy(info.type, "fujitsu_apanel", I2C_NAME_SIZE);
 		i2c_new_device(&priv->adapter, &info);
 	}
-#endif
-#if defined CONFIG_SENSORS_FSCHMD || defined CONFIG_SENSORS_FSCHMD_MODULE
+
 	if (dmi_name_in_vendors("FUJITSU"))
 		dmi_walk(dmi_check_onboard_devices, &priv->adapter);
 #endif


-- 
Jean Delvare

Re: [PATCH] i2c-i801: Don't depend on other kernel driver config options

[Woodhouse, David]

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

On Fri, 2011-05-13 at 11:01 +0100, Jean Delvare wrote:
> Don't let other driver config options influence us, as it makes the
> code more complex and fragile for a small benefit. There's nothing
> wrong with instantiating I2C devices even if they don't have a driver.
> And we're talking about 835 extra bytes in the binary on x86-64,
> that's hardly worth arguing about. 

Looks sane to me; thanks. Should it be CONFIG_DMI instead of (or in
addition to) CONFIG_X86?
-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6242 bytes --]

Re: [PATCH] i2c-i801: Don't depend on other kernel driver config options

[Jean Delvare]

Hi David,

On Fri, 13 May 2011 11:23:06 +0100, Woodhouse, David wrote:
> On Fri, 2011-05-13 at 11:01 +0100, Jean Delvare wrote:
> > Don't let other driver config options influence us, as it makes the
> > code more complex and fragile for a small benefit. There's nothing
> > wrong with instantiating I2C devices even if they don't have a driver.
> > And we're talking about 835 extra bytes in the binary on x86-64,
> > that's hardly worth arguing about. 
> 
> Looks sane to me; thanks. Should it be CONFIG_DMI instead of (or in
> addition to) CONFIG_X86?

Instead of, no. IA64 has DMI but doesn't want this code.

In addition to, yes, that would make sense. I expected the extra code to
transparently vanish when CONFIG_DMI isn't set, but gcc is apparently
not smart enough for this, so it could use some help.

BTW, it may be desirable to make the apanel part depend on DMI, even if
DMI is technically not needed to find the slave address. Scanning the
BIOS memory for an arbitrary string is costly (a minimum of 4096 calls
to check_signature(), which in turn calls readb() at least once, in the
negative case.) Checking for the DMI vendor first should be way less
costly for the negative case.

I'll submit a new patch set later today, thanks for the suggestions.

-- 
Jean Delvare