Re: [PATCH 5/7] pinctrl: qcom: Add IPQ5018 pinctrl driver

[Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>]

On Mon 28 Sep 00:15 CDT 2020, Varadarajan Narayanan wrote:
> diff --git a/drivers/pinctrl/qcom/pinctrl-ipq5018.c b/drivers/pinctrl/qcom/pinctrl-ipq5018.c
[..]
> +static const struct msm_function ipq5018_functions[] = {
[..]
> +	FUNCTION(qspi_clk),
> +	FUNCTION(qspi_cs),
> +	FUNCTION(qspi0),
> +	FUNCTION(qspi1),
> +	FUNCTION(qspi2),
> +	FUNCTION(qspi3),

Instead of having one function name per pin it typically leads to
cleaner DT if you group these under the same name (i.e. "qspi")

Same seems to apply to sdc, wci, xfem at least.

> +	FUNCTION(reset_out),
> +	FUNCTION(sdc1_clk),
> +	FUNCTION(sdc1_cmd),
> +	FUNCTION(sdc10),
> +	FUNCTION(sdc11),
> +	FUNCTION(sdc12),
> +	FUNCTION(sdc13),
> +	FUNCTION(wci0),
> +	FUNCTION(wci1),
> +	FUNCTION(wci2),
> +	FUNCTION(wci3),
> +	FUNCTION(wci4),
> +	FUNCTION(wci5),
> +	FUNCTION(wci6),
> +	FUNCTION(wci7),
> +	FUNCTION(wsa_swrm),
> +	FUNCTION(wsi_clk3),
> +	FUNCTION(wsi_data3),
> +	FUNCTION(wsis_reset),
> +	FUNCTION(xfem0),
> +	FUNCTION(xfem1),
> +	FUNCTION(xfem2),
> +	FUNCTION(xfem3),
> +	FUNCTION(xfem4),
> +	FUNCTION(xfem5),
> +	FUNCTION(xfem6),
> +	FUNCTION(xfem7),
> +};
> +static const struct msm_pingroup ipq5018_groups[] = {
> +	PINGROUP(0, atest_char0, _, qdss_cti_trig_out_a0, wci0, wci0, xfem0,

What's up with wci0 being both function 4 and 5?

> +		 _, _, _),
> +	PINGROUP(1, atest_char1, _, qdss_cti_trig_in_a0, wci1, wci1, xfem1,
> +		 _, _, _),

Please don't like break these, better blow the line length limit in
favor or readability.

> +	PINGROUP(2, atest_char2, _, qdss_cti_trig_out_a1, wci2, wci2, xfem2,
> +		 _, _, _),
> +	PINGROUP(3, atest_char3, _, qdss_cti_trig_in_a1, wci3, wci3, xfem3,
> +		 _, _, _),

Regards,
Bjorn