Hi,
On 24 May 2018 at 16:04, Eugeniu Rosca roscaeugeniu@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@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@suse.cz
- [Due to update] Dropped: Reviewed-by: Simon Glass sjg@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@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@5"; testfdt8 = "/a-test";
fdt_dummy0 = "/translation-test@8000/dev@0,0";fdt_dummy1 = "/translation-test@8000/dev@1,100";fdt_dummy2 = "/translation-test@8000/dev@2,200";fdt_dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42";
fdt-dummy0 = "/translation-test@8000/dev@0,0";fdt-dummy1 = "/translation-test@8000/dev@1,100";fdt-dummy2 = "/translation-test@8000/dev@2,200";fdt-dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@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