On Fri, Feb 02, 2024 at 04:28:07PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 02, 2024 at 11:59:01AM +0100, Mike Looijmans wrote:
Hit "Send" by a mistake, here is the full review.
...
Why this can't be the part of drivers/iio/adc/ti-ads124s08.c?
Seems to me the command list is the same, registers are different though.
Broadly the Q is have you checked other existing drivers if they can
be used as a base. If not, perhaps a word in the cover letter is good to have
(sorry if I asked this already).
...
> > +#define ADS1298_WAKEUP 0x02
> > +#define ADS1298_STANDBY 0x04
> > +#define ADS1298_RESET 0x06
> > +#define ADS1298_START 0x08
> > +#define ADS1298_STOP 0x0A
> > +#define ADS1298_RDATAC 0x10
> > +#define ADS1298_SDATAC 0x11
> > +#define ADS1298_RDATA 0x12
>
> Leftovers.
...
> > +#define ADS1298_CLOCKS_TO_USECS(x) \
> > + (DIV_ROUND_UP((x * MICROHZ_PER_HZ), ADS1298_CLK_RATE_HZ))
>
> Wrong place of parentheses, should be "(x) * ...".
> Don't you need to include math.h?
...
> > +#define ADS1298_MASK_CONFIG1_DR 0x07
GENMASK() ?
...
> > +struct ads1298_private {
> > + const struct ads1298_chip_info *chip_info;
> > + struct spi_device *spi;
> > + struct regulator *reg_avdd;
> > + struct regulator *reg_vref;
> > + struct clk *clk;
> > + struct regmap *regmap;
> > + struct completion completion;
> > + struct iio_trigger *trig;
> > + struct spi_transfer rdata_xfer;
> > + struct spi_message rdata_msg;
Do you use this outside of the ->probe()? I just ask since I removed some
context already...
> > + spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
> > + int rdata_xfer_busy;
> > +
> > + /* Temporary storage for demuxing data after SPI transfer */
> > + u32 bounce_buffer[ADS1298_MAX_CHANNELS];
> > +
> > + /* For synchronous SPI exchanges (read/write registers) */
> > + u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
> > +
> > + /* Buffer used for incoming SPI data */
> > + u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
Cacheline aligned?
I see the cmd_buffer, but shouldn't this be also aligned?
> > + /* Contains the RDATA command and zeroes to clock out */
> > + u8 tx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
> > +};
...
> > +static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
> > +{
> > + struct spi_transfer cmd_xfer = {
I would use xfer[]...
> > + .tx_buf = priv->cmd_buffer,
> > + .rx_buf = priv->cmd_buffer,
> > + .len = sizeof(command),
In other cases you use plain number, perhaps 1 is okay here for consistency's
sake?
> > + .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
> > + .delay = {
> > + .value = 2,
> > + .unit = SPI_DELAY_UNIT_USECS,
> > + },
> > + };
> > +
> > + priv->cmd_buffer[0] = command;
> > +
> > + return spi_sync_transfer(priv->spi, &cmd_xfer, 1);
...and ARRAY_SIZE(), but either way I'm fine, i.o.w. this is also okay.
> > +}
...
> > + /* Enable the channel */
Too many spaces before */.
...
> > + /* Cannot take longer than 40ms (250Hz) */
> > + ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
> > + if (!ret)
> > + return -ETIMEDOUT;
> > +
> > + return 0;
Can be other way around
if (ret)
return 0;
return -ETIMEDOUT;
But the original is also okay.
> > +}
...
> > + if (priv->clk)
> > + rate = clk_get_rate(priv->clk);
> > + else
> > + rate = ADS1298_CLK_RATE_HZ;
Dead code (here and elsewhere). You probably wanted _optional clk APIs
in the probe.
> > + if (!rate)
> > + return -EINVAL;
...
> > + factor = (rate >> ADS1298_SHIFT_DR_HR) / val;
> > + if (factor >= 128) {
> > + cfg = ADS1298_LOWEST_DR;
> > + } else if (factor <= 1) {
> > + cfg = ADS1298_MASK_CONFIG1_HR; /* Fastest possible */
> > + } else {
> > + cfg = fls(factor) - 1;
> > + cfg |= ADS1298_MASK_CONFIG1_HR; /* Use HR mode */
> > + }
What about:
factor = (rate >> ADS1298_SHIFT_DR_HR) / val;
if (factor >= 128)
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 */
(will need log2.h to be included)
...
> > + *val = ret / 1000; /* Convert to millivolts */
MILLI ?
...
> > + *val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
> > + 23);
Second time magic 23, can you define a constant and use it everywhere?
...
> > +static int ads1298_reg_write(void *context, unsigned int reg, unsigned int val)
> > +{
> > + struct ads1298_private *priv = context;
> > + struct spi_transfer reg_write_xfer = {
> > + .tx_buf = priv->cmd_buffer,
> > + .rx_buf = priv->cmd_buffer,
> > + .len = 3,
> > + .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
> > + .delay = {
> > + .value = 2,
> > + .unit = SPI_DELAY_UNIT_USECS,
> > + },
> > + };
> > +
> > + priv->cmd_buffer[0] = ADS1298_CMD_WREG | reg;
> > + priv->cmd_buffer[1] = 0x0;
> > + priv->cmd_buffer[2] = val;
Sounds to me like put_unaligned_be16().
> > +
> > + return spi_sync_transfer(priv->spi, ®_write_xfer, 1);
> > +}
...
> > + priv->cmd_buffer[0] = ADS1298_CMD_RREG | reg;
> > + priv->cmd_buffer[1] = 0x0;
> > + priv->cmd_buffer[2] = 0;
Ditto.
> > + ret = spi_sync_transfer(priv->spi, ®_read_xfer, 1);
> > + if (ret)
> > + return ret;
...
> > + *val = priv->cmd_buffer[2];
Just wondering if the above is correct assumption, this probably needs to be
get_unaligned_be16().
...
> > + unsigned long flags;
> > +
> > + /* Notify we're no longer waiting for the SPI transfer to complete */
> > + spin_lock_irqsave(&priv->irq_busy_lock, flags);
> > + priv->rdata_xfer_busy = 0;
> > + spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
Use cleanup.h?
...
> > +static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
> > + const unsigned long *scan_mask)
> > +{
> > + struct ads1298_private *priv = iio_priv(indio_dev);
> > + unsigned int val;
> > + int ret;
> > + int i;
> > +
> > + /* Make the interrupt routines start with a clean slate */
> > + ads1298_rdata_unmark_busy(priv);
> > +
> > + /* Power down channels that aren't in use */
This comment does not describe why you need to write to _all_ channels.
> > + for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
> > + val = test_bit(i, scan_mask) ? 0 : ADS1298_MASK_CH_PD;
With above in mind, this perhaps needs to be one of for_each_set_bit(scan_mask) /
for_each_clear_bit(scan_mask).
> > + ret = regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
> > + ADS1298_MASK_CH_PD, val);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
...
> > + unsigned long flags;
cleanup.h
> > + int wasbusy;
> > +
> > + spin_lock_irqsave(&priv->irq_busy_lock, flags);
> > +
> > + wasbusy = --priv->rdata_xfer_busy;
> > + if (wasbusy) {
> > + /*
> > + * 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;
> > + }
> > +
> > + spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
...
> > +static irqreturn_t ads1298_interrupt(int irq, void *dev_id)
Ditto.
...
> > + .cache_type = REGCACHE_RBTREE,
Why not MAPPLE TREE?
...
> > +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)";
Hmm... Maybe "" is enough? What is the practice in IIO for this? Jonathan?
> > + }
> > +}
...
> > + dev_info(dev, "Found %s, %u channels\n", ads1298_family_name(val),
> > + 4 + 2 * (val & ADS1298_MASK_ID_CHANNELS));
How is this useful? Can't sysfs already give an answer to these Q:s?
...
> > + if (IS_ERR(priv->reg_vref)) {
> > + if (PTR_ERR(priv->reg_vref) == -ENODEV)
> > + priv->reg_vref = NULL;
> > + else
Redundant if you check for an error cases first.
> > + return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
> > + "Failed to get vref regulator\n");
> > + } else {
> > + ret = regulator_enable(priv->reg_vref);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> > + priv->reg_vref);
> > + if (ret)
> > + return ret;
> > + }
...
> > + priv->clk = devm_clk_get_enabled(dev, "clk");
_optional? See above.
> > + if (IS_ERR(priv->clk))
> > + return dev_err_probe(dev, PTR_ERR(priv->clk),
> > + "Failed to get clk\n");
One line? (Even in strict, i.e. 80 limit, mode checkpatch won't complain on it)
...
> > + priv->regmap = devm_regmap_init(dev, NULL, priv,
> > + &ads1298_regmap_config);
One line? (It's 81, which we may very well tolerate)
> > + if (IS_ERR(priv->regmap))
> > + return PTR_ERR(priv->regmap);
...
> > + if (reset_gpio) {
> > + udelay(ADS1298_CLOCKS_TO_USECS(2));
Why this delay (the after one is explained, though)?
> > + gpiod_set_value(reset_gpio, 0);
> > + } 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
