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