* [PATCH v4 0/2] media: atmel-isi: rework on the clock part and add runtime pm support @ 2015-05-26 8:00 Josh Wu 2015-05-26 8:00 ` [PATCH v4 1/2] media: atmel-isi: " Josh Wu 2015-05-26 8:00 ` [PATCH v4 2/2] media: atmel-isi: remove mck back compatiable code as it's not need Josh Wu 0 siblings, 2 replies; 6+ messages in thread From: Josh Wu @ 2015-05-26 8:00 UTC (permalink / raw) To: Guennadi Liakhovetski, Laurent Pinchart Cc: Linux Media Mailing List, Nicolas Ferre, linux-arm-kernel, Josh Wu This patch series fix the peripheral clock code and enable runtime pm support. Also it clean up the code which is for the compatiblity of mck. Changes in v4: - need to call pm_runtime_disable() in atmel_isi_remove(). - merged the patch which remove isi disable code in atmel_isi_probe() as isi peripherial clock is not enabled in this moment. - refine the commit log Changes in v3: - remove useless definition: ISI_DEFAULT_MCLK_FREQ - remove some isi disable code if peripheral clock is disabled. Changes in v2: - merged v1 two patch into one. - use runtime_pm_put() instead of runtime_pm_put_sync() - enable peripheral clock before access ISI registers. - totally remove clock_start()/clock_stop() as they are optional. Josh Wu (2): media: atmel-isi: add runtime pm support media: atmel-isi: remove mck back compatiable code as it's not need drivers/media/platform/soc_camera/atmel-isi.c | 100 ++++++++++++-------------- 1 file changed, 46 insertions(+), 54 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/2] media: atmel-isi: add runtime pm support 2015-05-26 8:00 [PATCH v4 0/2] media: atmel-isi: rework on the clock part and add runtime pm support Josh Wu @ 2015-05-26 8:00 ` Josh Wu 2015-05-26 8:16 ` Laurent Pinchart 2015-05-26 8:00 ` [PATCH v4 2/2] media: atmel-isi: remove mck back compatiable code as it's not need Josh Wu 1 sibling, 1 reply; 6+ messages in thread From: Josh Wu @ 2015-05-26 8:00 UTC (permalink / raw) To: Guennadi Liakhovetski, Laurent Pinchart Cc: Linux Media Mailing List, Nicolas Ferre, linux-arm-kernel, Josh Wu The runtime pm resume/suspend will enable/disable pclk (ISI peripheral clock). And we need to call runtime_pm_get_sync()/runtime_pm_put() when we need access ISI registers. In atmel_isi_probe(), remove the isi disable code as in the moment ISI peripheral clock is not enable yet. In the meantime, as clock_start()/clock_stop() is used to control the mclk not ISI peripheral clock. So move this to start[stop]_streaming() function. Signed-off-by: Josh Wu <josh.wu@atmel.com> --- Changes in v4: - need to call pm_runtime_disable() in atmel_isi_remove(). - merged the patch which remove isi disable code in atmel_isi_probe() as isi peripherial clock is not enabled in this moment. - refine the commit log Changes in v3: None Changes in v2: - merged v1 two patch into one. - use runtime_pm_put() instead of runtime_pm_put_sync() - enable peripheral clock before access ISI registers. drivers/media/platform/soc_camera/atmel-isi.c | 54 +++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 8 deletions(-) diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c index 2879026..194e875 100644 --- a/drivers/media/platform/soc_camera/atmel-isi.c +++ b/drivers/media/platform/soc_camera/atmel-isi.c @@ -20,6 +20,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/slab.h> #include <media/atmel-isi.h> @@ -386,6 +387,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count) struct atmel_isi *isi = ici->priv; int ret; + pm_runtime_get_sync(ici->v4l2_dev.dev); + /* Reset ISI */ ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET); if (ret < 0) { @@ -445,6 +448,8 @@ static void stop_streaming(struct vb2_queue *vq) ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE); if (ret < 0) dev_err(icd->parent, "Disable ISI timed out\n"); + + pm_runtime_put(ici->v4l2_dev.dev); } static struct vb2_ops isi_video_qops = { @@ -516,7 +521,13 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd, if (mf->code != xlate->code) return -EINVAL; + /* Enable PM and peripheral clock before operate isi registers */ + pm_runtime_get_sync(ici->v4l2_dev.dev); + ret = configure_geometry(isi, pix->width, pix->height, xlate->code); + + pm_runtime_put(ici->v4l2_dev.dev); + if (ret < 0) return ret; @@ -736,14 +747,9 @@ static int isi_camera_clock_start(struct soc_camera_host *ici) struct atmel_isi *isi = ici->priv; int ret; - ret = clk_prepare_enable(isi->pclk); - if (ret) - return ret; - if (!IS_ERR(isi->mck)) { ret = clk_prepare_enable(isi->mck); if (ret) { - clk_disable_unprepare(isi->pclk); return ret; } } @@ -758,7 +764,6 @@ static void isi_camera_clock_stop(struct soc_camera_host *ici) if (!IS_ERR(isi->mck)) clk_disable_unprepare(isi->mck); - clk_disable_unprepare(isi->pclk); } static unsigned int isi_camera_poll(struct file *file, poll_table *pt) @@ -855,9 +860,14 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd) cfg1 |= ISI_CFG1_THMASK_BEATS_16; + /* Enable PM and peripheral clock before operate isi registers */ + pm_runtime_get_sync(ici->v4l2_dev.dev); + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); isi_writel(isi, ISI_CFG1, cfg1); + pm_runtime_put(ici->v4l2_dev.dev); + return 0; } @@ -889,6 +899,7 @@ static int atmel_isi_remove(struct platform_device *pdev) sizeof(struct fbd) * MAX_BUFFER_NUM, isi->p_fb_descriptors, isi->fb_descriptors_phys); + pm_runtime_disable(&pdev->dev); return 0; } @@ -1027,8 +1038,6 @@ static int atmel_isi_probe(struct platform_device *pdev) if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10) isi->width_flags |= 1 << 9; - isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); - irq = platform_get_irq(pdev, 0); if (IS_ERR_VALUE(irq)) { ret = irq; @@ -1049,6 +1058,9 @@ static int atmel_isi_probe(struct platform_device *pdev) soc_host->v4l2_dev.dev = &pdev->dev; soc_host->nr = pdev->id; + pm_suspend_ignore_children(&pdev->dev, true); + pm_runtime_enable(&pdev->dev); + if (isi->pdata.asd_sizes) { soc_host->asd = isi->pdata.asd; soc_host->asd_sizes = isi->pdata.asd_sizes; @@ -1062,6 +1074,7 @@ static int atmel_isi_probe(struct platform_device *pdev) return 0; err_register_soc_camera_host: + pm_runtime_disable(&pdev->dev); err_req_irq: err_ioremap: vb2_dma_contig_cleanup_ctx(isi->alloc_ctx); @@ -1074,6 +1087,30 @@ err_alloc_ctx: return ret; } +static int atmel_isi_runtime_suspend(struct device *dev) +{ + struct soc_camera_host *soc_host = to_soc_camera_host(dev); + struct atmel_isi *isi = container_of(soc_host, + struct atmel_isi, soc_host); + + clk_disable_unprepare(isi->pclk); + + return 0; +} +static int atmel_isi_runtime_resume(struct device *dev) +{ + struct soc_camera_host *soc_host = to_soc_camera_host(dev); + struct atmel_isi *isi = container_of(soc_host, + struct atmel_isi, soc_host); + + return clk_prepare_enable(isi->pclk); +} + +static const struct dev_pm_ops atmel_isi_dev_pm_ops = { + SET_RUNTIME_PM_OPS(atmel_isi_runtime_suspend, + atmel_isi_runtime_resume, NULL) +}; + static const struct of_device_id atmel_isi_of_match[] = { { .compatible = "atmel,at91sam9g45-isi" }, { } @@ -1085,6 +1122,7 @@ static struct platform_driver atmel_isi_driver = { .driver = { .name = "atmel_isi", .of_match_table = of_match_ptr(atmel_isi_of_match), + .pm = &atmel_isi_dev_pm_ops, }, }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] media: atmel-isi: add runtime pm support 2015-05-26 8:00 ` [PATCH v4 1/2] media: atmel-isi: " Josh Wu @ 2015-05-26 8:16 ` Laurent Pinchart 2015-05-26 9:14 ` Josh Wu 0 siblings, 1 reply; 6+ messages in thread From: Laurent Pinchart @ 2015-05-26 8:16 UTC (permalink / raw) To: Josh Wu Cc: Guennadi Liakhovetski, Linux Media Mailing List, Nicolas Ferre, linux-arm-kernel Hi Josh, Thank you for the patch. On Tuesday 26 May 2015 16:00:09 Josh Wu wrote: > The runtime pm resume/suspend will enable/disable pclk (ISI peripheral > clock). > And we need to call runtime_pm_get_sync()/runtime_pm_put() when we need > access ISI registers. In atmel_isi_probe(), remove the isi disable code > as in the moment ISI peripheral clock is not enable yet. > > In the meantime, as clock_start()/clock_stop() is used to control the > mclk not ISI peripheral clock. So move this to start[stop]_streaming() > function. > > Signed-off-by: Josh Wu <josh.wu@atmel.com> > --- > > Changes in v4: > - need to call pm_runtime_disable() in atmel_isi_remove(). > - merged the patch which remove isi disable code in atmel_isi_probe() as > isi peripherial clock is not enabled in this moment. > - refine the commit log > > Changes in v3: None > Changes in v2: > - merged v1 two patch into one. > - use runtime_pm_put() instead of runtime_pm_put_sync() > - enable peripheral clock before access ISI registers. > > drivers/media/platform/soc_camera/atmel-isi.c | 54 ++++++++++++++++++++---- > 1 file changed, 46 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/atmel-isi.c > b/drivers/media/platform/soc_camera/atmel-isi.c index 2879026..194e875 > 100644 > --- a/drivers/media/platform/soc_camera/atmel-isi.c > +++ b/drivers/media/platform/soc_camera/atmel-isi.c > @@ -20,6 +20,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > #include <linux/slab.h> > > #include <media/atmel-isi.h> > @@ -386,6 +387,8 @@ static int start_streaming(struct vb2_queue *vq, > unsigned int count) struct atmel_isi *isi = ici->priv; > int ret; > > + pm_runtime_get_sync(ici->v4l2_dev.dev); > + > /* Reset ISI */ > ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET); > if (ret < 0) { You need to call pm_runtime_put() in the error path, otherwise an atmel_isi_wait_status() failure will keep the device enabled. > @@ -445,6 +448,8 @@ static void stop_streaming(struct vb2_queue *vq) > ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE); > if (ret < 0) > dev_err(icd->parent, "Disable ISI timed out\n"); > + > + pm_runtime_put(ici->v4l2_dev.dev); > } > > static struct vb2_ops isi_video_qops = { > @@ -516,7 +521,13 @@ static int isi_camera_set_fmt(struct soc_camera_device > *icd, if (mf->code != xlate->code) > return -EINVAL; > > + /* Enable PM and peripheral clock before operate isi registers */ > + pm_runtime_get_sync(ici->v4l2_dev.dev); > + > ret = configure_geometry(isi, pix->width, pix->height, xlate->code); > + > + pm_runtime_put(ici->v4l2_dev.dev); I think it would simplify the code to move the configure_geometry() call to start_streaming() as you already call pm_runtime_get_sync() there. That can be done in a separate patch though. > + > if (ret < 0) > return ret; > > @@ -736,14 +747,9 @@ static int isi_camera_clock_start(struct > soc_camera_host *ici) struct atmel_isi *isi = ici->priv; > int ret; > > - ret = clk_prepare_enable(isi->pclk); > - if (ret) > - return ret; > - > if (!IS_ERR(isi->mck)) { > ret = clk_prepare_enable(isi->mck); > if (ret) { > - clk_disable_unprepare(isi->pclk); > return ret; > } > } > @@ -758,7 +764,6 @@ static void isi_camera_clock_stop(struct soc_camera_host > *ici) > > if (!IS_ERR(isi->mck)) > clk_disable_unprepare(isi->mck); > - clk_disable_unprepare(isi->pclk); > } > > static unsigned int isi_camera_poll(struct file *file, poll_table *pt) > @@ -855,9 +860,14 @@ static int isi_camera_set_bus_param(struct > soc_camera_device *icd) > > cfg1 |= ISI_CFG1_THMASK_BEATS_16; > > + /* Enable PM and peripheral clock before operate isi registers */ > + pm_runtime_get_sync(ici->v4l2_dev.dev); > + > isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); > isi_writel(isi, ISI_CFG1, cfg1); > > + pm_runtime_put(ici->v4l2_dev.dev); > + > return 0; > } > > @@ -889,6 +899,7 @@ static int atmel_isi_remove(struct platform_device > *pdev) sizeof(struct fbd) * MAX_BUFFER_NUM, > isi->p_fb_descriptors, > isi->fb_descriptors_phys); > + pm_runtime_disable(&pdev->dev); > > return 0; > } > @@ -1027,8 +1038,6 @@ static int atmel_isi_probe(struct platform_device > *pdev) if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10) > isi->width_flags |= 1 << 9; > > - isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); > - > irq = platform_get_irq(pdev, 0); > if (IS_ERR_VALUE(irq)) { > ret = irq; > @@ -1049,6 +1058,9 @@ static int atmel_isi_probe(struct platform_device > *pdev) soc_host->v4l2_dev.dev = &pdev->dev; > soc_host->nr = pdev->id; > > + pm_suspend_ignore_children(&pdev->dev, true); > + pm_runtime_enable(&pdev->dev); > + > if (isi->pdata.asd_sizes) { > soc_host->asd = isi->pdata.asd; > soc_host->asd_sizes = isi->pdata.asd_sizes; > @@ -1062,6 +1074,7 @@ static int atmel_isi_probe(struct platform_device > *pdev) return 0; > > err_register_soc_camera_host: > + pm_runtime_disable(&pdev->dev); > err_req_irq: > err_ioremap: > vb2_dma_contig_cleanup_ctx(isi->alloc_ctx); > @@ -1074,6 +1087,30 @@ err_alloc_ctx: > return ret; > } > > +static int atmel_isi_runtime_suspend(struct device *dev) > +{ > + struct soc_camera_host *soc_host = to_soc_camera_host(dev); > + struct atmel_isi *isi = container_of(soc_host, > + struct atmel_isi, soc_host); > + > + clk_disable_unprepare(isi->pclk); > + > + return 0; > +} > +static int atmel_isi_runtime_resume(struct device *dev) > +{ > + struct soc_camera_host *soc_host = to_soc_camera_host(dev); > + struct atmel_isi *isi = container_of(soc_host, > + struct atmel_isi, soc_host); > + > + return clk_prepare_enable(isi->pclk); > +} > + > +static const struct dev_pm_ops atmel_isi_dev_pm_ops = { > + SET_RUNTIME_PM_OPS(atmel_isi_runtime_suspend, > + atmel_isi_runtime_resume, NULL) > +}; > + > static const struct of_device_id atmel_isi_of_match[] = { > { .compatible = "atmel,at91sam9g45-isi" }, > { } > @@ -1085,6 +1122,7 @@ static struct platform_driver atmel_isi_driver = { > .driver = { > .name = "atmel_isi", > .of_match_table = of_match_ptr(atmel_isi_of_match), > + .pm = &atmel_isi_dev_pm_ops, > }, > }; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] media: atmel-isi: add runtime pm support 2015-05-26 8:16 ` Laurent Pinchart @ 2015-05-26 9:14 ` Josh Wu 2015-05-26 9:58 ` Josh Wu 0 siblings, 1 reply; 6+ messages in thread From: Josh Wu @ 2015-05-26 9:14 UTC (permalink / raw) To: Laurent Pinchart Cc: Guennadi Liakhovetski, Linux Media Mailing List, Nicolas Ferre, linux-arm-kernel Hi, Laurent On 5/26/2015 4:16 PM, Laurent Pinchart wrote: > Hi Josh, > > Thank you for the patch. > > On Tuesday 26 May 2015 16:00:09 Josh Wu wrote: >> The runtime pm resume/suspend will enable/disable pclk (ISI peripheral >> clock). >> And we need to call runtime_pm_get_sync()/runtime_pm_put() when we need >> access ISI registers. In atmel_isi_probe(), remove the isi disable code >> as in the moment ISI peripheral clock is not enable yet. >> >> In the meantime, as clock_start()/clock_stop() is used to control the >> mclk not ISI peripheral clock. So move this to start[stop]_streaming() >> function. >> >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> --- >> >> Changes in v4: >> - need to call pm_runtime_disable() in atmel_isi_remove(). >> - merged the patch which remove isi disable code in atmel_isi_probe() as >> isi peripherial clock is not enabled in this moment. >> - refine the commit log >> >> Changes in v3: None >> Changes in v2: >> - merged v1 two patch into one. >> - use runtime_pm_put() instead of runtime_pm_put_sync() >> - enable peripheral clock before access ISI registers. >> >> drivers/media/platform/soc_camera/atmel-isi.c | 54 ++++++++++++++++++++---- >> 1 file changed, 46 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c >> b/drivers/media/platform/soc_camera/atmel-isi.c index 2879026..194e875 >> 100644 >> --- a/drivers/media/platform/soc_camera/atmel-isi.c >> +++ b/drivers/media/platform/soc_camera/atmel-isi.c >> @@ -20,6 +20,7 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> #include <linux/slab.h> >> >> #include <media/atmel-isi.h> >> @@ -386,6 +387,8 @@ static int start_streaming(struct vb2_queue *vq, >> unsigned int count) struct atmel_isi *isi = ici->priv; >> int ret; >> >> + pm_runtime_get_sync(ici->v4l2_dev.dev); >> + >> /* Reset ISI */ >> ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET); >> if (ret < 0) { > You need to call pm_runtime_put() in the error path, otherwise an > atmel_isi_wait_status() failure will keep the device enabled. Nice catch. Thanks. > >> @@ -445,6 +448,8 @@ static void stop_streaming(struct vb2_queue *vq) >> ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE); >> if (ret < 0) >> dev_err(icd->parent, "Disable ISI timed out\n"); >> + >> + pm_runtime_put(ici->v4l2_dev.dev); >> } >> >> static struct vb2_ops isi_video_qops = { >> @@ -516,7 +521,13 @@ static int isi_camera_set_fmt(struct soc_camera_device >> *icd, if (mf->code != xlate->code) >> return -EINVAL; >> >> + /* Enable PM and peripheral clock before operate isi registers */ >> + pm_runtime_get_sync(ici->v4l2_dev.dev); >> + >> ret = configure_geometry(isi, pix->width, pix->height, xlate->code); >> + >> + pm_runtime_put(ici->v4l2_dev.dev); > I think it would simplify the code to move the configure_geometry() call to > start_streaming() as you already call pm_runtime_get_sync() there. That can be > done in a separate patch though. I think it's a good idea. I would like to make this in a separate patch and not include in this series. In summary, I'll resend this patch just with the fix you mentioned in above (fix the error path in start_streaming). Is that okay for you? Best Regards, Josh Wu > >> + >> if (ret < 0) >> return ret; >> >> @@ -736,14 +747,9 @@ static int isi_camera_clock_start(struct >> soc_camera_host *ici) struct atmel_isi *isi = ici->priv; >> int ret; >> >> - ret = clk_prepare_enable(isi->pclk); >> - if (ret) >> - return ret; >> - >> if (!IS_ERR(isi->mck)) { >> ret = clk_prepare_enable(isi->mck); >> if (ret) { >> - clk_disable_unprepare(isi->pclk); >> return ret; >> } >> } >> @@ -758,7 +764,6 @@ static void isi_camera_clock_stop(struct soc_camera_host >> *ici) >> >> if (!IS_ERR(isi->mck)) >> clk_disable_unprepare(isi->mck); >> - clk_disable_unprepare(isi->pclk); >> } >> >> static unsigned int isi_camera_poll(struct file *file, poll_table *pt) >> @@ -855,9 +860,14 @@ static int isi_camera_set_bus_param(struct >> soc_camera_device *icd) >> >> cfg1 |= ISI_CFG1_THMASK_BEATS_16; >> >> + /* Enable PM and peripheral clock before operate isi registers */ >> + pm_runtime_get_sync(ici->v4l2_dev.dev); >> + >> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); >> isi_writel(isi, ISI_CFG1, cfg1); >> >> + pm_runtime_put(ici->v4l2_dev.dev); >> + >> return 0; >> } >> >> @@ -889,6 +899,7 @@ static int atmel_isi_remove(struct platform_device >> *pdev) sizeof(struct fbd) * MAX_BUFFER_NUM, >> isi->p_fb_descriptors, >> isi->fb_descriptors_phys); >> + pm_runtime_disable(&pdev->dev); >> >> return 0; >> } >> @@ -1027,8 +1038,6 @@ static int atmel_isi_probe(struct platform_device >> *pdev) if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10) >> isi->width_flags |= 1 << 9; >> >> - isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); >> - >> irq = platform_get_irq(pdev, 0); >> if (IS_ERR_VALUE(irq)) { >> ret = irq; >> @@ -1049,6 +1058,9 @@ static int atmel_isi_probe(struct platform_device >> *pdev) soc_host->v4l2_dev.dev = &pdev->dev; >> soc_host->nr = pdev->id; >> >> + pm_suspend_ignore_children(&pdev->dev, true); >> + pm_runtime_enable(&pdev->dev); >> + >> if (isi->pdata.asd_sizes) { >> soc_host->asd = isi->pdata.asd; >> soc_host->asd_sizes = isi->pdata.asd_sizes; >> @@ -1062,6 +1074,7 @@ static int atmel_isi_probe(struct platform_device >> *pdev) return 0; >> >> err_register_soc_camera_host: >> + pm_runtime_disable(&pdev->dev); >> err_req_irq: >> err_ioremap: >> vb2_dma_contig_cleanup_ctx(isi->alloc_ctx); >> @@ -1074,6 +1087,30 @@ err_alloc_ctx: >> return ret; >> } >> >> +static int atmel_isi_runtime_suspend(struct device *dev) >> +{ >> + struct soc_camera_host *soc_host = to_soc_camera_host(dev); >> + struct atmel_isi *isi = container_of(soc_host, >> + struct atmel_isi, soc_host); >> + >> + clk_disable_unprepare(isi->pclk); >> + >> + return 0; >> +} >> +static int atmel_isi_runtime_resume(struct device *dev) >> +{ >> + struct soc_camera_host *soc_host = to_soc_camera_host(dev); >> + struct atmel_isi *isi = container_of(soc_host, >> + struct atmel_isi, soc_host); >> + >> + return clk_prepare_enable(isi->pclk); >> +} >> + >> +static const struct dev_pm_ops atmel_isi_dev_pm_ops = { >> + SET_RUNTIME_PM_OPS(atmel_isi_runtime_suspend, >> + atmel_isi_runtime_resume, NULL) >> +}; >> + >> static const struct of_device_id atmel_isi_of_match[] = { >> { .compatible = "atmel,at91sam9g45-isi" }, >> { } >> @@ -1085,6 +1122,7 @@ static struct platform_driver atmel_isi_driver = { >> .driver = { >> .name = "atmel_isi", >> .of_match_table = of_match_ptr(atmel_isi_of_match), >> + .pm = &atmel_isi_dev_pm_ops, >> }, >> }; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] media: atmel-isi: add runtime pm support 2015-05-26 9:14 ` Josh Wu @ 2015-05-26 9:58 ` Josh Wu 0 siblings, 0 replies; 6+ messages in thread From: Josh Wu @ 2015-05-26 9:58 UTC (permalink / raw) To: Laurent Pinchart Cc: Nicolas Ferre, Guennadi Liakhovetski, linux-arm-kernel, Linux Media Mailing List On 5/26/2015 5:14 PM, Josh Wu wrote: > Hi, Laurent > > On 5/26/2015 4:16 PM, Laurent Pinchart wrote: >> Hi Josh, >> >> Thank you for the patch. >> >> On Tuesday 26 May 2015 16:00:09 Josh Wu wrote: >>> The runtime pm resume/suspend will enable/disable pclk (ISI peripheral >>> clock). >>> And we need to call runtime_pm_get_sync()/runtime_pm_put() when we need >>> access ISI registers. In atmel_isi_probe(), remove the isi disable code >>> as in the moment ISI peripheral clock is not enable yet. >>> >>> In the meantime, as clock_start()/clock_stop() is used to control the >>> mclk not ISI peripheral clock. So move this to start[stop]_streaming() >>> function. >>> >>> Signed-off-by: Josh Wu <josh.wu@atmel.com> >>> --- >>> >>> Changes in v4: >>> - need to call pm_runtime_disable() in atmel_isi_remove(). >>> - merged the patch which remove isi disable code in >>> atmel_isi_probe() as >>> isi peripherial clock is not enabled in this moment. >>> - refine the commit log >>> >>> Changes in v3: None >>> Changes in v2: >>> - merged v1 two patch into one. >>> - use runtime_pm_put() instead of runtime_pm_put_sync() >>> - enable peripheral clock before access ISI registers. >>> >>> drivers/media/platform/soc_camera/atmel-isi.c | 54 >>> ++++++++++++++++++++---- >>> 1 file changed, 46 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c >>> b/drivers/media/platform/soc_camera/atmel-isi.c index 2879026..194e875 >>> 100644 >>> --- a/drivers/media/platform/soc_camera/atmel-isi.c >>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c >>> @@ -20,6 +20,7 @@ >>> #include <linux/kernel.h> >>> #include <linux/module.h> >>> #include <linux/platform_device.h> >>> +#include <linux/pm_runtime.h> >>> #include <linux/slab.h> >>> >>> #include <media/atmel-isi.h> >>> @@ -386,6 +387,8 @@ static int start_streaming(struct vb2_queue *vq, >>> unsigned int count) struct atmel_isi *isi = ici->priv; >>> int ret; >>> >>> + pm_runtime_get_sync(ici->v4l2_dev.dev); >>> + >>> /* Reset ISI */ >>> ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET); >>> if (ret < 0) { >> You need to call pm_runtime_put() in the error path, otherwise an >> atmel_isi_wait_status() failure will keep the device enabled. > > Nice catch. Thanks. > >> >>> @@ -445,6 +448,8 @@ static void stop_streaming(struct vb2_queue *vq) >>> ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE); >>> if (ret < 0) >>> dev_err(icd->parent, "Disable ISI timed out\n"); >>> + >>> + pm_runtime_put(ici->v4l2_dev.dev); >>> } >>> >>> static struct vb2_ops isi_video_qops = { >>> @@ -516,7 +521,13 @@ static int isi_camera_set_fmt(struct >>> soc_camera_device >>> *icd, if (mf->code != xlate->code) >>> return -EINVAL; >>> >>> + /* Enable PM and peripheral clock before operate isi registers */ >>> + pm_runtime_get_sync(ici->v4l2_dev.dev); >>> + >>> ret = configure_geometry(isi, pix->width, pix->height, >>> xlate->code); >>> + >>> + pm_runtime_put(ici->v4l2_dev.dev); >> I think it would simplify the code to move the configure_geometry() >> call to >> start_streaming() as you already call pm_runtime_get_sync() there. >> That can be >> done in a separate patch though. > > I think it's a good idea. I would like to make this in a separate > patch and not include in this series. > > In summary, I'll resend this patch just with the fix you mentioned in > above (fix the error path in start_streaming). > Is that okay for you? As I found another issue on stop_streaming(), so I sent a v5 series for this whole patch set with a new fix. Please check it. BTW: I don't include the simplify method: move configure_geometry() into start_streaming(), I perfer do that after this series is merged. Best Regards, Josh Wu > > Best Regards, > Josh Wu > >> >>> + >>> if (ret < 0) >>> return ret; >>> >>> @@ -736,14 +747,9 @@ static int isi_camera_clock_start(struct >>> soc_camera_host *ici) struct atmel_isi *isi = ici->priv; >>> int ret; >>> >>> - ret = clk_prepare_enable(isi->pclk); >>> - if (ret) >>> - return ret; >>> - >>> if (!IS_ERR(isi->mck)) { >>> ret = clk_prepare_enable(isi->mck); >>> if (ret) { >>> - clk_disable_unprepare(isi->pclk); >>> return ret; >>> } >>> } >>> @@ -758,7 +764,6 @@ static void isi_camera_clock_stop(struct >>> soc_camera_host >>> *ici) >>> >>> if (!IS_ERR(isi->mck)) >>> clk_disable_unprepare(isi->mck); >>> - clk_disable_unprepare(isi->pclk); >>> } >>> >>> static unsigned int isi_camera_poll(struct file *file, poll_table >>> *pt) >>> @@ -855,9 +860,14 @@ static int isi_camera_set_bus_param(struct >>> soc_camera_device *icd) >>> >>> cfg1 |= ISI_CFG1_THMASK_BEATS_16; >>> >>> + /* Enable PM and peripheral clock before operate isi registers */ >>> + pm_runtime_get_sync(ici->v4l2_dev.dev); >>> + >>> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); >>> isi_writel(isi, ISI_CFG1, cfg1); >>> >>> + pm_runtime_put(ici->v4l2_dev.dev); >>> + >>> return 0; >>> } >>> >>> @@ -889,6 +899,7 @@ static int atmel_isi_remove(struct platform_device >>> *pdev) sizeof(struct fbd) * MAX_BUFFER_NUM, >>> isi->p_fb_descriptors, >>> isi->fb_descriptors_phys); >>> + pm_runtime_disable(&pdev->dev); >>> >>> return 0; >>> } >>> @@ -1027,8 +1038,6 @@ static int atmel_isi_probe(struct platform_device >>> *pdev) if (isi->pdata.data_width_flags & ISI_DATAWIDTH_10) >>> isi->width_flags |= 1 << 9; >>> >>> - isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); >>> - >>> irq = platform_get_irq(pdev, 0); >>> if (IS_ERR_VALUE(irq)) { >>> ret = irq; >>> @@ -1049,6 +1058,9 @@ static int atmel_isi_probe(struct platform_device >>> *pdev) soc_host->v4l2_dev.dev = &pdev->dev; >>> soc_host->nr = pdev->id; >>> >>> + pm_suspend_ignore_children(&pdev->dev, true); >>> + pm_runtime_enable(&pdev->dev); >>> + >>> if (isi->pdata.asd_sizes) { >>> soc_host->asd = isi->pdata.asd; >>> soc_host->asd_sizes = isi->pdata.asd_sizes; >>> @@ -1062,6 +1074,7 @@ static int atmel_isi_probe(struct platform_device >>> *pdev) return 0; >>> >>> err_register_soc_camera_host: >>> + pm_runtime_disable(&pdev->dev); >>> err_req_irq: >>> err_ioremap: >>> vb2_dma_contig_cleanup_ctx(isi->alloc_ctx); >>> @@ -1074,6 +1087,30 @@ err_alloc_ctx: >>> return ret; >>> } >>> >>> +static int atmel_isi_runtime_suspend(struct device *dev) >>> +{ >>> + struct soc_camera_host *soc_host = to_soc_camera_host(dev); >>> + struct atmel_isi *isi = container_of(soc_host, >>> + struct atmel_isi, soc_host); >>> + >>> + clk_disable_unprepare(isi->pclk); >>> + >>> + return 0; >>> +} >>> +static int atmel_isi_runtime_resume(struct device *dev) >>> +{ >>> + struct soc_camera_host *soc_host = to_soc_camera_host(dev); >>> + struct atmel_isi *isi = container_of(soc_host, >>> + struct atmel_isi, soc_host); >>> + >>> + return clk_prepare_enable(isi->pclk); >>> +} >>> + >>> +static const struct dev_pm_ops atmel_isi_dev_pm_ops = { >>> + SET_RUNTIME_PM_OPS(atmel_isi_runtime_suspend, >>> + atmel_isi_runtime_resume, NULL) >>> +}; >>> + >>> static const struct of_device_id atmel_isi_of_match[] = { >>> { .compatible = "atmel,at91sam9g45-isi" }, >>> { } >>> @@ -1085,6 +1122,7 @@ static struct platform_driver atmel_isi_driver >>> = { >>> .driver = { >>> .name = "atmel_isi", >>> .of_match_table = of_match_ptr(atmel_isi_of_match), >>> + .pm = &atmel_isi_dev_pm_ops, >>> }, >>> }; > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] media: atmel-isi: remove mck back compatiable code as it's not need 2015-05-26 8:00 [PATCH v4 0/2] media: atmel-isi: rework on the clock part and add runtime pm support Josh Wu 2015-05-26 8:00 ` [PATCH v4 1/2] media: atmel-isi: " Josh Wu @ 2015-05-26 8:00 ` Josh Wu 1 sibling, 0 replies; 6+ messages in thread From: Josh Wu @ 2015-05-26 8:00 UTC (permalink / raw) To: Guennadi Liakhovetski, Laurent Pinchart Cc: Linux Media Mailing List, Nicolas Ferre, linux-arm-kernel, Josh Wu The master clock should handled by sensor itself. Signed-off-by: Josh Wu <josh.wu@atmel.com> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes in v4: None Changes in v3: - remove useless definition: ISI_DEFAULT_MCLK_FREQ Changes in v2: - totally remove clock_start()/clock_stop() as they are optional. drivers/media/platform/soc_camera/atmel-isi.c | 46 --------------------------- 1 file changed, 46 deletions(-) diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c index 194e875..65d9bc4 100644 --- a/drivers/media/platform/soc_camera/atmel-isi.c +++ b/drivers/media/platform/soc_camera/atmel-isi.c @@ -35,7 +35,6 @@ #define VID_LIMIT_BYTES (16 * 1024 * 1024) #define MIN_FRAME_RATE 15 #define FRAME_INTERVAL_MILLI_SEC (1000 / MIN_FRAME_RATE) -#define ISI_DEFAULT_MCLK_FREQ 25000000 /* Frame buffer descriptor */ struct fbd { @@ -83,8 +82,6 @@ struct atmel_isi { struct completion complete; /* ISI peripherial clock */ struct clk *pclk; - /* ISI_MCK, feed to camera sensor to generate pixel clock */ - struct clk *mck; unsigned int irq; struct isi_platform_data pdata; @@ -741,31 +738,6 @@ static void isi_camera_remove_device(struct soc_camera_device *icd) icd->devnum); } -/* Called with .host_lock held */ -static int isi_camera_clock_start(struct soc_camera_host *ici) -{ - struct atmel_isi *isi = ici->priv; - int ret; - - if (!IS_ERR(isi->mck)) { - ret = clk_prepare_enable(isi->mck); - if (ret) { - return ret; - } - } - - return 0; -} - -/* Called with .host_lock held */ -static void isi_camera_clock_stop(struct soc_camera_host *ici) -{ - struct atmel_isi *isi = ici->priv; - - if (!IS_ERR(isi->mck)) - clk_disable_unprepare(isi->mck); -} - static unsigned int isi_camera_poll(struct file *file, poll_table *pt) { struct soc_camera_device *icd = file->private_data; @@ -875,8 +847,6 @@ static struct soc_camera_host_ops isi_soc_camera_host_ops = { .owner = THIS_MODULE, .add = isi_camera_add_device, .remove = isi_camera_remove_device, - .clock_start = isi_camera_clock_start, - .clock_stop = isi_camera_clock_stop, .set_fmt = isi_camera_set_fmt, .try_fmt = isi_camera_try_fmt, .get_formats = isi_camera_get_formats, @@ -913,7 +883,6 @@ static int atmel_isi_probe_dt(struct atmel_isi *isi, /* Default settings for ISI */ isi->pdata.full_mode = 1; - isi->pdata.mck_hz = ISI_DEFAULT_MCLK_FREQ; isi->pdata.frate = ISI_CFG1_FRATE_CAPTURE_ALL; np = of_graph_get_next_endpoint(np, NULL); @@ -989,21 +958,6 @@ static int atmel_isi_probe(struct platform_device *pdev) INIT_LIST_HEAD(&isi->video_buffer_list); INIT_LIST_HEAD(&isi->dma_desc_head); - /* ISI_MCK is the sensor master clock. It should be handled by the - * sensor driver directly, as the ISI has no use for that clock. Make - * the clock optional here while platforms transition to the correct - * model. - */ - isi->mck = devm_clk_get(dev, "isi_mck"); - if (!IS_ERR(isi->mck)) { - /* Set ISI_MCK's frequency, it should be faster than pixel - * clock. - */ - ret = clk_set_rate(isi->mck, isi->pdata.mck_hz); - if (ret < 0) - return ret; - } - isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev, sizeof(struct fbd) * MAX_BUFFER_NUM, &isi->fb_descriptors_phys, -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-26 13:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-26 8:00 [PATCH v4 0/2] media: atmel-isi: rework on the clock part and add runtime pm support Josh Wu 2015-05-26 8:00 ` [PATCH v4 1/2] media: atmel-isi: " Josh Wu 2015-05-26 8:16 ` Laurent Pinchart 2015-05-26 9:14 ` Josh Wu 2015-05-26 9:58 ` Josh Wu 2015-05-26 8:00 ` [PATCH v4 2/2] media: atmel-isi: remove mck back compatiable code as it's not need Josh Wu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).