Message-ID: <eb4e7420-a4f9-1ddb-3394-c478a3e1f32f@fi.rohmeurope.com>
Date:   Mon, 26 Sep 2022 05:02:44 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     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 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.

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

Thanks for all the help!

Yours,
	-- Matti

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

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

Powered by blists - more mailing lists