| Message ID | 20241017-add-performance-tuning-configuration-v3-1-e7289791f523@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: proximity: hx9023s: Add performance tuning function | expand |
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>; >
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
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 > >
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 >> >>
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>;
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(+)