Re: [PATCH v3 2/6] iio: light: Add gain-time-scale helpers

[Matti Vaittinen]

On 3/13/23 15:59, Matti Vaittinen wrote:
> On 3/13/23 15:29, Andy Shevchenko wrote:
> > On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
> > > On 3/13/23 14:40, Andy Shevchenko wrote:
> > > > On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
> > > > > On Sun, 12 Mar 2023 17:06:38 +0000
> > > > > Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > ...
> >
> > > > > Ah. I forgot the tests that don't have a device so can't use devm.
> > > >
> > > > Why not? I have seen, IIRC, test cases inside the kernel that fakes 
> > > > the device
> > > > for that.
> > >
> > > I'd appreciated any pointer for such an example if you have one at 
> > > hand. (I
> > > can do the digging if you don't though!)
> > >
> > > I am not a fan of unit tests. They add huge amount of inertia to
> > > development, and in worst case, they stop people from contributing where
> > > improving a feature requires test code modification(s). And harder 
> > > the test
> > > code is to understand, worse the unwanted side-effects. Also, harder the
> > > test code is to read, more time and effort it requires to analyze a test
> > > failure... Hence, I am _very_ conservative what comes to adding size 
> > > of test
> > > code with anything that is not strictly required.
> > >
> > > After that being said, unit tests are a great tool when carefully 
> > > used - and
> > > I assume/hope stubbing a device for devm_ tests does not add much 
> > > extra...
> > > But let me see if I can find an example :)
> >
> > drivers/gpu/drm/tests/drm_managed_test.c ?
> >
> > (somewhere underneath:
> >
> >   ret = platform_driver_register(&fake_platform_driver);
> >
> > which suggests... what exactly? :-)

Thanks to pointer from Andy I found the 
drm_kunit_helper_[alloc/free]_device() functions. I renamed them to 
test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add 
declarations to include/kunit/test-helpers.h fixed KConfigs and existing 
callers + added the tests for managed interfaces. I have this in place 
in my personal playground where I am working towards the v4 of the series.

...

After that I asked from Maxime if he had a reason to not make those 
generic and available to other subsystems besides drm in the first place...

And Maxime was kind enough to point me to the fact that something like 
this was done in the CCF context:
https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@xxxxxxxxxx/

I like the 'single function to get the dummy device which can be passed 
to devm'-approach used in drm helpers. I do also like Stephen's idea of 
having the prototypes in kunit/platform_device.h which matches the 
linux/platform_device.h.

However, I don't know when Stephen's work will be finished and merged to 
IIO-tree so that it could be used/extended for the needs of these tests.

Meanwhile, I don't think it makes sense to go forward with my changes 
splitting the helpers out of drm until we see what Stephen's changes 
will bring us. On the other hand, I don't like delaying the gts-helpers 
or the sensor drivers.

So, any suggestions what I should do? I see following options:

1) Drop the tests for managed interfaces for now.
2) Add the tests with a yet-another duplicate implementation of the
   dummy device for devm.
3) Add the tests using the helpers from drm as they are now.

option 1):
I like it as it would be an easy way (for now) - but I hate it as it may 
be a hard way as well. In my experience, when a driver/helper lands 
upstream it will get first few fixes quite fast - and not having a test 
available upstream when this happens is bad. Bad because it means the 
out-of-tree test may get broken, and bad because there is no easy way to 
test the fixes.

option 2):
I hate it because it makes the test code more complex - and duplicates 
the kernel code which is never nice. This could be reworked later when 
Stephens work is done though.

option 3):
It's in general not nice to use functions exported for some other 
subsystem's specific purposes. This would however keep the test code at 
minimum, while leaving the same "I swear I'll fix this later when 
dependencies have settled" - possibility as option 2) did.

Oh, in theory there is option 4) to just send out the changes I did(*) 
which pull the drm_kunit_helper_[alloc/free]_device() out of the DRM - 
but I guess that would lead some extra work to merge this later with 
stuff Stephen's series does introduce.

Any suggestions which of the options to proceed with?



(*) For those interested in seeing the result of pulling the 
drm_kunit_helper_[alloc/free]_device() out of DRM tests, below are links 
to my personal playground with following remarks:
1) code one finds from there may be 100% untested
2) code one finds there may be written just for fun, or for a very
   specific purpose
3) code one finds there is generally not maintained, may be rebased, may
   vanish or turn into rabbits or turn you into a rabbit when you wear a
   top hat.

commits to look at there are
https://github.com/M-Vaittinen/linux/commit/15d07e799f7c7fddc91030b16266d4a8bbaf1cc1

https://github.com/M-Vaittinen/linux/commit/6b4c4ba38b1f838fb0074befd2ca8734604464da



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

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