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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Sep 2020 13:34:26 +0530
From:   Varadarajan Narayanan <varada@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     agross@...nel.org, robh+dt@...nel.org, mturquette@...libre.com,
        sboyd@...nel.org, linus.walleij@...aro.org,
        catalin.marinas@....com, will@...nel.org, p.zabel@...gutronix.de,
        nsekar@...eaurora.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, sricharan@...eaurora.org
Subject: Re: [PATCH 5/7] pinctrl: qcom: Add IPQ5018 pinctrl driver

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