Re: [PATCH 2/2] i2c-au1550: convert to platform driver

[Domen Puncer <domen.puncer@telargo.com>]

On 17/05/07 12:38 +0200, Jean Delvare wrote:
> Hi Manuel,
> 
> On Wed, 16 May 2007 07:34:40 +0200, Manuel Lauss wrote:
> > Convert the i2c-au1550 driver to platform_driver.
> > 
> > * Convert the core i2c-au1550 driver to platform_driver, get rid of
> >   the i2c-au1550.h file, and remove the unused pb1550_wm_codec_write().

The "unused" pb1550_wm_codec_write, is used by out-of-tree cim driver,
which never really worked for me. I guess it (or a nicer solution)
can be a part of cim driver.

> > * register the platform device for Alchemy boards which previously
> >   defined the i2c psc base address in their headers.
> > * update au1x psc header for the new driver style.

It's much easier to review split patches (patch per logical change).

...
> > diff -Naurp linux-2.6.21-a/drivers/i2c/busses/i2c-au1550.c linux-2.6.21-au/drivers/i2c/busses/i2c-au1550.c
> > --- linux-2.6.21-a/drivers/i2c/busses/i2c-au1550.c	2007-05-15 20:32:44.000000000 +0200
> > +++ linux-2.6.21-au/drivers/i2c/busses/i2c-au1550.c	2007-05-15 21:39:34.000000000 +0200
...
> > @@ -110,24 +103,18 @@ wait_master_done(struct i2c_au1550_data 
> >  static int
> >  do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd)
> >  {
> > -	volatile psc_smb_t	*sp;
> > -	u32			stat;
> > -
> > -	sp = (volatile psc_smb_t *)(adap->psc_base);
> > +	u32 stat, base = adap->psc_base;
> >  
> >  	/* Reset the FIFOs, clear events.
> >  	*/
> > -	stat = sp->psc_smbstat;
> > -	sp->psc_smbevnt = PSC_SMBEVNT_ALLCLR;
> > +	stat = au_readl(base + PSC_SMBSTAT);
> > +	au_writel(PSC_SMBEVNT_ALLCLR, base + PSC_SMBEVNT);
> >  	au_sync();
> > -
> >  	if (!(stat & PSC_SMBSTAT_TE) || !(stat & PSC_SMBSTAT_RE)) {
> > -		sp->psc_smbpcr = PSC_SMBPCR_DC;
> > +		au_writel(PSC_SMBPCR_DC, base + PSC_SMBPCR);
> >  		au_sync();
> > -		do {
> > -			stat = sp->psc_smbpcr;
> > -			au_sync();
> > -		} while ((stat & PSC_SMBPCR_DC) != 0);
> > +		while (au_readl(base + PSC_SMBPCR) & PSC_SMBPCR_DC)
> > +			msleep(0);
> 
> You are changing the behavior here, while this patch is supposed to
> only convert the driver to the new device driver model.

Well... since msleep(0) is nothing, it's the same, but it does
look weird.


...
> >  	/* Now, set up the PSC for SMBus PIO mode.
> >  	*/
> > -	sp = (volatile psc_smb_t *)(adap->psc_base);
> > -	sp->psc_ctrl = PSC_CTRL_DISABLE;
> > +	au_writel(PSC_CTRL_DISABLE, base + PSC_CTRL_OFFSET);
> >  	au_sync();
> > -	sp->psc_sel = PSC_SEL_PS_SMBUSMODE;
> > -	sp->psc_smbcfg = 0;
> > +	au_writel(PSC_SEL_PS_SMBUSMODE, base + PSC_SEL_OFFSET);

au_sync();?


...	
> > +	while (!(au_readl(base + PSC_SMBSTAT) & PSC_SMBSTAT_DR))
> >  		au_sync();
> > -	} while ((stat & PSC_SMBSTAT_DR) == 0);
> >  
> > -	return i2c_add_adapter(i2c_adap);
> > -}
> > +	ret = i2c_add_adapter(&priv->adap);
> > +	if (ret == 0) {
> > +		platform_set_drvdata(pdev, priv);
> > +		return 0;
> > +	}
> >  
> > +	au_writel(0, base + PSC_SMBCFG);

au_sync();?

> > +	au_writel(PSC_CTRL_DISABLE, base + PSC_CTRL_OFFSET);
> > +	au_sync();
> >  
> > -int
> > -i2c_au1550_del_bus(struct i2c_adapter *adap)
> > -{
> > -	return i2c_del_adapter(adap);
> > +	kfree(priv);
> > +out:	return ret;
> 
> Label should be on its own line.
> 
> >  }
> >  
> >  static int
> > -pb1550_reg(struct i2c_client *client)
> > +i2c_au1550_remove(struct platform_device *pdev)
> >  {
> > -	return 0;
> > +	struct i2c_au1550_data *priv = platform_get_drvdata(pdev);
> > +	u32 base;
> > +
> > +	if (priv) {
> 
> This test doesn't seem needed, there's no way this function can be
> called if i2c_au1550_probe() didn't succeed, so the driver data must
> have been set.
> 
> > +		platform_set_drvdata(pdev, NULL);
> > +		base = priv->psc_base;
> > +		i2c_del_adapter(&priv->adap);
> > +		au_writel(0, base + PSC_SMBCFG);

au_sync();?

> > +		au_writel(PSC_CTRL_DISABLE, base + PSC_CTRL_OFFSET);
> > +		au_sync();
> > +		kfree(priv);
> > +	}
> > +	return 0;	
> >  }
> >  
> >  static int
> > -pb1550_unreg(struct i2c_client *client)
> > +i2c_au1550_suspend(struct platform_device *pdev, pm_message_t state)
> >  {
> > +	struct i2c_au1550_data *priv = platform_get_drvdata(pdev);
> > +	u32 base = priv->psc_base;
> > +
> > +	au_writel(PSC_CTRL_SUSPEND, base + PSC_CTRL_OFFSET);
> > +	au_sync();
> >  	return 0;
> >  }

I don't think this will work for "hibernate".
Clocks and other settings should be saved and restored on resume, but
maybe board code already does that.


...
> Other than these minor comments, the patch looks really good. Thanks
> for your work. Domen, feel free to add you own comments.

Unfortunately I don't have au1200 based board set-up anymore, so I can't
test this.
It looks OK to me.


	Domen