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! ~~
