Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Oct 2022 15:42:06 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jagath Jog J <jagathjog1996@gmail.com>,
        Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
        Cosmin Tanislav <demonsingur@gmail.com>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A
 accelerometer
Message-ID: <Y06fHg6JdSA2GbII@smile.fi.intel.com>
References: <cover.1665066397.git.mazziesaccount@gmail.com>
 <88e24b01da9f44ebf5fcd8344ded0b75ff742fbf.1665066397.git.mazziesaccount@gmail.com>
 <Yz8fK7j8pxlU76xt@smile.fi.intel.com>
 <20221009133351.6ff4894b@jic23-huawei>
 <Y0O4fiQQX0M317h/@smile.fi.intel.com>
 <7abed64a-d544-a228-b5f1-4c7b5a3bd1be@fi.rohmeurope.com>
 <20221014142232.000038df@huawei.com>
 <b7017e04-83a0-da3e-4ea7-28a2d34febc8@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b7017e04-83a0-da3e-4ea7-28a2d34febc8@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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