Hi Vladimir, thank you for review. Jassi, Dong, any other comments? If no, i'll send tomorrow updated version. On 02.08.2018 09:54, Vladimir Zapolskiy wrote: > Hi Oleksij, > > two more nitpickings fro my side. > > On 08/02/2018 10:23 AM, Oleksij Rempel wrote: >> The i.MX Messaging Unit is a two side block which allows applications >> implement communication over this sides. >> >> The MU includes the following features: >> - Messaging control by interrupts or by polling >> - Four general-purpose interrupt requests reflected to the other side >> - Three general-purpose flags reflected to the other side >> - Four receive registers with maskable interrupt >> - Four transmit registers with maskable interrupt >> >> Reviewed-by: Vladimir Zapolskiy <vz@xxxxxxxxx> >> Reviewed-by: Dong Aisheng <aisheng.dong@xxxxxxx> >> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> > > [snip] > >> +static irqreturn_t imx_mu_isr(int irq, void *p) >> +{ >> + struct mbox_chan *chan = p; >> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); >> + struct imx_mu_con_priv *cp = chan->con_priv; >> + u32 val, ctrl, dat; >> + >> + ctrl = imx_mu_read(priv, IMX_MU_xCR); >> + val = imx_mu_read(priv, IMX_MU_xSR); >> + >> + switch (cp->type) { >> + case IMX_MU_TYPE_TX: >> + val &= IMX_MU_xSR_TEn(cp->idx) & >> + (ctrl & IMX_MU_xCR_TIEn(cp->idx)); >> + break; >> + case IMX_MU_TYPE_RX: >> + val &= IMX_MU_xSR_RFn(cp->idx) & >> + (ctrl & IMX_MU_xCR_RIEn(cp->idx)); >> + break; >> + case IMX_MU_TYPE_RXDB: >> + val &= IMX_MU_xSR_GIPn(cp->idx) & >> + (ctrl & IMX_MU_xCR_GIEn(cp->idx)); >> + break; >> + default: >> + break; >> + } >> + >> + if (!val) { >> + return IRQ_NONE; >> + } else if (val == IMX_MU_xSR_TEn(cp->idx)) { > > Please drop the 'else' branch above. > > Here you can either just start from a new 'if', or all together drop 'if (!val)' > and return IRQ_NONE at the end of the if-else construction. > >> + imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx)); >> + mbox_chan_txdone(chan, 0); >> + } else if (val == IMX_MU_xSR_RFn(cp->idx)) { >> + dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx)); >> + mbox_chan_received_data(chan, (void *)&dat); >> + } else if (val == IMX_MU_xSR_GIPn(cp->idx)) { >> + imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), IMX_MU_xSR); >> + mbox_chan_received_data(chan, NULL); >> + } else { >> + dev_warn_ratelimited(priv->dev, "Not handled interrupt\n"); >> + return IRQ_NONE; >> + } >> + >> + return IRQ_HANDLED; >> +} > > [snip] > >> + for (i = 0; i < IMX_MU_CHANS; i++) { >> + struct imx_mu_con_priv *cp = &priv->con_priv[i]; >> + >> + cp->idx = i % 4; >> + cp->type = (i - cp->idx) >> 2; > > cp->type = i >> 2; > >> + cp->chan = &priv->mbox_chans[i]; >> + priv->mbox_chans[i].con_priv = cp; >> + snprintf(cp->irq_desc, sizeof(cp->irq_desc), >> + "imx_mu_chan[%i-%i]", cp->type, cp->idx); >> + } >> + >> + priv->side_b = of_property_read_bool(np, "fsl,mu-side-b"); >> + >> + spin_lock_init(&priv->xcr_lock); >> + >> + priv->mbox.dev = dev; >> + priv->mbox.ops = &imx_mu_ops; >> + priv->mbox.chans = priv->mbox_chans; >> + priv->mbox.num_chans = IMX_MU_CHANS; >> + priv->mbox.of_xlate = imx_mu_xlate; >> + priv->mbox.txdone_irq = true; >> + >> + platform_set_drvdata(pdev, priv); >> + >> + imx_mu_init_generic(priv); >> + >> + return mbox_controller_register(&priv->mbox); >> +} > > -- > Best wishes, > Vladimir >
Attachment:
signature.asc
Description: OpenPGP digital signature