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.
...
> > > + 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.
> > > + 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.
...
> > > + 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?
...
> > > + 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?
> I did forget to use "cansleep" here, will add that.
--
With Best Regards,
Andy Shevchenko
