Re: [PATCH v3 2/2] iio: adc: ti-ads1298: Add driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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