AD7768 upstream - #3146
Conversation
3b730a1 to
f160b71
Compare
fec0c1a to
5b36ef0
Compare
f9b6ebc to
ca4d3a2
Compare
0041a58 to
efef9aa
Compare
|
Hey @nunojsa - could you please have a look this one? |
Sure. I asked first for llm-review. Let's see what comes out after that. |
LLM reviewThis series adds support for the AD7768/AD7768-4 8/4-channel simultaneous-sampling run: 25312690089
|
|
Changelog after LLM Review fixes:
|
nunojsa
left a comment
nunojsa
left a comment
There was a problem hiding this comment.
Alright! Plenty of comments already :). Will also trigger a llm review
| maxItems: 1 | ||
| description: | | ||
| GPIO reset pin. | ||
| Either resets or reset-gpios should be specified, not both. |
There was a problem hiding this comment.
missing blank line. And why the two resets?
| Power mode selection: | ||
| 0 - Low power mode | ||
| 1 - Median mode | ||
| 2 - Fast mode |
There was a problem hiding this comment.
I think we can support the above with linux PM subsystem. What comes to mind:
runtime PM: Toggle between median and fast mode
system PM: toggle between low and median mode.
Something to have in mind
There was a problem hiding this comment.
If we are still using this, use strings and then drop the bindings header. I'm not 100% sure but I think they are not encouraged that much
There was a problem hiding this comment.
With my comments, we might not need this header
LLM reviewThis series adds support for the AD7768 (8-channel) and AD7768-4 (4-channel) simultaneous-sampling 24-bit Sigma-Delta ADCs, with IIO backend CRC support for the AXI ADC. run: 27197818159
|
|
Changelog: dt-bindings: iio: adc: Add AD7768
include/dt-bindings/iio/adc/adi,ad7768.h
drivers/iio/adc/Kconfig
MAINTAINERS
drivers/iio/adc/ad7768.c
|
|
Changelog: |
LLM reviewThis series adds support for the Analog Devices AD7768 and AD7768-4 simultaneous sampling ADC devices with complete driver implementation, device tree bindings, documentation, and CRC backend support. run: 27742069075 Series OverviewThe 5-commit series implements:
Compilation StatusSuccessfully compiled for x86_64 and arm64 without errors. Generated modules: ad7768.o (29.7 KB), ad7768-gpio.o (9.7 KB). Design Highlights
Checkpatch IssuesAll commits have checkpatch warnings that are false positives in git diff/format-patch mode:
These do not affect functionality. Minor Issues Found
Verification DataCompilation testing: Code analysis:
Code Quality
Recommendation: Ready for acceptance. Commit message typos are optional cosmetic fixes. |
|
Changelog:
|
nunojsa
left a comment
nunojsa
left a comment
There was a problem hiding this comment.
Here it goes another round. I guess it's plenty already
| Power mode selection: | ||
| 0 - Low power mode | ||
| 1 - Median mode | ||
| 2 - Fast mode |
There was a problem hiding this comment.
If we are still using this, use strings and then drop the bindings header. I'm not 100% sure but I think they are not encouraged that much
| enum: [1, 2, 4, 8] | ||
| description: | ||
| Number of data output lines used for serial interface. | ||
| AD7768 supports 1, 2, or 8 lines. AD7768-4 supports 1 or 4 lines. |
There was a problem hiding this comment.
If I'm not missing nothing, you can have the above as 1,2 and 8 and then no need for the else branch in the allof condition
There was a problem hiding this comment.
the dtbs check fails with this structure..
adc@1 (adi,ad7768-4): adi,data-lines-number: 4 is not one of [1, 2, 8]
|
|
||
| MODULE_AUTHOR("Janani Sunil <janani.sunil@analog.com>"); | ||
| MODULE_DESCRIPTION("Analog Devices AD7768 GPIO auxiliary driver"); | ||
| MODULE_LICENSE("GPL"); |
There was a problem hiding this comment.
This does not belong in here... See this series:
https://lore.kernel.org/linux-hwmon/20260502-ltc4283-support-v13-0-1c206542e652@analog.com/
Also needs to be in it's own patch
| ret = regmap_update_bits(st->data->regmap, AD7768_REG_GPIO_CONTROL, | ||
| AD7768_GPIO_UGPIO_ENABLE, | ||
| AD7768_GPIO_UGPIO_ENABLE); | ||
| mutex_unlock(st->data->lock); |
There was a problem hiding this comment.
the lock here is not needed
| { | ||
| pm_runtime_mark_last_busy(st->data->dev); | ||
| pm_runtime_put_autosuspend(st->data->dev); | ||
| } |
There was a problem hiding this comment.
normally this kind of helpers are not really needed. Up to you
There was a problem hiding this comment.
The helper was added because there were 7 instances of such a usage
| { | ||
| struct ad7768_state *st = iio_priv(indio_dev); | ||
|
|
||
| pm_runtime_put(&st->spi->dev); |
There was a problem hiding this comment.
No autosuspend()? Any particular reason?
There was a problem hiding this comment.
No particular reason, just an oversight. Will add pm_runtime_set_autosuspend_delay() and switch all pm_runtime_put() paths to pm_runtime_put_autosuspend().
| ret = -EINVAL; | ||
| } | ||
|
|
||
| pm_runtime_put(&st->spi->dev); |
There was a problem hiding this comment.
With the cleanup macros you can return in place
| { | ||
| int ret; | ||
|
|
||
| lockdep_assert_held(&st->lock); |
There was a problem hiding this comment.
Seems a bit too much to have this annotation for a driver that is not overly complicated. That said, up to you to keep or not
|
|
||
| ret = regmap_read(st->regmap, base_reg + 2, &lsb); | ||
| if (ret) | ||
| return ret; |
There was a problem hiding this comment.
Is it possible to do a bulk read?
There was a problem hiding this comment.
I realized now that configuring use_single_read=true can enable bulk reading in this case, which I overlooked earlier.
Using bulk read now.
| if (ret < 0) | ||
| return ret; | ||
|
|
||
| *(u8 *)val_buf = ((u8 *)&st->d16)[1]; |
There was a problem hiding this comment.
Same comments as in other series. I suspect the above can be simplified a lot. Use the arguments you get from regmap and make sure to have endianism right and read mask.
nunojsa
left a comment
nunojsa
left a comment
There was a problem hiding this comment.
Here it goes. I think this is already in an "upstreamable shape". Take my latest comments and take it upstream. The only think I would think a bit more is the regmap custom functions.
|
|
||
| st->d16 = cpu_to_be16(AD7768_SPI_READ_CMD | | ||
| FIELD_PREP(AD7768_SPI_REG_MASK, reg)); | ||
|
|
There was a problem hiding this comment.
I think I have commented on this one already. Can't we have regmap dealing with endianism and setting up the read mask? So the above is not needed? And the below be16_to_cpu()
There was a problem hiding this comment.
This part follows an off-frame SPI protocol (with two separate CS assertions) and the custom bus cannot be avoided in this case.. hence the cpu_to_be16()
| if (ret) | ||
| return ret; | ||
|
|
||
| guard(mutex)(&st->lock); |
There was a problem hiding this comment.
Just to make sure. None of the gpio registers overlap with the ones we deal with in the IIO driver? Otherwise we would need to share the lock.
There was a problem hiding this comment.
I confirm that the IIO driver doesn't apply any regmap_read/write calls on any GPIO registers. It is only listed in the readable_reg. Since that access goes through regmap's own internal lock, there wouldn't be any overlapsin this case.
|
Changelog:
|
|
Rebase with mirror_ci/jic23/iio/testing. No code changes |
|
Changelog:
|
Devicetree Bindings for AD7768-4 (4 channel) and AD7768 (8 channel) simultaneous sampling ADCs. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Add a backend operation to enable or disable Cyclic Redundancy Check processing for data integrity verification. When enabled, the backend will generate, verify, or process CRC information for data samples transmitted over the interface, allowing the host to detect corrupted samples. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
The AXI ADC register access paths serialize transactions with st->lock, but probe does not initialize it. Initialize the mutex before registering the backend. Fixes: 7ecb8ee ("iio: adc: adi-axi-adc: support digital interface calibration") Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Add support for enabling and disabling Cyclic Redundancy Check (CRC) processing in the AXI ADC backend. CRC provides data integrity verification for high-speed ADC data streams, ensuring reliable data transfer between the ADC frontend and backend processing systems. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Add core support for the AD7768 and AD7768-4 simultaneous sampling ADCs. Configure supplies, clock and reset, use a custom regmap bus for the SPI protocol, and parse the enabled channels and input buffer settings from devicetree. Connect the converter to an IIO backend for buffered capture with CRC, provide a fixed safe wideband sampling configuration and add runtime power management. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Derive the available output data rates from MCLK and expose per-channel sampling frequency and filter controls. Select the fastest compatible power mode for the enabled channels and map matching sampling frequency and filter combinations onto the two hardware channel profiles. Configure the data clock divider and wait for the selected filters to settle before capture. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Expose the per-channel offset and gain calibration registers through the IIO calibbias and calibscale attributes. Use bulk regmap operations and unaligned big-endian helpers to transfer the three register bytes. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Expose the per-channel synchronization phase offset through the IIO conversion-delay attribute. Derive the delay resolution from MCLK, power mode and decimation rate. Validate the requested delay and program the corresponding phase register when applying the active channel configuration. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Expose the on-chip common mode voltage output through the regulator framework. Support the three fixed output levels and the supply-dependent AVDD1/2 setting. Keep the ADC runtime active while VCM is enabled and release the runtime PM reference when the output is disabled. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Register an auxiliary device when the AD7768 is described as a GPIO controller. This allows the GPIO driver to share the parent regmap and runtime power-management state. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Use regmap_test_bits() when reading a single GPIO value from a normal register and when reading the direction bit. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Some gpio-regmap consumers share their regmap with a parent device that may be runtime suspended. GPIO register accesses must resume that device first. Add an optional pm_dev field and acquire it before register translation or access. Release it using runtime autosuspend after each operation. Keep the device active across the complete direction-output sequence and propagate failure when setting the initial output value. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
The AD7768 provides five GPIOs controlled through registers shared with the parent IIO device. Register an auxiliary gpio-regmap driver and use the parent device for runtime PM. The device has separate input-state and output-latch registers. Add a reg_mask_xlate() callback that checks the line direction and reads the programmed output latch for output lines while retaining input-state reads for input lines. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Add driver documentation for AD7768. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
|
Changelog:
|
3 participants
PR Description
necessary to understand them. List any dependencies required for this change.
any space), or simply check them after publishing the PR.
description and try to push all related PRs simultaneously.
PR Type
PR Checklist