lkml.org 
[lkml]   [2018]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 04/14] mmc: mmci: introduce dma_priv pointer to mmci_host
On 1 August 2018 at 11:36, Ludovic Barre <ludovic.Barre@st.com> wrote:
> From: Ludovic Barre <ludovic.barre@st.com>
>
> 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.

>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
> 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.

> +
> +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

\
 
 \ /
  Last update: 2018-09-03 14:16    [W:0.175 / U:1.608 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site