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

[Andy Shevchenko]

On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote:
> 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

My opinion on that, you should not depend on the others work if they are slow.
It might take ages for them to finish and upstream that. Why should you wait?

OTOH you may inform them with your patches that may have a chance to land before
theirs. It might motivate them to speed up their work and coordinate the outcome
that all stakeholders will be happy.

That said, I would choose option 4), i.e. provide a series where you decouple
thingy from DRM and Cc to Stephen with a cover letter note that explains why
you chosen this way and what alternatives you are open to.

-- 
With Best Regards,
Andy Shevchenko