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

[Petre Rodan <petre.rodan@subdimension.ro>]

[-- Attachment #1: Type: text/plain, Size: 4378 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]