| Message ID | 20090331124348.330571158@denx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
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.
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.
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);
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(-)