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.
...
> +/* 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)
> +#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?
> + cfg = ADS1298_LOWEST_DR;
> + else if (factor)
> + cfg = ADS1298_MASK_CONFIG1_HR | ilog2(factor); /* Use HR mode */
> + else
> + cfg = ADS1298_MASK_CONFIG1_HR; /* Fastest possible */
...
> + *val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));
Outer parentheses are redundant.
...
> + wasbusy = --priv->rdata_xfer_busy;
Why preincrement? How would it be different from postincrement?
> + if (wasbusy) {
To me more robust code would look like
if (wasbusy < 1)
return;
...
if (wasbusy > 1)
...
> + /*
> + * 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);
...
> + struct device *dev = &priv->spi->dev;
> + int ret;
> + unsigned int val;
Better ordering is so called reversed xmas tree (longest lines first):
struct device *dev = &priv->spi->dev;
unsigned int val;
int ret;
...
> + /* Device initializes into RDATAC mode, which we don't want. */
No period at the end of one-line comments (be consistent in the style over
comments of the same class).
...
> + 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?
...
> + /* VREF can be supplied externally. Otherwise use internal reference */
Better to use comma as it's one-line comment. Or make it multi-line with
respective periods.
...
> + ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> + priv->reg_vref);
Can be one line.
> + if (ret)
> + return ret;
...
> + ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> + priv->reg_avdd);
One line.
> + if (ret)
> + return ret;
...
> + 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.
> + } else {
> + ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
> + if (ret)
> + return dev_err_probe(dev, ret, "RESET failed\n");
> + }
> + /* Wait 18 clock cycles for reset command to complete */
> + udelay(ADS1298_CLOCKS_TO_USECS(18));
--
With Best Regards,
Andy Shevchenko
