diff mbox

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

Message ID 1472076269-4731-6-git-send-email-kdasu.kdev@gmail.com
State Superseded
Delegated to: Cyrille Pitchen
Headers show

Commit Message

Kamal Dasu Aug. 24, 2016, 10:04 p.m. UTC 
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>
---
 drivers/mtd/devices/m25p80.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Florian Fainelli Oct. 10, 2016, 8:04 a.m. UTC | #1 
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)
>  			return ret;
> -		return msg.retlen;
>  	}
>  
>  	spi_message_init(&m);
>
Cyrille Pitchen Oct. 10, 2016, 8:29 a.m. UTC | #2 
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.
>>  			return ret;
>> -		return msg.retlen;
>>  	}
>>  
>>  	spi_message_init(&m);
>>
> 

Best regards,

Cyrille
Kamal Dasu Oct. 13, 2016, 9:15 p.m. UTC | #3 
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.


>>>                      return ret;
>>> -            return msg.retlen;
>>>      }
>>>
>>>      spi_message_init(&m);
>>>
>>
>
> Best regards,
>
> Cyrille
Cyrille Pitchen Oct. 14, 2016, 1:17 p.m. UTC | #4 
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
Florian Fainelli Nov. 29, 2016, 1:32 a.m. UTC | #5 
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@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.

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.
Cyrille Pitchen Nov. 29, 2016, 2:06 p.m. UTC | #6 
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@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.
> 
> 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?

Best regards,

Cyrille
Marek Vasut Dec. 1, 2016, 3:45 p.m. UTC | #7 
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@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.
>>
>> 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.
Kamal Dasu Jan. 20, 2017, 7:35 p.m. UTC | #8 
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@gmail.com> 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@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.
>>>
>>> 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
diff mbox

Patch

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)
 			return ret;
-		return msg.retlen;
 	}
 
 	spi_message_init(&m);