Re: [PATCH 1/5] ARM: vexpress: Get rid of MMIO_P2V

[Pawel Moll <pawel.moll@arm.com>]

On Thu, 2011-11-17 at 15:43 +0000, Russell King - ARM Linux wrote:
> > +/* Tile's peripherals static mappings should start here */
> > +#define V2T_PERIPH 0xf8200000
> > +#define V2T_PERIPH_P2V(offset) ((void __iomem *)(V2T_PERIPH | (offset)))
> > +
> 
> Please get rid of these blank lines at the end of files.

Patch splitting leftover. Will fix.

> > +static void __iomem *v2m_sysreg_base;
> > +
> > +
> > +
> 
> More useless blank lines.

Ok, will change that. I just like the way that the data are visually
separated from the code like here:

ceade897 (Russell King 2010-02-11 21:44:53 +0000  68)         .init   = v2m_timer_init,
ceade897 (Russell King 2010-02-11 21:44:53 +0000  69) };
ceade897 (Russell King 2010-02-11 21:44:53 +0000  70) 
ceade897 (Russell King 2010-02-11 21:44:53 +0000  71) 
ceade897 (Russell King 2010-02-11 21:44:53 +0000  72) static DEFINE_SPINLOCK(v2m_cfg_lock);

or here:

ceade897 (Russell King 2010-02-11 21:44:53 +0000 289)         &rtc_device,
ceade897 (Russell King 2010-02-11 21:44:53 +0000 290) };
ceade897 (Russell King 2010-02-11 21:44:53 +0000 291) 
ceade897 (Russell King 2010-02-11 21:44:53 +0000 292) 
ceade897 (Russell King 2010-02-11 21:44:53 +0000 293) static long v2m_osc_round(struct clk *clk, unsigned long rate)

> >  static void __init v2m_timer_init(void)
> >  {
> > +	void *sysctl_base;
> > +	void *timer01_base;
> 
> Do you not use sparse?  __iomem.

Ok.

> > +	unsigned int timer01_irq;
> >  	u32 scctrl;
> >  
> > +		sysctl_base = ioremap(V2M_SYSCTL, SZ_4K);
> > +		BUG_ON(!sysctl_base);
> > +		timer01_base = ioremap(V2M_TIMER01, SZ_4K);
> > +		BUG_ON(!timer01_base);
> > +		timer01_irq = IRQ_V2M_TIMER0;
> 
> What's going on with the indentation here?

Patch-splitting artefact again, will fix.

> > @@ -413,6 +431,10 @@ static void __init v2m_populate_ct_desc(void)
> >  static void __init v2m_map_io(void)
> >  {
> >  	iotable_init(v2m_io_desc, ARRAY_SIZE(v2m_io_desc));
> > +
> > +	/* Will become an ioremap() when possible */
> > +	v2m_sysreg_base = V2M_PERIPH_P2V(V2M_SYSREGS);
> 
> It won't if it stays here.

Excuse my inferior English language capabilities, but what do you
suggest here?

All comments appreciated as always, thanks!

Pawel