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

From: Krzysztof Kozlowski
Date: Tue Mar 25 2025 - 06:40:33 EST


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.

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