Re: [PATCH v7 06/14] spi: spi-mem: Kill the spi_mem_dtr_supports_op() helper

Miquel Raynal
2021-12-21T09:58:50Z

Hi Pratyush,

p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:28:42 +0530:

> On 17/12/21 05:16PM, Miquel Raynal wrote:
> > Now that spi_mem_default_supports_op() has access to the static
> > controller capabilities (related to memory operations), and now that
> > these capabilities have been filled by the impacted controllers, there
> > is no need for a specific helper checking only DTR operations, so let's
> > just kill spi_mem_dtr_supports_op() and simply use
> > spi_mem_default_supports_op() instead.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > drivers/spi/spi-cadence-quadspi.c | 5 +----
> > drivers/spi/spi-mem.c | 10 ----------
> > drivers/spi/spi-mxic.c | 10 +---------
> > include/linux/spi/spi-mem.h | 11 -----------
> > 4 files changed, 2 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > index e137b1ec85d4..67d05ee8d6a0 100644
> > --- a/drivers/spi/spi-mxic.c
> > +++ b/drivers/spi/spi-mxic.c
> > @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
> > static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > const struct spi_mem_op *op)
> > {
> > - bool all_false;
> > -
> > if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> > op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> > return false;
> > @@ -348,13 +346,7 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > if (op->addr.nbytes > 7)
> > return false;
> >
> > - all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > - !op->data.dtr;
> > -
> > - if (all_false)
> > - return spi_mem_default_supports_op(mem, op);
> > - else
> > - return spi_mem_dtr_supports_op(mem, op);
> > + return spi_mem_default_supports_op(mem, op);
>
> Does this controller support mixed DTR modes? If it doesn't then it
> should reject mixed DTR ops before calling
> spi_mem_default_supports_op(). Anyway, the current driver doesn't check
> for it either so this change does not make anything worse at the very
> least.

The Cadence SPI driver does something like:

all_true = cmd.dtr && addr.dtr ...;
all_false = !cmd.dtr && !addr.dtr ...;
if (!all_true || !all_false)
return false;

This basically rules out any mixed DTR operation.

I believe Macronix code is inspired from this function, but they
intentionally dropped the all_true check, making the situation boolean:
either there is at least one DTR op, or there is none. So I believe
this was done on purpose and this controller supports mixed DTR ops.

> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Thanks,
Miquèl