[patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b]

From: Jochen Friedrich <jochen-NIgtFMG+Po8@public.gmane.org>

This driver uses the port of 2.4 code from Vitaly Bordug and the actual
algorithm used by the i2c driver of the DBox code on cvs.tuxboc.org from
Felix Domke and Gillem converted to an of_platform_driver.  Tested on CPM1
(MPC823 on dbox2 hardware) and CPM2 (MPC8272).

Signed-off-by: Jochen Friedrich <jochen-NIgtFMG+Po8@public.gmane.org>
Cc: Vitaly Bordug <vitb-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: Felix Domke <tmbinc-hi6Y0CQ0nG0@public.gmane.org>
Cc: <htoa-hi6Y0CQ0nG0@public.gmane.org>
Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
---

 drivers/i2c/busses/Kconfig   |   10 
 drivers/i2c/busses/Makefile  |    1 
 drivers/i2c/busses/i2c-cpm.c |  728 +++++++++++++++++++++++++++++++++
 3 files changed, 739 insertions(+)

diff -puN drivers/i2c/busses/Kconfig~i2c-add-support-for-i2c-bus-on-freescale-cpm1-cpm2-controllers drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig~i2c-add-support-for-i2c-bus-on-freescale-cpm1-cpm2-controllers
+++ a/drivers/i2c/busses/Kconfig
@@ -116,6 +116,16 @@ config I2C_BLACKFIN_TWI_CLK_KHZ
 	help
 	  The unit of the TWI clock is kHz.
 
+config I2C_CPM
+	tristate "Freescale CPM1 or CPM2 (MPC8xx/826x)"
+	depends on (CPM1 || CPM2) && OF_I2C
+	help
+	  This supports the use of the I2C interface on Freescale
+	  processors with CPM1 or CPM2.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-cpm.
+
 config I2C_DAVINCI
 	tristate "DaVinci I2C driver"
 	depends on ARCH_DAVINCI
diff -puN drivers/i2c/busses/Makefile~i2c-add-support-for-i2c-bus-on-freescale-cpm1-cpm2-controllers drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile~i2c-add-support-for-i2c-bus-on-freescale-cpm1-cpm2-controllers
+++ a/drivers/i2c/busses/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
+obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
diff -puN /dev/null drivers/i2c/busses/i2c-cpm.c
--- /dev/null
+++ a/drivers/i2c/busses/i2c-cpm.c
@@ -0,0 +1,728 @@
+/*
+ * Freescale CPM1/CPM2 I2C interface.
+ * Copyright (c) 1999 Dan Malek (dmalek-/sdJLaoWuQc@public.gmane.org).
+ *
+ * moved into proper i2c interface;
+ * Brad Parker (brad-k3bCrp9g88tBDgjK7y7TUQ@public.gmane.org)
+ *
+ * Parts from dbox2_i2c.c (cvs.tuxbox.org)
+ * (C) 2000-2001 Felix Domke (tmbinc-hi6Y0CQ0nG0@public.gmane.org), Gillem (htoa-hi6Y0CQ0nG0@public.gmane.org)
+ *
+ * (C) 2007 Montavista Software, Inc.
+ * Vitaly Bordug <vitb-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
+ *
+ * Converted to of_platform_device. Renamed to i2c-cpm.c.
+ * (C) 2007,2008 Jochen Friedrich <jochen-NIgtFMG+Po8@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
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/stddef.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/dma-mapping.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_i2c.h>
+#include <sysdev/fsl_soc.h>
+#include <asm/cpm.h>
+
+/* Try to define this if you have an older CPU (earlier than rev D4) */
+/* However, better use a GPIO based bitbang driver in this case :/   */
+#undef	I2C_CHIP_ERRATA
+
+#define CPM_MAX_READ    513
+#define CPM_MAXBD       4
+
+#define I2C_EB			(0x10) /* Big endian mode */
+#define I2C_EB_CPM2		(0x30) /* Big endian mode, memory snoop */
+
+#define DPRAM_BASE		((u8 __iomem __force *)cpm_muram_addr(0))
+
+/* I2C parameter RAM. */
+struct i2c_ram {
+	ushort  rbase;		/* Rx Buffer descriptor base address */
+	ushort  tbase;		/* Tx Buffer descriptor base address */
+	u_char  rfcr;		/* Rx function code */
+	u_char  tfcr;		/* Tx function code */
+	ushort  mrblr;		/* Max receive buffer length */
+	uint    rstate;		/* Internal */
+	uint    rdp;		/* Internal */
+	ushort  rbptr;		/* Rx Buffer descriptor pointer */
+	ushort  rbc;		/* Internal */
+	uint    rxtmp;		/* Internal */
+	uint    tstate;		/* Internal */
+	uint    tdp;		/* Internal */
+	ushort  tbptr;		/* Tx Buffer descriptor pointer */
+	ushort  tbc;		/* Internal */
+	uint    txtmp;		/* Internal */
+	char    res1[4];	/* Reserved */
+	ushort  rpbase;		/* Relocation pointer */
+	char    res2[2];	/* Reserved */
+};
+
+#define I2COM_START	0x80
+#define I2COM_MASTER	0x01
+#define I2CER_TXE	0x10
+#define I2CER_BUSY	0x04
+#define I2CER_TXB	0x02
+#define I2CER_RXB	0x01
+#define I2MOD_EN	0x01
+
+/* I2C Registers */
+struct i2c_reg {
+	u8	i2mod;
+	u8	res1[3];
+	u8	i2add;
+	u8	res2[3];
+	u8	i2brg;
+	u8	res3[3];
+	u8	i2com;
+	u8	res4[3];
+	u8	i2cer;
+	u8	res5[3];
+	u8	i2cmr;
+};
+
+struct cpm_i2c {
+	char *base;
+	struct of_device *ofdev;
+	struct i2c_adapter adap;
+	uint dp_addr;
+	int version; /* CPM1=1, CPM2=2 */
+	int irq;
+	int cp_command;
+	struct i2c_reg __iomem *i2c_reg;
+	struct i2c_ram __iomem *i2c_ram;
+	u16 i2c_addr;
+	wait_queue_head_t i2c_wait;
+	cbd_t __iomem *tbase;
+	cbd_t __iomem *rbase;
+	u_char *txbuf[CPM_MAXBD];
+	u_char *rxbuf[CPM_MAXBD];
+	u32 txdma[CPM_MAXBD];
+	u32 rxdma[CPM_MAXBD];
+};
+
+static irqreturn_t cpm_i2c_interrupt(int irq, void *dev_id)
+{
+	struct cpm_i2c *cpm;
+	struct i2c_reg __iomem *i2c_reg;
+	struct i2c_adapter *adap = dev_id;
+	int i;
+
+	cpm = i2c_get_adapdata(dev_id);
+	i2c_reg = cpm->i2c_reg;
+
+	/* Clear interrupt. */
+	i = in_8(&i2c_reg->i2cer);
+	out_8(&i2c_reg->i2cer, i);
+
+	dev_dbg(&adap->dev, "Interrupt: %x\n", i);
+
+	/* Get me going again. */
+	wake_up_interruptible(&cpm->i2c_wait);
+
+	return i ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void cpm_reset_i2c_params(struct cpm_i2c *cpm)
+{
+	struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
+
+	/* Set up the IIC parameters in the parameter ram. */
+	out_be16(&i2c_ram->tbase, (u8 __iomem *)cpm->tbase - DPRAM_BASE);
+	out_be16(&i2c_ram->rbase, (u8 __iomem *)cpm->rbase - DPRAM_BASE);
+
+	if (cpm->version == 1) {
+		out_8(&i2c_ram->tfcr, I2C_EB);
+		out_8(&i2c_ram->rfcr, I2C_EB);
+	} else {
+		out_8(&i2c_ram->tfcr, I2C_EB_CPM2);
+		out_8(&i2c_ram->rfcr, I2C_EB_CPM2);
+	}
+
+	out_be16(&i2c_ram->mrblr, CPM_MAX_READ);
+
+	out_be32(&i2c_ram->rstate, 0);
+	out_be32(&i2c_ram->rdp, 0);
+	out_be16(&i2c_ram->rbptr, 0);
+	out_be16(&i2c_ram->rbc, 0);
+	out_be32(&i2c_ram->rxtmp, 0);
+	out_be32(&i2c_ram->tstate, 0);
+	out_be32(&i2c_ram->tdp, 0);
+	out_be16(&i2c_ram->tbptr, 0);
+	out_be16(&i2c_ram->tbc, 0);
+	out_be32(&i2c_ram->txtmp, 0);
+}
+
+static void cpm_i2c_force_close(struct i2c_adapter *adap)
+{
+	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
+	struct i2c_reg __iomem *i2c_reg = cpm->i2c_reg;
+
+	dev_dbg(&adap->dev, "cpm_i2c_force_close()\n");
+
+	cpm_command(cpm->cp_command, CPM_CR_CLOSE_RX_BD);
+
+	out_8(&i2c_reg->i2cmr, 0x00);	/* Disable all interrupts */
+	out_8(&i2c_reg->i2cer, 0xff);
+}
+
+static void cpm_i2c_parse_message(struct i2c_adapter *adap,
+	struct i2c_msg *pmsg, int num, int tx, int rx)
+{
+	cbd_t __iomem *tbdf;
+	cbd_t __iomem *rbdf;
+	u_char addr;
+	u_char *tb;
+	u_char *rb;
+	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
+
+	tbdf = cpm->tbase + tx;
+	rbdf = cpm->rbase + rx;
+
+	addr = pmsg->addr << 1;
+	if (pmsg->flags & I2C_M_RD)
+		addr |= 1;
+
+	tb = cpm->txbuf[tx];
+	rb = cpm->rxbuf[rx];
+
+	/* Align read buffer */
+	rb = (u_char *) (((ulong) rb + 1) & ~1);
+
+	tb[0] = addr;		/* Device address byte w/rw flag */
+
+	out_be16(&tbdf->cbd_datlen, pmsg->len + 1);
+	out_be16(&tbdf->cbd_sc, 0);
+
+	if (!(pmsg->flags & I2C_M_NOSTART))
+		setbits16(&tbdf->cbd_sc, BD_I2C_START);
+
+	if (tx + 1 == num)
+		setbits16(&tbdf->cbd_sc, BD_SC_LAST | BD_SC_WRAP);
+
+	if (pmsg->flags & I2C_M_RD) {
+		/*
+		 * To read, we need an empty buffer of the proper length.
+		 * All that is used is the first byte for address, the remainder
+		 * is just used for timing (and doesn't really have to exist).
+		 */
+
+		dev_dbg(&adap->dev, "cpm_i2c_read(abyte=0x%x)\n", addr);
+
+		out_be16(&rbdf->cbd_datlen, 0);
+		out_be16(&rbdf->cbd_sc, BD_SC_EMPTY | BD_SC_INTRPT);
+
+		if (rx + 1 == CPM_MAXBD)
+			setbits16(&rbdf->cbd_sc, BD_SC_WRAP);
+
+		eieio();
+		setbits16(&tbdf->cbd_sc, BD_SC_READY);
+	} else {
+		dev_dbg(&adap->dev, "cpm_iic_write(abyte=0x%x)\n", addr);
+
+		memcpy(tb+1, pmsg->buf, pmsg->len);
+
+		eieio();
+		setbits16(&tbdf->cbd_sc, BD_SC_READY | BD_SC_INTRPT);
+	}
+}
+
+static int cpm_i2c_check_message(struct i2c_adapter *adap,
+	struct i2c_msg *pmsg, int tx, int rx)
+{
+	cbd_t __iomem *tbdf;
+	cbd_t __iomem *rbdf;
+	u_char *tb;
+	u_char *rb;
+	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
+
+	tbdf = cpm->tbase + tx;
+	rbdf = cpm->rbase + rx;
+
+	tb = cpm->txbuf[tx];
+	rb = cpm->rxbuf[rx];
+
+	/* Align read buffer */
+	rb = (u_char *) (((uint) rb + 1) & ~1);
+
+	eieio();
+	if (pmsg->flags & I2C_M_RD) {
+		dev_dbg(&adap->dev, "tx sc 0x%04x, rx sc 0x%04x\n",
+			in_be16(&tbdf->cbd_sc), in_be16(&rbdf->cbd_sc));
+
+		if (in_be16(&tbdf->cbd_sc) & BD_SC_NAK) {
+			dev_err(&adap->dev, "IIC read; No ack\n");
+			return -EIO;
+		}
+		if (in_be16(&rbdf->cbd_sc) & BD_SC_EMPTY) {
+			dev_err(&adap->dev,
+				"IIC read; complete but rbuf empty\n");
+			return -EREMOTEIO;
+		}
+		if (in_be16(&rbdf->cbd_sc) & BD_SC_OV) {
+			dev_err(&adap->dev, "IIC read; Overrun\n");
+			return -EREMOTEIO;
+		}
+		memcpy(pmsg->buf, rb, pmsg->len);
+	} else {
+		dev_dbg(&adap->dev, "tx sc %d 0x%04x\n", tx,
+			in_be16(&tbdf->cbd_sc));
+
+		if (in_be16(&tbdf->cbd_sc) & BD_SC_NAK) {
+			dev_err(&adap->dev, "IIC write; No ack\n");
+			return -EIO;
+		}
+		if (in_be16(&tbdf->cbd_sc) & BD_SC_UN) {
+			dev_err(&adap->dev, "IIC write; Underrun\n");
+			return -EIO;
+		}
+		if (in_be16(&tbdf->cbd_sc) & BD_SC_CL) {
+			dev_err(&adap->dev, "IIC write; Collision\n");
+			return -EIO;
+		}
+	}
+	return 0;
+}
+
+static int cpm_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
+	struct i2c_reg __iomem *i2c_reg = cpm->i2c_reg;
+	struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
+	struct i2c_msg *pmsg;
+	int ret, i;
+	int tptr;
+	int rptr;
+	cbd_t __iomem *tbdf;
+	cbd_t __iomem *rbdf;
+
+	if (num > CPM_MAXBD)
+		return -EINVAL;
+
+	/* Check if we have any oversized READ requests */
+	for (i = 0; i < num; i++) {
+		pmsg = &msgs[i];
+		if (pmsg->len >= CPM_MAX_READ)
+			return -EINVAL;
+	}
+
+	/* Reset to use first buffer */
+	out_be16(&i2c_ram->rbptr, in_be16(&i2c_ram->rbase));
+	out_be16(&i2c_ram->tbptr, in_be16(&i2c_ram->tbase));
+
+	tbdf = cpm->tbase;
+	rbdf = cpm->rbase;
+
+	tptr = 0;
+	rptr = 0;
+
+	while (tptr < num) {
+		pmsg = &msgs[tptr];
+		dev_dbg(&adap->dev, "R: %d T: %d\n", rptr, tptr);
+
+		cpm_i2c_parse_message(adap, pmsg, num, tptr, rptr);
+		if (pmsg->flags & I2C_M_RD)
+			rptr++;
+		tptr++;
+	}
+	/* Start transfer now */
+	/* Enable RX/TX/Error interupts */
+	out_8(&i2c_reg->i2cmr, I2CER_BUSY | I2CER_TXB | I2CER_RXB);
+	out_8(&i2c_reg->i2cer, 0xff);	/* Clear interrupt status */
+	/* Chip bug, set enable here */
+	setbits8(&i2c_reg->i2mod, I2MOD_EN);	/* Enable */
+	/* Begin transmission */
+	setbits8(&i2c_reg->i2com, I2COM_MASTER);
+
+	tptr = 0;
+	rptr = 0;
+
+	while (tptr < num) {
+		/* Check for outstanding messages */
+		dev_dbg(&adap->dev, "test ready.\n");
+		pmsg = &msgs[tptr];
+		if (pmsg->flags & I2C_M_RD)
+			ret = wait_event_interruptible_timeout(cpm->i2c_wait,
+				!(in_be16(&rbdf[rptr].cbd_sc) & BD_SC_EMPTY),
+				1 * HZ);
+		else
+			ret = wait_event_interruptible_timeout(cpm->i2c_wait,
+				!(in_be16(&tbdf[tptr].cbd_sc) & BD_SC_READY),
+				1 * HZ);
+		if (ret == 0) {
+			ret = -EREMOTEIO;
+			dev_dbg(&adap->dev, "I2C read: timeout!\n");
+			goto out_err;
+		}
+		if (ret > 0) {
+			dev_dbg(&adap->dev, "ready.\n");
+			ret = cpm_i2c_check_message(adap, pmsg, tptr, rptr);
+			tptr++;
+			if (pmsg->flags & I2C_M_RD)
+				rptr++;
+			if (ret)
+				goto out_err;
+		}
+	}
+#ifdef I2C_CHIP_ERRATA
+	/*
+	 * Chip errata, clear enable. This is not needed on rev D4 CPUs.
+	 * Disabling I2C too early may cause too short stop condition
+	 */
+	udelay(4);
+	clrbits8(&i2c_reg->i2mod, I2MOD_EN);
+#endif
+	return (num);
+
+out_err:
+	cpm_i2c_force_close(adap);
+#ifdef I2C_CHIP_ERRATA
+	/*
+	 * Chip errata, clear enable. This is not needed on rev D4 CPUs.
+	 */
+	clrbits8(&i2c_reg->i2mod, I2MOD_EN);
+#endif
+	return ret;
+}
+
+static u32 cpm_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+}
+
+/* -----exported algorithm data: -------------------------------------	*/
+
+static const struct i2c_algorithm cpm_i2c_algo = {
+	.master_xfer = cpm_i2c_xfer,
+	.functionality = cpm_i2c_func,
+};
+
+static const struct i2c_adapter cpm_ops = {
+	.owner		= THIS_MODULE,
+	.name		= "i2c-cpm",
+	.algo		= &cpm_i2c_algo,
+	.class		= I2C_CLASS_HWMON,
+};
+
+static int __devinit cpm_i2c_setup(struct cpm_i2c *cpm)
+{
+	struct of_device *ofdev = cpm->ofdev;
+	const u32 *data;
+	int len, ret, i;
+	void __iomem *i2c_base;
+	cbd_t __iomem *tbdf;
+	cbd_t __iomem *rbdf;
+	unsigned char brg;
+
+	dev_dbg(&cpm->ofdev->dev, "cpm_i2c_setup()\n");
+
+	init_waitqueue_head(&cpm->i2c_wait);
+
+	cpm->irq = of_irq_to_resource(ofdev->node, 0, NULL);
+	if (cpm->irq == NO_IRQ)
+		return -EINVAL;
+
+	/* Install interrupt handler. */
+	ret = request_irq(cpm->irq, cpm_i2c_interrupt, 0, "cpm_i2c",
+			  &cpm->adap);
+	if (ret)
+		goto out_irq;
+
+	/* IIC parameter RAM */
+	i2c_base = of_iomap(ofdev->node, 1);
+	if (i2c_base == NULL) {
+		ret = -EINVAL;
+		goto out_irq;
+	}
+
+	if (of_device_is_compatible(ofdev->node, "fsl,cpm1-i2c")) {
+
+		/* Check for and use a microcode relocation patch. */
+		cpm->i2c_ram = i2c_base;
+		cpm->i2c_addr = in_be16(&cpm->i2c_ram->rpbase);
+
+		/*
+		 * Maybe should use cpm_muram_alloc instead of hardcoding
+		 * this in micropatch.c
+		 */
+		if (cpm->i2c_addr) {
+			cpm->i2c_ram = cpm_muram_addr(cpm->i2c_addr);
+			iounmap(i2c_base);
+		}
+
+		cpm->version = 1;
+
+	} else if (of_device_is_compatible(ofdev->node, "fsl,cpm2-i2c")) {
+		cpm->i2c_addr = cpm_muram_alloc(sizeof(struct i2c_ram), 64);
+		cpm->i2c_ram = cpm_muram_addr(cpm->i2c_addr);
+		out_be16(i2c_base, cpm->i2c_addr);
+		iounmap(i2c_base);
+
+		cpm->version = 2;
+
+	} else {
+		iounmap(i2c_base);
+		ret = -EINVAL;
+		goto out_irq;
+	}
+
+	/* I2C control/status registers */
+	cpm->i2c_reg = of_iomap(ofdev->node, 0);
+	if (cpm->i2c_reg == NULL) {
+		ret = -EINVAL;
+		goto out_ram;
+	}
+
+	data = of_get_property(ofdev->node, "fsl,cpm-command", &len);
+	if (!data || len != 4) {
+		ret = -EINVAL;
+		goto out_reg;
+	}
+	cpm->cp_command = *data;
+
+	data = of_get_property(ofdev->node, "linux,i2c-class", &len);
+	if (data && len == 4)
+		cpm->adap.class = *data;
+
+	/*
+	 * Allocate space for CPM_MAXBD transmit and receive buffer
+	 * descriptors in the DP ram.
+	 */
+	cpm->dp_addr = cpm_muram_alloc(sizeof(cbd_t) * 2 * CPM_MAXBD, 8);
+	if (!cpm->dp_addr) {
+		ret = -ENOMEM;
+		goto out_reg;
+	}
+
+	cpm->tbase = cpm_muram_addr(cpm->dp_addr);
+	cpm->rbase = cpm_muram_addr(cpm->dp_addr + sizeof(cbd_t) * CPM_MAXBD);
+
+	/* Allocate TX and RX buffers */
+
+	tbdf = cpm->tbase;
+	rbdf = cpm->rbase;
+
+	for (i = 0; i < CPM_MAXBD; i++) {
+		cpm->rxbuf[i] = dma_alloc_coherent(
+			NULL, CPM_MAX_READ + 1, &cpm->rxdma[i], GFP_KERNEL);
+		if (!cpm->rxbuf[i]) {
+			ret = -ENOMEM;
+			goto out_muram;
+		}
+		out_be32(&rbdf[i].cbd_bufaddr, ((cpm->rxdma[i] + 1) & ~1));
+
+		cpm->txbuf[i] = (unsigned char *)dma_alloc_coherent(
+			NULL, CPM_MAX_READ + 1, &cpm->txdma[i], GFP_KERNEL);
+		if (!cpm->txbuf[i]) {
+			ret = -ENOMEM;
+			goto out_muram;
+		}
+		out_be32(&tbdf[i].cbd_bufaddr, cpm->txdma[i]);
+	}
+
+	/* Initialize Tx/Rx parameters. */
+
+	cpm_reset_i2c_params(cpm);
+
+	dev_dbg(&cpm->ofdev->dev, "i2c_ram %p, i2c_addr 0x%04x\n",
+		cpm->i2c_ram, cpm->i2c_addr);
+	dev_dbg(&cpm->ofdev->dev, "tbase 0x%04x, rbase 0x%04x\n",
+		(u8 __iomem *)cpm->tbase - DPRAM_BASE,
+		(u8 __iomem *)cpm->rbase - DPRAM_BASE);
+
+	cpm_command(cpm->cp_command, CPM_CR_INIT_TRX);
+
+	/*
+	 * Select an invalid address. Just make sure we don't use loopback mode
+	 */
+	out_8(&cpm->i2c_reg->i2add, 0xfe);
+
+	/* Make clock run at 60 kHz. */
+
+	brg = get_brgfreq() / (32 * 2 * 60000) - 3;
+	out_8(&cpm->i2c_reg->i2brg, brg);
+
+	out_8(&cpm->i2c_reg->i2mod, 0x00);
+	out_8(&cpm->i2c_reg->i2com, I2COM_MASTER);	/* Master mode */
+
+	/* Disable interrupts. */
+	out_8(&cpm->i2c_reg->i2cmr, 0);
+	out_8(&cpm->i2c_reg->i2cer, 0xff);
+
+	return 0;
+
+out_muram:
+	for (i = 0; i < CPM_MAXBD; i++) {
+		if (cpm->rxbuf[i])
+			dma_free_coherent(NULL, CPM_MAX_READ + 1,
+				cpm->rxbuf[i], cpm->rxdma[i]);
+		if (cpm->txbuf[i])
+			dma_free_coherent(NULL, CPM_MAX_READ + 1,
+				cpm->txbuf[i], cpm->txdma[i]);
+	}
+	cpm_muram_free(cpm->dp_addr);
+out_reg:
+	iounmap(cpm->i2c_reg);
+out_ram:
+	if ((cpm->version == 1) && (!cpm->i2c_addr))
+		iounmap(cpm->i2c_ram);
+	if (cpm->version == 2)
+		cpm_muram_free(cpm->i2c_addr);
+out_irq:
+	free_irq(cpm->irq, &cpm->adap);
+	return ret;
+}
+
+static void cpm_i2c_shutdown(struct cpm_i2c *cpm)
+{
+	int i;
+
+	/* Shut down I2C. */
+	clrbits8(&cpm->i2c_reg->i2mod, I2MOD_EN);
+
+	/* Disable interrupts */
+	out_8(&cpm->i2c_reg->i2cmr, 0);
+	out_8(&cpm->i2c_reg->i2cer, 0xff);
+
+	free_irq(cpm->irq, &cpm->adap);
+
+	/* Free all memory */
+	for (i = 0; i < CPM_MAXBD; i++) {
+		dma_free_coherent(NULL, CPM_MAX_READ + 1,
+			cpm->rxbuf[i], cpm->rxdma[i]);
+		dma_free_coherent(NULL, CPM_MAX_READ + 1,
+			cpm->txbuf[i], cpm->txdma[i]);
+	}
+
+	cpm_muram_free(cpm->dp_addr);
+	iounmap(cpm->i2c_reg);
+
+	if ((cpm->version == 1) && (!cpm->i2c_addr))
+		iounmap(cpm->i2c_ram);
+	if (cpm->version == 2)
+		cpm_muram_free(cpm->i2c_addr);
+
+	return;
+}
+
+static int __devinit cpm_i2c_probe(struct of_device *ofdev,
+			 const struct of_device_id *match)
+{
+	int result;
+	struct cpm_i2c *cpm;
+
+	cpm = kzalloc(sizeof(struct cpm_i2c), GFP_KERNEL);
+	if (!cpm)
+		return -ENOMEM;
+
+	cpm->ofdev = ofdev;
+
+	dev_set_drvdata(&ofdev->dev, cpm);
+
+	cpm->adap = cpm_ops;
+	i2c_set_adapdata(&cpm->adap, cpm);
+	cpm->adap.dev.parent = &ofdev->dev;
+
+	result = cpm_i2c_setup(cpm);
+	if (result) {
+		dev_err(&ofdev->dev, "Unable to init hardware\n");
+		goto out_free;
+	}
+
+	/* register new adapter to i2c module... */
+
+	result = i2c_add_adapter(&cpm->adap);
+	if (result < 0) {
+		dev_err(&ofdev->dev, "Unable to register with I2C\n");
+		goto out_shut;
+	}
+
+	dev_dbg(&ofdev->dev, "hw routines for %s registered.\n",
+		cpm->adap.name);
+
+	/*
+	 * register OF I2C devices
+	 */
+	of_register_i2c_devices(&cpm->adap, ofdev->node);
+
+	return 0;
+out_shut:
+	cpm_i2c_shutdown(cpm);
+out_free:
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(cpm);
+
+	return result;
+}
+
+static int __devexit cpm_i2c_remove(struct of_device *ofdev)
+{
+	struct cpm_i2c *cpm = dev_get_drvdata(&ofdev->dev);
+
+	i2c_del_adapter(&cpm->adap);
+
+	cpm_i2c_shutdown(cpm);
+
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(cpm);
+
+	return 0;
+}
+
+static const struct of_device_id cpm_i2c_match[] = {
+	{
+		.compatible = "fsl,cpm-i2c",
+	},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, cpm_i2c_match);
+
+static struct of_platform_driver cpm_i2c_driver = {
+	.match_table	= cpm_i2c_match,
+	.probe		= cpm_i2c_probe,
+	.remove		= __devexit_p(cpm_i2c_remove),
+	.driver		= {
+		.name	= "fsl-i2c-cpm",
+		.owner	= THIS_MODULE,
+	}
+};
+
+static int __init cpm_i2c_init(void)
+{
+	return of_register_platform_driver(&cpm_i2c_driver);
+}
+
+static void __exit cpm_i2c_exit(void)
+{
+	of_unregister_platform_driver(&cpm_i2c_driver);
+}
+
+module_init(cpm_i2c_init);
+module_exit(cpm_i2c_exit);
+
+MODULE_AUTHOR("Jochen Friedrich <jochen-NIgtFMG+Po8@public.gmane.org>");
+MODULE_DESCRIPTION("I2C-Bus adapter routines for CPM boards");
+MODULE_LICENSE("GPL");
_

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1992 bytes --]

On Wed, May 14, 2008 at 04:14:45PM -0700, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org wrote:

I just managed to test the latest version of this driver.

> +static int cpm_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
> +	struct i2c_reg __iomem *i2c_reg = cpm->i2c_reg;
> +	struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
> +	struct i2c_msg *pmsg;
> +	int ret, i;
> +	int tptr;
> +	int rptr;
> +	cbd_t __iomem *tbdf;
> +	cbd_t __iomem *rbdf;
> +
> +	if (num > CPM_MAXBD)
> +		return -EINVAL;
> +
> +	/* Check if we have any oversized READ requests */
> +	for (i = 0; i < num; i++) {
> +		pmsg = &msgs[i];
> +		if (pmsg->len >= CPM_MAX_READ)
> +			return -EINVAL;
> +	}
> +
> +	/* Reset to use first buffer */
> +	out_be16(&i2c_ram->rbptr, in_be16(&i2c_ram->rbase));
> +	out_be16(&i2c_ram->tbptr, in_be16(&i2c_ram->tbase));
> +
> +	tbdf = cpm->tbase;
> +	rbdf = cpm->rbase;
> +
> +	tptr = 0;
> +	rptr = 0;
> +
> +	while (tptr < num) {
> +		pmsg = &msgs[tptr];
> +		dev_dbg(&adap->dev, "R: %d T: %d\n", rptr, tptr);
> +
> +		cpm_i2c_parse_message(adap, pmsg, num, tptr, rptr);
> +		if (pmsg->flags & I2C_M_RD)
> +			rptr++;
> +		tptr++;
> +	}
> +	/* Start transfer now */
> +	/* Enable RX/TX/Error interupts */
> +	out_8(&i2c_reg->i2cmr, I2CER_BUSY | I2CER_TXB | I2CER_RXB);
> +	out_8(&i2c_reg->i2cer, 0xff);	/* Clear interrupt status */
> +	/* Chip bug, set enable here */
> +	setbits8(&i2c_reg->i2mod, I2MOD_EN);	/* Enable */
> +	/* Begin transmission */
> +	setbits8(&i2c_reg->i2com, I2COM_MASTER);
Bummer! This should not be I2COM_MASTER but I2COM_START! Otherwise no
I2C communication will happen.

After applying this change, the driver works fine on an MPC8260 (like
the previous version did).

All the best,

   Wolfram

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- 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 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Jean Delvare]

Hi Wolfram,

On Fri, 16 May 2008 10:37:43 +0200, Wolfram Sang wrote:
> On Wed, May 14, 2008 at 04:14:45PM -0700, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org wrote:
> 
> I just managed to test the latest version of this driver.
> 
> > +static int cpm_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> > +{
> > +	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
> > +	struct i2c_reg __iomem *i2c_reg = cpm->i2c_reg;
> > +	struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
> > +	struct i2c_msg *pmsg;
> > +	int ret, i;
> > +	int tptr;
> > +	int rptr;
> > +	cbd_t __iomem *tbdf;
> > +	cbd_t __iomem *rbdf;
> > +
> > +	if (num > CPM_MAXBD)
> > +		return -EINVAL;
> > +
> > +	/* Check if we have any oversized READ requests */
> > +	for (i = 0; i < num; i++) {
> > +		pmsg = &msgs[i];
> > +		if (pmsg->len >= CPM_MAX_READ)
> > +			return -EINVAL;
> > +	}
> > +
> > +	/* Reset to use first buffer */
> > +	out_be16(&i2c_ram->rbptr, in_be16(&i2c_ram->rbase));
> > +	out_be16(&i2c_ram->tbptr, in_be16(&i2c_ram->tbase));
> > +
> > +	tbdf = cpm->tbase;
> > +	rbdf = cpm->rbase;
> > +
> > +	tptr = 0;
> > +	rptr = 0;
> > +
> > +	while (tptr < num) {
> > +		pmsg = &msgs[tptr];
> > +		dev_dbg(&adap->dev, "R: %d T: %d\n", rptr, tptr);
> > +
> > +		cpm_i2c_parse_message(adap, pmsg, num, tptr, rptr);
> > +		if (pmsg->flags & I2C_M_RD)
> > +			rptr++;
> > +		tptr++;
> > +	}
> > +	/* Start transfer now */
> > +	/* Enable RX/TX/Error interupts */
> > +	out_8(&i2c_reg->i2cmr, I2CER_BUSY | I2CER_TXB | I2CER_RXB);
> > +	out_8(&i2c_reg->i2cer, 0xff);	/* Clear interrupt status */
> > +	/* Chip bug, set enable here */
> > +	setbits8(&i2c_reg->i2mod, I2MOD_EN);	/* Enable */
> > +	/* Begin transmission */
> > +	setbits8(&i2c_reg->i2com, I2COM_MASTER);
>
> Bummer! This should not be I2COM_MASTER but I2COM_START! Otherwise no
> I2C communication will happen.
> 
> After applying this change, the driver works fine on an MPC8260 (like
> the previous version did).

Would you have the possibility to review and ack this patch (with the
suggested fix above, of course)? I'd like to include it in my i2c tree
now as it has been around for some time but I'm a bit short of time to
review it and I also can't test it. If you can do that then I'll trust
your review and take the patch quickly. With the time saved, I could,
say, review the at24c driver ;)

Thanks,
-- 
Jean Delvare

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Jochen Friedrich]

Hi Jean,

>> Bummer! This should not be I2COM_MASTER but I2COM_START! Otherwise no
>> I2C communication will happen.
>>
>> After applying this change, the driver works fine on an MPC8260 (like
>> the previous version did).
> 
> Would you have the possibility to review and ack this patch (with the
> suggested fix above, of course)?

I'd like to add yet another small change:

It doesn't make sense to add fsl,cpm-i2c as second compatible entry in the
device tree just for loading the driver. The match table can use
fsl,cpm1-i2c and fsl,cpm2-i2c directly.

Thanks,
Jochen

diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
index 2bd0512..7c0f6d4 100644
--- a/drivers/i2c/busses/i2c-cpm.c
+++ b/drivers/i2c/busses/i2c-cpm.c
@@ -693,7 +693,10 @@ static int __devexit cpm_i2c_remove(struct of_device *ofdev)
 
 static const struct of_device_id cpm_i2c_match[] = {
 	{
-		.compatible = "fsl,cpm-i2c",
+		.compatible = "fsl,cpm1-i2c",
+	},
+	{
+		.compatible = "fsl,cpm2-i2c",
 	},
 	{},
 };

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1018 bytes --]

On Fri, May 16, 2008 at 09:08:18PM +0200, Jean Delvare wrote:
> Would you have the possibility to review and ack this patch (with the
> suggested fix above, of course)? I'd like to include it in my i2c tree
> now as it has been around for some time but I'm a bit short of time to
> review it and I also can't test it. If you can do that then I'll trust
> your review and take the patch quickly.
Well, I could review it and give further testing on Monday. Still, I
wouldn't call my experiences in the linux-kernelspace (yet) big enough
to trust only this review. Still, thanks for your trust :)

> With the time saved, I could, say, review the at24c driver ;)
Hehe, nice offer. Though, I'd suggest to wait for the upcoming version
of the at24-driver, which will include proper device table support. I
already made up my mind how to implement it, just have to get hacking.

   Wolfram

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- 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 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 25110 bytes --]

On Wed, May 14, 2008 at 04:14:45PM -0700, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org wrote:

Here is a more detailed review of this driver. After applying my fix, it
worked fine on a MPC8260 + X24645 (EEPROM) + LM84 (Sensor) + RS5C372
(RTC). I use this and the previous version for more than two weeks on a
daily basis now.

> From: Jochen Friedrich <jochen-NIgtFMG+Po8@public.gmane.org>
> 
> This driver uses the port of 2.4 code from Vitaly Bordug and the actual
> algorithm used by the i2c driver of the DBox code on cvs.tuxboc.org from
> Felix Domke and Gillem converted to an of_platform_driver.  Tested on CPM1
> (MPC823 on dbox2 hardware) and CPM2 (MPC8272).
I think a short summary of what changed since the last revision would be
helpful. I see, for example, that you removed a mutex and I assume that
its functionality is now handled at another layer. A comment would be
helpful, me thinks.

> 
> Signed-off-by: Jochen Friedrich <jochen-NIgtFMG+Po8@public.gmane.org>
> Cc: Vitaly Bordug <vitb-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Felix Domke <tmbinc-hi6Y0CQ0nG0@public.gmane.org>
> Cc: <htoa-hi6Y0CQ0nG0@public.gmane.org>
> Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> ---
> 
>  drivers/i2c/busses/Kconfig   |   10 
>  drivers/i2c/busses/Makefile  |    1 
>  drivers/i2c/busses/i2c-cpm.c |  728 +++++++++++++++++++++++++++++++++
>  3 files changed, 739 insertions(+)
> 
> diff -puN drivers/i2c/busses/Kconfig~i2c-add-support-for-i2c-bus-on-freescale-cpm1-cpm2-controllers drivers/i2c/busses/Kconfig
> --- a/drivers/i2c/busses/Kconfig~i2c-add-support-for-i2c-bus-on-freescale-cpm1-cpm2-controllers
> +++ a/drivers/i2c/busses/Kconfig
> @@ -116,6 +116,16 @@ config I2C_BLACKFIN_TWI_CLK_KHZ
>  	help
>  	  The unit of the TWI clock is kHz.
>  
> +config I2C_CPM
> +	tristate "Freescale CPM1 or CPM2 (MPC8xx/826x)"
> +	depends on (CPM1 || CPM2) && OF_I2C
> +	help
> +	  This supports the use of the I2C interface on Freescale
> +	  processors with CPM1 or CPM2.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-cpm.
> +
>  config I2C_DAVINCI
>  	tristate "DaVinci I2C driver"
>  	depends on ARCH_DAVINCI
> diff -puN drivers/i2c/busses/Makefile~i2c-add-support-for-i2c-bus-on-freescale-cpm1-cpm2-controllers drivers/i2c/busses/Makefile
> --- a/drivers/i2c/busses/Makefile~i2c-add-support-for-i2c-bus-on-freescale-cpm1-cpm2-controllers
> +++ a/drivers/i2c/busses/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111
>  obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
>  obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
>  obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
> +obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
>  obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
>  obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
>  obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
> diff -puN /dev/null drivers/i2c/busses/i2c-cpm.c
> --- /dev/null
> +++ a/drivers/i2c/busses/i2c-cpm.c
> @@ -0,0 +1,728 @@
> +/*
> + * Freescale CPM1/CPM2 I2C interface.
> + * Copyright (c) 1999 Dan Malek (dmalek-/sdJLaoWuQc@public.gmane.org).
> + *
> + * moved into proper i2c interface;
> + * Brad Parker (brad-k3bCrp9g88tBDgjK7y7TUQ@public.gmane.org)
> + *
> + * Parts from dbox2_i2c.c (cvs.tuxbox.org)
> + * (C) 2000-2001 Felix Domke (tmbinc-hi6Y0CQ0nG0@public.gmane.org), Gillem (htoa-hi6Y0CQ0nG0@public.gmane.org)
> + *
> + * (C) 2007 Montavista Software, Inc.
> + * Vitaly Bordug <vitb-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> + *
> + * Converted to of_platform_device. Renamed to i2c-cpm.c.
> + * (C) 2007,2008 Jochen Friedrich <jochen-NIgtFMG+Po8@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
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/errno.h>
> +#include <linux/stddef.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_i2c.h>
> +#include <sysdev/fsl_soc.h>
> +#include <asm/cpm.h>
> +
> +/* Try to define this if you have an older CPU (earlier than rev D4) */
> +/* However, better use a GPIO based bitbang driver in this case :/   */
> +#undef	I2C_CHIP_ERRATA
> +
> +#define CPM_MAX_READ    513
> +#define CPM_MAXBD       4
> +
> +#define I2C_EB			(0x10) /* Big endian mode */
> +#define I2C_EB_CPM2		(0x30) /* Big endian mode, memory snoop */
> +
> +#define DPRAM_BASE		((u8 __iomem __force *)cpm_muram_addr(0))
> +
> +/* I2C parameter RAM. */
> +struct i2c_ram {
> +	ushort  rbase;		/* Rx Buffer descriptor base address */
> +	ushort  tbase;		/* Tx Buffer descriptor base address */
> +	u_char  rfcr;		/* Rx function code */
> +	u_char  tfcr;		/* Tx function code */
> +	ushort  mrblr;		/* Max receive buffer length */
> +	uint    rstate;		/* Internal */
> +	uint    rdp;		/* Internal */
> +	ushort  rbptr;		/* Rx Buffer descriptor pointer */
> +	ushort  rbc;		/* Internal */
> +	uint    rxtmp;		/* Internal */
> +	uint    tstate;		/* Internal */
> +	uint    tdp;		/* Internal */
> +	ushort  tbptr;		/* Tx Buffer descriptor pointer */
> +	ushort  tbc;		/* Internal */
> +	uint    txtmp;		/* Internal */
> +	char    res1[4];	/* Reserved */
> +	ushort  rpbase;		/* Relocation pointer */
> +	char    res2[2];	/* Reserved */
> +};
> +
> +#define I2COM_START	0x80
> +#define I2COM_MASTER	0x01
> +#define I2CER_TXE	0x10
> +#define I2CER_BUSY	0x04
> +#define I2CER_TXB	0x02
> +#define I2CER_RXB	0x01
> +#define I2MOD_EN	0x01
> +
> +/* I2C Registers */
> +struct i2c_reg {
> +	u8	i2mod;
> +	u8	res1[3];
> +	u8	i2add;
> +	u8	res2[3];
> +	u8	i2brg;
> +	u8	res3[3];
> +	u8	i2com;
> +	u8	res4[3];
> +	u8	i2cer;
> +	u8	res5[3];
> +	u8	i2cmr;
> +};
> +
> +struct cpm_i2c {
> +	char *base;
> +	struct of_device *ofdev;
> +	struct i2c_adapter adap;
> +	uint dp_addr;
> +	int version; /* CPM1=1, CPM2=2 */
> +	int irq;
> +	int cp_command;
> +	struct i2c_reg __iomem *i2c_reg;
> +	struct i2c_ram __iomem *i2c_ram;
> +	u16 i2c_addr;
> +	wait_queue_head_t i2c_wait;
> +	cbd_t __iomem *tbase;
> +	cbd_t __iomem *rbase;
> +	u_char *txbuf[CPM_MAXBD];
> +	u_char *rxbuf[CPM_MAXBD];
> +	u32 txdma[CPM_MAXBD];
> +	u32 rxdma[CPM_MAXBD];
> +};
> +
> +static irqreturn_t cpm_i2c_interrupt(int irq, void *dev_id)
> +{
> +	struct cpm_i2c *cpm;
> +	struct i2c_reg __iomem *i2c_reg;
> +	struct i2c_adapter *adap = dev_id;
> +	int i;
> +
> +	cpm = i2c_get_adapdata(dev_id);
> +	i2c_reg = cpm->i2c_reg;
> +
> +	/* Clear interrupt. */
> +	i = in_8(&i2c_reg->i2cer);
> +	out_8(&i2c_reg->i2cer, i);
> +
> +	dev_dbg(&adap->dev, "Interrupt: %x\n", i);
> +
> +	/* Get me going again. */
I doubt the usefulness of this comment a bit :)

> +	wake_up_interruptible(&cpm->i2c_wait);
> +
> +	return i ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static void cpm_reset_i2c_params(struct cpm_i2c *cpm)
> +{
> +	struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
> +
> +	/* Set up the IIC parameters in the parameter ram. */
Minor nit, in most cases within the driver I2C was preferred to IIC. Is
there a preferred way, Jean?

> +	out_be16(&i2c_ram->tbase, (u8 __iomem *)cpm->tbase - DPRAM_BASE);
> +	out_be16(&i2c_ram->rbase, (u8 __iomem *)cpm->rbase - DPRAM_BASE);
> +
> +	if (cpm->version == 1) {
> +		out_8(&i2c_ram->tfcr, I2C_EB);
> +		out_8(&i2c_ram->rfcr, I2C_EB);
> +	} else {
> +		out_8(&i2c_ram->tfcr, I2C_EB_CPM2);
> +		out_8(&i2c_ram->rfcr, I2C_EB_CPM2);
> +	}
> +
> +	out_be16(&i2c_ram->mrblr, CPM_MAX_READ);
> +
> +	out_be32(&i2c_ram->rstate, 0);
> +	out_be32(&i2c_ram->rdp, 0);
> +	out_be16(&i2c_ram->rbptr, 0);
> +	out_be16(&i2c_ram->rbc, 0);
> +	out_be32(&i2c_ram->rxtmp, 0);
> +	out_be32(&i2c_ram->tstate, 0);
> +	out_be32(&i2c_ram->tdp, 0);
> +	out_be16(&i2c_ram->tbptr, 0);
> +	out_be16(&i2c_ram->tbc, 0);
> +	out_be32(&i2c_ram->txtmp, 0);
> +}
> +
> +static void cpm_i2c_force_close(struct i2c_adapter *adap)
> +{
> +	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
> +	struct i2c_reg __iomem *i2c_reg = cpm->i2c_reg;
> +
> +	dev_dbg(&adap->dev, "cpm_i2c_force_close()\n");
> +
> +	cpm_command(cpm->cp_command, CPM_CR_CLOSE_RX_BD);
> +
> +	out_8(&i2c_reg->i2cmr, 0x00);	/* Disable all interrupts */
> +	out_8(&i2c_reg->i2cer, 0xff);
> +}
> +
> +static void cpm_i2c_parse_message(struct i2c_adapter *adap,
> +	struct i2c_msg *pmsg, int num, int tx, int rx)
> +{
> +	cbd_t __iomem *tbdf;
> +	cbd_t __iomem *rbdf;
> +	u_char addr;
> +	u_char *tb;
> +	u_char *rb;
> +	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
> +
> +	tbdf = cpm->tbase + tx;
> +	rbdf = cpm->rbase + rx;
> +
> +	addr = pmsg->addr << 1;
> +	if (pmsg->flags & I2C_M_RD)
> +		addr |= 1;
> +
> +	tb = cpm->txbuf[tx];
> +	rb = cpm->rxbuf[rx];
> +
> +	/* Align read buffer */
> +	rb = (u_char *) (((ulong) rb + 1) & ~1);
> +
> +	tb[0] = addr;		/* Device address byte w/rw flag */
> +
> +	out_be16(&tbdf->cbd_datlen, pmsg->len + 1);
> +	out_be16(&tbdf->cbd_sc, 0);
> +
> +	if (!(pmsg->flags & I2C_M_NOSTART))
> +		setbits16(&tbdf->cbd_sc, BD_I2C_START);
> +
> +	if (tx + 1 == num)
> +		setbits16(&tbdf->cbd_sc, BD_SC_LAST | BD_SC_WRAP);
> +
> +	if (pmsg->flags & I2C_M_RD) {
> +		/*
> +		 * To read, we need an empty buffer of the proper length.
> +		 * All that is used is the first byte for address, the remainder
> +		 * is just used for timing (and doesn't really have to exist).
> +		 */
> +
> +		dev_dbg(&adap->dev, "cpm_i2c_read(abyte=0x%x)\n", addr);
> +
> +		out_be16(&rbdf->cbd_datlen, 0);
> +		out_be16(&rbdf->cbd_sc, BD_SC_EMPTY | BD_SC_INTRPT);
> +
> +		if (rx + 1 == CPM_MAXBD)
> +			setbits16(&rbdf->cbd_sc, BD_SC_WRAP);
> +
> +		eieio();
> +		setbits16(&tbdf->cbd_sc, BD_SC_READY);
> +	} else {
> +		dev_dbg(&adap->dev, "cpm_iic_write(abyte=0x%x)\n", addr);
> +
> +		memcpy(tb+1, pmsg->buf, pmsg->len);
> +
> +		eieio();
> +		setbits16(&tbdf->cbd_sc, BD_SC_READY | BD_SC_INTRPT);
> +	}
> +}
> +
> +static int cpm_i2c_check_message(struct i2c_adapter *adap,
> +	struct i2c_msg *pmsg, int tx, int rx)
> +{
> +	cbd_t __iomem *tbdf;
> +	cbd_t __iomem *rbdf;
> +	u_char *tb;
> +	u_char *rb;
> +	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
> +
> +	tbdf = cpm->tbase + tx;
> +	rbdf = cpm->rbase + rx;
> +
> +	tb = cpm->txbuf[tx];
> +	rb = cpm->rxbuf[rx];
> +
> +	/* Align read buffer */
> +	rb = (u_char *) (((uint) rb + 1) & ~1);
> +
> +	eieio();
> +	if (pmsg->flags & I2C_M_RD) {
> +		dev_dbg(&adap->dev, "tx sc 0x%04x, rx sc 0x%04x\n",
> +			in_be16(&tbdf->cbd_sc), in_be16(&rbdf->cbd_sc));
> +
> +		if (in_be16(&tbdf->cbd_sc) & BD_SC_NAK) {
> +			dev_err(&adap->dev, "IIC read; No ack\n");
> +			return -EIO;
> +		}
> +		if (in_be16(&rbdf->cbd_sc) & BD_SC_EMPTY) {
> +			dev_err(&adap->dev,
> +				"IIC read; complete but rbuf empty\n");
> +			return -EREMOTEIO;
> +		}
> +		if (in_be16(&rbdf->cbd_sc) & BD_SC_OV) {
> +			dev_err(&adap->dev, "IIC read; Overrun\n");
> +			return -EREMOTEIO;
> +		}
> +		memcpy(pmsg->buf, rb, pmsg->len);
> +	} else {
> +		dev_dbg(&adap->dev, "tx sc %d 0x%04x\n", tx,
> +			in_be16(&tbdf->cbd_sc));
> +
> +		if (in_be16(&tbdf->cbd_sc) & BD_SC_NAK) {
> +			dev_err(&adap->dev, "IIC write; No ack\n");
> +			return -EIO;
> +		}
> +		if (in_be16(&tbdf->cbd_sc) & BD_SC_UN) {
> +			dev_err(&adap->dev, "IIC write; Underrun\n");
> +			return -EIO;
> +		}
> +		if (in_be16(&tbdf->cbd_sc) & BD_SC_CL) {
> +			dev_err(&adap->dev, "IIC write; Collision\n");
> +			return -EIO;
> +		}

The dev_err-statements are too strong, IMHO. For example, the
at24-driver tries to write as fast as possible and may recieve a NACK,
then it will wait a bit and retry. I wouldn't call this NACK an error
then. I also wonder if it is worth a warning, as there is a timeout
message later on, which will be printed as dev_dbg only. As other
drivers I glimpsed at also don't write anything on NACK, maybe dev_dbg
consistency would be preferable.

> +	}
> +	return 0;
> +}
> +
> +static int cpm_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct cpm_i2c *cpm = i2c_get_adapdata(adap);
> +	struct i2c_reg __iomem *i2c_reg = cpm->i2c_reg;
> +	struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
> +	struct i2c_msg *pmsg;
> +	int ret, i;
> +	int tptr;
> +	int rptr;
> +	cbd_t __iomem *tbdf;
> +	cbd_t __iomem *rbdf;
> +
> +	if (num > CPM_MAXBD)
> +		return -EINVAL;
> +
> +	/* Check if we have any oversized READ requests */
> +	for (i = 0; i < num; i++) {
> +		pmsg = &msgs[i];
> +		if (pmsg->len >= CPM_MAX_READ)
> +			return -EINVAL;
> +	}
> +
> +	/* Reset to use first buffer */
> +	out_be16(&i2c_ram->rbptr, in_be16(&i2c_ram->rbase));
> +	out_be16(&i2c_ram->tbptr, in_be16(&i2c_ram->tbase));
> +
> +	tbdf = cpm->tbase;
> +	rbdf = cpm->rbase;
> +
> +	tptr = 0;
> +	rptr = 0;
> +
> +	while (tptr < num) {
> +		pmsg = &msgs[tptr];
> +		dev_dbg(&adap->dev, "R: %d T: %d\n", rptr, tptr);
> +
> +		cpm_i2c_parse_message(adap, pmsg, num, tptr, rptr);
> +		if (pmsg->flags & I2C_M_RD)
> +			rptr++;
> +		tptr++;
> +	}
> +	/* Start transfer now */
> +	/* Enable RX/TX/Error interupts */
> +	out_8(&i2c_reg->i2cmr, I2CER_BUSY | I2CER_TXB | I2CER_RXB);
> +	out_8(&i2c_reg->i2cer, 0xff);	/* Clear interrupt status */
> +	/* Chip bug, set enable here */
> +	setbits8(&i2c_reg->i2mod, I2MOD_EN);	/* Enable */
> +	/* Begin transmission */
> +	setbits8(&i2c_reg->i2com, I2COM_MASTER);
As already said, this must be I2COM_START instead of I2COM_MASTER!

> +
> +	tptr = 0;
> +	rptr = 0;
> +
> +	while (tptr < num) {
> +		/* Check for outstanding messages */
> +		dev_dbg(&adap->dev, "test ready.\n");
> +		pmsg = &msgs[tptr];
> +		if (pmsg->flags & I2C_M_RD)
> +			ret = wait_event_interruptible_timeout(cpm->i2c_wait,
> +				!(in_be16(&rbdf[rptr].cbd_sc) & BD_SC_EMPTY),
> +				1 * HZ);
> +		else
> +			ret = wait_event_interruptible_timeout(cpm->i2c_wait,
> +				!(in_be16(&tbdf[tptr].cbd_sc) & BD_SC_READY),
> +				1 * HZ);
> +		if (ret == 0) {
> +			ret = -EREMOTEIO;
> +			dev_dbg(&adap->dev, "I2C read: timeout!\n");
"I2C transfer: timeout" might be more apropriate.

> +			goto out_err;
> +		}
> +		if (ret > 0) {
> +			dev_dbg(&adap->dev, "ready.\n");
> +			ret = cpm_i2c_check_message(adap, pmsg, tptr, rptr);
> +			tptr++;
> +			if (pmsg->flags & I2C_M_RD)
> +				rptr++;
> +			if (ret)
> +				goto out_err;
> +		}
> +	}
> +#ifdef I2C_CHIP_ERRATA
> +	/*
> +	 * Chip errata, clear enable. This is not needed on rev D4 CPUs.
> +	 * Disabling I2C too early may cause too short stop condition
> +	 */
> +	udelay(4);
> +	clrbits8(&i2c_reg->i2mod, I2MOD_EN);
> +#endif
> +	return (num);
> +
> +out_err:
> +	cpm_i2c_force_close(adap);
> +#ifdef I2C_CHIP_ERRATA
> +	/*
> +	 * Chip errata, clear enable. This is not needed on rev D4 CPUs.
> +	 */
> +	clrbits8(&i2c_reg->i2mod, I2MOD_EN);
> +#endif
> +	return ret;
> +}
> +
> +static u32 cpm_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +}
> +
> +/* -----exported algorithm data: -------------------------------------	*/
> +
> +static const struct i2c_algorithm cpm_i2c_algo = {
> +	.master_xfer = cpm_i2c_xfer,
> +	.functionality = cpm_i2c_func,
> +};
> +
> +static const struct i2c_adapter cpm_ops = {
> +	.owner		= THIS_MODULE,
> +	.name		= "i2c-cpm",
> +	.algo		= &cpm_i2c_algo,
> +	.class		= I2C_CLASS_HWMON,
> +};
> +
> +static int __devinit cpm_i2c_setup(struct cpm_i2c *cpm)
> +{
> +	struct of_device *ofdev = cpm->ofdev;
> +	const u32 *data;
> +	int len, ret, i;
> +	void __iomem *i2c_base;
> +	cbd_t __iomem *tbdf;
> +	cbd_t __iomem *rbdf;
> +	unsigned char brg;
> +
> +	dev_dbg(&cpm->ofdev->dev, "cpm_i2c_setup()\n");
> +
> +	init_waitqueue_head(&cpm->i2c_wait);
> +
> +	cpm->irq = of_irq_to_resource(ofdev->node, 0, NULL);
> +	if (cpm->irq == NO_IRQ)
> +		return -EINVAL;
> +
> +	/* Install interrupt handler. */
> +	ret = request_irq(cpm->irq, cpm_i2c_interrupt, 0, "cpm_i2c",
> +			  &cpm->adap);
> +	if (ret)
> +		goto out_irq;
return ret? (as request_irq failed, no need to call free_irq).

> +
> +	/* IIC parameter RAM */
> +	i2c_base = of_iomap(ofdev->node, 1);
> +	if (i2c_base == NULL) {
> +		ret = -EINVAL;
> +		goto out_irq;
> +	}
> +
> +	if (of_device_is_compatible(ofdev->node, "fsl,cpm1-i2c")) {
> +
> +		/* Check for and use a microcode relocation patch. */
> +		cpm->i2c_ram = i2c_base;
> +		cpm->i2c_addr = in_be16(&cpm->i2c_ram->rpbase);
> +
> +		/*
> +		 * Maybe should use cpm_muram_alloc instead of hardcoding
> +		 * this in micropatch.c
> +		 */
> +		if (cpm->i2c_addr) {
> +			cpm->i2c_ram = cpm_muram_addr(cpm->i2c_addr);
> +			iounmap(i2c_base);
> +		}
> +
> +		cpm->version = 1;
> +
> +	} else if (of_device_is_compatible(ofdev->node, "fsl,cpm2-i2c")) {
> +		cpm->i2c_addr = cpm_muram_alloc(sizeof(struct i2c_ram), 64);
> +		cpm->i2c_ram = cpm_muram_addr(cpm->i2c_addr);
> +		out_be16(i2c_base, cpm->i2c_addr);
> +		iounmap(i2c_base);
> +
> +		cpm->version = 2;
> +
> +	} else {
> +		iounmap(i2c_base);
> +		ret = -EINVAL;
> +		goto out_irq;
> +	}
> +
> +	/* I2C control/status registers */
> +	cpm->i2c_reg = of_iomap(ofdev->node, 0);
> +	if (cpm->i2c_reg == NULL) {
> +		ret = -EINVAL;
> +		goto out_ram;
> +	}
> +
> +	data = of_get_property(ofdev->node, "fsl,cpm-command", &len);
> +	if (!data || len != 4) {
> +		ret = -EINVAL;
> +		goto out_reg;
> +	}
> +	cpm->cp_command = *data;
> +
> +	data = of_get_property(ofdev->node, "linux,i2c-class", &len);
> +	if (data && len == 4)
> +		cpm->adap.class = *data;
> +
> +	/*
> +	 * Allocate space for CPM_MAXBD transmit and receive buffer
> +	 * descriptors in the DP ram.
> +	 */
> +	cpm->dp_addr = cpm_muram_alloc(sizeof(cbd_t) * 2 * CPM_MAXBD, 8);
> +	if (!cpm->dp_addr) {
> +		ret = -ENOMEM;
> +		goto out_reg;
> +	}
> +
> +	cpm->tbase = cpm_muram_addr(cpm->dp_addr);
> +	cpm->rbase = cpm_muram_addr(cpm->dp_addr + sizeof(cbd_t) * CPM_MAXBD);
> +
> +	/* Allocate TX and RX buffers */
> +
> +	tbdf = cpm->tbase;
> +	rbdf = cpm->rbase;
> +
> +	for (i = 0; i < CPM_MAXBD; i++) {
> +		cpm->rxbuf[i] = dma_alloc_coherent(
> +			NULL, CPM_MAX_READ + 1, &cpm->rxdma[i], GFP_KERNEL);
> +		if (!cpm->rxbuf[i]) {
> +			ret = -ENOMEM;
> +			goto out_muram;
> +		}
> +		out_be32(&rbdf[i].cbd_bufaddr, ((cpm->rxdma[i] + 1) & ~1));
> +
> +		cpm->txbuf[i] = (unsigned char *)dma_alloc_coherent(
> +			NULL, CPM_MAX_READ + 1, &cpm->txdma[i], GFP_KERNEL);
> +		if (!cpm->txbuf[i]) {
> +			ret = -ENOMEM;
> +			goto out_muram;
> +		}
> +		out_be32(&tbdf[i].cbd_bufaddr, cpm->txdma[i]);
> +	}
> +
> +	/* Initialize Tx/Rx parameters. */
> +
> +	cpm_reset_i2c_params(cpm);
> +
> +	dev_dbg(&cpm->ofdev->dev, "i2c_ram %p, i2c_addr 0x%04x\n",
> +		cpm->i2c_ram, cpm->i2c_addr);
> +	dev_dbg(&cpm->ofdev->dev, "tbase 0x%04x, rbase 0x%04x\n",
> +		(u8 __iomem *)cpm->tbase - DPRAM_BASE,
> +		(u8 __iomem *)cpm->rbase - DPRAM_BASE);
> +
> +	cpm_command(cpm->cp_command, CPM_CR_INIT_TRX);
> +
> +	/*
> +	 * Select an invalid address. Just make sure we don't use loopback mode
> +	 */
> +	out_8(&cpm->i2c_reg->i2add, 0xfe);

Very minor; I'd still prefer '0x7f << 1' to 0xfe.

> +
> +	/* Make clock run at 60 kHz. */
> +
> +	brg = get_brgfreq() / (32 * 2 * 60000) - 3;

Thanks to the original specs, this formula is a bit confusing. Is 32
coming from the "standard" brg divider and the 50% duty cycle? If so, I
fail to see a way to make it more readable, but I can confirm the
result.

> +	out_8(&cpm->i2c_reg->i2brg, brg);
> +
> +	out_8(&cpm->i2c_reg->i2mod, 0x00);
> +	out_8(&cpm->i2c_reg->i2com, I2COM_MASTER);	/* Master mode */
> +
> +	/* Disable interrupts. */
> +	out_8(&cpm->i2c_reg->i2cmr, 0);
> +	out_8(&cpm->i2c_reg->i2cer, 0xff);
> +
> +	return 0;
> +
> +out_muram:
> +	for (i = 0; i < CPM_MAXBD; i++) {
> +		if (cpm->rxbuf[i])
> +			dma_free_coherent(NULL, CPM_MAX_READ + 1,
> +				cpm->rxbuf[i], cpm->rxdma[i]);
> +		if (cpm->txbuf[i])
> +			dma_free_coherent(NULL, CPM_MAX_READ + 1,
> +				cpm->txbuf[i], cpm->txdma[i]);
> +	}
> +	cpm_muram_free(cpm->dp_addr);
> +out_reg:
> +	iounmap(cpm->i2c_reg);
> +out_ram:
> +	if ((cpm->version == 1) && (!cpm->i2c_addr))
> +		iounmap(cpm->i2c_ram);
> +	if (cpm->version == 2)
> +		cpm_muram_free(cpm->i2c_addr);
> +out_irq:
> +	free_irq(cpm->irq, &cpm->adap);
> +	return ret;
> +}
> +
> +static void cpm_i2c_shutdown(struct cpm_i2c *cpm)
> +{
> +	int i;
> +
> +	/* Shut down I2C. */
> +	clrbits8(&cpm->i2c_reg->i2mod, I2MOD_EN);
> +
> +	/* Disable interrupts */
> +	out_8(&cpm->i2c_reg->i2cmr, 0);
> +	out_8(&cpm->i2c_reg->i2cer, 0xff);
> +
> +	free_irq(cpm->irq, &cpm->adap);
> +
> +	/* Free all memory */
> +	for (i = 0; i < CPM_MAXBD; i++) {
> +		dma_free_coherent(NULL, CPM_MAX_READ + 1,
> +			cpm->rxbuf[i], cpm->rxdma[i]);
> +		dma_free_coherent(NULL, CPM_MAX_READ + 1,
> +			cpm->txbuf[i], cpm->txdma[i]);
> +	}
> +
> +	cpm_muram_free(cpm->dp_addr);
> +	iounmap(cpm->i2c_reg);
> +
> +	if ((cpm->version == 1) && (!cpm->i2c_addr))
> +		iounmap(cpm->i2c_ram);
> +	if (cpm->version == 2)
> +		cpm_muram_free(cpm->i2c_addr);
> +
> +	return;
return not needed.

> +}
> +
> +static int __devinit cpm_i2c_probe(struct of_device *ofdev,
> +			 const struct of_device_id *match)
> +{
> +	int result;
> +	struct cpm_i2c *cpm;
> +
> +	cpm = kzalloc(sizeof(struct cpm_i2c), GFP_KERNEL);
> +	if (!cpm)
> +		return -ENOMEM;
> +
> +	cpm->ofdev = ofdev;
> +
> +	dev_set_drvdata(&ofdev->dev, cpm);
> +
> +	cpm->adap = cpm_ops;
> +	i2c_set_adapdata(&cpm->adap, cpm);
> +	cpm->adap.dev.parent = &ofdev->dev;
> +
> +	result = cpm_i2c_setup(cpm);
> +	if (result) {
> +		dev_err(&ofdev->dev, "Unable to init hardware\n");
> +		goto out_free;
> +	}
> +
> +	/* register new adapter to i2c module... */
> +
> +	result = i2c_add_adapter(&cpm->adap);
> +	if (result < 0) {
> +		dev_err(&ofdev->dev, "Unable to register with I2C\n");
> +		goto out_shut;
> +	}
> +
> +	dev_dbg(&ofdev->dev, "hw routines for %s registered.\n",
> +		cpm->adap.name);
> +
> +	/*
> +	 * register OF I2C devices
> +	 */
> +	of_register_i2c_devices(&cpm->adap, ofdev->node);
> +
> +	return 0;
> +out_shut:
> +	cpm_i2c_shutdown(cpm);
> +out_free:
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +	kfree(cpm);
> +
> +	return result;
> +}
> +
> +static int __devexit cpm_i2c_remove(struct of_device *ofdev)
> +{
> +	struct cpm_i2c *cpm = dev_get_drvdata(&ofdev->dev);
> +
> +	i2c_del_adapter(&cpm->adap);
> +
> +	cpm_i2c_shutdown(cpm);
> +
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +	kfree(cpm);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id cpm_i2c_match[] = {
> +	{
> +		.compatible = "fsl,cpm-i2c",
Add Jochen's own patch here :)

> +	},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, cpm_i2c_match);
> +
> +static struct of_platform_driver cpm_i2c_driver = {
> +	.match_table	= cpm_i2c_match,
> +	.probe		= cpm_i2c_probe,
> +	.remove		= __devexit_p(cpm_i2c_remove),
> +	.driver		= {
> +		.name	= "fsl-i2c-cpm",
> +		.owner	= THIS_MODULE,
> +	}
> +};
> +
> +static int __init cpm_i2c_init(void)
> +{
> +	return of_register_platform_driver(&cpm_i2c_driver);
> +}
> +
> +static void __exit cpm_i2c_exit(void)
> +{
> +	of_unregister_platform_driver(&cpm_i2c_driver);
> +}
> +
> +module_init(cpm_i2c_init);
> +module_exit(cpm_i2c_exit);
> +
> +MODULE_AUTHOR("Jochen Friedrich <jochen-NIgtFMG+Po8@public.gmane.org>");
> +MODULE_DESCRIPTION("I2C-Bus adapter routines for CPM boards");
> +MODULE_LICENSE("GPL");
> _
> 
> _______________________________________________
> i2c mailing list
> i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
> http://lists.lm-sensors.org/mailman/listinfo/i2c

All the best,

   Wolfram

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- 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 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Jean Delvare]

Hi Wolfram,

On Mon, 19 May 2008 17:54:43 +0200, Wolfram Sang wrote:
> On Wed, May 14, 2008 at 04:14:45PM -0700, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org wrote:
> 
> Here is a more detailed review of this driver. After applying my fix, it
> worked fine on a MPC8260 + X24645 (EEPROM) + LM84 (Sensor) + RS5C372
> (RTC). I use this and the previous version for more than two weeks on a
> daily basis now.

Thanks for the review.

> > +	/* Set up the IIC parameters in the parameter ram. */
>
> Minor nit, in most cases within the driver I2C was preferred to IIC. Is
> there a preferred way, Jean?

"I2C" is preferred.

> > +	if (pmsg->flags & I2C_M_RD) {
> > +		dev_dbg(&adap->dev, "tx sc 0x%04x, rx sc 0x%04x\n",
> > +			in_be16(&tbdf->cbd_sc), in_be16(&rbdf->cbd_sc));
> > +
> > +		if (in_be16(&tbdf->cbd_sc) & BD_SC_NAK) {
> > +			dev_err(&adap->dev, "IIC read; No ack\n");
> > +			return -EIO;
> > +		}
> > +		if (in_be16(&rbdf->cbd_sc) & BD_SC_EMPTY) {
> > +			dev_err(&adap->dev,
> > +				"IIC read; complete but rbuf empty\n");
> > +			return -EREMOTEIO;
> > +		}
> > +		if (in_be16(&rbdf->cbd_sc) & BD_SC_OV) {
> > +			dev_err(&adap->dev, "IIC read; Overrun\n");
> > +			return -EREMOTEIO;
> > +		}
> > +		memcpy(pmsg->buf, rb, pmsg->len);
> > +	} else {
> > +		dev_dbg(&adap->dev, "tx sc %d 0x%04x\n", tx,
> > +			in_be16(&tbdf->cbd_sc));
> > +
> > +		if (in_be16(&tbdf->cbd_sc) & BD_SC_NAK) {
> > +			dev_err(&adap->dev, "IIC write; No ack\n");
> > +			return -EIO;
> > +		}
> > +		if (in_be16(&tbdf->cbd_sc) & BD_SC_UN) {
> > +			dev_err(&adap->dev, "IIC write; Underrun\n");
> > +			return -EIO;
> > +		}
> > +		if (in_be16(&tbdf->cbd_sc) & BD_SC_CL) {
> > +			dev_err(&adap->dev, "IIC write; Collision\n");
> > +			return -EIO;
> > +		}
> 
> The dev_err-statements are too strong, IMHO. For example, the
> at24-driver tries to write as fast as possible and may recieve a NACK,
> then it will wait a bit and retry. I wouldn't call this NACK an error
> then. I also wonder if it is worth a warning, as there is a timeout
> message later on, which will be printed as dev_dbg only. As other
> drivers I glimpsed at also don't write anything on NACK, maybe dev_dbg
> consistency would be preferable.
> 
> > +	}
> > +	return 0;
> > +}

I agree that dev_err() on nack is too strong, most drivers log it at
dev_dbg() level. However I fail to see the relation with timeout? A
nack isn't a timeout. A timeout would be very wrong and should be
reported with dev_err() I think.

Oh, BTW, nacks should be reported with -ENXIO according to:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-document-standard-fault-codes.patch
It might be worth checking that this new driver complies with these
freshly adopted error codes standard.

Thanks,
-- 
Jean Delvare

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Ben Dooks]

On Mon, May 19, 2008 at 06:49:07PM +0200, Jean Delvare wrote:
> Hi Wolfram,
> 
> > The dev_err-statements are too strong, IMHO. For example, the
> > at24-driver tries to write as fast as possible and may recieve a NACK,
> > then it will wait a bit and retry. I wouldn't call this NACK an error
> > then. I also wonder if it is worth a warning, as there is a timeout
> > message later on, which will be printed as dev_dbg only. As other
> > drivers I glimpsed at also don't write anything on NACK, maybe dev_dbg
> > consistency would be preferable.

dev_err() for a number of i2c bus errors are too strong, think about
the case where a system is having i2cprobe run on it (you will get a
lot of errors). It isn't as if the error is lost downstream anyway.

> > > +	return 0;
> > > +}
> 
> I agree that dev_err() on nack is too strong, most drivers log it at
> dev_dbg() level. However I fail to see the relation with timeout? A
> nack isn't a timeout. A timeout would be very wrong and should be
> reported with dev_err() I think.
> 
> Oh, BTW, nacks should be reported with -ENXIO according to:
> http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-document-standard-fault-codes.patch
> It might be worth checking that this new driver complies with these
> freshly adopted error codes standard.

Hmm, where ECONREFUSED or EPIPE (if NAK in already selected device)
entertained?


-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Jean Delvare]

Hi Ben,

On Sun, 1 Jun 2008 23:24:28 +0100, Ben Dooks wrote:
> On Mon, May 19, 2008 at 06:49:07PM +0200, Jean Delvare wrote:
> > Oh, BTW, nacks should be reported with -ENXIO according to:
> > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-document-standard-fault-codes.patch
> > It might be worth checking that this new driver complies with these
> > freshly adopted error codes standard.
> 
> Hmm, where ECONREFUSED or EPIPE (if NAK in already selected device)
> entertained?

There's no such thing as an "already selected device". The bus driver
doesn't know whether a given transaction is meant for probing purposes
(in which case a failure is more or less expected) or if it is a real
transaction. So it must either always log (which can spam the log) or
never log (which can cause some errors to go unnoticed.) Once a bus
driver returns proper error codes, I think that the best approach is to
not log anything (or only at debug level) on NAK and to let the chip
driver deal with the error.

As for the error code, it doesn't matter that much I think, as long as
it is consistent. We've settled for ENXIO and I wouldn't change this
now without a very good reason.

-- 
Jean Delvare

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Ben Dooks]

On Mon, Jun 02, 2008 at 09:08:50AM +0200, Jean Delvare wrote:
> Hi Ben,
> 
> On Sun, 1 Jun 2008 23:24:28 +0100, Ben Dooks wrote:
> > On Mon, May 19, 2008 at 06:49:07PM +0200, Jean Delvare wrote:
> > > Oh, BTW, nacks should be reported with -ENXIO according to:
> > > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-document-standard-fault-codes.patch
> > > It might be worth checking that this new driver complies with these
> > > freshly adopted error codes standard.
> > 
> > Hmm, where ECONREFUSED or EPIPE (if NAK in already selected device)
> > entertained?

[snip]

> As for the error code, it doesn't matter that much I think, as long as
> it is consistent. We've settled for ENXIO and I wouldn't change this
> now without a very good reason.

Sorry, I meant what happens if a NAK is received after the address
part of the i2c_msg has been sent, when sending the msg data? Is this
a case for an error like EPIPE, ENOLINK or EREMOTEIO?

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Jean Delvare]

On Mon, 2 Jun 2008 20:26:30 +0100, Ben Dooks wrote:
> On Mon, Jun 02, 2008 at 09:08:50AM +0200, Jean Delvare wrote:
> > Hi Ben,
> > 
> > On Sun, 1 Jun 2008 23:24:28 +0100, Ben Dooks wrote:
> > > On Mon, May 19, 2008 at 06:49:07PM +0200, Jean Delvare wrote:
> > > > Oh, BTW, nacks should be reported with -ENXIO according to:
> > > > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-document-standard-fault-codes.patch
> > > > It might be worth checking that this new driver complies with these
> > > > freshly adopted error codes standard.
> > > 
> > > Hmm, where ECONREFUSED or EPIPE (if NAK in already selected device)
> > > entertained?
> 
> [snip]
> 
> > As for the error code, it doesn't matter that much I think, as long as
> > it is consistent. We've settled for ENXIO and I wouldn't change this
> > now without a very good reason.
> 
> Sorry, I meant what happens if a NAK is received after the address
> part of the i2c_msg has been sent, when sending the msg data? Is this
> a case for an error like EPIPE, ENOLINK or EREMOTEIO?

Our current error codes document suggests EIO. EPIPE and ENOLINK do not
make much sense IMHO. EREMOTEIO would make some sense, but I suspect
that many drivers won't be able to isolate this specific error and thus
will still return EIO. David, what do you think?

-- 
Jean Delvare

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[David Brownell]

On Monday 02 June 2008, Jean Delvare wrote:
> 
> > Sorry, I meant what happens if a NAK is received after the address
> > part of the i2c_msg has been sent, when sending the msg data? Is this
> > a case for an error like EPIPE, ENOLINK or EREMOTEIO?
> 
> Our current error codes document suggests EIO.

And also says it's sufficiently vague as to make avoiding it
be worthwhile.  :)


> EPIPE and ENOLINK do not 
> make much sense IMHO. EREMOTEIO would make some sense, but I suspect
> that many drivers won't be able to isolate this specific error and thus
> will still return EIO. David, what do you think?

This is one of those cases where the I2C programming interface is
insufficient.  As noted in the comment added to i2c_transfer() by
the patch updating fault code passthrough handling in i2c-core.c:

> >          *  - When we get a NAK after transmitting N bytes to a slave,
> >          *    there is no way to report "N" ... or to let the master
> >          *    continue executing the rest of this combined message, if
> >          *    that's the appropriate response.
 
As I mentioned earlier, the best way to address this issue would
be to carve a 16 bit field out of the existing struct padding and
use that from updated i2c_adapter drivers to report N (actual_len).

The standard way to report short writes is not via errno values...
but by reporting how many bytes were written.  See "man 2 write".
It's not what the I2C stack does now, though.

That could be done if i2c-core got some updates, at least with
any i2c adapters that get updated to better report this fault
(using that new field living in what's now struct padding).

Failing such updates, I'd avoid EREMOTEIO since that's what USB
uses to indicate short *reads* (not writes) when they're flagged
for reporting as errors (not the default).  EFBIG (too big)
would make more sense if short writes must be reported as some
kind of fault.

- Dave

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Ben Dooks]

On Mon, Jun 02, 2008 at 02:03:46PM -0700, David Brownell wrote:
> On Monday 02 June 2008, Jean Delvare wrote:
> > 
> > > Sorry, I meant what happens if a NAK is received after the address
> > > part of the i2c_msg has been sent, when sending the msg data? Is this
> > > a case for an error like EPIPE, ENOLINK or EREMOTEIO?
> > 
> > Our current error codes document suggests EIO.
> 
> And also says it's sufficiently vague as to make avoiding it
> be worthwhile.  :)
> 
> 
> > EPIPE and ENOLINK do not 
> > make much sense IMHO. EREMOTEIO would make some sense, but I suspect
> > that many drivers won't be able to isolate this specific error and thus
> > will still return EIO. David, what do you think?
> 
> This is one of those cases where the I2C programming interface is
> insufficient.  As noted in the comment added to i2c_transfer() by
> the patch updating fault code passthrough handling in i2c-core.c:
> 
> > >          *  - When we get a NAK after transmitting N bytes to a slave,
> > >          *    there is no way to report "N" ... or to let the master
> > >          *    continue executing the rest of this combined message, if
> > >          *    that's the appropriate response.

I thought the i2c_msg flag to ignore NAK was the way to let the master
continue, although this does mean that you loose the fact that a NAK
actually happened.

The case of letting the master continue after such an error still
makes reporting the amount of data written difficult, how do you
indicate I wrote m bytes, got an error, and then continued on to
write all n bytes of the i2c_msgs provided? Can anyone think of
an actual example of where this would be useful?
  
> As I mentioned earlier, the best way to address this issue would
> be to carve a 16 bit field out of the existing struct padding and
> use that from updated i2c_adapter drivers to report N (actual_len).

So you are suggesting adding a done field to the i2c_msg structure
that is updated by the bus driver as it goes along? For arm, this
should fit, but are there any other architectures out there that are
going to be affected by this change in structure (it does, iirc, get
exported to userspace).

If you add a __u16 done field to the i2c_msg, then you either have to
find a special value for 'not filled in' and have the i2c core fill
in the i2c_msg array at start time?

> The standard way to report short writes is not via errno values...
> but by reporting how many bytes were written.  See "man 2 write".
> It's not what the I2C stack does now, though.

Yes, that does make sense to return the amount of data written to
the user, however having a quick look through all users of
i2c_transfer() all just want to know if all the messages where
sent (which is an easier check that to compute the amount of
data in all the messages).

Would it better to add an new i2c_transfer-alike call which would
return the amount of data written, instead of going around modifying
all users of i2c_transfer() ? This would also make transfering from
one call to the other, with i2c_transfer becoming deprecated?

I was just wondering if it would be useful to add an extra field
after the buffer to indicate the error that happened to the message
such as NAK, bus arbitration, etc.
 
> That could be done if i2c-core got some updates, at least with
> any i2c adapters that get updated to better report this fault
> (using that new field living in what's now struct padding).

As noted above, is it padding on _all_ architectures?
 
> Failing such updates, I'd avoid EREMOTEIO since that's what USB
> uses to indicate short *reads* (not writes) when they're flagged
> for reporting as errors (not the default).  EFBIG (too big)
> would make more sense if short writes must be reported as some
> kind of fault.

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Jean Delvare]

Hi Ben, hi David,

On Mon, 2 Jun 2008 23:19:23 +0100, Ben Dooks wrote:
> On Mon, Jun 02, 2008 at 02:03:46PM -0700, David Brownell wrote:
> > On Monday 02 June 2008, Jean Delvare wrote:
> > > 
> > > > Sorry, I meant what happens if a NAK is received after the address
> > > > part of the i2c_msg has been sent, when sending the msg data? Is this
> > > > a case for an error like EPIPE, ENOLINK or EREMOTEIO?
> > > 
> > > Our current error codes document suggests EIO.
> > 
> > And also says it's sufficiently vague as to make avoiding it
> > be worthwhile.  :)
> > 
> > 
> > > EPIPE and ENOLINK do not 
> > > make much sense IMHO. EREMOTEIO would make some sense, but I suspect
> > > that many drivers won't be able to isolate this specific error and thus
> > > will still return EIO. David, what do you think?
> > 
> > This is one of those cases where the I2C programming interface is
> > insufficient.  As noted in the comment added to i2c_transfer() by
> > the patch updating fault code passthrough handling in i2c-core.c:
> > 
> > > >          *  - When we get a NAK after transmitting N bytes to a slave,
> > > >          *    there is no way to report "N" ... or to let the master
> > > >          *    continue executing the rest of this combined message, if
> > > >          *    that's the appropriate response.
> 
> I thought the i2c_msg flag to ignore NAK was the way to let the master
> continue, although this does mean that you loose the fact that a NAK
> actually happened.
> 
> The case of letting the master continue after such an error still
> makes reporting the amount of data written difficult, how do you
> indicate I wrote m bytes, got an error, and then continued on to
> write all n bytes of the i2c_msgs provided? Can anyone think of
> an actual example of where this would be useful?

The "ignore nack" flag is a hack, it's a divergence from the I2C
standard to support a couple broken devices out there. We don't give a
damn to reporting fine-grained errors when using it.

>   
> > As I mentioned earlier, the best way to address this issue would
> > be to carve a 16 bit field out of the existing struct padding and
> > use that from updated i2c_adapter drivers to report N (actual_len).
> 
> So you are suggesting adding a done field to the i2c_msg structure
> that is updated by the bus driver as it goes along? For arm, this
> should fit, but are there any other architectures out there that are
> going to be affected by this change in structure (it does, iirc, get
> exported to userspace).

Yes, it is exported to user-space. But I guess that the binary
compatibility constraints are the same for user-space and kernel-space.

> 
> If you add a __u16 done field to the i2c_msg, then you either have to
> find a special value for 'not filled in' and have the i2c core fill
> in the i2c_msg array at start time?
> 
> > The standard way to report short writes is not via errno values...
> > but by reporting how many bytes were written.  See "man 2 write".
> > It's not what the I2C stack does now, though.
> 
> Yes, that does make sense to return the amount of data written to
> the user, however having a quick look through all users of
> i2c_transfer() all just want to know if all the messages where
> sent (which is an easier check that to compute the amount of
> data in all the messages).
> 
> Would it better to add an new i2c_transfer-alike call which would
> return the amount of data written, instead of going around modifying
> all users of i2c_transfer() ? This would also make transfering from
> one call to the other, with i2c_transfer becoming deprecated?
> 
> I was just wondering if it would be useful to add an extra field
> after the buffer to indicate the error that happened to the message
> such as NAK, bus arbitration, etc.

The main question here is: who needs this? I2C errors don't happen
frequently, and when they happen, how many devices are able to recover
with a partial transaction based on how far the initial transaction
went? I suspect that most devices will need to restart the transaction
from scratch anyway, and for those who could spare a few bytes, it it
really worth it? Driver authors most certainly won't bother.

Given the compatibility constraints, this all looks like a lot of work
and pain for a benefit which is essentially aesthetic. I agree that the
current API isn't nice, but very much doubt that there's anything to
win by reworking it in practice. So I think my time will be much better
used for other matters.

>  
> > That could be done if i2c-core got some updates, at least with
> > any i2c adapters that get updated to better report this fault
> > (using that new field living in what's now struct padding).
> 
> As noted above, is it padding on _all_ architectures?
>  
> > Failing such updates, I'd avoid EREMOTEIO since that's what USB
> > uses to indicate short *reads* (not writes) when they're flagged
> > for reporting as errors (not the default).  EFBIG (too big)
> > would make more sense if short writes must be reported as some
> > kind of fault.
> 


-- 
Jean Delvare

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[David Brownell]

On Tuesday 03 June 2008, Jean Delvare wrote:

> > > This is one of those cases where the I2C programming interface is
> > > insufficient.  As noted in the comment added to i2c_transfer() by
> > > the patch updating fault code passthrough handling in i2c-core.c:
> > > 
> > > > >          *  - When we get a NAK after transmitting N bytes to a slave,
> > > > >          *    there is no way to report "N" ... or to let the master
> > > > >          *    continue executing the rest of this combined message, if
> > > > >          *    that's the appropriate response.
> > 
> > I thought the i2c_msg flag to ignore NAK was the way to let the master
> > continue, although this does mean that you loose the fact that a NAK
> > actually happened.

My understanding is that it's a protocol tweaking option which
would not act that way.


> > The case of letting the master continue after such an error still
> > makes reporting the amount of data written difficult, how do you
> > indicate I wrote m bytes, got an error, and then continued on to
> > write all n bytes of the i2c_msgs provided?

"wrote N bytes then got an error" --> report N, then either
	(a) discard the fault code, which is the typical solution, OR
	(b) also record the fault code

As I commented, recording N is easy since i2c_msg has some bytes
that are currently wasted for use as padding ... on any 32-bit
system using "natural" alignment.  Additionally recording a fault
code could be a problem.  Possibly worth addressing both issues
at the same time, but I'm not sure.


> > Can anyone think of 
> > an actual example of where this would be useful?

I mentioned it because it came up in the context of implementing
the PMBus "group protocol".  Briefly, one I2C combined transaction
sends messages to several slaves ... and they hold off execution
until the terminating STOP.  That achieves synchronized changes;
you may need to coordinate several supply voltage updates.

That clearly opens the door to situations where it might be more
important to ensure that, say, the first and third commands are
both received than to allow a fault on the second command make
the first execute without the third.

In my mind that's more useful as an example of current limitations
(ergo the comment) than something critical to address right now.

However, inability to report N seems more significant to me.
(It's also easier to address.)


> > > As I mentioned earlier, the best way to address this issue would
> > > be to carve a 16 bit field out of the existing struct padding and
> > > use that from updated i2c_adapter drivers to report N (actual_len).
> > 
> > So you are suggesting adding a done field to the i2c_msg structure
> > that is updated by the bus driver as it goes along? For arm, this
> > should fit, but are there any other architectures out there that are
> > going to be affected by this change in structure (it does, iirc, get
> > exported to userspace).
> 
> Yes, it is exported to user-space. But I guess that the binary
> compatibility constraints are the same for user-space and kernel-space.

Well, the struct padding rules are ISTR now incorporated into
the C spec rather than being compiler-specific.  So in that sense
the rules are the same ... both for kernel-space and between
platforms, at least for stuff as simple as i2c_msg.

 
> > If you add a __u16 done field to the i2c_msg, then you either have to
> > find a special value for 'not filled in' and have the i2c core fill
> > in the i2c_msg array at start time?

Simplest would be "__u16 actual_len" which can be initialized to zero.
SMBus "quick command" would be a minor glitch.  Drivers looking at that
would presumably need to cope with adapters that haven't been fixed to
update that field ... e.g. if it comes back as zero, they'll assume
it's the same as the requested length.  (Might be better if i2c-core
patched that and emitted a single runtime warning for that i2c adapter
driver; details TBD.)

(There could be other approaches of course, but that one seems likely
to cause the least overall trouble.)


> > > The standard way to report short writes is not via errno values...
> > > but by reporting how many bytes were written.  See "man 2 write".
> > > It's not what the I2C stack does now, though.
> > 
> > Yes, that does make sense to return the amount of data written to
> > the user, however having a quick look through all users of
> > i2c_transfer() all just want to know if all the messages where
> > sent (which is an easier check that to compute the amount of
> > data in all the messages).

Today all they *can* know is that much!

Even the code which would *want* to report the actual number of
bytes transferred (like i2c-dev, so it can correctly implement
read/write syscall semantics) can only cope within the limits
of the current (deficient) interface.


> > Would it better to add an new i2c_transfer-alike call which would
> > return the amount of data written, instead of going around modifying
> > all users of i2c_transfer() ? This would also make transfering from
> > one call to the other, with i2c_transfer becoming deprecated?
> > 
> > I was just wondering if it would be useful to add an extra field
> > after the buffer to indicate the error that happened to the message
> > such as NAK, bus arbitration, etc.

If we all concur with the observation that this is one of the
types of fault that the current interface hides, the question
becomes what to do about it.  My initial take was to describe
the issue ... so it could be addressed later, as needed.


> The main question here is: who needs this?

i2c-dev doesn't currently implement write()/read() syscalls
correctly for partial writes or reads ... it can't detect
those cases.  (Just one datapoint.)

But the short answer is:  anyone who wants robust code will
have noticed the current interface doesn't support it.


> I2C errors don't happen frequently,

I tend to agree, but that's not unique to I2C.  And it's
orthogonal to the issue of whether it's possible to cope
well with errors when they *do* appear.


> and when they happen, how many devices are able to recover 
> with a partial transaction based on how far the initial transaction
> went? I suspect that most devices will need to restart the transaction
> from scratch anyway, and for those who could spare a few bytes, it it
> really worth it? Driver authors most certainly won't bother.

You're presuming one recovery strategy.  But another issue is
how to tell what actually happened, so that for example the
faults can be logged in enough detail to notice patterns.

 
> Given the compatibility constraints, this all looks like a lot of work
> and pain for a benefit which is essentially aesthetic. I agree that the
> current API isn't nice, but very much doubt that there's anything to
> win by reworking it in practice. So I think my time will be much better
> used for other matters.

Hence my observation that adding paths to report the value of N
are minimal ... no "rework".


> > > That could be done if i2c-core got some updates, at least with
> > > any i2c adapters that get updated to better report this fault
> > > (using that new field living in what's now struct padding).
> > 
> > As noted above, is it padding on _all_ architectures?

I believe so.  "Natural Alignment" requires it exist on 32-bit
CPUs; and even more on 64-bit ones.

  
> > > Failing such updates, I'd avoid EREMOTEIO since that's what USB
> > > uses to indicate short *reads* (not writes) when they're flagged
> > > for reporting as errors (not the default).  EFBIG (too big)
> > > would make more sense if short writes must be reported as some
> > > kind of fault.
> > 
> 
>

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Trent Piepho]

On Mon, 2 Jun 2008, Ben Dooks wrote:
> makes reporting the amount of data written difficult, how do you
> indicate I wrote m bytes, got an error, and then continued on to
> write all n bytes of the i2c_msgs provided? Can anyone think of
> an actual example of where this would be useful?

Useful enough that someone would actually write the code to use it?  No.

> Yes, that does make sense to return the amount of data written to
> the user, however having a quick look through all users of
> i2c_transfer() all just want to know if all the messages where
> sent (which is an easier check that to compute the amount of
> data in all the messages).
>
> Would it better to add an new i2c_transfer-alike call which would
> return the amount of data written, instead of going around modifying
> all users of i2c_transfer() ? This would also make transfering from
> one call to the other, with i2c_transfer becoming deprecated?

My experience with I2C drivers is the same.  No one cares how many bytes
were written before a NAK, they're just going to take same failure path no
matter what (assuming they even check for an error at all).

I think it's enough to document the interface can't provide this
information, and if someone does have a real use for it in the future, let
them add support when the time comes.  There's no point in creating an
interface that no one is going to use.

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Jochen Friedrich]

Hi Wolfram,

> I think a short summary of what changed since the last revision would be
> helpful. I see, for example, that you removed a mutex and I assume that
> its functionality is now handled at another layer. A comment would be
> helpful, me thinks.

The mutex was used to lock the i2c bus, but i2c-core already does this.

> All the best,

Thanks for the review. If you want I can resend...

Jochen.

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

Re: [patch 3/3] i2c: add support for i2c bus on Freescale CPM1/CPM2 controllers

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 315 bytes --]

Hi Jochen,

> Thanks for the review. If you want I can resend...
Please do. And maybe you could comment on my assumption about the value 32 in
the brg-formula?

All the best,

   Wolfram

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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