linux
linux copied to clipboard
analogdevicesinc
Dev/pulsar
I've implemented most of the changes (2 remaining changes) you suggested but in the process I found some bugs which will take some time to hash out. The only two remaining changes are the ad_pulsar_read_channel loop and the settings which you mentioned that are called only in preenable, of which I'm not sure yet.
Dragos suggested I submit V2 for review while I work on V3 to correct the bugs I get in some cases.
With this in mind I will force push V2 on this branch in the coming days, but please do not pull it in to the master branch yet.
I've implemented most of the changes (2 remaining changes) you suggested but in the process I found some bugs which will take some time to hash out.
Yeah, this driver needs some more work to get in shape...
...the settings which you mentioned that are called only in preenable, of which I'm not sure yet.
Note that I'm not saying this is an issue... I was just wondering why we have like this instead of just writing things directly on the device.
but please do not pull it in to the master branch yet.
Move this PR into draft and move it back when you think it's ready to be merged...
V1 -> V2
- replaced 0xFFFF with GENMASK()
- rewritten ad_pulsar_reg_access to a simpler form
- reading ref_clk_rate during probe
- replaced ref_clk in the device struct with ref_clk_rate
- replaced custome attribuite with "_available" infrastructure
- moved _disable() APIs closer to probe
- added missing error checks
- replaced 0x3FF9 with a sugestive define
- removed redundants error check
- added spi id table
- fixed nits
- replaced fwnode_property_present with fwnode_property_read_bool
Moved to draft while fixing some bugs.
V2 -> V3
- dt-bindings:
- fixed redundant new lines for some descriptions
- using spi-peripheral-props.yaml
- added "adi" prefix to driver specific properties
- created if condition for channels with multiple functionalities
- fixed the example DTS based on the new bindings
- updated license
- fixed filter function not being implemented correctly
- added missing components to the device_id tables
- changed kzalloc to kcalloc
- using reg property from the DTS as an index for channels
- split the ad_pulsar_chip_infos array in to individual structures for each device
- fixed missing adaq4003 chip_info
- added spi_get_device_id as a fallback to device_get_match_data
- moved match tables to the end of the file
- made SPI TX and RX buffers DMA safe
- added has_power_up_seq, has_filter, has_turbo, has_reset to the ad_pulsar_chip_info structure and removed the ad_pulsar_spi_init function
I had to rebase the branch to master to solve some conflicts in Kconfig.adi
V3 -> V4
- replaced ret<0 checks with ret checks
- fixed // comments using /**/
- replaced hweight_long with bitmap_weight
- using IIO_DMA_MINALIGN and added the define manually for 5.10 compatibility
- implemented a channel template to simplify the improve the channel parsing
- removed the has_reset flag because the underlying issue is in the 2 acquisition delay required by some ADCs
- rewrote the buffered acquisition to reflect the Datasheet requirements for the ADCs with sequencers Check page 31 of the AD7689 Datasheet https://www.analog.com/media/en/technical-documentation/data-sheets/ad7682_7689.pdf
V4 -> V5
- Enabled parsing of differential channels for pulsars without a sequencer
- Release firmware node if channel setup terminates with error
- Adjusted regulator_get_voltage() return handling
- Dropped support for AD40xx devices because pulsars and AD40xx devices have completely different configuration register structures.
- Stored the value of the configuration register (CFG) for debugfs_reg_access
- Renamed ad_pulsar_reg_read() -> ad_pulsar_read_channel() to improve readability
- Sign-extend differential readings so we have negative values when IN- > IN+ voltage
- Hide filter attribute if device has no configurable low pass filter
Applied all suggestions to V5 that I didn't comment about. Since there's still some adjusts to do, I'll work on them and send a V6 after a tackle the remaining points.
V5 -> V6
- Adjusted code style and clean up according to comments to patch set version 5
- Removed unused enum elements
- Dropped turbo mode support entirely because pulsars don't have such capability
- Removed a undue exceeding 2-bit shift in channel setup
- Updated the CFG register to disable the ADC internal buffer in buffer postdisable
- Renamed seq_buf -> cfg_reg to reflect the fact that the array stores values to be written to the configuration register
- Use AXI SPI int transfers for all ADCs so the AXI SPI engine initializes correctly
- Renamed attribute sequencer -> cfg_register. The pulsars that have sequencer also have additional capabilities such as configurable low pass filter, hardware buffers, and a CFG register to config those all. Having one of those features implies having the others so the renaming makes it more clear what type of pulsar we are handling.
- Now driver throws EINVAL if device node has no child describing its input channels
- Unsuppress word_delay at the last word in a SPI transfer when configuring buffered readings, making sure the SPI engine ignores triggers that might happen too early (between the last SPI word in a message and the start of the next SPI transfer) if the sampling frequency is higher than tcycle multiplied by the number of channels enabled.
- Adjusted sequenced readings to work correctly when only 2 channels are enabled
- Droped arch/arm/boot/dts/zynq-coraz7s-adaq4003.dts since support for AD40xx was dropped.
- Added a MAINTAINERS file entry. Not sure who will be maintaining the driver so I included everyone who worked on it. Also, what is the mailing list for adi/mainline drivers? Left vgker as placeholder.
By the way, thanks for all the comments to V5. Hope we have been able to address them all.
V6 -> V7
- Rebased the commits on top of master. That was the only change.
The PulSAR ADC family comprises single-channel, 4-channel, or 8-channel devices. This driver has been tested with eval-ad7687-PMDZ (single-channel pulsar) and eval-ad7689-ebz (8-channel pulsar), and verified to work except for one case. All readings were the same for every channel when reading multiple channels in buffered mode. That is a bug we were not able to hunt down. Other than that, everything functions as expected. Single-shot readings for each and every channel return data that is coherent with what is provided as input and buffered readings bring in reasonable results when only one channel is enabled.
So, I suggest we merge it with master or have another code review. Otherwise, I worry this driver may sit here forever and rot with time. We may add a comment in the code or in the commit message to document what is not working (if worth it).
Thanks