RE: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Chuanhua Han <chuanhua.han@xxxxxxx>]

HI,Boris,

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> Sent: 2018年9月28日 14:45
> To: Chuanhua Han <chuanhua.han@xxxxxxx>
> Cc: broonie@xxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; eha@xxxxxxxx
> Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function
> 
> Hi Chuanhua,
> 
> On Fri, 21 Sep 2018 15:06:26 +0800
> Chuanhua Han <chuanhua.han@xxxxxxx> wrote:
> 
> > Before we add this spi_transfer to the spi_message chain table, we
> > need bits_per_word_mask based on spi_control to set the bits_per_word
> > of this spi_transfer.
> 
> It's not clear to me what you're trying to fix/improve. Can you give more
> details on what the problem is?
> 
> >
> > Signed-off-by: Chuanhua Han <chuanhua.han@xxxxxxx>
> > ---
> >  drivers/spi/spi-mem.c | 39
> +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index
> > eb72dba71d83..717e711c0952 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -175,6 +175,41 @@ bool spi_mem_supports_op(struct spi_mem
> *mem,
> > const struct spi_mem_op *op)  }
> > EXPORT_SYMBOL_GPL(spi_mem_supports_op);
> >
> > +/**
> > + * spi_set_xfer_bpw() - Set the bits_per_word for each transfer based on
> > + *			the bits_per_word_mask of the spi controller
> > + * @ctrl: the spi controller
> > + * @xfer: the spi transfer
> > + *
> > + * This function sets the bits_per_word for each transfer based on
> > +the spi
> > + * controller's bits_per_word_mask to improve the efficiency of spi
> transport.
> > + *
> > + * Return: 0 in case of success, a negative error code otherwise.
> > + */
> > +int spi_set_xfer_bpw(struct spi_controller *ctlr, struct spi_transfer
> > +*xfer) {
> > +	if (!ctlr || !xfer) {
> > +		dev_err(&ctlr->dev,
> > +			"Fail to set bits_per_word for spi transfer\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ctlr->bits_per_word_mask) {
> > +		if (!(xfer->len % 4)) {
> > +			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(32))
> > +				xfer->bits_per_word = 32;
> > +		} else if (!(xfer->len % 2)) {
> > +			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(16))
> > +				xfer->bits_per_word = 16;
> > +		} else {
> > +			xfer->bits_per_word = 8;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(spi_set_xfer_bpw);
> 
> Why is this function placed in spi-mem.c, and more importantly, why is it
> exported?
Since bits_per_word is not judged by spi every time it is transmitted, the code defaults to bits_per_word=8 so that bits_per_word cannot be implemented if it wants to transfer a specific spi controller, so it is flexible to assign bits_per_word according to the spi controller's bits_per_word_mask before each transfer spi. Ah, for which export, there is really no need to deliberately remove.
> 
> > +
> >  /**
> >   * spi_mem_exec_op() - Execute a memory operation
> >   * @mem: the SPI memory
> > @@ -252,6 +287,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
> >  	xfers[xferpos].tx_buf = tmpbuf;
> >  	xfers[xferpos].len = sizeof(op->cmd.opcode);
> >  	xfers[xferpos].tx_nbits = op->cmd.buswidth;
> > +	spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
> 
> It's still unclear why you need to specify a bits_per_word value, but if this is
> needed, it's probably something you want to add to spi.c, when a message is
> queued.
To specify a specific bits_per_word to be able to use the xspi (32bit) mode of the fsl_dspi module to transfer data, you can look at my PATCH 2/2.
 Do not add a value in spis.c that takes into account that the value assigned to bits_per_word is decided before the transfer. Thanks for your check and reply!
> 
> >  	spi_message_add_tail(&xfers[xferpos], &msg);
> >  	xferpos++;
> >  	totalxferlen++;
> > @@ -266,6 +302,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
> >  		xfers[xferpos].tx_buf = tmpbuf + 1;
> >  		xfers[xferpos].len = op->addr.nbytes;
> >  		xfers[xferpos].tx_nbits = op->addr.buswidth;
> > +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
> >  		spi_message_add_tail(&xfers[xferpos], &msg);
> >  		xferpos++;
> >  		totalxferlen += op->addr.nbytes;
> > @@ -276,6 +313,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;
> > +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
> >  		spi_message_add_tail(&xfers[xferpos], &msg);
> >  		xferpos++;
> >  		totalxferlen += op->dummy.nbytes;
> > @@ -291,6 +329,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
> >  		}
> >
> >  		xfers[xferpos].len = op->data.nbytes;
> > +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
> >  		spi_message_add_tail(&xfers[xferpos], &msg);
> >  		xferpos++;
> >  		totalxferlen += op->data.nbytes;