| Message ID | 20240129092512.23602-3-quic_tengfan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | update SM4450 pinctrl document | expand |
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
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 <quic_tengfan@quicinc.com> > --- > .../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 >
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.
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 <quic_tengfan@quicinc.com> >> --- >> .../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 >>
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
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.
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
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 <quic_tengfan@quicinc.com> --- .../bindings/pinctrl/qcom,sm4450-tlmm.yaml | 51 +++++++------------ 1 file changed, 17 insertions(+), 34 deletions(-)