Date:   Tue, 21 Mar 2023 14:16:54 +0100
From:   Mehdi Djait <mehdi.djait.k@gmail.com>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     jic23@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        andriy.shevchenko@linux.intel.com, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132
 accelerometer
Message-ID: <ZBmuRqjDH9rTtiVy@carbian>
References: <cover.1679009443.git.mehdi.djait.k@gmail.com>
 <00babd12-0ab7-de2a-857b-a58a0948b85c@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <00babd12-0ab7-de2a-857b-a58a0948b85c@gmail.com>
Precedence: bulk

Hi Matti,

On Sun, Mar 19, 2023 at 09:43:19AM +0200, Matti Vaittinen wrote:
> Hi Mehdi,
> 
> Things have been piling up for me during last two weeks... I will do proper
> review during next week.
> 
> On 3/17/23 01:48, Mehdi Djait wrote:
> > KX132 accelerometer is a sensor which:
> > 	- supports G-ranges of (+/-) 2, 4, 8, and 16G
> > 	- can be connected to I2C or SPI
> > 	- has internal HW FIFO buffer
> > 	- supports various ODRs (output data rates)
> > 
> > The KX132 accelerometer is very similair to the KX022A.
> > One key difference is number of bits to report the number of data bytes that
> > have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.
> 
> The KX022A has 16bits of data in HiRes mode. This is the default for kx022a
> driver.

I am talking here about "Buffer Sample Level number of bits": kx022a uses
8 bits: just BUF_STATUS_1 to report the status of the buffer. kx132 uses
BUF_STATUS_1 and the Bit0, Bit1 of BUF_STATUS_2. 

That's the reason for adding the kx022a_get_fifo_bytes function. 

> 
> > A complete list of differences is listed in [1]
> > 
> > 
> > [1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1
> 
> This document is somewhat misleading. It does not contain KX022A but the
> older KX022. Kionix has the somewhat confusing habit of having very similar
> names for models with - occasionally significant - differences. (My own
> opinion).

Yes, I am aware that it does not contain KX022A, but from my
understanding of your code, the document can be used to highlight the 
difference I mentioned

> 
> I the "Technical referene manual" is more interesting document than the
> data-sheet:
> 
> https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> 
> I have heard that there have been a few very different versions of KX132 as
> well. Not sure if they have "leaked" out to public though. In any case, for
> the kx132 it might be safest to use the full model name - especially in the
> DT compatibles.
> 

I will change it to kx132-1211

> Finally, AFAIK the key "thing" in KX132 is the "ADP" (Advanced Data Path)
> feature which allows filtering the data "in sensor". Unfortunately, I am not
> really familiar with this feature. Do you think this is something that might
> get configured only once at start-up depending on the purpose of the board?
> If yes, this might be something that will end-up having properties in
> device-tree. If yes, then it might be a good idea to have own binding doc
> for KX132. Currently it seems Ok to have them in the same binding doc
> though.
> 

Correct me if I am wrong: the Devicetree is a description of the
hardware and the transitioning document states:

"From a hardware perspective, all these sensors
have an identical pad/pin layouts and identical pin functionality"

I was thinking about adding an advanced_data_path boolean to the chip_info 
struct and providing different driver callbacks depending on the value.

Something like in the bmc150-accel-core: iio_info for bmc150_accel_info
and iio_info for bmc150_accel_info_fifo

--
Kind Regards
Mehdi Djait