linux-kernel - Re: [PATCH v2 02/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add pressure-triplet

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZYmKlJRrhonht4Zx@sunspire>
Date: Mon, 25 Dec 2023 15:58:44 +0200
From: Petre Rodan <petre.rodan@...dimension.ro>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, Andreas Klinger <ak@...klinger.de>,
	Jonathan Cameron <jic23@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH v2 02/10] dt-bindings: iio: pressure:
 honeywell,mprls0025pa.yaml add pressure-triplet


hello,

On Mon, Dec 25, 2023 at 02:34:04PM +0100, Krzysztof Kozlowski wrote:
> On 25/12/2023 14:23, Petre Rodan wrote:
> > honeywell,transfer-function:
> > [..]
> > honeywell,pressure-triplet:
> > [..]
> > honeywell,pmin-pascal:
> > [..]
> > honeywell,pmax-pascal:
> > [..]
> > 
> > since the last 3 are tied together as we will see below.
> > is there any reason you want this order to change?
> 
> I just don't get why moving the code instead of adding new property next
> to them.

as I also said in the comments and in my last reply I want the user to not feel
in any way obliged to fill in pmin-pascal, pmax-pascal.
and since a user reads this file from top to bottom, the order in which these
properties are shown to him is important, and it is the one above.

> The order is often alphabetical.

can we please make an exception?

> >>> +  honeywell,pmin-pascal:
> >>> +    description:
> >>> +      Minimum pressure value the sensor can measure in pascal.
> >>> +      To be specified only if honeywell,pressure-triplet is not set.
> >>
> >> The last sentence is redundant - schema should enforce that.
> > 
> > when someone generates the dtbo files via
> > 
> > cpp -nostdinc -I include -I ${LINUX_SRC}/include/ -I arch -undef -x assembler-with-cpp ${file}.dts "${BUILD_DIR}/${file}.dts.preprocessed"
> > dtc -@ -I dts -O dtb -o "${BUILD_DIR}/${file}.dtbo" "${BUILD_DIR}/${file}.dts.preprocessed"
> 
> And how this command matters? DT overlays are checked, so error is printed.
> 
> > 
> > the schema is not checked in any way.
> 
> When I run `make` the schema is also not checked, so is it an argument
> to add anything to the binding? No. Drop redundant text.
> 
> > so unless people can be bothered to understand the yaml intricacies in the
> > bindings file, I feel they need to see that redundant information there, see below.
> 
> 
> 
> > 
> >>> +oneOf:
> >>> +  - required:
> >>> +      - honeywell,pmin-pascal
> >>> +      - honeywell,pmax-pascal
> >>> +  - required:
> >>> +      - honeywell,pressure-triplet
> >>> +
> >>> +allOf:
> >>> +  - if:
> >>> +      required:
> >>> +        - honeywell,pressure-triplet
> >>> +    then:
> >>> +      properties:
> >>> +        honeywell,pmin-pascal: false
> >>> +        honeywell,pmax-pascal: false
> >>
> >> This allOf is not needed.
> > 
> > speaking for intricacies, if the allOf is removed, then a binding containing
> > 
> > honeywell,pmax-pascal = <840000>;
> > honeywell,pressure-triplet = "0015PA";
> > 
> > would be considered to be correct by the schema, but that would be the incorrect
> > result. so afaict allOf needs to stay, and so does the redundant text.
> 
> Really? Did you test it?

for more hours than I would have liked. the allOf was provided with kindness by
Conor in my first revision.

testing it:
 1. invalid yaml with both honeywell,pmax-pascal and honeywell,pressure-triplet
 defined passes the schema check if the allOf is removed:

 # make DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml  DT_CHECKER_FLAGS=-m dt_binding_check
 # echo $?
0

 2. the same invalid yaml but with the allOf not removed issues this output:

 [..]/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.example.dtb: pressure@18: honeywell,pmax-pascal: False schema does not allow [[84000]]
         from schema $id: http://devicetree.org/schemas/iio/pressure/honeywell,mprls0025pa.yaml#

which is the expected behaviour. so AFAICT the allOf block is required, as well
as the redundant text for the humans that read the human-readable parts of the
bindings file.

invalid yaml example used above:

    #include <dt-bindings/gpio/gpio.h>
    #include <dt-bindings/interrupt-controller/irq.h>
    i2c {
        #address-cells = <1>;
        #size-cells = <0>;

        pressure@18 {
            compatible = "honeywell,mprls0025pa";
            reg = <0x18>;
            reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
            interrupt-parent = <&gpio3>;
            interrupts = <21 IRQ_TYPE_EDGE_RISING>;

            honeywell,pmax-pascal = <84000>;
            honeywell,pressure-triplet = "0025PA";
            honeywell,transfer-function = <1>;
            vdd-supply = <&vcc_3v3>;
        };
    };

best regards,
peter

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)