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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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








[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux