On Sun, 13 Dec 2020 10:54:26 +0100
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
> On Sat, 12 Dec 2020 09:28:50 -0800
> Sowjanya Komatineni <skomatineni@xxxxxxxxxx> wrote:
>
> > On 12/12/20 2:57 AM, Boris Brezillon wrote:
> > > On Fri, 11 Dec 2020 13:15:59 -0800
> > > Sowjanya Komatineni <skomatineni@xxxxxxxxxx> wrote:
> > >
> > >> This patch adds a flag SPI_MASTER_USES_HW_DUMMY_CYCLES for the controllers
> > >> that support transfer of dummy cycles by the hardware directly.
> > > Hm, not sure this is a good idea. I mean, if we expect regular SPI
> > > devices to use this feature, then why not, but if it's just for
> > > spi-mem, I'd recommend implementing a driver-specific exec_op() instead
> > > of using the default one.
> >
> > dummy cycles programming is SPI device specific.
> >
> > Transfer of dummy bytes by SW or HW controller can be depending on
> > features supported by controller.
> >
> > Adding controller driver specific exec_op() Just for skipping dummy
> > bytes transfer will have so much of redundant code pretty much what all
> > spi_mem_exec_op does.
> >
> > So in v1, I handled this in controller driver by skipping SW transfer of
> > dummy bytes during dummy phase and programming dummy cycles in
> > controller register to allow HW to transfer.
> >
> > Based on v1 feedback discussion, added this flag
> > SPI_MASTER_USES_HW_DUMMY_CYCLES which can be used by controllers
> > supporting HW dummy bytes transfer and updated spi_mem_exec_op to skip
> > SW dummy bytes.
> >
> > This helps other controllers supporting HW transfer of dummy bytes as
> > well just to set the flag and use dummy cycles directly.
>
> Except saying a spi_message has X dummy cycle is not precise enough.
> Where are those dummy cycles in the transfer sequence? spi-mem has well
> defined sequencing (cmd[+addr][+dummy][+data]) so we know exacly where
> dummy cycles are, but trying to retro-fit the dummy-cycle concept in
> the generic spi_message is confusing IMHO. If want to avoid code
> duplication, I'm pretty sure the driver can be reworked so the
> spi_transfer/exec_op() path can share most of the logic (that probably
> implies declaring a tegra_qspi_op).
Something like that might also do the trick:
--->8---
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index ef53290b7d24..8b0476f37fbb 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -353,6 +353,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1;
xfers[xferpos].len = op->dummy.nbytes;
xfers[xferpos].tx_nbits = op->dummy.buswidth;
+ xfers[xferpos].dummy_data = 1;
spi_message_add_tail(&xfers[xferpos], &msg);
xferpos++;
totalxferlen += op->dummy.nbytes;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 99380c0825db..ecf7989318c5 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -807,6 +807,10 @@ extern void spi_res_release(struct spi_controller *ctlr,
* transfer. If 0 the default (from @spi_device) is used.
* @bits_per_word: select a bits_per_word other than the device default
* for this transfer. If 0 the default (from @spi_device) is used.
+ * @dummy_data: set to 1 for a dummy transfer (a transfer whose data is
+ * ignored). Controllers that are able to issue dummy cycles can ignore
+ * tx_buf, for those that can't tx_buf will contain dummy bytes. The
+ * number of dummy cycles to issue is (len * tx_bits) / 8.
* @cs_change: affects chipselect after this transfer completes
* @cs_change_delay: delay between cs deassert and assert when
* @cs_change is set and @spi_transfer is not the last in @spi_message
@@ -919,6 +923,7 @@ struct spi_transfer {
struct sg_table tx_sg;
struct sg_table rx_sg;
+ unsigned dummy_data:1;
unsigned cs_change:1;
unsigned tx_nbits:3;
unsigned rx_nbits:3;
