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

[Wolfram Sang]

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: signature.asc
Description: PGP signature