Re: [PATCH 2/8] platform/x86/intel/sdsi: Combine read and write mailbox flows

["David E. Box" <david.e.box@linux.intel.com>]

Hi Sathya,

On Thu, 2024-02-01 at 09:31 -0800, Kuppuswamy Sathyanarayanan wrote:
> 
> On 1/31/24 5:07 PM, David E. Box wrote:
> > The current mailbox commands are either read-only or write-only and the
> > flow is different for each. New commands will need to send and receive
> > data. In preparation for these commands, create a common polling function
> > to handle sending data and receiving in the same transaction.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++--------------
> >  1 file changed, 44 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/sdsi.c
> > b/drivers/platform/x86/intel/sdsi.c
> > index a70c071de6e2..05a35f2f85b6 100644
> > --- a/drivers/platform/x86/intel/sdsi.c
> > +++ b/drivers/platform/x86/intel/sdsi.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/overflow.h>
> >  #include <linux/pci.h>
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> > @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status)
> >         }
> >  }
> >  
> > -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info
> > *info,
> > -                             size_t *data_size)
> > +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info
> > *info,
> > +                         size_t *data_size)
> >  {
> >         struct device *dev = priv->dev;
> >         u32 total, loop, eom, status, message_size;
> > @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv,
> > struct sdsi_mbox_info *inf
> >  
> >         lockdep_assert_held(&priv->mb_lock);
> >  
> > -       /* Format and send the read command */
> > -       control = FIELD_PREP(CTRL_EOM, 1) |
> > -                 FIELD_PREP(CTRL_SOM, 1) |
> > -                 FIELD_PREP(CTRL_RUN_BUSY, 1) |
> > -                 FIELD_PREP(CTRL_PACKET_SIZE, info->size);
> > -       writeq(control, priv->control_addr);
> > -
> >         /* For reads, data sizes that are larger than the mailbox size are
> > read in packets. */
> >         total = 0;
> >         loop = 0;
> >         do {
> > -               void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop);
> >                 u32 packet_size;
> >  
> >                 /* Poll on ready bit */
> > @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv,
> > struct sdsi_mbox_info *inf
> >                 if (ret)
> >                         break;
> >  
> > +               if (!packet_size) {
> > +                       sdsi_complete_transaction(priv);
> > +                       break;
> > +               }
> > +
> 
> It seems to be a generic check. Is this related to converting to a read/write
> function or
> a common fix you added together in this patch.

Yes, it's related. The code that follows this only applies to reads. This check
would be true only for writes which only need to poll once to confirm the write
was successful. I'll add a comment to make it clear.