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

[David Lechner]

On 12/08/2017 09:34 AM, Sekhar Nori wrote:
> 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

Thank you for the suggestion. I think it will be better as well. I'll 
respin the series and we'll see how it works.