Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Vaittinen, Matti]

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@xxxxxxxxxxxxxxx> 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.

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? 
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 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.

> 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...


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

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