Re: [PATCH 10/18] pinctrl: qcom: sa8775p: add the pinctrl driver for the sa8775p platform

[Bartosz Golaszewski]

On Tue, Jan 10, 2023 at 5:26 PM Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
>
> On Mon, Jan 09, 2023 at 06:45:03PM +0100, Bartosz Golaszewski wrote:
> [..]
> > +enum sa8775p_functions {
> > +     msm_mux_gpio,
> > +     msm_mux_atest_char,
> > +     msm_mux_atest_char0,
> > +     msm_mux_atest_char1,
> > +     msm_mux_atest_char2,
> > +     msm_mux_atest_char3,
> > +     msm_mux_atest_usb2,
> > +     msm_mux_atest_usb20,
> > +     msm_mux_atest_usb21,
> > +     msm_mux_atest_usb22,
> > +     msm_mux_atest_usb23,
>
> Please squash these to a single msm_mux_atest.

How about staying consistent with the sc8280xp which is the closest
platform to sa8775p and do a group for atest_char, a separate group
for atest_usb2...

>
> > +     msm_mux_audio_ref,
> > +     msm_mux_cam_mclk,
> > +     msm_mux_cci_async,
> > +     msm_mux_cci_i2c,
> > +     msm_mux_cci_timer0,
> > +     msm_mux_cci_timer1,
> > +     msm_mux_cci_timer2,
> > +     msm_mux_cci_timer3,
> > +     msm_mux_cci_timer4,
> > +     msm_mux_cci_timer5,
> > +     msm_mux_cci_timer6,
> > +     msm_mux_cci_timer7,
> > +     msm_mux_cci_timer8,
> > +     msm_mux_cci_timer9,
> > +     msm_mux_cri_trng,
> > +     msm_mux_cri_trng0,
> > +     msm_mux_cri_trng1,
> > +     msm_mux_dbg_out,
> > +     msm_mux_ddr_bist,
> > +     msm_mux_ddr_pxi0,
> > +     msm_mux_ddr_pxi1,
> > +     msm_mux_ddr_pxi2,
> > +     msm_mux_ddr_pxi3,
> > +     msm_mux_ddr_pxi4,
> > +     msm_mux_ddr_pxi5,
> > +     msm_mux_edp0_hot,
> > +     msm_mux_edp0_lcd,
> > +     msm_mux_edp1_hot,
> > +     msm_mux_edp1_lcd,
> > +     msm_mux_edp2_hot,
> > +     msm_mux_edp2_lcd,
> > +     msm_mux_edp3_hot,
> > +     msm_mux_edp3_lcd,
> > +     msm_mux_emac0_mcg0,
> > +     msm_mux_emac0_mcg1,
> > +     msm_mux_emac0_mcg2,
> > +     msm_mux_emac0_mcg3,
> > +     msm_mux_emac0_mdc,
> > +     msm_mux_emac0_mdio,
> > +     msm_mux_emac0_ptp,
>
> msm_mux_emac0
>
> > +     msm_mux_emac1_mcg0,
> > +     msm_mux_emac1_mcg1,
> > +     msm_mux_emac1_mcg2,
> > +     msm_mux_emac1_mcg3,
> > +     msm_mux_emac1_mdc,
> > +     msm_mux_emac1_mdio,
> > +     msm_mux_emac1_ptp,
>
> msm_mux_emac1
>

...leave these two here as is...

> > +     msm_mux_gcc_gp1,
> > +     msm_mux_gcc_gp2,
> > +     msm_mux_gcc_gp3,
> > +     msm_mux_gcc_gp4,
> > +     msm_mux_gcc_gp5,
> > +     msm_mux_hs0_mi2s,
> > +     msm_mux_hs1_mi2s,
> > +     msm_mux_hs2_mi2s,
> > +     msm_mux_ibi_i3c,
> > +     msm_mux_jitter_bist,
> > +     msm_mux_mdp0_vsync0,
> > +     msm_mux_mdp0_vsync1,
> > +     msm_mux_mdp0_vsync2,
> > +     msm_mux_mdp0_vsync3,
> > +     msm_mux_mdp0_vsync4,
> > +     msm_mux_mdp0_vsync5,
> > +     msm_mux_mdp0_vsync6,
> > +     msm_mux_mdp0_vsync7,
> > +     msm_mux_mdp0_vsync8,
> > +     msm_mux_mdp1_vsync0,
> > +     msm_mux_mdp1_vsync1,
> > +     msm_mux_mdp1_vsync2,
> > +     msm_mux_mdp1_vsync3,
> > +     msm_mux_mdp1_vsync4,
> > +     msm_mux_mdp1_vsync5,
> > +     msm_mux_mdp1_vsync6,
> > +     msm_mux_mdp1_vsync7,
> > +     msm_mux_mdp1_vsync8,
> > +     msm_mux_mdp_vsync,
> > +     msm_mux_mi2s1_data0,
> > +     msm_mux_mi2s1_data1,
> > +     msm_mux_mi2s1_sck,
> > +     msm_mux_mi2s1_ws,
> > +     msm_mux_mi2s2_data0,
> > +     msm_mux_mi2s2_data1,
> > +     msm_mux_mi2s2_sck,
> > +     msm_mux_mi2s2_ws,
> > +     msm_mux_mi2s_mclk0,
> > +     msm_mux_mi2s_mclk1,
> > +     msm_mux_pcie0_clkreq,
> > +     msm_mux_pcie1_clkreq,
> > +     msm_mux_phase_flag0,
> > +     msm_mux_phase_flag1,
> > +     msm_mux_phase_flag10,
> > +     msm_mux_phase_flag11,
> > +     msm_mux_phase_flag12,
> > +     msm_mux_phase_flag13,
> > +     msm_mux_phase_flag14,
> > +     msm_mux_phase_flag15,
> > +     msm_mux_phase_flag16,
> > +     msm_mux_phase_flag17,
> > +     msm_mux_phase_flag18,
> > +     msm_mux_phase_flag19,
> > +     msm_mux_phase_flag2,
> > +     msm_mux_phase_flag20,
> > +     msm_mux_phase_flag21,
> > +     msm_mux_phase_flag22,
> > +     msm_mux_phase_flag23,
> > +     msm_mux_phase_flag24,
> > +     msm_mux_phase_flag25,
> > +     msm_mux_phase_flag26,
> > +     msm_mux_phase_flag27,
> > +     msm_mux_phase_flag28,
> > +     msm_mux_phase_flag29,
> > +     msm_mux_phase_flag3,
> > +     msm_mux_phase_flag30,
> > +     msm_mux_phase_flag31,
> > +     msm_mux_phase_flag4,
> > +     msm_mux_phase_flag5,
> > +     msm_mux_phase_flag6,
> > +     msm_mux_phase_flag7,
> > +     msm_mux_phase_flag8,
> > +     msm_mux_phase_flag9,
>
> msm_mux_phase_flag
>

... change this one as you suggest...

> > +     msm_mux_pll_bist,
> > +     msm_mux_pll_clk,
> > +     msm_mux_prng_rosc0,
> > +     msm_mux_prng_rosc1,
> > +     msm_mux_prng_rosc2,
> > +     msm_mux_prng_rosc3,
> > +     msm_mux_qdss_cti,
> > +     msm_mux_qdss_gpio,
> > +     msm_mux_qdss_gpio0,
> > +     msm_mux_qdss_gpio1,
> > +     msm_mux_qdss_gpio10,
> > +     msm_mux_qdss_gpio11,
> > +     msm_mux_qdss_gpio12,
> > +     msm_mux_qdss_gpio13,
> > +     msm_mux_qdss_gpio14,
> > +     msm_mux_qdss_gpio15,
> > +     msm_mux_qdss_gpio2,
> > +     msm_mux_qdss_gpio3,
> > +     msm_mux_qdss_gpio4,
> > +     msm_mux_qdss_gpio5,
> > +     msm_mux_qdss_gpio6,
> > +     msm_mux_qdss_gpio7,
> > +     msm_mux_qdss_gpio8,
> > +     msm_mux_qdss_gpio9,
>
> msm_mux_qdss

... and have these as qdss_cti and qdss_gpio.

>
> > +     msm_mux_qup0_se0,
> > +     msm_mux_qup0_se1,
> > +     msm_mux_qup0_se2,
> > +     msm_mux_qup0_se3,
> > +     msm_mux_qup0_se4,
> > +     msm_mux_qup0_se5,
> > +     msm_mux_qup1_se0,
> > +     msm_mux_qup1_se1,
> > +     msm_mux_qup1_se2,
> > +     msm_mux_qup1_se3,
> > +     msm_mux_qup1_se4,
> > +     msm_mux_qup1_se5,
> > +     msm_mux_qup1_se6,
> > +     msm_mux_qup2_se0,
> > +     msm_mux_qup2_se1,
> > +     msm_mux_qup2_se2,
> > +     msm_mux_qup2_se3,
> > +     msm_mux_qup2_se4,
> > +     msm_mux_qup2_se5,
> > +     msm_mux_qup2_se6,
> > +     msm_mux_qup3_se0,
> > +     msm_mux_sail_top,
> > +     msm_mux_sailss_emac0,
> > +     msm_mux_sailss_ospi,
> > +     msm_mux_sgmii_phy,
> > +     msm_mux_tb_trig,
> > +     msm_mux_tgu_ch0,
> > +     msm_mux_tgu_ch1,
> > +     msm_mux_tgu_ch2,
> > +     msm_mux_tgu_ch3,
> > +     msm_mux_tgu_ch4,
> > +     msm_mux_tgu_ch5,
> > +     msm_mux_tsense_pwm1,
> > +     msm_mux_tsense_pwm2,
> > +     msm_mux_tsense_pwm3,
> > +     msm_mux_tsense_pwm4,
> > +     msm_mux_usb2phy_ac,
> > +     msm_mux_vsense_trigger,
> > +     msm_mux__,
> > +};
> > +
> [..]
> > +static const struct msm_pingroup sa8775p_groups[] = {
> > +     [0] = PINGROUP(0, _, _, _, _, _, _, _, _, _),
> > +     [1] = PINGROUP(1, pcie0_clkreq, _, _, _, _, _, _, _, _),
> > +     [2] = PINGROUP(2, _, _, _, _, _, _, _, _, _),
> > +     [3] = PINGROUP(3, pcie1_clkreq, _, _, _, _, _, _, _, _),
> > +     [4] = PINGROUP(4, _, _, _, _, _, _, _, _, _),
> > +     [5] = PINGROUP(5, _, _, _, _, _, _, _, _, _),
> > +     [6] = PINGROUP(6, emac0_ptp, emac0_ptp, emac1_ptp, emac1_ptp, _, _, _, _, _),
>
> How is it possible to select the first or the second one of these
> functions when they are named the same?
>

I think Prasad and Yadu (the original authors of the driver) followed
the example from sc8280xp:

c0e4c71a9e7ce (Bjorn Andersson 2022-03-08 14:11:32 -0800 1804) [156] =
PINGROUP(156, qup6, emac0_ptp, emac0_ptp, _, _, _, _),

Do you remember what your original intention was? I also see that the
GPIOs repeat in the groups definitions:

 980 static const char * const emac0_ptp_groups[] = {
 981         "gpio130", "gpio130", "gpio131", "gpio131", "gpio156", "gpio156",
 982         "gpio157", "gpio157", "gpio158", "gpio158", "gpio159", "gpio159",
 983 };

[...]

Bart