2008-01-31 12:54:14

by Jochen Friedrich

[permalink] [raw]
Subject: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Using the port of 2.4 code from Vitaly Bordug <[email protected]>
and the actual algorithm used by the i2c driver of the DBox code on
cvs.tuxboc.org from Tmbinc, Gillem ([email protected]). Renamed i2c-rpx.c and
i2c-algo-8xx.c to i2c-cpm.c and converted the driver to an
of_platform_driver.

Signed-off-by: Jochen Friedrich <[email protected]>
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-cpm.c | 759 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 770 insertions(+), 0 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-cpm.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b61f56b..1d18db7 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -114,6 +114,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) && I2C && PPC_OF
+ 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 --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index ea7068f..1291e2b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o
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 --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
new file mode 100644
index 0000000..7191427
--- /dev/null
+++ b/drivers/i2c/busses/i2c-cpm.c
@@ -0,0 +1,759 @@
+/*
+ * Freescale CPM1/CPM2 I2C interface.
+ * Copyright (c) 1999 Dan Malek ([email protected]).
+ *
+ * moved into proper i2c interface;
+ * Brad Parker ([email protected])
+ *
+ * (C) 2007 Montavista Software, Inc.
+ * Vitaly Bordug <[email protected]>
+ *
+ * RPX lite specific parts of the i2c interface
+ * Update: There actually isn't anything RPXLite-specific about this module.
+ * This should work for most any CPM board. The console messages have been
+ * changed to eliminate RPXLite references.
+ *
+ 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.
+ *
+ * moved into proper i2c interface; separated out platform specific
+ * parts into i2c-8xx.c
+ * Brad Parker ([email protected])
+ *
+ * Parts from dbox2_i2c.c (cvs.tuxbox.org)
+ * (C) 2000-2001 Tmbinc, Gillem ([email protected])
+ *
+ * (C) 2007 Montavista Software, Inc.
+ * Vitaly Bordug <[email protected]>
+ *
+ * Converted to of_platform_device. Renamed to i2c-cpm.c.
+ * (C) 2007 Jochen Friedrich <[email protected]>
+ */
+
+#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/sched.h>
+#include <linux/stddef.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/time.h>
+#include <linux/dma-mapping.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <sysdev/fsl_soc.h>
+#include <asm/cpm.h>
+
+/*
+ * Wait for patch from Jon Smirl
+ * #include "powerpc-common.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 CPM_CR_INIT_TRX (0x00)
+#define CPM_CR_CLOSE_RXBD (0x07)
+
+#define I2C_EB (0x10) /* Big endian mode */
+
+/* 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 */
+};
+
+/* 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;
+ struct mutex i2c_mutex; /* Protects I2C CPM */
+ 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, cpm->dp_addr);
+ out_be16(&i2c_ram->rbase, cpm->dp_addr + sizeof(cbd_t) * CPM_MAXBD);
+
+ out_8(&i2c_ram->tfcr, I2C_EB);
+ out_8(&i2c_ram->rfcr, I2C_EB);
+
+ 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, in_be16(&i2c_ram->rbase));
+ 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, in_be16(&i2c_ram->tbase));
+ 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_RXBD);
+
+ 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 *tbdf, *rbdf;
+ u_char addr;
+ u_char *tb;
+ u_char *rb;
+ struct cpm_i2c *cpm = i2c_get_adapdata(adap);
+ struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
+ int i, dscan;
+
+ tbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->tbase));
+ rbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->rbase));
+
+ /*
+ * This chip can't do zero length writes. However, the i2c core uses
+ * them to scan for devices. The best we can do is to convert them
+ * into 1 byte reads
+ */
+
+ dscan = ((pmsg->len == 0) && (num == 1));
+
+ addr = pmsg->addr << 1;
+ if ((pmsg->flags & I2C_M_RD) || dscan)
+ addr |= 1;
+
+ tb = cpm->txbuf[tx];
+ rb = cpm->rxbuf[rx];
+
+ /* Align read buffer */
+ rb = (u_char *) (((ulong) rb + 1) & ~1);
+
+ if ((pmsg->flags & I2C_M_RD) || dscan) {
+ /*
+ * 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).
+ */
+ tb[0] = addr; /* Device address byte w/rw flag */
+
+ dev_dbg(&adap->dev, "cpm_i2c_read(abyte=0x%x)\n", addr);
+
+ if (dscan)
+ tbdf[tx].cbd_datlen = 2;
+ else
+ tbdf[tx].cbd_datlen = pmsg->len + 1;
+
+ tbdf[tx].cbd_sc = 0;
+
+ if (!(pmsg->flags & I2C_M_NOSTART))
+ tbdf[tx].cbd_sc |= BD_I2C_START;
+ if (tx + 1 == num)
+ tbdf[tx].cbd_sc |= BD_SC_LAST | BD_SC_WRAP;
+
+ rbdf[rx].cbd_datlen = 0;
+ rbdf[rx].cbd_sc = BD_SC_EMPTY | BD_SC_INTRPT;
+
+ if (rx + 1 == CPM_MAXBD)
+ tbdf[rx].cbd_sc |= BD_SC_WRAP;
+
+ eieio();
+ tbdf[tx].cbd_sc |= BD_SC_READY;
+ } else {
+ tb[0] = addr; /* Device address byte w/rw flag */
+ for (i = 0; i < pmsg->len; i++)
+ tb[i+1] = pmsg->buf[i];
+
+ dev_dbg(&adap->dev, "cpm_iic_write(abyte=0x%x)\n", addr);
+
+ tbdf[tx].cbd_datlen = pmsg->len + 1;
+ tbdf[tx].cbd_sc = 0;
+
+ if (!(pmsg->flags & I2C_M_NOSTART))
+ tbdf[tx].cbd_sc |= BD_I2C_START;
+
+ if (tx + 1 == num)
+ tbdf[tx].cbd_sc |= BD_SC_LAST | BD_SC_WRAP;
+
+ eieio();
+ tbdf[tx].cbd_sc |= BD_SC_READY | BD_SC_INTRPT;
+
+ dev_dbg(&adap->dev, "tx sc %d %04x\n",
+ tx, tbdf[tx].cbd_sc);
+ }
+}
+
+static int cpm_i2c_check_message(struct i2c_adapter *adap,
+ struct i2c_msg *pmsg, int tx, int rx)
+{
+ cbd_t *tbdf, *rbdf;
+ u_char *tb;
+ u_char *rb;
+ struct cpm_i2c *cpm = i2c_get_adapdata(adap);
+ struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
+ int i;
+
+ tbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->tbase));
+ rbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->rbase));
+
+ tb = cpm->txbuf[tx];
+ rb = cpm->rxbuf[rx];
+
+ /* Align read buffer */
+ rb = (u_char *) (((uint) rb + 1) & ~1);
+
+ if (pmsg->flags & I2C_M_RD) {
+ dev_dbg(&adap->dev, "rx sc %04x, rx sc %04x\n",
+ tbdf[tx].cbd_sc, rbdf[rx].cbd_sc);
+
+ if (tbdf[tx].cbd_sc & BD_SC_NAK) {
+ dev_dbg(&adap->dev, "IIC read; No ack\n");
+
+ if (pmsg->flags & I2C_M_IGNORE_NAK)
+ return 0;
+ else
+ return -EIO;
+ }
+ if (rbdf[rx].cbd_sc & BD_SC_EMPTY) {
+ dev_dbg(&adap->dev,
+ "IIC read; complete but rbuf empty\n");
+ return -EREMOTEIO;
+ }
+ if (rbdf[rx].cbd_sc & BD_SC_OV) {
+ dev_dbg(&adap->dev, "IIC read; Overrun\n");
+ return -EREMOTEIO;
+ }
+ for (i = 0; i < pmsg->len; i++)
+ pmsg->buf[i] = rb[i];
+ } else {
+ dev_dbg(&adap->dev, "tx sc %d %04x\n", tx, tbdf[tx].cbd_sc);
+
+ if (tbdf[tx].cbd_sc & BD_SC_NAK) {
+ dev_dbg(&adap->dev, "IIC write; No ack\n");
+
+ if (pmsg->flags & I2C_M_IGNORE_NAK)
+ return 0;
+ else
+ return -EIO;
+ }
+ if (tbdf[tx].cbd_sc & BD_SC_UN) {
+ dev_dbg(&adap->dev, "IIC write; Underrun\n");
+ return -EIO;
+ }
+ if (tbdf[tx].cbd_sc & BD_SC_CL) {
+ dev_dbg(&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, *rmsg;
+ int ret, i;
+ int tptr;
+ int rptr;
+ cbd_t *tbdf, *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;
+ }
+
+ mutex_lock(&cpm->i2c_mutex);
+
+ /* 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 = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->tbase));
+ rbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->rbase));
+
+ tptr = 0;
+ rptr = 0;
+
+ while (tptr < num) {
+ pmsg = &msgs[tptr];
+ dev_dbg(&adap->dev, "i2c-algo-cpm.o: " "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 */
+ /* Chip bug, set enable here */
+ out_8(&i2c_reg->i2cmr, 0x13); /* Enable RX/TX/Error interupts */
+ out_8(&i2c_reg->i2cer, 0xff); /* Clear interrupt status */
+ out_8(&i2c_reg->i2mod, in_8(&i2c_reg->i2mod) | 1); /* Enable */
+ /* Begin transmission */
+ out_8(&i2c_reg->i2com, in_8(&i2c_reg->i2com) | 0x80);
+
+ tptr = 0;
+ rptr = 0;
+
+ while (tptr < num) {
+ /* Check for outstanding messages */
+ dev_dbg(&adap->dev, "test ready.\n");
+ if (!(tbdf[tptr].cbd_sc & BD_SC_READY)) {
+ dev_dbg(&adap->dev, "ready.\n");
+ rmsg = &msgs[tptr];
+ ret = cpm_i2c_check_message(adap, rmsg, tptr, rptr);
+ tptr++;
+ if (rmsg->flags & I2C_M_RD)
+ rptr++;
+ if (ret)
+ goto out_err;
+ } else {
+ dev_dbg(&adap->dev, "not ready.\n");
+ ret = wait_event_interruptible_timeout(cpm->i2c_wait,
+ !(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;
+ }
+ }
+ }
+#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);
+ out_8(&i2c_reg->i2mod, in_8(&i2c_reg->i2mod) | ~1);
+#endif
+ mutex_unlock(&cpm->i2c_mutex);
+ 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.
+ */
+ out_8(&i2c_reg->i2mod, in_8(&i2c_reg->i2mod) | ~1);
+#endif
+ mutex_unlock(&cpm->i2c_mutex);
+ return ret;
+}
+
+static u32 cpm_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+/* -----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",
+ .id = I2C_HW_MPC8XX_EPON,
+ .algo = &cpm_i2c_algo,
+ .class = I2C_CLASS_HWMON,
+};
+
+static int 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 *tbdf, *rbdf;
+ unsigned char brg;
+
+ pr_debug("i2c-cpm: cpm_i2c_setup()\n");
+
+ init_waitqueue_head(&cpm->i2c_wait);
+ mutex_init(&cpm->i2c_mutex);
+
+ 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;
+ }
+
+ /* Initialize Tx/Rx parameters. */
+
+ cpm_reset_i2c_params(cpm);
+
+ pr_debug("i2c-cpm: i2c_ram %x, dp_addr 0x%x\n", (uint) cpm->i2c_ram,
+ cpm->dp_addr);
+ pr_debug("i2c-cpm: tbase %d, rbase %d\n",
+ in_be16(&cpm->i2c_ram->tbase), in_be16(&cpm->i2c_ram->rbase));
+
+ tbdf = (cbd_t *) cpm_muram_addr(in_be16(&cpm->i2c_ram->tbase));
+ rbdf = (cbd_t *) cpm_muram_addr(in_be16(&cpm->i2c_ram->rbase));
+
+ /* Allocate TX and RX buffers */
+
+ 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;
+ }
+ 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;
+ }
+ tbdf[i].cbd_bufaddr = cpm->txdma[i];
+ }
+
+ 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, 0x01); /* 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. */
+ out_8(&cpm->i2c_reg->i2mod, in_8(&cpm->i2c_reg->i2mod) | ~1);
+
+ /* 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) {
+ printk(KERN_ERR "i2c-cpm: Unable to init hardware\n");
+ goto out;
+ }
+
+ /* register new adapter to i2c module... */
+
+ result = i2c_add_adapter(&cpm->adap);
+ if (result < 0) {
+ printk(KERN_ERR "i2c-cpm: Unable to register with I2C\n");
+ goto out2;
+ }
+
+ pr_debug("i2c-cpm: hw routines for %s registered.\n", cpm->adap.name);
+
+ /*
+ * Wait for patch from Jon Smirl
+ * of_register_i2c_devices(&cpm->adap, ofdev->node);
+ */
+
+ return 0;
+out2:
+ cpm_i2c_shutdown(cpm);
+out:
+ 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("Dan Malek <[email protected]>");
+MODULE_DESCRIPTION("I2C-Bus adapter routines for CPM boards");
+MODULE_LICENSE("GPL");
--
1.5.3.8


2008-02-21 12:05:42

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Hallo Jochen,

On Thu, 31 Jan 2008 13:54:01 +0100, Jochen Friedrich wrote:
> Using the port of 2.4 code from Vitaly Bordug <[email protected]>
> and the actual algorithm used by the i2c driver of the DBox code on
> cvs.tuxboc.org from Tmbinc, Gillem ([email protected]). Renamed i2c-rpx.c and
> i2c-algo-8xx.c to i2c-cpm.c and converted the driver to an
> of_platform_driver.
>
> Signed-off-by: Jochen Friedrich <[email protected]>
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-cpm.c | 759 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 770 insertions(+), 0 deletions(-)
> create mode 100644 drivers/i2c/busses/i2c-cpm.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index b61f56b..1d18db7 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -114,6 +114,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) && I2C && PPC_OF

The dependency on I2C is now handled at menu level, you no longer need
to mention it here.

> + 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 --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index ea7068f..1291e2b 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o
> 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 --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
> new file mode 100644
> index 0000000..7191427
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cpm.c
> @@ -0,0 +1,759 @@
> +/*
> + * Freescale CPM1/CPM2 I2C interface.
> + * Copyright (c) 1999 Dan Malek ([email protected]).
> + *
> + * moved into proper i2c interface;
> + * Brad Parker ([email protected])
> + *
> + * (C) 2007 Montavista Software, Inc.
> + * Vitaly Bordug <[email protected]>
> + *
> + * RPX lite specific parts of the i2c interface
> + * Update: There actually isn't anything RPXLite-specific about this module.
> + * This should work for most any CPM board. The console messages have been
> + * changed to eliminate RPXLite references.

Maybe this block comment can be removed entirely? It adds more
confusion than value IMHO.

> + *
> + 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.
> + *
> + * moved into proper i2c interface; separated out platform specific
> + * parts into i2c-8xx.c
> + * Brad Parker ([email protected])

Redundant with one of the block comments above.

> + *
> + * Parts from dbox2_i2c.c (cvs.tuxbox.org)
> + * (C) 2000-2001 Tmbinc, Gillem ([email protected])
> + *
> + * (C) 2007 Montavista Software, Inc.
> + * Vitaly Bordug <[email protected]>

Redundant as well.

> + *
> + * Converted to of_platform_device. Renamed to i2c-cpm.c.
> + * (C) 2007 Jochen Friedrich <[email protected]>
> + */

Please group all the copyright statements together, having some before
the GPL boilerplate and some after is rather confusing.

> +
> +#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/sched.h>

It's not obvious to me why you need to include <linux/sched.h>, please
clarify.

> +#include <linux/stddef.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/time.h>

I don't think that you need <linux/time.h> either?

> +#include <linux/dma-mapping.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <sysdev/fsl_soc.h>
> +#include <asm/cpm.h>
> +
> +/*
> + * Wait for patch from Jon Smirl
> + * #include "powerpc-common.h"
> + */

It doesn't make sense to merge this comment upstream.

> +
> +/* 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 CPM_CR_INIT_TRX (0x00)
> +#define CPM_CR_CLOSE_RXBD (0x07)
> +
> +#define I2C_EB (0x10) /* Big endian mode */
> +
> +/* 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 */
> +};
> +
> +/* 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;
> + struct mutex i2c_mutex; /* Protects I2C CPM */

"I2C CPM" isn't very descriptive. Best would be to list the structure
fields which are protected.

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

Extra '.

> + 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, cpm->dp_addr);
> + out_be16(&i2c_ram->rbase, cpm->dp_addr + sizeof(cbd_t) * CPM_MAXBD);
> +
> + out_8(&i2c_ram->tfcr, I2C_EB);
> + out_8(&i2c_ram->rfcr, I2C_EB);
> +
> + 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, in_be16(&i2c_ram->rbase));
> + 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, in_be16(&i2c_ram->tbase));
> + 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_RXBD);
> +
> + 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 *tbdf, *rbdf;
> + u_char addr;
> + u_char *tb;
> + u_char *rb;
> + struct cpm_i2c *cpm = i2c_get_adapdata(adap);
> + struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
> + int i, dscan;
> +
> + tbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->tbase));
> + rbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->rbase));
> +
> + /*
> + * This chip can't do zero length writes. However, the i2c core uses
> + * them to scan for devices. The best we can do is to convert them
> + * into 1 byte reads
> + */

Nack. New-style i2c drivers don't require this probing mechanism,
that's what you want to use if your I2C master doesn't support
zero-byte messages. Well, that's most probably what you want to use on
embedded platforms anyway.

So, please no hack for zero-byte messages. If they aren't supported by
the hardware, they aren't supported by the driver either.

> +
> + dscan = ((pmsg->len == 0) && (num == 1));
> +
> + addr = pmsg->addr << 1;
> + if ((pmsg->flags & I2C_M_RD) || dscan)
> + addr |= 1;
> +
> + tb = cpm->txbuf[tx];
> + rb = cpm->rxbuf[rx];
> +
> + /* Align read buffer */
> + rb = (u_char *) (((ulong) rb + 1) & ~1);
> +
> + if ((pmsg->flags & I2C_M_RD) || dscan) {
> + /*
> + * 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).
> + */
> + tb[0] = addr; /* Device address byte w/rw flag */
> +
> + dev_dbg(&adap->dev, "cpm_i2c_read(abyte=0x%x)\n", addr);
> +
> + if (dscan)
> + tbdf[tx].cbd_datlen = 2;
> + else
> + tbdf[tx].cbd_datlen = pmsg->len + 1;
> +
> + tbdf[tx].cbd_sc = 0;
> +
> + if (!(pmsg->flags & I2C_M_NOSTART))
> + tbdf[tx].cbd_sc |= BD_I2C_START;
> + if (tx + 1 == num)
> + tbdf[tx].cbd_sc |= BD_SC_LAST | BD_SC_WRAP;
> +
> + rbdf[rx].cbd_datlen = 0;
> + rbdf[rx].cbd_sc = BD_SC_EMPTY | BD_SC_INTRPT;
> +
> + if (rx + 1 == CPM_MAXBD)
> + tbdf[rx].cbd_sc |= BD_SC_WRAP;
> +
> + eieio();
> + tbdf[tx].cbd_sc |= BD_SC_READY;
> + } else {
> + tb[0] = addr; /* Device address byte w/rw flag */
> + for (i = 0; i < pmsg->len; i++)
> + tb[i+1] = pmsg->buf[i];
> +
> + dev_dbg(&adap->dev, "cpm_iic_write(abyte=0x%x)\n", addr);
> +
> + tbdf[tx].cbd_datlen = pmsg->len + 1;
> + tbdf[tx].cbd_sc = 0;
> +
> + if (!(pmsg->flags & I2C_M_NOSTART))
> + tbdf[tx].cbd_sc |= BD_I2C_START;
> +
> + if (tx + 1 == num)
> + tbdf[tx].cbd_sc |= BD_SC_LAST | BD_SC_WRAP;
> +
> + eieio();
> + tbdf[tx].cbd_sc |= BD_SC_READY | BD_SC_INTRPT;
> +
> + dev_dbg(&adap->dev, "tx sc %d %04x\n",
> + tx, tbdf[tx].cbd_sc);
> + }
> +}
> +
> +static int cpm_i2c_check_message(struct i2c_adapter *adap,
> + struct i2c_msg *pmsg, int tx, int rx)
> +{
> + cbd_t *tbdf, *rbdf;
> + u_char *tb;
> + u_char *rb;
> + struct cpm_i2c *cpm = i2c_get_adapdata(adap);
> + struct i2c_ram __iomem *i2c_ram = cpm->i2c_ram;
> + int i;
> +
> + tbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->tbase));
> + rbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->rbase));
> +
> + tb = cpm->txbuf[tx];
> + rb = cpm->rxbuf[rx];
> +
> + /* Align read buffer */
> + rb = (u_char *) (((uint) rb + 1) & ~1);
> +
> + if (pmsg->flags & I2C_M_RD) {
> + dev_dbg(&adap->dev, "rx sc %04x, rx sc %04x\n",
> + tbdf[tx].cbd_sc, rbdf[rx].cbd_sc);
> +
> + if (tbdf[tx].cbd_sc & BD_SC_NAK) {
> + dev_dbg(&adap->dev, "IIC read; No ack\n");
> +
> + if (pmsg->flags & I2C_M_IGNORE_NAK)
> + return 0;

Are there known I2C chips on this platform which need this flag? Its
implementation is purely optional, only a few broken I2C chips need it,
so it should really only be implemented when strictly needed.

> + else
> + return -EIO;
> + }
> + if (rbdf[rx].cbd_sc & BD_SC_EMPTY) {
> + dev_dbg(&adap->dev,
> + "IIC read; complete but rbuf empty\n");
> + return -EREMOTEIO;
> + }
> + if (rbdf[rx].cbd_sc & BD_SC_OV) {
> + dev_dbg(&adap->dev, "IIC read; Overrun\n");
> + return -EREMOTEIO;
> + }
> + for (i = 0; i < pmsg->len; i++)
> + pmsg->buf[i] = rb[i];
> + } else {
> + dev_dbg(&adap->dev, "tx sc %d %04x\n", tx, tbdf[tx].cbd_sc);
> +
> + if (tbdf[tx].cbd_sc & BD_SC_NAK) {
> + dev_dbg(&adap->dev, "IIC write; No ack\n");
> +
> + if (pmsg->flags & I2C_M_IGNORE_NAK)
> + return 0;
> + else
> + return -EIO;
> + }
> + if (tbdf[tx].cbd_sc & BD_SC_UN) {
> + dev_dbg(&adap->dev, "IIC write; Underrun\n");
> + return -EIO;
> + }
> + if (tbdf[tx].cbd_sc & BD_SC_CL) {
> + dev_dbg(&adap->dev, "IIC write; Collision\n");
> + return -EIO;
> + }
> + }
> + return 0;
> +}

Many of the dev_dbg() calls in the function above could be replaced
with dev_err(), methinks.

> +
> +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, *rmsg;
> + int ret, i;
> + int tptr;
> + int rptr;
> + cbd_t *tbdf, *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;
> + }
> +
> + mutex_lock(&cpm->i2c_mutex);
> +
> + /* 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 = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->tbase));
> + rbdf = (cbd_t *) cpm_muram_addr(in_be16(&i2c_ram->rbase));
> +
> + tptr = 0;
> + rptr = 0;
> +
> + while (tptr < num) {
> + pmsg = &msgs[tptr];
> + dev_dbg(&adap->dev, "i2c-algo-cpm.o: " "R: %d T: %d\n",

Incorrect (and unneeded) file name reference.

> + rptr, tptr);
> +
> + cpm_i2c_parse_message(adap, pmsg, num, tptr, rptr);
> + if (pmsg->flags & I2C_M_RD)
> + rptr++;
> + tptr++;
> + }
> + /* Start transfer now */
> + /* Chip bug, set enable here */
> + out_8(&i2c_reg->i2cmr, 0x13); /* Enable RX/TX/Error interupts */
> + out_8(&i2c_reg->i2cer, 0xff); /* Clear interrupt status */
> + out_8(&i2c_reg->i2mod, in_8(&i2c_reg->i2mod) | 1); /* Enable */
> + /* Begin transmission */
> + out_8(&i2c_reg->i2com, in_8(&i2c_reg->i2com) | 0x80);
> +
> + tptr = 0;
> + rptr = 0;
> +
> + while (tptr < num) {
> + /* Check for outstanding messages */
> + dev_dbg(&adap->dev, "test ready.\n");
> + if (!(tbdf[tptr].cbd_sc & BD_SC_READY)) {
> + dev_dbg(&adap->dev, "ready.\n");
> + rmsg = &msgs[tptr];
> + ret = cpm_i2c_check_message(adap, rmsg, tptr, rptr);
> + tptr++;
> + if (rmsg->flags & I2C_M_RD)
> + rptr++;
> + if (ret)
> + goto out_err;
> + } else {
> + dev_dbg(&adap->dev, "not ready.\n");
> + ret = wait_event_interruptible_timeout(cpm->i2c_wait,
> + !(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;
> + }
> + }
> + }
> +#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);
> + out_8(&i2c_reg->i2mod, in_8(&i2c_reg->i2mod) | ~1);
> +#endif
> + mutex_unlock(&cpm->i2c_mutex);
> + 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.
> + */
> + out_8(&i2c_reg->i2mod, in_8(&i2c_reg->i2mod) | ~1);
> +#endif
> + mutex_unlock(&cpm->i2c_mutex);
> + return ret;
> +}
> +
> +static u32 cpm_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

Actually I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK)
according to my comment above.

> +}
> +
> +/* -----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",
> + .id = I2C_HW_MPC8XX_EPON,

Not defined anywhere, so this won't build. This ID is optional, so if
you don't need it, just don't set it. With the advent of new-style i2c
device drivers, the use of these IDs will hopefully phase out.

> + .algo = &cpm_i2c_algo,
> + .class = I2C_CLASS_HWMON,
> +};
> +
> +static int cpm_i2c_setup(struct cpm_i2c *cpm)

Could be __devinit.

> +{
> + struct of_device *ofdev = cpm->ofdev;
> + const u32 *data;
> + int len, ret, i;
> + void __iomem *i2c_base;
> + cbd_t *tbdf, *rbdf;
> + unsigned char brg;
> +
> + pr_debug("i2c-cpm: cpm_i2c_setup()\n");

dev_dbg()

> +
> + init_waitqueue_head(&cpm->i2c_wait);
> + mutex_init(&cpm->i2c_mutex);
> +
> + 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;
> + }
> +
> + /* Initialize Tx/Rx parameters. */
> +
> + cpm_reset_i2c_params(cpm);
> +
> + pr_debug("i2c-cpm: i2c_ram %x, dp_addr 0x%x\n", (uint) cpm->i2c_ram,
> + cpm->dp_addr);
> + pr_debug("i2c-cpm: tbase %d, rbase %d\n",
> + in_be16(&cpm->i2c_ram->tbase), in_be16(&cpm->i2c_ram->rbase));

dev_dbg()

> +
> + tbdf = (cbd_t *) cpm_muram_addr(in_be16(&cpm->i2c_ram->tbase));
> + rbdf = (cbd_t *) cpm_muram_addr(in_be16(&cpm->i2c_ram->rbase));
> +
> + /* Allocate TX and RX buffers */
> +
> + 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;
> + }
> + 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;
> + }
> + tbdf[i].cbd_bufaddr = cpm->txdma[i];
> + }
> +
> + 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, 0x01); /* 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. */
> + out_8(&cpm->i2c_reg->i2mod, in_8(&cpm->i2c_reg->i2mod) | ~1);
> +
> + /* 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) {
> + printk(KERN_ERR "i2c-cpm: Unable to init hardware\n");

dev_err()

> + goto out;
> + }
> +
> + /* register new adapter to i2c module... */
> +
> + result = i2c_add_adapter(&cpm->adap);
> + if (result < 0) {
> + printk(KERN_ERR "i2c-cpm: Unable to register with I2C\n");

dev_err()

> + goto out2;
> + }
> +
> + pr_debug("i2c-cpm: hw routines for %s registered.\n", cpm->adap.name);

dev_dbg()

> +
> + /*
> + * Wait for patch from Jon Smirl
> + * of_register_i2c_devices(&cpm->adap, ofdev->node);
> + */

Again I won't merge this comment upstream.

> +
> + return 0;
> +out2:
> + cpm_i2c_shutdown(cpm);
> +out:

Better give more explicit names to your labels (for example
err_shutdown and err_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("Dan Malek <[email protected]>");

Why not your own name?

> +MODULE_DESCRIPTION("I2C-Bus adapter routines for CPM boards");
> +MODULE_LICENSE("GPL");


--
Jean Delvare

2008-02-22 11:19:52

by Jochen Friedrich

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Hi Jean,

>> +/*
>> + * Wait for patch from Jon Smirl
>> + * #include "powerpc-common.h"
>> + */
>
> It doesn't make sense to merge this comment upstream.

I know you don't like the patch from Jon Smirl and you also explained your reasons.
Fortunately, I2c no longer uses numeric device IDs but names. So what are the alternatives?

1. modify the I2c subsystem to accept OF names additionally to I2c names (proposed by Jon smirl).
2. record the I2c name in the dts tree, either as seperate tag (like linux,i2c-name="<i2c-name>")
or as additional compatible entry (like compatible="...", "linux,<i2c-name>").
3. use a glue layer with a translation map.

What is the preferred way to do this?

Thanks,
Jochen

2008-02-23 12:43:49

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Hi Jochen,

On Fri, 22 Feb 2008 12:16:16 +0100, Jochen Friedrich wrote:
> Hi Jean,
>
> >> +/*
> >> + * Wait for patch from Jon Smirl
> >> + * #include "powerpc-common.h"
> >> + */
> >
> > It doesn't make sense to merge this comment upstream.
>
> I know you don't like the patch from Jon Smirl and you also explained your reasons.
> Fortunately, I2c no longer uses numeric device IDs but names. So what are the alternatives?
>
> 1. modify the I2c subsystem to accept OF names additionally to I2c names (proposed by Jon smirl).

The problem I have with this is that it breaks compatibility. The chip
name is not only used for device/driver matching, it is also exported
to userspace as a sysfs attribute ("name"). Applications might rely on
it. At least libsensors does.

One way to work around the problem would be to use separate fields for
device/driver matching and the "name" attribute. However, this will add
some complexity to the i2c-core code and cost some memory as well, so
it is far from perfect.

> 2. record the I2c name in the dts tree, either as seperate tag (like linux,i2c-name="<i2c-name>")
> or as additional compatible entry (like compatible="...", "linux,<i2c-name>").

This would work, and it would require almost no change to the current
i2c-core code, but I thought that these dts files were not under our
control?

The problem I see with this approach is that the name translation would
have to be done for each dts file, rather than just once for each device
type. This means more work, but maybe this can be done if at least part
of it is automated. I admit that I never considered this option because
I considered the dts files as read-only. If this assumption was
incorrect, then maybe this is the best solution. Can you please
elaborate on the details of this proposal?

> 3. use a glue layer with a translation map.

This is what we do at the moment (see i2c_devices in
arch/powerpc/sysdev/fsl_soc.c). Jon Smirl is worried that it won't
scale well though, and I can't disagree.

> What is the preferred way to do this?

I still have to think about it. I didn't have much time to work on this
during the last 2 weeks, hopefully I will fine some time to experiment
again soon. As I underlined before, my patch set affects no less than 5
subsystems with different needs and expectations, it's no trivial
change.

--
Jean Delvare

2008-02-23 21:24:46

by Olof Johansson

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

On Fri, Feb 22, 2008 at 12:16:16PM +0100, Jochen Friedrich wrote:

> Fortunately, I2c no longer uses numeric device IDs but names. So what are the alternatives?
>
> 1. modify the I2c subsystem to accept OF names additionally to I2c names (proposed by Jon smirl).

Sounds like Jean isn't very excited about this idea..

> 2. record the I2c name in the dts tree, either as seperate tag (like linux,i2c-name="<i2c-name>")
> or as additional compatible entry (like compatible="...", "linux,<i2c-name>").

I have to say no on this one. The device tree is not supposed to know
about how linux uses devices, there are firmwares out there that don't
use DTS for thier device trees, etc.

> 3. use a glue layer with a translation map.

In my opinion this is an OK solution since the same information has to
be added somewhere already anyway -- eiither to the drivers or to this
translation table. It should of course be an abstacted shared table,
preferrably contained under the i2c source directories since several
platforms and architectures might share them.


-Olof

2008-02-24 15:16:44

by Jochen Friedrich

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Hi Olof,

>> 2. record the I2c name in the dts tree, either as seperate tag (like linux,i2c-name="<i2c-name>")
>> or as additional compatible entry (like compatible="...", "linux,<i2c-name>").
>
> I have to say no on this one. The device tree is not supposed to know
> about how linux uses devices, there are firmwares out there that don't
> use DTS for thier device trees, etc.

I still believe this this could be done for embedded devices which are usually booted
via wrapper or U-Boot as those devices will most probably use the most exotic I2c devices
out there (e.g. home-grown devices used by stbs). However, I'm not an device tree expert.

>> 3. use a glue layer with a translation map.
>
> In my opinion this is an OK solution since the same information has to
> be added somewhere already anyway -- eiither to the drivers or to this
> translation table. It should of course be an abstacted shared table,
> preferrably contained under the i2c source directories since several
> platforms and architectures might share them.

I could think of a mixture between 2. and 3.:

Using the compatible attribute with the manufacturer stripped off as I2c name by default
and using an exception table. For now, the struct i2c_driver_device would currently only
need one entry ("dallas,ds1374", "rtc-ds1374").

Thanks,
Jochen

2008-02-24 16:19:37

by [email protected]

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

On 2/22/08, Jochen Friedrich <[email protected]> wrote:
> Hi Jean,
>
>
> >> +/*
> >> + * Wait for patch from Jon Smirl
> >> + * #include "powerpc-common.h"
> >> + */
> >
> > It doesn't make sense to merge this comment upstream.
>
>
> I know you don't like the patch from Jon Smirl and you also explained your reasons.
> Fortunately, I2c no longer uses numeric device IDs but names. So what are the alternatives?
>
> 1. modify the I2c subsystem to accept OF names additionally to I2c names (proposed by Jon smirl).

The correct statement is: modify the i2c subsystem to support the
standard kernel driver aliasing mechanism. Leaving powerpc and OF out
of the argument for the moment, i2c has a custom aliasing scheme on
the x86 too.

So as a first step, can we remove the custom i2c aliasing scheme and
change i2c to use standard module aliases on the x86? Patches for this
already exist.

On 2/23/08, Jean Delvare <[email protected]> wrote:
> The problem I have with this is that it breaks compatibility. The chip
> name is not only used for device/driver matching, it is also exported
> to userspace as a sysfs attribute ("name"). Applications might rely on
> it. At least libsensors does.

I think there is some confusion here. The OF aliases are only used by
the kernel to load the correct driver. Would doing something like this
help?

static struct i2c_device_id pcf8563_id[] = {
{"pcf8563", 0, "sysfs_legacy_name"},
{"rtc8564", 0, "sysfs_legacy_name"},
OF_ID("phillips,pcf8563", &pcf8563_id[0], 0)
OF_ID("epson,rtc8564", &pcf8563_id[1], 0)
{},
};
MODULE_DEVICE_TABLE(i2c, pcf8563_id);

Then in the probe function you can use the pointer to find the base id
entry and i2c never has to be aware that the OF alias exists.

> 2. record the I2c name in the dts tree, either as separate tag (like linux,i2c-name="<i2c-name>")

Not really practical for the millions of machines (all PowerPC Macs)
already shipped.

> or as additional compatible entry (like compatible="...", "linux,<i2c-name>").
> 3. use a glue layer with a translation map.

Audio codecs have exactly the same problem. There are probably other
devices that also need mapping.

This mapping table will need to contain a map from the OF names to the
kernel driver names. It will need to stored permanently in RAM and
contain all possible mappings. This table will only grow in size.

The kernel has a widely used mechanism for mapping -- aliases in the
device drivers. Why do we want to build a new, parallel one?

What we are doing now is option 4.

4. Use kconfig to build custom kernels for each target system. Don't
load drivers automatically based on what the BIOS tells us.

--
Jon Smirl
[email protected]

2008-02-24 18:11:25

by Olof Johansson

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Hi,

On Sun, Feb 24, 2008 at 04:16:30PM +0100, Jochen Friedrich wrote:
> Hi Olof,
>
> >> 2. record the I2c name in the dts tree, either as seperate tag (like linux,i2c-name="<i2c-name>")
> >> or as additional compatible entry (like compatible="...", "linux,<i2c-name>").
> >
> > I have to say no on this one. The device tree is not supposed to know
> > about how linux uses devices, there are firmwares out there that don't
> > use DTS for thier device trees, etc.
>
> I still believe this this could be done for embedded devices which are usually booted
> via wrapper or U-Boot as those devices will most probably use the most exotic I2c devices
> out there (e.g. home-grown devices used by stbs). However, I'm not an device tree expert.

Sorry, but you're wrong in your assumptions. Not all embedded devices
use U-boot, and not all use the wrapper. It's just a bad idea to encode
linux-specific things in the device tree, period.

And even if you DO decide to go that route, guess what? You need a
translation table just as with (3) anyway!

> >> 3. use a glue layer with a translation map.
> >
> > In my opinion this is an OK solution since the same information has to
> > be added somewhere already anyway -- eiither to the drivers or to this
> > translation table. It should of course be an abstacted shared table,
> > preferrably contained under the i2c source directories since several
> > platforms and architectures might share them.
>
> I could think of a mixture between 2. and 3.:
>
> Using the compatible attribute with the manufacturer stripped off as I2c name by default
> and using an exception table. For now, the struct i2c_driver_device would currently only
> need one entry ("dallas,ds1374", "rtc-ds1374").

You still need the translation table, you're just flattening the
namespace to one string instead of two, the same information still has
to be encoded. I can't see what the benefit of this approach compared to
the other one is. "dallas,ds1374" already only has one translation entry
in the table?


-Olof

2008-02-25 18:35:49

by Jochen Friedrich

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Hi Olof,

> And even if you DO decide to go that route, guess what? You need a
> translation table just as with (3) anyway!

True.

>>>> 3. use a glue layer with a translation map.
>>> In my opinion this is an OK solution since the same information has to
>>> be added somewhere already anyway -- eiither to the drivers or to this
>>> translation table. It should of course be an abstacted shared table,
>>> preferrably contained under the i2c source directories since several
>>> platforms and architectures might share them.
>> I could think of a mixture between 2. and 3.:
>>
>> Using the compatible attribute with the manufacturer stripped off as I2c name by default
>> and using an exception table. For now, the struct i2c_driver_device would currently only
>> need one entry ("dallas,ds1374", "rtc-ds1374").
>
> You still need the translation table, you're just flattening the
> namespace to one string instead of two, the same information still has
> to be encoded. I can't see what the benefit of this approach compared to
> the other one is. "dallas,ds1374" already only has one translation entry
> in the table?

As soon as http://lists.lm-sensors.org/pipermail/i2c/2008-January/002752.html has been
applied, one could get rid of all entries where the I2c (alias) name can be obtained from
the OF name just by stripping the manufacturer.

Thanks,
Jochen

2008-03-25 18:46:20

by Jochen Friedrich

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Hi Jean,

>> What is the preferred way to do this?
>
> I still have to think about it. I didn't have much time to work on this
> during the last 2 weeks, hopefully I will fine some time to experiment
> again soon. As I underlined before, my patch set affects no less than 5
> subsystems with different needs and expectations, it's no trivial
> change.

Sorry for the late response, but I was pretty busy the last couple of weeks.

I'm thinking about something like this (based on http://patchwork.ozlabs.org/linuxppc/patch?id=16282 to 16284):

This patch implements various helpers to support OF bindings for
the i2c API.

Signed-off-by: Jochen Friedrich <[email protected]>
---
drivers/of/Kconfig | 6 +++
drivers/of/Makefile | 1 +
drivers/of/i2c.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_i2c.h | 24 ++++++++++
4 files changed, 144 insertions(+), 0 deletions(-)
create mode 100644 drivers/of/i2c.c
create mode 100644 include/linux/of_i2c.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index c03072b..3cff449 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -1,3 +1,9 @@
config OF_DEVICE
def_bool y
depends on OF && (SPARC || PPC_OF)
+
+config OF_I2C
+ def_bool y
+ depends on OF && PPC_OF && I2C
+ help
+ OpenFirmware I2C accessors
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index ab9be5d..655b982 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,2 +1,3 @@
obj-y = base.o
obj-$(CONFIG_OF_DEVICE) += device.o platform.o
+obj-$(CONFIG_OF_I2C) += i2c.o
diff --git a/drivers/of/i2c.c b/drivers/of/i2c.c
new file mode 100644
index 0000000..d3d1e9e
--- /dev/null
+++ b/drivers/of/i2c.c
@@ -0,0 +1,113 @@
+/*
+ * OF helpers for the I2C API
+ *
+ * Copyright (c) 2008 Jochen Friedrich <[email protected]>
+ *
+ * Based on a previous patch from Jon Smirl <[email protected]>
+ *
+ * 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.
+ */
+
+#include <linux/i2c.h>
+#include <asm/prom.h>
+
+struct i2c_driver_device {
+ char *of_device;
+ char *i2c_type;
+};
+
+static struct i2c_driver_device i2c_devices[] = {
+ {"dallas,ds1374", "rtc-ds1374",},
+};
+
+static int of_find_i2c_driver(struct device_node *node,
+ struct i2c_board_info *info)
+{
+ int i, cplen;
+ const char *compatible;
+ const char *p;
+
+ /* 1. search for exception list entry */
+ for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
+ if (!of_device_is_compatible(node, i2c_devices[i].of_device))
+ continue;
+ if (strlcpy(info->type, i2c_devices[i].i2c_type,
+ I2C_NAME_SIZE) >= I2C_NAME_SIZE)
+ return -ENOMEM;
+
+ return 0;
+ }
+
+ compatible = of_get_property(node, "compatible", &cplen);
+ if (!compatible)
+ return -ENODEV;
+
+ /* 2. search for linux,<i2c-type> entry */
+ p = compatible;
+ while (cplen > 0) {
+ if (!strncmp(p, "linux,", 6)) {
+ p += 6;
+ if (strlcpy(info->type, p,
+ I2C_NAME_SIZE) >= I2C_NAME_SIZE)
+ return -ENOMEM;
+ return 0;
+ }
+
+ i = strlen(p) + 1;
+ p += i;
+ cplen -= i;
+ }
+
+ /* 3. take fist compatible entry and strip manufacturer */
+ p = strchr(compatible, ',');
+ if (!p)
+ return -ENODEV;
+ p++;
+ if (strlcpy(info->type, p, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
+ return -ENOMEM;
+ return 0;
+}
+
+void of_register_i2c_devices(struct i2c_adapter *adap,
+ struct device_node *adap_node)
+{
+ void *result;
+ struct device_node *node = NULL;
+
+ while ((node = of_get_next_child(adap_node, node))) {
+ struct i2c_board_info info = {};
+ const u32 *addr;
+ int len;
+
+ addr = of_get_property(node, "reg", &len);
+ if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
+ printk(KERN_ERR
+ "of-i2c: invalid i2c device entry\n");
+ continue;
+ }
+
+ info.irq = irq_of_parse_and_map(node, 0);
+ if (info.irq == NO_IRQ)
+ info.irq = -1;
+
+ if (of_find_i2c_driver(node, &info) < 0)
+ continue;
+
+ info.addr = *addr;
+
+ request_module(info.type);
+
+ result = i2c_new_device(adap, &info);
+ if (result == NULL) {
+ printk(KERN_ERR
+ "of-i2c: Failed to load driver for %s\n",
+ info.type);
+ irq_dispose_mapping(info.irq);
+ continue;
+ }
+ }
+}
+EXPORT_SYMBOL(of_register_i2c_devices);
diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h
new file mode 100644
index 0000000..2e5a967
--- /dev/null
+++ b/include/linux/of_i2c.h
@@ -0,0 +1,24 @@
+/*
+ * Generic I2C API implementation for PowerPC.
+ *
+ * Copyright (c) 2008 Jochen Friedrich <[email protected]>
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_OF_I2C_H
+#define __LINUX_OF_I2C_H
+
+#include <linux/i2c.h>
+
+#ifdef CONFIG_OF_I2C
+
+void of_register_i2c_devices(struct i2c_adapter *adap,
+ struct device_node *adap_node);
+
+#endif /* CONFIG_OF_I2C */
+
+#endif /* __LINUX_OF_I2C_H */
--
1.5.4.4

2008-03-26 00:41:59

by Stephen Rothwell

[permalink] [raw]
Subject: Re: [PATCHv4 2.6.25] i2c: adds support for i2c bus on Freescale CPM1/CPM2 controllers

Hi Jochen,

Firstly, you should probably cc Dave Miller <[email protected]> on
anything in drivers/of as Sparc is the other user of this stuff (just in
case they are interested).

On Tue, 25 Mar 2008 19:46:41 +0100 Jochen Friedrich <[email protected]> wrote:
>
> +++ b/drivers/of/i2c.c
> @@ -0,0 +1,113 @@
> +/*
> + * OF helpers for the I2C API
> + *
> + * Copyright (c) 2008 Jochen Friedrich <[email protected]>
> + *
> + * Based on a previous patch from Jon Smirl <[email protected]>
> + *
> + * 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.
> + */
> +
> +#include <linux/i2c.h>
> +#include <asm/prom.h>

You should really include <linux/of.h> instead.

> +struct i2c_driver_device {
> + char *of_device;
> + char *i2c_type;
> +};
> +
> +static struct i2c_driver_device i2c_devices[] = {
> + {"dallas,ds1374", "rtc-ds1374",},
^
You don't need the comma before the brace (unless you are anticipating
that struct i2c_driver_device will change. Even then, its not a big
thing.

> +void of_register_i2c_devices(struct i2c_adapter *adap,
> + struct device_node *adap_node)
> +{
> + void *result;
> + struct device_node *node = NULL;
> +
> + while ((node = of_get_next_child(adap_node, node))) {

for_each_child_of_node(adap_node, node) {
And then you don't need to initialise "node" above.

> + info.irq = irq_of_parse_and_map(node, 0);
> + if (info.irq == NO_IRQ)
> + info.irq = -1;
> +
> + if (of_find_i2c_driver(node, &info) < 0)
> + continue;

Do you need to clean up after the irq_of_parse_and_map() above?

--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/


Attachments:
(No filename) (1.84 kB)
(No filename) (189.00 B)
Download all attachments