Re: [PATCH v3 2/3] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 16 Feb 2024 16:09:38 +0530
Jishnu Prakash <quic_jprakash@quicinc.com> wrote:

> Hi Jonathan,
> 
> On 1/1/2024 11:32 PM, Jonathan Cameron wrote:
> > On Sun, 31 Dec 2023 22:42:36 +0530
> > Jishnu Prakash <quic_jprakash@quicinc.com> wrote:
> >  
> >> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the  
> 
> >> +
> >> +      label:
> >> +        $ref: /schemas/types.yaml#/definitions/string
> >> +        description: |
> >> +            ADC input of the platform as seen in the schematics.
> >> +            For thermistor inputs connected to generic AMUX or GPIO inputs
> >> +            these can vary across platform for the same pins. Hence select
> >> +            the platform schematics name for this channel.  
> > defined in adc.yaml, so should just have a reference to that here.
> >  
> >> +
> >> +      qcom,decimation:
> >> +        $ref: /schemas/types.yaml#/definitions/uint32
> >> +        description: |
> >> +            This parameter is used to decrease ADC sampling rate.
> >> +            Quicker measurements can be made by reducing decimation ratio.  
> > Why is this in DT rather than as a userspace control?  
> 
> 
> We don't intend this property to be something that can be controlled 
> from userspace - if a client wants to read an ADC channel from 
> userspace, we only intend to provide them the processed value, 
> calculated with a fixed set of ADC properties mentioned in the 
> corresponding channel node in DT.

Why?  This is a way to control precision of an ADC channel read out.
That's policy rather than dependent on the hardware.
To be in DT we (mostly) need it to be related to the hardware configuration
(i.e. what it is wired to etc).


> 
> 
> >> +        enum: [ 85, 340, 1360 ]
> >> +        default: 1360
> >> +  
> 
> >> +
> >> +      qcom,hw-settle-time:
> >> +        $ref: /schemas/types.yaml#/definitions/uint32
> >> +        description: |
> >> +            Time between AMUX getting configured and the ADC starting
> >> +            conversion. The 'hw_settle_time' is an index used from valid values
> >> +            and programmed in hardware to achieve the hardware settling delay.
> >> +        enum: [ 15, 100, 200, 300, 400, 500, 600, 700, 1000, 2000, 4000,
> >> +                8000, 16000, 32000, 64000, 128000 ]
> >> +        default: 15  
> > only currently defined for muxes but we have settle-time-us which has benefit of
> > providing the units (which are missing here from the description as well)
> >  
> >> +
> >> +      qcom,avg-samples:
> >> +        $ref: /schemas/types.yaml#/definitions/uint32
> >> +        description: |
> >> +            Number of samples to be used for measurement.
> >> +            Averaging provides the option to obtain a single measurement
> >> +            from the ADC that is an average of multiple samples. The value
> >> +            selected is 2^(value).  
> > Why is this in dt?  Why not just userspace control (in_voltageX_oversampling_ratio
> >
> > If it needs to be, we do have standard DT bindings for it in adc.yaml  
> 
> 
> avg-samples is also something we don't want the client to modify from 
> userspace. As for using adc.yaml, I think I could use settling-time-us 
> and oversampling-ratio from it for the above two properties.

Same as for above.  This is policy. If you want to control it that belongs
in a udev script or similar, not the DT bindings.
We tend to resist defining such policy in DT because it isn't a characteristic
of the hardware and depending on the usecase userspace may have good reason
to tweak the settings (or consumer drivers if you have those as sometimes
these numbers are about getting a particular precision needed for what
we are measuring to be useful for another driver).

There is some legacy for this though as you point out. So that may be a
strong enough argument for why we should make an exception this time.
If so make that clear in the patch description.

> 
> However, Krzysztof has mentioned in another comment that I should put 
> properties common to ADC5 Gen3 and older QCOM VADC devices in a common 
> schema. If I now try replacing the existing qcom,hw-settle-time and 
> qcom,avg-samples properties with settling-time-us and oversampling-ratio 
> for older devices too, I would have to make several DT changes for 
> existing devices...are you fine with this? Or should I just replace 
> these two properties for ADC5 Gen3?

If you change DT binding for older devices, you will need to maintain
backwards compatibility.  It's fine to deprecate them in the binding
docs etc, but not the driver (as there may be old DT on devices that
can't be easily updated).

> 
> 
> I'll address your other comments in the next patchset.
> 
> 
> Thanks,
> 
> Jishnu
>