diff mbox series

[v8,1/4] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to __v4l2_ctrl_modify_range

Message ID 20230106072931.2317597-2-shravan.chippa@microchip.com
State New
Headers
Series media: i2c: imx334: support lower bandwidth mode |

Commit Message

shravan chippa Jan. 6, 2023, 7:29 a.m. UTC 
  From: Shravan Chippa <shravan.chippa@microchip.com>

For evry mode we will get new set of values for hbalnk so use
__v4l2_ctrl_modify_range() to support multi modes for hblank.

The hblank value is readonly in the driver. because of this the function
returns error if we try to change. so added dumy return case in
imx334_set_ctrl function

Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Jan. 6, 2023, 9:45 a.m. UTC | #1 
Hi Sharavan,

I'm a bit confused here

On Fri, Jan 06, 2023 at 12:59:28PM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> For evry mode we will get new set of values for hbalnk so use
> __v4l2_ctrl_modify_range() to support multi modes for hblank.
>
> The hblank value is readonly in the driver. because of this the function
> returns error if we try to change. so added dumy return case in
> imx334_set_ctrl function
>
> Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 7b0a9086447d..ebacba3059b3 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -382,7 +382,8 @@ static int imx334_update_controls(struct imx334 *imx334,
>  	if (ret)
>  		return ret;
>
> -	ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> +	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
> +				       mode->hblank, 1, mode->hblank);
>  	if (ret)
>  		return ret;
>
> @@ -480,6 +481,9 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
>
>  		pm_runtime_put(imx334->dev);
>
> +		break;
> +	case V4L2_CID_HBLANK:
> +		ret = 0;

Hblank is said to be read-only

	if (imx334->hblank_ctrl)
		imx334->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;

So you shouldn't need this safety measure here.

However I see that __v4l2_ctrl_modify_range() can call s_ctrl() if the
current value has to be adjusted to the new limits.

Hans, how does this work ? Do we need the above even if the control is
said to be RO ?

Sharavan: have you experienced failures here, or is this just for
safety ?


>  		break;
>  	default:
>  		dev_err(imx334->dev, "Invalid control %d", ctrl->id);
> --
> 2.34.1
>
shravan chippa Jan. 6, 2023, 11:58 a.m. UTC | #2 
> -----Original Message-----
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Sent: 06 January 2023 03:15 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>; Hans Verkuil
> <hverkuil@xs4all.nl>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v8 1/4] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to
> __v4l2_ctrl_modify_range
> 
> [You don't often get email from jacopo.mondi@ideasonboard.com. Learn
> why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Sharavan,
> 
> I'm a bit confused here
> 
> On Fri, Jan 06, 2023 at 12:59:28PM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > For evry mode we will get new set of values for hbalnk so use
> > __v4l2_ctrl_modify_range() to support multi modes for hblank.
> >
> > The hblank value is readonly in the driver. because of this the
> > function returns error if we try to change. so added dumy return case
> > in imx334_set_ctrl function
> >
> > Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  drivers/media/i2c/imx334.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 7b0a9086447d..ebacba3059b3 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -382,7 +382,8 @@ static int imx334_update_controls(struct imx334
> *imx334,
> >       if (ret)
> >               return ret;
> >
> > -     ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> > +     ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
> > +                                    mode->hblank, 1, mode->hblank);
> >       if (ret)
> >               return ret;
> >
> > @@ -480,6 +481,9 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >               pm_runtime_put(imx334->dev);
> >
> > +             break;
> > +     case V4L2_CID_HBLANK:
> > +             ret = 0;
> 
> Hblank is said to be read-only

Yes, read-only.
> 
>         if (imx334->hblank_ctrl)
>                 imx334->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> 
> So you shouldn't need this safety measure here.
> 
> However I see that __v4l2_ctrl_modify_range() can call s_ctrl() if the current
> value has to be adjusted to the new limits.
> 
> Hans, how does this work ? Do we need the above even if the control is said
> to be RO ?
> 
> Sharavan: have you experienced failures here, or is this just for safety ?

The value is changing if we change the mode. 
While changing mode. I have experienced failures.

Thanks,
Shravan.

> 
> 
> >               break;
> >       default:
> >               dev_err(imx334->dev, "Invalid control %d", ctrl->id);
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 7b0a9086447d..ebacba3059b3 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -382,7 +382,8 @@  static int imx334_update_controls(struct imx334 *imx334,
 	if (ret)
 		return ret;
 
-	ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
+	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
+				       mode->hblank, 1, mode->hblank);
 	if (ret)
 		return ret;
 
@@ -480,6 +481,9 @@  static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		pm_runtime_put(imx334->dev);
 
+		break;
+	case V4L2_CID_HBLANK:
+		ret = 0;
 		break;
 	default:
 		dev_err(imx334->dev, "Invalid control %d", ctrl->id);