> +static int
> +zx2967_i2c_xfer_read_bytes(struct zx2967_i2c_info *zx_i2c, u32 bytes)
> +{
> + unsigned long time_left;
> +
> + reinit_completion(&zx_i2c->complete);
> + zx2967_i2c_writel(zx_i2c, bytes - 1, REG_RDCONF);
> + zx2967_i2c_start_ctrl(zx_i2c);
> +
> + time_left = wait_for_completion_timeout(&zx_i2c->complete,
> + I2C_TIMEOUT);
> + if (time_left == 0) {
> + dev_err(DEV(zx_i2c), "read i2c transfer timed out\n");
> + disable_irq(zx_i2c->irq);
> + zx2967_i2c_reset_hardware(zx_i2c);
> + return -EIO;
> + }
> +
> + return zx2967_i2c_empty_rx_fifo(zx_i2c, bytes);
> +}
Timeouts do happen on an I2C bus (e.g. when an EEPROM is in an erase
cycle). Or try 'i2cdetect -r'. At least, the dev_err should go. But I
also wonder if you really need to reset the hardware?
> +static int zx2967_i2c_xfer_read(struct zx2967_i2c_info *zx_i2c)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < zx_i2c->access_cnt; i++) {
> + ret = zx2967_i2c_xfer_read_bytes(zx_i2c, I2C_FIFO_MAX);
> + if (ret)
> + return ret;
> + }
> +
> + if (zx_i2c->residue > 0) {
> + ret = zx2967_i2c_xfer_read_bytes(zx_i2c, zx_i2c->residue);
> + if (ret)
> + return ret;
> + }
> +
> + zx_i2c->residue = 0;
> + zx_i2c->access_cnt = 0;
> + return 0;
> +}
> +
This function...
> +static int zx2967_i2c_xfer_write(struct zx2967_i2c_info *zx_i2c)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < zx_i2c->access_cnt; i++) {
> + ret = zx2967_i2c_xfer_write_bytes(zx_i2c, I2C_FIFO_MAX);
> + if (ret)
> + return ret;
> + }
> +
> + if (zx_i2c->residue > 0) {
> + ret = zx2967_i2c_xfer_write_bytes(zx_i2c, zx_i2c->residue);
> + if (ret)
> + return ret;
> + }
> +
> + zx_i2c->residue = 0;
> + zx_i2c->access_cnt = 0;
> + return 0;
> +}
... and this are one example where the read- and write-counterparts are
very similar. I wonder if we would do better having just one function
and handle the difference with some if(). Example here: If you'd unify
zx2967_i2c_xfer_write_bytes() and zx2967_i2c_xfer_read_bytes() into one
generic function then zx2967_i2c_xfer_write() and zx2967_i2c_xfer_read
automatically become identical AFAICS. Thoughts?
Wolfram
signature.asc
Description: PGP signature
