On 24 March 2018 at 23:19, Wolfram Sang <wsa@xxxxxxxxxxxxx> wrote:
> 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.
>
Thanks Wolfram.
I have fixed all these up and am about to send out the v6.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
