Skip to content Skip to conversation

PCA9564 platform driver

PATCH Applied Wolfram Sang 3 participants 17 messages Feb 6, 2008 – Mar 16, 2008 11 messages not in the archive I2C subsystem host drivers

Share & tools

Wolfram Sang

author

2008-02-06 20:20 UTC #0 lore raw

v2 cover PCA9564 platform driver

Hello,

this is my second try of a patch series adding a platform driver for the PCA9564. I included most suggestions from last time; if not, I tried to make clear why. One point I should mention again is that I did not use NO_IRQ as I read on lkml, that 0 is the equivalent for NO_IRQ and everything else is more like an unwanted exception.

I built this driver on x86 and blackfin. The actual testing was done using the latter.

All the best,

Wolfram

Signature · 7 lines

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

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

Wolfram Sang

author

2008-02-06 20:20 UTC #1 lore raw

PATCH v2 1/3 Remove trailing whitespaces and unnecessary UTF

Remove trailing whitespaces to make further patches more readable. Also remove an unnecessary UTF-char for simplicity ("us" for microseconds is fine enough).

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

---
 drivers/i2c/algos/i2c-algo-pca.c |   42 +++++++++++++++++++--------------------
 drivers/i2c/busses/i2c-pca-isa.c |    4 +--
 2 files changed, 23 insertions(+), 23 deletions(-)

drivers/i2c/algos/i2c-algo-pca.c +21 ~~-21~~

Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.c
===================================================================
@@ -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)
 {

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

Index: linux-playground/drivers/i2c/busses/i2c-pca-isa.c
===================================================================
@@ -78,7 +78,7 @@
 	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

@@ -93,7 +93,7 @@
 		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;

Signature · 2 lines

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

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jean Delvare

Applied

2008-02-12 16:31 UTC #3 lore raw

Re: [PATCHv2 1/3] Remove trailing whitespaces and unnecessary UTF

Hi Wolfram,

Quoted 4 lines · Wolfram Sang

Remove trailing whitespaces to make further patches more readable. Also remove an unnecessary UTF-char for simplicity ("us" for microseconds is fine enough).
Signed-off-by Wolfram Sang <[email protected]>

Applied, thanks.

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

Wolfram Sang

author

2008-02-06 20:20 UTC #4 lore raw

PATCH v2 2/3 Extend the PCA9564-algorithm and adapt its only user (pca-isa).

In reply to Wolfram Sang

The separation between algorithm and adapter was unsharp at places. This was partly hidden by the fact, that the ISA-driver allowed just one instance and had all private data in static variables. This patch makes neccessary preparations to add a platform driver on top of the algorithm, while still supporting ISA. Note: Due to lack of hardware, the ISA-driver could not be tested except that it builds.

Concerning the core struct i2c_algo_pca_data:

- A private data field was added, all hardware dependant data may go here. Similar to other algorithms, now a pointer to this data is passed to the adapter's functions. In order to make as less changes as possible to the ISA-driver, it leaves the private data empty and still only uses its static variables.

- A "reset_chip" function pointer was added; such a functionality must come from the adapter, not the algorithm.

- use a variable "i2c_clock" instead of a function pointer "get_clock", allowing for write access to a default in case a wrong value was supplied.

In the algorithm-file:

- move "i2c-pca-algo.h" into "linux/i2c-algo-pca.h"
- now using per_instance timeout values (i2c_adap->timeout)
- error messages specify the device, not only the driver name
- restructure initialization to easily support "i2c_add_numbered_adapter"
- drop "retries" and "own" (i2c address) as they were unused

(The state-machine for I2C-communication was not touched.)

In the ISA-driver:

- adapt to new algorithm
- updated tests to variable "irq" to the convention that 0 is NO_IRQ

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

---
 drivers/i2c/algos/i2c-algo-pca.c |   87 +++++++++++++++++++--------------------
 drivers/i2c/algos/i2c-algo-pca.h |   26 -----------
 drivers/i2c/busses/i2c-pca-isa.c |   59 ++++++++++----------------
 include/linux/i2c-algo-pca.h     |   37 ++++++++++++++--
 4 files changed, 98 insertions(+), 111 deletions(-)

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

Index: linux-playground/include/linux/i2c-algo-pca.h
===================================================================
@@ -1,14 +1,41 @@
 #ifndef _LINUX_I2C_ALGO_PCA_H
 #define _LINUX_I2C_ALGO_PCA_H
 
+/* Clock speeds for the bus */
+#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
+
+/* PCA9564 registers */
+#define I2C_PCA_STA		0x00 /* STATUS  Read Only  */
+#define I2C_PCA_TO		0x00 /* TIMEOUT Write Only */
+#define I2C_PCA_DAT		0x01 /* DATA    Read/Write */
+#define I2C_PCA_ADR		0x02 /* OWN ADR Read/Write */
+#define I2C_PCA_CON		0x03 /* CONTROL Read/Write */
+
+#define I2C_PCA_CON_AA		0x80 /* Assert Acknowledge */
+#define I2C_PCA_CON_ENSIO	0x40 /* Enable */
+#define I2C_PCA_CON_STA		0x20 /* Start */
+#define I2C_PCA_CON_STO		0x10 /* Stop */
+#define I2C_PCA_CON_SI		0x08 /* Serial Interrupt */
+#define I2C_PCA_CON_CR		0x07 /* Clock Rate (MASK) */
+
 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);
+	/* i2c_clock values are defined in linux/i2c-algo-pca.h */
+	unsigned int			i2c_clock;
 };
 
 int i2c_pca_add_bus(struct i2c_adapter *);
+int i2c_pca_add_numbered_bus(struct i2c_adapter *);
 
 #endif /* _LINUX_I2C_ALGO_PCA_H */

drivers/i2c/algos/i2c-algo-pca.h deleted +0 ~~-26~~

Index: linux-playground/drivers/i2c/algos/i2c-algo-pca.h
===================================================================
@@ -1,26 +0,0 @@
-#ifndef I2C_PCA9564_H
-#define I2C_PCA9564_H 1
-
-#define I2C_PCA_STA		0x00 /* STATUS  Read Only  */
-#define I2C_PCA_TO		0x00 /* TIMEOUT Write Only */
-#define I2C_PCA_DAT		0x01 /* DATA    Read/Write */
-#define I2C_PCA_ADR		0x02 /* OWN ADR Read/Write */
-#define I2C_PCA_CON		0x03 /* CONTROL Read/Write */
-
-#define I2C_PCA_CON_AA		0x80 /* Assert Acknowledge */
-#define I2C_PCA_CON_ENSIO	0x40 /* Enable */
-#define I2C_PCA_CON_STA		0x20 /* Start */
-#define I2C_PCA_CON_STO		0x10 /* Stop */
-#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 */

drivers/i2c/algos/i2c-algo-pca.c +43 ~~-44~~

160 lines not on this page · raw message

drivers/i2c/busses/i2c-pca-isa.c +23 ~~-36~~

161 lines not on this page · raw message

Signature · 7 lines

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

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jean Delvare

2008-02-12 17:14 UTC #6 lore raw

Re: [PATCHv2 2/3] Extend the PCA9564-algorithm and adapt its only user (pca-isa).

Hi Wolfram,

Quoted 29 lines · Wolfram Sang

The separation between algorithm and adapter was unsharp at places. This was partly hidden by the fact, that the ISA-driver allowed just one instance and had all private data in static variables. This patch makes neccessary preparations to add a platform driver on top of the algorithm, while still supporting ISA. Note: Due to lack of hardware, the ISA-driver could not be tested except that it builds.
Concerning the core struct i2c_algo_pca_data:
- A private data field was added, all hardware dependant data may go here. Similar to other algorithms, now a pointer to this data is passed to the adapter's functions. In order to make as less changes as possible to the ISA-driver, it leaves the private data empty and still only uses its static variables.
- A "reset_chip" function pointer was added; such a functionality must come from the adapter, not the algorithm.
- use a variable "i2c_clock" instead of a function pointer "get_clock", allowing for write access to a default in case a wrong value was supplied.
In the algorithm-file:
- move "i2c-pca-algo.h" into "linux/i2c-algo-pca.h"
- now using per_instance timeout values (i2c_adap->timeout)
- error messages specify the device, not only the driver name
- restructure initialization to easily support "i2c_add_numbered_adapter"
- drop "retries" and "own" (i2c address) as they were unused

Quoted 6 lines

(The state-machine for I2C-communication was not touched.)
In the ISA-driver:
- adapt to new algorithm
- updated tests to variable "irq" to the convention that 0 is NO_IRQ

I'm fine with everything except this. I'm not saying that the changes in irq tests aren't correct (the original code looks weird) but this is a functional change, unrelated with the rest of your patch. So if you really want to change it, that must be a separate patch.

No need to resend, I've reverted these changes myself. Patch applied, thanks.

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

Wolfram Sang

author

2008-02-06 20:20 UTC #7 lore raw

PATCH v2 3/3 Add platform driver on top of the new pca-algorithm

In reply to Wolfram Sang

Tested on a blackfin.

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

---
 drivers/i2c/busses/Kconfig            |   11 +
 drivers/i2c/busses/Makefile           |    1 
 drivers/i2c/busses/i2c-pca-platform.c |  278 ++++++++++++++++++++++++++++++++++
 include/linux/i2c-pca-platform.h      |   12 +
 4 files changed, 302 insertions(+)

drivers/i2c/busses/i2c-pca-platform.c new file +278 -0

Index: linux-playground/drivers/i2c/busses/i2c-pca-platform.c
===================================================================
@@ -0,0 +1,278 @@
+/*
+ *  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.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c-algo-pca.h>
+#include <linux/i2c-pca-platform.h>
+
+#include <asm/irq.h>
+#include <asm/io.h>
+
+#ifdef GENERIC_GPIO
+#include <asm/gpio.h>
+#endif
+
+#define res_len(r)		((r)->end - (r)->start + 1)
+
+struct i2c_pca_pf_data {
+	void __iomem			*reg_base;
+	int				irq;	/* if 0, use polling */
+	wait_queue_head_t		wait;
+	struct i2c_adapter		adap;
+	struct i2c_algo_pca_data	algo_data;
+	unsigned long			io_base;
+	unsigned long			io_size;
+};
+
+/* 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) {
+		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");
+}
+
+#ifdef GENERIC_GPIO
+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);
+}
+#endif
+
+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);
+	/* If irq is 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 e_alloc;
+	}
+
+	init_waitqueue_head(&i2c->wait);
+
+	i2c->adap.nr = pdev->id >= 0 ? 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.timeout	= platform_data->timeout;
+
+	i2c->reg_base = ioremap(res->start, res_len(res));
+	if (!i2c->reg_base) {
+		ret = -EIO;
+		goto e_remap;
+	}
+	i2c->io_base	= res->start;
+	i2c->io_size	= res_len(res);
+	i2c->irq	= irq;
+
+	i2c->algo_data.i2c_clock	= platform_data->i2c_clock_speed;
+	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;
+
+#ifdef GENERIC_GPIO
+	/* Use NO_GPIO if this macro is in kernel somwhen? */
+	if (platform_data->gpio > -1)
+		i2c->algo_data.reset_chip	= i2c_pca_pf_resetchip;
+	else
+		i2c->algo_data.reset_chip	= i2c_pca_pf_dummyreset;
+#else
+	i2c->algo_data.reset_chip	= i2c_pca_pf_dummyreset;
+#endif
+	if (irq) {
+		ret = request_irq(irq, i2c_pca_pf_handler,
+			IRQF_TRIGGER_FALLING, i2c->adap.name, i2c);
+		if (ret)
+			goto e_reqirq;
+	}
+
+	if (i2c_pca_add_numbered_bus(&i2c->adap) < 0) {
+		dev_err(&i2c->adap.dev, "Failed to add PCA9564\n");
+		goto e_adapt;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	dev_info(&i2c->adap.dev, "PCA9564 registered.\n");
+	return 0;
+
+e_adapt:
+	if (irq)
+		free_irq(irq, i2c);
+e_reqirq:
+	iounmap(i2c->reg_base);
+e_remap:
+	kfree(i2c);
+e_alloc:
+	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)
+		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		= __devexit_p(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 <[email protected]>");
+MODULE_DESCRIPTION("I2C-PCA9564 platform driver");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_pca_pf_init);
+module_exit(i2c_pca_pf_exit);
+

drivers/i2c/busses/Kconfig +11 -0

Index: linux-playground/drivers/i2c/busses/Kconfig
===================================================================
@@ -641,6 +641,17 @@
 	  delays when I2C/SMBus chip drivers are loaded (e.g. at boot
 	  time).  If unsure, say N.
 
+config I2C_PCA_PLATFORM
+	tristate "PCA9564 as platform device"
+	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 || ARCH_ORION) && EXPERIMENTAL

drivers/i2c/busses/Makefile +1 -0

Index: linux-playground/drivers/i2c/busses/Makefile
===================================================================
@@ -30,6 +30,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

include/linux/i2c-pca-platform.h new file +12 -0

Index: linux-playground/include/linux/i2c-pca-platform.h
===================================================================
@@ -0,0 +1,12 @@
+#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 */
+};
+
+#endif /* I2C_PCA9564_PLATFORM_H */

Signature · 7 lines

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

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jean Delvare

2008-02-12 21:53 UTC #9 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Hi Wolfram,

Quoted 61 lines · Wolfram Sang

Tested on a blackfin.

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

---
 drivers/i2c/busses/Kconfig            |   11 +
 drivers/i2c/busses/Makefile           |    1 
 drivers/i2c/busses/i2c-pca-platform.c |  278 ++++++++++++++++++++++++++++++++++
 include/linux/i2c-pca-platform.h      |   12 +
 4 files changed, 302 insertions(+)

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

drivers/i2c/busses/i2c-pca-platform.c new file +45 -0

@@ -0,0 +1,278 @@
+/*
+ *  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.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c-algo-pca.h>
+#include <linux/i2c-pca-platform.h>
+
+#include <asm/irq.h>
+#include <asm/io.h>
+
+#ifdef GENERIC_GPIO
+#include <asm/gpio.h>
+#endif
+
+#define res_len(r)		((r)->end - (r)->start + 1)
+
+struct i2c_pca_pf_data {
+	void __iomem			*reg_base;
+	int				irq;	/* if 0, use polling */
+	wait_queue_head_t		wait;
+	struct i2c_adapter		adap;
+	struct i2c_algo_pca_data	algo_data;
+	unsigned long			io_base;
+	unsigned long			io_size;
+};
+
+/* Read/Write functions for different register alignments */
+

drivers/i2c/busses/i2c-pca-platform.c line 55

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+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);

Shouldn't this be ioread16?

drivers/i2c/busses/i2c-pca-platform.c line 62

Quoted 7 lines

drivers/i2c/busses/i2c-pca-platform.c new file +7 -0

+}
+
+static int i2c_pca_pf_readbyte32(void *pd, int reg)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	return ioread8(i2c->reg_base + reg * 4);
+}

And ioread32?

Quoted 23 lines

drivers/i2c/busses/i2c-pca-platform.c new file +23 -0

+
+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;

drivers/i2c/busses/i2c-pca-platform.c line 95

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+	int ret = 0;
+
+	if (i2c->irq) {
+		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);

No timeout?

Quoted 9 lines

drivers/i2c/busses/i2c-pca-platform.c new file +9 -0

+	}
+
+	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");

drivers/i2c/busses/i2c-pca-platform.c line 114

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+}
+
+#ifdef GENERIC_GPIO
+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);

The i2c_clock gets to be copied to the driver data structure, but for the gpio you have to fetch it from the platform device data? Not very consistent.

Quoted 38 lines

drivers/i2c/busses/i2c-pca-platform.c new file +38 -0

+	ndelay(100);
+	gpio_set_value(platform_data->gpio, 1);
+}
+#endif
+
+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);
+	/* If irq is 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);

drivers/i2c/busses/i2c-pca-platform.c line 162

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+	if (!i2c) {
+		ret = -ENOMEM;
+		goto e_alloc;
+	}
+
+	init_waitqueue_head(&i2c->wait);
+
+	i2c->adap.nr = pdev->id >= 0 ? pdev->id : 0;
+
+	i2c->adap.owner   = THIS_MODULE;

No alignment in code please.

drivers/i2c/busses/i2c-pca-platform.c line 164

Quoted 2 lines

drivers/i2c/busses/i2c-pca-platform.c new file +2 -0
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "i2c-pca9564.%u",
+		pdev->id);

That's a bit confusing given that the driver isn't named i2c-pca9564. Other drivers usually include the address to distinguish between multiple device for example (..., "PCA9564 adapter at %04lx", res->start).

Quoted 26 lines

drivers/i2c/busses/i2c-pca-platform.c new file +26 -0

+	i2c->adap.algo_data 	= &i2c->algo_data;
+	i2c->adap.dev.parent 	= &pdev->dev;
+	i2c->adap.timeout	= platform_data->timeout;
+
+	i2c->reg_base = ioremap(res->start, res_len(res));
+	if (!i2c->reg_base) {
+		ret = -EIO;
+		goto e_remap;
+	}
+	i2c->io_base	= res->start;
+	i2c->io_size	= res_len(res);
+	i2c->irq	= irq;
+
+	i2c->algo_data.i2c_clock	= platform_data->i2c_clock_speed;
+	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:

drivers/i2c/busses/i2c-pca-platform.c line 200

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+	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;
+
+#ifdef GENERIC_GPIO
+	/* Use NO_GPIO if this macro is in kernel somwhen? */

No "somwhen" in my dictionary.

Quoted 27 lines

drivers/i2c/busses/i2c-pca-platform.c new file +27 -0

+	if (platform_data->gpio > -1)
+		i2c->algo_data.reset_chip	= i2c_pca_pf_resetchip;
+	else
+		i2c->algo_data.reset_chip	= i2c_pca_pf_dummyreset;
+#else
+	i2c->algo_data.reset_chip	= i2c_pca_pf_dummyreset;
+#endif
+	if (irq) {
+		ret = request_irq(irq, i2c_pca_pf_handler,
+			IRQF_TRIGGER_FALLING, i2c->adap.name, i2c);
+		if (ret)
+			goto e_reqirq;
+	}
+
+	if (i2c_pca_add_numbered_bus(&i2c->adap) < 0) {
+		dev_err(&i2c->adap.dev, "Failed to add PCA9564\n");
+		goto e_adapt;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	dev_info(&i2c->adap.dev, "PCA9564 registered.\n");
+	return 0;
+
+e_adapt:
+	if (irq)
+		free_irq(irq, i2c);

drivers/i2c/busses/i2c-pca-platform.c line 237

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+e_reqirq:
+	iounmap(i2c->reg_base);
+e_remap:
+	kfree(i2c);
+e_alloc:
+	release_mem_region(res->start, res_len(res));
+	return ret;
+}
+
+static int i2c_pca_pf_remove(struct platform_device *pdev)

Missing __devexit.

Quoted 11 lines

drivers/i2c/busses/i2c-pca-platform.c new file +11 -0

+{
+	struct i2c_pca_pf_data *i2c = platform_get_drvdata(pdev);
+	platform_set_drvdata(pdev, NULL);
+
+	i2c_del_adapter(&i2c->adap);
+
+	if (i2c->irq)
+		free_irq(i2c->irq, i2c);
+
+	iounmap(i2c->reg_base);
+	release_mem_region(i2c->io_base, i2c->io_size);

drivers/i2c/busses/i2c-pca-platform.c line 258

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+	kfree(i2c);
+
+	return 0;
+}
+
+static struct platform_driver i2c_pca_pf_driver = {
+	.probe		= i2c_pca_pf_probe,
+	.remove		= __devexit_p(i2c_pca_pf_remove),
+	.driver		= {
+		.name	= "pca9564_platform",

Missing .owner = THIS_MODULE.

Quoted 25 lines

drivers/i2c/busses/i2c-pca-platform.c new file +20 -0

+	},
+};
+
+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 <[email protected]>");
+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
===================================================================

drivers/i2c/busses/Kconfig line 649

Quoted 9 lines

drivers/i2c/busses/Kconfig +6 -0

@@ -641,6 +641,17 @@
 	  delays when I2C/SMBus chip drivers are loaded (e.g. at boot
 	  time).  If unsure, say N.
 
+config I2C_PCA_PLATFORM
+	tristate "PCA9564 as platform device"
+	select I2C_ALGOPCA
+	default n
+	help
+	  This driver supports a memory mapped Philips PCA 9564

For consistency, no space between "PCA" and "9564".

drivers/i2c/busses/Kconfig line 650

Quoted 1 line

drivers/i2c/busses/Kconfig +1 -0
+	  Parallel bus to I2C bus controller

Lowercase P, missing trailing dot.

Quoted 24 lines

drivers/i2c/busses/Kconfig +4 -0
+
+	  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 || ARCH_ORION) && EXPERIMENTAL
Index: linux-playground/drivers/i2c/busses/Makefile
===================================================================
drivers/i2c/busses/Makefile +1 -0
@@ -30,6 +30,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
===================================================================

include/linux/i2c-pca-platform.h line 4

Quoted 4 lines

include/linux/i2c-pca-platform.h new file +4 -0

@@ -0,0 +1,12 @@
+#ifndef I2C_PCA9564_PLATFORM_H
+#define I2C_PCA9564_PLATFORM_H
+
+struct i2c_pca9564_pf_platform_data {

"pf" is redundant with "platform", isn't it?

include/linux/i2c-pca-platform.h line 9

Quoted 5 lines

include/linux/i2c-pca-platform.h new file +5 -0

+	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 */

A rather curious time unit if you ask me.

Quoted 4 lines

include/linux/i2c-pca-platform.h new file +3 -0
+};
+
+#endif /* I2C_PCA9564_PLATFORM_H */

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

author

2008-03-07 15:52 UTC #11 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Hi Jean,

nearly a month since your reply, I am sorry. Work was shifting in a different direction meanwhile.

(Everything I did not comment on is already fixed as suggested)

Quoted 6 lines

Quoted 6 lines

drivers/i2c/busses/i2c-pca-platform.c new file +6 -0

+/* 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);

Quoted 8 lines

drivers/i2c/busses/i2c-pca-platform.c line 55
Quoted 6 lines
drivers/i2c/busses/i2c-pca-platform.c new file +6 -0
+}
+
+static int i2c_pca_pf_readbyte16(void *pd, int reg)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	return ioread8(i2c->reg_base + reg * 2);
Shouldn't this be ioread16?

I don't think so. The registers get scattered differently in the address-room, but their size itself is still 8 bit. I expect that ioread8 gives me just those 8 bits I want, hiding that some busses actually do 32-bit accesses only. Am I wrong?

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c line 95

Quoted 8 lines

drivers/i2c/busses/i2c-pca-platform.c new file +8 -0

+	if (i2c->irq) {
+		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);

No timeout?

You got a point there. The thing is that the underlying pca-algorithm does not have a timeout when expecting this condition (interrupt bit set). Even when using interrupts instead of polling, it will just sleep forever, if there goes something _really_ wrong. I could copy at least this behaviour by replacing the udelay(100); with an msleep(x) with x being a module parameter. I hope this is good enough, connecting the IRQ is preferred anyhow. (I am afraid that I don't have the time to add a sane and tested timeout mechanism to the algorithm)

Quoted 3 lines

The i2c_clock gets to be copied to the driver data structure, but for the gpio you have to fetch it from the platform device data? Not very consistent.

ACK.

Quoted 2 lines

drivers/i2c/busses/i2c-pca-platform.c line 162
Quoted 1 line
drivers/i2c/busses/i2c-pca-platform.c new file +1 -0
+	i2c->adap.owner   = THIS_MODULE;
No alignment in code please.

Really? Well, if you say so...

Quoted 4 lines

That's a bit confusing given that the driver isn't named i2c-pca9564. Other drivers usually include the address to distinguish between multiple device for example (..., "PCA9564 adapter at %04lx", res->start).

Okay, will look for something better.

Quoted 2 lines

Quoted 1 line
+struct i2c_pca9564_pf_platform_data {
"pf" is redundant with "platform", isn't it?

My intention was that 'i2c_pca9564_pf_' is the prefix for everything related to this platform-driver, and platform_data happens to be ...well... the platform data :)

Quoted 2 lines

include/linux/i2c-pca-platform.h line 9
Quoted 1 line
include/linux/i2c-pca-platform.h new file +1 -0
+	int timeout;		/* timeout = this value * 10us */
A rather curious time unit if you ask me.

Given by the algorithm. I didn't question it.

Best wishes,

Wolfram

Signature · 8 lines

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


_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

Jean Delvare

2008-03-08 10:13 UTC #12 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Hi Wolfgang,

Quoted 2 lines · Wolfram Sang

nearly a month since your reply, I am sorry. Work was shifting in a different direction meanwhile.

No problem, I have been pretty busy myself.

Quoted 16 lines

(Everything I did not comment on is already fixed as suggested)
Quoted 6 lines
Quoted 6 lines
drivers/i2c/busses/i2c-pca-platform.c new file +6 -0
+/* 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);
Quoted 8 lines
drivers/i2c/busses/i2c-pca-platform.c line 55
Quoted 6 lines
drivers/i2c/busses/i2c-pca-platform.c new file +6 -0
+}
+
+static int i2c_pca_pf_readbyte16(void *pd, int reg)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+	return ioread8(i2c->reg_base + reg * 2);
Shouldn't this be ioread16?

Quoted 4 lines

I don't think so. The registers get scattered differently in the address-room, but their size itself is still 8 bit. I expect that ioread8 gives me just those 8 bits I want, hiding that some busses actually do 32-bit accesses only. Am I wrong?

I really don't know, I am not familiar with the hardware. Leave it as is, we'll fix it later if someone complains.

Quoted 10 lines

Quoted 10 lines
drivers/i2c/busses/i2c-pca-platform.c line 95
Quoted 8 lines
drivers/i2c/busses/i2c-pca-platform.c new file +8 -0
+	if (i2c->irq) {
+		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);
No timeout?

Quoted 8 lines

You got a point there. The thing is that the underlying pca-algorithm does not have a timeout when expecting this condition (interrupt bit set). Even when using interrupts instead of polling, it will just sleep forever, if there goes something _really_ wrong. I could copy at least this behaviour by replacing the udelay(100); with an msleep(x) with x being a module parameter. I hope this is good enough, connecting the IRQ is preferred anyhow. (I am afraid that I don't have the time to add a sane and tested timeout mechanism to the algorithm)

I'd rather not make the driver more complex for a mode you do not want to encourage anyway. Leave the code as is, once again we can improve it later if really needed.

Waiting for an updated patch, then I'll push it to -mm. Patches 1/3 and 2/3 are already there.

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

author

2008-03-10 11:26 UTC #14 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Quoted 1 line · Jean Delvare

Hi Wolfgang,

"Wolfram" please... :)

Quoted 2 lines

Waiting for an updated patch, then I'll push it to -mm. Patches 1/3 and 2/3 are already there.

Attached. It works on Blackfin and builds on x86 and x86-64. Hopefully everything is caught now...

All the best,

Wolfram

===

Subject: Add platform driver on top of the new pca-algorithm
From: Wolfram Sang <[email protected]>

Changes since last revision:
- check against CONFIG_GENERIC_GPIO (was GENERIC_GPIO :( )
- don't use platform data anymore, copy all over to own struct
- give info about mem & irq when booting (and switch to printk as device is not yet registered)
- added comment about problems with polling
- added proper __devinit and __devexit
- added owner to module
- removed whitespace alignment in code
- driver now named "i2c-pca-platform" (as the module)
- fixed typos in Kconfig

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

===

Tested on a blackfin.

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

---
 drivers/i2c/busses/Kconfig            |   15 +
 drivers/i2c/busses/Makefile           |    1 
 drivers/i2c/busses/i2c-pca-platform.c |  297 ++++++++++++++++++++++++++++++++++
 include/linux/i2c-pca-platform.h      |   12 +
 4 files changed, 323 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-pca-platform.c new file +297 -0

Index: linux-playground/drivers/i2c/busses/i2c-pca-platform.c
===================================================================
@@ -0,0 +1,297 @@
+/*
+ *  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.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c-algo-pca.h>
+#include <linux/i2c-pca-platform.h>
+
+#include <asm/irq.h>
+#include <asm/io.h>
+
+#ifdef CONFIG_GENERIC_GPIO
+#include <asm/gpio.h>
+#endif
+
+#define res_len(r)		((r)->end - (r)->start + 1)
+
+struct i2c_pca_pf_data {
+	void __iomem			*reg_base;
+	int				irq;	/* if 0, use polling */
+#ifdef CONFIG_GENERIC_GPIO
+	int				gpio;
+#endif
+	wait_queue_head_t		wait;
+	struct i2c_adapter		adap;
+	struct i2c_algo_pca_data	algo_data;
+	unsigned long			io_base;
+	unsigned long			io_size;
+};
+
+/* 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) {
+		ret = wait_event_interruptible(i2c->wait,
+			i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
+			& I2C_PCA_CON_SI);
+	} else {
+		/*
+		 * Do polling...
+		 * XXX: Could get stuck in extreme cases!
+		 *      Maybe add timeout, but using irqs is preferred anyhow.
+		 */
+		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;
+	printk(KERN_WARNING "%s: No reset-pin found. Chip may get stuck!\n",
+		i2c->adap.name);
+}
+
+#ifdef CONFIG_GENERIC_GPIO
+static void i2c_pca_pf_resetchip(void *pd)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+
+	gpio_set_value(i2c->gpio, 0);
+	ndelay(100);
+	gpio_set_value(i2c->gpio, 1);
+}
+#endif
+
+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 __devinit 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;
+
+	printk(KERN_INFO "Registering PCA9564 ");
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_irq(pdev, 0);
+	/* If irq is 0, we do polling. */
+
+	if (res == NULL) {
+		ret = -ENODEV;
+		goto e_print;
+	}
+
+	if (!request_mem_region(res->start, res_len(res), res->name)) {
+		ret = -ENOMEM;
+		goto e_print;
+	}
+
+	i2c = kzalloc(sizeof(struct i2c_pca_pf_data), GFP_KERNEL);
+	if (!i2c) {
+		ret = -ENOMEM;
+		goto e_alloc;
+	}
+
+	init_waitqueue_head(&i2c->wait);
+
+	i2c->reg_base = ioremap(res->start, res_len(res));
+	if (!i2c->reg_base) {
+		ret = -EIO;
+		goto e_remap;
+	}
+	i2c->io_base = res->start;
+	i2c->io_size = res_len(res);
+	i2c->irq = irq;
+
+	printk("at 0x%08x ", res->start);
+
+	i2c->adap.nr = pdev->id >= 0 ? pdev->id : 0;
+	i2c->adap.owner = THIS_MODULE;
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "PCA9564 at 0x%08x",
+		res->start);
+	i2c->adap.algo_data = &i2c->algo_data;
+	i2c->adap.dev.parent = &pdev->dev;
+	i2c->adap.timeout = platform_data->timeout;
+
+	i2c->algo_data.i2c_clock = platform_data->i2c_clock_speed;
+	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;
+
+#ifdef CONFIG_GENERIC_GPIO
+	/* Use NO_GPIO when this macro is in mainline */
+	i2c->gpio = platform_data->gpio;
+	if (platform_data->gpio > -1)
+		i2c->algo_data.reset_chip = i2c_pca_pf_resetchip;
+	else
+		i2c->algo_data.reset_chip = i2c_pca_pf_dummyreset;
+#else
+	i2c->algo_data.reset_chip = i2c_pca_pf_dummyreset;
+#endif
+	if (irq) {
+		ret = request_irq(irq, i2c_pca_pf_handler,
+			IRQF_TRIGGER_FALLING, i2c->adap.name, i2c);
+		if (ret)
+			goto e_reqirq;
+	}
+
+	printk("using irq %d ", irq);
+
+	if (i2c_pca_add_numbered_bus(&i2c->adap) < 0) {
+		ret = -ENODEV;
+		goto e_adapt;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	printk("succeeded.\n");
+	return 0;
+
+e_adapt:
+	if (irq)
+		free_irq(irq, i2c);
+e_reqirq:
+	iounmap(i2c->reg_base);
+e_remap:
+	kfree(i2c);
+e_alloc:
+	release_mem_region(res->start, res_len(res));
+e_print:
+	printk("FAILED! (%d)\n", ret);
+	return ret;
+}
+
+static int __devexit 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)
+		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 = __devexit_p(i2c_pca_pf_remove),
+	.driver = {
+		.name = "i2c-pca-platform",
+		.owner = THIS_MODULE,
+	},
+};
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("I2C-PCA9564 platform driver");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_pca_pf_init);
+module_exit(i2c_pca_pf_exit);
+

drivers/i2c/busses/Kconfig +13 -2

Index: linux-playground/drivers/i2c/busses/Kconfig
===================================================================
@@ -632,8 +632,8 @@
 	select I2C_ALGOPCA
 	default n
 	help
-	  This driver supports ISA boards using the Philips PCA 9564
-	  Parallel bus to I2C bus controller
+	  This driver supports ISA boards using the Philips PCA9564
+	  parallel bus to I2C bus controller.
 	  
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-pca-isa.

@@ -643,6 +643,17 @@
 	  delays when I2C/SMBus chip drivers are loaded (e.g. at boot
 	  time).  If unsure, say N.
 
+config I2C_PCA_PLATFORM
+	tristate "PCA9564 as platform device"
+	select I2C_ALGOPCA
+	default n
+	help
+	  This driver supports a memory mapped Philips PCA9564
+	  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 || ARCH_ORION) && EXPERIMENTAL

drivers/i2c/busses/Makefile +1 -0

Index: linux-playground/drivers/i2c/busses/Makefile
===================================================================
@@ -30,6 +30,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

include/linux/i2c-pca-platform.h new file +12 -0

Index: linux-playground/include/linux/i2c-pca-platform.h
===================================================================
@@ -0,0 +1,12 @@
+#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 */
+};
+
+#endif /* I2C_PCA9564_PLATFORM_H */

Signature · 2 lines

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

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jean Delvare

2008-03-10 21:31 UTC #16 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Hi Wolfram,

Quoted 17 lines · Wolfram Sang

Quoted 1 line · Jean Delvare
Hi Wolfgang,
"Wolfram" please... :)
Quoted 2 lines
Waiting for an updated patch, then I'll push it to -mm. Patches 1/3 and 2/3 are already there.
Attached. It works on Blackfin and builds on x86 and x86-64. Hopefully everything is caught now...
All the best,
Wolfram
===

Quoted 8 lines

Subject: Add platform driver on top of the new pca-algorithm
From: Wolfram Sang <[email protected]>
Changes since last revision:
- check against CONFIG_GENERIC_GPIO (was GENERIC_GPIO :( )
- don't use platform data anymore, copy all over to own struct
- give info about mem & irq when booting (and switch to printk as device is not yet registered)

While the i2c_adapter device doesn't exist before i2c_add_adapter() is called, the underlying platform device exists as soon as probe() is entered.

Quoted 12 lines

- added comment about problems with polling
- added proper __devinit and __devexit
- added owner to module
- removed whitespace alignment in code
- driver now named "i2c-pca-platform" (as the module)
- fixed typos in Kconfig
Signed-off-by Wolfram Sang <[email protected]>
===
Tested on a blackfin.

Quoted 8 lines

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

---
 drivers/i2c/busses/Kconfig            |   15 +
 drivers/i2c/busses/Makefile           |    1 
 drivers/i2c/busses/i2c-pca-platform.c |  297 ++++++++++++++++++++++++++++++++++
 include/linux/i2c-pca-platform.h      |   12 +
 4 files changed, 323 insertions(+), 2 deletions(-)

I'm almost happy now, except for:

drivers/i2c/busses/i2c-pca-platform.c line 149

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+static int __devinit 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;
+
+	printk(KERN_INFO "Registering PCA9564 ");

Do not split printks that way. Other kernel threads might write to the log buffer as well and then the log is totally messed up. You must always call printk with complete lines.

Quoted 94 lines

drivers/i2c/busses/i2c-pca-platform.c new file +94 -0

+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_irq(pdev, 0);
+	/* If irq is 0, we do polling. */
+
+	if (res == NULL) {
+		ret = -ENODEV;
+		goto e_print;
+	}
+
+	if (!request_mem_region(res->start, res_len(res), res->name)) {
+		ret = -ENOMEM;
+		goto e_print;
+	}
+
+	i2c = kzalloc(sizeof(struct i2c_pca_pf_data), GFP_KERNEL);
+	if (!i2c) {
+		ret = -ENOMEM;
+		goto e_alloc;
+	}
+
+	init_waitqueue_head(&i2c->wait);
+
+	i2c->reg_base = ioremap(res->start, res_len(res));
+	if (!i2c->reg_base) {
+		ret = -EIO;
+		goto e_remap;
+	}
+	i2c->io_base = res->start;
+	i2c->io_size = res_len(res);
+	i2c->irq = irq;
+
+	printk("at 0x%08x ", res->start);
+
+	i2c->adap.nr = pdev->id >= 0 ? pdev->id : 0;
+	i2c->adap.owner = THIS_MODULE;
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "PCA9564 at 0x%08x",
+		res->start);
+	i2c->adap.algo_data = &i2c->algo_data;
+	i2c->adap.dev.parent = &pdev->dev;
+	i2c->adap.timeout = platform_data->timeout;
+
+	i2c->algo_data.i2c_clock = platform_data->i2c_clock_speed;
+	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;
+
+#ifdef CONFIG_GENERIC_GPIO
+	/* Use NO_GPIO when this macro is in mainline */
+	i2c->gpio = platform_data->gpio;
+	if (platform_data->gpio > -1)
+		i2c->algo_data.reset_chip = i2c_pca_pf_resetchip;
+	else
+		i2c->algo_data.reset_chip = i2c_pca_pf_dummyreset;
+#else
+	i2c->algo_data.reset_chip = i2c_pca_pf_dummyreset;
+#endif
+	if (irq) {
+		ret = request_irq(irq, i2c_pca_pf_handler,
+			IRQF_TRIGGER_FALLING, i2c->adap.name, i2c);
+		if (ret)
+			goto e_reqirq;
+	}
+
+	printk("using irq %d ", irq);
+
+	if (i2c_pca_add_numbered_bus(&i2c->adap) < 0) {
+		ret = -ENODEV;
+		goto e_adapt;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	printk("succeeded.\n");
+	return 0;
+
+e_adapt:
+	if (irq)
+		free_irq(irq, i2c);

drivers/i2c/busses/i2c-pca-platform.c line 253

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+e_reqirq:
+	iounmap(i2c->reg_base);
+e_remap:
+	kfree(i2c);
+e_alloc:
+	release_mem_region(res->start, res_len(res));
+e_print:
+	printk("FAILED! (%d)\n", ret);
+	return ret;
+}

Also see David Brownell's objections, which I second.

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

author

2008-03-14 14:50 UTC #18 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Hello,

I sent this patch to myself a minute ago and could apply it. Dunno what went wrong the last time. I am very sorry (I know that patch fixing is annoying).

All the best,

Wolfram

---

Subject: Add platform driver on top of the new pca-algorithm
From: Wolfram Sang <[email protected]>

Changes since last revision:
- use <linux/gpio.h> and remove #ifdefs CONFIG_GENERIC_GPIO
- correct reference to gpio_is_valid()
- register and setup gpio in the driver
- just print whole lines with printk
- removed warnings

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

---

Changes since last revision:
- check against CONFIG_GENERIC_GPIO (was GENERIC_GPIO :( )
- don't use platform data anymore, copy all over to own struct
- give info about mem & irq when booting (and switch to printk as device is not yet registered)
- add comment about problems with polling
- added proper __devinit and __devexit
- added owner to module
- removed whitespace alignment in code
- driver now named "i2c-pca-platform" (as the module)
- fixed typos in Kconfig

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

---

Tested on a blackfin.

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

---
 drivers/i2c/busses/Kconfig            |   15 +
 drivers/i2c/busses/Makefile           |    1 
 drivers/i2c/busses/i2c-pca-platform.c |  298 ++++++++++++++++++++++++++++++++++
 include/linux/i2c-pca-platform.h      |   12 +
 4 files changed, 324 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-pca-platform.c new file +298 -0

Index: linux-playground/drivers/i2c/busses/i2c-pca-platform.c
===================================================================
@@ -0,0 +1,298 @@
+/*
+ *  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.
+
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c-algo-pca.h>
+#include <linux/i2c-pca-platform.h>
+#include <linux/gpio.h>
+
+#include <asm/irq.h>
+#include <asm/io.h>
+
+#define res_len(r)		((r)->end - (r)->start + 1)
+
+struct i2c_pca_pf_data {
+	void __iomem			*reg_base;
+	int				irq;	/* if 0, use polling */
+	int				gpio;
+	wait_queue_head_t		wait;
+	struct i2c_adapter		adap;
+	struct i2c_algo_pca_data	algo_data;
+	unsigned long			io_base;
+	unsigned long			io_size;
+};
+
+/* 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) {
+		ret = wait_event_interruptible(i2c->wait,
+			i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
+			& I2C_PCA_CON_SI);
+	} else {
+		/*
+		 * Do polling...
+		 * XXX: Could get stuck in extreme cases!
+		 *      Maybe add timeout, but using irqs is preferred anyhow.
+		 */
+		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;
+	printk(KERN_WARNING "%s: No reset-pin found. Chip may get stuck!\n",
+		i2c->adap.name);
+}
+
+static void i2c_pca_pf_resetchip(void *pd)
+{
+	struct i2c_pca_pf_data *i2c = pd;
+
+	gpio_set_value(i2c->gpio, 0);
+	ndelay(100);
+	gpio_set_value(i2c->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 __devinit 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 = 0;
+	int irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_irq(pdev, 0);
+	/* If irq is 0, we do polling. */
+
+	if (res == NULL) {
+		ret = -ENODEV;
+		goto e_print;
+	}
+
+	if (!request_mem_region(res->start, res_len(res), res->name)) {
+		ret = -ENOMEM;
+		goto e_print;
+	}
+
+	i2c = kzalloc(sizeof(struct i2c_pca_pf_data), GFP_KERNEL);
+	if (!i2c) {
+		ret = -ENOMEM;
+		goto e_alloc;
+	}
+
+	init_waitqueue_head(&i2c->wait);
+
+	i2c->reg_base = ioremap(res->start, res_len(res));
+	if (!i2c->reg_base) {
+		ret = -EIO;
+		goto e_remap;
+	}
+	i2c->io_base = res->start;
+	i2c->io_size = res_len(res);
+	i2c->irq = irq;
+
+	i2c->adap.nr = pdev->id >= 0 ? pdev->id : 0;
+	i2c->adap.owner = THIS_MODULE;
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "PCA9564 at 0x%08lx",
+		(unsigned long) res->start);
+	i2c->adap.algo_data = &i2c->algo_data;
+	i2c->adap.dev.parent = &pdev->dev;
+	i2c->adap.timeout = platform_data->timeout;
+
+	i2c->algo_data.i2c_clock = platform_data->i2c_clock_speed;
+	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;
+
+
+	i2c->gpio = platform_data->gpio;
+	i2c->algo_data.reset_chip = i2c_pca_pf_dummyreset;
+
+	/* Use gpio_is_valid() when in mainline */
+	if (i2c->gpio > -1)
+		ret = gpio_request(i2c->gpio, i2c->adap.name);
+		if (ret == 0) {
+			gpio_direction_output(i2c->gpio, 1);
+			i2c->algo_data.reset_chip = i2c_pca_pf_resetchip;
+		} else {
+			printk(KERN_WARNING "%s: Registering gpio failed!\n",
+				i2c->adap.name);
+			i2c->gpio = ret;
+		}
+
+	if (irq) {
+		ret = request_irq(irq, i2c_pca_pf_handler,
+			IRQF_TRIGGER_FALLING, i2c->adap.name, i2c);
+		if (ret)
+			goto e_reqirq;
+	}
+
+	if (i2c_pca_add_numbered_bus(&i2c->adap) < 0) {
+		ret = -ENODEV;
+		goto e_adapt;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	printk(KERN_INFO "%s registered.\n", i2c->adap.name);
+
+	return 0;
+
+e_adapt:
+	if (irq)
+		free_irq(irq, i2c);
+e_reqirq:
+	if (i2c->gpio > -1)
+		gpio_free(i2c->gpio);
+
+	iounmap(i2c->reg_base);
+e_remap:
+	kfree(i2c);
+e_alloc:
+	release_mem_region(res->start, res_len(res));
+e_print:
+	printk(KERN_ERR "Registering PCA9564 FAILED! (%d)\n", ret);
+	return ret;
+}
+
+static int __devexit 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)
+		free_irq(i2c->irq, i2c);
+
+	if (i2c->gpio > -1)
+		gpio_free(i2c->gpio);
+
+	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 = __devexit_p(i2c_pca_pf_remove),
+	.driver = {
+		.name = "i2c-pca-platform",
+		.owner = THIS_MODULE,
+	},
+};
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("I2C-PCA9564 platform driver");
+MODULE_LICENSE("GPL");
+
+module_init(i2c_pca_pf_init);
+module_exit(i2c_pca_pf_exit);
+

drivers/i2c/busses/Kconfig +13 -2

Index: linux-playground/drivers/i2c/busses/Kconfig
===================================================================
@@ -632,8 +632,8 @@
 	select I2C_ALGOPCA
 	default n
 	help
-	  This driver supports ISA boards using the Philips PCA 9564
-	  Parallel bus to I2C bus controller
+	  This driver supports ISA boards using the Philips PCA9564
+	  parallel bus to I2C bus controller.
 	  
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-pca-isa.

@@ -643,6 +643,17 @@
 	  delays when I2C/SMBus chip drivers are loaded (e.g. at boot
 	  time).  If unsure, say N.
 
+config I2C_PCA_PLATFORM
+	tristate "PCA9564 as platform device"
+	select I2C_ALGOPCA
+	default n
+	help
+	  This driver supports a memory mapped Philips PCA9564
+	  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 || ARCH_ORION) && EXPERIMENTAL

drivers/i2c/busses/Makefile +1 -0

Index: linux-playground/drivers/i2c/busses/Makefile
===================================================================
@@ -30,6 +30,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

include/linux/i2c-pca-platform.h new file +12 -0

Index: linux-playground/include/linux/i2c-pca-platform.h
===================================================================
@@ -0,0 +1,12 @@
+#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 */
+};
+
+#endif /* I2C_PCA9564_PLATFORM_H */

Signature · 2 lines

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

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jean Delvare

2008-03-14 18:46 UTC #20 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Quoted 116 lines · Wolfram Sang

Hello,
I sent this patch to myself a minute ago and could apply it. Dunno what went wrong the last time. I am very sorry (I know that patch fixing is annoying).
All the best,
Wolfram
---
Subject: Add platform driver on top of the new pca-algorithm
From: Wolfram Sang <[email protected]>
Changes since last revision:
- use <linux/gpio.h> and remove #ifdefs CONFIG_GENERIC_GPIO
- correct reference to gpio_is_valid()
- register and setup gpio in the driver
- just print whole lines with printk
- removed warnings
Signed-off-by Wolfram Sang <[email protected]>
---
Changes since last revision:
- check against CONFIG_GENERIC_GPIO (was GENERIC_GPIO :( )
- don't use platform data anymore, copy all over to own struct
- give info about mem & irq when booting (and switch to printk as device is not yet registered)
- add comment about problems with polling
- added proper __devinit and __devexit
- added owner to module
- removed whitespace alignment in code
- driver now named "i2c-pca-platform" (as the module)
- fixed typos in Kconfig
Signed-off-by Wolfram Sang <[email protected]>
---
Tested on a blackfin.
Signed-off-by Wolfram Sang <[email protected]>
---
 drivers/i2c/busses/Kconfig            |   15 +
 drivers/i2c/busses/Makefile           |    1 
 drivers/i2c/busses/i2c-pca-platform.c |  298 ++++++++++++++++++++++++++++++++++
 include/linux/i2c-pca-platform.h      |   12 +
 4 files changed, 324 insertions(+), 2 deletions(-)
Index: linux-playground/drivers/i2c/busses/i2c-pca-platform.c
===================================================================
drivers/i2c/busses/i2c-pca-platform.c new file +60 -0
(...)
+static int __devinit 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 = 0;
+	int irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_irq(pdev, 0);
+	/* If irq is 0, we do polling. */
+
+	if (res == NULL) {
+		ret = -ENODEV;
+		goto e_print;
+	}
+
+	if (!request_mem_region(res->start, res_len(res), res->name)) {
+		ret = -ENOMEM;
+		goto e_print;
+	}
+
+	i2c = kzalloc(sizeof(struct i2c_pca_pf_data), GFP_KERNEL);
+	if (!i2c) {
+		ret = -ENOMEM;
+		goto e_alloc;
+	}
+
+	init_waitqueue_head(&i2c->wait);
+
+	i2c->reg_base = ioremap(res->start, res_len(res));
+	if (!i2c->reg_base) {
+		ret = -EIO;
+		goto e_remap;
+	}
+	i2c->io_base = res->start;
+	i2c->io_size = res_len(res);
+	i2c->irq = irq;
+
+	i2c->adap.nr = pdev->id >= 0 ? pdev->id : 0;
+	i2c->adap.owner = THIS_MODULE;
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "PCA9564 at 0x%08lx",
+		(unsigned long) res->start);
+	i2c->adap.algo_data = &i2c->algo_data;
+	i2c->adap.dev.parent = &pdev->dev;
+	i2c->adap.timeout = platform_data->timeout;
+
+	i2c->algo_data.i2c_clock = platform_data->i2c_clock_speed;
+	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;

drivers/i2c/busses/i2c-pca-platform.c line 202

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+	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;
+
+

No double blank lines inside functions please (they confuse patch too easily.)

Quoted 4 lines

drivers/i2c/busses/i2c-pca-platform.c new file +4 -0

+	i2c->gpio = platform_data->gpio;
+	i2c->algo_data.reset_chip = i2c_pca_pf_dummyreset;
+
+	/* Use gpio_is_valid() when in mainline */

drivers/i2c/busses/i2c-pca-platform.c line 216

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+	if (i2c->gpio > -1)
+		ret = gpio_request(i2c->gpio, i2c->adap.name);
+		if (ret == 0) {
+			gpio_direction_output(i2c->gpio, 1);
+			i2c->algo_data.reset_chip = i2c_pca_pf_resetchip;
+		} else {
+			printk(KERN_WARNING "%s: Registering gpio failed!\n",
+				i2c->adap.name);
+			i2c->gpio = ret;
+		}

You're missing curly braces around this block, aren't you?

Quoted 25 lines

drivers/i2c/busses/i2c-pca-platform.c new file +25 -0

+
+	if (irq) {
+		ret = request_irq(irq, i2c_pca_pf_handler,
+			IRQF_TRIGGER_FALLING, i2c->adap.name, i2c);
+		if (ret)
+			goto e_reqirq;
+	}
+
+	if (i2c_pca_add_numbered_bus(&i2c->adap) < 0) {
+		ret = -ENODEV;
+		goto e_adapt;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	printk(KERN_INFO "%s registered.\n", i2c->adap.name);
+
+	return 0;
+
+e_adapt:
+	if (irq)
+		free_irq(irq, i2c);
+e_reqirq:
+	if (i2c->gpio > -1)
+		gpio_free(i2c->gpio);

drivers/i2c/busses/i2c-pca-platform.c line 251

Quoted 10 lines

drivers/i2c/busses/i2c-pca-platform.c new file +10 -0

+
+	iounmap(i2c->reg_base);
+e_remap:
+	kfree(i2c);
+e_alloc:
+	release_mem_region(res->start, res_len(res));
+e_print:
+	printk(KERN_ERR "Registering PCA9564 FAILED! (%d)\n", ret);
+	return ret;
+}

The rest looks OK, so I can fix this myself and queue up your patch for 2.6.26.

Thanks,

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wolfram Sang

author

2008-03-16 14:28 UTC #22 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Quoted 5 lines · Jean Delvare

Quoted 5 lines

drivers/i2c/busses/i2c-pca-platform.c new file +5 -0

+	/* Use gpio_is_valid() when in mainline */
+	if (i2c->gpio > -1)
+		ret = gpio_request(i2c->gpio, i2c->adap.name);
+		if (ret == 0) {
+			gpio_direction_output(i2c->gpio, 1);

Quoted 8 lines

drivers/i2c/busses/i2c-pca-platform.c line 216
Quoted 6 lines
drivers/i2c/busses/i2c-pca-platform.c new file +6 -0
+			i2c->algo_data.reset_chip = i2c_pca_pf_resetchip;
+		} else {
+			printk(KERN_WARNING "%s: Registering gpio failed!\n",
+				i2c->adap.name);
+			i2c->gpio = ret;
+		}
You're missing curly braces around this block, aren't you?

Holy ..., yes, you are right. This is why I used to have braces even around single instructions after if. This is, where CodingStyle gives me most trouble.

Quoted 2 lines

The rest looks OK, so I can fix this myself and queue up your patch for 2.6.26.

Ah, finally! \o/ Thanks a _lot_ for your help, I learnt a lot and this will hopefully show in my next submission :)

Best wishes,

Wolfram

Signature · 2 lines

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

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jean Delvare

2008-03-16 15:44 UTC #24 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Quoted 15 lines · Wolfram Sang

Quoted 5 lines · Jean Delvare
Quoted 5 lines
drivers/i2c/busses/i2c-pca-platform.c new file +5 -0
+	/* Use gpio_is_valid() when in mainline */
+	if (i2c->gpio > -1)
+		ret = gpio_request(i2c->gpio, i2c->adap.name);
+		if (ret == 0) {
+			gpio_direction_output(i2c->gpio, 1);
Quoted 8 lines
drivers/i2c/busses/i2c-pca-platform.c line 216
Quoted 6 lines
drivers/i2c/busses/i2c-pca-platform.c new file +6 -0
+			i2c->algo_data.reset_chip = i2c_pca_pf_resetchip;
+		} else {
+			printk(KERN_WARNING "%s: Registering gpio failed!\n",
+				i2c->adap.name);
+			i2c->gpio = ret;
+		}
You're missing curly braces around this block, aren't you?

Quoted 3 lines

Holy ..., yes, you are right. This is why I used to have braces even around single instructions after if. This is, where CodingStyle gives me most trouble.

I can't agree more... This type of warning I am carefully ignoring when checkpatch.pl complains. In many cases, the extra braces don't hurt and protect us against future mistakes.

Quoted 5 lines

Quoted 2 lines
The rest looks OK, so I can fix this myself and queue up your patch for 2.6.26.
Ah, finally! \o/ Thanks a _lot_ for your help, I learnt a lot and this will hopefully show in my next submission :)

You're welcome. It has been a pleasure to work with you and I am looking forward to your next submissions!

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

Trent Piepho

2008-03-16 19:55 UTC #26 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

Quoted 16 lines · Jean Delvare

Quoted 15 lines · Wolfram Sang

Quoted 13 lines · Jean Delvare

Review

Quoted 11 lines

(quoted diff) +11 -0

+	/* Use gpio_is_valid() when in mainline */
+	if (i2c->gpio > -1)
+		ret = gpio_request(i2c->gpio, i2c->adap.name);
+		if (ret == 0) {
+			gpio_direction_output(i2c->gpio, 1);
+			i2c->algo_data.reset_chip = i2c_pca_pf_resetchip;
+		} else {
+			printk(KERN_WARNING "%s: Registering gpio failed!\n",
+				i2c->adap.name);
+			i2c->gpio = ret;
+		}

You're missing curly braces around this block, aren't you?

Quoted 7 lines

Quoted 3 lines
Holy ..., yes, you are right. This is why I used to have braces even around single instructions after if. This is, where CodingStyle gives me most trouble.
I can't agree more... This type of warning I am carefully ignoring when checkpatch.pl complains. In many cases, the extra braces don't hurt and protect us against future mistakes.

I've been bit by this exact same problem enough times that I've come to the same conclusion. Adding a second statement to a one-statement if and then forgetting to add the brances almost always produces code that compiles without warnings and almost works. It ends up being very hard to track down, and even when you do, it's easy to stare at right at the bug and not see the missing braces.

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

Jean Delvare

2008-03-12 10:43 UTC #27 lore raw

Re: [PATCHv2 3/3] Add platform driver on top of the new pca-algorithm

In reply to a message not in the archive

Hi Wolfram:

Just noticed the following warnings on x86_64:
drivers/i2c/busses/i2c-pca-platform.c: In function "i2c_pca_pf_probe":
drivers/i2c/busses/i2c-pca-platform.c:181: warning: format "%08x" expects type "unsigned int", but argument 2 has type "resource_size_t" drivers/i2c/busses/i2c-pca-platform.c:186: warning: format "%08x" expects type "unsigned int", but argument 4 has type "resource_size_t" drivers/i2c/busses/i2c-pca-platform.c:186: warning: format "%08x" expects type "unsigned int", but argument 4 has type "resource_size_t"

Please fix in next iteration.

Signature · 6 lines

Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c