[patch 0/6] PCA9564-platform driver

[patch 0/6] PCA9564-platform driver

[w.sang-bIcnvbaLZ9MEGnE8C9+IrQ]

This is a series to add a platform driver for the PCA9564. For this, I needed
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. I developed it using a Blackfin based board. This driver should
be able to handle multiple instances (unlike the ISA one), but again
I could not test this feature myself.

I am looking forward to feedback! Thanks in advance.

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 1/6] Removing trailing whitespaces from pca-algorithm

[w.sang-bIcnvbaLZ9MEGnE8C9+IrQ]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: i2c_algos_pca-trailing.diff --]
[-- Type: text/plain, Size: 4683 bytes --]

This makes further patches more readable.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-pca.c |   42 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.c
===================================================================
--- linux-playground.orig/drivers/i2c/algos/i2c-algo-pca.c	2008-01-21 11:27:18.000000000 +0100
+++ linux-playground/drivers/i2c/algos/i2c-algo-pca.c	2008-01-21 12:39:28.000000000 +0100
@@ -1,5 +1,5 @@
 /*
- *  i2c-algo-pca.c i2c driver algorithms for PCA9564 adapters                
+ *  i2c-algo-pca.c i2c driver algorithms for PCA9564 adapters
  *    Copyright (C) 2004 Arcom Control Systems
  *
  *  This program is free software; you can redistribute it and/or modify
@@ -99,7 +99,7 @@
  *
  * returns after the address has been sent
  */
-static void pca_address(struct i2c_algo_pca_data *adap, 
+static void pca_address(struct i2c_algo_pca_data *adap,
 			struct i2c_msg *msg)
 {
 	int sta = pca_get_con(adap);
@@ -108,9 +108,9 @@
 	addr = ( (0x7f & msg->addr) << 1 );
 	if (msg->flags & I2C_M_RD )
 		addr |= 1;
-	DEB2("=== SLAVE ADDRESS %#04x+%c=%#04x\n", 
+	DEB2("=== SLAVE ADDRESS %#04x+%c=%#04x\n",
 	     msg->addr, msg->flags & I2C_M_RD ? 'R' : 'W', addr);
-	
+
 	pca_outw(adap, I2C_PCA_DAT, addr);
 
 	sta &= ~(I2C_PCA_CON_STO|I2C_PCA_CON_STA|I2C_PCA_CON_SI);
@@ -124,7 +124,7 @@
  *
  * Returns after the byte has been transmitted
  */
-static void pca_tx_byte(struct i2c_algo_pca_data *adap, 
+static void pca_tx_byte(struct i2c_algo_pca_data *adap,
 			__u8 b)
 {
 	int sta = pca_get_con(adap);
@@ -142,19 +142,19 @@
  *
  * returns immediately.
  */
-static void pca_rx_byte(struct i2c_algo_pca_data *adap, 
+static void pca_rx_byte(struct i2c_algo_pca_data *adap,
 			__u8 *b, int ack)
 {
 	*b = pca_inw(adap, I2C_PCA_DAT);
 	DEB2("=== READ %#04x %s\n", *b, ack ? "ACK" : "NACK");
 }
 
-/* 
+/*
  * Setup ACK or NACK for next received byte and wait for it to arrive.
  *
  * Returns after next byte has arrived.
  */
-static void pca_rx_ack(struct i2c_algo_pca_data *adap, 
+static void pca_rx_ack(struct i2c_algo_pca_data *adap,
 		       int ack)
 {
 	int sta = pca_get_con(adap);
@@ -168,8 +168,8 @@
 	pca_wait(adap);
 }
 
-/* 
- * Reset the i2c bus / SIO 
+/*
+ * Reset the i2c bus / SIO
  */
 static void pca_reset(struct i2c_algo_pca_data *adap)
 {
@@ -203,14 +203,14 @@
 		for (curmsg = 0; curmsg < num; curmsg++) {
 			int addr, i;
 			msg = &msgs[curmsg];
-			
+
 			addr = (0x7f & msg->addr) ;
-		
+
 			if (msg->flags & I2C_M_RD )
-				printk(KERN_INFO "    [%02d] RD %d bytes from %#02x [%#02x, ...]\n", 
+				printk(KERN_INFO "    [%02d] RD %d bytes from %#02x [%#02x, ...]\n",
 				       curmsg, msg->len, addr, (addr<<1) | 1);
 			else {
-				printk(KERN_INFO "    [%02d] WR %d bytes to %#02x [%#02x%s", 
+				printk(KERN_INFO "    [%02d] WR %d bytes to %#02x [%#02x%s",
 				       curmsg, msg->len, addr, addr<<1,
 				       msg->len == 0 ? "" : ", ");
 				for(i=0; i < msg->len; i++)
@@ -237,7 +237,7 @@
 		case 0x10: /* A repeated start condition has been transmitted */
 			pca_address(adap, msg);
 			break;
-			
+
 		case 0x18: /* SLA+W has been transmitted; ACK has been received */
 		case 0x28: /* Data byte in I2CDAT has been transmitted; ACK has been received */
 			if (numbytes < msg->len) {
@@ -287,7 +287,7 @@
 		case 0x38: /* Arbitration lost during SLA+W, SLA+R or data bytes */
 			DEB2("Arbitration lost\n");
 			goto out;
-			
+
 		case 0x58: /* Data byte has been received; NOT ACK has been returned */
 			if ( numbytes == msg->len - 1 ) {
 				pca_rx_byte(adap, &msg->buf[numbytes], 0);
@@ -320,13 +320,13 @@
 			printk(KERN_ERR DRIVER ": unhandled SIO state 0x%02x\n", state);
 			break;
 		}
-		
+
 	}
 
 	ret = curmsg;
  out:
 	DEB1(KERN_CRIT "}}} transfered %d/%d messages. "
-	     "status is %#04x. control is %#04x\n", 
+	     "status is %#04x. control is %#04x\n",
 	     curmsg, num, pca_status(adap),
 	     pca_get_con(adap));
 	return ret;
@@ -350,7 +350,7 @@
 	pca_outw(adap, I2C_PCA_ADR, own << 1);
 
 	pca_set_con(adap, I2C_PCA_CON_ENSIO | clock);
-	udelay(500); /* 500 µs for oscilator to stabilise */
+	udelay(500); /* 500 us for oscilator to stabilise */
 
 	return 0;
 }
@@ -360,8 +360,8 @@
 	.functionality	= pca_func,
 };
 
-/* 
- * registering functions to load algorithms at runtime 
+/*
+ * registering functions to load algorithms at runtime
  */
 int i2c_pca_add_bus(struct i2c_adapter *adap)
 {

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry


[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 1/6] Removing trailing whitespaces from pca-algorithm

[Jean Delvare]

Hi Wolfram,

On Mon, 21 Jan 2008 15:20:11 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> This makes further patches more readable.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-pca.c |   42 +++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)

Applied, thanks.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 2/6] Extension of pca-algorithm

[w.sang-bIcnvbaLZ9MEGnE8C9+IrQ]

[-- Attachment #1: i2c_algos_i2c-algo-pca-extension.diff --]
[-- Type: text/plain, Size: 8277 bytes --]

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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 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
===================================================================
--- linux-playground.orig/include/linux/i2c-algo-pca.h	2008-01-21 12:39:28.000000000 +0100
+++ linux-playground/include/linux/i2c-algo-pca.h	2008-01-21 12:42:14.000000000 +0100
@@ -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);
-	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);
+	int  (*wait_for_completion)	(void *data);
+	void (*reset_chip)		(void *data);
+	unsigned int			my_slave_addr;
+	unsigned int			i2c_clock;
+	unsigned int			timeout;
 };
 
-int i2c_pca_add_bus(struct i2c_adapter *);
+int i2c_pca_add_adapter(struct i2c_adapter *);
+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
===================================================================
--- linux-playground.orig/drivers/i2c/algos/i2c-algo-pca.h	2008-01-21 12:39:28.000000000 +0100
+++ linux-playground/drivers/i2c/algos/i2c-algo-pca.h	2008-01-21 12:42:14.000000000 +0100
@@ -14,13 +14,4 @@
 #define I2C_PCA_CON_SI		0x08 /* Serial Interrupt */
 #define I2C_PCA_CON_CR		0x07 /* Clock Rate (MASK) */
 
-#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 */
Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.c
===================================================================
--- linux-playground.orig/drivers/i2c/algos/i2c-algo-pca.c	2008-01-21 12:39:28.000000000 +0100
+++ linux-playground/drivers/i2c/algos/i2c-algo-pca.c	2008-01-21 12:56:43.000000000 +0100
@@ -1,6 +1,12 @@
 /*
  *  i2c-algo-pca.c i2c driver algorithms for PCA9564 adapters
  *    Copyright (C) 2004 Arcom Control Systems
+ *    Copyright (C) 2008 Pengutronix
+ *
+ *  History:
+ *        2004: initial version [Ian Campbell]
+ *    Jan 2008: extended algo_data and adapter initialization to support
+ *              platform drivers [Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
  *
  *  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)
 {
 	static int freqs[] = {330,288,217,146,88,59,44,36};
 	int own, clock;
+	struct i2c_algo_pca_data *pca_data = adap->algo_data;
+
+	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 */
+
+	adap->algo = &pca_algo;
+	adap->retries = 1;
 
-	own = pca_own(adap);
-	clock = pca_clock(adap);
-	DEB1(KERN_INFO DRIVER ": own address is %#04x\n", own);
-	DEB1(KERN_INFO DRIVER ": clock freqeuncy is %dkHz\n", freqs[clock]);
+	pca_reset(pca_data);
 
-	pca_outw(adap, I2C_PCA_ADR, own << 1);
+	own = pca_own(pca_data);
+	clock = pca_clock(pca_data);
 
-	pca_set_con(adap, I2C_PCA_CON_ENSIO | clock);
+	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]);
+
+	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 <icampbell-2sJRl1BP9u0AvxtiuMwx3w@public.gmane.org>");
 MODULE_DESCRIPTION("I2C-Bus PCA9564 algorithm");

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 2/6] Extension of pca-algorithm

[Jean Delvare]

Hi Wolfram,

On Mon, 21 Jan 2008 15:20:12 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> It now conatins a private data field and more per-instance data. The reset 

Typo: contains.

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

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

> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  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(-)
> 

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.

> Index: linux-playground/include/linux/i2c-algo-pca.h
> ===================================================================
> --- linux-playground.orig/include/linux/i2c-algo-pca.h	2008-01-21 12:39:28.000000000 +0100
> +++ linux-playground/include/linux/i2c-algo-pca.h	2008-01-21 12:42:14.000000000 +0100
> @@ -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);
> -	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);
> +	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?

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

> +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
> ===================================================================
> --- linux-playground.orig/drivers/i2c/algos/i2c-algo-pca.h	2008-01-21 12:39:28.000000000 +0100
> +++ linux-playground/drivers/i2c/algos/i2c-algo-pca.h	2008-01-21 12:42:14.000000000 +0100
> @@ -14,13 +14,4 @@
>  #define I2C_PCA_CON_SI		0x08 /* Serial Interrupt */
>  #define I2C_PCA_CON_CR		0x07 /* Clock Rate (MASK) */
>  
> -#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.

> Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.c
> ===================================================================
> --- linux-playground.orig/drivers/i2c/algos/i2c-algo-pca.c	2008-01-21 12:39:28.000000000 +0100
> +++ linux-playground/drivers/i2c/algos/i2c-algo-pca.c	2008-01-21 12:56:43.000000000 +0100
> @@ -1,6 +1,12 @@
>  /*
>   *  i2c-algo-pca.c i2c driver algorithms for PCA9564 adapters
>   *    Copyright (C) 2004 Arcom Control Systems
> + *    Copyright (C) 2008 Pengutronix
> + *
> + *  History:
> + *        2004: initial version [Ian Campbell]
> + *    Jan 2008: extended algo_data and adapter initialization to support
> + *              platform drivers [Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

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

>   *
>   *  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)
>  {
>  	static int freqs[] = {330,288,217,146,88,59,44,36};
>  	int own, clock;
> +	struct i2c_algo_pca_data *pca_data = adap->algo_data;
> +
> +	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?

> +
> +	adap->algo = &pca_algo;
> +	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.

>  
> -	own = pca_own(adap);
> -	clock = pca_clock(adap);
> -	DEB1(KERN_INFO DRIVER ": own address is %#04x\n", own);
> -	DEB1(KERN_INFO DRIVER ": clock freqeuncy is %dkHz\n", freqs[clock]);
> +	pca_reset(pca_data);
>  
> -	pca_outw(adap, I2C_PCA_ADR, own << 1);
> +	own = pca_own(pca_data);
> +	clock = pca_clock(pca_data);
>  
> -	pca_set_con(adap, I2C_PCA_CON_ENSIO | clock);
> +	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.

> +
> +	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 <icampbell-2sJRl1BP9u0AvxtiuMwx3w@public.gmane.org>");
>  MODULE_DESCRIPTION("I2C-Bus PCA9564 algorithm");
> 


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 2/6] Extension of pca-algorithm

[Wolfram Sang]

Hello Jean,

first of all thank you for your detailed constructive criticism; I 
appreciate your effort very much (and will try harder next time)!

Jean Delvare wrote:

> 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.)
Will do!

> 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.
I understand, will restructure it this way...

> 
>> Index: linux-playground/include/linux/i2c-algo-pca.h
>> ===================================================================
[...]
>>  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);
>> -	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);
>> +	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?
I adopted this behaviour from the other algorithms, which do all use 
void *data here. I guess, i2c-pca-algo used to pass the struct as it did 
not have any private_data field before. The ISA-driver as the only user 
of i2c-pca-algo never touched this parameter, as it gained all 
neccessary data from its own static variables.

> struct i2c_adapter already has a timeout field, so why define your own?
Sorry, this the result of a mistake in my notes. Will fix.

>> -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().
I thought it makes it more "readable" what this function actually does. 
Besides doing initialization, it mainly calls i2c_add_adapter. So, 
giving them a similar name, it could perhaps be more obvious that this 
function is a wrapper. But if you prefer consistency, I will go back to 
i2c_pca_add_bus.

>> Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.h
>> ===================================================================
>> --- linux-playground.orig/drivers/i2c/algos/i2c-algo-pca.h	2008-01-21 12:39:28.000000000 +0100
>> +++ linux-playground/drivers/i2c/algos/i2c-algo-pca.h	2008-01-21 12:42:14.000000000 +0100
>> @@ -14,13 +14,4 @@
>>  #define I2C_PCA_CON_SI		0x08 /* Serial Interrupt */
>>  #define I2C_PCA_CON_CR		0x07 /* Clock Rate (MASK) */
>>  
>> -#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.
I'd like to do this, but it gave me a hard time already. The reason is, 
that, in general, I would say all register definitions of the chip 
should be inside the algorithm, as this is the abstraction which deals 
with the chip and works with its registers. Now the problem is, 
i2c-pca-algo defines a function wait_for_completion which has to be 
filled by the adapter. (BTW, this routine used to have the name 
"wait_for_interrupt". I renamed it, because it also does polling when no 
interrupt is assigned, so this is a bit misleading) This function needs 
to check a bit from the PCA9564 when calling wait_event_interruptible, 
so it needs two definitions to work, namely those:

#define I2C_PCA_CON		0x03 /* CONTROL Read/Write */
#define I2C_PCA_CON_SI		0x08 /* Serial Interrupt */

Furthermore it might be nice to have those definitions within the ISR, 
so we can check if the interrupt was really caused by the PCA9564.

I see the following options, which all have drawbacks.

1) Do it like now, have a seperate i2c-pca-algo.h in /drivers/i2c/algos 
and include it in algorithm and adapter. Drawback: There is an 
additional .h-file.

2) Put all register-definitions into /include/linux/i2c-pca-algo.h. 
Drawback: All registers become visible, although not needed in most 
cases when this file is being included.

3) Just put the two #defines from above into 
/include/linux/i2c-pca-algo.h, the rest into i2c-pca-algo.c. Drawback: 
Register definitions are scattered -> messy -> horrible.

4) Duplicate those two #defines in the adapter source code: Drawback: 
Spoils maintainability pretty much. Also horrible.

5) Move the wait_for_completion-function from the adapter to the 
algorithm. Drawback: This function is connected to the wait_queue and 
the ISR which better belong to the adapter and not the algorithm. I fear 
the outcome will be unneccessarily complicated. We also lose the 
possibility to check if the interrupt was caused by the PCA.

6) Make the algorithm have a function i2c_pca_get_si_status which 
reports the desired condition. Export it, so it can be called from 
adapters. Drawback: I wonder if adapters should call the algorithms 
except for foo_add_bus? Sounds like wrong direction to me.

I'd be happy to get opinions here (or option number 7 which is so simple 
that I don't see it :))

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

>> +	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?
It was in the original PCA-driver; as it was never removed, I thought 
there is a reason for being there.


>> +	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.
It used to be set to 3 without being implemented :) Will drop it.


>> +	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.
Legacy bug: Will fix.


-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 3/6] Minor typos in i2c/algos/Kconfig

[w.sang-bIcnvbaLZ9MEGnE8C9+IrQ]

[-- Attachment #1: i2c_algos_Kconfig_typo.diff --]
[-- Type: text/plain, Size: 2021 bytes --]

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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/algos/Kconfig |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-playground/drivers/i2c/algos/Kconfig
===================================================================
--- linux-playground.orig/drivers/i2c/algos/Kconfig	2008-01-21 11:04:53.000000000 +0100
+++ linux-playground/drivers/i2c/algos/Kconfig	2008-01-21 11:35:19.000000000 +0100
@@ -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.

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 3/6] Minor typos in i2c/algos/Kconfig

[Jean Delvare]

Hi Wolfram,

On Mon, 21 Jan 2008 15:20:13 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> 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.

> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/i2c/algos/Kconfig |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux-playground/drivers/i2c/algos/Kconfig
> ===================================================================
> --- linux-playground.orig/drivers/i2c/algos/Kconfig	2008-01-21 11:04:53.000000000 +0100
> +++ linux-playground/drivers/i2c/algos/Kconfig	2008-01-21 11:35:19.000000000 +0100
> @@ -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.
> 


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 3/6] Minor typos in i2c/algos/Kconfig

[Wolfram Sang]

Hi Jean,

Jean Delvare wrote:
> Hi Wolfram,
> 
> On Mon, 21 Jan 2008 15:20:13 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
>> 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?

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 3/6] Minor typos in i2c/algos/Kconfig

[Jean Delvare]

On Thu, 24 Jan 2008 14:47:11 +0100, Wolfram Sang wrote:
> Jean Delvare wrote:
> > On Mon, 21 Jan 2008 15:20:13 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> >> 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?

I will send such a patch later today.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 4/6] Platform driver for the PCA9564.

[w.sang-bIcnvbaLZ9MEGnE8C9+IrQ]

[-- Attachment #1: i2c_busses_i2c-pca-platform.diff --]
[-- Type: text/plain, Size: 10085 bytes --]

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/Kconfig            |   11 +
 drivers/i2c/busses/Makefile           |    1 
 drivers/i2c/busses/i2c-pca-platform.c |  281 ++++++++++++++++++++++++++++++++++
 include/linux/i2c-pca-platform.h      |   13 +
 4 files changed, 306 insertions(+)

Index: linux-playground/drivers/i2c/busses/i2c-pca-platform.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-playground/drivers/i2c/busses/i2c-pca-platform.c	2008-01-21 13:10:54.000000000 +0100
@@ -0,0 +1,281 @@
+/*
+ *  i2c_pca_platform.c
+ *
+ *  Platform driver for the PCA9564 I2C controller.
+ *
+ *  Copyright (C) 2008 Pengutronix
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  History:
+ *    Jan 2008: Initial version [Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c-algo-pca.h>
+#include <linux/i2c-pca-platform.h>
+#include <linux/irq.h>
+#include <linux/io.h>
+
+#include <asm/gpio.h>
+
+#include "../algos/i2c-algo-pca.h"
+
+#define res_len(r)		((r)->end - (r)->start + 1)
+
+struct i2c_pca_pf_data {
+	void __iomem			*reg_base;
+	unsigned long			io_base;
+	unsigned long			io_size;
+	int				irq;	/* if negative, use polling */
+	struct i2c_adapter		adap;
+	struct i2c_algo_pca_data	algo_data;
+	wait_queue_head_t		wait;
+};
+
+/* Read/Write functions for different register alignments */
+
+static int i2c_pca_pf_readbyte8(void *pd, int reg)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	return ioread8(i2c->reg_base + reg);
+}
+
+static int i2c_pca_pf_readbyte16(void *pd, int reg)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	return ioread8(i2c->reg_base + reg * 2);
+}
+
+static int i2c_pca_pf_readbyte32(void *pd, int reg)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	return ioread8(i2c->reg_base + reg * 4);
+}
+
+static void i2c_pca_pf_writebyte8(void *pd, int reg, int val)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	iowrite8(val, i2c->reg_base + reg);
+}
+
+static void i2c_pca_pf_writebyte16(void *pd, int reg, int val)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	iowrite8(val, i2c->reg_base + reg * 2);
+}
+
+static void i2c_pca_pf_writebyte32(void *pd, int reg, int val)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	iowrite8(val, i2c->reg_base + reg * 4);
+}
+
+
+static int i2c_pca_pf_waitforcompletion(void *pd)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	int ret = 0;
+
+	if (i2c->irq > -1) {
+		ret = wait_event_interruptible(i2c->wait,
+			i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
+			& I2C_PCA_CON_SI);
+	} else {
+		while ((i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
+				& I2C_PCA_CON_SI) == 0)
+			udelay(100);
+	}
+
+	return ret;
+}
+
+static void i2c_pca_pf_dummyreset(void *pd)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	dev_warn(&i2c->adap.dev, "No reset-pin found. Chip may get stuck!\n");
+}
+
+static void i2c_pca_pf_resetchip(void *pd)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	struct i2c_pca9564_pf_platform_data *platform_data =
+			i2c->adap.dev.parent->platform_data;
+
+	gpio_set_value(platform_data->gpio, 0);
+	ndelay(100);
+	gpio_set_value(platform_data->gpio, 1);
+}
+
+static irqreturn_t i2c_pca_pf_handler(int this_irq, void *dev_id)
+{
+	struct i2c_pca_pf_data *i2c = dev_id;
+
+	if ((i2c->algo_data.read_byte(i2c, I2C_PCA_CON) & I2C_PCA_CON_SI) == 0)
+		return IRQ_NONE;
+
+	wake_up_interruptible(&i2c->wait);
+
+	return IRQ_HANDLED;
+}
+
+
+static int i2c_pca_pf_probe(struct platform_device *pdev)
+{
+	struct i2c_pca_pf_data *i2c;
+	struct resource *res;
+	struct i2c_pca9564_pf_platform_data *platform_data =
+				pdev->dev.platform_data;
+	int ret;
+	int irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_irq(pdev, 0);
+
+	/* No need to check 'irq'. If it is below 0, we do polling. */
+	if (res == NULL)
+		return -ENODEV;
+
+	if (!request_mem_region(res->start, res_len(res), res->name))
+		return -ENOMEM;
+
+	i2c = kzalloc(sizeof(struct i2c_pca_pf_data), GFP_KERNEL);
+	if (!i2c) {
+		ret = -ENOMEM;
+		goto emalloc;
+	}
+
+	init_waitqueue_head(&i2c->wait);
+
+	/*
+	 * If "pdev->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 = pdev->id != -1 ? pdev->id : 0;
+
+	i2c->adap.owner   = THIS_MODULE;
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "i2c-pca9564.%u",
+		pdev->id);
+	i2c->adap.algo_data 	= &i2c->algo_data;
+	i2c->adap.dev.parent 	= &pdev->dev;
+	i2c->adap.class   	= I2C_CLASS_HWMON;
+
+	i2c->reg_base = ioremap(res->start, res_len(res));
+	if (!i2c->reg_base) {
+		ret = -EIO;
+		goto eremap;
+	}
+	i2c->io_base	= res->start;
+	i2c->io_size	= res_len(res);
+	i2c->irq	= irq;
+
+	i2c->algo_data.my_slave_addr	= platform_data->my_slave_addr;
+	i2c->algo_data.i2c_clock	= platform_data->i2c_clock_speed;
+	i2c->algo_data.timeout		= platform_data->timeout;
+
+	i2c->algo_data.data	= i2c;
+
+	switch (res->flags & IORESOURCE_MEM_TYPE_MASK) {
+	case IORESOURCE_MEM_32BIT:
+		i2c->algo_data.write_byte	= i2c_pca_pf_writebyte32;
+		i2c->algo_data.read_byte	= i2c_pca_pf_readbyte32;
+		break;
+	case IORESOURCE_MEM_16BIT:
+		i2c->algo_data.write_byte	= i2c_pca_pf_writebyte16;
+		i2c->algo_data.read_byte	= i2c_pca_pf_readbyte16;
+		break;
+	case IORESOURCE_MEM_8BIT:
+	default:
+		i2c->algo_data.write_byte	= i2c_pca_pf_writebyte8;
+		i2c->algo_data.read_byte	= i2c_pca_pf_readbyte8;
+		break;
+	}
+
+	i2c->algo_data.wait_for_completion	= i2c_pca_pf_waitforcompletion;
+
+	if (platform_data->gpio > -1)
+		i2c->algo_data.reset_chip	= i2c_pca_pf_resetchip;
+	else
+		i2c->algo_data.reset_chip	= i2c_pca_pf_dummyreset;
+
+	if (irq > -1) {
+		ret = request_irq(irq, i2c_pca_pf_handler,
+			IRQF_TRIGGER_FALLING, i2c->adap.name, i2c);
+		if (ret)
+			goto ereqirq;
+	}
+
+	if (i2c_pca_add_numbered_adapter(&i2c->adap) < 0) {
+		dev_err(&i2c->adap.dev, "Failed to add PCA9564\n");
+		goto eadapt;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	dev_info(&i2c->adap.dev, "PCA9564 registered.\n");
+	return 0;
+
+eadapt:
+	if (irq > -1)
+		free_irq(irq, i2c);
+ereqirq:
+	iounmap(i2c->reg_base);
+eremap:
+	kfree(i2c);
+emalloc:
+	release_mem_region(res->start, res_len(res));
+	return ret;
+}
+
+static int i2c_pca_pf_remove(struct platform_device *pdev)
+{
+	struct i2c_pca_pf_data *i2c = platform_get_drvdata(pdev);
+	platform_set_drvdata(pdev, NULL);
+
+	i2c_del_adapter(&i2c->adap);
+
+	if (i2c->irq > -1)
+		free_irq(i2c->irq, i2c);
+
+	iounmap(i2c->reg_base);
+	release_mem_region(i2c->io_base, i2c->io_size);
+	kfree(i2c);
+
+	return 0;
+}
+
+static struct platform_driver i2c_pca_pf_driver = {
+	.probe		= i2c_pca_pf_probe,
+	.remove		= i2c_pca_pf_remove,
+	.driver		= {
+		.name	= "pca9564_platform",
+	},
+};
+
+static int __init i2c_pca_pf_init(void)
+{
+	return platform_driver_register(&i2c_pca_pf_driver);
+}
+
+static void __exit i2c_pca_pf_exit(void)
+{
+	platform_driver_unregister(&i2c_pca_pf_driver);
+}
+
+MODULE_AUTHOR("Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
+MODULE_DESCRIPTION("I2C-PCA9564 platform driver");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_pca_pf_init);
+module_exit(i2c_pca_pf_exit);
+
Index: linux-playground/drivers/i2c/busses/Kconfig
===================================================================
--- linux-playground.orig/drivers/i2c/busses/Kconfig	2008-01-21 12:39:28.000000000 +0100
+++ linux-playground/drivers/i2c/busses/Kconfig	2008-01-21 13:10:54.000000000 +0100
@@ -646,6 +646,17 @@
 	  delays when I2C/SMBus chip drivers are loaded (e.g. at boot
 	  time).  If unsure, say N.
 
+config I2C_PCA_PLATFORM
+	tristate "PCA9564 on platform bus"
+	select I2C_ALGOPCA
+	default n
+	help
+	  This driver supports a memory mapped Philips PCA 9564
+	  Parallel bus to I2C bus controller
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-pca-platform.
+
 config I2C_MV64XXX
 	tristate "Marvell mv64xxx I2C Controller"
 	depends on MV64X60 && EXPERIMENTAL
Index: linux-playground/drivers/i2c/busses/Makefile
===================================================================
--- linux-playground.orig/drivers/i2c/busses/Makefile	2008-01-21 12:39:28.000000000 +0100
+++ linux-playground/drivers/i2c/busses/Makefile	2008-01-21 13:10:55.000000000 +0100
@@ -31,6 +31,7 @@
 obj-$(CONFIG_I2C_PARPORT_LIGHT)	+= i2c-parport-light.o
 obj-$(CONFIG_I2C_PASEMI)	+= i2c-pasemi.o
 obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
+obj-$(CONFIG_I2C_PCA_PLATFORM)	+= i2c-pca-platform.o
 obj-$(CONFIG_I2C_PIIX4)		+= i2c-piix4.o
 obj-$(CONFIG_I2C_PMCMSP)	+= i2c-pmcmsp.o
 obj-$(CONFIG_I2C_PNX)		+= i2c-pnx.o
Index: linux-playground/include/linux/i2c-pca-platform.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-playground/include/linux/i2c-pca-platform.h	2008-01-21 13:10:55.000000000 +0100
@@ -0,0 +1,13 @@
+#ifndef I2C_PCA9564_PLATFORM_H
+#define I2C_PCA9564_PLATFORM_H
+
+struct i2c_pca9564_pf_platform_data {
+	int gpio;		/* pin to reset chip. driver will work when
+				 * not supplied (negative value), but it
+				 * cannot exit some error conditions then */
+	int i2c_clock_speed;	/* values are defined in linux/i2c-algo-pca.h */
+	int timeout;		/* timeout = this value * 10us */
+	int my_slave_addr;	/* yet unused in pca-algo */
+};
+
+#endif /* I2C_PCA9564_PLATFORM_H */

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 5/6] pca-isa driver uses the new pca-algorithm

[w.sang-bIcnvbaLZ9MEGnE8C9+IrQ]

[-- Attachment #1: i2c_busses_i2c-pca-isa_update.diff --]
[-- Type: text/plain, Size: 4176 bytes --]

This is untested, due to no hardware!

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 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
===================================================================
--- linux-playground.orig/drivers/i2c/busses/i2c-pca-isa.c	2008-01-21 12:39:28.000000000 +0100
+++ linux-playground/drivers/i2c/busses/i2c-pca-isa.c	2008-01-21 13:19:48.000000000 +0100
@@ -1,6 +1,12 @@
 /*
  *  i2c-pca-isa.c driver for PCA9564 on ISA boards
  *    Copyright (C) 2004 Arcom Control Systems
+ *    Copyright (C) 2008 Pengutronix
+ *
+ *  History:
+ *        2004: initial version [Ian Campbell]
+ *    Jan 2008: updates to use the newer PCA-algorithm
+ *              [Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
  *
  *  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>
@@ -30,16 +35,14 @@
 #include <linux/isa.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-pca.h>
-
-#include <asm/io.h>
-#include <asm/irq.h>
+#include <linux/io.h>
+#include <linux/irq.h>
 
 #include "../algos/i2c-algo-pca.h"
 
 #define IO_SIZE 4
 
 #undef DEBUG_IO
-//#define DEBUG_IO
 
 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);
 }
 
-static int
-pca_isa_readbyte(struct i2c_algo_pca_data *adap, int reg)
+static int pca_isa_readbyte(void *pd, int reg)
 {
 	int res = inb(base+reg);
 #ifdef DEBUG_IO
 	{
-		static char *names[] = { "STA", "DAT", "ADR", "CON" };	
+		static char *names[] = { "STA", "DAT", "ADR", "CON" };
 		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;
 
@@ -93,23 +84,32 @@
 		ret = wait_event_interruptible(pca_wait,
 					       pca_isa_readbyte(adap, I2C_PCA_CON) & I2C_PCA_CON_SI);
 	} else {
-		while ((pca_isa_readbyte(adap, I2C_PCA_CON) & I2C_PCA_CON_SI) == 0) 
+		while ((pca_isa_readbyte(adap, I2C_PCA_CON) & I2C_PCA_CON_SI) == 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");
+}
+
 static irqreturn_t pca_handler(int this_irq, void *dev_id) {
 	wake_up_interruptible(&pca_wait);
 	return IRQ_HANDLED;
 }
 
 static struct i2c_algo_pca_data pca_isa_data = {
-	.get_own		= pca_isa_getown,
-	.get_clock		= pca_isa_getclock,
+	.data			= NULL,
 	.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 = {
@@ -157,7 +157,7 @@
 {
 	i2c_del_adapter(&pca_isa_ops);
 
-	if (irq > 0) {
+	if (irq > -1) {
 		disable_irq(irq);
 		free_irq(irq, &pca_isa_ops);
 	}

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 5/6] pca-isa driver uses the new pca-algorithm

[Jean Delvare]

Hi Wolfram,

On Mon, 21 Jan 2008 15:20:15 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> 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.

> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  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
> ===================================================================
> --- linux-playground.orig/drivers/i2c/busses/i2c-pca-isa.c	2008-01-21 12:39:28.000000000 +0100
> +++ linux-playground/drivers/i2c/busses/i2c-pca-isa.c	2008-01-21 13:19:48.000000000 +0100
> @@ -1,6 +1,12 @@
>  /*
>   *  i2c-pca-isa.c driver for PCA9564 on ISA boards
>   *    Copyright (C) 2004 Arcom Control Systems
> + *    Copyright (C) 2008 Pengutronix
> + *
> + *  History:
> + *        2004: initial version [Ian Campbell]
> + *    Jan 2008: updates to use the newer PCA-algorithm
> + *              [Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

No history in drivers.

>   *
>   *  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>
> @@ -30,16 +35,14 @@
>  #include <linux/isa.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-algo-pca.h>
> -
> -#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.

>  
>  #include "../algos/i2c-algo-pca.h"
>  
>  #define IO_SIZE 4
>  
>  #undef DEBUG_IO
> -//#define DEBUG_IO

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

>  
>  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);
>  }
>  
> -static int
> -pca_isa_readbyte(struct i2c_algo_pca_data *adap, int reg)
> +static int pca_isa_readbyte(void *pd, int reg)
>  {
>  	int res = inb(base+reg);
>  #ifdef DEBUG_IO
>  	{
> -		static char *names[] = { "STA", "DAT", "ADR", "CON" };	
> +		static char *names[] = { "STA", "DAT", "ADR", "CON" };

Such whitespace cleanups would rather belong to patch 1/6.

>  		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;
>  
> @@ -93,23 +84,32 @@
>  		ret = wait_event_interruptible(pca_wait,
>  					       pca_isa_readbyte(adap, I2C_PCA_CON) & I2C_PCA_CON_SI);
>  	} else {
> -		while ((pca_isa_readbyte(adap, I2C_PCA_CON) & I2C_PCA_CON_SI) == 0) 
> +		while ((pca_isa_readbyte(adap, I2C_PCA_CON) & I2C_PCA_CON_SI) == 0)

Same here.

>  			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");
> +}
> +
>  static irqreturn_t pca_handler(int this_irq, void *dev_id) {
>  	wake_up_interruptible(&pca_wait);
>  	return IRQ_HANDLED;
>  }
>  
>  static struct i2c_algo_pca_data pca_isa_data = {
> -	.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.

>  	.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 = {
> @@ -157,7 +157,7 @@
>  {
>  	i2c_del_adapter(&pca_isa_ops);
>  
> -	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.

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


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 5/6] pca-isa driver uses the new pca-algorithm

[Wolfram Sang]

Jean Delvare wrote:
> Hi Wolfram,
> 
> On Mon, 21 Jan 2008 15:20:15 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
>> 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.
Err? Seems like a bug in my workflow. Will fix!

> No history in drivers.
Will be deleted.

>> -#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.
checkpatch.pl told me so.

>>  #undef DEBUG_IO
>> -//#define DEBUG_IO
> While you're here, the line before could be discarded as well.
OK.

  >>  static struct i2c_algo_pca_data pca_isa_data = {
>> -	.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.
The pointer is passed along, but never used with the ISA-driver. 
Everything needed is in static variables (base address, wait_queue). I 
set the pointer explicitly to NULL, so the assignment is "marked" to be 
fully intentional. Then again, a comment would also do.

Note: I was thinking about removing the static variables and creating an 
apropriate struct dynamically; but I fear this change is too big for not 
having the hardware.

>> -	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.
Everything inside the ISA-driver checked against -1, so I assumed this 
to be a bug. Should have explained this in detail, sorry, I forgot. 
After reading the recent thread about NO_IRQ, I also came to the 
conclusion, that I better should use this, too.

Thanks again for your review!

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[patch 6/6] Minor fixes for pxa-driver

[w.sang-bIcnvbaLZ9MEGnE8C9+IrQ]

[-- Attachment #1: i2c_busses_i2c_pxa-fixes.diff --]
[-- Type: text/plain, Size: 1810 bytes --]

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.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-pxa.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux-playground/drivers/i2c/busses/i2c-pxa.c
===================================================================
--- linux-playground.orig/drivers/i2c/busses/i2c-pxa.c	2008-01-21 12:39:28.000000000 +0100
+++ linux-playground/drivers/i2c/busses/i2c-pxa.c	2008-01-21 13:21:10.000000000 +0100
@@ -870,7 +870,7 @@
 	spin_lock_init(&i2c->lock);
 	init_waitqueue_head(&i2c->wait);
 
-	sprintf(i2c->adap.name, "pxa_i2c-i2c.%u", dev->id);
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u", dev->id);
 
 	i2c->clk = clk_get(&dev->dev, "I2CCLK");
 	if (IS_ERR(i2c->clk)) {
@@ -963,6 +963,7 @@
 		local_irq_enable();
 	}
 #endif
+	iounmap(i2c->reg_base);
 eremap:
 	clk_put(i2c->clk);
 eclk:
@@ -991,7 +992,7 @@
 		local_irq_enable();
 	}
 #endif
-
+	iounmap(i2c->reg_base);
 	release_mem_region(i2c->iobase, i2c->iosize);
 	kfree(i2c);
 
@@ -1011,9 +1012,9 @@
 	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");

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Jean Delvare]

Hi Wolfram,

On Mon, 21 Jan 2008 15:20:16 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> 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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 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
@@ -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

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Eric Miao]

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

- eric

> -----Original Message-----
> From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
> Sent: Wednesday, February 13, 2008 4:59 PM
> To: Wolfram Sang
> Cc: i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org; Mike Rapoport; Eric Miao
> Subject: Re: [i2c] [patch 6/6] Minor fixes for pxa-driver
> 
> Hi Wolfram,
> 
> On Mon, 21 Jan 2008 15:20:16 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> > 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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  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
> @@ -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
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Jean Delvare]

Hi Eric,

On Wed, 13 Feb 2008 01:22:52 -0800, Eric Miao wrote:
> 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 ??

There is a dependency:

-	sprintf(i2c->adap.name, "pxa_i2c-i2c.%u", dev->id);

+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
+		 i2c->adap.nr);

We are using i2c->adap.nr instead of dev->id to generate the adapter
name. This is to avoid a name of "pxa_i2c-i2c.-1" if there is a single
platform device, "pxa_i2c-i2c.0" looks better I think.

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

Yes, these are essentially cleanups, I don't think that anything bad
was really happening in practice before this patch.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Mike Rapoport]

Jean Delvare wrote:
> Hi Wolfram,
> 
> On Mon, 21 Jan 2008 15:20:16 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
>> 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.)

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.


> From: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 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.
> 

[ snip ]



-- 
Sincerely yours,
Mike.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Jean Delvare]

Hi Mike,

On Wed, 13 Feb 2008 13:57:21 +0200, Mike Rapoport wrote:
> Jean Delvare wrote:
> > 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

This patch no longer exists as a file because it has already been merged
into Linus' tree:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=59d70df025473931c500d6d60510798e4bfa3279

My patch was generated on top of Linus' tree so it certainly applies
there.

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

Thanks for testing and reporting!

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Eric Miao]

Yeah, it also works here on zylonite/pxa3xx.

> -----Original Message-----
> From: Mike Rapoport [mailto:mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org]
> Sent: Wednesday, February 13, 2008 7:57 PM
> To: Jean Delvare
> Cc: Wolfram Sang; i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org; Eric Miao
> Subject: Re: [i2c] [patch 6/6] Minor fixes for pxa-driver
> 
> Jean Delvare wrote:
> > Hi Wolfram,
> >
> > On Mon, 21 Jan 2008 15:20:16 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> >> 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.)
> 
> 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.
> 
> 
> > From: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > 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.
> >
> 
> [ snip ]
> 
> 
> 
> --
> Sincerely yours,
> Mike.

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Wolfram Sang]

Hi Jean.

Jean Delvare wrote:

>  	.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...

All the best,

    Wolfram

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Jean Delvare]

On Thu, 14 Feb 2008 19:55:46 +0100, Wolfram Sang wrote:
> Hi Jean.
> 
> Jean Delvare wrote:
> 
> >  	.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 mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Eric Miao]

I modified this by hand when the compiler complains, I thought it was typo
and ignored this :-(

- eric

> -----Original Message-----
> From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
> Sent: Friday, February 15, 2008 6:12 AM
> To: Wolfram Sang
> Cc: i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org; Eric Miao; Mike Rapoport
> Subject: Re: [i2c] [patch 6/6] Minor fixes for pxa-driver
> 
> On Thu, 14 Feb 2008 19:55:46 +0100, Wolfram Sang wrote:
> > Hi Jean.
> >
> > Jean Delvare wrote:
> >
> > >  	.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 mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 6/6] Minor fixes for pxa-driver

[Jean Delvare]

Patch updated after receiving this comment (in private) from David
Brownell:

> 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.

* * * * *

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 <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Tested-by: Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
Tested-by: Eric Miao <ymiao3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-pxa.c |   25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

--- linux-2.6.25-rc2.orig/drivers/i2c/busses/i2c-pxa.c	2008-02-23 18:18:46.000000000 +0100
+++ linux-2.6.25-rc2/drivers/i2c/busses/i2c-pxa.c	2008-02-23 22:42:54.000000000 +0100
@@ -999,7 +999,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)) {
@@ -1050,13 +1057,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");
@@ -1080,6 +1080,7 @@ eadapt:
 ereqirq:
 	clk_disable(i2c->clk);
 	i2c_pxa_disable(dev);
+	iounmap(i2c->reg_base);
 eremap:
 	clk_put(i2c->clk);
 eclk:
@@ -1089,7 +1090,7 @@ emalloc:
 	return ret;
 }
 
-static int i2c_pxa_remove(struct platform_device *dev)
+static int __exit i2c_pxa_remove(struct platform_device *dev)
 {
 	struct pxa_i2c *i2c = platform_get_drvdata(dev);
 
@@ -1103,6 +1104,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);
 
@@ -1111,9 +1113,10 @@ static int i2c_pxa_remove(struct platfor
 
 static struct platform_driver i2c_pxa_driver = {
 	.probe		= i2c_pxa_probe,
-	.remove		= i2c_pxa_remove,
+	.remove		= __exit_p(i2c_pxa_remove),
 	.driver		= {
 		.name	= "pxa2xx-i2c",
+		.owner	= THIS_MODULE,
 	},
 };
 
@@ -1122,7 +1125,7 @@ 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)
 {
 	platform_driver_unregister(&i2c_pxa_driver);
 }


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [patch 0/6] PCA9564-platform driver

[Wolfram Sang]

w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> 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.
Meanwhile he replied to me and stated that he also has no longer access 
to the ISA hardware. Pity.

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c