LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* where is the sk_buff allocation
From: Ming Liu @ 2007-04-22 18:14 UTC (permalink / raw)
  To: linuxppc-embedded

Dear all,
I am reading and understanding the Xilinx Temac device driver. I know that 
hard_start_xmit() is used to transmit the packets. In this function, a 
pointer skb which points to a sk_buff needs to be passed to show the 
location of the socket buffer. But where is that sk_buff allocation 
sentence, and how its address (skb) is passed to the hard_start_xmit() 
function? I want to change the address of the sk_buff in my system and try 
to fit this socket buffer in a pre-defined location.

Thanks for your telling.

BR
Ming

_________________________________________________________________
享用世界上最大的电子邮件系统― MSN Hotmail。  http://www.hotmail.com  

^ permalink raw reply

* Re: [PATCH][RFC] i2c: adds support for i2c bus on 8xx
From: Vitaly Bordug @ 2007-04-22 11:29 UTC (permalink / raw)
  To: Jean Delvare; +Cc: linuxppc-dev@ozlabs.org, lkml
In-Reply-To: <20070421095707.6b42a312@hyperion.delvare>

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@kernel.crashing.org>
> > ---
> > 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@jlc.net).
> > + *
> > +    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@heeltoe.com)
> > + */
> > +
> > +
> > +/* $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@heeltoe.com>");
> > +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@jlc.net>");
> > -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@jlc.net>");
> > +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

^ permalink raw reply

* Re: [PATCH] powerpc pseries eeh: Convert to kthread API
From: Christoph Hellwig @ 2007-04-22 12:31 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: <Andrew Morton, linux-s390, linux-kernel, Christoph Hellwig,
	linuxppc-dev, Paul Mackerras, containers, Oleg Nesterov
In-Reply-To: <11769695763104-git-send-email-ebiederm@xmission.com>

On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xmission.com>
> 
> This patch modifies the startup of eehd to use kthread_run
> not a combination of kernel_thread and daemonize.  Making
> the code slightly simpler and more maintainable.

This one has the same scheme as the various s390 drivers where a thread
is spawned using a workqueue on demand.  I think we should not blindly
convert it but think a litte more about it.

The first question is obviously, is this really something we want?
spawning kernel thread on demand without reaping them properly seems
quite dangerous.

The second question is whether this is the right implementation.
kthread_create already works by using a workqueue to create the thread
and then waits for it.  If we really want to support creating threads
asynchronously on demand we should have a proper API in kthread.c for
this instead of spreading workqueues.

^ permalink raw reply

* Re: [PATCH 7/7] MPIC MSI backend
From: Segher Boessenkool @ 2007-04-22  7:06 UTC (permalink / raw)
  To: Milton Miller; +Cc: linuxppc-dev, linux-pci
In-Reply-To: <e4520bd4be50f2884003a9421863627c@bga.com>

>> Index: msi-new/arch/powerpc/sysdev/mpic_htmsi.c
>
> I just stumbled across drivers/pci/htirq.c, is that of any use?

That file deals with setting up HT APICs.  On all
current PowerPC systems with HT this is done (and
done correctly) by the firmware; there is no reason
to change any of it in the kernel.

mpic_htmsi deals with MSIs that come in over hypertransport
to the MPIC on CPC925/CPC945; this has nothing to
do with HT APICs.


Segher

^ permalink raw reply

* Re: generic check_legacy_ioport
From: Olaf Hering @ 2007-04-22  6:46 UTC (permalink / raw)
  To: Milton Miller; +Cc: linuxppc-dev, paulus
In-Reply-To: <200704220515.l3M5F7eH005962@sullivan.realtime.net>

On Sun, Apr 22, Milton Miller wrote:

> These are all ifdef'd for the driver being built-in.

Yes, silly Kbuild.

> Can we write someting generic instead that finds the isa
> bus, then walks children looking for a direct child that
> has a matching reg to the requested port?

Why bother?
Nothing else has device_type xyz in the device-tree, my patch handles
all cases. The code can be updated when some new board comes along with
different requirements.

^ permalink raw reply

* Re: git tree
From: Grant Likely @ 2007-04-22  5:43 UTC (permalink / raw)
  To: David H. Lynch Jr; +Cc: Linux PPC
In-Reply-To: <462AB3AF.2050509@comcast.net>

On 4/21/07, David H. Lynch Jr <dhlii@comcast.net> wrote:
>     I am looking for something more like how to manage development with
> possibly multiple
>     devices/projects.
>     Not necescarily what is possible, but what Standard Operating
> Procedure for most developers.

The papers on http://www.jdl.com are very good at covering
collaboration with git.  Take a look at them, and then feel free to
ask more questions.

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

^ permalink raw reply

* Re: generic check_legacy_ioport
From: Milton Miller @ 2007-04-22  5:15 UTC (permalink / raw)
  To: Olaf Hering; +Cc: linuxppc-dev, paulus
In-Reply-To: <20070420185107.GA4972@aepfle.de>

On 2007-04-21 at 04:51:07, Olaf Hering wrote:
> check_legacy_ioport makes only sense on PREP, CHRP and pSeries.
> They may have an isa node with PS/2, parport, floppy and serial ports.
> Cell has IPMI, check for that too.
> 
> Remove the check_legacy_ioport call from ppc_md, its not needed anymore.
> Hardware capabilities come from the device-tree.

..
> Index: b/arch/powerpc/kernel/setup-common.c
> ===================================================================
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -498,9 +498,47 @@ void probe_machine(void)
>  
>  int check_legacy_ioport(unsigned long base_port)
>  {
> -	if (ppc_md.check_legacy_ioport == NULL)
> +	struct device_node *np;
> +
> +	switch(base_port) {
> +#ifdef CONFIG_SERIO_I8042
> +	case I8042_DATA_REG:
> +		np = of_find_node_by_type(NULL, "8042");
> +		if (np == NULL)
> +			return -ENODEV;
> +		of_node_put(np);
> +		return 0;
> +#endif
> +#ifdef CONFIG_BLK_DEV_FD
> +	case FDC_BASE: /* FDC1 */
> +		np = of_find_node_by_type(NULL, "fdc");
> +		if (np == NULL)
> +			return -ENODEV;
> +		of_node_put(np);
> +		return 0;
> +#endif
> +#ifdef CONFIG_IPMI_HANDLER
> +	/* IPMI */
> +	case 0xca2:
> +	case 0xca9:
> +	case 0xe4:
> +		np = of_find_node_by_type(NULL, "ipmi");
> +		if (np == NULL)
> +			return -ENODEV;
> +		of_node_put(np);
>  		return 0;
> -	return ppc_md.check_legacy_ioport(base_port);
> +#endif

These are all ifdef'd for the driver being built-in.   Also,
they check that a given type of device exists, but not that
the device is at the expected address.

Can we write someting generic instead that finds the isa
bus, then walks children looking for a direct child that
has a matching reg to the requested port?

If this doesn't catch the cell IPMI case then another
approach is to have a table of (port, expected device type)
and loop rather than the case statement.   

milton

^ permalink raw reply

* Re: [PATCH 5/7] Enable MSI mappings for MPIC
From: Milton Miller @ 2007-04-22  4:53 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, linux-pci
In-Reply-To: <1177200410.6080.26.camel@localhost.localdomain>

On Apr 21, 2007, at 7:06 PM, Benjamin Herrenschmidt wrote:
>> This is just begging to be written in terms of
>> pci_bus_find_ht_capability.  Well,
>> we have pci_bus_find_capability, and pci_find_ht_capability, I'm sure
>> we could
>> create that.   It also means this and the other nearby functions want
>> to be
>> written in terms of a struct pci_bus and config accessors.
>
> A lot of that code has to run way before the PCI config stuff is even
> useable and certainly before the PCI layer data structures have been
> created.... Sucks but that's how it is for now. We might be able to
> tweak and delay init of the APICs etc... but I don't think I can be
> bothered for now as it's a bit of a corner case.
>

I wasn't proposing to make the code wait until the pci layer had
discovered the ht bridge.  Rather I was thinking that a struct
pci_bus instance created for the purpose with only ops and maybe
sysdata initialized.  The purpose being to reuse the generic code.

Maybe I'll have to code something up.   Perhaps when I'm a bit
less overloaded.

milton

^ permalink raw reply

* Re: git tree
From: David H. Lynch Jr @ 2007-04-22  1:00 UTC (permalink / raw)
  To: Grant Likely; +Cc: Linux PPC
In-Reply-To: <528646bc0704170048u25cd535ar783c737bb47d9fdf@mail.gmail.com>

Grant Likely wrote:
> I haven't quite decided yet.  The -temac and -sysace branches are a
> bit of an experiment.  I thought it might be a good idea to maintain
> the drivers in seperate branches so it is easy to get a diff on just
> that driver; but the individual drivers are pretty seperate anyway (in
> different directories).  I think I'll probably drop the -temac and
> -sysace branches, and just maintain all my changes in the -dev branch.
>  The -forupstream branch is specifically for patches that are due to
> go upstream.  I'll add patches there when I think they are suitable
> for mainline, and post them to the list.
    I would greatly appreciate pointers to so reference for using git
for kernel development.
    I have looked through most of the howto's and have a basic
competence with git.

    I am looking for something more like how to manage development with
possibly multiple
    devices/projects.
    Not necescarily what is possible, but what Standard Operating
Procedure for most developers.


   



-- 
Dave Lynch 					  	    DLA Systems
Software Development:  				         Embedded Linux
717.627.3770 	       dhlii@dlasys.net 	  http://www.dlasys.net
fax: 1.253.369.9244 			           Cell: 1.717.587.7774
Over 25 years' experience in platforms, languages, and technologies too numerous to list.

"Any intelligent fool can make things bigger and more complex... It takes a touch of genius - and a lot of courage to move in the opposite direction."
Albert Einstein

^ permalink raw reply

* Re: RFC: powerpc: Start splitting up mmu.h by MMU type
From: Benjamin Herrenschmidt @ 2007-04-22  0:35 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linuxppc-dev, David Gibson
In-Reply-To: <17961.19168.571692.127151@cargo.ozlabs.ibm.com>


> > Specifically it splits the ppc64 specific (or more strictly, the
> > 64-bit hash table specific) parts of asm-powerpc/mmu.h into its own
> > file, asm-powerpc/mmu-hash64/mmu.h.  Similarly, PowerPC 44x
> > definitions are taken from asm-ppc/mmu.h, trimmed down for use in
> > arch/powerpc and place in their own file, asm-powerpc/mmu-44x/mmu.h.
> 
> Why do we want to create a directory per mmu type?  What other files
> would go in there?

Especially if we're ever going to get a different MMU type on 64 bits, I
would really like to try to be able to build a kernel supporting both
(if realistic).

Ben.

^ permalink raw reply

* Re: [PATCH 5/7] Enable MSI mappings for MPIC
From: Benjamin Herrenschmidt @ 2007-04-22  0:06 UTC (permalink / raw)
  To: Milton Miller; +Cc: linuxppc-dev, linux-pci
In-Reply-To: <0846c0d8efafe7258075826b9c869fea@bga.com>


> This is just begging to be written in terms of 
> pci_bus_find_ht_capability.  Well,
> we have pci_bus_find_capability, and pci_find_ht_capability, I'm sure 
> we could
> create that.   It also means this and the other nearby functions want 
> to be
> written in terms of a struct pci_bus and config accessors.

A lot of that code has to run way before the PCI config stuff is even
useable and certainly before the PCI layer data structures have been
created.... Sucks but that's how it is for now. We might be able to
tweak and delay init of the APICs etc... but I don't think I can be
bothered for now as it's a bit of a corner case.

Ben.

^ permalink raw reply

* Re: [PATCH 7/7] MPIC MSI backend
From: Milton Miller @ 2007-04-21 23:17 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, linux-pci
In-Reply-To: <20070419073556.73A6BDDE4A@ozlabs.org>

On Apr 19, 2007, Michael Ellerman wrote:
> MPIC MSI backend. Based on code from Segher, heavily hacked by me.
> Renamed to mpic_htmsi, as it only deals with MSI over Hypertransport.
...
> Index: msi-new/arch/powerpc/sysdev/Makefile
> ===================================================================
> --- msi-new.orig/arch/powerpc/sysdev/Makefile
> +++ msi-new/arch/powerpc/sysdev/Makefile
> @@ -4,6 +4,7 @@ endif
>
>  mpic-obj-y			:= mpic.o
>  mpic-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o
> +mpic-obj-$(CONFIG_PCI_MSI)	+= mpic_htmsi.o
>  obj-$(CONFIG_MPIC)		+= $(mpic-obj-y)

This is overly complicated.  You aren't going to be making this
a seperate link object anyways.   It should end up somehting like

mpic-msi-$(CONFIG_PCI_MSI)	:= mpic_msi.o mpic_htmsi.o
obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-y)

> -static void mpic_unmask_irq(unsigned int irq)
> +void mpic_unmask_irq(unsigned int irq)
...
> -static void mpic_mask_irq(unsigned int irq)
> +void mpic_mask_irq(unsigned int irq)
...
Ok mpic.c is big enough already.

> Index: msi-new/arch/powerpc/sysdev/mpic.h
> ===================================================================
> --- msi-new.orig/arch/powerpc/sysdev/mpic.h
> +++ msi-new/arch/powerpc/sysdev/mpic.h
> @@ -11,14 +11,23 @@
>   *
>   */
>
> +#include <linux/irq.h>
> +

Why do you need this now?   You already had uses of irq_ht_umber_t
and struct mpic.  Unless this should be in the earlier patch?

>  #ifdef CONFIG_PCI_MSI
>  extern void mpic_msi_reserve_hwirq(struct mpic *mpic, irq_hw_number_t 
> hwirq);
> +extern int mpic_htmsi_init(struct mpic *mpic);
>  #else
>  static inline void mpic_msi_reserve_hwirq(struct mpic *mpic,
>  					  irq_hw_number_t hwirq)
>  {
>  	return;
>  }
> +static inline int mpic_htmsi_init(struct mpic *mpic) { return -1; }

more lines please

>  #endif
>
> +extern int mpic_set_irq_type(unsigned int virq, unsigned int 
> flow_type);
> +extern void mpic_end_irq(unsigned int irq);
> +extern void mpic_mask_irq(unsigned int irq);
> +extern void mpic_unmask_irq(unsigned int irq);
> +
>  #endif /* _POWERPC_SYSDEV_MPIC_H */
> Index: msi-new/arch/powerpc/sysdev/mpic_htmsi.c

I just stumbled across drivers/pci/htirq.c, is that of any use?


> +
> +int mpic_htmsi_init(struct mpic *mpic)
> +{
> +	int rc;
> +
> +	rc = mpic_msi_init_allocator(mpic);
> +	if (rc) {
> +		pr_debug("htmsi: Error allocating bitmap!\n");
> +		return rc;
> +	}
> +
> +	pr_debug("htmsi: Registering MPIC MSI callbacks.\n");
> +
> +	BUG_ON(msi_mpic);
> +	msi_mpic = mpic;
> +
> +	ppc_md.setup_msi_irqs = htmsi_setup_msi_irqs;
> +	ppc_md.teardown_msi_irqs = htmsi_teardown_msi_irqs;
> +	ppc_md.msi_check_device = htmsi_msi_check_device;

Unlike the rtas patch, this one didn't advertise it was
going to set the arch hooks unconditionally.

milton

^ permalink raw reply

* Re: [PATCH 6/7] MPIC MSI allocator
From: Milton Miller @ 2007-04-21 23:17 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, linux-pci
In-Reply-To: <20070419073555.D2C3EDDEFD@ozlabs.org>

On Apr 19, 2007, Michael Ellerman wrote:
> To support MSI on MPIC we need a way to reserve and allocate hardware 
> irq
> numbers, this patch implements an allocator for that.

> Index: msi-new/arch/powerpc/sysdev/mpic_msi.c
> ===================================================================
> --- /dev/null
> +++ msi-new/arch/powerpc/sysdev/mpic_msi.c
...
> +irq_hw_number_t mpic_msi_alloc_hwirqs(struct mpic *mpic, int num)
> +{
> +	unsigned long flags;
> +	int offset, order = fls(num) - 1;

get_count_order would be more clear.  Also it has
a correction factor.  (Applies several places).

> +#ifdef CONFIG_MPIC_BROKEN_U3
> +static int mpic_msi_reserve_u3_hwirqs(struct mpic *mpic)
> +{
> +	irq_hw_number_t hwirq;
> +	struct irq_host_ops *ops = mpic->irqhost->ops;
> +	struct device_node *np;
> +	int flags, index, i;
> +	struct of_irq oirq;
> +
> +	pr_debug("mpic: found U3, guessing msi allocator setup\n");
> +
> +	/* Reserve source numbers we know are reserved in the HW */
> +	for (i = 0;   i < 8;   i++) __mpic_msi_reserve_hwirq(mpic, i);
> +	for (i = 42;  i < 46;  i++) __mpic_msi_reserve_hwirq(mpic, i);
> +	for (i = 100; i < 105; i++) __mpic_msi_reserve_hwirq(mpic, i);

More lines please.

> +#else
> +static int mpic_msi_reserve_u3_hwirqs(struct mpic *mpic) { return -1; 
> }

and here.

> +	if (len % 8 != 0) {
> +		printk(KERN_WARNING "mpic: Malformed msi-available-ranges "
> +		       "property on %s\n", mpic->of_node->full_name);
> +		return -EINVAL;
> +	}
> +
> +	bitmap_allocate_region(mpic->hwirq_bitmap, 0,
> +			       fls(mpic->irq_count) - 1);
> +
> +	/* Format is: (<u32 start> <u32 count>)+ */
> +	len /= sizeof(u32);
> +	for (i = 0; i < len / 2; i++, p += 2)

how about just dividing by the calculated 8 above?  or use
count = len / 8.

> +		mpic_msi_free_hwirqs(mpic, *p, *(p + 1));
> +
> +	return 0;
> +}
> +
> +int mpic_msi_init_allocator(struct mpic *mpic)
> +{
> +	int rc, size;
> +
> +	BUG_ON(mpic->hwirq_bitmap);
> +	spin_lock_init(&mpic->bitmap_lock);
> +
> +	size = mpic->irq_count / 8;
> +	pr_debug("mpic: allocator bitmap size is 0x%x bytes\n", size);

BITS_TO_LONGS() * sizeof(long).   We need to round up to longs to
use bitmask_*, especially being a big endian architecture.

milton

^ permalink raw reply

* Re: [PATCH 5/7] Enable MSI mappings for MPIC
From: Milton Miller @ 2007-04-21 23:16 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, linux-pci
In-Reply-To: <20070419073555.01990DDECB@ozlabs.org>

On Apr 19, 2007, Michael Ellerman wrote:
> On some Apple machines the HT MSI mappings are not enabled by 
> firmware, so
> we need to do it by hand.
>

> +#ifdef CONFIG_PCI_MSI
> +static void __init mpic_scan_ht_msi(struct mpic *mpic, u8 __iomem 
> *devbase,
> +				    unsigned int devfn)
> +{
> +	u8 __iomem *base;
> +	u8 pos, flags;
> +	u64 addr = 0;
> +
> +	for (pos = readb(devbase + PCI_CAPABILITY_LIST); pos != 0;
> +	     pos = readb(devbase + pos + PCI_CAP_LIST_NEXT)) {
> +		u8 id = readb(devbase + pos + PCI_CAP_LIST_ID);
> +		if (id == PCI_CAP_ID_HT) {
> +			id = readb(devbase + pos + 3);
> +			if ((id & HT_5BIT_CAP_MASK) == HT_CAPTYPE_MSI_MAPPING)
>

This is just begging to be written in terms of 
pci_bus_find_ht_capability.  Well,
we have pci_bus_find_capability, and pci_find_ht_capability, I'm sure 
we could
create that.   It also means this and the other nearby functions want 
to be
written in terms of a struct pci_bus and config accessors.

milton

^ permalink raw reply

* Re: [PATCH 2/7] Powerpc MSI infrastructure
From: Milton Miller @ 2007-04-21 23:15 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, linux-pci
In-Reply-To: <20070419073553.5F2ABDDE3C@ozlabs.org>

On Apr 19, 2007, Michael Ellerman wrote:
> This patch provides the architecture specific hooks to support MSI on
> powerpc. We implement the newly added arch_setup_msi_irqs() and
> arch_teardown_msi_irqs(), and then delegate to ppc_md routines.
>

> Index: msi-new/arch/powerpc/kernel/msi.c
> ===================================================================
> --- /dev/null
> +++ msi-new/arch/powerpc/kernel/msi.c
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright 2006-2007, Michael Ellerman, IBM Corporation.
> + *
> + * 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/kernel.h>
> +#include <linux/msi.h>
> +
> +#include <asm/machdep.h>
> +
> +int arch_msi_check_device(struct pci_dev* dev, int nvec, int type)
> +{
> +	if (ppc_md.msi_check_device) {
> +		pr_debug("msi: Using platform check routine.\n");
> +		return ppc_md.msi_check_device(dev, nvec, type);
> +	}
> +
> +	if (!ppc_md.setup_msi_irqs || !ppc_md.teardown_msi_irqs) {
> +		pr_debug("msi: Platform doesn't provide MSI callbacks.\n");
> +		return -ENOSYS;
> +	}

Should we not do this check first?  Or do you expect some platform
to fill out these hooks in the check call above?  Othewise we will
branch to function pointer NULL.

> +
> +        return 0;
> +}
> +
> +int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> +{
> +	return ppc_md.setup_msi_irqs(dev, nvec, type);
> +}
> +
> +void arch_teardown_msi_irqs(struct pci_dev *dev)
> +{
> +	return ppc_md.teardown_msi_irqs(dev);
> +}


> Index: msi-new/arch/powerpc/kernel/Makefile
> ===================================================================
> --- msi-new.orig/arch/powerpc/kernel/Makefile
> +++ msi-new/arch/powerpc/kernel/Makefile

> +obj-$(CONFIG_PCI_MSI)		+= msi.o


Do we really need a new file for these simple hooks?  It doesn't look
like anthing else is going to be added to it either.   I'd prefer to
just do an ifdef in pci.c.   Hmmm, we don't seem to have that, but
these are soo small I'd include machdep.h and put them inline in
asm-powerpc/pci.h.

milton

^ permalink raw reply

* Re: [PATCH 0/5] Support 64K pages mapping of SPE local stores on 4K kernel
From: Benjamin Herrenschmidt @ 2007-04-21 22:44 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: linuxppc-dev, cbe-oss-dev
In-Reply-To: <200704212233.34521.arnd.bergmann@de.ibm.com>

On Sat, 2007-04-21 at 22:33 +0200, Arnd Bergmann wrote:
> On Friday 02 March 2007, Benjamin Herrenschmidt wrote:
> > This serie of patches supports userland mappings of SPE local stores
> > using 64K hardware pages rather than 4K on a kernel using 4K pages to
> > improve performances.
> 
> Ben, I have problems porting these patches to powerpc.git#for-2.6.22.
> Could you port them yourself and submit them to Paul? What is the
> status of the second patch, do you have a version suitable for upstream?

I have more up to date versions of these, I'll do the port. The generic
change needs to be done differently for upstream. I've been posting
regulary a serie of patches that change get_unmapped_area() for that
which I hope will get upstream but I yet to have some feedback from
akpm...

Ben.

^ permalink raw reply

* [POWERPC] Fix comment typo in Kurobox device tree
From: Grant Likely @ 2007-04-21 21:24 UTC (permalink / raw)
  To: linuxppc-dev, G. Liakhovetski, paulus

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
 arch/powerpc/boot/dts/kuroboxHD.dts |    6 +++---
 arch/powerpc/boot/dts/kuroboxHG.dts |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/boot/dts/kuroboxHD.dts b/arch/powerpc/boot/dts/kuroboxHD.dts
index 373110d..157dc98 100644
--- a/arch/powerpc/boot/dts/kuroboxHD.dts
+++ b/arch/powerpc/boot/dts/kuroboxHD.dts
@@ -125,17 +125,17 @@ build with: "dtc -f -I dts -O dtb -o kuroboxHD.dtb -V 16 kuroboxHD.dts"
 			interrupt-parent = <4400>;
 			interrupt-map-mask = <f800 0 0 7>;
 			interrupt-map = <
-				/* IDSEL 0x11 - IRQ0 ETH */
+				/* IDSEL 11 - IRQ0 ETH */
 				5800 0 0 1 4400 0 1
 				5800 0 0 2 4400 1 1
 				5800 0 0 3 4400 2 1
 				5800 0 0 4 4400 3 1
-				/* IDSEL 0x12 - IRQ1 IDE0 */
+				/* IDSEL 12 - IRQ1 IDE0 */
 				6000 0 0 1 4400 1 1
 				6000 0 0 2 4400 2 1
 				6000 0 0 3 4400 3 1
 				6000 0 0 4 4400 0 1
-				/* IDSEL 0x14 - IRQ3 USB2.0 */
+				/* IDSEL 14 - IRQ3 USB2.0 */
 				7000 0 0 1 4400 3 1
 				7000 0 0 2 4400 3 1
 				7000 0 0 3 4400 3 1
diff --git a/arch/powerpc/boot/dts/kuroboxHG.dts b/arch/powerpc/boot/dts/kuroboxHG.dts
index f3542dc..919eb29 100644
--- a/arch/powerpc/boot/dts/kuroboxHG.dts
+++ b/arch/powerpc/boot/dts/kuroboxHG.dts
@@ -125,17 +125,17 @@ build with: "dtc -f -I dts -O dtb -o kuroboxHG.dtb -V 16 kuroboxHG.dts"
 			interrupt-parent = <4400>;
 			interrupt-map-mask = <f800 0 0 7>;
 			interrupt-map = <
-				/* IDSEL 0x11 - IRQ0 ETH */
+				/* IDSEL 11 - IRQ0 ETH */
 				5800 0 0 1 4400 0 1
 				5800 0 0 2 4400 1 1
 				5800 0 0 3 4400 2 1
 				5800 0 0 4 4400 3 1
-				/* IDSEL 0x12 - IRQ1 IDE0 */
+				/* IDSEL 12 - IRQ1 IDE0 */
 				6000 0 0 1 4400 1 1
 				6000 0 0 2 4400 2 1
 				6000 0 0 3 4400 3 1
 				6000 0 0 4 4400 0 1
-				/* IDSEL 0x14 - IRQ3 USB2.0 */
+				/* IDSEL 14 - IRQ3 USB2.0 */
 				7000 0 0 1 4400 3 1
 				7000 0 0 2 4400 3 1
 				7000 0 0 3 4400 3 1
-- 
1.5.1

^ permalink raw reply related

* Re: [PATCH 0/5] Support 64K pages mapping of SPE local stores on 4K kernel
From: Arnd Bergmann @ 2007-04-21 20:33 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: cbe-oss-dev
In-Reply-To: <1172835163.743765.616483133775.qpush@grosgo>

On Friday 02 March 2007, Benjamin Herrenschmidt wrote:
> This serie of patches supports userland mappings of SPE local stores
> using 64K hardware pages rather than 4K on a kernel using 4K pages to
> improve performances.

Ben, I have problems porting these patches to powerpc.git#for-2.6.22.
Could you port them yourself and submit them to Paul? What is the
status of the second patch, do you have a version suitable for upstream?

	Arnd <><

^ permalink raw reply

* Re: Problem cloning Linus' GIT tree!
From: David Woodhouse @ 2007-04-21 19:23 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linuxppc-embedded
In-Reply-To: <200704171532.00215.laurent.pinchart@technotrade.biz>

On Tue, 2007-04-17 at 15:31 +0200, Laurent Pinchart wrote:
> I'm not sure about that. The http repository is not broken, it's just 
> stripped. The http:// protocol handler in git might need to be fixed. 

The http:// protocol handler is rarely going to work properly. If your
firewall is broken, I would suggest just fixing it.

-- 
dwmw2

^ permalink raw reply

* Re: [PATCH 1/4 v2] powerpc: document max-speed and interface-type properties
From: Segher Boessenkool @ 2007-04-21 17:22 UTC (permalink / raw)
  To: Andy Fleming; +Cc: linuxppc-dev
In-Reply-To: <6358CE18-B118-4BC1-B46A-D485449273D5@freescale.com>

>> Sure there's an argument for describing what type of
>> interface the PHY is connected on, if it supports more
>> than one -- but that should be a property in the PHY
>> node, not the controller node, since you can have multiple
>> PHYs connected to the same controller, possibly on
>> different interfaces each.
>
> I just want to reiterate that I disagree strongly with this statement. 
>  If you have multiple PHYs hooked up to one ethernet controller, 
> you're going to need to change the device tree to use a different PHY, 
> anyway.  That, or have Linux ignore the phandle that points to the 
> connected PHY.

On some configurations, what PHY to use is a runtime
decision, not a static thing.  No device tree change
is necessary then.

> And if you put it in the PHY node, you haven't really helped out the 
> people who can change the interface type by flipping a dip switch.  I 
> can think of two or three boards off the top of my head that do that 
> (though I know of very few people who actually use this 
> functionality).

Either the firmware can detect the switch setting
and create a proper device tree, or the user will
need to create the right static device tree.

> My point, again, is that the interface type is not strongly tied to 
> the PHY.  It is strongly tied to the board configuration.  We *could* 
> put the interface type in the PHY node, but I want to disabuse you of 
> the notion that it would be any better there.

I still think it's the best place for this information.


Segher

^ permalink raw reply

* Re: POSIX semaphores on ppc_6xx
From: Arnd Bergmann @ 2007-04-21 15:33 UTC (permalink / raw)
  To: linuxppc-embedded, bwarren
In-Reply-To: <1177091726.3904.21.camel@saruman.qstreams.net>

On Friday 20 April 2007, Ben Warren wrote:
> The cross-compiled glibc that is included in ELDK 4.0 doesn't seem to
> have support for POSIX semaphores. =A0I get the following runtime errors:
>=20
> =A0 =A0sem_open: Function not implemented
>=20
> when trying to implement a named semaphore, and=20
>=20
> =A0 =A0sem_init: Function not implemented
>=20
> when trying to implement an unnamed semaphore.
>=20
> Does anybody know if POSIX semaphores have been ported to ppc_6xx, and
> if so, where I could find a binary copy?

This normally works in every modern glibc. Maybe you were using an
set of kernel headers that is too old. Make sure that you have
up-to-date version of both kernel and glibc and that you use the
kernel header files you get from 'make headers_install' in the kernel.

	Arnd <><

^ permalink raw reply

* Re: [PATCH][RFC] i2c: adds support for i2c bus on 8xx
From: Jean Delvare @ 2007-04-21  7:57 UTC (permalink / raw)
  To: Vitaly Bordug; +Cc: linuxppc-dev@ozlabs.org, lkml
In-Reply-To: <20070420082714.4f10f186@localhost.localdomain>

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

> Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
> ---
> 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.

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

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

> @@ -0,0 +1,622 @@
> +/*
> + * i2c-algo-8xx.c i2x driver algorithms for MPC8XX CPM
> + * Copyright (c) 1999 Dan Malek (dmalek@jlc.net).
> + *
> +    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@heeltoe.com)
> + */
> +
> +
> +/* $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.

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

> +	} 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.

> +	}
> +
> +	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.

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

> +
> +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@heeltoe.com>");
> +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?

> +	.owner		= THIS_MODULE,
> +	.name		= "m8xx",

Find a better name (e.g. "i2c-rpx").

> +	.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()?

> +
> +static int __init i2c_rpx_init(void)
>  {
> -	i2c_8xx_del_bus(&rpx_ops);
> +	return driver_register(&i2c_rpx_driver);
>  }
>  
> -MODULE_AUTHOR("Dan Malek <dmalek@jlc.net>");
> -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@jlc.net>");
> +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.

> +
> +extern int i2c_8xx_add_bus(struct i2c_adapter *);
> +extern int i2c_8xx_del_bus(struct i2c_adapter *);
> +
> +#endif /* I2C_ALGO_8XX_H */
> +


-- 
Jean Delvare

^ permalink raw reply

* Re: [RFC/PATCH] Change 440GP platform to ppc440
From: Paul Mackerras @ 2007-04-21  0:20 UTC (permalink / raw)
  To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <Pine.LNX.4.64.0704191615040.25069@localhost.localdomain>

Kumar Gala writes:

> I don't see any reason the 440GP should its own platform.  Posting this to
> see if anyone thinks otherwise.

I recall dimly that there was some user-visible difference between the
440GP and the other 440 processors, and that's why we made the
platform string different.  I don't recall what the difference was.

Paul.

^ permalink raw reply

* Re: RFC: powerpc: Start splitting up mmu.h by MMU type
From: Paul Mackerras @ 2007-04-20 23:21 UTC (permalink / raw)
  To: David Gibson; +Cc: linuxppc-dev
In-Reply-To: <20070419065430.GA5862@localhost.localdomain>

David Gibson writes:

> This patch makes a start to sorting out the tangled mess of MMU
> related header files in asm-powerpc, and weaning arch/powerpc off the
> remaining asm-ppc headers it uses.

Good...

> Specifically it splits the ppc64 specific (or more strictly, the
> 64-bit hash table specific) parts of asm-powerpc/mmu.h into its own
> file, asm-powerpc/mmu-hash64/mmu.h.  Similarly, PowerPC 44x
> definitions are taken from asm-ppc/mmu.h, trimmed down for use in
> arch/powerpc and place in their own file, asm-powerpc/mmu-44x/mmu.h.

Why do we want to create a directory per mmu type?  What other files
would go in there?

Paul.

^ permalink raw reply

* mpc5200 linux 2.4 wakeup interrupt problem
From: RonLee @ 2007-04-20 22:07 UTC (permalink / raw)
  To: linuxppc-embedded


Hello,
 
I am running linux on a mpc5200 chip and Im having problems with the
mpc5200's simple and wakeup interrupts.  The version of Linux I am starting
with is linuxppc_2_4_devel_1_0_0 from Denx.  Modifications have been made to
the kernel, but none for enabling the wakeup register set and/or the wakeup
interrupts.
 
Does a patch exist to enable this behavior?  Can anyone point me in the
right direction if I would like to enable the wakeup interrupts myself?
 
I noticed the behavior when attempting to write a device driver to interface
with the psc1_4 pin on the mpc5200 chip.  I am able to get the chip to
generate interrupts, but when the interrupts occur I get the following error
message in /proc/kmsg "Unhandled interrupt c, disabled".  After getting this
error, I installed an interrupt handler on IRQ 12 (hex c).  This caused the
kernel to lock up.
 
Thanks for any help,
-Ron


-- 
View this message in context: http://www.nabble.com/mpc5200-linux-2.4-wakeup-interrupt-problem-tf3620873.html#a10111122
Sent from the linuxppc-embedded mailing list archive at Nabble.com.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox