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

On 5/9/2026 1:02 AM, [email protected] wrote:
> 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 <[email protected]>
>
> 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?

Yes, Shikra doesn’t seem to support eGPIO.
I’ll remove the eGPIO-related bits and .egpio_func to avoid any 
unintended behavior.

Thanks
Komal