Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 7 Feb 2022 12:27:19 +0100
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Tyrone Ting <warp5tw@gmail.com>
Cc:     avifishman70@gmail.com, tmaimon77@gmail.com, tali.perry1@gmail.com,
        venture@google.com, yuenn@google.com, benjaminfair@google.com,
        robh+dt@kernel.org, krzysztof.kozlowski@canonical.com,
        semen.protsenko@linaro.org, yangyicong@hisilicon.com,
        wsa@kernel.org, jie.deng@intel.com, sven@svenpeter.dev,
        bence98@sch.bme.hu, lukas.bulwahn@gmail.com, arnd@arndb.de,
        olof@lixom.net, andriy.shevchenko@linux.intel.com,
        tali.perry@nuvoton.com, Avi.Fishman@nuvoton.com,
        tomer.maimon@nuvoton.com, KWLIU@nuvoton.com, JJLIU0@nuvoton.com,
        kfting@nuvoton.com, devicetree@vger.kernel.org,
        openbmc@lists.ozlabs.org, linux-i2c@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/6] i2c: npcm: Fix timeout calculation
Message-ID: <YgECF6jZ7wj8TOfl@latitude>
References: <20220207063338.6570-1-warp5tw@gmail.com>
 <20220207063338.6570-3-warp5tw@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="RZxU18GLaf6NWGkB"
Content-Disposition: inline
In-Reply-To: <20220207063338.6570-3-warp5tw@gmail.com>
Precedence: bulk


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

Hello,

On Mon, Feb 07, 2022 at 02:33:34PM +0800, Tyrone Ting wrote:
> From: Tali Perry <tali.perry1@gmail.com>
>=20
> Use adap.timeout for timeout calculation instead of hard-coded
> value of 35ms.


> Use syscon to access gcr, instead of "compatible".

Please put the GCR/syscon change into a separate patch, because it is
not obvious from the commit title that such a change would happen in
this patch.

>=20
> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller drive=
r")
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
>=20
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-np=
cm7xx.c
> index 2ad166355ec9..ddeee6f53621 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -2047,7 +2047,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter =
*adap, struct i2c_msg *msgs,
>  	u16 nwrite, nread;
>  	u8 *write_data, *read_data;
>  	u8 slave_addr;
> -	int timeout;
> +	unsigned long timeout;
>  	int ret =3D 0;
>  	bool read_block =3D false;
>  	bool read_PEC =3D false;
> @@ -2099,13 +2099,13 @@ static int npcm_i2c_master_xfer(struct i2c_adapte=
r *adap, struct i2c_msg *msgs,
>  	 * 9: bits per transaction (including the ack/nack)
>  	 */
>  	timeout_usec =3D (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + =
nwrite);
> -	timeout =3D max(msecs_to_jiffies(35), usecs_to_jiffies(timeout_usec));
> +	timeout =3D max(bus->adap.timeout, usecs_to_jiffies(timeout_usec));
>  	if (nwrite >=3D 32 * 1024 || nread >=3D 32 * 1024) {
>  		dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
>  		return -EINVAL;
>  	}
> =20
> -	time_left =3D jiffies + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
> +	time_left =3D jiffies + timeout + 1;
>  	do {
>  		/*
>  		 * we must clear slave address immediately when the bus is not
> @@ -2131,7 +2131,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter =
*adap, struct i2c_msg *msgs,
>  	}
> =20
>  	npcm_i2c_init_params(bus);
> -	bus->dest_addr =3D slave_addr;
> +	bus->dest_addr =3D slave_addr << 1;

This seems unrelated to timeout calculation.

>  	bus->msgs =3D msgs;
>  	bus->msgs_num =3D num;
>  	bus->cmd_err =3D 0;
> @@ -2233,9 +2233,9 @@ static int npcm_i2c_probe_bus(struct platform_devic=
e *pdev)
>  	struct i2c_adapter *adap;
>  	struct clk *i2c_clk;
>  	static struct regmap *gcr_regmap;
> -	static struct regmap *clk_regmap;
>  	int irq;
>  	int ret;
> +	struct device_node *np =3D pdev->dev.of_node;
> =20
>  	bus =3D devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>  	if (!bus)
> @@ -2250,15 +2250,11 @@ static int npcm_i2c_probe_bus(struct platform_dev=
ice *pdev)
>  		return PTR_ERR(i2c_clk);
>  	bus->apb_clk =3D clk_get_rate(i2c_clk);
> =20
> -	gcr_regmap =3D syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr"=
);
> +	gcr_regmap =3D syscon_regmap_lookup_by_phandle(np, "syscon");
>  	if (IS_ERR(gcr_regmap))
>  		return PTR_ERR(gcr_regmap);
>  	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
> =20
> -	clk_regmap =3D syscon_regmap_lookup_by_compatible("nuvoton,npcm750-clk"=
);
> -	if (IS_ERR(clk_regmap))
> -		return PTR_ERR(clk_regmap);

I agree that clk_regmap can be removed, but I'd rather see it in a
separate patch, because it's unrelated to the timeout calculation.

> -
>  	bus->reg =3D devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(bus->reg))
>  		return PTR_ERR(bus->reg);
> @@ -2269,7 +2265,7 @@ static int npcm_i2c_probe_bus(struct platform_devic=
e *pdev)
>  	adap =3D &bus->adap;
>  	adap->owner =3D THIS_MODULE;
>  	adap->retries =3D 3;
> -	adap->timeout =3D HZ;
> +	adap->timeout =3D msecs_to_jiffies(35);
>  	adap->algo =3D &npcm_i2c_algo;
>  	adap->quirks =3D &npcm_i2c_quirks;
>  	adap->algo_data =3D bus;


Best regards,
Jonathan

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

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

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmIBAhcACgkQCDBEmo7z
X9uCbA/+NL/rpWypOlFcnBB6ISQecB6S6RoUnN3cnFt2SaNw2nQGu5P/Nrzbx1L4
9fBT0i385RpgaUqTHIV3ldE+75kstoCC4af1nuTXWG03Y6jhEbly/Hs0u6f4TOnJ
2y3DnEigDmCcG7K57OlsucUlhQMpJ4591qgZJV80Jf6ZjkmBR6VXnSPY0ISAwOjB
RH+QZyG91QRDIS7Az8Xw0PnaEhy/YntqCGICaT2D3yXcTWb8qPGJNu0uaBOVP+9o
vNPbJ76zLdsuW351xKFE2uY3cxnqwwznqeU4dzWBKe/wDC77wCq0Irsi+y7dA3S5
dna8ZzSXBPP0wDAltMiMqQKrZp8L21DWZeT/V2/ohg7XhQ9Q2fLIWLhf9XUupTAZ
rEJH+Ebq1GVSJZdEj7lKmxUlL2C1FvMb8g4yMItrs/5+ZpsY6XZbZHonWJGJ32IZ
zS/mCGJyIwb8/K2J/ts9zYCOqXgaybLXmyf5vWwEp3udg5ZicnruqiG4xtZDysSk
KLKQpdLKVaPwqdzHdZaxshXP1P/cgF53BAy0uIadanoBRF0MdgaJdOZuA69jVN5Q
Jr7V27NC2zCWOX+QG9x63hS5MXCnwcwCUn9qgS3WezBZWB1Y7/QPfuOZC8ETgIP0
Le3Ngiv9rt9esIaW9YjoW3/+QBxhulJKMiYerTqGWN/2fVD35CI=
=QKMT
-----END PGP SIGNATURE-----

--RZxU18GLaf6NWGkB--