Re: [PATCH v8 07/28] mailbox: Allow direct registration to a channel

[Elliot Berman]

On 1/9/2023 1:34 PM, Alex Elder wrote:
> On 12/19/22 4:58 PM, Elliot Berman wrote:
> > Support virtual mailbox controllers and clients which are not platform
> > devices or come from the devicetree by allowing them to match client to
> > channel via some other mechanism.
>
> The new function behaves very much like mbox_request_channel()
> did before.
>
> The new function differs from omap_mbox_request_channel() in that
> it can change the if chan->txdone_method is TXDONE_BY_POLL, it
> is changed to TXDONE_BY_ACK if the client's knows_txdone field is
> set.  Is this OK?  Why?

Both of the current drivers that use mbox_bind_client use TXDONE_BY_IRQ, 
so this doesn't cause issue for checking whether the client has 
txdone_method.

> It also assumes chan->mbox->ops->startup us non-null (though that
> isn't really a problem).
>
> > Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>
> > ---
> >   drivers/mailbox/mailbox.c      | 96 ++++++++++++++++++++++++----------
> >   drivers/mailbox/omap-mailbox.c | 18 ++-----
> >   drivers/mailbox/pcc.c          | 18 ++-----
> >   include/linux/mailbox_client.h |  1 +
> >   4 files changed, 76 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index 4229b9b5da98..adf36c05fa43 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -317,6 +317,71 @@ int mbox_flush(struct mbox_chan *chan, unsigned 
> > long timeout)
> >   }
> >   EXPORT_SYMBOL_GPL(mbox_flush);
> > +static int __mbox_bind_client(struct mbox_chan *chan, struct 
> > mbox_client *cl)
>
> There should be an unbind_client() call.  At a minimum, you are
> calling try_module_get(), and the matching module_put() call
> would belong there.  And even though one might just call
> module_put() elsewhere for this, it would be cleaner to have
> a function that similarly encapsulates the shutdown call
> as well.

The function for this is "mbox_free_channel".

Thanks,
Elliot