On Fri, Jun 14, 2024 at 10:19:42AM +0100, Sudeep Holla wrote: > On Fri, May 10, 2024 at 11:19:48AM +0800, Peng Fan (OSS) wrote: > > There was some coding style error reported(unbalanced {}) which made me > look at the code again. I don't think we need to splat out error. > > > @@ -300,8 +326,30 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo, > > static void mailbox_clear_channel(struct scmi_chan_info *cinfo) > > { > > struct scmi_mailbox *smbox = cinfo->transport_info; > > + struct device *cdev = cinfo->dev; > > + struct mbox_chan *intr; > > + int ret; > > > > shmem_clear_channel(smbox->shmem); > > + > > + if (!shmem_channel_intr_enabled(smbox->shmem)) > > + return; > > + > > + if (smbox->chan_platform_receiver) > > + intr = smbox->chan_platform_receiver; > > + else if (smbox->chan) > > + intr = smbox->chan; > > + else { > > + dev_err(cdev, "Channel INTR wrongly set?\n"); > > + return; > > + } > > > > If it is OK I would like to fix it up with below change. > Hi, > Regards, > Sudeep > > -->8 > > diff --git i/drivers/firmware/arm_scmi/mailbox.c w/drivers/firmware/arm_scmi/mailbox.c > index adb69a6a0223..3bb3fba8f478 100644 > --- i/drivers/firmware/arm_scmi/mailbox.c > +++ w/drivers/firmware/arm_scmi/mailbox.c > @@ -326,30 +326,25 @@ static void mailbox_fetch_notification(struct scmi_chan_info *cinfo, > static void mailbox_clear_channel(struct scmi_chan_info *cinfo) > { > struct scmi_mailbox *smbox = cinfo->transport_info; > - struct device *cdev = cinfo->dev; > - struct mbox_chan *intr; > + struct mbox_chan *intr_chan = NULL; > int ret; > > shmem_clear_channel(smbox->shmem); > > - if (!shmem_channel_intr_enabled(smbox->shmem)) > - return; > - > if (smbox->chan_platform_receiver) > - intr = smbox->chan_platform_receiver; > + intr_chan = smbox->chan_platform_receiver; > else if (smbox->chan) > - intr = smbox->chan; > - else { > - dev_err(cdev, "Channel INTR wrongly set?\n"); > + intr_chan = smbox->chan; > + > + if (!(intr_chan && shmem_channel_intr_enabled(smbox->shmem))) > return; > - } Fine with dropping the dev_err() but is not this cumulative negated-if a bit cryptic...also you can bail out early straight away as before when platform has not required any P2A completion irq...I mean something like struct mbox_chan *intr_chan = NULL; shmem_clear_channel(smbox->shmem); if (!shmem_channel_intr_enabled(smbox->shmem)) return; if (smbox->chan_platform_receiver) intr_chan = smbox->chan_platform_receiver; else if (smbox->chan) intr_chan = smbox->chan; if (!intr_chan) return; (or just a dangling else return;) .. no strongs opinion here really, though. Thanks, Cristian