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

login
register
mail settings
Submitter Wolfgang Grandegger
Date March 31, 2009, 12:43 p.m.
Message ID <20090331124348.330571158@denx.de>
Download mbox | patch
Permalink /patch/25402/
State Superseded, archived
Headers show

Comments

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

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