Hi Geert,
On 28 June 2022 14:42 Geert Uytterhoeven wrote:
> On Fri, Jun 24, 2022 at 12:18 PM 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.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
> > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
Thanks for your review!
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-rzv2m.c
>
> > +enum bcr_index {
> > + RZV2M_I2C_100K = 0,
> > + RZV2M_I2C_400K,
> > +};
> > +
> > +struct bitrate_config {
> > + unsigned int percent_low;
> > + unsigned int min_hold_time_ns;
> > +};
> > +
> > +static const struct bitrate_config bitrate_configs[] = {
> > + {47, 3450},
> > + {52, 900},
>
> These are indexed by bcr_index, so perhaps
>
> .[RZV2M_I2C_100K] = { 47, 3450 },
> ...
>
> ?
Ok
> > +};
>
> > +/* Calculate IICB0WL and IICB0WH */
> > +static int rzv2m_i2c_clock_calculate(struct device *dev,
> > + struct rzv2m_i2c_priv *priv)
> > +{
> > + const struct bitrate_config *config;
> > + unsigned int hold_time_ns;
> > + unsigned int total_pclks;
> > + unsigned int trf_pclks;
> > + unsigned long pclk_hz;
> > + struct i2c_timings t;
> > + u32 trf_ns;
> > +
> > + i2c_parse_fw_timings(dev, &t, true);
> > +
> > + pclk_hz = clk_get_rate(priv->clk);
> > + total_pclks = pclk_hz / t.bus_freq_hz;
> > +
> > + trf_ns = t.scl_rise_ns + t.scl_fall_ns;
> > + trf_pclks = (u64)pclk_hz * trf_ns / NSEC_PER_SEC;
>
> This is an open-coded 64-by-ul division, which may cause link failures
> when compile-tested on 32-bit. Please use mul_u64_u32_div() instead.
Sure
> > +
> > + /* Config setting */
> > + switch (t.bus_freq_hz) {
> > + case I2C_MAX_FAST_MODE_FREQ:
> > + priv->bus_mode = RZV2M_I2C_400K;
> > + break;
> > + case I2C_MAX_STANDARD_MODE_FREQ:
> > + priv->bus_mode = RZV2M_I2C_100K;
> > + break;
> > + default:
> > + dev_err(dev, "transfer speed is invalid\n");
> > + return -EINVAL;
> > + }
> > + config = &bitrate_configs[priv->bus_mode];
> > +
> > + /* IICB0WL = (percent_low / Transfer clock) x PCLK */
> > + priv->iicb0wl = total_pclks * config->percent_low / 100;
> > + if (priv->iicb0wl > 0x3ff)
> > + return -EINVAL;
> > +
> > + /* IICB0WH = ((percent_high / Transfer clock) x PCLK) - (tR +
> tF) */
> > + priv->iicb0wh = total_pclks - priv->iicb0wl - trf_pclks;
> > + if (priv->iicb0wh > 0x3ff)
> > + return -EINVAL;
> > +
> > + /*
> > + * Data hold time must be less than 0.9us in fast mode and
> > + * 3.45us in standard mode.
> > + * Data hold time = IICB0WL[9:2] / PCLK
> > + */
> > + hold_time_ns = (u64)(priv->iicb0wl >> 2) * NSEC_PER_SEC /
> pclk_hz;
>
> div64_ul()
Ok
> > + if (hold_time_ns > config->min_hold_time_ns) {
> > + dev_err(dev, "data hold time %dns is over %dns\n",
> > + hold_time_ns, config->min_hold_time_ns);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
>
> > +static int rzv2m_i2c_write_with_ACK(struct rzv2m_i2c_priv *priv, u32
> data)
>
> rzv2m_i2c_write_with_ack
Ok
<snip>
> > + *data = (u8)(data_tmp & 0xff);
>
> No need to cast or mask.
Ok
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg
> *msg,
> > + int *count)
>
> unsigned int *count
Ok
> > +{
> > + int i, ret = 0;
>
> unsigned int i
Ok
> > +
> > + for (i = 0; i < msg->len; i++) {
> > + ret = rzv2m_i2c_write_with_ACK(priv, msg->buf[i]);
> > + if (ret < 0)
> > + break;
>
> return ret?
Yes, makes sense as *count is only used if ret is >= 0
> > + }
> > + *count = i;
> > +
> > + return ret;
> > +}
> > +
> > +static int rzv2m_i2c_receive(struct rzv2m_i2c_priv *priv, struct
> i2c_msg *msg,
> > + int *count)
>
> unsigned int *count
Ok
> > +{
> > + int i, ret = 0;
>
> unsigned int i
Ok
> > +
> > + for (i = 0; i < msg->len; i++) {
> > + ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
> > + ((msg->len - 1) == i));
> > + if (ret < 0)
> > + break;
>
> return ret?
Ok
> > + }
> > + *count = i;
> > +
> > + return ret;
> > +}
> > +
> > +static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
> > + struct i2c_msg *msg, int read)
>
> No need to pass read, as you have access to the full msg, and 10-bit
> addressing is rare?
Ok, makes sense
> > +{
> > + 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 |= read;
> > + /* Send 1st address(extend code) */
> > + ret = rzv2m_i2c_write_with_ACK(priv, addr);
> > + 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;
> > +}
>
> > +static int rzv2m_i2c_master_xfer1(struct rzv2m_i2c_priv *priv,
> > + struct i2c_msg *msg, int stop)
> > +{
> > + int count = 0;
>
> unsigned int
Ok
> > + int ret, read = !!(msg->flags & I2C_M_RD);
> > +
> > + /* Send start condition */
> > + writel(IICB0STT, priv->base + IICB0TRG);
> > +
> > + ret = rzv2m_i2c_send_address(priv, msg, read);
> > +
>
> Please drop this blank line.
Ok
> > + if (ret == 0) {
> > + if (read)
> > + ret = rzv2m_i2c_receive(priv, msg, &count);
> > + else
> > + ret = rzv2m_i2c_send(priv, msg, &count);
> > +
> > + if ((ret == 0) && stop)
> > + ret = rzv2m_i2c_stop_condition(priv);
> > + }
> > +
> > + if (ret == -ENXIO)
> > + rzv2m_i2c_stop_condition(priv);
> > + else if (ret < 0)
> > + rzv2m_i2c_init(priv);
> > + else
> > + ret = count;
> > +
> > + return ret;
> > +}
> > +
> > +static int rzv2m_i2c_master_xfer(struct i2c_adapter *adap,
> > + struct i2c_msg *msgs, int num)
> > +{
> > + struct rzv2m_i2c_priv *priv = i2c_get_adapdata(adap);
> > + struct device *dev = rzv2m_i2c_priv_to_dev(priv);
> > + int ret, i;
>
> unsigned int i
Ok
> > +
> > + pm_runtime_get_sync(dev);
>
> Please use pm_runtime_resume_and_get() in new code
> (and check its return code?).
Ok
> > +
> > + if (readl(priv->base + IICB0STR0) & IICB0SSBS) {
> > + ret = -EAGAIN;
> > + goto out;
> > + }
> > +
> > + /* I2C main transfer */
> > + for (i = 0; i < num; i++) {
> > + ret = rzv2m_i2c_master_xfer1(priv, &msgs[i], (i == (num
> - 1)));
> > + if (ret < 0)
> > + goto out;
> > + }
> > + ret = num;
> > +
> > +out:
> > + pm_runtime_put(dev);
I notice that a lot of i2c drivers use pm_runtime_mark_last_busy() and
pm_runtime_put_autosuspend() here. I think they make sense here as well.
> > +
> > + return ret;
> > +}
>
> > +static struct i2c_algorithm rzv2m_i2c_algo = {
> > + .master_xfer = rzv2m_i2c_master_xfer,
> > + .functionality = rzv2m_i2c_func,
>
> No .(un)reg_slave()? ;-)
> The hardware seems to support it.
You are right that the HW supports it, though I haven’t tested it!
I currently don't have something I can test it with either.
Thanks!
Phil
