Messages in this thread | | | Date | Wed, 31 Jan 2024 17:10:08 +0100 | From | Mike Looijmans <> | Subject | Re: [PATCH 2/2] iio: adc: ti-ads1298: Add driver |
| |
Hi Andy,
Took me a while to get back to this. Incorporated all of your comments, except that I seem to be unable to count characters. Most places, I have more than 80 characters in a line it I don't indent as I did. I'll try to improve my indents...
On 13-12-2023 15:55, Andy Shevchenko wrote: > On Wed, Dec 13, 2023 at 10:47:22AM +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. > ... > >> +config TI_ADS1298 >> + tristate "Texas Instruments ADS1298" >> + depends on SPI >> + select IIO_BUFFER >> + default y > Huh?! > >> + help >> + If you say yes here you get support for Texas Instruments ADS1298 >> + medical ADC chips >> + >> + This driver can also be built as a module. If so, the module will be >> + called ti-ads1298. > ... > >> +#include <linux/bitfield.h> >> +#include <linux/clk.h> >> +#include <linux/err.h> >> +#include <linux/delay.h> >> +#include <linux/device.h> >> +#include <linux/kernel.h> > Is it used as a proxy? (At least for array_size.h) > Please use real headers in such case. > >> +#include <linux/module.h> >> +#include <linux/regmap.h> >> +#include <linux/slab.h> >> +#include <linux/sysfs.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/regulator/consumer.h> >> +#include <linux/spi/spi.h> > This is interesting grouping, but okay, I understand the point. > >> +#include <linux/iio/iio.h> >> +#include <linux/iio/buffer.h> >> +#include <linux/iio/kfifo_buf.h> >> +#include <linux/iio/sysfs.h> > ... > >> +#define ADS1298_CLK_RATE 2048000 > Units? _HZ ? > > ... > >> +/* Outputs status word and 8 samples of 24 bits each, plus the command byte */ > /* Outputs status word and 8 24-bit samples, plus the command byte */ > > a bit shorter. > >> +#define ADS1298_SPI_RDATA_BUFFER_SIZE ((ADS1298_MAX_CHANNELS + 1) * 3 + 1) > ... > >> +#define ADS1298_CHAN(index) \ >> +{ \ >> + .type = IIO_VOLTAGE, \ >> + .indexed = 1, \ >> + .channel = index, \ >> + .address = 3 * index + 4, \ > Hmm... does this 3 have a distinct definition? > >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >> + BIT(IIO_CHAN_INFO_SCALE), \ > Can be written as below > > .info_mask_separate = \ > BIT(IIO_CHAN_INFO_RAW) | \ > BIT(IIO_CHAN_INFO_SCALE), \ > >> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ >> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > .info_mask_shared_by_all = \ > BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > >> + .scan_index = index, \ >> + .scan_type = { \ >> + .sign = 's', \ >> + .realbits = 24, \ >> + .storagebits = 32, \ >> + .endianness = IIO_BE, \ >> + }, \ >> +} > ... > >> +static int ads1298_write_cmd(struct ads1298_private *priv, u8 command) >> +{ >> + struct spi_transfer cmd_xfer = { >> + .tx_buf = priv->cmd_buffer, >> + .rx_buf = priv->cmd_buffer, >> + .len = 1, > sizeof(command) ? > >> + .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW, >> + .delay.value = 2, >> + .delay.unit = SPI_DELAY_UNIT_USECS, > .delay = { > .value = ... > .unit = ... > }, > >> + }; >> + priv->cmd_buffer[0] = command; >> + >> + return spi_sync_transfer(priv->spi, &cmd_xfer, 1); >> +} > ... > >> + /* Cannot take longer than 40ms (250Hz) */ >> + ret = wait_for_completion_timeout(&priv->completion, >> + msecs_to_jiffies(50)); > One line? > >> + if (!ret) >> + return -ETIMEDOUT; > ... > >> + if (cfg & ADS1298_MASK_CONFIG1_HR) >> + rate >>= 6; /* HR mode */ >> + else >> + rate >>= 7; /* LP mode */ > Are those magic numbers defined? > > ... > >> + factor = (rate >> 6) / val; > Is it the same 6 semantically as above? > > ... > >> + return IIO_VAL_FRACTIONAL_LOG2; >> +} >> + >> + > One blank line is enough. > >> + *val = sign_extend32(get_unaligned_be24( >> + priv->rx_buffer + chan->address), 23); > Strange indentation. > > *val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address), > 23);
Doesn't fit, first line is 83 characters by my count...
> ... > >> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >> + ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val); >> + if (!ret) { > Why not using standard pattern? > > if (ret) > return ret; > > (see below) > >> + ret = IIO_VAL_INT; >> + *val = (16 << (*val & ADS1298_MASK_CONFIG1_DR)); > Outer parentheses are redundant. > >> + } >> + break; > return IIO_VAL_INT; > > >> + default: >> + ret = -EINVAL; >> + break; > return directly. > >> + } >> + return ret; > It will gone. > > ... > >> +static int ads1298_write_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, int val, >> + int val2, long mask) >> +{ >> + struct ads1298_private *priv = iio_priv(indio_dev); >> + int ret; > No need, just return directly. > >> + switch (mask) { >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + ret = ads1298_set_samp_freq(priv, val); >> + break; >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + return ret; >> +} > ... > >> +static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg, >> + unsigned int writeval, unsigned int *readval) >> +{ >> + struct ads1298_private *priv = iio_priv(indio_dev); >> + if (!readval) > Perhaps positive conditional? > > if (readval) > return readval; > return writeval; > >> + return regmap_write(priv->regmap, reg, writeval); >> + >> + return regmap_read(priv->regmap, reg, readval); >> +} > ... > >> + /* Power down channels that aren't in use */ >> + for (i = 0; i < ADS1298_MAX_CHANNELS; i++) { >> + regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i), >> + ADS1298_MASK_CH_PD, >> + test_bit(i, scan_mask) ? >> + 0 : ADS1298_MASK_CH_PD); > Broken indentation. > >> + } > ... > >> +static void ads1298_rdata_unmark_busy(struct ads1298_private *priv) >> +{ >> + 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 = false; >> + spin_unlock_irqrestore(&priv->irq_busy_lock, flags); > Perhaps switch to use guard()? > (And scoped_guard() where it makes sense.) > >> +} >> + /* Transfer 24-bit value into 32-bit array */ >> + memcpy(bounce + 1, data, 3); > Hmm... Wouldn't get_unaligned_..24() work here better? > >> + bounce += 4; > If so, you can iterate over u32 members directly without this += 4. > > ... > >> +static const char *ads1298_family_name(unsigned int id) >> +{ >> + switch (id & 0xe0) { > GENMASK() ? > >> + case 0x80: >> + return "ADS129x"; >> + case 0xc0: >> + return "ADS129xR"; > Can we have these all magics be defined? > >> + default: >> + return "(unknown)"; >> + } >> +} > ... > >> + if ((val & 0x18) == 0x10) { > Ditto. > >> + dev_info(dev, "Found %s, %d channels\n", >> + ads1298_family_name(val), >> + 4 + 2 * (val & 0x07)); > Ditto for 0x07. > >> + } else { >> + dev_err(dev, "Unknown ID: 0x%x\n", val); >> + return -ENODEV; >> + } > ... > >> + indio_dev = devm_iio_device_alloc(dev, sizeof(*priv)); >> + if (indio_dev == NULL) > We do !indio_dev in this case. > >> + return -ENOMEM; > ... > >> + ret = devm_add_action_or_reset(dev, ads1298_reg_disable, >> + priv->reg_vref); > One line? > > >> + if (ret) >> + return ret; > ... > >> + return dev_err_probe(dev, PTR_ERR(priv->clk), >> + "Failed to get clk\n"); > Ditto. > > ... > >> + return dev_err_probe(dev, ret, >> + "Failed to enable avdd regulator\n"); > Ditto. > > ... > >> + ret = devm_add_action_or_reset(dev, ads1298_reg_disable, >> + priv->reg_avdd); > Ditto. > > ... > >> + priv->regmap = devm_regmap_init(dev, NULL, priv, >> + &ads1298_regmap_config); > Ditto. > > ... > >> + /* Avoid giving all the same name, iio scope doesn't handle that well */ > IIO > >> + indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "%s@%s", >> + spi_get_device_id(spi)->name, >> + dev_name(dev)); > No error check? > >> + if (reset_gpio) { >> + udelay(1); /* Minimum pulse width is 2 clocks at 2MHz */ > How do you know it's 2MHz? Is it always the same on all platforms / setups? > >> + 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 */ > Ditto. > >> + udelay(9); > ... > >> + ret = devm_request_irq(&spi->dev, spi->irq, &ads1298_interrupt, > &spi->dev is different to dev? > >> + IRQF_TRIGGER_FALLING, indio_dev->name, >> + indio_dev); > ... > >> + ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev, >> + &ads1298_setup_ops); > Ditto. > > ... > >> +static const struct spi_device_id ads1298_id[] = { >> + { "ads1298", }, > Inner comma is not needed. > >> + { } >> +}; > ... > >> +static const struct of_device_id ads1298_of_table[] = { >> + { .compatible = "ti,ads1298" }, >> + { }, > Drop the comma in terminator entry. > >> +};
-- 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@topic.nl W: www.topic.nl
| |