Received-SPF: pass (google.com: domain of linux-kernel+bounces-69007-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 16 Feb 2024 17:01:49 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Mike Looijmans <mike.looijmans@topic.nl>
Cc: devicetree@vger.kernel.org, linux-iio@vger.kernel.org, Andy Shevchenko
 <andriy.shevchenko@linux.intel.com>, Arnd Bergmann <arnd@kernel.org>,
 Lars-Peter Clausen <lars@metafoo.de>, Liam Beguin <liambeguin@gmail.com>,
 Liam Girdwood <lgirdwood@gmail.com>, Maksim Kiselev
 <bigunclemax@gmail.com>, Marcus Folkesson <marcus.folkesson@gmail.com>,
 Marius Cristea <marius.cristea@microchip.com>, Mark Brown
 <broonie@kernel.org>, Niklas Schnelle <schnelle@linux.ibm.com>, Okan Sahin
 <okan.sahin@analog.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/2] iio: adc: ti-ads1298: Add driver
Message-ID: <20240216170149.040ff86b@jic23-huawei>
In-Reply-To: <20240216153020.485201-2-mike.looijmans@topic.nl>
References: <20240216153020.485201-1-mike.looijmans@topic.nl>
	<1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.52058f93-4994-4138-8075-7b04acaef74b@emailsignatures365.codetwo.com>
	<20240216153020.485201-2-mike.looijmans@topic.nl>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 16 Feb 2024 16:30:20 +0100
Mike Looijmans <mike.looijmans@topic.nl> 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.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
> Changes in v5:
> Derive the name from the chip ID register
> Fail probe if the ID is unknown

This is a fun corner when it comes to DT bindings where we may
have fallback compatibles for parts introduced in the future.
For those we are supposed to ignore ID registers and just
assume they are correct (in IIO I tends to suggest we print
a message to say we are carrying on despite not recognising the
ID)  We could go half way here and assume the channel count
can always be gotten from the register, but relax the family bit
to allow families that aren't yet 'born'.

If we don't assume channel count, we would need to add explicit
entries to the ID tables for each of the device supported.

Anyhow, I have no problem leaving that as a problem for another day.

> Interpret the number of channels (only tested the "8")

The way you've done this will not work if you want to add a timestamp
(which is tricky in this device anyway) but for now it is fine.

I tweaked the order in kconfig and Makefile whilst applying.
The relevant sections aren't in alphanumeric order as currently
the TI parts with longer names come later. Having said that this
fits better after ADS1100 than where you have it so I've moved
it up to there.

However...

  CHECK   drivers/iio/adc/ti-ads1298.c                                                                                                             
drivers/iio/adc/ti-ads1298.c:424:13: warning: context imbalance in 'ads1298_rdata_unmark_busy' - wrong count at exit
drivers/iio/adc/ti-ads1298.c:465:9: warning: context imbalance in 'ads1298_rdata_release_busy_or_restart' - wrong count at exit
drivers/iio/adc/ti-ads1298.c:531:9: warning: context imbalance in 'ads1298_interrupt' - wrong count at exit
  MODPOST Module.symvers                                                 
sparse isn't happy (and I upgraded it to make sure I had anything new for guard() etc)

I think it is the missing context tracking referred to here:
https://lore.kernel.org/linux-sparse/Zag2fYsyJDtDR7a6@google.com/

Anyhow, looks like false positives so applied to the togreg branch of iio.git
but first pushed out as testing to let 0-day take a look.

Thanks,

Jonathan