Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181219221105.3004-1-ilina@codeaurora.org> <20181219221105.3004-7-ilina@codeaurora.org>
In-Reply-To: <20181219221105.3004-7-ilina@codeaurora.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 20 Dec 2018 10:14:25 -0800
Message-ID: <CAD=FV=V_+wbOhiVNHYAiZ0zD8tC6vCbZpWAZOhOESSnH12Dqig@mail.gmail.com>
Subject: Re: [PATCH 6/7] arm64: dts: msm: add PDC device bindings for sdm845
To:     Lina Iyer <ilina@codeaurora.org>
Cc:     Stephen Boyd <sboyd@kernel.org>, Evan Green <evgreen@chromium.org>,
        Marc Zyngier <marc.zyngier@arm.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Raju P L S S S N <rplsssn@codeaurora.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Wed, Dec 19, 2018 at 2:11 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
> Add PDC interrupt controller device bindings for SDM845.
>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v3:
>         - Fix PDC map, use GIC SPI port number for hwirq
> Changes in v2:
>         - Order by address
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)

nit: ${SUBJECT} makes it sounds like you're adding something into the
"Documentation/devicetree/bindings" folder, but you're not.  Also you
probably want the prefix "qcom", not "msm" since it ends up in the
"qcom" dir.  Also, subject should say that this is the interrupt
controller.  How about:

arm64: dts: qcom: add PDC interrupt controller for sdm845


> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index b72bdb0a31a5..8e15392a6f64 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1278,6 +1278,15 @@
>                         #reset-cells = <1>;
>                 };

nit: the above node looks like the tail end of pdc reset controller.
That has a unit address of b2e0000.  Your unit address is smaller than
the the pdc reset controller so you should be above it, not below it.


> +               pdc: interrupt-controller@b220000 {

nit: Maybe the label should be "pdc_intc" not just "pdc".  This is
just the node for the interrupt controller, not the whole pdc, right?


> +                       compatible = "qcom,sdm845-pdc";
> +                       reg = <0xb220000 0x30000>;

nit: apparently common practice for Quaclomm dts is to pad the address
in the "reg" field to all 8 digits.  So the above should be:

reg = <0x0b220000 0x30000>;

NOTE: it's important to _not_ pad the unit address in the node name
(so you got that right).  Only update the "reg".  For context:

https://lkml.kernel.org/r/CAD=FV=WrvRH6QpaQ67yw2MFz8RP59ozkSfQC4+OAM_8fAbGZuw@mail.gmail.com


-Doug