Re: [PATCH v5 2/5] mbox: add polarfire soc system controller mailbox

[Jassi Brar <jassisinghbrar@gmail.com>]

On Mon, Apr 12, 2021 at 11:04 AM <conor.dooley@microchip.com> 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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv