[PATCH-V2 4/4] ARM: OMAP2+: CLEANUP: Add new config option for different DPLL features

[Hiremath, Vaibhav]

On Wed, May 09, 2012 at 04:20:06, Hilman, Kevin wrote:
> Vaibhav Hiremath <hvaibhav at ti.com> writes:
> 
> > This patch cleans up dpll_data structure, making structure
> > fields definition based on feature availability for given SoC,
> > managed using Kconfig rules.
> >
> > SOC_HAS_OMAP3_DPLL_IDLE: idle state
> > SOC_HAS_OMAP3_DPLL_RECAL: recalibration capability
> > SOC_HAS_OMAP3_DPLL_DCO_SEL: dco selection
> > SOC_HAS_OMAP3_DPLL_SDDIV: sigma-delta div factor
> > SOC_HAS_OMAP3_DPLL_FREQSEL: frequency selection
> >
> > Signed-off-by: Vaibhav Hiremath <hvaibhav at ti.com>
> > Cc: Tony Lindgren <tony at atomide.com>
> > Cc: Kevin Hilman <khilman at ti.com>
> > Cc: Paul Walmsley <paul at pwsan.com>
> 
> Paul is the one to make the call here, but personally I'd much prefer to
> just see the existing ifdefs go away[1] instead of adding a bunch of new
> ones.  
> 
> Yes, doing so would add a few fields to a struct that may be unused, but
> IMO, the improvement in readabilitly and maintainability is improved.
> 
> Note that the users of these fields are not changed and are still based
> on Kconfig/Makefile values as before (e.g. CONFIG_ARCH_OMAP3), so to me
> this creates another layer of obfuscation.
> 

This will bring in difference on omap2 alone build OR omap4, am33xx alone 
builds.
But again, how much it makes sense, and how much benefits we bring-in by 
adding config options for every bit-fields (like this) needs to be looked at.
And that's the reason, I submitted first version as a RFC.


> 
> [1]
> diff --git a/arch/arm/plat-omap/include/plat/clock.h b/arch/arm/plat-omap/include/plat/clock.h
> index d0ef57c..656b986 100644
> --- a/arch/arm/plat-omap/include/plat/clock.h
> +++ b/arch/arm/plat-omap/include/plat/clock.h
> @@ -156,7 +156,6 @@ struct dpll_data {
>  	u8			min_divider;
>  	u16			max_divider;
>  	u8			modes;
> -#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
>  	void __iomem		*autoidle_reg;
>  	void __iomem		*idlest_reg;
>  	u32			autoidle_mask;
> @@ -167,7 +166,6 @@ struct dpll_data {
>  	u8			auto_recal_bit;
>  	u8			recal_en_bit;
>  	u8			recal_st_bit;
> -#  endif
>  	u8			flags;
>  };
>  

Honestly, I wanted to propose this first. 
I am OK, as long as we all agree on this.

Paul, can you conform on this?

Thanks,
Vaibhav