[PATCH] i2c-i801: Don't probe for slaves on IDF channels

[PATCH] i2c-i801: Don't probe for slaves on IDF channels

[Jean Delvare]

I don't know if Fujitsu is ever going to produce Patsburg-based
machines, but if they do, I'd rather not probe the secondary (IDF)
SMBus channels. At least not until we have a good reason for doing so.

On a side note, I'm not even sure if it is right to enable detection
of HWMON and DDC devices on the IDF channels. Time will tell...

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
Hans, David, what do you think?

 drivers/i2c/busses/i2c-i801.c |   45 +++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)

--- linux-2.6.39-rc7.orig/drivers/i2c/busses/i2c-i801.c	2011-05-10 09:39:07.000000000 +0200
+++ linux-2.6.39-rc7/drivers/i2c/busses/i2c-i801.c	2011-05-10 15:03:52.000000000 +0200
@@ -158,6 +158,7 @@ static struct pci_driver i801_driver;
 #define FEATURE_BLOCK_BUFFER	(1 << 1)
 #define FEATURE_BLOCK_PROC	(1 << 2)
 #define FEATURE_I2C_BLOCK_READ	(1 << 3)
+#define FEATURE_IDF		(1 << 4)
 
 static const char *i801_feature_names[] = {
 	"SMBus PEC",
@@ -735,6 +736,29 @@ static void __devinit dmi_check_onboard_
 }
 #endif
 
+/* Register optional slaves */
+static void __devinit i801_probe_optional_slaves(struct i801_priv *priv)
+{
+	/* Only register slaves on main SMBus channel */
+	if (priv->features & FEATURE_IDF)
+		return;
+
+#if defined CONFIG_INPUT_APANEL || defined CONFIG_INPUT_APANEL_MODULE
+	if (apanel_addr) {
+		struct i2c_board_info info;
+
+		memset(&info, 0, sizeof(struct i2c_board_info));
+		info.addr = apanel_addr;
+		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
+}
+
 static int __devinit i801_probe(struct pci_dev *dev,
 				const struct pci_device_id *id)
 {
@@ -753,6 +777,11 @@ static int __devinit i801_probe(struct p
 
 	priv->pci_dev = dev;
 	switch (dev->device) {
+	case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
+	case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1:
+	case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2:
+		priv->features |= FEATURE_IDF;
+		/* fall through */
 	default:
 		priv->features |= FEATURE_I2C_BLOCK_READ;
 		/* fall through */
@@ -838,21 +867,7 @@ static int __devinit i801_probe(struct p
 		goto exit_release;
 	}
 
-	/* Register optional slaves */
-#if defined CONFIG_INPUT_APANEL || defined CONFIG_INPUT_APANEL_MODULE
-	if (apanel_addr) {
-		struct i2c_board_info info;
-
-		memset(&info, 0, sizeof(struct i2c_board_info));
-		info.addr = apanel_addr;
-		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
+	i801_probe_optional_slaves(priv);
 
 	pci_set_drvdata(dev, priv);
 	return 0;


-- 
Jean Delvare

Re: [PATCH] i2c-i801: Don't probe for slaves on IDF channels

[Hans de Goede]

Hi,

On 05/10/2011 04:08 PM, Jean Delvare wrote:
> I don't know if Fujitsu is ever going to produce Patsburg-based
> machines, but if they do, I'd rather not probe the secondary (IDF)
> SMBus channels. At least not until we have a good reason for doing so.
>
> On a side note, I'm not even sure if it is right to enable detection
> of HWMON and DDC devices on the IDF channels. Time will tell...
>
> Signed-off-by: Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Hans de Goede<hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> CC: David Woodhouse<David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> Hans, David, what do you think?

Not probing for the FSC devices on the idf channel sounds reasonable
to me. Note that Fujitsu has moved to using super io chips for hwmon
now instead of their own i2c solutions, so for newer motherboards
there aren't any of these i2c chips present at all.

Regards,

Hans

Re: [PATCH] i2c-i801: Don't probe for slaves on IDF channels

[Jean Delvare]

On Tue, 10 May 2011 16:41:54 +0200, Hans de Goede wrote:
> Hi,
> 
> On 05/10/2011 04:08 PM, Jean Delvare wrote:
> > I don't know if Fujitsu is ever going to produce Patsburg-based
> > machines, but if they do, I'd rather not probe the secondary (IDF)
> > SMBus channels. At least not until we have a good reason for doing so.
> >
> > On a side note, I'm not even sure if it is right to enable detection
> > of HWMON and DDC devices on the IDF channels. Time will tell...
> >
> > Signed-off-by: Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > Cc: Hans de Goede<hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > CC: David Woodhouse<David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> > Hans, David, what do you think?
> 
> Not probing for the FSC devices on the idf channel sounds reasonable
> to me. Note that Fujitsu has moved to using super io chips for hwmon
> now instead of their own i2c solutions, so for newer motherboards
> there aren't any of these i2c chips present at all.

Good to know, thanks for the info. Good move I'd say, it should be less
work for us to support their boards. 

-- 
Jean Delvare

Re: [PATCH] i2c-i801: Don't probe for slaves on IDF channels

[Woodhouse, David]

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

On Tue, 2011-05-10 at 15:08 +0100, Jean Delvare wrote:
> I don't know if Fujitsu is ever going to produce Patsburg-based
> machines, but if they do, I'd rather not probe the secondary (IDF)
> SMBus channels. At least not until we have a good reason for doing so.

Seems reasonable in principle.

> +#if defined CONFIG_INPUT_APANEL || defined CONFIG_INPUT_APANEL_MODULE

> +#if defined CONFIG_SENSORS_FSCHMD || defined CONFIG_SENSORS_FSCHMD_MODULE

This kind of ifdef is horrid though, and should be avoided at all costs.
A setting of CONFIG_xxx_MODULE should not affect the *static* kernel
image. You should be able to enable an additional module, build it and
use it without having to rebuild/reboot the kernel.

I know there are *some* cases where we violate that rule, but that
doesn't make it any nicer.

-- 
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 probe for slaves on IDF channels

[Jean Delvare]

Hi David,

On Tue, 10 May 2011 15:50:12 +0100, Woodhouse, David wrote:
> On Tue, 2011-05-10 at 15:08 +0100, Jean Delvare wrote:
> > I don't know if Fujitsu is ever going to produce Patsburg-based
> > machines, but if they do, I'd rather not probe the secondary (IDF)
> > SMBus channels. At least not until we have a good reason for doing so.
> 
> Seems reasonable in principle.

OK, I'll push this change upstream then, thanks.

Any opinion on the detection class flags for the IDF channels? Do you
have any idea what these IDF channels are supposed to be dedicated to?

> > +#if defined CONFIG_INPUT_APANEL || defined CONFIG_INPUT_APANEL_MODULE
> 
> > +#if defined CONFIG_SENSORS_FSCHMD || defined CONFIG_SENSORS_FSCHMD_MODULE
> 
> This kind of ifdef is horrid though, and should be avoided at all costs.
> A setting of CONFIG_xxx_MODULE should not affect the *static* kernel
> image. You should be able to enable an additional module, build it and
> use it without having to rebuild/reboot the kernel.

In general, the i2c-i801 driver will be built as a module, so that
won't affect the kernel image. While I understand your point, there was
a reason for the horrid ifdefs. The idea was that we don't want to bloat
the kernel (or the i2c-i801 module) with hardware system specific code
if the builder knows upfront that the kernel will never run on the
relevant hardware system. So, if the builder doesn't select the apanel
and fschmd drivers, we build the i2c-i801 driver without the specific
code for systems with these devices. The fact is that i2c-i801 is a very
popular driver when apanel and fschmd are only used on a few systems.

Of course this would go wrong if a user was to build the apanel or
fschmd driver out of tree, but we couldn't find any reason why anybody
would be doing so.

With x86 becoming an embedded architecture, it seemed relevant to let
people build as small an x86 kernel as possible, thus the decision to
go for the horrid ifdefs.

> I know there are *some* cases where we violate that rule, but that
> doesn't make it any nicer.

Note that my patch is merely moving this code around, it's not
introducing it.

If you really think we should change this, then maybe a good compromise
would be to use #ifdef CONFIG_X86 instead of the module-specific ones.
This would let at least non-x86 systems built the i2c-i801 driver
without the hardware specific code. But OTOH I'm not even sure if
there's any non-x86 system out there using an Intel 82801 chipset,
so maybe that would be exactly equivalent to dropping the ifdefs
altogether. Or IA64 systems maybe...?

Yet another possibility would be a dedicated Kconfig option
(CONFIG_I2C_I801_POPULATE or even CONFIG_I2C_POPULATE and make it
cross-driver) that would be hidden and enabled by default, and visible
only when CONFIG_EMBEDDED is set?

In both cases, that would be an all-or-nothing setting, but I guess
that people who care about the size of i2c-i801 have a proper way to
declare their I2C devices on the SMBus. Or at least I hope so...

I'm glad we're discussing this now, as I am on my way to add even more
such hardware-specific code to the i2c-i801 driver. All in all this
really shows how much x86 lacks proper description for the hardware
setup at the firmware or platform code level, but fixing this is out of
scope for me.

-- 
Jean Delvare