linux-kernel - Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

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: Thu, 14 Mar 2024 09:30:52 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Sumit Garg <sumit.garg@...aro.org>
Cc: linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
 andersson@...nel.org, konrad.dybcio@...aro.org, robh+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, stephan@...hold.net,
 caleb.connolly@...aro.org, neil.armstrong@...aro.org,
 laetitia.mariottini@...com, pascal.eberhard@...com, abdou.saker@...com,
 jimmy.lalande@...com, benjamin.missey@....se.com,
 daniel.thompson@...aro.org, linux-kernel@...r.kernel.org,
 Jagdish Gediya <jagdish.gediya@...aro.org>
Subject: Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC
 board DTS

On 14/03/2024 09:19, Sumit Garg wrote:
>>> +             compatible = "smsc,usb3503";
>>> +             reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
>>> +             initial-mode = <1>;
>>> +     };
>>> +
>>> +     usb_id: usb-id {
>>> +             compatible = "linux,extcon-usb-gpio";
>>> +             id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&usb_id_default>;
>>> +     };
>>> +
>>> +     hdmi-out {
>>> +             compatible = "hdmi-connector";
>>> +             type = "a";
>>> +
>>> +             port {
>>> +                     hdmi_con: endpoint {
>>> +                             remote-endpoint = <&adv7533_out>;
>>> +                     };
>>> +             };
>>> +     };
>>> +
>>> +     gpio-keys {
>>> +             compatible = "gpio-keys";
>>> +             autorepeat;
>>> +
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&msm_key_volp_n_default>;
>>> +
>>> +             button {
>>> +                     label = "Volume Up";
>>> +                     linux,code = <KEY_VOLUMEUP>;
>>> +                     gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;
>>> +             };
>>> +     };
>>> +
>>> +     leds {
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&pm8916_mpps_leds>;
>>
>> First property is always compatible. Please apply DTS coding style rules.
> 
> Ack.
> 
>>
>>> +
>>> +             compatible = "gpio-leds";
>>> +             #address-cells = <1>;
>>> +             #size-cells = <0>;
>>
>> That's not a bus.
>>
>> It does not look like you tested the DTS against bindings. Please run
>> `make dtbs_check W=1` (see
>> Documentation/devicetree/bindings/writing-schema.rst or
>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>> for instructions).
> 
> I assumed earlier that W=1 is sufficient for DT schema checks but it

W=1 as in make? No, it is not. It's flag changing the build process.
dtbs_check is separate target.

> looks like those are two different entities. However, I added these
> address and size cells properties only to get rid of warnings reported
> by W=1, see below:
> 
> $ make qcom/apq8016-schneider-hmibsc.dtb W=1
>   DTC     arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb
> arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:96.9-103.5:
> Warning (unit_address_vs_reg): /leds/led@5: node has a unit name, but
> no reg or ranges property
> arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:105.9-112.5:
> Warning (unit_address_vs_reg): /leds/led@6: node has a unit name, but
> no reg or ranges property

Wait, so you saw the warnings and ignored them? These are legitimate
warnings, although they don't give you full answer.

> <snip>
> 
> So it looks like W=1 is reporting false warnings and we should rather

Warnings were true.

> rely on dtbs_check only.

It's really independent. There is only one case where W=1 produces
warnings you could ignore (ports/port in graphs). At least I am not
aware of anything else.

Although Qualcomm does not use clean-check-maintainer-profile, but
already some archs do (RISC-V, Samsung). For these YOU MUST RUN
DTBS_CHECK and fix ALL new warnings. But even for Qualcomm, you are
expected to run dtbs_check. And why would you not run it? You can
automate checks and save reviewers time with automatic tools, but you
decide to skip it? Srsly, that's huge waste of reviewers time!

..

>>> +
>>> +&blsp_i2c4_default {
>>
>> None of your overrides look like have proper alphabetical order. Please
>> use alphabetical order.
>>
> 
> Although these are already following the same order as
> apq8016-sbc.dts, would you like the two DTS files based on the same
> SoC to follow different orders?

I don't know about Konrad and Bjorn, but to me it does not matter that
some existing board has obvious style issues. What matters to me, that
new code does not have these obvious style issues.

You can wait for Konrad's point of view on that, if you want to be sure.

Best regards,
Krzysztof