Re: [PATCH v2 11/16] bus: mhi: core: Add support for data transfer

From: Arnd Bergmann <arnd@arndb.de>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: gregkh <gregkh@linuxfoundation.org>,
	smohanad@codeaurora.org, Jeffrey Hugo <jhugo@codeaurora.org>,
	Kalle Valo <kvalo@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	hemantk@codeaurora.org,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 11/16] bus: mhi: core: Add support for data transfer
Date: Mon, 17 Feb 2020 17:13:37 +0100	[thread overview]
Message-ID: <CAK8P3a1DOta5Uj3dNFWVqwgyPKs0cQsoymXE7svcOZoPiY+YGw@mail.gmail.com> (raw)
In-Reply-To: <20200131135009.31477-12-manivannan.sadhasivam@linaro.org>

On Fri, Jan 31, 2020 at 2:51 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:

> @@ -648,6 +715,31 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
>                 mhi_chan->db_cfg.pollcfg = ch_cfg->pollcfg;
>                 mhi_chan->xfer_type = ch_cfg->data_type;
>
> +               switch (mhi_chan->xfer_type) {
> +               case MHI_BUF_RAW:
> +                       mhi_chan->gen_tre = mhi_gen_tre;
> +                       mhi_chan->queue_xfer = mhi_queue_buf;
> +                       break;
> +               case MHI_BUF_SKB:
> +                       mhi_chan->queue_xfer = mhi_queue_skb;
> +                       break;
> +               case MHI_BUF_SCLIST:
> +                       mhi_chan->gen_tre = mhi_gen_tre;
> +                       mhi_chan->queue_xfer = mhi_queue_sclist;
> +                       break;
> +               case MHI_BUF_NOP:
> +                       mhi_chan->queue_xfer = mhi_queue_nop;
> +                       break;
> +               case MHI_BUF_DMA:
> +               case MHI_BUF_RSC_DMA:
> +                       mhi_chan->queue_xfer = mhi_queue_dma;
> +                       break;
> +               default:
> +                       dev_err(mhi_cntrl->dev,
> +                               "Channel datatype not supported\n");
> +                       goto error_chan_cfg;
> +               }
> +

While looking through the driver to see how the DMA gets handled, I came
across the multitude of mhi_queue_* functions, which seems like a
layering violation to me, given that they are all implemented by the
core code as well, and the client driver needs to be aware of
which one to call. Are you able to lift these out of the common interface
and make the client driver call these directly, or maybe provide a direct
interface based on mhi_buf_info to replace these?

      Arnd