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

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 = (rate >> ADS1298_SHIFT_DR_HR) / val;
>>>> +	if (factor >= 128)
>>> I just realized that this comparison is probably better in a form
>>>
>>> 	if (factor >= 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 the
>> internal sample rate).
>>
>> But "128" could be written as "1 << ADS1298_SHIFT_DR_LP" which is the max
>> oversampling factor.
> Use BIT(..._DR_LP) and we are done here.

Ok.


> ...
>
>>>> +	wasbusy = --priv->rdata_xfer_busy;
>>> Why preincrement? How would it be different from postincrement?
>> Maybe better write this as:
>>
>> --priv->rdata_xfer_busy;
>>
>> wasbusy = 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 
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 driver
>> called our completion callback without us calling spi_async first)
> You never know what may go wrong in the future :-) That said, I prefer robust
> code against non-robust.

Maybe: BUG_ON(!priv->rdata_xfer_busy)

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

I made rdata_xfer_busy unsigned as it should have been.


> ...
>
>>>> +	wasbusy = 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 = Waiting for DRDY interrupt
>>
>> 1 = SPI transfer in progress
>>
>> 2 = DRDY occured during SPI transfer, should start another on completion
>>
>>> 2 = Multiple DRDY during SPI transfer, overflow, we lost rdata_xfer_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 them).
>>> Why is it here?
>> Compiler complains that the expression is "unsigned long". Probably because
>>   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 more
>>> 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_value.
> Then perhaps you want reset framework to be used instead?
>
> Dunno, but this comment seems confusing in a way that you somewhere asserted it
> and it's not obvious where and here is the delay out of a blue. Perhaps you may
> extend the comment?

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

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


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

Powered by blists - more mailing lists