| Message ID | 20231122-imx-csis-v1-2-0617368eb996@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx-mipi-csis: csis clock fixes | expand |
Hi Tomi, Thank you for the patch. On Wed, Nov 22, 2023 at 03:13:49PM +0200, Tomi Valkeinen wrote: > The driver always enables the clocks at probe() and disables them only > at remove(). It is not clear why the driver does this, as it supports > runtime PM, and enables and disables the clocks in the runtime resume > and suspend callbacks. Also, in the case runtime PM is not available, > the driver calls the resume and suspend callbacks manually from probe() > and remove(). Probably a historical mistake. It predates my involvement with the driver :-) > Drop the unnecessary clock enable, thus enabling the clocks only when > actually needed. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/nxp/imx-mipi-csis.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c > index b39d7aeba750..b08f6d2e7516 100644 > --- a/drivers/media/platform/nxp/imx-mipi-csis.c > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c > @@ -1435,24 +1435,18 @@ static int mipi_csis_probe(struct platform_device *pdev) > /* Reset PHY and enable the clocks. */ > mipi_csis_phy_reset(csis); > > - ret = mipi_csis_clk_enable(csis); > - if (ret < 0) { > - dev_err(csis->dev, "failed to enable clocks: %d\n", ret); > - return ret; > - } > - > /* Now that the hardware is initialized, request the interrupt. */ > ret = devm_request_irq(dev, irq, mipi_csis_irq_handler, 0, > dev_name(dev), csis); > if (ret) { > dev_err(dev, "Interrupt request failed\n"); > - goto err_disable_clock; > + return ret; > } > > /* Initialize and register the subdev. */ > ret = mipi_csis_subdev_init(csis); > if (ret < 0) > - goto err_disable_clock; > + return ret; > > platform_set_drvdata(pdev, &csis->sd); > > @@ -1486,8 +1480,6 @@ static int mipi_csis_probe(struct platform_device *pdev) > v4l2_async_nf_unregister(&csis->notifier); > v4l2_async_nf_cleanup(&csis->notifier); > v4l2_async_unregister_subdev(&csis->sd); > -err_disable_clock: > - mipi_csis_clk_disable(csis); > > return ret; > } > @@ -1506,7 +1498,6 @@ static void mipi_csis_remove(struct platform_device *pdev) > mipi_csis_runtime_suspend(&pdev->dev); > > pm_runtime_disable(&pdev->dev); > - mipi_csis_clk_disable(csis); > v4l2_subdev_cleanup(&csis->sd); > media_entity_cleanup(&csis->sd.entity); > pm_runtime_set_suspended(&pdev->dev); >
diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c index b39d7aeba750..b08f6d2e7516 100644 --- a/drivers/media/platform/nxp/imx-mipi-csis.c +++ b/drivers/media/platform/nxp/imx-mipi-csis.c @@ -1435,24 +1435,18 @@ static int mipi_csis_probe(struct platform_device *pdev) /* Reset PHY and enable the clocks. */ mipi_csis_phy_reset(csis); - ret = mipi_csis_clk_enable(csis); - if (ret < 0) { - dev_err(csis->dev, "failed to enable clocks: %d\n", ret); - return ret; - } - /* Now that the hardware is initialized, request the interrupt. */ ret = devm_request_irq(dev, irq, mipi_csis_irq_handler, 0, dev_name(dev), csis); if (ret) { dev_err(dev, "Interrupt request failed\n"); - goto err_disable_clock; + return ret; } /* Initialize and register the subdev. */ ret = mipi_csis_subdev_init(csis); if (ret < 0) - goto err_disable_clock; + return ret; platform_set_drvdata(pdev, &csis->sd); @@ -1486,8 +1480,6 @@ static int mipi_csis_probe(struct platform_device *pdev) v4l2_async_nf_unregister(&csis->notifier); v4l2_async_nf_cleanup(&csis->notifier); v4l2_async_unregister_subdev(&csis->sd); -err_disable_clock: - mipi_csis_clk_disable(csis); return ret; } @@ -1506,7 +1498,6 @@ static void mipi_csis_remove(struct platform_device *pdev) mipi_csis_runtime_suspend(&pdev->dev); pm_runtime_disable(&pdev->dev); - mipi_csis_clk_disable(csis); v4l2_subdev_cleanup(&csis->sd); media_entity_cleanup(&csis->sd.entity); pm_runtime_set_suspended(&pdev->dev);
The driver always enables the clocks at probe() and disables them only at remove(). It is not clear why the driver does this, as it supports runtime PM, and enables and disables the clocks in the runtime resume and suspend callbacks. Also, in the case runtime PM is not available, the driver calls the resume and suspend callbacks manually from probe() and remove(). Drop the unnecessary clock enable, thus enabling the clocks only when actually needed. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/nxp/imx-mipi-csis.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)