Message-ID: <20241020140638.127a9dbf@jic23-huawei>
Date: Sun, 20 Oct 2024 14:06:38 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Yasin Lee <yasin.lee.x@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, yasin.lee.x@...look.com, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] dt-bindings: iio: tyhx,hx9023s: Add performance
 tuning configuration

On Thu, 17 Oct 2024 18:36:44 +0800
Yasin Lee <yasin.lee.x@...il.com> wrote:

> When hardware design introduces significant sensor data noise,
> performance can be improved by adjusting register settings.

Questions inline. Mostly around why these controls belong in DT.
What do they have to do with hardware / wiring etc rather than being
appropriate for userspace controls.

So almost all are definite no to being suitable for device tree bindings.

Jonathan


> 
> Signed-off-by: Yasin Lee <yasin.lee.x@...il.com>
> ---
>  .../bindings/iio/proximity/tyhx,hx9023s.yaml       | 195 +++++++++++++++++++++
>  1 file changed, 195 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml b/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml
> index 64ce8bc8bd36..af419a3335eb 100644
> --- a/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml
> +++ b/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml
> @@ -28,6 +28,189 @@ properties:
>  
>    vdd-supply: true
>  
> +  tyhx,dither:
> +    description: Enable spread spectrum function.

Why not turn this on all the time?  The datasheet I found suggests
this is to reduce EMI.

> +    type: boolean
> +
> +  tyhx,chop:
> +    description: Enable chop function.

No idea what this is in a proximity sensor. The datasheet says no more than you have here.
Without more info very hard to review this one.

> +    type: boolean
> +
> +  tyhx,odr:
> +    description: |
> +      Defines the sensor scanning period. The values range from 0x00 to 0x1F,
> +      corresponding to the following periods.

Userspace should be controlling this not DT.
+ it already does I think. So not appropriate for DT.
If you need a default add a udev script to set it.




> +      Val: Period
> +      0x00: Min (no idle time)
> +      0x01: 2 ms
> +      0x02: 4 ms
> +      0x03: 6 ms
> +      0x04: 8 ms
> +      0x05: 10 ms
> +      0x06: 14 ms
> +      0x07: 18 ms
> +      0x08: 22 ms
> +      0x09: 26 ms
> +      0x0A: 30 ms
> +      0x0B: 34 ms
> +      0x0C: 38 ms
> +      0x0D: 42 ms
> +      0x0E: 46 ms
> +      0x0F: 50 ms
> +      0x10: 56 ms
> +      0x11: 62 ms
> +      0x12: 68 ms
> +      0x13: 74 ms
> +      0x14: 80 ms
> +      0x15: 90 ms
> +      0x16: 100 ms
> +      0x17: 200 ms
> +      0x18: 300 ms
> +      0x19: 400 ms
> +      0x1A: 600 ms
> +      0x1B: 800 ms
> +      0x1C: 1000 ms
> +      0x1D: 2000 ms
> +      0x1E: 3000 ms
> +      0x1F: 4000 ms
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0x00
> +    maximum: 0x1F
> +
> +  tyhx,range:
> +    description: |
> +      Defines the full-scale range for each channel.
> +      The values correspond to the following full-scale ranges.
> +      Val: Full Scale
> +      0x0: 1.25pF
> +      0x1: 2.5pF
> +      0x2: 3.75pF
> +      0x3: 5pF
> +      0x4: 0.625pF

This one 'might' be appropriate in DT if the value it should take reflects
sensing plate design etc connected to the chip.  Is that the case here?

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 5
> +    maxItems: 5
> +
> +  tyhx,avg:
> +    description: |

This and next both appear to be oversampling in IIO userspace controls.
Not appropriate for DT as it is a policy decision trading off effective
data rate against noise.  The datasheet doesn't provide enough information
for me to understand what the difference is.
Maybe OSR is considered to be increase in sampling that doesn't affect the
scanning period, whereas averaging is multiple sampling periods?



> +      Defines the ADC averaging value for each channel.
> +      The values correspond to the following averages.
> +      Val: Avg Number
> +      0x0: 1
> +      0x1: 2
> +      0x2: 4
> +      0x3: 8
> +      0x4: 16
> +      0x5: 32
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 5
> +    maxItems: 5
> +
> +  tyhx,osr:
> +    description: |
> +      Defines the ADC oversampling rate (OSR) for each channel.
> +      The values correspond to the following OSR.
> +      Val: OSR
> +      0x0: 16
> +      0x1: 32
> +      0x2: 64
> +      0x3: 128
> +      0x4: 256
> +      0x5: 512
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 5
> +    maxItems: 5
> +
> +  tyhx,sample-num:
> +    description: |
So this is coupled with scanning period given above, but is again
not suitable for DT as it is a policy choice that userspace should be
controlling.

> +      Defines the ADC sample frequency.
> +      The sample frequency can be calculated with the following formula:
> +      Fsample = 1.0 / ( sample_num * 200ns ),
> +      where `sample_num` is the value in the register in decimal.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0x00
> +    maximum: 0xFF
> +
> +  tyhx,integration-num:
> +    description: The integration number should be the same as the `sample-num` above.
If we were considering the previous one, then why have this as well?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0x00
> +    maximum: 0xFF
> +
> +  tyhx,lp-alpha:
> +    description: |
> +      Defines the coefficient for the first-order low pass filter for each channel.
> +      The values correspond to the following coefficients.

Map this to userspace filter controls.  Note that userspace is not going to figure
out the filter design so you need to map it to 3DB points.
Not suitable for device tree.


> +      Val: Coefficient
> +      0x0: 1
> +      0x1: 1/2
> +      0x2: 1/4
> +      0x3: 1/8
> +      0x4: 1/16
> +      0x5: 1/32
> +      0x6: 1/64
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 5
> +    maxItems: 5
> +
> +  tyhx,bl-up-alpha:
> +    description: |
> +      Defines the up coefficient of the first-order low pass filter for each channel.
> +      The values correspond to the following coefficients.
> +      Val: Coefficient
> +      0x0: 0
> +      0x1: 1
> +      0x2: 1/2
> +      0x3: 1/4
> +      0x4: 1/8
> +      0x5: 1/16
> +      0x6: 1/32
> +      0x7: 1/64
> +      0x8: 1/128
> +      0x9: 1/256
> +      0xA: 1/512
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 5
> +    maxItems: 5
> +
> +  tyhx,bl-down-alpha:
> +    description: |
> +      Defines the down coefficient of the first-order low pass filter for each channel.
> +      The values correspond to the following coefficients.
> +      Val: Coefficient
> +      0x0: 0
> +      0x1: 1
> +      0x2: 1/2
> +      0x3: 1/4
> +      0x4: 1/8
> +      0x5: 1/16
> +      0x6: 1/32
> +      0x7: 1/64
> +      0x8: 1/128
> +      0x9: 1/256
> +      0xA: 1/512
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 5
> +    maxItems: 5
> +
> +  tyhx,drdy-interrupt:
> +    description: Enable the interrupt function of each channel when the conversion is ready.
userspace control.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0x00
> +    maximum: 0x1F
> +
> +  tyhx,int-high-num:
> +    description: Defines the Proximity persistency number (Near).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0x1
> +    maximum: 0xF
> +
> +  tyhx,int-low-num:
> +    description: Defines the Proximity persistency number (Far).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0x1
> +    maximum: 0xF
> +
>    "#address-cells":
>      const: 1
>  
> @@ -65,6 +248,18 @@ examples:
>          interrupt-parent = <&pio>;
>          interrupts = <16 IRQ_TYPE_EDGE_FALLING>;
>          vdd-supply = <&pp1800_prox>;
> +        tyhx,odr = <0x17>;
> +        tyhx,range = <0x4 0x4 0x4 0x4 0x4>;
> +        tyhx,avg = <0x3 0x3 0x3 0x0 0x0>;
> +        tyhx,osr = <0x4 0x4 0x4 0x0 0x0>;
> +        tyhx,sample-num = <0x65>;
> +        tyhx,integration-num = <0x65>;
> +        tyhx,lp-alpha = <0x2 0x2 0x2 0x2 0x2>;
> +        tyhx,bl-up-alpha = <0x8 0x8 0x8 0x8 0x8>;
> +        tyhx,bl-down-alpha = <0x1 0x1 0x1 0x1 0x1>;
> +        tyhx,drdy-interrupt = <0x1F>;
> +        tyhx,int-high-num = <0x1>;
> +        tyhx,int-low-num = <0x1>;
>  
>          #address-cells = <1>;
>          #size-cells = <0>;
> 

Powered by blists - more mailing lists