[U-Boot] [PATCH v3 6/6] test: dm: Fix wrong aliases property names

[Simon Glass]

Hi,

On 24 May 2018 at 16:04, Eugeniu Rosca <roscaeugeniu at gmail.com> wrote:
> Hi Simon,
>
> On Tue, May 22, 2018 at 05:30:40PM -0600, Simon Glass wrote:
>> Hi Eugeniu,
>>
>> On 19 May 2018 at 06:13, Eugeniu Rosca <roscaeugeniu at gmail.com> wrote:
>
> --snip--
>
>> > v2->v3:
>> > * Fixed an issue in the test code (test/dm/test-fdt.c) generated by the
>> >   DTS update (arch/sandbox/dts/test.dts) in [PATCH v2].
>> > * Changed commit summary line, to cover test/dm/test-fdt.c.
>> > * Added: Reported-by: Petr Vorel <pvorel at suse.cz>
>> > * [Due to update] Dropped: Reviewed-by: Simon Glass <sjg at chromium.org>
>> >
>> > v1->v2:
>> > * Newly pushed
>> >
>> >  arch/sandbox/dts/test.dts | 8 ++++----
>> >  test/dm/test-fdt.c        | 2 +-
>> >  2 files changed, 5 insertions(+), 5 deletions(-)
>> >
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> See below
>>
>> > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
>> > index 683b1970e0af..3e87c5c0f3fd 100644
>> > --- a/arch/sandbox/dts/test.dts
>> > +++ b/arch/sandbox/dts/test.dts
>> > @@ -27,10 +27,10 @@
>> >                 testfdt3 = "/b-test";
>> >                 testfdt5 = "/some-bus/c-test at 5";
>> >                 testfdt8 = "/a-test";
>> > -               fdt_dummy0 = "/translation-test at 8000/dev at 0,0";
>> > -               fdt_dummy1 = "/translation-test at 8000/dev at 1,100";
>> > -               fdt_dummy2 = "/translation-test at 8000/dev at 2,200";
>> > -               fdt_dummy3 = "/translation-test at 8000/noxlatebus at 3,300/dev at 42";
>> > +               fdt-dummy0 = "/translation-test at 8000/dev at 0,0";
>> > +               fdt-dummy1 = "/translation-test at 8000/dev at 1,100";
>> > +               fdt-dummy2 = "/translation-test at 8000/dev at 2,200";
>> > +               fdt-dummy3 = "/translation-test at 8000/noxlatebus at 3,300/dev at 42";
>> >                 usb0 = &usb_0;
>> >                 usb1 = &usb_1;
>> >                 usb2 = &usb_2;
>> > diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
>> > index 8196844e89a7..66d0df5629a2 100644
>> > --- a/test/dm/test-fdt.c
>> > +++ b/test/dm/test-fdt.c
>> > @@ -425,7 +425,7 @@ static const struct udevice_id fdt_dummy_ids[] = {
>> >  };
>> >
>> >  UCLASS_DRIVER(fdt_dummy) = {
>> > -       .name           = "fdt_dummy",
>> > +       .name           = "fdt-dummy",
>>
>> You should not need to change this one, and I worry that it is
>> confusing since this is the driver name, not a compatible string.
>
> I am not familiar with the in-tree U-boot unit tests, but doing a small
> experiment I get clear evidence that there is a tight
> relationship/dependency between the name of the entries in the aliases
> DTS node and the "name" field of UCLASS_DRIVER using that DTS.

Yes you are right - the alias uses the uclass name, sorry.

>
> The experiment is running 'ut dm' in sandbox before and after below
> patch (I also had to fix two null pointer dereferences in
> test/dm/bus.c to avoid segmentation faults with the patch applied):
>
> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
> index 8196844e89a7..5b715e965269 100644
> --- a/test/dm/test-fdt.c
> +++ b/test/dm/test-fdt.c
> @@ -94,7 +94,7 @@ int testfdt_ping(struct udevice *dev, int pingval, int *pingret)
>  }
>
>  UCLASS_DRIVER(testfdt) = {
> -       .name           = "testfdt",
> +       .name           = "test_fdt",
>         .id             = UCLASS_TEST_FDT,
>         .flags          = DM_UC_FLAG_SEQ_ALIAS,
>  };
>
> Before the patch, I get: Failures: 9
> After the patch, I get: Failures: 25
>
> So, while the aliases entries are certainly not compatible strings,
> not keeping them aligned to UCLASS_DRIVER->name values leads to test
> failures. I am not sure if this is something specific to architecture of
> the tests or applies generically to any driver which fetches its
> configuration from DTS. I was hoping to get an assessment from somebody
> with more experience in this area.
>
> Besides the above, it is not clear to me if your Reviewed-by applies to
> to this patch partially (since you expressed some concerns) or applies
> globally, in which case the concerns are not major?

It means that I've reviewed the patch and I'd like some changes, but
don't want to review it after you have made those changes, so you
should add the Reviewed-by tag when doing the next version.

But in this case your change is correct, so please don't worry. It's
unfortunate that the uclass name needs a hypen, but I understand why.

Regards,
Simon