Message-ID: <20250405184346.3c4b1234@jic23-huawei>
Date: Sat, 5 Apr 2025 18:43:46 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Nuno Sa
 <nuno.sa@...log.com>, David Lechner <dlechner@...libre.com>, Javier
 Carrasco <javier.carrasco.cruz@...il.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

On Tue, 1 Apr 2025 15:33:15 +0300
Matti Vaittinen <mazziesaccount@...il.com> wrote:

> On 31/03/2025 14:22, Jonathan Cameron wrote:
> > On Mon, 31 Mar 2025 11:03:58 +0300
> > Matti Vaittinen <mazziesaccount@...il.com> 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@...il.com>  
> > 
> > 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!
 
> 
> 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!

Jonathan

> 
> Yours,
> 	-- Matti
> 

Powered by blists - more mailing lists