From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Jonathan Cameron <jic23@kernel.org>,
Matti Vaittinen <mazziesaccount@gmail.com>,
"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
Date: Fri, 14 Oct 2022 14:22:32 +0100 [thread overview]
Message-ID: <20221014142232.000038df@huawei.com> (raw)
In-Reply-To: <7abed64a-d544-a228-b5f1-4c7b5a3bd1be@fi.rohmeurope.com>
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");
> >>>
> >>> Why?! We usually do not allow module parameters in the new code.
> >>
> >> Badly broken hardware - was my suggestion. Alternatives if there
> >> are usecases that need to use the fifo, but it can wedge hard in a
> >> fashion that is impossible to prevent from the driver? My gut
> >> feeling is still drop the support entirely with a strong comment in
> >> the code that the hardware is broken in a fashion we don't know how
> >> to work around.
>
> I did some quick study regarding couple of other Kionix sensors. (like
> KX122 and old KX022 - without the 'A'). It seems to me that the register
> interfaces between many of the sensors are largely identical. Extending
> the driver to support those seems pretty straightforward (scales and
> resolution may need tweaking, as does the FIFO size) but register
> contents and even offsets are largely identical.
Last kionix part I had was a kxsd9 and I don't recall that having a fifo
so obviously didn't hit this issue.
>
> As said, it seems the Kionix sensors may have different size of internal
> FIFOs, or even no FIFO at all. So, maybe we could provide a
> "kionix,fifo-enable" flag (or even "kionix,fifo-size") from device-tree?
For device where we don't have reports of this issue, that should be derived
from the compatible (or even better a whoami register if there is one).
The driver should know the fifo-size if it isn't discoverable.
> 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.
>
> >
> > I also would drop this from upstream and if anybody curious, provide
> > some kind of GitHub gist for that.
>
> Well, I think we all agree that downstream code hosted in some
> unofficial github repositories are rarely that valuable. They're less
> reliable, less tested, less reviewed, less secure and pretty much
> impossible to maintain in a way that interested user could get a version
> matching his preferred kernel.
>
> There are reasons why I (people) keep sending the drivers to upstream -
> and why some companies even spend $$ for that. Having this feature in
> downstream repo is not nearly on same page for user's point of view as
> is having the support upstream.
It's not really support if it comes with big warnings and potentially we
even taint the kernel of someone turns it on...
>
> > 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...
:)
Jonathan
next prev parent reply other threads:[~2022-10-14 13:22 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-06 14:35 [RFC PATCH v2 0/5] iio: Support ROHM/Kionix kx022a Matti Vaittinen
2022-10-06 14:36 ` [RFC PATCH v2 1/5] regulator: Add devm helpers for get and enable Matti Vaittinen
2022-10-06 16:17 ` Andy Shevchenko
2022-10-09 12:24 ` Jonathan Cameron
2022-10-10 6:11 ` Andy Shevchenko
2022-10-10 9:24 ` Matti Vaittinen
2022-10-14 12:42 ` Jonathan Cameron
2022-10-10 4:13 ` Matti Vaittinen
2022-10-10 6:12 ` Andy Shevchenko
2022-10-06 14:37 ` [RFC PATCH v2 2/5] " Matti Vaittinen
2022-10-06 14:37 ` [RFC PATCH v2 3/5] dt-bindings: iio: Add KX022A accelerometer Matti Vaittinen
2022-10-06 15:23 ` Krzysztof Kozlowski
2022-10-06 15:32 ` Matti Vaittinen
2022-10-09 12:27 ` Jonathan Cameron
2022-10-10 9:28 ` Matti Vaittinen
2022-10-06 14:38 ` [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM " Matti Vaittinen
2022-10-06 18:32 ` Andy Shevchenko
2022-10-07 7:04 ` Joe Perches
2022-10-07 9:22 ` Andy Shevchenko
2022-10-07 9:23 ` Andy Shevchenko
2022-10-09 12:33 ` Jonathan Cameron
2022-10-10 6:15 ` Andy Shevchenko
2022-10-11 9:10 ` Vaittinen, Matti
2022-10-14 13:22 ` Jonathan Cameron [this message]
2022-10-18 11:27 ` Matti Vaittinen
2022-10-18 12:42 ` Andy Shevchenko
2022-10-10 9:12 ` Matti Vaittinen
2022-10-10 11:58 ` Andy Shevchenko
2022-10-10 13:20 ` Vaittinen, Matti
2022-10-10 14:09 ` Andy Shevchenko
2022-10-11 6:56 ` Vaittinen, Matti
2022-10-14 13:34 ` Jonathan Cameron
2022-10-12 7:40 ` Matti Vaittinen
2022-10-14 13:42 ` Jonathan Cameron
2022-10-18 11:10 ` Matti Vaittinen
2022-10-24 16:41 ` Jonathan Cameron
2022-10-06 14:38 ` [RFC PATCH v2 5/5] MAINTAINERS: Add KX022A maintainer entry Matti Vaittinen
2022-10-09 12:38 ` Jonathan Cameron
2022-10-10 9:31 ` Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221014142232.000038df@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=Matti.Vaittinen@fi.rohmeurope.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=demonsingur@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jagathjog1996@gmail.com \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mazziesaccount@gmail.com \
--cc=nikita.yoush@cogentembedded.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).