Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0ac86ecb-bff4-6f13-1109-0410c8050cab@linaro.org>
Date:   Tue, 30 Aug 2022 01:45:20 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.2
Subject: Re: [PATCH v3 2/2] spmi: pmic-arb: Add support for PMIC v7
Content-Language: en-GB
To:     Vinod Koul <vkoul@kernel.org>, Stephen Boyd <sboyd@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        David Collins <quic_collinsd@quicinc.com>
Cc:     linux-arm-msm@vger.kernel.org,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        David Collins <collinsd@codeaurora.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        David Dai <daidavid1@codeaurora.org>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220201134108.2677578-1-vkoul@kernel.org>
 <20220201134108.2677578-3-vkoul@kernel.org>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <20220201134108.2677578-3-vkoul@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 01/02/2022 16:41, Vinod Koul wrote:
> From: David Collins <collinsd@codeaurora.org>
> 
> PMIC v7 has different offset values and seqeunces, so add support for
> this new version of PMIC
> 
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>   drivers/spmi/spmi-pmic-arb.c | 233 ++++++++++++++++++++++++++++++++---
>   1 file changed, 214 insertions(+), 19 deletions(-)


As I was asking Stephen about the fate of this patch series I could not 
stop my self from noticing that one of his comments ([1]) from v1 was 
ignored.
Let me quote it here:

 > The driver is already pretty hard to read because it combines so many
 > generations of spmi arbiter hardware from qcom into one file. It would
 > probably be better to start over and simplify the new version of the
 > driver, possibly sharing code between the two files if possible, but
 > otherwise dropping lots of cruft along the way and simplifying review
 > burden.

After taking a glance, I thought maybe we should really follow this 
approach. And it also allows us to start with the new bindings:

spmi@c400000 {
    compatible = "qcom,spmi-pmic-arb-v7";

    reg = <..... both arb registers as following....>;
    reg-names = "core", "chnls", "observer", "cnfg0", "intr0", "cnfg1", 
"intr1";

    interrupts = <&pdc 1 HIGH>, <&pdc 3 HIGH>;
    interrupt-names = "arb0", "arb1"; /* are the names necessary at all? */

    #address-cells = <1>;
    #size-cells = <0>;

    spmi_bus: bus@0 {
       reg = <0>;
       #address-cells = <2>;
       #size-cells = <0>;
       #interrupt-cells = <4>;
       interrupt-controller;

       pmic@.... {
           // etc.
       };

    };

    spmi1_bus: bus@1 {
       reg = <1>;
       #address-cells = <2>;
       #size-cells = <0>;
       #interrupt-cells = <4>;
       interrupt-controller;
    };
};

Note, this drops the qcom,ee (which is always 0 for all devices I see in 
mainline)) and qcom,channel (which if I understood correctly is used 
only for pmic-arb-v1, ugh). It uses common reg = <N> property instead of
cooked qcom,bus-id. And last, but not least, it save us from huuge 
comments in the source code telling why devm_platform_ioremap_resource 
can not be used.

[1] 
https://lore.kernel.org/linux-arm-msm/20211210020148.B2EA6C004DD@smtp.kernel.org/


-- 
With best wishes
Dmitry