[PATCH 04/14] mmc: mmci: introduce dma_priv pointer to mmci_host
From: Ulf Hansson <hidden>
Date: 2018-09-03 12:15:31
Also in:
linux-devicetree, linux-mmc, lkml
On 1 August 2018 at 11:36, Ludovic Barre [off-list ref] wrote:
From: Ludovic Barre <redacted> This patch introduces dma_priv pointer to define specific needs for each dma engine. This patch is needed to prepare sdmmc variant with internal dma which not use dmaengine API.
Overall this looks good, however a couple a few things below, mostly nitpicks.
quoted hunk ↗ jump to hunk
Signed-off-by: Ludovic Barre <redacted> --- drivers/mmc/host/mmci.c | 165 +++++++++++++++++++++++++-------------- drivers/mmc/host/mmci.h | 20 +---- drivers/mmc/host/mmci_qcom_dml.c | 6 +- 3 files changed, 112 insertions(+), 79 deletions(-)diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index 8144a87..bdc48c3 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c@@ -415,31 +415,57 @@ static void mmci_init_sg(struct mmci_host *host, struct mmc_data *data) * no custom DMA interfaces are supported. */ #ifdef CONFIG_DMA_ENGINE -static void mmci_dma_setup(struct mmci_host *host) +struct dmaengine_next {
I would rather rename this struct to something along the lines of "mmci_dma_next", that should follow how most of the data structures are named in mmci.
+ struct dma_async_tx_descriptor *dma_desc; + struct dma_chan *dma_chan;
For these two, I think you should remove the "dma_" prefix from their names. At least to me, it's of obvious they are about dma if they are part of a struct used (and named) used solely for that purpose.
+ s32 cookie;
+};
+
+struct dmaengine_priv {
+ struct dma_chan *dma_current;
+ struct dma_chan *dma_rx_channel;
+ struct dma_chan *dma_tx_channel;
+ struct dma_async_tx_descriptor *dma_desc_current;
+ struct dmaengine_next next_data;
+ bool dma_in_progress;For similar reasons as above, I suggest to rename the struct to "mmci_dma_priv" and to drop the "dma_" prefix from the variable names.
+}; + +#define __dmae_inprogress(dmae) ((dmae)->dma_in_progress)
How about naming this to mmci_dma_inprogress() instead? BTW, in general it looks like you are a bit fond of using "__" as function name prefix for internally called functions. Please try to avoid that, but rather try to pick names that explains what the functions do.
quoted hunk ↗ jump to hunk
+ +static int mmci_dma_setup(struct mmci_host *host) { const char *rxname, *txname; + struct dmaengine_priv *dmae; - host->dma_rx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "rx"); - host->dma_tx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "tx"); + dmae = devm_kzalloc(mmc_dev(host->mmc), sizeof(*dmae), GFP_KERNEL); + if (!dmae) + return -ENOMEM; + + host->dma_priv = dmae; + + dmae->dma_rx_channel = dma_request_slave_channel(mmc_dev(host->mmc), + "rx"); + dmae->dma_tx_channel = dma_request_slave_channel(mmc_dev(host->mmc), + "tx"); /* initialize pre request cookie */ - host->next_data.cookie = 1; + dmae->next_data.cookie = 1; /* * If only an RX channel is specified, the driver will * attempt to use it bidirectionally, however if it is * is specified but cannot be located, DMA will be disabled. */ - if (host->dma_rx_channel && !host->dma_tx_channel) - host->dma_tx_channel = host->dma_rx_channel; + if (dmae->dma_rx_channel && !dmae->dma_tx_channel) + dmae->dma_tx_channel = dmae->dma_rx_channel; - if (host->dma_rx_channel) - rxname = dma_chan_name(host->dma_rx_channel); + if (dmae->dma_rx_channel) + rxname = dma_chan_name(dmae->dma_rx_channel); else rxname = "none"; - if (host->dma_tx_channel) - txname = dma_chan_name(host->dma_tx_channel); + if (dmae->dma_tx_channel) + txname = dma_chan_name(dmae->dma_tx_channel); else txname = "none";@@ -450,15 +476,15 @@ static void mmci_dma_setup(struct mmci_host *host) * Limit the maximum segment size in any SG entry according to * the parameters of the DMA engine device. */ - if (host->dma_tx_channel) { - struct device *dev = host->dma_tx_channel->device->dev; + if (dmae->dma_tx_channel) { + struct device *dev = dmae->dma_tx_channel->device->dev; unsigned int max_seg_size = dma_get_max_seg_size(dev); if (max_seg_size < host->mmc->max_seg_size) host->mmc->max_seg_size = max_seg_size; } - if (host->dma_rx_channel) { - struct device *dev = host->dma_rx_channel->device->dev; + if (dmae->dma_rx_channel) { + struct device *dev = dmae->dma_rx_channel->device->dev; unsigned int max_seg_size = dma_get_max_seg_size(dev); if (max_seg_size < host->mmc->max_seg_size)@@ -466,7 +492,9 @@ static void mmci_dma_setup(struct mmci_host *host) } if (host->ops && host->ops->dma_setup) - host->ops->dma_setup(host); + return host->ops->dma_setup(host);
I agree that converting the ->dma_setup() callback to return an int makes sense. However, please make that a separate change and while doing that, don't forget to implement the error path, as that is missing here.
+ + return 0; }
[...] Kind regards Uffe