On Mon, Apr 12, 2021 at 11:04 AM <conor.dooley@xxxxxxxxxxxxx> wrote:
> diff --git a/drivers/mailbox/mailbox-mpfs.c b/drivers/mailbox/mailbox-mpfs.c
> +
> +struct mpfs_mbox {
> + struct mbox_controller controller;
> + struct device *dev;
> + int irq;
> + void __iomem *mbox_base;
> + void __iomem *int_reg;
> + struct mbox_chan *chans;
>
As I said previously .... struct mbox_chan chans[1] ?
> +static bool mpfs_mbox_busy(struct mpfs_mbox *mbox)
> +{
> + u32 status;
> +
> + status = readl_relaxed(mbox->mbox_base + SERVICES_SR_OFFSET);
> +
> + return status & SCB_STATUS_BUSY_MASK;
> +}
> +
> +static struct mpfs_mbox *mbox_chan_to_mpfs_mbox(struct mbox_chan *chan)
> +{
> + if (!chan)
> + return NULL;
> +
The return value is not always checked before use in the caller.
And I don't see how could 'chan' ever be null. So, maybe just discard
this function ?
> + return (struct mpfs_mbox *)chan->con_priv;
> +}
> +
> +static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> + u32 val = 0u;
> + u16 opt_sel;
> + u32 tx_trigger;
> + struct mpfs_mss_msg *msg = data;
> + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> +
> + mbox->response = msg->response;
> + mbox->resp_offset = msg->resp_offset;
> +
> + if (mpfs_mbox_busy(mbox))
> + return -EBUSY;
> +
This check should be unnecessary because the api won't call
send_data() unless the last one is done.
> +static inline bool mpfs_mbox_pending(struct mpfs_mbox *mbox)
> +{
> + u32 status;
> +
> + status = readl_relaxed(mbox->mbox_base + SERVICES_SR_OFFSET);
> +
> + return !(status & SCB_STATUS_BUSY_MASK);
> +}
> +
Isn't this !mpfs_mbox_busy() ?
> +static void mpfs_mbox_rx_data(struct mbox_chan *chan)
> +{
> + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> + u32 i;
> + u16 num_words = ALIGN((mbox->response->resp_size), (4)) / 4U;
> + struct mpfs_mss_response *response = mbox->response;
> +
Usually the decs are in ascending or descending order. Please choose one.
> +
> +static int mpfs_mbox_startup(struct mbox_chan *chan)
> +{
> + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan);
> + int ret = 0;
> +
> + if (!mbox)
> + return -EINVAL;
> + ret = devm_request_irq(mbox->dev, mbox->irq, mpfs_mbox_inbox_isr, 0, "mpfs-mailbox", chan);
>
scripts/checkpatch should catch missing newline before "ret =" ?
cheers.
