Re: [PATCH v5 04/13] spi: spi-mem: Create a helper to gather all the supports_op checks

[Pratyush Yadav <p.yadav@xxxxxx>]

Hi Miquel,

On 14/12/21 12:41PM, Miquel Raynal wrote:
> So far we check the support for:
> - regular operations
> - dtr operations
> Soon, we will also need to check the support for ECC operations.
> 
> As the combinatorial will increase exponentially, let's gather all the
> checks in a single generic function which takes a capabilities structure
> as input. This new helper is supposed to be called by the currently
> exported functions instead of repeating a similar implementation.

Nice! I think this is a very good idea.

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
>  drivers/spi/spi-mem.c       | 34 +++++++++++++++++++++++++---------
>  include/linux/spi/spi-mem.h |  8 ++++++++
>  2 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 37f4443ce9a0..4c6944d7b174 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -160,26 +160,42 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
>  	return true;
>  }
>  
> +static bool spi_mem_generic_supports_op(struct spi_mem *mem,
> +					const struct spi_mem_op *op,
> +					struct spi_mem_controller_caps *caps)
> +{
> +	if (!caps->dtr) {

Nitpick: Please turn the order of the if-else around:

  if (caps->dtr)
    ...
  else
    ...

> +		if (op->cmd.dtr || op->addr.dtr ||
> +		    op->dummy.dtr || op->data.dtr)
> +			return false;
> +
> +		if (op->cmd.nbytes != 1)
> +			return false;
> +	} else {
> +		if (op->cmd.nbytes != 2)
> +			return false;
> +	}
> +
> +	return spi_mem_check_buswidth(mem, op);
> +}
> +
>  bool spi_mem_dtr_supports_op(struct spi_mem *mem,
>  			     const struct spi_mem_op *op)
>  {
> -	if (op->cmd.nbytes != 2)
> -		return false;
> +	struct spi_mem_controller_caps caps = {
> +		.dtr = true,
> +	};
>  
> -	return spi_mem_check_buswidth(mem, op);
> +	return spi_mem_generic_supports_op(mem, op, &caps);
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op);
>  
>  bool spi_mem_default_supports_op(struct spi_mem *mem,
>  				 const struct spi_mem_op *op)

I know this would require a little bit more work from you, but can we 
make spi_mem_default_supports_op() accept the caps as a parameter? And 
drop spi_mem_dtr_supports_op() while we are at it? This would be a lot 
neater I think since we won't have lots of wrapper functions lying 
around.

>  {
> -	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
> -		return false;
> +	struct spi_mem_controller_caps caps = {};
>  
> -	if (op->cmd.nbytes != 1)
> -		return false;
> -
> -	return spi_mem_check_buswidth(mem, op);
> +	return spi_mem_generic_supports_op(mem, op, &caps);
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
>  
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index 85e2ff7b840d..f365efcfb719 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -220,6 +220,14 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
>  	return mem->drvpriv;
>  }
>  
> +/**
> + * struct spi_mem_controller_caps - SPI memory controller capabilities
> + * @dtr: Supports DTR operations
> + */
> +struct spi_mem_controller_caps {
> +	bool dtr;
> +};
> +
>  /**
>   * struct spi_controller_mem_ops - SPI memory operations
>   * @adjust_op_size: shrink the data xfer of an operation to match controller's
> -- 
> 2.27.0
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.