> -----Original Message-----
> From: Grant Likely [mailto:glik...@secretlab.ca] On Behalf Of
> Grant Likely
> Sent: Monday, July 26, 2010 8:14 AM
> To: Hu Mingkai-B21284; "@angua.secretlab.ca"@angua.secretlab.ca
> Cc: linuxppc-...@ozlabs.org; ga...@kernel.crashing.org; Zang
> Roy-R61911
> Subject: Re: [PATCH 1/6] spi/mpc8xxx: refactor the common
> code for SPI/eSPI controller
>
> On Tue, Jul 20, 2010 at 10:08:20AM +0800, Mingkai Hu wrote:
> > Refactor the common code to file spi_mpc8xxx.c used by SPI/eSPI
> > controller driver, move the SPI controller driver to a new file
> > fsl_spi.c, and leave the QE/CPM SPI controller code in this file.
> >
> > Because the register map of the SPI controller and eSPI controller
> > is so different, also leave the code operated the register to the
> > driver code, not the common code.
> >
> > Signed-off-by: Mingkai Hu <mingkai...@freescale.com>
> > ---
> > drivers/spi/Kconfig | 13 +-
> > drivers/spi/Makefile | 1 +
> > drivers/spi/fsl_spi.c | 1118
> ++++++++++++++++++++++++++++++++++++++++++
> > drivers/spi/spi_mpc8xxx.c | 1198
> ++-------------------------------------------
> > drivers/spi/spi_mpc8xxx.h | 135 +++++
>
> Please name files spi-*.[ch]. I'm going to start enforcing
> this naming convention in the drivers/spi directory.
>
Ok.
> > 5 files changed, 1299 insertions(+), 1166 deletions(-)
> > create mode 100644 drivers/spi/fsl_spi.c
> > create mode 100644 drivers/spi/spi_mpc8xxx.h
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 91c2f4f..cd564e2 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -183,11 +183,18 @@ config SPI_MPC512x_PSC
> > Controller in SPI master mode.
> >
> > config SPI_MPC8xxx
> > - tristate "Freescale MPC8xxx SPI controller"
> > + bool
>
> This should be tristate so it can be loaded as a module.
>
This option is selected by FSL_SPI and FSL_ESPI option, can we load the
driver as a module?
> > depends on FSL_SOC
> > help
> > - This enables using the Freescale MPC8xxx SPI
> controllers in master
> > - mode.
> > + This enables using the Freescale MPC8xxx SPI/eSPI controllers
> > + driver library.
>
> Drop the help text entirely. It isn't needed on
> non-user-visible options.
>
Ok.
> > +
> > +config SPI_FSL_SPI
> > + tristate "Freescale SPI controller"
>
> "Freescale SPI controller is rather generic and doesn't give any clues
> as to which devices actually have this controller. At the very least
> the help text should state what parts contain this controller.
>
Ok.
> On that note, the naming convention seems a little loose.
> Since both the eSPI and SPI are using SPI_MPC8xxx, then
> really the names should be:
>
> config SPI_MPC8xxx_SPI
> and
> config SPI_MPC8xxx_ESPI
>
I'll chage it, SPI_MPC8xxx_SPI/ESPI is more clearer.
> > +static void __exit fsl_spi_exit(void)
> > +{
> > + of_unregister_platform_driver(&of_fsl_spi_driver);
> > + legacy_driver_unregister();
> > +}
>
> It would appear that the legacy driver should *also* be
> separated out into its own module. I realize you're just cut
> & pasting code here, but it should be considered for a followup patch.
>
Can we remove the legacy driver thoroughly?
If we separated out the code, what's the proper name for it?
spi_mpc8xxx_legacy.c?
> > +
> > +module_init(fsl_spi_init);
> > +module_exit(fsl_spi_exit);
>
> module_init() should appear immediately after the init
> fsl_spi_init() function.
>
Ok.
Thanks,
Mingkai
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
