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

[Jonathan Cameron]

On Mon, 7 Apr 2025 09:10:05 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

> 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.

In theory possible but I suspect expensive as a controllable precision
reference would be needed (a DAC probably wouldn't have enough current?)

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

Agreed. It can in theory happen and did on one ancient board I had
where the reference voltage was wired to a pair of devices and one
had a higher minimum voltage than the other. That was pre DT times though
and I suspect now we'd just put the voltage that works for both in DT.

> 
> >> 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.
Absolutely.

> 
> Yours
> 	-- Matti