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