Move AD2S1210 driver out of staging

[pamolloy]

Move drivers/staging/iio/resolver/ad2s1210.c out of staging and integrate into existing drivers as necessary.

Tasks

• Review existing driver and clean-up as necessary
• If not already setup, setup hardware for testing
• Test driver on above hardware
• Push to ADI Github repository for CI/CD
• Open PR on ADI linux repository for review
• Submit upstream to mainline kernel mailing list
• Make changes based on a review and submit new series as necessary
• Document driver and part on ADI Wiki. Note that only serial interface is supported

Dependencies

• adi-ses/sea-misc/issues/2
• Support for AD2S1210 evaluation board on ZedBoard hdl#1147
• adi-ses/sea-misc/issues/16

[apelete]

Internal review ongoing.

[dlech]

Copying the big-picture questions/suggestions from the PR here since they are probably better discussed in an issue (see #2261 for more context):

Ideas for additional changes:

• Don't set indexed flag. This will remove the 0 from the channel names. Since there is only one channel of each type, the index isn't needed, but this would be a breaking change for users of the staging driver.
• resolution seems like a better name than bits for the attribute that gets and sets the resolution, but this would be a breaking change to users of the staging driver.
• excitation_frequency seems like a better name than fexcit for the attribute that gets and sets the excitation frequency, but this would be a breaking change to users of the staging driver.
• It would be nice if all of the threshold attributes used values in millivolts or millidegrees instead of the raw register value so that userspace doesn't have to do the conversion, but this would be a breaking change to users of the staging driver.
• Add an enable_hysteresis attribute for getting and setting the hysteresis flag in the control register. Do users need this feature?
• Add a phase_lock_range attribute to get and set the phase lock range flag in the control register. Do users need this feature?
• Make mode (and resolution?) gpios optional. It looks like there was some effort to do this in the reverted merge commits but it was a bit buggy. We need to define the supported scenarios first.
• Would it make more sense to make "sample-gpios" active low in the devicetree and invert the logic in the driver?
• We could do some more refactoring to reduce duplicate code when setting the excitation frequency.
• It could be useful to do something with the fault gpios so that faults could be detected out of band instead of polling the fault register.
• Need to implement IIO buffer (and trigger?) in order to use with IIO oscilloscope.

[mhennerich]

Copying the big-picture questions/suggestions from the PR here since they are probably better discussed in an issue (see #2261 for more context):

Ideas for additional changes:

• Don't set indexed flag. This will remove the 0 from the channel names. Since there is only one channel of each type, the index isn't needed, but this would be a breaking change for users of the staging driver.

While un-staging this driver, there might be other braking changes required. While they are in general undesirable, now is the time to do those. And I'm almost sure mainline review will likely as for those as well.

• resolution seems like a better name than bits for the attribute that gets and sets the resolution, but this would be a breaking change to users of the staging driver.

I think the proper IIO way is to use SCALE for that. Make SCALE writable and provide options via SCALE_AVAILABLE to chose different resolutions.

• excitation_frequency seems like a better name than fexcit for the attribute that gets and sets the excitation frequency, but this would be a breaking change to users of the staging driver.

excitation_frequency is better than fexcit. In general this needs to be discussed on the IIO mailing list.

• It would be nice if all of the threshold attributes used values in millivolts or millidegrees instead of the raw register value so that userspace doesn't have to do the conversion, but this would be a breaking change to users of the staging driver.

I think this stuff should be moved to the IIO event ABI. In this case they use RAW values and SCALE is used to get to proper units.

• Add an enable_hysteresis attribute for getting and setting the hysteresis flag in the control register. Do users need this feature?

IIO_EV_INFO_HYSTERESIS ?

• Add a phase_lock_range attribute to get and set the phase lock range flag in the control register. Do users need this feature?

I think we need to be able to control that. Proper naming should be subject of IIO mailing list discussion.

• Make mode (and resolution?) gpios optional. It looks like there was some effort to do this in the reverted merge commits but it was a bit buggy. We need to define the supported scenarios first.

ACK - I think we should support both normal and configuration mode.

• Would it make more sense to make "sample-gpios" active low in the devicetree and invert the logic in the driver?

/SAMPLE is active low signal. So I think this would make sense.

• We could do some more refactoring to reduce duplicate code when setting the excitation frequency.

Sure - mailing list review might ask you this anyways.

• It could be useful to do something with the fault gpios so that faults could be detected out of band instead of polling the fault register.

Yes, Optional Interrupt support is always desirable.

• Need to implement IIO buffer (and trigger?) in order to use with IIO oscilloscope.

Yes, triggered buffer support is always desirable.

Adding @nunojsa for comments as well.

[dlech]

• Add an enable_hysteresis attribute for getting and setting the hysteresis flag in the control register. Do users need this feature?

IIO_EV_INFO_HYSTERESIS ?

In this case, the hysteresis is for damping noise in the position value, so I don't think it works to link it to an event.

IIO_CHAN_INFO_HYSTERESIS might work though (or IIO_CHAN_INFO_DEBOUNCE_COUNT?).

[dlech]

• It would be nice if all of the threshold attributes used values in millivolts or millidegrees instead of the raw register value so that userspace doesn't have to do the conversion, but this would be a breaking change to users of the staging driver.

I think this stuff should be moved to the IIO event ABI. In this case they use RAW values and SCALE is used to get to proper units.

All of these thresholds are for internal signals in the AD2S1210 chip. Are you saying we should add channels for these internal signals where we can't read the value but can have events?

[dlech]

• resolution seems like a better name than bits for the attribute that gets and sets the resolution, but this would be a breaking change to users of the staging driver.

I think the proper IIO way is to use SCALE for that. Make SCALE writable and provide options via SCALE_AVAILABLE to chose different resolutions.

This sounds like a good idea, but also tricky because there is only one resolution setting for everything. If you change the scale of the position channel, it will also change the scale of the angle channel as well as the LOT threshold scale. All of these have different scales (and types) so we can't use shared_by_*. I have a feeling this could confuse usespace programs that don't expect SCALE to change after they read it (e.g. a userspace program reads all of the scales, then writes the scale for the position channel - this also changes the scale of the velocity channel but the userspace program doesn't expect this and keeps using the old value it already read).

Perhaps another way to look at this is that the resolution setting depends on the application. It needs to be chosen to fit the maximum speed and/or the minimum precision required for the application. So maybe this is something for devicetree instead? This seems like it is in a gray area between configuration (not appropriate use of devicetree) and describing the hardware (appropriate use of devcietree) but there are properties like spi-max-frequency already. It would certainly simplify things in the driver if we did not have to allow for the case that the resolution can be changed at runtime.

Also, regarding the suggestion:

Make mode (and resolution?) gpios optional.

We will already need a device tree property to indicate how the resolution inputs are hard-wired in the case when there are no gpios connected to them. So maybe we just always require this property anyway and only use a fixed resolution described by the devicetree?

[mhennerich]

• Add an enable_hysteresis attribute for getting and setting the hysteresis flag in the control register. Do users need this feature?

IIO_EV_INFO_HYSTERESIS ?

In this case, the hysteresis is for damping noise in the position value, so I don't think it works to link it to an event.

IIO_CHAN_INFO_HYSTERESIS might work though (or IIO_CHAN_INFO_DEBOUNCE_COUNT?).

That would work.

[mhennerich]

• It would be nice if all of the threshold attributes used values in millivolts or millidegrees instead of the raw register value so that userspace doesn't have to do the conversion, but this would be a breaking change to users of the staging driver.

I think this stuff should be moved to the IIO event ABI. In this case they use RAW values and SCALE is used to get to proper units.

All of these thresholds are for internal signals in the AD2S1210 chip. Are you saying we should add channels for these internal signals where we can't read the value but can have events?

Maybe it's a good idea to propose a device monitoring channel type?

[mhennerich]

• resolution seems like a better name than bits for the attribute that gets and sets the resolution, but this would be a breaking change to users of the staging driver.

I think the proper IIO way is to use SCALE for that. Make SCALE writable and provide options via SCALE_AVAILABLE to chose different resolutions.

This sounds like a good idea, but also tricky because there is only one resolution setting for everything. If you change the scale of the position channel, it will also change the scale of the angle channel as well as the LOT threshold scale. All of these have different scales (and types) so we can't use shared_by_*. I have a feeling this could confuse usespace programs that don't expect SCALE to change after they read it (e.g. a userspace program reads all of the scales, then writes the scale for the position channel - this also changes the scale of the velocity channel but the userspace program doesn't expect this and keeps using the old value it already read).

Perhaps another way to look at this is that the resolution setting depends on the application. It needs to be chosen to fit the maximum speed and/or the minimum precision required for the application. So maybe this is something for devicetree instead? This seems like it is in a gray area between configuration (not appropriate use of devicetree) and describing the hardware (appropriate use of devcietree) but there are properties like spi-max-frequency already. It would certainly simplify things in the driver if we did not have to allow for the case that the resolution can be changed at runtime.

Fair point

Also, regarding the suggestion:

Make mode (and resolution?) gpios optional.

We will already need a device tree property to indicate how the resolution inputs are hard-wired in the case when there are no gpios connected to them. So maybe we just always require this property anyway and only use a fixed resolution described by the devicetree?

This would work for me as well.

[dlech]

• It would be nice if all of the threshold attributes used values in millivolts or millidegrees instead of the raw register value so that userspace doesn't have to do the conversion, but this would be a breaking change to users of the staging driver.

I think this stuff should be moved to the IIO event ABI. In this case they use RAW values and SCALE is used to get to proper units.

All of these thresholds are for internal signals in the AD2S1210 chip. Are you saying we should add channels for these internal signals where we can't read the value but can have events?

Maybe it's a good idea to propose a device monitoring channel type?

I also came across #341 which adds a new IIO_FLAGS channel type which seems like the same sort of thing. Will look into it more.

[mhennerich]

• It would be nice if all of the threshold attributes used values in millivolts or millidegrees instead of the raw register value so that userspace doesn't have to do the conversion, but this would be a breaking change to users of the staging driver.

I think this stuff should be moved to the IIO event ABI. In this case they use RAW values and SCALE is used to get to proper units.

All of these thresholds are for internal signals in the AD2S1210 chip. Are you saying we should add channels for these internal signals where we can't read the value but can have events?

Maybe it's a good idea to propose a device monitoring channel type?

I also came across #341 which adds a new IIO_FLAGS channel type which seems like the same sort of thing. Will look into it more.

Well - whatever we end up doing it needs to be accepted mainline.

[dlech]

Well - whatever we end up doing it needs to be accepted mainline.

Upstream recommended using events for the faults so this is the direction I went. The iio maintainer has requested feedback from "someone from Analog" to make sure the proposed changes make sense: https://lore.kernel.org/linux-iio/20230930163209.17ee0020@jic23-huawei/

Could you please take a look @mhennerich or @nunojsa ?

[dlech]

To summarize the event patches, in the PATCH v3 series, I proposed the following event and register mapping:

Fault bit	Fault description	IIO channel	IIO channel label	IIO event type	IIO event direction	IIO event info	Config register
D7	Sine/cosine inputs clipped	altvoltage1_x_or_y	sine/cosine	mag	none	N/A	N/A
D6	Sine/cosine inputs below LOS threshold	altvoltage0	monitor signal	thresh	falling	value	LOS Threshold
D5	Sine/cosine inputs exceed DOS overrange threshold	altvoltage0	monitor signal	thresh	rising	value	DOS Overrange Threshold
D4	Sine/cosine inputs exceed DOS mismatch threshold	altvoltage0	monitor signal	mag	none	value	DOS Mismatch Threshold
D3	Tracking error exceeds LOT threshold	angl1	tracking error	thresh	rising	value	LOT High Threshold
		angl1	tracking error	thresh	rising	hysteresis	LOT Low Threshold
D2	Velocity exceeds maximum tracking rate	anglvel0	velocity	thresh	rising	N/A	N/A
D1	Phase error exceeds phase lock range	phase0	synthetic reference	mag	none	value	Control: phase lock range bit
D0	Configuration parity error	N/A

What Jonathan has suggested is:

• dropping the x_or_y on the sine/cosine channels and emitting duplicate events on the individual input channels
• dropping the "monitor signal" channel
  • LOS and DOS overrange get moved to sine/cosine channels and result in duplicate events and duplicate _value attributes
  • creating new altvoltage1-altvoltage0 differential channel for DOS mismatch
• mag, none should probably be mag, rising for phase error and DOS mismatch.

Getting rid of the x_or_y is clearly a needed change since it is known to be problematic (according to Jonathan).

IMHO, getting rid of the "monitor signal" channel isn't really much of an improvement since it doesn't make for fewer channels or fewer events per channel and we still have to have other "internal" (not really in or out) channels for other events. But really it would be fine either way, I think.

[mhennerich]

I really don't have a strong preference here either. One or the other way this needs to be properly documented to make sense and being useful. I understand Jonathan's desire to avoid those 'all or nothing/topsy-turvy' channels, and instead modeling the underlaying behavior as close as possible using existing types. I agree it doesn't make it simpler in this case.