Re: [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer

From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Mehdi Djait <mehdi.djait.k@gmail.com>, jic23@kernel.org
Cc: 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
Date: Sun, 19 Mar 2023 09:43:19 +0200	[thread overview]
Message-ID: <00babd12-0ab7-de2a-857b-a58a0948b85c@gmail.com> (raw)
In-Reply-To: <cover.1679009443.git.mehdi.djait.k@gmail.com>

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.

> 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).

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.

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.

Anyways, I'll have proper look at this series during the next week - 
Thanks for the contribution! Much appreciated!

Yours,
	-- Matti

> Mehdi Djait (3):
>    dt-bindings: iio: Add KX132 accelerometer
>    iio: accel: kionix-kx022a: Add chip_info structure
>    iio: accel: Add support for Kionix/ROHM KX132 accelerometer
> 
>   .../bindings/iio/accel/kionix,kx022a.yaml     |  13 +-
>   drivers/iio/accel/kionix-kx022a-i2c.c         |  21 +-
>   drivers/iio/accel/kionix-kx022a-spi.c         |  24 +-
>   drivers/iio/accel/kionix-kx022a.c             | 413 +++++++++++-------
>   drivers/iio/accel/kionix-kx022a.h             | 181 +++++++-
>   5 files changed, 464 insertions(+), 188 deletions(-)
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~