Re: [PATCH 5/7] dw_dmac: autoconfigure data_width or get it via platform data

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, 2012-09-18 at 12:41 +0530, viresh kumar wrote: 
> On Mon, Sep 17, 2012 at 1:09 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > @@ -647,7 +657,11 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
> >                 return NULL;
> >         }
> >
> > -       src_width = dst_width = dwc_fast_fls(src | dest | len);
> > +       src_width = dst_width = min_t(unsigned int,
> > +                                     /* For memory-to-memory transfers we
> > +                                      * always use AHB master 1 */
> 
> Sorry couldn't understand your comment. :(
> We use master 0 for src and master 1 for dst in memcpy.
Yeah, it might require to rewrite the logic a bit to depend on proper
AHB master properties.

I will fix it.

> > @@ -747,7 +764,9 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> >                         mem = sg_dma_address(sg);
> >                         len = sg_dma_len(sg);
> >
> > -                       mem_width = dwc_fast_fls(mem | len);
> > +                       mem_width = min_t(unsigned int,
> > +                                         data_width,
> > +                                         dwc_fast_fls(mem | len));
> 
> can above three be merged into a single line or two lines?
I think so, if there no objections from checkpatch.pl side.

> > @@ -807,7 +828,9 @@ slave_sg_todev_fill_desc:
> >                         mem = sg_dma_address(sg);
> >                         len = sg_dma_len(sg);
> >
> > -                       mem_width = dwc_fast_fls(mem | len);
> > +                       mem_width = min_t(unsigned int,
> > +                                         data_width,
> > +                                         dwc_fast_fls(mem | len));
> 
> ditto
See above.

> > @@ -1413,6 +1436,17 @@ static int __devinit dw_probe(struct platform_device *pdev)
> >
> >         /* get hardware configuration parameters */
> >         max_blk_size = dma_readl(dw, MAX_BLK_SIZE);
> > +       if (autocfg)
> > +               dw->nr_masters = (dw_params >> DW_PARAMS_NR_MASTER & 3) + 1;
> > +       else
> > +               dw->nr_masters = pdata->nr_masters;
> > +       for (i = 0; i < dw->nr_masters; i++) {
> > +               if (autocfg)
> > +                       dw->data_width[i] =
> > +                               (dw_params >> DW_PARAMS_DATA_WIDTH(i) & 3) + 2;
> > +               else
> > +                       dw->data_width[i] = pdata->data_width[i];
> > +       }
> 
> There are many autocfg, if, else statements now in probe. Can you try
> to group these
> into as minimum if, else calls?
Ok, will try to optimize it.

> > --- a/drivers/dma/dw_dmac_regs.h
> > +++ b/drivers/dma/dw_dmac_regs.h
> > @@ -106,6 +106,12 @@ struct dw_dma_regs {
> >
> >  /* Bitfields in DW_PARAMS */
> >  #define DW_PARAMS_NR_CHAN      8               /* number of channels */
> > +#define DW_PARAMS_NR_MASTER    11              /* number of AHB masters */
> > +#define DW_PARAMS_DATA_WIDTH(n)        (15 + 2 * (n))
> > +#define DW_PARAMS_DATA_WIDTH1  15              /* master 1 data width */
> > +#define DW_PARAMS_DATA_WIDTH2  17              /* master 2 data width */
> > +#define DW_PARAMS_DATA_WIDTH3  19              /* master 3 data width */
> > +#define DW_PARAMS_DATA_WIDTH4  21              /* master 4 data width */
> >  #define DW_PARAMS_EN           28              /* encoded parameters */

> Do these changes in a single patch 2/7
>From this comment I guess you would like to have dw_dmac_regs.h changes
at one place. Am I right?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy