From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Subject: Re: [PATCH v6, 5/8] mtd: m25p80: Let m25p80_read() fallback to spi transfer Date: Thu, 1 Dec 2016 16:45:01 +0100 Message-ID: <3260e7e5-fcf1-599a-e7b4-fd7aaef5be2e@gmail.com> References: <1472076269-4731-1-git-send-email-kdasu.kdev@gmail.com> <1472076269-4731-6-git-send-email-kdasu.kdev@gmail.com> <4b3b3d3e-b3f8-1d5b-65e3-0c37b6a29096@gmail.com> <8309370e-f99f-ed51-4789-c1d97d9d8191@atmel.com> <7697b2fa-4660-c791-e891-a22c8bc5390f@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: vigneshr-l0cyMroinI0@public.gmane.org, Kamal Dasu , linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Yendapally Reddy Dhananjaya Reddy , Mark Brown , linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org, Jon Mason , Brian Norris To: Cyrille Pitchen , Florian Fainelli , Kamal Dasu , Marek Vasut Return-path: In-Reply-To: Sender: linux-spi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: 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 >>>> 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 >>>>>> >>>>>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html