Jassi Brar wrote:

Sent: Wednesday, July 27, 2011 4:58 PM
To: Boojin Kim
Cc: Kukjin Kim; Vinod Koul; Mark Brown; Grant Likely; linux-samsung-
soc at vger.kernel.org; Dan Williams; linux-arm-
kernel at lists.infradead.org
Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations

On Wed, Jul 27, 2011 at 10:47 AM, Boojin Kim [off-list ref]
wrote:

quoted

Jassi Brar wrote:

quoted

Sent: Wednesday, July 27, 2011 10:34 AM
To: Boojin Kim
Cc: linux-arm-kernel at lists.infradead.org; linux-samsung-
soc at vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
Likely; Mark Brown
Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA

operations

quoted

quoted

On Tue, Jul 26, 2011 at 3:05 PM, Boojin Kim

[off-list ref]

quoted

quoted

wrote:

quoted

Jassi Brar Wrote:

quoted

Sent: Monday, July 25, 2011 8:52 PM
To: Boojin Kim
Cc: linux-arm-kernel at lists.infradead.org; linux-samsung-
soc at vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
Likely; Mark Brown
Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA

operations

quoted

quoted

On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim

[off-list ref]

quoted

quoted

wrote:

quoted

+
+static bool pl330_filter(struct dma_chan *chan, void *param)
+{
+       struct dma_pl330_peri *peri = (struct dma_pl330_peri

*)chan-

quoted

quoted

quoted

private;
+       unsigned dma_ch = (unsigned)param;
+
+       if (peri->peri_id != dma_ch)
+               return false;
+
+       return true;
+}

This is what I meant... if we keep chan_id for paltform assigned

IDs,

quoted

quoted

these filter functions could simply become

static bool pl330_filter(struct dma_chan *chan, void *param)
{
        return chan->chan_id == param
}

And ideally in the long run, we could just drop the filter

callback

quoted

quoted

quoted

quoted

and add expected channel ID to the request_channel call.

The chan_id is set by dmaengine. So, We don't use it to hold the

user specific

quoted

Id.

Not for long.
See  https://lkml.org/lkml/2011/7/26/245

quoted

quoted

quoted

+
+static inline int s3c_dma_trigger(unsigned ch)
+{
+       return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
+}
+
+static inline int s3c_dma_started(unsigned ch)
+{
+       return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
+}
+
+static inline int s3c_dma_flush(unsigned ch)
+{
+       return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
+}
+
+static inline int s3c_dma_stop(unsigned ch)
+{
+       return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
+}
+
+static struct samsung_dma_ops s3c_dma_ops = {
+       .request        = s3c_dma_request,
+       .release        = s3c_dma_release,
+       .prepare        = s3c_dma_prepare,
+       .trigger        = s3c_dma_trigger,
+       .started        = s3c_dma_started,
+       .flush          = s3c_dma_flush,
+       .stop           = s3c_dma_stop,

These last 4 should be gnereallized into one callback with OP

argument.

quoted

I don't have any idea about it. Can you explain it in more detail?

static int s3c_dma_control(unsigned ch, enum s3c2410_chan_op/*or
similar*/ op)
 {
        return s3c2410_dma_ctrl(ch, op);
 }

static struct samsung_dma_ops s3c_dma_ops = {
        .request        = s3c_dma_request,
        .release        = s3c_dma_release,
        .prepare        = s3c_dma_prepare,
        .control        = s3c_dma_control,

'Struct samsung_dma_ops' is used for both 'S3C-DMA-OPS' for 's3c

dma' and

quoted

'DMA-OPS' for 'dmaengine'.
If I modify "Struct samsung_dma_ops" as your guide, It gives un-

expectable

quoted

effect to DMA-OPS.
Actually, We don't want the client with 'DMA-OPS' uses 'enum

s3c2410_chan_op'

quoted

operation anymore.

" enum s3c2410_chan_op /* or similar */ "  <<<<  note 'or similar'

Defining a callback for each value of the argument doesn't make sense.

Yes, The command may not be 'enum s3c2410_chan_op'. But it's very similar with 
'enum s3c2410_chan_op'.
So, It brings same result that I sad.
Actually, I quietly agree with your comment. If the 'struct samsung_dma_ops' 
is only used for 's3c-dma-ops', I would address your comment. But, 'struct 
samsung_dma_ops' is used for all Samsung dma clients. I aim that 'struct 
samsung_dma_ops' provide the DMA clients with 'dmaengine' friendly DMA 
interface without any other command.