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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 09, 2012 at 04:20:06, Hilman, Kevin wrote:
> Vaibhav Hiremath <hvaibhav@xxxxxx> 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@xxxxxx>
> > Cc: Tony Lindgren <tony@xxxxxxxxxxx>
> > Cc: Kevin Hilman <khilman@xxxxxx>
> > Cc: Paul Walmsley <paul@xxxxxxxxx>
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux