[U-Boot] [PATCH 03/10] sunxi: Introduce a hidden ARCH_SUN6I Kconfig bool

[Michal Suchanek]

On 15 April 2015 at 10:07, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
>
> On 15-04-15 10:01, Michal Suchanek wrote:
>>
>> On 15 April 2015 at 09:35, Hans de Goede <hdegoede at redhat.com> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 15-04-15 08:51, Michal Suchanek wrote:
>>>>
>>>>
>>>> On 14 April 2015 at 18:06, Hans de Goede <hdegoede at redhat.com> wrote:
>>>>>
>>>>>
>>>>> sun6i and newer (derived) SoCs such as the sun8i-a23, sun8i-a33 and
>>>>> sun9i
>>>>> have a various things in common, like having separate ahb reset control
>>>>> registers, the SID living inside the pmic, custom pmic busses, new
>>>>> style
>>>>> watchdog, etc.
>>>>>
>>>>> This commit introduces a new hidden ARCH_SUN6I Kconfig bool which can
>>>>> be
>>>>> used to check for these features avoiding the need for an ever growing
>>>>> list
>>>>> of "#if defined CONFIG_MACH_SUN?I" conditionals as we add support for
>>>>> more
>>>>> "new style" sunxi SoCs.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>>> ---
>>>>>    arch/arm/cpu/armv7/sunxi/board.c            | 18 +++++++++---------
>>>>>    arch/arm/cpu/armv7/sunxi/cpu_info.c         |  2 +-
>>>>>    arch/arm/cpu/armv7/sunxi/usbc.c             |  4 ++--
>>>>>    arch/arm/include/asm/arch-sunxi/cpu_sun4i.h | 12 ++++++------
>>>>>    arch/arm/include/asm/arch-sunxi/mmc.h       |  3 +--
>>>>>    arch/arm/include/asm/arch-sunxi/timer.h     |  8 ++++----
>>>>>    board/sunxi/Kconfig                         |  9 +++++++++
>>>>>    board/sunxi/gmac.c                          |  6 +++---
>>>>>    drivers/mmc/sunxi_mmc.c                     |  3 +--
>>>>>    drivers/video/sunxi_display.c               | 10 +++++-----
>>>>>    10 files changed, 41 insertions(+), 34 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c
>>>>> b/arch/arm/cpu/armv7/sunxi/board.c
>>>>> index 6471c6b..30d5974 100644
>>>>> --- a/arch/arm/cpu/armv7/sunxi/board.c
>>>>> +++ b/arch/arm/cpu/armv7/sunxi/board.c
>>>>> @@ -173,7 +173,15 @@ void board_init_f(ulong dummy)
>>>>>
>>>>>    void reset_cpu(ulong addr)
>>>>>    {
>>>>> -#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) ||
>>>>> defined(CONFIG_MACH_SUN7I)
>>>>> +#ifdef CONFIG_ARCH_SUN6I
>>>>
>>>>
>>>>
>>>> Hello,
>>>>
>>>> this looks wrong.
>>>>
>>>> Either this is ARCH_SUNXI and it coverts all or it's ARCH_SUN6I and
>>>> then SUN4I and SUN5I should still be checked separately.
>>>
>>>
>>>
>>> Look closer, the blocks before / after the #else are swapped to avoid
>>> needing
>>> to use #ifndef without good reason as that is ugly.
>>
>>
>> So you mean that
>>
>> ARCH_SUN6I is
>> !defined(MACH_SUN4I) && !defined(MACH_SUN5I) && !defined(MACH_SUN7I) &&
>> (defined(MACH_SUN6I) || defined(MACH_SUN8I) || defined(MACH_SUN9I))
>>
>> Now that is ugly.
>>
>> This probably needs a better name then.
>
>
> What this needs is less bikeshedding and you to actually take your time
> to properly read the patch.
>
> If you read the help text in the Kconfig file then you will see that
>
> ARCH_SUN6I is set for sun6i derived designs, which all have a number
> of things in common, such as having separate ahb reset registers
> in the ccm.
>

Sorry, I see the code is technically correct.

However, if the purpose of this patch was to make the code clearer
then it fails at that.

It is unclear even reading the commit message, and that is not
included in the code.

It is not obvious which MACH_SUN?I are ARCH_SUN6I derived. So if you
can come up with a descriptive name for 'a number of things in common,
such as having separate ahb reset registers in the ccm' that's fine
otherwise this obfuscates the code, not clarifies.

That's not to say that simplifying the ifdefs does not have some
value. Just expect 'WTH does SUN7I not select SUN6I?' come up from
time to time.

Thanks

Michal