Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

Wolfram Sang
2026-03-22T19:58:55Z

[-- Attachment #1: Type: text/plain, Size: 8120 bytes --]

Hi Geert,

thank you for the review!

> > +
> > +struct mfis_per_chan_priv {
>
> I would drop the "per_".

Probably OK, will think about it.

>
> > + u32 reg;
> > + int irq;
> > +};
> > +
> > +struct mfis_priv {
> > + struct device *dev;
> > + struct mfis_reg common_reg;
> > + struct mfis_reg mbox_reg;
> > + const struct mfis_info *info;
> > +
> > + /* mailbox private data */
> > + struct mbox_controller mbox;
> > + struct mfis_per_chan_priv *per_chan_privs;
>
> Likewise.
> This could be a flexible array, if it weren't for the hwspinlock array
> you will have to add later, right?

No, hwspinlock doesn't need any private data here. But something else
could come in the future maybe? I also don't see a big advantage of the
flexible array, too. Maybe it's too late in the evening...

> > +static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)
>
> Both writel() and iowrite32() use the inverse order of "reg" and "val".
> But I can understand you want to keep "mreg" and "reg" together.

Yes, please!

> > +/****************************************************
> > + * Mailbox
>
> Missing closing asterisk ;-)

OK.

>
> > + ****************************************************/
> > +
> > +#define mfis_mb_mbox_to_priv(_m) container_of((_m), struct mfis_priv, mbox)
>
> Both "mb" and "mbox", so perhaps mfis_mbox_to_priv()?
> And perhaps s/mb_/mbox_/ everywhere?

Well, not sure here. 'mb' marks the mailbox part of the driver. 'mbox'
is the variable we work on. So, it has a pattern.

> > + struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> > + int ret = 0;
> > +
> > + if (per_chan_priv->irq)
> > + ret = request_irq(per_chan_priv->irq, mfis_mb_iicr_interrupt, 0,
> > + dev_name(chan->mbox->dev), chan);
> > +
> > + return ret;
>
> You can reduce indentation, and get rid of ret, by inverting the
> conditional.

I like this.

> > +static void mfis_mb_shutdown(struct mbox_chan *chan)
> > +{
> > + struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> > +
> > + free_irq(per_chan_priv->irq, chan);
>
> if (per_chan_priv->irq) ...
>
> free_irq() seems to support zero, but irq_to_desc() still has to
> traverse the mtree.

I checked it and it prints a warning, so 'if' is good.

> > + chan_num = sp->args[0];
>
> "chan_num" is the hardware channel number, and should be validated
> against mpriv->info->mb_num_channels.

Ack!

>
> > + chan_flags = sp->args[1];
> > +
> > + /* Channel layout is described in mfis_mb_probe() */
>
> Given you already use "chan += ..." below, perhaps:
>
> chan = mbox->chans + chan_num;
>
> > + if (priv->info->mb_channels_are_unidir) {
> > + is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> > + chan = mbox->chans + 2 * chan_num + is_only_rx;
>
> ...and:
>
> chan += chan_num + is_only_rx;
>
> > + } else {
> > + is_only_rx = false;
> > + chan = mbox->chans + chan_num;
>
> ... and drop this line?
> With a proper preinitalization of is_only_rx, the whole "else" branch
> can be dropped.

I agree it saves a few lines, but I really think the original code is
easier to understand. Channel layout is already wickes and doing 'channel
+=' twice is harder to understand than a simple assignment IMO.

>
> > + }
> > +
> > + if (priv->info->mb_reg_comes_from_dt) {
> > + tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
> > + if (tx_uses_eicr)
> > + chan += mbox->num_chans / 2;
> > + } else {
> > + tx_uses_eicr = priv->info->mb_tx_uses_eicr;
> > + }
>
> "chan - mbox->chans" is the logical channel number, and should be
> validated against mbox_num_chans, to avoid out-of-bound accesses.

"chan - ..."? You mean "chan + ..."?

>
> > +
> > + per_chan_priv = chan->con_priv;
> > + per_chan_priv->reg = chan_num * 0x1000 + (tx_uses_eicr ^ is_only_rx) * 4;
>
> I think it would be good to document this register is either an IICR
> or EICR register offset, through:
> 1. A comment, or
> 2. Definitions and code, e.g
>
> #define MFISIICR 0x00
> #define MFISEICR 0x04
>
> if (tx_uses_eicr ^ is_only_rx)
> per_chan_priv->reg = chan_num * 0x1000 + MFISEICR;
> else
> per_chan_priv->reg = chan_num * 0x1000 + MFISIICR;
>
> Or:
>
> #define MFISIICR(i) ((i) * 0x1000 + 0x00)
> #define MFISEICR(i) ((i) * 0x1000 + 0x04)
>
> per_chan_priv->reg = (tx_uses_eicr ^ is_only_rx) ? MFISEICR(chan_num)
> : MFISIICR(chan_num);
>

I like the latter one. Let my try this. But maybe it will be just a
comment in the end ;)

> > +
> > + if (!priv->info->mb_channels_are_unidir || is_only_rx) {
> > + char irqname[8];
> > + char suffix = tx_uses_eicr ? 'i' : 'e';
> > +
> > + /* "ch0i" or "ch0e" */
> > + scnprintf(irqname, sizeof(irqname), "ch%d%c", chan_num, suffix);
>
> "%u" for unsigned chan_num.

OK:

>
> > +
> > + per_chan_priv->irq = of_irq_get_byname(mbox->dev->of_node, irqname);
> > + if (per_chan_priv->irq < 0)
> > + return ERR_PTR(per_chan_priv->irq);
> > + else if (per_chan_priv->irq == 0)
>
> No need for "else" after "return".

OK.

>
> > + return ERR_PTR(-ENOENT);
> > + }
> > +
> > + return chan;
> > +}
> > +
> > +static int mfis_mb_probe(struct mfis_priv *priv)
> > +{
> > + struct device *dev = priv->dev;
> > + struct mbox_chan *chan;
> > + struct mbox_controller *mbox;
> > + unsigned int i, num_chan = priv->info->mb_num_channels;
>
> "i" is only used in the for-loop below, so you could declare it in the
> for-statement. As a bonus, that would avoid mixing the declaration of
> uninitialized and initialized variables.

Yes!

>
> > +
> > + if (priv->info->mb_channels_are_unidir)
> > + /* Channel layout: Ch0-TX, Ch0-RX, Ch1-TX... */
> > + num_chan *= 2;
> > +
> > + if (priv->info->mb_reg_comes_from_dt)
> > + /* Channel layout: <n> IICR channels, <n> EICR channels */
> > + num_chan *= 2;
>
> Curly braces around multi-line if-branches (even if they are comments)?

OK? I don't mind.

> > +/****************************************************
> > + * Common
>
> Missing closing asterisk.

OK.

>
> > + ****************************************************/
> >
> > +static int mfis_reg_probe(struct platform_device *pdev, struct mfis_priv *priv,
> > + struct mfis_reg *mreg, const char *name, bool required)
> > +{
> > + struct resource *res;
> > + void __iomem *base;
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> > +
> > + /* If there is no mailbox resource, registers are in the common space */
> > + if (!res && !required) {
>
> Given you only test for the negated "!required", perhaps invert the
> logic, and replace "required" by "optional"?

I had this first and it looked weird that the first call had "false" and
the second one "true". I liked it better this way but don't really mind.

>
> > + priv->mbox_reg = priv->common_reg;
>
> This left me looking for an assignment to priv->mbox_reg in the "else"
> branch ;-) Then I realized "&priv->mbox_reg" is passed as the "mreg"
> parameter. Hence perhaps:
>
> *mreg = priv->common_reg;
>
> ?

Yup, that should work.

I will work on the next version tomorrow. Thank you for your input!

Happy hacking,

Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]