Re: [PATCH v6, 5/8] mtd: m25p80: Let m25p80_read() fallback to spi transfer

From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
To: Kamal Dasu <kamal.dasu@broadcom.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	vigneshr@ti.com, Kamal Dasu <kdasu.kdev@gmail.com>,
	linux-spi@vger.kernel.org,
	Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>,
	Mark Brown <broonie@kernel.org>,
	linux-mtd@lists.infradead.org,
	bcm-kernel-feedback-list@broadcom.com,
	Jon Mason <jon.mason@broadcom.com>,
	Brian Norris <computersforpeace@gmail.com>
Subject: Re: [PATCH v6, 5/8] mtd: m25p80: Let m25p80_read() fallback to spi transfer
Date: Fri, 14 Oct 2016 15:17:44 +0200	[thread overview]
Message-ID: <ae2c984e-2bcf-380e-a257-dbe786973af1@atmel.com> (raw)
In-Reply-To: <CAKekbesq1deU_0TCV4STAi=o8jZwoc_u+9zkdZuEhmd+BGwU2A@mail.gmail.com>

Le 13/10/2016 à 23:15, Kamal Dasu a écrit :
> On Mon, Oct 10, 2016 at 4:29 AM, Cyrille Pitchen
> <cyrille.pitchen@atmel.com> wrote:
>> Hi all,
>>
>>
>> Le 10/10/2016 à 10:04, Florian Fainelli a écrit :
>>> On 08/24/2016 03:04 PM, Kamal Dasu wrote:
>>>> In m25p80_read() even though spi_flash_read() is supported
>>>> by some drivers, under certain circumstances like unaligned
>>>> buffer, address or address range limitations on certain SoCs
>>>> let it fallback to core spi reads. Such drivers are expected
>>>> to return -EAGAIN so that the m25p80_read() uses standard
>>>> spi transfer.
>>>>
>>>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>>>
>>> MTD folks, any comments on this?
>>>
>>>> ---
>>>>  drivers/mtd/devices/m25p80.c | 11 +++++++++--
>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>>> index 9cf7fcd..77c2d2c 100644
>>>> --- a/drivers/mtd/devices/m25p80.c
>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>> @@ -155,9 +155,16 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>              msg.data_nbits = m25p80_rx_nbits(nor);
>>>>
>>>>              ret = spi_flash_read(spi, &msg);
>>>> -            if (ret < 0)
>>>> +
>>>> +            if (ret >= 0)
>>>> +                    return msg.retlen;
>>>> +
>>>> +            /*
>>>> +             * some spi master drivers might need to fallback to
>>>> +             * normal spi transfer
>>>> +             */
>>>> +            if (ret != -EAGAIN)
>> I just wonder whether EINVAL would be a better choice.
> 
>  spi_flash_read calls the down stream controller driver with all
> params addresses however  accelerated transfer is not possible by the
> controller due to alignment issues, it needs to indicate to m25p call
> to try the normal transfer, hence use of EAGAIN seemed appropriate.
> 
>

Yes, I think I've understood the purpose of this patch. In the example you
gave, the actual implementation of spi_flash_read() works fine with aligned
addresses but doesn't support unaligned addresses. Hence, such unaligned
addresses are invalid argument for spi_flash_read() and we should fall back
to the legacy implementation of m25p80_read().

My point is just that EINVAL clearly tells that the SPI controller driver
implementation of spi_flash_read() doesn't support the given input
parameters, here an unaligned address, whereas EAGAIN suggests that some
hardware resource is temporarily unavailable and we could call spi_flash_read()
again later. However, in this case, spi_flash_read() would still fail even if
called later.

That's why I've suggested EINVAL might have been a better choice than EAGAIN,
but honestly it's not a big deal, only a detail. So if most people prefer to
keep EAGAIN, I'm perfectly fine with it! :)

I don't want my comment to delay the integration of this patch.

>>>>                      return ret;
>>>> -            return msg.retlen;
>>>>      }
>>>>
>>>>      spi_message_init(&m);
>>>>
>>>
>>
>> Best regards,
>>
>> Cyrille

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/