[Sound-open-firmware] [PATCH_V4] cnl: dma: refine dma interrupt processing on cnl

[Liam Girdwood]

On Tue, 2018-03-13 at 20:06 +0800, rander.wang wrote:
> Hi Liam,
> 
> On 3/13/2018 5:11 PM, Liam Girdwood wrote:
> > On Tue, 2018-03-13 at 13:58 +0800, Rander Wang wrote:
> > > On cnl, all the dma share the same interrupt pin, so
> > > all the dma controller need to be checked in interrupt
> > > function.
> > > 
> > > ---
> > > V4: refine code style
> > > 
> > > Signed-off-by: Rander Wang <rander.wang at linux.intel.com>
> > > ---
> > >   src/drivers/dw-dma.c | 29 ++++++++++++++++++++++++++++-
> > >   1 file changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/drivers/dw-dma.c b/src/drivers/dw-dma.c
> > > index 7659bec..690dafc 100644
> > > --- a/src/drivers/dw-dma.c
> > > +++ b/src/drivers/dw-dma.c
> > > @@ -1032,7 +1032,7 @@ static int dw_dma_probe(struct dma *dma)
> > >   #else
> > >   
> > >   /* this will probably be called at the end of every period copied */
> > > -static void dw_dma_irq_handler(void *data)
> > > +static void dma_irq_handler(void *data)
> > >   {
> > >   	struct dma *dma = (struct dma *)data;
> > >   	struct dma_pdata *p = dma_get_drvdata(dma);
> > > @@ -1138,6 +1138,33 @@ static void dw_dma_irq_handler(void *data)
> > >   	}
> > >   }
> > >   
> > > +#if defined CONFIG_CANNONLAKE
> > > +/*All the dma share the same interrupt on CNL, so check each dma*/
> > > +/*controller when interrupt coming, then do the work            */
> > > +static void dw_dma_irq_handler(void *data)
> > > +{
> > > +	struct dma *dma;
> > > +	uint32_t status_intr;
> > > +
> > > +	/*check interrupt status of DMA controller 0*/
> > > +	dma = dma_get(DMA_GP_LP_DMAC0);
> > > +	status_intr = dw_read(dma, DW_INTR_STATUS);
> > > +	if (status_intr)
> > > +		dma_irq_handler(dma);
> > > +
> > > +	/*check interrupt status of DMA controller 1*/
> > > +	dma = dma_get(DMA_GP_LP_DMAC1);
> > > +	status_intr = dw_read(dma, DW_INTR_STATUS);
> > > +	if (status_intr)
> > > +		dma_irq_handler(dma);
> > > +}
> > > +#else
> > > +static void dw_dma_irq_handler(void *data)
> > > +{
> > 
> > Why do we have this wrapper ? If both DMA IRQ handlers have the same name we
> > can
> > register them once and wont need a wrapper.
> > 
> > Liam
> > 
> 
>    you mean to check the platform in the dw_dma_irq_handler ? for marco 
> definition DMA_GP_LP_DMAC0
> 
> is only available on CNL & APL, still need ifdef.
> 

Ok, I've had a look at the code with this applied and see the problem. V3 is a
better fit atm and I think we need to clean this up for 1.2. There is a lot of
code duplication between APL, CNL and generic in probe and IRQ handlers in this
driver.

Liam

> Rander
> 
> > 
> > 
> > > +	dma_irq_handler(data);
> > > +}
> > > +#endif
> > > +
> > >   static int dw_dma_probe(struct dma *dma)
> > >   {
> > >   	struct dma_pdata *dw_pdata;
> 
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware