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
