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: <a0631800-f3d5-ff13-b316-9bc027275a82@linaro.org>
Date:   Wed, 5 Jul 2023 11:34:35 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH V7 1/2] dt-bindings: firmware: bootstats: Add the dtschema
Content-Language: en-US
To:     Souradeep Chowdhury <quic_schowdhu@quicinc.com>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Rob Herring <robh+dt@kernel.org>, Arnd Bergmann <arnd@arndb.de>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        Sibi Sankar <quic_sibis@quicinc.com>,
        Rajendra Nayak <quic_rjendra@quicinc.com>
References: <cover.1688533340.git.quic_schowdhu@quicinc.com>
 <b3105990e021a71039f621e6c4e70ab05fb348fa.1688533340.git.quic_schowdhu@quicinc.com>
 <d339d413-5242-0d5a-96f6-c2f670e5e5dc@linaro.org>
 <968fb5d3-6cd8-7850-47e7-682e26f9ee5f@quicinc.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <968fb5d3-6cd8-7850-47e7-682e26f9ee5f@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 05/07/2023 10:33, Souradeep Chowdhury wrote:
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>> +
>>> +  abl-time:
>>> +    description: The property to store the duration of abl in ms.
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>
>> I have no clue what this entire binding is about. Nothing can bind to
>> it, no usage explained. Properties are not used to "store the duration".
>> This does not look like suitable for DT, drop entire binding.
> 
> This binding was created as per the suggestion on version 6 of the patch 
> by Arnd. The idea was that these 2 devicetree properties will be used to 
> populate the bootstat values from the bootloader and exposed to the user 
> via /sys/firmware/devicetree/ directly.
> 
> Details in the link below:-
> 
> https://lore.kernel.org/lkml/7d397e67-5d56-4975-98af-1ac9746c07f4@app.fastmail.com/T/#mbdc9ad95fcbb5ad7b56c6996a3933899b42d982c
> 
> Can you suggest any alternative way to represent this as a binding?

Then you should clearly state in the binding how this is going to be
used and who is going to populate it. Not only in the binding but also
in commit msg which currently has 0 rationale and answers to "why". Your
commit msg explained only "what", which is usually obvious and much less
important. Your commit should stand on its own and should clearly
explain why we need this feature at all, what problem it solves.

And before you claim that there is some discussion under link or some
cover letter - these do not matter. Commit and bindings matter.

What's more, I don't think that Arnd's advice is correct here - DT is
suppose to describe hardware or firmware. These properties are coming
from firmware but they are not describing any firmware or hardware
characteristics. Instead they are debugging of current boot status.

I will leave the decision on that for Rob, however anyway binding is
very vague and incorrect, so I would expect he will come with the same
concerns regardless whether it is suitable to DT or is not.


Best regards,
Krzysztof