Received-SPF: pass (google.com: domain of linux-kernel+bounces-55022-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::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: <4c6654f5-2d9e-4c1b-a5de-7bdeacf5e99f@topic.nl>
Date: Tue, 6 Feb 2024 14:33:47 +0100
User-Agent: Mozilla Thunderbird
From: Mike Looijmans <mike.looijmans@topic.nl>
Subject: Re: [PATCH v3 2/2] iio: adc: ti-ads1298: Add driver
Content-Language: en-US, nl
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: devicetree@vger.kernel.org, linux-iio@vger.kernel.org,
 Jonathan Cameron <jic23@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
References: <20240206065818.2016910-1-mike.looijmans@topic.nl>
 <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.fd628a1a-a926-426e-a239-bfd8c9858b94@emailsignatures365.codetwo.com>
 <20240206065818.2016910-2-mike.looijmans@topic.nl>
 <ZcIsuiuisQjTIxJv@smile.fi.intel.com>
Organization: Topic
In-Reply-To: <ZcIsuiuisQjTIxJv@smile.fi.intel.com>
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:
 lVBFOywXOD1oGo9aUS+NxClpTg5J/nsJyzVHkTi77aJFXOYR/vsPx8U3vXlNiqVOSABGs5xxNJuNyAC9719C4kYCXVfU7ML4nkpj/TIi9sQMzTMcazBaUbPFylmBpkCgJlffD+iTUjhTg0e630u3IFHqTChw1daqSXOtveTuaiK8lOHVQMVYkXTRprtPSpD1rBANlhQJqLCrBQp5Fb5/9F5poZV97NHbV3fNynzlr+vekGZYvTESjWDosclchWhcbOi/iSkxNLrs4cGupjN0m1O+5P9QFo9Cp6KMLMqEoq1dX1ENK6X457i7rkNMcszkaZH5GB6woxE81uWvJ2M1YBV0XLlBfPhlyhFyCSE8xLh5kHSL23ixUazUH5pm7w9Jd4k2W4BhexY5jpkyBdjD4CPbzF6xCiE0qrwzarDQz0MTL29fEDPy4afQXM41AU0A8gu/mBzYu/UZYib4XpVEbqX3mEkb1u3QPU34T0z5uaXRGE61r2DzGK5cvfB9BDlCmOxCSMQrfSa93zIv23b3pD82ILaT5mZ96M7TIjU7coTOwoSOls5RMF7jqFu1nAX28aiDhYMjAA+qDAdmJwUS2gW+FPr7EXQBHrE2IrYz2IsnGs8poTpbLXkO02u/Lkg1394B0btk1CH98dqjM1w67QqKDPN0TCwGl3kW7u3qJ+A=
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)(366004)(396003)(346002)(136003)(376002)(39840400004)(230922051799003)(64100799003)(1800799012)(186009)(451199024)(478600001)(2616005)(52116002)(36756003)(38350700005)(53546011)(36916002)(6506007)(6512007)(41300700001)(38100700002)(15974865002)(26005)(86362001)(83380400001)(31696002)(6486002)(8676002)(4326008)(8936002)(2906002)(7416002)(31686004)(5660300002)(44832011)(54906003)(66476007)(66556008)(316002)(66946007)(6916009)(43740500002)(45980500001);DIR:OUT;SFP:1102;
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS5PR04MB10043
X-CodeTwo-MessageID: 8b0bce26-243a-475a-94b5-8de6178611d4.20240206133351@westeu11-emailsignatures-cloud.codetwo.com
X-CodeTwoProcessed: true
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped:
 DU6PEPF00009527.eurprd02.prod.outlook.com
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id-Prvs:
	c1d034eb-e0ff-41e6-eb2a-08dc27183fe4
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	+5//xjd6YuTo5Nnq7MJsRdSwtF1LYHruDSXITF17YBH5oFiJYtdQ+iKA8vCCE2+2MFjfZOVJg+z/HEtwQkYQhkqyiihFEovFuqPhYVqNF2UCbz7KWNJVCxyjOQ/GcWKV+s7eHY5GjpDxGMIgMYoJAw1VhGsms/fy88GPCBuoh5gjOyRU9Bv+VqMlfWP9EnSQV5SKbij3Ux7iU3XgKAt9xarfHkCldGPr1SUApLKnHFKbu/CwJPtUHkMik9hgmQlGPNs6R5e2Q/5W6CB+2mDyDSS7GEcQ6zgwiTpOUB5phhODkdadhnc+asqgDinvU6pVMCRSeoVd+OWPvvZ07n3LwXby7SiAxSiHw8DbNQQO3SR+iw4q3m9zUb9Zhl8sGKhddTbDDn+KLRYJ+CiIGbH0LCk+wK2HxoZ65WrnABdUFXHwo9brEoWbq70v88WfncsPP1WHLlHG1B0dyPjjpjg0zF0f8gbVW4RoKSQuxgJTuGn/54u5X9nzSMjrPHs3FBsj9xI8f1zNR58+Ym+QD2ZZcllHoj2KB5i9aFXqRYQkdYbs7YyTLGnvr6mgnYZlJxg+RmIUTk1LQ8QIRK9ESSmYJF7pqlvnnqED82B3z1oNKMMbsWu3nDq00tbXfKyT4I0WsAKYCyclZtUSR3Yg8fx0BpD2Z7xFh+F73p4tFRwW8Wirxr6nQkoicxvdil9qq4pxQDGAPYRSsZ1RlXDkzpUFWWf1FlcKYo/nTc72scQsxo3Jl+EXXvPGIvvbjbPQSeOQhR44647z6CJxi31BgCTPRg==
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)(376002)(39840400004)(396003)(136003)(346002)(230922051799003)(451199024)(64100799003)(82310400011)(186009)(1800799012)(46966006)(36840700001)(336012)(86362001)(15974865002)(2616005)(26005)(41300700001)(31696002)(7416002)(6486002)(5660300002)(2906002)(478600001)(316002)(6916009)(54906003)(70206006)(70586007)(36756003)(4326008)(8676002)(8936002)(6512007)(44832011)(6506007)(36916002)(53546011)(36860700001)(40480700001)(47076005)(7636003)(356005)(7596003)(31686004)(83380400001)(43740500002)(18886075002);DIR:OUT;SFP:1102;
X-OriginatorOrg: topic.nl
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 13:33:52.8660
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b92c7f7-628d-4d04-5f72-08dc271842e2
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:
	DU6PEPF00009527.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV1PR04MB9184

On 06-02-2024 13:57, Andy Shevchenko wrote:
> On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans 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.
> Thanks for an update, I have only a few style comments and a single one a=
bout
> comparison (see below). If you are going to address them as suggested, fe=
el
> free to add
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> to the next version.
>
> ...

Thanks for reviewing...


>> +/* Registers */
>> +#define ADS1298_REG_ID		0x00
>> +#define ADS1298_MASK_ID_FAMILY			GENMASK(7, 3)
>> +#define ADS1298_MASK_ID_CHANNELS		GENMASK(2, 0)
>> +#define ADS1298_ID_FAMILY_ADS129X		0x90
>> +#define ADS1298_ID_FAMILY_ADS129XR		0xd0
> + Blank line? (And so on for all registers that have bitfields defined)

Makes sense... Looks too cluttered as it is now.


>
>> +#define ADS1298_REG_CONFIG1	0x01
>> +#define ADS1298_MASK_CONFIG1_HR			BIT(7)
>> +#define ADS1298_MASK_CONFIG1_DR			GENMASK(2, 0)
>> +#define ADS1298_SHIFT_DR_HR			6
>> +#define ADS1298_SHIFT_DR_LP			7
>> +#define ADS1298_LOWEST_DR			0x06
> ...
>
>> +	factor =3D (rate >> ADS1298_SHIFT_DR_HR) / val;
>> +	if (factor >=3D 128)
> I just realized that this comparison is probably better in a form
>
> 	if (factor >=3D ADS1298_MASK_CONFIG1_HR)
>
> as it points out why this is a special case in comparison to 'if (factor)=
'
> below. What do you think?

The "HR" bit sets the device to high-res mode (which apparently doubles=20
the internal sample rate).

But "128" could be written as "1 << ADS1298_SHIFT_DR_LP" which is the=20
max oversampling factor.


> ...
>
>> +	wasbusy =3D --priv->rdata_xfer_busy;
> Why preincrement? How would it be different from postincrement?

Maybe better write this as:

--priv->rdata_xfer_busy;

wasbusy =3D priv->rdata_xfer_busy;

I want the value after decrementing it.


>> +	if (wasbusy) {
> To me more robust code would look like
>
> 	if (wasbusy < 1)
> 		return;
> 	...
> 	if (wasbusy > 1)
> 		...

wasbusy could have been unsigned.

This code will only ever execute with rdata_xfer_busy > 0 (or the SPI=20
driver called our completion callback without us calling spi_async first)

>
>> +		/*
>> +		 * DRDY interrupt occurred before SPI completion. Start a new
>> +		 * SPI transaction now to retrieve the data that wasn't latched
>> +		 * into the ADS1298 chip's transfer buffer yet.
>> +		 */
>> +		spi_async(priv->spi, &priv->rdata_msg);
>> +		/*
>> +		 * If more than one DRDY took place, there was an overrun. Since
>> +		 * the sample is already lost, reset the counter to 1 so that
>> +		 * we will wait for a DRDY interrupt after this SPI transaction.
>> +		 */
>> +		if (wasbusy > 1)
>> +			priv->rdata_xfer_busy =3D 1;
>> +	}
> ...
>
>> +		/*
>> +		 * for a single transfer mode we're kept in direct_mode until
> For
>
>> +		 * completion, avoiding a race with buffered IO.
>> +		 */
> ...
>
>> +	wasbusy =3D priv->rdata_xfer_busy++;
> So, it starts from negative?
>
>> +	/* When no SPI transfer in transit, start one now */
>> +	if (!wasbusy)
> To be compatible with above perhaps
>
> 	if (wasbusy < 1)
>
> also makes it more robust (all negative numbers will be considered the sa=
me.
>
>> +		spi_async(priv->spi, &priv->rdata_msg);

The "rdata_xfer_busy" starts at 0.

Increments when a DRDY occurs.

Decrements when SPI completion is reported.

So the meaning of "rdata_xfer_busy" is:

0 =3D Waiting for DRDY interrupt

1 =3D SPI transfer in progress

2 =3D DRDY occured during SPI transfer, should start another on completion

 >2 =3D Multiple DRDY during SPI transfer, overflow, we=20
lost=C2=A0rdata_xfer_busy - 2 samples


> ...
>
>
>> +	dev_dbg(dev, "Found %s, %u channels\n", ads1298_family_name(val),
>> +		(unsigned int)(4 + 2 * (val & ADS1298_MASK_ID_CHANNELS)));
> Castings in printf() is a big red flag usually (it's rarely we need them)=
.
> Why is it here?

Compiler complains that the expression is "unsigned long". Probably=20
because of ADS1298_MASK_ID_CHANNELS being so.


> ...
>
>> +	if (reset_gpio) {
>> +		/* Minimum reset pulsewidth is 2 clock cycles */
>> +		udelay(ADS1298_CLOCKS_TO_USECS(2));
>> +		gpiod_set_value(reset_gpio, 0);
> I would rewrite it as
>
> 		/* Minimum reset pulsewidth is 2 clock cycles */
> 		gpiod_set_value(reset_gpio, 1);
> 		udelay(ADS1298_CLOCKS_TO_USECS(2));
> 		gpiod_set_value(reset_gpio, 0);
>
> to be sure we have a reset done correctly, and the comment will make more
> sense.

If used, the reset must be asserted *before* the voltages and clocks are=20
activated. This would obfuscate that, and add a redundant call to set_value=
.

I did forget to use "cansleep" here, will add that.


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