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@xxxxxxxx
W: www.topic.nl
