Re: [RFC PATCH] I2C bus driver for SH7760 SoC

[Manuel L. <ma...@ro...>]

Hi Paul,

On Tue, Oct 30, 2007 at 09:27:17AM +0900, Paul Mundt wrote:
> Basically this looks good to me, though a few minor nits:
> 
> On Thu, Oct 25, 2007 at 08:23:27PM +0200, Manuel Lauss wrote:
> > +struct cami2c {
> > +	void __iomem *iobase;	/* channel base address */
> > +	struct i2c_adapter adap;
> > +	u32 func;		/* supported functions */
> > +	int irq;		/* IRQ vector */
> > +	int chan;		/* channel number (0 or 1) */
> > +
> chan should be a bitfield? Or dynamically calculated based on the number
> of registered platform devices?

It's useless junk now. Removed.

 
> > +/* calculate CCR register setting for a desired scl clock */
> > +static int __devinit calc_CCR(unsigned long scl_freq)
> > +{
> > +	struct clk *mclk;
> > +	unsigned long mck, m1, dff, odff, iclk;
> > +	signed char cdf, cdfm = 0;
> > +	int scgd, scgdm;
> > +
> > +	mclk = clk_get(NULL, "module_clk");
> > +	if (!mclk) {
> > +		mck = CONFIG_SH_PCLK_FREQ;
> 
> If the module_clk isn't valid, the system is going to be in bad enough
> shape as it is. You should error out on this rather than using
> SH_PCLK_FREQ directly. ie,
> 
> 	if (IS_ERR(mclk))
> 		return PTR_ERR(mclk);

done.


> > +	id->iobase = (void __iomem *)res->start;
> 
> You should be ioremap()'ing or ioport_map()'ing this. Either one should
> be sane here. Presently this will whine if you use 64-bit resources. The
> existing in-tree users that do this casting need to be changed also.

Hm, who'd want to enable 64bit resources on a CPU with a 29bit external
address space? ;)
Added ioremap() for good measure.

 
> > +out2:	free_irq(id->irq, (void *)id);
> 
> Useless cast.

Gone.


> > +static int __devexit sh7760_i2c_remove(struct platform_device *pdev)
> > +{
> > +	struct cami2c *id = platform_get_drvdata(pdev);
> > +
> > +	if (id) {
> > +		i2c_del_adapter(&id->adap);
> > +		free_irq(id->irq, (void *)id);
> 
> Likewise.

Gone too.

Thank you!
	Manuel Lauss