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

[Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>]

On 3/19/2026 7:47 AM, Bjorn Andersson wrote:
> 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.

Sure, will take care of this in next spin.

>
>> +	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?

Function 3 and 4 are audio_pri_mclk_out and audio_pri_mclk_in 
respectively and either one of them can be selected based on the codec's 
mclock requirement. Let me expand the full function to avoid these 
confusion in the next spin.

>
>> +	[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