Received-SPF: pass (google.com: domain of linux-kernel+bounces-55107-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
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: <67387cf4-1065-4313-b4c6-054128ba8f3a@topic.nl>
Date: Tue, 6 Feb 2024 15:25:30 +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>
 <4c6654f5-2d9e-4c1b-a5de-7bdeacf5e99f@topic.nl>
 <ZcI5PoWojKRrdpVl@smile.fi.intel.com>
Organization: Topic
In-Reply-To: <ZcI5PoWojKRrdpVl@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:
 xrCUI7Jgjs23o8Ej7u312I6zGaCUe7fqBZPiAvIzdTjuRmpTwsT2C0PwF0A5gXMB/OcTxmJFNtx8eBSWrm8zNrE1GOkr3xu7xDBl/eLRqX8GATso5Sf5JsHhAvIZ5VXjrljXxK5koEOkhLMJ1BfI/olc9R2BhZ1b1P5xuZz1JKHBimmCQKDoF4NzLTkIbSeCKlk/b91rG9835o6TSfT/zdSeI/SUDF28HqYcexgHKD5VVrwybacdtBi/WxZfwgNwqe9l3jmpwSJM4XYcv3WYgVDQ20hAGnhjuR+H++RPiby5qOMmBUHu4HRdpdEQaRBkKYjK4czUX06gJmYBxJKFAq6ZXY6YDvwRq8PuvDbu3pc3W7d1xinQbyKidEzkdswvi0vpMpxJBV+usoTtBhEwytab97ElwA0UGxQMAWXpkSvEyCLNYsIcg4ykktdZre9U2VERjEOlgNly+cqhaJpviP5v6Di6ObmNedd5U6uUuYBkohUJ/Bf2vSvrHyZ05gKaWkj4k4J4OH5Nj+GKk7zqwm8wvPQ6/QfAwG/uf5QLghOQwaHRlYqOvwQ3Uvv1Xep+hRUpGP1nFCxyFOdTR9XFxfvMfpm2bZfDKQtOkBLPiH8UmxqfXjSfggSkdj50ubrygIAoNdgLyF83DThB58W6QsxKkVKklMWZW0SUh57XVh0=
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)(39830400003)(346002)(136003)(376002)(366004)(230922051799003)(186009)(1800799012)(64100799003)(451199024)(31686004)(2906002)(83380400001)(38100700002)(41300700001)(26005)(38350700005)(66556008)(6916009)(54906003)(66476007)(66946007)(316002)(31696002)(7416002)(86362001)(4326008)(8676002)(8936002)(44832011)(53546011)(52116002)(36916002)(2616005)(6506007)(6512007)(5660300002)(36756003)(15974865002)(6486002)(478600001)(45980500001)(43740500002);DIR:OUT;SFP:1102;
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB7213
X-CodeTwo-MessageID: 2d35f37d-0f97-4b54-bfad-0a1c71c834aa.20240206142536@westeu11-emailsignatures-cloud.codetwo.com
X-CodeTwoProcessed: true
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped:
 DB1PEPF0003922E.eurprd03.prod.outlook.com
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id-Prvs:
	983a7879-5b8f-4fe7-67bc-08dc271f7914
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	OuUNnY0BKjDsg4pdx6PnNix6uiBm4dW6WSkJrBpugScjIe/19T3gVIDfapMenTWflneRtXt38i//nPkxvyUfVM59b2ThaA0vqeMPsxSgRcUPPYqk5n/1/B9mrHh7zlr+6OiZUnzD63yVp+eB1XQx5KxA592GoUdSQs+mpBFGD2TRlyWjnHlkGiD95pCW+HaRSg4TWsG+CqTQyiTHAkM9aKfGRRNn2oBx1TOZhgHlUTF4HsPJOOD0QdXuwsvDxXZ3mA8yImdoEa1YBtbVZu890HHVVGqTMHRvKs/aa7b8hlQJur44JmB1r9UH8x/fKXCxr94iSsR4RPbenQMpe+Wsd6NQhwtuWXOB6tXDBdslp0GxNoGqC2EaUBupghhOHx674zUGqX3+5hdFHGvbVe2djNYonKEHPXgCRgS9m+W4zvn7PgpvVP1/SZNhEGsg/78qIKPuUY3JMctgWB205sxBaNepDPQX1xoaZ4vjB5mHsa+BYo194otVChBRVZFziVLb8Eopo4uErxZL2XprD1+cqbJwxxMxGFMj1hmd9D8rgcnUkFgEC0eKfowA+zKgRJYDEZlvSDXdBIDcxU8/ivOBs/564ZGGVi0ot1zrub32KJ2N7a1SOeVs5asVGk1YwCP18AZGN82NPNCHixdLSDbZdN3qFGjiWyB0u8hsCwH8KbwClns5DTZIVF66kIjg8+4PDZT58gK/xO6Em+OU4CVljmFLEMBY1us42Tx/H/okKQgiNkmHxP9QZMUG42oZJEoEH54wXTR2GUxqbR3sc1fXYg==
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)(346002)(39830400003)(136003)(396003)(376002)(230922051799003)(64100799003)(1800799012)(451199024)(82310400011)(186009)(46966006)(36840700001)(7636003)(356005)(7596003)(31686004)(47076005)(40480700001)(36916002)(53546011)(36860700001)(6506007)(83380400001)(26005)(31696002)(86362001)(6512007)(41300700001)(15974865002)(2616005)(7416002)(8676002)(8936002)(36756003)(4326008)(336012)(2906002)(478600001)(6486002)(5660300002)(316002)(70586007)(54906003)(44832011)(70206006)(6916009)(43740500002)(18886075002);DIR:OUT;SFP:1102;
X-OriginatorOrg: topic.nl
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 14:25:37.5129
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 68f64125-ac2d-4087-2355-08dc271f7d42
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:
	DB1PEPF0003922E.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8PR04MB6780

On 06-02-2024 14:50, Andy Shevchenko wrote:
> On Tue, Feb 06, 2024 at 02:33:47PM +0100, Mike Looijmans wrote:
>> On 06-02-2024 13:57, Andy Shevchenko wrote:
>>> On Tue, Feb 06, 2024 at 07:58:18AM +0100, Mike Looijmans wrote:
> ...
>
>>>> +	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 (facto=
r)'
>>> below. What do you think?
>> The "HR" bit sets the device to high-res mode (which apparently doubles =
the
>> internal sample rate).
>>
>> But "128" could be written as "1 << ADS1298_SHIFT_DR_LP" which is the ma=
x
>> oversampling factor.
> Use BIT(..._DR_LP) and we are done here.

Ok.


> ...
>
>>>> +	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.
> Yes, looks more obvious.

Having done that, and looking at it again, it's better to just eliminate=20
the local "wasbusy" altogether. More concise.


>
>>>> +	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 dr=
iver
>> called our completion callback without us calling spi_async first)
> You never know what may go wrong in the future :-) That said, I prefer ro=
bust
> code against non-robust.

Maybe: BUG_ON(!priv->rdata_xfer_busy)

Adds more code, dunno what weighs heavier... Haven't seen other drivers=20
do this though.

I made rdata_xfer_busy unsigned as it should have been.


> ...
>
>>>> +	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 =
same.
>>>
>>>> +		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 completi=
on
>>
>>> 2 =3D Multiple DRDY during SPI transfer, overflow, we lost=C2=A0rdata_x=
fer_busy -
>> 2 samples
>
> Maybe another good comment is needed here as well?

I thought I had it covered with the comments... I'll add more.


>
> ...
>
>>>> +	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 the=
m).
>>> Why is it here?
>> Compiler complains that the expression is "unsigned long". Probably beca=
use
>>   of ADS1298_MASK_ID_CHANNELS being so.
> So, use the unsigned long specifier and drop casting.
>
> ...
>
>>>> +	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 mo=
re
>>> sense.
>> If used, the reset must be asserted *before* the voltages and clocks are
>> activated. This would obfuscate that, and add a redundant call to set_va=
lue.
> Then perhaps you want reset framework to be used instead?
>
> Dunno, but this comment seems confusing in a way that you somewhere asser=
ted it
> and it's not obvious where and here is the delay out of a blue. Perhaps y=
ou may
> extend the comment?

Could use devm_reset_control_get_optional_shared() I guess, but that=20
would change devicetree bindings as well...

And it wouldn't change the order, as it'd still be asserted at the start=20
of probe()


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