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