[U-Boot] [PATCH 17/60] ARM: tegra: move apb_misc.h

Stephen Warren
2016-04-21T21:14:57Z

On 04/21/2016 02:59 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 20 April 2016 at 15:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/20/2016 01:26 PM, Simon Glass wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 19 April 2016 at 14:58, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> This header is only needed by code local to mach-tegra/, so move it there
>>>> to avoid polluting the global include path.
>>
>>
>>>> diff --git a/arch/arm/mach-tegra/tegra20/warmboot_avp.c
>>>> b/arch/arm/mach-tegra/tegra20/warmboot_avp.c
>>>> index 616358b657be..0ae401c569b6 100644
>>>> --- a/arch/arm/mach-tegra/tegra20/warmboot_avp.c
>>>> +++ b/arch/arm/mach-tegra/tegra20/warmboot_avp.c
>>>> @@ -11,10 +11,10 @@
>>>> #include <asm/arch/flow.h>
>>>> #include <asm/arch/pinmux.h>
>>>> #include <asm/arch/tegra.h>
>>>> -#include <asm/arch-tegra/apb_misc.h>
>>>> #include <asm/arch-tegra/clk_rst.h>
>>>> #include <asm/arch-tegra/pmc.h>
>>>> #include <asm/arch-tegra/warmboot.h>
>>>> +#include "../apb_misc.h"
>>>
>>>
>>> Is this really an improvement?
>>
>>
>> Yes:-P
>>
>>> What's the rationale?
>>
>>
>> As mentioned in the commit description, the content of the header is only
>> used by code in arch/arm/mach-tegra/. By moving the header file out of a
>> directory that's part of the include path, we ensure that code (such as
>> drivers and core code) can't access the header without explicitly doing
>> something unusual, which should ring alarm bells. This will help avoid
>> future additions of code that touches Tegra internals rather than accessing
>> functionality through standard/generic (or even custom but explicitly
>> "exported") APIs.
>
> Well if that's what you want, OK.
>
> The ../xxx.h looks odd to me - like a temporary hack. Also if we have
> a driver for SoC functionality then it will likely need access to this
> stuff. But I can't see a better way to do this, and it does achieve
> your goal...
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks.

If we do end up with a "real" driver for this in drivers/xxx/, I would
hope the register header would move into the same directory along with
the C file so nothing else could access it, and other code would only
access those registers by calling functions in the driver, those
functions being prototyped in some public header file in include/ or
arch/arm/mach-tegra/include/mach/ depending on whether the API was
U-Boot-wide or Tegra-specific.

This current patch does mean if we get a driver, we'll have to move the
header again, but I don't expect that to happen imminently (perhaps
ever?) for most of the code in arch/arm/mach-tegra, with the possible
exception of clock/reset/XUSB drivers. Equally, there would be so much
change involved in converting everything not to access the register
directly but instead use clean APIs that including a header move as well
would likely be close to noise.