[PATCH v3 0/2] iio: proximity: hx9023s: Add performance tuning function

[PATCH v3 0/2] iio: proximity: hx9023s: Add performance tuning function

[Yasin Lee]

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

Signed-off-by: Yasin Lee <yasin.lee.x@gmail.com>
---
Changes in v3:
- Removed global register block configuration:
  - Replaced global tuning blob with individual properties for each configurable setting.
- New properties added for individual configurations:
  - tyhx,odr: Added scanning period configuration.
  - tyhx,range: Full-scale range configuration for each channel.
  - tyhx,avg: ADC averaging number configuration for each channel.
  - tyhx,osr: Oversampling rate configuration for each channel.
  - tyhx,sample-num: ADC sample frequency configuration.
  - tyhx,integration-num: Integration number configuration.
  - tyhx,lp-alpha: Low-pass filter coefficient configuration for each channel.
  - tyhx,bl-up-alpha: Baseline filter up coefficient configuration.
  - tyhx,bl-down-alpha: Baseline filter down coefficient configuration.
  - tyhx,drdy-interrupt: Added interrupt function enable configuration.
  - tyhx,int-high-num: Proximity persistency number (Near).
  - tyhx,int-low-num: Proximity persistency number (Far).
- General improvements:
  - Improved description clarity for all properties.
  - Updated examples section for better clarity and accuracy based on new properties.
- Parsing functions added:
  - Implemented parsing functions for the newly added properties.
- Link to v2: https://lore.kernel.org/r/20240926-add-performance-tuning-configuration-v2-0-fdbb654f5767@gmail.com

Changes in v2:
- In the YAML file, boundary constraints have been applied to the `tyhx,performance-tuning` property, requiring the number of elements to be between 2 and 512. The description also informs users that the number of elements must be a multiple of 2.
- In the function implementation, boundary checks have been added for this property, ensuring that the number of elements is even.
- Link to v1: https://lore.kernel.org/r/20240923-add-performance-tuning-configuration-v1-0-587220c8aece@gmail.com

---
Yasin Lee (2):
      dt-bindings: iio: tyhx,hx9023s: Add performance tuning configuration
      iio: proximity: hx9023s: Add performance tuning function

 .../bindings/iio/proximity/tyhx,hx9023s.yaml       | 195 +++++++++++++++++
 drivers/iio/proximity/hx9023s.c                    | 234 +++++++++++++++++++++
 2 files changed, 429 insertions(+)
---
base-commit: 7f6f44a9e58cd19093b544423bc04e1d668ec341
change-id: 20240923-add-performance-tuning-configuration-e2016e4d6e02

Best regards,
-- 
Yasin Lee <yasin.lee.x@gmail.com>

[PATCH v3 1/2] dt-bindings: iio: tyhx,hx9023s: Add performance tuning configuration

[Yasin Lee]

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

Signed-off-by: Yasin Lee <yasin.lee.x@gmail.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.
+    type: boolean
+
+  tyhx,chop:
+    description: Enable chop function.
+    type: boolean
+
+  tyhx,odr:
+    description: |
+      Defines the sensor scanning period. The values range from 0x00 to 0x1F,
+      corresponding to the following periods.
+      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
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 5
+    maxItems: 5
+
+  tyhx,avg:
+    description: |
+      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: |
+      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.
+    $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.
+      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.
+    $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>;

-- 
2.43.0

Re: [PATCH v3 1/2] dt-bindings: iio: tyhx,hx9023s: Add performance tuning configuration

[Jonathan Cameron]

On Thu, 17 Oct 2024 18:36:44 +0800
Yasin Lee <yasin.lee.x@gmail.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@gmail.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>;
> 

Re: [PATCH v3 1/2] dt-bindings: iio: tyhx,hx9023s: Add performance tuning configuration

[Yasin Lee]

On 10/20/24 21:06, Jonathan Cameron wrote:
> On Thu, 17 Oct 2024 18:36:44 +0800
> Yasin Lee <yasin.lee.x@gmail.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
>
Hi Jonathan,

Thank you for the suggestions in your recent email. Following your 
advice, I discussed these configurations in detail with engineers from 
the HX9023S supplier. Based on their feedback, these settings are not 
intended to be exposed to end-users. Typically, these configurations are 
adjusted during the DVT phase of the end product by the supplier to 
optimize performance, after which they are finalized and not meant to be 
modified dynamically at the user level.

Given this approach, it seems more appropriate to provide these settings 
as part of a firmware file, allowing the configuration to be kept 
internal and managed without user-level access. If this approach aligns 
with your thoughts, I can prepare and submit a new patch focused on 
firmware parsing and handling for these configurations.

Thank you again for your valuable guidance, and I look forward to your 
feedback.

Best regards,
Yasin Lee

Re: [PATCH v3 1/2] dt-bindings: iio: tyhx,hx9023s: Add performance tuning configuration

[Jonathan Cameron]

On Thu, 14 Nov 2024 23:16:51 +0800
Yasin Lee <yasin.lee.x@gmail.com> wrote:

> On 10/20/24 21:06, Jonathan Cameron wrote:
> > On Thu, 17 Oct 2024 18:36:44 +0800
> > Yasin Lee <yasin.lee.x@gmail.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
> >  
> Hi Jonathan,
> 
> Thank you for the suggestions in your recent email. Following your 
> advice, I discussed these configurations in detail with engineers from 
> the HX9023S supplier. Based on their feedback, these settings are not 
> intended to be exposed to end-users. Typically, these configurations are 
> adjusted during the DVT phase of the end product by the supplier to 
> optimize performance, after which they are finalized and not meant to be 
> modified dynamically at the user level.
> 
> Given this approach, it seems more appropriate to provide these settings 
> as part of a firmware file, allowing the configuration to be kept 
> internal and managed without user-level access. If this approach aligns 
> with your thoughts, I can prepare and submit a new patch focused on 
> firmware parsing and handling for these configurations.

Whilst I agree that a typical user may well not modify these settings
that doesn't necessarily make them suitable for control from the
Device Tree. Some may be but settings like ODR are about use case
not physical hardware. Average and OSR are normally a question of
trading off noise against data rate - that's policy not a fundamental
characteristic of the hardware. Filter controls are similar.

For other such as Dither, there may hardware configurations where it
doesn't need to be turned, only but does it do any harm? I'd be
somewhat surprised if the right thing to do there isn't to just hard
code it to turned on.

The enabling of dataready interrupt is entirely down to how the
device is being used, not the platform.

If these devices are being used in embedded platforms for a specific
purpose, then a simple udev rule or similar can configure the
defaults whilst still allowing them to be easily tweaked.
If you are dealing with standardized software it will already understand
many of the userspace ABI calls and have appropriate configuration files.

That is the appropriate level for such control, not device
tree.

If you have a strong case why a setting is never a policy decision
but rather a hard characteristic of the system, then that one may
be appropriate for DT.  Examples of this in the past have been things
like output voltage ranges for DACs because the hardware beyond
this device may only cope with some settings.

Jonathan



> 
> Thank you again for your valuable guidance, and I look forward to your 
> feedback.
> 
> Best regards,
> Yasin Lee
> 
> 

Re: [PATCH v3 1/2] dt-bindings: iio: tyhx,hx9023s: Add performance tuning configuration

[Yasin Lee]

On 11/23/24 21:21, Jonathan Cameron wrote:
> On Thu, 14 Nov 2024 23:16:51 +0800
> Yasin Lee <yasin.lee.x@gmail.com> wrote:
>
>> On 10/20/24 21:06, Jonathan Cameron wrote:
>>> On Thu, 17 Oct 2024 18:36:44 +0800
>>> Yasin Lee <yasin.lee.x@gmail.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
>>>   
>> Hi Jonathan,
>>
>> Thank you for the suggestions in your recent email. Following your
>> advice, I discussed these configurations in detail with engineers from
>> the HX9023S supplier. Based on their feedback, these settings are not
>> intended to be exposed to end-users. Typically, these configurations are
>> adjusted during the DVT phase of the end product by the supplier to
>> optimize performance, after which they are finalized and not meant to be
>> modified dynamically at the user level.
>>
>> Given this approach, it seems more appropriate to provide these settings
>> as part of a firmware file, allowing the configuration to be kept
>> internal and managed without user-level access. If this approach aligns
>> with your thoughts, I can prepare and submit a new patch focused on
>> firmware parsing and handling for these configurations.
> Whilst I agree that a typical user may well not modify these settings
> that doesn't necessarily make them suitable for control from the
> Device Tree. Some may be but settings like ODR are about use case
> not physical hardware. Average and OSR are normally a question of
> trading off noise against data rate - that's policy not a fundamental
> characteristic of the hardware. Filter controls are similar.
>
> For other such as Dither, there may hardware configurations where it
> doesn't need to be turned, only but does it do any harm? I'd be
> somewhat surprised if the right thing to do there isn't to just hard
> code it to turned on.
>
> The enabling of dataready interrupt is entirely down to how the
> device is being used, not the platform.
>
> If these devices are being used in embedded platforms for a specific
> purpose, then a simple udev rule or similar can configure the
> defaults whilst still allowing them to be easily tweaked.
> If you are dealing with standardized software it will already understand
> many of the userspace ABI calls and have appropriate configuration files.
>
> That is the appropriate level for such control, not device
> tree.
>
> If you have a strong case why a setting is never a policy decision
> but rather a hard characteristic of the system, then that one may
> be appropriate for DT.  Examples of this in the past have been things
> like output voltage ranges for DACs because the hardware beyond
> this device may only cope with some settings.
>
> Jonathan
>
Hi Jonathan,

Thank you for your detailed explanation and insights. I fully agree with 
your point that settings such as ODR, Average, OSR, and filter-related 
configurations, being policy-driven, should not be included in the 
Device Tree.

As you mentioned, the dither setting is typically left disabled in most 
cases. This device is indeed used for specific purposes in embedded 
platforms, and there is no requirement for runtime flexibility in 
adjusting these configurations.

Given this, I have decided to drop this submission. Moving forward, I 
plan to address varying hardware requirements by adapting these 
configurations using a firmware-based approach.

Thank you again for your guidance and support!

Best regards,
Yasin Lee
>
>> Thank you again for your valuable guidance, and I look forward to your
>> feedback.
>>
>> Best regards,
>> Yasin Lee
>>
>>

[PATCH v3 2/2] iio: proximity: hx9023s: Add performance tuning function

[Yasin Lee]

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

Signed-off-by: Yasin Lee <yasin.lee.x@gmail.com>
---
 drivers/iio/proximity/hx9023s.c | 234 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 234 insertions(+)

diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
index 8b9f84400e00..5d0338588616 100644
--- a/drivers/iio/proximity/hx9023s.c
+++ b/drivers/iio/proximity/hx9023s.c
@@ -61,6 +61,7 @@
 #define HX9023S_OFFSET_DAC4_9_8                0x1E
 #define HX9023S_SAMPLE_NUM_7_0                 0x1F
 #define HX9023S_INTEGRATION_NUM_7_0            0x21
+#define HX9023S_GLOBAL_CTRL2                   0x23
 #define HX9023S_CH_NUM_CFG                     0x24
 #define HX9023S_LP_ALP_4_CFG                   0x29
 #define HX9023S_LP_ALP_1_0_CFG                 0x2A
@@ -623,6 +624,235 @@ static int hx9023s_property_get(struct hx9023s_data *data)
 	return 0;
 }
 
+static int hx9023s_performance_tuning(struct hx9023s_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+	bool dither;
+	bool chop;
+	u32 odr;
+	u32 range[HX9023S_CH_NUM];
+	u32 avg[HX9023S_CH_NUM];
+	u32 osr[HX9023S_CH_NUM];
+	u32 sample_time;
+	u32 integration_time;
+	u32 lp_alpha[HX9023S_CH_NUM];
+	u32 bl_up_alpha[HX9023S_CH_NUM];
+	u32 bl_down_alpha[HX9023S_CH_NUM];
+	u32 drdy_interrput;
+	u32 int_high_num;
+	u32 int_low_num;
+	u32 temp;
+
+	/* dither */
+	dither = device_property_read_bool(dev, "tyhx,dither");
+	if (dither)
+		ret = regmap_update_bits(data->regmap, HX9023S_GLOBAL_CTRL0, BIT(6), BIT(6));
+	else
+		ret = regmap_update_bits(data->regmap, HX9023S_GLOBAL_CTRL0, BIT(6), 0);
+
+	/* chop */
+	chop = device_property_read_bool(dev, "tyhx,chop");
+	if (chop)
+		ret = regmap_update_bits(data->regmap, HX9023S_GLOBAL_CTRL2, GENMASK(4, 0),
+					GENMASK(4, 0));
+	else
+		ret = regmap_update_bits(data->regmap, HX9023S_GLOBAL_CTRL2, GENMASK(4, 0), 0);
+
+	/* odr */
+	ret = device_property_read_u32(dev, "tyhx,odr", &odr);
+	if (!ret) {
+		ret = regmap_update_bits(data->regmap, HX9023S_PRF_CFG, GENMASK(4, 0),
+					FIELD_PREP(GENMASK(4, 0), odr));
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update odr\n");
+	}
+
+	/* range */
+	ret = device_property_read_u32_array(dev, "tyhx,range", range, ARRAY_SIZE(range));
+	if (!ret) {
+		temp = FIELD_PREP(GENMASK(2, 0), range[0]) | FIELD_PREP(GENMASK(6, 4), range[1]);
+		ret = regmap_update_bits(data->regmap, HX9023S_RANGE_7_0, GENMASK(6, 0), temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update range for ch0 and ch1\n");
+
+		temp = FIELD_PREP(GENMASK(2, 0), range[2]) | FIELD_PREP(GENMASK(6, 4), range[3]);
+		ret = regmap_update_bits(data->regmap, HX9023S_RANGE_9_8, GENMASK(6, 0), temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update range for ch2 and ch3\n");
+
+		temp = FIELD_PREP(GENMASK(2, 0), range[4]);
+		ret = regmap_update_bits(data->regmap, HX9023S_RANGE_18_16, GENMASK(2, 0), temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update range for ch4\n");
+	}
+
+	/* avg */
+	ret = device_property_read_u32_array(dev, "tyhx,avg", avg, ARRAY_SIZE(avg));
+	if (!ret) {
+		temp = FIELD_PREP(GENMASK(7, 5), avg[0]);
+		ret = regmap_update_bits(data->regmap, HX9023S_AVG0_NOSR0_CFG, GENMASK(7, 5),
+					temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update avg for ch0\n");
+
+		temp = FIELD_PREP(GENMASK(6, 4), avg[2]) | FIELD_PREP(GENMASK(2, 0), avg[1]);
+		ret = regmap_update_bits(data->regmap, HX9023S_AVG12_CFG, GENMASK(6, 0), temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update avg for ch1 and ch2\n");
+
+		temp = FIELD_PREP(GENMASK(6, 4), avg[4]) | FIELD_PREP(GENMASK(2, 0), avg[3]);
+		ret = regmap_update_bits(data->regmap, HX9023S_AVG34_CFG, GENMASK(6, 0), temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update avg for ch3 and ch4\n");
+	}
+
+	/* osr */
+	ret = device_property_read_u32_array(dev, "tyhx,osr", osr, ARRAY_SIZE(osr));
+	if (!ret) {
+		temp = FIELD_PREP(GENMASK(4, 2), osr[0]);
+		ret = regmap_update_bits(data->regmap, HX9023S_AVG0_NOSR0_CFG, GENMASK(4, 2),
+					temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update osr for ch0\n");
+
+		temp = FIELD_PREP(GENMASK(6, 4), osr[2]) | FIELD_PREP(GENMASK(2, 0), osr[1]);
+		ret = regmap_update_bits(data->regmap, HX9023S_NOSR12_CFG, GENMASK(6, 0), temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update osr for ch1 and ch2\n");
+
+		temp = FIELD_PREP(GENMASK(6, 4), osr[4]) | FIELD_PREP(GENMASK(2, 0), osr[3]);
+		ret = regmap_update_bits(data->regmap, HX9023S_NOSR34_CFG, GENMASK(6, 0), temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update osr for ch3 and ch4\n");
+	}
+
+	/* sample time */
+	ret = device_property_read_u32(dev, "tyhx,sample-time", &sample_time);
+	if (!ret) {
+		ret = regmap_write(data->regmap, HX9023S_SAMPLE_NUM_7_0, sample_time);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update sample_time\n");
+	}
+
+	/* integration time */
+	ret = device_property_read_u32(dev, "tyhx,integration-time", &integration_time);
+	if (!ret) {
+		ret = regmap_write(data->regmap, HX9023S_INTEGRATION_NUM_7_0, integration_time);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update integration_time\n");
+	}
+
+	/* lp-alpha */
+	ret = device_property_read_u32_array(dev, "tyhx,lp-alpha", lp_alpha, ARRAY_SIZE(lp_alpha));
+	if (!ret) {
+		temp = FIELD_PREP(GENMASK(6, 4), lp_alpha[1])
+			| FIELD_PREP(GENMASK(2, 0), lp_alpha[0]);
+		ret = regmap_write(data->regmap, HX9023S_LP_ALP_1_0_CFG, temp);
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"Failed to update lp-alpha for ch0 and ch1\n");
+
+		temp = FIELD_PREP(GENMASK(6, 4), lp_alpha[3])
+			| FIELD_PREP(GENMASK(2, 0), lp_alpha[2]);
+		ret = regmap_write(data->regmap, HX9023S_LP_ALP_3_2_CFG, temp);
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"Failed to update lp-alpha for ch2 and ch3\n");
+
+		temp = FIELD_PREP(GENMASK(2, 0), lp_alpha[4]);
+		ret = regmap_update_bits(data->regmap, HX9023S_LP_ALP_4_CFG, GENMASK(2, 0), temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update lp-alpha for ch4\n");
+	}
+
+	/* bl-up-alpha */
+	ret = device_property_read_u32_array(dev, "tyhx,bl-up-alpha",
+					bl_up_alpha, ARRAY_SIZE(bl_up_alpha));
+	if (!ret) {
+		temp = FIELD_PREP(GENMASK(7, 4), bl_up_alpha[1])
+			| FIELD_PREP(GENMASK(3, 0), bl_up_alpha[0]);
+		ret = regmap_write(data->regmap, HX9023S_UP_ALP_1_0_CFG, temp);
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"Failed to update bl-up-alpha for ch0 and ch1\n");
+
+		temp = FIELD_PREP(GENMASK(7, 4), bl_up_alpha[3])
+			| FIELD_PREP(GENMASK(3, 0), bl_up_alpha[2]);
+		ret = regmap_write(data->regmap, HX9023S_UP_ALP_3_2_CFG, temp);
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"Failed to update bl-up-alpha for ch2 and ch3\n");
+
+		temp = FIELD_PREP(GENMASK(3, 0), bl_up_alpha[4]);
+		ret = regmap_update_bits(data->regmap, HX9023S_DN_UP_ALP_0_4_CFG, GENMASK(3, 0),
+					temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update bl-up-alpha for ch4\n");
+	}
+
+	/* bl-down-alpha */
+	ret = device_property_read_u32_array(dev, "tyhx,bl-down-alpha",
+					bl_down_alpha, ARRAY_SIZE(bl_down_alpha));
+	if (!ret) {
+		temp = FIELD_PREP(GENMASK(7, 4), bl_down_alpha[0]);
+		ret = regmap_update_bits(data->regmap, HX9023S_DN_UP_ALP_0_4_CFG, GENMASK(7, 4),
+					temp);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update bl-dn-alpha for ch0\n");
+
+		temp = FIELD_PREP(GENMASK(7, 4), bl_down_alpha[2])
+			| FIELD_PREP(GENMASK(3, 0), bl_down_alpha[1]);
+		ret = regmap_write(data->regmap, HX9023S_DN_ALP_2_1_CFG, temp);
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"Failed to update bl-dn-alpha for ch1 and ch2\n");
+
+		temp = FIELD_PREP(GENMASK(7, 4), bl_down_alpha[4])
+			| FIELD_PREP(GENMASK(3, 0), bl_down_alpha[3]);
+		ret = regmap_write(data->regmap, HX9023S_DN_ALP_4_3_CFG, temp);
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"Failed to update bl-dn-alpha for ch3 and ch4\n");
+	}
+
+	/* dydy-interrupt */
+	ret = device_property_read_u32(dev, "tyhx,drdy-interrupt", &drdy_interrput);
+	if (!ret) {
+		ret = regmap_update_bits(data->regmap, HX9023S_CALI_DIFF_CFG, GENMASK(7, 4),
+					FIELD_PREP(GENMASK(7, 4), drdy_interrput));
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"Failed to update drdy-interrput for ch0~ch3\n");
+
+		ret = regmap_update_bits(data->regmap, HX9023S_DITHER_CFG, BIT(7),
+					FIELD_PREP(BIT(7), drdy_interrput >> 4));
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"Failed to update drdy-interrput for ch4\n");
+	}
+
+	/* int-high-num */
+	ret = device_property_read_u32(dev, "tyhx,int-high-num", &int_high_num);
+	if (!ret) {
+		ret = regmap_update_bits(data->regmap, HX9023S_PROX_INT_HIGH_CFG, GENMASK(3, 0),
+					FIELD_PREP(GENMASK(3, 0), int_high_num));
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update int-high-num\n");
+	}
+
+	/* int-low-num */
+	ret = device_property_read_u32(dev, "tyhx,int-low-num", &int_low_num);
+	if (!ret) {
+		ret = regmap_update_bits(data->regmap, HX9023S_PROX_INT_LOW_CFG, GENMASK(3, 0),
+					FIELD_PREP(GENMASK(3, 0), int_low_num));
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to update int-low-num\n");
+	}
+
+	return 0;
+}
+
 static int hx9023s_update_chan_en(struct hx9023s_data *data,
 				  unsigned long chan_read,
 				  unsigned long chan_event)
@@ -1045,6 +1275,10 @@ static int hx9023s_probe(struct i2c_client *client)
 	if (ret)
 		return dev_err_probe(dev, ret, "channel config failed\n");
 
+	ret = hx9023s_performance_tuning(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "performance tuning failed\n");
+
 	ret = regcache_sync(data->regmap);
 	if (ret)
 		return dev_err_probe(dev, ret, "regcache sync failed\n");

-- 
2.43.0

Re: [PATCH v3 2/2] iio: proximity: hx9023s: Add performance tuning function

[Jonathan Cameron]

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

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

Given review of previous patch that I just sent, I'm not going to review
the implementation at this point.

Jonathan