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

Miquel Raynal
2021-12-15T16:19:29Z

Hi Pratyush,

p.yadav@ti.com wrote on Wed, 15 Dec 2021 01:23:17 +0530:

> 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.

Thanks!

>
> >
> > 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
> ...

Yup

>
> > + 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.

Following Boris' review I planned to kill spi_mem_dtr_supports_op(). I
might as well update spi_mem_default_supports_op() so that controllers
can provide their static capacities if any.

> > {
> > - 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
> >
>

Thanks,
Miquèl