Re: [PATCH v4 06/23] mailbox: Allow controller specific mapping using fwnode

[Anup Patel]

On Tue, May 27, 2025 at 5:11 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Sun, May 25, 2025 at 02:16:53PM +0530, Anup Patel wrote:
> > Introduce optional fw_node() callback which allows a mailbox controller
> > driver to provide controller specific mapping using fwnode.
> >
> > The Linux OF framework already implements fwnode operations for the
> > Linux DD framework so the fw_xlate() callback works fine with device
> > tree as well.
>
> ...
>
> >  struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
> >  {
> > +     struct fwnode_reference_args fwspec;
>
> + property.h (if not done yet)

This needs fwnode.h

>
> > -     int ret;
> > +     int i, ret;
>
> Why is 'i' signed?

No particular reason because 'i' is not being assigned a negative
value so both signed and unsigned will work fine.

>
> > -     if (!dev || !dev->of_node) {
> > -             pr_debug("%s: No owner device node\n", __func__);
> > +     if (!dev || !dev->fwnode) {
>
> Do not dereference fwnode directly. Use dev_fwnode.
>
> > +             pr_debug("%s: No owner %s\n", __func__, !dev ? "device" : "fwnode");
>
> Use positive conditional.
>
> __func__ is redundant it debug messages. With Dynamic Debug enabled it may be
> switched at run-time.

Okay, I will drop the __func__

>
> >               return ERR_PTR(-ENODEV);
> >       }
> >
> > -     ret = of_parse_phandle_with_args(dev->of_node, "mboxes", "#mbox-cells",
> > -                                      index, &spec);
> > +     ret = fwnode_property_get_reference_args(dev->fwnode, "mboxes",
>
>         struct fwnode_handle *fwnode = dev_fwnode(dev);
>
> > +                                              "#mbox-cells", 0, index, &fwspec);
> >       if (ret) {
> >               dev_dbg(dev, "%s: can't parse \"mboxes\" property\n", __func__);
> >               return ERR_PTR(ret);
> >       }
>
> > +     memset(&spec, 0, sizeof(spec));
> > +     if (dev->of_node) {
>
> What is this check for?

It's a paranoid check on my part. I will drop the check
in the next revision.

>
> > +             spec.np = to_of_node(fwspec.fwnode);
> > +             spec.args_count = fwspec.nargs;
> > +             for (i = 0; i < spec.args_count; i++)
> > +                     spec.args[i] = fwspec.args[i];
> > +     }
> > +
> >       mutex_lock(&con_mutex);
> >
> >       chan = ERR_PTR(-EPROBE_DEFER);
> > -     list_for_each_entry(mbox, &mbox_cons, node)
> > -             if (mbox->dev->of_node == spec.np) {
> > +     list_for_each_entry(mbox, &mbox_cons, node) {
> > +             if (mbox->fw_xlate && mbox->dev->fwnode == fwspec.fwnode) {
> > +                     chan = mbox->fw_xlate(mbox, &fwspec);
> > +                     if (!IS_ERR(chan))
> > +                             break;
> > +             } else if (mbox->of_xlate && mbox->dev->of_node == spec.np) {
> >                       chan = mbox->of_xlate(mbox, &spec);
> >                       if (!IS_ERR(chan))
> >                               break;
> >               }
>
>
>                 if (!IS_ERR(...))
>                         break;
>
> is common.

Latest mailbox.c has changed so I will check and update accordingly.

>
>
> > +     }
>
> ...
>
> > +fw_mbox_index_xlate(struct mbox_controller *mbox,
> > +                 const struct fwnode_reference_args *sp)
>
> One line?

Okay, I will update.

Regards,
Anup