RE: [PATCH 5.10.y-cip 19/23] i2c: Add Renesas RZ/V2M controller

Biju Das
2023-05-24T12:35:15Z

Hi Pavel,

Thanks for the feedback.

> Subject: Re: [PATCH 5.10.y-cip 19/23] i2c: Add Renesas RZ/V2M controller
>
> Hi!
>
> Minor details I noticed during review (they should not prevent merge):
>
> > commit e0ca796a151bd4de76efd71aade2b4805a95c93d upstream.
> >
> > Yet another i2c controller from Renesas that is found on the RZ/V2M
> > (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
>
> KHz should be kHz.

I agree.

>
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -961,6 +961,16 @@ config HAVE_S3C2410_I2C
> > include I2C support for any machine, kindly select this in the
> > respective Kconfig file.
> >
> > +config I2C_RZV2M
> > + tristate "Renesas RZ/V2M adapter"
> > + depends on ARCH_RENESAS || COMPILE_TEST
> > + help
> > + If you say yes to this option, support will be included for the
> > + Renesas RZ/V2M I2C interface.
>
> Double space (" ").

OK, will fix this in mainline.

>
> > +#define bit_setl(addr, val) writel(readl(addr) | (val),
> (addr))
> > +#define bit_clrl(addr, val) writel(readl(addr) & ~(val),
> (addr))
>
> Normally we'd put macro names in all uppercase.

Yes, Will try converting to uppercase in mainline.

>
> > +static int rzv2m_i2c_probe(struct platform_device *pdev) {
> ...
> > + pm_runtime_get_sync(dev);
> > + rzv2m_i2c_init(priv);
> > + pm_runtime_put(dev);
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + ret = i2c_add_numbered_adapter(adap);
> > + if (ret < 0)
> > + pm_runtime_disable(dev);
>
> Is it possible that bit_clrl(priv->base + IICB0CTL0, IICB0IICE); is
> missing before pm_runtime_disable?

You mean error case, I guess it is not required as it is just probe()??

As per hardware manual, When the IICB0CTL0.IICB0IICE bit = 0b, the SCLm, SDAm pins are
in a high impedance state.

Currently rzv2m_i2c_init() is setting IICB0CTL0.IICB0IICE bit to 1.

Cheers,
Biju