Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 4 Jul 2024 10:35:45 +0200
Petar Stoykov <pd.pstoykov@gmail.com> wrote:

> 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.
> 
I understand your nervousness, but put it in the binding doc anyway (not necessarily
in your dts though!)

There is a difference between what the binding requires (to work on
any operating system etc) and what the Linux driver stack requires.
The binding should specify any supply that is needed for the device to do anything
useful.

That doesn't mean that you need to provide it on a board if it's always
powered up.

The Linux driver should include regulator enabling for the supply. If it's not
provided in firmware because it's always one, then the dummy regulators that
will get used are fine.

devm_regulator_get_enable() is all that's needed




> > > +
> > > +  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>;
> > > +      };
> > > +    };
> > >  
> >