2024-03-25 18:01:43

by Andrew Davis

Subject: [PATCH 12/13] mailbox: omap: Reverse FIFO busy check logic

It is much more clear to check if the hardware FIFO is full and return
EBUSY if true. This allows us to also remove one level of indention
from the core of this function. It also makes the similarities between
omap_mbox_chan_send_noirq() and omap_mbox_chan_send() more obvious.

Signed-off-by: Andrew Davis <[email protected]>
---
drivers/mailbox/omap-mailbox.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
index 8e2760d2c5b0c..c5d4083125856 100644
--- a/drivers/mailbox/omap-mailbox.c
+++ b/drivers/mailbox/omap-mailbox.c
@@ -375,34 +375,33 @@ static void omap_mbox_chan_shutdown(struct mbox_chan *chan)

static int omap_mbox_chan_send_noirq(struct omap_mbox *mbox, u32 msg)
{
- int ret = -EBUSY;
+ if (mbox_fifo_full(mbox))
+ return -EBUSY;

- if (!mbox_fifo_full(mbox)) {
- omap_mbox_enable_irq(mbox, IRQ_RX);
- mbox_fifo_write(mbox, msg);
- ret = 0;
- omap_mbox_disable_irq(mbox, IRQ_RX);
+ omap_mbox_enable_irq(mbox, IRQ_RX);
+ mbox_fifo_write(mbox, msg);
+ omap_mbox_disable_irq(mbox, IRQ_RX);

- /* we must read and ack the interrupt directly from here */
- mbox_fifo_read(mbox);
- ack_mbox_irq(mbox, IRQ_RX);
- }
+ /* we must read and ack the interrupt directly from here */
+ mbox_fifo_read(mbox);
+ ack_mbox_irq(mbox, IRQ_RX);

- return ret;
+ return 0;
}

static int omap_mbox_chan_send(struct omap_mbox *mbox, u32 msg)
{
- int ret = -EBUSY;
-
- if (!mbox_fifo_full(mbox)) {
- mbox_fifo_write(mbox, msg);
- ret = 0;
+ if (mbox_fifo_full(mbox)) {
+ /* always enable the interrupt */
+ omap_mbox_enable_irq(mbox, IRQ_TX);
+ return -EBUSY;
}

+ mbox_fifo_write(mbox, msg);
+
/* always enable the interrupt */
omap_mbox_enable_irq(mbox, IRQ_TX);
- return ret;
+ return 0;
}

static int omap_mbox_chan_send_data(struct mbox_chan *chan, void *data)
--
2.39.2

2024-04-01 23:32:42

by Hari Nagalla

Subject: Re: [PATCH 12/13] mailbox: omap: Reverse FIFO busy check logic

On 3/25/24 12:20, Andrew Davis wrote:
>
> static int omap_mbox_chan_send_noirq(struct omap_mbox *mbox, u32 msg)
> {
> - int ret = -EBUSY;
> + if (mbox_fifo_full(mbox))
> + return -EBUSY;
>
> - if (!mbox_fifo_full(mbox)) {
> - omap_mbox_enable_irq(mbox, IRQ_RX);
> - mbox_fifo_write(mbox, msg);
> - ret = 0;
> - omap_mbox_disable_irq(mbox, IRQ_RX);
> + omap_mbox_enable_irq(mbox, IRQ_RX);
> + mbox_fifo_write(mbox, msg);
> + omap_mbox_disable_irq(mbox, IRQ_RX);
>
> - /* we must read and ack the interrupt directly from here */
> - mbox_fifo_read(mbox);
> - ack_mbox_irq(mbox, IRQ_RX);
> - }
> + /* we must read and ack the interrupt directly from here */
> + mbox_fifo_read(mbox);
> + ack_mbox_irq(mbox, IRQ_RX);
>
> - return ret;
> + return 0;
> }
Is n't the interrupt supposed to be IRQ_TX above? i.e TX ready signal?

2024-04-01 23:50:19

by Andrew Davis

Subject: Re: [PATCH 12/13] mailbox: omap: Reverse FIFO busy check logic

On 4/1/24 6:31 PM, Hari Nagalla wrote:
> On 3/25/24 12:20, Andrew Davis wrote:
>>   static int omap_mbox_chan_send_noirq(struct omap_mbox *mbox, u32 msg)
>>   {
>> -    int ret = -EBUSY;
>> +    if (mbox_fifo_full(mbox))
>> +        return -EBUSY;
>> -    if (!mbox_fifo_full(mbox)) {
>> -        omap_mbox_enable_irq(mbox, IRQ_RX);
>> -        mbox_fifo_write(mbox, msg);
>> -        ret = 0;
>> -        omap_mbox_disable_irq(mbox, IRQ_RX);
>> +    omap_mbox_enable_irq(mbox, IRQ_RX);
>> +    mbox_fifo_write(mbox, msg);
>> +    omap_mbox_disable_irq(mbox, IRQ_RX);
>> -        /* we must read and ack the interrupt directly from here */
>> -        mbox_fifo_read(mbox);
>> -        ack_mbox_irq(mbox, IRQ_RX);
>> -    }
>> +    /* we must read and ack the interrupt directly from here */
>> +    mbox_fifo_read(mbox);
>> +    ack_mbox_irq(mbox, IRQ_RX);
>> -    return ret;
>> +    return 0;
>>   }
> Is n't the interrupt supposed to be IRQ_TX above? i.e TX ready signal?

Hmm, could be, but this patch doesn't actually change anything, only moves code
around for readability. So if we were are ack'ing the wrong interrupt, then it
was wrong before. We should check that and fix it if needed in a follow up patch.

Andrew