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

Jassi Brar
2025-09-21T05:01:11Z

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

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

> +
> +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 :)

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