[PATCH / RFC] Basic OMAP2430 High speed I2C & Speed handling (final)

[PATCH / RFC] Basic OMAP2430 High speed I2C & Speed handling (final)

[Syed Mohammed, Khasim]

This patch moves I2C speed parameter (from module) to platform data.
This is part of a discussion that Nishanth and I were discussing (also
on
lmsensors). 

I have also added basic High Speed support based on I2C bus speed. 
This patch is tested for high speed I2C (with TWL4030 Keypad) and works
as expected.
Signed-off-by: Syed Mohammed Khasim  <x0khasim@ti.com>

Files:
 drivers/i2c/busses/i2c-omap.c
 arch/arm/mach-omap2/clock.h
 arch/arm/mach-omap2/devices.c
 arch/arm/plat-omap/devices.c 

=======================================================================
--- linux-omap/drivers/i2c/busses/i2c-omap.c	2007-01-08
18:56:31.000000000 -0600
+++ lin_for_hsi2c/drivers/i2c/busses/i2c-omap.c	2007-04-11
11:47:54.000000000 -0500
@@ -3,7 +3,10 @@
  *
  * Copyright (C) 2003 MontaVista Software, Inc.
  * Copyright (C) 2004 Texas Instruments.
- *
+ *
+ * Updated to support High Speed I2C mode by
+ * Syed Mohammed Khasim <x0khasim@ti.com>
+ *
  * Updated to work with multiple I2C interfaces on 24xx by
  * Tony Lindgren <tony@atomide.com> and Imre Deak <imre.deak@nokia.com>
  * Copyright (C) 2005 Nokia Corporation
@@ -87,6 +90,7 @@
 /* I2C Configuration Register (OMAP_I2C_CON): */
 #define OMAP_I2C_CON_EN		(1 << 15)	/* I2C module
enable */
 #define OMAP_I2C_CON_BE		(1 << 14)	/* Big endian
mode */
+#define OMAP_I2C_CON_OPMODE	(1 << 12)	/* High Speed support */
 #define OMAP_I2C_CON_STB	(1 << 11)	/* Start byte mode
(master) */
 #define OMAP_I2C_CON_MST	(1 << 10)	/* Master/slave mode */
 #define OMAP_I2C_CON_TRX	(1 << 9)	/* TX/RX mode (master
only) */
@@ -95,6 +99,10 @@
 #define OMAP_I2C_CON_STP	(1 << 1)	/* Stop cond (master
only) */
 #define OMAP_I2C_CON_STT	(1 << 0)	/* Start condition
(master) */
 
+/* I2C SCL time value when Master */
+#define OMAP_I2C_SCLL_HSSCLL	8
+#define OMAP_I2C_SCLH_HSSCLH	8
+
 /* I2C System Test Register (OMAP_I2C_SYSTEST): */
 #ifdef DEBUG
 #define OMAP_I2C_SYSTEST_ST_EN		(1 << 15)	/* System test
enable */
@@ -113,12 +121,6 @@
 /* I2C System Configuration Register (OMAP_I2C_SYSC): */
 #define OMAP_I2C_SYSC_SRST		(1 << 1)	/* Soft Reset */
 
-/* REVISIT: Use platform_data instead of module parameters */
-/* Fast Mode = 400 kHz, Standard = 100 kHz */
-static int clock = 100; /* Default: 100 kHz */
-module_param(clock, int, 0);
-MODULE_PARM_DESC(clock, "Set I2C clock in kHz: 400=fast mode (default
== 100)");
-
 struct omap_i2c_dev {
 	struct device		*dev;
 	void __iomem		*base;		/* virtual */
@@ -127,6 +129,7 @@ struct omap_i2c_dev {
 	struct clk		*fclk;		/* Functional clock */
 	struct completion	cmd_complete;
 	struct resource		*ioarea;
+	u32			speed;		/* Speed of bus in Khz
*/
 	u16			cmd_err;
 	u8			*buf;
 	size_t			buf_len;
@@ -154,17 +157,30 @@ static int omap_i2c_get_clocks(struct om
 			return -ENODEV;
 		}
 	}
-
-	dev->fclk = clk_get(dev->dev, "i2c_fck");
-	if (IS_ERR(dev->fclk)) {
-		if (dev->iclk != NULL) {
-			clk_put(dev->iclk);
-			dev->iclk = NULL;
+
+	if (cpu_is_omap2430()) {
+		dev->fclk = clk_get(dev->dev, "i2chs_fck");
+		if (IS_ERR(dev->fclk)) {
+			if (dev->iclk != NULL) {
+				clk_put(dev->iclk);
+				dev->iclk = NULL;
+			}
+			dev->fclk = NULL;
+			return -ENODEV;
 		}
-		dev->fclk = NULL;
-		return -ENODEV;
 	}
-
+	else {
+		dev->fclk = clk_get(dev->dev, "i2c_fck");
+		if (IS_ERR(dev->fclk)) {
+			if (dev->iclk != NULL) {
+				clk_put(dev->iclk);
+				dev->iclk = NULL;
+			}
+			dev->fclk = NULL;
+			return -ENODEV;
+		}
+	}
+
 	return 0;
 }
 
@@ -194,9 +210,11 @@ static void omap_i2c_disable_clocks(stru
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
 {
-	u16 psc = 0;
+	u16 psc = 0, scll = 0, sclh = 0;
+	u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
 	unsigned long fclk_rate = 12000000;
 	unsigned long timeout;
+	unsigned long internal_clk = 0;
 
 	if (!dev->rev1) {
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
OMAP_I2C_SYSC_SRST);
@@ -238,28 +256,59 @@ static int omap_i2c_init(struct omap_i2c
 		if (fclk_rate > 12000000)
 			psc = fclk_rate / 12000000;
 	}
+
+	if (cpu_is_omap2430()) {
+
+		/* HS I2C controller should be operated at 19.2 Mhz */
+		internal_clk = 19200;
+		fclk_rate = clk_get_rate(dev->fclk) / 1000;
+
+		/* Compute prescaler divisor */
+		psc = fclk_rate / internal_clk;
+		psc = psc - 1;
+
+		/* If configured for High Speed */
+		if (dev->speed > 400) {
+			/* For first phase of HS mode */
+			fsscll = internal_clk / (400 * 2) - 6;
+			fssclh = internal_clk / (400 * 2) - 6;
+
+			/* For second phase of HS mode */
+			hsscll = fclk_rate / (dev->speed * 2) - 6;
+			hssclh = fclk_rate / (dev->speed * 2) - 6;
+		}
+		else {
+			/* To handle F/S modes */
+			fsscll = internal_clk / (dev->speed * 2) - 6;
+			fssclh = internal_clk / (dev->speed * 2) - 6;
+		}
+		scll = (hsscll << OMAP_I2C_SCLL_HSSCLL) | fsscll;
+		sclh = (hssclh << OMAP_I2C_SCLH_HSSCLH) | fssclh;
+	}
+	else {
+		/* Program desired operating rate */
+		fclk_rate /= (psc + 1) * 1000;
+		if (psc > 2)
+			psc = 2;
+		scll = fclk_rate / (dev->speed * 2) - 7 + psc;
+		sclh = fclk_rate / (dev->speed * 2) - 7 + psc;
+	}
 
 	/* Setup clock prescaler to obtain approx 12MHz I2C module
clock: */
 	omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
-
-	/* Program desired operating rate */
-	fclk_rate /= (psc + 1) * 1000;
-	if (psc > 2)
-		psc = 2;
-
-	omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG,
-			   fclk_rate / (clock * 2) - 7 + psc);
-	omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG,
-			   fclk_rate / (clock * 2) - 7 + psc);
-
+
+	/* SCL low and high time values */
+	omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
+	omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
+
 	/* Take the I2C module out of reset: */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
 	/* Enable interrupts */
 	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG,
-			   (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
-			    OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
-			    OMAP_I2C_IE_AL));
+				(OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
+				OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
+				OMAP_I2C_IE_AL));
 	return 0;
 }
 
@@ -335,6 +384,11 @@ static int omap_i2c_xfer_msg(struct i2c_
 	dev->cmd_err = 0;
 
 	w = OMAP_I2C_CON_EN | OMAP_I2C_CON_MST | OMAP_I2C_CON_STT;
+
+	/* High speed configuration */
+	if (dev->speed > 400)
+		w |= OMAP_I2C_CON_OPMODE;
+
 	if (msg->flags & I2C_M_TEN)
 		w |= OMAP_I2C_CON_XA;
 	if (!(msg->flags & I2C_M_RD))
@@ -581,7 +635,8 @@ omap_i2c_probe(struct platform_device *p
 	struct i2c_adapter	*adap;
 	struct resource		*mem, *irq, *ioarea;
 	int r;
-
+	u32 *speed = NULL;
+
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {
@@ -601,17 +656,18 @@ omap_i2c_probe(struct platform_device *p
 		return -EBUSY;
 	}
 
-	if (clock > 200)
-		clock = 400;	/* Fast mode */
-	else
-		clock = 100;	/* Standard mode */
-
 	dev = kzalloc(sizeof(struct omap_i2c_dev), GFP_KERNEL);
 	if (!dev) {
 		r = -ENOMEM;
 		goto err_release_region;
 	}
-
+
+	if (pdev->dev.platform_data != NULL)
+		speed = (u32 *) pdev->dev.platform_data;
+	else
+		*speed = 100; /* Defualt speed */
+
+	dev->speed = *speed;
 	dev->dev = &pdev->dev;
 	dev->irq = irq->start;
 	dev->base = (void __iomem *) IO_ADDRESS(mem->start);
@@ -637,7 +693,7 @@ omap_i2c_probe(struct platform_device *p
 	}
 	r = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
 	dev_info(dev->dev, "bus %d rev%d.%d at %d kHz\n",
-		 pdev->id, r >> 4, r & 0xf, clock);
+		 pdev->id, r >> 4, r & 0xf, dev->speed);
 
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);

--- linux-omap/arch/arm/mach-omap2/clock.h	2007-03-13
13:03:43.000000000 -0600
+++ lin_for_hsi2c/arch/arm/mach-omap2/clock.h	2007-04-10
11:02:37.000000000 -0500
@@ -1761,7 +1761,8 @@ static struct clk i2c2_fck = {
 };
 
 static struct clk i2chs2_fck = {
-	.name		= "i2chs2_fck",
+	.name		= "i2chs_fck",
+	.id		= 2,
 	.parent		= &func_96m_ck,
 	.flags		= CLOCK_IN_OMAP243X,
 	.enable_reg	= (void __iomem *)&CM_FCLKEN2_CORE,
@@ -1790,7 +1791,8 @@ static struct clk i2c1_fck = {
 };
 
 static struct clk i2chs1_fck = {
-	.name		= "i2chs1_fck",
+	.name		= "i2chs_fck",
+	.id		= 1,
 	.parent		= &func_96m_ck,
 	.flags		= CLOCK_IN_OMAP243X,
 	.enable_reg	= (void __iomem *)&CM_FCLKEN2_CORE,

--- linux-omap/arch/arm/mach-omap2/devices.c	2007-04-02
02:29:57.000000000 -0500
+++ lin_for_hsi2c/arch/arm/mach-omap2/devices.c	2007-04-11
12:01:30.000000000 -0500
@@ -30,6 +30,12 @@
 #define OMAP2_I2C_BASE2		0x48072000
 #define OMAP2_I2C_INT2		57
 
+#if defined(CONFIG_MACH_OMAP_2430SDP)
+static u32 omap2_i2c2_clkrate	= 2600;
+#else
+static u32 omap2_i2c2_clkrate	= 100;
+#endif
+
 static struct resource i2c_resources2[] = {
 	{
 		.start		= OMAP2_I2C_BASE2,
@@ -47,6 +53,9 @@ static struct platform_device omap_i2c_d
 	.id             = 2,
 	.num_resources	= ARRAY_SIZE(i2c_resources2),
 	.resource	= i2c_resources2,
+	.dev		= {
+		.platform_data	= &omap2_i2c2_clkrate,
+	},
 };
 
 /* See also arch/arm/plat-omap/devices.c for first I2C on 24xx */

--- linux-omap/arch/arm/plat-omap/devices.c	2007-04-04
18:13:49.000000000 -0500
+++ lin_for_hsi2c/arch/arm/plat-omap/devices.c	2007-04-11
12:02:19.000000000 -0500
@@ -97,6 +97,12 @@ static inline void omap_init_dsp(void) {
 #define OMAP1_I2C_INT		INT_I2C
 #define OMAP2_I2C_INT1		56
 
+#if defined(CONFIG_MACH_OMAP_2430SDP)
+	static u32 omap2_i2c1_clkrate	= 400;
+#else
+	static u32 omap2_i2c1_clkrate	= 100;
+#endif
+
 static struct resource i2c_resources1[] = {
 	{
 		.start		= 0,
@@ -116,6 +122,9 @@ static struct platform_device omap_i2c_d
 	.id             = 1,
 	.num_resources	= ARRAY_SIZE(i2c_resources1),
 	.resource	= i2c_resources1,
+	.dev		= {
+		.platform_data	= &omap2_i2c1_clkrate,
+	},
 };
 
 /* See also arch/arm/mach-omap2/devices.c for second I2C on 24xx */

Re: [PATCH / RFC] Basic OMAP2430 High speed I2C & Speed handling (final)

[Tony Lindgren]

Hi,

* Syed Mohammed, Khasim <x0khasim@ti.com> [070412 01:21]:
> This patch moves I2C speed parameter (from module) to platform data.
> This is part of a discussion that Nishanth and I were discussing (also
> on
> lmsensors). 

OK, great.

> I have also added basic High Speed support based on I2C bus speed. 
> This patch is tested for high speed I2C (with TWL4030 Keypad) and works
> as expected.

Some comments below.

> --- linux-omap/drivers/i2c/busses/i2c-omap.c	2007-01-08
> 18:56:31.000000000 -0600
> +++ lin_for_hsi2c/drivers/i2c/busses/i2c-omap.c	2007-04-11
> 11:47:54.000000000 -0500
> @@ -3,7 +3,10 @@
>   *
>   * Copyright (C) 2003 MontaVista Software, Inc.
>   * Copyright (C) 2004 Texas Instruments.
> - *
> + *
> + * Updated to support High Speed I2C mode by
> + * Syed Mohammed Khasim <x0khasim@ti.com>
> + *
>   * Updated to work with multiple I2C interfaces on 24xx by
>   * Tony Lindgren <tony@atomide.com> and Imre Deak <imre.deak@nokia.com>
>   * Copyright (C) 2005 Nokia Corporation
> @@ -87,6 +90,7 @@

Let's rewrite the the header a bit to leave out change log type
comments, something like this would be better in general:

Copyright (C) 2003 MontaVista Software, Inc.
Copyright (C) 2004 - 2007 Texas Instruments
Copyright (C) 2005 Nokia Corporation
...

Written by:
...

This will avoid getting comments like "change log should be in git"
on LKML.


> @@ -154,17 +157,30 @@ static int omap_i2c_get_clocks(struct om
>  			return -ENODEV;
>  		}
>  	}
> -
> -	dev->fclk = clk_get(dev->dev, "i2c_fck");
> -	if (IS_ERR(dev->fclk)) {
> -		if (dev->iclk != NULL) {
> -			clk_put(dev->iclk);
> -			dev->iclk = NULL;
> +
> +	if (cpu_is_omap2430()) {
> +		dev->fclk = clk_get(dev->dev, "i2chs_fck");
> +		if (IS_ERR(dev->fclk)) {
> +			if (dev->iclk != NULL) {
> +				clk_put(dev->iclk);
> +				dev->iclk = NULL;
> +			}
> +			dev->fclk = NULL;
> +			return -ENODEV;
>  		}
> -		dev->fclk = NULL;
> -		return -ENODEV;
>  	}
> -
> +	else {
> +		dev->fclk = clk_get(dev->dev, "i2c_fck");
> +		if (IS_ERR(dev->fclk)) {
> +			if (dev->iclk != NULL) {
> +				clk_put(dev->iclk);
> +				dev->iclk = NULL;
> +			}
> +			dev->fclk = NULL;
> +			return -ENODEV;
> +		}
> +	}
> +
>  	return 0;
>  }

Is the i2c_fck needed on 2430? If not, we could just rename i2chs_fck
to i2c_fck in clock.h. Then just add a comment that i2c_fck is available
for 2430 too, but i2chs_fck is the preferred one to use.

Doing this would leave out the clock tinkering above.


> @@ -238,28 +256,59 @@ static int omap_i2c_init(struct omap_i2c
>  		if (fclk_rate > 12000000)
>  			psc = fclk_rate / 12000000;
>  	}
> +
> +	if (cpu_is_omap2430()) {
> +
> +		/* HS I2C controller should be operated at 19.2 Mhz */
> +		internal_clk = 19200;
> +		fclk_rate = clk_get_rate(dev->fclk) / 1000;
> +
> +		/* Compute prescaler divisor */
> +		psc = fclk_rate / internal_clk;
> +		psc = psc - 1;
> +
> +		/* If configured for High Speed */
> +		if (dev->speed > 400) {
> +			/* For first phase of HS mode */
> +			fsscll = internal_clk / (400 * 2) - 6;
> +			fssclh = internal_clk / (400 * 2) - 6;
> +
> +			/* For second phase of HS mode */
> +			hsscll = fclk_rate / (dev->speed * 2) - 6;
> +			hssclh = fclk_rate / (dev->speed * 2) - 6;
> +		}
> +		else {
> +			/* To handle F/S modes */
> +			fsscll = internal_clk / (dev->speed * 2) - 6;
> +			fssclh = internal_clk / (dev->speed * 2) - 6;
> +		}
> +		scll = (hsscll << OMAP_I2C_SCLL_HSSCLL) | fsscll;
> +		sclh = (hssclh << OMAP_I2C_SCLH_HSSCLH) | fssclh;
> +	}
> +	else {
> +		/* Program desired operating rate */
> +		fclk_rate /= (psc + 1) * 1000;
> +		if (psc > 2)
> +			psc = 2;
> +		scll = fclk_rate / (dev->speed * 2) - 7 + psc;
> +		sclh = fclk_rate / (dev->speed * 2) - 7 + psc;
> +	}

According to CodingStyle else above should be on the same line with
the brace. It's on separate lines in two places above.


>  	/* Setup clock prescaler to obtain approx 12MHz I2C module
> clock: */
>  	omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
> -
> -	/* Program desired operating rate */
> -	fclk_rate /= (psc + 1) * 1000;
> -	if (psc > 2)
> -		psc = 2;
> -
> -	omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG,
> -			   fclk_rate / (clock * 2) - 7 + psc);
> -	omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG,
> -			   fclk_rate / (clock * 2) - 7 + psc);
> -
> +
> +	/* SCL low and high time values */
> +	omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
> +	omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
> +
>  	/* Take the I2C module out of reset: */
>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>  
>  	/* Enable interrupts */
>  	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG,
> -			   (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
> -			    OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
> -			    OMAP_I2C_IE_AL));
> +				(OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
> +				OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
> +				OMAP_I2C_IE_AL));
>  	return 0;
>  }

Please separate formatting changes to a separate patch with no
functional changes. Maybe one follow-up patch for the getting rid
of the changelog in the header, and formatting changes?


> --- linux-omap/arch/arm/mach-omap2/devices.c	2007-04-02
> 02:29:57.000000000 -0500
> +++ lin_for_hsi2c/arch/arm/mach-omap2/devices.c	2007-04-11
> 12:01:30.000000000 -0500
> @@ -30,6 +30,12 @@
>  #define OMAP2_I2C_BASE2		0x48072000
>  #define OMAP2_I2C_INT2		57
>  
> +#if defined(CONFIG_MACH_OMAP_2430SDP)
> +static u32 omap2_i2c2_clkrate	= 2600;
> +#else
> +static u32 omap2_i2c2_clkrate	= 100;
> +#endif
> +
>  static struct resource i2c_resources2[] = {
>  	{
>  		.start		= OMAP2_I2C_BASE2,
> @@ -47,6 +53,9 @@ static struct platform_device omap_i2c_d
>  	.id             = 2,
>  	.num_resources	= ARRAY_SIZE(i2c_resources2),
>  	.resource	= i2c_resources2,
> +	.dev		= {
> +		.platform_data	= &omap2_i2c2_clkrate,
> +	},
>  };
>  
>  /* See also arch/arm/plat-omap/devices.c for first I2C on 24xx */

The if defined(CONFIG_MACH_OMAP_2430SDP) adds an unnecessary dependency
for not being possibly able to compile in multiple omaps. Let's rather
just do something with if (cpu_is_omap2430()) to set the
omap2_i2c2_clkrate.


> --- linux-omap/arch/arm/plat-omap/devices.c	2007-04-04
> 18:13:49.000000000 -0500
> +++ lin_for_hsi2c/arch/arm/plat-omap/devices.c	2007-04-11
> 12:02:19.000000000 -0500
> @@ -97,6 +97,12 @@ static inline void omap_init_dsp(void) {
>  #define OMAP1_I2C_INT		INT_I2C
>  #define OMAP2_I2C_INT1		56
>  
> +#if defined(CONFIG_MACH_OMAP_2430SDP)
> +	static u32 omap2_i2c1_clkrate	= 400;
> +#else
> +	static u32 omap2_i2c1_clkrate	= 100;
> +#endif
> +
>  static struct resource i2c_resources1[] = {
>  	{
>  		.start		= 0,
> @@ -116,6 +122,9 @@ static struct platform_device omap_i2c_d
>  	.id             = 1,
>  	.num_resources	= ARRAY_SIZE(i2c_resources1),
>  	.resource	= i2c_resources1,
> +	.dev		= {
> +		.platform_data	= &omap2_i2c1_clkrate,
> +	},
>  };
>  
>  /* See also arch/arm/mach-omap2/devices.c for second I2C on 24xx */

Same here with omap2_i2c1_clkrate.

Regards,

Tony

RE: [PATCH / RFC] Basic OMAP2430 High speed I2C & Speed handling (final)

[Syed Mohammed, Khasim]

Hi Tony,

Thanks for the comments, my response below.

>
>Let's rewrite the the header a bit to leave out change log type
>comments, something like this would be better in general:
>
>Copyright (C) 2003 MontaVista Software, Inc.
>Copyright (C) 2004 - 2007 Texas Instruments
>Copyright (C) 2005 Nokia Corporation
>...
>
>Written by:
>...
>
>This will avoid getting comments like "change log should be in git"
>on LKML.
>
>

OK ... Is the below one correct?

/*
 * TI OMAP I2C master mode driver
 *
 * Copyright (C) 2003 MontaVista Software, Inc.
 * Copyright (C) 2005 Nokia Corporation
 * Copyright (C) 2004 - 2007 Texas Instruments.
 *
 * Written by:
 * 	Tony Lindgren <tony@atomide.com> 
 * 	Imre Deak <imre.deak@nokia.com>
 * 	Juha Yrjölä <juha.yrjola@nokia.com>
 * 	Syed Khasim <x0khasim@ti.com>

>> +	else {
>> +		dev->fclk = clk_get(dev->dev, "i2c_fck");
>> +		if (IS_ERR(dev->fclk)) {
>> +			if (dev->iclk != NULL) {
>> +				clk_put(dev->iclk);
>> +				dev->iclk = NULL;
>> +			}
>> +			dev->fclk = NULL;
>> +			return -ENODEV;
>> +		}
>> +	}
>> +
>>  	return 0;
>>  }
>
>Is the i2c_fck needed on 2430? If not, we could just rename i2chs_fck
>to i2c_fck in clock.h. Then just add a comment that i2c_fck is available
>for 2430 too, but i2chs_fck is the preferred one to use.
>

i2c_fck is available for 2430 but i2chs_fck is preferred to use. I will add this comment in clock.h?

>> +	else {
>> +		/* Program desired operating rate */
>> +		fclk_rate /= (psc + 1) * 1000;
>> +		if (psc > 2)
>> +			psc = 2;
>> +		scll = fclk_rate / (dev->speed * 2) - 7 + psc;
>> +		sclh = fclk_rate / (dev->speed * 2) - 7 + psc;
>> +	}
>
>According to CodingStyle else above should be on the same line with
>the brace. It's on separate lines in two places above.

Mistake :( , Will correct them.

>>
>>  	/* Enable interrupts */
>>  	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG,
>> -			   (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
>> -			    OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
>> -			    OMAP_I2C_IE_AL));
>> +				(OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
>> +				OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
>> +				OMAP_I2C_IE_AL));
>>  	return 0;
>>  }
>
>Please separate formatting changes to a separate patch with no
>functional changes. Maybe one follow-up patch for the getting rid
>of the changelog in the header, and formatting changes?

OK, will send a patch with these two changes.

>> +#if defined(CONFIG_MACH_OMAP_2430SDP)
>> +static u32 omap2_i2c2_clkrate	= 2600;
>> +#else
>> +static u32 omap2_i2c2_clkrate	= 100;
>> +#endif
>> +
>
>The if defined(CONFIG_MACH_OMAP_2430SDP) adds an unnecessary dependency
>for not being possibly able to compile in multiple omaps. Let's rather
>just do something with if (cpu_is_omap2430()) to set the
>omap2_i2c2_clkrate.

This is a known problem that we were initially discussing. OMAP2430 has 2 I2C blocks, on a given board any one of these can be configured for HS and other can be for FS/S. So the speed basically depends on the board and not on the processor. We have to move this to board file, but changing all board files for adding this definition is another problem. Please suggest.

>>  /* See also arch/arm/mach-omap2/devices.c for second I2C on 24xx */
>
>Same here with omap2_i2c1_clkrate.
>

Same this as above.

Regards,
Khasim

Re: [PATCH / RFC] Basic OMAP2430 High speed I2C & Speed handling (final)

[Tony Lindgren]

* Syed Mohammed, Khasim <x0khasim@ti.com> [070417 21:08]:
> Hi Tony,
> 
> Thanks for the comments, my response below.
> 
> >
> >Let's rewrite the the header a bit to leave out change log type
> >comments, something like this would be better in general:
> >
> >Copyright (C) 2003 MontaVista Software, Inc.
> >Copyright (C) 2004 - 2007 Texas Instruments
> >Copyright (C) 2005 Nokia Corporation
> >...
> >
> >Written by:
> >...
> >
> >This will avoid getting comments like "change log should be in git"
> >on LKML.
> >
> >
> 
> OK ... Is the below one correct?
> 
> /*
>  * TI OMAP I2C master mode driver
>  *
>  * Copyright (C) 2003 MontaVista Software, Inc.
>  * Copyright (C) 2005 Nokia Corporation
>  * Copyright (C) 2004 - 2007 Texas Instruments.
>  *
>  * Written by:
>  * 	Tony Lindgren <tony@atomide.com> 
>  * 	Imre Deak <imre.deak@nokia.com>
>  * 	Juha Yrjölä <juha.yrjola@nokia.com>
>  * 	Syed Khasim <x0khasim@ti.com>

How about something like below as it was originally written by somebody
at MontaVista:

/*
 * TI OMAP I2C master mode driver
 *
 * Copyright (C) 2003 MontaVista Software, Inc.
 * Copyright (C) 2005 Nokia Corporation
 * Copyright (C) 2004 - 2007 Texas Instruments.
 *
 * Originally written by MontaVista Software, Inc.
 * Additional contributions by:
 *    Tony Lindgren <tony@atomide.com> 
 *    Imre Deak <imre.deak@nokia.com>
 *    Juha Yrjölä <juha.yrjola@nokia.com>
 *    Syed Khasim <x0khasim@ti.com>

> >> +	else {
> >> +		dev->fclk = clk_get(dev->dev, "i2c_fck");
> >> +		if (IS_ERR(dev->fclk)) {
> >> +			if (dev->iclk != NULL) {
> >> +				clk_put(dev->iclk);
> >> +				dev->iclk = NULL;
> >> +			}
> >> +			dev->fclk = NULL;
> >> +			return -ENODEV;
> >> +		}
> >> +	}
> >> +
> >>  	return 0;
> >>  }
> >
> >Is the i2c_fck needed on 2430? If not, we could just rename i2chs_fck
> >to i2c_fck in clock.h. Then just add a comment that i2c_fck is available
> >for 2430 too, but i2chs_fck is the preferred one to use.
> >
> 
> i2c_fck is available for 2430 but i2chs_fck is preferred to use. I will add this comment in clock.h?

Yes if i2chs_fck can be used in all cases. If it's board specific, it
should be set in the board-*.c code.

<snip>

> >> +#if defined(CONFIG_MACH_OMAP_2430SDP)
> >> +static u32 omap2_i2c2_clkrate	= 2600;
> >> +#else
> >> +static u32 omap2_i2c2_clkrate	= 100;
> >> +#endif
> >> +
> >
> >The if defined(CONFIG_MACH_OMAP_2430SDP) adds an unnecessary dependency
> >for not being possibly able to compile in multiple omaps. Let's rather
> >just do something with if (cpu_is_omap2430()) to set the
> >omap2_i2c2_clkrate.
> 
> This is a known problem that we were initially discussing. OMAP2430 has 2 I2C blocks, on a given board any one of these can be configured for HS and other can be for FS/S. So the speed basically depends on the board and not on the processor. We have to move this to board file, but changing all board files for adding this definition is another problem. Please suggest.
> 
> >>  /* See also arch/arm/mach-omap2/devices.c for second I2C on 24xx */
> >
> >Same here with omap2_i2c1_clkrate.
> >
> 
> Same this as above.

If the clock and rate can be board specific, then you can board specific
i2c_set_clock() function. There are some examples, see board-n800-usb.c
for example.

Regards,

Tony

RE: [PATCH / RFC] Basic OMAP2430 High speed I2C & Speed handling (final)

[Syed Mohammed, Khasim]

>Well you could keep it only for omap1 in device.c.

OK, I will do the following, please correct me if I am wrong.

- I will keep the I2C data structure in plat-omap/device.c under a OMAP1 macro.

- Will remove I2C platform_data structure from mach-omap2/device.c

- Add a platform_data structure and i2c_set_clock() function (similar to n800-usb.c) in 
	board-2430sdp.c
	board-h4.c
	board-n800.c
	board-apollon.c

- In I2C driver call the set_clock function to set the clock and speed.

Regards,
Khasim

>-----Original Message-----
>From: Tony Lindgren [mailto:tony@atomide.com]
>Sent: Tuesday, April 17, 2007 6:12 PM
>To: Syed Mohammed, Khasim
>Subject: Re: [PATCH / RFC] Basic OMAP2430 High speed I2C & Speed handling
>(final)
>
>* Syed Mohammed, Khasim <x0khasim@ti.com> [070417 23:09]:
>>
>> >If the clock and rate can be board specific, then you can board specific
>> >i2c_set_clock() function. There are some examples, see board-n800-usb.c
>> >for example.
>> >
>>
>> But what should happen to the definitions in device.c? If I remove them
>it might break other boards. I am more worried about omap1s.
>>
>> Should I still keep device.c one?
>
>Well you could keep it only for omap1 in device.c.
>
>Tony

Re: [PATCH / RFC] Basic OMAP2430 High speed I2C & Speed handling (final)

[Tony Lindgren]

* Syed Mohammed, Khasim <x0khasim@ti.com> [070417 23:20]:
> >Well you could keep it only for omap1 in device.c.
> 
> OK, I will do the following, please correct me if I am wrong.
> 
> - I will keep the I2C data structure in plat-omap/device.c under a OMAP1 macro.
> 
> - Will remove I2C platform_data structure from mach-omap2/device.c
> 
> - Add a platform_data structure and i2c_set_clock() function (similar to n800-usb.c) in 
> 	board-2430sdp.c
> 	board-h4.c
> 	board-n800.c
> 	board-apollon.c
> 
> - In I2C driver call the set_clock function to set the clock and speed.

If it's only an issue with 2430, you could still do the I2C init in
devices.c for 2420.

Tony