Re: [PATCH][RFC] i2c: adds support for i2c bus on 8xx

From: Vitaly Bordug
Date: Sun Apr 22 2007 - 14:13:22 EST

Next message: David Brownell: "Re: gtod/clocksource/clockevents documentation"
Previous message: Junio C Hamano: "Today's 'master' leaves .idx/.pack in 0400"
In reply to: Jean Delvare: "Re: [PATCH][RFC] i2c: adds support for i2c bus on 8xx"
Next in thread: Jean Delvare: "Re: [PATCH][RFC] i2c: adds support for i2c bus on 8xx"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <vitb@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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>;
> > };
> > + i2c@860 {
> > + 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 (dmalek@xxxxxxx).
> > + *
> > + 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 (brad@xxxxxxxxxxx)
> > + */
> > +
> > +
> > +/* $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 <brad@xxxxxxxxxxx>");
> > +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 <dmalek@xxxxxxx>");
> > -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 <dmalek@xxxxxxx>");
> > +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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: David Brownell: "Re: gtod/clocksource/clockevents documentation"
Previous message: Junio C Hamano: "Today's 'master' leaves .idx/.pack in 0400"
In reply to: Jean Delvare: "Re: [PATCH][RFC] i2c: adds support for i2c bus on 8xx"
Next in thread: Jean Delvare: "Re: [PATCH][RFC] i2c: adds support for i2c bus on 8xx"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]