Re: [RFC PATCH 4/6] spi: ti-qspi: Implement the spi_mem interface

Boris Brezillon
2018-02-12T16:08:35Z

On Mon, 12 Feb 2018 21:30:09 +0530
Vignesh R <vigneshr@ti.com> wrote:

> On 12-Feb-18 6:01 PM, Boris Brezillon wrote:
> > On Mon, 12 Feb 2018 17:13:55 +0530
> > Vignesh R <vigneshr@ti.com> wrote:
> >
> >> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:
> >> > From: Boris Brezillon <boris.brezillon@free-electrons.com>
> >> >
> >> > The spi_mem interface is meant to replace the spi_flash_read() one.
> >> > Implement the ->exec_op() method so that we can smoothly get rid of the
> >> > old interface.
> >> >
> >> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> >> > ---
> >> >  drivers/spi/spi-ti-qspi.c | 85 +++++++++++++++++++++++++++++++++++++++--------
> >> >  1 file changed, 72 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
> >> > index c24d9b45a27c..40cac3ef6cc9 100644
> >> > --- a/drivers/spi/spi-ti-qspi.c
> >> > +++ b/drivers/spi/spi-ti-qspi.c
> >>
> >> [...]
> >>
> >> > +static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> >> > +   .exec_op = ti_qspi_exec_mem_op,
> >>
> >>        .supports_op = ti_qspi_supports_mem_op,
> >>
> >> Its required as per spi_controller_check_ops() in Patch 1/6
> >
> > ->supports_op() is optional, and if it's missing, the core will do the
> > regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op()
> > implementation).
>
> You might have overlooked spi_controller_check_ops() from Patch 1/6:
> +static int spi_controller_check_ops(struct spi_controller *ctlr)
> +{
> + /*
> + * The controller can implement only the high-level SPI-memory
> + * operations if it does not support regular SPI transfers.
> + */
> + if (ctlr->mem_ops) {
> + if (!ctlr->mem_ops->supports_op ||
> + !ctlr->mem_ops->exec_op)
> + return -EINVAL;
> + } else if (!ctlr->transfer && !ctlr->transfer_one &&
> + !ctlr->transfer_one_message) {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
>
> So if ->supports_op() is not populated by SPI controller driver, then
> driver probe fails with -EINVAL. This is what I observed on my TI
> hardware when testing this patch series.

Correct. Then I should fix spi_controller_check_ops() to allow empty
ctlr->mem_ops->supports_op.

>
> > This being said, if you think a custom ->supports_op()
> > implementation is needed for this controller I can add one.
> >
>
> spi_mem_supports_op() should suffice for now if above issue is fixed.

Cool. IIUC, you tested the series on a TI SoC. Does it work as
expected? Do you see any perf regressions?

Regards,

Boris

--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering