On 02/04/21 06:31PM, Sean Anderson wrote: > On 4/1/21 3:31 PM, Pratyush Yadav wrote: > > spi_mem_default_supports_op() rejects DTR ops by default to ensure that > > the controller drivers that haven't been updated with DTR support > > continue to reject them. It also makes sure that controllers that don't > > support DTR mode at all (which is most of them at the moment) also > > reject them. > > > > This means that controller drivers that want to support DTR mode can't > > use spi_mem_default_supports_op(). Driver authors have to roll their own > > supports_op() function and mimic the buswidth checks. See > > spi-cadence-quadspi.c for example. Or even worse, driver authors might > > skip it completely or get it wrong. > > > > Add spi_mem_dtr_supports_op(). It provides a basic sanity check for DTR > > ops and performs the buswidth requirement check. Move the logic for > > checking buswidth in spi_mem_default_supports_op() to a separate > > function so the logic is not repeated twice. > > > > Signed-off-by: Pratyush Yadav <p.ya...@ti.com> > > --- > > drivers/spi/spi-mem.c | 22 +++++++++++++++++++--- > > include/spi-mem.h | 2 ++ > > 2 files changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > > index 541cd0e5a7..be1737a2c6 100644 > > --- a/drivers/spi/spi-mem.c > > +++ b/drivers/spi/spi-mem.c > > @@ -145,8 +145,8 @@ static int spi_check_buswidth_req(struct spi_slave > > *slave, u8 buswidth, bool tx) > > return -ENOTSUPP; > > } > > -bool spi_mem_default_supports_op(struct spi_slave *slave, > > - const struct spi_mem_op *op) > > +static bool spi_mem_check_buswidth(struct spi_slave *slave, > > + const struct spi_mem_op *op) > > { > > if (spi_check_buswidth_req(slave, op->cmd.buswidth, true)) > > return false; > > @@ -164,13 +164,29 @@ bool spi_mem_default_supports_op(struct spi_slave > > *slave, > > op->data.dir == SPI_MEM_DATA_OUT)) > > return false; > > + return true; > > +} > > + > > +bool spi_mem_dtr_supports_op(struct spi_slave *slave, > > + const struct spi_mem_op *op) > > +{ > > + if (op->cmd.nbytes != 2) > > + return false; > > Why does the command bytes need to be 2?
They need to be 2 if the command buswidth is 8 because otherwise the command phase will only take up half a cycle. This should have been if (op->cmd.buswidth == 8 && op->cmd.nbytes != 2). > > --Sean > > > + > > + return spi_mem_check_buswidth(slave, op); > > +} > > +EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op); > > + > > +bool spi_mem_default_supports_op(struct spi_slave *slave, > > + const struct spi_mem_op *op) > > +{ > > if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr) > > return false; > > if (op->cmd.nbytes != 1) > > return false;> - return true; > > + return spi_mem_check_buswidth(slave, op); > > } > > EXPORT_SYMBOL_GPL(spi_mem_default_supports_op); > > diff --git a/include/spi-mem.h b/include/spi-mem.h > > index dc53b517c1..37a9128c5b 100644 > > --- a/include/spi-mem.h > > +++ b/include/spi-mem.h > > @@ -249,6 +249,8 @@ spi_controller_dma_unmap_mem_op_data(struct > > spi_controller *ctlr, > > int spi_mem_adjust_op_size(struct spi_slave *slave, struct spi_mem_op > > *op); > > bool spi_mem_supports_op(struct spi_slave *slave, const struct spi_mem_op > > *op); > > +bool spi_mem_dtr_supports_op(struct spi_slave *slave, > > + const struct spi_mem_op *op); > > bool spi_mem_default_supports_op(struct spi_slave *slave, > > const struct spi_mem_op *op); > > > -- Regards, Pratyush Yadav Texas Instruments Inc.