Re: [PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

[Matti Vaittinen]

On 05/04/2025 20:43, Jonathan Cameron wrote:
> On Tue, 1 Apr 2025 15:33:15 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
>
> > On 31/03/2025 14:22, Jonathan Cameron wrote:
> > > On Mon, 31 Mar 2025 11:03:58 +0300
> > > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> > >    
> > > > The ROHM BD79104 ADC has identical SPI communication logic as the
> > > > ti-adc128s052. Eg, SPI transfer should be 16 clk cycles, conversion is
> > > > started when the CS is pulled low, and channel selection is done by
> > > > writing the channel ID after two zero bits. Data is contained in
> > > > big-endian format in the last 12 bits.
> > >
> > > Nicely found match.  Sometimes these are tricky to spot.
> > >    
> > > > The BD79104 has two input voltage pins. Data sheet uses terms "vdd" and
> > > > "iovdd". The "vdd" is used also as an analog reference voltage. Hence
> > > > the driver expects finding these from the device-tree, instead of having
> > > > the "vref" only as TI's driver.
> > > >
> > > > NOTE: The TI's data sheet[1] does show that the TI's IC does actually
> > > > have two voltage inputs as well. Pins are called Va (analog reference)
> > > > and Vd (digital supply pin) - but I keep the existing driver behaviour
> > > > for the TI's IC "as is", because I have no HW to test changes, and
> > > > because I have no real need to touch it.
> > > >
> > > > NOTE II: The BD79104 requires SPI MODE 3.
> > > >
> > > > NOTE III: I used evaluation board "BD79104FV-EVK-001" made by ROHM. With
> > > > this board I had to drop the SPI speed below the 20M which is mentioned
> > > > in the data-sheet [2]. This, however, may be a limitation of the EVK
> > > > board, not the component itself.
> > > >
> > > > [1]: https://www.ti.com/lit/ds/symlink/adc128s052.pdf
> > > >
> > > > [2]:
> > > > https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf
> > > >   
> > > Prefer Datasheet tags with # [1]
> > > after them for the cross references.
> > >
> > > Those belong here in the tag block (no blank lines)
> > > > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> > >
> > > One request for an additional cleanup precursor patch given you are
> > > touching the relevant code anyway.   It's a small one that you can
> > > test so hope you don't mind doing that whilst here.
> > >
> > > I'm relying on the incredibly small chance anyone has a variable
> > > regulator wired up to the reference that they are modifying at runtime.
> > > I have seen that done (once long ago on a crazy dev board for a really
> > > noisy humidity sensor) when the reference was VDD but not on a separate
> > > reference pin.  That means we almost certainly won't break the existing
> > > parts and can't have a regression on your new one so we should be fine
> > > to make the change.
> >
> > The change you ask for is indeed small. I have no real objections
> > against implementing it (and I actually wrote it already) - but I am
> > still somewhat hesitant. As you say, (it seems like) the idea of the
> > original code is to allow changing the vref at runtime. It looks to me
> > this might've been intentional choice. I am not terribly happy about
> > dropping the working functionality, when the gained simplification isn't
> > particularly massive.
>
> Hmm. I suspect this was added at my request (or copied from where I requested
> it)  Back when we did this there was no advantage in doing it at probe
> as it was just a question of store a value or store a pointer we had
> to get anyway.  So I tended to advocate what I now think was a bit silly,
> that someone elses board might have it changing...
>
> User space wise, what code checks for random scaling changes?  So it
> was best effort at best anyway!

Ah, right. I suppose this should've been accompanied with scale setting 
which could've changed the regulator voltage - and I have no idea if 
such hardware would make any sense.

The slim chance I can imagine is that the reference voltage can't be 
set/known at probe time.

> > Because of this, I am thinking of adding the patch dropping the
> > functionality as an RFC. Leaving that floating on the list for a while
> > would at least have my ass partially covered ;)
> >
> > I'd rather not delayed the support for the BD79104 though. So - would it
> > be okay if I didn't implement the clean-up as a precursory patch, but
> > did it as a last patch of the series? That will make it a tad more
> > complex to review, but it'd allow taking the BD79104 changes in while
> > leaving the RFC to float on a list. (Also, I'm not sure if you can push
> > an RFC in next without taking it in for the cycle?)
>
> I'll probably just merge it even as an RFC :)  That way it's my
> fault if we break someone and they shout!

That's fine for me. Well, doing it this way around (as a last patch) 
should ease reverting, should it be needed.

Yours
	-- Matti