linux-kernel - Re: [PATCH v4 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]

Date: Fri, 16 Feb 2024 15:01:26 +0100
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Jonathan Cameron <jic23@...nel.org>
CC: devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 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 v4 2/2] iio: adc: ti-ads1298: Add driver

On 16-02-2024 14:38, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 07:37:36 +0100
> Mike Looijmans <mike.looijmans@...ic.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@...ic.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.

Makes sense. So It would say "ads1296" or "ads1298r" for example. I 
guess we prefer all lower-case here.


>
> 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?
>
The only reason I tried to make the "name" unique is that IIO 
oscilloscope (ab)uses "name" as a unique identifier. That's something 
that ought to be fixed in IIO oscilloscope.

Giving the chip name would reveal the actual detected hardware, there's 
currently no other way for the user to find that out.

-- 
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