Hi Ezequiel,
On Mon, Nov 10, 2014 at 11:30 AM, Ezequiel Garcia
<ezequiel.garcia@xxxxxxxxxx> wrote:
> From: James Hogan <james.hogan@xxxxxxxxxx>
>
> Add support for the IMG I2C Serial Control Bus (SCB) found on the
> Pistachio and TZ1090 SoCs.
>
> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> [Ezequiel: code cleaning and rebasing]
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@xxxxxxxxxx>
A few minor comments below, otherwise:
Reviewed-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> +/* 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 */
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
