Re: [PATCH v2 2/2] pinctrl: qcom: Introduce IPQ5210 TLMM driver

[Bjorn Andersson <andersson@kernel.org>]

On Wed, Mar 18, 2026 at 12:44:31PM +0530, Kathiravan Thirumoorthy wrote:
> diff --git a/drivers/pinctrl/qcom/pinctrl-ipq5210.c b/drivers/pinctrl/qcom/pinctrl-ipq5210.c
[..]
> +static const struct pinfunction ipq5210_functions[] = {
[..]
> +	MSM_PIN_FUNCTION(qup_se5_l01),
> +	MSM_PIN_FUNCTION(qup_se5_l10),
> +	MSM_PIN_FUNCTION(qup_se5_l11),
> +	MSM_PIN_FUNCTION(qup_se5_l2),
> +	MSM_PIN_FUNCTION(qup_se5_l3),
> +	MSM_PIN_FUNCTION(qup_se5_l4),
> +	MSM_PIN_FUNCTION(qup_se5_l5),

Listing each pin of each QUP SE as their own function forces the DT
author to write one state definition per pin. Group these into their
logical functions instead.

Same thing with other logical functions that you have split into
multiple separate functions.

> +	MSM_PIN_FUNCTION(resout),
> +	MSM_PIN_FUNCTION(rx_los00),
> +	MSM_PIN_FUNCTION(rx_los01),
> +	MSM_PIN_FUNCTION(rx_los10),
> +	MSM_PIN_FUNCTION(rx_los11),
> +	MSM_PIN_FUNCTION(rx_los20),
> +	MSM_PIN_FUNCTION(rx_los21),
> +	MSM_PIN_FUNCTION(sdc_clk),
> +	MSM_PIN_FUNCTION(sdc_cmd),
> +	MSM_PIN_FUNCTION(sdc_data),
> +	MSM_PIN_FUNCTION(tsens_max),
> +};
> +
> +static const struct msm_pingroup ipq5210_groups[] = {
> +	[0] = PINGROUP(0, sdc_data, qspi_data, pwm, _, _, _, _, _, _),
> +	[1] = PINGROUP(1, sdc_data, qspi_data, pwm, _, _, _, _, _, _),
> +	[2] = PINGROUP(2, sdc_data, qspi_data, pwm, _, _, _, _, _, _),
> +	[3] = PINGROUP(3, sdc_data, qspi_data, pwm, _, _, _, _, _, _),
> +	[4] = PINGROUP(4, sdc_cmd, qspi_cs_n, _, _, _, _, _, _, _),
> +	[5] = PINGROUP(5, sdc_clk, qspi_clk, _, _, _, _, _, _, _),
> +	[6] = PINGROUP(6, qup_se0_l2, led0, pwm, _, cri_trng0, qdss_tracedata_a, _, _, _),
> +	[7] = PINGROUP(7, qup_se0_l3, led1, pwm, _, cri_trng1, qdss_tracedata_a, _, _, _),
> +	[8] = PINGROUP(8, qup_se0_l0, pwm, audio_pri2, audio_pri2, _, cri_trng2, qdss_tracedata_a, _, _),

How can both function 3 and 4 be "audio_pri2", do you expect the system
integrator to be able to select function 4?

> +	[9] = PINGROUP(9, qup_se0_l1, led2, pwm, _, cri_trng3, qdss_tracedata_a, _, _, _),
> +	[10] = PINGROUP(10, pon_rx_los, qup_se3_l3, pwm, _, _, qdss_tracedata_a, _, _, _),
> +	[11] = PINGROUP(11, pon_active_led, qup_se3_l2, pwm, _, _, qdss_tracedata_a, _, _, _),
> +	[12] = PINGROUP(12, pon_tx_dis, qup_se2_l3, pwm, audio_pri0, audio_pri0, _, qrng_rosc0, qdss_tracedata_a, _),
> +	[13] = PINGROUP(13, gpn_tx_dis, qup_se2_l2, pwm, audio_pri3, audio_pri3, _, qrng_rosc1, qdss_tracedata_a, _),

Same here.

Regards,
Bjorn