Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen <mazziesaccount@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3517 bytes --]

On 10/14/22 16:22, Jonathan Cameron wrote:
> On Tue, 11 Oct 2022 09:10:21 +0000
> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> 
>> On 10/10/22 09:15, Andy Shevchenko wrote:
>>> On Sun, Oct 09, 2022 at 01:33:51PM +0100, Jonathan Cameron wrote:
>>>> On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko
>>>> <andriy.shevchenko@linux.intel.com> wrote:
>>>>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
>>>
>>> ...
>>>    
>>>>>> +module_param(g_kx022a_use_buffer, bool, 0);
>>>>>> +MODULE_PARM_DESC(g_kx022a_use_buffer, +		 "Buffer samples. Use
>>>>>> at own risk. Fifo must not overflow");
// snip

>> This would be a way to have the FIFO disabled by default and warn users
>> via dt-binding docs if they decide to explicitly enable the FIFO.
>> (Besides, I believe the FIFO is usable on at least some of the Kionix
>> sensors - because I've heard it is used on a few platforms).
>>
>> This could give us safe defaults while not shutting the doors from those
>> who wish to use the FIFO. Sure we need a buy-in from Krzysztof / Rob,
>> but that may be less of an obstacle compared to the module param if Greg
>> is so strongly oppsoing those. (And the dt-property could also provide
>> some technical merites as these sensors seem to really have differencies
>> in FIFOs).
> 
> I'm dubious about having this for known broken parts - but I guess you
> can propose it and see what the dt-maintainers say.
> 
> I don't want to see fifo size in the dt binding though.

// snip

>>> Also it needs some communication
>>>   with a vendor to clarify the things.
>>
>> I do communication with the vendor on a daily basis :] Nowadays Kionix
>> is part of ROHM, and Finland SWDC has been collaboration with Kionix
>> even before they "merged" (but I have not been part of the "sensor team"
>> back then).
>>
>> Unfortunately, reaching the correct people inside the company is hard
>> and occasionally impossible - long story...
> 
> :)

Just a note. I have done some further investigation (further testing 
combined with tracing the SPI lines) and also got some answer(s) from 
the ASIC designers. First thing is that the underlying FIFO is 258 bytes 
and can hold up-to 43 HiRes samples. This is also fixed in the recent 
data-sheet. The register informing how many bytes of data is stored in 
FIFO is still only 8bits. When FIFO is full, the magic value 255 is used 
to indicate the 258 bytes of data. This explains the strange 42,5 
samples (255 bytes) of data being reported.

Furthermore, I've noticed that the FIFO appearing to be "stuck", eg. 
amount of data stored in FIFO not decreasing when data is read is 100% 
reproducible. The condition is that the PC1 bit (accelerometer state 
control) is toggled before the FIFO is read. The issue does not seem to 
be reproducible if the PC1 is not touched. I have also asked if this is 
a known limitation.

Anyways, it seems we have a solution to the problem. We simply handle 
the case when 255 bytes is reported correctly (by reading 258 bytes of 
valid data) - and we prevent changing the sensor configuration when FIFO 
is enabled.

I will implement these changes to the v3 and drop both the module-param 
and the dt-property. Sorry for the hassle and thanks for the 
patience/support!

Yours
	-- Matti

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

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


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 5335 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]