Re: [PATCH v3 2/2] i2c: Add Renesas RZ/V2M controller

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Jul 01, 2022 at 05:39:16PM +0100, Phil Edworthy wrote:
> Yet another i2c controller from Renesas that is found on the RZ/V2M
> (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.

...

> +		/* 10-bit address
> +		 *   addr_1: 5'b11110 | addr[9:8] | (R/nW)
> +		 *   addr_2: addr[7:0]
> +		 */

/*
 * Multi-line comments should be
 * formatted like in this example.
 * (Of course use as much space up
 * to 80 as possible.)
 */

Ditto for other done in similar way, if any.

...

> +		addr = 0xf0 | ((msg->addr >> 7) & 0x06);

GENMASK() ?

...

> +	if (!ret) {
> +		if (read)
> +			ret = rzv2m_i2c_receive(priv, msg, &count);
> +		else
> +			ret = rzv2m_i2c_send(priv, msg, &count);

> +		if ((!ret) && stop)

Too many parentheses.

> +			ret = rzv2m_i2c_stop_condition(priv);
> +	}

...

> +		ret = rzv2m_i2c_master_xfer1(priv, &msgs[i], (i == (num - 1)));

Too many parentheses.

> +		if (ret < 0)
> +			goto out;

...

> +static const struct of_device_id rzv2m_i2c_ids[] = {
> +	{ .compatible = "renesas,rzv2m-i2c" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rzv2m_i2c_ids);

This can be moved after the ->probe() closer to the actual user of the table.

...

> +	priv = devm_kzalloc(dev, sizeof(struct rzv2m_i2c_priv), GFP_KERNEL);

sizeof(*priv)

> +	if (!priv)
> +		return -ENOMEM;

...

> +static int rzv2m_i2c_suspend(struct device *dev)
> +{
> +	struct rzv2m_i2c_priv *priv = dev_get_drvdata(dev);

> +	pm_runtime_get_sync(dev);

Isn't guaranteed by the runtime PM that device is runtime powered on the system
suspend?

> +	bit_clrl(priv->base + IICB0CTL0, IICB0IICE);
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}

...

> +static int rzv2m_i2c_resume(struct device *dev)
> +{
> +	struct rzv2m_i2c_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = rzv2m_i2c_clock_calculate(dev, priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	pm_runtime_get_sync(dev);

I'm not sure how it's suppose to work. Isn't it a no-op here?

> +	rzv2m_i2c_init(priv);
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko