Re: [PATCH v5 1/2] i2c: Add Imagination Technologies I2C SCB driver

Hi Ezequiel,

On Mon, Nov 10, 2014 at 11:30 AM, Ezequiel Garcia
[off-list ref] wrote:

From: James Hogan <redacted>

Add support for the IMG I2C Serial Control Bus (SCB) found on the
Pistachio and TZ1090 SoCs.

Signed-off-by: James Hogan <redacted>
[Ezequiel: code cleaning and rebasing]
Signed-off-by: Ezequiel Garcia <redacted>

A few minor comments below, otherwise:

Reviewed-by: Andrew Bresticker <redacted>

+/* Force a bus reset sequence and wait for it to complete */
+static void img_i2c_reset_bus(struct img_i2c *i2c)
+{
+       unsigned long flags;
+
+       spin_lock_irqsave(&i2c->lock, flags);
+       reinit_completion(&i2c->msg_complete);
+       img_i2c_reset_start(i2c);
+       spin_unlock_irqrestore(&i2c->lock, flags);
+
+       wait_for_completion_timeout(&i2c->msg_complete, IMG_I2C_TOUT);

Check return value here?

+static int img_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg *msgs,
+                       int num)
+{
+       struct img_i2c *i2c = i2c_get_adapdata(i2c_adap);
+       bool atomic = false;
+       int i, ret;
+
+       if (i2c->mode == MODE_SUSPEND) {
+               WARN(1, "refusing to service transaction in suspended state\n");
+               return -EIO;
+       }
+
+       if (i2c->mode == MODE_FATAL)
+               return -EIO;
+
+       for (i = 0; i < num; i++) {
+               if (likely(msgs[i].len))
+                       continue;
+               /*
+                * 0 byte reads are not possible because the slave could try
+                * and pull the data line low, preventing a stop bit.
+                */
+               if (unlikely(msgs[i].flags & I2C_M_RD))
+                       return -EIO;
+               /*
+                * 0 byte writes are possible and used for probing, but we
+                * cannot do them in automatic mode, so use atomic mode
+                * instead.
+                */
+               atomic = true;
+       }
+
+       ret = clk_prepare_enable(i2c->scb_clk);
+       if (ret)
+               return ret;
+
+       for (i = 0; i < num; i++) {
+               struct i2c_msg *msg = &msgs[i];
+               unsigned long flags;
+
+               spin_lock_irqsave(&i2c->lock, flags);
+
+               /*
+                * Make a copy of the message struct. We mustn't modify the
+                * original or we'll confuse drivers and i2c-dev.
+                */
+               i2c->msg = *msg;
+               i2c->msg_status = 0;
+
+               /*
+                * After the last message we must have waited for a stop bit.
+                * Not waiting can cause problems when the clock is disabled
+                * before the stop bit is sent, and the linux I2C interface
+                * requires separate transfers not to joined with repeated
+                * start.
+                */
+               i2c->last_msg = (i == num - 1);
+               reinit_completion(&i2c->msg_complete);
+
+               if (atomic)
+                       img_i2c_atomic_start(i2c);
+               else if (msg->flags & I2C_M_RD)
+                       img_i2c_read(i2c);
+               else
+                       img_i2c_write(i2c);
+               spin_unlock_irqrestore(&i2c->lock, flags);
+
+               wait_for_completion_timeout(&i2c->msg_complete, IMG_I2C_TOUT);

... and here?

+               del_timer_sync(&i2c->check_timer);
+               if (i2c->msg_status)
+                       break;
+       }
+
+       clk_disable_unprepare(i2c->scb_clk);
+
+       return i2c->msg_status ? i2c->msg_status : num;
+}

+static int img_i2c_init(struct img_i2c *i2c)
+{
+       unsigned int clk_khz, bitrate_khz, clk_period, tckh, tckl, tsdh;
+       unsigned int i, ret, data, prescale, inc, int_bitrate;
+       unsigned int filt, filt_disable, filt_bypass;
+       struct img_i2c_timings timing;
+       u32 rev;
+
+       ret = clk_prepare_enable(i2c->scb_clk);
+       if (ret)
+               return ret;
+
+       rev = img_i2c_readl(i2c, SCB_CORE_REV_REG);
+       if ((rev & 0x00ffffff) < 0x00020200) {
+               dev_info(i2c->adap.dev.parent,
+                        "Unknown hardware revision (%d.%d.%d.%d)\n",
+                        (rev >> 24) & 0xff, (rev >> 16) & 0xff,
+                        (rev >> 8) & 0xff, rev & 0xff);
+               clk_disable_unprepare(i2c->scb_clk);
+               return -EINVAL;
+       }
+
+       if (rev == REL_SOC_IP_SCB_2_2_1)
+               i2c->need_wr_rd_fence = true;
+
+       bitrate_khz = i2c->bitrate / 1000;
+       clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
+
+       /* Determine what mode we're in from the bitrate */
+       timing = timings[0];
+       for (i = 0; i < ARRAY_SIZE(timings); i++) {
+               if (i2c->bitrate <= timings[i].max_bitrate) {
+                       timing = timings[i];
+                       break;
+               }
+       }
+
+       /* Find the prescale that would give us that inc (approx delay = 0) */
+       prescale = SCB_OPT_INC * clk_khz / (256 * 16 * bitrate_khz);
+       prescale = clamp_t(unsigned int, prescale, 1, 8);
+       clk_khz /= prescale;
+
+       /* Setup the clock increment value */
+       inc = ((256 * 16 * bitrate_khz) /
+              (clk_khz - (16 * bitrate_khz * (clk_khz / 1000) *
+                          i2c->busdelay) / 10000));
+
+       /* Setup the filter clock value */
+       filt_bypass = 0;
+       filt_disable = 0;
+       filt = 0;
+       if (clk_khz < 20000) {
+               filt_disable = SCB_FILT_DISABLE;
+       } else if (clk_khz < 40000) {
+               filt_bypass = SCB_FILT_BYPASS;
+       } else {
+               /* Calculate filter clock */
+               filt = ((640000) / ((clk_khz / 1000) * (250 - i2c->busdelay)));
+               if ((640000) % ((clk_khz / 1000) * (250 - i2c->busdelay))) {
+                       /* Scale up */
+                       inc++;
+               }
+               if (filt > SCB_FILT_INC_MASK)
+                       filt = SCB_FILT_INC_MASK;
+       }
+       data = filt_disable | filt_bypass |
+              ((filt & SCB_FILT_INC_MASK) << SCB_FILT_INC_SHIFT) |
+              ((inc & SCB_INC_MASK) << SCB_INC_SHIFT) |
+              (prescale - 1);
+       img_i2c_writel(i2c, SCB_CLK_SET_REG, data);

Do filt_bypass/filt_disable really need to be separate variables?
Can't we just use filt in all three cases?

+#ifdef CONFIG_PM_SLEEP
+static int img_i2c_suspend(struct device *dev)
+{
+       struct img_i2c *i2c = dev_get_drvdata(dev);
+
+       img_i2c_switch_mode(i2c, MODE_SUSPEND);
+
+       clk_disable(i2c->sys_clk);

Why not unprepare as well?

+
+       return 0;
+}
+
+static int img_i2c_resume(struct device *dev)
+{
+       struct img_i2c *i2c = dev_get_drvdata(dev);
+       int ret;
+
+       ret = clk_enable(i2c->sys_clk);

... and then prepare here?

+       if (ret)
+               return ret;
+
+       img_i2c_init(i2c);
+
+       return 0;
+}
+#endif /* CONFIG_PM_SLEEP */