Add support for AD4052 ADC Family

[gastmaier]

PR Description

Add:

• driver support for AD4052/AD4058/AD4050/AD4056 (16-bit/12-bit SAR ADC SPI)
• devicetree binding

Optional:

• sample devicetree for coraz7s
• kconfig imply driver

Features:

• Single shot using gpio as ADC CNV
• Buffer readings using AXI PWM ADC CNV and ADC GP1 Data Ready as AXI SPI Engine Offload Trigger
• Monitor mode with ADC GP0 as interrupt trigger (either IIO threshold direction event)
  • User should do register access to get event details.
• Functional modes via oversampling config: Sample Mode (0/1) or Burst Averaging Mode (>1)
  • Burst Avg enables configuring averaging filter length value (AVG_CONFIG.AVG_WIN_LEN)
• DEVICE_CONFIG.POWER_MODE : put device on sleep at pm_runtime ops.
• TIMER_CONFIG.FS_BURST_AUTO : configure burst avg enable and trigger mode sample rates via channel sampling frequency config.

Overall, the highlight of this device is the monitor capability, leveraged by the autonomous trigger mode and exposed as IIO Event.

Linux driver doc:

• live: https://gastmaier.github.io/documentation/linux/drivers/iio-adc/ad4052/index.html
• source: https://github.com/gastmaier/documentation/blob/main/docs/linux/drivers/iio-adc/ad4052/index.rst

HDL: analogdevicesinc/hdl#1504
Datasheet: www.analog.com/ad4052
Tested on Coraz7s

PR Type

• Bug fix (a change that fixes an issue)
• New feature (a change that adds new functionality)
• Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

• I have conducted a self-review of my own code changes
• I have tested the changes on the relevant hardware
• I have updated the documentation outside this repo accordingly (if there is the case)

[gastmaier]

Forced pushed to:

• Add support to TIMER_CONFIG.FS_BURST_AUTO
• Only write GP during probe.
• Simplify IRQ Handler, leave to the user to clear the irq, because:
  • Entering config mode causes the irq to unset.
  • Re-entering monitor mode sets the irq back if still outside the threshold range, causing new handler calls.
  • "Ignoring next interrupt" would generate obnoxious logic.

Forced pushed (2) to:

• Invert fs_burst_auto array entries, default all devices to 2MSPS (Yes, this value is -EINVAL for ad4056/ad4058, but is the datasheet reset value)

Forced pushed (3) to:

• Clean-up unused enums.
• Rename ad405x to ad4052 in the dts

[nunojsa]

Here it goes first round of review...

I think this one can be upstreamed without the offload bits

[gastmaier]

Change log v0 -> v1

Review changes:

• Move functional modes from devicetree entry to different OVERSAMPLING_RATIO values, affecting RAW read and buffered.
• Documentation YAML fixes, removal of functional mode.
• Header on alphabetical order.
• Use field_prep
• Remove forward declaration, reorder methods instead.
• Fix cache line aligment.
• Use devm runtime enable, remove manual remove.
• Use dev_err_probe
• General Documentation/.yaml fixes
• Make all regmap_bulk dma safe.
• Remove/simplify switch cases where not needed.
• Instead of using of_node, use spi->irq directly
• Use regmap update bits.
• Use fsleep
• Use oversampling iio property (value 0 is invalid, 1 put on Sample Mode, > 1 Burst Averaging mode)

New:

• Add regmap access tables

Design changes:

• Under IIO Event thrshold enabled (device Monitor Mode), return device access busy for every other access, simplifying logic by not requiring exiting and re-entering mode and having an obscure "monitoring downtime" during access.
• Use oversampling iio prop instead of devicetree entry to switch functional modes.

[nunojsa]

I'm also not sure if gpio is something you should depend on or just select it. I think the single shot reading could be something useful to have even more to make this driver more upstreamable without offload support (we can still some questions about it though).

[nunojsa]

Also one not on the commit message. Style is iio: adc: .... Drop the drivers:

[nunojsa]

I would double check other examples of oversampling. To me this is a 1 or 0 thing. Either it's enabled it not. Your mode variable also just seems to have two states here so the way you're handling val raises some questions.

[gastmaier]

The device supports from 2 to 4096 in powers of 2 samples for avg mode.
Decided to reserve 0 and 1 to disable the feature (return to sample mode)

[gastmaier]

Change log v1 -> v2

Review changes:

• Include all peripheral properties
• Drop cnv-gpio description
• Updated dt-binding commit description
• Replace d32 array by d16/d32 union (requires ignoring misleading checkpatch warning).
• Remove MODULE_IMPORT_NS(IIO_AD4052);
• Add error handling where missing
• remove {} in single statement ifs, coherent style
• Use multiple scan masks for changing precision due to oversampling
• Move all if (st->mode == AD4052_MONITOR_MODE) inside the IIO mutex claim.
• Drop switches where there is only one channel/event type
• Remove unused AD4052_AVERAGING_MODE (would require a timed trigger) and AD4052_CONFIG_MODE (implicit mode outside any reading)
• ad4052_set_avg_filter pass st directly.
• Use different variables instead of the array for the chip_info

Changes:

• Use GPIO interrupt instead of PS IRQ
• All GPIOs are optional, does not require GPIO on KConfig. The user shall bring his own external trigger or short the CS and CNV, for example.
• Move PWM sampling rate to IIO_ATTR
• Move ext fs_burst_auto to channel sampling freq
• Use Data Ready assertions for single shot readings, timeout after 1 second

Extra force push to resolve merge conflict

[nunojsa]

__spi_unoptimize_message cannot be called on a message that is not currently optimized and spi_unoptimize_message only checks for defer_optimize_message, that just indicates the controller doesn't really optimize until the transfer is actually done.

Am I missing something here?

https://elixir.bootlin.com/linux/v6.13.4/source/drivers/spi/spi.c#L4344

AFAICT, it will just be called and then anything can happen... You should not make any assumption on how core code and even more importantly how a spi controller will handle this. The docs clearly stated that the calls should be balanced. Not doing so is clearly a driver bug IMO.

If I call devm_optimize_message, it will add the spi_unoptimize_message action to the list of managed resources over and over again, which is a no-no as explained in the last paragraph.

yes, since this function is also called from an userspace interface, indeed devm_action is also not the way to go...

[gastmaier]

V8

Applies changes from previous discussion.

• Drop ".yaml" from commit title dt-bindings: iio: adc: Add adi,ad4052
• Heap soft reset array pattern
• Reorder ad4052_update_xfers and devm_iio_device_register to ensure no races.
• Drop raw read optimize due to awkward un-balance (1).
• Balance offload xfer optimize, as usual.

# V7..V8
git range-diff  7ff88f0cf1f902d62b757b6fef9032fbcfb4f0cc~..9d68079b812aca6eda93ce960843b3d96cfe8e87  7ff88f0cf1f902d62b757b6fef9032fbcfb4f0cc~..af2cdc23c1068cfcdbc0a413e1e4668b67fd449f
# driver only
git diff eeea804d53fa1cc9a77c5e05a541093d3e9e1f54..2d865d174b43e0c39a6008153394f1be4e478645

(1) The source of the unbalance was the raw read, because the message needed to be re-optimized on oversampling write to be ready for the raw read, or balance on the raw read, which makes no sense from the optimization point-of-view.
So I removed optimization for raw read and balanced the offload_xfer on the buffer ops.
Now without the raw xfer optimization, .remove was also removed.

Counter-intuitive behaviour: io_buffer_enabled(indio_dev)) does return true during buffer_preenable, so the requested scan type obtained is correct, even though the counterintuitive naming/sequence.

Another point in favor of dropping raw xfer optimization is that oversampling set optimization could fail, setting the xfer in the un-optimized state, and on remove it would indeed call un-optimized on a un-optimized message, which in my tests do cause panic.

[nunojsa]

Just some minor comment... Looks ready to go upstream

[nunojsa]

the above header looks suspicious? Leftover?

[nunojsa]

Oh, you have it in here... Please use device property APIs. fwnode_irq_get()

[gastmaier]

V9

• replace of_node with fwnode_irq_get, dropping the include.
• replaced iio_device_claim_direct_scoped with ad4052_iio_device_claim_direct because is more similar with the current upstream recommended, resulting in fewer touched lines
• move iio_device_claim_direct_scoped, wait_event to an auxiliary method, explaning why it is needed.
• add __maybe_unused to pm methods, since without the PM enabled, they are unused

V8..V9
# driver only
git diff 2d865d174b43e0c39a6008153394f1be4e478645..71e7eefdc99352e634527d5ad4d61d543319d9cb

[nunojsa]

I'm not doing another round but saw this in the log which got my attention:

add __maybe_unused to pm methods, since without the PM enabled, they are unused

pm_ptr() should prevent that... Maybe that means you should instead use pm_sleep_ptr()? This macros were introduced exactly to avoid marking PM function with maybe_unused as the compiler/linker should be able to detect and remove "dead" code.

[gastmaier]

If CONF_PM is disabled the methods are sinked/discarted and the relation between the .pm and the method is never created

If CONFIG_PM or CONFIG_PM_SLEEP are enabled (respectively), these macros will resolve to their argument, otherwise to NULL.

https://lore.kernel.org/all/[email protected]/

with pm_sleep_ptr, if CONF_PM and CONF_PM_SLEEP are unset, the pointer is NULL and the warning is raised.
If I am overthinking and we just don't care about PM being disable, we sure can remove that.

[nunojsa]

Can you test with RUNTIME_PM_OPS() instead of SET_RUNTIME_PM_OPS()? The reason I'm pushing a bit is because, as you saw, this pm_sleep() macros were indeed introduced to avoid those __maybe_unused.

BTW, I think pm_ptr() is indeed the right one to use (as you're using runtime PM)

[gastmaier]

Yep, you are right, the modern way is RUNTIME_PM_OPS not SET_RUNTIME_PM_OPS .
Tested.
https://patchwork.kernel.org/project/linux-media/patch/[email protected]/

[gastmaier]

V10

• replace SET_RUNTIME_PM_OPS with the RUNTIME_PM_OPS.
• Fixup aux iio_device_claim

V9..V10
# driver only
git diff 71e7eefdc99352e634527d5ad4d61d543319d9cb..455d4985a811f8d0a7b1344238bd1b208a092e64

[gastmaier]

Up-streaming at https://lore.kernel.org/all/[email protected]/

Below is a small bash script to convert from 'current' upstream and adi-linux

driver=drivers/iio/adc/ad4052.c
doc=Documentation/iio/ad4052.rst
bin=Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
abi=Documentation/ABI/testing/sysfs-bus-iio
adi=.
up=../../linux-upstream

up2adi () {
        cp $up/$driver $adi/$driver
        cp $up/$doc $adi/$doc
        cp $up/$bin $adi/$bin
        cp $up/$abi $adi/$abi
        sed -i -e 's/iio_device_release_direct(/iio_device_release_direct_mode(/g' \
               -e 's/\!iio_device_claim_direct(/iio_device_claim_direct_mode(/g' \
               -e 's/bool state/int state/' \
               -e 's/("IIO_DMAENGINE_BUFFER")/(IIO_DMAENGINE_BUFFER)/' $adi/$driver
}

adi2up () {
        cp $adi/$driver $up/$driver
        cp $adi/$doc $up/$doc
        cp $adi/$bin $up/$bin
        #cp $adi/$abi $up/$abi
        sed -i -e 's/iio_device_release_direct_mode(/iio_device_release_direct(/g' \
               -e 's/iio_device_claim_direct_mode(/\!iio_device_claim_direct(/g' \
               -e 's/int state/bool state/' \
               -e 's/(IIO_DMAENGINE_BUFFER)/("IIO_DMAENGINE_BUFFER")/' $up/$driver
}

"$@"