[REVIEW PATCH 0/3] Add unlocked v4l2_grab_ctrl(), use it in smiapp driver

[REVIEW PATCH 0/3] Add unlocked v4l2_grab_ctrl(), use it in smiapp driver

[Sakari Ailus]

Hi,

This patchset adds an unlocked variant of v4l2_grab_ctrl() which then is
used by the smiapp driver.


-- 
Regards,
Sakari

[REVIEW PATCH 1/3] v4l: Add an unlocked variant of v4l2_grab_ctrl()

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Commonly the control mutex is shared with the rest of the driver, which
already holds the mutex when accessing the control framework. Add
unlocked v4l2_grab_ctrl(), __v4l2_grab_ctrl() for this purpose.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c |    6 ++----
 include/media/v4l2-ctrls.h           |   13 ++++++++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 45c5b47..a80bc9f 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2344,14 +2344,13 @@ EXPORT_SYMBOL(v4l2_ctrl_activate);
 
    Just call this and the framework will block any attempts to change
    these controls. */
-void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
+void __v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
 {
 	bool old;
 
 	if (ctrl == NULL)
 		return;
 
-	v4l2_ctrl_lock(ctrl);
 	if (grabbed)
 		/* set V4L2_CTRL_FLAG_GRABBED */
 		old = test_and_set_bit(1, &ctrl->flags);
@@ -2360,9 +2359,8 @@ void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
 		old = test_and_clear_bit(1, &ctrl->flags);
 	if (old != grabbed)
 		send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_FLAGS);
-	v4l2_ctrl_unlock(ctrl);
 }
-EXPORT_SYMBOL(v4l2_ctrl_grab);
+EXPORT_SYMBOL(__v4l2_ctrl_grab);
 
 /* Log the control name and value */
 static void log_ctrl(const struct v4l2_ctrl *ctrl,
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 911f3e5..b50d1dd 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -607,6 +607,9 @@ struct v4l2_ctrl *v4l2_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id);
   */
 void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
 
+/** __v4l2_ctrl_grab() - Unlocked variant of v4l2_ctrl_grab() */
+void __v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
+
 /** v4l2_ctrl_grab() - Mark the control as grabbed or not grabbed.
   * @ctrl:	The control to (de)activate.
   * @grabbed:	True if the control should become grabbed.
@@ -620,7 +623,15 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
   * This function assumes that the control handler is not locked and will
   * take the lock itself.
   */
-void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
+static inline void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
+{
+	if (!ctrl)
+		return;
+
+	v4l2_ctrl_lock(ctrl);
+	__v4l2_ctrl_grab(ctrl, grabbed);
+	v4l2_ctrl_unlock(ctrl);
+}
 
 
 /** __v4l2_ctrl_modify_range() - Unlocked variant of v4l2_ctrl_modify_range() */
-- 
1.7.10.4

[REVIEW PATCH 2/3] smiapp: Correctly serialise stream start / stop

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The stream state was stored in sensor->streaming, but access to it was not
serialised properly. Fix this by moving the mutex to smiapp_set_stream().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   54 ++++++++++++++------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index d47eff5..e534f1b 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1391,28 +1391,26 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	mutex_lock(&sensor->mutex);
-
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_CSI_DATA_FORMAT,
 			    (sensor->csi_format->width << 8) |
 			    sensor->csi_format->compressed);
 	if (rval)
-		goto out;
+		return rval;
 
 	rval = smiapp_pll_configure(sensor);
 	if (rval)
-		goto out;
+		return rval;
 
 	/* Analog crop start coordinates */
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_X_ADDR_START,
 			    sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].left);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_Y_ADDR_START,
 			    sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].top);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	/* Analog crop end coordinates */
 	rval = smiapp_write(
@@ -1420,14 +1418,14 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 		sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].left
 		+ sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width - 1);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	rval = smiapp_write(
 		sensor, SMIAPP_REG_U16_Y_ADDR_END,
 		sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].top
 		+ sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height - 1);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	/*
 	 * Output from pixel array, including blanking, is set using
@@ -1441,25 +1439,25 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 			sensor, SMIAPP_REG_U16_DIGITAL_CROP_X_OFFSET,
 			sensor->scaler->crop[SMIAPP_PAD_SINK].left);
 		if (rval < 0)
-			goto out;
+			return rval;
 
 		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_DIGITAL_CROP_Y_OFFSET,
 			sensor->scaler->crop[SMIAPP_PAD_SINK].top);
 		if (rval < 0)
-			goto out;
+			return rval;
 
 		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_WIDTH,
 			sensor->scaler->crop[SMIAPP_PAD_SINK].width);
 		if (rval < 0)
-			goto out;
+			return rval;
 
 		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_HEIGHT,
 			sensor->scaler->crop[SMIAPP_PAD_SINK].height);
 		if (rval < 0)
-			goto out;
+			return rval;
 	}
 
 	/* Scaling */
@@ -1468,23 +1466,23 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 		rval = smiapp_write(sensor, SMIAPP_REG_U16_SCALING_MODE,
 				    sensor->scaling_mode);
 		if (rval < 0)
-			goto out;
+			return rval;
 
 		rval = smiapp_write(sensor, SMIAPP_REG_U16_SCALE_M,
 				    sensor->scale_m);
 		if (rval < 0)
-			goto out;
+			return rval;
 	}
 
 	/* Output size from sensor */
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_X_OUTPUT_SIZE,
 			    sensor->src->crop[SMIAPP_PAD_SRC].width);
 	if (rval < 0)
-		goto out;
+		return rval;
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_Y_OUTPUT_SIZE,
 			    sensor->src->crop[SMIAPP_PAD_SRC].height);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	if ((sensor->limits[SMIAPP_LIMIT_FLASH_MODE_CAPABILITY] &
 	     (SMIAPP_FLASH_MODE_CAPABILITY_SINGLE_STROBE |
@@ -1493,22 +1491,17 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 	    sensor->platform_data->strobe_setup->trigger != 0) {
 		rval = smiapp_setup_flash_strobe(sensor);
 		if (rval)
-			goto out;
+			return rval;
 	}
 
 	rval = smiapp_call_quirk(sensor, pre_streamon);
 	if (rval) {
 		dev_err(&client->dev, "pre_streamon quirks failed\n");
-		goto out;
+		return rval;
 	}
 
-	rval = smiapp_write(sensor, SMIAPP_REG_U8_MODE_SELECT,
+	return smiapp_write(sensor, SMIAPP_REG_U8_MODE_SELECT,
 			    SMIAPP_MODE_SELECT_STREAMING);
-
-out:
-	mutex_unlock(&sensor->mutex);
-
-	return rval;
 }
 
 static int smiapp_stop_streaming(struct smiapp_sensor *sensor)
@@ -1516,18 +1509,15 @@ static int smiapp_stop_streaming(struct smiapp_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	mutex_lock(&sensor->mutex);
 	rval = smiapp_write(sensor, SMIAPP_REG_U8_MODE_SELECT,
 			    SMIAPP_MODE_SELECT_SOFTWARE_STANDBY);
 	if (rval)
-		goto out;
+		return rval;
 
 	rval = smiapp_call_quirk(sensor, post_streamoff);
 	if (rval)
 		dev_err(&client->dev, "post_streamoff quirks failed\n");
 
-out:
-	mutex_unlock(&sensor->mutex);
 	return rval;
 }
 
@@ -1538,10 +1528,12 @@ out:
 static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int rval;
+	int rval = 0;
+
+	mutex_lock(&sensor->mutex);
 
 	if (sensor->streaming == enable)
-		return 0;
+		goto out;
 
 	if (enable) {
 		sensor->streaming = true;
@@ -1553,6 +1545,8 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 		sensor->streaming = false;
 	}
 
+out:
+	mutex_unlock(&sensor->mutex);
 	return rval;
 }
 
-- 
1.7.10.4

[REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Instead of returning -EBUSY in s_ctrl(), use __v4l2_ctrl_grab() to mark the
controls grabbed.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index e534f1b..6658f61 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -423,9 +423,6 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	case V4L2_CID_HFLIP:
 	case V4L2_CID_VFLIP:
-		if (sensor->streaming)
-			return -EBUSY;
-
 		if (sensor->hflip->val)
 			orient |= SMIAPP_IMAGE_ORIENTATION_HFLIP;
 
@@ -469,9 +466,6 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 			+ ctrl->val);
 
 	case V4L2_CID_LINK_FREQ:
-		if (sensor->streaming)
-			return -EBUSY;
-
 		return smiapp_pll_update(sensor);
 
 	case V4L2_CID_TEST_PATTERN: {
@@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 	if (sensor->streaming == enable)
 		goto out;
 
-	if (enable) {
-		sensor->streaming = true;
+	if (enable)
 		rval = smiapp_start_streaming(sensor);
-		if (rval < 0)
-			sensor->streaming = false;
-	} else {
+	else
 		rval = smiapp_stop_streaming(sensor);
-		sensor->streaming = false;
-	}
+
+	sensor->streaming = enable;
+	__v4l2_ctrl_grab(sensor->hflip, enable);
+	__v4l2_ctrl_grab(sensor->vflip, enable);
+	__v4l2_ctrl_grab(sensor->link_freq, enable);
 
 out:
 	mutex_unlock(&sensor->mutex);
-- 
1.7.10.4

Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls

[Hans Verkuil]

Hi Sakari,

Thanks for the patch series, but I have one question, see below.

On 02/25/15 13:33, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Instead of returning -EBUSY in s_ctrl(), use __v4l2_ctrl_grab() to mark the
> controls grabbed.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c |   20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index e534f1b..6658f61 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -423,9 +423,6 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	case V4L2_CID_HFLIP:
>  	case V4L2_CID_VFLIP:
> -		if (sensor->streaming)
> -			return -EBUSY;
> -
>  		if (sensor->hflip->val)
>  			orient |= SMIAPP_IMAGE_ORIENTATION_HFLIP;
>  
> @@ -469,9 +466,6 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
>  			+ ctrl->val);
>  
>  	case V4L2_CID_LINK_FREQ:
> -		if (sensor->streaming)
> -			return -EBUSY;
> -
>  		return smiapp_pll_update(sensor);
>  
>  	case V4L2_CID_TEST_PATTERN: {
> @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
>  	if (sensor->streaming == enable)
>  		goto out;
>  
> -	if (enable) {
> -		sensor->streaming = true;
> +	if (enable)
>  		rval = smiapp_start_streaming(sensor);
> -		if (rval < 0)
> -			sensor->streaming = false;
> -	} else {
> +	else
>  		rval = smiapp_stop_streaming(sensor);
> -		sensor->streaming = false;
> -	}
> +
> +	sensor->streaming = enable;
> +	__v4l2_ctrl_grab(sensor->hflip, enable);
> +	__v4l2_ctrl_grab(sensor->vflip, enable);
> +	__v4l2_ctrl_grab(sensor->link_freq, enable);

Just checking: is it really not possible to change these controls
while streaming? Most devices I know of allow changing this on the fly.

If it is really not possible, then you can add my Ack for this series:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

>  
>  out:
>  	mutex_unlock(&sensor->mutex);
> 

Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls

[Sakari Ailus]

Hi Hans,

On Wed, Feb 25, 2015 at 02:45:58PM +0100, Hans Verkuil wrote:
...
> > @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
> >  	if (sensor->streaming == enable)
> >  		goto out;
> >  
> > -	if (enable) {
> > -		sensor->streaming = true;
> > +	if (enable)
> >  		rval = smiapp_start_streaming(sensor);
> > -		if (rval < 0)
> > -			sensor->streaming = false;
> > -	} else {
> > +	else
> >  		rval = smiapp_stop_streaming(sensor);
> > -		sensor->streaming = false;
> > -	}
> > +
> > +	sensor->streaming = enable;
> > +	__v4l2_ctrl_grab(sensor->hflip, enable);
> > +	__v4l2_ctrl_grab(sensor->vflip, enable);
> > +	__v4l2_ctrl_grab(sensor->link_freq, enable);
> 
> Just checking: is it really not possible to change these controls
> while streaming? Most devices I know of allow changing this on the fly.
> 
> If it is really not possible, then you can add my Ack for this series:

I'm not sure what the sensors would do in practice, but the problem is that
changing the values of these control affect the pixel order. That's why
changing them has been prevented while streaming.

Thanks for the ack!

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls

[Hans Verkuil]

On 02/25/15 14:57, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Feb 25, 2015 at 02:45:58PM +0100, Hans Verkuil wrote:
> ...
>>> @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
>>>  	if (sensor->streaming == enable)
>>>  		goto out;
>>>  
>>> -	if (enable) {
>>> -		sensor->streaming = true;
>>> +	if (enable)
>>>  		rval = smiapp_start_streaming(sensor);
>>> -		if (rval < 0)
>>> -			sensor->streaming = false;
>>> -	} else {
>>> +	else
>>>  		rval = smiapp_stop_streaming(sensor);
>>> -		sensor->streaming = false;
>>> -	}
>>> +
>>> +	sensor->streaming = enable;
>>> +	__v4l2_ctrl_grab(sensor->hflip, enable);
>>> +	__v4l2_ctrl_grab(sensor->vflip, enable);
>>> +	__v4l2_ctrl_grab(sensor->link_freq, enable);
>>
>> Just checking: is it really not possible to change these controls
>> while streaming? Most devices I know of allow changing this on the fly.
>>
>> If it is really not possible, then you can add my Ack for this series:
> 
> I'm not sure what the sensors would do in practice, but the problem is that
> changing the values of these control affect the pixel order. That's why
> changing them has been prevented while streaming.

Ah, OK.

Can you add a comment explaining why this is done?

BTW, I understand that HFLIP will cause changes in the pixel order,
but VFLIP and link_freq should be OK, I would expect.

Regards,

	Hans

Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls

[Sakari Ailus]

Hi Hans,

On Wed, Feb 25, 2015 at 03:14:16PM +0100, Hans Verkuil wrote:
> On 02/25/15 14:57, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Feb 25, 2015 at 02:45:58PM +0100, Hans Verkuil wrote:
> > ...
> >>> @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
> >>>  	if (sensor->streaming == enable)
> >>>  		goto out;
> >>>  
> >>> -	if (enable) {
> >>> -		sensor->streaming = true;
> >>> +	if (enable)
> >>>  		rval = smiapp_start_streaming(sensor);
> >>> -		if (rval < 0)
> >>> -			sensor->streaming = false;
> >>> -	} else {
> >>> +	else
> >>>  		rval = smiapp_stop_streaming(sensor);
> >>> -		sensor->streaming = false;
> >>> -	}
> >>> +
> >>> +	sensor->streaming = enable;
> >>> +	__v4l2_ctrl_grab(sensor->hflip, enable);
> >>> +	__v4l2_ctrl_grab(sensor->vflip, enable);
> >>> +	__v4l2_ctrl_grab(sensor->link_freq, enable);
> >>
> >> Just checking: is it really not possible to change these controls
> >> while streaming? Most devices I know of allow changing this on the fly.
> >>
> >> If it is really not possible, then you can add my Ack for this series:
> > 
> > I'm not sure what the sensors would do in practice, but the problem is that
> > changing the values of these control affect the pixel order. That's why
> > changing them has been prevented while streaming.
> 
> Ah, OK.
> 
> Can you add a comment explaining why this is done?
> 
> BTW, I understand that HFLIP will cause changes in the pixel order,
> but VFLIP and link_freq should be OK, I would expect.

Sure I can add a comment. Same for vflip, it will change the pixel order.
The flip controls will change the readout direction. For example, a 4x4
bayer sensor:

GRGR
BGBG
GRGR
BGBG

Without flipping, the readout of the first line will be GRGR while the
second is BGBG. With vertical flipping, the first line read out from the
sensor will be BGBG and the second GRGR.

The link frequency cannot be changed since this would change the sensor PLL
configuration and the CSI-2 bus frequency, neither of which are changeable
while streaming.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [REVIEW PATCH 3/3] smiapp: Use __v4l2_ctrl_grab() to grab controls

[Hans Verkuil]

On 02/25/15 15:22, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Feb 25, 2015 at 03:14:16PM +0100, Hans Verkuil wrote:
>> On 02/25/15 14:57, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Wed, Feb 25, 2015 at 02:45:58PM +0100, Hans Verkuil wrote:
>>> ...
>>>>> @@ -1535,15 +1529,15 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
>>>>>  	if (sensor->streaming == enable)
>>>>>  		goto out;
>>>>>  
>>>>> -	if (enable) {
>>>>> -		sensor->streaming = true;
>>>>> +	if (enable)
>>>>>  		rval = smiapp_start_streaming(sensor);
>>>>> -		if (rval < 0)
>>>>> -			sensor->streaming = false;
>>>>> -	} else {
>>>>> +	else
>>>>>  		rval = smiapp_stop_streaming(sensor);
>>>>> -		sensor->streaming = false;
>>>>> -	}
>>>>> +
>>>>> +	sensor->streaming = enable;
>>>>> +	__v4l2_ctrl_grab(sensor->hflip, enable);
>>>>> +	__v4l2_ctrl_grab(sensor->vflip, enable);
>>>>> +	__v4l2_ctrl_grab(sensor->link_freq, enable);
>>>>
>>>> Just checking: is it really not possible to change these controls
>>>> while streaming? Most devices I know of allow changing this on the fly.
>>>>
>>>> If it is really not possible, then you can add my Ack for this series:
>>>
>>> I'm not sure what the sensors would do in practice, but the problem is that
>>> changing the values of these control affect the pixel order. That's why
>>> changing them has been prevented while streaming.
>>
>> Ah, OK.
>>
>> Can you add a comment explaining why this is done?
>>
>> BTW, I understand that HFLIP will cause changes in the pixel order,
>> but VFLIP and link_freq should be OK, I would expect.
> 
> Sure I can add a comment. Same for vflip, it will change the pixel order.
> The flip controls will change the readout direction. For example, a 4x4
> bayer sensor:
> 
> GRGR
> BGBG
> GRGR
> BGBG
> 
> Without flipping, the readout of the first line will be GRGR while the
> second is BGBG. With vertical flipping, the first line read out from the
> sensor will be BGBG and the second GRGR.

Ah, of course. A comment would be useful indeed as this is not immediately
obvious (well, not to me at least!).

> 
> The link frequency cannot be changed since this would change the sensor PLL
> configuration and the CSI-2 bus frequency, neither of which are changeable
> while streaming.
> 

Sorry, my mistake. I confused the link frequency with the powerline frequency
control (50/60 Hz). Of course the link frequency can't be changed while
streaming.

Regards,

	Hans