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

[Krzysztof Kozlowski <krzk@kernel.org>]

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