| Message ID | 20200312113154.GC20562@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1a421ebab6bb5bf65001743ba9fef48e94fb345a |
| Headers | show |
| Series | spi: spi-nxp-fspi: Fix a NULL vs IS_ERR() check in probe | expand |
On Thu, Mar 12, 2020 at 6:32 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > The platform_get_resource_byname() function returns NULL on error, it > doesn't return error pointers. > > Fixes: d166a73503ef ("spi: fspi: dynamically alloc AHB memory") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > The commit message for commit d166a73503ef ("spi: fspi: dynamically > alloc AHB memory") is not very good. Why is it necessary to allocate > the AHB memory dynamically instead of during probe? Also I suspect that > Adam should have recieved authorship credit for that patch. It wasn't my patch, I just pulled it in from NXP's repo. The true author is Han Xu. When I pulled in the series from NXP, I found the flexSPI on the i.MX8MM to become functional, and my company has a board with a qspi flash on it. adam > > drivers/spi/spi-nxp-fspi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c > index 019f40e2917c..1ccda82da206 100644 > --- a/drivers/spi/spi-nxp-fspi.c > +++ b/drivers/spi/spi-nxp-fspi.c > @@ -1019,8 +1019,8 @@ static int nxp_fspi_probe(struct platform_device *pdev) > > /* find the resources - controller memory mapped space */ > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "fspi_mmap"); > - if (IS_ERR(res)) { > - ret = PTR_ERR(res); > + if (!res) { > + ret = -ENODEV; > goto err_put_ctrl; > } > > -- > 2.20.1 >
Hi Dan, On Thu, Mar 12, 2020 at 8:33 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > The platform_get_resource_byname() function returns NULL on error, it > doesn't return error pointers. > > Fixes: d166a73503ef ("spi: fspi: dynamically alloc AHB memory") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > The commit message for commit d166a73503ef ("spi: fspi: dynamically > alloc AHB memory") is not very good. Why is it necessary to allocate > the AHB memory dynamically instead of during probe? Also I suspect that Agreed and I made the same comment during review: https://patchwork.kernel.org/patch/11361581/ Thanks
On Thu, Mar 12, 2020 at 09:11:44AM -0300, Fabio Estevam wrote: > On Thu, Mar 12, 2020 at 8:33 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > The commit message for commit d166a73503ef ("spi: fspi: dynamically > > alloc AHB memory") is not very good. Why is it necessary to allocate > > the AHB memory dynamically instead of during probe? Also I suspect that > Agreed and I made the same comment during review: > https://patchwork.kernel.org/patch/11361581/ Indeed. There had been an earlier discussion of what it was doing IIRC but it didn't make it's way into the changelog.
On Thu, Mar 12, 2020 at 06:58:31AM -0500, Adam Ford wrote: > On Thu, Mar 12, 2020 at 6:32 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > The platform_get_resource_byname() function returns NULL on error, it > > doesn't return error pointers. > > > > Fixes: d166a73503ef ("spi: fspi: dynamically alloc AHB memory") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > The commit message for commit d166a73503ef ("spi: fspi: dynamically > > alloc AHB memory") is not very good. Why is it necessary to allocate > > the AHB memory dynamically instead of during probe? Also I suspect that > > Adam should have recieved authorship credit for that patch. > > It wasn't my patch, I just pulled it in from NXP's repo. The true > author is Han Xu. When I pulled in the series from NXP, I found the > flexSPI on the i.MX8MM to become functional, and my company has a > board with a qspi flash on it. You should have put your Signed-off-by after Han Xu's. They should be in chronological order so whoever handles a patch adds their S-o-b to the end. regards, dan carpenter
On Fri, Mar 13, 2020 at 04:16:44AM +0000, Ashish Kumar wrote: > This patch needs to drop, until further debugged, since flexspi is > shared ip it is not having desired result on few boards, but it is > needed for i.mx series, This is one of the comments from han xu. I > believe further modifications will be needed, if not commit msg and > other things can be update. Please send a patch doing the revert with a changelog explaining why it's needed. Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c index 019f40e2917c..1ccda82da206 100644 --- a/drivers/spi/spi-nxp-fspi.c +++ b/drivers/spi/spi-nxp-fspi.c @@ -1019,8 +1019,8 @@ static int nxp_fspi_probe(struct platform_device *pdev) /* find the resources - controller memory mapped space */ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "fspi_mmap"); - if (IS_ERR(res)) { - ret = PTR_ERR(res); + if (!res) { + ret = -ENODEV; goto err_put_ctrl; }
The platform_get_resource_byname() function returns NULL on error, it doesn't return error pointers. Fixes: d166a73503ef ("spi: fspi: dynamically alloc AHB memory") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- The commit message for commit d166a73503ef ("spi: fspi: dynamically alloc AHB memory") is not very good. Why is it necessary to allocate the AHB memory dynamically instead of during probe? Also I suspect that Adam should have recieved authorship credit for that patch. drivers/spi/spi-nxp-fspi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)