Re: [PATCH v3] i2c: add Renesas R-Car I2C driver

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2701 bytes --]

On Thu, Sep 27, 2012 at 11:44:25PM -0700, Kuninori Morimoto wrote:
> R-Car I2C is similar with SH7760 I2C.
> But the SH7760 I2C driver had many workaround operations, since H/W had bugs.
> Thus, it was pointless to keep compatible between SH7760 and R-Car I2C drivers.
> This patch creates new Renesas R-Car I2C driver.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>

Only minor issues which might be fixed later. Applied to -next.

> +static void rcar_i2c_write(struct rcar_i2c_priv *priv, int reg, u32 val)
> +{
> +	writel(val, priv->io + reg);
> +}
> +
> +static u32 rcar_i2c_read(struct rcar_i2c_priv *priv, int reg)
> +{
> +	return readl(priv->io + reg);
> +}

Not sure if those needed encapsulation.

...

> +static int rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
> +{
> +	struct i2c_msg *msg = priv->msg;
> +
> +	/*
> +	 * FIXME
> +	 * sometimes, unknown interrupt happened.
> +	 * Do nothing
> +	 */
> +	if (!(msr & MDE))
> +		return 0;

Uh, is this a known hardware flaw?

...

> +static int __devinit rcar_i2c_probe(struct platform_device *pdev)
> +{
> +	struct i2c_rcar_platform_data *pdata = pdev->dev.platform_data;
> +	struct rcar_i2c_priv *priv;
> +	struct i2c_adapter *adap;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	u32 bus_speed;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "no mmio resources\n");
> +		return -ENODEV;
> +	}
> +
> +	priv = devm_kzalloc(dev, sizeof(struct rcar_i2c_priv), GFP_KERNEL);
> +	if (!priv) {
> +		dev_err(dev, "no mem for private data\n");
> +		return -ENOMEM;
> +	}
> +
> +	bus_speed = 100000; /* default 100 kHz */
> +	if (pdata && pdata->bus_speed)
> +		bus_speed = pdata->bus_speed;
> +	ret = rcar_i2c_clock_calculate(priv, bus_speed, dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	priv->io = devm_ioremap(dev, res->start, resource_size(res));

devm_request_mem_region is missing. Or use the helper
devm_request_and_ioremap.

> +	if (!priv->io) {
> +		dev_err(dev, "cannot ioremap\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	init_waitqueue_head(&priv->wait);
> +	spin_lock_init(&priv->lock);
> +
> +	adap			= &priv->adap;
> +	adap->nr		= pdev->id;
> +	adap->algo		= &rcar_i2c_algo;
> +	adap->class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +	adap->retries		= 3;

Do you really need the class?

Rest looks good.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]