Message-ID: <6bfe66a8-5c98-4e1f-90dd-60a8f45f213d@gmail.com>
Date: Mon, 17 Mar 2025 09:34:31 +0200
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>, Nuno Sa <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Olivier Moysan <olivier.moysan@...s.st.com>,
 Guillaume Stols <gstols@...libre.com>,
 Dumitru Ceclan <mitrutzceclan@...il.com>,
 Trevor Gamblin <tgamblin@...libre.com>,
 Matteo Martelli <matteomartelli3@...il.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 João Paulo Gonçalves <joao.goncalves@...adex.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v7 06/10] iio: adc: Support ROHM BD79124 ADC

On 16/03/2025 13:02, Jonathan Cameron wrote:
> On Thu, 13 Mar 2025 09:19:03 +0200
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
>> an automatic measurement mode, with an alarm interrupt for out-of-window
>> measurements. The window is configurable for each channel.
>>
>> The I2C protocol for manual start of the measurement and data reading is
>> somewhat peculiar. It requires the master to do clock stretching after
>> sending the I2C slave-address until the slave has captured the data.
>> Needless to say this is not well suopported by the I2C controllers.
>>
>> Thus the driver does not support the BD79124's manual measurement mode
> Given you are going to be doing a v8 and I'm bored on a train, so utterly
> trivial comments that you get as a frequent contributor as things to
> consider for future patches. (I'm sure it's just what you always wanted
> :)

I don't mind these! :)

I can admit (and I have admitted it), sometimes I get irritated by 
comments. Maybe that's just natural. Still, in many occasions I am not 
irritated/annoyed at all, but I am actually genuinely glad I've learnt a 
thing or 2. Maybe it is also connected to the tone of voice when 
comments are given?

> In theory should be imperative though I don't care as much as some.
> 
> Hence, do not support the....
> 
>> but implements the measurements using automatic measurement mode relying
>> on the BD79124's ability of storing latest measurements into register.
>>
>> The driver does also support configuring the threshold events for
>> detecting the out-of-window events.
> Trivial editorial comment: that 'does' is not providing anything use
> in modern English (might have done in the past, no idea!)

I have learned my English back in the 90's. Back then they told us we 
were taught British English. My teacher had probably been taught 
somewhere in the 70's. So, what I've learned is likely not modern 
English - besides, I liked girls and sports more than school, so I never 
was an A-class student ... ;)

I suppose you might know better than I (do) ;)

Well, I rarely get feedback from what I write - so this kind of things 
are valuable. I think I've developed a habit of adding this 'do' to the 
places where it's not necessary. Sometimes just out of a habit, and 
sometimes (in my ears) it strengthens the meaning. And I have no idea 
how correct the usage is unless I (do) get the feedback.

So, thanks.

> "Also support configure the threshold..."
> 
>>
>> The BD79124 keeps asserting IRQ for as long as the measured voltage is
>> out of the configured window. Thus the driver masks the received event
>> for a fixed duration (1 second) when an event is handled. This prevents
>> the user-space from choking on the events
>>
>> The ADC input pins can be also configured as general purpose outputs.
>> Those pins which don't have corresponding ADC channel node in the
>> device-tree will be controllable as GPO.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> 
> A few trivial things inline.
> 
> Jonathan
> 
>> +/*
>> + * The high and low limits as well as the recent result values are stored in
>> + * the same way in 2 consequent registers. The first register contains 4 bits
>> + * of the value. These bits are stored in the high bits [7:4] of register, but
>> + * they represent the low bits [3:0] of the value.
>> + * The value bits [11:4] are stored in the next regoster.
>> + *
>> + * Conver the integer to register format and write it using rmw cycle.
> Convert?

Right. As a note to self, also register, not regoster.

Thanks!

Yours,
	-- Matti

Powered by blists - more mailing lists