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

[Jassi Brar <jassisinghbrar@gmail.com>]

On Tue, Jul 8, 2025 at 8:54 AM Guomin chen <guomin.chen@cixtech.com> wrote:
....
> > > +/* [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?


> > > +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?
....

> > > +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?

....
> > > +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.