Messages in this thread | | | Date | Mon, 5 Feb 2024 16:25:19 +0100 | From | Mike Looijmans <> | Subject | Re: [PATCH v2 2/2] iio: adc: ti-ads1298: Add driver |
| |
Assume "yes" to all suggestions not mentioned below.
I'll send out a v3 later.
On 05-02-2024 14:55, Andy Shevchenko wrote: > 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). > > ...
The ads124s08 command list is the same, but that's about all that's similar.
>>> + 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...
Yes, probe() initializes the structs, they're used in the interrupt routines.
> >>> + 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?
I understood from Jonathan that that wasn't needed... "My" SPI controller is rather dumb and doesn't even have DMA.
Will take a closer look though.
>>> + else >>> + rate = ADS1298_CLK_RATE_HZ; > Dead code (here and elsewhere). You probably wanted _optional clk APIs > in the probe.
Yes "optional" was intended.
> ... > >>> +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().
Added comment to explain the first zero is the "number of registers to read/write minus one".
> ... > >>> + 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? > > ...
Will dive into it, is new to me... Looks like C++ "scoped_xxx" at a first glance.
> >>> +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.
Yeah, need to configure them all. Most common is that the cached value is already okay and no actual write register will happen.
> >>> + 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).
Probably more efficient to access the device registers in a natural order.
> ... > >>> + .cache_type = REGCACHE_RBTREE, > Why not MAPPLE TREE?
Reading the description this driver isn't a good candidate - the map isn't sparse and the hardware can do bulk read/write (though the driver doesn't use it).
..
-- 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
| |