[PATCH v8 01/28] spi: spi-mem: allow specifying whether an op is DTR or not

[Pratyush Yadav]

On 05/04/21 09:12AM, Sean Anderson wrote:
> 
> On 4/5/21 7:47 AM, Tom Rini wrote:
> > On Mon, Apr 05, 2021 at 01:55:06PM +0530, Pratyush Yadav wrote:
> > > On 02/04/21 06:21PM, Sean Anderson wrote:
> > > > 
> > > > On 4/1/21 3:31 PM, Pratyush Yadav wrote:
> > > > > Each phase is given a separate 'dtr' field so mixed protocols like
> > > > > 4S-4D-4D can be supported.
> > > > > 
> > > > > Signed-off-by: Pratyush Yadav <p.yadav at ti.com>
> > > > > ---
> > > > >    drivers/spi/spi-mem.c | 3 +++
> > > > >    include/spi-mem.h     | 8 ++++++++
> > > > >    2 files changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > > > > index c095ae9505..427f7c13c5 100644
> > > > > --- a/drivers/spi/spi-mem.c
> > > > > +++ b/drivers/spi/spi-mem.c
> > > > > @@ -164,6 +164,9 @@ bool spi_mem_default_supports_op(struct spi_slave *slave,
> > > > >    				   op->data.dir == SPI_MEM_DATA_OUT))
> > > > >    		return false;
> > > > > +	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
> > > > > +		return false;
> > > > > +
> > > > >    	return true;
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
> > > > > diff --git a/include/spi-mem.h b/include/spi-mem.h
> > > > > index 8be3e2bf6b..9e6b044548 100644
> > > > > --- a/include/spi-mem.h
> > > > > +++ b/include/spi-mem.h
> > > > > @@ -71,6 +71,7 @@ enum spi_mem_data_dir {
> > > > >     * struct spi_mem_op - describes a SPI memory operation
> > > > >     * @cmd.buswidth: number of IO lines used to transmit the command
> > > > >     * @cmd.opcode: operation opcode
> > > > > + * @cmd.dtr: whether the command opcode should be sent in DTR mode or not
> > > > >     * @addr.nbytes: number of address bytes to send. Can be zero if the operation
> > > > >     *		 does not need to send an address
> > > > >     * @addr.buswidth: number of IO lines used to transmit the address cycles
> > > > > @@ -78,10 +79,13 @@ enum spi_mem_data_dir {
> > > > >     *	      Note that only @addr.nbytes are taken into account in this
> > > > >     *	      address value, so users should make sure the value fits in the
> > > > >     *	      assigned number of bytes.
> > > > > + * @addr.dtr: whether the address should be sent in DTR mode or not
> > > > >     * @dummy.nbytes: number of dummy bytes to send after an opcode or address. Can
> > > > >     *		  be zero if the operation does not require dummy bytes
> > > > >     * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes
> > > > > + * @dummy.dtr: whether the dummy bytes should be sent in DTR mode or not
> > > > >     * @data.buswidth: number of IO lanes used to send/receive the data
> > > > > + * @data.dtr: whether the data should be sent in DTR mode or not
> > > > >     * @data.dir: direction of the transfer
> > > > >     * @data.buf.in: input buffer
> > > > >     * @data.buf.out: output buffer
> > > > > @@ -90,21 +94,25 @@ struct spi_mem_op {
> > > > >    	struct {
> > > > >    		u8 buswidth;
> > > > >    		u8 opcode;
> > > > > +		u8 dtr : 1;
> > > > >    	} cmd;
> > > > >    	struct {
> > > > >    		u8 nbytes;
> > > > >    		u8 buswidth;
> > > > > +		u8 dtr : 1;
> > > > >    		u64 val;
> > > > >    	} addr;
> > > > >    	struct {
> > > > >    		u8 nbytes;
> > > > >    		u8 buswidth;
> > > > > +		u8 dtr : 1;
> > > > >    	} dummy;
> > > > >    	struct {
> > > > >    		u8 buswidth;
> > > > > +		u8 dtr : 1;
> > > > >    		enum spi_mem_data_dir dir;
> > > > >    		unsigned int nbytes;
> > > > >    		/* buf.{in,out} must be DMA-able. */
> > > > > 
> > > > 
> > > > I know this is following the Linux code, but are bitfields kosher for
> > > > U-Boot? This is more of a general question than a specific critique of
> > > > this code.
> > > 
> > > I'm not sure. I did a quick grep and I do see some usages of bitfields
> > > but I'm not sure if these just slipped in with ports from Linux or they
> > > are "officially" allowed.
> > 
> > We try to be a friendly environment to people used to working on the
> > Linux kernel, so is there a reason we wouldn't allow bitfields?
> > 
> 
> Linus has historically [1] gone on the record against the use of
> bitfields, primarily due to portability and optimization concerns.
> However, he recently [2] has condoned them for some struct types. I was
> just interested in what the current stance was since there isn't (e.g.)
> a section in process/coding_style.rst about when to use bitfields. And
> of course, traditionally this sort of thing would be done with a flags
> member.
> 
> --Sean
> 
> [1] https://yarchive.net/comp/linux/bitfields.html

FWIW this is mostly about using bitfields to describe register layout, 
where reordering by the compiler would be a problem. In this case though 
it is a kernel/u-boot private data structure and any amount of 
reordering should make no difference at all.

I'm not sure if there are any other pitfalls with bitfields but they are 
certainly more convenient to use compared to a 'flags' field.

> [2] https://lore.kernel.org/lkml/CA+55aFwVZk1OfB9T2v014PTAKFhtVan_Zj2dOjnCy3x6E4UJfA@mail.gmail.com/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.