Re: [PATCH v2 2/2] mailbox: add Cixtech mailbox driver

[Guomin chen <guomin.chen@cixtech.com>]

On Tue, Mar 25, 2025 at 11:39:34AM +0100, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On 25/03/2025 11:18, Guomin Chen wrote:
> > +
> > +static int cix_mbox_startup(struct mbox_chan *chan)
> > +{
> > +     struct cix_mbox_priv *priv = to_cix_mbox_priv(chan->mbox);
> > +     struct cix_mbox_con_priv *cp = chan->con_priv;
> > +     int ret;
> > +     int index = cp->index;
> > +     u32 val_32;
> > +
> > +     ret = request_irq(priv->irq, cix_mbox_isr, 0,
> > +                       dev_name(priv->dev), chan);
> > +     if (ret) {
> > +             dev_err(priv->dev,
> > +                     "Unable to acquire IRQ %d\n",
> > +                     priv->irq);
> 
> Odd wrapping. All over the code. See coding style. Please follow it
> precisely.
hi Krzysztof
Thank you. I will carefully review the coding style issues you mentioned and correct them in the next version.

Best regards
Guomin Chen
> 
> > +             return ret;
> > +     }
> > +
> > +     dev_info(priv->dev, "%s, irq %d, dir %d, type %d, index %d\n",
> > +              __func__, priv->irq, priv->dir, cp->type, cp->index);
> 
> Drop or dev_dbg.
> 
> > +
> > +     switch (cp->type) {
> > +     case CIX_MBOX_TYPE_DB:
> > +             /* Overwrite txdone_method for DB channel */
> > +             chan->txdone_method = TXDONE_BY_ACK;
> > +             fallthrough;
> > +     case CIX_MBOX_TYPE_REG:
> > +             if (priv->dir == MBOX_TX) {
> > +                     /* Enable ACK interrupt */
> > +                     val_32 = cix_mbox_read(priv, INT_ENABLE);
> > +                     val_32 |= ACK_INT;
> > +                     cix_mbox_write(priv, val_32, INT_ENABLE);
> > +             } else {
> > +                     /* Enable Doorbell interrupt */
> > +                     val_32 = cix_mbox_read(priv, INT_ENABLE_SIDE_B);
> > +                     val_32 |= DB_INT;
> > +                     cix_mbox_write(priv, val_32, INT_ENABLE_SIDE_B);
> > +             }
> > +             break;
> > +     case CIX_MBOX_TYPE_FIFO:
> > +             /* reset fifo */
> > +             cix_mbox_write(priv, FIFO_RST_BIT, FIFO_RST);
> > +             /* set default watermark */
> > +             cix_mbox_write(priv, FIFO_WM_DEFAULT, FIFO_WM);
> > +             if (priv->dir == MBOX_TX) {
> > +                     /* Enable fifo overflow interrupt */
> > +                     val_32 = cix_mbox_read(priv, INT_ENABLE);
> > +                     val_32 |= FIFO_OFLOW_INT;
> > +                     cix_mbox_write(priv, val_32, INT_ENABLE);
> > +             } else {
> > +                     /* Enable fifo full/underflow interrupt */
> > +                     val_32 = cix_mbox_read(priv, INT_ENABLE_SIDE_B);
> > +                     val_32 |= FIFO_UFLOW_INT|FIFO_WM01_INT;
> > +                     cix_mbox_write(priv, val_32, INT_ENABLE_SIDE_B);
> > +             }
> > +             break;
> > +     case CIX_MBOX_TYPE_FAST:
> 
> ...
> 
> > +
> > +static int cix_mbox_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct cix_mbox_priv *priv;
> > +     int ret;
> > +     u32 dir;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->dev = dev;
> > +     priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(priv->base))
> > +             return PTR_ERR(priv->base);
> > +
> > +     priv->irq = platform_get_irq(pdev, 0);
> > +     if (priv->irq < 0)
> > +             return priv->irq;
> > +
> > +     if (device_property_read_u32(dev, "cix,mbox-dir", &dir)) {
> > +             dev_err(priv->dev, "cix,mbox_dir property not found\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if ((dir != MBOX_TX)
> > +         && (dir != MBOX_RX)) {
> 
> Odd style. Please follow Linux kernel coding style. There is no wrapping
> after 20 characters.
> 
> > +             dev_err(priv->dev, "Dir value is not expected! dir %d\n", dir);
> > +             return -EINVAL;
> > +     }
> > +
> > +     cix_mbox_init(priv);
> > +
> > +     priv->dir = (int)dir;
> > +     priv->mbox.dev = dev;
> > +     priv->mbox.ops = &cix_mbox_chan_ops;
> > +     priv->mbox.chans = priv->mbox_chans;
> > +     priv->mbox.txdone_irq = true;
> > +     priv->mbox.num_chans = CIX_MBOX_CHANS;
> > +     priv->mbox.of_xlate = NULL;
> > +     dev_info(priv->dev, "%s, irq %d, dir %d\n",
> > +              __func__, priv->irq, priv->dir);
> 
> Drop, your driver is supposed to be silent on success.
> 
> > +
> > +     platform_set_drvdata(pdev, priv);
> > +     ret = devm_mbox_controller_register(dev, &priv->mbox);
> > +     if (ret)
> > +             dev_err(dev, "Failed to register mailbox %d\n", ret);
> > +
> Best regards,
> Krzysztof