Hi Wolfram,
thanks for the review!
> > +
> > + /* Settings */
> > + unsigned int scl_frequency;
> > +
> > + /* Synchronization & notification */
> > + spinlock_t lock;
>
> Why the lock? The core has per-adapter locks anyhow.
I'm using it to lock the rk3x_i2c struct during interrupts. It's needed there
because an operation can timeout, which means the interrupt can occur at any
time and possibly conflict with the cleanup I do after a timeout.
I looked around in i2c-exynos5.c, i2c-pxa.c and others, and they do it the
same way. Could you explain in more detail why it's not needed?
> > +static void rk3x_i2c_handle_write(struct rk3x_i2c *i2c, unsigned int ipd)
> > +{
> > + if (!(ipd & REG_INT_MBTF)) {
> > + rk3x_i2c_stop(i2c, -ENXIO);
> > + dev_err(i2c->dev, "unexpected irq in WRITE: 0x%x\n", ipd);
> > + rk3x_i2c_clean_ipd(i2c);
> > + return;
> > + }
> > +
> > + /* ack interrupt */
> > + i2c_writel(i2c, REG_INT_MBTF, REG_IPD);
> > +
> > + /* are we finished? */
> > + if (i2c->processed == i2c->msg->len)
> > + rk3x_i2c_stop(i2c, i2c->error);
> > + else
> > + rk3x_i2c_fill_transmit_buf(i2c);
>
> It looks to me that you STOP after every message? You should use
> REPEATED_START inbetween messages and only stop after the last message
> of a transfer.
I had a fight with the hw today and finally got it to issue a REPEATED_START
for multiple "boring" messages. Will be included in the next version.
> > +/**
> > + * Setup I2C registers for an I2C operation specified by msgs, num.
> > + *
> > + * Must be called with i2c->lock held.
> > + *
> > + * @msgs: I2C msgs to process
> > + * @num: Number of msgs
> > + *
> > + * returns: Number of I2C msgs processed or negative in case of error
> > + */
> > +static int rk3x_i2c_setup(struct rk3x_i2c *i2c, struct i2c_msg *msgs, int
> > num) +{
> > + u32 addr = (msgs[0].addr & 0x7f) << 1;
> > + int ret = 0;
> > +
> > + /*
> > + * The I2C adapter can issue a small (len < 4) write packet before
> > + * reading. This speeds up SMBus-style register reads.
> > + * The MRXADDR/MRXRADDR hold the slave address and the slave register
> > + * address in this case.
> > + */
> > +
> > + if (num >= 2 && msgs[0].len < 4
> > + && !(msgs[0].flags & I2C_M_RD)
> > + && (msgs[1].flags & I2C_M_RD)) {
> > + u32 reg_addr = 0;
> > +
> > + dev_dbg(i2c->dev, "Combined write/read from addr 0x%x\n",
> > + addr >> 1);
> > +
> > + if (msgs[0].len == 0)
> > + return -EINVAL;
>
> Can the controller do SMBUS_QUICK (len == 0) in general? For the case it
> cannot do it only in this multi-packet mode, then you should fall back
> to the "boring" mode.
Actually, I wasn't aware that (len == 0) is a valid case. The hw supports it
in both modes, I just tested that. So the check is going away.
I'm cleaning up now and you can expect a new version of the patch today.
Cheers,
Max
--
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
