Re: [Patch 2/3] via-sdmmc: via-sdmmc.c

[Ben Dooks <ben-linux@fluff.org>]

On Tue, Dec 16, 2008 at 02:31:42PM +0100, Arnd Bergmann wrote:
> On Tuesday 16 December 2008, JosephChan@via.com.tw wrote:
> > VIA MSP SD/MMC card reader driver of via-sdmmc
> 
> The code looks very nice, good work.
> 
> > +static void via_save_pcictrlreg(struct via_crdr_chip *vcrdr_chip)
> > +{
> > +	struct pcictrlreg *pm_pcictrl_reg;
> > +	void __iomem *addrbase;
> > +
> > +	pm_pcictrl_reg = &(vcrdr_chip->pm_pcictrl_reg);
> > +	addrbase = vcrdr_chip->pcictrl_mmiobase;
> > +
> > +	pm_pcictrl_reg->pciclkgat_reg = readb(addrbase + PCICLKGATT_REG);
> > +	pm_pcictrl_reg->pciclkgat_reg |=
> > +	    PCI_CLKGATT_POWSEL | PCI_CLKGATT_POWOFF;
> > +	pm_pcictrl_reg->pcimscclk_reg = readb(addrbase + PCIMSCCLK_REG);
> > +	pm_pcictrl_reg->pcisdclk_reg = readb(addrbase + PCISDCCLK_REG);
> > +	pm_pcictrl_reg->pcidmaclk_reg = readb(addrbase + PCIDMACLK_REG);
> > +	pm_pcictrl_reg->pciintctrl_reg = readb(addrbase + PCIINTCTRL_REG);
> > +	pm_pcictrl_reg->pciintstatus_reg = readb(addrbase + PCIINTSTATUS_REG);
> > +	pm_pcictrl_reg->pcitmoctrl_reg = readb(addrbase + PCITMOCTRL_REG);
> > +}
> 
> Since you already define the data structure for the save area, how about
> using it for the register accesses as well? You could drop all the PCI*_REG
> macro definitions and do

GAH! NO, I belive Linus has very clear views about using structures
to define register layouts (and I share them).
 
> struct pcictrlreg __iomem *pcr = vcrdr_chip->pcictrl_mmiobase;
> pm_pcictrl_reg->pcisdclk_reg = readb(&pcr->pcisdclk_reg);

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'