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

[Paul M. <le...@li...>]

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?

> +/* 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);

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

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

Useless cast.

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

Once those are tidied up, feel free to add my Acked-by.