> -----Original Message----- > From: Mark Brown [mailto:broonie@xxxxxxxxxx] > Sent: 12 November 2014 23:06 > To: Andrew Bresticker > Cc: James Hartley; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; > Kumar Gala; Grant Likely; Ezequiel Garcia; devicetree@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/2] spi: Add driver for IMG SPFI controller > > On Wed, Nov 12, 2014 at 02:54:57PM -0800, Andrew Bresticker wrote: > > On Wed, Nov 12, 2014 at 2:07 PM, Mark Brown <broonie@xxxxxxxxxx> > wrote: > > > >> drivers/spi/spi-img.c | 703 > > >> ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > How about spi-img-spfi? That way if someone makes another SPI > > > controller (say a more generic one, this one seems flash > > > specialized) there won't be a collision. > > > Despite the name, I believe this controller is used for generic SPI > > stuff as well. I'm not sure if there is a separate one which is more > > generic (James?). There is another generic img spi hardware block which is not register compatible and doesn't have the flash interface which is also in existing SoC's. There is a separate img-spi driver for that which has not yet been mainlined. I think spi-img-spfi would be ok for this driver. There is also another sfc (Serial Flash Controller), block in the pipeline, so that could then be called spi-img-sfc. > > It would still be better to use a name less impressively generic - this is an > entire company, not even a product line. > > > >> + cpu_relax(); > > > > Seems random - we already relax in the data transfer? > > > We don't relax in the transfers - would that be a better place to put > > it? I thought it was better here since we reach this point once the > > TX FIFO has filled or RX FIFO has emptied. > > Oh, that was the FIFO drain I was thinking of. I guess here is fine. > > > >> + if (tx_buf) > > >> + spfi_flush_tx_fifo(spfi); > > >> + spfi_disable(spfi); > > > > What does the enable and disable actually do? Should this be > > > runtime PM? > > > It starts/stops the transfer. The control registers (bit clock, > > transfer mode, etc.) must be programmed before the enable bit is set > > and the bit does not clear itself upon completion of the transfer. I > > don't think it makes sense to have this be part of runtime PM. > > Perhaps these functions need to be called start() and stop() then - the > names sound like they gate the IP? > > > > This will unconditionally claim to have handled an interrupt even if > > > it didn't get any interrupt status it has ever heard of. At the > > > very least it should log unknown interrupts, ideally return IRQ_NONE > > > though since it seems to be a clear on read interrupt that's a bit > misleading. > > > There's a clear register actually (see the writel() above), but yes, > > an error and returning IRQ_NONE sound appropriate in the event of an > > unexpected interrupt. > > Don't add the error print - the interrupt core has diagnostics already and it > won't be nice if the interrupt ends up shared. My recommendation was > intended as an either/or. James. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
