| Message ID | 20240702-mainline_sdp500-v3-1-0902047b3eee@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Sensirion SDP500 | expand |
On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote: > From: Petar Stoykov <pd.pstoykov@gmail.com> > > Sensirion SDP500 is a digital differential pressure sensor. It provides > a digital I2C output. Add devicetree bindings requiring the compatible > string and I2C slave address (reg). > You did not test your code before sending. Please respond to existing feedback from v1 and v2, thus confirm that you understood it and you are or are not going to implement it. It does not look like you tested the bindings, at least after quick look. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. Best regards, Krzysztof
On Tue, 02 Jul 2024 16:59:08 +0200, Petar Stoykov wrote: > Sensirion SDP500 is a digital differential pressure sensor. It provides > a digital I2C output. Add devicetree bindings requiring the compatible > string and I2C slave address (reg). > > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com> > --- > .../bindings/iio/pressure/sensirion,sdp500.yaml | 41 ++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename $id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.example.dtb: pressure@40: 'vdd-supply' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240702-mainline_sdp500-v3-1-0902047b3eee@gmail.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Tue, 02 Jul 2024 16:59:08 +0200 Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@kernel.org> wrote: > From: Petar Stoykov <pd.pstoykov@gmail.com> > > Sensirion SDP500 is a digital differential pressure sensor. It provides > a digital I2C output. Add devicetree bindings requiring the compatible > string and I2C slave address (reg). > > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com> > --- > .../bindings/iio/pressure/sensirion,sdp500.yaml | 41 ++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml > new file mode 100644 > index 000000000000..6b3e54def367 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml > @@ -0,0 +1,41 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml# Naming mismatch (as already reporteD). > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: sdp500/sdp510 pressure sensor with I2C bus interface > + > +maintainers: > + - Petar Stoykov <petar.stoykov@prodrive-technologies.com> > + > +description: | > + Pressure sensor from Sensirion with I2C bus interface. > + There is no software difference between sdp500 and sdp510. > + > +properties: > + compatible: > + enum: > + - sensirion,sdp500 > + - sensirion,sdp510 Include the vdd-supply both as a property and in requried. vdd-supply: true; is enough for the property. > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + pressure@40 { > + compatible = "sensirion,sdp500"; > + reg = <0x40>; > + vdd-supply = <&foo>; > + }; > + }; >
On Tue, Jul 2, 2024 at 5:15 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote: > > From: Petar Stoykov <pd.pstoykov@gmail.com> > > > > Sensirion SDP500 is a digital differential pressure sensor. It provides > > a digital I2C output. Add devicetree bindings requiring the compatible > > string and I2C slave address (reg). > > > > You did not test your code before sending. I tested the driver for sdp500 on our system and it worked well. I must admit that I forgot to change the dts to sdp510 and retest. > > Please respond to existing feedback from v1 and v2, thus confirm that > you understood it and you are or are not going to implement it. > I tried to reply to all previous comments. Sorry if I missed something. > It does not look like you tested the bindings, at least after quick > look. Please run `make dt_binding_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > Maybe you need to update your dtschema and yamllint. I didn't know about that dt_binding_check. Then I spent a few hours yesterday fighting with dependencies to get it running. Thanks! > > Best regards, > Krzysztof >
On Tue, Jul 2, 2024 at 10:18 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Tue, 02 Jul 2024 16:59:08 +0200 > Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@kernel.org> wrote: > > > From: Petar Stoykov <pd.pstoykov@gmail.com> > > > > Sensirion SDP500 is a digital differential pressure sensor. It provides > > a digital I2C output. Add devicetree bindings requiring the compatible > > string and I2C slave address (reg). > > > > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com> > > --- > > .../bindings/iio/pressure/sensirion,sdp500.yaml | 41 ++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml > > new file mode 100644 > > index 000000000000..6b3e54def367 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml > > @@ -0,0 +1,41 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml# > > Naming mismatch (as already reporteD). Same mistake as in the MAINTAINERS file. Sigh. > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: sdp500/sdp510 pressure sensor with I2C bus interface > > + > > +maintainers: > > + - Petar Stoykov <petar.stoykov@prodrive-technologies.com> > > + > > +description: | > > + Pressure sensor from Sensirion with I2C bus interface. > > + There is no software difference between sdp500 and sdp510. > > + > > +properties: > > + compatible: > > + enum: > > + - sensirion,sdp500 > > + - sensirion,sdp510 > > Include the vdd-supply both as a property and in requried. > > vdd-supply: true; > > is enough for the property. Thank you for the example! But I don't think it's a required property. Our system is an example where it's not needed. The device is always powered on and Linux has no control over it's supply. One can argue that it's nice to write down what is that power supply but to me it's just noise, because I can write anything there and it would still work. So I prefer to not add anything instead of putting a value I can't trust. > > + > > + reg: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + pressure@40 { > > + compatible = "sensirion,sdp500"; > > + reg = <0x40>; > > + vdd-supply = <&foo>; > > + }; > > + }; > > >
On Tue, Jul 2, 2024 at 6:30 PM Rob Herring (Arm) <robh@kernel.org> wrote: > > > On Tue, 02 Jul 2024 16:59:08 +0200, Petar Stoykov wrote: > > Sensirion SDP500 is a digital differential pressure sensor. It provides > > a digital I2C output. Add devicetree bindings requiring the compatible > > string and I2C slave address (reg). > > > > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com> > > --- > > .../bindings/iio/pressure/sensirion,sdp500.yaml | 41 ++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename > $id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml > file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.example.dtb: pressure@40: 'vdd-supply' does not match any of the regexes: 'pinctrl-[0-9]+' > from schema $id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml# > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240702-mainline_sdp500-v3-1-0902047b3eee@gmail.com > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade I did not know about this tool before. And I did need to update a bunch of dependencies to make it work. Thanks! > > Please check and re-submit after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your schema. > The setting of DT_SCHEMA_FILES was a huge help in saving time!!
On 04/07/2024 10:18, Petar Stoykov wrote: > On Tue, Jul 2, 2024 at 5:15 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote: >>> From: Petar Stoykov <pd.pstoykov@gmail.com> >>> >>> Sensirion SDP500 is a digital differential pressure sensor. It provides >>> a digital I2C output. Add devicetree bindings requiring the compatible >>> string and I2C slave address (reg). >>> >> >> You did not test your code before sending. > > I tested the driver for sdp500 on our system and it worked well. > I must admit that I forgot to change the dts to sdp510 and retest. > >> >> Please respond to existing feedback from v1 and v2, thus confirm that >> you understood it and you are or are not going to implement it. >> > > I tried to reply to all previous comments. Sorry if I missed something. > >> It does not look like you tested the bindings, at least after quick >> look. Please run `make dt_binding_check` (see >> Documentation/devicetree/bindings/writing-schema.rst for instructions). >> Maybe you need to update your dtschema and yamllint. > > I didn't know about that dt_binding_check. Then I spent a few hours > yesterday fighting with dependencies to get it running. It's just one command: pipx install dtschema yamllint (or pip, depending on your system) Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml new file mode 100644 index 000000000000..6b3e54def367 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml @@ -0,0 +1,41 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: sdp500/sdp510 pressure sensor with I2C bus interface + +maintainers: + - Petar Stoykov <petar.stoykov@prodrive-technologies.com> + +description: | + Pressure sensor from Sensirion with I2C bus interface. + There is no software difference between sdp500 and sdp510. + +properties: + compatible: + enum: + - sensirion,sdp500 + - sensirion,sdp510 + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + pressure@40 { + compatible = "sensirion,sdp500"; + reg = <0x40>; + vdd-supply = <&foo>; + }; + };