[U-Boot] [PATCH v4 1/7] arm: vf610: Add IOMUX support for Vybrid VF610

[Benoît Thébaudeau]

Hi Stefano,

On Wednesday, May 29, 2013 8:21:00 AM, Stefano Babic wrote:
> On 29/05/2013 07:29, Wang Huan-B18965 wrote:
> 
> >> Where is this one defined? I don't see it in include/configs/vf610twr.h.
> >>
> > [Alison Wang] CONFIG_IOMUX_SHARE_CONF_REG is defined in
> > arch/arm/include/asm/arch-vf610/imx-regs.h. Because this is not a board
> > configuration, it is related to the SOC.
> > 
> > Please refer to Stefano's comments below which also could be found in the
> > email on May 15th.
> > 
> > Stefano wrote:
> >> +
> >> +/* MUX mode and PAD ctrl are in one register */
> >> +#define CONFIG_IOMUX_SHARE_CONF_REG
> > 
> > NAK. This is not a board configuration, it is related to the SOC. This
> > setup should flow into the related imx-regs.h for this SOC. When you set
> > CONFIG_MVF600, this value should be set automatically.
> > 
> >> Why not use "#ifdef CONFIG_VF610" since this is a platform-dependent
> >> code, and not a board-specific config option?
> > [Alison Wang] I use this CONFIG_IOMUX_SHARE_CONF_REG option, because this
> > part of codes
> > not only could be used on VF610 platform, but also could be used on VF620
> > or other platforms.
> > When it is used on VF620 or others, you could just enable
> > CONFIG_IOMUX_SHARE_CONF_REG
> > in the related imx-regs.h.
> > Otherwise, if "ifdef CONFIG_VF610" is used, you need to add "#if
> > defined(CONFIG_VF610) || defined(CONFIG_VF620)"
> > When this part of codes is also used on VF620. Then when this part of codes
> > is used on VF630 too, this line
> > will be very very long.
> 
> Agree. This is a property of the processor and should be automatically
> set. It should not flow into board config file, and having a family of
> processor we cannot use ifdef CONFIG_VF610.
> 
> IMHO the patch is ok.
> 
> Acked-by: Stefano Babic <sbabic at denx.de>

I agree:
Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau at advansee.com>

Best regards,
Benoît