Re: [PATCH v8 5/9] mailbox: add CIX mailbox driver

[Guomin chen <guomin.chen@cixtech.com>]

On Mon, May 19, 2025 at 12:46:54PM -0500, Jassi Brar wrote:
> [Some people who received this message don't often get email from jassisinghbrar@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL
> 
> Hi,
> 
Hi Jassi 
  Thank you for your feedback.
> > diff --git a/drivers/mailbox/cix-mailbox.c b/drivers/mailbox/cix-mailbox.c
> > new file mode 100644
> > index 000000000000..c2783dd7d145
> > --- /dev/null
> > +++ b/drivers/mailbox/cix-mailbox.c
> > @@ -0,0 +1,632 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2025 Cix Technology Group Co., Ltd.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "mailbox.h"
> > +
> > +/* Register define */
> > +#define REG_MSG(n)     (0x0 + 0x4*(n))                 /* 0x0~0x7c */
> > +#define REG_DB_ACK     REG_MSG(CIX_MBOX_MSG_LEN)       /* 0x80 */
> > +#define ERR_COMP       (REG_DB_ACK + 0x4)              /* 0x84 */
> > +#define ERR_COMP_CLR   (REG_DB_ACK + 0x8)              /* 0x88 */
> > +#define REG_F_INT(IDX) (ERR_COMP_CLR + 0x4*(IDX+1))    /* 0x8c~0xa8 */
> > +#define FIFO_WR                (REG_F_INT(MBOX_FAST_IDX+1))    /* 0xac */
> > +#define FIFO_RD                (FIFO_WR + 0x4)                 /* 0xb0 */
> > +#define FIFO_STAS      (FIFO_WR + 0x8)                 /* 0xb4 */
> > +#define FIFO_WM                (FIFO_WR + 0xc)                 /* 0xb8 */
> > +#define INT_ENABLE     (FIFO_WR + 0x10)                /* 0xbc */
> > +#define INT_ENABLE_SIDE_B      (FIFO_WR + 0x14)        /* 0xc0 */
> > +#define INT_CLEAR      (FIFO_WR + 0x18)                /* 0xc4 */
> > +#define INT_STATUS     (FIFO_WR + 0x1c)                /* 0xc8 */
> > +#define FIFO_RST       (FIFO_WR + 0x20)                /* 0xcc */
> > +
> > +/* [0~7] Fast channel
> > + * [8] doorbell base channel
> > + * [9]fifo base channel
> > + * [10] register base channel
> > + */
> > +#define CIX_MBOX_CHANS         (11)
> > +
> > +/*
> > + * The maximum transmission size is 32 words or 128 bytes.
> > + */
> > +#define CIX_MBOX_MSG_LEN       (32)    /* Max length = 32 words */
> > +#define MBOX_MSG_LEN_MASK      (0x7fL) /* Max length = 128 bytes */
> > +
> >
> Move these above register defines where these are used.
> Also, no need for brackets around numbers. Here and elsewhere.
> ....
Okay, I will move these two macros to the beginning and
remove the brackets around numbers.

> > +
> > +static void cix_mbox_isr_reg(struct mbox_chan *chan)
> > +{
> > +       struct cix_mbox_priv *priv = to_cix_mbox_priv(chan->mbox);
> > +       u32 int_status;
> > +       u32 data[CIX_MBOX_MSG_LEN];
> > +       int i;
> > +       u32 len;
> >
> cosmetic: tidy these up by merging and sorting in reverse christmas tree.
> 
Okay, I will make an overall adjustment according to this rule.

> > +
> > +       int_status = cix_mbox_read(priv, INT_STATUS);
> > +
> > +       if (priv->dir == MBOX_RX) {
> > +               /* rx interrupt is triggered */
> > +               if (int_status & DB_INT) {
> > +                       cix_mbox_write(priv, DB_INT, INT_CLEAR);
> > +                       data[0] = cix_mbox_read(priv, REG_MSG(0));
> > +                       len = mbox_get_msg_size(data);
> > +                       for (i = 0; i < len; i++)
> > +                               data[i] = cix_mbox_read(priv, REG_MSG(i));
> > +
> > +                       /* trigger ack interrupt */
> > +                       cix_mbox_write(priv, DB_ACK_INT_BIT, REG_DB_ACK);
> > +                       mbox_chan_received_data(chan, data);
> > +               }
> > +       } else {
> > +               /* tx ack interrupt is triggered */
> > +               if (int_status & ACK_INT) {
> > +                       cix_mbox_write(priv, ACK_INT, INT_CLEAR);
> > +                       mbox_chan_txdone(chan, 0);
> > +               }
> > +       }
> > +}
> > +
> > +static void cix_mbox_isr_fifo(struct mbox_chan *chan)
> > +{
> > +       struct cix_mbox_priv *priv = to_cix_mbox_priv(chan->mbox);
> > +       u32 data[CIX_MBOX_MSG_LEN] = { 0 };
> >
> Is it really needed? Can we do with just zeroing the byte after valid data?
> At least move it under "FIFO waterMark interrupt is generated", so it
> is only done when needed.
Yes, In some cases it is not necessary.
I will move this under "FIFO waterMark interrupt is generated." 

> > +
> > +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);
> >
> Can we do this later just before returning from the function? Or
> atleast free the irq before error returns.
This cannot be done before the return, as it needs to be registered
before the interrupt enable register. 
However, I do need to free this IRQ before the error return.

> Also please make sure you run scripts/checkpatch and have all warnings cleared.
I have already run scripts/checkpatch to perform the check. 
The warning you mentioned is due to the need to update the MAINTAINERS file for
the newly added files, right?

Thanks
Guomin Chen