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:
signature.asc
Description: PGP signature
