[RFC] I2C: fsl-i2c: make device probing configurable via FDT

[RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Wolfgang Grandegger]

Currently, the I2C buses are probed for HWMON I2C devices, which might
not be acceptable in same cases. This patch makes device probing
configurable through the property "probe" of the FDT I2C device node:

       i2c@3000 {
            ...
            compatible = "fsl-i2c";
            probe;
	    ...
        };

Assuming that systems using fsl-i2c should have proper platform data,
probing is disabled by default. Unfortunately, that's not the case and
various DTS files would require to be updated.

There have already been some discussion on this subject with Jean
under the following thread:

  http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060008.html

Please comment.

Thanks,

Wolfgang.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 arch/powerpc/sysdev/fsl_soc.c |    4 ++++
 drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
 include/linux/fsl_devices.h   |    1 +
 3 files changed, 19 insertions(+), 3 deletions(-)

Index: linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
===================================================================
--- linux-2.6-denx.orig/drivers/i2c/busses/i2c-mpc.c
+++ linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
@@ -306,13 +306,21 @@ static const struct i2c_algorithm mpc_al
 	.functionality = mpc_functionality,
 };
 
-static struct i2c_adapter mpc_ops = {
+static struct i2c_adapter mpc_probe_ops = {
 	.owner = THIS_MODULE,
 	.name = "MPC adapter",
 	.id = I2C_HW_MPC107,
 	.algo = &mpc_algo,
 	.class = I2C_CLASS_HWMON,
-	.timeout = 1,
+ 	.timeout = 1,
+};
+
+static struct i2c_adapter mpc_ops = {
+	.owner = THIS_MODULE,
+	.name = "MPC adapter",
+	.id = I2C_HW_MPC107,
+	.algo = &mpc_algo,
+ 	.timeout = 1,
 };
 
 static int fsl_i2c_probe(struct platform_device *pdev)
@@ -354,7 +362,10 @@ static int fsl_i2c_probe(struct platform
 	mpc_i2c_setclock(i2c);
 	platform_set_drvdata(pdev, i2c);
 
-	i2c->adap = mpc_ops;
+	(i2c->flags & FSL_I2C_DEV_PROBE)
+		i2c->adap = mpc_probe_ops;
+	else
+		i2c->adap = mpc_ops;
 	i2c->adap.nr = pdev->id;
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &pdev->dev;
Index: linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
===================================================================
--- linux-2.6-denx.orig/arch/powerpc/sysdev/fsl_soc.c
+++ linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
@@ -518,6 +518,10 @@ static int __init fsl_i2c_of_init(void)
 		if (flags)
 			i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200;
 
+		flags = of_get_property(np, "probe", NULL);
+		if (flags)
+			i2c_data.device_flags |= FSL_I2C_DEV_PROBE;
+
 		ret =
 		    platform_device_add_data(i2c_dev, &i2c_data,
 					     sizeof(struct
Index: linux-2.6-denx/include/linux/fsl_devices.h
===================================================================
--- linux-2.6-denx.orig/include/linux/fsl_devices.h
+++ linux-2.6-denx/include/linux/fsl_devices.h
@@ -82,6 +82,7 @@ struct fsl_i2c_platform_data {
 /* Flags related to I2C device features */
 #define FSL_I2C_DEV_SEPARATE_DFSRR	0x00000001
 #define FSL_I2C_DEV_CLOCK_5200		0x00000002
+#define FSL_I2C_DEV_PROBE		0x00000004
 
 enum fsl_usb2_operating_modes {
 	FSL_USB2_MPH_HOST,

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Jean Delvare]

Hi Wolfgang,

On Wed, 16 Jul 2008 12:47:25 +0200, Wolfgang Grandegger wrote:
> Currently, the I2C buses are probed for HWMON I2C devices, which might
> not be acceptable in same cases. This patch makes device probing
> configurable through the property "probe" of the FDT I2C device node:
> 
>        i2c@3000 {
>             ...
>             compatible = "fsl-i2c";
>             probe;
> 	    ...
>         };
> 
> Assuming that systems using fsl-i2c should have proper platform data,
> probing is disabled by default. Unfortunately, that's not the case and
> various DTS files would require to be updated.
> 
> There have already been some discussion on this subject with Jean
> under the following thread:
> 
>   http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060008.html
> 
> Please comment.
> 
> Thanks,
> 
> Wolfgang.
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  arch/powerpc/sysdev/fsl_soc.c |    4 ++++
>  drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
>  include/linux/fsl_devices.h   |    1 +
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
> ===================================================================
> --- linux-2.6-denx.orig/drivers/i2c/busses/i2c-mpc.c
> +++ linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
> @@ -306,13 +306,21 @@ static const struct i2c_algorithm mpc_al
>  	.functionality = mpc_functionality,
>  };
>  
> -static struct i2c_adapter mpc_ops = {
> +static struct i2c_adapter mpc_probe_ops = {
>  	.owner = THIS_MODULE,
>  	.name = "MPC adapter",
>  	.id = I2C_HW_MPC107,
>  	.algo = &mpc_algo,
>  	.class = I2C_CLASS_HWMON,
> -	.timeout = 1,
> + 	.timeout = 1,

Your patch is adding undesirable leading white space.

> +};
> +
> +static struct i2c_adapter mpc_ops = {
> +	.owner = THIS_MODULE,
> +	.name = "MPC adapter",
> +	.id = I2C_HW_MPC107,
> +	.algo = &mpc_algo,
> + 	.timeout = 1,
>  };

I don't think you need 2 separate structures. Just set the class field
at runtime (see below.)

>  
>  static int fsl_i2c_probe(struct platform_device *pdev)
> @@ -354,7 +362,10 @@ static int fsl_i2c_probe(struct platform
>  	mpc_i2c_setclock(i2c);
>  	platform_set_drvdata(pdev, i2c);
>  
> -	i2c->adap = mpc_ops;
> +	(i2c->flags & FSL_I2C_DEV_PROBE)

Missing if? :)

> +		i2c->adap = mpc_probe_ops;
> +	else
> +		i2c->adap = mpc_ops;

Or just:

	i2c->adap = mpc_ops;
	if (i2c->flags & FSL_I2C_DEV_PROBE)
		i2c->adap.flags = I2C_CLASS_HWMON;

>  	i2c->adap.nr = pdev->id;
>  	i2c_set_adapdata(&i2c->adap, i2c);
>  	i2c->adap.dev.parent = &pdev->dev;
> Index: linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
> ===================================================================
> --- linux-2.6-denx.orig/arch/powerpc/sysdev/fsl_soc.c
> +++ linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
> @@ -518,6 +518,10 @@ static int __init fsl_i2c_of_init(void)
>  		if (flags)
>  			i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200;
>  
> +		flags = of_get_property(np, "probe", NULL);
> +		if (flags)
> +			i2c_data.device_flags |= FSL_I2C_DEV_PROBE;
> +
>  		ret =
>  		    platform_device_add_data(i2c_dev, &i2c_data,
>  					     sizeof(struct
> Index: linux-2.6-denx/include/linux/fsl_devices.h
> ===================================================================
> --- linux-2.6-denx.orig/include/linux/fsl_devices.h
> +++ linux-2.6-denx/include/linux/fsl_devices.h
> @@ -82,6 +82,7 @@ struct fsl_i2c_platform_data {
>  /* Flags related to I2C device features */
>  #define FSL_I2C_DEV_SEPARATE_DFSRR	0x00000001
>  #define FSL_I2C_DEV_CLOCK_5200		0x00000002
> +#define FSL_I2C_DEV_PROBE		0x00000004
>  
>  enum fsl_usb2_operating_modes {
>  	FSL_USB2_MPH_HOST,

As a side note: in general it is not a good idea to use a template for
struct i2c_adapter. This structure is relatively large compared to the
few fields you need to set. If you insist on having a template, it
should at least be marked either const or __initdata.

-- 
Jean Delvare

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Wolfgang Grandegger]

Jean Delvare wrote:
> Hi Wolfgang,
> 
> On Wed, 16 Jul 2008 12:47:25 +0200, Wolfgang Grandegger wrote:
>> Currently, the I2C buses are probed for HWMON I2C devices, which might
>> not be acceptable in same cases. This patch makes device probing
>> configurable through the property "probe" of the FDT I2C device node:
>>
>>        i2c@3000 {
>>             ...
>>             compatible = "fsl-i2c";
>>             probe;
>> 	    ...
>>         };
>>
>> Assuming that systems using fsl-i2c should have proper platform data,
>> probing is disabled by default. Unfortunately, that's not the case and
>> various DTS files would require to be updated.
>>
>> There have already been some discussion on this subject with Jean
>> under the following thread:
>>
>>   http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060008.html
>>
>> Please comment.
>>
>> Thanks,
>>
>> Wolfgang.
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  arch/powerpc/sysdev/fsl_soc.c |    4 ++++
>>  drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
>>  include/linux/fsl_devices.h   |    1 +
>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> Index: linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
>> ===================================================================
>> --- linux-2.6-denx.orig/drivers/i2c/busses/i2c-mpc.c
>> +++ linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
>> @@ -306,13 +306,21 @@ static const struct i2c_algorithm mpc_al
>>  	.functionality = mpc_functionality,
>>  };
>>  
>> -static struct i2c_adapter mpc_ops = {
>> +static struct i2c_adapter mpc_probe_ops = {
>>  	.owner = THIS_MODULE,
>>  	.name = "MPC adapter",
>>  	.id = I2C_HW_MPC107,
>>  	.algo = &mpc_algo,
>>  	.class = I2C_CLASS_HWMON,
>> -	.timeout = 1,
>> + 	.timeout = 1,
> 
> Your patch is adding undesirable leading white space.
> 
>> +};
>> +
>> +static struct i2c_adapter mpc_ops = {
>> +	.owner = THIS_MODULE,
>> +	.name = "MPC adapter",
>> +	.id = I2C_HW_MPC107,
>> +	.algo = &mpc_algo,
>> + 	.timeout = 1,
>>  };
> 
> I don't think you need 2 separate structures. Just set the class field
> at runtime (see below.)
> 
>>  
>>  static int fsl_i2c_probe(struct platform_device *pdev)
>> @@ -354,7 +362,10 @@ static int fsl_i2c_probe(struct platform
>>  	mpc_i2c_setclock(i2c);
>>  	platform_set_drvdata(pdev, i2c);
>>  
>> -	i2c->adap = mpc_ops;
>> +	(i2c->flags & FSL_I2C_DEV_PROBE)
> 
> Missing if? :)

Oops, I obviously forgot the quilt refresh before sending.

> 
>> +		i2c->adap = mpc_probe_ops;
>> +	else
>> +		i2c->adap = mpc_ops;
> 
> Or just:
> 
> 	i2c->adap = mpc_ops;
> 	if (i2c->flags & FSL_I2C_DEV_PROBE)
> 		i2c->adap.flags = I2C_CLASS_HWMON;

Right, mpc_ops is not a pointer, as I realize now.

>>  	i2c->adap.nr = pdev->id;
>>  	i2c_set_adapdata(&i2c->adap, i2c);
>>  	i2c->adap.dev.parent = &pdev->dev;
>> Index: linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
>> ===================================================================
>> --- linux-2.6-denx.orig/arch/powerpc/sysdev/fsl_soc.c
>> +++ linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
>> @@ -518,6 +518,10 @@ static int __init fsl_i2c_of_init(void)
>>  		if (flags)
>>  			i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200;
>>  
>> +		flags = of_get_property(np, "probe", NULL);
>> +		if (flags)
>> +			i2c_data.device_flags |= FSL_I2C_DEV_PROBE;
>> +
>>  		ret =
>>  		    platform_device_add_data(i2c_dev, &i2c_data,
>>  					     sizeof(struct
>> Index: linux-2.6-denx/include/linux/fsl_devices.h
>> ===================================================================
>> --- linux-2.6-denx.orig/include/linux/fsl_devices.h
>> +++ linux-2.6-denx/include/linux/fsl_devices.h
>> @@ -82,6 +82,7 @@ struct fsl_i2c_platform_data {
>>  /* Flags related to I2C device features */
>>  #define FSL_I2C_DEV_SEPARATE_DFSRR	0x00000001
>>  #define FSL_I2C_DEV_CLOCK_5200		0x00000002
>> +#define FSL_I2C_DEV_PROBE		0x00000004
>>  
>>  enum fsl_usb2_operating_modes {
>>  	FSL_USB2_MPH_HOST,
> 
> As a side note: in general it is not a good idea to use a template for
> struct i2c_adapter. This structure is relatively large compared to the
> few fields you need to set. If you insist on having a template, it
> should at least be marked either const or __initdata.

Wolfgang.

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Jon Smirl]

On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Currently, the I2C buses are probed for HWMON I2C devices, which might
>  not be acceptable in same cases. This patch makes device probing
>  configurable through the property "probe" of the FDT I2C device node:

All this patch seems to be doing is removing class I2C_CLASS_HWMON via
a device tree flag, PROBE.

Why do you need to do this? The OF conversion patch that is working
its way through the system lets you put the address of the device into
the device tree node. Probing shouldn't be necessary at all.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Wolfgang Grandegger]

Jon Smirl wrote:
> On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Currently, the I2C buses are probed for HWMON I2C devices, which might
>>  not be acceptable in same cases. This patch makes device probing
>>  configurable through the property "probe" of the FDT I2C device node:
> 
> All this patch seems to be doing is removing class I2C_CLASS_HWMON via
> a device tree flag, PROBE.

Yep.

> Why do you need to do this? The OF conversion patch that is working
> its way through the system lets you put the address of the device into
> the device tree node. Probing shouldn't be necessary at all.

Currently I2C_CLASS_HWMON is used for all I2C adapters and therefore 
probing is done even with a proper I2C node defined in the DTS file. 
That's what the patch is fixing leaving the possibility to (re-)enable 
legacy probing.

For some more details you may want to have a look to Jean's mail on that 
subject: http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060012.html.

Wolfgang.

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Jon Smirl]

On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Jon Smirl wrote:
>
> > On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> >
> > > Currently, the I2C buses are probed for HWMON I2C devices, which might
> > >  not be acceptable in same cases. This patch makes device probing
> > >  configurable through the property "probe" of the FDT I2C device node:
> > >
> >
> > All this patch seems to be doing is removing class I2C_CLASS_HWMON via
> > a device tree flag, PROBE.
> >
>
>  Yep.
>
>
> > Why do you need to do this? The OF conversion patch that is working
> > its way through the system lets you put the address of the device into
> > the device tree node. Probing shouldn't be necessary at all.
> >
>
>  Currently I2C_CLASS_HWMON is used for all I2C adapters and therefore
> probing is done even with a proper I2C node defined in the DTS file. That's
> what the patch is fixing leaving the possibility to (re-)enable legacy
> probing.

Shouldn't we just remove I2C_CLASS_HWMON since everything can now be
explicitly described in the device tree? What is an example of a case
where we still need to probe?

>
>  For some more details you may want to have a look to Jean's mail on that
> subject:
> http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060012.html.
>
>  Wolfgang.
>
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Jon Smirl]

On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Jon Smirl wrote:
>
> > On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> >
> > > Currently, the I2C buses are probed for HWMON I2C devices, which might
> > >  not be acceptable in same cases. This patch makes device probing
> > >  configurable through the property "probe" of the FDT I2C device node:
> > >
> >
> > All this patch seems to be doing is removing class I2C_CLASS_HWMON via
> > a device tree flag, PROBE.

If this is implemented it shouldn't be a device tree option, it should
be a compile time option in the Kconfig system. We don't want to
pollute a platform independent device tree with Linux clutter.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Grant Likely]

On Wed, Jul 16, 2008 at 10:24:22AM -0400, Jon Smirl wrote:
> On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> > Jon Smirl wrote:
> >
> > > On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
> > >
> > > > Currently, the I2C buses are probed for HWMON I2C devices, which might
> > > >  not be acceptable in same cases. This patch makes device probing
> > > >  configurable through the property "probe" of the FDT I2C device node:
> > > >
> > >
> > > All this patch seems to be doing is removing class I2C_CLASS_HWMON via
> > > a device tree flag, PROBE.
> 
> If this is implemented it shouldn't be a device tree option, it should
> be a compile time option in the Kconfig system. We don't want to
> pollute a platform independent device tree with Linux clutter.

I'm not sure about this.  It is somewhat describing the hardware.  It
sounds like it is saying "hey, I've got an i2c bus here, but I don't
know what is on it; so instead of providing a set of child nodes, you
should probe this bus to find out what it has".

I don't see any problem specifying it in the device tree.

Making it a Kconfig option forces *all* i2c busses on supported
platforms into either probe or no-probe mode.  Not a multiplatform
friendly solution.  Heck, we've got multiple i2c busses on some chips and
it is conceivable that only one would need to be probed.

Cheers,
g.

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Jon Smirl]

On 7/16/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Jul 16, 2008 at 10:24:22AM -0400, Jon Smirl wrote:
>  > On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
>  > > Jon Smirl wrote:
>  > >
>  > > > On 7/16/08, Wolfgang Grandegger <wg@grandegger.com> wrote:
>  > > >
>  > > > > Currently, the I2C buses are probed for HWMON I2C devices, which might
>  > > > >  not be acceptable in same cases. This patch makes device probing
>  > > > >  configurable through the property "probe" of the FDT I2C device node:
>  > > > >
>  > > >
>  > > > All this patch seems to be doing is removing class I2C_CLASS_HWMON via
>  > > > a device tree flag, PROBE.
>  >
>  > If this is implemented it shouldn't be a device tree option, it should
>  > be a compile time option in the Kconfig system. We don't want to
>  > pollute a platform independent device tree with Linux clutter.
>
>
> I'm not sure about this.  It is somewhat describing the hardware.  It
>  sounds like it is saying "hey, I've got an i2c bus here, but I don't
>  know what is on it; so instead of providing a set of child nodes, you
>  should probe this bus to find out what it has".

Probing an i2c bus does not necessarily come to a good conclusion. The
probes for some chips can alter the states in others. People have
accidentally changed voltage settings and fried CPU chips. The process
is not well defined.

I'm not saying remove i2c probing altogether, some things require
probing like EDID data from a monitor. Since these are embedded system
I just think it is pretty unlikely that stuff like that is going to
get hooked to an i2c-mpc bus.  That was behind the idea to push it
back to a compile time option.

>
>  I don't see any problem specifying it in the device tree.
>
>  Making it a Kconfig option forces *all* i2c busses on supported
>  platforms into either probe or no-probe mode.  Not a multiplatform
>  friendly solution.  Heck, we've got multiple i2c busses on some chips and
>  it is conceivable that only one would need to be probed.
>
>  Cheers,
>
> g.
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Timur Tabi]

Jon Smirl wrote:

> Probing an i2c bus does not necessarily come to a good conclusion. The
> probes for some chips can alter the states in others. People have
> accidentally changed voltage settings and fried CPU chips. The process
> is not well defined.

I agree.  We should not be implementing any feature that encourages I2C bus
probing.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Grant Likely]

On Wed, Jul 16, 2008 at 12:47:25PM +0200, Wolfgang Grandegger wrote:
> Currently, the I2C buses are probed for HWMON I2C devices, which might
> not be acceptable in same cases. This patch makes device probing
> configurable through the property "probe" of the FDT I2C device node:
>
>       i2c@3000 {
>            ...
>            compatible = "fsl-i2c";
>            probe;
> 	    ...
>        };

You need to add documentation to booting-without-of.txt about what the
'probe' property means.  Also, 'probe' is a pretty generic term being
used in an i2c specific context.  Can you prefix it with i2c or
something to protect the namespace?  'i2c-probe' perhaps?

> Assuming that systems using fsl-i2c should have proper platform data,
> probing is disabled by default. Unfortunately, that's not the case and
> various DTS files would require to be updated.

This comment doesn't seem to be correct.

> ---
> arch/powerpc/sysdev/fsl_soc.c |    4 ++++
> drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
> include/linux/fsl_devices.h   |    1 +

This will need to be reworked for current top of tree.  i2c-mpc
dependence on fsl_soc.c has been removed.

Thanks,
g.

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Jochen Friedrich]

Hi Grant,

> On Wed, Jul 16, 2008 at 12:47:25PM +0200, Wolfgang Grandegger wrote:
>> Currently, the I2C buses are probed for HWMON I2C devices, which might
>> not be acceptable in same cases. This patch makes device probing
>> configurable through the property "probe" of the FDT I2C device node:
>>
>>       i2c@3000 {
>>            ...
>>            compatible = "fsl-i2c";
>>            probe;
>> 	    ...
>>        };
> 
> You need to add documentation to booting-without-of.txt about what the
> 'probe' property means.  Also, 'probe' is a pretty generic term being
> used in an i2c specific context.  Can you prefix it with i2c or
> something to protect the namespace?  'i2c-probe' perhaps?

In i2c-cpm, I currently use the property "linux,i2c-class" to set the
i2c class directly as documented in dts-bindings/fsl/cpm_qe/cpm/i2c.txt.

We should agree on *one* method to set the i2c class.

Thanks,
Jochen

Re: [RFC] I2C: fsl-i2c: make device probing configurable via FDT

[Wolfgang Grandegger]

Grant Likely wrote:
> On Wed, Jul 16, 2008 at 12:47:25PM +0200, Wolfgang Grandegger wrote:
>> Currently, the I2C buses are probed for HWMON I2C devices, which might
>> not be acceptable in same cases. This patch makes device probing
>> configurable through the property "probe" of the FDT I2C device node:
>>
>>       i2c@3000 {
>>            ...
>>            compatible = "fsl-i2c";
>>            probe;
>> 	    ...
>>        };
> 
> You need to add documentation to booting-without-of.txt about what the
> 'probe' property means.  Also, 'probe' is a pretty generic term being
> used in an i2c specific context.  Can you prefix it with i2c or
> something to protect the namespace?  'i2c-probe' perhaps?

Of course, no problem. I'm also aware of booting-without-of.txt but 
first I wanted to bring up the problem-
> 
>> Assuming that systems using fsl-i2c should have proper platform data,
>> probing is disabled by default. Unfortunately, that's not the case and
>> various DTS files would require to be updated.
> 
> This comment doesn't seem to be correct.

Well, maybe it's due to my bad English. What I wanted to say is that by 
default probing should be disabled. But that requires various DTS files 
to be fixed.

> 
>> ---
>> arch/powerpc/sysdev/fsl_soc.c |    4 ++++
>> drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
>> include/linux/fsl_devices.h   |    1 +
> 
> This will need to be reworked for current top of tree.  i2c-mpc
> dependence on fsl_soc.c has been removed.

I kown. Will prepare a new patch a.s.a.p.

Wolfgang.

Is there relationship between address translation enabled and PLB timeout error?

[Evangelion]

Hello, all:
    I am building a Linux kernel module for PPC405EP. My developing
board is PPChameleonEVB. I am debugging with BDI2000 and GDB, and my
problem is:
    In GDB, a section of the codes is disassembled to:
      mfmsr   r0
      ori     r0,r0,32768
      mtmsr   r0
      blr
    From BDI2000, I have checked that after "ori", GPR0 contains
"0x00029030". This value should be written into MSR by "mtmsr" to set EE
bit of MSR as 1, but after single step in BDI, "mtmsr" does not work as
hoped. MSR becomes "0x00000030", GPR0 becomes some weird number, and
there is "Step timeout detected". Meanwhile, the board traps into "Data
machine check in kernel mode". I also have tried "wrteei 1" instead of
the codes above, but failed again. However, those codes works well in
PPC440EP Yosemite board.
    I have compared PPChameleonEVB and Yosemite. It seems the most
significant difference is for PPChameleonEVB, address translation is
enabled by default (MSR[IR, DR] = 1), while for Yosemite, it is disabled
(MSR[IR, DR] = 0). And from the error message, I think there is a
problem with PLB because PLB0_BESR becomes 0x00c00000 from 0x00000000
and PLB0_BEAR becomes 0x03066004 when machine check happens. Based on
the processor manual, the PLB0_BESR shows the PLB Timeout Error Status
and the value means the error operation is reading the processor core
DCU. The PLB0_BEAR shows the address of the access on which a bus
timeout error occurred. So I have the following questions for this
moment:
    1. Is it possible or not that address translation leads to the PLB
timeout error? If that is the cause, how to fix the problem?
    2. Is the address in PLB0_BEAR a memory address (real address or
effective address) or a bus address (not an address in any kind of
memory)?
    3. Are there other reasons for the machine check in this situation?
    4. Is it an unrecoverable hardware problem (bug) or not?

Here is the debugging log:

405EP>ti
    Core number       : 0
    Core state        : debug mode
    Debug entry cause : single step
    Current PC        : 0xc32b1008
    Current CR        : 0x84000084
    Current MSR       : 0x00021030
    Current LR        : 0xc32b46c4
405EP>rd
GPR00: 00029030 c1dd9d60 c1fe7bf0 00000000
GPR04: 00000001 00000000 c32b2c8c 00000000
GPR08: c3068000 c3068000 00000001 c3062000
GPR12: 00000000 10019dd8 c32c0000 c32b0000
GPR16: 00000001 c32b0000 00000002 7ff4f670
GPR20: 00000028 c32b0000 c32b0000 10011000
GPR24: c306a000 00000000 00000000 10012c6c
GPR28: c18e4000 c32c0000 00000000 00000000
CR   : 84000084     MSR: 00021030
405EP>ti
    Core number       : 0
    Core state        : debug mode
    Debug entry cause : JTAG stop request
    Current PC        : 0xc000490c
    Current CR        : 0x42000082
    Current MSR       : 0x00000030
    Current LR        : 0xc001f1b8
# Step timeout detected
405EP>rd
GPR00: 03929800 c02f3e60 c3066000 000102f1
GPR04: 00005424 00000007 c0146f3c c0260000
GPR08: 00000000 c02d0000 c3062000 00000000
GPR12: 00000000 10019dd8 c32c0000 c32b0000
GPR16: 00000001 c32b0000 00000002 7ff4f670
GPR20: 00000028 c32b0000 c32b0000 10011000
GPR24: c306a000 00000000 00000000 10012c6c
GPR28: c02f0000 00000152 c3066000 c02f0000
CR   : 42000082     MSR: 00000030

==========================================
Here is the error message:

Data machine check in kernel mode.
PLB0: BEAR= 0x03066004 ACR=   0x00000000 BESR=  0x00c00000
PLB0 to OPB: BEAR= 0x04000000 BESR0= 0x00000000 BESR1= 0x00000000
Oops: machine check, sig: 7 [#1]
NIP: 00002AD0 LR: 000005A0 CTR: C000CC58
REGS: c02f3f50 TRAP: 0202   Not tainted (2.6.19.2-eldk)
MSR: 00021000 <ME>  CR: 24000084  XER: 20000000
TASK = c3066000[0] '' THREAD: c02d2000
GPR00: 00029030 C1DD9CA0 C3066000 C1DD9CB0 00000001 00000000 C32B2C8C
00000000
GPR08: C3068000 00000000 00021032 01DD9CA0 030661B0 10019DD8 C32C0000
C32B0000
GPR16: 00000001 C32B0000 00000002 7FF4F670 00000028 C32B0000 C32B0000
10011000
GPR24: C306A000 00000000 00000000 10012C6C C18E4000 C32C0000 00000000
00000000
NIP [00002AD0] 0x2ad0
LR [000005A0] 0x5a0
Call Trace:
Instruction dump:
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX 
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX 
Kernel panic - not syncing: Attempted to kill the idle task!
 <0>Rebooting in 180 seconds..

Thanks a lot for advice!

Best Regards

Evangelion
July 17th, 2008

__________________________________________________
¸Ï¿ì×¢²áÑÅ»¢³¬´óÈÝÁ¿Ãâ·ÑÓÊÏä?
http://cn.mail.yahoo.com