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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Oct 18, 2022 at 02:27:20PM +0300, Matti Vaittinen wrote:
> 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!

This is very good news, Matti, thanks for pushing this forward and finding
better solution!

-- 
With Best Regards,
Andy Shevchenko