Received-SPF: pass (google.com: domain of linux-kernel+bounces-29270-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 17 Jan 2024 11:27:08 -0600
From: Rob Herring <robh@kernel.org>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Petre Rodan <petre.rodan@subdimension.ro>,
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>
Subject: Re: [PATCH 2/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml
 add sleep-mode
Message-ID: <20240117172708.GA2695411-robh@kernel.org>
References: <20240110172306.31273-1-petre.rodan@subdimension.ro>
 <20240110172306.31273-3-petre.rodan@subdimension.ro>
 <bc37f7d8-c43f-4751-9216-fc95f439b2f6@linaro.org>
 <ZaDqlmXJD6if1xK7@sunspire>
 <20240116173023.GA139792-robh@kernel.org>
 <20240117165001.00003046@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240117165001.00003046@Huawei.com>

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@kernel.org> 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@subdimension.ro>
> > > > > ---
> > > > >  .../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