Re: [PATCH 04/14] mmc: mmci: introduce dma_priv pointer to mmci_host

[Ulf Hansson]

On 1 August 2018 at 11:36, Ludovic Barre <ludovic.Barre@xxxxxx> wrote:
> From: Ludovic Barre <ludovic.barre@xxxxxx>
>
> 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@xxxxxx>
> ---
>  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