On Tue, Jul 8, 2025 at 8:54 AM Guomin chen [off-list ref] wrote:
....

quoted

quoted

+/* [0~7] Fast channel
+ * [8] doorbell base channel
+ * [9]fifo base channel
+ * [10] register base channel
+ */
+#define MBOX_FAST_IDX          7
+#define MBOX_DB_IDX            8
+#define MBOX_FIFO_IDX          9
+#define MBOX_REG_IDX           10
+#define CIX_MBOX_CHANS         11
+

if it is not really a single controller owning different channels,
maybe implement only what you currently use.

As mentioned in the previous email, a single controller can support
multiple different channels.

OK. I am not too worried about having all variants in one driver esp
when it is manageable and share the code.
Unless I am overlooking something. Arnd?

quoted

quoted

+static u32 cix_mbox_read(struct cix_mbox_priv *priv, u32 offset)
+{
+       if (priv->use_shmem)
+               return ioread32(priv->base + offset - SHMEM_OFFSET);
+       else
+               return ioread32(priv->base + offset);
+}
+

use_shmem is set for only CIX_MBOX_TYPE_DB, but it affects every read/write.
Maybe instead adjust the base for TYPE_DB?

The reason we introduced use_shmem here is that we had to adjust the base
address of TYPE_DB to resolve the reg conflict in the DTS.
This change has virtually no impact on performance.

Yes, I can see it should have no impact on performance and I think
adjusting the base once
during init is cleaner than checking the flag every read/write.
But wait... use_shmem is a controller wide flag, and isn't
priv->use_shmem always set to true  in cix_mbox_init() ?
Is the driver even tested?
....

quoted

quoted

+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 index = cp->index, ret;
+       u32 val_32;
+
+       ret = request_irq(priv->irq, cix_mbox_isr, 0,
+                         dev_name(priv->dev), chan);

The same irq is requested for each channel. How do you expect it to
work? Maybe request it just once in probe and pass the 'priv' instead
of 'chan' , and in the cix_mbox_isr handle according to INT_STATUS

For the same mailbox controller, there won't be multiple channels
simultaneously requesting the same IRQ, so there won't be an issue
here. As you mentioned, if we need to handle multiple channels working
concurrently in the future, we would need to modify cix_mbox_isr.
However, that is not required at the moment.

Is it too hard to do it right already?

....

quoted

quoted

+MODULE_AUTHOR("Cix Technology Group Co., Ltd.");
+MODULE_DESCRIPTION("CIX mailbox driver");
+MODULE_LICENSE("GPL");

GPL v2 ? according to the SPDX-License-Identifier

And please make sure you run scripts/checkpatch.pl

Yes, I'm also confused here. I was previously using GPL v2,
but when I ran checkpatch.pl, it triggered a warning due to
commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE 'GPL'
vs. 'GPL v2' bogosity").  So I changed it to GPL.

Sorry, I was wrong. It should simply be GPL

Thanks.