Message-ID: <ae33de64-1ba1-4bd2-a139-3f0b5986f41e@gmail.com>
Date: Fri, 21 Mar 2025 10:01:00 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Jonathan Cameron <jic23@...nel.org>, 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>,
 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 v9 6/8] iio: adc: Support ROHM BD79124 ADC

On 20/03/2025 15:16, Andy Shevchenko wrote:
> On Thu, Mar 20, 2025 at 10:22:00AM +0200, Matti Vaittinen 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 do not support the BD79124's manual measurement mode but implement
>> the measurements using automatic measurement mode, relying on the
>> BD79124's ability of storing latest measurements into register.
>>
>> Support also configuring the threshold events for detecting the
>> out-of-window events.
>>
>> The BD79124 keeps asserting IRQ for as long as the measured voltage is
>> out of the configured window. Thus, prevent the user-space from choking
>> on the events and mask the received event for a fixed duration (1 second)
>> when an event is handled.
>>
>> The ADC input pins can be also configured as general purpose outputs.
>> Make those pins which don't have corresponding ADC channel node in the
>> device-tree controllable as GPO.
> 
> ...
> 
>> +struct bd79124_raw {
>> +	u8 val_bit0_3; /* Is set in high bits of the byte */
>> +	u8 val_bit4_11;
>> +};
> 
> Again, this is confusing.
> 
> Just put a bit order map in the comment as I suggested previously.
> When I see variable name containing bit range like above I think
> about the same bit order, i.e. with your comment it makes like this
> 
> bit number	7 6 5 4 3 2 1 0
> data bit	0 1 2 3 x x x x

Gah. I think I now understand what you're after. And, I agree, I haven't 
been as clear as I could've been.

The pit numbers in the struct members:
	u8 val_bit0_3; and u8 val_bit4_11;

are _not_ intended to represent the bit ordering - only the bit 
positions. Like, bits from bit 0 to bit 3 are stored in high bits of 
this u8 - where the "0 to 3" was just picked as order based on it being 
from the smaller to greater (which I believe is grammatically typical) - 
not based on how the bits are ordered in the register. If the order of 
the bits was indeed reverted, then we should see much more complex 
conversions than what is presented in these macros.

I will update the variable names to:

val_bit3_0; and val_bit11_4; I think it should sort out the confusion. I 
won't go to bit level representation of the full registers:

 > bit number	7 6 5 4 3 2 1 0
 > data bit	3 2 1 0 x x x x

and

 > bit number	7 6 5 4 3 2 1 0
 > data bit	b a 9 8 7 6 5 4

because it suggests there is something very strange in the registers 
(which is not the case) - and it is hard to spot if some bits have 
indeed changed the place.

Yours,
	-- Matti

Powered by blists - more mailing lists