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

[Jassi Brar <jassisinghbrar@gmail.com>]

On Wed, Jul 27, 2011 at 10:47 AM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Jassi Brar wrote:
>> Sent: Wednesday, July 27, 2011 10:34 AM
>> To: Boojin Kim
>> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-
>> soc@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@samsung.com>
>> wrote:
>> > Jassi Brar Wrote:
>> >> Sent: Monday, July 25, 2011 8:52 PM
>> >> To: Boojin Kim
>> >> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-
>> >> soc@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@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.