Message-ID: <8912d34b-ae45-d62f-6d39-a52cf76df098@fi.rohmeurope.com>
Date:   Fri, 17 Mar 2023 10:57:35 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Maxime Ripard <maxime@...no.tech>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        Stephen Boyd <sboyd@...nel.org>
Subject: Re: [PATCH v3 2/6] iio: light: Add gain-time-scale helpers

On 3/17/23 12:19, Maxime Ripard wrote:
> 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@...nel.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.

Thanks Andy and Maxime! I appreciate your help.

I guess I'll ping Stephen with a separate mail (although he's in CC 
here) and CC the relevant patches to him as well. I assume that gives 
him a chance to take a look what I am doing and suggest changes if needed.

My contribution is anyways small - it's mostly just renaming and moving 
those two SRm helper APIs + changing the callers. Most of that is 
probably outside the scope of his work - it would just fit the same 
files he will be adding =)

Yours,
	-- Matti


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

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

Powered by blists - more mailing lists