Re: [PATCH 2/2] pinctrl: qcom: Add MSM8952 tlmm pinctrl driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Mismatch between pin group function lists (`*_groups`) and `PINGROUP` definitions.
- [Low] The Kconfig symbol PINCTRL_MSM8952 lacks `depends on ARM64 || COMPILE_TEST`.
- [Low] The macro expansion for `MSM_PIN_FUNCTION(NA)` relies on an undeclared `NA_groups` array.
--

--- Patch [2]: [PATCH 2/2] pinctrl: qcom: Add MSM8952 tlmm pinctrl driver ---
commit 58f7f872112151d63645301862bbc088eef619bb
Author: Muzaffer Kadir <[email protected]>
pinctrl: qcom: Add MSM8952 tlmm pinctrl driver

This commit adds the pinctrl, pinmux, pinconf, and gpiolib driver for
the Qualcomm TLMM block found on the Qualcomm MSM8952 platform.

> diff --git a/drivers/pinctrl/qcom/Kconfig.msm b/drivers/pinctrl/qcom/Kconfig.msm
> index 9409e678ec6dc..8b631e641e161 100644
> --- a/drivers/pinctrl/qcom/Kconfig.msm
> +++ b/drivers/pinctrl/qcom/Kconfig.msm
> @@ -222,6 +222,12 @@ config PINCTRL_MSM8917
>  	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
>  	  Qualcomm TLMM block found on the Qualcomm MSM8917, MSM8937 platform.
>  
> +config PINCTRL_MSM8952
> +	tristate "Qualcomm 8952 pin controller driver"

[Severity: Low]
Should this Kconfig entry include a dependency on ARM64 || COMPILE_TEST?

Omitting it might expose the driver to build systems for unsupported
architectures.

> +	help
> +	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> +	  Qualcomm TLMM block found on the Qualcomm MSM8952 platform.
> +

[ ... ]

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm8952.c b/drivers/pinctrl/qcom/pinctrl-msm8952.c
> new file mode 100644
> index 0000000000000..b21d035fc5042
> --- /dev/null
> +++ b/drivers/pinctrl/qcom/pinctrl-msm8952.c

[ ... ]

> +static const char *const qdss_tracedata_b_groups[] = {
> +	"gpio8",  "gpio9",  "gpio12", "gpio13", "gpio23", "gpio42",
> +	"gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio66",
> +	"gpio86", "gpio87", "gpio88", "gpio92",
> +};

[Severity: Medium]
Are some of these GPIOs disconnected from the actual PINGROUP function
assignments?

If a device tree requests a function listed in a group array like this one
but the function is not supported by the actual PINGROUP definition, could
msm_pinmux_set_mux() fail to find the function, trigger a WARN_ON, and fail
with -EINVAL?

[ ... ]

> +static const char *const atest_combodac_to_gpio_native_groups[] = {
> +	"gpio4",  "gpio12", "gpio13", "gpio20", "gpio21", "gpio28",  "gpio29",
> +	"gpio30", "gpio39", "gpio40", "gpio41", "gpio42", "gpio43",  "gpio44",
> +	"gpio45", "gpio46", "gpio47", "gpio48", "gpio67", "gpio115",
> +};

[Severity: Medium]
Similarly, is there a mismatch between this function list and the PINGROUP
definitions?

[ ... ]

> +static const struct pinfunction msm8952_functions[] = {

[ ... ]

> +	MSM_PIN_FUNCTION(atest_char),
> +	MSM_PIN_FUNCTION(atest_tsens),
> +	MSM_PIN_FUNCTION(bimc_dte1),
> +	MSM_PIN_FUNCTION(NA),

[Severity: Low]
Will this fail to compile?

The macro expansion for MSM_PIN_FUNCTION(NA) appears to rely on an NA_groups
array, which does not seem to be declared anywhere in the file.

> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2