Re: [PATCH v3 2/2] pinctrl: qcom: Add Shikra pinctrl driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Inconsistent eGPIO (Embedded GPIO) hardware configuration.
--

commit a63bd64b64bae3d595ab0e510830b193aeee7ba6
Author: Komal Bajaj <komal.bajaj@oss.qualcomm.com>

pinctrl: qcom: Add Shikra pinctrl driver

Add pinctrl driver for TLMM block found in Shikra SoC.

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

[ ... ]

> +#define PINGROUP(id, f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, f11)	\
> +	{						\
> +		.grp = PINCTRL_PINGROUP("gpio" #id,	\
> +			gpio##id##_pins,		\
> +			ARRAY_SIZE(gpio##id##_pins)),	\
> +		.funcs = (int[]){			\
> +			msm_mux_gpio, /* gpio mode */	\
> +			msm_mux_##f1,			\
> +			msm_mux_##f2,			\
> +			msm_mux_##f3,			\
> +			msm_mux_##f4,			\
> +			msm_mux_##f5,			\
> +			msm_mux_##f6,			\
> +			msm_mux_##f7,			\
> +			msm_mux_##f8,			\
> +			msm_mux_##f9,			\
> +			msm_mux_##f10,			\
> +			msm_mux_##f11 /* egpio mode */	\
> +		},					\
> +		.nfuncs = 12,				\
> +		.ctl_reg = REG_SIZE * id,		\
> +		.io_reg = 0x4 + REG_SIZE * id,		\
> +		.intr_cfg_reg = 0x8 + REG_SIZE * id,	\
> +		.intr_status_reg = 0xc + REG_SIZE * id,	\
> +		.mux_bit = 2,			\
> +		.pull_bit = 0,			\
> +		.drv_bit = 6,			\
> +		.egpio_enable = 12,		\
> +		.egpio_present = 11,	\

[ ... ]

> +static const struct msm_pingroup shikra_groups[] = {
> +	[0] = PINGROUP(0, qup0_se0, m_voc, _, phase_flag, _, _, _, _, _, _, _),
> +	[1] = PINGROUP(1, qup0_se0, mpm_pwr, ddr_bist, _, phase_flag, atest_tsens, _, _, _, _, _),

[ ... ]

> +static const struct msm_pinctrl_soc_data shikra_tlmm = {
> +	.pins = shikra_pins,
> +	.npins = ARRAY_SIZE(shikra_pins),
> +	.functions = shikra_functions,
> +	.nfunctions = ARRAY_SIZE(shikra_functions),
> +	.groups = shikra_groups,
> +	.ngroups = ARRAY_SIZE(shikra_groups),
> +	.ngpios = 166,
> +	.wakeirq_map = shikra_mpm_map,
> +	.nwakeirq_map = ARRAY_SIZE(shikra_mpm_map),
> +	.egpio_func = 11,
> +};

Is the hardware eGPIO configuration completely implemented?

The driver configures .egpio_func = 11 in shikra_tlmm and defines
egpio_enable = 12 and egpio_present = 11 bits in the PINGROUP macro, indicating
that eGPIO is mapped to function index 11.

However, the driver does not define the msm_mux_egpio enum, the egpio_groups
array, or MSM_PIN_FUNCTION(egpio). Instead, it passes the dummy function _ for
the 11th mux slot (f11) across all pins in shikra_groups.

Because .egpio_func is non-zero, the core pinctrl-msm driver will actively
clear bit 12 (egpio_enable) of ctl_reg during any pin mux operation if bit 11
is set.

If the SoC does not support eGPIO and these bits serve another purpose, could
this unexpectedly change the hardware state on every pin mux change? Conversely,
if it does support eGPIO, it appears the feature would be unusable because it
cannot be selected via the device tree since the function is not exposed.

Could you clarify if eGPIO is intended to be supported on this SoC?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-shikra-pinctrl-v3-0-771144cdc411@oss.qualcomm.com?part=2