| Message ID | 940bbdfc5c67282ab461b9c82b55f18fc34c959d.1378965270.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday, September 12, 2013 2:55 PM, Michal Simek wrote: > > devm_iounmap is called automatically that's why remove it from the code > dev_set_drvdata(dev, NULL) is called by generic code > after device_release or on probe failure. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> Reviewed-by: Jingoo Han <jg1.han@samsung.com> > --- > drivers/video/xilinxfb.c | 28 ++++++---------------------- > 1 file changed, 6 insertions(+), 22 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/09/13 08:54, Michal Simek wrote: > @@ -394,16 +382,12 @@ static int xilinxfb_release(struct device *dev) > /* Turn off the display */ > xilinx_fb_out32(drvdata, REG_CTRL, 0); > > - /* Release the resources, as allocated based on interface */ > - if (drvdata->flags & BUS_ACCESS_FLAG) > - devm_iounmap(dev, drvdata->regs); > #ifdef CONFIG_PPC_DCR > - else > + /* Release the resources, as allocated based on interface */ > + if (!(drvdata->flags & BUS_ACCESS_FLAG)) > dcr_unmap(drvdata->dcr_host, drvdata->dcr_len); > #endif I might be mistaken, and it's not strictly part of this series, but shouldn't dcr_unmap be called somewhere else also, if the probe fails? Tomi
On 09/16/2013 11:51 AM, Tomi Valkeinen wrote: > On 12/09/13 08:54, Michal Simek wrote: > >> @@ -394,16 +382,12 @@ static int xilinxfb_release(struct device *dev) >> /* Turn off the display */ >> xilinx_fb_out32(drvdata, REG_CTRL, 0); >> >> - /* Release the resources, as allocated based on interface */ >> - if (drvdata->flags & BUS_ACCESS_FLAG) >> - devm_iounmap(dev, drvdata->regs); >> #ifdef CONFIG_PPC_DCR >> - else >> + /* Release the resources, as allocated based on interface */ >> + if (!(drvdata->flags & BUS_ACCESS_FLAG)) >> dcr_unmap(drvdata->dcr_host, drvdata->dcr_len); >> #endif > > I might be mistaken, and it's not strictly part of this series, but > shouldn't dcr_unmap be called somewhere else also, if the probe fails? yes. It should. Thanks, Michal
On 16/09/13 13:33, Michal Simek wrote: > On 09/16/2013 11:51 AM, Tomi Valkeinen wrote: >> On 12/09/13 08:54, Michal Simek wrote: >> >>> @@ -394,16 +382,12 @@ static int xilinxfb_release(struct device *dev) >>> /* Turn off the display */ >>> xilinx_fb_out32(drvdata, REG_CTRL, 0); >>> >>> - /* Release the resources, as allocated based on interface */ >>> - if (drvdata->flags & BUS_ACCESS_FLAG) >>> - devm_iounmap(dev, drvdata->regs); >>> #ifdef CONFIG_PPC_DCR >>> - else >>> + /* Release the resources, as allocated based on interface */ >>> + if (!(drvdata->flags & BUS_ACCESS_FLAG)) >>> dcr_unmap(drvdata->dcr_host, drvdata->dcr_len); >>> #endif >> >> I might be mistaken, and it's not strictly part of this series, but >> shouldn't dcr_unmap be called somewhere else also, if the probe fails? > > yes. It should. Do you want me to apply these patches as they are, or do you want to improve the series to include the dcr_unmap fix? Tomi
Hi Tomi, On 09/16/2013 12:34 PM, Tomi Valkeinen wrote: > On 16/09/13 13:33, Michal Simek wrote: >> On 09/16/2013 11:51 AM, Tomi Valkeinen wrote: >>> On 12/09/13 08:54, Michal Simek wrote: >>> >>>> @@ -394,16 +382,12 @@ static int xilinxfb_release(struct device *dev) >>>> /* Turn off the display */ >>>> xilinx_fb_out32(drvdata, REG_CTRL, 0); >>>> >>>> - /* Release the resources, as allocated based on interface */ >>>> - if (drvdata->flags & BUS_ACCESS_FLAG) >>>> - devm_iounmap(dev, drvdata->regs); >>>> #ifdef CONFIG_PPC_DCR >>>> - else >>>> + /* Release the resources, as allocated based on interface */ >>>> + if (!(drvdata->flags & BUS_ACCESS_FLAG)) >>>> dcr_unmap(drvdata->dcr_host, drvdata->dcr_len); >>>> #endif >>> >>> I might be mistaken, and it's not strictly part of this series, but >>> shouldn't dcr_unmap be called somewhere else also, if the probe fails? >> >> yes. It should. > > Do you want me to apply these patches as they are, or do you want to > improve the series to include the dcr_unmap fix? Sorry I have missed this email. Yes please apply it as is. I don't have ppc hw here to be able to test this change. Thanks, Michal
On 30/09/13 15:05, Michal Simek wrote: > Hi Tomi, > > On 09/16/2013 12:34 PM, Tomi Valkeinen wrote: >> On 16/09/13 13:33, Michal Simek wrote: >>> On 09/16/2013 11:51 AM, Tomi Valkeinen wrote: >>>> On 12/09/13 08:54, Michal Simek wrote: >>>> >>>>> @@ -394,16 +382,12 @@ static int xilinxfb_release(struct device *dev) >>>>> /* Turn off the display */ >>>>> xilinx_fb_out32(drvdata, REG_CTRL, 0); >>>>> >>>>> - /* Release the resources, as allocated based on interface */ >>>>> - if (drvdata->flags & BUS_ACCESS_FLAG) >>>>> - devm_iounmap(dev, drvdata->regs); >>>>> #ifdef CONFIG_PPC_DCR >>>>> - else >>>>> + /* Release the resources, as allocated based on interface */ >>>>> + if (!(drvdata->flags & BUS_ACCESS_FLAG)) >>>>> dcr_unmap(drvdata->dcr_host, drvdata->dcr_len); >>>>> #endif >>>> >>>> I might be mistaken, and it's not strictly part of this series, but >>>> shouldn't dcr_unmap be called somewhere else also, if the probe fails? >>> >>> yes. It should. >> >> Do you want me to apply these patches as they are, or do you want to >> improve the series to include the dcr_unmap fix? > > Sorry I have missed this email. > > Yes please apply it as is. I don't have ppc hw here to be able to test this > change. This series does not apply. Can you rebase on top of linux-next, and resend? Tomi
On 10/09/2013 11:02 AM, Tomi Valkeinen wrote: > On 30/09/13 15:05, Michal Simek wrote: >> Hi Tomi, >> >> On 09/16/2013 12:34 PM, Tomi Valkeinen wrote: >>> On 16/09/13 13:33, Michal Simek wrote: >>>> On 09/16/2013 11:51 AM, Tomi Valkeinen wrote: >>>>> On 12/09/13 08:54, Michal Simek wrote: >>>>> >>>>>> @@ -394,16 +382,12 @@ static int xilinxfb_release(struct device *dev) >>>>>> /* Turn off the display */ >>>>>> xilinx_fb_out32(drvdata, REG_CTRL, 0); >>>>>> >>>>>> - /* Release the resources, as allocated based on interface */ >>>>>> - if (drvdata->flags & BUS_ACCESS_FLAG) >>>>>> - devm_iounmap(dev, drvdata->regs); >>>>>> #ifdef CONFIG_PPC_DCR >>>>>> - else >>>>>> + /* Release the resources, as allocated based on interface */ >>>>>> + if (!(drvdata->flags & BUS_ACCESS_FLAG)) >>>>>> dcr_unmap(drvdata->dcr_host, drvdata->dcr_len); >>>>>> #endif >>>>> >>>>> I might be mistaken, and it's not strictly part of this series, but >>>>> shouldn't dcr_unmap be called somewhere else also, if the probe fails? >>>> >>>> yes. It should. >>> >>> Do you want me to apply these patches as they are, or do you want to >>> improve the series to include the dcr_unmap fix? >> >> Sorry I have missed this email. >> >> Yes please apply it as is. I don't have ppc hw here to be able to test this >> change. > > This series does not apply. Can you rebase on top of linux-next, and resend? Do you mean Stephen Rothwell linux-next or any your linux-next branch? No problem to do so if you send me link to the repo. Thanks, Michal
On 09/10/13 13:25, Michal Simek wrote: >> This series does not apply. Can you rebase on top of linux-next, and resend? > > Do you mean Stephen Rothwell linux-next or any your linux-next branch? > No problem to do so if you send me link to the repo. Either one, my for-next is in Stephen's tree: git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git for-next Tomi
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c index fd9c430..7e3036c 100644 --- a/drivers/video/xilinxfb.c +++ b/drivers/video/xilinxfb.c @@ -260,10 +260,9 @@ static int xilinxfb_assign(struct platform_device *pdev, res = platform_get_resource(pdev, IORESOURCE_MEM, 0); drvdata->regs = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(drvdata->regs)) { - rc = PTR_ERR(drvdata->regs); - goto err_region; - } + if (IS_ERR(drvdata->regs)) + return PTR_ERR(drvdata->regs); + drvdata->regs_phys = res->start; } @@ -279,11 +278,7 @@ static int xilinxfb_assign(struct platform_device *pdev, if (!drvdata->fb_virt) { dev_err(dev, "Could not allocate frame buffer memory\n"); - rc = -ENOMEM; - if (drvdata->flags & BUS_ACCESS_FLAG) - goto err_fbmem; - else - goto err_region; + return -ENOMEM; } /* Clear (turn to black) the framebuffer */ @@ -363,13 +358,6 @@ err_cmap: /* Turn off the display */ xilinx_fb_out32(drvdata, REG_CTRL, 0); -err_fbmem: - if (drvdata->flags & BUS_ACCESS_FLAG) - devm_iounmap(dev, drvdata->regs); - -err_region: - dev_set_drvdata(dev, NULL); - return rc; } @@ -394,16 +382,12 @@ static int xilinxfb_release(struct device *dev) /* Turn off the display */ xilinx_fb_out32(drvdata, REG_CTRL, 0); - /* Release the resources, as allocated based on interface */ - if (drvdata->flags & BUS_ACCESS_FLAG) - devm_iounmap(dev, drvdata->regs); #ifdef CONFIG_PPC_DCR - else + /* Release the resources, as allocated based on interface */ + if (!(drvdata->flags & BUS_ACCESS_FLAG)) dcr_unmap(drvdata->dcr_host, drvdata->dcr_len); #endif - dev_set_drvdata(dev, NULL); - return 0; }
devm_iounmap is called automatically that's why remove it from the code dev_set_drvdata(dev, NULL) is called by generic code after device_release or on probe failure. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- drivers/video/xilinxfb.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) -- 1.8.2.3