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

Pratyush Yadav
2021-12-14T19:54:20Z

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@bootlin.com>
> ---
> 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.