On Fri, Jan 3, 2025 at 12:30 PM Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:
>
> On Sun, Dec 29, 2024 at 9:46 PM Karl.Li <karl.li@xxxxxxxxxxxx> wrote:
> ....
> > +
> > +static irqreturn_t mtk_apu_mailbox_irq(int irq, void *data)
> > +{
> > + struct mbox_chan *chan = data;
> > + struct mtk_apu_mailbox *apu_mbox = get_mtk_apu_mailbox(chan->mbox);
> > + struct mbox_chan *link = &apu_mbox->mbox.chans[0];
> > + u8 data_cnt = fls(readl(apu_mbox->regs + MTK_APU_MBOX_OUTBOX_IRQ));
> > +
> > + memcpy_fromio(apu_mbox->msgs.data, apu_mbox->regs + MTK_APU_MBOX_OUTBOX,
> > + sizeof(*apu_mbox->msgs.data) * data_cnt);
> > +
> > + mbox_chan_received_data(link, apu_mbox->msgs.data);
> > +
> > + return IRQ_WAKE_THREAD;
> > +}
> > +
> You don't seem to do anything that 'ack' the irq line. So if you merge
> this into mtk_apu_mailbox_irq_thread() and do
> devm_request_threaded_irq(dev, irq, NULL,
> mtk_apu_mailbox_irq_thread, IRQF_ONESHOT, ...);
> you can avoid adding a new api and keep your client code simple.
> mbox_chan_received_data() is strictly between the client and
> controller driver. If it is not called from an atomic context, feel
> free to do memcpy() from it.
That sounds great. Could we perhaps update the API documentation and
drop the "atomic" requirement, or update it to something you describe
here as "between the client and controller driver".
Disclosure: I've been doing pre-reviews for this series and we were
operating under the assumption that mbox_chan_received_data() and
its callbacks must work under atomic contexts.
Thanks
ChenYu
