[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <67387cf4-1065-4313-b4c6-054128ba8f3a@topic.nl>
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