Re: [PATCH v1 1/6] ARM: davinci: clean up map_io functions

[Sekhar Nori]

On Thursday 07 December 2017 10:44 PM, David Lechner wrote:
> On 12/07/2017 08:52 AM, Sekhar Nori wrote:
>> On Saturday 02 December 2017 08:04 AM, David Lechner wrote:
>>> This cleans up the map_io functions in the board init files for
>>> mach-davinci.
>>>
>>> Most of the boards had a wrapper function around <board>_init(). This
>>> wrapper is removed and the function is used directly. Additionally, the
>>> <board>_init() functions are renamed to <board>_map_io() to match the
>>> field name.
>>>
>>> Signed-off-by: David Lechner <david@xxxxxxxxxxxxxx>
>>
>>> diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c
>>> b/arch/arm/mach-davinci/board-dm646x-evm.c
>>> index cb0a41e..f0e2762 100644
>>> --- a/arch/arm/mach-davinci/board-dm646x-evm.c
>>> +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
>>> @@ -716,16 +716,6 @@ static void __init evm_init_i2c(void)
>>> Â }
>>> Â #endif
>>> Â -#define DM6467T_EVM_REF_FREQÂÂÂÂÂÂÂ 33000000
>>> -
>>> -static void __init davinci_map_io(void)
>>> -{
>>> -ÂÂÂ dm646x_init();
>>
>> The call to dm646x_init() is dropped here, but I don't see it added
>> back, at least in this patch.
> 
> dm646x_init() is renamed to dm646x_map_io(), which is used directly in
> MACHINE_START().

Ah, I missed that. But I think its a symptom of too many things going on
in the patch. How about splitting the patch to:

a) Remove trivial <board>_map_io() wrappers and use <soc>_init()
directly to initialize .map_io

b) Rename <soc>_init() to <soc>_map_io

> 
>>
>>> diff --git a/arch/arm/mach-davinci/dm646x.c
>>> b/arch/arm/mach-davinci/dm646x.c
>>> index da21353..b3be5c8 100644
>>> --- a/arch/arm/mach-davinci/dm646x.c
>>> +++ b/arch/arm/mach-davinci/dm646x.c
>>> @@ -17,6 +17,7 @@
>>> Â #include <linux/platform_data/edma.h>
>>> Â #include <linux/platform_data/gpio-davinci.h>
>>> Â +#include <asm/mach-types.h>
>>> Â #include <asm/mach/map.h>
>>> Â Â #include <mach/cputype.h>
>>> @@ -952,11 +953,16 @@ int __init dm646x_init_edma(struct
>>> edma_rsv_info *rsv)
>>> ÂÂÂÂÂ return IS_ERR(edma_pdev) ? PTR_ERR(edma_pdev) : 0;
>>> Â }
>>> Â -void __init dm646x_init(void)
>>> +#define DM6467T_EVM_REF_FREQÂÂÂÂÂÂÂ 33000000
>>> +
>>> +void __init dm646x_map_io(void)
>>> Â {
>>> ÂÂÂÂÂ davinci_common_init(&davinci_soc_info_dm646x);
>>> ÂÂÂÂÂ davinci_map_sysmod();
>>> ÂÂÂÂÂ davinci_clk_init(davinci_soc_info_dm646x.cpu_clks);
>>> +
>>> +ÂÂÂ if (machine_is_davinci_dm6467tevm())
>>> +ÂÂÂÂÂÂÂ davinci_set_refclk_rate(DM6467T_EVM_REF_FREQ);
>>> Â }
>>
>> I think we should leave the DM646x case out of this since there are
>> additional issues like introducing these EVM specific defines in a file
>> meant for SoC.
> 
> I agree with the sentiment. This quirk gets moved around several times
> in this series just to keep things working for a git bisect even if it
> is not the ideal place for it to be.
> 
> Currently, all boards use a common reference frequency from the common
> SoC files instead of the board file. I have not done so in this version
> of the series, but I could rework it so that this happens, which would
> remove the need for this quirk altogether. But even then, it would
> probably get shuffled around a bit before being eliminated.

We should avoid shuffling and modifying the same code repeatedly if we
can. I think, it will be easier to read if patch 2 and 1 are interchanged.

There is a lot going on and I wasn't sure how that will look so I tried
it briefly and pushed a branch.

Do have a look, but this seems better to me.

https://git.kernel.org/pub/scm/linux/kernel/git/nsekhar/linux-davinci.git/log/?h=for-davidl

Thanks,
Sekhar