Re: [PATCH v4 05/12] spi: spi-mem: Export the spi_mem_generic_supports_op() helper

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Thu,  9 Dec 2021 18:40:39 +0100
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> The combination of checks against the number of supported operations is
> going to increase exponentially each time we add a new parameter. So far
> we only had a dtr parameter. Now we are introducing an ECC parameter. We
> need to make this helper available for drivers with specific needs,
> instead of creating another set of helpers each time we want to check
> something new. In the future if we see that many different drivers use
> the same parameter values, we might be tempted to create a specific
> helper for that. But for now, let's just make the generic one available.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
>  drivers/spi/spi-mem.c       |  7 ++++---
>  include/linux/spi/spi-mem.h | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 9e06cd918273..48b55395178f 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -160,9 +160,9 @@ 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,
> -					bool dtr, bool ecc)
> +bool spi_mem_generic_supports_op(struct spi_mem *mem,
> +				 const struct spi_mem_op *op,
> +				 bool dtr, bool ecc)

Looks like you're replacing a maintenance burden by another. Every time
you'll add a new capability, you'll have to patch all the wrappers and
the drivers using this generic function to pass the extra parameter.
How about passing a

	struct spi_mem_controller_caps {
		bool dtr;
		bool ecc;
	};

or making the caps a bitmask, such that next time you add a new cap, you
can just assume all drivers have it set to 0 by default.

>  {
>  	if (!dtr) {
>  		if (op->cmd.dtr || op->addr.dtr ||
> @@ -183,6 +183,7 @@ static bool spi_mem_generic_supports_op(struct spi_mem *mem,
>  
>  	return spi_mem_check_buswidth(mem, op);
>  }
> +EXPORT_SYMBOL_GPL(spi_mem_generic_supports_op);
>  
>  bool spi_mem_dtr_supports_op(struct spi_mem *mem,
>  			     const struct spi_mem_op *op)
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index 3be594be24c0..07f637cbe77d 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -320,6 +320,10 @@ void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
>  					  const struct spi_mem_op *op,
>  					  struct sg_table *sg);
>  
> +bool spi_mem_generic_supports_op(struct spi_mem *mem,
> +				 const struct spi_mem_op *op,
> +				 bool dtr, bool ecc);
> +
>  bool spi_mem_default_supports_op(struct spi_mem *mem,
>  				 const struct spi_mem_op *op);
>  
> @@ -327,6 +331,7 @@ bool spi_mem_dtr_supports_op(struct spi_mem *mem,
>  			     const struct spi_mem_op *op);
>  
>  #else
> +
>  static inline int
>  spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,
>  				   const struct spi_mem_op *op,
> @@ -342,6 +347,14 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
>  {
>  }
>  
> +static inline
> +bool spi_mem_generic_supports_op(struct spi_mem *mem,
> +				 const struct spi_mem_op *op,
> +				 bool dtr, bool ecc)
> +{
> +	return false;
> +}
> +
>  static inline
>  bool spi_mem_default_supports_op(struct spi_mem *mem,
>  				 const struct spi_mem_op *op)