Re: [RFC PATCH 02/12] sh_mobile_ceu_camera: implement the control handler.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 12 Jan 2011, Hans Verkuil wrote:

> And since this is the last and only host driver that uses controls, also
> remove the now obsolete control fields from soc_camera.h.
> 
> Signed-off-by: Hans Verkuil <hverkuil@xxxxxxxxx>
> ---
>  drivers/media/video/sh_mobile_ceu_camera.c |   95 ++++++++++++---------------
>  include/media/soc_camera.h                 |    4 -
>  2 files changed, 42 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
> index 954222b..f007f57 100644
> --- a/drivers/media/video/sh_mobile_ceu_camera.c
> +++ b/drivers/media/video/sh_mobile_ceu_camera.c
> @@ -112,6 +112,7 @@ struct sh_mobile_ceu_dev {
>  
>  	unsigned int image_mode:1;
>  	unsigned int is_16bit:1;
> +	unsigned int added_controls:1;
>  };
>  
>  struct sh_mobile_ceu_cam {
> @@ -133,6 +134,12 @@ struct sh_mobile_ceu_cam {
>  	enum v4l2_mbus_pixelcode code;
>  };
>  
> +static inline struct soc_camera_device *to_icd(struct v4l2_ctrl *ctrl)

I've been told a while ago not to use "inline" in .c files, and to let the 
compiler decide instead. Also this file has no inline directives in it 
until now, please, keep it that way.

> +{
> +	return container_of(ctrl->handler, struct soc_camera_device,
> +							ctrl_handler);
> +}
> +
>  static unsigned long make_bus_param(struct sh_mobile_ceu_dev *pcdev)
>  {
>  	unsigned long flags;
> @@ -490,6 +497,33 @@ out:
>  	return IRQ_HANDLED;
>  }
>  
> +static int sh_mobile_ceu_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct soc_camera_device *icd = to_icd(ctrl);
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +	struct sh_mobile_ceu_dev *pcdev = ici->priv;
> +
> +	ici = to_soc_camera_host(icd->dev.parent);
> +	pcdev = ici->priv;

These two are redundant.

> +	switch (ctrl->id) {
> +	case V4L2_CID_SHARPNESS:
> +		switch (icd->current_fmt->host_fmt->fourcc) {
> +		case V4L2_PIX_FMT_NV12:
> +		case V4L2_PIX_FMT_NV21:
> +		case V4L2_PIX_FMT_NV16:
> +		case V4L2_PIX_FMT_NV61:
> +			ceu_write(pcdev, CLFCR, !ctrl->val);
> +			return 0;
> +		}
> +		break;
> +	}
> +	return -EINVAL;
> +}
> +
> +static const struct v4l2_ctrl_ops sh_mobile_ceu_ctrl_ops = {
> +	.s_ctrl = sh_mobile_ceu_s_ctrl,
> +};
> +
>  /* Called with .video_lock held */
>  static int sh_mobile_ceu_add_device(struct soc_camera_device *icd)
>  {
> @@ -500,6 +534,14 @@ static int sh_mobile_ceu_add_device(struct soc_camera_device *icd)
>  	if (pcdev->icd)
>  		return -EBUSY;
>  
> +	if (!pcdev->added_controls) {
> +		v4l2_ctrl_new_std(&icd->ctrl_handler, &sh_mobile_ceu_ctrl_ops,
> +				V4L2_CID_SHARPNESS, 0, 1, 1, 0);

Hm, am I missing something with this new API? You register a handler for 
only one control ID, and in the handler itself you check once more, which 
ID it is?...

> +		if (icd->ctrl_handler.error)
> +			return icd->ctrl_handler.error;
> +		pcdev->added_controls = 1;
> +	}
> +
>  	dev_info(icd->dev.parent,
>  		 "SuperH Mobile CEU driver attached to camera %d\n",
>  		 icd->devnum);

Thanks
Guennadi

> @@ -1789,55 +1831,6 @@ static void sh_mobile_ceu_init_videobuf(struct videobuf_queue *q,
>  				       icd, &icd->video_lock);
>  }
>  
> -static int sh_mobile_ceu_get_ctrl(struct soc_camera_device *icd,
> -				  struct v4l2_control *ctrl)
> -{
> -	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> -	struct sh_mobile_ceu_dev *pcdev = ici->priv;
> -	u32 val;
> -
> -	switch (ctrl->id) {
> -	case V4L2_CID_SHARPNESS:
> -		val = ceu_read(pcdev, CLFCR);
> -		ctrl->value = val ^ 1;
> -		return 0;
> -	}
> -	return -ENOIOCTLCMD;
> -}
> -
> -static int sh_mobile_ceu_set_ctrl(struct soc_camera_device *icd,
> -				  struct v4l2_control *ctrl)
> -{
> -	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> -	struct sh_mobile_ceu_dev *pcdev = ici->priv;
> -
> -	switch (ctrl->id) {
> -	case V4L2_CID_SHARPNESS:
> -		switch (icd->current_fmt->host_fmt->fourcc) {
> -		case V4L2_PIX_FMT_NV12:
> -		case V4L2_PIX_FMT_NV21:
> -		case V4L2_PIX_FMT_NV16:
> -		case V4L2_PIX_FMT_NV61:
> -			ceu_write(pcdev, CLFCR, !ctrl->value);
> -			return 0;
> -		}
> -		return -EINVAL;
> -	}
> -	return -ENOIOCTLCMD;
> -}
> -
> -static const struct v4l2_queryctrl sh_mobile_ceu_controls[] = {
> -	{
> -		.id		= V4L2_CID_SHARPNESS,
> -		.type		= V4L2_CTRL_TYPE_BOOLEAN,
> -		.name		= "Low-pass filter",
> -		.minimum	= 0,
> -		.maximum	= 1,
> -		.step		= 1,
> -		.default_value	= 0,
> -	},
> -};
> -
>  static struct soc_camera_host_ops sh_mobile_ceu_host_ops = {
>  	.owner		= THIS_MODULE,
>  	.add		= sh_mobile_ceu_add_device,
> @@ -1848,15 +1841,11 @@ static struct soc_camera_host_ops sh_mobile_ceu_host_ops = {
>  	.set_crop	= sh_mobile_ceu_set_crop,
>  	.set_fmt	= sh_mobile_ceu_set_fmt,
>  	.try_fmt	= sh_mobile_ceu_try_fmt,
> -	.set_ctrl	= sh_mobile_ceu_set_ctrl,
> -	.get_ctrl	= sh_mobile_ceu_get_ctrl,
>  	.reqbufs	= sh_mobile_ceu_reqbufs,
>  	.poll		= sh_mobile_ceu_poll,
>  	.querycap	= sh_mobile_ceu_querycap,
>  	.set_bus_param	= sh_mobile_ceu_set_bus_param,
>  	.init_videobuf	= sh_mobile_ceu_init_videobuf,
> -	.controls	= sh_mobile_ceu_controls,
> -	.num_controls	= ARRAY_SIZE(sh_mobile_ceu_controls),
>  };
>  
>  struct bus_wait {
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index ee61ffb..b71b26e 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -83,13 +83,9 @@ struct soc_camera_host_ops {
>  	int (*reqbufs)(struct soc_camera_device *, struct v4l2_requestbuffers *);
>  	int (*querycap)(struct soc_camera_host *, struct v4l2_capability *);
>  	int (*set_bus_param)(struct soc_camera_device *, __u32);
> -	int (*get_ctrl)(struct soc_camera_device *, struct v4l2_control *);
> -	int (*set_ctrl)(struct soc_camera_device *, struct v4l2_control *);
>  	int (*get_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
>  	int (*set_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
>  	unsigned int (*poll)(struct file *, poll_table *);
> -	const struct v4l2_queryctrl *controls;
> -	int num_controls;
>  };
>  
>  #define SOCAM_SENSOR_INVERT_PCLK	(1 << 0)
> -- 
> 1.7.0.4
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux