Hi Tomi,
On Jun 28, 2024 at 11:14:50 +0300, Tomi Valkeinen wrote:
> Hi,
>
> On 27/06/2024 16:09, Jai Luthra wrote:
> > From: Jayshri Pawar <jpawar@xxxxxxxxxxx>
> >
> > Use runtime power management hooks to save power when CSI-RX is not in
> > use. Also stop/start any in-progress streams, which might happen during
> > a system suspend/resume cycle.
> >
> > Signed-off-by: Jayshri Pawar <jpawar@xxxxxxxxxxx>
> > Co-developed-by: Jai Luthra <j-luthra@xxxxxx>
> > Signed-off-by: Jai Luthra <j-luthra@xxxxxx>
> > ---
> > drivers/media/platform/cadence/cdns-csi2rx.c | 43 +++++++++++++++++++++++++++-
> > 1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 6f7d27a48eff..751eadbe61ef 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -366,6 +366,12 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> > struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > int ret = 0;
> > + if (enable) {
> > + ret = pm_runtime_resume_and_get(csi2rx->dev);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > mutex_lock(&csi2rx->lock);
> > if (enable) {
> > @@ -375,8 +381,10 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> > */
> > if (!csi2rx->count) {
> > ret = csi2rx_start(csi2rx);
> > - if (ret)
> > + if (ret) {
> > + pm_runtime_put(csi2rx->dev);
> > goto out;
> > + }
> > }
> > csi2rx->count++;
> > @@ -388,6 +396,8 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> > */
> > if (!csi2rx->count)
> > csi2rx_stop(csi2rx);
> > +
> > + pm_runtime_put(csi2rx->dev);
> > }
> > out:
> > @@ -661,6 +671,29 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> > return ret;
> > }
> > +static int csi2rx_suspend(struct device *dev)
> > +{
> > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> > +
> > + mutex_lock(&csi2rx->lock);
> > + if (csi2rx->count)
> > + csi2rx_stop(csi2rx);
> > + mutex_unlock(&csi2rx->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int csi2rx_resume(struct device *dev)
> > +{
> > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> > +
> > + mutex_lock(&csi2rx->lock);
> > + if (csi2rx->count)
> > + csi2rx_start(csi2rx);
> > + mutex_unlock(&csi2rx->lock);
> > + return 0;
> > +}
> > +
>
> I don't think this looks correct. Afaik the runtime suspend/resume is not
> called on system suspend/resume. You could change the SET_RUNTIME_PM_OPS to
> use the same callbacks for runtime and system suspend, but I think that's a
> bad idea. Runtime suspend is not supposed to turn off the streaming. The
> driver is supposed to turn off the streaming, then call runtime_put, which
> would result in runtime suspend callback getting called.
Agreed, runtime PM should only be called once streams are stopped and
device is not in use.
These hooks look like system suspend/resume hooks to me.
I don't think this patch makes much sense in this series anyway, I was
carrying it to bring the drivers at parity with the vendor kernel.
I will drop this from my series and let Changhuang carry it in his
series for overall PM support for cdns-csi2rx.
>
> Tomi
>
> > static int csi2rx_probe(struct platform_device *pdev)
> > {
> > struct csi2rx_priv *csi2rx;
> > @@ -707,6 +740,7 @@ static int csi2rx_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_cleanup;
> > + pm_runtime_enable(csi2rx->dev);
> > ret = v4l2_async_register_subdev(&csi2rx->subdev);
> > if (ret < 0)
> > goto err_free_state;
> > @@ -721,6 +755,7 @@ static int csi2rx_probe(struct platform_device *pdev)
> > err_free_state:
> > v4l2_subdev_cleanup(&csi2rx->subdev);
> > + pm_runtime_disable(csi2rx->dev);
> > err_cleanup:
> > v4l2_async_nf_unregister(&csi2rx->notifier);
> > v4l2_async_nf_cleanup(&csi2rx->notifier);
> > @@ -739,9 +774,14 @@ static void csi2rx_remove(struct platform_device *pdev)
> > v4l2_async_unregister_subdev(&csi2rx->subdev);
> > v4l2_subdev_cleanup(&csi2rx->subdev);
> > media_entity_cleanup(&csi2rx->subdev.entity);
> > + pm_runtime_disable(csi2rx->dev);
> > kfree(csi2rx);
> > }
> > +static const struct dev_pm_ops csi2rx_pm_ops = {
> > + SET_RUNTIME_PM_OPS(csi2rx_suspend, csi2rx_resume, NULL)
> > +};
> > +
> > static const struct of_device_id csi2rx_of_table[] = {
> > { .compatible = "starfive,jh7110-csi2rx" },
> > { .compatible = "cdns,csi2rx" },
> > @@ -756,6 +796,7 @@ static struct platform_driver csi2rx_driver = {
> > .driver = {
> > .name = "cdns-csi2rx",
> > .of_match_table = csi2rx_of_table,
> > + .pm = &csi2rx_pm_ops,
> > },
> > };
> > module_platform_driver(csi2rx_driver);
> >
>
--
Thanks,
Jai
GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145
