> -----Original Message-----
> From: Grant Likely [mailto:glik...@secretlab.ca] On Behalf Of
> Grant Likely
> Sent: Monday, July 26, 2010 8:25 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-...@ozlabs.org; ga...@kernel.crashing.org; Zang
> Roy-R61911
> Subject: Re: [PATCH 2/6] eSPI: add eSPI controller support
>
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
> > cd564e2..c647a00 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -196,6 +196,13 @@ config SPI_FSL_SPI
> > help
> > This enables using the Freescale SPI controllers in
> master mode.
> >
> > +config SPI_FSL_ESPI
> > + tristate "Freescale eSPI controller"
> > + depends on FSL_SOC
> > + select SPI_MPC8xxx
> > + help
> > + This enables using the Freescale eSPI controllers in
> master mode.
> > +
>
> Ditto from last patch. config SPI_MPC8xxx_SPI
>
Ok.
> > config SPI_OMAP_UWIRE
> > tristate "OMAP1 MicroWire"
> > depends on ARCH_OMAP1
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
> > dca9fea..6af459b 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -36,6 +36,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC)
> += mpc52xx_psc_spi.o
> > obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
> > obj-$(CONFIG_SPI_MPC8xxx) += spi_mpc8xxx.o
> > obj-$(CONFIG_SPI_FSL_SPI) += fsl_spi.o
> > +obj-$(CONFIG_SPI_FSL_ESPI) += fsl_espi.o
>
> spi_mpc8xxx_espi.o
>
Ok.
> > obj-$(CONFIG_SPI_PPC4xx) += spi_ppc4xx.o
> > obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
> > obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx_hw.o
> > diff --git a/drivers/spi/fsl_espi.c
> b/drivers/spi/fsl_espi.c new file
> > mode 100644 index 0000000..ac70c8c
> > --- /dev/null
> > +++ b/drivers/spi/fsl_espi.c
> > @@ -0,0 +1,562 @@
> > +/*
> > + * MPC8xxx eSPI controller driver.
> > + *
> > + * Copyright 2010 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms of the GNU General Public License as
> published
> > +by the
> > + * Free Software Foundation; either version 2 of the License, or
> > +(at your
> > + * option) any later version.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/irq.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/fsl_devices.h>
> > +#include <linux/mm.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_spi.h>
> > +#include <sysdev/fsl_soc.h>
> > +
> > +#include "spi_mpc8xxx.h"
> > +
> > +/* eSPI Controller mode register definitions */
> > +#define SPMODE_ENABLE (1 << 31)
> > +#define SPMODE_LOOP (1 << 30)
> > +#define SPMODE_TXTHR(x) ((x) << 8)
> > +#define SPMODE_RXTHR(x) ((x) << 0)
> > +
> > +/* eSPI Controller CS mode register definitions */
> > +#define CSMODE_CI_INACTIVEHIGH (1 << 31)
> > +#define CSMODE_CP_BEGIN_EDGECLK (1 << 30)
> > +#define CSMODE_REV (1 << 29)
> > +#define CSMODE_DIV16 (1 << 28)
> > +#define CSMODE_PM(x) ((x) << 24)
> > +#define CSMODE_POL_1 (1 << 20)
> > +#define CSMODE_LEN(x) ((x) << 16)
> > +#define CSMODE_BEF(x) ((x) << 12)
> > +#define CSMODE_AFT(x) ((x) << 8)
> > +#define CSMODE_CG(x) ((x) << 3)
> > +
> > +/* Default mode/csmode for eSPI controller */ #define
> SPMODE_INIT_VAL
> > +(SPMODE_TXTHR(4) | SPMODE_RXTHR(3)) #define CSMODE_INIT_VAL
> > +(CSMODE_POL_1 | CSMODE_BEF(0) \
> > + | CSMODE_AFT(0) | CSMODE_CG(1))
> > +
> > +/* SPIE register values */
> > +#define SPIE_NE 0x00000200 /* Not empty */
> > +#define SPIE_NF 0x00000100 /* Not full */
> > +
> > +/* SPIM register values */
> > +#define SPIM_NE 0x00000200 /* Not empty */
> > +#define SPIM_NF 0x00000100 /* Not full */
> > +#define SPIE_RXCNT(reg) ((reg >> 24) & 0x3F)
> > +#define SPIE_TXCNT(reg) ((reg >> 16) & 0x3F)
> > +
> > +/* SPCOM register values */
> > +#define SPCOM_CS(x) ((x) << 30)
> > +#define SPCOM_TRANLEN(x) ((x) << 0)
> > +#define SPCOM_TRANLEN_MAX 0xFFFF /* Max
> transaction length */
>
> Inconsistent whitespacing. Some lines use tabs; others
> spaces. Should be consistent on all the lines.
>
Ok.
> > +
> > +static
> > +int fsl_espi_setup_transfer(struct spi_device *spi, struct
> > +spi_transfer *t)
>
> Break the line in the arguments instead of the declaration.
> When grepping, the stuff at the front of the line is more
> important to see. So:
>
> +static int fsl_espi_setup_transfer(struct spi_device *spi,
> + struct spi_transfer *t)
>
Ok.
> > +{
> > + struct mpc8xxx_spi *mpc8xxx_spi;
> > + u8 bits_per_word, pm;
> > + u32 hz;
> > + struct spi_mpc8xxx_cs *cs = spi->controller_state;
> > +
> > + mpc8xxx_spi = spi_master_get_devdata(spi->master);
> > +
> > + if (t) {
> > + bits_per_word = t->bits_per_word;
> > + hz = t->speed_hz;
> > + } else {
> > + bits_per_word = 0;
> > + hz = 0;
> > + }
>
> Just initialize bits_per_word and hz to 0 when they are
> declared to eliminate the else clause.
>
Good suggestion.
> > +
> > + /* spi_transfer level calls that work per-word */
> > + if (!bits_per_word)
> > + bits_per_word = spi->bits_per_word;
> > +
> > + /* Make sure its a bit width we support [4..16] */
> > + if ((bits_per_word < 4) || (bits_per_word > 16))
> > + return -EINVAL;
> > +
> > + if (!hz)
> > + hz = spi->max_speed_hz;
> > +
> > + cs->rx_shift = 0;
> > + cs->tx_shift = 0;
> > + cs->get_rx = mpc8xxx_spi_rx_buf_u32;
> > + cs->get_tx = mpc8xxx_spi_tx_buf_u32;
> > + if (bits_per_word <= 8) {
> > + cs->rx_shift = 8 - bits_per_word;
> > + } else if (bits_per_word <= 16) {
> > + cs->rx_shift = 16 - bits_per_word;
> > + if (spi->mode & SPI_LSB_FIRST)
> > + cs->get_tx = fsl_espi_tx_buf_lsb;
> > + } else
> > + return -EINVAL;
> > +
> > + mpc8xxx_spi->rx_shift = cs->rx_shift;
> > + mpc8xxx_spi->tx_shift = cs->tx_shift;
> > + mpc8xxx_spi->get_rx = cs->get_rx;
> > + mpc8xxx_spi->get_tx = cs->get_tx;
> > +
> > + bits_per_word = bits_per_word - 1;
> > +
> > + /* mask out bits we are going to set */
> > + cs->hw_mode &= ~(CSMODE_LEN(0xF) | CSMODE_DIV16
> > + | CSMODE_PM(0xF));
> > +
> > + cs->hw_mode |= CSMODE_LEN(bits_per_word);
> > +
> > + if ((mpc8xxx_spi->spibrg / hz) > 64) {
> > + cs->hw_mode |= CSMODE_DIV16;
> > + pm = (mpc8xxx_spi->spibrg - 1) / (hz * 64) + 1;
> > +
> > + WARN_ONCE(pm > 16, "%s: Requested speed is too
> low: %d Hz. "
> > + "Will use %d Hz instead.\n",
> dev_name(&spi->dev),
> > + hz, mpc8xxx_spi->spibrg / 1024);
> > + if (pm > 16)
> > + pm = 16;
> > + } else
> > + pm = (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1;
>
> When the if clause has { }, please use braces in the else clause also.
>
Ok.
> > +static const struct of_device_id of_fsl_espi_match[] = {
> > + { .compatible = "fsl,espi" },
>
> Not good practice. Use the real chip name in the compatible
> value. "fsl,<chip>-espi".
>
Ok.
> > + {},
>
> NIT: Drop the comma here to hint that no more elements should
> follow after the null entry.
>
Ok.
> > +};
> > +MODULE_DEVICE_TABLE(of, of_fsl_espi_match);
> > +
> > +static struct of_platform_driver of_fsl_espi_driver = {
> > + .driver = {
> > + .name = "fsl_espi",
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_fsl_espi_match,
> > + },
> > + .probe = of_fsl_espi_probe,
> > + .remove = __devexit_p(of_fsl_espi_remove),
> > +};
> > +
> > +static int __init fsl_espi_init(void) {
> > + return of_register_platform_driver(&of_fsl_espi_driver);
> > +}
> > +
> > +static void __exit fsl_espi_exit(void) {
> > + of_unregister_platform_driver(&of_fsl_espi_driver);
> > +}
> > +
> > +module_init(fsl_espi_init);
>
> Move module_init() to right below fsl_espi_init.
>
Ok.
> > +module_exit(fsl_espi_exit);
> > +
> > +MODULE_AUTHOR("Mingkai Hu");
> > +MODULE_DESCRIPTION("Enhanced MPC8xxx SPI Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/spi/spi_mpc8xxx.h b/drivers/spi/spi_mpc8xxx.h
> > index dcc6443..a8e8270 100644
> > --- a/drivers/spi/spi_mpc8xxx.h
> > +++ b/drivers/spi/spi_mpc8xxx.h
> > @@ -20,6 +20,7 @@
> >
> > /* SPI Controller registers */
> > struct mpc8xxx_spi_reg {
> > +#ifndef CONFIG_SPI_FSL_ESPI
> > u8 res1[0x20];
> > __be32 mode;
> > __be32 event;
> > @@ -27,6 +28,16 @@ struct mpc8xxx_spi_reg {
> > __be32 command;
> > __be32 transmit;
> > __be32 receive;
> > +#else
> > + __be32 mode; /* 0x000 - eSPI mode register */
> > + __be32 event; /* 0x004 - eSPI event register */
> > + __be32 mask; /* 0x008 - eSPI mask register */
> > + __be32 command; /* 0x00c - eSPI command register */
> > + __be32 transmit; /* 0x010 - eSPI transmit FIFO
> access register*/
> > + __be32 receive; /* 0x014 - eSPI receive FIFO
> access register*/
> > + u8 res1[8]; /* 0x018 - 0x01c reserved */
> > + __be32 csmode[4]; /* 0x020 - 0x02c eSPI cs mode
> register */
> > +#endif
>
> Not multiplatform friendly. If the two devices use different
> register maps, then the register map needs to be defined in
> the .c file. You need to code for the case where a single
> kernel may contain both drivers.
>
You're right, this will disable the kernel supporting both drivers.
I'll fix it.
Thanks,
Mingkai
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
