| Message ID | 1472076269-4731-6-git-send-email-kdasu.kdev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)