Re: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Robert Hancock <robert.hancock@calian.com>]

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@calian.com>
> > 
> > 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@xilinx.com/
> > 
> > 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@calian.com>
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> 
> 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
>