On Mon, 26 Sep 2022 05:02:44 +0000
"Vaittinen, Matti" <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> On 9/24/22 18:17, Jonathan Cameron wrote:
> > On Fri, 23 Sep 2022 09:31:12 +0300
> > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> >
> >> On 9/22/22 20:03, Jonathan Cameron wrote:
> >>> On Wed, 21 Sep 2022 14:45:35 +0300
> >>> Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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
>
