Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: varada=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 2D953C433CA
Date:   Tue, 29 Sep 2020 13:34:26 +0530
From:   Varadarajan Narayanan <varada@codeaurora.org>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     agross@kernel.org, robh+dt@kernel.org, mturquette@baylibre.com,
        sboyd@kernel.org, linus.walleij@linaro.org,
        catalin.marinas@arm.com, will@kernel.org, p.zabel@pengutronix.de,
        nsekar@codeaurora.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, sricharan@codeaurora.org
Subject: Re: [PATCH 5/7] pinctrl: qcom: Add IPQ5018 pinctrl driver
Message-ID: <20200929080425.GA21805@codeaurora.org>
References: <1601270140-4306-1-git-send-email-varada@codeaurora.org>
 <1601270140-4306-6-git-send-email-varada@codeaurora.org>
 <20200928184322.GB71055@builder.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200928184322.GB71055@builder.lan>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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