[PATCH v5 0/2] add VCP mailbox driver

[PATCH v5 0/2] add VCP mailbox driver

[Jjian Zhou]

Hi everyone,

This is v5 of my VCP mailbox driver.

Changes since v5:
- bindngs:
  - Patch 1 fix 'make dt_binding_check' errors.

Changes since v4:
- binding:
  - Match the binding file name and compatible.
- mtk-vcp-mailbox.c:
  - Drop 'dev_dbg(dev, "MTK VCP mailbox initialized\n")'.
- Since the reviewer hopes to combine the VCP IPC driver and
  the VCP driver for a unified review, the original three patches
  have been split into two parts: the VCP mailbox driver and
  the binding remain together, while the VCP IPC driver is merged
  with the VCP driver and submitted as one.
- Link to v4
  https://lore.kernel.org/all/20250820094545.23821-1-jjian.zhou@mediatek.com/

Changes since v3:
- binding:
  - Remove unused lable '|' and 'vcp_mailbox0'.
- Link to v3
  https://lore.kernel.org/all/20250317110331.2776-1-jjian.zhou@mediatek.com/

Changes since v1:
- Link to v1
  https://lore.kernel.org/all/20250305082047.15746-1-jjian.zhou@mediatek.com/

In the v2 version, there is ongoing discussion about whether the VCP's
IPC should use mailbox or rpmsg. To prevent the discussion records
from being lost, the previous discussion link is attached.
https://lore.kernel.org/all/CAGXv+5FXqZb_v2dQNgCKbFpJrLhbVk3f0sWrrMCVk3jaWwoBqA@mail.gmail.com/

Jjian Zhou (2):
  dt-bindings: mailbox: mediatek,mt8196-vcp-mbox: add mtk vcp-mbox
    document
  mailbox: mediatek: Add mtk-vcp-mailbox driver

 .../mailbox/mediatek,mt8196-vcp-mbox.yaml     |  49 +++++
 drivers/mailbox/Kconfig                       |   9 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/mtk-vcp-mailbox.c             | 174 ++++++++++++++++++
 include/linux/mailbox/mtk-vcp-mailbox.h       |  32 ++++
 5 files changed, 266 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml
 create mode 100644 drivers/mailbox/mtk-vcp-mailbox.c
 create mode 100644 include/linux/mailbox/mtk-vcp-mailbox.h

--
2.46.0

[PATCH v5 1/2] dt-bindings: mailbox: mediatek,mt8196-vcp-mbox: add mtk vcp-mbox document

[Jjian Zhou]

The MTK VCP mailbox enables the SoC to communicate with the VCP by passing
messages through 64 32-bit wide registers. It has 32 interrupt vectors in
either direction for signalling purposes.

This adds a binding for Mediatek VCP mailbox.

Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
---
 .../mailbox/mediatek,mt8196-vcp-mbox.yaml     | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml
new file mode 100644
index 000000000000..7b1c5165e64e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/mediatek,mt8196-vcp-mbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek Video Companion Processor (VCP) mailbox
+
+maintainers:
+  - Jjian Zhou <Jjian.Zhou@mediatek.com>
+
+description:
+  The MTK VCP mailbox enables the SoC to communicate with the VCP by passing
+  messages through 64 32-bit wide registers. It has 32 interrupt vectors in
+  either direction for signalling purposes.
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt8196-vcp-mbox
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#mbox-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    mailbox@31b80000 {
+        compatible = "mediatek,mt8196-vcp-mbox";
+        reg = <0x31b80000 0x1000>;
+        interrupts = <GIC_SPI 789 IRQ_TYPE_LEVEL_HIGH 0>;
+        #mbox-cells = <0>;
+    };
-- 
2.46.0

[PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou]

Add mtk-vcp-mailbox driver to support the communication with
VCP remote microprocessor.

Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
---
 drivers/mailbox/Kconfig                 |   9 ++
 drivers/mailbox/Makefile                |   2 +
 drivers/mailbox/mtk-vcp-mailbox.c       | 174 ++++++++++++++++++++++++
 include/linux/mailbox/mtk-vcp-mailbox.h |  32 +++++
 4 files changed, 217 insertions(+)
 create mode 100644 drivers/mailbox/mtk-vcp-mailbox.c
 create mode 100644 include/linux/mailbox/mtk-vcp-mailbox.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 02432d4a5ccd..c28bdb855663 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -294,6 +294,15 @@ config MTK_CMDQ_MBOX
 	  critical time limitation, such as updating display configuration
 	  during the vblank.

+config MTK_VCP_MBOX
+	tristate "MediaTek VCP Mailbox Support"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	help
+	  Say yes here to add support for the MediaTek VCP mailbox driver.
+	  The mailbox implementation provides access from the application
+	  processor to Video Companion Processor Unit.
+	  If unsure say N.
+
 config ZYNQMP_IPI_MBOX
 	tristate "Xilinx ZynqMP IPI Mailbox"
 	depends on ARCH_ZYNQMP && OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 98a68f838486..07278871d254 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -63,6 +63,8 @@ obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o

 obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o

+obj-$(CONFIG_MTK_VCP_MBOX)	+= mtk-vcp-mailbox.o
+
 obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o

 obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
diff --git a/drivers/mailbox/mtk-vcp-mailbox.c b/drivers/mailbox/mtk-vcp-mailbox.c
new file mode 100644
index 000000000000..6f48215896d2
--- /dev/null
+++ b/drivers/mailbox/mtk-vcp-mailbox.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 MediaTek Corporation. All rights reserved.
+ * Author: Jjian Zhou <jjian.zhou.@mediatek.com>
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/mtk-vcp-mailbox.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct mtk_vcp_mbox_priv {
+	void __iomem *base;
+	struct device *dev;
+	struct mbox_controller mbox;
+	const struct mtk_vcp_mbox_cfg *cfg;
+	struct mtk_ipi_info ipi_recv;
+};
+
+struct mtk_vcp_mbox_cfg {
+	u32 set_in;
+	u32 clr_out;
+};
+
+static inline struct mtk_vcp_mbox_priv *get_mtk_vcp_mbox_priv(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct mtk_vcp_mbox_priv, mbox);
+}
+
+static irqreturn_t mtk_vcp_mbox_irq_thread(int irq, void *data)
+{
+	struct mbox_chan *chan = data;
+	struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);
+
+	/* get irq status */
+	priv->ipi_recv.irq_status = readl(priv->base + priv->cfg->clr_out);
+
+	__ioread32_copy(priv->ipi_recv.msg, priv->base, MBOX_SLOT_MAX_SIZE / 4);
+
+	mbox_chan_received_data(chan, &priv->ipi_recv);
+
+	/* clear irq status */
+	writel(priv->ipi_recv.irq_status, priv->base + priv->cfg->clr_out);
+
+	return IRQ_HANDLED;
+}
+
+static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *sp)
+{
+	if (sp->args_count)
+		return NULL;
+
+	return mbox->chans;
+}
+
+static int mtk_vcp_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);
+	struct mtk_ipi_info *ipi_info = data;
+	u32 status;
+
+	if (!ipi_info->msg) {
+		dev_err(priv->dev, "msg buffer is NULL.\n");
+		return -EINVAL;
+	}
+
+	status = readl(priv->base + priv->cfg->set_in) & BIT(ipi_info->index);
+	if (status) {
+		dev_warn(priv->dev, "mailbox IPI %d is busy.\n", ipi_info->id);
+		return -EBUSY;
+	}
+
+	if (ipi_info->slot_ofs + ipi_info->len > MBOX_SLOT_MAX_SIZE)
+		return -EINVAL;
+	__iowrite32_copy(priv->base + ipi_info->slot_ofs, ipi_info->msg,
+			 ipi_info->len);
+
+	writel(BIT(ipi_info->index), priv->base + priv->cfg->set_in);
+
+	return 0;
+}
+
+static bool mtk_vcp_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct mtk_ipi_info *ipi_info = chan->active_req;
+	struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);
+
+	return !(readl(priv->base + priv->cfg->set_in) & BIT(ipi_info->index));
+}
+
+static const struct mbox_chan_ops mtk_vcp_mbox_chan_ops = {
+	.send_data	= mtk_vcp_mbox_send_data,
+	.last_tx_done	= mtk_vcp_mbox_last_tx_done,
+};
+
+static int mtk_vcp_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_vcp_mbox_priv *priv;
+	struct mbox_controller *mbox;
+	int ret, irq;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	mbox = &priv->mbox;
+	mbox->dev = dev;
+	mbox->ops = &mtk_vcp_mbox_chan_ops;
+	mbox->txdone_irq = false;
+	mbox->txdone_poll = true;
+	mbox->of_xlate = mtk_vcp_mbox_xlate;
+	mbox->num_chans = 1;
+	mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans), GFP_KERNEL);
+	if (!mbox->chans)
+		return -ENOMEM;
+
+	priv->ipi_recv.msg = devm_kzalloc(dev, MBOX_SLOT_MAX_SIZE, GFP_KERNEL);
+	if (!priv->ipi_recv.msg)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->cfg = of_device_get_match_data(dev);
+	if (!priv->cfg)
+		return -EINVAL;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_threaded_irq(dev, irq, NULL,
+					mtk_vcp_mbox_irq_thread, IRQF_ONESHOT,
+					dev_name(dev), mbox->chans);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
+
+	return devm_mbox_controller_register(dev, &priv->mbox);
+}
+
+static const struct mtk_vcp_mbox_cfg mt8196_cfg = {
+	.set_in		= 0x100,
+	.clr_out	= 0x10C,
+};
+
+static const struct of_device_id mtk_vcp_mbox_of_match[] = {
+	{ .compatible = "mediatek,mt8196-vcp-mbox", .data = &mt8196_cfg },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_vcp_mbox_of_match);
+
+static struct platform_driver mtk_vcp_mbox_driver = {
+	.probe		= mtk_vcp_mbox_probe,
+	.driver = {
+		.name	= "mtk_vcp_mbox",
+		.of_match_table = mtk_vcp_mbox_of_match,
+	},
+};
+module_platform_driver(mtk_vcp_mbox_driver);
+
+MODULE_AUTHOR("Jjian Zhou <jjian.zhou@mediatek.com>");
+MODULE_DESCRIPTION("MTK VCP Mailbox Controller");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mailbox/mtk-vcp-mailbox.h b/include/linux/mailbox/mtk-vcp-mailbox.h
new file mode 100644
index 000000000000..143fb0d06e30
--- /dev/null
+++ b/include/linux/mailbox/mtk-vcp-mailbox.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Copyright (c) 2025 MediaTek Inc.
+ */
+
+#ifndef __MTK_VCP_MAILBOX_H__
+#define __MTK_VCP_MAILBOX_H__
+
+#define MBOX_SLOT_MAX_SIZE	0x100 /* mbox max slot size */
+
+/**
+ * struct mtk_ipi_info - mailbox message info for mtk-vcp-mailbox
+ * @msg: The share buffer between IPC and mailbox driver
+ * @len: Message length
+ * @id: This is for identification purposes and not actually used
+ *	by the mailbox hardware.
+ * @index: The signal number of the mailbox message.
+ * @slot_ofs: Data slot offset.
+ * @irq_status: Captures incoming signals for the RX path.
+ *
+ * It is used between IPC with mailbox driver.
+ */
+struct mtk_ipi_info {
+	void *msg;
+	u32 len;
+	u32 id;
+	u32 index;
+	u32 slot_ofs;
+	u32 irq_status;
+};
+
+#endif
--
2.46.0

Re: [PATCH v5 1/2] dt-bindings: mailbox: mediatek,mt8196-vcp-mbox: add mtk vcp-mbox document

[Chen-Yu Tsai]

On Fri, Aug 22, 2025 at 10:12 AM Jjian Zhou <jjian.zhou@mediatek.com> wrote:
>
> The MTK VCP mailbox enables the SoC to communicate with the VCP by passing
> messages through 64 32-bit wide registers. It has 32 interrupt vectors in
> either direction for signalling purposes.
>
> This adds a binding for Mediatek VCP mailbox.
>
> Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

> ---
>  .../mailbox/mediatek,mt8196-vcp-mbox.yaml     | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml
> new file mode 100644
> index 000000000000..7b1c5165e64e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/mediatek,mt8196-vcp-mbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Video Companion Processor (VCP) mailbox
> +
> +maintainers:
> +  - Jjian Zhou <Jjian.Zhou@mediatek.com>
> +
> +description:
> +  The MTK VCP mailbox enables the SoC to communicate with the VCP by passing
> +  messages through 64 32-bit wide registers. It has 32 interrupt vectors in
> +  either direction for signalling purposes.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8196-vcp-mbox
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#mbox-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#mbox-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    mailbox@31b80000 {
> +        compatible = "mediatek,mt8196-vcp-mbox";
> +        reg = <0x31b80000 0x1000>;
> +        interrupts = <GIC_SPI 789 IRQ_TYPE_LEVEL_HIGH 0>;
> +        #mbox-cells = <0>;
> +    };
> --
> 2.46.0
>

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Chen-Yu Tsai]

On Fri, Aug 22, 2025 at 10:12 AM Jjian Zhou <jjian.zhou@mediatek.com> wrote:
>
> Add mtk-vcp-mailbox driver to support the communication with
> VCP remote microprocessor.
>
> Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

A couple minor comments below.

> ---
>  drivers/mailbox/Kconfig                 |   9 ++
>  drivers/mailbox/Makefile                |   2 +
>  drivers/mailbox/mtk-vcp-mailbox.c       | 174 ++++++++++++++++++++++++
>  include/linux/mailbox/mtk-vcp-mailbox.h |  32 +++++
>  4 files changed, 217 insertions(+)
>  create mode 100644 drivers/mailbox/mtk-vcp-mailbox.c
>  create mode 100644 include/linux/mailbox/mtk-vcp-mailbox.h
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 02432d4a5ccd..c28bdb855663 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -294,6 +294,15 @@ config MTK_CMDQ_MBOX
>           critical time limitation, such as updating display configuration
>           during the vblank.
>
> +config MTK_VCP_MBOX
> +       tristate "MediaTek VCP Mailbox Support"
> +       depends on ARCH_MEDIATEK || COMPILE_TEST
> +       help
> +         Say yes here to add support for the MediaTek VCP mailbox driver.
> +         The mailbox implementation provides access from the application
> +         processor to Video Companion Processor Unit.
> +         If unsure say N.
> +
>  config ZYNQMP_IPI_MBOX
>         tristate "Xilinx ZynqMP IPI Mailbox"
>         depends on ARCH_ZYNQMP && OF
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 98a68f838486..07278871d254 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -63,6 +63,8 @@ obj-$(CONFIG_MTK_ADSP_MBOX)   += mtk-adsp-mailbox.o
>
>  obj-$(CONFIG_MTK_CMDQ_MBOX)    += mtk-cmdq-mailbox.o
>
> +obj-$(CONFIG_MTK_VCP_MBOX)     += mtk-vcp-mailbox.o
> +
>  obj-$(CONFIG_ZYNQMP_IPI_MBOX)  += zynqmp-ipi-mailbox.o
>
>  obj-$(CONFIG_SUN6I_MSGBOX)     += sun6i-msgbox.o
> diff --git a/drivers/mailbox/mtk-vcp-mailbox.c b/drivers/mailbox/mtk-vcp-mailbox.c
> new file mode 100644
> index 000000000000..6f48215896d2
> --- /dev/null
> +++ b/drivers/mailbox/mtk-vcp-mailbox.c
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 MediaTek Corporation. All rights reserved.
> + * Author: Jjian Zhou <jjian.zhou.@mediatek.com>
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/mtk-vcp-mailbox.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +struct mtk_vcp_mbox_priv {
> +       void __iomem *base;
> +       struct device *dev;
> +       struct mbox_controller mbox;
> +       const struct mtk_vcp_mbox_cfg *cfg;
> +       struct mtk_ipi_info ipi_recv;
> +};
> +
> +struct mtk_vcp_mbox_cfg {
> +       u32 set_in;
> +       u32 clr_out;
> +};
> +
> +static inline struct mtk_vcp_mbox_priv *get_mtk_vcp_mbox_priv(struct mbox_controller *mbox)
> +{
> +       return container_of(mbox, struct mtk_vcp_mbox_priv, mbox);
> +}
> +
> +static irqreturn_t mtk_vcp_mbox_irq_thread(int irq, void *data)
> +{
> +       struct mbox_chan *chan = data;
> +       struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);

Since there's only one mailbox, you can just pass the private data,
instead of passing the mbox_chan.

> +
> +       /* get irq status */
> +       priv->ipi_recv.irq_status = readl(priv->base + priv->cfg->clr_out);
> +
> +       __ioread32_copy(priv->ipi_recv.msg, priv->base, MBOX_SLOT_MAX_SIZE / 4);
> +
> +       mbox_chan_received_data(chan, &priv->ipi_recv);
> +
> +       /* clear irq status */
> +       writel(priv->ipi_recv.irq_status, priv->base + priv->cfg->clr_out);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller *mbox,
> +                                           const struct of_phandle_args *sp)
> +{
> +       if (sp->args_count)
> +               return NULL;
> +
> +       return mbox->chans;
> +}
> +
> +static int mtk_vcp_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);
> +       struct mtk_ipi_info *ipi_info = data;
> +       u32 status;
> +
> +       if (!ipi_info->msg) {
> +               dev_err(priv->dev, "msg buffer is NULL.\n");
> +               return -EINVAL;
> +       }
> +
> +       status = readl(priv->base + priv->cfg->set_in) & BIT(ipi_info->index);
> +       if (status) {
> +               dev_warn(priv->dev, "mailbox IPI %d is busy.\n", ipi_info->id);
> +               return -EBUSY;
> +       }
> +
> +       if (ipi_info->slot_ofs + ipi_info->len > MBOX_SLOT_MAX_SIZE)
> +               return -EINVAL;
> +       __iowrite32_copy(priv->base + ipi_info->slot_ofs, ipi_info->msg,
> +                        ipi_info->len);
> +
> +       writel(BIT(ipi_info->index), priv->base + priv->cfg->set_in);
> +
> +       return 0;
> +}
> +
> +static bool mtk_vcp_mbox_last_tx_done(struct mbox_chan *chan)
> +{
> +       struct mtk_ipi_info *ipi_info = chan->active_req;
> +       struct mtk_vcp_mbox_priv *priv = get_mtk_vcp_mbox_priv(chan->mbox);
> +
> +       return !(readl(priv->base + priv->cfg->set_in) & BIT(ipi_info->index));
> +}
> +
> +static const struct mbox_chan_ops mtk_vcp_mbox_chan_ops = {
> +       .send_data      = mtk_vcp_mbox_send_data,
> +       .last_tx_done   = mtk_vcp_mbox_last_tx_done,
> +};
> +
> +static int mtk_vcp_mbox_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct mtk_vcp_mbox_priv *priv;
> +       struct mbox_controller *mbox;
> +       int ret, irq;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->dev = dev;
> +       mbox = &priv->mbox;
> +       mbox->dev = dev;
> +       mbox->ops = &mtk_vcp_mbox_chan_ops;
> +       mbox->txdone_irq = false;
> +       mbox->txdone_poll = true;
> +       mbox->of_xlate = mtk_vcp_mbox_xlate;
> +       mbox->num_chans = 1;
> +       mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans), GFP_KERNEL);
> +       if (!mbox->chans)
> +               return -ENOMEM;
> +
> +       priv->ipi_recv.msg = devm_kzalloc(dev, MBOX_SLOT_MAX_SIZE, GFP_KERNEL);
> +       if (!priv->ipi_recv.msg)
> +               return -ENOMEM;
> +
> +       priv->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(priv->base))
> +               return PTR_ERR(priv->base);
> +
> +       priv->cfg = of_device_get_match_data(dev);
> +       if (!priv->cfg)
> +               return -EINVAL;
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0)
> +               return irq;
> +
> +       ret = devm_request_threaded_irq(dev, irq, NULL,
> +                                       mtk_vcp_mbox_irq_thread, IRQF_ONESHOT,
> +                                       dev_name(dev), mbox->chans);
> +       if (ret < 0)
> +               return ret;
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       return devm_mbox_controller_register(dev, &priv->mbox);
> +}
> +
> +static const struct mtk_vcp_mbox_cfg mt8196_cfg = {
> +       .set_in         = 0x100,
> +       .clr_out        = 0x10C,

                                ^ use lowercase for hex numbers.

> +};
> +
> +static const struct of_device_id mtk_vcp_mbox_of_match[] = {
> +       { .compatible = "mediatek,mt8196-vcp-mbox", .data = &mt8196_cfg },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_vcp_mbox_of_match);
> +
> +static struct platform_driver mtk_vcp_mbox_driver = {
> +       .probe          = mtk_vcp_mbox_probe,
> +       .driver = {
> +               .name   = "mtk_vcp_mbox",
> +               .of_match_table = mtk_vcp_mbox_of_match,
> +       },
> +};
> +module_platform_driver(mtk_vcp_mbox_driver);
> +
> +MODULE_AUTHOR("Jjian Zhou <jjian.zhou@mediatek.com>");
> +MODULE_DESCRIPTION("MTK VCP Mailbox Controller");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mailbox/mtk-vcp-mailbox.h b/include/linux/mailbox/mtk-vcp-mailbox.h
> new file mode 100644
> index 000000000000..143fb0d06e30
> --- /dev/null
> +++ b/include/linux/mailbox/mtk-vcp-mailbox.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/*
> + * Copyright (c) 2025 MediaTek Inc.
> + */
> +
> +#ifndef __MTK_VCP_MAILBOX_H__
> +#define __MTK_VCP_MAILBOX_H__
> +
> +#define MBOX_SLOT_MAX_SIZE     0x100 /* mbox max slot size */
> +
> +/**
> + * struct mtk_ipi_info - mailbox message info for mtk-vcp-mailbox
> + * @msg: The share buffer between IPC and mailbox driver
> + * @len: Message length
> + * @id: This is for identification purposes and not actually used
> + *     by the mailbox hardware.
> + * @index: The signal number of the mailbox message.
> + * @slot_ofs: Data slot offset.
> + * @irq_status: Captures incoming signals for the RX path.
> + *
> + * It is used between IPC with mailbox driver.
> + */
> +struct mtk_ipi_info {
> +       void *msg;
> +       u32 len;
> +       u32 id;
> +       u32 index;
> +       u32 slot_ofs;
> +       u32 irq_status;
> +};
> +
> +#endif
> --
> 2.46.0
>

Re: [PATCH v5 1/2] dt-bindings: mailbox: mediatek,mt8196-vcp-mbox: add mtk vcp-mbox document

[Krzysztof Kozlowski]

On Fri, Aug 22, 2025 at 10:12:08AM +0800, Jjian Zhou wrote:
> The MTK VCP mailbox enables the SoC to communicate with the VCP by passing
> messages through 64 32-bit wide registers. It has 32 interrupt vectors in
> either direction for signalling purposes.
> 
> This adds a binding for Mediatek VCP mailbox.
> 
> Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> ---
>  .../mailbox/mediatek,mt8196-vcp-mbox.yaml     | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-mbox.yaml

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v5 0/2] add VCP mailbox driver

[Jjian Zhou (周建)]

On Fri, 2025-08-22 at 10:12 +0800, Jjian Zhou wrote:

This email is just a reminder to review the patch:


https://lore.kernel.org/all/CAGXv+5GHSinkMHhBxRekfCxFK8my8JqeeRBsGXGuWDqH=MjmTQ@mail.gmail.com/

> Hi everyone,
> 
> This is v5 of my VCP mailbox driver.
> 
> Changes since v5:
> - bindngs:
>   - Patch 1 fix 'make dt_binding_check' errors.
> 
> Changes since v4:
> - binding:
>   - Match the binding file name and compatible.
> - mtk-vcp-mailbox.c:
>   - Drop 'dev_dbg(dev, "MTK VCP mailbox initialized\n")'.
> - Since the reviewer hopes to combine the VCP IPC driver and
>   the VCP driver for a unified review, the original three patches
>   have been split into two parts: the VCP mailbox driver and
>   the binding remain together, while the VCP IPC driver is merged
>   with the VCP driver and submitted as one.
> - Link to v4
>   
> https://lore.kernel.org/all/20250820094545.23821-1-jjian.zhou@mediatek.com/
> 
> Changes since v3:
> - binding:
>   - Remove unused lable '|' and 'vcp_mailbox0'.
> - Link to v3
>   
> https://lore.kernel.org/all/20250317110331.2776-1-jjian.zhou@mediatek.com/
> 
> Changes since v1:
> - Link to v1
>   
> https://lore.kernel.org/all/20250305082047.15746-1-jjian.zhou@mediatek.com/
> 
> In the v2 version, there is ongoing discussion about whether the
> VCP's
> IPC should use mailbox or rpmsg. To prevent the discussion records
> from being lost, the previous discussion link is attached.
> 
https://lore.kernel.org/all/CAGXv+5FXqZb_v2dQNgCKbFpJrLhbVk3f0sWrrMCVk3jaWwoBqA@mail.gmail.com/
> 
> Jjian Zhou (2):
>   dt-bindings: mailbox: mediatek,mt8196-vcp-mbox: add mtk vcp-mbox
>     document
>   mailbox: mediatek: Add mtk-vcp-mailbox driver
> 
>  .../mailbox/mediatek,mt8196-vcp-mbox.yaml     |  49 +++++
>  drivers/mailbox/Kconfig                       |   9 +
>  drivers/mailbox/Makefile                      |   2 +
>  drivers/mailbox/mtk-vcp-mailbox.c             | 174
> ++++++++++++++++++
>  include/linux/mailbox/mtk-vcp-mailbox.h       |  32 ++++
>  5 files changed, 266 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/mailbox/mediatek,mt8196-vcp-
> mbox.yaml
>  create mode 100644 drivers/mailbox/mtk-vcp-mailbox.c
>  create mode 100644 include/linux/mailbox/mtk-vcp-mailbox.h
> 
> --
> 2.46.0
> 

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jassi Brar]

On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com> wrote:

.....

> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +struct mtk_vcp_mbox_priv {
Maybe 'mtk_vcp_mbox' is a more appropriate name ?

> +       void __iomem *base;
> +       struct device *dev;
> +       struct mbox_controller mbox;
> +       const struct mtk_vcp_mbox_cfg *cfg;
> +       struct mtk_ipi_info ipi_recv;

Maybe also have "struct mbox_chan chan[1]; " so that you don't have to
allocate one during the probe.
Also if you have  "struct mbox_controller mbox;" as the first member,
you could simply typecast that to get this structure.
Something like "struct mpfs_mbox" in mailbox-mpfs.c

....
> +
> +static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller *mbox,
> +                                           const struct of_phandle_args *sp)
> +{
> +       if (sp->args_count)
> +               return NULL;
> +
> +       return mbox->chans;

return &mbox->chans[0]   seems better.

thnx

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Chen-Yu Tsai]

On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com> wrote:
>
> .....
>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +struct mtk_vcp_mbox_priv {
> Maybe 'mtk_vcp_mbox' is a more appropriate name ?
>
> > +       void __iomem *base;
> > +       struct device *dev;
> > +       struct mbox_controller mbox;
> > +       const struct mtk_vcp_mbox_cfg *cfg;
> > +       struct mtk_ipi_info ipi_recv;
>
> Maybe also have "struct mbox_chan chan[1]; " so that you don't have to
> allocate one during the probe.

> Also if you have  "struct mbox_controller mbox;" as the first member,
> you could simply typecast that to get this structure.
> Something like "struct mpfs_mbox" in mailbox-mpfs.c

I read somewhere that this way of subclassing is not recommended.
Instead the base class should explicitly not be the first member.
And then container_of() should be used.

I don't remember where I read this though. But I think the explicit
container_of() is easier for understanding the intent.

ChenYu

> ....
> > +
> > +static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller *mbox,
> > +                                           const struct of_phandle_args *sp)
> > +{
> > +       if (sp->args_count)
> > +               return NULL;
> > +
> > +       return mbox->chans;
>
> return &mbox->chans[0]   seems better.
>
> thnx

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Thu, 2025-09-18 at 18:50 -0500, Jassi Brar wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com>
> wrote:
> 
> .....
> 
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +struct mtk_vcp_mbox_priv {
> 
> Maybe 'mtk_vcp_mbox' is a more appropriate name ?

OK, I replace mtk_vcp_mbox_priv with mtk_vcp_mbox in the next version.

> 
> > +       void __iomem *base;
> > +       struct device *dev;
> > +       struct mbox_controller mbox;
> > +       const struct mtk_vcp_mbox_cfg *cfg;
> > +       struct mtk_ipi_info ipi_recv;
> 
> Maybe also have "struct mbox_chan chan[1]; " so that you don't have
> to
> allocate one during the probe.
> Also if you have  "struct mbox_controller mbox;" as the first member,
> you could simply typecast that to get this structure.
> Something like "struct mpfs_mbox" in mailbox-mpfs.c
> 
> ....
> > +
> > +static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller
> > *mbox,
> > +                                           const struct
> > of_phandle_args *sp)
> > +{
> > +       if (sp->args_count)
> > +               return NULL;
> > +
> > +       return mbox->chans;
> 
> return &mbox->chans[0]   seems better.

OK.

> 
> thnx

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jassi Brar]

On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com> wrote:
> >
> > .....
> >
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +
> > > +struct mtk_vcp_mbox_priv {
> > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> >
> > > +       void __iomem *base;
> > > +       struct device *dev;
> > > +       struct mbox_controller mbox;
> > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > +       struct mtk_ipi_info ipi_recv;
> >
> > Maybe also have "struct mbox_chan chan[1]; " so that you don't have to
> > allocate one during the probe.
>
> > Also if you have  "struct mbox_controller mbox;" as the first member,
> > you could simply typecast that to get this structure.
> > Something like "struct mpfs_mbox" in mailbox-mpfs.c
>
> I read somewhere that this way of subclassing is not recommended.
> Instead the base class should explicitly not be the first member.
> And then container_of() should be used.
>
> I don't remember where I read this though. But I think the explicit
> container_of() is easier for understanding the intent.
>
And how does container_of() work ? :)
typcasting the first member to its parent is the simplest form of container_of.

-j

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Nicolas Frattaroli]

On Friday, 19 September 2025 18:32:12 Central European Summer Time Jassi Brar wrote:
> On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > >
> > > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com> wrote:
> > >
> > > .....
> > >
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/slab.h>
> > > > +
> > > > +struct mtk_vcp_mbox_priv {
> > > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> > >
> > > > +       void __iomem *base;
> > > > +       struct device *dev;
> > > > +       struct mbox_controller mbox;
> > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > +       struct mtk_ipi_info ipi_recv;
> > >
> > > Maybe also have "struct mbox_chan chan[1]; " so that you don't have to
> > > allocate one during the probe.
> >
> > > Also if you have  "struct mbox_controller mbox;" as the first member,
> > > you could simply typecast that to get this structure.
> > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> >
> > I read somewhere that this way of subclassing is not recommended.
> > Instead the base class should explicitly not be the first member.
> > And then container_of() should be used.
> >
> > I don't remember where I read this though. But I think the explicit
> > container_of() is easier for understanding the intent.
> >
> And how does container_of() work ? :)
> typcasting the first member to its parent is the simplest form of container_of.
> 
> -j
> 
> 

Which is why it's completely equivalent and since code is supposed
to communicate meaning to humans, container_of should be used.

"In this case if nobody ever reorders members you can avoid a
preprocessor macro that will get optimised away" is pointless
nitpicking. It's a strictly worse way to do the same thing.

Why bikeshed the use of container_of of all things?

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jassi Brar]

On Fri, Sep 19, 2025 at 2:02 PM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> On Friday, 19 September 2025 18:32:12 Central European Summer Time Jassi Brar wrote:
> > On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> > >
> > > On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > >
> > > > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com> wrote:
> > > >
> > > > .....
> > > >
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/slab.h>
> > > > > +
> > > > > +struct mtk_vcp_mbox_priv {
> > > > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> > > >
> > > > > +       void __iomem *base;
> > > > > +       struct device *dev;
> > > > > +       struct mbox_controller mbox;
> > > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > > +       struct mtk_ipi_info ipi_recv;
> > > >
> > > > Maybe also have "struct mbox_chan chan[1]; " so that you don't have to
> > > > allocate one during the probe.
> > >
> > > > Also if you have  "struct mbox_controller mbox;" as the first member,
> > > > you could simply typecast that to get this structure.
> > > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > >
> > > I read somewhere that this way of subclassing is not recommended.
> > > Instead the base class should explicitly not be the first member.
> > > And then container_of() should be used.
> > >
> > > I don't remember where I read this though. But I think the explicit
> > > container_of() is easier for understanding the intent.
> > >
> > And how does container_of() work ? :)
> > typcasting the first member to its parent is the simplest form of container_of.
> >
> > -j
> >
> >
>
> Which is why it's completely equivalent and since code is supposed
> to communicate meaning to humans, container_of should be used.
>
Nobody is suggesting typecasting cfg, dev or anything else.
Typecasting between mailbox controllers is fine and arguably easier on
the eyes than using a container_of.

-j

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Sat, 2025-09-20 at 23:02 -0500, Jassi Brar wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Sep 19, 2025 at 2:02 PM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> > 
> > On Friday, 19 September 2025 18:32:12 Central European Summer Time
> > Jassi Brar wrote:
> > > On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <wenst@chromium.org>
> > > wrote:
> > > > 
> > > > On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <
> > > > jassisinghbrar@gmail.com> wrote:
> > > > > 
> > > > > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <
> > > > > jjian.zhou@mediatek.com> wrote:
> > > > > 
> > > > > .....
> > > > > 
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/of.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/slab.h>
> > > > > > +
> > > > > > +struct mtk_vcp_mbox_priv {
> > > > > 
> > > > > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> > > > > 
> > > > > > +       void __iomem *base;
> > > > > > +       struct device *dev;
> > > > > > +       struct mbox_controller mbox;
> > > > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > > > +       struct mtk_ipi_info ipi_recv;
> > > > > 
> > > > > Maybe also have "struct mbox_chan chan[1]; " so that you
> > > > > don't have to
> > > > > allocate one during the probe.
> > > > > Also if you have  "struct mbox_controller mbox;" as the first
> > > > > member,
> > > > > you could simply typecast that to get this structure.
> > > > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > > > 
> > > > I read somewhere that this way of subclassing is not
> > > > recommended.
> > > > Instead the base class should explicitly not be the first
> > > > member.
> > > > And then container_of() should be used.
> > > > 
> > > > I don't remember where I read this though. But I think the
> > > > explicit
> > > > container_of() is easier for understanding the intent.
> > > > 
> > > 
> > > And how does container_of() work ? :)
> > > typcasting the first member to its parent is the simplest form of
> > > container_of.
> > > 
> > > -j
> > > 
> > > 
> > 
> > Which is why it's completely equivalent and since code is supposed
> > to communicate meaning to humans, container_of should be used.
> > 
> 
> Nobody is suggesting typecasting cfg, dev or anything else.
> Typecasting between mailbox controllers is fine and arguably easier
> on
> the eyes than using a container_of.
> 
> -j

OK. How about:
struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan-
>con_priv;

Thanks.

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Fri, 2025-08-22 at 13:48 +0800, Chen-Yu Tsai wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Aug 22, 2025 at 10:12 AM Jjian Zhou <jjian.zhou@mediatek.com>
> wrote:
> > 
> > Add mtk-vcp-mailbox driver to support the communication with
> > VCP remote microprocessor.
> > 
> > Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> 
> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> 
> A couple minor comments below.
> 
> > ---
> >  drivers/mailbox/Kconfig                 |   9 ++
> >  drivers/mailbox/Makefile                |   2 +
> >  drivers/mailbox/mtk-vcp-mailbox.c       | 174
> > ++++++++++++++++++++++++
> >  include/linux/mailbox/mtk-vcp-mailbox.h |  32 +++++
> >  4 files changed, 217 insertions(+)
> >  create mode 100644 drivers/mailbox/mtk-vcp-mailbox.c
> >  create mode 100644 include/linux/mailbox/mtk-vcp-mailbox.h
> > 
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index 02432d4a5ccd..c28bdb855663 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -294,6 +294,15 @@ config MTK_CMDQ_MBOX
> >           critical time limitation, such as updating display
> > configuration
> >           during the vblank.
> > 
> > +config MTK_VCP_MBOX
> > +       tristate "MediaTek VCP Mailbox Support"
> > +       depends on ARCH_MEDIATEK || COMPILE_TEST
> > +       help
> > +         Say yes here to add support for the MediaTek VCP mailbox
> > driver.
> > +         The mailbox implementation provides access from the
> > application
> > +         processor to Video Companion Processor Unit.
> > +         If unsure say N.
> > +
> >  config ZYNQMP_IPI_MBOX
> >         tristate "Xilinx ZynqMP IPI Mailbox"
> >         depends on ARCH_ZYNQMP && OF
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 98a68f838486..07278871d254 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -63,6 +63,8 @@ obj-$(CONFIG_MTK_ADSP_MBOX)   += mtk-adsp-
> > mailbox.o
> > 
> >  obj-$(CONFIG_MTK_CMDQ_MBOX)    += mtk-cmdq-mailbox.o
> > 
> > +obj-$(CONFIG_MTK_VCP_MBOX)     += mtk-vcp-mailbox.o
> > +
> >  obj-$(CONFIG_ZYNQMP_IPI_MBOX)  += zynqmp-ipi-mailbox.o
> > 
> >  obj-$(CONFIG_SUN6I_MSGBOX)     += sun6i-msgbox.o
> > diff --git a/drivers/mailbox/mtk-vcp-mailbox.c
> > b/drivers/mailbox/mtk-vcp-mailbox.c
> > new file mode 100644
> > index 000000000000..6f48215896d2
> > --- /dev/null
> > +++ b/drivers/mailbox/mtk-vcp-mailbox.c
> > @@ -0,0 +1,174 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2025 MediaTek Corporation. All rights reserved.
> > + * Author: Jjian Zhou <jjian.zhou.@mediatek.com>
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/mailbox/mtk-vcp-mailbox.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +struct mtk_vcp_mbox_priv {
> > +       void __iomem *base;
> > +       struct device *dev;
> > +       struct mbox_controller mbox;
> > +       const struct mtk_vcp_mbox_cfg *cfg;
> > +       struct mtk_ipi_info ipi_recv;
> > +};
> > +
> > +struct mtk_vcp_mbox_cfg {
> > +       u32 set_in;
> > +       u32 clr_out;
> > +};
> > +
> > +static inline struct mtk_vcp_mbox_priv
> > *get_mtk_vcp_mbox_priv(struct mbox_controller *mbox)
> > +{
> > +       return container_of(mbox, struct mtk_vcp_mbox_priv, mbox);
> > +}
> > +
> > +static irqreturn_t mtk_vcp_mbox_irq_thread(int irq, void *data)
> > +{
> > +       struct mbox_chan *chan = data;
> > +       struct mtk_vcp_mbox_priv *priv =
> > get_mtk_vcp_mbox_priv(chan->mbox);
> 
> Since there's only one mailbox, you can just pass the private data,
> instead of passing the mbox_chan.

I got it. Modification like below:

struct mtk_vcp_mbox_priv *priv = data;
ret = devm_request_threaded_irq(dev, irq, NULL,
                                mtk_vcp_mbox_irq_thread, IRQF_ONESHOT,
                                dev_name(dev), priv);

> 
> > +
> > +       /* get irq status */
> > +       priv->ipi_recv.irq_status = readl(priv->base + priv->cfg-
> > >clr_out);
> > +
> > +       __ioread32_copy(priv->ipi_recv.msg, priv->base,
> > MBOX_SLOT_MAX_SIZE / 4);
> > +
> > +       mbox_chan_received_data(chan, &priv->ipi_recv);
> > +
> > +       /* clear irq status */
> > +       writel(priv->ipi_recv.irq_status, priv->base + priv->cfg-
> > >clr_out);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller
> > *mbox,
> > +                                           const struct
> > of_phandle_args *sp)
> > +{
> > +       if (sp->args_count)
> > +               return NULL;
> > +
> > +       return mbox->chans;
> > +}
> > +
> > +static int mtk_vcp_mbox_send_data(struct mbox_chan *chan, void
> > *data)
> > +{
> > +       struct mtk_vcp_mbox_priv *priv =
> > get_mtk_vcp_mbox_priv(chan->mbox);
> > +       struct mtk_ipi_info *ipi_info = data;
> > +       u32 status;
> > +
> > +       if (!ipi_info->msg) {
> > +               dev_err(priv->dev, "msg buffer is NULL.\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       status = readl(priv->base + priv->cfg->set_in) &
> > BIT(ipi_info->index);
> > +       if (status) {
> > +               dev_warn(priv->dev, "mailbox IPI %d is busy.\n",
> > ipi_info->id);
> > +               return -EBUSY;
> > +       }
> > +
> > +       if (ipi_info->slot_ofs + ipi_info->len >
> > MBOX_SLOT_MAX_SIZE)
> > +               return -EINVAL;
> > +       __iowrite32_copy(priv->base + ipi_info->slot_ofs, ipi_info-
> > >msg,
> > +                        ipi_info->len);
> > +
> > +       writel(BIT(ipi_info->index), priv->base + priv->cfg-
> > >set_in);
> > +
> > +       return 0;
> > +}
> > +
> > +static bool mtk_vcp_mbox_last_tx_done(struct mbox_chan *chan)
> > +{
> > +       struct mtk_ipi_info *ipi_info = chan->active_req;
> > +       struct mtk_vcp_mbox_priv *priv =
> > get_mtk_vcp_mbox_priv(chan->mbox);
> > +
> > +       return !(readl(priv->base + priv->cfg->set_in) &
> > BIT(ipi_info->index));
> > +}
> > +
> > +static const struct mbox_chan_ops mtk_vcp_mbox_chan_ops = {
> > +       .send_data      = mtk_vcp_mbox_send_data,
> > +       .last_tx_done   = mtk_vcp_mbox_last_tx_done,
> > +};
> > +
> > +static int mtk_vcp_mbox_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct mtk_vcp_mbox_priv *priv;
> > +       struct mbox_controller *mbox;
> > +       int ret, irq;
> > +
> > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       priv->dev = dev;
> > +       mbox = &priv->mbox;
> > +       mbox->dev = dev;
> > +       mbox->ops = &mtk_vcp_mbox_chan_ops;
> > +       mbox->txdone_irq = false;
> > +       mbox->txdone_poll = true;
> > +       mbox->of_xlate = mtk_vcp_mbox_xlate;
> > +       mbox->num_chans = 1;
> > +       mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans),
> > GFP_KERNEL);
> > +       if (!mbox->chans)
> > +               return -ENOMEM;
> > +
> > +       priv->ipi_recv.msg = devm_kzalloc(dev, MBOX_SLOT_MAX_SIZE,
> > GFP_KERNEL);
> > +       if (!priv->ipi_recv.msg)
> > +               return -ENOMEM;
> > +
> > +       priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(priv->base))
> > +               return PTR_ERR(priv->base);
> > +
> > +       priv->cfg = of_device_get_match_data(dev);
> > +       if (!priv->cfg)
> > +               return -EINVAL;
> > +
> > +       irq = platform_get_irq(pdev, 0);
> > +       if (irq < 0)
> > +               return irq;
> > +
> > +       ret = devm_request_threaded_irq(dev, irq, NULL,
> > +                                       mtk_vcp_mbox_irq_thread,
> > IRQF_ONESHOT,
> > +                                       dev_name(dev), mbox-
> > >chans);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       platform_set_drvdata(pdev, priv);
> > +
> > +       return devm_mbox_controller_register(dev, &priv->mbox);
> > +}
> > +
> > +static const struct mtk_vcp_mbox_cfg mt8196_cfg = {
> > +       .set_in         = 0x100,
> > +       .clr_out        = 0x10C,
> 
>                                 ^ use lowercase for hex numbers.

Thanks.

> 
> > +};
> > +
> > +static const struct of_device_id mtk_vcp_mbox_of_match[] = {
> > +       { .compatible = "mediatek,mt8196-vcp-mbox", .data =
> > &mt8196_cfg },
> > +       {},
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_vcp_mbox_of_match);
> > +
> > +static struct platform_driver mtk_vcp_mbox_driver = {
> > +       .probe          = mtk_vcp_mbox_probe,
> > +       .driver = {
> > +               .name   = "mtk_vcp_mbox",
> > +               .of_match_table = mtk_vcp_mbox_of_match,
> > +       },
> > +};
> > +module_platform_driver(mtk_vcp_mbox_driver);
> > +
> > +MODULE_AUTHOR("Jjian Zhou <jjian.zhou@mediatek.com>");
> > +MODULE_DESCRIPTION("MTK VCP Mailbox Controller");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mailbox/mtk-vcp-mailbox.h
> > b/include/linux/mailbox/mtk-vcp-mailbox.h
> > new file mode 100644
> > index 000000000000..143fb0d06e30
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-vcp-mailbox.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> > +/*
> > + * Copyright (c) 2025 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_VCP_MAILBOX_H__
> > +#define __MTK_VCP_MAILBOX_H__
> > +
> > +#define MBOX_SLOT_MAX_SIZE     0x100 /* mbox max slot size */
> > +
> > +/**
> > + * struct mtk_ipi_info - mailbox message info for mtk-vcp-mailbox
> > + * @msg: The share buffer between IPC and mailbox driver
> > + * @len: Message length
> > + * @id: This is for identification purposes and not actually used
> > + *     by the mailbox hardware.
> > + * @index: The signal number of the mailbox message.
> > + * @slot_ofs: Data slot offset.
> > + * @irq_status: Captures incoming signals for the RX path.
> > + *
> > + * It is used between IPC with mailbox driver.
> > + */
> > +struct mtk_ipi_info {
> > +       void *msg;
> > +       u32 len;
> > +       u32 id;
> > +       u32 index;
> > +       u32 slot_ofs;
> > +       u32 irq_status;
> > +};
> > +
> > +#endif
> > --
> > 2.46.0
> > 

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Nicolas Frattaroli]

On Monday, 22 September 2025 09:17:27 Central European Summer Time Jjian Zhou (周建) wrote:
> On Sat, 2025-09-20 at 23:02 -0500, Jassi Brar wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Fri, Sep 19, 2025 at 2:02 PM Nicolas Frattaroli
> > <nicolas.frattaroli@collabora.com> wrote:
> > > 
> > > On Friday, 19 September 2025 18:32:12 Central European Summer Time
> > > Jassi Brar wrote:
> > > > On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <wenst@chromium.org>
> > > > wrote:
> > > > > 
> > > > > On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <
> > > > > jassisinghbrar@gmail.com> wrote:
> > > > > > 
> > > > > > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <
> > > > > > jjian.zhou@mediatek.com> wrote:
> > > > > > 
> > > > > > .....
> > > > > > 
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/of.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/slab.h>
> > > > > > > +
> > > > > > > +struct mtk_vcp_mbox_priv {
> > > > > > 
> > > > > > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> > > > > > 
> > > > > > > +       void __iomem *base;
> > > > > > > +       struct device *dev;
> > > > > > > +       struct mbox_controller mbox;
> > > > > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > > > > +       struct mtk_ipi_info ipi_recv;
> > > > > > 
> > > > > > Maybe also have "struct mbox_chan chan[1]; " so that you
> > > > > > don't have to
> > > > > > allocate one during the probe.
> > > > > > Also if you have  "struct mbox_controller mbox;" as the first
> > > > > > member,
> > > > > > you could simply typecast that to get this structure.
> > > > > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > > > > 
> > > > > I read somewhere that this way of subclassing is not
> > > > > recommended.
> > > > > Instead the base class should explicitly not be the first
> > > > > member.
> > > > > And then container_of() should be used.
> > > > > 
> > > > > I don't remember where I read this though. But I think the
> > > > > explicit
> > > > > container_of() is easier for understanding the intent.
> > > > > 
> > > > 
> > > > And how does container_of() work ? :)
> > > > typcasting the first member to its parent is the simplest form of
> > > > container_of.
> > > > 
> > > > -j
> > > > 
> > > > 
> > > 
> > > Which is why it's completely equivalent and since code is supposed
> > > to communicate meaning to humans, container_of should be used.
> > > 
> > 
> > Nobody is suggesting typecasting cfg, dev or anything else.
> > Typecasting between mailbox controllers is fine and arguably easier
> > on
> > the eyes than using a container_of.
> > 
> > -j
> 
> OK. How about:
> struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan-
> >con_priv;
> 
> Thanks.
> 

An explicit cast would be worse, as at that point you're telling
C to completely ignore any semblance of a type system it has.

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Mon, 2025-09-22 at 15:10 +0200, Nicolas Frattaroli wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Monday, 22 September 2025 09:17:27 Central European Summer Time
> Jjian Zhou (周建) wrote:
> > On Sat, 2025-09-20 at 23:02 -0500, Jassi Brar wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On Fri, Sep 19, 2025 at 2:02 PM Nicolas Frattaroli
> > > <nicolas.frattaroli@collabora.com> wrote:
> > > > 
> > > > On Friday, 19 September 2025 18:32:12 Central European Summer
> > > > Time
> > > > Jassi Brar wrote:
> > > > > On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <
> > > > > wenst@chromium.org>
> > > > > wrote:
> > > > > > 
> > > > > > On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <
> > > > > > jassisinghbrar@gmail.com> wrote:
> > > > > > > 
> > > > > > > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <
> > > > > > > jjian.zhou@mediatek.com> wrote:
> > > > > > > 
> > > > > > > .....
> > > > > > > 
> > > > > > > > +#include <linux/module.h>
> > > > > > > > +#include <linux/of.h>
> > > > > > > > +#include <linux/platform_device.h>
> > > > > > > > +#include <linux/slab.h>
> > > > > > > > +
> > > > > > > > +struct mtk_vcp_mbox_priv {
> > > > > > > 
> > > > > > > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> > > > > > > 
> > > > > > > > +       void __iomem *base;
> > > > > > > > +       struct device *dev;
> > > > > > > > +       struct mbox_controller mbox;
> > > > > > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > > > > > +       struct mtk_ipi_info ipi_recv;
> > > > > > > 
> > > > > > > Maybe also have "struct mbox_chan chan[1]; " so that you
> > > > > > > don't have to
> > > > > > > allocate one during the probe.
> > > > > > > Also if you have  "struct mbox_controller mbox;" as the
> > > > > > > first
> > > > > > > member,
> > > > > > > you could simply typecast that to get this structure.
> > > > > > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > > > > > 
> > > > > > I read somewhere that this way of subclassing is not
> > > > > > recommended.
> > > > > > Instead the base class should explicitly not be the first
> > > > > > member.
> > > > > > And then container_of() should be used.
> > > > > > 
> > > > > > I don't remember where I read this though. But I think the
> > > > > > explicit
> > > > > > container_of() is easier for understanding the intent.
> > > > > > 
> > > > > 
> > > > > And how does container_of() work ? :)
> > > > > typcasting the first member to its parent is the simplest
> > > > > form of
> > > > > container_of.
> > > > > 
> > > > > -j
> > > > > 
> > > > > 
> > > > 
> > > > Which is why it's completely equivalent and since code is
> > > > supposed
> > > > to communicate meaning to humans, container_of should be used.
> > > > 
> > > 
> > > Nobody is suggesting typecasting cfg, dev or anything else.
> > > Typecasting between mailbox controllers is fine and arguably
> > > easier
> > > on
> > > the eyes than using a container_of.
> > > 
> > > -j
> > 
> > OK. How about:
> > struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan-
> > > con_priv;
> > 
> > Thanks.
> > 
> 
> An explicit cast would be worse, as at that point you're telling
> C to completely ignore any semblance of a type system it has.
> 
> 
> 
     struct mtk_vcp_mbox *priv; 
     priv->dev = dev;
     priv->chans[0].con_priv = priv;
The type of con_priv is "void *". 
Would the conversion mentioned above also have the issue you mentioned?

Thanks.

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Thu, 2025-09-18 at 18:50 -0500, Jassi Brar wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com>
> wrote:
> 
> .....
> 
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +struct mtk_vcp_mbox_priv {
> 
> Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> 
> > +       void __iomem *base;
> > +       struct device *dev;
> > +       struct mbox_controller mbox;
> > +       const struct mtk_vcp_mbox_cfg *cfg;
> > +       struct mtk_ipi_info ipi_recv;
> 
> Maybe also have "struct mbox_chan chan[1]; " so that you don't have
> to
> allocate one during the probe.
> Also if you have  "struct mbox_controller mbox;" as the first member,
> you could simply typecast that to get this structure.
> Something like "struct mpfs_mbox" in mailbox-mpfs.c
> 

The define "struct mbox_chan chan[1]" has style error.
ERROR:FLEXIBLE_ARRAY: Use C99 flexible arrays - see 
https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
#81: FILE: drivers/mailbox/mtk-vcp-mailbox.c:24:
+       struct mbox_chan chans[0];
+};

How about this error?

> ....
> > +
> > +static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller
> > *mbox,
> > +                                           const struct
> > of_phandle_args *sp)
> > +{
> > +       if (sp->args_count)
> > +               return NULL;
> > +
> > +       return mbox->chans;
> 
> return &mbox->chans[0]   seems better.
> 
> thnx

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Thu, 2025-09-18 at 18:50 -0500, Jassi Brar wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com>
> wrote:
> 
> .....
> 
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +struct mtk_vcp_mbox_priv {
> 
> Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> 
> > +       void __iomem *base;
> > +       struct device *dev;
> > +       struct mbox_controller mbox;
> > +       const struct mtk_vcp_mbox_cfg *cfg;
> > +       struct mtk_ipi_info ipi_recv;
> 
> Maybe also have "struct mbox_chan chan[1]; " so that you don't have
> to
> allocate one during the probe.
> Also if you have  "struct mbox_controller mbox;" as the first member,
> you could simply typecast that to get this structure.
> Something like "struct mpfs_mbox" in mailbox-mpfs.c
> 


ERROR:FLEXIBLE_ARRAY: Use C99 flexible arrays - see 
https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
#81: FILE: drivers/mailbox/mtk-vcp-mailbox.c:24:
+       struct mbox_chan chans[1];
+};

Can we ignore this error?

> ....
> > +
> > +static struct mbox_chan *mtk_vcp_mbox_xlate(struct mbox_controller
> > *mbox,
> > +                                           const struct
> > of_phandle_args *sp)
> > +{
> > +       if (sp->args_count)
> > +               return NULL;
> > +
> > +       return mbox->chans;
> 
> return &mbox->chans[0]   seems better.
> 
> thnx

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Nicolas Frattaroli]

On Tuesday, 23 September 2025 04:35:59 Central European Summer Time Jjian Zhou (周建) wrote:
> On Mon, 2025-09-22 at 15:10 +0200, Nicolas Frattaroli wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Monday, 22 September 2025 09:17:27 Central European Summer Time
> > Jjian Zhou (周建) wrote:
> > > On Sat, 2025-09-20 at 23:02 -0500, Jassi Brar wrote:
> > > > External email : Please do not click links or open attachments
> > > > until
> > > > you have verified the sender or the content.
> > > > 
> > > > 
> > > > On Fri, Sep 19, 2025 at 2:02 PM Nicolas Frattaroli
> > > > <nicolas.frattaroli@collabora.com> wrote:
> > > > > 
> > > > > On Friday, 19 September 2025 18:32:12 Central European Summer
> > > > > Time
> > > > > Jassi Brar wrote:
> > > > > > On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <
> > > > > > wenst@chromium.org>
> > > > > > wrote:
> > > > > > > 
> > > > > > > On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <
> > > > > > > jassisinghbrar@gmail.com> wrote:
> > > > > > > > 
> > > > > > > > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <
> > > > > > > > jjian.zhou@mediatek.com> wrote:
> > > > > > > > 
> > > > > > > > .....
> > > > > > > > 
> > > > > > > > > +#include <linux/module.h>
> > > > > > > > > +#include <linux/of.h>
> > > > > > > > > +#include <linux/platform_device.h>
> > > > > > > > > +#include <linux/slab.h>
> > > > > > > > > +
> > > > > > > > > +struct mtk_vcp_mbox_priv {
> > > > > > > > 
> > > > > > > > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> > > > > > > > 
> > > > > > > > > +       void __iomem *base;
> > > > > > > > > +       struct device *dev;
> > > > > > > > > +       struct mbox_controller mbox;
> > > > > > > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > > > > > > +       struct mtk_ipi_info ipi_recv;
> > > > > > > > 
> > > > > > > > Maybe also have "struct mbox_chan chan[1]; " so that you
> > > > > > > > don't have to
> > > > > > > > allocate one during the probe.
> > > > > > > > Also if you have  "struct mbox_controller mbox;" as the
> > > > > > > > first
> > > > > > > > member,
> > > > > > > > you could simply typecast that to get this structure.
> > > > > > > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > > > > > > 
> > > > > > > I read somewhere that this way of subclassing is not
> > > > > > > recommended.
> > > > > > > Instead the base class should explicitly not be the first
> > > > > > > member.
> > > > > > > And then container_of() should be used.
> > > > > > > 
> > > > > > > I don't remember where I read this though. But I think the
> > > > > > > explicit
> > > > > > > container_of() is easier for understanding the intent.
> > > > > > > 
> > > > > > 
> > > > > > And how does container_of() work ? :)
> > > > > > typcasting the first member to its parent is the simplest
> > > > > > form of
> > > > > > container_of.
> > > > > > 
> > > > > > -j
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Which is why it's completely equivalent and since code is
> > > > > supposed
> > > > > to communicate meaning to humans, container_of should be used.
> > > > > 
> > > > 
> > > > Nobody is suggesting typecasting cfg, dev or anything else.
> > > > Typecasting between mailbox controllers is fine and arguably
> > > > easier
> > > > on
> > > > the eyes than using a container_of.
> > > > 
> > > > -j
> > > 
> > > OK. How about:
> > > struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan-
> > > > con_priv;
> > > 
> > > Thanks.
> > > 
> > 
> > An explicit cast would be worse, as at that point you're telling
> > C to completely ignore any semblance of a type system it has.
> > 
> > 
> > 
>      struct mtk_vcp_mbox *priv; 
>      priv->dev = dev;
>      priv->chans[0].con_priv = priv;
> The type of con_priv is "void *". 
> Would the conversion mentioned above also have the issue you mentioned?
> 
> Thanks.
> 

No, in that case the cast is implicit. While void pointers do
subvert the type system, they are needed in this case because
the con_priv member needs to point at structs of any type.

The problem is that when you do something like

  struct apple *a = something;
  struct orange *o = (struct orange *)a;

then if the two structs (apple and orange) are incompatible,
the compiler won't even yell at you, because you're explicitly
casting.

With an implicit cast:

  struct apple *a = something;
  struct orange *o = a;

the compiler will tell you if you're doing something wrong.
Here's a userspace code example to illustrate the point:

    #include <stdio.h>

    struct apple {
            const char *name;
            unsigned int weight;
    };

    struct orange {
            int x;
            int y;
            int z;
    };

    int main(int argc, char** argv)
    {
            struct apple a = {"Granny Smith", 200};
            // won't compile, good!
            /* struct orange *o = &a; */
            // will compile, bad!
            struct orange *o = (struct orange *)&a;

            printf("%d\n", o->x);

            return 0;
    }

If you comment out the second struct orange line and uncomment the
first, then you'll get a compilation error, which is what we want
because the two structs are incompatible and we don't want the
assignment to work in this case, as that would be a bug.

The second struct orange line always compiles, even though the two
structs are incompatible, and will cause nonsense to be printed.

I hope this illustrates the point I was trying to make, which is
that explicit casts make it harder to find issues because they
force the language to simply accept the cast rather than give us
a compilation error when something nonsensical is being done.

Kind regards,
Nicolas Frattaroli

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Tue, 2025-09-23 at 11:06 +0200, Nicolas Frattaroli wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tuesday, 23 September 2025 04:35:59 Central European Summer Time
> Jjian Zhou (周建) wrote:
> > On Mon, 2025-09-22 at 15:10 +0200, Nicolas Frattaroli wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On Monday, 22 September 2025 09:17:27 Central European Summer
> > > Time
> > > Jjian Zhou (周建) wrote:
> > > > On Sat, 2025-09-20 at 23:02 -0500, Jassi Brar wrote:
> > > > > External email : Please do not click links or open
> > > > > attachments
> > > > > until
> > > > > you have verified the sender or the content.
> > > > > 
> > > > > 
> > > > > On Fri, Sep 19, 2025 at 2:02 PM Nicolas Frattaroli
> > > > > <nicolas.frattaroli@collabora.com> wrote:
> > > > > > 
> > > > > > On Friday, 19 September 2025 18:32:12 Central European
> > > > > > Summer
> > > > > > Time
> > > > > > Jassi Brar wrote:
> > > > > > > On Fri, Sep 19, 2025 at 3:31 AM Chen-Yu Tsai <
> > > > > > > wenst@chromium.org>
> > > > > > > wrote:
> > > > > > > > 
> > > > > > > > On Fri, Sep 19, 2025 at 7:50 AM Jassi Brar <
> > > > > > > > jassisinghbrar@gmail.com> wrote:
> > > > > > > > > 
> > > > > > > > > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <
> > > > > > > > > jjian.zhou@mediatek.com> wrote:
> > > > > > > > > 
> > > > > > > > > .....
> > > > > > > > > 
> > > > > > > > > > +#include <linux/module.h>
> > > > > > > > > > +#include <linux/of.h>
> > > > > > > > > > +#include <linux/platform_device.h>
> > > > > > > > > > +#include <linux/slab.h>
> > > > > > > > > > +
> > > > > > > > > > +struct mtk_vcp_mbox_priv {
> > > > > > > > > 
> > > > > > > > > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> > > > > > > > > 
> > > > > > > > > > +       void __iomem *base;
> > > > > > > > > > +       struct device *dev;
> > > > > > > > > > +       struct mbox_controller mbox;
> > > > > > > > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > > > > > > > +       struct mtk_ipi_info ipi_recv;
> > > > > > > > > 
> > > > > > > > > Maybe also have "struct mbox_chan chan[1]; " so that
> > > > > > > > > you
> > > > > > > > > don't have to
> > > > > > > > > allocate one during the probe.
> > > > > > > > > Also if you have  "struct mbox_controller mbox;" as
> > > > > > > > > the
> > > > > > > > > first
> > > > > > > > > member,
> > > > > > > > > you could simply typecast that to get this structure.
> > > > > > > > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > > > > > > > 
> > > > > > > > I read somewhere that this way of subclassing is not
> > > > > > > > recommended.
> > > > > > > > Instead the base class should explicitly not be the
> > > > > > > > first
> > > > > > > > member.
> > > > > > > > And then container_of() should be used.
> > > > > > > > 
> > > > > > > > I don't remember where I read this though. But I think
> > > > > > > > the
> > > > > > > > explicit
> > > > > > > > container_of() is easier for understanding the intent.
> > > > > > > > 
> > > > > > > 
> > > > > > > And how does container_of() work ? :)
> > > > > > > typcasting the first member to its parent is the simplest
> > > > > > > form of
> > > > > > > container_of.
> > > > > > > 
> > > > > > > -j
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > Which is why it's completely equivalent and since code is
> > > > > > supposed
> > > > > > to communicate meaning to humans, container_of should be
> > > > > > used.
> > > > > > 
> > > > > 
> > > > > Nobody is suggesting typecasting cfg, dev or anything else.
> > > > > Typecasting between mailbox controllers is fine and arguably
> > > > > easier
> > > > > on
> > > > > the eyes than using a container_of.
> > > > > 
> > > > > -j
> > > > 
> > > > OK. How about:
> > > > struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan-
> > > > > con_priv;
> > > > 
> > > > Thanks.
> > > > 
> > > 
> > > An explicit cast would be worse, as at that point you're telling
> > > C to completely ignore any semblance of a type system it has.
> > > 
> > > 
> > > 
> > 
> >      struct mtk_vcp_mbox *priv;
> >      priv->dev = dev;
> >      priv->chans[0].con_priv = priv;
> > The type of con_priv is "void *".
> > Would the conversion mentioned above also have the issue you
> > mentioned?
> > 
> > Thanks.
> > 
> 
> No, in that case the cast is implicit. While void pointers do
> subvert the type system, they are needed in this case because
> the con_priv member needs to point at structs of any type.
> 
> The problem is that when you do something like
> 
>   struct apple *a = something;
>   struct orange *o = (struct orange *)a;
> 
> then if the two structs (apple and orange) are incompatible,
> the compiler won't even yell at you, because you're explicitly
> casting.
> 
> With an implicit cast:
> 
>   struct apple *a = something;
>   struct orange *o = a;
> 
> the compiler will tell you if you're doing something wrong.
> Here's a userspace code example to illustrate the point:
> 
>     #include <stdio.h>
> 
>     struct apple {
>             const char *name;
>             unsigned int weight;
>     };
> 
>     struct orange {
>             int x;
>             int y;
>             int z;
>     };
> 
>     int main(int argc, char** argv)
>     {
>             struct apple a = {"Granny Smith", 200};
>             // won't compile, good!
>             /* struct orange *o = &a; */
>             // will compile, bad!
>             struct orange *o = (struct orange *)&a;
> 
>             printf("%d\n", o->x);
> 
>             return 0;
>     }
> 
> If you comment out the second struct orange line and uncomment the
> first, then you'll get a compilation error, which is what we want
> because the two structs are incompatible and we don't want the
> assignment to work in this case, as that would be a bug.
> 
> The second struct orange line always compiles, even though the two
> structs are incompatible, and will cause nonsense to be printed.
> 
> I hope this illustrates the point I was trying to make, which is
> that explicit casts make it harder to find issues because they
> force the language to simply accept the cast rather than give us
> a compilation error when something nonsensical is being done.
> 
> Kind regards,
> Nicolas Frattaroli
> 
> 

I should not directly assign a variable to priv after an explicit 
cast; instead, I should use implicit casting. This way, the compiler 
can catch type mismatch errors, avoiding unexpected errors caused by 
explicit casting.
Like:
  static int mtk_vcp_mbox_send_data(struct mbox_chan *chan, void *data)
  {
     // should be
     struct mtk_vcp_mbox *priv = chan->con_priv; 
     // should not 

struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan->con_priv; 



     

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jassi Brar]

On Tue, Sep 23, 2025 at 1:46 AM Jjian Zhou (周建) <Jjian.Zhou@mediatek.com> wrote:
>
> On Thu, 2025-09-18 at 18:50 -0500, Jassi Brar wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >
> >
> > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <jjian.zhou@mediatek.com>
> > wrote:
> >
> > .....
> >
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +
> > > +struct mtk_vcp_mbox_priv {
> >
> > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> >
> > > +       void __iomem *base;
> > > +       struct device *dev;
> > > +       struct mbox_controller mbox;
> > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > +       struct mtk_ipi_info ipi_recv;
> >
> > Maybe also have "struct mbox_chan chan[1]; " so that you don't have
> > to
> > allocate one during the probe.
> > Also if you have  "struct mbox_controller mbox;" as the first member,
> > you could simply typecast that to get this structure.
> > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> >
>
>
> ERROR:FLEXIBLE_ARRAY: Use C99 flexible arrays - see
> https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> #81: FILE: drivers/mailbox/mtk-vcp-mailbox.c:24:
> +       struct mbox_chan chans[1];
> +};
>
> Can we ignore this error?
>
It seems single element style is deprecated. While we know what we are
doing, let us placate that error by
       struct mbox_chan mch;
       struct mbox_chan chans[];
and set  chans = &mch

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jassi Brar]

On Mon, Sep 22, 2025 at 2:17 AM Jjian Zhou (周建) <Jjian.Zhou@mediatek.com> wrote:
>

> > > > > >
> > > > > > > +       void __iomem *base;
> > > > > > > +       struct device *dev;
> > > > > > > +       struct mbox_controller mbox;
> > > > > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > > > > +       struct mtk_ipi_info ipi_recv;
> > > > > >
> > > > > > Maybe also have "struct mbox_chan chan[1]; " so that you
> > > > > > don't have to
> > > > > > allocate one during the probe.
> > > > > > Also if you have  "struct mbox_controller mbox;" as the first
> > > > > > member,
> > > > > > you could simply typecast that to get this structure.
> > > > > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > > > >
> > > > > I read somewhere that this way of subclassing is not
> > > > > recommended.
> > > > > Instead the base class should explicitly not be the first
> > > > > member.
> > > > > And then container_of() should be used.
> > > > >
> > > > > I don't remember where I read this though. But I think the
> > > > > explicit
> > > > > container_of() is easier for understanding the intent.
> > > > >
> > > >
> > > > And how does container_of() work ? :)
> > > > typcasting the first member to its parent is the simplest form of
> > > > container_of.
> > > >
> > > > -j
> > > >
> > > >
> > >
> > > Which is why it's completely equivalent and since code is supposed
> > > to communicate meaning to humans, container_of should be used.
> > >
> >
> > Nobody is suggesting typecasting cfg, dev or anything else.
> > Typecasting between mailbox controllers is fine and arguably easier
> > on
> > the eyes than using a container_of.
> >
> > -j
>
> OK. How about:
> struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan->con_priv;
>
You don't need to typecast a void *. So simply do
    struct mtk_vcp_mbox *priv = chan->con_priv;

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Tue, 2025-09-23 at 08:16 -0500, Jassi Brar wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Mon, Sep 22, 2025 at 2:17 AM Jjian Zhou (周建) <
> Jjian.Zhou@mediatek.com> wrote:
> > 
> > > > > > > 
> > > > > > > > +       void __iomem *base;
> > > > > > > > +       struct device *dev;
> > > > > > > > +       struct mbox_controller mbox;
> > > > > > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > > > > > +       struct mtk_ipi_info ipi_recv;
> > > > > > > 
> > > > > > > Maybe also have "struct mbox_chan chan[1]; " so that you
> > > > > > > don't have to
> > > > > > > allocate one during the probe.
> > > > > > > Also if you have  "struct mbox_controller mbox;" as the
> > > > > > > first
> > > > > > > member,
> > > > > > > you could simply typecast that to get this structure.
> > > > > > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > > > > > 
> > > > > > I read somewhere that this way of subclassing is not
> > > > > > recommended.
> > > > > > Instead the base class should explicitly not be the first
> > > > > > member.
> > > > > > And then container_of() should be used.
> > > > > > 
> > > > > > I don't remember where I read this though. But I think the
> > > > > > explicit
> > > > > > container_of() is easier for understanding the intent.
> > > > > > 
> > > > > 
> > > > > And how does container_of() work ? :)
> > > > > typcasting the first member to its parent is the simplest
> > > > > form of
> > > > > container_of.
> > > > > 
> > > > > -j
> > > > > 
> > > > > 
> > > > 
> > > > Which is why it's completely equivalent and since code is
> > > > supposed
> > > > to communicate meaning to humans, container_of should be used.
> > > > 
> > > 
> > > Nobody is suggesting typecasting cfg, dev or anything else.
> > > Typecasting between mailbox controllers is fine and arguably
> > > easier
> > > on
> > > the eyes than using a container_of.
> > > 
> > > -j
> > 
> > OK. How about:
> > struct mtk_vcp_mbox *priv = (struct mtk_vcp_mbox *)chan->con_priv;
> > 
> 
> You don't need to typecast a void *. So simply do
>     struct mtk_vcp_mbox *priv = chan->con_priv;

OK. This seems to convey the same meaning as what Nicolas said.
I modify it to this format.
Thank you.

Re: [PATCH v5 2/2] mailbox: mediatek: Add mtk-vcp-mailbox driver

[Jjian Zhou (周建)]

On Tue, 2025-09-23 at 08:11 -0500, Jassi Brar wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tue, Sep 23, 2025 at 1:46 AM Jjian Zhou (周建) <
> Jjian.Zhou@mediatek.com> wrote:
> > 
> > On Thu, 2025-09-18 at 18:50 -0500, Jassi Brar wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On Thu, Aug 21, 2025 at 9:12 PM Jjian Zhou <
> > > jjian.zhou@mediatek.com>
> > > wrote:
> > > 
> > > .....
> > > 
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/slab.h>
> > > > +
> > > > +struct mtk_vcp_mbox_priv {
> > > 
> > > Maybe 'mtk_vcp_mbox' is a more appropriate name ?
> > > 
> > > > +       void __iomem *base;
> > > > +       struct device *dev;
> > > > +       struct mbox_controller mbox;
> > > > +       const struct mtk_vcp_mbox_cfg *cfg;
> > > > +       struct mtk_ipi_info ipi_recv;
> > > 
> > > Maybe also have "struct mbox_chan chan[1]; " so that you don't
> > > have
> > > to
> > > allocate one during the probe.
> > > Also if you have  "struct mbox_controller mbox;" as the first
> > > member,
> > > you could simply typecast that to get this structure.
> > > Something like "struct mpfs_mbox" in mailbox-mpfs.c
> > > 
> > 
> > 
> > ERROR:FLEXIBLE_ARRAY: Use C99 flexible arrays - see
> > 
https://urldefense.com/v3/__https://docs.kernel.org/process/deprecated.html*zero-length-and-one-element-arrays__;Iw!!CTRNKA9wMg0ARbw!kLlX71HetZwXMZ9UmdtvMEKKykbZINIUNVW5nLVfoztdkSJMlaa3QfbzZxGKIkSgqmHHc2Lvfwpj4URsOVCtekNCFh5L$
> > #81: FILE: drivers/mailbox/mtk-vcp-mailbox.c:24:
> > +       struct mbox_chan chans[1];
> > +};
> > 
> > Can we ignore this error?
> > 
> 
> It seems single element style is deprecated. While we know what we
> are
> doing, let us placate that error by
>        struct mbox_chan mch;
>        struct mbox_chan chans[];
> and set  chans = &mch

Do you have any specific considerations for suggesting this approach?
Wo
uld it be sufficient to just define a 'struct mbox_chan chans'?