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

[Kamal Dasu <kdasu.kdev@xxxxxxxxx>]

Marek,

I will resend patch with s/EGAIN/EINVAL/ and also include the related
change in the broadcom controller driver as well.

Thnaks
Kamal

On Thu, Dec 1, 2016 at 10:45 AM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
> On 11/29/2016 03:06 PM, Cyrille Pitchen wrote:
>> Hi all,
>>
>> +Marek
>>
>> Le 29/11/2016 à 02:32, Florian Fainelli a écrit :
>>> On 10/14/2016 06:17 AM, Cyrille Pitchen wrote:
>>>> Le 13/10/2016 à 23:15, Kamal Dasu a écrit :
>>>>> On Mon, Oct 10, 2016 at 4:29 AM, Cyrille Pitchen
>>>>> <cyrille.pitchen@xxxxxxxxx> 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@xxxxxxxxx>
>>>>>>>
>>>>>>> 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.
>>>
>>> So what are we going to do now, should Kamal resubmit and
>>> s/EGAIN/EINVAL/ or is EAGAIN good enough? If EINVAL needs to be used,
>>> which I agree with you seems like a valid error code to return, this
>>> does imply changing the Broadcom QSPI driver though... so we have to
>>> coordinate the two changes to be merged through the same cycle to avoid
>>> regressions.
>>>
>>
>> Please replace the EAGAIN error code by EINVAL then I agree to merge this
>> patch in the github spi-nor tree.
>>
>> I know the EAGAIN error code has already been introduced in spi-bcm-qspi.c
>> through the spi tree but since currently m25p280.c handles neither EAGAIN
>> nor EINVAL as the returned code of spi_flash_read(), I guess no regression
>> will be introduced. The feature will work as expected once both the m25p80.c
>> and spi-bcm-qspi.c use the very same error code.
>>
>> Marek, any comment?
>
> I'm fine with this patch, but a patch for the broadcom controller would
> be great.
>
> --
> Best regards,
> Marek Vasut
--
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