Re: [PATCH 0/8] clk: Add kunit tests for fixed rate and parent data

[Frank Rowand]

On 3/6/23 06:53, Rob Herring wrote:
> On Sat, Mar 4, 2023 at 9:39 AM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>>
>> On 3/2/23 17:57, Stephen Boyd wrote:
>>> Quoting Rob Herring (2023-03-02 12:18:34)
>>>> On Thu, Mar 2, 2023 at 1:44 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>>>>>
>>>>> Quoting Rob Herring (2023-03-02 09:13:59)
>>>>>>
>>>>>> Good to see bindings for this. I've been meaning to do something about
>>>>>> the DT unittest ones being undocumented, but I hadn't really decided
>>>>>> whether it was worth writing schemas for them. The compatibles at
>>>>>> least show up with 'make dt_compatible_check'. Perhaps we want to just
>>>>>> define some vendor (not 'linux') that's an exception rather than
>>>>>> requiring schemas (actually, that already works for 'foo').
>>>>>
>>>>> Sure. Maybe "kunit" should be the vendor prefix? Or "dtbunit"?
>>>>
>>>> We'd want to use the same thing on the DT unittests or anything else
>>>> potentially. How about just 'test'?
>>>
>>> Sounds good.
>>>
>>>>
>>>>>> It's
>>>>>> likely that we want test DTs that fail normal checks and schemas get
>>>>>> in the way of that as we don't have a way to turn off checks.
>>>>>
>>>>> Having the schemas is nice to make sure tests that are expecting some
>>>>> binding are actually getting that. But supporting broken bindings is
>>>>> also important to test any error paths in functions that parse
>>>>> properties. Maybe we keep the schema and have it enforce that incorrect
>>>>> properties are being set?
>>>>
>>>> I wasn't suggesting throwing them out. More why I hadn't written any I guess.
>>>>
>>>>> Do we really need to test incorrect bindings? Doesn't the
>>>>> dt_bindings_check catch these problems so we don't have to write DTB
>>>>> verifiers in the kernel?
>>>>
>>>> Fair enough. Using my frequently stated position against me. :)
>>>>
>>>> I do have a secret plan to implement (debug) type checks into the
>>>> of_property_* APIs by extracting the type information from schemas
>>>> into C.
>>>>
>>>
>>> Ok. I suspect we may want to test error paths though so I don't know
>>
>> Yes, exactly.
>>
>>> what to do here. For now I'll just leave the bindings in place and
>>> change the prefix to "test".
>>>
>>>>
>>>>>> We already have GPIO tests in the DT unittests, so why is clocks
>>>>>> different? Or should the GPIO tests be moved out (yes, please!)?
>>>>>
>>>>> Ah I didn't notice the GPIO tests in there. There are i2c tests too,
>>>>> right? All I can say is clks are using kunit, that's the difference ;-)
>>>>
>>>> Yeah, they should perhaps all move to the subsystems.
>>>
>>> Got it.
>>>
>>>>
>>>>>> What happens when/if the DT unittest is converted to kunit? I think
>>>>>> that would look confusing from the naming. My initial thought is
>>>>>> 'kunit' should be dropped from the naming of a lot of this. Note that
>>>>>> the original kunit submission converted the DT unittests. I would
>>>>>> still like to see that happen. Frank disagreed over what's a unit test
>>>>>> or not, then agreed, then didn't... I don't really care. If there's a
>>>>>> framework to use, then we should use it IMO.
>>>>>
>>>>> Honestly I don't want to get involved in migrating the existing DT
>>>>> unittest code to kunit. I'm aware that it was attempted years ago when
>>>>> kunit was introduced. Maybe if the overlay route works well enough I can
>>>>> completely sidestep introducing any code in drivers/of/ besides some
>>>>> kunit wrappers for this. I'll cross my fingers!
>>>>
>>>> Yeah, I wasn't expecting you to. I just want to make sure this meshes
>>>> with any future conversion to kunit.
>>>
>>> Phew!
>>>
>>>>
>>>> There's also some plans to always populate the DT root node if not
>>>> present. That may help here. Or not. There's been a few versions
>>>> posted with Frank's in the last week or 2.
>>>>
>>>
>>> Ok. I think I have some time to try this overlay approach so let me see
>>> what is needed.
>>
>> Please avoid overlays.  See my other replies in this thread for why.
> 
> If overlays work for the constrained environment of unit tests, then
> use them. If overlays are not to be used, then remove the support from
> the kernel. Putting issues in a todo list is not going to get them
> done. Having users will.

Overlays are not used to enable OF unittests that are unrelated to
overlays (to the best of my memory - I reserve the right to be
corrected).  Overlay usage in OF unittests is specifically to test
overlay features.

> 
> Rob