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

[Viorel Suman (OSS)]

Hi Mark,

Thank you for your review.
 
> On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
> > +static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr) {
> > +	struct device *dev = &xcvr->pdev->dev;
> > +	const struct firmware *fw;
> > +	int ret = 0, rem, off, out, page = 0, size = FSL_XCVR_REG_OFFSET;
> > +	u32 mask, val;
> > +
> > +	ret = request_firmware(&fw, xcvr->fw_name, dev);
> > +	if (ret) {
> > +		dev_err(dev, "failed to request firmware.\n");
> > +		return ret;
> > +	}
> > +
> > +	rem = fw->size;
> 
> It would be good to see some explicit validation of the image size, at least
> printing an error message if the image is bigger than can be loaded.  The code
> should be safe in that it won't overflow the device region it's writing to but it
> feels like it'd be better to tell people if we spot a problem rather than just silently
> truncating the file.

Make sense, will improve this part in the next version.

> > +static irqreturn_t irq0_isr(int irq, void *devid) {
> > +	struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> > +	struct device *dev = &xcvr->pdev->dev;
> > +	struct regmap *regmap = xcvr->regmap;
> > +	void __iomem *reg_ctrl, *reg_buff;
> > +	u32 isr, val, i;
> > +
> > +	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.

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

> > +static irqreturn_t irq1_isr(int irq, void *devid) {
> > +	struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> > +	struct device *dev = &xcvr->pdev->dev;
> > +
> > +	dev_dbg(dev, "irq[1]: %d\n", irq);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> Is there any value in even requesting this and irq2 given the lack of meaningful
> handling?

No, will remove it in v2.

Thank you,
Viorel