PCA9564-platform driver

This makes further patches more readable.

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/algos/i2c-algo-pca.c |   42 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

It now conatins a private data field and more per-instance data. The reset

routine has been moved to the adapters. It also supports add_numbered_adapter.

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/algos/i2c-algo-pca.c |   94 ++++++++++++++++++++++-----------------
 drivers/i2c/algos/i2c-algo-pca.h |    9 ---
 include/linux/i2c-algo-pca.h     |   25 +++++++---
 3 files changed, 73 insertions(+), 55 deletions(-)

Index: linux-playground/include/linux/i2c-algo-pca.h
===================================================================

include/linux/i2c-algo-pca.h +9 -4

@@ -1,14 +1,27 @@
 #ifndef _LINUX_I2C_ALGO_PCA_H
 #define _LINUX_I2C_ALGO_PCA_H
 
+#define I2C_PCA_CON_330kHz	0x00
+#define I2C_PCA_CON_288kHz	0x01
+#define I2C_PCA_CON_217kHz	0x02
+#define I2C_PCA_CON_146kHz	0x03
+#define I2C_PCA_CON_88kHz	0x04
+#define I2C_PCA_CON_59kHz	0x05
+#define I2C_PCA_CON_44kHz	0x06
+#define I2C_PCA_CON_36kHz	0x07
+
 struct i2c_algo_pca_data {
-	int  (*get_own)			(struct i2c_algo_pca_data *adap); /* Obtain own address */
-	int  (*get_clock)		(struct i2c_algo_pca_data *adap);
-	void (*write_byte)		(struct i2c_algo_pca_data *adap, int reg, int val);
-	int  (*read_byte)		(struct i2c_algo_pca_data *adap, int reg);

include/linux/i2c-algo-pca.h +1 -0

+int i2c_pca_add_numbered_adapter(struct i2c_adapter *);
 
 #endif /* _LINUX_I2C_ALGO_PCA_H */

Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.h
===================================================================

drivers/i2c/algos/i2c-algo-pca.h +0 -0

@@ -14,13 +14,4 @@
 #define I2C_PCA_CON_SI		0x08 /* Serial Interrupt */
 #define I2C_PCA_CON_CR		0x07 /* Clock Rate (MASK) */
 

Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.c
===================================================================

drivers/i2c/algos/i2c-algo-pca.c +14 -18

  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by

@@ -21,7 +27,6 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/delay.h>
-#include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>

@@ -36,15 +41,16 @@
 
 static int i2c_debug;
 
-#define pca_outw(adap, reg, val) adap->write_byte(adap, reg, val)
-#define pca_inw(adap, reg) adap->read_byte(adap, reg)
+#define pca_outw(adap, reg, val) adap->write_byte(adap->data, reg, val)
+#define pca_inw(adap, reg) adap->read_byte(adap->data, reg)
 
 #define pca_status(adap) pca_inw(adap, I2C_PCA_STA)
-#define pca_clock(adap) adap->get_clock(adap)
-#define pca_own(adap) adap->get_own(adap)
+#define pca_clock(adap) adap->i2c_clock
+#define pca_own(adap) adap->my_slave_addr
 #define pca_set_con(adap, val) pca_outw(adap, I2C_PCA_CON, val)
 #define pca_get_con(adap) pca_inw(adap, I2C_PCA_CON)
-#define pca_wait(adap) adap->wait_for_interrupt(adap)
+#define pca_wait(adap) adap->wait_for_completion(adap->data)
+#define pca_reset(adap) adap->reset_chip(adap->data)
 
 /*
  * Generate a start condition on the i2c bus.

@@ -168,15 +174,6 @@
 	pca_wait(adap);
 }
 
-/*
- * Reset the i2c bus / SIO
- */
-static void pca_reset(struct i2c_algo_pca_data *adap)
-{
-	/* apparently only an external reset will do it. not a lot can be done */
-	printk(KERN_ERR DRIVER ": Haven't figured out how to do a reset yet\n");
-}
-
 static int pca_xfer(struct i2c_adapter *i2c_adap,
                     struct i2c_msg *msgs,
                     int num)

@@ -187,7 +184,7 @@
 	int numbytes = 0;
 	int state;
 	int ret;
-	int timeout = 100;
+	int timeout = adap->timeout;
 
 	while ((state = pca_status(adap)) != 0xf8 && timeout--) {
 		msleep(10);

@@ -317,7 +314,7 @@
 			pca_reset(adap);
 			goto out;
 		default:
-			printk(KERN_ERR DRIVER ": unhandled SIO state 0x%02x\n", state);
+			dev_err(&i2c_adap->dev, "unhandled SIO state 0x%02x\n", state);
 			break;
 		}
 

@@ -337,51 +334,68 @@
         return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
 }
 
-static int pca_init(struct i2c_algo_pca_data *adap)
+static const struct i2c_algorithm pca_algo = {
+	.master_xfer	= pca_xfer,
+	.functionality	= pca_func,
+};
+
+static int pca_init(struct i2c_adapter *adap)

drivers/i2c/algos/i2c-algo-pca.c +0 -3

-	own = pca_own(adap);
-	clock = pca_clock(adap);
-	DEB1(KERN_INFO DRIVER ": own address is %#04x\n", own);

drivers/i2c/algos/i2c-algo-pca.c +19 -16

+
+	pca_outw(pca_data, I2C_PCA_ADR, own << 1);
+
+	pca_set_con(pca_data, I2C_PCA_CON_ENSIO | clock);
 	udelay(500); /* 500 us for oscilator to stabilise */
 
 	return 0;
 }
 
-static const struct i2c_algorithm pca_algo = {
-	.master_xfer	= pca_xfer,
-	.functionality	= pca_func,
-};
-
 /*
  * registering functions to load algorithms at runtime
  */
-int i2c_pca_add_bus(struct i2c_adapter *adap)
+int i2c_pca_add_adapter(struct i2c_adapter *adap)
 {
-	struct i2c_algo_pca_data *pca_adap = adap->algo_data;
 	int rval;
 
-	/* register new adapter to i2c module... */
-	adap->algo = &pca_algo;
+	rval = pca_init(adap);
+	if (rval)
+		return rval;
 
-	adap->timeout = 100;		/* default values, should	*/
-	adap->retries = 3;		/* be replaced by defines	*/
+	return i2c_add_adapter(adap);
+}
+EXPORT_SYMBOL(i2c_pca_add_adapter);
 
-	if ((rval = pca_init(pca_adap)))
-		return rval;
+int i2c_pca_add_numbered_adapter(struct i2c_adapter *adap)
+{
+	int rval;
 
-	rval = i2c_add_adapter(adap);
+	rval = pca_init(adap);
+	if (rval)
+		return rval;
 
-	return rval;
+	return i2c_add_numbered_adapter(adap);
 }
-EXPORT_SYMBOL(i2c_pca_add_bus);
+EXPORT_SYMBOL(i2c_pca_add_numbered_adapter);
 
 MODULE_AUTHOR("Ian Campbell <[email protected]>");
 MODULE_DESCRIPTION("I2C-Bus PCA9564 algorithm");
 

I would like a more detailed changelog entry. Please explain all the changes you did and why you had to do them. If might be obvious to you, but it may not be to others (including me.)

This patch break the i2c-pca-isa driver, which is not OK. Your patchset should be such that everything still builds and work after each incremental patch. In practice, this means that patches 2/6 and 5/6 should be merged. Except for white-space cleanups which would rather belong to patch 1/6.

Quoted 3 lines

Index: linux-playground/include/linux/i2c-algo-pca.h
===================================================================

Quoted 9 lines

struct i2c_algo_pca_data {

include/linux/i2c-algo-pca.h +3 -5

-	int  (*get_own)			(struct i2c_algo_pca_data *adap); /* Obtain own address */
-	int  (*get_clock)		(struct i2c_algo_pca_data *adap);
-	void (*write_byte)		(struct i2c_algo_pca_data *adap, int reg, int val);
-	int  (*read_byte)		(struct i2c_algo_pca_data *adap, int reg);
-	int  (*wait_for_interrupt)	(struct i2c_algo_pca_data *adap);
+	void 				*data;	/* private low level data */
+	void (*write_byte)		(void *data, int reg, int val);
+	int  (*read_byte)		(void *data, int reg);

include/linux/i2c-algo-pca.h line 22

Quoted 6 lines

include/linux/i2c-algo-pca.h +5 -0

+	int  (*wait_for_completion)	(void *data);
+	void (*reset_chip)		(void *data);
+	unsigned int			my_slave_addr;
+	unsigned int			i2c_clock;
+	unsigned int			timeout;
 };

I do not understand why you change struct i2c_algo_pca_data *adap to void *data in these prototypes?

struct i2c_adapter already has a timeout field, so why define your own?

include/linux/i2c-algo-pca.h line 24

Quoted 2 lines

include/linux/i2c-algo-pca.h +1 -1

-int i2c_pca_add_bus(struct i2c_adapter *);
+int i2c_pca_add_adapter(struct i2c_adapter *);

Why change this? All other algorithm drivers have i2c_foo_add_bus().

Quoted 8 lines

Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.h
===================================================================

drivers/i2c/algos/i2c-algo-pca.h +0 -0

@@ -14,13 +14,4 @@
 #define I2C_PCA_CON_SI		0x08 /* Serial Interrupt */
 #define I2C_PCA_CON_CR		0x07 /* Clock Rate (MASK) */
 

drivers/i2c/algos/i2c-algo-pca.h line 17

Quoted 10 lines

drivers/i2c/algos/i2c-algo-pca.h +0 -9

-#define I2C_PCA_CON_330kHz	0x00
-#define I2C_PCA_CON_288kHz	0x01
-#define I2C_PCA_CON_217kHz	0x02
-#define I2C_PCA_CON_146kHz	0x03
-#define I2C_PCA_CON_88kHz	0x04
-#define I2C_PCA_CON_59kHz	0x05
-#define I2C_PCA_CON_44kHz	0x06
-#define I2C_PCA_CON_36kHz	0x07
-
 #endif /* I2C_PCA9564_H */

Maybe you could get rid of this header file altogether? It never made much sense to me. Symbols internal to i2c-algo-pca do not need to be in a header file. Symbols needed by bus drivers or platform code should be in a header file under include/linux, not drivers/i2c/algos.

History doesn't belong there, we have git for that.

drivers/i2c/algos/i2c-algo-pca.h

Quoted 5 lines

drivers/i2c/algos/i2c-algo-pca.h +5 -0

+	if (pca_data->i2c_clock > 7) {
+		dev_warn(&adap->dev, "Invalid I2C clock speed selected. Trying default.\n");
+		pca_data->i2c_clock = I2C_PCA_CON_59kHz;
+	}
+	/* No need to check my_slave_addr. driver can't handle slave mode */

Why define it at all then?

drivers/i2c/algos/i2c-algo-pca.h

Quoted 1 line

drivers/i2c/algos/i2c-algo-pca.h +1 -0

+	adap->retries = 1;

Please don't set retries, the driver doesn't actually implement a retry mechanism, and this structure field will be dropped soon in favor of a different approach anyway.

drivers/i2c/algos/i2c-algo-pca.h

Quoted 2 lines

drivers/i2c/algos/i2c-algo-pca.h +2 -0

+	DEB1(KERN_INFO "%s: Own address is %#04x\n", adap->name, own);
+	DEB1(KERN_INFO "%s: Clock freqeuncy is %dkHz\n", adap->name, freqs[clock]);

Typo: frequency.

This patch corrects some minor typos. The term "below" in these paragraphs might also be replaced by "later" or something similar?

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/algos/Kconfig |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-playground/drivers/i2c/algos/Kconfig
===================================================================

drivers/i2c/algos/Kconfig +3 -3

@@ -9,7 +9,7 @@
 	help
 	  This allows you to use a range of I2C adapters called bit-banging
 	  adapters.  Say Y if you own an I2C adapter belonging to this class
-	  and then say Y to the specific driver for you adapter below.
+	  and then say Y to the specific driver for your adapter below.
 
 	  This support is also available as a module.  If so, the module 
 	  will be called i2c-algo-bit.

@@ -19,7 +19,7 @@
 	help
 	  This allows you to use a range of I2C adapters called PCF adapters.
 	  Say Y if you own an I2C adapter belonging to this class and then say
-	  Y to the specific driver for you adapter below.
+	  Y to the specific driver for your adapter below.
 
 	  This support is also available as a module.  If so, the module 
 	  will be called i2c-algo-pcf.

@@ -29,7 +29,7 @@
 	help
 	  This allows you to use a range of I2C adapters called PCA adapters.
 	  Say Y if you own an I2C adapter belonging to this class and then say
-	  Y to the specific driver for you adapter below.
+	  Y to the specific driver for your adapter below.
 
 	  This support is also available as a module.  If so, the module 
 	  will be called i2c-algo-pca.

Hi Wolfram,

Quoted 2 lines · On Mon, 21 Jan 2008 15:20:13 +0100, [email protected].…

This patch corrects some minor typos. The term "below" in these paragraphs might also be replaced by "later" or something similar?

You're right, this is a remnant from the times where all the i2c options were in a single menu. Nowadays, 'under "I2C Hardware Bus support"' would be more appropriate. Or... nothing at all. After all, the i2c "algorithm" modules are selected automatically whenever a driver that relies on them is selected. They are visible in the menu only so that one can enable them for an hypothetical out-of-tree driver
- but I doubt that this is terribly useful. So a 3rd option would be to simply remove these items from the configuration menu.

Quoted 11 lines · Jean Delvare

Quoted 2 lines · On Mon, 21 Jan 2008 15:20:13 +0100, [email protected].…

This patch corrects some minor typos. The term "below" in these paragraphs might also be replaced by "later" or something similar?

You're right, this is a remnant from the times where all the i2c options were in a single menu. Nowadays, 'under "I2C Hardware Bus support"' would be more appropriate. Or... nothing at all. After all, the i2c "algorithm" modules are selected automatically whenever a driver that relies on them is selected. They are visible in the menu only so that one can enable them for an hypothetical out-of-tree driver
- but I doubt that this is terribly useful. So a 3rd option would be to simply remove these items from the configuration menu.

I support the idea of removing them. If there is an out-of-tree driver, it will find a way to set the algorithm from outside. Shall I make such a patch, if there are no further objections?

This is untested, due to no hardware!

Signed-off-by Wolfram Sang <[email protected]>

---
 drivers/i2c/busses/i2c-pca-isa.c |   52 +++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

Index: linux-playground/drivers/i2c/busses/i2c-pca-isa.c
===================================================================

drivers/i2c/busses/i2c-pca-isa.c +0 -1

  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by

@@ -22,7 +28,6 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/delay.h>
-#include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/wait.h>

drivers/i2c/busses/i2c-pca-isa.c +1 -12

 static unsigned long base   = 0x330;
 static int irq 	  = 10;

@@ -52,18 +55,7 @@
 
 static wait_queue_head_t pca_wait;
 
-static int pca_isa_getown(struct i2c_algo_pca_data *adap)
-{
-	return (own);
-}
-
-static int pca_isa_getclock(struct i2c_algo_pca_data *adap)
-{
-	return (clock);
-}
-
-static void
-pca_isa_writebyte(struct i2c_algo_pca_data *adap, int reg, int val)
+static void pca_isa_writebyte(void *pd, int reg, int val)
 {
 #ifdef DEBUG_IO
 	static char *names[] = { "T/O", "DAT", "ADR", "CON" };

@@ -72,20 +64,19 @@
 	outb(val, base+reg);
 }

drivers/i2c/busses/i2c-pca-isa.c +1 -1

 		printk("*** read  %s => %#04x\n", names[reg], res);
 	}
 #endif
 	return res;
 }
 
-static int pca_isa_waitforinterrupt(struct i2c_algo_pca_data *adap)
+static int pca_isa_waitforcompletion(void *pd)
 {
 	int ret = 0;
 

drivers/i2c/busses/i2c-pca-isa.c +5 -0

 			udelay(100);
 	}
 	return ret;
 }
 
+static void pca_isa_resetchip(void *pd)
+{
+	/* apparently only an external reset will do it. not a lot can be done */
+	printk(KERN_ERR DRIVER ": Haven't figured out how to do a reset yet\n");
+}

drivers/i2c/busses/i2c-pca-isa.c +5 -1

 	.write_byte		= pca_isa_writebyte,
 	.read_byte		= pca_isa_readbyte,
-	.wait_for_interrupt	= pca_isa_waitforinterrupt,
+	.wait_for_competion	= pca_isa_waitforcompletion,
+	.reset_chip		= pca_isa_resetchip,
+	.my_slave_addr		= own,
+	.i2c_clock		= clock,
+	.timeout		= 100,
 };
 
 static struct i2c_adapter pca_isa_ops = {

drivers/i2c/busses/i2c-pca-isa.c +0 -0

 		disable_irq(irq);
 		free_irq(irq, &pca_isa_ops);
 	}

Hi Wolfram,

Quoted 1 line · On Mon, 21 Jan 2008 15:20:15 +0100, [email protected].…

This is untested, due to no hardware!

Did you at least try to build it? I guess not, because it fails here:

CC [M] drivers/i2c/busses/i2c-pca-isa.o
drivers/i2c/busses/i2c-pca-isa.c: In function `pca_isa_waitforcompletion':
drivers/i2c/busses/i2c-pca-isa.c:84: error: `adap' undeclared (first use in this function) drivers/i2c/busses/i2c-pca-isa.c:84: error: (Each undeclared identifier is reported only once drivers/i2c/busses/i2c-pca-isa.c:84: error: for each function it appears in.) drivers/i2c/busses/i2c-pca-isa.c: In function `pca_isa_resetchip':
drivers/i2c/busses/i2c-pca-isa.c:96: error: syntax error before "DRIVER" drivers/i2c/busses/i2c-pca-isa.c: At top level:
drivers/i2c/busses/i2c-pca-isa.c:108: error: unknown field wait_for_competion' specified in initializer drivers/i2c/busses/i2c-pca-isa.c:110: error: initializer element is not constant drivers/i2c/busses/i2c-pca-isa.c:110: error: (near initialization for pca_isa_data.my_slave_addr') drivers/i2c/busses/i2c-pca-isa.c:111: error: initializer element is not constant drivers/i2c/busses/i2c-pca-isa.c:111: error: (near initialization for pca_isa_data.i2c_clock') drivers/i2c/busses/i2c-pca-isa.c: In function pca_isa_probe':
drivers/i2c/busses/i2c-pca-isa.c:140: error: implicit declaration of function `i2c_pca_add_bus' make[3]: *** [drivers/i2c/busses/i2c-pca-isa.o] Error 1
make[2]: *** [drivers/i2c/busses] Error 2
make[1]: *** [drivers/i2c] Error 2
make: *** [drivers] Error 2

Please fix.

No history in drivers.

drivers/i2c/busses/i2c-pca-isa.c line 39

Quoted 4 lines

drivers/i2c/busses/i2c-pca-isa.c +2 -2

-#include <asm/io.h>
-#include <asm/irq.h>
+#include <linux/io.h>
+#include <linux/irq.h>

Why? Many drivers include <asm/io.h> and <asm/irq.h>, it looks OK to me.

Quoted 2 lines

#undef DEBUG_IO
-//#define DEBUG_IO

While you're here, the line before could be discarded as well.

drivers/i2c/busses/i2c-pca-isa.c line 105

Quoted 4 lines

static struct i2c_algo_pca_data pca_isa_data = {

drivers/i2c/busses/i2c-pca-isa.c +1 -2

-	.get_own		= pca_isa_getown,
-	.get_clock		= pca_isa_getclock,
+	.data			= NULL,

No need to initialize to NULL, the compiler does it for you. On a side note though, I fail to see how this could work, given that you changed the callbacks so that you pass this private data pointer to them instead of a pointer to struct i2c_algo_pca_data. This probably needs some more thinking.

drivers/i2c/busses/i2c-pca-isa.c line 160

Quoted 2 lines

drivers/i2c/busses/i2c-pca-isa.c +1 -1

-	if (irq > 0) {
+	if (irq > -1) {

This deserves an explanation... In the light of previous discussions on the i2c list, I'd rather expect comparisons with NO_IRQ.

While working on the PCA9564-platform driver, I sometimes had a glimpse at the pxa-driver. I found some suspicious places, and this patch contains my suggestions. Note: They are not tested, due to no hardware.

-----Original Message-----
From: Jean Delvare [mailto:[email protected]]
Sent: Wednesday, February 13, 2008 4:59 PM
To: Wolfram Sang
Cc: [email protected]; Mike Rapoport; Eric Miao
Subject: Re: [i2c] [patch 6/6] Minor fixes for pxa-driver

Hi Wolfram,

Quoted 1 line · On Mon, 21 Jan 2008 15:20:16 +0100, [email protected].…

While working on the PCA9564-platform driver, I sometimes had a glimpse at

the

Quoted 2 lines

pxa-driver. I found some suspicious places, and this patch contains my suggestions. Note: They are not tested, due to no hardware.

Looks good. I've added a couple more fixes, resulting patch is below. As I don't have hardware for testing either, I would appreciate if someone with a PXA board could test it and confirm that it causes no regression. Mike? Eric? I plan to send this patch to Linus for 2.6.25 (i.e. quickly.)

From: Wolfram Sang <[email protected]>
Subject: i2c-pxa: Misc fixes

While working on the PCA9564-platform driver, I sometimes had a glimpse at the pxa-driver. I found some suspicious places, and this patch contains my suggestions. Note: They are not tested, due to no hardware.

[JD: Some more fixes.]

Signed-off-by Wolfram Sang <[email protected]>

Signed-off-by Jean Delvare <[email protected]>

---
 drivers/i2c/busses/i2c-pxa.c |   27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

--- linux-2.6.25-rc1.orig/drivers/i2c/busses/i2c-pxa.c 2008-02-13
09:51:09.000000000 +0100
+++ linux-2.6.25-rc1/drivers/i2c/busses/i2c-pxa.c 2008-02-13
09:57:33.000000000 +0100

drivers/i2c/busses/i2c-pxa.c +15 -13

@@ -997,7 +997,14 @@ static int i2c_pxa_probe(struct platform
 	spin_lock_init(&i2c->lock);
 	init_waitqueue_head(&i2c->wait);
 
-	sprintf(i2c->adap.name, "pxa_i2c-i2c.%u", dev->id);
+	/*
+	 * If "dev->id" is negative we consider it as zero.
+	 * The reason to do so is to avoid sysfs names that only make
+	 * sense when there are multiple adapters.
+	 */
+	i2c->adap.nr = dev->id != -1 ? dev->id : 0;
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
+		 i2c->adap.nr);
 
 	i2c->clk = clk_get(&dev->dev, "I2CCLK");
 	if (IS_ERR(i2c->clk)) {

@@ -1048,13 +1055,6 @@ static int i2c_pxa_probe(struct platform
 	i2c->adap.algo_data = i2c;
 	i2c->adap.dev.parent = &dev->dev;
 
-	/*
-	 * If "dev->id" is negative we consider it as zero.
-	 * The reason to do so is to avoid sysfs names that only make
-	 * sense when there are multiple adapters.
-	 */
-	i2c->adap.nr = dev->id != -1 ? dev->id : 0;
-
 	ret = i2c_add_numbered_adapter(&i2c->adap);
 	if (ret < 0) {
 		printk(KERN_INFO "I2C: Failed to add bus\n");

@@ -1078,6 +1078,7 @@ eadapt:
 ereqirq:
 	clk_disable(i2c->clk);
 	i2c_pxa_disable(dev);
+	iounmap(i2c->reg_base);
 eremap:
 	clk_put(i2c->clk);
 eclk:

@@ -1087,7 +1088,7 @@ emalloc:
 	return ret;
 }
 
-static int i2c_pxa_remove(struct platform_device *dev)
+static int __devexit i2c_pxa_remove(struct platform_device *dev)
 {
 	struct pxa_i2c *i2c = platform_get_drvdata(dev);
 

@@ -1101,6 +1102,7 @@ static int i2c_pxa_remove(struct platfor
 	clk_put(i2c->clk);
 	i2c_pxa_disable(dev);
 
+	iounmap(i2c->reg_base);
 	release_mem_region(i2c->iobase, i2c->iosize);
 	kfree(i2c);
 

@@ -1109,9 +1111,10 @@ static int i2c_pxa_remove(struct platfor
 
 static struct platform_driver i2c_pxa_driver = {
 	.probe		= i2c_pxa_probe,
-	.remove		= i2c_pxa_remove,
+	.remove		= __devexit_p(i2c_pxa_remove),
 	.driver		= {
 		.name	= "pxa2xx-i2c",
+		.owner	= THIS_DRIVER,
 	},
 };
 

@@ -1120,9 +1123,9 @@ static int __init i2c_adap_pxa_init(void
 	return platform_driver_register(&i2c_pxa_driver);
 }
 
-static void i2c_adap_pxa_exit(void)
+static void __exit i2c_adap_pxa_exit(void)
 {
-	return platform_driver_unregister(&i2c_pxa_driver);
+	platform_driver_unregister(&i2c_pxa_driver);
 }
 
 MODULE_LICENSE("GPL");
 
--

Jean Delvare

Hi wolfram,

I can help test this of cuz, yet I have a few questions:

1. what's the exact reason of moving the i2c->adap.nr assignment to the begining, it looks this is unnecessary since I didn't find any dependency of the following code on i2c->adap.nr ??

2. the remaining changes looks good, so they are clean up right? instead of trying to fix something

Hi Wolfram,

Quoted 3 lines · On Mon, 21 Jan 2008 15:20:16 +0100, [email protected].…

While working on the PCA9564-platform driver, I sometimes had a glimpse at the pxa-driver. I found some suspicious places, and this patch contains my suggestions. Note: They are not tested, due to no hardware.

Looks good. I've added a couple more fixes, resulting patch is below. As I don't have hardware for testing either, I would appreciate if someone with a PXA board could test it and confirm that it causes no regression. Mike? Eric? I plan to send this patch to Linus for 2.6.25 (i.e. quickly.)

From: Wolfram Sang <[email protected]>
Subject: i2c-pxa: Misc fixes

While working on the PCA9564-platform driver, I sometimes had a glimpse at the pxa-driver. I found some suspicious places, and this patch contains my suggestions. Note: They are not tested, due to no hardware.

Quoted 5 lines · Jean Delvare

Looks good. I've added a couple more fixes, resulting patch is below. As I don't have hardware for testing either, I would appreciate if someone with a PXA board could test it and confirm that it causes no regression. Mike? Eric? I plan to send this patch to Linus for 2.6.25 (i.e. quickly.)

This patch doesn't apply on top of Linus' tree most probably due to a conflict with this patch by Eric Miao:

http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-pxa-use-cpu_is_pxa27x.patch

Except that, it seems to be Ok, at least with the hardware I have.

-----Original Message-----
From: Mike Rapoport [mailto:mike-UTxiZqZC01RS1MOuV/[email protected]]
Sent: Wednesday, February 13, 2008 7:57 PM
To: Jean Delvare
Cc: Wolfram Sang; [email protected]; Eric Miao
Subject: Re: [i2c] [patch 6/6] Minor fixes for pxa-driver

Quoted 4 lines · Jean Delvare

Hi Wolfram,

Quoted 1 line · On Mon, 21 Jan 2008 15:20:16 +0100, [email protected].…

While working on the PCA9564-platform driver, I sometimes had a glimpse at

the

Quoted 8 lines

Quoted 2 lines

pxa-driver. I found some suspicious places, and this patch contains my suggestions. Note: They are not tested, due to no hardware.

Looks good. I've added a couple more fixes, resulting patch is below. As I don't have hardware for testing either, I would appreciate if someone with a PXA board could test it and confirm that it causes no regression. Mike? Eric? I plan to send this patch to Linus for 2.6.25 (i.e. quickly.)

This patch doesn't apply on top of Linus' tree most probably due to a conflict with this patch by Eric Miao:

http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-pxa-use-cpu_is_
pxa27x.patch

Except that, it seems to be Ok, at least with the hardware I have.

Quoted 4 lines

From: Wolfram Sang <[email protected]>
Subject: i2c-pxa: Misc fixes

While working on the PCA9564-platform driver, I sometimes had a glimpse at

the

Quoted 3 lines

pxa-driver. I found some suspicious places, and this patch contains my suggestions. Note: They are not tested, due to no hardware.

[ snip ]

--
Sincerely yours,
Mike.

Hi Jean.

drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_remove(struct platfor · line 1117

Quoted 3 lines · Jean Delvare

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

 	.driver		= {
 		.name	= "pxa2xx-i2c",
+		.owner	= THIS_DRIVER,

THIS_MODULE? Was it just too obvious to be mentioned or how come it builds at all? My platform driver complains...

-----Original Message-----
From: Jean Delvare [mailto:[email protected]]
Sent: Friday, February 15, 2008 6:12 AM
To: Wolfram Sang
Cc: [email protected]; Eric Miao; Mike Rapoport
Subject: Re: [i2c] [patch 6/6] Minor fixes for pxa-driver

Quoted 9 lines · Wolfram Sang

Hi Jean.

drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_remove(struct platfor · line 1117

Quoted 3 lines · Jean Delvare

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

 	.driver		= {
 		.name	= "pxa2xx-i2c",
+		.owner	= THIS_DRIVER,

THIS_MODULE? Was it just too obvious to be mentioned or how come it builds at all? My platform driver complains...

Oops, you are totally right, thanks for catching my mistake.

This makes me wonder what Mike and Eric actually tested...

--
Jean Delvare

i2c-pxa-misc-fixes ... should use "__exit" and "__exit_p" instead
of "__devexit" and "__devexit_p" on the remove() function. The PXA platform bus is normal: it's not hot-unpluggable.

to extend the pca-algorithm a bit. I tried to adapt the existing ISA-driver to the extensions, but cannot test it. The original maintainer receives a cc.