Received-SPF: pass (google.com: domain of linux-kernel+bounces-139910-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 11 Apr 2024 09:02:52 +0200
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
To: Andi Shyti <andi.shyti@kernel.org>
Cc: linux-i2c@vger.kernel.org, Jean Delvare <jdelvare@suse.com>, 
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 06/18] i2c: i801: remove printout on handled timeouts
Message-ID: <zmkluzi3ncze67wei6eccd67cpuab2k7qw7cdgju4tg7rermv2@hw6ukejz4cvy>
Mail-Followup-To: Wolfram Sang <wsa+renesas@sang-engineering.com>, 
	Andi Shyti <andi.shyti@kernel.org>, linux-i2c@vger.kernel.org, Jean Delvare <jdelvare@suse.com>, 
	linux-kernel@vger.kernel.org
References: <20240410112418.6400-20-wsa+renesas@sang-engineering.com>
 <20240410112418.6400-26-wsa+renesas@sang-engineering.com>
 <242ogjpole3ltk5nu53knbfsxmmwcqfrbcivjh7fnkngvrroq5@cwspwdrtepwh>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="j6qt37qfsj4aexfx"
Content-Disposition: inline
In-Reply-To: <242ogjpole3ltk5nu53knbfsxmmwcqfrbcivjh7fnkngvrroq5@cwspwdrtepwh>


--j6qt37qfsj4aexfx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 10, 2024 at 02:21:58PM +0200, Andi Shyti wrote:
> Hi Wolfram,
>=20
> On Wed, Apr 10, 2024 at 01:24:20PM +0200, Wolfram Sang wrote:
> > I2C and SMBus timeouts are not something the user needs to be informed
> > about on controller level. The client driver may know if that really is
> > a problem and give more detailed information to the user. The controller
> > should just pass this information upwards. Turn all timeout related
> > printouts to debug level.
> >=20
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >=20
> > Here, I did not delete the printout to support checking the termination
> > process. The other drivers in this series do not have this SMBus
> > specific termination step.
> >=20
> >  drivers/i2c/busses/i2c-i801.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >=20
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i80=
1.c
> > index 4294c0c63cef..a42b5152f9bd 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -400,7 +400,7 @@ static int i801_check_post(struct i801_priv *priv, =
int status)
> >  	 * If the SMBus is still busy, we give up
> >  	 */
> >  	if (unlikely(status < 0)) {
> > -		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
> > +		dev_dbg(&priv->pci_dev->dev, "Transaction timeout\n");
>=20
> why after 5 patches of removing dev_err's, here you are changing
> them to dev_dbg?

The reasoning was explained above:

> > Here, I did not delete the printout to support checking the termination
> > process. The other drivers in this series do not have this SMBus
> > specific termination step.

This is also why I converted two calls here to dev_dbg. But read on
first.

> It's still good, but if we want to be strict, errors should
> print errors: as we are returning -ETIMEDOUT, then we are
> treating the case as an error and we should print error.

I strongly disagree. While we use an errno, we don't know if this is a
real error yet. It is more a return value saying that the transfer timed
out. The client driver knows. For some I2C clients this may be an error,
but for an EEPROM this might be an "oh, it might still be erasing a
page, let's try again after some defined delay".

Think of 'i2cdetect': If we printout something in the -ENXIO case (no
device responded to the address), the log file would have more than 100
entries on a typical I2C bus. Although we know that -ENXIO will be the
majority of cases and are fine with it.

> As you did before, I would just remove the printout here.

Maybe we could because there is still the "Terminating the current
operation" string as debug message making the code flow still clear.

> I will wait a bit for more comments and take patches 1 to 5 so
> that I can unburden you a little from them.

The patches have no dependencies. To keep mail traffic low, I suggest
you continue reviewing and I only resend the i801 patch?

Happy hacking,

   Wolfram


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

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

iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAmYXixwACgkQFA3kzBSg
KbZNVhAAlFgAEIg426rC7dki0Z759Q8EEARoqJcP/hRA+zOl/JQJ7z9uN7iZ5kNB
aJBMkyZ89ULzpwgtNnQaumua6M/VtQFJuJmnk1157Ff1MRq/nEA29q6botELWFoi
Gu2q2/iZbGf4rNn7CKhCEaPXedKo9g3546S2Dj+xyfGZSTGTjCJ0nlTl+1HraBYy
+OobuaFyQu8TYspodNKiPEiIzfHEtaKOwuWHeUAg5pTJvosYKJhIJqipmG2i6yV4
lTqMdIuCFoTPTygtV/Pykl5raLeyIhsH+zOPitafqHkpgP6pD9RKXoVwni6MMbMH
jKTBWInAhHohyn9IjO9RiwjPs2pmnzfcWwXrjNnJjJtuohFeYZ0YZeYDYW7dlGnl
WZyG+p66Fl51jDwIGIk97UEiG5GagsYXlIzD/iU89AeVdCcLVed14MgcLcDKIElU
NUslAX2K2EtyydwVaJETGrIRN8OSFrCjQBm/VG4mEJkBB89AOM9T0ZkfANAfJpLO
MeA1Hp4+bNLrvfBNPtWTgpXa7bipUDVuUNyoIGO/BqdRl5wLT+wfsz+u6XWHkPLw
pqJWWcxwBxT/STxxuYkRBD65OPCV/zphq7HoEgf186ry1BFwVnFpohBSL09bu9Lc
gKZ+UF9IQl7MQI3Jl1FwtYs+JC1c4b7WaKfG8GwZsRdqY0K/dX0=
=1D6q
-----END PGP SIGNATURE-----

--j6qt37qfsj4aexfx--