On 1/10/23 11:57 AM, Elliot Berman wrote:
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?
Sorry, reading that now, I see I placed an "if" in the wrong spot.
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.
I'm not so sure, but it's on you to make sure you don't break
anything... I see only two spots where TXDONE_BY_IRQ is set,
and TXDONE_BY_IRQ seems to be set when channels are freed.
I spent (too much) time trying to track this back but I'm
giving up. If you're sure it's correct, I accept that...
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.
n
The function for this is "mbox_free_channel".
My point is about the way you are abstracting the "bind" operation
as a (now encapsulated) part of requesting the channel. Yes, when
mbox_free_channel() is called, it effectively "unbinds" the channel.
But you're creating a "bind" abstraction, where it's not explicit
that you're requesting the channel. I'm suggesting you also create
an "unbind" operation to reverse that.
This is more important for the mbox_bind_client() call than
mbox_request_channel(). (And by the way, it looks like
pcc_mbox_free_channel() doesn't call pcc_mbox_free_channel()
as it should, but this unfamiliar code...)
And... it's weird to me that gh_rm_drv_probe() calls gh_msgq_init()
(to initialize the Gunhah message queue code), but then has to
call mbox_free_channel() *separate* from gh_msgq_remove(); the
free channel (or better, unbind client) should happen in the
message queue code.
It's not a critically important point, but now at least I hope
you understand what I'm trying to say.
-Alex
Thanks,
Elliot
