Hi Yong,
On Tue, Jan 15, 2019 at 12:38 PM Yong Zhi <yong.zhi@xxxxxxxxx> wrote:
>
> This addresses the below TODO item.
> - Use V4L2_CTRL_TYPE_MENU for dual-pipe mode control. (Sakari)
>
> Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx>
> ---
> drivers/staging/media/ipu3/TODO | 2 --
> drivers/staging/media/ipu3/include/intel-ipu3.h | 6 ------
> drivers/staging/media/ipu3/ipu3-v4l2.c | 18 +++++++++++++-----
> 3 files changed, 13 insertions(+), 13 deletions(-)
>
Thanks for the patch. Please see my comments inline.
> diff --git a/drivers/staging/media/ipu3/TODO b/drivers/staging/media/ipu3/TODO
> index 905bbb190217..0dc9a2e79978 100644
> --- a/drivers/staging/media/ipu3/TODO
> +++ b/drivers/staging/media/ipu3/TODO
> @@ -11,8 +11,6 @@ staging directory.
> - Prefix imgu for all public APIs, i.e. change ipu3_v4l2_register() to
> imgu_v4l2_register(). (Sakari)
>
> -- Use V4L2_CTRL_TYPE_MENU for dual-pipe mode control. (Sakari)
> -
> - IPU3 driver documentation (Laurent)
> Add diagram in driver rst to describe output capability.
> Comments on configuring v4l2 subdevs for CIO2 and ImgU.
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index ec0b74829351..eb6f52aca992 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> @@ -16,12 +16,6 @@
> #define V4L2_CID_INTEL_IPU3_BASE (V4L2_CID_USER_BASE + 0x10c0)
> #define V4L2_CID_INTEL_IPU3_MODE (V4L2_CID_INTEL_IPU3_BASE + 1)
>
> -/* custom ctrl to set pipe mode */
> -enum ipu3_running_mode {
> - IPU3_RUNNING_MODE_VIDEO = 0,
> - IPU3_RUNNING_MODE_STILL = 1,
> -};
> -
> /******************* ipu3_uapi_stats_3a *******************/
>
> #define IPU3_UAPI_MAX_STRIPES 2
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index c7936032beb9..d2a0b62d5688 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -12,6 +12,9 @@
>
> /******************** v4l2_subdev_ops ********************/
>
> +#define IPU3_RUNNING_MODE_VIDEO 0
> +#define IPU3_RUNNING_MODE_STILL 1
Just a single space after "#define" please.
> +
> static int ipu3_subdev_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> {
> struct imgu_v4l2_subdev *imgu_sd = container_of(sd,
> @@ -1035,15 +1038,20 @@ static const struct v4l2_ctrl_ops ipu3_subdev_ctrl_ops = {
> .s_ctrl = ipu3_sd_s_ctrl,
> };
>
> +static const char * const ipu3_ctrl_mode_strings[] = {
> + "Video mode",
> + "Still mode",
> + NULL,
Do you need this NULL entry? I don't see it in other drivers.
> +};
> +
> static const struct v4l2_ctrl_config ipu3_subdev_ctrl_mode = {
> .ops = &ipu3_subdev_ctrl_ops,
> .id = V4L2_CID_INTEL_IPU3_MODE,
> .name = "IPU3 Pipe Mode",
> - .type = V4L2_CTRL_TYPE_INTEGER,
> - .min = IPU3_RUNNING_MODE_VIDEO,
> - .max = IPU3_RUNNING_MODE_STILL,
> - .step = 1,
> - .def = IPU3_RUNNING_MODE_VIDEO,
> + .type = V4L2_CTRL_TYPE_MENU,
> + .max = ARRAY_SIZE(ipu3_ctrl_mode_strings) - 2,
> + .def = 0,
IPU3_RUNNING_MODE_VIDEO?
> + .qmenu = ipu3_ctrl_mode_strings,
> };
>
> /******************** Framework registration ********************/
> --
> 2.7.4
>
Best regards,
Tomasz
