Hi Jean,
Thanks for looking at it.
> > +
> > +/* NOTE: SMBus QUICK Probe feature is "emulated" by reading 1 byte from
> > + * the slave address, because the SH7760 I2C cannot be programmed
> > + * to send a stop immediately after receiving the slave ACK/NACK.
> > + * If this behavior is not wanted, set platform_data.noquick to 1.
> > + */
>
> This is not acceptable. This will have to be removed from the driver if
> you want me to take it upstream. The SMBus quick command should no
> longer be needed anyway with the advent of new-style i2c chip drivers.
I wrote that driver for 2.6.12 initially, and it was completely useless
without this hack. I'm working on implementing zero-byte transfers in a
saner way, but the hardware gives me headaches...
> > + OUT32(id, I2CMSR, 0);
> > + if (id->msg->flags & I2C_M_NOSTART)
>
> Do you actually need this? Is there any chip that is connected to this
> bus in practice which needs this flag? Implementing this feature is
> totally optional and should only be done when really needed (this is a
> violation of the I2C standard.)
The i2c header had this define in it, so I implemented it. I thought
it was pretty useful when splitting an i2c transaction into multiple
i2c_msgs. Consider it gone.
> > + if (id->msg->flags & I2C_M_NOSTART)
>
> Ditto.
see above.
> > +
> > + if (sh7760_i2c_busy_check(id)) {
> > + pr_debug("sh7760-i2c%d: bus is busy!\n", adap->nr);
>
> Please use dev_dbg() instead. Or even better dev_err(), as you return
> with an error, the user will want to know.
done.
> > + if (msg->flags & I2C_M_REV_DIR_ADDR)
> > + addr ^= 1;
>
> Again, do you actually need this? If not, please don't implement it.
see above.
> > + for (cdf = 3; cdf >= 0; cdf--) {
> > + iclk = mck / (1 + cdf);
> > + /* iclk must not be > 20MHz */
> > + if (iclk >= 20000000)
>
> > or >=?
>
> > + continue;
> > + for (scgd = 0; scgd < 63; scgd++) {
> > + m1 = iclk / (20 + (scgd << 3));
> > + dff = abs(scl_hz - m1);
> > + if (dff < odff) {
> > + odff = dff;
> > + cdfm = cdf;
> > + scgdm = scgd;
> > + }
> > + }
> > + }
>
> These imbricated loops don't look exactly optimal. Is there no way to
> compute the right value right away?
I'm open to suggestions. I just wanted to support (almost) every conceivable
scl clock and module clock.
> > + printk(KERN_INFO "SH7760 I2C-%d, %d kHz, mmio %08x, irq %d\n",
> > + id->adap.nr, pd->speed_khz, res->start, id->irq);
>
> Use dev_info (and you will no longer need to print id->adap.nr).
Oh, neat!
I'll resend once I've managed to get zero-byte transfers working without
ugly hacks.
Best regards,
Manuel Lauss
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
