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:   Thu, 17 Sep 2020 13:58:02 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Dmitry Osipenko <digetx@gmail.com>
Cc:     Jonathan Hunter <jonathanh@nvidia.com>,
        Laxman Dewangan <ldewangan@nvidia.com>,
        Wolfram Sang <wsa@the-dreams.de>,
        =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= <mirq-linux@rere.qmqm.pl>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        linux-i2c@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 25/34] i2c: tegra: Factor out register polling into
 separate function
Message-ID: <20200917115802.GD3515672@ulmo>
References: <20200908224006.25636-1-digetx@gmail.com>
 <20200908224006.25636-26-digetx@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="stFzI3XAseUQ09UW"
Content-Disposition: inline
In-Reply-To: <20200908224006.25636-26-digetx@gmail.com>
User-Agent: Mutt/1.14.6 (2020-07-11)
Precedence: bulk


--stFzI3XAseUQ09UW
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Sep 09, 2020 at 01:39:57AM +0300, Dmitry Osipenko wrote:
> Factor out register polling into a separate function in order to remove
> boilerplate code and make code cleaner.
>=20
> Reviewed-by: Micha=C5=82 Miros=C5=82aw <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 57 +++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 32 deletions(-)
>=20
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegr=
a.c
> index 405b87e28a98..e071de9ce106 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -514,10 +514,24 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev =
*i2c_dev)
>  	i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT);
>  }
> =20
> +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> +				   u32 reg, u32 mask, u32 delay_us,
> +				   u32 timeout_us)
> +{
> +	void __iomem *addr =3D i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
> +	u32 val;
> +
> +	if (!i2c_dev->is_curr_atomic_xfer)
> +		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
> +						  delay_us, timeout_us);
> +
> +	return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
> +						 delay_us, timeout_us);
> +}
> +
>  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>  {
> -	u32 mask, val, offset, reg_offset;
> -	void __iomem *addr;
> +	u32 mask, val, offset;
>  	int err;
> =20
>  	if (i2c_dev->hw->has_mst_fifo) {
> @@ -534,16 +548,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_de=
v *i2c_dev)
>  	val |=3D mask;
>  	i2c_writel(i2c_dev, val, offset);
> =20
> -	reg_offset =3D tegra_i2c_reg_addr(i2c_dev, offset);
> -	addr =3D i2c_dev->base + reg_offset;
> -
> -	if (i2c_dev->is_curr_atomic_xfer)
> -		err =3D readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask),
> -							1000, 1000000);
> -	else
> -		err =3D readl_relaxed_poll_timeout(addr, val, !(val & mask),
> -						 1000, 1000000);
> -
> +	err =3D tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000);
>  	if (err) {
>  		dev_err(i2c_dev->dev, "failed to flush FIFO\n");
>  		return err;
> @@ -553,30 +558,18 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_d=
ev *i2c_dev)
> =20
>  static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>  {
> -	unsigned long reg_offset;
> -	void __iomem *addr;
> -	u32 val;
>  	int err;
> =20
> -	if (i2c_dev->hw->has_config_load_reg) {
> -		reg_offset =3D tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
> -		addr =3D i2c_dev->base + reg_offset;
> -		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
> +	if (!i2c_dev->hw->has_config_load_reg)
> +		return 0;
> =20
> -		if (i2c_dev->is_curr_atomic_xfer)
> -			err =3D readl_relaxed_poll_timeout_atomic(
> -						addr, val, val =3D=3D 0, 1000,
> -						I2C_CONFIG_LOAD_TIMEOUT);
> -		else
> -			err =3D readl_relaxed_poll_timeout(
> -						addr, val, val =3D=3D 0, 1000,
> -						I2C_CONFIG_LOAD_TIMEOUT);
> +	i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
> =20
> -		if (err) {
> -			dev_warn(i2c_dev->dev,
> -				 "timeout waiting for config load\n");
> -			return err;
> -		}
> +	err =3D tegra_i2c_poll_register(i2c_dev, I2C_CONFIG_LOAD, 0xffffffff,
> +				      1000, I2C_CONFIG_LOAD_TIMEOUT);
> +	if (err) {
> +		dev_warn(i2c_dev->dev, "timeout waiting for config load\n");
> +		return err;
>  	}

The deindentation in this hunk is messing up the diffstat in my opinion.
It would probably be worth splitting that into a separate patch to make
it more evident that this patch actually removes boilerplate.

Thierry

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

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

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAl9jT0oACgkQ3SOs138+
s6GUHxAAnwK9bQJoihAW8/1DAfvP9xNcE5rcJoKtrMJAxgpAFcZQQiMUKSerk+8G
z5YRnKmv/JOZp9Yqavl6U6xDCYv9fR+D3TBQ34A3TRIrpazPwhffvt+QkzXaaj76
2ybOQY4x0UmQW6ipWWOxBSzxy0Zql1bpUU4z+Mh7RZaR7vKqv0JuVdwLnERvPZJM
+YV98iZJoJIIVRM5QFJBL8jaL6nqkOh9vmOfONjAL8z/xqYFuYKMWpTkf1vOJ6P3
peyRgTa9tgmvfWU/Rjr8DdBjBM/Y5s8aiJA4YO60S6R29fWEybnOud4UOXOixhM+
D8PMFGiA6B+iwt4EAgwBD7eio8bxwQJ1vz+mqWyzrUvnDpCGMTkd6tzUX3M2v3lr
WmIBQ09e9mK2H7DGn70WGn9GETn8ElMDhERQrAAIe+4u3BD9MhQAcK9nzMcIydyx
OvAcSNghr4of1rmVYvcn5EFluRyPLidGqB8HxZP75BDPsRQ8NBfJP/rTGJi62JZy
mgcfpdAXXCR0nw+efiVuAHYtqNJKnIDzgevU/zqIMEa336WCqaksdCdQLJcctWZ3
z6+z+DVL4ueZ8aiBBk18TQjtxK5fTVZF9fWI/SUU4ZapvJicRIuHw66TMVOBUMFz
R7q09gbPtYxwra6UOCmBIs+yiLLWpn7a+X7mFi8XS5mZOnXjk3g=
=CIb2
-----END PGP SIGNATURE-----

--stFzI3XAseUQ09UW--