[PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations

[Boojin Kim]

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 <boojin.kim at samsung.com>
> wrote:
> > Jassi Brar wrote:
> >> 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
> >>
> >> On Tue, Jul 26, 2011 at 3:05 PM, Boojin Kim
> <boojin.kim at samsung.com>
> >> wrote:
> >> > Jassi Brar Wrote:
> >> >> 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
> >> >>
> >> >> On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim
> >> <boojin.kim at samsung.com>
> >> >> wrote:
> >> >>
> >> >> > +
> >> >> > +static bool pl330_filter(struct dma_chan *chan, void *param)
> >> >> > +{
> >> >> > +       struct dma_pl330_peri *peri = (struct dma_pl330_peri
> >> *)chan-
> >> >> >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,
> >> >> 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
> >> >> 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
> >> > Id.
> >> Not for long.
> >> See  https://lkml.org/lkml/2011/7/26/245
> >>
> >>
> >> >> > +
> >> >> > +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.
> >> > 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
> > 'DMA-OPS' for 'dmaengine'.
> > If I modify "Struct samsung_dma_ops" as your guide, It gives un-
> expectable
> > effect to DMA-OPS.
> > Actually, We don't want the client with 'DMA-OPS' uses 'enum
> s3c2410_chan_op'
> > 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.