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

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 5115 bytes --]

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@kernel.org> 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@kernel.org/
> 
> 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?

I think the best course of action would be to synchronize with Stephen,
and make sure that whatever patch you're doing can be used for his work.

Once it works for both of you, then I guess it can go through the kunit
tree and you will use it both.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]