Re: [PATCH v5 1/3] dt-bindings:iio:adc: add generic settling-time-us and oversampling-ratio channel properties

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 29 Mar 2021 15:06:20 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Mon, Mar 29, 2021 at 11:25:32AM +0100, Jonathan Cameron wrote:
> > On Mon, 29 Mar 2021 09:31:29 +0200
> > Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >   
> > > Settling time and over sampling is a typical challenge for different IIO ADC
> > > devices. So, introduce channel specific settling-time-us and oversampling-ratio
> > > properties to cover this use case.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/iio/adc/adc.yaml | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adc.yaml b/Documentation/devicetree/bindings/iio/adc/adc.yaml
> > > index 912a7635edc4..d5bc86d2a2af 100644
> > > --- a/Documentation/devicetree/bindings/iio/adc/adc.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adc.yaml
> > > @@ -39,4 +39,12 @@ properties:
> > >        The first value specifies the positive input pin, the second
> > >        specifies the negative input pin.
> > >  
> > > +  settling-time-us:
> > > +    description:
> > > +      Time between enabling the channel and firs stable readings.  
> > 
> > first  
> 
> ack
> 
> > > +
> > > +  oversampling-ratio:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    description: Number of data samples which are averaged for each read.  
> > 
> > I think I've asked about this in previous reviews, but I want a clear statement
> > of why you think this property is a feature of the 'board' (and hence should be
> > in device tree) rather than setting sensible defaults and leaving any control
> > to userspace?  
> 
> yes, my reply was:

Ah. I missed it somewhere along the way, thanks for repeating here.

> > Oversampling is used as replacement of or addition to the low-pass filter. The
> > filter can be implemented on board, but it will change settling time
> > characteristic. Since low-pass filter is board specific characteristic, this
> > property belongs in device tree as well.  
> 
> I could imagine that this values can be overwritten from user space for
> diagnostic, but we need some working default values. 

Hmm. So low pass filters are interesting whether they are actually a characteristic
of the board (obviously they are if they are resistors/ caps etc on the board),
or of the application. Some applications want noisy messy data,
others not so much. What filter you need to achieve a specific noise level on
a given board is indeed a characteristic of the board.  However, what that noise
level is (which actually drives the decision) is not a board characteristic.
If we have a configurable filter, then that can be argued to be a policy decision
and hence userspace, not DT.

> 
> Should I integrate this comment in to the yaml?

Definitely.  Whilst I'm not that keen on this one, you have made a reasonable argument
that it is 'sort of' a board characteristic, so I can live with that as long as
it is there.   Perhaps the slightly amended version of the above.

"Oversampling is used as replacement of or addition to the low-pass filter.
In some cases, the desired filtering characteristics are a function the
device design and can interact with other characteristics such as
settling time."

Jonathan


> 
> Regards,
> Oleksij