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

[Vignesh R <vigneshr@xxxxxx>]

On 12-Feb-18 6:01 PM, Boris Brezillon wrote:
> On Mon, 12 Feb 2018 17:13:55 +0530
> Vignesh R <vigneshr@xxxxxx> wrote:
> 
>> On Tuesday 06 February 2018 04:51 AM, Boris Brezillon wrote:
>> > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
>> > 
>> > 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@xxxxxxxxxxxxxxxxxx>
>> > ---
>> >  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.

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

Regards
Vignesh

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html