lkml.org 
[lkml]   [2018]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 2/2] i2c: add support for Socionext SynQuacer I2C controller
Hi Ard,

> +static int synquacer_i2c_master_start(struct synquacer_i2c *i2c,
> + struct i2c_msg *pmsg)
> +{
> + unsigned char bsr, bcr;
> +
> + if (pmsg->flags & I2C_M_RD)
> + writeb((pmsg->addr << 1) | 1,
> + i2c->base + SYNQUACER_I2C_REG_DAR);
> + else
> + writeb(pmsg->addr << 1,
> + i2c->base + SYNQUACER_I2C_REG_DAR);

writeb(i2c_8bit_addr_from_msg(pmsg), i2c->base + SYNQUACER_I2C_REG_DAR);

?

> +static int synquacer_i2c_master_recover(struct synquacer_i2c *i2c)
> +{

This is the bus recovery mechanism with toggling SCL pulses, right?
That should be implemented using a 'struct i2c_bus_recovery_info' and
the core helpers.

> +static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
> + struct i2c_msg *msgs, int num)
> +{
> + unsigned char bsr;
> + unsigned long timeout, bb_timeout;
> + int ret;
> +
> + if (i2c->is_suspended)
> + return -EBUSY;
> +
> + synquacer_i2c_hw_init(i2c);
> + bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> + if (bsr & SYNQUACER_I2C_BSR_BB) {
> + dev_err(i2c->dev, "cannot get bus (bus busy)\n");
> + return -EBUSY;
> + }
> +
> + init_completion(&i2c->completion);

reinit_completion? And init_completion() in probe()?

> + /* ensure the stop has been through the bus */
> + bb_timeout = jiffies + HZ;
> + do {
> + bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> + if (!(bsr & SYNQUACER_I2C_BSR_BB))
> + return 0;
> + } while (time_before(jiffies, bb_timeout));

Busy looping for one second? And won't the bus_free detection at the
beginning of a transfer do?

> +static int synquacer_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct synquacer_i2c *i2c;
> + int retry;
> + int ret;
> +
> + if (!msgs)
> + return -EINVAL;
> + if (num <= 0)
> + return -EINVAL;

Hmm, this should be done by the core. I am surprised it doesn't do that yet :/

> +
> + i2c = i2c_get_adapdata(adap);
> + i2c->timeout_ms = calc_timeout_ms(i2c, msgs, num);
> +
> + dev_dbg(i2c->dev, "calculated timeout %d ms\n", i2c->timeout_ms);
> +
> + for (retry = 0; retry < adap->retries; retry++) {
> +
> + ret = synquacer_i2c_doxfer(i2c, msgs, num);
> + if (ret != -EAGAIN)
> + return ret;
> +
> + dev_dbg(i2c->dev, "Retrying transmission (%d)\n", retry);
> +
> + synquacer_i2c_master_recover(i2c);

Recovery is only when SDA is stuck low, held by a client. That is
nothing you should do just on any error.

If you want the driver in v4.17, I'd suggest to drop
synquacer_i2c_master_recover() now and add it incrementally later, using
the existing recovery infrastructure. The rest is only minor stuff and
needs not much further discussion IMO.

Regards,

Wolfram

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-03-25 00:20    [W:0.086 / U:6.896 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site