Re: [PATCH 2/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add sleep-mode

[Rob Herring]

On Wed, Jan 17, 2024 at 04:50:01PM +0000, Jonathan Cameron wrote:
> On Tue, 16 Jan 2024 11:30:23 -0600
> Rob Herring <robh@xxxxxxxxxx> wrote:
> 
> > On Fri, Jan 12, 2024 at 09:30:30AM +0200, Petre Rodan wrote:
> > > 
> > > Hello Krzysztof,
> > > 
> > > On Wed, Jan 10, 2024 at 09:48:34PM +0100, Krzysztof Kozlowski wrote:  
> > > > On 10/01/2024 18:22, Petre Rodan wrote:  
> > > > > Add sleep-mode property present in some custom chips.
> > > > > 
> > > > > This flag activates a special wakeup sequence prior to conversion.
> > > > > 
> > > > > Signed-off-by: Petre Rodan <petre.rodan@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > >  .../bindings/iio/pressure/honeywell,hsc030pa.yaml      | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > > index 89977b9f01cf..350da1d6991b 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > > @@ -86,6 +86,15 @@ properties:
> > > > >        Maximum pressure value the sensor can measure in pascal.
> > > > >        To be specified only if honeywell,pressure-triplet is set to "NA".
> > > > > 
> > > > > +  honeywell,sleep-mode:  
> > > > 
> > > > "Sleep mode" naming suggests there are choices, like mode foo and mode
> > > > bar. Probably you want something like "sleep-between-measurements" or
> > > > something matching how does it work.  
> > > 
> > > "sleep mode" is the terminology used by Honeywell and it defines a chip capability.
> > > it is present in the HSC/SSC and ABP series of ICs.
> > > 
> > > other such options (capabilities) include temperature output in the ABP series.
> > > 
> > > the action the driver needs to perform if this option is present is to provide a
> > > wake-up sequence before reading out the conversions.
> > > 
> > > now regarding a rename of this property, I would vote to leave it as is - for the
> > > users to have a 1:1 equivalence of terms between the driver and the datasheet.
> > > 
> > > I say that because for instance in circuit design when a part symbol and
> > > footprint is drawn based on a datasheet it is recommended to keep the same pin
> > > notations and the same block diagram as in the datasheet, precisely for this 1:1
> > > equivalence, so there is no uncertainty for the end-user.  
> > 
> > At least add a '-en' suffix so it is clear this property enables the 
> > mode. We have both flavors (enables and disables). 
> > 
> > Low power modes between samples is pretty common on these devices. We 
> > should consider if this should be a common property. Jonathan?
> 
> Normally it's a controllable things so we make it dependent on userspace
> interaction (runtime-pm or whether buffered capture is enabled).
> Policy thing so not appropriate in DT.
> 
> Here it's different because it's a particular device variant that must work in this
> fashion. Other device variants don't support it at all.
> 
> If it weren't for the obscene number of variants this would normally be
> derived from the compatible rather than being in DT at all.
> 
> So it's odd and I don't think appropriate for a common property.

All good details missing from the description and commit msg. Given 
that, I retract my suggestion to use '-en' as it not a case of enabling 
the feature. Probably worth just keeping the name as-is than discussing 
further.

Rob