On Tue, Feb 06, 2018 at 12:21:15AM +0100, Boris Brezillon wrote:
> Some controllers are exposing high-level interfaces to access various
> kind of SPI memories. Unfortunately they do not fit in the current
> spi_controller model and usually have drivers placed in
> drivers/mtd/spi-nor which are only supporting SPI NORs and not SPI
> memories in general.
> This is an attempt at defining a SPI memory interface which works for
> all kinds of SPI memories (NORs, NANDs, SRAMs).
It would be helpful if this changelog were to describe what the
problems are and how the patch addresses them. Someone reading the git
history should be able to tell what the patch is doing without having to
google around to find a cover letter for the patch series. It also
feels like this should be split up a bit - there's some preparatory
refactoring here, a new API, and an adaption layer to implement that API
with actual SPI controllers. It's a very large patch doing several
different things and splitting it up would make it easier to review.
I have to say I'm rather unclear why this is being done in the SPI core
rather than as a layer on top of it - devices doing the new API can't
support normal SPI clients at all and everything about this is entirely
flash specific. You've got a bit about that in your cover letter, I'll
reply there.
> @@ -2155,10 +2181,14 @@ int spi_register_controller(struct spi_controller *ctlr)
> spi_controller_is_slave(ctlr) ? "slave" : "master",
> dev_name(&ctlr->dev));
>
> - /* If we're using a queued driver, start the queue */
> - if (ctlr->transfer)
> + /*
> + * If we're using a queued driver, start the queue. Note that we don't
> + * need the queueing logic if the driver is only supporting high-level
> + * memory operations.
> + */
> + if (ctlr->transfer) {
> dev_info(dev, "controller is unqueued, this is deprecated\n");
> - else {
> + } else if (ctlr->transfer_one || ctlr->transfer_one_message) {
> status = spi_controller_initialize_queue(ctlr);
> if (status) {
> device_del(&ctlr->dev);
This for example feels like a separate refactoring which could be split
out.
> + if (op->data.dir == SPI_MEM_DATA_OUT)
> + dmadev = ctlr->dma_tx ?
> + ctlr->dma_tx->device->dev : ctlr->dev.parent;
> + else
> + dmadev = ctlr->dma_rx ?
> + ctlr->dma_rx->device->dev : ctlr->dev.parent;
Please don't abuse the ternery operator like this :(
Attachment:
signature.asc
Description: PGP signature
