Re: [PATCH v3 05/10] mailbox: add MediaTek GPUEB IPI mailbox

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Sunday, 21 September 2025 07:00:59 Central European Summer Time Jassi Brar wrote:
> On Wed, Sep 17, 2025 at 7:23 AM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> ....
> 
> > +#define MBOX_CTL_TX_STS                0x0000
> > +#define MBOX_CTL_IRQ_SET       0x0004
> > +#define MBOX_CTL_IRQ_CLR       0x0074
> > +#define MBOX_CTL_RX_STS                0x0078
> > +
> 1) Please don't pollute the global namespace. Make these something
> like MBOX_MTK_GPUEB_xxx. Here and elsewhere.

I tend to disagree. These don't pollute the global namespace, they're
defined as part of the file, so only pollute its local scope. I'm not
going to make 25 character long symbols just to work around an issue
that doesn't exist, but may exist in the unlikely future where
mailbox.h gets its own symbol named precisely the same way but
whoever adds it doesn't try to compile test every single mailbox
driver to make sure they didn't break anything.

> 2) You don't write short values, so maybe just 0x04, 0x04 0x74 and 0x78.
> 
> 
> > +#define MBOX_FULL              BIT(0) /* i.e. we've received data */
> > +#define MBOX_CLOGGED           BIT(1) /* i.e. the channel is shutdown */
> > +
> This is confusing. CLOGGED usually means malfunction, but it seems you
> want to call it STOPPED or UNINIT?

I don't agree that "CLOGGED usually means malfunction". To clog something
is to impede its flow, which in this case is the correct terminology to
refer to what's happened to the channel. "UNINIT" is wrong, it's initialised
properly. "STOPPED" is also wrong, it's not stopped, it still sends, it just
won't pass it on through.

> 
> 
> > +#define MBOX_MAX_RX_SIZE       32 /* in bytes */
> > +
> > +struct mtk_gpueb_mbox {
> > +       struct device *dev;
> > +       struct clk *clk;
> > +       void __iomem *mbox_mmio;
> > +       void __iomem *mbox_ctl;
> > +       struct mbox_controller mbox;
> > +       struct mtk_gpueb_mbox_chan *ch;
> > +       int irq;
> > +       const struct mtk_gpueb_mbox_variant *v;
> > +};
> Other structures have kernel-doc, so why not here too?
> ...
>

Because fully documenting all internal structures is not required
for acceptance and writing redundant explanations for members that
can be understood from name and context is redundant.

> > +
> > +static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
> > +{
> > +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> > +       int i;
> > +       u32 *values = data;
> > +
> maybe order in decreasing lengths ?
> 
> 
> > +
> > +       /*
> > +        * We don't want any fancy nonsense, just write the 32-bit values in
> > +        * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
> > +        */
> >
> Please make the comment technical. Currently it just expresses your
> distaste for fancy :)
> 

Then I will have to make an assertive statement about memory semantics
of those two calls and how they differ from writel, which I don't want
to do, because it would likely be inaccurate or not the full picture
as those two calls can do a variety of things depending on the platform.

Saying that I want 32-bit writes in order is much simpler than explaining
how the two mentioned calls some well-meaning future developer may wish
to replace this with don't do that.

> > +       for (i = 0; i < ch->c->tx_len; i += 4)
> > +               writel(values[i / 4], ch->ebm->mbox_mmio + ch->c->tx_offset + i);
> > +
> 
> ...
> > +
> > +static struct mbox_chan *
> > +mtk_gpueb_mbox_of_xlate(struct mbox_controller *mbox,
> > +                       const struct of_phandle_args *sp)
> > +{
> > +       struct mtk_gpueb_mbox *ebm = dev_get_drvdata(mbox->dev);
> > +
> > +       if (!sp->args_count)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       if (sp->args[0] >= ebm->v->num_channels)
> > +               return ERR_PTR(-ECHRNG);
> > +
> > +       return &mbox->chans[sp->args[0]];
> > +}
> >
> Just use the default of_mbox_index_xlate()
> 
> ....
> > +
> > +       for (i = 0; i < ebm->v->num_channels; i++) {
> 
> You make this block a bit cleaner by using a temporary variable
>         echan = &ebm->ch[i];
> and using echan instead of ebm->ch[i] a dozen times below.
> 
> > +               ebm->ch[i].c = &ebm->v->channels[i];
> > +               if (ebm->ch[i].c->rx_len > MBOX_MAX_RX_SIZE) {
> > +                       dev_err(ebm->dev, "Channel %s RX size (%d) too large\n",
> > +                               ebm->ch[i].c->name, ebm->ch[i].c->rx_len);
> > +                       return -EINVAL;
> > +               }
> > +               ebm->ch[i].full_name = devm_kasprintf(ebm->dev, GFP_KERNEL, "%s:%s",
> > +                                                     dev_name(ebm->dev), ebm->ch[i].c->name);
> > +               if (!ebm->ch[i].full_name)
> > +                       return -ENOMEM;
> > +
> > +               ebm->ch[i].ebm = ebm;
> > +               ebm->ch[i].num = i;
> > +               spin_lock_init(&ebm->mbox.chans[i].lock);
> > +               ebm->mbox.chans[i].con_priv = &ebm->ch[i];
> > +               atomic_set(&ebm->ch[i].rx_status, MBOX_CLOGGED);
> > +       }
> > +
> 
> 
> -j
>