Re: [PATCH] media: i2c: adv748x: Store the pixel rate ctrl on CSI objects

[Sakari Ailus]

Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> 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 

Acked-by: Sakari Ailus 

-- 
Sakari Ailus
sakari.ai...@iki.fi

Re: [PATCH] media: i2c: adv748x: Store the pixel rate ctrl on CSI objects

[Niklas Söderlund]

Hi Kieran,

On 2017-08-03 14:50:23 +0100, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> 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 

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 

> ---
> 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(>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(>ctrl_hdl, 1);
>  
> - ctrl = v4l2_ctrl_new_std(>ctrl_hdl, _csi2_ctrl_ops,
> -  V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1, 1);
> + tx->pixel_rate = v4l2_ctrl_new_std(>ctrl_hdl,
> +_csi2_ctrl_ops,
> +V4L2_CID_PIXEL_RATE, 1, INT_MAX,
> +1, 1);
>  
>   tx->sd.ctrl_handler = >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
> 

-- 
Regards,
Niklas Söderlund