Re: [PATCH v3 00/27] iio: resolver: move ad2s1210 out of staging

David Lechner
2023-09-29T22:02:40Z

On 9/29/23 12:23 PM, David Lechner wrote:
> From: David Lechner <david@lechnology.com>
>
> v3 changes:
>
> * Added description of A0/A1 lines in DT bindings.
> * Added power supply regulators to DT bindings.
> * Dropped "staging: iio: Documentation: document IIO resolver AD2S1210
> sysfs attributes" (these attributes are being removed instead).
> * Dropped applied patches:
> * "staging: iio: resolver: ad2s1210: fix ad2s1210_show_fault"
> * "iio: adc: MCP3564: fix the static checker warning"
> * Split "staging: iio: resolver: ad2s1210: fix probe" into multiple patches.
> * Moved sorting imports to separate patch.
> * Renamed fclkin to clkin_hz.
> * Added __be16 sample field to state struct for reading raw samples.
> * Split out new function ad2s1210_single_conversion() from
> ad2s1210_read_raw().
> * Split out new ad2s1210_get_hysteresis() and ad2s1210_set_hysteresis()
> functions.
> * Fixed multi-line comment style.
> * Added notes about soft reset not resetting config registers.
> * Made use of FIELD_PREP() macro.
> * Added more explanation to regmap commit message.
> * Removed datasheet names from channel specs.
> * Replaced "staging: iio: resolver: ad2s1210: rename fexcit attribute"
> with "staging: iio: resolver: ad2s1210: convert fexcit to channel
> attribute".
> * Replaced "staging: iio: resolver: ad2s1210: add phase_lock_range
> attributes" with "staging: iio: resolver: ad2s1210: add phase lock
> range support"
> * Added additional patches to convert custom device attributes to event
> attributes.
> * Added patch to add channel label attributes.
>
> v2 changes:
> * Address initial device tree patch feedback
> * Drop "iio: sysfs: add IIO_DEVICE_ATTR_NAMED_RW macro" (related cleanups
> also dropped for now, will address in a future series if needed)
> * Apply improvements as a series of patches to the staging driver. It is
> not quite ready for the move out of staging patch yet.
>
> This series has been tested on actual hardware using a EVAL-AD2S1210 evaluation
> board. (Note: not all device tree features have been implemented in the driver
> since the eval board doesn't support them out of the box. We plan to add them
> later if needed.)
>
> Most of the questions about dealing with faults from the v2 cover letter
> have been addressed. There is still the question about what to do with
> the current `fault` attribute (it is the only custom device attribute
> remaining from the original staging driver). It was suggested to split it
> out into multiple attributes in a subdirectory. Since we now have events
> for all of the faults, I'm wondering if this is something that is still needed.
> In the current implementation, it is possible to start listening to events,
> clear the faults and then read a sample to trigger events for any current
> faults so we have a way to get current faults already.
>
> There is also the matter of clearing faults. Writing the excitation
> frequency has a side-effect of clearing the faults, so we could use
> that as the reset. Or we could change the current fault attribute to
> write-only and rename it. Or is there a better way that I have overlooked?
>
> Once this last issue is addressed, I think this driver will be ready
> for consideration for moving out of staging.
> ---
> David Lechner (27):
> dt-bindings: iio: resolver: add devicetree bindings for ad2s1210
> staging: iio: resolver: ad2s1210: fix use before initialization
> staging: iio: resolver: ad2s1210: remove call to spi_setup()
> staging: iio: resolver: ad2s1210: check return of ad2s1210_initial()
> staging: iio: resolver: ad2s1210: remove spi_set_drvdata()
> staging: iio: resolver: ad2s1210: sort imports
> staging: iio: resolver: ad2s1210: always use 16-bit value for raw read
> staging: iio: resolver: ad2s1210: implement IIO_CHAN_INFO_SCALE
> staging: iio: resolver: ad2s1210: use devicetree to get CLKIN rate
> staging: iio: resolver: ad2s1210: use regmap for config registers
> staging: iio: resolver: ad2s1210: add debugfs reg access
> staging: iio: resolver: ad2s1210: remove config attribute
> staging: iio: resolver: ad2s1210: rework gpios
> staging: iio: resolver: ad2s1210: implement hysteresis as channel attr
> staging: iio: resolver: ad2s1210: refactor setting excitation frequency
> staging: iio: resolver: ad2s1210: read excitation frequency from control register
> staging: iio: resolver: ad2s1210: convert fexcit to channel attribute
> staging: iio: resolver: ad2s1210: convert resolution to devicetree property
> staging: iio: resolver: ad2s1210: add phase lock range support
> staging: iio: resolver: ad2s1210: add triggered buffer support
> staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs
> staging: iio: resolver: ad2s1210: convert LOS threshold to event attr
> staging: iio: resolver: ad2s1210: convert DOS overrange threshold to event attr
> staging: iio: resolver: ad2s1210: convert DOS mismatch threshold to event attr
> staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs
> staging: iio: resolver: ad2s1210: implement fault events
> staging: iio: resolver: ad2s1210: add label attribute support
>
> .../bindings/iio/resolver/adi,ad2s1210.yaml | 177 +++
> .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 +
> drivers/staging/iio/resolver/Kconfig | 1 +
> drivers/staging/iio/resolver/ad2s1210.c | 1583 +++++++++++++++-----
> 4 files changed, 1391 insertions(+), 397 deletions(-)
> ---
> base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec
> change-id: 20230925-ad2s1210-mainline-2791ef75e386
>

In the end, this is what sysfs looks like:

root@analog:~# tree /sys/bus/iio/devices/iio\:device1/
/sys/bus/iio/devices/iio:device1/
├── buffer
│ ├── data_available
│ ├── direction
│ ├── enable
│ ├── length
│ └── watermark
├── buffer0
│ ├── data_available
│ ├── direction
│ ├── enable
│ ├── in_angl0_en
│ ├── in_angl0_index
│ ├── in_angl0_type
│ ├── in_anglvel0_en
│ ├── in_anglvel0_index
│ ├── in_anglvel0_type
│ ├── in_timestamp_en
│ ├── in_timestamp_index
│ ├── in_timestamp_type
│ ├── length
│ └── watermark
├── current_timestamp_clock
├── dev
├── events
│ ├── in_altvoltage0_mag_reset_max
│ ├── in_altvoltage0_mag_reset_max_available
│ ├── in_altvoltage0_mag_reset_min
│ ├── in_altvoltage0_mag_reset_min_available
│ ├── in_altvoltage0_mag_value
│ ├── in_altvoltage0_mag_value_available
│ ├── in_altvoltage0_thresh_falling_value
│ ├── in_altvoltage0_thresh_falling_value_available
│ ├── in_altvoltage0_thresh_rising_value
│ ├── in_altvoltage0_thresh_rising_value_available
│ ├── in_angl1_thresh_rising_hysteresis
│ ├── in_angl1_thresh_rising_hysteresis_available
│ ├── in_angl1_thresh_rising_value
│ ├── in_angl1_thresh_rising_value_available
│ ├── in_phase0_mag_value
│ └── in_phase0_mag_value_available
├── fault
├── in_altvoltage0_label
├── in_altvoltage1_x_label
├── in_altvoltage1_y_label
├── in_angl0_hysteresis
├── in_angl0_hysteresis_available
├── in_angl0_label
├── in_angl0_raw
├── in_angl0_scale
├── in_angl1_label
├── in_anglvel0_label
├── in_anglvel0_raw
├── in_anglvel0_scale
├── in_phase0_label
├── name
├── of_node -> ../../../../../../../../firmware/devicetree/base/axi/spi@e0006000/ad2s1210@0
├── out_altvoltage0_frequency
├── out_altvoltage0_frequency_available
├── out_altvoltage0_label
├── power
│ ├── autosuspend_delay_ms
│ ├── control
│ ├── runtime_active_time
│ ├── runtime_status
│ └── runtime_suspended_time
├── scan_elements
│ ├── in_angl0_en
│ ├── in_angl0_index
│ ├── in_angl0_type
│ ├── in_anglvel0_en
│ ├── in_anglvel0_index
│ ├── in_anglvel0_type
│ ├── in_timestamp_en
│ ├── in_timestamp_index
│ └── in_timestamp_type
├── subsystem -> ../../../../../../../../bus/iio
├── trigger
│ └── current_trigger
├── uevent
└── waiting_for_supplier

8 directories, 72 files

And this is what the output of iio_info looks like:
(note: iio_info does not currently support events so those
attributes are not visible here)

iio:device1: ad2s1210 (buffer capable)
9 channels found:
angl0: (input, index: 0, format: be:U16/16>>0)
5 channel-specific attributes found:
attr 0: hysteresis value: 1
attr 1: hysteresis_available value: 0 1
attr 2: label value: position
attr 3: raw value: 12578
attr 4: scale value: 0.000095874
anglvel0: (input, index: 1, format: be:S16/16>>0)
3 channel-specific attributes found:
attr 0: label value: velocity
attr 1: raw value: 5
attr 2: scale value: 0.023968449
timestamp: (input, index: 2, format: le:S64/64>>0)
phase0: (input)
1 channel-specific attributes found:
attr 0: label value: synthetic reference
altvoltage0: (output)
3 channel-specific attributes found:
attr 0: frequency value: 10000
attr 1: frequency_available value: [2000 250 20000]
attr 2: label value: excitation
angl1: (input)
1 channel-specific attributes found:
attr 0: label value: tracking error
altvoltage1_y: (input)
1 channel-specific attributes found:
attr 0: label value: sine
altvoltage0: (input)
1 channel-specific attributes found:
attr 0: label value: monitor signal
altvoltage1_x: (input)
1 channel-specific attributes found:
attr 0: label value: cosine
3 device-specific attributes found:
attr 0: current_timestamp_clock value: realtime
attr 1: fault value: 0x00
attr 2: waiting_for_supplier value: 0
2 buffer-specific attributes found:
attr 0: data_available value: 0
attr 1: direction value: in
1 debug attributes found:
debug attr 0: direct_reg_access ERROR: Input/output error (5)
Current trigger: trigger0(test)