PXA3xx: Add support for power i2c bus

This patch adds support for power i2c bus on PXA3xx processor.

Signed-off-by Mike Rapoport <mike-UTxiZqZC01RS1MOuV/[email protected]>

Quoted 4 lines · Mike Rapoport

This patch adds support for power i2c bus on PXA3xx processor.

Signed-off-by Mike Rapoport <mike-UTxiZqZC01RS1MOuV/[email protected]>

I'd like the i2c driver to be converted to ioremap() and {read,write}l() first before this can happen.

pxa27x_set_i2c_power_info()

and

pxa3xx_set_i2c_power_info()

are OK to co-exist, no need for a unified version. IOW, I'd prefer the previous invocations of pxa_set_i2c_power_info() to be renamed "pxa27x_set_i2c_power_info()"

The caller is supposed to know which to call.

arch/arm/mach-pxa/generic.h +0 -1

@@ -982,10 +992,13 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
 		 i2c->adap.nr);
 
-	i2c->clk = clk_get(&dev->dev, "I2CCLK");

arch/arm/mach-pxa/generic.h line 12

Quoted 10 lines · Mike Rapoport

arch/arm/mach-pxa/generic.h +1 -1

index 041c048..13a786d 100644
@@ -9,9 +9,10 @@
  * published by the Free Software Foundation.
  */
 
-typedef int (*set_wake_t)(unsigned int, unsigned int);
+typedef int (*set_wake_t) (unsigned int, unsigned int);

Undoes perfectly good formatting.

Quoted 4 lines

arch/arm/mach-pxa/generic.h +0 -0

@@ -982,10 +992,13 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
 		 i2c->adap.nr);
 

arch/arm/mach-pxa/generic.h static int i2c_pxa_probe(struct platform_device *dev) · line 1001

Quoted 11 lines

arch/arm/mach-pxa/generic.h +7 -4

-	i2c->clk = clk_get(&dev->dev, "I2CCLK");
-	if (IS_ERR(i2c->clk)) {
-		ret = PTR_ERR(i2c->clk);
-		goto eclk;
+	/* PXA3xx has power I2C clock always on */
+	if (!(cpu_is_pxa3xx() && i2c->adap.nr == 1)) {
+		i2c->clk = clk_get(&dev->dev, "I2CCLK");
+		if (IS_ERR(i2c->clk)) {
+			ret = PTR_ERR(i2c->clk);
+			goto eclk;
+		}

Umm, no. It would be far better to provide a dummy clock rather than mess drivers up with this and lots of other conditional stuff.

Quoted 18 lines · Russell King - ARM Linux

arch/arm/mach-pxa/generic.h line 12

Quoted 10 lines · Mike Rapoport

arch/arm/mach-pxa/generic.h +1 -1

index 041c048..13a786d 100644
@@ -9,9 +9,10 @@
  * published by the Free Software Foundation.
  */
 
-typedef int (*set_wake_t)(unsigned int, unsigned int);
+typedef int (*set_wake_t) (unsigned int, unsigned int);

Undoes perfectly good formatting.

Quoted 4 lines

arch/arm/mach-pxa/generic.h +0 -0

@@ -982,10 +992,13 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
 		 i2c->adap.nr);
 

Quoted 14 lines

arch/arm/mach-pxa/generic.h static int i2c_pxa_probe(struct platform_device *dev) · line 1001

Quoted 11 lines

arch/arm/mach-pxa/generic.h +7 -4

-	i2c->clk = clk_get(&dev->dev, "I2CCLK");
-	if (IS_ERR(i2c->clk)) {
-		ret = PTR_ERR(i2c->clk);
-		goto eclk;
+	/* PXA3xx has power I2C clock always on */
+	if (!(cpu_is_pxa3xx() && i2c->adap.nr == 1)) {
+		i2c->clk = clk_get(&dev->dev, "I2CCLK");
+		if (IS_ERR(i2c->clk)) {
+			ret = PTR_ERR(i2c->clk);
+			goto eclk;
+		}

Umm, no. It would be far better to provide a dummy clock rather than mess drivers up with this and lots of other conditional stuff.

Would that be better?

Signed-off-by Mike Rapoport <[email protected]>

 arch/arm/mach-pxa/devices.h          |    1 +
 arch/arm/mach-pxa/include/mach/i2c.h |    6 ++++-
 arch/arm/mach-pxa/pxa27x.c           |    2 +-
 arch/arm/mach-pxa/pxa3xx.c           |   45 ++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-pxa.c         |   20 +++++++++++----
 5 files changed, 67 insertions(+), 7 deletions(-)

arch/arm/mach-pxa/devices.h +1 -0

index 887c738..bb04af4 100644
@@ -32,5 +32,6 @@ extern struct platform_device pxa27x_device_pwm0;
 extern struct platform_device pxa27x_device_pwm1;
 
 extern struct platform_device pxa3xx_device_nand;
+extern struct platform_device pxa3xx_device_i2c_power;
 
 void __init pxa_register_device(struct platform_device *dev, void *data);

arch/arm/mach-pxa/include/mach/i2c.h +5 -1

index 80596b0..897e717 100644
@@ -71,7 +71,11 @@ struct i2c_pxa_platform_data {
 extern void pxa_set_i2c_info(struct i2c_pxa_platform_data *info);
 
 #ifdef CONFIG_PXA27x
-extern void pxa_set_i2c_power_info(struct i2c_pxa_platform_data *info);
+extern void pxa27x_set_i2c_power_info(struct i2c_pxa_platform_data *info);
+#endif
+
+#ifdef CONFIG_PXA3xx
+extern void pxa3xx_set_i2c_power_info(struct i2c_pxa_platform_data *info);
 #endif
 
 #endif

arch/arm/mach-pxa/pxa27x.c +1 -1

index f9f6a9c..c33cf6a 100644
@@ -349,7 +349,7 @@ struct platform_device pxa27x_device_i2c_power = {
 	.num_resources	= ARRAY_SIZE(i2c_power_resources),
 };
 
-void __init pxa_set_i2c_power_info(struct i2c_pxa_platform_data *info)
+void __init pxa27x_set_i2c_power_info(struct i2c_pxa_platform_data *info)
 {
 	local_irq_disable();
 	PCFR |= PCFR_PI2CEN;

arch/arm/mach-pxa/pxa3xx.c +45 -0

index 03cbc38..b3cd5d0 100644
@@ -203,6 +203,19 @@ static const struct clkops clk_pout_ops = {
 	.disable	= clk_pout_disable,
 };
 
+static void clk_dummy_enable(struct clk *clk)
+{
+}
+
+static void clk_dummy_disable(struct clk *clk)
+{
+}
+
+static const struct clkops clk_dummy_ops = {
+	.enable		= clk_dummy_enable,
+	.disable	= clk_dummy_disable,
+};
+
 static struct clk pxa3xx_clks[] = {
 	{
 		.name           = "CLK_POUT",

@@ -211,6 +224,13 @@ static struct clk pxa3xx_clks[] = {
 		.delay          = 70,
 	},
 
+	/* Power I2C clock is always on */
+	{
+		.name		= "I2CCLK",
+		.ops		= &clk_dummy_ops,
+		.dev		= &pxa3xx_device_i2c_power.dev,
+	},
+
 	PXA3xx_CK("LCDCLK",  LCD,    &clk_pxa3xx_hsio_ops, &pxa_device_fb.dev),
 	PXA3xx_CK("CAMCLK",  CAMERA, &clk_pxa3xx_hsio_ops, NULL),
 	PXA3xx_CK("AC97CLK", AC97,   &clk_pxa3xx_ac97_ops, NULL),

@@ -509,6 +529,30 @@ void __init pxa3xx_init_irq(void)
  * device registration specific to PXA3xx.
  */
 
+static struct resource i2c_power_resources[] = {
+	{
+		.start  = 0x40f500c0,
+		.end    = 0x40f500d3,
+		.flags	= IORESOURCE_MEM,
+	}, {
+		.start	= IRQ_PWRI2C,
+		.end	= IRQ_PWRI2C,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+struct platform_device pxa3xx_device_i2c_power = {
+	.name		= "pxa2xx-i2c",
+	.id		= 1,
+	.resource	= i2c_power_resources,
+	.num_resources	= ARRAY_SIZE(i2c_power_resources),
+};
+
+void __init pxa3xx_set_i2c_power_info(struct i2c_pxa_platform_data *info)
+{
+	pxa3xx_device_i2c_power.dev.platform_data = info;
+}
+
 static struct platform_device *devices[] __initdata = {
 /*	&pxa_device_udc,	The UDC driver is PXA25x only */
 	&pxa_device_ffuart,

@@ -522,6 +566,7 @@ static struct platform_device *devices[] __initdata = {
 	&pxa3xx_device_ssp4,
 	&pxa27x_device_pwm0,
 	&pxa27x_device_pwm1,
+	&pxa3xx_device_i2c_power,
 };
 
 static struct sys_device pxa3xx_sysdev[] = {

drivers/i2c/busses/i2c-pxa.c +5 -5

index 44d8384..981bb07 100644
@@ -68,11 +68,21 @@ struct pxa_i2c {
 	int			use_pio;
 };
 
-#define _IBMR(i2c)	((i2c)->reg_base + 0)
-#define _IDBR(i2c)	((i2c)->reg_base + 8)
-#define _ICR(i2c)	((i2c)->reg_base + 0x10)
-#define _ISR(i2c)	((i2c)->reg_base + 0x18)
-#define _ISAR(i2c)	((i2c)->reg_base + 0x20)
+#define _IBMR(i2c)	\
+	((i2c)->reg_base + \
+	 ((cpu_is_pxa3xx() && ((i2c)->adap.nr == 1)) ? 0x0 : 0x0))
+#define _IDBR(i2c)	\
+	((i2c)->reg_base + \

On Thu, Aug 14, 2008 at 02:38:09PM +0300, Mike Rapoport wrote:

Not very nice, how about making the i2c structure have pointers to each of the registers which can be setup at probe time. Or even just have a multiplier in the structure, as these all look to be reg/2 in the case of pxa3xx and adap.nr == 1...

Note, adap.nr shouldn't really be used to differentiate, use the platform device number

(quoted diff) +2 -0

+#define _IDBR(i2c)	((i2c)->reg_base + (0x4 << (i2c)->reg_shift))
+#define _ICR(i2c)	((i2c)->reg_base + (0x8 << (i2c)->reg_shift))

etc.

then do in the probe:

	i2c->reg_shift = (cpu_is_pxa3xx() && <is_power_i2c>) ? 0 : 1;

Quoted 14 lines · Ben Dooks

On Thu, Aug 14, 2008 at 02:38:09PM +0300, Mike Rapoport wrote:

Not very nice, how about making the i2c structure have pointers to each of the registers which can be setup at probe time. Or even just have a multiplier in the structure, as these all look to be reg/2 in the case of pxa3xx and adap.nr == 1...

Note, adap.nr shouldn't really be used to differentiate, use the platform device number

drivers/i2c/busses/i2c-pxa.c +2 -0

+#define _IDBR(i2c)   ((i2c)->reg_base + (0x4 << (i2c)->reg_shift))
+#define _ICR(i2c)    ((i2c)->reg_base + (0x8 << (i2c)->reg_shift))

etc.

Quoted 4 lines

then do in the probe:

      i2c->reg_shift = (cpu_is_pxa3xx() && <is_power_i2c>) ? 0 : 1;

This is really much nicer. The fixed patch follows:

Signed-off-by Mike Rapoport <mike-UTxiZqZC01RS1MOuV/[email protected]>

This is really much nicer. The fixed patch follows:

Signed-off-by Mike Rapoport <[email protected]>

 arch/arm/mach-pxa/devices.h          |    1 +
 arch/arm/mach-pxa/include/mach/i2c.h |    6 ++++-
 arch/arm/mach-pxa/pxa27x.c           |    2 +-
 arch/arm/mach-pxa/pxa3xx.c           |   45 ++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-pxa.c         |   12 +++++----
 5 files changed, 59 insertions(+), 7 deletions(-)