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

5 Apr 2021

On Mon, Apr 05, 2021 at 07:27:56PM +0530, Pratyush Yadav wrote:
...
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@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_Zj2dOjnCy3x6E4...
Thanks for the history links Sean.  I think that in this case, if we're
following what the Linux kernel does, we're fine enough for now, but may
need to revisit later if we starting having size concerns.
In general yes, we shouldn't use bitfields, but follow Linux kernel for
the areas we try and keep mostly in sync.
-- 
Tom

    