[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <410f05ba-73e2-45fd-9d31-0c07d648cdd5@kernel.org>
Date: Tue, 25 Mar 2025 11:39:34 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Guomin Chen <guomin.chen@...tech.com>,
Jassi Brar <jassisinghbrar@...il.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
cix-kernel-upstream@...tech.com, Peter Chen <peter.chen@...tech.com>,
Lihua Liu <Lihua.Liu@...tech.com>
Subject: Re: [PATCH v2 2/2] mailbox: add Cixtech mailbox driver
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
Powered by blists - more mailing lists