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

[Varadarajan Narayanan <varada@xxxxxxxxxxxxxx>]

On Mon, Sep 28, 2020 at 01:43:22PM -0500, Bjorn Andersson wrote:
> 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")

Ok.

> 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),
> > +};

Ok.

> > +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?

Will check this.

> > +		 _, _, _),
> > +	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,
> > +		 _, _, _),

Ok.

> Regards,
> Bjorn

Will post updated patches soon.

Thanks
Varada
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation