| Message ID | 20210713125743.1540-3-a-nandan@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: cadence-quadspi: Fix DTR op checks and timeout in SPI NAND write operations | expand |
On Tue, Jul 13, 2021 at 12:57:42PM +0000, Apurva Nandan wrote: > + f_pdata->dtr = op->cmd.dtr && > + (op->addr.dtr || !op->addr.nbytes) && > + (op->data.dtr || !op->data.nbytes); I'm not sure anyone reading this code is going to figure out what it's doing without thinking about it, the combination of writing the bytes check with a !, putting it after the check for .dtr and not having any comments is a bit obscure. Something like (op->addr.nbytes && op.addr.dtr) might be a bit clearer, or a comment explicitly spelling it out.
On 14-Jul-21 12:09 AM, Mark Brown wrote: > On Tue, Jul 13, 2021 at 12:57:42PM +0000, Apurva Nandan wrote: > >> + f_pdata->dtr = op->cmd.dtr && >> + (op->addr.dtr || !op->addr.nbytes) && >> + (op->data.dtr || !op->data.nbytes); > > I'm not sure anyone reading this code is going to figure out what it's > doing without thinking about it, the combination of writing the bytes > check with a !, putting it after the check for .dtr and not having any > comments is a bit obscure. Something like > > (op->addr.nbytes && op.addr.dtr) > > might be a bit clearer, or a comment explicitly spelling it out. > Okay, I will add a comment explaining it, as other logic (with &&) won't deliver the behavior we expect. The logic it implements is: for an op to be dtr, if any phase has non-zero bytes, it must have dtr field set as true. So, it does p implies q: p->q = (!p || q) i.e. if phase has non-zero bytes (p) then check its dtr field (q). If all phases are empty, then follow what op.cmd.dtr says. Regards, Apurva Nandan
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index a2e1f4ce8b3e..a884678e8dff 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -322,7 +322,9 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE; f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE; f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; - f_pdata->dtr = op->data.dtr && op->cmd.dtr && op->addr.dtr; + f_pdata->dtr = op->cmd.dtr && + (op->addr.dtr || !op->addr.nbytes) && + (op->data.dtr || !op->data.nbytes); switch (op->data.buswidth) { case 0: @@ -1225,8 +1227,12 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem, { bool all_true, all_false; - all_true = op->cmd.dtr && op->addr.dtr && op->dummy.dtr && - op->data.dtr; + /* op->dummy.dtr is checked when converting nbytes into ncycles.*/ + all_true = op->cmd.dtr && + (op->addr.dtr || !op->addr.nbytes) && + (op->dummy.dtr || !op->dummy.nbytes) && + (op->data.dtr || !op->data.nbytes); + all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && !op->data.dtr;
buswidth and dtr fields in spi_mem_op are only valid when the corresponding spi_mem_op phase has a non-zero length. For example, SPI NAND core doesn't set buswidth when using SPI_MEM_OP_NO_ADDR phase. Fix the dtr checks in set_protocol() and suppports_mem_op() to ignore empty spi_mem_op phases, as checking for dtr field in empty phase will result in false negatives. Signed-off-by: Apurva Nandan <a-nandan@ti.com> --- drivers/spi/spi-cadence-quadspi.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)