[PATCH v8 04/28] spi: spi-mem: add spi_mem_dtr_supports_op()

[Sean Anderson]

On 4/5/21 3:40 AM, Pratyush Yadav wrote:
> On 02/04/21 06:31PM, Sean Anderson wrote:
>> On 4/1/21 3:31 PM, Pratyush Yadav wrote:
>>> spi_mem_default_supports_op() rejects DTR ops by default to ensure that
>>> the controller drivers that haven't been updated with DTR support
>>> continue to reject them. It also makes sure that controllers that don't
>>> support DTR mode at all (which is most of them at the moment) also
>>> reject them.
>>>
>>> This means that controller drivers that want to support DTR mode can't
>>> use spi_mem_default_supports_op(). Driver authors have to roll their own
>>> supports_op() function and mimic the buswidth checks. See
>>> spi-cadence-quadspi.c for example. Or even worse, driver authors might
>>> skip it completely or get it wrong.
>>>
>>> Add spi_mem_dtr_supports_op(). It provides a basic sanity check for DTR
>>> ops and performs the buswidth requirement check. Move the logic for
>>> checking buswidth in spi_mem_default_supports_op() to a separate
>>> function so the logic is not repeated twice.
>>>
>>> Signed-off-by: Pratyush Yadav <p.yadav at ti.com>
>>> ---
>>>    drivers/spi/spi-mem.c | 22 +++++++++++++++++++---
>>>    include/spi-mem.h     |  2 ++
>>>    2 files changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>>> index 541cd0e5a7..be1737a2c6 100644
>>> --- a/drivers/spi/spi-mem.c
>>> +++ b/drivers/spi/spi-mem.c
>>> @@ -145,8 +145,8 @@ static int spi_check_buswidth_req(struct spi_slave *slave, u8 buswidth, bool tx)
>>>    	return -ENOTSUPP;
>>>    }
>>> -bool spi_mem_default_supports_op(struct spi_slave *slave,
>>> -				 const struct spi_mem_op *op)
>>> +static bool spi_mem_check_buswidth(struct spi_slave *slave,
>>> +				   const struct spi_mem_op *op)
>>>    {
>>>    	if (spi_check_buswidth_req(slave, op->cmd.buswidth, true))
>>>    		return false;
>>> @@ -164,13 +164,29 @@ bool spi_mem_default_supports_op(struct spi_slave *slave,
>>>    				   op->data.dir == SPI_MEM_DATA_OUT))
>>>    		return false;
>>> +	return true;
>>> +}
>>> +
>>> +bool spi_mem_dtr_supports_op(struct spi_slave *slave,
>>> +			     const struct spi_mem_op *op)
>>> +{
>>> +	if (op->cmd.nbytes != 2)
>>> +		return false;
>>
>> Why does the command bytes need to be 2?
> 
> They need to be 2 if the command buswidth is 8 because otherwise the
> command phase will only take up half a cycle. This should have been if
> (op->cmd.buswidth == 8 && op->cmd.nbytes != 2).

Perhaps the most correct then is

int width = cmd.dtr ? cmd.buswidth * 2 : cmd.buswidth;
if (cmd.nbytes % width) { ... }

>>
>> --Sean
>>
>>> +
>>> +	return spi_mem_check_buswidth(slave, op);
>>> +}
>>> +EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op);
>>> +
>>> +bool spi_mem_default_supports_op(struct spi_slave *slave,
>>> +				 const struct spi_mem_op *op)
>>> +{
>>>    	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
>>>    		return false;
>>>    	if (op->cmd.nbytes != 1)
>>>    		return false;>   -	return true;
>>> +	return spi_mem_check_buswidth(slave, op);
>>>    }
>>>    EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
>>> diff --git a/include/spi-mem.h b/include/spi-mem.h
>>> index dc53b517c1..37a9128c5b 100644
>>> --- a/include/spi-mem.h
>>> +++ b/include/spi-mem.h
>>> @@ -249,6 +249,8 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
>>>    int spi_mem_adjust_op_size(struct spi_slave *slave, struct spi_mem_op *op);
>>>    bool spi_mem_supports_op(struct spi_slave *slave, const struct spi_mem_op *op);
>>> +bool spi_mem_dtr_supports_op(struct spi_slave *slave,
>>> +			     const struct spi_mem_op *op);
>>>    bool spi_mem_default_supports_op(struct spi_slave *slave,
>>>    				 const struct spi_mem_op *op);
>>>
>>
>