Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AD2S1210 updates #2261

Merged
merged 47 commits into from
Oct 31, 2023
Merged

AD2S1210 updates #2261

merged 47 commits into from
Oct 31, 2023

Conversation

dlech
Copy link
Collaborator

@dlech dlech commented Sep 11, 2023 •
edited
Loading

This is working towards getting the AD2S1210 resolver driver out of staging.

Overview:

  • The first few commits are getting the local version of the driver to exactly match mainline. This way new patches will apply cleanly and not have to be reworked before submitting upstream.
    • The first commit reverts some changes that were never merged upstream
    • All upstream patches since 5.15 are cherry-picked
    • One extra patch to backport IIO_DMA_MINALIGN (similar to some other drivers here)
  • The remaining commits are fixes and improvements
    • Fixed bugs in probe function.
    • fclkin attribute is removed and replaced with devicetree binding for getting clock rate.
    • control attribute is removed and replaced with debugfs register access.
    • gpio device tree bindings are replaced with something that is more likely to be accepted upstream

More detailed explanations of the changes can be found in the commit messages.

Ideas for additional changes:

  • 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.

Remaining tasks before submitting upstream:

  • add documentation for sysfs attributes
  • more testing is needed to ensure iio channels are working for all supported configuration combinations

Issue: #2234

@dlech
Copy link
Collaborator Author

dlech commented Sep 11, 2023

FYI, the checkpatch CI failure is due to the cherry-picked upstream commits. New patches look like they passed.

dt_bindings_check CI failure is due to missing dt-doc-validate so seems like something not caused by these changes.

nunojsa
nunojsa reviewed Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fairly good to me. It needs some re-arrangement of the commits (some of them are also including some multiple changes - not sure if the maintainer will like it much) but I guess you can start the upstreaming process...

Some things that I would like to mention:

  • I see the bindings CI job failed (need to see if there's anything we need to update in there). So, please locally make sure that the bindings are ok.
  • Other things that I saw that you can potentially clean are: remove of_match_ptr(), apparently no need for spi_set_drvdata() and spi_setup() is also usually not needed (called already by the spi core)

Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml Show resolved Hide resolved
drivers/staging/iio/resolver/ad2s1210.c Outdated
ret = ad2s1210_set_mode(st, MOD_CONFIG);
if (ret < 0)
return ret;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated change... This one and the above

drivers/staging/iio/resolver/ad2s1210.c Outdated
ad2s1210_set_resolution_pin(st);
ret = ad2s1210_set_resolution_gpios(st, udata);
if (ret < 0)
goto error_ret;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

drivers/staging/iio/resolver/ad2s1210.c Outdated Show resolved Hide resolved
drivers/staging/iio/resolver/ad2s1210.c Outdated Show resolved Hide resolved
drivers/staging/iio/resolver/ad2s1210.c Outdated Show resolved Hide resolved
drivers/staging/iio/resolver/ad2s1210.c Outdated Show resolved Hide resolved
drivers/staging/iio/resolver/ad2s1210.c Outdated Show resolved Hide resolved
drivers/staging/iio/resolver/ad2s1210.c Outdated
ad2s1210_initial(st);

return 0;
return devm_iio_device_register(&spi->dev, indio_dev);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure if staging works like normal flow. But, if so, this patch would be one the first patch that touches the driver (fixes come first because of backporting - not sure if we really backport staging fixes). Also, a Fixes tag might be needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. This is the first patch in the PR that isn't in 6.6 mainline already, so will be the first in the series submitted upstream.

drivers/staging/iio/resolver/ad2s1210.c Outdated Show resolved Hide resolved
@dlech
Copy link
Collaborator Author

dlech commented Sep 12, 2023

(some of them are also including some multiple changes - not sure if the maintainer will like it much)

Agreed. My first attempt was 50 patches which seemed like to much the other direction. So I will try to find some balance in between.

@pamolloy pamolloy added this to the BayLibre September Delivery milestone Sep 12, 2023
@dlech dlech mentioned this pull request Sep 12, 2023
Closed
8 tasks
@dlech dlech force-pushed the ad2s1210-updates branch 2 times, most recently from c0d0601 to 369166a Compare September 12, 2023 23:10
@dlech
Copy link
Collaborator Author

dlech commented Sep 12, 2023

Updates:

  • Added new patch with sysfs documentation.
  • Added new patch with devicetree for zed+eval board.
  • Fixed device tree bindings patch subject.
  • Fixed order of gpios in tables describing A0/A1/RES0/RES1 in dt-bindings.
  • Added dependence on COMMON_CLK in Kconfig.
  • Addressed other feedback comments as indicated above.

@dbogdan dbogdan requested a review from a team September 13, 2023 06:40
@dlech dlech marked this pull request as draft September 13, 2023 13:31
@dlech
Copy link
Collaborator Author

dlech commented Sep 13, 2023 •
edited
Loading

FYI for reviewers, per #2234 (comment) it looks like there will be a bit more work here before ready for review again. Although the patches here are unlikely to change much if you want to have a look anyway.

@dlech
Copy link
Collaborator Author

dlech commented Sep 13, 2023

Updates:

  • added patches to refactor and rename excitation frequency attribute and implementation
  • added patch to enable getting/setting hysteresis in the control register

@dlech
Copy link
Collaborator Author

dlech commented Sep 14, 2023

Updates:

  • changed clock name from "fclkin" to "clkin"
  • Added adi,resolution device tree property
  • New patch to implement phase lock range attributes
  • A couple new small fix patches to fix issues in the existing mainline driver
  • New patch to support the channel scale attribute

@dlech dlech force-pushed the ad2s1210-updates branch 2 times, most recently from 3d80ed8 to cbad00f Compare September 15, 2023 21:23
@dlech
Copy link
Collaborator Author

dlech commented Sep 15, 2023

Updates:

  • use regmap for register access
  • new patch for triggered buffer support

@dlech dlech force-pushed the ad2s1210-updates branch 5 times, most recently from 36a2d94 to 71da83b Compare September 19, 2023 14:32
@apelete apelete mentioned this pull request Sep 20, 2023
Merged
@dlech dlech force-pushed the ad2s1210-updates branch 2 times, most recently from d9fde79 to 12578cc Compare October 11, 2023 20:36
@dlech
Copy link
Collaborator Author

dlech commented Oct 16, 2023

Status update: There are just a few more patches pending upstream. Then I will add them here and remove the draft status.

dlech and others added 3 commits October 30, 2023 11:45
@dlech
staging: iio: ad2s1210: revert merge commits
78b7999 
This reverts ADI merge commits 67d8979 "Merge tag 'xilinx-v2021.1'
of https://github.com/Xilinx/linux-xlnx.git" and b6c8aa0 "Merge tag
'xilinx-v2020.1' into master-xilinx-v2020.1".

This gets the driver back to the state it is in the mainline kernel. A
different solution will be proposed to support optional gpios.

Signed-off-by: David Lechner <[email protected]>
@nunojsa @dlech
staging: iio: resolver: ads1210: fix config mode
508fa17 
As stated in the device datasheet [1], bits a0 and a1 have to be set to
1 for the configuration mode.

[1]: https://www.analog.com/media/en/technical-documentation/data-sheets/ad2s1210.pdf

Fixes: b19e9ad ("staging:iio:resolver:ad2s1210 general driver cleanup")
Cc: stable <[email protected]>
Signed-off-by: Nuno Sá <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
@robherring @dlech
staging: iio: resolver: ad2s1210: Add explicit include for of.h
e9e19fa 
With linux/acpi.h no longer implicitly including of.h, add an explicit
include of of.h to fix the following error:

drivers/staging/iio/resolver/ad2s1210.c:706:21: error: implicit declaration of function 'of_match_ptr' is invalid in C99 [-Werror,-Wimplicit-function-declaration]

Acked-by: Jonathan Cameron <[email protected]>
Reviewed-by: Jacob Keller <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Rob Herring <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
dlech and others added 10 commits October 30, 2023 11:45
@dlech
staging: iio: resolver: ad2s1210: convert DOS mismatch threshold to e…
ece0564 
…vent attr

The AD2S1210 has a programmable threshold for the degradation of signal
(DOS) mismatch fault. This fault is triggered when the difference in
voltage between the sine and cosine inputs exceeds the threshold. In
other words, when the magnitude of sine and cosine inputs are equal,
the AC component of the monitor signal is zero and when the magnitudes
of the sine and cosine inputs are not equal, the AC component of the
monitor signal is the difference between the sine and cosine inputs.
So the fault occurs when the magnitude of the AC component of the
monitor signal exceeds the DOS mismatch threshold voltage.

This patch converts the custom device DOS mismatch threshold attribute
to an event magnitude attribute on the monitor signal channel.

The attribute now uses millivolts instead of the raw register value in
accordance with the IIO ABI.

Emitting the event will be implemented in a later patch.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs
5b0fe5c 
The AD2S1210 has a programmable threshold for the degradation of signal
(DOS) mismatch fault. This fault is triggered when the difference in
amplitude between the sine and cosine inputs exceeds the threshold.

The DOS reset min/max registers on the chip provide initial values
for internal tracking of the min/max of the monitor signal after the
fault register is cleared.

This patch converts the custom device DOS reset min/max threshold
attributes custom event attributes on the monitor signal channel.

The attributes now use millivolts instead of the raw register value in
accordance with the IIO ABI.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
iio: event: add optional event label support
47094aa 
This adds a new optional field to struct iio_info to allow drivers to
specify a label for the event. This is useful for cases where there are
many events or the event attribute name is not descriptive enough or
where an event doesn't have any other attributes.

The implementation is based on the existing label support for channels.
So either all events of a device have a label attribute or none do.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
staging: iio: resolver: ad2s1210: implement fault events
62a0e05 
When reading the position and velocity on the AD2S1210, there is also a
3rd byte following the two data bytes that contains the fault flag bits.
This patch adds support for reading this byte and generating events when
faults occur.

The faults are mapped to various channels and event type in order to
have a unique event for each fault.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
staging: iio: resolver: ad2s1210: add register/fault support summary
23d7440 
The ad2s1210 driver shoe-horns the register and fault support into IIO
events. The mapping between the registers/faults and the events is not
obvious. To save users from having to read the entire driver to figure
out how to use it, add a summary of the register/fault support to the
top of the file.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
staging: iio: resolver: ad2s1210: add label attribute support
da106b6 
The ad2s1210 resolver driver has quite a few channels, mostly for
internal signals for event support. This makes it difficult to know
which channel is which. This patch adds a label attribute to the
channels to make it easier to identify them.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
staging: iio: resolver: ad2s1210: remove fault attribute
84d7cc5 
Faults have been converted to events and we are now polling the fault
register each time we read a sample, so we no longer need the fault
attribute.

This attribute was not suitable for promotion out of staging anyway
since it was returning multiple values in a single attribute.

The fault clearing feature should not be needed unless we need to
support the fault output pins on the chip which is not currently
supported. So we can add this feature back in if we need it later.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
staging: iio: resolver: ad2s1210: refactor sample toggle
12d1ead 
This refactors the sample line toggle in the ad2s1210 resolver driver
to a separate function. The sample has some timing requirements, so
this ensures that it is always done the same way, both in the existing
call sites and any future usage.

Previously, the sample line was kept on for the duration of the read,
but this is not necessary. Data is latched in on the rising edge and
after the specified delay the state of the sample line does not matter.

Signed-off-by: David Lechner <[email protected]>
@dlech
staging: iio: resolver: ad2s1210: clear faults after soft reset
e3e270e 
When a software reset is performed on the AD2S1210 to make the selected
excitation frequency take effect, it always triggers faults on the
input signals because the output signal is interrupted momentarily.
So we need to clear the faults after the software reset to avoid
triggering fault events the next time a sample is read.

The datasheet specifies a time t[track] in Table 27 that specifies the
settle time in milliseconds after a reset depending on the selected
resolution. This is used in the driver to add an appropriate delay.

Signed-off-by: David Lechner <[email protected]>
@dlech
locking: Introduce __cleanup() based infrastructure
97c72b2 
Use __attribute__((__cleanup__(func))) to build:

 - simple auto-release pointers using __free()

 - 'classes' with constructor and destructor semantics for
   scope-based resource management.

 - lock guards based on the above classes.

Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/20230612093537.614161713%40infradead.org
Peter Zijlstra and others added 8 commits October 30, 2023 12:19
@dlech
kbuild: Drop -Wdeclaration-after-statement
d5b28e2 
With the advent on scope-based resource management it comes really
tedious to abide by the contraints of -Wdeclaration-after-statement.

It will still be recommeneded to place declarations at the start of a
scope where possible, but it will no longer be enforced.

Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/CAHk-%3Dwi-RyoUhbChiVaJZoZXheAwnJ7OO%3DGxe85BkPAd93TwDA%40mail.gmail.com
@dlech
staging: iio: resolver: ad2s1210: simplify code with guard(mutex)
8528283 
We can simplify the code and get rid of most of the gotos by using
guard(mutex) from cleanup.h.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
iio: resolver: ad2s1210: move out of staging
290d1b6 
This moves the ad2s1210 resolver driver out of staging. The driver has
been fixed up and is ready to graduate.

Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
iio: resolver: ad2s1210: remove DRV_NAME macro
342ea2f 
The DRV_NAME macro is only used in one place in the ad2s1210 driver and
is not really needed so let's remove it.

Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
iio: resolver: ad2s1210: remove of_match_ptr()
0970e11 
To be consistent with the rest of iio, remove of_match_ptr(). It does
not do anything useful here.

Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: David Lechner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
iio: resolver: ad2s1210: add support for adi,fixed-mode
bc8c5d7 
It is possible to use the AD2S1210 with hardwired mode pins (A0 and A1).
According to the devicetree bindings, in this case the adi,fixed-mode
property will specify which of the 3 possible modes the mode pins are
hardwired for and the gpio-modes property is not allowed.

This adds support for the case where the mode pins are hardwired for
config mode. In this configuration, the position and value must be read
from the config register.

The case of hardwired position or velocity mode is not supported as
there would be no way to configure the device.

Signed-off-by: David Lechner <[email protected]>
Reviewed-by: Nuno Sa <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
iio: resolver: ad2s1210: add reset gpio support
56e3c1d 
This adds support for the optional reset gpio to the ad2s1210 resolver
driver. If the gpio is present in the device tree, it is toggled during
driver probe before the reset of the device initialization. As per the
devicetree bindings, it is expected for the gpio to configured as active
low.

Suggested-by: Michael Hennerich <[email protected]>
Signed-off-by: David Lechner <[email protected]>
Acked-by: Michael Hennerich <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
@dlech
arm: dts: zynq-adv7511: ad2s1210: add devicetree for ad2s1210 eval
087e7af 
This offers basic support for EVAL-AD2S1210SDZ on the ZedBoard.

Co-developed-by: Apelete Seketeli <[email protected]>
Signed-off-by: Apelete Seketeli <[email protected]>
Signed-off-by: David Lechner <[email protected]>
@dlech
Copy link
Collaborator Author

dlech commented Oct 30, 2023 •
edited
Loading

I have rebased on the current master and added the remaining patches that were accepted upstream.

To summarize:

  • The first comment reverts all ADI tree changes to get the driver back exactly equal to the mainline staging driver as it was in upstream v6.1.
  • Then all but the last commit have been cherry-picked from upstream.
    • The first 6 were made between 6.2 and 6.6.
    • Most of the remaining ones were the updates done by BayLibre. Most will be landing in 6.7 - the last 2 probably not until 6.8.
    • This is also picking up 2 patches that introduced linux/cleanup.h since that is used in some of the later ad2s1210 patches.
  • The final patch is adding the device tree for the AD2S1210 Eval board connected to a ZedBoard. (This is the only patch that hasn't been through upstream review and acceptance.)

Note: the CI failures (DCO and checkpatch) are caused by some of the cherry-picked upstream commits, so should be safe to ignore.

Also did a quick smoke test to make sure this is still compiling and is reading data on our ZedBoard.

@dlech dlech marked this pull request as ready for review October 30, 2023 17:28
@dlech dlech requested a review from nunojsa October 30, 2023 17:28
@dlech dlech mentioned this pull request Oct 30, 2023
Merged
@nunojsa
Copy link
Collaborator

nunojsa commented Oct 31, 2023

This is also picking up 2 patches that introduced linux/cleanup.h since that is used in some of the later ad2s1210 patches.

How much are you using these new macros? Normally I'm not too keen in backporting things like this but bah, this should also not give me much pain when updating to a newer kernel...

@dlech
Copy link
Collaborator Author

dlech commented Oct 31, 2023

How much are you using these new macros? Normally I'm not too keen in backporting things like this but bah, this should also not give me much pain when updating to a newer kernel...

This is the only place so far. But the cleanup.h patches applied cleanly so back-porting it should be less trouble than reworking the last few patches here.

@dlech dlech merged commit ea304c1 into analogdevicesinc:master Oct 31, 2023
9 of 12 checks passed
@dlech dlech deleted the ad2s1210-updates branch October 31, 2023 14:27
@dlech dlech linked an issue Oct 31, 2023 that may be closed by this pull request
Move AD2S1210 driver out of staging #2234
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nunojsa nunojsa nunojsa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
BayLibre September Delivery
Development

Successfully merging this pull request may close these issues.

Move AD2S1210 driver out of staging
4 participants
@dlech @nunojsa @pamolloy @robherring