On Sat, 21 Apr 2007 09:57:07 +0200 Jean Delvare wrote: > Hi Vitaly, > > On Fri, 20 Apr 2007 08:27:14 +0400, Vitaly Bordug wrote: > > Utilized devicetree to store I2C data, ported i2c-algo-8xx.c > > from 2.4 approach(which remains nearly intact), refined i2c-rpx.c. > > I2C functionality has been validated on mpc885ads with EEPROM > > access. > > Thanks for working on this. I was about to kill i2c-rpx because it's > broken for so long: > http://lists.lm-sensors.org/pipermail/i2c/2007-March/000970.html >
Noticed this :) > > Signed-off-by: Vitaly Bordug <[EMAIL PROTECTED]> > > --- > > Jean, > > > > The patch below may have rough edges but I'd appreciate you to take > > a look. It adds I2C capabilities of PQ series (mpc8xx mostly) or, > > more correctly, takes them off from the 2.4 kernel and makes it > > work. > > OK. I can't comment on the platform-specific part I am not familiar > with, but here's a quick review of the rest. > > > Validated with quilt tree residing at > > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/ > > > > > > arch/powerpc/boot/dts/mpc885ads.dts | 7 > > arch/powerpc/platforms/8xx/mpc885ads_setup.c | 14 + > > arch/powerpc/sysdev/fsl_soc.c | 61 +++ > > drivers/i2c/algos/Kconfig | 2 > > drivers/i2c/algos/Makefile | 1 > > drivers/i2c/algos/i2c-algo-8xx.c | 622 > > ++++++++++++++++++++++++++ > > drivers/i2c/busses/Kconfig | 4 > > drivers/i2c/busses/i2c-rpx.c | 129 ++++- > > include/linux/i2c-algo-8xx.h | 29 + 9 files > > changed, 822 insertions(+), 47 deletions(-) > > I wonder what's the point of having a separate i2c algorithm driver. > We don't expect any other driver than i2c-rpx to ever use it, do we? > In that case, all the code should be added to i2c-rpx directly, this > will makes things more simple and more efficient. > That is how it was back in 2.4 - if you see combine is a good move, I'm OK with it. But what shouldn't be rpc then - basically rpx(lite) is 8xx-based target, so let's call it all mpc8xx then. > > diff --git a/arch/powerpc/boot/dts/mpc885ads.dts > > b/arch/powerpc/boot/dts/mpc885ads.dts index 19d2d79..90e047a 100644 > > --- a/arch/powerpc/boot/dts/mpc885ads.dts > > +++ b/arch/powerpc/boot/dts/mpc885ads.dts > > @@ -188,6 +188,13 @@ > > interrupts = <1d 3>; > > interrupt-parent = <930>; > > }; > > + [EMAIL PROTECTED] { > > + device_type = "i2c"; > > + compatible = "fsl-i2c-cpm"; > > + reg = <860 20 3c80 30>; > > + interrupts = <10 3>; > > + interrupt-parent = <930>; > > + }; > > }; > > }; > > }; > > diff --git a/arch/powerpc/platforms/8xx/mpc885ads_setup.c > > b/arch/powerpc/platforms/8xx/mpc885ads_setup.c index > > 9bd81c7..d32e066 100644 --- > > a/arch/powerpc/platforms/8xx/mpc885ads_setup.c +++ > > b/arch/powerpc/platforms/8xx/mpc885ads_setup.c @@ -51,6 +51,7 @@ > > static void init_smc1_uart_ioports(struc static void > > init_smc2_uart_ioports(struct fs_uart_platform_info* fpi); static > > void init_scc3_ioports(struct fs_platform_info* ptr); static void > > init_irda_ioports(void); +static void init_i2c_ioports(void); > > > > void __init mpc885ads_board_setup(void) > > { > > @@ -120,6 +121,10 @@ #endif > > #ifdef CONFIG_8XX_SIR > > init_irda_ioports(); > > #endif > > + > > +#ifdef CONFIG_I2C_RPXLITE > > + init_i2c_ioports(); > > +#endif > > } > > > > > > @@ -361,6 +366,15 @@ static void init_irda_ioports() > > immr_unmap(cp); > > } > > > > +static void init_i2c_ioports() > > +{ > > + cpm8xx_t *cp = (cpm8xx_t *)immr_map(im_cpm); > > + > > + setbits32(&cp->cp_pbpar, 0x00000030); > > + setbits32(&cp->cp_pbdir, 0x00000030); > > + setbits16(&cp->cp_pbodr, 0x0030); > > +} > > If !CONFIG_I2C_RPXLITE, you define a static function which you never > use. This is inefficient, and gcc will complain. > yap, this one is board-specific anyway, will fix. > > + > > int platform_device_skip(const char *model, int id) > > { > > #ifdef CONFIG_MPC8xx_SECOND_ETH_SCC3 > > diff --git a/arch/powerpc/sysdev/fsl_soc.c > > b/arch/powerpc/sysdev/fsl_soc.c index 419b688..7ecd537 100644 > > --- a/arch/powerpc/sysdev/fsl_soc.c > > +++ b/arch/powerpc/sysdev/fsl_soc.c > > @@ -331,7 +331,7 @@ static int __init fsl_i2c_of_init(void) > > for (np = NULL, i = 0; > > (np = of_find_compatible_node(np, "i2c", > > "fsl-i2c")) != NULL; i++) { > > - struct resource r[2]; > > + struct resource r[3]; > > struct fsl_i2c_platform_data i2c_data; > > const unsigned char *flags = NULL; > > > > @@ -1215,4 +1215,63 @@ err: > > > > arch_initcall(fs_irda_of_init); > > > > +static const char *i2c_regs = "regs"; > > +static const char *i2c_pram = "pram"; > > +static const char *i2c_irq = "interrupt"; > > + > > +static int __init fsl_i2c_cpm_of_init(void) > > +{ > > + struct device_node *np; > > + unsigned int i; > > + struct platform_device *i2c_dev; > > + int ret; > > + > > + for (np = NULL, i = 0; > > + (np = of_find_compatible_node(np, "i2c", > > "fsl-i2c-cpm")) != NULL; > > + i++) { > > + struct resource r[3]; > > + struct fsl_i2c_platform_data i2c_data; > > + > > + memset(&r, 0, sizeof(r)); > > + memset(&i2c_data, 0, sizeof(i2c_data)); > > + > > + ret = of_address_to_resource(np, 0, &r[0]); > > + if (ret) > > + goto err; > > + r[0].name = i2c_regs; > > + > > + ret = of_address_to_resource(np, 1, &r[1]); > > + if (ret) > > + goto err; > > + r[1].name = i2c_pram; > > + > > + r[2].start = r[2].end = irq_of_parse_and_map(np, > > 0); > > + r[2].flags = IORESOURCE_IRQ; > > + r[2].name = i2c_irq; > > + > > + i2c_dev = > > platform_device_register_simple("fsl-i2c-cpm", i, &r[0], 3); > > + if (IS_ERR(i2c_dev)) { > > + ret = PTR_ERR(i2c_dev); > > + goto err; > > + } > > + > > + ret = > > + platform_device_add_data(i2c_dev, &i2c_data, > > + sizeof(struct > > + > > fsl_i2c_platform_data)); > > + if (ret) > > + goto unreg; > > + } > > + > > + return 0; > > + > > +unreg: > > + platform_device_unregister(i2c_dev); > > +err: > > + return ret; > > +} > > + > > +arch_initcall(fsl_i2c_cpm_of_init); > > + > > + > > #endif /* CONFIG_8xx */ > > diff --git a/drivers/i2c/algos/Kconfig b/drivers/i2c/algos/Kconfig > > index 5889907..7d7fb87 100644 > > --- a/drivers/i2c/algos/Kconfig > > +++ b/drivers/i2c/algos/Kconfig > > @@ -37,6 +37,8 @@ config I2C_ALGOPCA > > config I2C_ALGO8XX > > tristate "MPC8xx CPM I2C interface" > > depends on 8xx > > + help > > + 8xx I2C Algorithm > > > > config I2C_ALGO_SGI > > tristate "I2C SGI interfaces" > > diff --git a/drivers/i2c/algos/Makefile b/drivers/i2c/algos/Makefile > > index cac1051..1bd3b37 100644 > > --- a/drivers/i2c/algos/Makefile > > +++ b/drivers/i2c/algos/Makefile > > @@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_ALGOBIT) += i2c-algo-bi > > obj-$(CONFIG_I2C_ALGOPCF) += i2c-algo-pcf.o > > obj-$(CONFIG_I2C_ALGOPCA) += i2c-algo-pca.o > > obj-$(CONFIG_I2C_ALGO_SGI) += i2c-algo-sgi.o > > +obj-$(CONFIG_I2C_ALGO8XX) += i2c-algo-8xx.o > > > > ifeq ($(CONFIG_I2C_DEBUG_ALGO),y) > > EXTRA_CFLAGS += -DDEBUG > > diff --git a/drivers/i2c/algos/i2c-algo-8xx.c > > b/drivers/i2c/algos/i2c-algo-8xx.c new file mode 100644 > > index 0000000..b1f414a > > --- /dev/null > > +++ b/drivers/i2c/algos/i2c-algo-8xx.c > > General comment for this file: all the printks need a level (KERN_*) > and some prefix so that the user knows they come from this driver. Or > you can switch to pr_info/pr_debug or even dev_info/dev_dbg. > OK > > @@ -0,0 +1,622 @@ > > +/* > > + * i2c-algo-8xx.c i2x driver algorithms for MPC8XX CPM > > + * Copyright (c) 1999 Dan Malek ([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. > > + > > + 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-rpx.c > > + * Brad Parker ([EMAIL PROTECTED]) > > + */ > > + > > + > > +/* $Id: i2c-algo-8xx.c,v 1.15 2004/11/20 08:02:24 khali Exp $ */ > > Drop this. > > > + > > +#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/i2c.h> > > +#include <linux/i2c-algo-8xx.h> > > +#include <asm/io.h> > > +#include <asm/cacheflush.h> > > +#include <asm/time.h> > > +#include <asm/mpc8xx.h> > > + > > +#define CPM_MAX_READ 513 > > +#undef I2C_CHIP_ERRATA /* Try ot define this if you have an > > older CPU(earlier than rev D4) */ > > Broken indentation, line too long, and typo in the comment. > > > + > > +static wait_queue_head_t iic_wait; > > +static ushort r_tbase, r_rbase; > > + > > +int cpm_debug = 0; > > Should be static and not initialized explicitly. > > > +int cpm_scan = 0; > > Please drop this, same can be done in a generic (and safer) way from > user-space using i2c-dev + i2cdetect. > > > + > > +static irqreturn_t cpm_iic_interrupt(int irq, void *dev_id) > > +{ > > + i2c8xx_t *i2c = (i2c8xx_t *) dev_id; > > + if (cpm_debug > 1) > > + printk("cpm_iic_interrupt(dev_id=%p)\n", dev_id); > > +#if 0 > > + /* Chip errata, clear enable. This is not needed on rev D4 > > CPUs */ > > + /* This should probably be removed and replaced by > > I2C_CHIP_ERRATA stuff */ > > + /* Someone with a buggy CPU needs to confirm that */ > > + out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | ~1); > > +#endif > > + /* Clear interrupt. > > + */ > > + out_8(&i2c->i2c_i2cer, 0xff); > > + > > + /* Get 'me going again. > > + */ > > + wake_up_interruptible(&iic_wait); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static void cpm_iic_init(struct i2c_algo_8xx_data *cpm) > > +{ > > + iic_t *iip = cpm->iip; > > + i2c8xx_t *i2c = cpm->i2c; > > + unsigned char brg; > > + > > + if (cpm_debug) > > + printk(KERN_DEBUG "cpm_iic_init()\n"); > > + > > + /* Initialize the parameter ram. > > + * We need to make sure many things are initialized to > > zero, > > + * especially in the case of a microcode patch. > > + */ > > + iip->iic_rstate = 0; > > + iip->iic_rdp = 0; > > + iip->iic_rbptr = 0; > > + iip->iic_rbc = 0; > > + iip->iic_rxtmp = 0; > > + iip->iic_tstate = 0; > > + iip->iic_tdp = 0; > > + iip->iic_tbptr = 0; > > + iip->iic_tbc = 0; > > + iip->iic_txtmp = 0; > > Maybe a memset on the whole structure would be more efficient? Looks > to me like you're zeroing almost all fields. > This can mangle with microcode patch, so if we'll do memset something could be messed. > > + > > + /* Set up the IIC parameters in the parameter ram. > > + */ > > + iip->iic_tbase = r_tbase = cpm->dp_addr; > > + iip->iic_rbase = r_rbase = cpm->dp_addr + sizeof(cbd_t) * > > 2; + > > + if (cpm_debug) { > > + printk("iip %p, dp_addr 0x%x\n", cpm->iip, > > cpm->dp_addr); > > + printk("iic_tbase %d, r_tbase %d\n", > > iip->iic_tbase, r_tbase); > > + } > > + > > + iip->iic_tfcr = SMC_EB; > > + iip->iic_rfcr = SMC_EB; > > + > > + /* Set maximum receive size. > > + */ > > + iip->iic_mrblr = CPM_MAX_READ; > > + > > + /* Initialize Tx/Rx parameters. > > + */ > > + if (cpm->reloc == 0) { > > + cpm8xx_t *cp = cpm->cp; > > + u16 v = mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_INIT_TRX) > > | CPM_CR_FLG; + > > + out_be16(&cp->cp_cpcr, v); > > + while (in_be16(&cp->cp_cpcr) & CPM_CR_FLG) ; > > This could block, you need to add some form of timeout. > OK. Here and below lies almost a copy of 2.4 code... > > + } else { > > + iip->iic_rbptr = iip->iic_rbase; > > + iip->iic_tbptr = iip->iic_tbase; > > + iip->iic_rstate = 0; > > + iip->iic_tstate = 0; > > + } > > + > > + /* Select an arbitrary address. Just make sure it is > > unique. > > + */ > > + out_8(&i2c->i2c_i2add, 0xfe); > > + > > + /* Make clock run at 60 KHz. > > kHz (small k) > > > + */ > > + brg = (unsigned char)(ppc_proc_freq / (32 * 2 * 60000) - > > 3); > > Useless cast. > > > + out_8(&i2c->i2c_i2brg, brg); > > + > > + out_8(&i2c->i2c_i2mod, 0x00); > > + out_8(&i2c->i2c_i2com, 0x01); /* Master mode */ > > + > > + /* Disable interrupts. > > + */ > > + out_8(&i2c->i2c_i2cmr, 0); > > + out_8(&i2c->i2c_i2cer, 0xff); > > + > > + init_waitqueue_head(&iic_wait); > > + > > + /* Install interrupt handler. > > + */ > > + if (cpm_debug) { > > + printk("%s[%d] Install ISR for IRQ %d\n", > > + __func__, __LINE__, CPMVEC_I2C); > > + } > > + request_irq(cpm->irq, cpm_iic_interrupt, 0, "8xx_i2c", > > i2c); +} > > + > > +static int cpm_iic_shutdown(struct i2c_algo_8xx_data *cpm) > > +{ > > + i2c8xx_t *i2c = cpm->i2c; > > + > > + /* Shut down IIC. > > + */ > > + out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | ~1); > > + out_8(&i2c->i2c_i2cmr, 0); > > + out_8(&i2c->i2c_i2cer, 0xff); > > + > > + return (0); > > +} > > + > > +static void cpm_reset_iic_params(iic_t * iip) > > +{ > > + iip->iic_tbase = r_tbase; > > + iip->iic_rbase = r_rbase; > > + > > + iip->iic_tfcr = SMC_EB; > > + iip->iic_rfcr = SMC_EB; > > + > > + iip->iic_mrblr = CPM_MAX_READ; > > + > > + iip->iic_rstate = 0; > > + iip->iic_rdp = 0; > > + iip->iic_rbptr = iip->iic_rbase; > > + iip->iic_rbc = 0; > > + iip->iic_rxtmp = 0; > > + iip->iic_tstate = 0; > > + iip->iic_tdp = 0; > > + iip->iic_tbptr = iip->iic_tbase; > > + iip->iic_tbc = 0; > > + iip->iic_txtmp = 0; > > +} > > + > > +#define BD_SC_NAK ((ushort)0x0004) /* NAK - > > did not respond */ +#define BD_SC_OV > > ((ushort)0x0002) /* OV - receive overrun */ +#define > > CPM_CR_CLOSE_RXBD ((ushort)0x0007) + > > +static void force_close(struct i2c_algo_8xx_data *cpm) > > +{ > > + i2c8xx_t *i2c = cpm->i2c; > > + if (cpm->reloc == 0) { /* micro code disabled */ > > + cpm8xx_t *cp = cpm->cp; > > + u16 v = mk_cr_cmd(CPM_CR_CH_I2C, > > CPM_CR_CLOSE_RXBD) | CPM_CR_FLG; + > > + if (cpm_debug) > > + printk("force_close()\n"); > > + > > + out_be16(&cp->cp_cpcr, v); > > + while (in_be16(&cp->cp_cpcr) & CPM_CR_FLG) ; > > + } > > + out_8(&i2c->i2c_i2cmr, 0x00); /* Disable all > > interrupts */ > > + out_8(&i2c->i2c_i2cer, 0xff); > > +} > > + > > +/* Read from IIC... > > + * abyte = address byte, with r/w flag already set > > + */ > > +static int > > +cpm_iic_read(struct i2c_algo_8xx_data *cpm, u_char abyte, char > > *buf, int count) +{ > > + iic_t *iip = cpm->iip; > > + i2c8xx_t *i2c = cpm->i2c; > > + cbd_t *tbdf, *rbdf; > > + u_char *tb; > > + unsigned long tmo; > > + int res = 0; > > + > > + if (count >= CPM_MAX_READ) > > + return -EINVAL; > > + > > + /* check for and use a microcode relocation patch */ > > + if (cpm->reloc) { > > + cpm_reset_iic_params(iip); > > + } > > + > > + tbdf = (cbd_t *) cpm_dpram_addr(iip->iic_tbase); > > + rbdf = (cbd_t *) cpm_dpram_addr(iip->iic_rbase); > > + > > + /* 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 = cpm->temp; > > + tb = (u_char *) (((uint) tb + 15) & ~15); > > + tb[0] = abyte; /* Device address byte w/rw > > flag */ + > > + flush_dcache_range((unsigned long)tb, (unsigned long)(tb + > > 1)); + > > + if (cpm_debug) > > + printk("cpm_iic_read(abyte=0x%x)\n", abyte); > > + > > + tbdf->cbd_bufaddr = __pa(tb); > > + tbdf->cbd_datlen = count + 1; > > + tbdf->cbd_sc = BD_SC_READY | BD_SC_LAST | BD_SC_WRAP | > > BD_IIC_START; + > > + iip->iic_mrblr = count + 1; /* prevent excessive > > read, +1 > > + is needed otherwise > > will the > > + RXB interrupt come too > > early */ + > > + /* flush will invalidate too. */ > > + flush_dcache_range((unsigned long)buf, (unsigned long)(buf > > + count)); + > > + rbdf->cbd_datlen = 0; > > + rbdf->cbd_bufaddr = __pa(buf); > > + rbdf->cbd_sc = BD_SC_EMPTY | BD_SC_WRAP | BD_SC_INTRPT; > > + > > + if (count > 16) { > > + /* Chip bug, set enable here */ > > + out_8(&i2c->i2c_i2cmr, 0x13); /* Enable > > some interupts */ > > + out_8(&i2c->i2c_i2cer, 0xff); > > + out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | > > 1); /* Enable */ > > + out_8(&i2c->i2c_i2com, in_8(&i2c->i2c_i2com) | > > 0x80); /* Begin transmission */ + > > + /* Wait for IIC transfer */ > > + res = wait_event_interruptible_timeout(iic_wait, > > 0, 1 * HZ); > > + } else { /* busy wait for small transfers, > > its faster */ > > + out_8(&i2c->i2c_i2cmr, 0x00); /* Disable > > I2C interupts */ > > + out_8(&i2c->i2c_i2cer, 0xff); > > + out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | > > 1); /* Enable */ > > + out_8(&i2c->i2c_i2com, in_8(&i2c->i2c_i2com) | > > 0x80); /* Begin transmission */ + > > + tmo = jiffies + 1 * HZ; > > + while (!(in_8(&i2c->i2c_i2cer) & 0x11 || > > time_after(jiffies, tmo))) ;/* Busy wait, with a timeout */ > > This could result in a one-second busy loop, not very friendly for > other drivers. It should sleep while waiting. Line too long, please > fold. > Can you please elaborate a little here (or just point to the similar code)? I assume we should not block here, handling timeout in a waitqueue... > > + } > > + > > + if ( res < 0) { > > + force_close(cpm); > > + if (cpm_debug) > > + printk("IIC read: timeout!\n"); > > + return -EIO; > > + } > > +#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->i2c_i2mod, in_8(&i2c->i2c_i2mod) | ~1); > > +#endif > > + if (cpm_debug) { > > + printk("tx sc %04x, rx sc %04x\n", tbdf->cbd_sc, > > rbdf->cbd_sc); > > + } > > + > > + if (tbdf->cbd_sc & BD_SC_READY) { > > + printk("IIC read; complete but tbuf ready\n"); > > + force_close(cpm); > > + printk("tx sc %04x, rx sc %04x\n", tbdf->cbd_sc, > > rbdf->cbd_sc); > > + } > > + > > + if (tbdf->cbd_sc & BD_SC_NAK) { > > + if (cpm_debug) > > + printk("IIC read; no ack\n"); > > + return -EREMOTEIO; > > + } > > + > > + if (rbdf->cbd_sc & BD_SC_EMPTY) { > > + /* force_close(cpm); */ > > + if (cpm_debug) { > > + printk("IIC read; complete but rbuf > > empty\n"); > > + printk("tx sc %04x, rx sc %04x\n", > > + tbdf->cbd_sc, rbdf->cbd_sc); > > + } > > + return -EREMOTEIO; > > + } > > + > > + if (rbdf->cbd_sc & BD_SC_OV) { > > + if (cpm_debug) > > + printk("IIC read; Overrun\n"); > > + return -EREMOTEIO;; > > Doubled semi-colon. > > > + } > > + > > + if (cpm_debug) > > + printk("read %d bytes\n", rbdf->cbd_datlen); > > + > > + if (rbdf->cbd_datlen < count) { > > + if (cpm_debug) > > + printk("IIC read; short, wanted %d got > > %d\n", > > + count, rbdf->cbd_datlen); > > + return 0; > > + } > > + > > + return count; > > +} > > + > > +/* Write to IIC... > > + * addr = address byte, with r/w flag already set > > + */ > > +static int > > +cpm_iic_write(struct i2c_algo_8xx_data *cpm, u_char abyte, char > > *buf, int count) +{ > > + iic_t *iip = cpm->iip; > > + i2c8xx_t *i2c = cpm->i2c; > > + cbd_t *tbdf; > > + u_char *tb; > > + unsigned long tmo; > > + int res = 0; > > + > > + /* check for and use a microcode relocation patch */ > > + if (cpm->reloc) { > > + cpm_reset_iic_params(iip); > > + } > > + tb = cpm->temp; > > + tb = (u_char *) (((uint) tb + 15) & ~15); > > + *tb = abyte; /* Device address byte w/rw > > flag */ + > > + flush_dcache_range((unsigned long)tb, (unsigned long)(tb + > > 1)); > > + flush_dcache_range((unsigned long)buf, (unsigned long)(buf > > + count)); + > > + if (cpm_debug) > > + printk("cpm_iic_write(abyte=0x%x)\n", abyte); > > + > > + /* set up 2 descriptors */ > > + > > + > > Doubled blank line. > > > + tbdf = (cbd_t *) cpm_dpram_addr(iip->iic_tbase); > > + > > + tbdf[0].cbd_bufaddr = __pa(tb); > > + tbdf[0].cbd_datlen = 1; > > + tbdf[0].cbd_sc = BD_SC_READY | BD_IIC_START; > > + > > + tbdf[1].cbd_bufaddr = __pa(buf); > > + tbdf[1].cbd_datlen = count; > > + tbdf[1].cbd_sc = BD_SC_READY | BD_SC_INTRPT | BD_SC_LAST | > > BD_SC_WRAP; + > > + if (count > 16) { > > + /* Chip bug, set enable here */ > > + out_8(&i2c->i2c_i2cmr, 0x13); /* Enable > > some interupts */ > > + out_8(&i2c->i2c_i2cer, 0xff); > > + out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | > > 1); /* Enable */ > > + out_8(&i2c->i2c_i2com, in_8(&i2c->i2c_i2com) | > > 0x80); /* Begin transmission */ + > > + /* Wait for IIC transfer */ > > + res = wait_event_interruptible_timeout(iic_wait, > > 0, 1 * HZ); > > + } else { /* busy wait for small transfers, > > its faster */ > > + out_8(&i2c->i2c_i2cmr, 0x00); /* Disable > > I2C interupts */ > > + out_8(&i2c->i2c_i2cer, 0xff); > > + out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | > > 1); /* Enable */ > > + out_8(&i2c->i2c_i2com, in_8(&i2c->i2c_i2com) | > > 0x80); /* Begin transmission */ > > + tmo = jiffies + 1 * HZ; > > + while (!(in_8(&i2c->i2c_i2cer) & 0x12 || > > time_after(jiffies, tmo))) ;/* Busy wait, with a timeout */ > > Same as above, should sleep, and line to long. > > > + } > > + > > + if ( res < 0) { > > + force_close(cpm); > > + if (cpm_debug) > > + printk("IIC write: timeout!\n"); > > + return -EIO; > > + } > > +#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->i2c_i2mod, in_8(&i2c->i2c_i2mod) | ~1); > > +#endif > > + if (cpm_debug) { > > + printk("tx0 sc %04x, tx1 sc %04x\n", > > + tbdf[0].cbd_sc, tbdf[1].cbd_sc); > > + } > > + > > + if (tbdf->cbd_sc & BD_SC_NAK) { > > + if (cpm_debug) > > + printk("IIC write; no ack\n"); > > + return 0; > > + } > > + > > + if (tbdf->cbd_sc & BD_SC_READY) { > > + if (cpm_debug) > > + printk("IIC write; complete but tbuf > > ready\n"); > > + return 0; > > + } > > + > > + return count; > > +} > > + > > +/* See if an IIC address exists.. > > + * addr = 7 bit address, unshifted > > + */ > > +static int cpm_iic_tryaddress(struct i2c_algo_8xx_data *cpm, int > > addr) +{ > > + iic_t *iip = cpm->iip; > > + i2c8xx_t *i2c = cpm->i2c; > > + cbd_t *tbdf, *rbdf; > > + u_char *tb; > > + unsigned long len; > > + int res = 0; > > + > > + if (cpm_debug > 1) > > + printk("cpm_iic_tryaddress(cpm=%p,addr=%d)\n", > > cpm, addr); + > > + /* check for and use a microcode relocation patch */ > > + if (cpm->reloc) { > > + cpm_reset_iic_params(iip); > > + } > > + > > + if (cpm_debug && addr == 0) { > > + printk("iip %p, dp_addr 0x%x\n", cpm->iip, > > cpm->dp_addr); > > + printk("iic_tbase %d, r_tbase %d\n", > > iip->iic_tbase, r_tbase); > > + } > > + > > + tbdf = (cbd_t *) cpm_dpram_addr(iip->iic_tbase); > > + rbdf = (cbd_t *) cpm_dpram_addr(iip->iic_rbase); > > + > > + tb = cpm->temp; > > + tb = (u_char *) (((uint) tb + 15) & ~15); > > + > > + /* do a simple read */ > > + tb[0] = (addr << 1) | 1; /* device address (+ read) > > */ > > + len = 2; > > + > > + flush_dcache_range((unsigned long)tb, (unsigned long)(tb + > > 2)); + > > + tbdf->cbd_bufaddr = __pa(tb); > > + tbdf->cbd_datlen = len; > > + tbdf->cbd_sc = BD_SC_READY | BD_SC_LAST | BD_SC_WRAP | > > BD_IIC_START; + > > + rbdf->cbd_datlen = 0; > > + rbdf->cbd_bufaddr = __pa(tb + 2); > > + rbdf->cbd_sc = BD_SC_EMPTY | BD_SC_WRAP | BD_SC_INTRPT; > > + > > + out_8(&i2c->i2c_i2cmr, 0x13); /* Enable some > > interupts */ > > + out_8(&i2c->i2c_i2cer, 0xff); > > + out_8(&i2c->i2c_i2mod, in_8(&i2c->i2c_i2mod) | > > 1); /* Enable */ > > + out_8(&i2c->i2c_i2com, in_8(&i2c->i2c_i2com) | > > 0x80); /* Begin transmission */ + > > + if (cpm_debug > 1) > > + printk("about to sleep\n"); > > + > > + /* wait for IIC transfer */ > > + res = wait_event_interruptible_timeout(iic_wait, 0, 1 * > > HZ); + > > +#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->i2c_i2mod, in_8(&i2c->i2c_i2mod) | ~1); > > +#endif > > + > > + if ( res < 0) { > > + force_close(cpm); > > + if (cpm_debug) > > + printk("IIC tryaddress: timeout!\n"); > > + return -EIO; > > + } > > + > > + if (cpm_debug > 1) > > + printk("back from sleep\n"); > > + > > + if (tbdf->cbd_sc & BD_SC_NAK) { > > + if (cpm_debug > 1) > > + printk("IIC try; no ack\n"); > > + return 0; > > + } > > + > > + if (tbdf->cbd_sc & BD_SC_READY) { > > + printk("IIC try; complete but tbuf ready\n"); > > + } > > + > > + return 1; > > +} > > + > > +static int cpm_xfer(struct i2c_adapter *adap, struct i2c_msg > > msgs[], int num) > > Use *msgs instead of msgs[]. > > > +{ > > + struct i2c_algo_8xx_data *cpm = adap->algo_data; > > + struct i2c_msg *pmsg; > > + int i, ret; > > + u_char addr; > > + > > + for (i = 0; i < num; i++) { > > + pmsg = &msgs[i]; > > + > > + if (cpm_debug) > > + printk("i2c-algo-8xx.o: " > > + "#%d addr=0x%x flags=0x%x len=%d\n > > buf=%lx\n", > > + i, pmsg->addr, pmsg->flags, > > pmsg->len, > > + (unsigned long)pmsg->buf); > > + > > + addr = pmsg->addr << 1; > > + if (pmsg->flags & I2C_M_RD) > > + addr |= 1; > > + if (pmsg->flags & I2C_M_REV_DIR_ADDR) > > + addr ^= 1; > > + > > + if (!(pmsg->flags & I2C_M_NOSTART)) { > > + } > > Either something is missing here and you must add it, or this > statement is useless and you should drop it. > heh, correct. > > + if (pmsg->flags & I2C_M_RD) { > > + /* read bytes into buffer */ > > + ret = cpm_iic_read(cpm, addr, pmsg->buf, > > pmsg->len); > > + if (cpm_debug) > > + printk("i2c-algo-8xx.o: read %d > > bytes\n", ret); > > + if (ret < pmsg->len) { > > + return (ret < 0) ? ret : > > -EREMOTEIO; > > + } > > + } else { > > + /* write bytes from buffer */ > > + ret = cpm_iic_write(cpm, addr, pmsg->buf, > > pmsg->len); > > + if (cpm_debug) > > + printk("i2c-algo-8xx.o: wrote > > %d\n", ret); > > + if (ret < pmsg->len) { > > + return (ret < 0) ? ret : > > -EREMOTEIO; > > + } > > + } > > + } > > + return (num); > > Useless parentheses. > > > +} > > You do not appear to handle repeated start. I can tell because the > code handles all messages the exact same way, be they the first, > second or last message of a group. This means that you don't really > implement the I2C protocol, but an approximation of it. It might be > sufficient for some I2C chips, but others will break. Look in the > specifications of your device for how this could be fixed. > I doubt 8xx has a full-fledged i2c protocol stuff onboard, and basic code that were residing in 2.4 repo suite my needs quite well (afaict many others just don't care :)). I just think it is silly to drop the code already implemented and working even if it requires some efforts to bring it up to shape. > > + > > +static u32 cpm_func(struct i2c_adapter *adap) > > +{ > > + return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR | > > + I2C_FUNC_PROTOCOL_MANGLING; > > +} > > I2C_FUNC_I2C is missing. I2C_FUNC_10BIT_ADDR isn't implemented so it > shouldn't be advertised. I2C_FUNC_PROTOCOL_MANGLING should only be > implemented if really needed, so I'd suggest that you drop it for now > (together with I2C_M_REV_DIR_ADDR and I2C_M_NOSTART above). > > > + > > +/* -----exported algorithm data: > > ------------------------------------- */ + > > +static struct i2c_algorithm cpm_algo = { > > + .master_xfer = cpm_xfer, > > + .functionality = cpm_func, > > +}; > > + > > +/* > > + * registering functions to load algorithms at runtime > > + */ > > +int i2c_8xx_add_bus(struct i2c_adapter *adap) > > +{ > > + struct i2c_algo_8xx_data *cpm = adap->algo_data; > > + int i; > > + > > + if (cpm_debug) > > + printk("i2c-algo-8xx.o: hw routines for %s > > registered.\n", > > + adap->name); > > + > > + /* register new adapter to i2c module... */ > > + > > + adap->algo = &cpm_algo; > > + > > + i2c_add_adapter(adap); > > + cpm_iic_init(cpm); > > Should be in the reverse order, init first, register second. > > > + > > + /* scan bus */ > > + if (cpm_scan) { > > + printk(KERN_INFO " i2c-algo-8xx.o: scanning bus > > %s...\n", > > + adap->name); > > + for (i = 0; i < 128; i++) { > > + if (cpm_iic_tryaddress(cpm, i)) > > + printk("(%02x)",i<<1); > > + } > > + printk("\n"); > > + } > > As explained above, please remove this. > > > + > > + return 0; > > +} > > + > > +int i2c_8xx_del_bus(struct i2c_adapter *adap) > > +{ > > + struct i2c_algo_8xx_data *cpm = adap->algo_data; > > + > > + cpm_iic_shutdown(cpm); > > + > > + return i2c_del_adapter(adap); > > Should be in the reverse order, unregister first, shutdown second. > > > +} > > + > > +EXPORT_SYMBOL(i2c_8xx_add_bus); > > +EXPORT_SYMBOL(i2c_8xx_del_bus); > > + > > +MODULE_AUTHOR("Brad Parker <[EMAIL PROTECTED]>"); > > +MODULE_DESCRIPTION("I2C-Bus MPC8XX algorithm"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index 4afc795..83d9aef 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -385,8 +385,8 @@ config I2C_PROSAVAGE > > will be called i2c-prosavage. > > > > config I2C_RPXLITE > > - tristate "Embedded Planet RPX Lite/Classic support" > > - depends on RPXLITE || RPXCLASSIC > > + tristate "Embedded Planet RPX Lite/Classic and Freescale > > 86x/885 ads support" > > + depends on RPXLITE || RPXCLASSIC || MPC86XADS || MPC885ADS > > select I2C_ALGO8XX > > Please add a help text. > > > > > config I2C_S3C2410 > > diff --git a/drivers/i2c/busses/i2c-rpx.c > > b/drivers/i2c/busses/i2c-rpx.c index 8764df0..ebddd94 100644 > > --- a/drivers/i2c/busses/i2c-rpx.c > > +++ b/drivers/i2c/busses/i2c-rpx.c > > @@ -14,88 +14,129 @@ > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/init.h> > > +#include <linux/io.h> > > #include <linux/stddef.h> > > +#include <linux/platform_device.h> > > #include <linux/i2c.h> > > #include <linux/i2c-algo-8xx.h> > > #include <asm/mpc8xx.h> > > #include <asm/commproc.h> > > +#include <asm/fs_pd.h> > > > > > > -static void > > -rpx_iic_init(struct i2c_algo_8xx_data *data) > > +struct m8xx_i2c { > > + char *base; > > + struct device *dev; > > + struct i2c_adapter adap; > > + struct i2c_algo_8xx_data *algo_8xx; > > +}; > > + > > +static struct i2c_algo_8xx_data rpx_data; > > + > > +static struct i2c_adapter rpx_ops = { > > Could be const? > prolly yes. > > + .owner = THIS_MODULE, > > + .name = "m8xx", > > Find a better name (e.g. "i2c-rpx"). > What about mpc8xx? > > + .id = I2C_HW_MPC8XX_EPON, > > + .algo_data = &rpx_data, > > +}; > > + > > +static int rpx_iic_init(struct m8xx_i2c *i2c) > > { > > volatile cpm8xx_t *cp; > > - volatile immap_t *immap; > > + struct resource *r; > > + struct i2c_algo_8xx_data *data = i2c->algo_8xx; > > + struct platform_device *pdev = > > to_platform_device(i2c->dev); + > > + cp = data->cp = (cpm8xx_t *)immr_map(im_cpm); /* > > pointer to Communication Processor */ > > Line too long. > > > > > - cp = cpmp; /* Get pointer to Communication > > Processor */ > > - immap = (immap_t *)IMAP_ADDR; /* and to internal > > registers */ > > + data->irq = platform_get_irq_byname(pdev, "interrupt"); > > + > > No blank line between assignation and test please. > > > + if (data->irq < 0) > > + return -EINVAL; > > > > - data->iip = (iic_t *)&cp->cp_dparam[PROFF_IIC]; > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > "pram"); > > + data->iip = ioremap(r->start, r->end - r->start + 1); > > + if (data->iip == NULL) > > + return -EINVAL; > > > > /* Check for and use a microcode relocation patch. > > - */ > > + */ > > if ((data->reloc = data->iip->iic_rpbase)) > > data->iip = (iic_t > > *)&cp->cp_dpmem[data->iip->iic_rpbase]; > > - data->i2c = (i2c8xx_t *)&(immap->im_i2c); > > - data->cp = cp; > > - > > - /* Initialize Port B IIC pins. > > - */ > > - cp->cp_pbpar |= 0x00000030; > > - cp->cp_pbdir |= 0x00000030; > > - cp->cp_pbodr |= 0x00000030; > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > "regs"); > > + data->i2c = ioremap(r->start, r->end - r->start + 1); > > + if (data->i2c == NULL) > > + return -EINVAL; > > > > /* Allocate space for two transmit and two receive buffer > > * descriptors in the DP ram. > > */ > > data->dp_addr = cpm_dpalloc(sizeof(cbd_t) * 4, 8); > > - > > - /* ptr to i2c area */ > > - data->i2c = (i2c8xx_t *)&(((immap_t *)IMAP_ADDR)->im_i2c); > > } > > > > -static int rpx_install_isr(int irq, void (*func)(void *), void > > *data) +static int i2c_rpx_probe(struct device *device) > > { > > - /* install interrupt handler */ > > - cpm_install_handler(irq, func, data); > > - > > - return 0; > > -} > > + int result = 0; > > Useless initialization. > > > + struct m8xx_i2c *i2c; > > + struct platform_device *pdev = to_platform_device(device); > > > > -static struct i2c_algo_8xx_data rpx_data = { > > - .setisr = rpx_install_isr > > -}; > > + if (!(i2c = kmalloc(sizeof(*i2c), GFP_KERNEL))) { > > + return -ENOMEM; > > + } > > + memset(i2c, 0, sizeof(*i2c)); > > Use kzalloc() instead. > > > + i2c->dev = device; > > + i2c->algo_8xx = &rpx_data; > > > > -static struct i2c_adapter rpx_ops = { > > - .owner = THIS_MODULE, > > - .name = "m8xx", > > - .id = I2C_HW_MPC8XX_EPON, > > - .algo_data = &rpx_data, > > -}; > > + rpx_iic_init(i2c); > > > > -int __init i2c_rpx_init(void) > > -{ > > - printk(KERN_INFO "i2c-rpx: i2c MPC8xx driver\n"); > > + dev_set_drvdata(device, i2c); > > > > - /* reset hardware to sane state */ > > - rpx_iic_init(&rpx_data); > > + i2c->adap = rpx_ops; > > + i2c_set_adapdata(&i2c->adap, i2c); > > + i2c->adap.dev.parent = &pdev->dev; > > > > - if (i2c_8xx_add_bus(&rpx_ops) < 0) { > > + if ((result = i2c_8xx_add_bus(&rpx_ops) < 0)) { > > printk(KERN_ERR "i2c-rpx: Unable to register with > > I2C\n"); > > - return -ENODEV; > > + kfree(i2c); > > } > > > > + return result; > > +} > > + > > + > > +static int i2c_rpx_remove(struct device *device) > > +{ > > + struct m8xx_i2c *i2c = dev_get_drvdata(device); > > + > > + i2c_8xx_del_bus(&i2c->adap); > > + dev_set_drvdata(device, NULL); > > + > > + kfree(i2c); > > return 0; > > } > > > > -void __exit i2c_rpx_exit(void) > > + > > +/* Structure for a device driver */ > > +static struct device_driver i2c_rpx_driver = { > > + .name = "fsl-i2c-cpm", > > + .bus = &platform_bus_type, > > + .probe = i2c_rpx_probe, > > + .remove = i2c_rpx_remove, > > +}; > > Why don't you declare it as a struct platform_driver, register it with > platform_driver_register() and unregister it with > platform_driver_unregister()? > Well. This stuff belongs to CPM1, of the mpc8xx family, but the target boards are different, and they may/should provide board specific inits and filling of platform data. With platform_driver_register we may end up with ifdef stuff here (which is evil). > > + > > +static int __init i2c_rpx_init(void) > > { > > - i2c_8xx_del_bus(&rpx_ops); > > + return driver_register(&i2c_rpx_driver); > > } > > > > -MODULE_AUTHOR("Dan Malek <[EMAIL PROTECTED]>"); > > -MODULE_DESCRIPTION("I2C-Bus adapter routines for MPC8xx boards"); > > +static void __exit i2c_rpx_exit(void) > > +{ > > + driver_unregister(&i2c_rpx_driver); > > +} > > > > module_init(i2c_rpx_init); > > module_exit(i2c_rpx_exit); > > + > > +MODULE_AUTHOR("Dan Malek <[EMAIL PROTECTED]>"); > > +MODULE_DESCRIPTION("I2C-Bus adapter routines for MPC8xx boards"); > > diff --git a/include/linux/i2c-algo-8xx.h > > b/include/linux/i2c-algo-8xx.h new file mode 100644 > > index 0000000..a644512 > > --- /dev/null > > +++ b/include/linux/i2c-algo-8xx.h > > @@ -0,0 +1,29 @@ > > +/* > > ------------------------------------------------------------------------- > > */ +/* i2c-algo-8xx.h i2c driver algorithms for MPX8XX > > CPM */ +/* > > ------------------------------------------------------------------------- > > */ + +/* $Id$ */ > > Delete this. > > > + > > +#ifndef I2C_ALGO_8XX_H > > +#define I2C_ALGO_8XX_H > > + > > +#include <linux/i2c.h> > > +#include <asm/8xx_immap.h> > > +#include <asm/commproc.h> > > + > > +struct i2c_algo_8xx_data { > > + uint dp_addr; > > + int reloc; > > + int irq; > > + i2c8xx_t *i2c; > > + iic_t *iip; > > + cpm8xx_t *cp; > > + > > + u_char temp[513]; > > Shouldn't this be CPM_MAX_READ instead of a hard-coded value? > > > +}; > > Alignment in this structure is inconsistent, some lines use tabs other > use spaces. Please don't mix. > I'm OK with this and upper small nits - great thanks for review! > > + > > +extern int i2c_8xx_add_bus(struct i2c_adapter *); > > +extern int i2c_8xx_del_bus(struct i2c_adapter *); > > + > > +#endif /* I2C_ALGO_8XX_H */ > > + > > -- Sincerely, Vitaly - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/