Received-SPF: pass (google.com: domain of linux-kernel+bounces-11078-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 25 Dec 2023 15:58:44 +0200
From: Petre Rodan <petre.rodan@subdimension.ro>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, Andreas Klinger <ak@it-klinger.de>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>
Subject: Re: [PATCH v2 02/10] dt-bindings: iio: pressure:
 honeywell,mprls0025pa.yaml add pressure-triplet
Message-ID: <ZYmKlJRrhonht4Zx@sunspire>
References: <20231224143500.10940-1-petre.rodan@subdimension.ro>
 <20231224143500.10940-3-petre.rodan@subdimension.ro>
 <49525adf-1540-4801-8cdf-be1c0fe640f6@linaro.org>
 <ZYmCOVvgcElnxRuH@sunspire>
 <503bc876-59d1-4fcb-b0b5-2dd88c62987c@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="8eqye/wz0y0EfH/9"
Content-Disposition: inline
In-Reply-To: <503bc876-59d1-4fcb-b0b5-2dd88c62987c@linaro.org>


--8eqye/wz0y0EfH/9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable


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:
> > [..]
> >=20
> > since the last 3 are tied together as we will see below.
> > is there any reason you want this order to change?
>=20
> 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 the=
se
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.
> >=20
> > when someone generates the dtbo files via
> >=20
> > cpp -nostdinc -I include -I ${LINUX_SRC}/include/ -I arch -undef -x ass=
embler-with-cpp ${file}.dts "${BUILD_DIR}/${file}.dts.preprocessed"
> > dtc -@ -I dts -O dtb -o "${BUILD_DIR}/${file}.dtbo" "${BUILD_DIR}/${fil=
e}.dts.preprocessed"
>=20
> And how this command matters? DT overlays are checked, so error is printe=
d.
>=20
> >=20
> > the schema is not checked in any way.
>=20
> 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.
>=20
> > 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.
>=20
>=20
>=20
> >=20
> >>> +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.
> >=20
> > speaking for intricacies, if the allOf is removed, then a binding conta=
ining
> >=20
> > honeywell,pmax-pascal =3D <840000>;
> > honeywell,pressure-triplet =3D "0015PA";
> >=20
> > 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.
>=20
> Really? Did you test it?

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

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

 # make DT_SCHEMA_FILES=3DDocumentation/devicetree/bindings/iio/pressure/ho=
neywell,mprls0025pa.yaml  DT_CHECKER_FLAGS=3D-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 allo=
w [[84000]]
         from schema $id: http://devicetree.org/schemas/iio/pressure/honeyw=
ell,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 =3D <1>;
        #size-cells =3D <0>;

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

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

best regards,
peter

--8eqye/wz0y0EfH/9
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE2Ap/wXYVGTXsPl+pzyaZmYROfzAFAmWJipQACgkQzyaZmYRO
fzB7xQ/+P2RrKV2a9LIob6nXbDg842zpZXDQI2ctFUjpI+yy1HKlwoDaxo0Siccq
zeikZ5CIlAC2tLTWUrHuH2iWvur2Aqp1HLDpAbXX/LI+itBSOGJ32GXf8yV2DlJ3
YQlUQGBXKnEvu6kncDrQzjdVJlKgm/JxPx0tMqv/NXiuypyxh6by+DvTBih3/pyF
LegHEdJbD8rk3cwrFY3TbU4I0xoIi/xnukh53RM8fcscsRoWxHxRwKvscI3XcLfU
1LLiMbxjeYxmtltlD22CL3h6kjAp3FB102XpvZcsCuNQ3YZ9Gxl/c8J2Uso4i79z
OSV73Q3PdXVidE+eX6e+3Sql5h8yjxn9efhGEAzO/2NE6d1WaXgzS04O7TelalKs
WF6g4FXlkcFjEWbj61kHQ5eHA4z1hg5kRZCGC91uLIDJs06kHJpxy9Eb9H4aZJC+
RuhsYdx4TqqUM7qQM+JrSm9edCR/6oGM3j/OxEZ7DaI+CjBUO85FAVtQZ6WSX62+
mNXdGvuVUumH5Xskg7XlJ1usFBb8XYBul8TzrilIWaxVOGo9ohVlNCyFD4rlY2+T
7bB/ZoBR8V0lvMEoqqnYK4wnQKnjEexXGMa/P5JFzb3NArJYGLF2XAUTuzLK2cyf
+yim3kYeIrjQi+kgUYAHzbFPgD2i2bpZoOM3mZdKYRS38U7MlVU=
=pZxl
-----END PGP SIGNATURE-----

--8eqye/wz0y0EfH/9--