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

From: Viorel Suman (OSS)
Date: Fri Sep 18 2020 - 11:34:05 EST


> 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.