Hi Arnd,
On Mon, Feb 17, 2020 at 05:13:37PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 31, 2020 at 2:51 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@xxxxxxxxxx> 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?
>
It sounds reasonable to me. Let me discuss this internally with Qcom guys to
see if they have any objections.
Thanks,
Mani
> Arnd
