diff mbox

media: i2c: adv748x: Store the pixel rate ctrl on CSI objects

Message ID 1501768223-23654-1-git-send-email-kbingham@kernel.org (mailing list archive)
State New, archived
Headers show

Commit Message

Kieran Bingham Aug. 3, 2017, 1:50 p.m. UTC
From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The current implementation has to search the list of controls for the
pixel rate control, each time it is set.  This can be optimised easily
by storing the ctrl pointer in the CSI/TX object, and referencing that
directly.

While at it, fix up a missing blank line also highlighted in review
comments.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Small enhancement and fixup as suggested by Sakari, after driver acceptance.

Niklas, with my current 8 Camera set up - I can't fully test this change.
Could you give it a spin if you get chance please?

 drivers/media/i2c/adv748x/adv748x-afe.c  |  1 +
 drivers/media/i2c/adv748x/adv748x-csi2.c | 15 +++++++--------
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 3 files changed, 9 insertions(+), 8 deletions(-)

Comments

Niklas Söderlund Aug. 3, 2017, 2:32 p.m. UTC | #1
Hi Kieran,

On 2017-08-03 14:50:23 +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The current implementation has to search the list of controls for the
> pixel rate control, each time it is set.  This can be optimised easily
> by storing the ctrl pointer in the CSI/TX object, and referencing that
> directly.
> 
> While at it, fix up a missing blank line also highlighted in review
> comments.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

It won't apply cleanly to the media-tree since there already is a commit 
there which cleans-up the unused variable in dv748x_csi2_init_controls()

Apart from that:

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> Small enhancement and fixup as suggested by Sakari, after driver acceptance.
> 
> Niklas, with my current 8 Camera set up - I can't fully test this change.
> Could you give it a spin if you get chance please?
> 
>  drivers/media/i2c/adv748x/adv748x-afe.c  |  1 +
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 15 +++++++--------
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index b33ccfc08708..134d981d69d3 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -262,6 +262,7 @@ static int adv748x_afe_g_input_status(struct v4l2_subdev *sd, u32 *status)
>  	ret = adv748x_afe_status(afe, status, NULL);
>  
>  	mutex_unlock(&state->mutex);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index b4fee7f52d6a..609d960c0749 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -223,13 +223,12 @@ static const struct v4l2_subdev_ops adv748x_csi2_ops = {
>  
>  int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate)
>  {
> -	struct v4l2_ctrl *ctrl;
> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>  
> -	ctrl = v4l2_ctrl_find(sd->ctrl_handler, V4L2_CID_PIXEL_RATE);
> -	if (!ctrl)
> +	if (!tx->pixel_rate)
>  		return -EINVAL;
>  
> -	return v4l2_ctrl_s_ctrl_int64(ctrl, rate);
> +	return v4l2_ctrl_s_ctrl_int64(tx->pixel_rate, rate);
>  }
>  
>  static int adv748x_csi2_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -248,12 +247,12 @@ static const struct v4l2_ctrl_ops adv748x_csi2_ctrl_ops = {
>  
>  static int adv748x_csi2_init_controls(struct adv748x_csi2 *tx)
>  {
> -	struct v4l2_ctrl *ctrl;
> -
>  	v4l2_ctrl_handler_init(&tx->ctrl_hdl, 1);
>  
> -	ctrl = v4l2_ctrl_new_std(&tx->ctrl_hdl, &adv748x_csi2_ctrl_ops,
> -				 V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1, 1);
> +	tx->pixel_rate = v4l2_ctrl_new_std(&tx->ctrl_hdl,
> +					   &adv748x_csi2_ctrl_ops,
> +					   V4L2_CID_PIXEL_RATE, 1, INT_MAX,
> +					   1, 1);
>  
>  	tx->sd.ctrl_handler = &tx->ctrl_hdl;
>  	if (tx->ctrl_hdl.error) {
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index cc4151b5b31e..6789e2f3bc8c 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -97,6 +97,7 @@ struct adv748x_csi2 {
>  
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;
> +	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_subdev sd;
>  };
>  
> -- 
> 2.7.4
>
Sakari Ailus Aug. 4, 2017, 5:09 p.m. UTC | #2
Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The current implementation has to search the list of controls for the
> pixel rate control, each time it is set.  This can be optimised easily
> by storing the ctrl pointer in the CSI/TX object, and referencing that
> directly.
> 
> While at it, fix up a missing blank line also highlighted in review
> comments.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
diff mbox

Patch

diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index b33ccfc08708..134d981d69d3 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -262,6 +262,7 @@  static int adv748x_afe_g_input_status(struct v4l2_subdev *sd, u32 *status)
 	ret = adv748x_afe_status(afe, status, NULL);
 
 	mutex_unlock(&state->mutex);
+
 	return ret;
 }
 
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index b4fee7f52d6a..609d960c0749 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -223,13 +223,12 @@  static const struct v4l2_subdev_ops adv748x_csi2_ops = {
 
 int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate)
 {
-	struct v4l2_ctrl *ctrl;
+	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
 
-	ctrl = v4l2_ctrl_find(sd->ctrl_handler, V4L2_CID_PIXEL_RATE);
-	if (!ctrl)
+	if (!tx->pixel_rate)
 		return -EINVAL;
 
-	return v4l2_ctrl_s_ctrl_int64(ctrl, rate);
+	return v4l2_ctrl_s_ctrl_int64(tx->pixel_rate, rate);
 }
 
 static int adv748x_csi2_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -248,12 +247,12 @@  static const struct v4l2_ctrl_ops adv748x_csi2_ctrl_ops = {
 
 static int adv748x_csi2_init_controls(struct adv748x_csi2 *tx)
 {
-	struct v4l2_ctrl *ctrl;
-
 	v4l2_ctrl_handler_init(&tx->ctrl_hdl, 1);
 
-	ctrl = v4l2_ctrl_new_std(&tx->ctrl_hdl, &adv748x_csi2_ctrl_ops,
-				 V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1, 1);
+	tx->pixel_rate = v4l2_ctrl_new_std(&tx->ctrl_hdl,
+					   &adv748x_csi2_ctrl_ops,
+					   V4L2_CID_PIXEL_RATE, 1, INT_MAX,
+					   1, 1);
 
 	tx->sd.ctrl_handler = &tx->ctrl_hdl;
 	if (tx->ctrl_hdl.error) {
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index cc4151b5b31e..6789e2f3bc8c 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -97,6 +97,7 @@  struct adv748x_csi2 {
 
 	struct media_pad pads[ADV748X_CSI2_NR_PADS];
 	struct v4l2_ctrl_handler ctrl_hdl;
+	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_subdev sd;
 };