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

[Sean Anderson <seanga2@gmail.com>]

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
[2] https://lore.kernel.org/lkml/CA+55aFwVZk1OfB9T2v014PTAKFhtVan_Zj2dOjnCy3x6E4UJfA at mail.gmail.com/