i2c: skip address detection if provided in board_info

PATCH In review jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected] 3 participants 13 messages Oct 11, 2010 – Oct 13, 2010 6 messages not in the archive

Subsystems (2)

I2C subsystem host drivers I2C

lore

Share & tools

lore

jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]

author

2010-10-11 23:10 UTC #0 lore raw

PATCH 1/1 i2c: skip address detection if provided in board_info

From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

If the address of a given device is already provided by platform init code, e.g. from system firmware, there is no need to call the driver's detect() function for finding the matching address from the driver's address list.

Avoiding such detection might save boot time.

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

---
 drivers/i2c/i2c-core.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

drivers/i2c/i2c-core.c +32 -0

index 6649176..e4f7feb 100644
@@ -1499,6 +1499,31 @@ static int i2c_detect_address(struct i2c_client *temp_client,
 	return 0;
 }
 
+static int i2c_scan_board_info(struct i2c_adapter *adapter, struct i2c_driver *driver)
+{
+	struct i2c_devinfo	*devinfo;
+	int ret = -ENODEV;
+
+	down_read(&__i2c_board_lock);
+	list_for_each_entry(devinfo, &__i2c_board_list, list) {
+		if (!strncmp(devinfo->board_info.type, driver->driver.name,
+				I2C_NAME_SIZE)) {
+			dev_info(&adapter->dev, "found i2c board info %s\n",
+				driver->driver.name);
+			if (devinfo->board_info.addr) {
+				ret = 0;
+				goto scan_exit;
+			}
+		}
+	}
+
+scan_exit:
+	up_read(&__i2c_board_lock);
+
+	return ret;
+}
+
+
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 {
 	const unsigned short *address_list;

@@ -1506,6 +1531,13 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 	int i, err = 0;
 	int adap_id = i2c_adapter_id(adapter);
 
+	/* There is no need to detect i2c address if board info is provided */
+	if (!i2c_scan_board_info(adapter, driver)) {
+		dev_info(&adapter->dev, "Skip address detection for %s\n",
+			driver->driver.name);
+		return 0;
+	}
+
 	address_list = driver->address_list;
 	if (!driver->detect || !address_list)
 		return 0;

Signature · 1 line

1.7.0.4

A message that is not in the archive (sent off-list or not yet received): 1286838635-16474-1-git-send-email-jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]

Jean Delvare

2010-10-12 07:28 UTC #2 lore raw

Hi Jacob,

Quoted 8 lines · On Mon, 11 Oct 2010 16:10:35 -0700, jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public…

From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>
If the address of a given device is already provided by platform init code, e.g. from system firmware, there is no need to call the driver's detect() function for finding the matching address from the driver's address list.
Avoiding such detection might save boot time.

i2c_detect() is a no-op if adapter->class is 0, and if you have platform init data describing the chips on your I2C adapter then you certainly don't want to set the adapter class to anything other than 0.

So I'd rather avoid optimizing a case which isn't supposed to happen in the first place.

The only optimization which I think would be valuable is checking the class before allocating the temporary i2c_client structure. I'll send a patch doing that in a minute.

Quoted 13 lines

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

---
 drivers/i2c/i2c-core.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

drivers/i2c/i2c-core.c +0 -0

index 6649176..e4f7feb 100644
@@ -1499,6 +1499,31 @@ static int i2c_detect_address(struct i2c_client *temp_client,
 	return 0;
 }

drivers/i2c/i2c-core.c static int i2c_detect_address(struct i2c_client *temp_client, · line 1510

Quoted 10 lines

drivers/i2c/i2c-core.c +9 -0

 
+static int i2c_scan_board_info(struct i2c_adapter *adapter, struct i2c_driver *driver)
+{
+	struct i2c_devinfo	*devinfo;
+	int ret = -ENODEV;
+
+	down_read(&__i2c_board_lock);
+	list_for_each_entry(devinfo, &__i2c_board_list, list) {
+		if (!strncmp(devinfo->board_info.type, driver->driver.name,
+				I2C_NAME_SIZE)) {

This is wrong anyway. Comparing device names with driver names only works in rare cases and shouldn't be relied upon.

Quoted 33 lines

drivers/i2c/i2c-core.c +23 -0

+			dev_info(&adapter->dev, "found i2c board info %s\n",
+				driver->driver.name);
+			if (devinfo->board_info.addr) {
+				ret = 0;
+				goto scan_exit;
+			}
+		}
+	}
+
+scan_exit:
+	up_read(&__i2c_board_lock);
+
+	return ret;
+}
+
+
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 {
 	const unsigned short *address_list;

@@ -1506,6 +1531,13 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 	int i, err = 0;
 	int adap_id = i2c_adapter_id(adapter);
 
+	/* There is no need to detect i2c address if board info is provided */
+	if (!i2c_scan_board_info(adapter, driver)) {
+		dev_info(&adapter->dev, "Skip address detection for %s\n",
+			driver->driver.name);
+		return 0;
+	}
+
 	address_list = driver->address_list;
 	if (!driver->detect || !address_list)
 		return 0;

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Feng Tang

2010-10-12 08:21 UTC #4 lore raw

Hi Jean,

Quoted 13 lines · On Tue, 12 Oct 2010 15:28:22 +0800 Jean Delvare…

Hi Jacob,
Quoted 8 lines · On Mon, 11 Oct 2010 16:10:35 -0700, jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public…
From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>
If the address of a given device is already provided by platform init code, e.g. from system firmware, there is no need to call the driver's detect() function for finding the matching address from the driver's address list.
Avoiding such detection might save boot time.

Quoted 7 lines

i2c_detect() is a no-op if adapter->class is 0, and if you have platform init data describing the chips on your I2C adapter then you certainly don't want to set the adapter class to anything other than 0.
So I'd rather avoid optimizing a case which isn't supposed to happen in the first place.

I checked the i2c devices drivers in drivers/hwmon/, most of them set the class to I2C_CLASS_HWMON, and many i2c adapter drivers also setting their class to I2C_CLASS_HWMON. So there is still many cases for i2c_detect get called.

Under drivers/hwmon/, many i2c devices driver provide an list with 4 or more addresses, imaging a platform with >= 5 i2c adapters case, so it will take more than 20 i2c transfers to init one i2c device driver, and in 10 or 20 seconds (if the timeout for a i2c transfer of that adapter driver is 500ms or 1 second). It will be a disaster if we build in all the drivers in drivers/hwmon/.

So can we have a global flag like i2c_skip_autodetect, which is 0 by default, and could be set in platform init code to prevent the detecting from really happen, if we are confident the platform code has inited i2c board info correctly?

Thanks,
Feng

drivers/i2c/i2c-core.c +4 -1

index bea4c50..7727105 100644
@@ -1493,6 +1493,9 @@ static int i2c_detect_address(struct i2c_client *temp_client,
 	return 0;
 }
 
+int i2c_skip_auto_detect;
+EXPORT_SYMBOL(i2c_skip_auto_detect);
+
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 {
 	const unsigned short *address_list;

@@ -1501,7 +1504,7 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 	int adap_id = i2c_adapter_id(adapter);
 
 	address_list = driver->address_list;
-	if (!driver->detect || !address_list)
+	if (!driver->detect || !address_list || i2c_skip_auto_detect)
 		return 0;
 
 	/* Set up a temporary client to help detect callback */

Quoted 18 lines

The only optimization which I think would be valuable is checking the class before allocating the temporary i2c_client structure. I'll send a patch doing that in a minute.
Quoted 13 lines
Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>
---
 drivers/i2c/i2c-core.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)
drivers/i2c/i2c-core.c +0 -0
index 6649176..e4f7feb 100644
@@ -1499,6 +1499,31 @@ static int i2c_detect_address(struct
 i2c_client *temp_client, return 0;
 }

Jean Delvare

2010-10-12 08:47 UTC #5 lore raw

Hi Feng,

Quoted 18 lines · Feng Tang

Quoted 16 lines · On Tue, 12 Oct 2010 15:28:22 +0800 Jean Delvare…
Quoted 8 lines · On Mon, 11 Oct 2010 16:10:35 -0700, jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public…
From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>
If the address of a given device is already provided by platform init code, e.g. from system firmware, there is no need to call the driver's detect() function for finding the matching address from the driver's address list.
Avoiding such detection might save boot time.
i2c_detect() is a no-op if adapter->class is 0, and if you have platform init data describing the chips on your I2C adapter then you certainly don't want to set the adapter class to anything other than 0.

Quoted 7 lines

Quoted 2 lines
So I'd rather avoid optimizing a case which isn't supposed to happen in the first place.
I checked the i2c devices drivers in drivers/hwmon/, most of them set the class to I2C_CLASS_HWMON, and many i2c adapter drivers also setting their class to I2C_CLASS_HWMON. So there is still many cases for i2c_detect get called.

Of course i2c_detect gets called at times, otherwise we would delete it altogether ;)

Please point me to the I2C adapter driver(s) you care about, and also the platform init code for your system. I would like to see how it looks like.

I think it would also help if I had a global picture of your project and what you are trying to achieve. I presume we are talking about an embedded system? With a custom kernel maybe? Which platform/architecture?

Quoted 5 lines

Under drivers/hwmon/, many i2c devices driver provide an list with 4 or more addresses, imaging a platform with >= 5 i2c adapters case, so it will take more than 20 i2c transfers to init one i2c device driver, and in 10 or 20 seconds (if the timeout for a i2c transfer of that adapter driver is 500ms or 1 second). It will be a disaster if we build in all the drivers in drivers/hwmon/.

But you normally only load the hwmon drivers you need on a given machine. That's only a couple of them. Or are you, by any chance, building a monolithic kernel with all hwmon drivers included? This would indeed be a problem, the i2c subsystem isn't prepared for that.

One possible workaround for this case would be to cache the results of i2c_default_probe(), to avoid probing the same address repeatedly. This will increase the memory consumption though, as we won't know when we can get rid of that cache. I think the best thing to do here is: just don't do that.

Quoted 4 lines

So can we have a global flag like i2c_skip_autodetect, which is 0 by default, and could be set in platform init code to prevent the detecting from really happen, if we are confident the platform code has inited i2c board info correctly?

How is this different from the same platform init code clearing the adapter's class (or even better, not setting it in the first place)? And you don't need any code change for that.

Quoted 15 lines

drivers/i2c/i2c-core.c +3 -0

index bea4c50..7727105 100644
@@ -1493,6 +1493,9 @@ static int i2c_detect_address(struct i2c_client *temp_client,
 	return 0;
 }
 
+int i2c_skip_auto_detect;
+EXPORT_SYMBOL(i2c_skip_auto_detect);
+
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 {
 	const unsigned short *address_list;

drivers/i2c/i2c-core.c static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver) · line 1510

Quoted 8 lines

drivers/i2c/i2c-core.c +1 -1

@@ -1501,7 +1504,7 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
 	int adap_id = i2c_adapter_id(adapter);
 
 	address_list = driver->address_list;
-	if (!driver->detect || !address_list)
+	if (!driver->detect || !address_list || i2c_skip_auto_detect)
 		return 0;
 
 	/* Set up a temporary client to help detect callback */

If the point is to skip the whole detection logic on some embedded systems, then it might make more sense to make it a build-time option. A build-time option would have the benefit of shaving some code off your kernel binary: i2c_detect_address and i2c_detect in i2c-core.c, but also possibly in individual i2c device drivers and maybe ultimately in some i2c data structures. This is of course way more intrusive than your proposal, but depending on your actual goal, it might also be much more efficient and thus worth the effort.

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Feng Tang

2010-10-12 09:30 UTC #7 lore raw

Quoted 32 lines · On Tue, 12 Oct 2010 16:47:07 +0800 Jean Delvare…

Hi Feng,
Quoted 18 lines · Feng Tang
Quoted 16 lines · On Tue, 12 Oct 2010 15:28:22 +0800 Jean Delvare…
Quoted 8 lines · On Mon, 11 Oct 2010 16:10:35 -0700, jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public…
From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>
If the address of a given device is already provided by platform init code, e.g. from system firmware, there is no need to call the driver's detect() function for finding the matching address from the driver's address list.
Avoiding such detection might save boot time.
i2c_detect() is a no-op if adapter->class is 0, and if you have platform init data describing the chips on your I2C adapter then you certainly don't want to set the adapter class to anything other than 0.
Quoted 7 lines
Quoted 2 lines
So I'd rather avoid optimizing a case which isn't supposed to happen in the first place.
I checked the i2c devices drivers in drivers/hwmon/, most of them set the class to I2C_CLASS_HWMON, and many i2c adapter drivers also setting their class to I2C_CLASS_HWMON. So there is still many cases for i2c_detect get called.
Of course i2c_detect gets called at times, otherwise we would delete it altogether ;)

Quoted 8 lines

Please point me to the I2C adapter driver(s) you care about, and also the platform init code for your system. I would like to see how it looks like.
I think it would also help if I had a global picture of your project and what you are trying to achieve. I presume we are talking about an embedded system? With a custom kernel maybe? Which platform/architecture?

Yes, we are working a x86 based embedded system, Intel's Moorestown and Medfield systems, its current platform init code is arch/x86/kernel/mrst.c, but the I2c init code hasn't been pushed upstream yet :(, but basically it get i2c devices info from a SFI table (drivers/sfi/) and use i2c_register_board_info for them.

The adapter driver is i2c-mrst.c which is not in current i2c subsystem yet, seems it has been submitted to i2c devel list before. And our platform has many identical adapters.

Quoted 8 lines

Quoted 7 lines
Under drivers/hwmon/, many i2c devices driver provide an list with 4 or more addresses, imaging a platform with >= 5 i2c adapters case, so it will take more than 20 i2c transfers to init one i2c device driver, and in 10 or 20 seconds (if the timeout for a i2c transfer of that adapter driver is 500ms or 1 second). It will be a disaster if we build in all the drivers in drivers/hwmon/.

Quoted 4 lines

But you normally only load the hwmon drivers you need on a given machine. That's only a couple of them. Or are you, by any chance, building a monolithic kernel with all hwmon drivers included? This would indeed be a problem, the i2c subsystem isn't prepared for that.

Actually we run into a long boot time problem while we build in only one i2c device driver (drivers/hwmon/emc1403.c) which has 4 addresses in its address lit, it took more than 10 seconds for the driver to init on our platforms. So if we build in more i2c hwmon drivers, things will get much worse. That's why Jacob posted the patch.

I did read code about setting adapter->class, seems most driver set its class in their proble function, trying set it in platform code may looks not very clean.

Yours suggestion of using a build time option definitely will shrink the kernel size. But our platform is under arch/x86, and one general rule is to one generic kernel config should work for all x86 platforms, so I'm afraid it can't solve our problem.

So I still prefer the option of adding a flag, and leave the choice for platform code whether to do the detect. If you agree, I would make a cleaner patch.

Thanks,
Feng

Quoted 49 lines

One possible workaround for this case would be to cache the results of i2c_default_probe(), to avoid probing the same address repeatedly. This will increase the memory consumption though, as we won't know when we can get rid of that cache. I think the best thing to do here
is: just don't do that.
Quoted 4 lines
So can we have a global flag like i2c_skip_autodetect, which is 0 by default, and could be set in platform init code to prevent the detecting from really happen, if we are confident the platform code has inited i2c board info correctly?
How is this different from the same platform init code clearing the adapter's class (or even better, not setting it in the first place)? And you don't need any code change for that.
Quoted 15 lines
drivers/i2c/i2c-core.c +3 -0
index bea4c50..7727105 100644
@@ -1493,6 +1493,9 @@ static int i2c_detect_address(struct
 i2c_client *temp_client, return 0;
 }
 
+int i2c_skip_auto_detect;
+EXPORT_SYMBOL(i2c_skip_auto_detect);
+
 static int i2c_detect(struct i2c_adapter *adapter, struct
 i2c_driver *driver) {
 	const unsigned short *address_list;
drivers/i2c/i2c-core.c static int i2c_detect(struct i2c_adapter · line 1510
Quoted 8 lines
drivers/i2c/i2c-core.c +1 -1
@@ -1501,7 +1504,7 @@ static int i2c_detect(struct i2c_adapter
 *adapter, struct i2c_driver *driver) int adap_id =
 i2c_adapter_id(adapter); 
 	address_list = driver->address_list;
-	if (!driver->detect || !address_list)
+	if (!driver->detect || !address_list ||
 i2c_skip_auto_detect) return 0;
 
 	/* Set up a temporary client to help detect callback */  
If the point is to skip the whole detection logic on some embedded systems, then it might make more sense to make it a build-time option. A build-time option would have the benefit of shaving some code off your kernel binary: i2c_detect_address and i2c_detect in i2c-core.c, but also possibly in individual i2c device drivers and maybe ultimately in some i2c data structures. This is of course way more intrusive than your proposal, but depending on your actual goal, it might also be much more efficient and thus worth the effort.

Jean Delvare

2010-10-12 11:34 UTC #8 lore raw

Hi Feng,

Quoted 20 lines · Feng Tang

Quoted 11 lines · On Tue, 12 Oct 2010 16:47:07 +0800 Jean Delvare…
Of course i2c_detect gets called at times, otherwise we would delete it altogether ;)
Please point me to the I2C adapter driver(s) you care about, and also the platform init code for your system. I would like to see how it looks like.
I think it would also help if I had a global picture of your project and what you are trying to achieve. I presume we are talking about an embedded system? With a custom kernel maybe? Which platform/architecture?
Yes, we are working a x86 based embedded system, Intel's Moorestown and Medfield systems, its current platform init code is arch/x86/kernel/mrst.c, but the I2c init code hasn't been pushed upstream yet :(, but basically it get i2c devices info from a SFI table (drivers/sfi/) and use i2c_register_board_info for them.

Quoted 3 lines

The adapter driver is i2c-mrst.c which is not in current i2c subsystem yet, seems it has been submitted to i2c devel list before. And our platform has many identical adapters.

I remember that driver, I reviewed it long ago (more than 1 year ago I think). Apparently driver was renamed to i2c-intel-mid meanwhile. Still not upstream indeed, but the last version of the driver posted by Alan Cox [1] does _not_ set the i2c_adapter.class field.

[1] http://marc.info/?l=linux-i2c&m=128292492431251&w=2

Quoted 11 lines

Quoted 5 lines
(...)
But you normally only load the hwmon drivers you need on a given machine. That's only a couple of them. Or are you, by any chance, building a monolithic kernel with all hwmon drivers included? This would indeed be a problem, the i2c subsystem isn't prepared for that.
Actually we run into a long boot time problem while we build in only one i2c device driver (drivers/hwmon/emc1403.c) which has 4 addresses in its address lit, it took more than 10 seconds for the driver to init on our platforms. So if we build in more i2c hwmon drivers, things will get much worse. That's why Jacob posted the patch.

This suggests that the i2c adapter driver is pretty bad at handling NACKs when trying to access a non-existing device. It might be worth checking if this can be improved, because that case could happen not only during device detection but also in other circumstances.

Quoted 3 lines

I did read code about setting adapter->class, seems most driver set its class in their proble function, trying set it in platform code may looks not very clean.

Actually this is very clean and by far my preferred way to solve your problem. This is exactly how things are done on the PXA platform (check i2c-pxa.c.)

Quoted 4 lines

Yours suggestion of using a build time option definitely will shrink the kernel size. But our platform is under arch/x86, and one general rule is to one generic kernel config should work for all x86 platforms, so I'm afraid it can't solve our problem.

OK, fair enough.

Quoted 3 lines

So I still prefer the option of adding a flag, and leave the choice for platform code whether to do the detect. If you agree, I would make a cleaner patch.

And I prefer that you set the class flag value in the platform data and have your i2c adapter driver read it. This is already supported and should work very fine without any change to i2c-core. I see no point in adding a global flag when we already have a much finer-grained setting available.

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jacob Pan

author

2010-10-12 18:01 UTC #10 lore raw

Quoted 11 lines

Quoted 5 lines
So I still prefer the option of adding a flag, and leave the choice for platform code whether to do the detect. If you agree, I would make a cleaner patch.
And I prefer that you set the class flag value in the platform data and have your i2c adapter driver read it. This is already supported and should work very fine without any change to i2c-core. I see no point in adding a global flag when we already have a much finer-grained setting available.

I agree that assigning the class code to adapter will solve our problem but I do have a question/concern.

What if you have mixed devices under one adapter with some prefer to have detect funtion some not. Then all detect functions will be ignored if i2c adpater class code is 0. The granularity of information provided by our FW is per i2c device.

Seems we have three cases:
1. adaptor class = 0, no detect
2. adaptor class !=0, FW provide address via board info, no detect
3. adaptor class !=0, no FW board info, do detect by default, but should allow platform code override

I would think we need a global flag for case #3.

Thanks,

Jacob

Jean Delvare

2010-10-13 07:26 UTC #11 lore raw

Hi Jacob,

Quoted 2 lines · Jacob Pan

What if you have mixed devices under one adapter with some prefer to have detect funtion some not.

Devices don't have detect functions. Drivers do.

Quoted 11 lines

Then all detect functions will be ignored
if i2c adpater class code is 0. The granularity of information provided by our FW is per i2c device.
Seems we have three cases:
1. adaptor class = 0, no detect
2. adaptor class !=0, FW provide address via board info, no detect
3. adaptor class !=0, no FW board info, do detect by default, but should allow platform code override
I would think we need a global flag for case #3.

You complain that the granularity of the current implementation is insufficient, but you propose to solve that problem using a _global_ flag? This doesn't make sense, sorry.

There are actually two main cases, not three:

1. Adapter class == 0, no detection, platform data provides all device information.
2. Adapter class != 0, detection of all devices, no platform data.

Mixed cases aren't supported, because it is expected that all devices are declared as platform data, or none is. This seems to work OK so far for everybody. If it doesn't work for you, please explain why, in details.

Note though that there is some level of granularity because the adapter class is a bitfield. So it is possible to declare all I2C devices as platform data except the hardware monitoring devices, for example, and set adapter class = I2C_CLASS_HWMON.

There aren't many bits really used, BTW, mostly I2C_CLASS_HWMON and I2C_CLASS_SPD, and the trend is to remove the unused class flags rather than to add new ones. However, if you need a new I2C_CLASS flag, this can certainly be done.

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jacob Pan

author

2010-10-13 15:54 UTC #13 lore raw

Hi Jean,

Jean Delvare Wed, 13 Oct 2010 09:26:54 +0200

Quoted 2 lines

Devices don't have detect functions. Drivers do.

Yes, i understand. I was referring to i2c device driver vs adapter driver. I will make it clearer next time.

Quoted 10 lines

Quoted 10 lines
Then all detect functions will be ignored
if i2c adpater class code is 0. The granularity of information provided by our FW is per i2c device.
Seems we have three cases:
1. adaptor class = 0, no detect
2. adaptor class !=0, FW provide address via board info, no detect
3. adaptor class !=0, no FW board info, do detect by default, but should allow platform code override

Quoted 6 lines

Quoted 1 line
I would think we need a global flag for case #3.
You complain that the granularity of the current implementation is insufficient, but you propose to solve that problem using a _global_ flag? This doesn't make sense, sorry.

to me, the global flag solves the problem where we want faster boottime in all cases. so it is kinda of a different problem.

Quoted 11 lines

There are actually two main cases, not three:
1. Adapter class == 0, no detection, platform data provides all device information.
2. Adapter class != 0, detection of all devices, no platform data.
Mixed cases aren't supported, because it is expected that all devices are declared as platform data, or none is. This seems to work OK so far for everybody. If it doesn't work for you, please explain why, in details.

it works for us in practice, at least for today. perhaps I am just playing devil's advocate, but also for the completeness of the logic for future use.

Quoted 10 lines

Note though that there is some level of granularity because the adapter class is a bitfield. So it is possible to declare all I2C devices as platform data except the hardware monitoring devices, for example, and set adapter class = I2C_CLASS_HWMON.
There aren't many bits really used, BTW, mostly I2C_CLASS_HWMON and I2C_CLASS_SPD, and the trend is to remove the unused class flags rather than to add new ones. However, if you need a new I2C_CLASS flag, this can certainly be done.

Can we make an explicit bit I2C_CLASS_NO_DETECT so that is is more explicit? It also allows co-existance with other flags so that we can handle mixed cases?

Thanks,
Jacob

Jean Delvare

2010-10-13 16:07 UTC #14 lore raw

Quoted 9 lines · Jacob Pan

Jean Delvare Wed, 13 Oct 2010 09:26:54 +0200
Quoted 4 lines
Mixed cases aren't supported, because it is expected that all devices are declared as platform data, or none is. This seems to work OK so far for everybody. If it doesn't work for you, please explain why, in details.
it works for us in practice, at least for today. perhaps I am just playing devil's advocate, but also for the completeness of the logic for future use.

OK, thanks for clarifying. I do not expect this case to ever happen. But if it ever does, then feel free to come back to me with all the details and we'll discuss it again. I don't think it makes sense to add support for a case which doesn't exist, because it's impossible to figure out the best way to fix it until we actually see it.

Quoted 5 lines

Quoted 5 lines
Note though that there is some level of granularity because the adapter class is a bitfield. So it is possible to declare all I2C devices as platform data except the hardware monitoring devices, for example, and set adapter class = I2C_CLASS_HWMON.

Quoted 8 lines

Quoted 4 lines
There aren't many bits really used, BTW, mostly I2C_CLASS_HWMON and I2C_CLASS_SPD, and the trend is to remove the unused class flags rather than to add new ones. However, if you need a new I2C_CLASS flag, this can certainly be done.
Can we make an explicit bit I2C_CLASS_NO_DETECT so that is is more explicit? It also allows co-existance with other flags so that we can handle mixed cases?

And what exactly would this bit do?

Confused.

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jacob Pan

author

2010-10-13 17:01 UTC #16 lore raw

Hi Jean,

Quoted 6 lines

Quoted 3 lines
Can we make an explicit bit I2C_CLASS_NO_DETECT so that is is more explicit? It also allows co-existance with other flags so that we can handle mixed cases?
And what exactly would this bit do?

You can scratch this idea. I thought the i2c adapter's class code has some use other than matching with device class. but it seems it is the only use. I was thinking about having HWMON|NO_DETECT so that adapter can still maintain the class identity.
Anyway, i don't think it is needed. Sorry for the confusion :)

Thanks,

Jacob

Feng Tang

2010-10-13 17:04 UTC #17 lore raw

In reply to a message not in the archive

Hi Jean,

Quoted 27 lines · On Tue, 12 Oct 2010 19:34:25 +0800 Jean Delvare…

Hi Feng,
Quoted 20 lines · Feng Tang
Quoted 11 lines · On Tue, 12 Oct 2010 16:47:07 +0800 Jean Delvare…
Of course i2c_detect gets called at times, otherwise we would delete it altogether ;)
Please point me to the I2C adapter driver(s) you care about, and also the platform init code for your system. I would like to see how it looks like.
I think it would also help if I had a global picture of your project and what you are trying to achieve. I presume we are talking about an embedded system? With a custom kernel maybe? Which platform/architecture?
Yes, we are working a x86 based embedded system, Intel's Moorestown and Medfield systems, its current platform init code is arch/x86/kernel/mrst.c, but the I2c init code hasn't been pushed upstream yet :(, but basically it get i2c devices info from a SFI table (drivers/sfi/) and use i2c_register_board_info for them.
Quoted 3 lines
The adapter driver is i2c-mrst.c which is not in current i2c subsystem yet, seems it has been submitted to i2c devel list before. And our platform has many identical adapters.

Quoted 6 lines

I remember that driver, I reviewed it long ago (more than 1 year ago I think). Apparently driver was renamed to i2c-intel-mid meanwhile. Still not upstream indeed, but the last version of the driver posted by Alan Cox [1] does _not_ set the i2c_adapter.class field.
[1] http://marc.info/?l=linux-i2c&m=128292492431251&w=2

Yes, it was. But the latest code we are using set the class, that's why we hit the problem :(

Quoted 20 lines

Quoted 8 lines
Quoted 7 lines
(...)
But you normally only load the hwmon drivers you need on a given machine. That's only a couple of them. Or are you, by any chance, building a monolithic kernel with all hwmon drivers included? This would indeed be a problem, the i2c subsystem isn't prepared for that.
Quoted 6 lines
Actually we run into a long boot time problem while we build in only one i2c device driver (drivers/hwmon/emc1403.c) which has 4 addresses in its address lit, it took more than 10 seconds for the driver to init on our platforms. So if we build in more i2c hwmon drivers, things will get much worse. That's why Jacob posted the patch.
This suggests that the i2c adapter driver is pretty bad at handling NACKs when trying to access a non-existing device. It might be worth checking if this can be improved, because that case could happen not only during device detection but also in other circumstances.

Quoted 7 lines

Quoted 3 lines
I did read code about setting adapter->class, seems most driver set its class in their proble function, trying set it in platform code may looks not very clean.
Actually this is very clean and by far my preferred way to solve your problem. This is exactly how things are done on the PXA platform (check i2c-pxa.c.)

Yes, setting the class to 0 should fix our problem, though still need the driver author to confirm the change doesn't have other side effects. Thanks for your time helping figure this out.

But I still have some concern (not specific to our platforms), I just checked, there are 42 i2c adapter drivers set their class to I2C_CLASS_HWMON and 50 hwmon i2c device drivers set it as well. So if a kernel has an adapter driver and hwmon driver which both set I2C_CLASS_HWMON, the long init time problem will show up again, and it will be worse if there are multiple adapters HW there.

Thanks,
Feng

Quoted 18 lines

Quoted 5 lines
Yours suggestion of using a build time option definitely will shrink the kernel size. But our platform is under arch/x86, and one general rule is to one generic kernel config should work for all x86 platforms, so I'm afraid it can't solve our problem.
OK, fair enough.
Quoted 3 lines
So I still prefer the option of adding a flag, and leave the choice for platform code whether to do the detect. If you agree, I would make a cleaner patch.
And I prefer that you set the class flag value in the platform data and have your i2c adapter driver read it. This is already supported and should work very fine without any change to i2c-core. I see no point in adding a global flag when we already have a much finer-grained setting available.

Jean Delvare

2010-10-13 09:29 UTC #18 lore raw

Quoted 6 lines · Feng Tang

But I still have some concern (not specific to our platforms), I just checked, there are 42 i2c adapter drivers set their class to I2C_CLASS_HWMON and 50 hwmon i2c device drivers set it as well. So if a kernel has an adapter driver and hwmon driver which both set I2C_CLASS_HWMON, the long init time problem will show up again, and it will be worse if there are multiple adapters HW there.

It has been that way for a couple years now and has never been reported to be a problem in practice.

The overall number of drivers setting class flag I2C_CLASS_HWMON is irrelevant, as on a given system, only a few I2C adapter and device drivers will be loaded. As I said before, it would become a problem if you loaded a huger number of I2C-based hwmon device drivers you don't need, which would happen if you were to build a kernel with all hwmon drivers build-in. But this doesn't sound like a sane thing to do.

Also please note that I still believe that your I2C adapter driver could be improved to better deal with this case. Loading the adm1021 hwmon driver on my systems only takes 21 ms, even though it probes 9 addresses. If proving 4 * 5 addresses on your system took a noticeable time, then I suspect that your adapter driver isn't dealing with that case properly.

But anyway, if you're on an embedded system and care a lot about very fast boot times, then you don't want to do any I2c device probing at all. You want all your devices enumerated as platform data, that's the fastest option.

Signature · 1 line

Jean Delvare