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

[Wolfram Sang <wsa@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

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.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]