RE: [PATCH v3 3/7] eSPI: add eSPI controller support

[Hu Mingkai-B21284]

> -----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-gene...@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(&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(
                        &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

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev