RE: [PATCH 2/2] i2c: Add Renesas RZ/V2M controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux