Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Vinod Koul <vkoul@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	linux-arm-msm@vger.kernel.org,
	Prasad Sodagudi <psodagud@codeaurora.org>,
	Andy Gross <agross@kernel.org>,
	David Brown <david.brown@linaro.org>,
	linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
	"Isaac J . Manjarres" <isaacm@codeaurora.org>
Subject: Re: [PATCH 2/2] pinctrl: qcom: Add SM8150 pinctrl driver
Date: Sun, 16 Jun 2019 21:17:36 -0700
Message-ID: <20190617041736.GD750@tuxbook-pro> (raw)
In-Reply-To: <20190614053032.24208-2-vkoul@kernel.org>

On Thu 13 Jun 22:30 PDT 2019, Vinod Koul wrote:

> From: Prasad Sodagudi <psodagud@codeaurora.org>
> 
> Add initial pinctrl driver to support pin configuration with
> pinctrl framework for SM8150
> 
> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>

I presume you did stuff to make it fit with my upstream tiling, mention
that here.

[..]
> diff --git a/drivers/pinctrl/qcom/pinctrl-sm8150.c b/drivers/pinctrl/qcom/pinctrl-sm8150.c
[..]
> +static const struct pinctrl_pin_desc sm8150_pins[] = {
[..]
> +	PINCTRL_PIN(178, "UFS_RESET"),

Please follow
https://lore.kernel.org/linux-arm-msm/20190606010249.3538-2-bjorn.andersson@linaro.org/
for ufs_reset.

> +};
[..]
> +enum sm8150_functions {
> +	msm_mux_phase_flag8,

Please sort these alphabetically and please squash all the phase_flag*
into msm_mux_phase_flag.

> +	msm_mux_phase_flag7,
> +	msm_mux_emac_pps,
> +	msm_mux_qup12,
> +	msm_mux_qup16,
> +	msm_mux_tsif1_clk,

Please squash all tsif1 into msm_mux_tsif1.

> +	msm_mux_qup8,
> +	msm_mux_qspi_cs,
> +	msm_mux_tgu_ch3,
> +	msm_mux_tsif1_en,
> +	msm_mux_qspi0,
> +	msm_mux_mdp_vsync0,
> +	msm_mux_mdp_vsync1,
> +	msm_mux_mdp_vsync2,
> +	msm_mux_mdp_vsync3,
> +	msm_mux_tgu_ch0,
> +	msm_mux_tsif1_data,
> +	msm_mux_qspi1,
> +	msm_mux_sdc4_cmd,

Squash sdc4_cmd, sdc4_clk and sdc4{0,1,2,3} into msm_mux_sdc4.

> +	msm_mux_phase_flag31,
> +	msm_mux_tgu_ch1,
> +	msm_mux_wlan1_adc1,
> +	msm_mux_tsif1_sync,
> +	msm_mux_qspi2,
> +	msm_mux_sdc43,
> +	msm_mux_vfr_1,
> +	msm_mux_phase_flag30,
> +	msm_mux_tgu_ch2,
> +	msm_mux_wlan1_adc0,
> +	msm_mux_tsif2_clk,

Please squash all tsif2

> +	msm_mux_qup11,
> +	msm_mux_qspi_clk,
> +	msm_mux_sdc4_clk,
> +	msm_mux_phase_flag27,
> +	msm_mux_wlan2_adc1,
> +	msm_mux_tsif2_en,
> +	msm_mux_qspi3,
> +	msm_mux_sdc42,
> +	msm_mux_phase_flag26,
> +	msm_mux_wlan2_adc0,
> +	msm_mux_tsif2_data,
> +	msm_mux_sdc41,
> +	msm_mux_phase_flag25,
> +	msm_mux_tsif2_sync,
> +	msm_mux_sdc40,
> +	msm_mux_tsif2_error,
> +	msm_mux_phase_flag11,
> +	msm_mux_sd_write,
> +	msm_mux_tsif1_error,
> +	msm_mux_qup7,
> +	msm_mux_ddr_bist,
> +	msm_mux_ddr_pxi3,
> +	msm_mux_atest_usb13,
> +	msm_mux_ddr_pxi1,
> +	msm_mux_pll_bypassnl,
> +	msm_mux_atest_usb12,
> +	msm_mux_pll_reset,
> +	msm_mux_pci_e1,
> +	msm_mux_uim2_data,
> +	msm_mux_uim2_clk,
> +	msm_mux_uim2_reset,
> +	msm_mux_uim2_present,

Please squash uim2.

> +	msm_mux_uim1_data,
> +	msm_mux_uim1_clk,
> +	msm_mux_uim1_reset,
> +	msm_mux_uim1_present,

Please squash uim1.

> +	msm_mux_uim_batt,
> +	msm_mux_usb2phy_ac,
> +	msm_mux_aoss_cti,
> +	msm_mux_qup1,
> +	msm_mux_rgmii_txc,

Please squash all the rmiii_*

> +	msm_mux_phase_flag20,
> +	msm_mux_rgmii_rxc,
> +	msm_mux_phase_flag19,
> +	msm_mux_adsp_ext,
> +	msm_mux_rgmii_rx,
> +	msm_mux_phase_flag18,
> +	msm_mux_rgmii_rxd0,
> +	msm_mux_phase_flag17,
> +	msm_mux_rgmii_rxd1,
> +	msm_mux_phase_flag16,
> +	msm_mux_qup5,
> +	msm_mux_rgmii_rxd2,
> +	msm_mux_phase_flag15,
> +	msm_mux_rgmii_rxd3,
> +	msm_mux_phase_flag14,
> +	msm_mux_rgmii_tx,
> +	msm_mux_phase_flag13,
> +	msm_mux_rgmii_txd0,
> +	msm_mux_phase_flag12,
> +	msm_mux_atest_usb22,
> +	msm_mux_emac_phy,
> +	msm_mux_hs3_mi2s,
> +	msm_mux_sec_mi2s,
> +	msm_mux_qup2,
> +	msm_mux_phase_flag9,
> +	msm_mux_phase_flag4,
> +	msm_mux_phase_flag21,
> +	msm_mux_jitter_bist,
> +	msm_mux_atest_usb21,
> +	msm_mux_pll_bist,
> +	msm_mux_atest_usb20,
> +	msm_mux_atest_char0,
> +	msm_mux_ter_mi2s,
> +	msm_mux_gcc_gp1,
> +	msm_mux_atest_char1,
> +	msm_mux_atest_char2,
> +	msm_mux_atest_char3,
> +	msm_mux_qua_mi2s,
> +	msm_mux_pri_mi2s,
> +	msm_mux_qup3,
> +	msm_mux_phase_flag29,
> +	msm_mux_ddr_pxi0,
> +	msm_mux_pri_mi2s_ws,
> +	msm_mux_phase_flag28,
> +	msm_mux_vsense_trigger,
> +	msm_mux_atest_usb1,
> +	msm_mux_atest_usb11,
> +	msm_mux_ddr_pxi2,
> +	msm_mux_dbg_out,
> +	msm_mux_atest_usb10,
> +	msm_mux_spkr_i2s,
> +	msm_mux_audio_ref,
> +	msm_mux_lpass_slimbus,
> +	msm_mux_tsense_pwm1,
> +	msm_mux_tsense_pwm2,
> +	msm_mux_btfm_slimbus,
> +	msm_mux_hs1_mi2s,
> +	msm_mux_cri_trng0,
> +	msm_mux_hs2_mi2s,
> +	msm_mux_cri_trng1,
> +	msm_mux_cri_trng,
> +	msm_mux_sp_cmu,
> +	msm_mux_prng_rosc,
> +	msm_mux_qup0,
> +	msm_mux_gpio,
> +	msm_mux_qup6,
> +	msm_mux_rgmii_txd1,
> +	msm_mux_rgmii_txd2,
> +	msm_mux_rgmii_txd3,
> +	msm_mux_qup_l6,
> +	msm_mux_rgmii_mdc,
> +	msm_mux_qup_l5,
> +	msm_mux_mdp_vsync,
> +	msm_mux_edp_lcd,
> +	msm_mux_qup10,
> +	msm_mux_m_voc,
> +	msm_mux_edp_hot,
> +	msm_mux_cam_mclk,
> +	msm_mux_qdss_gpio0,

Please squash all qdss_gpio into msm_mux_qdss (iirc qdss_cti is a
separate thing).

> +	msm_mux_qdss_gpio1,
> +	msm_mux_qdss_gpio2,
> +	msm_mux_qdss_gpio3,
> +	msm_mux_cci_i2c,
> +	msm_mux_qdss_gpio4,
> +	msm_mux_phase_flag3,
> +	msm_mux_qdss_gpio5,
> +	msm_mux_phase_flag2,
> +	msm_mux_qdss_gpio6,
> +	msm_mux_phase_flag1,
> +	msm_mux_qdss_gpio7,
> +	msm_mux_cci_timer0,
> +	msm_mux_gcc_gp2,
> +	msm_mux_qdss_gpio8,
> +	msm_mux_cci_timer1,
> +	msm_mux_gcc_gp3,
> +	msm_mux_qdss_gpio,
> +	msm_mux_cci_timer2,
> +	msm_mux_qup18,
> +	msm_mux_qdss_gpio9,
> +	msm_mux_cci_timer3,
> +	msm_mux_cci_async,
> +	msm_mux_qdss_gpio10,
> +	msm_mux_cci_timer4,
> +	msm_mux_qdss_gpio11,
> +	msm_mux_qdss_gpio12,
> +	msm_mux_qup15,
> +	msm_mux_qdss_gpio15,
> +	msm_mux_qdss_gpio13,
> +	msm_mux_qdss_gpio14,
> +	msm_mux_pci_e0,
> +	msm_mux_qup_l4,
> +	msm_mux_agera_pll,
> +	msm_mux_usb_phy,
> +	msm_mux_qup9,
> +	msm_mux_qup13,
> +	msm_mux_qdss_cti,
> +	msm_mux_qup14,
> +	msm_mux_qup4,
> +	msm_mux_qup17,
> +	msm_mux_qup19,
> +	msm_mux_phase_flag5,
> +	msm_mux_phase_flag0,
> +	msm_mux_phase_flag22,
> +	msm_mux_rgmii_mdio,
> +	msm_mux_phase_flag10,
> +	msm_mux_atest_char,
> +	msm_mux_nav_pps,
> +	msm_mux_atest_usb2,
> +	msm_mux_qlink_request,
> +	msm_mux_qlink_enable,
> +	msm_mux_wmss_reset,
> +	msm_mux_atest_usb23,
> +	msm_mux_phase_flag6,
> +	msm_mux_pa_indicator,
> +	msm_mux_phase_flag23,
> +	msm_mux_mss_lte,
> +	msm_mux_phase_flag24,
> +	msm_mux__,
> +};
[..]
> +static const struct msm_function sm8150_functions[] = {
> +	FUNCTION(phase_flag8),

Please sort this array as well.

> +	FUNCTION(phase_flag7),
[..]
> +};
> +
> +/*
> + * Every pin is maintained as a single group, and missing or non-existing pin
> + * would be maintained as dummy group to synchronize pin group index with
> + * pin descriptor registered with pinctrl core.
> + * Clients would not be able to request these dummy pin groups.
> + */
> +static const struct msm_pingroup sm8150_groups[] = {
[..]
> +	[58] = PINGROUP(58, SOUTH, qup17, qup19, qdss_cti, qdss_cti, _, _, _, _, _),

qdss_cti can't be both function 3 and 4 of a single pin.

[..]
> +static struct platform_driver sm8150_pinctrl_driver = {
> +	.driver = {
> +		.name = "sm8150-pinctrl",
> +		.owner = THIS_MODULE,

No .owner in platform_driver

> +		.of_match_table = sm8150_pinctrl_of_match,
> +	},
> +	.probe = sm8150_pinctrl_probe,
> +	.remove = msm_pinctrl_remove,
> +};

Regards,
Bjorn

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-14  5:30 [PATCH 1/2] dt-bindings: pinctrl: qcom: Add SM8150 pinctrl binding Vinod Koul
2019-06-14  5:30 ` [PATCH 2/2] pinctrl: qcom: Add SM8150 pinctrl driver Vinod Koul
2019-06-17  4:17   ` Bjorn Andersson [this message]
2019-06-17  6:26     ` Vinod Koul
2019-06-17  4:20 ` [PATCH 1/2] dt-bindings: pinctrl: qcom: Add SM8150 pinctrl binding Bjorn Andersson
2019-06-17  6:21   ` Vinod Koul
2019-06-17  6:37 ` Manivannan Sadhasivam
2019-06-17  9:35   ` Vinod Koul

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190617041736.GD750@tuxbook-pro \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@kernel.org \
    --cc=david.brown@linaro.org \
    --cc=isaacm@codeaurora.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=psodagud@codeaurora.org \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox