Hi Andy,
Thanks for your review!
On 28 June 2022 22:08 Andy Shevchenko wrote:
> 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).
Ok
> > +#define rzv2m_i2c_priv_to_dev(p) ((p)->adap.dev.parent)
>
> It's longer than the actual expression. Why do you need this?!
Ok, no need for it
> > +static const struct bitrate_config bitrate_configs[] = {
> > + [RZV2M_I2C_100K] = { 47, 3450 },
> > + [RZV2M_I2C_400K] = { 52, 900},
>
> Missed space.
Ok
> > + if (priv->iicb0wl > 0x3ff)
>
> GENMASK() ?
> Or (BIT(x) - 1) in case to tell that there is an HW limitation of x bits?
Ok, BIT makes sense here
> > + if (priv->iicb0wh > 0x3ff)
>
> Ditto.
Ok
> > + if (!last) {
>
> Why not positive conditional?
Ok
> > +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.
Ok
> > + ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
> > + ((msg->len - 1) == i));
>
> Too many parentheses.
Ok
> > +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;
Ok
> > + 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.
Sorry, I don't follow you. How is (ret == 0) any different to (!ret) ?
> > +static const struct of_device_id rzv2m_i2c_ids[] = {
> > + { .compatible = "renesas,rzv2m-i2c", },
>
> Inner comma is not needed.
Ok
> > + 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.
Ok
> > + adap->dev.of_node = dev->of_node;
>
> device_set_node()
Since we don't need to set dev->fwnode, why is device_set_node() any
better?
Thanks
Phil
