RE: [PATCH v3 3/7] eSPI: add eSPI controller support
> -Original Message-
> From: Anton Vorontsov [mailto:cbouatmai...@gmail.com]
> Sent: Friday, October 01, 2010 7:22 PM
> To: Hu Mingkai-B21284
> Cc: linuxppc-...@ozlabs.org; spi-devel-general@lists.sourceforge.net; linux-
> m...@lists.infradead.org; Gala Kumar-B11780
> Subject: Re: [PATCH v3 3/7] eSPI: add eSPI controller support
>
> Hello Mingkai,
>
> There are mostly cosmetic comments down below.
>
> > + /* spin until TX is done */
> > + while (((events = mpc8xxx_spi_read_reg(®_base->event))
> > + & SPIE_NF) == 0)
> > + cpu_relax();
>
> This is dangerous. There's a handy spin_event_timeout() in asm/delay.h.
>
When timeout, can I use return in the interrupt function directly like this?
if (!(events & SPIE_NF)) {
int ret;
/* spin until TX is done */
ret = spin_event_timeout(((events = mpc8xxx_spi_read_reg(
®_base->event)) & SPIE_NF) == 0, 1000, 0);
if (!ret) {
dev_err(mspi->dev, "tired waiting for SPIE_NF\n");
return;
}
}
> > +}
> > +
> > +static const struct of_device_id of_fsl_espi_match[] = {
> > + { .compatible = "fsl,mpc8536-espi" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, of_fsl_espi_match);
> > +
> > +static struct of_platform_driver 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(&fsl_espi_driver);
> > +}
> > +module_init(fsl_espi_init);
> > +
> > +static void __exit fsl_espi_exit(void) {
> > + of_unregister_platform_driver(&fsl_espi_driver);
> > +}
> > +module_exit(fsl_espi_exit);
> > +
> > +MODULE_AUTHOR("Mingkai Hu");
> > +MODULE_DESCRIPTION("Enhanced Freescale SPI Driver");
>
> This sounds like that this is an enhanced version of the Freescale SPI driver,
> which it is not. ;-)
>
I quoted from the UM, maybe the enhancement is the controller takes over the
CS signal from the HW point view.
I changed all the other code according to your comments.
Thanks,
Mingkai
--
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3.
Spend less time writing and rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH v3 3/7] eSPI: add eSPI controller support
Hello Mingkai,
There are mostly cosmetic comments down below.
On Thu, Sep 30, 2010 at 04:00:42PM +0800, Mingkai Hu wrote:
[...]
> +/*
> + * Freescale 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Please move the sysdev/ include after linux/.
Will make it a bit prettier. :-)
> +#include
> +#include
> +
> +#include "spi_fsl_lib.h"
[...]
> +static void fsl_espi_change_mode(struct spi_device *spi)
> +{
> + struct mpc8xxx_spi *mspi = spi_master_get_devdata(spi->master);
> + struct spi_mpc8xxx_cs *cs = spi->controller_state;
> + struct fsl_espi_reg *reg_base = (struct fsl_espi_reg *)mspi->reg_base;
No need for the type cast. The same for the rest of the code.
> + __be32 __iomem *mode;
> + __be32 __iomem *espi_mode = NULL;
> + u32 tmp;
> + unsigned long flags;
> +
> + espi_mode = ®_base->mode;
> + mode = ®_base->csmode[spi->chip_select];
Could save a few lines by turning this into initializers.
> +
> + /* Turn off IRQs locally to minimize time that SPI is disabled. */
> + local_irq_save(flags);
> +
> + /* Turn off SPI unit prior changing mode */
> + tmp = mpc8xxx_spi_read_reg(espi_mode);
> + mpc8xxx_spi_write_reg(espi_mode, tmp & ~SPMODE_ENABLE);
> + mpc8xxx_spi_write_reg(mode, cs->hw_mode);
> + mpc8xxx_spi_write_reg(espi_mode, tmp);
> +
> + local_irq_restore(flags);
> +}
> +
> +static u32 fsl_espi_tx_buf_lsb(struct mpc8xxx_spi *mpc8xxx_spi)
> +{
> + u32 data;
> + u16 data_h, data_l;
u16 data_h;
u16 data_l;
> +
No need for this empty line.
> + const u32 *tx = mpc8xxx_spi->tx;
<- Instead, add an empty line here.
> + if (!tx)
> + return 0;
> +
> + data = *tx++ << mpc8xxx_spi->tx_shift;
> + data_l = data & 0x;
> + data_h = (data >> 16) & 0x;
> + swab16s(&data_l);
> + swab16s(&data_h);
> + data = data_h | data_l;
> +
> + mpc8xxx_spi->tx = tx;
> + return data;
> +}
> +
> +static int fsl_espi_setup_transfer(struct spi_device *spi,
> + struct spi_transfer *t)
> +{
> + struct mpc8xxx_spi *mpc8xxx_spi;
> + int bits_per_word = 0;
> + u8 pm;
> + u32 hz = 0;
> + struct spi_mpc8xxx_cs *cs = spi->controller_state;
Stray tab.
> +
> + mpc8xxx_spi = spi_master_get_devdata(spi->master);
Could move this to the initializer.
> +
> + if (t) {
> + bits_per_word = t->bits_per_word;
> + hz = t->speed_hz;
> + }
> +
> + /* 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;
} else {
}
> +
> + 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));
No need to break this statement.
> +
> + 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;
> + }
> + if (pm)
> + pm--;
> +
> + cs->hw_mode |= CSMODE_PM(pm);
> +
> + fsl_espi_change_mode(spi);
> + return 0;
> +}
> +
> +int fsl_espi_cpu_bufs(struct mpc8xxx_spi *mspi, struct spi_transfer *t,
>
