| Message ID | 1433501966-30176-2-git-send-email-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Hans, On Fri, Jun 5, 2015 at 12:59 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > Bitrot has set in for this driver and the sh-vou.0 clock was never enabled, > so this driver didn't do anything. In addition, the clock was incorrectly > defined in clock-sh7724.c. Fix this. I think the clock should be enabled automatically using Runtime PM. drivers/sh/pm_runtime.c should configure the "NULL" (i.e. the first) clock for power management, after which pm_runtime_get_sync() will enable it. > While we're at it: use proper resource managed calls. Shouldn't that be a separate patch? Especially if the real fix becomes a one-liner (see below). > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > Cc: Magnus Damm <damm@opensource.se> > --- > arch/sh/kernel/cpu/sh4a/clock-sh7724.c | 2 +- > drivers/media/platform/sh_vou.c | 54 ++++++++++++---------------------- > 2 files changed, 20 insertions(+), 36 deletions(-) > > diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7724.c b/arch/sh/kernel/cpu/sh4a/clock-sh7724.c > index c187b95..f1df899 100644 > --- a/arch/sh/kernel/cpu/sh4a/clock-sh7724.c > +++ b/arch/sh/kernel/cpu/sh4a/clock-sh7724.c > @@ -343,7 +343,7 @@ static struct clk_lookup lookups[] = { > CLKDEV_CON_ID("2ddmac0", &mstp_clks[HWBLK_2DDMAC]), > CLKDEV_DEV_ID("sh_fsi.0", &mstp_clks[HWBLK_SPU]), > CLKDEV_CON_ID("jpu0", &mstp_clks[HWBLK_JPU]), > - CLKDEV_DEV_ID("sh-vou.0", &mstp_clks[HWBLK_VOU]), > + CLKDEV_CON_ID("sh-vou.0", &mstp_clks[HWBLK_VOU]), I don't know which SH board you have, but both arch/sh/boards/mach-ecovec24/setup.c and arch/sh/boards/mach-se/7724/setup.c create the platform device as: static struct platform_device vou_device = { .name = "sh-vou", .id = -1, }; so unless I'm mistaken, the platform device's name will be "sh-vou", not "sh-vou.0". Does it work if you just correct the name in the CLKDEV_DEV_ID() line? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/06/2015 11:42 PM, Geert Uytterhoeven wrote: > Hi Hans, > > On Fri, Jun 5, 2015 at 12:59 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> Bitrot has set in for this driver and the sh-vou.0 clock was never enabled, >> so this driver didn't do anything. In addition, the clock was incorrectly >> defined in clock-sh7724.c. Fix this. > > I think the clock should be enabled automatically using Runtime PM. > drivers/sh/pm_runtime.c should configure the "NULL" (i.e. the first) clock > for power management, after which pm_runtime_get_sync() will enable it. Ah, that works too after I fixed a small bug in sh_vou_release (status should have been reset to SH_VOU_INITIALISING). >> While we're at it: use proper resource managed calls. > > Shouldn't that be a separate patch? Especially if the real fix becomes a > one-liner (see below). I'll split it up. >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> Cc: Magnus Damm <damm@opensource.se> >> --- >> arch/sh/kernel/cpu/sh4a/clock-sh7724.c | 2 +- >> drivers/media/platform/sh_vou.c | 54 ++++++++++++---------------------- >> 2 files changed, 20 insertions(+), 36 deletions(-) >> >> diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7724.c b/arch/sh/kernel/cpu/sh4a/clock-sh7724.c >> index c187b95..f1df899 100644 >> --- a/arch/sh/kernel/cpu/sh4a/clock-sh7724.c >> +++ b/arch/sh/kernel/cpu/sh4a/clock-sh7724.c >> @@ -343,7 +343,7 @@ static struct clk_lookup lookups[] = { >> CLKDEV_CON_ID("2ddmac0", &mstp_clks[HWBLK_2DDMAC]), >> CLKDEV_DEV_ID("sh_fsi.0", &mstp_clks[HWBLK_SPU]), >> CLKDEV_CON_ID("jpu0", &mstp_clks[HWBLK_JPU]), >> - CLKDEV_DEV_ID("sh-vou.0", &mstp_clks[HWBLK_VOU]), >> + CLKDEV_CON_ID("sh-vou.0", &mstp_clks[HWBLK_VOU]), > > I don't know which SH board you have, but both > arch/sh/boards/mach-ecovec24/setup.c and > arch/sh/boards/mach-se/7724/setup.c create the platform device as: I have a R0P7724LC0011/21RL (mach-ecovec24 based) board. > > static struct platform_device vou_device = { > .name = "sh-vou", > .id = -1, > }; > > so unless I'm mistaken, the platform device's name will be "sh-vou", > not "sh-vou.0". > > Does it work if you just correct the name in the CLKDEV_DEV_ID() line? Yes, that works. Thanks for the help, I'll report this patch series with these fixes. Regards, Hans > > Thanks! > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/arch/sh/kernel/cpu/sh4a/clock-sh7724.c b/arch/sh/kernel/cpu/sh4a/clock-sh7724.c index c187b95..f1df899 100644 --- a/arch/sh/kernel/cpu/sh4a/clock-sh7724.c +++ b/arch/sh/kernel/cpu/sh4a/clock-sh7724.c @@ -343,7 +343,7 @@ static struct clk_lookup lookups[] = { CLKDEV_CON_ID("2ddmac0", &mstp_clks[HWBLK_2DDMAC]), CLKDEV_DEV_ID("sh_fsi.0", &mstp_clks[HWBLK_SPU]), CLKDEV_CON_ID("jpu0", &mstp_clks[HWBLK_JPU]), - CLKDEV_DEV_ID("sh-vou.0", &mstp_clks[HWBLK_VOU]), + CLKDEV_CON_ID("sh-vou.0", &mstp_clks[HWBLK_VOU]), CLKDEV_CON_ID("beu0", &mstp_clks[HWBLK_BEU0]), CLKDEV_DEV_ID("sh_mobile_ceu.0", &mstp_clks[HWBLK_CEU0]), CLKDEV_CON_ID("veu0", &mstp_clks[HWBLK_VEU0]), diff --git a/drivers/media/platform/sh_vou.c b/drivers/media/platform/sh_vou.c index 829e85c..9e98233 100644 --- a/drivers/media/platform/sh_vou.c +++ b/drivers/media/platform/sh_vou.c @@ -21,6 +21,7 @@ #include <linux/slab.h> #include <linux/videodev2.h> #include <linux/module.h> +#include <linux/clk.h> #include <media/sh_vou.h> #include <media/v4l2-common.h> @@ -65,6 +66,7 @@ struct sh_vou_device { struct video_device vdev; atomic_t use_count; struct sh_vou_pdata *pdata; + struct clk *clk; spinlock_t lock; void __iomem *base; /* State information */ @@ -1300,7 +1302,7 @@ static int sh_vou_probe(struct platform_device *pdev) struct i2c_adapter *i2c_adap; struct video_device *vdev; struct sh_vou_device *vou_dev; - struct resource *reg_res, *region; + struct resource *reg_res; struct v4l2_subdev *subdev; int irq, ret; @@ -1312,10 +1314,16 @@ static int sh_vou_probe(struct platform_device *pdev) return -ENODEV; } - vou_dev = kzalloc(sizeof(*vou_dev), GFP_KERNEL); + vou_dev = devm_kzalloc(&pdev->dev, sizeof(*vou_dev), GFP_KERNEL); if (!vou_dev) return -ENOMEM; + vou_dev->clk = devm_clk_get(&pdev->dev, "sh-vou.0"); + if (IS_ERR(vou_dev->clk)) { + dev_err(&pdev->dev, "cannot get clock\n"); + return PTR_ERR(vou_dev->clk); + } + INIT_LIST_HEAD(&vou_dev->queue); spin_lock_init(&vou_dev->lock); mutex_init(&vou_dev->fop_lock); @@ -1340,28 +1348,18 @@ static int sh_vou_probe(struct platform_device *pdev) pix->sizeimage = VOU_MAX_IMAGE_WIDTH * 2 * 480; pix->colorspace = V4L2_COLORSPACE_SMPTE170M; - region = request_mem_region(reg_res->start, resource_size(reg_res), - pdev->name); - if (!region) { - dev_err(&pdev->dev, "VOU region already claimed\n"); - ret = -EBUSY; - goto ereqmemreg; - } + vou_dev->base = devm_ioremap_resource(&pdev->dev, reg_res); + if (IS_ERR(vou_dev->base)) + return PTR_ERR(vou_dev->base); - vou_dev->base = ioremap(reg_res->start, resource_size(reg_res)); - if (!vou_dev->base) { - ret = -ENOMEM; - goto emap; - } - - ret = request_irq(irq, sh_vou_isr, 0, "vou", vou_dev); + ret = devm_request_irq(&pdev->dev, irq, sh_vou_isr, 0, "vou", vou_dev); if (ret < 0) - goto ereqirq; + return ret; ret = v4l2_device_register(&pdev->dev, &vou_dev->v4l2_dev); if (ret < 0) { dev_err(&pdev->dev, "Error registering v4l2 device\n"); - goto ev4l2devreg; + return ret; } vdev = &vou_dev->vdev; @@ -1383,6 +1381,7 @@ static int sh_vou_probe(struct platform_device *pdev) goto ei2cgadap; } + clk_prepare_enable(vou_dev->clk); ret = sh_vou_hw_init(vou_dev); if (ret < 0) goto ereset; @@ -1403,43 +1402,28 @@ static int sh_vou_probe(struct platform_device *pdev) evregdev: ei2cnd: ereset: + clk_disable_unprepare(vou_dev->clk); i2c_put_adapter(i2c_adap); ei2cgadap: pm_runtime_disable(&pdev->dev); v4l2_device_unregister(&vou_dev->v4l2_dev); -ev4l2devreg: - free_irq(irq, vou_dev); -ereqirq: - iounmap(vou_dev->base); -emap: - release_mem_region(reg_res->start, resource_size(reg_res)); -ereqmemreg: - kfree(vou_dev); return ret; } static int sh_vou_remove(struct platform_device *pdev) { - int irq = platform_get_irq(pdev, 0); struct v4l2_device *v4l2_dev = platform_get_drvdata(pdev); struct sh_vou_device *vou_dev = container_of(v4l2_dev, struct sh_vou_device, v4l2_dev); struct v4l2_subdev *sd = list_entry(v4l2_dev->subdevs.next, struct v4l2_subdev, list); struct i2c_client *client = v4l2_get_subdevdata(sd); - struct resource *reg_res; - if (irq > 0) - free_irq(irq, vou_dev); pm_runtime_disable(&pdev->dev); video_unregister_device(&vou_dev->vdev); + clk_disable_unprepare(vou_dev->clk); i2c_put_adapter(client->adapter); v4l2_device_unregister(&vou_dev->v4l2_dev); - iounmap(vou_dev->base); - reg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (reg_res) - release_mem_region(reg_res->start, resource_size(reg_res)); - kfree(vou_dev); return 0; }