linux-kernel - Re: [PATCH v2 2/3] pinctrl: tegra: Add Tegra186 pinmux driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <yw2uglyxxx22d3lwyezy34wdniouu32zppfgwqs5omny3ge5zd@iuqo4qmi55a2>
Date: Tue, 10 Jun 2025 11:40:27 +0200
From: Thierry Reding <thierry.reding@...il.com>
To: webgeek1234@...il.com
Cc: Linus Walleij <linus.walleij@...aro.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Jonathan Hunter <jonathanh@...dia.com>, 
	Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] pinctrl: tegra: Add Tegra186 pinmux driver

On Sun, Jun 08, 2025 at 09:13:14PM -0500, Aaron Kling via B4 Relay wrote:
> From: Aaron Kling <webgeek1234@...il.com>
> 
> This is based on Nvidia's downstream 5.10 driver, rewritten to match the
> mainline Tegra194 pinmux driver.

A few words upfront, to justify why I'm being pedantic. Originally the
pinmux drivers were generated using the tegra-pinmux-scripts[0]. This
project was later archived because Tegra210 was deemed to be the last
chip to need a pin controller. It then turned out that Tegra194 needed
pinmuxing for certain pins, and then more, so we ended up with a full
pinmux driver for it. However, we also deemed Tegra194 to be an
exception, so that's why that pinctrl driver was a one-off job.

I now regret these decisions because the same formatting mistakes are
now proliferating, which is exactly what the scripts were meant to
avoid.

One thing that's not clear from this patch set is whether we actually
need the Tegra186 pinmux driver, or you're only adding it because it
happens to be present in a 5.10 downstream driver. Do you actually have
a requirement for setting pins dynamically at runtime? Do you need to be
able to set a static configuration at boot that can't be set using some
earlier bootloader/firmware mechanism?

If we really need this pinctrl driver it may be worth resurrecting the
tegra-pinmux-scripts so that we can add these drivers based on the
generated files as originally intended.

As announced, there's a few pedantic nitpicks down below.

[0]: https://github.com/NVIDIA/tegra-pinmux-scripts

[...]
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra186.c b/drivers/pinctrl/tegra/pinctrl-tegra186.c
[...]
> +static const unsigned int pex_l0_rst_n_pa0_pins[] = {
> +	TEGRA_PIN_PEX_L0_RST_N_PA0,
> +};
> +static const unsigned int pex_l0_clkreq_n_pa1_pins[] = {
> +	TEGRA_PIN_PEX_L0_CLKREQ_N_PA1,
> +};

Typically there'd be a blank line to separate each of these structures.
Or maybe we can come up with some notation to make these single lines?

> +static const unsigned int sdmmc4_clk_pins[] = {};
> +
> +static const unsigned int sdmmc4_cmd_pins[] = {};
> +
> +static const unsigned int sdmmc4_dqs_pins[] = {};
> +
> +static const unsigned int sdmmc4_dat7_pins[] = {};
> +
> +static const unsigned int sdmmc4_dat6_pins[] = {};
> +
> +static const unsigned int sdmmc4_dat5_pins[] = {};
> +
> +static const unsigned int sdmmc4_dat4_pins[] = {};
> +
> +static const unsigned int sdmmc4_dat3_pins[] = {};
> +
> +static const unsigned int sdmmc4_dat2_pins[] = {};
> +
> +static const unsigned int sdmmc4_dat1_pins[] = {};
> +
> +static const unsigned int sdmmc4_dat0_pins[] = {};

These look out of place. Ideally these would simply be NULL and 0 in the
respective PINGROUP definitions, but not sure if that's something that
can be done with the preprocessor. Are these guaranteed not to take up
any space in the generated binary?

[...]
> +#define PIN_PINGROUP_ENTRY_Y(r, bank, pupd, e_io_hv, e_lpbk, e_input,	\
> +			     e_lpdr, e_pbias_buf, gpio_sfio_sel, \
> +			     e_od, schmitt_b, drvtype, epreemp,	\
> +			     io_reset, rfu_in)			\

It looks like there's an alignment issue in this macro.

> +#define PINGROUP(pg_name, f0, f1, f2, f3, r, bank, pupd, e_io_hv, e_lpbk, e_input, e_lpdr, e_pbias_buf, \
> +			gpio_sfio_sel, e_od, schmitt_b, drvtype, epreemp, io_reset, rfu_in)		\
> +	{							\
> +		.name = #pg_name,				\
> +		.pins = pg_name##_pins,				\
> +		.npins = ARRAY_SIZE(pg_name##_pins),		\
> +			.funcs = {				\
> +				TEGRA_MUX_##f0,			\
> +				TEGRA_MUX_##f1,			\
> +				TEGRA_MUX_##f2,			\
> +				TEGRA_MUX_##f3,			\
> +			},					\

This .funcs substructure seems to be indented wrongly. I see that that's
also the case for Tegra194, but it's correct on Tegra210, so it looks
like you copied from the wrong source. =)

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)