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 linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html