diff mbox series

[05/57] media: i2c: imx214: Drop check for reentrant .s_stream()

Message ID 20230914181704.4811-6-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: i2c: Reduce cargo cult | expand

Commit Message

Laurent Pinchart Sept. 14, 2023, 6:16 p.m. UTC
The subdev .s_stream() operation shall not be called to start streaming
on an already started subdev, or stop streaming on a stopped subdev.
Remove the check that guards against that condition.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx214.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Ricardo Ribalda Delgado Sept. 18, 2023, 7:52 a.m. UTC | #1
Hi Laurent

On Thu, Sep 14, 2023 at 8:17 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The subdev .s_stream() operation shall not be called to start streaming
> on an already started subdev, or stop streaming on a stopped subdev.
> Remove the check that guards against that condition.
>

In general I agree with the patch, but I think it would be safer to
land it in two stages.

first:

       if WARN_ON(imx214->streaming == enable)
               return 0;

and in the next version replace it completely.

otherwise

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/i2c/imx214.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 2f9c8582f940..e2805173f4b1 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -775,9 +775,6 @@ static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
>         struct imx214 *imx214 = to_imx214(subdev);
>         int ret;
>
> -       if (imx214->streaming == enable)
> -               return 0;
> -
>         if (enable) {
>                 ret = pm_runtime_resume_and_get(imx214->dev);
>                 if (ret < 0)
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Sept. 18, 2023, 8:07 a.m. UTC | #2
Hi Ricardo,

On Mon, Sep 18, 2023 at 09:52:32AM +0200, Ricardo Ribalda Delgado wrote:
> On Thu, Sep 14, 2023 at 8:17 PM Laurent Pinchart wrote:
> >
> > The subdev .s_stream() operation shall not be called to start streaming
> > on an already started subdev, or stop streaming on a stopped subdev.
> > Remove the check that guards against that condition.
> >
> 
> In general I agree with the patch, but I think it would be safer to
> land it in two stages.
> 
> first:
> 
>        if WARN_ON(imx214->streaming == enable)
>                return 0;
> 
> and in the next version replace it completely.

I don't want to send another series of 57 patches to remove the
WARN_ON() in a few kernel versions :-S Fortunately, it seems we have a
better option, I can centralize the check in call_s_stream() !

And, on a side note, how many kernel versions would we need before
removing the WARN_ON() ?

Something I have on my radar is replacing all direct .s_stream() calls
with v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams(), but
that's more work than I can tackle at the moment.

> otherwise
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/i2c/imx214.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 2f9c8582f940..e2805173f4b1 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -775,9 +775,6 @@ static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
> >         struct imx214 *imx214 = to_imx214(subdev);
> >         int ret;
> >
> > -       if (imx214->streaming == enable)
> > -               return 0;
> > -
> >         if (enable) {
> >                 ret = pm_runtime_resume_and_get(imx214->dev);
> >                 if (ret < 0)
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 2f9c8582f940..e2805173f4b1 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -775,9 +775,6 @@  static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
 	struct imx214 *imx214 = to_imx214(subdev);
 	int ret;
 
-	if (imx214->streaming == enable)
-		return 0;
-
 	if (enable) {
 		ret = pm_runtime_resume_and_get(imx214->dev);
 		if (ret < 0)