Date:   Mon, 29 Nov 2021 11:39:56 +0100
From:   Wolfram Sang <wsa@kernel.org>
To:     Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
Cc:     linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, nicolas.ferre@microchip.com,
        alexandre.belloni@bootlin.com, ludovic.desroches@microchip.com,
        andrew@sanpeople.com, mhoffman@lightlink.com, khali@linux-fr.org
Subject: Re: [PATCH 1/3] i2c: at91: move i2c_recover_bus() outside of
 at91_do_twi_transfer()
Message-ID: <YaSt/CXWmZGmt+te@kunai>
Mail-Followup-To: Wolfram Sang <wsa@kernel.org>,
        Codrin Ciubotariu <codrin.ciubotariu@microchip.com>,
        linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, nicolas.ferre@microchip.com,
        alexandre.belloni@bootlin.com, ludovic.desroches@microchip.com,
        andrew@sanpeople.com, mhoffman@lightlink.com, khali@linux-fr.org
References: <20210727111554.1338832-1-codrin.ciubotariu@microchip.com>
 <20210727111554.1338832-2-codrin.ciubotariu@microchip.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="3OthpyYLRMGfk6Jx"
Content-Disposition: inline
In-Reply-To: <20210727111554.1338832-2-codrin.ciubotariu@microchip.com>
Precedence: bulk


--3OthpyYLRMGfk6Jx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi Codrin,

sorry for the super-long delay. There is an issue here with regard to
bus recovery which affetcs more drivers and I can't make up my mind how
to handle it...

> Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")

Sidenote: I don't think this is a fix.

> +	if (ret < 0) {
> +		/*
> +		 * some faulty I2C slave devices might hold SDA down;
> +		 * we can send a bus clear command, hoping that the pins will be
> +		 * released
> +		 */
> +		i2c_recover_bus(&dev->adapter);
> +	} else {
> +		ret = num;
> +	}

So, one issue is more straightforward. Bus recovery is applied on all
errors. It should only be called when SDA is stuck.

The other issue is that bus recovery is applied after a transfer. The
I2C specs mention bus recovery only at the beginning of a transfer when
SDA is detected low. I think it also makes more sense because the bus
may also be stuck because of a misbehaving bootloader etc. This will be
caught when the check is done at the beginning.

However, moving the detection to the beginning leaves room for a
regression, because your driver already does it at the end of a
transfer. However, I'd think all regressions coming up need seperate
fixing anyhow. Unless I overlooked something, of course.

So, I think it should be moved to the beginning of a transfer, but I am
open for discussion, so we get the best possible bus recovery in Linux.

Happy hacking,

   Wolfram


--3OthpyYLRMGfk6Jx
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAmGkrfgACgkQFA3kzBSg
KbY+Fw//XBnb5n5/XOjcc6s8oJlvjKElkgzd8bDBmXWnRC8XyQPjiWNO4+oXLhOu
UkkIaGzL7RcQEc0Bc1JswFqCK8PKdQQDGCJ+BIpk0e9Yfz0Srwg066PJWBuXtNlw
L+11VKeIPG/18dwrk9HFxP/rQV6mu7hjTAFDUNvUrc7WKR0V/O1ll6ci8RUJ72Vq
y86v+YF3eTOaBhy61AvttrTitCGyVb0m0TWt3i5nYY+BMKVLns96COqw/UE5GA7c
jkhRaScyYy+QWhj3o5myWB3K6ynika+5kh1OhHFYsV9xkrdngAR1bLgroeV9PJ91
EcOZkFJ6fbbaDKBWNeGi4f3UDCpvSyQlPy8uAHXsmF2hGI5SUiW7mWXlezUbQzyk
i3yFNtlsCQ6lY7qM+iA4JXtRSu1vB2wR69kK67RgpNl+K71mFYdutKVPZcDYHAza
yOIB6Su6VYge1G2EaBAhdfopBUJyfDqMIBRgowQqd73urdHT4qcFLhRhnnhssvX6
W2OW7kY3uQvN5Kx1zoxbZGprdMGw/tMIosCesVWWOn+iGNYXiUxpDOr3yyLMvhz2
u1Rpq8Mn99tnA8TSwmuJnlvW7TTsKFY1y5kpbj3U6Uh49FAPyQMeS2jqT0L7fd58
rrWJ3M8BUS05ISoyfWvrqaOmW6YjIMCqYGOgmxLdEkyd9uppPTM=
=dQkj
-----END PGP SIGNATURE-----

--3OthpyYLRMGfk6Jx--