On Thu, 2022-07-07 at 23:03 +0200, Wolfram Sang wrote: > Hi, > > On Mon, Jul 04, 2022 at 11:24:20AM +0530, Shubhrajyoti Datta wrote: > > From: Robert Hancock <robert.hancock@xxxxxxxxxx> > > > > Hook up the standard GPIO/pinctrl-based recovery support.. > > In the discurssion > > https://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xxxxxxxxxx/ > > > > recovery should be done at the beginning of the transaction. > > Here we are doing the recovery at the beginning on a timeout. > > Which is still wrong. > > > > > Signed-off-by: Robert Hancock <robert.hancock@xxxxxxxxxx> > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx> > > This is an AMD address, but the one you sent from is from Xilinx? > > > if (time_left == 0) { > > + i2c_recover_bus(adap); > > This is the wrong part. > > > cdns_i2c_master_reset(adap); > > dev_err(id->adap.dev.parent, > > "timeout waiting on completion\n"); > > @@ -852,8 +858,12 @@ static int cdns_i2c_master_xfer(struct > > i2c_adapter *adap, struct i2c_msg *msgs, > > #endif > > > > /* Check if the bus is free */ > > - if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) > > { > > + ret = readl_poll_timeout(id->membase + CDNS_I2C_SR_OFFSET, > > reg, > > + !(reg & CDNS_I2C_SR_BA), > > + CDNS_I2C_POLL_US, > > CDNS_I2C_TIMEOUT_US); > > + if (ret) { > > ret = -EAGAIN; > > + i2c_recover_bus(adap); > > goto out; > > This is proper. > > > } > > > > @@ -1278,6 +1288,7 @@ static int cdns_i2c_probe(struct > > platform_device *pdev) > > id->adap.retries = 3; /* Default retry value. */ > > id->adap.algo_data = id; > > id->adap.dev.parent = &pdev->dev; > > + id->adap.bus_recovery_info = &id->rinfo; > > Since 'rinfo' is never populated with actual data, I am quite sure > this > patch has never been tested. I think this (setting bus_recovery_info to point to a zeroed structure) is sufficient to allow the generic recovery initialization in i2c-core- base.c to work. i2c_gpio_init_recovery should fill in the required info based on the available pinctrl and gpio configuration in this case. > > Regards, > > Wolfram >
