Re: [PATCH v3 5/5] spi: spi-geni-qcom: Don't keep a local state variable

From: Doug Anderson
Date: Thu Jun 18 2020 - 11:06:10 EST


Hi,

On Wed, Jun 17, 2020 at 2:16 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Douglas Anderson (2020-06-16 03:40:50)
> > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > index 63a62548b078..6feea88d63ac 100644
> > --- a/drivers/spi/spi-geni-qcom.c
> > +++ b/drivers/spi/spi-geni-qcom.c
> > @@ -63,13 +63,6 @@
> > #define TIMESTAMP_AFTER BIT(3)
> > #define POST_CMD_DELAY BIT(4)
> >
> > -enum spi_m_cmd_opcode {
> > - CMD_NONE,
> > - CMD_XFER,
> > - CMD_CS,
> > - CMD_CANCEL,
> > -};
> > -
> > struct spi_geni_master {
> > struct geni_se se;
> > struct device *dev;
> > @@ -81,10 +74,11 @@ struct spi_geni_master {
> > unsigned int tx_rem_bytes;
> > unsigned int rx_rem_bytes;
> > const struct spi_transfer *cur_xfer;
> > - struct completion xfer_done;
> > + struct completion cs_done;
> > + struct completion cancel_done;
> > + struct completion abort_done;
>
> I wonder if it would be better to use the wait_bit() APIs. That would
> let us have one word for various bits like CS_DONE, CANCEL_DONE,
> ABORT_DONE and then wake up the waiters when the particular bit happens
> to come in through the isr. It is probably over-engineering though
> because it saves a handful of bytes while increasing complexity.
>
> Otherwise I like this patch.

I'm going to leave it as-is for v4. We can always make it a future
improvement if we want.

-Doug