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