From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.free-electrons.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1elGef-0001vo-1r for linux-mtd@lists.infradead.org; Mon, 12 Feb 2018 16:08:35 +0000 Date: Mon, 12 Feb 2018 17:08:09 +0100 From: Boris Brezillon To: Vignesh R Cc: David Woodhouse , Brian Norris , Boris Brezillon , Marek Vasut , Richard Weinberger , Cyrille Pitchen , "linux-mtd@lists.infradead.org" , Mark Brown , "linux-spi@vger.kernel.org" , Peter Pan , Frieder Schrempf , Yogesh Gaur , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Kamal Dasu Subject: Re: [RFC PATCH 4/6] spi: ti-qspi: Implement the spi_mem interface Message-ID: <20180212170809.3f0b5e20@bbrezillon> In-Reply-To: <67e61203-a2e9-853c-6cda-7226499611c2@ti.com> References: <20180205232120.5851-1-boris.brezillon@bootlin.com> <20180205232120.5851-5-boris.brezillon@bootlin.com> <6a9eaaaf-20a6-b332-03d0-9d16e24d0b3d@ti.com> <20180212133158.3032442f@bbrezillon> <67e61203-a2e9-853c-6cda-7226499611c2@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Mon, 12 Feb 2018 21:30:09 +0530 Vignesh R wrote: > On 12-Feb-18 6:01 PM, Boris Brezillon wrote: > > On Mon, 12 Feb 2018 17:13:55 +0530 > > Vignesh R wrote: > > =20 > >> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote: =20 > >> > From: Boris Brezillon > >> >=20 > >> > 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. > >> >=20 > >> > Signed-off-by: Boris Brezillon > >> > --- > >> >=C2=A0 drivers/spi/spi-ti-qspi.c | 85 +++++++++++++++++++++++++++++++= ++++++++-------- > >> >=C2=A0 1 file changed, 72 insertions(+), 13 deletions(-) > >> >=20 > >> > 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=C2=A0 =20 > >>=20 > >> [...] > >> =20 > >> > +static const struct spi_controller_mem_ops ti_qspi_mem_ops =3D { > >> > +=C2=A0=C2=A0 .exec_op =3D ti_qspi_exec_mem_op,=C2=A0 =20 > >>=20 > >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .supports_op =3D ti_qspi_sup= ports_mem_op, > >>=20 > >> Its required as per spi_controller_check_ops() in Patch 1/6 =20 > > =20 > > ->supports_op() is optional, and if it's missing, the core will do the = =20 > > regular QuadSPI/DualSPI/SingleSPI check (see spi_mem_supports_op() > > implementation). =20 >=20 > 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; > +} > + >=20 > 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. >=20 > > This being said, if you think a custom ->supports_op() > > implementation is needed for this controller I can add one. > > =20 >=20 > 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 --=20 Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com