Message-ID: <99d1d972-d9a5-4ca3-811a-b22083bea4e6@gmail.com>
Date: Mon, 7 Apr 2025 09:10:05 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
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 05/04/2025 20:43, Jonathan Cameron wrote:
> 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!

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

Powered by blists - more mailing lists