Hi Jacopo,
On Fri, Mar 11, 2022 at 05:34:43PM +0100, Jacopo Mondi wrote:
> On Fri, Mar 11, 2022 at 03:55:32PM +0200, Laurent Pinchart wrote:
> > The subdev .s_power() operation is deprecated. Drop it, requiring sensor
> > drivers to correctly use runtime PM instead of relying on .s_power().
> >
> > As this driver has just been moved out of staging, and necessary drivers
> > to implement a full camera pipeline are still in staging, no platform
> > depends yet on this API being called. There is thus no risk of
> > regression.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/platform/imx/imx-mipi-csis.c | 11 ++---------
> > 1 file changed, 2 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c
> > index 7baedc854186..6e06d19c1334 100644
> > --- a/drivers/media/platform/imx/imx-mipi-csis.c
> > +++ b/drivers/media/platform/imx/imx-mipi-csis.c
> > @@ -937,7 +937,7 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
> > static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > {
> > struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > - int ret;
> > + int ret = 0;
> >
> > if (enable) {
> > ret = mipi_csis_calculate_params(csis);
> > @@ -949,10 +949,6 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > ret = pm_runtime_resume_and_get(csis->dev);
> > if (ret < 0)
> > return ret;
> > -
> > - ret = v4l2_subdev_call(csis->src_sd, core, s_power, 1);
> > - if (ret < 0 && ret != -ENOIOCTLCMD)
> > - goto done;
> > }
> >
> > mutex_lock(&csis->lock);
> > @@ -973,9 +969,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > csis->state |= ST_STREAMING;
> > } else {
> > v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
> > - ret = v4l2_subdev_call(csis->src_sd, core, s_power, 0);
> > - if (ret == -ENOIOCTLCMD)
> > - ret = 0;
> > +
>
> I think mipi_csis_s_stream() could be simplified even more
>
> Now it looks like
>
> if (enable) {
> pm_resume_and_get();
> ...
> }
>
> mutex_lock();
>
> if (enable) {
>
> } else {
>
> }
>
> out:
> mutex_unlock()
> if (ret < 0 || !enable)
> pm_runtime_put()
>
> return ret;
>
> Would it look better as
>
> if (!enable) {
> lock();
> ...
> pm_runtime_put();
> ...
> unlock();
> return;
> }
>
> /* non critical section stuff */
> pm_resume_and_get();
>
> lock();
>
> /* critical section stuff */
> if (ret)
> goto out;
>
> unlock();
>
> return 0;
>
> out:
> pm_runtime_put();
> return ret;
Additional cleanup is likely possible, I haven't really looked at that.
I was happy anough with a first simplification :-)
> This patch is good though, so the rework if desired can be done on top
> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>
> > mipi_csis_stop_stream(csis);
> > csis->state &= ~ST_STREAMING;
> > if (csis->debug.enable)
> > @@ -985,7 +979,6 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > unlock:
> > mutex_unlock(&csis->lock);
> >
> > -done:
> > if (!enable || ret < 0)
> > pm_runtime_put(csis->dev);
> >
--
Regards,
Laurent Pinchart
