[U-Boot] [PATCH 2/3] fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello Stephen,

On 09/24/2015 07:29 PM, Stephen Warren wrote:
> On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:
>> After rework in lib/fdtdec.c, the function fdtdec_get_addr()
>> doesn't work for nodes with #size-cells property, set to 0.
>>
>> To get GPIO's 'reg' property, the code should use one of:
>> fdtdec_get_addr_size_auto_no/parent() function.
>>
>> Fortunately dm core provides a function to get the property.
>>
>> This commit reworks function gpio_exynos_bind(), to properly
>> use dev_get_addr() for GPIO device.
>>
>> This prevents setting a wrong base register for Exynos GPIOs.
>
> Migrating everything to dev_get_addr() is the correct long-term fix, so
> this patch,
>
> Acked-by: Stephen Warren <swarren@nvidia.com>
>
> ... although I'd have liked to see a smaller diff that didn't both
> re-order all the code /and/ call a different function, but I suppose
> that's not possible given the need to pass the device object to
> dev_get_addr(). You could have used fdtdec_get_addr_size_auto_parent()
> directly.

Yes, it's not a single line diff, but the driver supports driver-model, 
so it's natural that it should use driver model API if can, instead of 
fdtdec API.

This approach makes things easier to test and catch mistakes in the future.

>
>
> I think it'd be good to fix fdtdec_get_addr_size() to have the same
> semantics that it previously did. There might be other code in U-Boot
> that's affected by the same issue, and fixing fdtdec_get_addr_size()
> would make sure that all got fixed too. Are you willing to send that
> patch too?
>
> Essentially, fdtdec_get_addr_size() used to assume:
>
> #address-cells == sizeof(fdt_addr_t)
> if sizep == NULL:
>      #size-cells == 0
> else:
>      #size-cells == sizeof(fdt_addr_t)
>
> However, it now assumes:
>
> #address-cells == sizeof(fdt_addr_t)
> #size-cells == sizeof(fdt_addr_t)
>
> Let's just add that condition back by doing something like the following
> in fdtdec_get_addr_size():
>
> u32 ns;
>
> if (sizep)
>      ns = sizeof(fdt_size_t) / sizeof(fdt32_t);
> else
>      ns = 0;
>
> ... and replacing the ns parameter that's passed to
> fdtdec_get_addr_size_fixed() with that variable, rather than hard-coding
> it.
>

Sorry, currently I have some other things to do, and I wouldn't prefer 
fixing this without proper testing. Such core things should be tested in 
sandbox by couple of unit tests.

This seem to be okay, but is still wrong.

We should always call fdtdec_get_addr_size_fixed() with arguments, which 
fits to the dtb, instead of hardcoded values.

So, only the implementation of function

fdtdec_get_addr_size_auto_parent()

seem to be correct.

It check the real #address-cells and #size-cells.

If this is slow, then maybe we need some cache with nodes, its 
parents/childs and its size/addr cells to be checked only once?

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com