Received-SPF: pass (google.com: domain of linux-kernel+bounces-68648-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 16 Feb 2024 13:38:16 +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>, 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 v4 2/2] iio: adc: ti-ads1298: Add driver
Message-ID: <20240216133816.18c42c99@jic23-huawei>
In-Reply-To: <20240214063736.88283-2-mike.looijmans@topic.nl>
References: <20240214063736.88283-1-mike.looijmans@topic.nl>
	<1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.da2f422c-5c6b-41b4-8916-d1a90f0f0973@emailsignatures365.codetwo.com>
	<20240214063736.88283-2-mike.looijmans@topic.nl>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 14 Feb 2024 07:37:36 +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>
> 
Hi Mike,

One final thing noticed on a (hopefully) last read through.

/sys/bus/iio:device0/name is going to read ads1298 whichever
chip is detected.

Would be more useful to users if it identified the actual
part given that is easily read from the ID register.

Jonathan

> ---
> 
> Changes in v4:
> Explain rdata_xfer_busy better and remove post-decrement
> Reset assert explanation and add cansleep
> Additional style changes
> 
> Changes in v3:
> Indentation fixups
> Remove unused headers
> Remove #define leftovers
> Use devm_get_clk_optional_enabled
> Use ilog2 instead of fls()-1
> Magic "23" replaced
> Explain the extra "0" in read/write register
> use guard() from cleanup.h
> use REGCACHE_MAPLE
> 
> Changes in v2:
> Remove accidental "default y" in Kconfig
> Indentation and similar cosmetic fixes
> Magic numbers into constants
> Short return paths in read_raw and write_raw
> DMA buffer alignment
> Bounce buffer is u32 instead of u8
> Avoid races using claim_direct_mode
> Check errors on all register accesses
> Immediate SPI restart to reduce underruns
> "name" is chip name, not unique

I missed this until having a final read through but it's not the chip name
in the driver currently - the name is always ads1298 despite there being a handy
ID register that tells us what we actually have.


> 
>  drivers/iio/adc/Kconfig      |  11 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ti-ads1298.c | 766 +++++++++++++++++++++++++++++++++++
>  3 files changed, 778 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-ads1298.c

> +
> +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)";
> +	}
> +}

..

> +static int ads1298_probe(struct spi_device *spi)
> +{

..

> +
> +	priv->tx_buffer[0] = ADS1298_CMD_RDATA;
> +	priv->rdata_xfer.tx_buf = priv->tx_buffer;
> +	priv->rdata_xfer.rx_buf = priv->rx_buffer;
> +	priv->rdata_xfer.len = ADS1298_SPI_RDATA_BUFFER_SIZE;
> +	/* Must keep CS low for 4 clocks */
> +	priv->rdata_xfer.delay.value = 2;
> +	priv->rdata_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
> +	spi_message_init_with_transfers(&priv->rdata_msg, &priv->rdata_xfer, 1);
> +	priv->rdata_msg.complete = &ads1298_rdata_complete;
> +	priv->rdata_msg.context = indio_dev;
> +
> +	indio_dev->name = spi_get_device_id(spi)->name;

I was going to just tweak this whilst applying.  Using the spi device id often
ends up being fragile in the long term because of the split between the different
ID tables and the mess that happens if fallback compatibles are in use and
the spi IDs are missing (you will get a warning about this at runtime
but it'll carry on anyway).

Easier to just hard code the name for now and when you have multiple
devices supported, add this to a chip_info type structure.
Or we could make it support the more specific name given the detection above.
Is there a reason to not do that given a more accurate name is
easy to work out and may be useful to a user?

Jonathan