On 16-02-2024 14:38, Jonathan Cameron wrote:
On Wed, 14 Feb 2024 07:37:36 +0100
Mike Looijmans <mike.looijmans@xxxxxxxx> wrote:
Skeleton driver for the TI ADS1298 medical ADC. This device is
typically used for ECG and similar measurements. Supports data
acquisition at configurable scale and sampling frequency.
Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
Hi Mike,
One final thing noticed on a (hopefully) last read through.
/sys/bus/iio:device0/name is going to read ads1298 whichever
chip is detected.
Would be more useful to users if it identified the actual
part given that is easily read from the ID register.
Makes sense. So It would say "ads1296" or "ads1298r" for example. I
guess we prefer all lower-case here.
Jonathan
---
Changes in v4:
Explain rdata_xfer_busy better and remove post-decrement
Reset assert explanation and add cansleep
Additional style changes
Changes in v3:
Indentation fixups
Remove unused headers
Remove #define leftovers
Use devm_get_clk_optional_enabled
Use ilog2 instead of fls()-1
Magic "23" replaced
Explain the extra "0" in read/write register
use guard() from cleanup.h
use REGCACHE_MAPLE
Changes in v2:
Remove accidental "default y" in Kconfig
Indentation and similar cosmetic fixes
Magic numbers into constants
Short return paths in read_raw and write_raw
DMA buffer alignment
Bounce buffer is u32 instead of u8
Avoid races using claim_direct_mode
Check errors on all register accesses
Immediate SPI restart to reduce underruns
"name" is chip name, not unique
I missed this until having a final read through but it's not the chip name
in the driver currently - the name is always ads1298 despite there being a handy
ID register that tells us what we actually have.
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-ads1298.c | 766 +++++++++++++++++++++++++++++++++++
3 files changed, 778 insertions(+)
create mode 100644 drivers/iio/adc/ti-ads1298.c
+
+static const char *ads1298_family_name(unsigned int id)
+{
+ switch (id & ADS1298_MASK_ID_FAMILY) {
+ case ADS1298_ID_FAMILY_ADS129X:
+ return "ADS129x";
+ case ADS1298_ID_FAMILY_ADS129XR:
+ return "ADS129xR";
+ default:
+ return "(unknown)";
+ }
+}
...
+static int ads1298_probe(struct spi_device *spi)
+{
...
+
+ priv->tx_buffer[0] = ADS1298_CMD_RDATA;
+ priv->rdata_xfer.tx_buf = priv->tx_buffer;
+ priv->rdata_xfer.rx_buf = priv->rx_buffer;
+ priv->rdata_xfer.len = ADS1298_SPI_RDATA_BUFFER_SIZE;
+ /* Must keep CS low for 4 clocks */
+ priv->rdata_xfer.delay.value = 2;
+ priv->rdata_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
+ spi_message_init_with_transfers(&priv->rdata_msg, &priv->rdata_xfer, 1);
+ priv->rdata_msg.complete = &ads1298_rdata_complete;
+ priv->rdata_msg.context = indio_dev;
+
+ indio_dev->name = spi_get_device_id(spi)->name;
I was going to just tweak this whilst applying. Using the spi device id often
ends up being fragile in the long term because of the split between the different
ID tables and the mess that happens if fallback compatibles are in use and
the spi IDs are missing (you will get a warning about this at runtime
but it'll carry on anyway).
Easier to just hard code the name for now and when you have multiple
devices supported, add this to a chip_info type structure.
Or we could make it support the more specific name given the detection above.
Is there a reason to not do that given a more accurate name is
easy to work out and may be useful to a user?
The only reason I tried to make the "name" unique is that IIO
oscilloscope (ab)uses "name" as a unique identifier. That's something
that ought to be fixed in IIO oscilloscope.
Giving the chip name would reveal the actual detected hardware, there's
currently no other way for the user to find that out.
--
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: mike.looijmans@xxxxxxxx
W: www.topic.nl
