Re: [PATCH v5 12/13] spi: mxic: Use spi_mem_generic_supports_op()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Tue, 14 Dec 2021 12:41:39 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> This driver can be simplified a little bit by using
> spi_mem_generic_supports_op() instead of the
> spi_mem_default/dtr_supports_op() couple. The all_false boolean is
> inverted to become a dtr boolean, which checks if at least one of the
> operation member uses dtr mode. The idea behind this change is to
> simplify the introduction of the pipelined ECC engine.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-mxic.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index 485a7f2afb44..5e71aa630504 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
>  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
>  				     const struct spi_mem_op *op)
>  {
> -	bool all_false;
> +	struct spi_mem_controller_caps caps = {};
>  
>  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
>  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
>  	if (op->addr.nbytes > 7)
>  		return false;
>  
> -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> -		    !op->data.dtr;
> +	caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;

Are you sure that's what you want to do? spi_mem_controller_caps is
supposed to encode the controller capabilities, not whether the
operation contains a DTR cycle or not. I'd expect this caps object to be
statically defined, with possibly one instance per-compat if the caps
depend on the HW revision.

>  
> -	if (all_false)
> -		return spi_mem_default_supports_op(mem, op);
> -	else
> -		return spi_mem_dtr_supports_op(mem, op);
> +	return spi_mem_generic_supports_op(mem, op, &caps);
>  }
>  
>  static int mxic_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)