On 06-02-2024 13:57, Andy Shevchenko wrote:
On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans 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.
Thanks for an update, I have only a few style comments and a single one about
comparison (see below). If you are going to address them as suggested, feel
free to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
to the next version.
...
Thanks for reviewing...
+/* Registers */
+#define ADS1298_REG_ID 0x00
+#define ADS1298_MASK_ID_FAMILY GENMASK(7, 3)
+#define ADS1298_MASK_ID_CHANNELS GENMASK(2, 0)
+#define ADS1298_ID_FAMILY_ADS129X 0x90
+#define ADS1298_ID_FAMILY_ADS129XR 0xd0
+ Blank line? (And so on for all registers that have bitfields defined)
Makes sense... Looks too cluttered as it is now.
+#define ADS1298_REG_CONFIG1 0x01
+#define ADS1298_MASK_CONFIG1_HR BIT(7)
+#define ADS1298_MASK_CONFIG1_DR GENMASK(2, 0)
+#define ADS1298_SHIFT_DR_HR 6
+#define ADS1298_SHIFT_DR_LP 7
+#define ADS1298_LOWEST_DR 0x06
...
+ factor = (rate >> ADS1298_SHIFT_DR_HR) / val;
+ if (factor >= 128)
I just realized that this comparison is probably better in a form
if (factor >= ADS1298_MASK_CONFIG1_HR)
as it points out why this is a special case in comparison to 'if (factor)'
below. What do you think?
The "HR" bit sets the device to high-res mode (which apparently doubles
the internal sample rate).
But "128" could be written as "1 << ADS1298_SHIFT_DR_LP" which is the
max oversampling factor.
...
+ wasbusy = --priv->rdata_xfer_busy;
Why preincrement? How would it be different from postincrement?
Maybe better write this as:
--priv->rdata_xfer_busy;
wasbusy = priv->rdata_xfer_busy;
I want the value after decrementing it.
+ if (wasbusy) {
To me more robust code would look like
if (wasbusy < 1)
return;
...
if (wasbusy > 1)
...
wasbusy could have been unsigned.
This code will only ever execute with rdata_xfer_busy > 0 (or the SPI
driver called our completion callback without us calling spi_async first)
+ /*
+ * DRDY interrupt occurred before SPI completion. Start a new
+ * SPI transaction now to retrieve the data that wasn't latched
+ * into the ADS1298 chip's transfer buffer yet.
+ */
+ spi_async(priv->spi, &priv->rdata_msg);
+ /*
+ * If more than one DRDY took place, there was an overrun. Since
+ * the sample is already lost, reset the counter to 1 so that
+ * we will wait for a DRDY interrupt after this SPI transaction.
+ */
+ if (wasbusy > 1)
+ priv->rdata_xfer_busy = 1;
+ }
...
+ /*
+ * for a single transfer mode we're kept in direct_mode until
For
+ * completion, avoiding a race with buffered IO.
+ */
...
+ wasbusy = priv->rdata_xfer_busy++;
So, it starts from negative?
+ /* When no SPI transfer in transit, start one now */
+ if (!wasbusy)
To be compatible with above perhaps
if (wasbusy < 1)
also makes it more robust (all negative numbers will be considered the same.
+ spi_async(priv->spi, &priv->rdata_msg);
The "rdata_xfer_busy" starts at 0.
Increments when a DRDY occurs.
Decrements when SPI completion is reported.
So the meaning of "rdata_xfer_busy" is:
0 = Waiting for DRDY interrupt
1 = SPI transfer in progress
2 = DRDY occured during SPI transfer, should start another on completion
>2 = Multiple DRDY during SPI transfer, overflow, we
lost rdata_xfer_busy - 2 samples
...
+ dev_dbg(dev, "Found %s, %u channels\n", ads1298_family_name(val),
+ (unsigned int)(4 + 2 * (val & ADS1298_MASK_ID_CHANNELS)));
Castings in printf() is a big red flag usually (it's rarely we need them).
Why is it here?
Compiler complains that the expression is "unsigned long". Probably
because of ADS1298_MASK_ID_CHANNELS being so.
...
+ if (reset_gpio) {
+ /* Minimum reset pulsewidth is 2 clock cycles */
+ udelay(ADS1298_CLOCKS_TO_USECS(2));
+ gpiod_set_value(reset_gpio, 0);
I would rewrite it as
/* Minimum reset pulsewidth is 2 clock cycles */
gpiod_set_value(reset_gpio, 1);
udelay(ADS1298_CLOCKS_TO_USECS(2));
gpiod_set_value(reset_gpio, 0);
to be sure we have a reset done correctly, and the comment will make more
sense.
If used, the reset must be asserted *before* the voltages and clocks are
activated. This would obfuscate that, and add a redundant call to set_value.
I did forget to use "cansleep" here, will add that.
--
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
