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

Miquel Raynal
2021-12-15T17:45:29Z

Hi Boris,

boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100:

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

In order to keep the series easy to review I decided to go for the
following approach:
* Introduce the spi_mem_generic_supports_op_helper() which takes a
capabilities structure. This helper gathers all the checks from
spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These
two helpers now call the new one with either a NULL pointer in the
former case, or a structure with the .dtr parameter set to true in
the latter.
* Change the API of spi_mem_default_supports_op(), this involves
updating many different drivers so this change does only that in a
very transparent way, with no functional changes at all. All the
drivers provide a NULL parameter for the capabilities structure.
* Actually make use of the new parameter of
spi_mem_default_supports_op() in the drivers Cadence and Macronix,
which do have DTR support. This kills the spi_mem_dtr_supports_op()
helper.
* Kill the temporary spi_mem_generic_supports_op() helper by moving
all the logic back into spi_mem_default_supports_op().

This approach is really straightforward and easily bisectable if
needed. While working on this, I fixed the check we discussed on IRC
about the command parameter when in a DTR operation. I also reverted
the logic in the various checks, as you suggested.

Thanks,
Miquèl