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

[Jonathan Cameron]

On Tue, 11 Oct 2022 09:10:21 +0000
"Vaittinen, Matti" <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> 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@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.

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