On Tue, Jun 28, 2022 at 08:45:26PM +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.
...
> +#include <linux/of_device.h>
No user of this header.
But missed mod_devicetable.h (Okay, for I2C drivers we usually rely on i2c.h to
include it for us, but it's cleaner to include directly).
...
> +#define rzv2m_i2c_priv_to_dev(p) ((p)->adap.dev.parent)
It's longer than the actual expression. Why do you need this?!
...
> +static const struct bitrate_config bitrate_configs[] = {
> + [RZV2M_I2C_100K] = { 47, 3450 },
> + [RZV2M_I2C_400K] = { 52, 900},
Missed space.
> +};
...
> + if (priv->iicb0wl > 0x3ff)
GENMASK() ?
Or (BIT(x) - 1) in case to tell that there is an HW limitation of x bits?
> + return -EINVAL;
...
> + if (priv->iicb0wh > 0x3ff)
Ditto.
> + return -EINVAL;
...
> + if (!last) {
Why not positive conditional?
> + } else {
> + }
...
> +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg *msg,
> + unsigned int *count)
> +{
> + unsigned int i;
> + int ret = 0;
Redundant assignment, you may return 0 directly.
Ditto for other similar cases in other functions.
> + for (i = 0; i < msg->len; i++) {
> + ret = rzv2m_i2c_write_with_ack(priv, msg->buf[i]);
> + if (ret < 0)
> + return ret;
> + }
> + *count = i;
> +
> + return ret;
> +}
...
> + ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
> + ((msg->len - 1) == i));
Too many parentheses.
> + if (ret < 0)
> + return ret;
...
> +static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
> + struct i2c_msg *msg)
> +{
> + u32 addr;
> + int ret;
> +
> + if (msg->flags & I2C_M_TEN) {
> + /* 10-bit address
> + * addr_1: 5'b11110 | addr[9:8] | (R/nW)
> + * addr_2: addr[7:0]
> + */
> + addr = 0xf0 | ((msg->addr >> 7) & 0x06);
> + addr |= !!(msg->flags & I2C_M_RD);
> + /* Send 1st address(extend code) */
> + ret = rzv2m_i2c_write_with_ack(priv, addr);
if (ret)
return ret;
> + if (ret == 0) {
> + /* Send 2nd address */
> + ret = rzv2m_i2c_write_with_ack(priv, msg->addr & 0xff);
> + }
> + } else {
> + /* 7-bit address */
> + addr = i2c_8bit_addr_from_msg(msg);
> + ret = rzv2m_i2c_write_with_ack(priv, addr);
> + }
> +
> + return ret;
> +}
...
> + ret = rzv2m_i2c_send_address(priv, msg);
> + if (ret == 0) {
This is a bit confusing if it's only comparison with "no error code" condition.
Use if (!ret) if there is no meaning of positive value. Same applies to other
cases in the code.
> + if (read)
> + ret = rzv2m_i2c_receive(priv, msg, &count);
> + else
> + ret = rzv2m_i2c_send(priv, msg, &count);
> +
> + if ((ret == 0) && stop)
Like here.
> + ret = rzv2m_i2c_stop_condition(priv);
> + }
...
> +static const struct of_device_id rzv2m_i2c_ids[] = {
> + { .compatible = "renesas,rzv2m-i2c", },
Inner comma is not needed.
> + { }
> +};
...
> + priv->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(priv->clk)) {
> + dev_err_probe(dev, PTR_ERR(priv->clk), "Can't get clock\n");
> + return PTR_ERR(priv->clk);
> + }
Why not
return dev_err_probe(...);
?
Ditto for the rest cases like this.
...
> + adap->dev.of_node = dev->of_node;
device_set_node()
--
With Best Regards,
Andy Shevchenko
