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

[Florian Fainelli <f.fainelli@xxxxxxxxx>]

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