2024-01-29 09:28:11

by Tengfei Fan

[permalink] [raw]
Subject: [PATCH v2 2/2] dt-bindings: pinctrl: qcom: consolidate functions to match with driver

Consolidate functions to match with SM4450 pinctrl driver, because
consolidate functions are being used in SM4450 pinctrl driver.

Fixes: 7bf8b78f86db ("dt-bindings: pinctrl: qcom: Add SM4450 pinctrl")
Signed-off-by: Tengfei Fan <[email protected]>
---
.../bindings/pinctrl/qcom,sm4450-tlmm.yaml | 51 +++++++------------
1 file changed, 17 insertions(+), 34 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
index bb675c8ec220..4109104de054 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
@@ -72,40 +72,23 @@ $defs:
description:
Specify the alternative function to be configured for the specified
pins.
- enum: [ gpio, atest_char, atest_char0, atest_char1, atest_char2,
- atest_char3, atest_usb0, atest_usb00, atest_usb01, atest_usb02,
- atest_usb03, audio_ref, cam_mclk, cci_async, cci_i2c,
- cci_timer0, cci_timer1, cci_timer2, cci_timer3, cci_timer4,
- cmu_rng0, cmu_rng1, cmu_rng2, cmu_rng3, coex_uart1, cri_trng,
- cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1,
- dp0_hot, gcc_gp1, gcc_gp2, gcc_gp3, host2wlan_sol, ibi_i3c,
- jitter_bist, mdp_vsync, mdp_vsync0, mdp_vsync1, mdp_vsync2,
- mdp_vsync3, mi2s0_data0, mi2s0_data1, mi2s0_sck, mi2s0_ws,
- mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws, mi2s_mclk0,
- mi2s_mclk1, nav_gpio0, nav_gpio1, nav_gpio2, pcie0_clk,
- phase_flag0, phase_flag1, phase_flag10, phase_flag11,
- phase_flag12, phase_flag13, phase_flag14, phase_flag15,
- phase_flag16, phase_flag17, phase_flag18, phase_flag19,
- phase_flag2, phase_flag20, phase_flag21, phase_flag22,
- phase_flag23, phase_flag24, phase_flag25, phase_flag26,
- phase_flag27, phase_flag28, phase_flag29, phase_flag3,
- phase_flag30, phase_flag31, phase_flag4, phase_flag5,
- phase_flag6, phase_flag7, phase_flag8, phase_flag9,
- pll_bist, pll_clk, prng_rosc0, prng_rosc1, prng_rosc2,
- prng_rosc3, qdss_cti, qdss_gpio, qdss_gpio0, qdss_gpio1,
- qdss_gpio10, qdss_gpio11, qdss_gpio12, qdss_gpio13, qdss_gpio14,
- qdss_gpio15, qdss_gpio2, qdss_gpio3, qdss_gpio4, qdss_gpio5,
- qdss_gpio6, qdss_gpio7, qdss_gpio8, qdss_gpio9, qlink0_enable,
- qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request,
- qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss,
- qup0_se0, qup0_se1, qup0_se2, qup0_se3, qup0_se4, qup0_se5,
- qup0_se6, qup0_se7, qup1_se0, qup1_se1, qup1_se2, qup1_se3,
- qup1_se4, qup1_se5, qup1_se6, sd_write, tb_trig, tgu_ch0,
- tgu_ch1, tgu_ch2, tgu_ch3, tmess_prng0, tmess_prng1,
- tmess_prng2, tmess_prng3, tsense_pwm1, tsense_pwm2, uim0_clk,
- uim0_data, uim0_present, uim0_reset, uim1_clk, uim1_data,
- uim1_present, uim1_reset, usb0_hs, usb0_phy, vfr_0, vfr_1,
- vsense_trigger ]
+ enum: [ gpio, atest_char, atest_usb0, audio_ref_clk, cam_mclk,
+ cci_async_in0, cci_i2c, cci, cmu_rng, coex_uart1_rx,
+ coex_uart1_tx, cri_trng, dbg_out_clk, ddr_bist,
+ ddr_pxi0_test, ddr_pxi1_test, gcc_gp1_clk, gcc_gp2_clk,
+ gcc_gp3_clk, host2wlan_sol, ibi_i3c_qup0, ibi_i3c_qup1,
+ jitter_bist_ref, mdp_vsync0_out, mdp_vsync1_out,
+ mdp_vsync2_out, mdp_vsync3_out, mdp_vsync, nav,
+ pcie0_clk_req, phase_flag, pll_bist_sync, pll_clk_aux,
+ prng_rosc, qdss_cti_trig0, qdss_cti_trig1, qdss_gpio,
+ qlink0_enable, qlink0_request, qlink0_wmss_reset,
+ qup0_se0, qup0_se1, qup0_se2, qup0_se3, qup0_se4,
+ qup1_se0, qup1_se1, qup1_se2, qup1_se2_l2, qup1_se3,
+ qup1_se4, sd_write_protect, tb_trig_sdc1, tb_trig_sdc2,
+ tgu_ch0_trigout, tgu_ch1_trigout, tgu_ch2_trigout,
+ tgu_ch3_trigout, tmess_prng, tsense_pwm1_out,
+ tsense_pwm2_out, uim0, uim1, usb0_hs_ac, usb0_phy_ps,
+ vfr_0_mira, vfr_0_mirb, vfr_1, vsense_trigger_mirnat ]

required:
- pins
--
2.17.1



2024-01-29 11:24:31

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt-bindings: pinctrl: qcom: consolidate functions to match with driver

On 29/01/2024 10:25, Tengfei Fan wrote:
> Consolidate functions to match with SM4450 pinctrl driver, because
> consolidate functions are being used in SM4450 pinctrl driver.

It's very difficult to see what changed from the diff, so please explain
brieflyl changes here.

What is that "consolidate functions" that you use in the driver?

Best regards,
Krzysztof


2024-01-30 04:39:14

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt-bindings: pinctrl: qcom: consolidate functions to match with driver

On Mon, Jan 29, 2024 at 05:25:12PM +0800, Tengfei Fan wrote:
> Consolidate functions to match with SM4450 pinctrl driver, because
> consolidate functions are being used in SM4450 pinctrl driver.

Please make your commit message start by describing the problem you're
solving, then provide a technical description of the change.

The problem statement can mention that you consolidated the functions in
the driver, but did not update the binding before it was merged. And for
a DeviceTree binding, it should document why this
non-backwards-compatible change is okay (such as the initial commit is
broken).

Regards,
Bjorn

>
> Fixes: 7bf8b78f86db ("dt-bindings: pinctrl: qcom: Add SM4450 pinctrl")
> Signed-off-by: Tengfei Fan <[email protected]>
> ---
> .../bindings/pinctrl/qcom,sm4450-tlmm.yaml | 51 +++++++------------
> 1 file changed, 17 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
> index bb675c8ec220..4109104de054 100644
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
> @@ -72,40 +72,23 @@ $defs:
> description:
> Specify the alternative function to be configured for the specified
> pins.
> - enum: [ gpio, atest_char, atest_char0, atest_char1, atest_char2,
> - atest_char3, atest_usb0, atest_usb00, atest_usb01, atest_usb02,
> - atest_usb03, audio_ref, cam_mclk, cci_async, cci_i2c,
> - cci_timer0, cci_timer1, cci_timer2, cci_timer3, cci_timer4,
> - cmu_rng0, cmu_rng1, cmu_rng2, cmu_rng3, coex_uart1, cri_trng,
> - cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1,
> - dp0_hot, gcc_gp1, gcc_gp2, gcc_gp3, host2wlan_sol, ibi_i3c,
> - jitter_bist, mdp_vsync, mdp_vsync0, mdp_vsync1, mdp_vsync2,
> - mdp_vsync3, mi2s0_data0, mi2s0_data1, mi2s0_sck, mi2s0_ws,
> - mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws, mi2s_mclk0,
> - mi2s_mclk1, nav_gpio0, nav_gpio1, nav_gpio2, pcie0_clk,
> - phase_flag0, phase_flag1, phase_flag10, phase_flag11,
> - phase_flag12, phase_flag13, phase_flag14, phase_flag15,
> - phase_flag16, phase_flag17, phase_flag18, phase_flag19,
> - phase_flag2, phase_flag20, phase_flag21, phase_flag22,
> - phase_flag23, phase_flag24, phase_flag25, phase_flag26,
> - phase_flag27, phase_flag28, phase_flag29, phase_flag3,
> - phase_flag30, phase_flag31, phase_flag4, phase_flag5,
> - phase_flag6, phase_flag7, phase_flag8, phase_flag9,
> - pll_bist, pll_clk, prng_rosc0, prng_rosc1, prng_rosc2,
> - prng_rosc3, qdss_cti, qdss_gpio, qdss_gpio0, qdss_gpio1,
> - qdss_gpio10, qdss_gpio11, qdss_gpio12, qdss_gpio13, qdss_gpio14,
> - qdss_gpio15, qdss_gpio2, qdss_gpio3, qdss_gpio4, qdss_gpio5,
> - qdss_gpio6, qdss_gpio7, qdss_gpio8, qdss_gpio9, qlink0_enable,
> - qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request,
> - qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss,
> - qup0_se0, qup0_se1, qup0_se2, qup0_se3, qup0_se4, qup0_se5,
> - qup0_se6, qup0_se7, qup1_se0, qup1_se1, qup1_se2, qup1_se3,
> - qup1_se4, qup1_se5, qup1_se6, sd_write, tb_trig, tgu_ch0,
> - tgu_ch1, tgu_ch2, tgu_ch3, tmess_prng0, tmess_prng1,
> - tmess_prng2, tmess_prng3, tsense_pwm1, tsense_pwm2, uim0_clk,
> - uim0_data, uim0_present, uim0_reset, uim1_clk, uim1_data,
> - uim1_present, uim1_reset, usb0_hs, usb0_phy, vfr_0, vfr_1,
> - vsense_trigger ]
> + enum: [ gpio, atest_char, atest_usb0, audio_ref_clk, cam_mclk,
> + cci_async_in0, cci_i2c, cci, cmu_rng, coex_uart1_rx,
> + coex_uart1_tx, cri_trng, dbg_out_clk, ddr_bist,
> + ddr_pxi0_test, ddr_pxi1_test, gcc_gp1_clk, gcc_gp2_clk,
> + gcc_gp3_clk, host2wlan_sol, ibi_i3c_qup0, ibi_i3c_qup1,
> + jitter_bist_ref, mdp_vsync0_out, mdp_vsync1_out,
> + mdp_vsync2_out, mdp_vsync3_out, mdp_vsync, nav,
> + pcie0_clk_req, phase_flag, pll_bist_sync, pll_clk_aux,
> + prng_rosc, qdss_cti_trig0, qdss_cti_trig1, qdss_gpio,
> + qlink0_enable, qlink0_request, qlink0_wmss_reset,
> + qup0_se0, qup0_se1, qup0_se2, qup0_se3, qup0_se4,
> + qup1_se0, qup1_se1, qup1_se2, qup1_se2_l2, qup1_se3,
> + qup1_se4, sd_write_protect, tb_trig_sdc1, tb_trig_sdc2,
> + tgu_ch0_trigout, tgu_ch1_trigout, tgu_ch2_trigout,
> + tgu_ch3_trigout, tmess_prng, tsense_pwm1_out,
> + tsense_pwm2_out, uim0, uim1, usb0_hs_ac, usb0_phy_ps,
> + vfr_0_mira, vfr_0_mirb, vfr_1, vsense_trigger_mirnat ]
>
> required:
> - pins
> --
> 2.17.1
>

2024-01-31 08:25:09

by Tengfei Fan

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt-bindings: pinctrl: qcom: consolidate functions to match with driver



On 1/29/2024 7:24 PM, Krzysztof Kozlowski wrote:
> On 29/01/2024 10:25, Tengfei Fan wrote:
>> Consolidate functions to match with SM4450 pinctrl driver, because
>> consolidate functions are being used in SM4450 pinctrl driver.
>
> It's very difficult to see what changed from the diff, so please explain
> brieflyl changes here.
>
> What is that "consolidate functions" that you use in the driver?
>
> Best regards,
> Krzysztof
>

please help to comfirm that the following description as commit message
whether it covers your concerns:

Pin alternative functions are consolidated(like: atest_char, phase_flag,
qdss_gpio etc.) in SM4450 pinctrl driver while they are still split in
DeviceTree binding file. SM4450 pinctrl function is broken if current
binding doc is followed. Update SM4450 pinctrl DeviceTree binding doc to
align with driver.


--
Thx and BRs,
Tengfei Fan

2024-01-31 08:28:53

by Tengfei Fan

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt-bindings: pinctrl: qcom: consolidate functions to match with driver



On 1/30/2024 12:38 PM, Bjorn Andersson wrote:
> On Mon, Jan 29, 2024 at 05:25:12PM +0800, Tengfei Fan wrote:
>> Consolidate functions to match with SM4450 pinctrl driver, because
>> consolidate functions are being used in SM4450 pinctrl driver.
>
> Please make your commit message start by describing the problem you're
> solving, then provide a technical description of the change.
>
> The problem statement can mention that you consolidated the functions in
> the driver, but did not update the binding before it was merged. And for
> a DeviceTree binding, it should document why this
> non-backwards-compatible change is okay (such as the initial commit is
> broken).
>
> Regards,
> Bjorn

Thank you for the comments, I will do new commit message as you
suggested that commit message start by describing the problem, then
provide a technical description of the chagne.

>
>>
>> Fixes: 7bf8b78f86db ("dt-bindings: pinctrl: qcom: Add SM4450 pinctrl")
>> Signed-off-by: Tengfei Fan <[email protected]>
>> ---
>> .../bindings/pinctrl/qcom,sm4450-tlmm.yaml | 51 +++++++------------
>> 1 file changed, 17 insertions(+), 34 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
>> index bb675c8ec220..4109104de054 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
>> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm4450-tlmm.yaml
>> @@ -72,40 +72,23 @@ $defs:
>> description:
>> Specify the alternative function to be configured for the specified
>> pins.
>> - enum: [ gpio, atest_char, atest_char0, atest_char1, atest_char2,
>> - atest_char3, atest_usb0, atest_usb00, atest_usb01, atest_usb02,
>> - atest_usb03, audio_ref, cam_mclk, cci_async, cci_i2c,
>> - cci_timer0, cci_timer1, cci_timer2, cci_timer3, cci_timer4,
>> - cmu_rng0, cmu_rng1, cmu_rng2, cmu_rng3, coex_uart1, cri_trng,
>> - cri_trng0, cri_trng1, dbg_out, ddr_bist, ddr_pxi0, ddr_pxi1,
>> - dp0_hot, gcc_gp1, gcc_gp2, gcc_gp3, host2wlan_sol, ibi_i3c,
>> - jitter_bist, mdp_vsync, mdp_vsync0, mdp_vsync1, mdp_vsync2,
>> - mdp_vsync3, mi2s0_data0, mi2s0_data1, mi2s0_sck, mi2s0_ws,
>> - mi2s2_data0, mi2s2_data1, mi2s2_sck, mi2s2_ws, mi2s_mclk0,
>> - mi2s_mclk1, nav_gpio0, nav_gpio1, nav_gpio2, pcie0_clk,
>> - phase_flag0, phase_flag1, phase_flag10, phase_flag11,
>> - phase_flag12, phase_flag13, phase_flag14, phase_flag15,
>> - phase_flag16, phase_flag17, phase_flag18, phase_flag19,
>> - phase_flag2, phase_flag20, phase_flag21, phase_flag22,
>> - phase_flag23, phase_flag24, phase_flag25, phase_flag26,
>> - phase_flag27, phase_flag28, phase_flag29, phase_flag3,
>> - phase_flag30, phase_flag31, phase_flag4, phase_flag5,
>> - phase_flag6, phase_flag7, phase_flag8, phase_flag9,
>> - pll_bist, pll_clk, prng_rosc0, prng_rosc1, prng_rosc2,
>> - prng_rosc3, qdss_cti, qdss_gpio, qdss_gpio0, qdss_gpio1,
>> - qdss_gpio10, qdss_gpio11, qdss_gpio12, qdss_gpio13, qdss_gpio14,
>> - qdss_gpio15, qdss_gpio2, qdss_gpio3, qdss_gpio4, qdss_gpio5,
>> - qdss_gpio6, qdss_gpio7, qdss_gpio8, qdss_gpio9, qlink0_enable,
>> - qlink0_request, qlink0_wmss, qlink1_enable, qlink1_request,
>> - qlink1_wmss, qlink2_enable, qlink2_request, qlink2_wmss,
>> - qup0_se0, qup0_se1, qup0_se2, qup0_se3, qup0_se4, qup0_se5,
>> - qup0_se6, qup0_se7, qup1_se0, qup1_se1, qup1_se2, qup1_se3,
>> - qup1_se4, qup1_se5, qup1_se6, sd_write, tb_trig, tgu_ch0,
>> - tgu_ch1, tgu_ch2, tgu_ch3, tmess_prng0, tmess_prng1,
>> - tmess_prng2, tmess_prng3, tsense_pwm1, tsense_pwm2, uim0_clk,
>> - uim0_data, uim0_present, uim0_reset, uim1_clk, uim1_data,
>> - uim1_present, uim1_reset, usb0_hs, usb0_phy, vfr_0, vfr_1,
>> - vsense_trigger ]
>> + enum: [ gpio, atest_char, atest_usb0, audio_ref_clk, cam_mclk,
>> + cci_async_in0, cci_i2c, cci, cmu_rng, coex_uart1_rx,
>> + coex_uart1_tx, cri_trng, dbg_out_clk, ddr_bist,
>> + ddr_pxi0_test, ddr_pxi1_test, gcc_gp1_clk, gcc_gp2_clk,
>> + gcc_gp3_clk, host2wlan_sol, ibi_i3c_qup0, ibi_i3c_qup1,
>> + jitter_bist_ref, mdp_vsync0_out, mdp_vsync1_out,
>> + mdp_vsync2_out, mdp_vsync3_out, mdp_vsync, nav,
>> + pcie0_clk_req, phase_flag, pll_bist_sync, pll_clk_aux,
>> + prng_rosc, qdss_cti_trig0, qdss_cti_trig1, qdss_gpio,
>> + qlink0_enable, qlink0_request, qlink0_wmss_reset,
>> + qup0_se0, qup0_se1, qup0_se2, qup0_se3, qup0_se4,
>> + qup1_se0, qup1_se1, qup1_se2, qup1_se2_l2, qup1_se3,
>> + qup1_se4, sd_write_protect, tb_trig_sdc1, tb_trig_sdc2,
>> + tgu_ch0_trigout, tgu_ch1_trigout, tgu_ch2_trigout,
>> + tgu_ch3_trigout, tmess_prng, tsense_pwm1_out,
>> + tsense_pwm2_out, uim0, uim1, usb0_hs_ac, usb0_phy_ps,
>> + vfr_0_mira, vfr_0_mirb, vfr_1, vsense_trigger_mirnat ]
>>
>> required:
>> - pins
>> --
>> 2.17.1
>>

--
Thx and BRs,
Tengfei Fan

2024-01-31 08:36:54

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt-bindings: pinctrl: qcom: consolidate functions to match with driver

On 31/01/2024 09:24, Tengfei Fan wrote:
>
>
> On 1/29/2024 7:24 PM, Krzysztof Kozlowski wrote:
>> On 29/01/2024 10:25, Tengfei Fan wrote:
>>> Consolidate functions to match with SM4450 pinctrl driver, because
>>> consolidate functions are being used in SM4450 pinctrl driver.
>>
>> It's very difficult to see what changed from the diff, so please explain
>> brieflyl changes here.
>>
>> What is that "consolidate functions" that you use in the driver?
>>
>> Best regards,
>> Krzysztof
>>
>
> please help to comfirm that the following description as commit message
> whether it covers your concerns:
>
> Pin alternative functions are consolidated(like: atest_char, phase_flag,
> qdss_gpio etc.) in SM4450 pinctrl driver while they are still split in
> DeviceTree binding file. SM4450 pinctrl function is broken if current
> binding doc is followed. Update SM4450 pinctrl DeviceTree binding doc to
> align with driver.

Please list the functions which are being removed and added. I usually
do not expect such commit msg, but this is an exception: diff is tricky
to parse.

Best regards,
Krzysztof


2024-01-31 08:44:04

by Tengfei Fan

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] dt-bindings: pinctrl: qcom: consolidate functions to match with driver



On 1/31/2024 4:34 PM, Krzysztof Kozlowski wrote:
> On 31/01/2024 09:24, Tengfei Fan wrote:
>>
>>
>> On 1/29/2024 7:24 PM, Krzysztof Kozlowski wrote:
>>> On 29/01/2024 10:25, Tengfei Fan wrote:
>>>> Consolidate functions to match with SM4450 pinctrl driver, because
>>>> consolidate functions are being used in SM4450 pinctrl driver.
>>>
>>> It's very difficult to see what changed from the diff, so please explain
>>> brieflyl changes here.
>>>
>>> What is that "consolidate functions" that you use in the driver?
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> please help to comfirm that the following description as commit message
>> whether it covers your concerns:
>>
>> Pin alternative functions are consolidated(like: atest_char, phase_flag,
>> qdss_gpio etc.) in SM4450 pinctrl driver while they are still split in
>> DeviceTree binding file. SM4450 pinctrl function is broken if current
>> binding doc is followed. Update SM4450 pinctrl DeviceTree binding doc to
>> align with driver.
>
> Please list the functions which are being removed and added. I usually
> do not expect such commit msg, but this is an exception: diff is tricky
> to parse.
>
> Best regards,
> Krzysztof
>

yes, I understand your concerns. I will list all the functions that need
to be updated.

--
Thx and BRs,
Tengfei Fan