[PATCH] i2c: skip address detection if provided in board_info

[PATCH] i2c: skip address detection if provided in board_info

[jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA]

From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

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/CA@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6649176..e4f7feb 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -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;
-- 
1.7.0.4

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jean Delvare]

Hi Jacob,

On Mon, 11 Oct 2010 16:10:35 -0700, jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org wrote:
> From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> 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.

> 
> Signed-off-by: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 6649176..e4f7feb 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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)) {

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

> +			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;


-- 
Jean Delvare

Re: [PATCH] i2c: skip address detection if provided in board_info

[Feng Tang]

Hi Jean,

On Tue, 12 Oct 2010 15:28:22 +0800
Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> Hi Jacob,
> 
> On Mon, 11 Oct 2010 16:10:35 -0700, jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
> wrote:
> > From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > 
> > 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.

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

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index bea4c50..7727105 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -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 */  

> 
> 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.
> 
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> >  drivers/i2c/i2c-core.c |   32 ++++++++++++++++++++++++++++++++
> >  1 files changed, 32 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 6649176..e4f7feb 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -1499,6 +1499,31 @@ static int i2c_detect_address(struct
> > i2c_client *temp_client, return 0;
> >  }

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jean Delvare]

Hi Feng,

On Tue, 12 Oct 2010 16:21:40 +0800, Feng Tang wrote:
> On Tue, 12 Oct 2010 15:28:22 +0800
> Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Mon, 11 Oct 2010 16:10:35 -0700, jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
> > wrote:
> > > From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > 
> > > 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.
> 
> 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?

> 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.

> 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.

> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index bea4c50..7727105 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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 */  

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

Re: [PATCH] i2c: skip address detection if provided in board_info

[Feng Tang]

On Tue, 12 Oct 2010 16:47:07 +0800
Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> Hi Feng,
> 
> On Tue, 12 Oct 2010 16:21:40 +0800, Feng Tang wrote:
> > On Tue, 12 Oct 2010 15:28:22 +0800
> > Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > > On Mon, 11 Oct 2010 16:10:35 -0700, jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
> > > wrote:
> > > > From: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > > 
> > > > 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.
> > 
> > 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?

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. 

> 
> > 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.

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

> 
> 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.
> 
> > 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.
> 
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index bea4c50..7727105 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -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 */  
> 
> 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.
> 

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jean Delvare]

Hi Feng,

On Tue, 12 Oct 2010 17:30:28 +0800, Feng Tang wrote:
> On Tue, 12 Oct 2010 16:47:07 +0800
> Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > 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.
> 
> 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

> > (...)
> > 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.

> 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.)

> 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.

> 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

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jacob Pan]

>
>> 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

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jean Delvare]

Hi Jacob,

On Tue, 12 Oct 2010 11:01:40 -0700, Jacob Pan wrote:
> 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.

> 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.

-- 
Jean Delvare

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jacob Pan]

Hi Jean,

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

>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.

>> 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.
>
to me, the global flag solves the problem where we want faster boottime
in all cases. so it is kinda of a different problem.

>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.

>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

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jean Delvare]

On Wed, 13 Oct 2010 08:54:18 -0700, Jacob Pan wrote:
> Jean Delvare Wed, 13 Oct 2010 09:26:54 +0200
> >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.

> >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?

And what exactly would this bit do?

Confused.

-- 
Jean Delvare

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jacob Pan]

Hi Jean,

>> 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

Re: [PATCH] i2c: skip address detection if provided in board_info

[Feng Tang]

Hi Jean,

On Tue, 12 Oct 2010 19:34:25 +0800
Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> Hi Feng,
> 
> On Tue, 12 Oct 2010 17:30:28 +0800, Feng Tang wrote:
> > On Tue, 12 Oct 2010 16:47:07 +0800
> > Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > > 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.
> > 
> > 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

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

> 
> > > (...)
> > > 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.
> 
> > 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   

> 
> > 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.
> 
> > 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.
> 

Re: [PATCH] i2c: skip address detection if provided in board_info

[Jean Delvare]

On Thu, 14 Oct 2010 01:04:22 +0800, Feng Tang wrote:
> 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.

-- 
Jean Delvare