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
