Message-ID: <20221002121145.2e3500ba@jic23-huawei>
Date:   Sun, 2 Oct 2022 12:11:45 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
Cc:     Matti Vaittinen <mazziesaccount@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        Jagath Jog J <jagathjog1996@...il.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A
 accelerometer

On Mon, 26 Sep 2022 05:02:44 +0000
"Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com> wrote:

> On 9/24/22 18:17, Jonathan Cameron wrote:
> > On Fri, 23 Sep 2022 09:31:12 +0300
> > Matti Vaittinen <mazziesaccount@...il.com> wrote:
> >   
> >> On 9/22/22 20:03, Jonathan Cameron wrote:  
> >>> On Wed, 21 Sep 2022 14:45:35 +0300
> >>> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> >>>      
> >>>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
> >>>> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
> >>>> tap/motion detection, wake-up & back-to-sleep events, four acceleration
> >>>> ranges (2, 4, 8 and 16g) and probably some other cool fatures.
> >>>>
> >>>> Add support for the basic accelerometer features such as getting the
> >>>> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> >>>> using the WMI IRQ).
> >>>>
> >>>> Important things to be added include the double-tap, motion
> >>>> detection and wake-up as well as the runtime power management.
> >>>>
> >>>> NOTE: Filling-up the hardware FIFO should be avoided. During my testing
> >>>> I noticed that filling up the hardware FIFO might mess-up the sample
> >>>> count. My sensor ended up in a state where amount of data in FIFO was
> >>>> reported to be 0xff bytes, which equals to 42,5 samples. Specification
> >>>> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
> >>>> least once the FIFO was stuck in a state where reading data from
> >>>> hwardware FIFO did not decrease the amount of data reported to be in the  
> >>> spell check this.
> >>>      
> >>>> FIFO - eg. FIFO was "stuck". The code has now an error count and 10
> >>>> reads with invalid FIFO data count will cause the fifo contents to be
> >>>> dropped.  
> >>> Ouch - that's nasty.  
> >>
> >> Indeed it is. As this commit states, this is pretty initial support for
> >> the accelerometer. I want to enable people to do basic experimenting and
> >> also use the component to some slow ODR solutions. Besides, having even
> >> a basic support in-tree enable people to add further improvements :) So,
> >> I am hoping / expecting to see improvements added also by other people
> >> using this. I think that after this initial support many people still
> >> _need_ for example the runtime PM. Maybe we will also end up with some
> >> nicer solution to the FIFO issues.  
> > 
> > My initial gut feeling is that, if that fifo issue doesn't have a clean
> > solution, we don't don't support the fifo (by default anyway -
> > could have a module parameter or something to turn it on).  Late handling
> > of interrupts is something that can happen for lots of reasons. It should
> > never be fatal!  
> 
> Tell me about it... More than 10-years ago I "inherited" maintenance of 
> a timing code which was built on periodic 10msec IRQ which incremented a 
> counter. (And after that there were many new generations of Linux based 
> devices with various "rt"-requirements). Missing an IRQ was fatal as 
> there were no hardware where we could read the correct timestamp and 
> other units in the system were no longer in sync if the counter was 
> wrong. Only recovery was complete system restart for all units - which 
> in that use case was really bad. I learned to absolutely hate the serial 
> prints over slow UART - and I learned to love irqsoff tracer. I don't 
> work for that company anymore and I believe the product has already 
> retired. Yet, I still get *shrugs* when I see hard time limits for 
> serving IRQs on Linux. (Other than that I kind of like pondering the 
> rt-challenges).
> 
> Anyways, I don't see a real risk for example when using the ODRs < 2 Hz 
> and setting the watermark to somewhere near 20 samples.
> 
> It's really unfortunate the HW has these "hickups" - but I think it is 
> still perfectly usable for many cases - we just really need to document 
> the corner cases somewhere.

I'd rather we gated it - so by default the fifo mode isn't there and
people who are really confident they want it set a module parameter or
similar.  Cuts down on the bug reports.

> 
> > Simple first.   Different matter if you add the other triggers later in
> > the same patch series, but history says half the interesting features
> > we plan to add never get added.  
> 
> ack.
> 
> >>> Must either handle both pins or at least know if it is irq2 and
> >>> treat that as no irq for now.  
> >>
> >> I don't want to try supporting both pins for now. It makes this somewhat
> >> more complex - especially if we want to support using two IRQs. That
> >> will require some thorough thinking which I don't have time to do right
> >> now :(
> >>
> >> I can add the irq-names to the bindings and add check to the probe so
> >> that if the irq2 is used we error out with nice 'not supported' message.
> >>  
> > 
> > A slightly nicer thing to do is support one irq, but let it be either irq1 or
> > irq2. If both are supplied just ignore the second one.  People have
> > an 'amusing' habit of only wiring up irq2 on boards.
> >   
> 
> Ok. It shouldn't be such a big change for the code. I'll see what it 
> will look like.
> 
> >>>> +
> >>>> +static int kx022a_chip_init(struct kx022a_data *data)
> >>>> +{
> >>>> +	int ret, dummy;
> >>>> +
> >>>> +	/*
> >>>> +	 * Disable IRQs because if the IRQs are left on (for example by
> >>>> +	 * a shutdown which did not deactivate the accelerometer) we do
> >>>> +	 * most probably end up flooding the system with unhandled IRQs
> >>>> +	 * and get the line disabled from SOC side.
> >>>> +	 */
> >>>> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);  
> >>>
> >>> Unusual to do this rather than a reset.  Quick look suggests there is
> >>> a suitable software reset (CNTL2)  
> >>
> >> Same thing was just told me by a colleague of mine yesterday. Reset
> >> simplifies this quite a bit. (I just wonder if we can trust the reset
> >> always initializes the IC to same defaults or if there may be OTP
> >> differencies. I am new to these sensors).
> >>  
> > 
> > I really hope we can rely on any documented reset values!  
> 
> That is a sane assumption to do - but in my experience we don't live in 
> a sane world ;) I've seen way too many cases where the defaults are 
> changed for ICs for example by changing OTP. And sometimes the OTP 
> change has not been visible to the drivers from any revision registers 
> or such.
> 
> I'm not talking about Kionix sensors though as I've never worked with 
> Kionix sensors before - so let's just try out the reset and fix things 
> if problems emerge. I am probably just a bit paranoid :)

Entirely reasonable!

Jonathan

> 
> Thanks for all the help!
> 
> Yours,
> 	-- Matti
> 

Powered by blists - more mailing lists