Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 23 Mar 2022 13:31:05 +0100
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Prathamesh Shete <pshete@nvidia.com>
Cc:     linus.walleij@linaro.org, jonathanh@nvidia.com,
        linux-gpio@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-kernel@vger.kernel.org, smangipudi@nvidia.com,
        EJ Hsu <ejh@nvidia.com>
Subject: Re: [PATCH] pinctrl: tegra: Set SFIO mode to Mux Register
Message-ID: <YjsTCRdc3yCLZkVY@orome>
References: <20220311043015.4027-1-pshete@nvidia.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="aDwnUUzQs80VnPan"
Content-Disposition: inline
In-Reply-To: <20220311043015.4027-1-pshete@nvidia.com>
User-Agent: Mutt/2.2.1 (c8109e14) (2022-02-19)
Precedence: bulk


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

On Fri, Mar 11, 2022 at 10:00:15AM +0530, Prathamesh Shete wrote:
> If the device has the 'sfsel' bit field, pin should be
> muxed to set to SFIO mode to be used for pinmux operation.
>=20
> Signed-off-by: EJ Hsu <ejh@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  drivers/pinctrl/tegra/pinctrl-tegra.c | 3 +++
>  1 file changed, 3 insertions(+)
>=20
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegr=
a/pinctrl-tegra.c
> index 50bd26a30ac0..30341c43da59 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -270,6 +270,9 @@ static int tegra_pinctrl_set_mux(struct pinctrl_dev *=
pctldev,
>  	val =3D pmx_readl(pmx, g->mux_bank, g->mux_reg);
>  	val &=3D ~(0x3 << g->mux_bit);
>  	val |=3D i << g->mux_bit;
> +	/* Set the SFIO/GPIO selection to SFIO when under pinmux control*/
> +	if (pmx->soc->sfsel_in_mux)
> +		val |=3D (1 << g->sfsel_bit);
>  	pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
> =20
>  	return 0;

So this is basically what tegra_pinctrl_gpio_disable_free() does. I'm
wondering if we need to do both, though. Are ->gpio_disable_free() and
->set_mux() always called in tandem? I suspect they are not because
otherwise this wouldn't be needed.

On the other hand, if ->set_mux() can be called in a code path without
->gpio_disable_free() then this may be necessary to get the pin out of
SF mode. But that doesn't necessarily mean that the reverse is true.
If it isn't possible for ->gpio_disable_free() to be called in a code
path that doesn't have ->set_mux() then this patch would make the former
implementation redundant.

That said, upon inspecting the pinmux core, I don't see a 1:1
correlation between the two, so this seems fine.

It might be worth stating in the commit message what the practical
implications are of this. That is, you're explaining what you do in the
commit message and assert that this is what should be done. But it'd be
more useful to say *why* this is necessary. Specifically if this fixes a
bug, then say what kind of bug this would fix.

Thierry

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

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

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmI7EwcACgkQ3SOs138+
s6EAVQ//UN048EY2ruCY7siM7mFkIa7bi2ZZNMtJUrAOCifmmEVcbiGLdznlNicU
OZZswzDUXCD5bq0Ouug4AaFX9GkadYwm5UvI/FpjEzG29LSvcdb3rqECX3DzDwvn
WjIGSI1QVH450nCg+FP2GtypTr8FvW2tsHZMKwia+xj2HMZQf63EjbMzDABAzvaC
FDL/iVzTisNyxDqhZrEVegydjTwNxJug1dsEfCH7obu47QcVnuAknQOigHn2KFsx
9IqCrva8VLTAm8i9kgRu7b1YJOhbMQRcQ+JzrYpdYi68pHkU+LtajTsqsIbjEEp3
wnGdUP3gqinwd2km0ORV4DqG3BOQK4oXQDi5kOJjiGKuMA1wCmu3A2daTd11VRAN
5IXTc3FkY8va1Fr4PRGJ3NjufGbovFoImOy5sT1AZ+jxTaNNN88xgJB7BrNGvNTE
lNxcCQm1j7UCVo7oyiVj689hWRJJqSTxUst93pY9zAU8QyCUIjVrOecypzF9nNIV
aawPUoWOy/eKP/4p/055sAFtfYafXCXcSQJVDw+L3bWMEYE3n3oKEDcrG8hHizz9
FUH7YcBKazmBF+qgcDFNbbyYznC0AvP8bdkwJNvu73p89iu9OxX+MeibU+yPoXxd
niR1JfvkDzWeDPXLk2A16c1iwn8VeiRGNwd5zIw2qgN8Qhgx614=
=5/zk
-----END PGP SIGNATURE-----

--aDwnUUzQs80VnPan--