RE: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

[Viorel Suman (OSS)]

> On Fri, Sep 18, 2020 at 03:02:39PM +0000, Viorel Suman (OSS) wrote:
> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much easier
> to read and reply to.
> 

My bad, will do.

> > > > +	regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr);
> > > > +	regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr);
> 
> > > This will unconditionally clear any interrupts, even those we don't
> > > understand - it might be better to only clear bits that are
> > > supported so the IRQ core can complain if there's something unexpected
> showing up.
> 
> > The ARM core registers itself in "fsl_xcvr_prepare" (the code below) just for a
> subset of all supported interrupts:
> > =====
> > 	ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_IER0,
> > 				 FSL_XCVR_IRQ_EARC_ALL,
> FSL_XCVR_IRQ_EARC_ALL); =====
> > FSL_XCVR_IRQ_EARC_ALL - this mask represents all the interrupts we are
> > interested in and we handle in interrupt handler, But this is just a subset of all
> interrupts the M0+ core is able to assert. Not very intuitive, I think I need to
> reword it somehow.
> 
> That's not the issue, the issue is that if we get into the ISR we just ack all the bits
> that are flagged by the hardware regardless of if we actually handled them.  This
> won't work if there are ever systems that share the interrupt and it works
> against safety/debugging features that the interrupt has in case something goes
> wrong and we get spurious interrupts.
> 

Thank you for explanation, will fix it in next version.

> > > > +	if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR)
> > > > +		dev_dbg(dev, "RX/TX FIFO full/empty\n");
> 
> > > Should this be dev_err()?
> 
> > The interrupt may be asserted right before DMA starts to fill the TX FIFO if I
> recall correctly.
> > I've added it just to debug the IP behavior, will check and change it to err it in
> next version if it is the case.
> 
> If it does come up normally then a comment or something to explain why this
> happens normally would probably be good.

Sure, ok.