Hi Komal,
> I have attached the updated patch, which addresses the most of review
> comments.
Here we go for another review:
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -287,6 +287,14 @@ config I2C_OCORES
> This driver can also be built as a module. If so, the module
> will be called i2c-ocores.
>
> +config I2C_OMAP
> + tristate "OMAP I2C adapter"
> + depends on I2C && ARCH_OMAP
> + default y if MACH_OMAP_H3 || MACH_OMAP_OSK
> + help
> + If you say yes to this option, support will be included for the
> + Texas Instruments OMAP(http://www.ti.com/omap) I2C driver.
> +
Missing space before opening parenthesis. And "including support for a
driver" still doesn't make sense, sorry. The driver is what supports a
device.
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-omap.c
> +/* I2C System Test Register (OMAP_I2C_SYSTEST): */
> +#ifdef DEBUG
> +#define OMAP_I2C_SYSTEST_ST_EN (1 << 15) /* System test enable */
> +#define OMAP_I2C_SYSTEST_FREE (1 << 14) /* Free running mode */
> +#define OMAP_I2C_SYSTEST_TMODE_MASK (3 << 12) /* Test mode select */
> +#define OMAP_I2C_SYSTEST_TMODE_SHIFT (12) /* Test mode select */
> +#define OMAP_I2C_SYSTEST_SCL_I (1 << 3) /* SCL line sense in */
> +#define OMAP_I2C_SYSTEST_SCL_O (1 << 2) /* SCL line drive out */
> +#define OMAP_I2C_SYSTEST_SDA_I (1 << 1) /* SDA line sense in */
> +#define OMAP_I2C_SYSTEST_SDA_O (1 << 0) /* SDA line drive out */
> +#endif
> +
> +/* I2C System Status register (OMAP_I2C_SYSS): */
> +#define OMAP_I2C_SYSS_RDONE 1 /* Reset Done */
Shouldn't it be (1 << 0) for consistency?
> +
> +/* I2C System Configuration Register (OMAP_I2C_SYSC): */
> +#define OMAP_I2C_SYSC_SRST (1 << 1) /* Soft Reset */
> +static int omap_i2c_get_clocks(struct omap_i2c_dev *dev)
> +{
> + if (cpu_is_omap16xx() || cpu_is_omap24xx()) {
> + dev->iclk = clk_get(dev->dev, "i2c_ick");
> + if (IS_ERR(dev->iclk)) {
> + dev->iclk = NULL;
> + return -ENODEV;
> + }
> + }
> +
> + dev->fclk = clk_get(dev->dev, "i2c_fck");
> + if (IS_ERR(dev->fclk)) {
> + if (dev->iclk != NULL) {
> + clk_put(dev->iclk);
> + dev->iclk = NULL;
> + return -ENODEV;
> + }
> + }
> +
> + return 0;
> +}
You broke the second error path while trying to address my previous
objection... dev->fclk is still not reset to NULL on error, and
additionally you return 0 (instead of -ENODEV) if dev->iclk == NULL and
clk_get(dev->dev, "i2c_fck") fails. Please fix.
> +static int omap_i2c_init(struct omap_i2c_dev *dev)
> +{
> + u16 psc = 0;
> + unsigned long fclk_rate = 12000000;
> + unsigned long timeout;
> +
> + if (!dev->rev1) {
> + omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, OMAP_I2C_SYSC_SRST);
> + /* For some reason we need to set the EN bit before the
> + * reset done bit gets set. */
> + timeout = jiffies + OMAP_I2C_TIMEOUT;
> + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> + while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG) &
> + OMAP_I2C_SYSS_RDONE)) {
> + if (time_after(jiffies, timeout)) {
> + dev_warn(dev->dev, "timeout waiting"
> + "for controller reset\n");
> + return -ETIMEDOUT;
> + }
> + msleep(1);
> + }
> + }
> + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> +
> + if (cpu_class_is_omap1()) {
> + struct clk *armxor_ck;
> +
> + armxor_ck = clk_get(NULL, "armxor_ck");
> + if (IS_ERR(armxor_ck)) {
> + dev_warn(dev->dev, "i2c: Could not get armxor_ck\n");
The "i2c: " should go away now that you use dev_warn() instead of
printk().
> + fclk_rate = 12000000;
This is already the value of fclk_rate, you initialized it at the top
of the function.
> + } else {
> + fclk_rate = clk_get_rate(armxor_ck);
> + clk_put(armxor_ck);
> + }
> +
> + if (fclk_rate > 16000000)
> + psc = (fclk_rate + 8000000) / 12000000;
> + else
> + psc = 0;
Ditto.
> + }
> +
> +/*
> + * Low level master read/write transaction.
> + */
> +static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> + struct i2c_msg *msg, int stop)
> +{
> + struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> + int r;
> + u16 w;
> + u8 zero_byte = 0;
> +
> + dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
> + msg->addr, msg->len, msg->flags, stop);
> +
> + omap_i2c_write_reg(dev, OMAP_I2C_SA_REG, msg->addr);
> +
> + /* Sigh, seems we can't do zero length transactions. Thus, we
> + * can't probe for devices w/o actually sending/receiving at least
> + * a single byte. So we'll set count to 1 for the zero length
> + * transaction case and hope we don't cause grief for some
> + * arbitrary device due to random byte write/read during
> + * probes.
> + */
> + /* REVISIT: Could the STB bit of I2C_CON be used with probing? */
> + if (msg->len == 0) {
> + dev->buf = &zero_byte;
> + dev->buf_len = 1;
> + } else {
Hm, I thought we had all agreed that it wasn't acceptable? If msg->len
== 0, you can't handle the message, so return an error. Don't even
write to the address register.
> + dev->buf = msg->buf;
> + dev->buf_len = msg->len;
> + }
> + omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
> +static u32
> +omap_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C;
> +}
Most i2c chip drivers will not work then. You can actually achieve most
SMBus transactions, just not Quick Command. So you should return
I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK.
> +#ifdef DEBUG
> + if (!(stat & OMAP_I2C_STAT_BB)) {
> + dev_warn(dev->dev, "XRDY while bus not busy\n");
> + bail_out = 1;
> + }
> +#endif
> + omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
> + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
> + if (bail_out)
> + omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_SBD);
> + continue;
> + }
Enabling debug should never change the functional behavior of a driver.
Rest looks OK to me. Please address the few remaining issues and
resubmit your patch for inclusion.
--
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
