Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 25 Dec 2023 14:34:04 +0100 | Subject | Re: [PATCH v2 02/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add pressure-triplet | From | Krzysztof Kozlowski <> |
| |
On 25/12/2023 14:23, Petre Rodan wrote: > > hello Krzysztof, > > On Mon, Dec 25, 2023 at 01:57:39PM +0100, Krzysztof Kozlowski wrote: >> On 24/12/2023 15:34, Petre Rodan wrote: >>> @@ -54,14 +57,6 @@ properties: >>> If not present the device is not reset during the probe. >>> maxItems: 1 >>> >>> - honeywell,pmin-pascal: >>> - description: >>> - Minimum pressure value the sensor can measure in pascal. >>> - >>> - honeywell,pmax-pascal: >>> - description: >>> - Maximum pressure value the sensor can measure in pascal. >>> - >>> honeywell,transfer-function: >>> description: | >>> Transfer function which defines the range of valid values delivered by the >>> @@ -72,17 +67,52 @@ properties: >>> enum: [1, 2, 3] >>> $ref: /schemas/types.yaml#/definitions/uint32 >>> >>> + honeywell,pressure-triplet: >> >> Why not putting it just before existing properties? > > I'd like to have pmin-pascal, pmax-pascal as the last two honeywell specific > properties, since they are not to be used unless someone has custom silicon. > so we will still have a block moved just like above. > the most logic order is the one I proposed above: > > 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.
The order is often alphabetical.
> >>> + 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?
Best regards, Krzysztof
| ![\](/images/icornerr.gif) |