linux-kernel - Re: [PATCH v2 2/2] iio: adc: ti-ads1298: Add driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7d7ea4e4-fcae-4966-b194-e1d328751b6b@topic.nl>
Date: Mon, 5 Feb 2024 16:25:19 +0100
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
 Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
 Liam Beguin <liambeguin@...il.com>, Liam Girdwood <lgirdwood@...il.com>,
 Maksim Kiselev <bigunclemax@...il.com>,
 Marcus Folkesson <marcus.folkesson@...il.com>,
 Marius Cristea <marius.cristea@...rochip.com>,
 Mark Brown <broonie@...nel.org>, Niklas Schnelle <schnelle@...ux.ibm.com>,
 Okan Sahin <okan.sahin@...log.com>, linux-kernel@...r.kernel.org
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@...ic.nl
W: www.topic.nl