-
Notifications
You must be signed in to change notification settings - Fork 835
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.
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
Add support for adaq776x-1 series and add missing features #2587
base: main
Are you sure you want to change the base?
Conversation
Add IIO attributes: - Filter type - Decimation rate for Sinc3 filter - Decimation rate for Sinc5 and Wideband filter - Power mode selection - MCLK divider - Common mode voltage Signed-off-by: PopPaul2021 <paul.pop@analog.com>
This adds support for ADAQ7767/68/69-1 series, which includes PGIA and AAF gains, configurable through devicetree. It also fixes sampling frequency calculation, sync pulse using registers and data acquisition with iio. the driver now can work in low-latency mode. In low-latency mode the filter type is fixed in Sinc5 Dec8 and ADC data width is set to 16 bits to reach maximum sample rate. When disabled, the ADC data is 24 bits and it is possible to change between filters in runtime (except for Sinc5 Dec8). Signed-off-by: jonathanns <Jonathan.Santos@analog.com>
All supported parts require that the MOSI line stays high while in idle. Signed-off-by: jonathanns <Jonathan.Santos@analog.com>
Add support for new spi-engine implementation on ad7768-1 driver. This optimizes the offload transfers by reducing the CS delay and removing the number of commands on offload's FIFO, which makes the low latency mode viable. Without the otimization there's not enough time between the end of a transfer and the start of the next one, reducing the actual sample rate. Signed-off-by: jonathanns <Jonathan.Santos@analog.com>
new required property Update sync-in GPIO number according to the current HDL version. Add low-latency property. Signed-off-by: jonathanns <Jonathan.Santos@analog.com>
Enables using the ADAQ7767-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <Jonathan.Santos@analog.com>
Enables using the ADAQ7768-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <Jonathan.Santos@analog.com>
Enables using the ADAQ7769-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <Jonathan.Santos@analog.com>
Add compatibles for supported parts in the ad7768-1 family: ADAQ7767-1, ADAQ7768-1 and ADAQ7769-1 Add new properties: adi,low-latency and adi,gain-milli Signed-off-by: jonathanns <Jonathan.Santos@analog.com>
4a14e55
to
9432dff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of review... There#s some stuff with ABI that needs to be figured. In your second patch, you also have:
"It also fixes sampling frequency calculation, sync pulse using registers
and data acquisition with iio."
Please don't mix fixes with new code being added. Fixes need to come first and be as standalone as possible.
Also note that I'm aware this driver already diverged from the upstream version but it would very nice (and it would make a lot of sense) if you could send these patches upstream with the new devices and new ABI. Naturally you would have to ommit the spi offload part of it but the new ABI would be very important to have it discussed (and agreed) upstream. Otherwise, what it will most likely happen is that we merge something in here that will then change when upstreaming the missing bits of this driver upstream.
{ AD7768_MCLK_DIV_4, AD7768_DEC_RATE_512, 2048, AD7768_MED_MODE }, | ||
{ AD7768_MCLK_DIV_4, AD7768_DEC_RATE_1024, 4096, AD7768_MED_MODE }, | ||
{ AD7768_MCLK_DIV_8, AD7768_DEC_RATE_1024, 8192, AD7768_MED_MODE }, | ||
{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing this? I guess it's because we have mclk on a runtime attr but more on that below...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we divided it into separete attributes
[SINC5_DEC_X8] = "SINC5_DEC_X8", | ||
[SINC5_DEC_X16] = "SINC5_DEC_X16", | ||
[SINC3] = "SINC3", | ||
[WIDEBAND] = "WIDEBAND", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something to bother when trying to upstream but take a look at this ABI:
I'm fairly sure that when upstreaming we will be asked to put this ABI in a more a generic file. Hence, it makes sense to see if there's something on that file that makes sense to be used in here.
[AD7768_ECO_MODE] = "AD7768_ECO_MODE", | ||
[AD7768_MED_MODE] = "AD7768_MED_MODE", | ||
[AD7768_FAST_MODE] = "AD7768_FAST_MODE", | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could implement the above with runtime PM but I'll defer this discussion for usptream...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look at the Power management module.
&ad7768_vcm_mode_enum), | ||
IIO_ENUM_AVAILABLE("common_mode_voltage", | ||
IIO_SHARED_BY_ALL, | ||
&ad7768_vcm_mode_enum), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no point in changing the above... Minimizing the diffs makes the reviewer life easier
IIO_ENUM("dec_rate", IIO_SHARED_BY_ALL, &ad7768_dec_rate_iio_enum), | ||
IIO_ENUM_AVAILABLE("dec_rate", IIO_SHARED_BY_ALL, &ad7768_dec_rate_iio_enum), | ||
IIO_ENUM("mclk_div", IIO_SHARED_BY_ALL, &ad7768_mclk_div_iio_enum), | ||
IIO_ENUM_AVAILABLE("mclk_div", IIO_SHARED_BY_ALL, &ad7768_mclk_div_iio_enum), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the mclk_div
about? Is this an internal divider for an output clock? Or is it for an input clk? If it's an input clock do we really need to change this at runtime or a DT parameter would be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MCLK is an input clock generated by a Crystal. mclk_div
is set by a configuration register to control the sampling frequency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we then somehow implement this as part of the sampling_frequency attr?
indio_dev->name = spi_get_device_id(spi)->name; | ||
ret = device_property_read_u32(&spi->dev, "adi,low-latency", &val); | ||
if (ret) | ||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be a mandatory property at all... Also seems like a boolean one. But more importantly, it raises the question about this being a DT property? It's at the very least not too coherent with the power_modes attr? Is this somehow related with those modes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that could be a boolean. We need something like this in the devicetree to set the IIO channel to either 16 or 24 bits.
In Sinc5 Decimation x8 mode, the sample width is set to 16 bits, while in other filter modes, the sample width is 24 bits.. Since we cannot change the IIO channel in runtime, we need a property in the device tree to choose between the two cases.
When this property is "enabled," the device will operate exclusively in Sinc5 Dec8 mode (16-bit samples). If the property is disabled, the filter mode can be chosen between the other options, and the sample width will default to 24 bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Sinc5 Decimation x8 mode, the sample width is set to 16 bits, while in other filter modes, the sample width is 24 bits.. Since we cannot change the IIO channel in runtime, we need a property in the device tree to choose between the two cases.
If I'm understanding correctly, you can now change the scan elements at run time... take a look at
https://elixir.bootlin.com/linux/v6.11/source/include/linux/iio/iio.h#L839
and how it's being used...
st->aaf_gain = AD7768_AAF_IN1; | ||
|
||
if (device_property_present(&spi->dev, "adi,aaf-gain") | ||
&& st->chip->has_variable_aaf) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't check for presence... Instead, do it like this:
ret = device_property_read_u32(&spi->dev, "adi,aaf-gain", &val);
if (!ret) {
if (!st->chip->has_variable_aaf)
return dev_err_probe(...);
// proceed with code validating the prop value
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted
* Request the SPI controller to make MOSI idle high. | ||
*/ | ||
spi->mode |= SPI_MOSI_IDLE_HIGH; | ||
ret = spi_setup(spi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a Fixes tag and be the first patch in the pull. Fixes should always come first in case we want to backport them.
Also wondering why this was never caught...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i will separate the fixes and move them to the beginning.
without that the device works, but not consistently. So I guess this slipped through.
@@ -4,6 +4,7 @@ | |||
* | |||
* Copyright 2017 Analog Devices Inc. | |||
*/ | |||
#include "linux/byteorder/little_endian.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like an header being auto added by clangd 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, i will check this out
tx_data[0] = AD7768_RD_FLAG_MSK(addr); | ||
xfer.tx_buf = tx_data; | ||
ret = spi_sync_transfer(st->spi, &xfer, 1); | ||
if (ret < 0) | ||
return ret; | ||
*data = (len == 1 ? st->data.buf[0] : st->data.word); | ||
*data = (len == 1 ? st->data.buf[1] : cpu_to_be32(st->data.word)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the above changes needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is necessary for the new spi-engine integration
Thanks for the feedback, I will re-arrange the commits and take a look at the ABI issue. I am sending these patches now to get a preliminar review before sending them upstream. So, when it is good enough, we can hold off this PR until they are upstreamed. |
PR Description
This adds support for ADAQ7768-1, ADAQ7769-1 and ADAQ7767-1 devices from the AD7768-1 family. It also includes:
PR Type
PR Checklist