Received-SPF: pass (google.com: domain of linux-kernel+bounces-68686-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Received-SPF: Pass (protection.outlook.com: domain of topic.nl designates
 20.93.157.195 as permitted sender) receiver=protection.outlook.com;
 client-ip=20.93.157.195; helo=westeu11-emailsignatures-cloud.codetwo.com;
 pr=C
Authentication-Results-Original: dkim=none (message not signed)
 header.d=none;dmarc=none action=none header.from=topic.nl;
Message-ID: <8a215799-c7cb-491e-8318-76d7e7f64bd5@topic.nl>
Date: Fri, 16 Feb 2024 15:01:26 +0100
User-Agent: Mozilla Thunderbird
From: Mike Looijmans <mike.looijmans@topic.nl>
Subject: Re: [PATCH v4 2/2] iio: adc: ti-ads1298: Add driver
To: Jonathan Cameron <jic23@kernel.org>
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
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>
 <20240216133816.18c42c99@jic23-huawei>
Content-Language: en-US, nl
Organization: Topic
In-Reply-To: <20240216133816.18c42c99@jic23-huawei>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
X-Microsoft-Antispam-Message-Info-Original:
 7sMFTz1yCxcHofPl0X0c3E7+vOgiULiDwXpdbJfFPJwn9s+hea/v+CyuxSz/o7sIx1sl4SdLYrxEnbzPbvyOhwcFODLnlLKICMVZM7nWSjdwexK/K/Q9Mklo3ShHar5Sqs5Sm9uBPfmRQOdpTgyV1JwYTV+CXDmu+z3G5jUhQsYIxydG7BUY+UqnQp3YnjZUm5DUoqwlPhzRroHBxgfX/KIEQbYw59v7+E4wJSe9KMo1g+8A+6DaHeRMtnyashHmOax7ELfrO/U+jn+5DNwQzMRHylA+6Shmkf7f2QKJ8QZwjrqOJ8dhj+pmGOvPGNEkb8TBza5vGhjJbpXmNk2QBZIR2dYcj1Xcxozx1PPuPBT1BdM6T6X25dd5Jy0egbT7hvMRH11z5b/t22HLEu9zp1p1YS8ekxzk8Asj9hBnAIrbosJU4iqL/7Q4x7UQaFAQFmZDhSziPPYFo3YMZV1PTDDIM2COL+lQ383Tkd0Qsya0eQeFxTI3cOESSJfOIphWZ9TIO2GMKPi0OcGYBouMYuxU3Rpjo1rqHdwqH+UVbBVDi/pq8l+Df6HwVsSAQNzaB9tqosjP6cqMTovn6dHLMg2GZsXoBZBVh6ISI9cgtzE=
X-Forefront-Antispam-Report-Untrusted:
 CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AS8PR04MB8644.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(396003)(376002)(136003)(366004)(346002)(39840400004)(230922051799003)(230273577357003)(451199024)(1800799012)(64100799003)(186009)(15974865002)(31696002)(36756003)(86362001)(53546011)(36916002)(52116002)(316002)(478600001)(41300700001)(6506007)(6512007)(54906003)(83380400001)(26005)(2616005)(38100700002)(6486002)(38350700005)(44832011)(8676002)(8936002)(4326008)(2906002)(7416002)(6916009)(66946007)(31686004)(5660300002)(66556008)(66476007);DIR:OUT;SFP:1102;
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR04MB8784
X-CodeTwo-MessageID: 35f08e41-1b7c-48aa-91f0-fcba0ea311e6.20240216140130@westeu11-emailsignatures-cloud.codetwo.com
X-CodeTwoProcessed: true
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped:
 DB5PEPF00014B93.eurprd02.prod.outlook.com
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id-Prvs:
	e0a70c5d-2dea-4895-ee5f-08dc2ef7c512
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	dv5Io9LtfmwItufyB4oseOiHIqp85/pizQrPEagU1yaUYp/W6GY0EN586+VHm3CiVggcDvxtHjfaifoYz/h0oW5+Yy3eGol7qchEILoFaRYI9F2j9/14pBD/bkTxF3FnmWyrwG3RJgXqOP2zxQ13m/An5M9y+jI3u2Ov5TmuL2djDrr/xAdGfLUXQKHL5wIJoPki3hOxGROWihmtZMkMDZEHqp5mAc90MoaxsjbaUCOmqiz6//9FiclKqC7nJxT68avfycnOb35o09kOyUAHl5zEunoAAAs61HcoRsgTwSQIAA8Cp6Qm13Qg4XrKdtJ+fXGZAw4EpU+DODdZM5NcHeimS9+CetR2mwWsboQ/7SGFK8LIsofqJeII7n7yYKLHGzbibz/4/53PHpgKO8dUOT+gOqlRWziP6TIu0ZzvgqtOkYIdxCBjC2oy5v/eO5BHKfNttLkRQH4FIj8/GZJu8nw0jhGCRGeq/cjnqEYJIX1qEfNWhinDGV6GyS+3TWI1CtzGGhLDOA1HMb04vqbEpTKg84moOugCp+nGQnuM5lAMgIApWqYbR2khOc14BYHo7dGYehZipxbKV4eiWTPv+UB1+v4iep8sj7aVAdPLfwUisKZ9sCEFWm41btyzCfMDIPYYBgwdHkgx4L9VbFZOuA==
X-Forefront-Antispam-Report:
	CIP:20.93.157.195;CTRY:NL;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:westeu11-emailsignatures-cloud.codetwo.com;PTR:westeu11-emailsignatures-cloud.codetwo.com;CAT:NONE;SFS:(13230031)(4636009)(136003)(346002)(376002)(39840400004)(396003)(230273577357003)(230922051799003)(82310400011)(186009)(451199024)(64100799003)(1800799012)(36860700004)(46966006)(6512007)(53546011)(2616005)(6486002)(478600001)(6506007)(336012)(26005)(7416002)(41300700001)(2906002)(4326008)(5660300002)(44832011)(6916009)(36916002)(316002)(70206006)(70586007)(8676002)(8936002)(86362001)(31696002)(15974865002)(36756003)(83380400001)(7636003)(7596003)(356005)(54906003)(31686004);DIR:OUT;SFP:1102;
X-OriginatorOrg: topic.nl
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2024 14:01:31.9781
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 2cf93b90-1b28-42af-5df5-08dc2ef7c7c1
X-MS-Exchange-CrossTenant-Id: 449607a5-3517-482d-8d16-41dd868cbda3
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=449607a5-3517-482d-8d16-41dd868cbda3;Ip=[20.93.157.195];Helo=[westeu11-emailsignatures-cloud.codetwo.com]
X-MS-Exchange-CrossTenant-AuthSource:
	DB5PEPF00014B93.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR04MB9391

On 16-02-2024 14:38, Jonathan Cameron wrote:
> 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.

Makes sense. So It would say "ads1296" or "ads1298r" for example. I=20
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 nam=
e
> 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] =3D ADS1298_CMD_RDATA;
>> +	priv->rdata_xfer.tx_buf =3D priv->tx_buffer;
>> +	priv->rdata_xfer.rx_buf =3D priv->rx_buffer;
>> +	priv->rdata_xfer.len =3D ADS1298_SPI_RDATA_BUFFER_SIZE;
>> +	/* Must keep CS low for 4 clocks */
>> +	priv->rdata_xfer.delay.value =3D 2;
>> +	priv->rdata_xfer.delay.unit =3D SPI_DELAY_UNIT_USECS;
>> +	spi_message_init_with_transfers(&priv->rdata_msg, &priv->rdata_xfer, 1=
);
>> +	priv->rdata_msg.complete =3D &ads1298_rdata_complete;
>> +	priv->rdata_msg.context =3D indio_dev;
>> +
>> +	indio_dev->name =3D 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 d=
ifferent
> ID tables and the mess that happens if fallback compatibles are in use an=
d
> 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 ab=
ove.
> 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=20
oscilloscope (ab)uses "name" as a unique identifier. That's something=20
that ought to be fixed in IIO oscilloscope.

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

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