Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties

From: Jonathan Cameron
Date: Sun Mar 24 2024 - 09:54:51 EST


On Fri, 22 Mar 2024 10:58:54 +0100
Andrej Picej <andrej.picej@xxxxxxxxx> wrote:

> On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
> > On 22/03/2024 08:39, Andrej Picej wrote:
> >> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
> >>> On 20/03/2024 13:05, Andrej Picej wrote:
> >>>> Hi Krzysztof,
> >>>>
> >>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
> >>>>> On 20/03/2024 11:04, Andrej Picej wrote:
> >>>>>> Document calibration properties and how to set them.
> >>>>>
> >>>>> Bindings are before users.
> >>>>
> >>>> will change patch order when I send a v2.
> >>>>
> >>>>>
> >>>>> Please use subject prefixes matching the subsystem. You can get them for
> >>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> >>>>> your patch is touching.
> >>>>> There is no file extension in prefixes.
> >>>>
> >>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
> >>>
> >>> Did you run the command I proposed? I don't see much of "/", but except
> >>> that looks good.
> >>
> >> Ok noted.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Andrej Picej <andrej.picej@xxxxxxxxx>
> >>>>>> ---
> >>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> >>>>>> 1 file changed, 15 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> index dacc526dc695..64958be62a6a 100644
> >>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> @@ -46,6 +46,21 @@ properties:
> >>>>>> "#io-channel-cells":
> >>>>>> const: 1
> >>>>>>
> >>>>>> + nxp,calib-avg-en:
> >>>>>> + description:
> >>>>>> + Enable or disable averaging of calibration time.
> >>>>>> + enum: [ 0, 1 ]
> >>>>>> +
> >>>>>> + nxp,calib-nr-samples:
> >>>>>> + description:
> >>>>>> + Selects the number of averaging samples to be used during calibration.
> >>>>>> + enum: [ 16, 32, 128, 512 ]
> >>>>>> +
> >>>>>> + nxp,calib-t-samples:
> >>>>>> + description:
> >>>>>> + Specifies the sample time of calibration conversions.
> >>>>>> + enum: [ 8, 16, 22, 32 ]
> >>>>>
> >>>>> No, use existing, generic properties. Open other bindings for this.
> >>>>
> >>>> You mean I should use generic properties for the ADC calibration
> >>>> settings? Is there already something in place? Because as I understand
> >>>> it, these calib-* values only effect the calibration process of the ADC.
> >>>
> >>> Please take a look at other devices and dtschema. We already have some
> >>> properties for this... but maybe they cannot be used?
> >>>
> >>
> >> I did look into other ADC devices, grep across iio/adc, adc bindings
> >> folders and couldn't find anything closely related to what we are
> >> looking for. Could you please point me to the properties that you think
> >> should be used for this?
> >
> > Indeed, there are few device specific like qcom,avg-samples. We have
> > though oversampling-ratio, settling-time-us and min-sample-time (which
> > is not that good because does not use unit suffix).
>
> Ok, these are examples but I think I should not use them, since these
> are i.MX93 ADC specific settings, which are used for configuration of
> calibration process, and are not related to the standard conversion
> process during runtime. Calibration process is the first step that
> should be done after every power-on reset.
>
> >
> > Then follow up questions:
> > - nxp,calib-avg-en: Why is it a board-level decision? I would assume
> > this depends on user choice and what kind of input you have (which could
> > be board dependent or could be runtime decision).
>
> Not really sure I get your question, so please elaborate if I missed the
> point.
> This is a user choice, to enable or disable the averaging function in
> calibration, but this is a board-level decision, probably relates on
> external ADC regulators and input connections. The same options are used
> for every ADC channel and this can not be a runtime decision, since
> calibration is done before the ADC is even registered.

I'll raise this question in reply to the cover letter or patch 1 where
it is perhaps more appropriate, but I'd really like to know more about why
these are useful at all.

>
> > - nxp,calib-t-samples: what does it mean? Time is expressed in time
> > units, but there is nothing about units in the property name.
> >
>
> You are right, basically this is "time" in cycles of AD_CLK. I should at
> least add that to the property description.
>
> Best regards,
> Andrej Picej