Re: [PATCH v11 2/3] iio: adc: max14001: New driver

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

On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote:
> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

...

> > Change I was not able to do:
> > - I could not remove bitrev16 because I am using an SPI controller that
> > does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using
> > the spi-lsb-first devicetree property for now, since this driver currently
> > works for both types of controllers: those that support it and those that
> > do not. I left a TODO comment to address this issue as soon as the SPI
> > kernel code starts handling the bit-reverse operation for controllers that
> > do not have this support. Once I finish my work on this driver, if the SPI
> > code still does not include this handling, I can submit patches to add it.
> 
> I looked more at what it would take to implement this in the SPI core code
> and found that it would actually be quite difficult to do in a generic way
> because there are so many edge/corner/n-dim cases. We can't change tx_buf
> data in-place because it might be const data that is in some memory area
> that can't be modified. And things would get complicated if different
> transfers pointed to the same buffer memory addresses anyway. So we would
> basically have to allocate new memory for all buffers, copy all tx data to
> that new memory, reverse all of the tx bits, and update all the pointers in
> the transfer structs. Then when the message was finished, we would have to
> reverse all of the rx bits, copy all of the rx buffers back to the original
> buffers and put all the buffer pointers back the way they were. But this
> could write over some of the original tx data if tx_buf and rx_buf point to
> the same original buffer, which would break things if a peripheral driver
> expected the tx data to persist.

And what's the problem here? We do the same with bounce-buffers in case
of DMA/IOMMU (okay, without actual data modification, but it's possible
on-the-fly).

> And we can't do this during the SPI optimize
> step because that currently allows the tx_buf data values to be modified after
> optimization.

This I don't know, so perhaps it's indeed a showstopper.

> So perhaps it is best to just handle it in the peripheral driver. It will
> be much more efficent that way anyway.
> 
> However, we still do want to handle SPI_LSB_FIRST now so that people with
> hardware support can be more efficient and we don't want things to break
> if someone puts spi-lsb-first in the devicetree.

...

> > +	if (ret < 0)
> > +		ret = 1250000;
> > +	else
> > +		ext_vrefin = 1;
> > +	st->vref_mV = ret / (MICRO / MILLI);
> 
> Just a style choice here, but in other drivers with similar handling
> we wrote it like this to avoid the extra if statement:

I didn't get this. You move from clear if to not-so-clear ternary. How is
the proposed code better?

> 	if (ret < 0 && ret != -ENODEV)
> 		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
> 
> 	ext_vrefin = ret != -ENODEV;
> 	st->vref_mV = ext_vrefin ? ret / 1000 : 1250;
> 
> Keeping (MICRO / MILLI) instead of 1000 is fine too. There are varying opinions
> on this.

> Or we could drop ext_vrefin and have:

It goes back and force. Can we keep the code as it's in this version?

> 	if (ret < 0 && ret != -ENODEV)
> 		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
> 
> 	if (ret != -ENODEV) {
> 		st->vref_mV = ret / 1000;
> 
> 		/* regmap set bits goes here. */
> 		... 
> 	} else {
> 		st->vref_mV = 1250;
> 	}

...

> > +			return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
> These lines are getting very long. We try to wrap to 80 characters
> as much as we can in the IIO subsystem.

Side note: checkpatch.pl almost never complained (okay, something like 15y+
ago) on long string literals at the end of statements. For the code lines
I fully support the wrapping.

-- 
With Best Regards,
Andy Shevchenko