[1/2] powerpc: i2c-mpc: preserve I2C clocking

Submitted by Wolfgang Grandegger on March 31, 2009, 12:43 p.m.

Details

Message ID 20090331124348.330571158@denx.de
State Superseded, archived
Headers show

Commit Message

Wolfgang Grandegger March 31, 2009, 12:43 p.m.
The I2c node property "fsl,preserve-clocking" allows to overtake the
clock settings from the boot loader and avoids the hard-coded setting.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/i2c/busses/i2c-mpc.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

David Gibson March 31, 2009, 11:06 p.m.
On Tue, Mar 31, 2009 at 02:43:39PM +0200, Wolfgang Grandegger wrote:
> The I2c node property "fsl,preserve-clocking" allows to overtake the
> clock settings from the boot loader and avoids the hard-coded setting.

Hrm.  This is dubious.  The device tree should generally describe
hardware, not OS/driver behaviour which is what this appears to be
doing.  There are exceptions, but you need to justify them.
Wolfgang Grandegger April 1, 2009, 7:40 a.m.
David Gibson wrote:
> On Tue, Mar 31, 2009 at 02:43:39PM +0200, Wolfgang Grandegger wrote:
>> The I2c node property "fsl,preserve-clocking" allows to overtake the
>> clock settings from the boot loader and avoids the hard-coded setting.
> 
> Hrm.  This is dubious.  The device tree should generally describe
> hardware, not OS/driver behaviour which is what this appears to be
> doing.  There are exceptions, but you need to justify them.

I think the purpose of this property is clear. How would you provide
that functionality instead? I suggested that a "clock-frequency = <0>"
property should do the trick but Grant preferred to be more explicit.

Wolfgang.
David Gibson April 2, 2009, 12:07 a.m.
On Wed, Apr 01, 2009 at 09:40:13AM +0200, Wolfgang Grandegger wrote:
> David Gibson wrote:
> > On Tue, Mar 31, 2009 at 02:43:39PM +0200, Wolfgang Grandegger wrote:
> >> The I2c node property "fsl,preserve-clocking" allows to overtake the
> >> clock settings from the boot loader and avoids the hard-coded setting.
> > 
> > Hrm.  This is dubious.  The device tree should generally describe
> > hardware, not OS/driver behaviour which is what this appears to be
> > doing.  There are exceptions, but you need to justify them.
> 
> I think the purpose of this property is clear. How would you provide
> that functionality instead? I suggested that a "clock-frequency = <0>"
> property should do the trick but Grant preferred to be more
> explicit.

I'm not saying the meaning is unclear, I'm saying it's describing
something that the device tree isn't meant to describe.  AFAICT it's
telling the driver what to do with the device, not a property of the
device itself.

Now, there are cases where it's acceptable to put this sort of
information in the device tree, because it's the least nasty available
solution (flash partition information for example).  But you need to
justify it, if that's so.

Patch hide | download patch | download mbox

Index: linux-2.6/drivers/i2c/busses/i2c-mpc.c
===================================================================
--- linux-2.6.orig/drivers/i2c/busses/i2c-mpc.c	2009-03-31 13:25:08.000000000 +0200
+++ linux-2.6/drivers/i2c/busses/i2c-mpc.c	2009-03-31 13:28:03.000000000 +0200
@@ -318,17 +318,24 @@ 
 {
 	int result = 0;
 	struct mpc_i2c *i2c;
+	int set_clock;
 
 	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
-	if (of_get_property(op->node, "dfsrr", NULL))
-		i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
-
-	if (of_device_is_compatible(op->node, "fsl,mpc5200-i2c") ||
-			of_device_is_compatible(op->node, "mpc5200-i2c"))
-		i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
+	if (of_get_property(op->node, "fsl,preserve-clocking", NULL)) {
+		set_clock = 0;
+	} else {
+		set_clock = 1;
+
+		if (of_get_property(op->node, "dfsrr", NULL))
+			i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
+
+		if (of_device_is_compatible(op->node, "fsl,mpc5200-i2c") ||
+		    of_device_is_compatible(op->node, "mpc5200-i2c"))
+			i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
+	}
 
 	init_waitqueue_head(&i2c->queue);
 
@@ -348,8 +355,9 @@ 
 			goto fail_request;
 		}
 	}
-	
-	mpc_i2c_setclock(i2c);
+
+	if (set_clock)
+		mpc_i2c_setclock(i2c);
 
 	dev_set_drvdata(&op->dev, i2c);