[PATCH v2 2/6] eSPI: add eSPI controller support

[Hu Mingkai-B21284]

> -----Original Message-----
> From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On Behalf Of Grant
> Likely
> Sent: Tuesday, August 10, 2010 2:45 PM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev at ozlabs.org; spi-devel-general at lists.sourceforge.net; Gala
> Kumar-B11780; Zang Roy-R61911
> Subject: Re: [PATCH v2 2/6] eSPI: add eSPI controller support
> 
> On Mon, Aug 2, 2010 at 1:52 AM, Mingkai Hu <Mingkai.hu at freescale.com> wrote:
> > Add eSPI controller support based on the library code spi_fsl_lib.c.
> >
> > The eSPI controller is newer controller 85xx/Pxxx devices supported.
> > There're some differences comparing to the SPI controller:
> >
> > 1. Has different register map and different bit definition
> >   So leave the code operated the register to the driver code, not
> >   the common code.
> >
> > 2. Support 4 dedicated chip selects
> >   The software can't controll the chip selects directly, The SPCOM[CS]
> >   field is used to select which chip selects is used, and the
> >   SPCOM[TRANLEN] field is set to tell the controller how long the CS
> >   signal need to be asserted. So the driver doesn't need the
> > chipselect
> >   related function when transfering data, just set corresponding
> > register
> >   fields to controll the chipseclect.
> >
> > 3. Different Transmit/Receive FIFO access register behavior
> >   For SPI controller, the Tx/Rx FIFO access register can hold only
> >   one character regardless of the character length, but for eSPI
> >   controller, the register can hold 4 or 2 characters according to
> >   the character lengths. Access the Tx/Rx FIFO access register of the
> >   eSPI controller will shift out/in 4/2 characters one time. For SPI
> >   subsystem, the command and data are put into different transfers, so
> >   we need to combine all the transfers to one transfer in order to
> > pass
> >   the transfer to eSPI controller.
> >
> > Signed-off-by: Mingkai Hu <Mingkai.hu at freescale.com>
> 
> I've not dug deep into this patch, but it seems pretty good.  I did notice one
> thing below...
> [...]
> 
> > diff --git a/drivers/spi/spi_fsl_lib.h b/drivers/spi/spi_fsl_lib.h
> > index 774e1c8..0772c98 100644
> > --- a/drivers/spi/spi_fsl_lib.h
> > +++ b/drivers/spi/spi_fsl_lib.h
> > @@ -22,10 +22,12 @@
> >  struct mpc8xxx_spi {
> >        struct device *dev;
> >        struct fsl_spi_reg __iomem *base;
> > +       struct fsl_espi_reg __iomem *espi_base;
> 
> There's got to be a cleaner way to do this.  Rather than putting both pointers
> into mpc8xxx_spi, I suggest each driver use its own fsl_spi_priv and
> fsl_espi_priv wrapper structures that contain the controller specific properties.
> 

Make sense, I'll change it.

Thanks,
Mingkai