* [RFC PATCH 02/12] sh_mobile_ceu_camera: implement the control handler.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-22 20:31 ` Guennadi Liakhovetski
2011-01-11 23:06 ` [RFC PATCH 03/12] mt9m001: convert to the control framework Hans Verkuil
` (10 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
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@xs4all.nl>
---
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)
+{
+ 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;
+ 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);
+ 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);
@@ -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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 02/12] sh_mobile_ceu_camera: implement the control handler.
2011-01-11 23:06 ` [RFC PATCH 02/12] sh_mobile_ceu_camera: implement the control handler Hans Verkuil
@ 2011-01-22 20:31 ` Guennadi Liakhovetski
2011-01-25 7:37 ` Hans Verkuil
0 siblings, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2011-01-22 20:31 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
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@xs4all.nl>
> ---
> 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/
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 02/12] sh_mobile_ceu_camera: implement the control handler.
2011-01-22 20:31 ` Guennadi Liakhovetski
@ 2011-01-25 7:37 ` Hans Verkuil
0 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-25 7:37 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
On Saturday, January 22, 2011 21:31:52 Guennadi Liakhovetski wrote:
> 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@xs4all.nl>
> > ---
> > 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.
OK.
> > +{
> > + 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.
Must have missed that. Will remove these lines.
>
> > + 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?...
I can remove the check, but having a switch, even if only for one control,
makes it very easy later to add additional controls. But if you prefer not
to have that, then I can easily remove it.
> > + 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
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH 03/12] mt9m001: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
2011-01-11 23:06 ` [RFC PATCH 02/12] sh_mobile_ceu_camera: implement the control handler Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-22 21:21 ` Guennadi Liakhovetski
2011-01-11 23:06 ` [RFC PATCH 04/12] mt9m111.c: " Hans Verkuil
` (9 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/mt9m001.c | 210 +++++++++++++++--------------------------
1 files changed, 75 insertions(+), 135 deletions(-)
diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
index f7fc88d..b9b6e33 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -15,6 +15,7 @@
#include <media/v4l2-subdev.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ctrls.h>
#include <media/soc_camera.h>
/*
@@ -84,15 +85,18 @@ static const struct mt9m001_datafmt mt9m001_monochrome_fmts[] = {
struct mt9m001 {
struct v4l2_subdev subdev;
+ struct v4l2_ctrl_handler hdl;
+ struct {
+ /* exposure/auto-exposure cluster */
+ struct v4l2_ctrl *autoexposure;
+ struct v4l2_ctrl *exposure;
+ };
struct v4l2_rect rect; /* Sensor window */
const struct mt9m001_datafmt *fmt;
const struct mt9m001_datafmt *fmts;
int num_fmts;
int model; /* V4L2_IDENT_MT9M001* codes from v4l2-chip-ident.h */
- unsigned int gain;
- unsigned int exposure;
unsigned short y_skip_top; /* Lines to skip at the top */
- unsigned char autoexposure;
};
static struct mt9m001 *to_mt9m001(const struct i2c_client *client)
@@ -209,7 +213,6 @@ static int mt9m001_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct mt9m001 *mt9m001 = to_mt9m001(client);
struct v4l2_rect rect = a->c;
- struct soc_camera_device *icd = client->dev.platform_data;
int ret;
const u16 hblank = 9, vblank = 25;
unsigned int total_h;
@@ -251,17 +254,18 @@ static int mt9m001_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
if (!ret)
ret = reg_write(client, MT9M001_WINDOW_HEIGHT,
rect.height + mt9m001->y_skip_top - 1);
- if (!ret && mt9m001->autoexposure) {
+ v4l2_ctrl_lock(mt9m001->autoexposure);
+ if (!ret && mt9m001->autoexposure->cur.val == V4L2_EXPOSURE_AUTO) {
ret = reg_write(client, MT9M001_SHUTTER_WIDTH, total_h);
if (!ret) {
- const struct v4l2_queryctrl *qctrl =
- soc_camera_find_qctrl(icd->ops,
- V4L2_CID_EXPOSURE);
- mt9m001->exposure = (524 + (total_h - 1) *
- (qctrl->maximum - qctrl->minimum)) /
- 1048 + qctrl->minimum;
+ struct v4l2_ctrl *ctrl = mt9m001->exposure;
+
+ ctrl->cur.val = (524 + (total_h - 1) *
+ (ctrl->maximum - ctrl->minimum)) /
+ 1048 + ctrl->minimum;
}
}
+ v4l2_ctrl_unlock(mt9m001->autoexposure);
if (!ret)
mt9m001->rect = rect;
@@ -421,107 +425,36 @@ static int mt9m001_s_register(struct v4l2_subdev *sd,
}
#endif
-static const struct v4l2_queryctrl mt9m001_controls[] = {
- {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Vertically",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- }, {
- .id = V4L2_CID_GAIN,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Gain",
- .minimum = 0,
- .maximum = 127,
- .step = 1,
- .default_value = 64,
- .flags = V4L2_CTRL_FLAG_SLIDER,
- }, {
- .id = V4L2_CID_EXPOSURE,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Exposure",
- .minimum = 1,
- .maximum = 255,
- .step = 1,
- .default_value = 255,
- .flags = V4L2_CTRL_FLAG_SLIDER,
- }, {
- .id = V4L2_CID_EXPOSURE_AUTO,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Automatic Exposure",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 1,
- }
-};
-
static struct soc_camera_ops mt9m001_ops = {
.set_bus_param = mt9m001_set_bus_param,
.query_bus_param = mt9m001_query_bus_param,
- .controls = mt9m001_controls,
- .num_controls = ARRAY_SIZE(mt9m001_controls),
};
-static int mt9m001_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct mt9m001, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct mt9m001 *mt9m001 = to_mt9m001(client);
+ struct v4l2_ctrl *exp = mt9m001->exposure;
int data;
switch (ctrl->id) {
case V4L2_CID_VFLIP:
- data = reg_read(client, MT9M001_READ_OPTIONS2);
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & 0x8000);
- break;
- case V4L2_CID_EXPOSURE_AUTO:
- ctrl->value = mt9m001->autoexposure;
- break;
- case V4L2_CID_GAIN:
- ctrl->value = mt9m001->gain;
- break;
- case V4L2_CID_EXPOSURE:
- ctrl->value = mt9m001->exposure;
- break;
- }
- return 0;
-}
-
-static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
-{
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct mt9m001 *mt9m001 = to_mt9m001(client);
- struct soc_camera_device *icd = client->dev.platform_data;
- const struct v4l2_queryctrl *qctrl;
- int data;
-
- qctrl = soc_camera_find_qctrl(&mt9m001_ops, ctrl->id);
-
- if (!qctrl)
- return -EINVAL;
-
- switch (ctrl->id) {
- case V4L2_CID_VFLIP:
- if (ctrl->value)
+ if (ctrl->val)
data = reg_set(client, MT9M001_READ_OPTIONS2, 0x8000);
else
data = reg_clear(client, MT9M001_READ_OPTIONS2, 0x8000);
if (data < 0)
return -EIO;
- break;
+ return 0;
+
case V4L2_CID_GAIN:
- if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
- return -EINVAL;
/* See Datasheet Table 7, Gain settings. */
- if (ctrl->value <= qctrl->default_value) {
+ if (ctrl->val <= ctrl->default_value) {
/* Pack it into 0..1 step 0.125, register values 0..8 */
- unsigned long range = qctrl->default_value - qctrl->minimum;
- data = ((ctrl->value - qctrl->minimum) * 8 + range / 2) / range;
+ unsigned long range = ctrl->default_value - ctrl->minimum;
+ data = ((ctrl->val - ctrl->minimum) * 8 + range / 2) / range;
dev_dbg(&client->dev, "Setting gain %d\n", data);
data = reg_write(client, MT9M001_GLOBAL_GAIN, data);
@@ -530,8 +463,8 @@ static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
} else {
/* Pack it into 1.125..15 variable step, register values 9..67 */
/* We assume qctrl->maximum - qctrl->default_value - 1 > 0 */
- unsigned long range = qctrl->maximum - qctrl->default_value - 1;
- unsigned long gain = ((ctrl->value - qctrl->default_value - 1) *
+ unsigned long range = ctrl->maximum - ctrl->default_value - 1;
+ unsigned long gain = ((ctrl->val - ctrl->default_value - 1) *
111 + range / 2) / range + 9;
if (gain <= 32)
@@ -547,47 +480,36 @@ static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
if (data < 0)
return -EIO;
}
+ return 0;
- /* Success */
- mt9m001->gain = ctrl->value;
- break;
- case V4L2_CID_EXPOSURE:
- /* mt9m001 has maximum == default */
- if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
- return -EINVAL;
- else {
- unsigned long range = qctrl->maximum - qctrl->minimum;
- unsigned long shutter = ((ctrl->value - qctrl->minimum) * 1048 +
+ case V4L2_CID_EXPOSURE_AUTO:
+ /* Force manual exposure if only the exposure was changed */
+ if (!ctrl->has_new)
+ ctrl->val = V4L2_EXPOSURE_MANUAL;
+ if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
+ unsigned long range = exp->maximum - exp->minimum;
+ unsigned long shutter = ((exp->val - exp->minimum) * 1048 +
range / 2) / range + 1;
dev_dbg(&client->dev,
"Setting shutter width from %d to %lu\n",
- reg_read(client, MT9M001_SHUTTER_WIDTH),
- shutter);
+ reg_read(client, MT9M001_SHUTTER_WIDTH), shutter);
if (reg_write(client, MT9M001_SHUTTER_WIDTH, shutter) < 0)
return -EIO;
- mt9m001->exposure = ctrl->value;
- mt9m001->autoexposure = 0;
- }
- break;
- case V4L2_CID_EXPOSURE_AUTO:
- if (ctrl->value) {
+ } else {
const u16 vblank = 25;
unsigned int total_h = mt9m001->rect.height +
mt9m001->y_skip_top + vblank;
- if (reg_write(client, MT9M001_SHUTTER_WIDTH,
- total_h) < 0)
+
+ if (reg_write(client, MT9M001_SHUTTER_WIDTH, total_h) < 0)
return -EIO;
- qctrl = soc_camera_find_qctrl(icd->ops, V4L2_CID_EXPOSURE);
- mt9m001->exposure = (524 + (total_h - 1) *
- (qctrl->maximum - qctrl->minimum)) /
- 1048 + qctrl->minimum;
- mt9m001->autoexposure = 1;
- } else
- mt9m001->autoexposure = 0;
- break;
+ exp->val = (524 + (total_h - 1) *
+ (exp->maximum - exp->minimum)) / 1048 +
+ exp->minimum;
+ }
+ return 0;
}
- return 0;
+ return -EINVAL;
}
/*
@@ -665,10 +587,7 @@ static int mt9m001_video_probe(struct soc_camera_device *icd,
dev_err(&client->dev, "Failed to initialise the camera\n");
/* mt9m001_init() has reset the chip, returning registers to defaults */
- mt9m001->gain = 64;
- mt9m001->exposure = 255;
-
- return ret;
+ return v4l2_ctrl_handler_setup(&mt9m001->hdl);
}
static void mt9m001_video_remove(struct soc_camera_device *icd)
@@ -691,9 +610,11 @@ static int mt9m001_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
return 0;
}
+static const struct v4l2_ctrl_ops mt9m001_ctrl_ops = {
+ .s_ctrl = mt9m001_s_ctrl,
+};
+
static struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
- .g_ctrl = mt9m001_g_ctrl,
- .s_ctrl = mt9m001_s_ctrl,
.g_chip_ident = mt9m001_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = mt9m001_g_register,
@@ -766,6 +687,28 @@ static int mt9m001_probe(struct i2c_client *client,
return -ENOMEM;
v4l2_i2c_subdev_init(&mt9m001->subdev, client, &mt9m001_subdev_ops);
+ v4l2_ctrl_handler_init(&mt9m001->hdl, 4);
+ v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
+ V4L2_CID_GAIN, 0, 127, 1, 64);
+ mt9m001->exposure = v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
+ V4L2_CID_EXPOSURE, 1, 255, 1, 255);
+ /*
+ * Simulated autoexposure. If enabled, we calculate shutter width
+ * ourselves in the driver based on vertical blanking and frame width
+ */
+ mt9m001->autoexposure = v4l2_ctrl_new_std_menu(&mt9m001->hdl,
+ &mt9m001_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
+ V4L2_EXPOSURE_AUTO);
+ mt9m001->subdev.ctrl_handler = &mt9m001->hdl;
+ if (mt9m001->hdl.error) {
+ int err = mt9m001->hdl.error;
+
+ kfree(mt9m001);
+ return err;
+ }
+ v4l2_ctrl_cluster(2, &mt9m001->autoexposure);
/* Second stage probe - when a capture adapter is there */
icd->ops = &mt9m001_ops;
@@ -776,15 +719,10 @@ static int mt9m001_probe(struct i2c_client *client,
mt9m001->rect.width = MT9M001_MAX_WIDTH;
mt9m001->rect.height = MT9M001_MAX_HEIGHT;
- /*
- * Simulated autoexposure. If enabled, we calculate shutter width
- * ourselves in the driver based on vertical blanking and frame width
- */
- mt9m001->autoexposure = 1;
-
ret = mt9m001_video_probe(icd, client);
if (ret) {
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&mt9m001->hdl);
kfree(mt9m001);
}
@@ -796,6 +734,8 @@ static int mt9m001_remove(struct i2c_client *client)
struct mt9m001 *mt9m001 = to_mt9m001(client);
struct soc_camera_device *icd = client->dev.platform_data;
+ v4l2_device_unregister_subdev(&mt9m001->subdev);
+ v4l2_ctrl_handler_free(&mt9m001->hdl);
icd->ops = NULL;
mt9m001_video_remove(icd);
kfree(mt9m001);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 03/12] mt9m001: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 03/12] mt9m001: convert to the control framework Hans Verkuil
@ 2011-01-22 21:21 ` Guennadi Liakhovetski
2011-01-23 3:38 ` Kim HeungJun
2011-01-25 7:54 ` Hans Verkuil
0 siblings, 2 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2011-01-22 21:21 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
On Wed, 12 Jan 2011, Hans Verkuil wrote:
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
> drivers/media/video/mt9m001.c | 210 +++++++++++++++--------------------------
> 1 files changed, 75 insertions(+), 135 deletions(-)
>
> diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
> index f7fc88d..b9b6e33 100644
> --- a/drivers/media/video/mt9m001.c
> +++ b/drivers/media/video/mt9m001.c
> @@ -15,6 +15,7 @@
>
> #include <media/v4l2-subdev.h>
> #include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-ctrls.h>
> #include <media/soc_camera.h>
>
> /*
> @@ -84,15 +85,18 @@ static const struct mt9m001_datafmt mt9m001_monochrome_fmts[] = {
>
> struct mt9m001 {
> struct v4l2_subdev subdev;
> + struct v4l2_ctrl_handler hdl;
> + struct {
> + /* exposure/auto-exposure cluster */
> + struct v4l2_ctrl *autoexposure;
> + struct v4l2_ctrl *exposure;
> + };
Hm, why an anonymous struct? Why not just put them directly at the top
level?
> struct v4l2_rect rect; /* Sensor window */
> const struct mt9m001_datafmt *fmt;
> const struct mt9m001_datafmt *fmts;
> int num_fmts;
> int model; /* V4L2_IDENT_MT9M001* codes from v4l2-chip-ident.h */
> - unsigned int gain;
> - unsigned int exposure;
> unsigned short y_skip_top; /* Lines to skip at the top */
> - unsigned char autoexposure;
> };
>
> static struct mt9m001 *to_mt9m001(const struct i2c_client *client)
> @@ -209,7 +213,6 @@ static int mt9m001_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> struct mt9m001 *mt9m001 = to_mt9m001(client);
> struct v4l2_rect rect = a->c;
> - struct soc_camera_device *icd = client->dev.platform_data;
> int ret;
> const u16 hblank = 9, vblank = 25;
> unsigned int total_h;
> @@ -251,17 +254,18 @@ static int mt9m001_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> if (!ret)
> ret = reg_write(client, MT9M001_WINDOW_HEIGHT,
> rect.height + mt9m001->y_skip_top - 1);
> - if (!ret && mt9m001->autoexposure) {
> + v4l2_ctrl_lock(mt9m001->autoexposure);
> + if (!ret && mt9m001->autoexposure->cur.val == V4L2_EXPOSURE_AUTO) {
> ret = reg_write(client, MT9M001_SHUTTER_WIDTH, total_h);
> if (!ret) {
> - const struct v4l2_queryctrl *qctrl =
> - soc_camera_find_qctrl(icd->ops,
> - V4L2_CID_EXPOSURE);
> - mt9m001->exposure = (524 + (total_h - 1) *
> - (qctrl->maximum - qctrl->minimum)) /
> - 1048 + qctrl->minimum;
> + struct v4l2_ctrl *ctrl = mt9m001->exposure;
> +
> + ctrl->cur.val = (524 + (total_h - 1) *
> + (ctrl->maximum - ctrl->minimum)) /
> + 1048 + ctrl->minimum;
> }
> }
> + v4l2_ctrl_unlock(mt9m001->autoexposure);
>
> if (!ret)
> mt9m001->rect = rect;
> @@ -421,107 +425,36 @@ static int mt9m001_s_register(struct v4l2_subdev *sd,
> }
> #endif
>
> -static const struct v4l2_queryctrl mt9m001_controls[] = {
> - {
> - .id = V4L2_CID_VFLIP,
> - .type = V4L2_CTRL_TYPE_BOOLEAN,
> - .name = "Flip Vertically",
> - .minimum = 0,
> - .maximum = 1,
> - .step = 1,
> - .default_value = 0,
> - }, {
> - .id = V4L2_CID_GAIN,
> - .type = V4L2_CTRL_TYPE_INTEGER,
> - .name = "Gain",
> - .minimum = 0,
> - .maximum = 127,
> - .step = 1,
> - .default_value = 64,
> - .flags = V4L2_CTRL_FLAG_SLIDER,
> - }, {
> - .id = V4L2_CID_EXPOSURE,
> - .type = V4L2_CTRL_TYPE_INTEGER,
> - .name = "Exposure",
> - .minimum = 1,
> - .maximum = 255,
> - .step = 1,
> - .default_value = 255,
> - .flags = V4L2_CTRL_FLAG_SLIDER,
> - }, {
> - .id = V4L2_CID_EXPOSURE_AUTO,
> - .type = V4L2_CTRL_TYPE_BOOLEAN,
> - .name = "Automatic Exposure",
> - .minimum = 0,
> - .maximum = 1,
> - .step = 1,
> - .default_value = 1,
> - }
> -};
> -
> static struct soc_camera_ops mt9m001_ops = {
> .set_bus_param = mt9m001_set_bus_param,
> .query_bus_param = mt9m001_query_bus_param,
> - .controls = mt9m001_controls,
> - .num_controls = ARRAY_SIZE(mt9m001_controls),
> };
>
> -static int mt9m001_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> +static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> + struct v4l2_subdev *sd =
> + &container_of(ctrl->handler, struct mt9m001, hdl)->subdev;
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> struct mt9m001 *mt9m001 = to_mt9m001(client);
This looks a bit clumsy to me, sorry. Above you already have "struct
mt9m001 *" (container_of(ctrl->handler, struct mt9m001, hdl)), but you
only use it implicitly to get to sd, and then mt9m001 is calculated
again...
> + struct v4l2_ctrl *exp = mt9m001->exposure;
> int data;
>
> switch (ctrl->id) {
> case V4L2_CID_VFLIP:
> - data = reg_read(client, MT9M001_READ_OPTIONS2);
> - if (data < 0)
> - return -EIO;
> - ctrl->value = !!(data & 0x8000);
> - break;
> - case V4L2_CID_EXPOSURE_AUTO:
> - ctrl->value = mt9m001->autoexposure;
> - break;
> - case V4L2_CID_GAIN:
> - ctrl->value = mt9m001->gain;
> - break;
> - case V4L2_CID_EXPOSURE:
> - ctrl->value = mt9m001->exposure;
> - break;
> - }
> - return 0;
> -}
> -
> -static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> -{
> - struct i2c_client *client = v4l2_get_subdevdata(sd);
> - struct mt9m001 *mt9m001 = to_mt9m001(client);
> - struct soc_camera_device *icd = client->dev.platform_data;
> - const struct v4l2_queryctrl *qctrl;
> - int data;
> -
> - qctrl = soc_camera_find_qctrl(&mt9m001_ops, ctrl->id);
> -
> - if (!qctrl)
> - return -EINVAL;
> -
> - switch (ctrl->id) {
> - case V4L2_CID_VFLIP:
> - if (ctrl->value)
> + if (ctrl->val)
> data = reg_set(client, MT9M001_READ_OPTIONS2, 0x8000);
> else
> data = reg_clear(client, MT9M001_READ_OPTIONS2, 0x8000);
> if (data < 0)
> return -EIO;
> - break;
> + return 0;
> +
> case V4L2_CID_GAIN:
> - if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
> - return -EINVAL;
> /* See Datasheet Table 7, Gain settings. */
> - if (ctrl->value <= qctrl->default_value) {
> + if (ctrl->val <= ctrl->default_value) {
> /* Pack it into 0..1 step 0.125, register values 0..8 */
> - unsigned long range = qctrl->default_value - qctrl->minimum;
> - data = ((ctrl->value - qctrl->minimum) * 8 + range / 2) / range;
> + unsigned long range = ctrl->default_value - ctrl->minimum;
> + data = ((ctrl->val - ctrl->minimum) * 8 + range / 2) / range;
>
> dev_dbg(&client->dev, "Setting gain %d\n", data);
> data = reg_write(client, MT9M001_GLOBAL_GAIN, data);
> @@ -530,8 +463,8 @@ static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> } else {
> /* Pack it into 1.125..15 variable step, register values 9..67 */
> /* We assume qctrl->maximum - qctrl->default_value - 1 > 0 */
> - unsigned long range = qctrl->maximum - qctrl->default_value - 1;
> - unsigned long gain = ((ctrl->value - qctrl->default_value - 1) *
> + unsigned long range = ctrl->maximum - ctrl->default_value - 1;
> + unsigned long gain = ((ctrl->val - ctrl->default_value - 1) *
> 111 + range / 2) / range + 9;
>
> if (gain <= 32)
> @@ -547,47 +480,36 @@ static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> if (data < 0)
> return -EIO;
> }
> + return 0;
>
> - /* Success */
> - mt9m001->gain = ctrl->value;
> - break;
> - case V4L2_CID_EXPOSURE:
> - /* mt9m001 has maximum == default */
> - if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
> - return -EINVAL;
> - else {
> - unsigned long range = qctrl->maximum - qctrl->minimum;
> - unsigned long shutter = ((ctrl->value - qctrl->minimum) * 1048 +
> + case V4L2_CID_EXPOSURE_AUTO:
> + /* Force manual exposure if only the exposure was changed */
> + if (!ctrl->has_new)
> + ctrl->val = V4L2_EXPOSURE_MANUAL;
> + if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
> + unsigned long range = exp->maximum - exp->minimum;
> + unsigned long shutter = ((exp->val - exp->minimum) * 1048 +
> range / 2) / range + 1;
>
> dev_dbg(&client->dev,
> "Setting shutter width from %d to %lu\n",
> - reg_read(client, MT9M001_SHUTTER_WIDTH),
> - shutter);
> + reg_read(client, MT9M001_SHUTTER_WIDTH), shutter);
> if (reg_write(client, MT9M001_SHUTTER_WIDTH, shutter) < 0)
> return -EIO;
> - mt9m001->exposure = ctrl->value;
> - mt9m001->autoexposure = 0;
> - }
> - break;
> - case V4L2_CID_EXPOSURE_AUTO:
> - if (ctrl->value) {
> + } else {
> const u16 vblank = 25;
> unsigned int total_h = mt9m001->rect.height +
> mt9m001->y_skip_top + vblank;
> - if (reg_write(client, MT9M001_SHUTTER_WIDTH,
> - total_h) < 0)
> +
> + if (reg_write(client, MT9M001_SHUTTER_WIDTH, total_h) < 0)
> return -EIO;
> - qctrl = soc_camera_find_qctrl(icd->ops, V4L2_CID_EXPOSURE);
> - mt9m001->exposure = (524 + (total_h - 1) *
> - (qctrl->maximum - qctrl->minimum)) /
> - 1048 + qctrl->minimum;
> - mt9m001->autoexposure = 1;
> - } else
> - mt9m001->autoexposure = 0;
> - break;
> + exp->val = (524 + (total_h - 1) *
> + (exp->maximum - exp->minimum)) / 1048 +
> + exp->minimum;
> + }
> + return 0;
> }
> - return 0;
> + return -EINVAL;
It seems to me, that you've dropped V4L2_CID_EXPOSURE here, was it
intentional? I won't verify this in detail now, because, if it wasn't
intentional and you fix it in v2, I'll have to re-check it anyway. Or is
it supposed to be handled by that V4L2_EXPOSURE_MANUAL? So, if the user
issues a V4L2_CID_EXPOSURE, are you getting V4L2_CID_EXPOSURE_AUTO with
val == V4L2_EXPOSURE_MANUAL instead? Weird...
> }
>
> /*
> @@ -665,10 +587,7 @@ static int mt9m001_video_probe(struct soc_camera_device *icd,
> dev_err(&client->dev, "Failed to initialise the camera\n");
>
> /* mt9m001_init() has reset the chip, returning registers to defaults */
> - mt9m001->gain = 64;
> - mt9m001->exposure = 255;
> -
> - return ret;
> + return v4l2_ctrl_handler_setup(&mt9m001->hdl);
> }
>
> static void mt9m001_video_remove(struct soc_camera_device *icd)
> @@ -691,9 +610,11 @@ static int mt9m001_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
> return 0;
> }
>
> +static const struct v4l2_ctrl_ops mt9m001_ctrl_ops = {
> + .s_ctrl = mt9m001_s_ctrl,
> +};
> +
> static struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
> - .g_ctrl = mt9m001_g_ctrl,
> - .s_ctrl = mt9m001_s_ctrl,
> .g_chip_ident = mt9m001_g_chip_ident,
> #ifdef CONFIG_VIDEO_ADV_DEBUG
> .g_register = mt9m001_g_register,
> @@ -766,6 +687,28 @@ static int mt9m001_probe(struct i2c_client *client,
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&mt9m001->subdev, client, &mt9m001_subdev_ops);
> + v4l2_ctrl_handler_init(&mt9m001->hdl, 4);
> + v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
> + V4L2_CID_GAIN, 0, 127, 1, 64);
> + mt9m001->exposure = v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
> + V4L2_CID_EXPOSURE, 1, 255, 1, 255);
> + /*
> + * Simulated autoexposure. If enabled, we calculate shutter width
> + * ourselves in the driver based on vertical blanking and frame width
> + */
> + mt9m001->autoexposure = v4l2_ctrl_new_std_menu(&mt9m001->hdl,
> + &mt9m001_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
> + V4L2_EXPOSURE_AUTO);
> + mt9m001->subdev.ctrl_handler = &mt9m001->hdl;
> + if (mt9m001->hdl.error) {
> + int err = mt9m001->hdl.error;
> +
> + kfree(mt9m001);
> + return err;
> + }
> + v4l2_ctrl_cluster(2, &mt9m001->autoexposure);
Ooh, is this the reason for that anonymous struct?...
>
> /* Second stage probe - when a capture adapter is there */
> icd->ops = &mt9m001_ops;
> @@ -776,15 +719,10 @@ static int mt9m001_probe(struct i2c_client *client,
> mt9m001->rect.width = MT9M001_MAX_WIDTH;
> mt9m001->rect.height = MT9M001_MAX_HEIGHT;
>
> - /*
> - * Simulated autoexposure. If enabled, we calculate shutter width
> - * ourselves in the driver based on vertical blanking and frame width
> - */
> - mt9m001->autoexposure = 1;
> -
> ret = mt9m001_video_probe(icd, client);
> if (ret) {
> icd->ops = NULL;
> + v4l2_ctrl_handler_free(&mt9m001->hdl);
> kfree(mt9m001);
> }
>
> @@ -796,6 +734,8 @@ static int mt9m001_remove(struct i2c_client *client)
> struct mt9m001 *mt9m001 = to_mt9m001(client);
> struct soc_camera_device *icd = client->dev.platform_data;
>
> + v4l2_device_unregister_subdev(&mt9m001->subdev);
hm, first, this is not really related, right? Secondly, are you sure this
is needed? It is now double with soc_camera_remove(). I know, it is safe,
but still, one of them looks superfluous to me.
> + v4l2_ctrl_handler_free(&mt9m001->hdl);
> icd->ops = NULL;
> mt9m001_video_remove(icd);
> kfree(mt9m001);
> --
> 1.7.0.4
>
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 03/12] mt9m001: convert to the control framework.
2011-01-22 21:21 ` Guennadi Liakhovetski
@ 2011-01-23 3:38 ` Kim HeungJun
2011-01-25 8:02 ` Hans Verkuil
2011-01-25 7:54 ` Hans Verkuil
1 sibling, 1 reply; 27+ messages in thread
From: Kim HeungJun @ 2011-01-23 3:38 UTC (permalink / raw)
To: Guennadi Liakhovetski, VerkuilHans
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Hello,
I'm reading threads about the new v4l2_ctrl framework and If you don't mind
I gotta tell you my humble opinion about testing result the new v4l2_ctrl
framework subdev.
I have actually similar curcumstance, with I2C subdev M5MOLS Fujitsu device
which is just send the patch and S5PC210 board for testing this, except not
using soc_camera framework.
But, it's maybe helpful to discuss about this changes to everyone.
2011. 1. 23., 오전 6:21, Guennadi Liakhovetski 작성:
> On Wed, 12 Jan 2011, Hans Verkuil wrote:
>
>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
[snip]
>> - case V4L2_CID_EXPOSURE:
>> - /* mt9m001 has maximum == default */
>> - if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
>> - return -EINVAL;
>> - else {
>> - unsigned long range = qctrl->maximum - qctrl->minimum;
>> - unsigned long shutter = ((ctrl->value - qctrl->minimum) * 1048 +
>> + case V4L2_CID_EXPOSURE_AUTO:
>> + /* Force manual exposure if only the exposure was changed */
>> + if (!ctrl->has_new)
>> + ctrl->val = V4L2_EXPOSURE_MANUAL;
>> + if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
>> + unsigned long range = exp->maximum - exp->minimum;
>> + unsigned long shutter = ((exp->val - exp->minimum) * 1048 +
>> range / 2) / range + 1;
>>
>> dev_dbg(&client->dev,
>> "Setting shutter width from %d to %lu\n",
>> - reg_read(client, MT9M001_SHUTTER_WIDTH),
>> - shutter);
>> + reg_read(client, MT9M001_SHUTTER_WIDTH), shutter);
>> if (reg_write(client, MT9M001_SHUTTER_WIDTH, shutter) < 0)
>> return -EIO;
>> - mt9m001->exposure = ctrl->value;
>> - mt9m001->autoexposure = 0;
>> - }
>> - break;
>> - case V4L2_CID_EXPOSURE_AUTO:
>> - if (ctrl->value) {
>> + } else {
>> const u16 vblank = 25;
>> unsigned int total_h = mt9m001->rect.height +
>> mt9m001->y_skip_top + vblank;
>> - if (reg_write(client, MT9M001_SHUTTER_WIDTH,
>> - total_h) < 0)
>> +
>> + if (reg_write(client, MT9M001_SHUTTER_WIDTH, total_h) < 0)
>> return -EIO;
>> - qctrl = soc_camera_find_qctrl(icd->ops, V4L2_CID_EXPOSURE);
>> - mt9m001->exposure = (524 + (total_h - 1) *
>> - (qctrl->maximum - qctrl->minimum)) /
>> - 1048 + qctrl->minimum;
>> - mt9m001->autoexposure = 1;
>> - } else
>> - mt9m001->autoexposure = 0;
>> - break;
>> + exp->val = (524 + (total_h - 1) *
>> + (exp->maximum - exp->minimum)) / 1048 +
>> + exp->minimum;
>> + }
>> + return 0;
>> }
>> - return 0;
>> + return -EINVAL;
>
> It seems to me, that you've dropped V4L2_CID_EXPOSURE here, was it
> intentional? I won't verify this in detail now, because, if it wasn't
> intentional and you fix it in v2, I'll have to re-check it anyway. Or is
> it supposed to be handled by that V4L2_EXPOSURE_MANUAL? So, if the user
> issues a V4L2_CID_EXPOSURE, are you getting V4L2_CID_EXPOSURE_AUTO with
> val == V4L2_EXPOSURE_MANUAL instead? Weird...
I also wonder first at this part for a long time like below:
1. when calling V4L2_CID_EXPOSURE_AUTO with V4L2_EXPOSURE_AUTO, it's ok.
2. when calling V4L2_CID_EXPOSURE_AUTO with V4L2_EXPOSURE_MANUAL, it's
also ok.
3. when calling V4L2_CID_EXPOSURE? where the device handle this CID?
but, after testing with application step by step, I finally know below:
when calling V4L2_CID_EXPOSURE, changing internal(v4l2_ctrl framework) variable,
exactly struct v4l2_ctrl exposure, which is register for probing time by
V4L2_CID_EXPOSURE, and clustered with struct v4l2_ctrl autoexposure. So, when
the device no needs to handle this values, but it automatically calls control clustered with
itself, in this case the V4L2_CID_EXPOSURE calls(just words)V4L2_CID_EXPOSURE_AUTO.
So, the my POV is that foo clustered with auto_foo calls auto_foo with foo's characteristics.
But, Hans probably would do more clear answer.
Regards,
Heungjun Kim
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 03/12] mt9m001: convert to the control framework.
2011-01-23 3:38 ` Kim HeungJun
@ 2011-01-25 8:02 ` Hans Verkuil
0 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-25 8:02 UTC (permalink / raw)
To: Kim HeungJun
Cc: Guennadi Liakhovetski, linux-media, Magnus Damm,
Kuninori Morimoto, Alberto Panizzo, Janusz Krzysztofik,
Marek Vasut, Robert Jarzmik
On Sunday, January 23, 2011 04:38:21 Kim HeungJun wrote:
> Hello,
>
> I'm reading threads about the new v4l2_ctrl framework and If you don't mind
> I gotta tell you my humble opinion about testing result the new v4l2_ctrl
> framework subdev.
> I have actually similar curcumstance, with I2C subdev M5MOLS Fujitsu device
> which is just send the patch and S5PC210 board for testing this, except not
> using soc_camera framework.
> But, it's maybe helpful to discuss about this changes to everyone.
>
> 2011. 1. 23., 오전 6:21, Guennadi Liakhovetski 작성:
>
> > On Wed, 12 Jan 2011, Hans Verkuil wrote:
> >
> >> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>
> [snip]
>
> >> - case V4L2_CID_EXPOSURE:
> >> - /* mt9m001 has maximum == default */
> >> - if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
> >> - return -EINVAL;
> >> - else {
> >> - unsigned long range = qctrl->maximum - qctrl->minimum;
> >> - unsigned long shutter = ((ctrl->value - qctrl->minimum) * 1048 +
> >> + case V4L2_CID_EXPOSURE_AUTO:
> >> + /* Force manual exposure if only the exposure was changed */
> >> + if (!ctrl->has_new)
> >> + ctrl->val = V4L2_EXPOSURE_MANUAL;
> >> + if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
> >> + unsigned long range = exp->maximum - exp->minimum;
> >> + unsigned long shutter = ((exp->val - exp->minimum) * 1048 +
> >> range / 2) / range + 1;
> >>
> >> dev_dbg(&client->dev,
> >> "Setting shutter width from %d to %lu\n",
> >> - reg_read(client, MT9M001_SHUTTER_WIDTH),
> >> - shutter);
> >> + reg_read(client, MT9M001_SHUTTER_WIDTH), shutter);
> >> if (reg_write(client, MT9M001_SHUTTER_WIDTH, shutter) < 0)
> >> return -EIO;
> >> - mt9m001->exposure = ctrl->value;
> >> - mt9m001->autoexposure = 0;
> >> - }
> >> - break;
> >> - case V4L2_CID_EXPOSURE_AUTO:
> >> - if (ctrl->value) {
> >> + } else {
> >> const u16 vblank = 25;
> >> unsigned int total_h = mt9m001->rect.height +
> >> mt9m001->y_skip_top + vblank;
> >> - if (reg_write(client, MT9M001_SHUTTER_WIDTH,
> >> - total_h) < 0)
> >> +
> >> + if (reg_write(client, MT9M001_SHUTTER_WIDTH, total_h) < 0)
> >> return -EIO;
> >> - qctrl = soc_camera_find_qctrl(icd->ops, V4L2_CID_EXPOSURE);
> >> - mt9m001->exposure = (524 + (total_h - 1) *
> >> - (qctrl->maximum - qctrl->minimum)) /
> >> - 1048 + qctrl->minimum;
> >> - mt9m001->autoexposure = 1;
> >> - } else
> >> - mt9m001->autoexposure = 0;
> >> - break;
> >> + exp->val = (524 + (total_h - 1) *
> >> + (exp->maximum - exp->minimum)) / 1048 +
> >> + exp->minimum;
> >> + }
> >> + return 0;
> >> }
> >> - return 0;
> >> + return -EINVAL;
> >
> > It seems to me, that you've dropped V4L2_CID_EXPOSURE here, was it
> > intentional? I won't verify this in detail now, because, if it wasn't
> > intentional and you fix it in v2, I'll have to re-check it anyway. Or is
> > it supposed to be handled by that V4L2_EXPOSURE_MANUAL? So, if the user
> > issues a V4L2_CID_EXPOSURE, are you getting V4L2_CID_EXPOSURE_AUTO with
> > val == V4L2_EXPOSURE_MANUAL instead? Weird...
>
> I also wonder first at this part for a long time like below:
>
> 1. when calling V4L2_CID_EXPOSURE_AUTO with V4L2_EXPOSURE_AUTO, it's ok.
> 2. when calling V4L2_CID_EXPOSURE_AUTO with V4L2_EXPOSURE_MANUAL, it's
> also ok.
> 3. when calling V4L2_CID_EXPOSURE? where the device handle this CID?
>
> but, after testing with application step by step, I finally know below:
> when calling V4L2_CID_EXPOSURE, changing internal(v4l2_ctrl framework) variable,
> exactly struct v4l2_ctrl exposure, which is register for probing time by
> V4L2_CID_EXPOSURE, and clustered with struct v4l2_ctrl autoexposure. So, when
> the device no needs to handle this values, but it automatically calls control clustered with
> itself, in this case the V4L2_CID_EXPOSURE calls(just words)V4L2_CID_EXPOSURE_AUTO.
>
> So, the my POV is that foo clustered with auto_foo calls auto_foo with foo's characteristics.
Correct. This tells me two things: 1) nobody ever reads documentation, and 2) I
must place a comment in the code making people aware that the V4L2_CID_EXPOSURE_AUTO
case will handle all controls in the cluster.
Regards,
Hans
>
> But, Hans probably would do more clear answer.
>
> Regards,
> Heungjun Kim
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
Hans Verkuil - video4linux developer - sponsored by Cisco
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH 03/12] mt9m001: convert to the control framework.
2011-01-22 21:21 ` Guennadi Liakhovetski
2011-01-23 3:38 ` Kim HeungJun
@ 2011-01-25 7:54 ` Hans Verkuil
1 sibling, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-25 7:54 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
On Saturday, January 22, 2011 22:21:23 Guennadi Liakhovetski wrote:
> On Wed, 12 Jan 2011, Hans Verkuil wrote:
>
> > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> > ---
> > drivers/media/video/mt9m001.c | 210 +++++++++++++++--------------------------
> > 1 files changed, 75 insertions(+), 135 deletions(-)
> >
> > diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
> > index f7fc88d..b9b6e33 100644
> > --- a/drivers/media/video/mt9m001.c
> > +++ b/drivers/media/video/mt9m001.c
> > @@ -15,6 +15,7 @@
> >
> > #include <media/v4l2-subdev.h>
> > #include <media/v4l2-chip-ident.h>
> > +#include <media/v4l2-ctrls.h>
> > #include <media/soc_camera.h>
> >
> > /*
> > @@ -84,15 +85,18 @@ static const struct mt9m001_datafmt mt9m001_monochrome_fmts[] = {
> >
> > struct mt9m001 {
> > struct v4l2_subdev subdev;
> > + struct v4l2_ctrl_handler hdl;
> > + struct {
> > + /* exposure/auto-exposure cluster */
> > + struct v4l2_ctrl *autoexposure;
> > + struct v4l2_ctrl *exposure;
> > + };
>
> Hm, why an anonymous struct? Why not just put them directly at the top
> level?
There are a few ways you can declare control clusters. This is the most obvious:
struct v4l2_ctrl *exp_cluster[2];
The only problem with this is that it is very annoying if you have to access
one of these controls: doing 'state->exp_cluster[CTRL_EXPOSURE]->cur.val' is
quite a mouthful.
The other approach is to define the pointers directly at top level:
struct mt9m001 {
...
/* exposure/auto-exposure cluster */
struct v4l2_ctrl *autoexposure;
struct v4l2_ctrl *exposure;
};
The problem with that is that it isn't clear that this is a unit and that
you can't just add a field in between.
Using an anonymous struct will 1) keep the ease of use and 2) visually put
these pointers together in a unit.
I've been using it everywhere I need to make a control cluster and it works
very nicely.
> > struct v4l2_rect rect; /* Sensor window */
> > const struct mt9m001_datafmt *fmt;
> > const struct mt9m001_datafmt *fmts;
> > int num_fmts;
> > int model; /* V4L2_IDENT_MT9M001* codes from v4l2-chip-ident.h */
> > - unsigned int gain;
> > - unsigned int exposure;
> > unsigned short y_skip_top; /* Lines to skip at the top */
> > - unsigned char autoexposure;
> > };
> >
> > static struct mt9m001 *to_mt9m001(const struct i2c_client *client)
> > @@ -209,7 +213,6 @@ static int mt9m001_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > struct mt9m001 *mt9m001 = to_mt9m001(client);
> > struct v4l2_rect rect = a->c;
> > - struct soc_camera_device *icd = client->dev.platform_data;
> > int ret;
> > const u16 hblank = 9, vblank = 25;
> > unsigned int total_h;
> > @@ -251,17 +254,18 @@ static int mt9m001_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> > if (!ret)
> > ret = reg_write(client, MT9M001_WINDOW_HEIGHT,
> > rect.height + mt9m001->y_skip_top - 1);
> > - if (!ret && mt9m001->autoexposure) {
> > + v4l2_ctrl_lock(mt9m001->autoexposure);
> > + if (!ret && mt9m001->autoexposure->cur.val == V4L2_EXPOSURE_AUTO) {
> > ret = reg_write(client, MT9M001_SHUTTER_WIDTH, total_h);
> > if (!ret) {
> > - const struct v4l2_queryctrl *qctrl =
> > - soc_camera_find_qctrl(icd->ops,
> > - V4L2_CID_EXPOSURE);
> > - mt9m001->exposure = (524 + (total_h - 1) *
> > - (qctrl->maximum - qctrl->minimum)) /
> > - 1048 + qctrl->minimum;
> > + struct v4l2_ctrl *ctrl = mt9m001->exposure;
> > +
> > + ctrl->cur.val = (524 + (total_h - 1) *
> > + (ctrl->maximum - ctrl->minimum)) /
> > + 1048 + ctrl->minimum;
> > }
> > }
> > + v4l2_ctrl_unlock(mt9m001->autoexposure);
> >
> > if (!ret)
> > mt9m001->rect = rect;
> > @@ -421,107 +425,36 @@ static int mt9m001_s_register(struct v4l2_subdev *sd,
> > }
> > #endif
> >
> > -static const struct v4l2_queryctrl mt9m001_controls[] = {
> > - {
> > - .id = V4L2_CID_VFLIP,
> > - .type = V4L2_CTRL_TYPE_BOOLEAN,
> > - .name = "Flip Vertically",
> > - .minimum = 0,
> > - .maximum = 1,
> > - .step = 1,
> > - .default_value = 0,
> > - }, {
> > - .id = V4L2_CID_GAIN,
> > - .type = V4L2_CTRL_TYPE_INTEGER,
> > - .name = "Gain",
> > - .minimum = 0,
> > - .maximum = 127,
> > - .step = 1,
> > - .default_value = 64,
> > - .flags = V4L2_CTRL_FLAG_SLIDER,
> > - }, {
> > - .id = V4L2_CID_EXPOSURE,
> > - .type = V4L2_CTRL_TYPE_INTEGER,
> > - .name = "Exposure",
> > - .minimum = 1,
> > - .maximum = 255,
> > - .step = 1,
> > - .default_value = 255,
> > - .flags = V4L2_CTRL_FLAG_SLIDER,
> > - }, {
> > - .id = V4L2_CID_EXPOSURE_AUTO,
> > - .type = V4L2_CTRL_TYPE_BOOLEAN,
> > - .name = "Automatic Exposure",
> > - .minimum = 0,
> > - .maximum = 1,
> > - .step = 1,
> > - .default_value = 1,
> > - }
> > -};
> > -
> > static struct soc_camera_ops mt9m001_ops = {
> > .set_bus_param = mt9m001_set_bus_param,
> > .query_bus_param = mt9m001_query_bus_param,
> > - .controls = mt9m001_controls,
> > - .num_controls = ARRAY_SIZE(mt9m001_controls),
> > };
> >
> > -static int mt9m001_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> > +static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > + struct v4l2_subdev *sd =
> > + &container_of(ctrl->handler, struct mt9m001, hdl)->subdev;
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > struct mt9m001 *mt9m001 = to_mt9m001(client);
>
> This looks a bit clumsy to me, sorry. Above you already have "struct
> mt9m001 *" (container_of(ctrl->handler, struct mt9m001, hdl)), but you
> only use it implicitly to get to sd, and then mt9m001 is calculated
> again...
Yeah, I'll improve this.
>
> > + struct v4l2_ctrl *exp = mt9m001->exposure;
> > int data;
> >
> > switch (ctrl->id) {
> > case V4L2_CID_VFLIP:
> > - data = reg_read(client, MT9M001_READ_OPTIONS2);
> > - if (data < 0)
> > - return -EIO;
> > - ctrl->value = !!(data & 0x8000);
> > - break;
> > - case V4L2_CID_EXPOSURE_AUTO:
> > - ctrl->value = mt9m001->autoexposure;
> > - break;
> > - case V4L2_CID_GAIN:
> > - ctrl->value = mt9m001->gain;
> > - break;
> > - case V4L2_CID_EXPOSURE:
> > - ctrl->value = mt9m001->exposure;
> > - break;
> > - }
> > - return 0;
> > -}
> > -
> > -static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> > -{
> > - struct i2c_client *client = v4l2_get_subdevdata(sd);
> > - struct mt9m001 *mt9m001 = to_mt9m001(client);
> > - struct soc_camera_device *icd = client->dev.platform_data;
> > - const struct v4l2_queryctrl *qctrl;
> > - int data;
> > -
> > - qctrl = soc_camera_find_qctrl(&mt9m001_ops, ctrl->id);
> > -
> > - if (!qctrl)
> > - return -EINVAL;
> > -
> > - switch (ctrl->id) {
> > - case V4L2_CID_VFLIP:
> > - if (ctrl->value)
> > + if (ctrl->val)
> > data = reg_set(client, MT9M001_READ_OPTIONS2, 0x8000);
> > else
> > data = reg_clear(client, MT9M001_READ_OPTIONS2, 0x8000);
> > if (data < 0)
> > return -EIO;
> > - break;
> > + return 0;
> > +
> > case V4L2_CID_GAIN:
> > - if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
> > - return -EINVAL;
> > /* See Datasheet Table 7, Gain settings. */
> > - if (ctrl->value <= qctrl->default_value) {
> > + if (ctrl->val <= ctrl->default_value) {
> > /* Pack it into 0..1 step 0.125, register values 0..8 */
> > - unsigned long range = qctrl->default_value - qctrl->minimum;
> > - data = ((ctrl->value - qctrl->minimum) * 8 + range / 2) / range;
> > + unsigned long range = ctrl->default_value - ctrl->minimum;
> > + data = ((ctrl->val - ctrl->minimum) * 8 + range / 2) / range;
> >
> > dev_dbg(&client->dev, "Setting gain %d\n", data);
> > data = reg_write(client, MT9M001_GLOBAL_GAIN, data);
> > @@ -530,8 +463,8 @@ static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> > } else {
> > /* Pack it into 1.125..15 variable step, register values 9..67 */
> > /* We assume qctrl->maximum - qctrl->default_value - 1 > 0 */
> > - unsigned long range = qctrl->maximum - qctrl->default_value - 1;
> > - unsigned long gain = ((ctrl->value - qctrl->default_value - 1) *
> > + unsigned long range = ctrl->maximum - ctrl->default_value - 1;
> > + unsigned long gain = ((ctrl->val - ctrl->default_value - 1) *
> > 111 + range / 2) / range + 9;
> >
> > if (gain <= 32)
> > @@ -547,47 +480,36 @@ static int mt9m001_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> > if (data < 0)
> > return -EIO;
> > }
> > + return 0;
> >
> > - /* Success */
> > - mt9m001->gain = ctrl->value;
> > - break;
> > - case V4L2_CID_EXPOSURE:
> > - /* mt9m001 has maximum == default */
> > - if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
> > - return -EINVAL;
> > - else {
> > - unsigned long range = qctrl->maximum - qctrl->minimum;
> > - unsigned long shutter = ((ctrl->value - qctrl->minimum) * 1048 +
> > + case V4L2_CID_EXPOSURE_AUTO:
> > + /* Force manual exposure if only the exposure was changed */
> > + if (!ctrl->has_new)
> > + ctrl->val = V4L2_EXPOSURE_MANUAL;
> > + if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
> > + unsigned long range = exp->maximum - exp->minimum;
> > + unsigned long shutter = ((exp->val - exp->minimum) * 1048 +
> > range / 2) / range + 1;
> >
> > dev_dbg(&client->dev,
> > "Setting shutter width from %d to %lu\n",
> > - reg_read(client, MT9M001_SHUTTER_WIDTH),
> > - shutter);
> > + reg_read(client, MT9M001_SHUTTER_WIDTH), shutter);
> > if (reg_write(client, MT9M001_SHUTTER_WIDTH, shutter) < 0)
> > return -EIO;
> > - mt9m001->exposure = ctrl->value;
> > - mt9m001->autoexposure = 0;
> > - }
> > - break;
> > - case V4L2_CID_EXPOSURE_AUTO:
> > - if (ctrl->value) {
> > + } else {
> > const u16 vblank = 25;
> > unsigned int total_h = mt9m001->rect.height +
> > mt9m001->y_skip_top + vblank;
> > - if (reg_write(client, MT9M001_SHUTTER_WIDTH,
> > - total_h) < 0)
> > +
> > + if (reg_write(client, MT9M001_SHUTTER_WIDTH, total_h) < 0)
> > return -EIO;
> > - qctrl = soc_camera_find_qctrl(icd->ops, V4L2_CID_EXPOSURE);
> > - mt9m001->exposure = (524 + (total_h - 1) *
> > - (qctrl->maximum - qctrl->minimum)) /
> > - 1048 + qctrl->minimum;
> > - mt9m001->autoexposure = 1;
> > - } else
> > - mt9m001->autoexposure = 0;
> > - break;
> > + exp->val = (524 + (total_h - 1) *
> > + (exp->maximum - exp->minimum)) / 1048 +
> > + exp->minimum;
> > + }
> > + return 0;
> > }
> > - return 0;
> > + return -EINVAL;
>
> It seems to me, that you've dropped V4L2_CID_EXPOSURE here, was it
> intentional? I won't verify this in detail now, because, if it wasn't
> intentional and you fix it in v2, I'll have to re-check it anyway. Or is
> it supposed to be handled by that V4L2_EXPOSURE_MANUAL? So, if the user
> issues a V4L2_CID_EXPOSURE, are you getting V4L2_CID_EXPOSURE_AUTO with
> val == V4L2_EXPOSURE_MANUAL instead? Weird...
If you cluster multiple controls (i.e. controls that need to be set atomically
for one reason or another), then whenever the application changes one or more
controls in that cluster only one call to s_ctrl will be made with the ID of
the first (aka 'master') control of the cluster. All the new values of all the
controls in the cluster are filled in, ready to be applied. Since EXPOSURE_AUTO
is the first control in the cluster, that's the ID you get here.
While this is all documented in v4l2-controls.txt your question makes it clear
that I need to add a comment before case statements like this.
>
> > }
> >
> > /*
> > @@ -665,10 +587,7 @@ static int mt9m001_video_probe(struct soc_camera_device *icd,
> > dev_err(&client->dev, "Failed to initialise the camera\n");
> >
> > /* mt9m001_init() has reset the chip, returning registers to defaults */
> > - mt9m001->gain = 64;
> > - mt9m001->exposure = 255;
> > -
> > - return ret;
> > + return v4l2_ctrl_handler_setup(&mt9m001->hdl);
> > }
> >
> > static void mt9m001_video_remove(struct soc_camera_device *icd)
> > @@ -691,9 +610,11 @@ static int mt9m001_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
> > return 0;
> > }
> >
> > +static const struct v4l2_ctrl_ops mt9m001_ctrl_ops = {
> > + .s_ctrl = mt9m001_s_ctrl,
> > +};
> > +
> > static struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
> > - .g_ctrl = mt9m001_g_ctrl,
> > - .s_ctrl = mt9m001_s_ctrl,
> > .g_chip_ident = mt9m001_g_chip_ident,
> > #ifdef CONFIG_VIDEO_ADV_DEBUG
> > .g_register = mt9m001_g_register,
> > @@ -766,6 +687,28 @@ static int mt9m001_probe(struct i2c_client *client,
> > return -ENOMEM;
> >
> > v4l2_i2c_subdev_init(&mt9m001->subdev, client, &mt9m001_subdev_ops);
> > + v4l2_ctrl_handler_init(&mt9m001->hdl, 4);
> > + v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
> > + V4L2_CID_VFLIP, 0, 1, 1, 0);
> > + v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
> > + V4L2_CID_GAIN, 0, 127, 1, 64);
> > + mt9m001->exposure = v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
> > + V4L2_CID_EXPOSURE, 1, 255, 1, 255);
> > + /*
> > + * Simulated autoexposure. If enabled, we calculate shutter width
> > + * ourselves in the driver based on vertical blanking and frame width
> > + */
> > + mt9m001->autoexposure = v4l2_ctrl_new_std_menu(&mt9m001->hdl,
> > + &mt9m001_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
> > + V4L2_EXPOSURE_AUTO);
> > + mt9m001->subdev.ctrl_handler = &mt9m001->hdl;
> > + if (mt9m001->hdl.error) {
> > + int err = mt9m001->hdl.error;
> > +
> > + kfree(mt9m001);
> > + return err;
> > + }
> > + v4l2_ctrl_cluster(2, &mt9m001->autoexposure);
>
> Ooh, is this the reason for that anonymous struct?...
>
> >
> > /* Second stage probe - when a capture adapter is there */
> > icd->ops = &mt9m001_ops;
> > @@ -776,15 +719,10 @@ static int mt9m001_probe(struct i2c_client *client,
> > mt9m001->rect.width = MT9M001_MAX_WIDTH;
> > mt9m001->rect.height = MT9M001_MAX_HEIGHT;
> >
> > - /*
> > - * Simulated autoexposure. If enabled, we calculate shutter width
> > - * ourselves in the driver based on vertical blanking and frame width
> > - */
> > - mt9m001->autoexposure = 1;
> > -
> > ret = mt9m001_video_probe(icd, client);
> > if (ret) {
> > icd->ops = NULL;
> > + v4l2_ctrl_handler_free(&mt9m001->hdl);
> > kfree(mt9m001);
> > }
> >
> > @@ -796,6 +734,8 @@ static int mt9m001_remove(struct i2c_client *client)
> > struct mt9m001 *mt9m001 = to_mt9m001(client);
> > struct soc_camera_device *icd = client->dev.platform_data;
> >
> > + v4l2_device_unregister_subdev(&mt9m001->subdev);
>
> hm, first, this is not really related, right? Secondly, are you sure this
> is needed? It is now double with soc_camera_remove(). I know, it is safe,
> but still, one of them looks superfluous to me.
As long as this subdev is only used with soc_camera, then this isn't needed.
But we are now getting close to the point where these subdevs are no longer
tied to soc_camera. And as soon as they can be used in e.g. USB devices, then
this call is needed. Many (all?) USB drivers will not unregister the subdevs
before removing the i2c adapter, instead they just delete the i2c adapter,
which forces the removal of all attached i2c drivers. So any i2c subdev drivers
need to call the unregister_subdev themselves as part of the cleanup.
This is actually documented in v4l2-framework.txt. It's not pretty, but unless
all drivers are audited for the order in which they clean up their subdevs and
i2c adapter we have to support both methods.
So this is a preparation for the moment the last connection to soc_camera is
removed.
If you really don't want this, then I can remove it. But then we must remember
to put this back later.
>
> > + v4l2_ctrl_handler_free(&mt9m001->hdl);
> > icd->ops = NULL;
> > mt9m001_video_remove(icd);
> > kfree(mt9m001);
>
> Thanks
> Guennadi
> ---
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH 04/12] mt9m111.c: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
2011-01-11 23:06 ` [RFC PATCH 02/12] sh_mobile_ceu_camera: implement the control handler Hans Verkuil
2011-01-11 23:06 ` [RFC PATCH 03/12] mt9m001: convert to the control framework Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-22 23:45 ` Guennadi Liakhovetski
2011-01-31 20:50 ` Robert Jarzmik
2011-01-11 23:06 ` [RFC PATCH 05/12] ov9640: " Hans Verkuil
` (8 subsequent siblings)
11 siblings, 2 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/mt9m111.c | 184 ++++++++++++-----------------------------
1 files changed, 54 insertions(+), 130 deletions(-)
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 53fa2a7..2328579 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -16,6 +16,7 @@
#include <media/v4l2-common.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ctrls.h>
#include <media/soc_camera.h>
/*
@@ -165,22 +166,19 @@ enum mt9m111_context {
struct mt9m111 {
struct v4l2_subdev subdev;
+ struct v4l2_ctrl_handler hdl;
+ struct v4l2_ctrl *gain;
int model; /* V4L2_IDENT_MT9M111 or V4L2_IDENT_MT9M112 code
* from v4l2-chip-ident.h */
enum mt9m111_context context;
struct v4l2_rect rect;
const struct mt9m111_datafmt *fmt;
- unsigned int gain;
- unsigned char autoexposure;
unsigned char datawidth;
unsigned int powered:1;
- unsigned int hflip:1;
- unsigned int vflip:1;
unsigned int swap_rgb_even_odd:1;
unsigned int swap_rgb_red_blue:1;
unsigned int swap_yuv_y_chromas:1;
unsigned int swap_yuv_cb_cr:1;
- unsigned int autowhitebalance:1;
};
static struct mt9m111 *to_mt9m111(const struct i2c_client *client)
@@ -679,43 +677,6 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
}
#endif
-static const struct v4l2_queryctrl mt9m111_controls[] = {
- {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Verticaly",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- }, {
- .id = V4L2_CID_HFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Horizontaly",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- }, { /* gain = 1/32*val (=>gain=1 if val==32) */
- .id = V4L2_CID_GAIN,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Gain",
- .minimum = 0,
- .maximum = 63 * 2 * 2,
- .step = 1,
- .default_value = 32,
- .flags = V4L2_CTRL_FLAG_SLIDER,
- }, {
- .id = V4L2_CID_EXPOSURE_AUTO,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Auto Exposure",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 1,
- }
-};
-
static int mt9m111_resume(struct soc_camera_device *icd);
static int mt9m111_suspend(struct soc_camera_device *icd, pm_message_t state);
@@ -724,8 +685,6 @@ static struct soc_camera_ops mt9m111_ops = {
.resume = mt9m111_resume,
.query_bus_param = mt9m111_query_bus_param,
.set_bus_param = mt9m111_set_bus_param,
- .controls = mt9m111_controls,
- .num_controls = ARRAY_SIZE(mt9m111_controls),
};
static int mt9m111_set_flip(struct i2c_client *client, int flip, int mask)
@@ -761,13 +720,11 @@ static int mt9m111_get_global_gain(struct i2c_client *client)
static int mt9m111_set_global_gain(struct i2c_client *client, int gain)
{
- struct mt9m111 *mt9m111 = to_mt9m111(client);
u16 val;
if (gain > 63 * 2 * 2)
return -EINVAL;
- mt9m111->gain = gain;
if ((gain >= 64 * 2) && (gain < 63 * 2 * 2))
val = (1 << 10) | (1 << 9) | (gain / 4);
else if ((gain >= 64) && (gain < 64 * 2))
@@ -780,115 +737,67 @@ static int mt9m111_set_global_gain(struct i2c_client *client, int gain)
static int mt9m111_set_autoexposure(struct i2c_client *client, int on)
{
- struct mt9m111 *mt9m111 = to_mt9m111(client);
int ret;
if (on)
ret = reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
else
ret = reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
-
- if (!ret)
- mt9m111->autoexposure = on;
-
return ret;
}
static int mt9m111_set_autowhitebalance(struct i2c_client *client, int on)
{
- struct mt9m111 *mt9m111 = to_mt9m111(client);
int ret;
if (on)
ret = reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
else
ret = reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
-
- if (!ret)
- mt9m111->autowhitebalance = on;
-
return ret;
}
-static int mt9m111_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int mt9m111_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct mt9m111, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct mt9m111 *mt9m111 = to_mt9m111(client);
int data;
switch (ctrl->id) {
- case V4L2_CID_VFLIP:
- if (mt9m111->context == HIGHPOWER)
- data = reg_read(READ_MODE_B);
- else
- data = reg_read(READ_MODE_A);
-
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & MT9M111_RMB_MIRROR_ROWS);
- break;
- case V4L2_CID_HFLIP:
- if (mt9m111->context == HIGHPOWER)
- data = reg_read(READ_MODE_B);
- else
- data = reg_read(READ_MODE_A);
-
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & MT9M111_RMB_MIRROR_COLS);
- break;
case V4L2_CID_GAIN:
data = mt9m111_get_global_gain(client);
if (data < 0)
return data;
- ctrl->value = data;
- break;
- case V4L2_CID_EXPOSURE_AUTO:
- ctrl->value = mt9m111->autoexposure;
- break;
- case V4L2_CID_AUTO_WHITE_BALANCE:
- ctrl->value = mt9m111->autowhitebalance;
- break;
+ ctrl->cur.val = data;
+ return 0;
}
- return 0;
+ return -EINVAL;
}
-static int mt9m111_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct mt9m111, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct mt9m111 *mt9m111 = to_mt9m111(client);
- const struct v4l2_queryctrl *qctrl;
- int ret;
-
- qctrl = soc_camera_find_qctrl(&mt9m111_ops, ctrl->id);
- if (!qctrl)
- return -EINVAL;
switch (ctrl->id) {
case V4L2_CID_VFLIP:
- mt9m111->vflip = ctrl->value;
- ret = mt9m111_set_flip(client, ctrl->value,
+ return mt9m111_set_flip(client, ctrl->val,
MT9M111_RMB_MIRROR_ROWS);
- break;
case V4L2_CID_HFLIP:
- mt9m111->hflip = ctrl->value;
- ret = mt9m111_set_flip(client, ctrl->value,
+ return mt9m111_set_flip(client, ctrl->val,
MT9M111_RMB_MIRROR_COLS);
- break;
case V4L2_CID_GAIN:
- ret = mt9m111_set_global_gain(client, ctrl->value);
- break;
+ return mt9m111_set_global_gain(client, ctrl->val);
+
case V4L2_CID_EXPOSURE_AUTO:
- ret = mt9m111_set_autoexposure(client, ctrl->value);
- break;
+ return mt9m111_set_autoexposure(client, ctrl->val);
+
case V4L2_CID_AUTO_WHITE_BALANCE:
- ret = mt9m111_set_autowhitebalance(client, ctrl->value);
- break;
- default:
- ret = -EINVAL;
+ return mt9m111_set_autowhitebalance(client, ctrl->val);
}
-
- return ret;
+ return -EINVAL;
}
static int mt9m111_suspend(struct soc_camera_device *icd, pm_message_t state)
@@ -896,8 +805,7 @@ static int mt9m111_suspend(struct soc_camera_device *icd, pm_message_t state)
struct i2c_client *client = to_i2c_client(to_soc_camera_control(icd));
struct mt9m111 *mt9m111 = to_mt9m111(client);
- mt9m111->gain = mt9m111_get_global_gain(client);
-
+ v4l2_ctrl_s_ctrl(mt9m111->gain, mt9m111_get_global_gain(client));
return 0;
}
@@ -908,11 +816,7 @@ static int mt9m111_restore_state(struct i2c_client *client)
mt9m111_set_context(client, mt9m111->context);
mt9m111_set_pixfmt(client, mt9m111->fmt->code);
mt9m111_setup_rect(client, &mt9m111->rect);
- mt9m111_set_flip(client, mt9m111->hflip, MT9M111_RMB_MIRROR_COLS);
- mt9m111_set_flip(client, mt9m111->vflip, MT9M111_RMB_MIRROR_ROWS);
- mt9m111_set_global_gain(client, mt9m111->gain);
- mt9m111_set_autoexposure(client, mt9m111->autoexposure);
- mt9m111_set_autowhitebalance(client, mt9m111->autowhitebalance);
+ v4l2_ctrl_handler_setup(&mt9m111->hdl);
return 0;
}
@@ -943,8 +847,6 @@ static int mt9m111_init(struct i2c_client *client)
ret = mt9m111_reset(client);
if (!ret)
ret = mt9m111_set_context(client, mt9m111->context);
- if (!ret)
- ret = mt9m111_set_autoexposure(client, mt9m111->autoexposure);
if (ret)
dev_err(&client->dev, "mt9m111 init failed: %d\n", ret);
return ret;
@@ -969,9 +871,6 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
to_soc_camera_host(icd->dev.parent)->nr != icd->iface)
return -ENODEV;
- mt9m111->autoexposure = 1;
- mt9m111->autowhitebalance = 1;
-
mt9m111->swap_rgb_even_odd = 1;
mt9m111->swap_rgb_red_blue = 1;
@@ -988,22 +887,24 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n", data);
break;
default:
- ret = -ENODEV;
dev_err(&client->dev,
"No MT9M111/MT9M112/MT9M131 chip detected register read %x\n",
data);
- goto ei2c;
+ return -ENODEV;
}
ret = mt9m111_init(client);
-
-ei2c:
- return ret;
+ if (ret)
+ return ret;
+ return v4l2_ctrl_handler_setup(&mt9m111->hdl);
}
+static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = {
+ .g_volatile_ctrl = mt9m111_g_volatile_ctrl,
+ .s_ctrl = mt9m111_s_ctrl,
+};
+
static struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = {
- .g_ctrl = mt9m111_g_ctrl,
- .s_ctrl = mt9m111_s_ctrl,
.g_chip_ident = mt9m111_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = mt9m111_g_register,
@@ -1067,6 +968,26 @@ static int mt9m111_probe(struct i2c_client *client,
return -ENOMEM;
v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
+ v4l2_ctrl_handler_init(&mt9m111->hdl, 5);
+ v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
+ V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
+ mt9m111->gain = v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
+ V4L2_CID_GAIN, 0, 63 * 2 * 2, 1, 32);
+ v4l2_ctrl_new_std_menu(&mt9m111->hdl,
+ &mt9m111_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
+ V4L2_EXPOSURE_AUTO);
+ mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
+ if (mt9m111->hdl.error) {
+ int err = mt9m111->hdl.error;
+
+ kfree(mt9m111);
+ return err;
+ }
+ mt9m111->gain->is_volatile = 1;
/* Second stage probe - when a capture adapter is there */
icd->ops = &mt9m111_ops;
@@ -1080,6 +1001,7 @@ static int mt9m111_probe(struct i2c_client *client,
ret = mt9m111_video_probe(icd, client);
if (ret) {
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&mt9m111->hdl);
kfree(mt9m111);
}
@@ -1091,7 +1013,9 @@ static int mt9m111_remove(struct i2c_client *client)
struct mt9m111 *mt9m111 = to_mt9m111(client);
struct soc_camera_device *icd = client->dev.platform_data;
+ v4l2_device_unregister_subdev(&mt9m111->subdev);
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&mt9m111->hdl);
kfree(mt9m111);
return 0;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 04/12] mt9m111.c: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 04/12] mt9m111.c: " Hans Verkuil
@ 2011-01-22 23:45 ` Guennadi Liakhovetski
2011-01-25 7:59 ` Hans Verkuil
2011-01-31 20:50 ` Robert Jarzmik
1 sibling, 1 reply; 27+ messages in thread
From: Guennadi Liakhovetski @ 2011-01-22 23:45 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
On Wed, 12 Jan 2011, Hans Verkuil wrote:
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
> drivers/media/video/mt9m111.c | 184 ++++++++++++-----------------------------
> 1 files changed, 54 insertions(+), 130 deletions(-)
>
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index 53fa2a7..2328579 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
[snip]
> -static int mt9m111_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> +static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> + struct v4l2_subdev *sd =
> + &container_of(ctrl->handler, struct mt9m111, hdl)->subdev;
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> - struct mt9m111 *mt9m111 = to_mt9m111(client);
> - const struct v4l2_queryctrl *qctrl;
> - int ret;
> -
> - qctrl = soc_camera_find_qctrl(&mt9m111_ops, ctrl->id);
> - if (!qctrl)
> - return -EINVAL;
>
> switch (ctrl->id) {
> case V4L2_CID_VFLIP:
> - mt9m111->vflip = ctrl->value;
> - ret = mt9m111_set_flip(client, ctrl->value,
> + return mt9m111_set_flip(client, ctrl->val,
> MT9M111_RMB_MIRROR_ROWS);
> - break;
> case V4L2_CID_HFLIP:
> - mt9m111->hflip = ctrl->value;
> - ret = mt9m111_set_flip(client, ctrl->value,
> + return mt9m111_set_flip(client, ctrl->val,
> MT9M111_RMB_MIRROR_COLS);
> - break;
> case V4L2_CID_GAIN:
> - ret = mt9m111_set_global_gain(client, ctrl->value);
> - break;
> + return mt9m111_set_global_gain(client, ctrl->val);
> +
> case V4L2_CID_EXPOSURE_AUTO:
> - ret = mt9m111_set_autoexposure(client, ctrl->value);
> - break;
> + return mt9m111_set_autoexposure(client, ctrl->val);
> +
> case V4L2_CID_AUTO_WHITE_BALANCE:
> - ret = mt9m111_set_autowhitebalance(client, ctrl->value);
> - break;
> - default:
> - ret = -EINVAL;
> + return mt9m111_set_autowhitebalance(client, ctrl->val);
> }
> -
> - return ret;
> + return -EINVAL;
> }
>
> static int mt9m111_suspend(struct soc_camera_device *icd, pm_message_t state)
[snip]
> @@ -1067,6 +968,26 @@ static int mt9m111_probe(struct i2c_client *client,
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
> + v4l2_ctrl_handler_init(&mt9m111->hdl, 5);
> + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> + V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
> + mt9m111->gain = v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> + V4L2_CID_GAIN, 0, 63 * 2 * 2, 1, 32);
> + v4l2_ctrl_new_std_menu(&mt9m111->hdl,
> + &mt9m111_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
> + V4L2_EXPOSURE_AUTO);
> + mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> + if (mt9m111->hdl.error) {
> + int err = mt9m111->hdl.error;
> +
> + kfree(mt9m111);
> + return err;
> + }
> + mt9m111->gain->is_volatile = 1;
I'm not sure I like this approach: you register each control separately,
but with the same handler, and then in that handler you switch-case again
to find out which control has to be processed... If we already register
them separately, and they share no code, apart from context extraction
from parameters - why not make separate handlers, waste some memory on a
couple more structs, but avoid run-time switching (I know it is not
critical, although, with still photo-shooting you might want to care about
the time between your controls and the actual shot), and win clarity?
>
> /* Second stage probe - when a capture adapter is there */
> icd->ops = &mt9m111_ops;
> @@ -1080,6 +1001,7 @@ static int mt9m111_probe(struct i2c_client *client,
> ret = mt9m111_video_probe(icd, client);
> if (ret) {
> icd->ops = NULL;
> + v4l2_ctrl_handler_free(&mt9m111->hdl);
> kfree(mt9m111);
> }
>
> @@ -1091,7 +1013,9 @@ static int mt9m111_remove(struct i2c_client *client)
> struct mt9m111 *mt9m111 = to_mt9m111(client);
> struct soc_camera_device *icd = client->dev.platform_data;
>
> + v4l2_device_unregister_subdev(&mt9m111->subdev);
Same here - don't like redundancy with soc_camera.c
Thanks
Guennadi
> icd->ops = NULL;
> + v4l2_ctrl_handler_free(&mt9m111->hdl);
> kfree(mt9m111);
>
> return 0;
> --
> 1.7.0.4
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 04/12] mt9m111.c: convert to the control framework.
2011-01-22 23:45 ` Guennadi Liakhovetski
@ 2011-01-25 7:59 ` Hans Verkuil
0 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-25 7:59 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
On Sunday, January 23, 2011 00:45:15 Guennadi Liakhovetski wrote:
> On Wed, 12 Jan 2011, Hans Verkuil wrote:
>
> > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> > ---
> > drivers/media/video/mt9m111.c | 184 ++++++++++++-----------------------------
> > 1 files changed, 54 insertions(+), 130 deletions(-)
> >
> > diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> > index 53fa2a7..2328579 100644
> > --- a/drivers/media/video/mt9m111.c
> > +++ b/drivers/media/video/mt9m111.c
>
> [snip]
>
> > -static int mt9m111_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> > +static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > + struct v4l2_subdev *sd =
> > + &container_of(ctrl->handler, struct mt9m111, hdl)->subdev;
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > - struct mt9m111 *mt9m111 = to_mt9m111(client);
> > - const struct v4l2_queryctrl *qctrl;
> > - int ret;
> > -
> > - qctrl = soc_camera_find_qctrl(&mt9m111_ops, ctrl->id);
> > - if (!qctrl)
> > - return -EINVAL;
> >
> > switch (ctrl->id) {
> > case V4L2_CID_VFLIP:
> > - mt9m111->vflip = ctrl->value;
> > - ret = mt9m111_set_flip(client, ctrl->value,
> > + return mt9m111_set_flip(client, ctrl->val,
> > MT9M111_RMB_MIRROR_ROWS);
> > - break;
> > case V4L2_CID_HFLIP:
> > - mt9m111->hflip = ctrl->value;
> > - ret = mt9m111_set_flip(client, ctrl->value,
> > + return mt9m111_set_flip(client, ctrl->val,
> > MT9M111_RMB_MIRROR_COLS);
> > - break;
> > case V4L2_CID_GAIN:
> > - ret = mt9m111_set_global_gain(client, ctrl->value);
> > - break;
> > + return mt9m111_set_global_gain(client, ctrl->val);
> > +
> > case V4L2_CID_EXPOSURE_AUTO:
> > - ret = mt9m111_set_autoexposure(client, ctrl->value);
> > - break;
> > + return mt9m111_set_autoexposure(client, ctrl->val);
> > +
> > case V4L2_CID_AUTO_WHITE_BALANCE:
> > - ret = mt9m111_set_autowhitebalance(client, ctrl->value);
> > - break;
> > - default:
> > - ret = -EINVAL;
> > + return mt9m111_set_autowhitebalance(client, ctrl->val);
> > }
> > -
> > - return ret;
> > + return -EINVAL;
> > }
> >
> > static int mt9m111_suspend(struct soc_camera_device *icd, pm_message_t state)
>
> [snip]
>
> > @@ -1067,6 +968,26 @@ static int mt9m111_probe(struct i2c_client *client,
> > return -ENOMEM;
> >
> > v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
> > + v4l2_ctrl_handler_init(&mt9m111->hdl, 5);
> > + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > + V4L2_CID_VFLIP, 0, 1, 1, 0);
> > + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > + V4L2_CID_HFLIP, 0, 1, 1, 0);
> > + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > + V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
> > + mt9m111->gain = v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > + V4L2_CID_GAIN, 0, 63 * 2 * 2, 1, 32);
> > + v4l2_ctrl_new_std_menu(&mt9m111->hdl,
> > + &mt9m111_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
> > + V4L2_EXPOSURE_AUTO);
> > + mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> > + if (mt9m111->hdl.error) {
> > + int err = mt9m111->hdl.error;
> > +
> > + kfree(mt9m111);
> > + return err;
> > + }
> > + mt9m111->gain->is_volatile = 1;
>
> I'm not sure I like this approach: you register each control separately,
> but with the same handler, and then in that handler you switch-case again
> to find out which control has to be processed... If we already register
> them separately, and they share no code, apart from context extraction
> from parameters - why not make separate handlers, waste some memory on a
> couple more structs, but avoid run-time switching (I know it is not
> critical, although, with still photo-shooting you might want to care about
> the time between your controls and the actual shot), and win clarity?
I find that as long as the handler for each control is short (one-liners
in this case), then there is no benefit to split it up. It just adds a
fair amount of code to the source which in my opinion does not make it
easier to read.
Regards,
Hans
>
> >
> > /* Second stage probe - when a capture adapter is there */
> > icd->ops = &mt9m111_ops;
> > @@ -1080,6 +1001,7 @@ static int mt9m111_probe(struct i2c_client *client,
> > ret = mt9m111_video_probe(icd, client);
> > if (ret) {
> > icd->ops = NULL;
> > + v4l2_ctrl_handler_free(&mt9m111->hdl);
> > kfree(mt9m111);
> > }
> >
> > @@ -1091,7 +1013,9 @@ static int mt9m111_remove(struct i2c_client *client)
> > struct mt9m111 *mt9m111 = to_mt9m111(client);
> > struct soc_camera_device *icd = client->dev.platform_data;
> >
> > + v4l2_device_unregister_subdev(&mt9m111->subdev);
>
> Same here - don't like redundancy with soc_camera.c
>
> Thanks
> Guennadi
>
> > icd->ops = NULL;
> > + v4l2_ctrl_handler_free(&mt9m111->hdl);
> > kfree(mt9m111);
> >
> > return 0;
>
> ---
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
Hans Verkuil - video4linux developer - sponsored by Cisco
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH 04/12] mt9m111.c: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 04/12] mt9m111.c: " Hans Verkuil
2011-01-22 23:45 ` Guennadi Liakhovetski
@ 2011-01-31 20:50 ` Robert Jarzmik
1 sibling, 0 replies; 27+ messages in thread
From: Robert Jarzmik @ 2011-01-31 20:50 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Guennadi Liakhovetski, Magnus Damm,
Kuninori Morimoto, Alberto Panizzo, Janusz Krzysztofik,
Marek Vasut
Hans Verkuil <hverkuil@xs4all.nl> writes:
[zip]
> @@ -1067,6 +968,26 @@ static int mt9m111_probe(struct i2c_client *client,
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
> + v4l2_ctrl_handler_init(&mt9m111->hdl, 5);
> + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> + V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
> + mt9m111->gain = v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> + V4L2_CID_GAIN, 0, 63 * 2 * 2, 1, 32);
> + v4l2_ctrl_new_std_menu(&mt9m111->hdl,
> + &mt9m111_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
> + V4L2_EXPOSURE_AUTO);
> + mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> + if (mt9m111->hdl.error) {
> + int err = mt9m111->hdl.error;
> +
> + kfree(mt9m111);
> + return err;
> + }
> + mt9m111->gain->is_volatile = 1;
Hi Hans,
I would like to shift all the control initializations into one subfunction,
called from mt9m111_probe(). Right now it's not an issue, but if future
development adds a lot of controls, I'd like the controls initialization to be
gathered in one method.
Apart from that, I have no special comment.
Cheers.
--
Robert
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH 05/12] ov9640: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (2 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 04/12] mt9m111.c: " Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-11 23:51 ` Marek Vasut
2011-01-11 23:06 ` [RFC PATCH 06/12] mt9t031: " Hans Verkuil
` (7 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/ov9640.c | 124 ++++++++++++++----------------------------
drivers/media/video/ov9640.h | 4 +-
2 files changed, 43 insertions(+), 85 deletions(-)
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
index 53d88a2..dbda50c 100644
--- a/drivers/media/video/ov9640.c
+++ b/drivers/media/video/ov9640.c
@@ -27,6 +27,7 @@
#include <linux/videodev2.h>
#include <media/v4l2-chip-ident.h>
#include <media/v4l2-common.h>
+#include <media/v4l2-ctrls.h>
#include <media/soc_camera.h>
#include "ov9640.h"
@@ -162,27 +163,6 @@ static enum v4l2_mbus_pixelcode ov9640_codes[] = {
V4L2_MBUS_FMT_RGB565_2X8_LE,
};
-static const struct v4l2_queryctrl ov9640_controls[] = {
- {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Vertically",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
- {
- .id = V4L2_CID_HFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Horizontally",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
-};
-
/* read a register */
static int ov9640_reg_read(struct i2c_client *client, u8 reg, u8 *val)
{
@@ -307,52 +287,25 @@ static unsigned long ov9640_query_bus_param(struct soc_camera_device *icd)
return soc_camera_apply_sensor_flags(icl, flags);
}
-/* Get status of additional camera capabilities */
-static int ov9640_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
-{
- struct ov9640_priv *priv = to_ov9640_sensor(sd);
-
- switch (ctrl->id) {
- case V4L2_CID_VFLIP:
- ctrl->value = priv->flag_vflip;
- break;
- case V4L2_CID_HFLIP:
- ctrl->value = priv->flag_hflip;
- break;
- }
- return 0;
-}
-
/* Set status of additional camera capabilities */
-static int ov9640_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int ov9640_s_ctrl(struct v4l2_ctrl *ctrl)
{
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov9640_priv *priv = to_ov9640_sensor(sd);
-
- int ret = 0;
+ struct ov9640_priv *priv = container_of(ctrl->handler, struct ov9640_priv, hdl);
+ struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
switch (ctrl->id) {
case V4L2_CID_VFLIP:
- priv->flag_vflip = ctrl->value;
- if (ctrl->value)
- ret = ov9640_reg_rmw(client, OV9640_MVFP,
+ if (ctrl->val)
+ return ov9640_reg_rmw(client, OV9640_MVFP,
OV9640_MVFP_V, 0);
- else
- ret = ov9640_reg_rmw(client, OV9640_MVFP,
- 0, OV9640_MVFP_V);
- break;
+ return ov9640_reg_rmw(client, OV9640_MVFP, 0, OV9640_MVFP_V);
case V4L2_CID_HFLIP:
- priv->flag_hflip = ctrl->value;
- if (ctrl->value)
- ret = ov9640_reg_rmw(client, OV9640_MVFP,
+ if (ctrl->val)
+ return ov9640_reg_rmw(client, OV9640_MVFP,
OV9640_MVFP_H, 0);
- else
- ret = ov9640_reg_rmw(client, OV9640_MVFP,
- 0, OV9640_MVFP_H);
- break;
+ return ov9640_reg_rmw(client, OV9640_MVFP, 0, OV9640_MVFP_H);
}
-
- return ret;
+ return -EINVAL;
}
/* Get chip identification */
@@ -664,8 +617,7 @@ static int ov9640_video_probe(struct soc_camera_device *icd,
if (!icd->dev.parent ||
to_soc_camera_host(icd->dev.parent)->nr != icd->iface) {
dev_err(&client->dev, "Parent missing or invalid!\n");
- ret = -ENODEV;
- goto err;
+ return -ENODEV;
}
/*
@@ -673,20 +625,14 @@ static int ov9640_video_probe(struct soc_camera_device *icd,
*/
ret = ov9640_reg_read(client, OV9640_PID, &pid);
+ if (!ret)
+ ret = ov9640_reg_read(client, OV9640_VER, &ver);
+ if (!ret)
+ ret = ov9640_reg_read(client, OV9640_MIDH, &midh);
+ if (!ret)
+ ret = ov9640_reg_read(client, OV9640_MIDL, &midl);
if (ret)
- goto err;
-
- ret = ov9640_reg_read(client, OV9640_VER, &ver);
- if (ret)
- goto err;
-
- ret = ov9640_reg_read(client, OV9640_MIDH, &midh);
- if (ret)
- goto err;
-
- ret = ov9640_reg_read(client, OV9640_MIDL, &midl);
- if (ret)
- goto err;
+ return ret;
switch (VERSION(pid, ver)) {
case OV9640_V2:
@@ -700,27 +646,25 @@ static int ov9640_video_probe(struct soc_camera_device *icd,
break;
default:
dev_err(&client->dev, "Product ID error %x:%x\n", pid, ver);
- ret = -ENODEV;
- goto err;
+ return -ENODEV;
}
dev_info(&client->dev, "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
devname, pid, ver, midh, midl);
-err:
- return ret;
+ return v4l2_ctrl_handler_setup(&priv->hdl);
}
+static const struct v4l2_ctrl_ops ov9640_ctrl_ops = {
+ .s_ctrl = ov9640_s_ctrl,
+};
+
static struct soc_camera_ops ov9640_ops = {
.set_bus_param = ov9640_set_bus_param,
.query_bus_param = ov9640_query_bus_param,
- .controls = ov9640_controls,
- .num_controls = ARRAY_SIZE(ov9640_controls),
};
static struct v4l2_subdev_core_ops ov9640_core_ops = {
- .g_ctrl = ov9640_g_ctrl,
- .s_ctrl = ov9640_s_ctrl,
.g_chip_ident = ov9640_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = ov9640_get_register,
@@ -775,12 +719,26 @@ static int ov9640_probe(struct i2c_client *client,
v4l2_i2c_subdev_init(&priv->subdev, client, &ov9640_subdev_ops);
- icd->ops = &ov9640_ops;
+ v4l2_ctrl_handler_init(&priv->hdl, 2);
+ v4l2_ctrl_new_std(&priv->hdl, &ov9640_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&priv->hdl, &ov9640_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ priv->subdev.ctrl_handler = &priv->hdl;
+ if (priv->hdl.error) {
+ int err = priv->hdl.error;
+
+ kfree(priv);
+ return err;
+ }
+
+ icd->ops = &ov9640_ops;
ret = ov9640_video_probe(icd, client);
if (ret) {
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&priv->hdl);
kfree(priv);
}
@@ -792,6 +750,8 @@ static int ov9640_remove(struct i2c_client *client)
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct ov9640_priv *priv = to_ov9640_sensor(sd);
+ v4l2_device_unregister_subdev(&priv->subdev);
+ v4l2_ctrl_handler_free(&priv->hdl);
kfree(priv);
return 0;
}
diff --git a/drivers/media/video/ov9640.h b/drivers/media/video/ov9640.h
index f8a51b7..6b33a97 100644
--- a/drivers/media/video/ov9640.h
+++ b/drivers/media/video/ov9640.h
@@ -198,12 +198,10 @@ struct ov9640_reg {
struct ov9640_priv {
struct v4l2_subdev subdev;
+ struct v4l2_ctrl_handler hdl;
int model;
int revision;
-
- bool flag_vflip;
- bool flag_hflip;
};
#endif /* __DRIVERS_MEDIA_VIDEO_OV9640_H__ */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 05/12] ov9640: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 05/12] ov9640: " Hans Verkuil
@ 2011-01-11 23:51 ` Marek Vasut
0 siblings, 0 replies; 27+ messages in thread
From: Marek Vasut @ 2011-01-11 23:51 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Guennadi Liakhovetski, Magnus Damm,
Kuninori Morimoto, Alberto Panizzo, Janusz Krzysztofik,
Robert Jarzmik
On Wednesday 12 January 2011 00:06:05 Hans Verkuil wrote:
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Hey, I can do a test-run on this one eventually ;-)
Cheers
> ---
> drivers/media/video/ov9640.c | 124
> ++++++++++++++---------------------------- drivers/media/video/ov9640.h |
> 4 +-
> 2 files changed, 43 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
> index 53d88a2..dbda50c 100644
> --- a/drivers/media/video/ov9640.c
> +++ b/drivers/media/video/ov9640.c
> @@ -27,6 +27,7 @@
> #include <linux/videodev2.h>
> #include <media/v4l2-chip-ident.h>
> #include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> #include <media/soc_camera.h>
>
> #include "ov9640.h"
> @@ -162,27 +163,6 @@ static enum v4l2_mbus_pixelcode ov9640_codes[] = {
> V4L2_MBUS_FMT_RGB565_2X8_LE,
> };
>
> -static const struct v4l2_queryctrl ov9640_controls[] = {
> - {
> - .id = V4L2_CID_VFLIP,
> - .type = V4L2_CTRL_TYPE_BOOLEAN,
> - .name = "Flip Vertically",
> - .minimum = 0,
> - .maximum = 1,
> - .step = 1,
> - .default_value = 0,
> - },
> - {
> - .id = V4L2_CID_HFLIP,
> - .type = V4L2_CTRL_TYPE_BOOLEAN,
> - .name = "Flip Horizontally",
> - .minimum = 0,
> - .maximum = 1,
> - .step = 1,
> - .default_value = 0,
> - },
> -};
> -
> /* read a register */
> static int ov9640_reg_read(struct i2c_client *client, u8 reg, u8 *val)
> {
> @@ -307,52 +287,25 @@ static unsigned long ov9640_query_bus_param(struct
> soc_camera_device *icd) return soc_camera_apply_sensor_flags(icl, flags);
> }
>
> -/* Get status of additional camera capabilities */
> -static int ov9640_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control
> *ctrl) -{
> - struct ov9640_priv *priv = to_ov9640_sensor(sd);
> -
> - switch (ctrl->id) {
> - case V4L2_CID_VFLIP:
> - ctrl->value = priv->flag_vflip;
> - break;
> - case V4L2_CID_HFLIP:
> - ctrl->value = priv->flag_hflip;
> - break;
> - }
> - return 0;
> -}
> -
> /* Set status of additional camera capabilities */
> -static int ov9640_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control
> *ctrl) +static int ov9640_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(sd);
> - struct ov9640_priv *priv = to_ov9640_sensor(sd);
> -
> - int ret = 0;
> + struct ov9640_priv *priv = container_of(ctrl->handler, struct
> ov9640_priv, hdl); + struct i2c_client *client =
> v4l2_get_subdevdata(&priv->subdev);
>
> switch (ctrl->id) {
> case V4L2_CID_VFLIP:
> - priv->flag_vflip = ctrl->value;
> - if (ctrl->value)
> - ret = ov9640_reg_rmw(client, OV9640_MVFP,
> + if (ctrl->val)
> + return ov9640_reg_rmw(client, OV9640_MVFP,
> OV9640_MVFP_V, 0);
> - else
> - ret = ov9640_reg_rmw(client, OV9640_MVFP,
> - 0, OV9640_MVFP_V);
> - break;
> + return ov9640_reg_rmw(client, OV9640_MVFP, 0, OV9640_MVFP_V);
> case V4L2_CID_HFLIP:
> - priv->flag_hflip = ctrl->value;
> - if (ctrl->value)
> - ret = ov9640_reg_rmw(client, OV9640_MVFP,
> + if (ctrl->val)
> + return ov9640_reg_rmw(client, OV9640_MVFP,
> OV9640_MVFP_H, 0);
> - else
> - ret = ov9640_reg_rmw(client, OV9640_MVFP,
> - 0, OV9640_MVFP_H);
> - break;
> + return ov9640_reg_rmw(client, OV9640_MVFP, 0, OV9640_MVFP_H);
> }
> -
> - return ret;
> + return -EINVAL;
> }
>
> /* Get chip identification */
> @@ -664,8 +617,7 @@ static int ov9640_video_probe(struct soc_camera_device
> *icd, if (!icd->dev.parent ||
> to_soc_camera_host(icd->dev.parent)->nr != icd->iface) {
> dev_err(&client->dev, "Parent missing or invalid!\n");
> - ret = -ENODEV;
> - goto err;
> + return -ENODEV;
> }
>
> /*
> @@ -673,20 +625,14 @@ static int ov9640_video_probe(struct
> soc_camera_device *icd, */
>
> ret = ov9640_reg_read(client, OV9640_PID, &pid);
> + if (!ret)
> + ret = ov9640_reg_read(client, OV9640_VER, &ver);
> + if (!ret)
> + ret = ov9640_reg_read(client, OV9640_MIDH, &midh);
> + if (!ret)
> + ret = ov9640_reg_read(client, OV9640_MIDL, &midl);
> if (ret)
> - goto err;
> -
> - ret = ov9640_reg_read(client, OV9640_VER, &ver);
> - if (ret)
> - goto err;
> -
> - ret = ov9640_reg_read(client, OV9640_MIDH, &midh);
> - if (ret)
> - goto err;
> -
> - ret = ov9640_reg_read(client, OV9640_MIDL, &midl);
> - if (ret)
> - goto err;
> + return ret;
>
> switch (VERSION(pid, ver)) {
> case OV9640_V2:
> @@ -700,27 +646,25 @@ static int ov9640_video_probe(struct
> soc_camera_device *icd, break;
> default:
> dev_err(&client->dev, "Product ID error %x:%x\n", pid, ver);
> - ret = -ENODEV;
> - goto err;
> + return -ENODEV;
> }
>
> dev_info(&client->dev, "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
> devname, pid, ver, midh, midl);
>
> -err:
> - return ret;
> + return v4l2_ctrl_handler_setup(&priv->hdl);
> }
>
> +static const struct v4l2_ctrl_ops ov9640_ctrl_ops = {
> + .s_ctrl = ov9640_s_ctrl,
> +};
> +
> static struct soc_camera_ops ov9640_ops = {
> .set_bus_param = ov9640_set_bus_param,
> .query_bus_param = ov9640_query_bus_param,
> - .controls = ov9640_controls,
> - .num_controls = ARRAY_SIZE(ov9640_controls),
> };
>
> static struct v4l2_subdev_core_ops ov9640_core_ops = {
> - .g_ctrl = ov9640_g_ctrl,
> - .s_ctrl = ov9640_s_ctrl,
> .g_chip_ident = ov9640_g_chip_ident,
> #ifdef CONFIG_VIDEO_ADV_DEBUG
> .g_register = ov9640_get_register,
> @@ -775,12 +719,26 @@ static int ov9640_probe(struct i2c_client *client,
>
> v4l2_i2c_subdev_init(&priv->subdev, client, &ov9640_subdev_ops);
>
> - icd->ops = &ov9640_ops;
> + v4l2_ctrl_handler_init(&priv->hdl, 2);
> + v4l2_ctrl_new_std(&priv->hdl, &ov9640_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&priv->hdl, &ov9640_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + priv->subdev.ctrl_handler = &priv->hdl;
> + if (priv->hdl.error) {
> + int err = priv->hdl.error;
> +
> + kfree(priv);
> + return err;
> + }
> +
> + icd->ops = &ov9640_ops;
>
> ret = ov9640_video_probe(icd, client);
>
> if (ret) {
> icd->ops = NULL;
> + v4l2_ctrl_handler_free(&priv->hdl);
> kfree(priv);
> }
>
> @@ -792,6 +750,8 @@ static int ov9640_remove(struct i2c_client *client)
> struct v4l2_subdev *sd = i2c_get_clientdata(client);
> struct ov9640_priv *priv = to_ov9640_sensor(sd);
>
> + v4l2_device_unregister_subdev(&priv->subdev);
> + v4l2_ctrl_handler_free(&priv->hdl);
> kfree(priv);
> return 0;
> }
> diff --git a/drivers/media/video/ov9640.h b/drivers/media/video/ov9640.h
> index f8a51b7..6b33a97 100644
> --- a/drivers/media/video/ov9640.h
> +++ b/drivers/media/video/ov9640.h
> @@ -198,12 +198,10 @@ struct ov9640_reg {
>
> struct ov9640_priv {
> struct v4l2_subdev subdev;
> + struct v4l2_ctrl_handler hdl;
>
> int model;
> int revision;
> -
> - bool flag_vflip;
> - bool flag_hflip;
> };
>
> #endif /* __DRIVERS_MEDIA_VIDEO_OV9640_H__ */
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH 06/12] mt9t031: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (3 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 05/12] ov9640: " Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-23 0:00 ` Guennadi Liakhovetski
2011-01-11 23:06 ` [RFC PATCH 07/12] mt9v022: " Hans Verkuil
` (6 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/mt9t031.c | 229 +++++++++++++++--------------------------
1 files changed, 81 insertions(+), 148 deletions(-)
diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index 7ce279c..cd73ef1 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -18,6 +18,7 @@
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
#include <media/v4l2-subdev.h>
+#include <media/v4l2-ctrls.h>
/*
* mt9t031 i2c address 0x5d
@@ -64,14 +65,17 @@
struct mt9t031 {
struct v4l2_subdev subdev;
+ struct v4l2_ctrl_handler hdl;
+ struct {
+ /* exposure/auto-exposure cluster */
+ struct v4l2_ctrl *autoexposure;
+ struct v4l2_ctrl *exposure;
+ };
struct v4l2_rect rect; /* Sensor window */
int model; /* V4L2_IDENT_MT9T031* codes from v4l2-chip-ident.h */
u16 xskip;
u16 yskip;
- unsigned int gain;
unsigned short y_skip_top; /* Lines to skip at the top */
- unsigned int exposure;
- unsigned char autoexposure;
};
static struct mt9t031 *to_mt9t031(const struct i2c_client *client)
@@ -211,61 +215,9 @@ enum {
MT9T031_CTRL_EXPOSURE_AUTO,
};
-static const struct v4l2_queryctrl mt9t031_controls[] = {
- [MT9T031_CTRL_VFLIP] = {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Vertically",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
- [MT9T031_CTRL_HFLIP] = {
- .id = V4L2_CID_HFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Horizontally",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
- [MT9T031_CTRL_GAIN] = {
- .id = V4L2_CID_GAIN,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Gain",
- .minimum = 0,
- .maximum = 127,
- .step = 1,
- .default_value = 64,
- .flags = V4L2_CTRL_FLAG_SLIDER,
- },
- [MT9T031_CTRL_EXPOSURE] = {
- .id = V4L2_CID_EXPOSURE,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Exposure",
- .minimum = 1,
- .maximum = 255,
- .step = 1,
- .default_value = 255,
- .flags = V4L2_CTRL_FLAG_SLIDER,
- },
- [MT9T031_CTRL_EXPOSURE_AUTO] = {
- .id = V4L2_CID_EXPOSURE_AUTO,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Automatic Exposure",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 1,
- }
-};
-
static struct soc_camera_ops mt9t031_ops = {
.set_bus_param = mt9t031_set_bus_param,
.query_bus_param = mt9t031_query_bus_param,
- .controls = mt9t031_controls,
- .num_controls = ARRAY_SIZE(mt9t031_controls),
};
/* target must be _even_ */
@@ -364,18 +316,20 @@ static int mt9t031_set_params(struct i2c_client *client,
if (ret >= 0)
ret = reg_write(client, MT9T031_WINDOW_HEIGHT,
rect->height + mt9t031->y_skip_top - 1);
- if (ret >= 0 && mt9t031->autoexposure) {
+ v4l2_ctrl_lock(mt9t031->autoexposure);
+ if (ret >= 0 && mt9t031->autoexposure->cur.val == V4L2_EXPOSURE_AUTO) {
unsigned int total_h = rect->height + mt9t031->y_skip_top + vblank;
ret = set_shutter(client, total_h);
if (ret >= 0) {
const u32 shutter_max = MT9T031_MAX_HEIGHT + vblank;
- const struct v4l2_queryctrl *qctrl =
- &mt9t031_controls[MT9T031_CTRL_EXPOSURE];
- mt9t031->exposure = (shutter_max / 2 + (total_h - 1) *
- (qctrl->maximum - qctrl->minimum)) /
- shutter_max + qctrl->minimum;
+ struct v4l2_ctrl *ctrl = mt9t031->exposure;
+
+ ctrl->cur.val = (shutter_max / 2 + (total_h - 1) *
+ (ctrl->maximum - ctrl->minimum)) /
+ shutter_max + ctrl->minimum;
}
}
+ v4l2_ctrl_unlock(mt9t031->autoexposure);
/* Re-enable register update, commit all changes */
if (ret >= 0)
@@ -543,71 +497,38 @@ static int mt9t031_s_register(struct v4l2_subdev *sd,
}
#endif
-static int mt9t031_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
-{
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct mt9t031 *mt9t031 = to_mt9t031(client);
- int data;
-
- switch (ctrl->id) {
- case V4L2_CID_VFLIP:
- data = reg_read(client, MT9T031_READ_MODE_2);
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & 0x8000);
- break;
- case V4L2_CID_HFLIP:
- data = reg_read(client, MT9T031_READ_MODE_2);
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & 0x4000);
- break;
- case V4L2_CID_EXPOSURE_AUTO:
- ctrl->value = mt9t031->autoexposure;
- break;
- case V4L2_CID_GAIN:
- ctrl->value = mt9t031->gain;
- break;
- case V4L2_CID_EXPOSURE:
- ctrl->value = mt9t031->exposure;
- break;
- }
- return 0;
-}
-
-static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int mt9t031_s_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct mt9t031, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct mt9t031 *mt9t031 = to_mt9t031(client);
- const struct v4l2_queryctrl *qctrl;
+ struct v4l2_ctrl *exp = mt9t031->exposure;
int data;
switch (ctrl->id) {
case V4L2_CID_VFLIP:
- if (ctrl->value)
+ if (ctrl->val)
data = reg_set(client, MT9T031_READ_MODE_2, 0x8000);
else
data = reg_clear(client, MT9T031_READ_MODE_2, 0x8000);
if (data < 0)
return -EIO;
- break;
+ return 0;
case V4L2_CID_HFLIP:
- if (ctrl->value)
+ if (ctrl->val)
data = reg_set(client, MT9T031_READ_MODE_2, 0x4000);
else
data = reg_clear(client, MT9T031_READ_MODE_2, 0x4000);
if (data < 0)
return -EIO;
- break;
+ return 0;
case V4L2_CID_GAIN:
- qctrl = &mt9t031_controls[MT9T031_CTRL_GAIN];
- if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
- return -EINVAL;
/* See Datasheet Table 7, Gain settings. */
- if (ctrl->value <= qctrl->default_value) {
+ if (ctrl->val <= ctrl->default_value) {
/* Pack it into 0..1 step 0.125, register values 0..8 */
- unsigned long range = qctrl->default_value - qctrl->minimum;
- data = ((ctrl->value - qctrl->minimum) * 8 + range / 2) / range;
+ unsigned long range = ctrl->default_value - ctrl->minimum;
+ data = ((ctrl->val - ctrl->minimum) * 8 + range / 2) / range;
dev_dbg(&client->dev, "Setting gain %d\n", data);
data = reg_write(client, MT9T031_GLOBAL_GAIN, data);
@@ -616,9 +537,9 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
} else {
/* Pack it into 1.125..128 variable step, register values 9..0x7860 */
/* We assume qctrl->maximum - qctrl->default_value - 1 > 0 */
- unsigned long range = qctrl->maximum - qctrl->default_value - 1;
+ unsigned long range = ctrl->maximum - ctrl->default_value - 1;
/* calculated gain: map 65..127 to 9..1024 step 0.125 */
- unsigned long gain = ((ctrl->value - qctrl->default_value - 1) *
+ unsigned long gain = ((ctrl->val - ctrl->default_value - 1) *
1015 + range / 2) / range + 9;
if (gain <= 32) /* calculated gain 9..32 -> 9..32 */
@@ -635,19 +556,16 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
if (data < 0)
return -EIO;
}
+ return 0;
- /* Success */
- mt9t031->gain = ctrl->value;
- break;
- case V4L2_CID_EXPOSURE:
- qctrl = &mt9t031_controls[MT9T031_CTRL_EXPOSURE];
- /* mt9t031 has maximum == default */
- if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
- return -EINVAL;
- else {
- const unsigned long range = qctrl->maximum - qctrl->minimum;
- const u32 shutter = ((ctrl->value - qctrl->minimum) * 1048 +
- range / 2) / range + 1;
+ case V4L2_CID_EXPOSURE_AUTO:
+ /* Force manual exposure if only the exposure was changed */
+ if (!ctrl->has_new)
+ ctrl->val = V4L2_EXPOSURE_MANUAL;
+ if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
+ unsigned int range = exp->maximum - exp->minimum;
+ unsigned int shutter = ((exp->val - exp->minimum) * 1048 +
+ range / 2) / range + 1;
u32 old;
get_shutter(client, &old);
@@ -655,12 +573,8 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
old, shutter);
if (set_shutter(client, shutter) < 0)
return -EIO;
- mt9t031->exposure = ctrl->value;
- mt9t031->autoexposure = 0;
- }
- break;
- case V4L2_CID_EXPOSURE_AUTO:
- if (ctrl->value) {
+ ctrl->val = V4L2_EXPOSURE_MANUAL;
+ } else {
const u16 vblank = MT9T031_VERTICAL_BLANK;
const u32 shutter_max = MT9T031_MAX_HEIGHT + vblank;
unsigned int total_h = mt9t031->rect.height +
@@ -668,14 +582,11 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
if (set_shutter(client, total_h) < 0)
return -EIO;
- qctrl = &mt9t031_controls[MT9T031_CTRL_EXPOSURE];
- mt9t031->exposure = (shutter_max / 2 + (total_h - 1) *
- (qctrl->maximum - qctrl->minimum)) /
- shutter_max + qctrl->minimum;
- mt9t031->autoexposure = 1;
- } else
- mt9t031->autoexposure = 0;
- break;
+ exp->val = (shutter_max / 2 + (total_h - 1) *
+ (exp->maximum - exp->minimum)) /
+ shutter_max + exp->minimum;
+ }
+ return 0;
default:
return -EINVAL;
}
@@ -766,15 +677,12 @@ static int mt9t031_video_probe(struct i2c_client *client)
dev_info(&client->dev, "Detected a MT9T031 chip ID %x\n", data);
ret = mt9t031_idle(client);
- if (ret < 0)
+ if (ret < 0) {
dev_err(&client->dev, "Failed to initialise the camera\n");
- else
+ } else {
vdev->dev.type = &mt9t031_dev_type;
-
- /* mt9t031_idle() has reset the chip to default. */
- mt9t031->exposure = 255;
- mt9t031->gain = 64;
-
+ v4l2_ctrl_handler_setup(&mt9t031->hdl);
+ }
return ret;
}
@@ -788,9 +696,11 @@ static int mt9t031_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
return 0;
}
+static const struct v4l2_ctrl_ops mt9t031_ctrl_ops = {
+ .s_ctrl = mt9t031_s_ctrl,
+};
+
static struct v4l2_subdev_core_ops mt9t031_subdev_core_ops = {
- .g_ctrl = mt9t031_g_ctrl,
- .s_ctrl = mt9t031_s_ctrl,
.g_chip_ident = mt9t031_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = mt9t031_g_register,
@@ -858,6 +768,32 @@ static int mt9t031_probe(struct i2c_client *client,
return -ENOMEM;
v4l2_i2c_subdev_init(&mt9t031->subdev, client, &mt9t031_subdev_ops);
+ v4l2_ctrl_handler_init(&mt9t031->hdl, 5);
+ v4l2_ctrl_new_std(&mt9t031->hdl, &mt9t031_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&mt9t031->hdl, &mt9t031_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&mt9t031->hdl, &mt9t031_ctrl_ops,
+ V4L2_CID_GAIN, 0, 127, 1, 64);
+
+ /*
+ * Simulated autoexposure. If enabled, we calculate shutter width
+ * ourselves in the driver based on vertical blanking and frame width
+ */
+ mt9t031->autoexposure = v4l2_ctrl_new_std_menu(&mt9t031->hdl,
+ &mt9t031_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
+ V4L2_EXPOSURE_AUTO);
+ mt9t031->exposure = v4l2_ctrl_new_std(&mt9t031->hdl, &mt9t031_ctrl_ops,
+ V4L2_CID_EXPOSURE, 1, 255, 1, 255);
+
+ mt9t031->subdev.ctrl_handler = &mt9t031->hdl;
+ if (mt9t031->hdl.error) {
+ int err = mt9t031->hdl.error;
+
+ kfree(mt9t031);
+ return err;
+ }
+ v4l2_ctrl_cluster(2, &mt9t031->autoexposure);
mt9t031->y_skip_top = 0;
mt9t031->rect.left = MT9T031_COLUMN_SKIP;
@@ -865,12 +801,6 @@ static int mt9t031_probe(struct i2c_client *client,
mt9t031->rect.width = MT9T031_MAX_WIDTH;
mt9t031->rect.height = MT9T031_MAX_HEIGHT;
- /*
- * Simulated autoexposure. If enabled, we calculate shutter width
- * ourselves in the driver based on vertical blanking and frame width
- */
- mt9t031->autoexposure = 1;
-
mt9t031->xskip = 1;
mt9t031->yskip = 1;
@@ -883,6 +813,7 @@ static int mt9t031_probe(struct i2c_client *client,
if (ret) {
if (icd)
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&mt9t031->hdl);
kfree(mt9t031);
}
@@ -894,8 +825,10 @@ static int mt9t031_remove(struct i2c_client *client)
struct mt9t031 *mt9t031 = to_mt9t031(client);
struct soc_camera_device *icd = client->dev.platform_data;
+ v4l2_device_unregister_subdev(&mt9t031->subdev);
if (icd)
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&mt9t031->hdl);
kfree(mt9t031);
return 0;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 06/12] mt9t031: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 06/12] mt9t031: " Hans Verkuil
@ 2011-01-23 0:00 ` Guennadi Liakhovetski
0 siblings, 0 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2011-01-23 0:00 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Same questions as to the previous ones:
1. how is V4L2_CID_EXPOSURE taken care of? I see, that the functionality
is provided by the control cluster, but, AFAIU, user just issuing that
control will not get the desired result?
2. separate handlers
3. v4l2_device_unregister_subdev()
Thanks
Guennadi
On Wed, 12 Jan 2011, Hans Verkuil wrote:
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
> drivers/media/video/mt9t031.c | 229 +++++++++++++++--------------------------
> 1 files changed, 81 insertions(+), 148 deletions(-)
>
> diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
> index 7ce279c..cd73ef1 100644
> --- a/drivers/media/video/mt9t031.c
> +++ b/drivers/media/video/mt9t031.c
> @@ -18,6 +18,7 @@
> #include <media/soc_camera.h>
> #include <media/v4l2-chip-ident.h>
> #include <media/v4l2-subdev.h>
> +#include <media/v4l2-ctrls.h>
>
> /*
> * mt9t031 i2c address 0x5d
> @@ -64,14 +65,17 @@
>
> struct mt9t031 {
> struct v4l2_subdev subdev;
> + struct v4l2_ctrl_handler hdl;
> + struct {
> + /* exposure/auto-exposure cluster */
> + struct v4l2_ctrl *autoexposure;
> + struct v4l2_ctrl *exposure;
> + };
> struct v4l2_rect rect; /* Sensor window */
> int model; /* V4L2_IDENT_MT9T031* codes from v4l2-chip-ident.h */
> u16 xskip;
> u16 yskip;
> - unsigned int gain;
> unsigned short y_skip_top; /* Lines to skip at the top */
> - unsigned int exposure;
> - unsigned char autoexposure;
> };
>
> static struct mt9t031 *to_mt9t031(const struct i2c_client *client)
> @@ -211,61 +215,9 @@ enum {
> MT9T031_CTRL_EXPOSURE_AUTO,
> };
>
> -static const struct v4l2_queryctrl mt9t031_controls[] = {
> - [MT9T031_CTRL_VFLIP] = {
> - .id = V4L2_CID_VFLIP,
> - .type = V4L2_CTRL_TYPE_BOOLEAN,
> - .name = "Flip Vertically",
> - .minimum = 0,
> - .maximum = 1,
> - .step = 1,
> - .default_value = 0,
> - },
> - [MT9T031_CTRL_HFLIP] = {
> - .id = V4L2_CID_HFLIP,
> - .type = V4L2_CTRL_TYPE_BOOLEAN,
> - .name = "Flip Horizontally",
> - .minimum = 0,
> - .maximum = 1,
> - .step = 1,
> - .default_value = 0,
> - },
> - [MT9T031_CTRL_GAIN] = {
> - .id = V4L2_CID_GAIN,
> - .type = V4L2_CTRL_TYPE_INTEGER,
> - .name = "Gain",
> - .minimum = 0,
> - .maximum = 127,
> - .step = 1,
> - .default_value = 64,
> - .flags = V4L2_CTRL_FLAG_SLIDER,
> - },
> - [MT9T031_CTRL_EXPOSURE] = {
> - .id = V4L2_CID_EXPOSURE,
> - .type = V4L2_CTRL_TYPE_INTEGER,
> - .name = "Exposure",
> - .minimum = 1,
> - .maximum = 255,
> - .step = 1,
> - .default_value = 255,
> - .flags = V4L2_CTRL_FLAG_SLIDER,
> - },
> - [MT9T031_CTRL_EXPOSURE_AUTO] = {
> - .id = V4L2_CID_EXPOSURE_AUTO,
> - .type = V4L2_CTRL_TYPE_BOOLEAN,
> - .name = "Automatic Exposure",
> - .minimum = 0,
> - .maximum = 1,
> - .step = 1,
> - .default_value = 1,
> - }
> -};
> -
> static struct soc_camera_ops mt9t031_ops = {
> .set_bus_param = mt9t031_set_bus_param,
> .query_bus_param = mt9t031_query_bus_param,
> - .controls = mt9t031_controls,
> - .num_controls = ARRAY_SIZE(mt9t031_controls),
> };
>
> /* target must be _even_ */
> @@ -364,18 +316,20 @@ static int mt9t031_set_params(struct i2c_client *client,
> if (ret >= 0)
> ret = reg_write(client, MT9T031_WINDOW_HEIGHT,
> rect->height + mt9t031->y_skip_top - 1);
> - if (ret >= 0 && mt9t031->autoexposure) {
> + v4l2_ctrl_lock(mt9t031->autoexposure);
> + if (ret >= 0 && mt9t031->autoexposure->cur.val == V4L2_EXPOSURE_AUTO) {
> unsigned int total_h = rect->height + mt9t031->y_skip_top + vblank;
> ret = set_shutter(client, total_h);
> if (ret >= 0) {
> const u32 shutter_max = MT9T031_MAX_HEIGHT + vblank;
> - const struct v4l2_queryctrl *qctrl =
> - &mt9t031_controls[MT9T031_CTRL_EXPOSURE];
> - mt9t031->exposure = (shutter_max / 2 + (total_h - 1) *
> - (qctrl->maximum - qctrl->minimum)) /
> - shutter_max + qctrl->minimum;
> + struct v4l2_ctrl *ctrl = mt9t031->exposure;
> +
> + ctrl->cur.val = (shutter_max / 2 + (total_h - 1) *
> + (ctrl->maximum - ctrl->minimum)) /
> + shutter_max + ctrl->minimum;
> }
> }
> + v4l2_ctrl_unlock(mt9t031->autoexposure);
>
> /* Re-enable register update, commit all changes */
> if (ret >= 0)
> @@ -543,71 +497,38 @@ static int mt9t031_s_register(struct v4l2_subdev *sd,
> }
> #endif
>
> -static int mt9t031_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> -{
> - struct i2c_client *client = v4l2_get_subdevdata(sd);
> - struct mt9t031 *mt9t031 = to_mt9t031(client);
> - int data;
> -
> - switch (ctrl->id) {
> - case V4L2_CID_VFLIP:
> - data = reg_read(client, MT9T031_READ_MODE_2);
> - if (data < 0)
> - return -EIO;
> - ctrl->value = !!(data & 0x8000);
> - break;
> - case V4L2_CID_HFLIP:
> - data = reg_read(client, MT9T031_READ_MODE_2);
> - if (data < 0)
> - return -EIO;
> - ctrl->value = !!(data & 0x4000);
> - break;
> - case V4L2_CID_EXPOSURE_AUTO:
> - ctrl->value = mt9t031->autoexposure;
> - break;
> - case V4L2_CID_GAIN:
> - ctrl->value = mt9t031->gain;
> - break;
> - case V4L2_CID_EXPOSURE:
> - ctrl->value = mt9t031->exposure;
> - break;
> - }
> - return 0;
> -}
> -
> -static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> +static int mt9t031_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> + struct v4l2_subdev *sd =
> + &container_of(ctrl->handler, struct mt9t031, hdl)->subdev;
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> struct mt9t031 *mt9t031 = to_mt9t031(client);
> - const struct v4l2_queryctrl *qctrl;
> + struct v4l2_ctrl *exp = mt9t031->exposure;
> int data;
>
> switch (ctrl->id) {
> case V4L2_CID_VFLIP:
> - if (ctrl->value)
> + if (ctrl->val)
> data = reg_set(client, MT9T031_READ_MODE_2, 0x8000);
> else
> data = reg_clear(client, MT9T031_READ_MODE_2, 0x8000);
> if (data < 0)
> return -EIO;
> - break;
> + return 0;
> case V4L2_CID_HFLIP:
> - if (ctrl->value)
> + if (ctrl->val)
> data = reg_set(client, MT9T031_READ_MODE_2, 0x4000);
> else
> data = reg_clear(client, MT9T031_READ_MODE_2, 0x4000);
> if (data < 0)
> return -EIO;
> - break;
> + return 0;
> case V4L2_CID_GAIN:
> - qctrl = &mt9t031_controls[MT9T031_CTRL_GAIN];
> - if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
> - return -EINVAL;
> /* See Datasheet Table 7, Gain settings. */
> - if (ctrl->value <= qctrl->default_value) {
> + if (ctrl->val <= ctrl->default_value) {
> /* Pack it into 0..1 step 0.125, register values 0..8 */
> - unsigned long range = qctrl->default_value - qctrl->minimum;
> - data = ((ctrl->value - qctrl->minimum) * 8 + range / 2) / range;
> + unsigned long range = ctrl->default_value - ctrl->minimum;
> + data = ((ctrl->val - ctrl->minimum) * 8 + range / 2) / range;
>
> dev_dbg(&client->dev, "Setting gain %d\n", data);
> data = reg_write(client, MT9T031_GLOBAL_GAIN, data);
> @@ -616,9 +537,9 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> } else {
> /* Pack it into 1.125..128 variable step, register values 9..0x7860 */
> /* We assume qctrl->maximum - qctrl->default_value - 1 > 0 */
> - unsigned long range = qctrl->maximum - qctrl->default_value - 1;
> + unsigned long range = ctrl->maximum - ctrl->default_value - 1;
> /* calculated gain: map 65..127 to 9..1024 step 0.125 */
> - unsigned long gain = ((ctrl->value - qctrl->default_value - 1) *
> + unsigned long gain = ((ctrl->val - ctrl->default_value - 1) *
> 1015 + range / 2) / range + 9;
>
> if (gain <= 32) /* calculated gain 9..32 -> 9..32 */
> @@ -635,19 +556,16 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> if (data < 0)
> return -EIO;
> }
> + return 0;
>
> - /* Success */
> - mt9t031->gain = ctrl->value;
> - break;
> - case V4L2_CID_EXPOSURE:
> - qctrl = &mt9t031_controls[MT9T031_CTRL_EXPOSURE];
> - /* mt9t031 has maximum == default */
> - if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
> - return -EINVAL;
> - else {
> - const unsigned long range = qctrl->maximum - qctrl->minimum;
> - const u32 shutter = ((ctrl->value - qctrl->minimum) * 1048 +
> - range / 2) / range + 1;
> + case V4L2_CID_EXPOSURE_AUTO:
> + /* Force manual exposure if only the exposure was changed */
> + if (!ctrl->has_new)
> + ctrl->val = V4L2_EXPOSURE_MANUAL;
> + if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
> + unsigned int range = exp->maximum - exp->minimum;
> + unsigned int shutter = ((exp->val - exp->minimum) * 1048 +
> + range / 2) / range + 1;
> u32 old;
>
> get_shutter(client, &old);
> @@ -655,12 +573,8 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> old, shutter);
> if (set_shutter(client, shutter) < 0)
> return -EIO;
> - mt9t031->exposure = ctrl->value;
> - mt9t031->autoexposure = 0;
> - }
> - break;
> - case V4L2_CID_EXPOSURE_AUTO:
> - if (ctrl->value) {
> + ctrl->val = V4L2_EXPOSURE_MANUAL;
> + } else {
> const u16 vblank = MT9T031_VERTICAL_BLANK;
> const u32 shutter_max = MT9T031_MAX_HEIGHT + vblank;
> unsigned int total_h = mt9t031->rect.height +
> @@ -668,14 +582,11 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
>
> if (set_shutter(client, total_h) < 0)
> return -EIO;
> - qctrl = &mt9t031_controls[MT9T031_CTRL_EXPOSURE];
> - mt9t031->exposure = (shutter_max / 2 + (total_h - 1) *
> - (qctrl->maximum - qctrl->minimum)) /
> - shutter_max + qctrl->minimum;
> - mt9t031->autoexposure = 1;
> - } else
> - mt9t031->autoexposure = 0;
> - break;
> + exp->val = (shutter_max / 2 + (total_h - 1) *
> + (exp->maximum - exp->minimum)) /
> + shutter_max + exp->minimum;
> + }
> + return 0;
> default:
> return -EINVAL;
> }
> @@ -766,15 +677,12 @@ static int mt9t031_video_probe(struct i2c_client *client)
> dev_info(&client->dev, "Detected a MT9T031 chip ID %x\n", data);
>
> ret = mt9t031_idle(client);
> - if (ret < 0)
> + if (ret < 0) {
> dev_err(&client->dev, "Failed to initialise the camera\n");
> - else
> + } else {
> vdev->dev.type = &mt9t031_dev_type;
> -
> - /* mt9t031_idle() has reset the chip to default. */
> - mt9t031->exposure = 255;
> - mt9t031->gain = 64;
> -
> + v4l2_ctrl_handler_setup(&mt9t031->hdl);
> + }
> return ret;
> }
>
> @@ -788,9 +696,11 @@ static int mt9t031_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
> return 0;
> }
>
> +static const struct v4l2_ctrl_ops mt9t031_ctrl_ops = {
> + .s_ctrl = mt9t031_s_ctrl,
> +};
> +
> static struct v4l2_subdev_core_ops mt9t031_subdev_core_ops = {
> - .g_ctrl = mt9t031_g_ctrl,
> - .s_ctrl = mt9t031_s_ctrl,
> .g_chip_ident = mt9t031_g_chip_ident,
> #ifdef CONFIG_VIDEO_ADV_DEBUG
> .g_register = mt9t031_g_register,
> @@ -858,6 +768,32 @@ static int mt9t031_probe(struct i2c_client *client,
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&mt9t031->subdev, client, &mt9t031_subdev_ops);
> + v4l2_ctrl_handler_init(&mt9t031->hdl, 5);
> + v4l2_ctrl_new_std(&mt9t031->hdl, &mt9t031_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&mt9t031->hdl, &mt9t031_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&mt9t031->hdl, &mt9t031_ctrl_ops,
> + V4L2_CID_GAIN, 0, 127, 1, 64);
> +
> + /*
> + * Simulated autoexposure. If enabled, we calculate shutter width
> + * ourselves in the driver based on vertical blanking and frame width
> + */
> + mt9t031->autoexposure = v4l2_ctrl_new_std_menu(&mt9t031->hdl,
> + &mt9t031_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
> + V4L2_EXPOSURE_AUTO);
> + mt9t031->exposure = v4l2_ctrl_new_std(&mt9t031->hdl, &mt9t031_ctrl_ops,
> + V4L2_CID_EXPOSURE, 1, 255, 1, 255);
> +
> + mt9t031->subdev.ctrl_handler = &mt9t031->hdl;
> + if (mt9t031->hdl.error) {
> + int err = mt9t031->hdl.error;
> +
> + kfree(mt9t031);
> + return err;
> + }
> + v4l2_ctrl_cluster(2, &mt9t031->autoexposure);
>
> mt9t031->y_skip_top = 0;
> mt9t031->rect.left = MT9T031_COLUMN_SKIP;
> @@ -865,12 +801,6 @@ static int mt9t031_probe(struct i2c_client *client,
> mt9t031->rect.width = MT9T031_MAX_WIDTH;
> mt9t031->rect.height = MT9T031_MAX_HEIGHT;
>
> - /*
> - * Simulated autoexposure. If enabled, we calculate shutter width
> - * ourselves in the driver based on vertical blanking and frame width
> - */
> - mt9t031->autoexposure = 1;
> -
> mt9t031->xskip = 1;
> mt9t031->yskip = 1;
>
> @@ -883,6 +813,7 @@ static int mt9t031_probe(struct i2c_client *client,
> if (ret) {
> if (icd)
> icd->ops = NULL;
> + v4l2_ctrl_handler_free(&mt9t031->hdl);
> kfree(mt9t031);
> }
>
> @@ -894,8 +825,10 @@ static int mt9t031_remove(struct i2c_client *client)
> struct mt9t031 *mt9t031 = to_mt9t031(client);
> struct soc_camera_device *icd = client->dev.platform_data;
>
> + v4l2_device_unregister_subdev(&mt9t031->subdev);
> if (icd)
> icd->ops = NULL;
> + v4l2_ctrl_handler_free(&mt9t031->hdl);
> kfree(mt9t031);
>
> return 0;
> --
> 1.7.0.4
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH 07/12] mt9v022: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (4 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 06/12] mt9t031: " Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-11 23:06 ` [RFC PATCH 08/12] ov772x: " Hans Verkuil
` (5 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/mt9v022.c | 271 ++++++++++++++++++-----------------------
1 files changed, 119 insertions(+), 152 deletions(-)
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index 6a784c8..49731f9 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -16,6 +16,7 @@
#include <media/v4l2-subdev.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ctrls.h>
#include <media/soc_camera.h>
/*
@@ -100,6 +101,17 @@ static const struct mt9v022_datafmt mt9v022_monochrome_fmts[] = {
struct mt9v022 {
struct v4l2_subdev subdev;
+ struct v4l2_ctrl_handler hdl;
+ struct {
+ /* exposure/auto-exposure cluster */
+ struct v4l2_ctrl *autoexposure;
+ struct v4l2_ctrl *exposure;
+ };
+ struct {
+ /* gain/auto-gain cluster */
+ struct v4l2_ctrl *autogain;
+ struct v4l2_ctrl *gain;
+ };
struct v4l2_rect rect; /* Sensor window */
const struct mt9v022_datafmt *fmt;
const struct mt9v022_datafmt *fmts;
@@ -178,6 +190,8 @@ static int mt9v022_init(struct i2c_client *client)
ret = reg_clear(client, MT9V022_BLACK_LEVEL_CALIB_CTRL, 1);
if (!ret)
ret = reg_write(client, MT9V022_DIGITAL_TEST_PATTERN, 0);
+ if (!ret)
+ return v4l2_ctrl_handler_setup(&mt9v022->hdl);
return ret;
}
@@ -502,217 +516,133 @@ static int mt9v022_s_register(struct v4l2_subdev *sd,
}
#endif
-static const struct v4l2_queryctrl mt9v022_controls[] = {
- {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Vertically",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- }, {
- .id = V4L2_CID_HFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Horizontally",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- }, {
- .id = V4L2_CID_GAIN,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Analog Gain",
- .minimum = 64,
- .maximum = 127,
- .step = 1,
- .default_value = 64,
- .flags = V4L2_CTRL_FLAG_SLIDER,
- }, {
- .id = V4L2_CID_EXPOSURE,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Exposure",
- .minimum = 1,
- .maximum = 255,
- .step = 1,
- .default_value = 255,
- .flags = V4L2_CTRL_FLAG_SLIDER,
- }, {
- .id = V4L2_CID_AUTOGAIN,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Automatic Gain",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 1,
- }, {
- .id = V4L2_CID_EXPOSURE_AUTO,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Automatic Exposure",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 1,
- }
-};
-
static struct soc_camera_ops mt9v022_ops = {
.set_bus_param = mt9v022_set_bus_param,
.query_bus_param = mt9v022_query_bus_param,
- .controls = mt9v022_controls,
- .num_controls = ARRAY_SIZE(mt9v022_controls),
};
-static int mt9v022_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct mt9v022, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
- const struct v4l2_queryctrl *qctrl;
+ struct mt9v022 *mt9v022 = to_mt9v022(client);
+ struct v4l2_ctrl *gain = mt9v022->gain;
+ struct v4l2_ctrl *exp = mt9v022->exposure;
unsigned long range;
int data;
- qctrl = soc_camera_find_qctrl(&mt9v022_ops, ctrl->id);
-
switch (ctrl->id) {
- case V4L2_CID_VFLIP:
- data = reg_read(client, MT9V022_READ_MODE);
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & 0x10);
- break;
- case V4L2_CID_HFLIP:
- data = reg_read(client, MT9V022_READ_MODE);
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & 0x20);
- break;
- case V4L2_CID_EXPOSURE_AUTO:
- data = reg_read(client, MT9V022_AEC_AGC_ENABLE);
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & 0x1);
- break;
case V4L2_CID_AUTOGAIN:
- data = reg_read(client, MT9V022_AEC_AGC_ENABLE);
- if (data < 0)
- return -EIO;
- ctrl->value = !!(data & 0x2);
- break;
- case V4L2_CID_GAIN:
+ if (!ctrl->cur.val)
+ return 0;
data = reg_read(client, MT9V022_ANALOG_GAIN);
if (data < 0)
return -EIO;
- range = qctrl->maximum - qctrl->minimum;
- ctrl->value = ((data - 16) * range + 24) / 48 + qctrl->minimum;
+ range = gain->maximum - gain->minimum;
+ gain->cur.val = ((data - 16) * range + 24) / 48 + gain->minimum;
+ return 0;
- break;
- case V4L2_CID_EXPOSURE:
+ case V4L2_CID_EXPOSURE_AUTO:
+ if (ctrl->cur.val != V4L2_CID_EXPOSURE_AUTO)
+ return 0;
data = reg_read(client, MT9V022_TOTAL_SHUTTER_WIDTH);
if (data < 0)
return -EIO;
- range = qctrl->maximum - qctrl->minimum;
- ctrl->value = ((data - 1) * range + 239) / 479 + qctrl->minimum;
-
- break;
+ range = exp->maximum - exp->minimum;
+ exp->cur.val = ((data - 1) * range + 239) / 479 + exp->minimum;
+ return 0;
}
- return 0;
+ return -EINVAL;
}
-static int mt9v022_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int mt9v022_s_ctrl(struct v4l2_ctrl *ctrl)
{
- int data;
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct mt9v022, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
- const struct v4l2_queryctrl *qctrl;
-
- qctrl = soc_camera_find_qctrl(&mt9v022_ops, ctrl->id);
- if (!qctrl)
- return -EINVAL;
+ struct mt9v022 *mt9v022 = to_mt9v022(client);
+ int data;
switch (ctrl->id) {
case V4L2_CID_VFLIP:
- if (ctrl->value)
+ if (ctrl->val)
data = reg_set(client, MT9V022_READ_MODE, 0x10);
else
data = reg_clear(client, MT9V022_READ_MODE, 0x10);
if (data < 0)
return -EIO;
- break;
+ return 0;
case V4L2_CID_HFLIP:
- if (ctrl->value)
+ if (ctrl->val)
data = reg_set(client, MT9V022_READ_MODE, 0x20);
else
data = reg_clear(client, MT9V022_READ_MODE, 0x20);
if (data < 0)
return -EIO;
- break;
- case V4L2_CID_GAIN:
- /* mt9v022 has minimum == default */
- if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
- return -EINVAL;
- else {
- unsigned long range = qctrl->maximum - qctrl->minimum;
+ return 0;
+ case V4L2_CID_AUTOGAIN:
+ /* Force manual gain if only the gain was changed */
+ if (!ctrl->has_new)
+ ctrl->val = 0;
+ if (ctrl->val) {
+ if (reg_set(client, MT9V022_AEC_AGC_ENABLE, 0x2) < 0)
+ return -EIO;
+ } else {
+ struct v4l2_ctrl *gain = mt9v022->gain;
+ /* mt9v022 has minimum == default */
+ unsigned long range = gain->maximum - gain->minimum;
/* Valid values 16 to 64, 32 to 64 must be even. */
- unsigned long gain = ((ctrl->value - qctrl->minimum) *
+ unsigned long gain_val = ((gain->val - gain->minimum) *
48 + range / 2) / range + 16;
- if (gain >= 32)
- gain &= ~1;
+
+ if (gain_val >= 32)
+ gain_val &= ~1;
+
/*
* The user wants to set gain manually, hope, she
* knows, what she's doing... Switch AGC off.
*/
-
if (reg_clear(client, MT9V022_AEC_AGC_ENABLE, 0x2) < 0)
return -EIO;
dev_dbg(&client->dev, "Setting gain from %d to %lu\n",
- reg_read(client, MT9V022_ANALOG_GAIN), gain);
- if (reg_write(client, MT9V022_ANALOG_GAIN, gain) < 0)
+ reg_read(client, MT9V022_ANALOG_GAIN), gain_val);
+ if (reg_write(client, MT9V022_ANALOG_GAIN, gain_val) < 0)
return -EIO;
}
- break;
- case V4L2_CID_EXPOSURE:
- /* mt9v022 has maximum == default */
- if (ctrl->value > qctrl->maximum || ctrl->value < qctrl->minimum)
- return -EINVAL;
- else {
- unsigned long range = qctrl->maximum - qctrl->minimum;
- unsigned long shutter = ((ctrl->value - qctrl->minimum) *
- 479 + range / 2) / range + 1;
+ return 0;
+ case V4L2_CID_EXPOSURE_AUTO:
+ /* Force manual exposure if only the exposure was changed */
+ if (!ctrl->has_new)
+ ctrl->val = V4L2_EXPOSURE_MANUAL;
+ if (ctrl->val == V4L2_EXPOSURE_AUTO) {
+ data = reg_set(client, MT9V022_AEC_AGC_ENABLE, 0x1);
+ } else {
+ struct v4l2_ctrl *exp = mt9v022->exposure;
+ unsigned long range = exp->maximum - exp->minimum;
+ unsigned long shutter = ((exp->val - exp->minimum) *
+ 479 + range / 2) / range + 1;
+
/*
* The user wants to set shutter width manually, hope,
* she knows, what she's doing... Switch AEC off.
*/
-
- if (reg_clear(client, MT9V022_AEC_AGC_ENABLE, 0x1) < 0)
+ data = reg_clear(client, MT9V022_AEC_AGC_ENABLE, 0x1);
+ if (data < 0)
return -EIO;
-
dev_dbg(&client->dev, "Shutter width from %d to %lu\n",
- reg_read(client, MT9V022_TOTAL_SHUTTER_WIDTH),
- shutter);
+ reg_read(client, MT9V022_TOTAL_SHUTTER_WIDTH),
+ shutter);
if (reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
- shutter) < 0)
+ shutter) < 0)
return -EIO;
}
- break;
- case V4L2_CID_AUTOGAIN:
- if (ctrl->value)
- data = reg_set(client, MT9V022_AEC_AGC_ENABLE, 0x2);
- else
- data = reg_clear(client, MT9V022_AEC_AGC_ENABLE, 0x2);
- if (data < 0)
- return -EIO;
- break;
- case V4L2_CID_EXPOSURE_AUTO:
- if (ctrl->value)
- data = reg_set(client, MT9V022_AEC_AGC_ENABLE, 0x1);
- else
- data = reg_clear(client, MT9V022_AEC_AGC_ENABLE, 0x1);
- if (data < 0)
- return -EIO;
- break;
+ return 0;
}
- return 0;
+ return -EINVAL;
}
/*
@@ -825,9 +755,12 @@ static int mt9v022_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
return 0;
}
+static const struct v4l2_ctrl_ops mt9v022_ctrl_ops = {
+ .g_volatile_ctrl = mt9v022_g_volatile_ctrl,
+ .s_ctrl = mt9v022_s_ctrl,
+};
+
static struct v4l2_subdev_core_ops mt9v022_subdev_core_ops = {
- .g_ctrl = mt9v022_g_ctrl,
- .s_ctrl = mt9v022_s_ctrl,
.g_chip_ident = mt9v022_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = mt9v022_g_register,
@@ -900,10 +833,41 @@ static int mt9v022_probe(struct i2c_client *client,
return -ENOMEM;
v4l2_i2c_subdev_init(&mt9v022->subdev, client, &mt9v022_subdev_ops);
+ v4l2_ctrl_handler_init(&mt9v022->hdl, 6);
+ v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ mt9v022->autogain = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+ V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+ mt9v022->gain = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+ V4L2_CID_GAIN, 0, 127, 1, 64);
+
+ /*
+ * Simulated autoexposure. If enabled, we calculate shutter width
+ * ourselves in the driver based on vertical blanking and frame width
+ */
+ mt9v022->autoexposure = v4l2_ctrl_new_std_menu(&mt9v022->hdl,
+ &mt9v022_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
+ V4L2_EXPOSURE_AUTO);
+ mt9v022->exposure = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+ V4L2_CID_EXPOSURE, 1, 255, 1, 255);
+
+ mt9v022->subdev.ctrl_handler = &mt9v022->hdl;
+ if (mt9v022->hdl.error) {
+ int err = mt9v022->hdl.error;
+
+ kfree(mt9v022);
+ return err;
+ }
+ v4l2_ctrl_cluster(2, &mt9v022->autoexposure);
+ v4l2_ctrl_cluster(2, &mt9v022->autogain);
+ mt9v022->gain->is_volatile = true;
+ mt9v022->exposure->is_volatile = true;
mt9v022->chip_control = MT9V022_CHIP_CONTROL_DEFAULT;
- icd->ops = &mt9v022_ops;
+ icd->ops = &mt9v022_ops;
/*
* MT9V022 _really_ corrupts the first read out line.
* TODO: verify on i.MX31
@@ -917,6 +881,7 @@ static int mt9v022_probe(struct i2c_client *client,
ret = mt9v022_video_probe(icd, client);
if (ret) {
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&mt9v022->hdl);
kfree(mt9v022);
}
@@ -928,8 +893,10 @@ static int mt9v022_remove(struct i2c_client *client)
struct mt9v022 *mt9v022 = to_mt9v022(client);
struct soc_camera_device *icd = client->dev.platform_data;
+ v4l2_device_unregister_subdev(&mt9v022->subdev);
icd->ops = NULL;
mt9v022_video_remove(icd);
+ v4l2_ctrl_handler_free(&mt9v022->hdl);
kfree(mt9v022);
return 0;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC PATCH 08/12] ov772x: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (5 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 07/12] mt9v022: " Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-11 23:06 ` [RFC PATCH 09/12] rj54n1cb0c: " Hans Verkuil
` (4 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/ov772x.c | 112 +++++++++++++++---------------------------
1 files changed, 39 insertions(+), 73 deletions(-)
diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
index 48895ef..b445ec0 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -23,6 +23,7 @@
#include <linux/videodev2.h>
#include <media/v4l2-chip-ident.h>
#include <media/v4l2-subdev.h>
+#include <media/v4l2-ctrls.h>
#include <media/soc_camera.h>
#include <media/soc_mediabus.h>
#include <media/ov772x.h>
@@ -400,6 +401,7 @@ struct ov772x_win_size {
struct ov772x_priv {
struct v4l2_subdev subdev;
+ struct v4l2_ctrl_handler hdl;
struct ov772x_camera_info *info;
const struct ov772x_color_format *cfmt;
const struct ov772x_win_size *win;
@@ -517,36 +519,6 @@ static const struct ov772x_win_size ov772x_win_qvga = {
.regs = ov772x_qvga_regs,
};
-static const struct v4l2_queryctrl ov772x_controls[] = {
- {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Vertically",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
- {
- .id = V4L2_CID_HFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Horizontally",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
- {
- .id = V4L2_CID_BAND_STOP_FILTER,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Band-stop filter",
- .minimum = 0,
- .maximum = 256,
- .step = 1,
- .default_value = 0,
- },
-};
-
/*
* general function
*/
@@ -643,26 +615,10 @@ static unsigned long ov772x_query_bus_param(struct soc_camera_device *icd)
return soc_camera_apply_sensor_flags(icl, flags);
}
-static int ov772x_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
-{
- struct ov772x_priv *priv = container_of(sd, struct ov772x_priv, subdev);
-
- switch (ctrl->id) {
- case V4L2_CID_VFLIP:
- ctrl->value = priv->flag_vflip;
- break;
- case V4L2_CID_HFLIP:
- ctrl->value = priv->flag_hflip;
- break;
- case V4L2_CID_BAND_STOP_FILTER:
- ctrl->value = priv->band_filter;
- break;
- }
- return 0;
-}
-
-static int ov772x_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct ov772x_priv, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov772x_priv *priv = container_of(sd, struct ov772x_priv, subdev);
int ret = 0;
@@ -670,25 +626,19 @@ static int ov772x_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
switch (ctrl->id) {
case V4L2_CID_VFLIP:
- val = ctrl->value ? VFLIP_IMG : 0x00;
- priv->flag_vflip = ctrl->value;
+ val = ctrl->val ? VFLIP_IMG : 0x00;
+ priv->flag_vflip = ctrl->val;
if (priv->info->flags & OV772X_FLAG_VFLIP)
val ^= VFLIP_IMG;
- ret = ov772x_mask_set(client, COM3, VFLIP_IMG, val);
- break;
+ return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
case V4L2_CID_HFLIP:
- val = ctrl->value ? HFLIP_IMG : 0x00;
- priv->flag_hflip = ctrl->value;
+ val = ctrl->val ? HFLIP_IMG : 0x00;
+ priv->flag_hflip = ctrl->val;
if (priv->info->flags & OV772X_FLAG_HFLIP)
val ^= HFLIP_IMG;
- ret = ov772x_mask_set(client, COM3, HFLIP_IMG, val);
- break;
+ return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
case V4L2_CID_BAND_STOP_FILTER:
- if ((unsigned)ctrl->value > 256)
- ctrl->value = 256;
- if (ctrl->value == priv->band_filter)
- break;
- if (!ctrl->value) {
+ if (!ctrl->val) {
/* Switch the filter off, it is on now */
ret = ov772x_mask_set(client, BDBASE, 0xff, 0xff);
if (!ret)
@@ -696,7 +646,7 @@ static int ov772x_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
BNDF_ON_OFF, 0);
} else {
/* Switch the filter on, set AEC low limit */
- val = 256 - ctrl->value;
+ val = 256 - ctrl->val;
ret = ov772x_mask_set(client, COM8,
BNDF_ON_OFF, BNDF_ON_OFF);
if (!ret)
@@ -704,11 +654,11 @@ static int ov772x_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
0xff, val);
}
if (!ret)
- priv->band_filter = ctrl->value;
- break;
+ priv->band_filter = ctrl->val;
+ return ret;
}
- return ret;
+ return -EINVAL;
}
static int ov772x_g_chip_ident(struct v4l2_subdev *sd,
@@ -1068,20 +1018,19 @@ static int ov772x_video_probe(struct soc_camera_device *icd,
ver,
i2c_smbus_read_byte_data(client, MIDH),
i2c_smbus_read_byte_data(client, MIDL));
-
- return 0;
+ return v4l2_ctrl_handler_setup(&priv->hdl);
}
+static const struct v4l2_ctrl_ops ov772x_ctrl_ops = {
+ .s_ctrl = ov772x_s_ctrl,
+};
+
static struct soc_camera_ops ov772x_ops = {
.set_bus_param = ov772x_set_bus_param,
.query_bus_param = ov772x_query_bus_param,
- .controls = ov772x_controls,
- .num_controls = ARRAY_SIZE(ov772x_controls),
};
static struct v4l2_subdev_core_ops ov772x_subdev_core_ops = {
- .g_ctrl = ov772x_g_ctrl,
- .s_ctrl = ov772x_s_ctrl,
.g_chip_ident = ov772x_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = ov772x_g_register,
@@ -1150,12 +1099,27 @@ static int ov772x_probe(struct i2c_client *client,
priv->info = icl->priv;
v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
+ v4l2_ctrl_handler_init(&priv->hdl, 3);
+ v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
+ V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
+ priv->subdev.ctrl_handler = &priv->hdl;
+ if (priv->hdl.error) {
+ int err = priv->hdl.error;
+
+ kfree(priv);
+ return err;
+ }
- icd->ops = &ov772x_ops;
+ icd->ops = &ov772x_ops;
ret = ov772x_video_probe(icd, client);
if (ret) {
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&priv->hdl);
kfree(priv);
}
@@ -1167,7 +1131,9 @@ static int ov772x_remove(struct i2c_client *client)
struct ov772x_priv *priv = to_ov772x(client);
struct soc_camera_device *icd = client->dev.platform_data;
+ v4l2_device_unregister_subdev(&priv->subdev);
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&priv->hdl);
kfree(priv);
return 0;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC PATCH 09/12] rj54n1cb0c: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (6 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 08/12] ov772x: " Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-11 23:06 ` [RFC PATCH 10/12] ov2640: " Hans Verkuil
` (3 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/rj54n1cb0c.c | 135 +++++++++++---------------------------
1 files changed, 39 insertions(+), 96 deletions(-)
diff --git a/drivers/media/video/rj54n1cb0c.c b/drivers/media/video/rj54n1cb0c.c
index 57e11b6..f8e4ef0 100644
--- a/drivers/media/video/rj54n1cb0c.c
+++ b/drivers/media/video/rj54n1cb0c.c
@@ -18,6 +18,7 @@
#include <media/soc_mediabus.h>
#include <media/v4l2-subdev.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ctrls.h>
#define RJ54N1_DEV_CODE 0x0400
#define RJ54N1_DEV_CODE2 0x0401
@@ -148,6 +149,7 @@ struct rj54n1_clock_div {
struct rj54n1 {
struct v4l2_subdev subdev;
+ struct v4l2_ctrl_handler hdl;
struct rj54n1_clock_div clk_div;
const struct rj54n1_datafmt *fmt;
struct v4l2_rect rect; /* Sensor window */
@@ -1202,134 +1204,57 @@ static int rj54n1_s_register(struct v4l2_subdev *sd,
}
#endif
-static const struct v4l2_queryctrl rj54n1_controls[] = {
- {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Vertically",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- }, {
- .id = V4L2_CID_HFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Horizontally",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- }, {
- .id = V4L2_CID_GAIN,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Gain",
- .minimum = 0,
- .maximum = 127,
- .step = 1,
- .default_value = 66,
- .flags = V4L2_CTRL_FLAG_SLIDER,
- }, {
- .id = V4L2_CID_AUTO_WHITE_BALANCE,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Auto white balance",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 1,
- },
-};
-
static struct soc_camera_ops rj54n1_ops = {
.set_bus_param = rj54n1_set_bus_param,
.query_bus_param = rj54n1_query_bus_param,
- .controls = rj54n1_controls,
- .num_controls = ARRAY_SIZE(rj54n1_controls),
};
-static int rj54n1_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int rj54n1_s_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct rj54n1, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct rj54n1 *rj54n1 = to_rj54n1(client);
int data;
switch (ctrl->id) {
case V4L2_CID_VFLIP:
- data = reg_read(client, RJ54N1_MIRROR_STILL_MODE);
- if (data < 0)
- return -EIO;
- ctrl->value = !(data & 1);
- break;
- case V4L2_CID_HFLIP:
- data = reg_read(client, RJ54N1_MIRROR_STILL_MODE);
- if (data < 0)
- return -EIO;
- ctrl->value = !(data & 2);
- break;
- case V4L2_CID_GAIN:
- data = reg_read(client, RJ54N1_Y_GAIN);
- if (data < 0)
- return -EIO;
-
- ctrl->value = data / 2;
- break;
- case V4L2_CID_AUTO_WHITE_BALANCE:
- ctrl->value = rj54n1->auto_wb;
- break;
- }
-
- return 0;
-}
-
-static int rj54n1_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
-{
- int data;
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct rj54n1 *rj54n1 = to_rj54n1(client);
- const struct v4l2_queryctrl *qctrl;
-
- qctrl = soc_camera_find_qctrl(&rj54n1_ops, ctrl->id);
- if (!qctrl)
- return -EINVAL;
-
- switch (ctrl->id) {
- case V4L2_CID_VFLIP:
- if (ctrl->value)
+ if (ctrl->val)
data = reg_set(client, RJ54N1_MIRROR_STILL_MODE, 0, 1);
else
data = reg_set(client, RJ54N1_MIRROR_STILL_MODE, 1, 1);
if (data < 0)
return -EIO;
- break;
+ return 0;
case V4L2_CID_HFLIP:
- if (ctrl->value)
+ if (ctrl->val)
data = reg_set(client, RJ54N1_MIRROR_STILL_MODE, 0, 2);
else
data = reg_set(client, RJ54N1_MIRROR_STILL_MODE, 2, 2);
if (data < 0)
return -EIO;
- break;
+ return 0;
case V4L2_CID_GAIN:
- if (ctrl->value > qctrl->maximum ||
- ctrl->value < qctrl->minimum)
- return -EINVAL;
- else if (reg_write(client, RJ54N1_Y_GAIN, ctrl->value * 2) < 0)
+ if (reg_write(client, RJ54N1_Y_GAIN, ctrl->val * 2) < 0)
return -EIO;
- break;
+ return 0;
case V4L2_CID_AUTO_WHITE_BALANCE:
/* Auto WB area - whole image */
- if (reg_set(client, RJ54N1_WB_SEL_WEIGHT_I, ctrl->value << 7,
+ if (reg_set(client, RJ54N1_WB_SEL_WEIGHT_I, ctrl->val << 7,
0x80) < 0)
return -EIO;
- rj54n1->auto_wb = ctrl->value;
- break;
+ rj54n1->auto_wb = ctrl->val;
+ return 0;
}
- return 0;
+ return -EINVAL;
}
+static const struct v4l2_ctrl_ops rj54n1_ctrl_ops = {
+ .s_ctrl = rj54n1_s_ctrl,
+};
+
static struct v4l2_subdev_core_ops rj54n1_subdev_core_ops = {
- .g_ctrl = rj54n1_g_ctrl,
- .s_ctrl = rj54n1_s_ctrl,
.g_chip_ident = rj54n1_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = rj54n1_g_register,
@@ -1426,6 +1351,22 @@ static int rj54n1_probe(struct i2c_client *client,
return -ENOMEM;
v4l2_i2c_subdev_init(&rj54n1->subdev, client, &rj54n1_subdev_ops);
+ v4l2_ctrl_handler_init(&rj54n1->hdl, 4);
+ v4l2_ctrl_new_std(&rj54n1->hdl, &rj54n1_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&rj54n1->hdl, &rj54n1_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&rj54n1->hdl, &rj54n1_ctrl_ops,
+ V4L2_CID_GAIN, 0, 127, 1, 66);
+ v4l2_ctrl_new_std(&rj54n1->hdl, &rj54n1_ctrl_ops,
+ V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
+ rj54n1->subdev.ctrl_handler = &rj54n1->hdl;
+ if (rj54n1->hdl.error) {
+ int err = rj54n1->hdl.error;
+
+ kfree(rj54n1);
+ return err;
+ }
icd->ops = &rj54n1_ops;
@@ -1444,11 +1385,11 @@ static int rj54n1_probe(struct i2c_client *client,
ret = rj54n1_video_probe(icd, client, rj54n1_priv);
if (ret < 0) {
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&rj54n1->hdl);
kfree(rj54n1);
return ret;
}
-
- return ret;
+ return v4l2_ctrl_handler_setup(&rj54n1->hdl);
}
static int rj54n1_remove(struct i2c_client *client)
@@ -1457,9 +1398,11 @@ static int rj54n1_remove(struct i2c_client *client)
struct soc_camera_device *icd = client->dev.platform_data;
struct soc_camera_link *icl = to_soc_camera_link(icd);
+ v4l2_device_unregister_subdev(&rj54n1->subdev);
icd->ops = NULL;
if (icl->free_bus)
icl->free_bus(icl);
+ v4l2_ctrl_handler_free(&rj54n1->hdl);
kfree(rj54n1);
return 0;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC PATCH 10/12] ov2640: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (7 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 09/12] rj54n1cb0c: " Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-11 23:06 ` [RFC PATCH 11/12] ov6550: " Hans Verkuil
` (2 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/ov2640.c | 88 ++++++++++++++---------------------------
1 files changed, 30 insertions(+), 58 deletions(-)
diff --git a/drivers/media/video/ov2640.c b/drivers/media/video/ov2640.c
index 0cea0cf..7c9dcad 100644
--- a/drivers/media/video/ov2640.c
+++ b/drivers/media/video/ov2640.c
@@ -21,6 +21,7 @@
#include <linux/videodev2.h>
#include <media/v4l2-chip-ident.h>
#include <media/v4l2-subdev.h>
+#include <media/v4l2-ctrls.h>
#include <media/soc_camera.h>
#include <media/soc_mediabus.h>
@@ -300,11 +301,10 @@ struct ov2640_win_size {
struct ov2640_priv {
struct v4l2_subdev subdev;
struct ov2640_camera_info *info;
+ struct v4l2_ctrl_handler hdl;
enum v4l2_mbus_pixelcode cfmt_code;
const struct ov2640_win_size *win;
int model;
- u16 flag_vflip:1;
- u16 flag_hflip:1;
};
/*
@@ -610,29 +610,6 @@ static enum v4l2_mbus_pixelcode ov2640_codes[] = {
};
/*
- * Supported controls
- */
-static const struct v4l2_queryctrl ov2640_controls[] = {
- {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Vertically",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- }, {
- .id = V4L2_CID_HFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Horizontally",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
-};
-
-/*
* General functions
*/
static struct ov2640_priv *to_ov2640(const struct i2c_client *client)
@@ -739,43 +716,23 @@ static unsigned long ov2640_query_bus_param(struct soc_camera_device *icd)
return soc_camera_apply_sensor_flags(icl, flags);
}
-static int ov2640_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
-{
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov2640_priv *priv = to_ov2640(client);
-
- switch (ctrl->id) {
- case V4L2_CID_VFLIP:
- ctrl->value = priv->flag_vflip;
- break;
- case V4L2_CID_HFLIP:
- ctrl->value = priv->flag_hflip;
- break;
- }
- return 0;
-}
-
-static int ov2640_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov2640_priv *priv = to_ov2640(client);
- int ret = 0;
u8 val;
switch (ctrl->id) {
case V4L2_CID_VFLIP:
- val = ctrl->value ? REG04_VFLIP_IMG : 0x00;
- priv->flag_vflip = ctrl->value ? 1 : 0;
- ret = ov2640_mask_set(client, REG04, REG04_VFLIP_IMG, val);
- break;
+ val = ctrl->val ? REG04_VFLIP_IMG : 0x00;
+ return ov2640_mask_set(client, REG04, REG04_VFLIP_IMG, val);
case V4L2_CID_HFLIP:
- val = ctrl->value ? REG04_HFLIP_IMG : 0x00;
- priv->flag_hflip = ctrl->value ? 1 : 0;
- ret = ov2640_mask_set(client, REG04, REG04_HFLIP_IMG, val);
- break;
+ val = ctrl->val ? REG04_HFLIP_IMG : 0x00;
+ return ov2640_mask_set(client, REG04, REG04_HFLIP_IMG, val);
}
- return ret;
+ return -EINVAL;
}
static int ov2640_g_chip_ident(struct v4l2_subdev *sd,
@@ -1067,7 +1024,7 @@ static int ov2640_video_probe(struct soc_camera_device *icd,
"%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
devname, pid, ver, midh, midl);
- return 0;
+ return v4l2_ctrl_handler_setup(&priv->hdl);
err:
return ret;
@@ -1076,13 +1033,13 @@ err:
static struct soc_camera_ops ov2640_ops = {
.set_bus_param = ov2640_set_bus_param,
.query_bus_param = ov2640_query_bus_param,
- .controls = ov2640_controls,
- .num_controls = ARRAY_SIZE(ov2640_controls),
+};
+
+static const struct v4l2_ctrl_ops ov2640_ctrl_ops = {
+ .s_ctrl = ov2640_s_ctrl,
};
static struct v4l2_subdev_core_ops ov2640_subdev_core_ops = {
- .g_ctrl = ov2640_g_ctrl,
- .s_ctrl = ov2640_s_ctrl,
.g_chip_ident = ov2640_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = ov2640_g_register,
@@ -1145,12 +1102,25 @@ static int ov2640_probe(struct i2c_client *client,
priv->info = icl->priv;
v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
+ v4l2_ctrl_handler_init(&priv->hdl, 2);
+ v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ priv->subdev.ctrl_handler = &priv->hdl;
+ if (priv->hdl.error) {
+ int err = priv->hdl.error;
+
+ kfree(priv);
+ return err;
+ }
icd->ops = &ov2640_ops;
ret = ov2640_video_probe(icd, client);
if (ret) {
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&priv->hdl);
kfree(priv);
} else {
dev_info(&adapter->dev, "OV2640 Probed\n");
@@ -1164,7 +1134,9 @@ static int ov2640_remove(struct i2c_client *client)
struct ov2640_priv *priv = to_ov2640(client);
struct soc_camera_device *icd = client->dev.platform_data;
+ v4l2_device_unregister_subdev(&priv->subdev);
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&priv->hdl);
kfree(priv);
return 0;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC PATCH 11/12] ov6550: convert to the control framework.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (8 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 10/12] ov2640: " Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-11 23:06 ` [RFC PATCH 12/12] soc_camera: remove the now obsolete controls/num_controls fields Hans Verkuil
2011-01-19 17:49 ` [RFC PATCH 01/12] soc_camera: add control handler support Guennadi Liakhovetski
11 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/ov6650.c | 383 +++++++++++++++---------------------------
1 files changed, 135 insertions(+), 248 deletions(-)
diff --git a/drivers/media/video/ov6650.c b/drivers/media/video/ov6650.c
index cf93de9..98abfcc 100644
--- a/drivers/media/video/ov6650.c
+++ b/drivers/media/video/ov6650.c
@@ -31,6 +31,7 @@
#include <media/soc_camera.h>
#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ctrls.h>
/* Register definitions */
@@ -177,7 +178,25 @@ struct ov6650_reg {
struct ov6650 {
struct v4l2_subdev subdev;
+ struct v4l2_ctrl_handler hdl;
+ struct {
+ /* exposure/autoexposure cluster */
+ struct v4l2_ctrl *autoexposure;
+ struct v4l2_ctrl *exposure;
+ };
+ struct {
+ /* gain/autogain cluster */
+ struct v4l2_ctrl *autogain;
+ struct v4l2_ctrl *gain;
+ };
+ struct {
+ /* blue/red/autowhitebalance cluster */
+ struct v4l2_ctrl *autowb;
+ struct v4l2_ctrl *blue;
+ struct v4l2_ctrl *red;
+ };
+ /*
int gain;
int blue;
int red;
@@ -190,7 +209,7 @@ struct ov6650 {
bool vflip;
bool hflip;
bool awb;
- bool agc;
+ bool agc;*/
bool half_scale; /* scale down output by 2 */
struct v4l2_rect rect; /* sensor cropping window */
unsigned long pclk_limit; /* from host */
@@ -210,126 +229,6 @@ static enum v4l2_mbus_pixelcode ov6650_codes[] = {
V4L2_MBUS_FMT_GREY8_1X8,
};
-static const struct v4l2_queryctrl ov6650_controls[] = {
- {
- .id = V4L2_CID_AUTOGAIN,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "AGC",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 1,
- },
- {
- .id = V4L2_CID_GAIN,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Gain",
- .minimum = 0,
- .maximum = 0x3f,
- .step = 1,
- .default_value = DEF_GAIN,
- },
- {
- .id = V4L2_CID_AUTO_WHITE_BALANCE,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "AWB",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 1,
- },
- {
- .id = V4L2_CID_BLUE_BALANCE,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Blue",
- .minimum = 0,
- .maximum = 0xff,
- .step = 1,
- .default_value = DEF_BLUE,
- },
- {
- .id = V4L2_CID_RED_BALANCE,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Red",
- .minimum = 0,
- .maximum = 0xff,
- .step = 1,
- .default_value = DEF_RED,
- },
- {
- .id = V4L2_CID_SATURATION,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Saturation",
- .minimum = 0,
- .maximum = 0xf,
- .step = 1,
- .default_value = 0x8,
- },
- {
- .id = V4L2_CID_HUE,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Hue",
- .minimum = 0,
- .maximum = HUE_MASK,
- .step = 1,
- .default_value = DEF_HUE,
- },
- {
- .id = V4L2_CID_BRIGHTNESS,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Brightness",
- .minimum = 0,
- .maximum = 0xff,
- .step = 1,
- .default_value = 0x80,
- },
- {
- .id = V4L2_CID_EXPOSURE_AUTO,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "AEC",
- .minimum = 0,
- .maximum = 3,
- .step = 1,
- .default_value = 0,
- },
- {
- .id = V4L2_CID_EXPOSURE,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Exposure",
- .minimum = 0,
- .maximum = 0xff,
- .step = 1,
- .default_value = DEF_AECH,
- },
- {
- .id = V4L2_CID_GAMMA,
- .type = V4L2_CTRL_TYPE_INTEGER,
- .name = "Gamma",
- .minimum = 0,
- .maximum = 0xff,
- .step = 1,
- .default_value = 0x12,
- },
- {
- .id = V4L2_CID_VFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Vertically",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
- {
- .id = V4L2_CID_HFLIP,
- .type = V4L2_CTRL_TYPE_BOOLEAN,
- .name = "Flip Horizontally",
- .minimum = 0,
- .maximum = 1,
- .step = 1,
- .default_value = 0,
- },
-};
-
/* read a register */
static int ov6650_reg_read(struct i2c_client *client, u8 reg, u8 *val)
{
@@ -466,166 +365,109 @@ static unsigned long ov6650_query_bus_param(struct soc_camera_device *icd)
}
/* Get status of additional camera capabilities */
-static int ov6650_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int ov6550_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct ov6650, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- uint8_t reg;
+ uint8_t reg, reg2;
int ret = 0;
switch (ctrl->id) {
case V4L2_CID_AUTOGAIN:
- ctrl->value = priv->agc;
- break;
- case V4L2_CID_GAIN:
- if (priv->agc) {
- ret = ov6650_reg_read(client, REG_GAIN, ®);
- ctrl->value = reg;
- } else {
- ctrl->value = priv->gain;
- }
- break;
+ if (!ctrl->cur.val)
+ return 0;
+ ret = ov6650_reg_read(client, REG_GAIN, ®);
+ if (!ret)
+ priv->gain->cur.val = reg;
+ return ret;
case V4L2_CID_AUTO_WHITE_BALANCE:
- ctrl->value = priv->awb;
- break;
- case V4L2_CID_BLUE_BALANCE:
- if (priv->awb) {
- ret = ov6650_reg_read(client, REG_BLUE, ®);
- ctrl->value = reg;
- } else {
- ctrl->value = priv->blue;
- }
- break;
- case V4L2_CID_RED_BALANCE:
- if (priv->awb) {
- ret = ov6650_reg_read(client, REG_RED, ®);
- ctrl->value = reg;
- } else {
- ctrl->value = priv->red;
+ if (!ctrl->cur.val)
+ return 0;
+ ret = ov6650_reg_read(client, REG_BLUE, ®);
+ if (!ret)
+ ret = ov6650_reg_read(client, REG_RED, ®2);
+ if (!ret) {
+ priv->blue->cur.val = reg;
+ priv->red->cur.val = reg2;
}
- break;
- case V4L2_CID_SATURATION:
- ctrl->value = priv->saturation;
- break;
- case V4L2_CID_HUE:
- ctrl->value = priv->hue;
- break;
- case V4L2_CID_BRIGHTNESS:
- ctrl->value = priv->brightness;
- break;
+ return ret;
case V4L2_CID_EXPOSURE_AUTO:
- ctrl->value = priv->aec;
- break;
- case V4L2_CID_EXPOSURE:
- if (priv->aec) {
- ret = ov6650_reg_read(client, REG_AECH, ®);
- ctrl->value = reg;
- } else {
- ctrl->value = priv->exposure;
- }
- break;
- case V4L2_CID_GAMMA:
- ctrl->value = priv->gamma;
- break;
- case V4L2_CID_VFLIP:
- ctrl->value = priv->vflip;
- break;
- case V4L2_CID_HFLIP:
- ctrl->value = priv->hflip;
- break;
+ if (ctrl->cur.val != V4L2_CID_EXPOSURE_AUTO)
+ return 0;
+ ret = ov6650_reg_read(client, REG_AECH, ®);
+ if (!ret)
+ priv->exposure->cur.val = reg;
+ return ret;
}
- return ret;
+ return -EINVAL;
}
/* Set status of additional camera capabilities */
-static int ov6650_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int ov6550_s_ctrl(struct v4l2_ctrl *ctrl)
{
+ struct v4l2_subdev *sd =
+ &container_of(ctrl->handler, struct ov6650, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
int ret = 0;
switch (ctrl->id) {
case V4L2_CID_AUTOGAIN:
+ /* Force manual gain if only the gain was changed */
+ if (!ctrl->has_new)
+ ctrl->val = 0;
ret = ov6650_reg_rmw(client, REG_COMB,
- ctrl->value ? COMB_AGC : 0, COMB_AGC);
- if (!ret)
- priv->agc = ctrl->value;
- break;
- case V4L2_CID_GAIN:
- ret = ov6650_reg_write(client, REG_GAIN, ctrl->value);
- if (!ret)
- priv->gain = ctrl->value;
- break;
+ ctrl->val ? COMB_AGC : 0, COMB_AGC);
+ if (!ret && !ctrl->val)
+ ret = ov6650_reg_write(client, REG_GAIN, priv->gain->val);
+ return ret;;
case V4L2_CID_AUTO_WHITE_BALANCE:
+ /* Force manual balance if only the red and/or blue balance
+ was changed */
+ if (!ctrl->has_new)
+ ctrl->val = 0;
ret = ov6650_reg_rmw(client, REG_COMB,
- ctrl->value ? COMB_AWB : 0, COMB_AWB);
- if (!ret)
- priv->awb = ctrl->value;
- break;
- case V4L2_CID_BLUE_BALANCE:
- ret = ov6650_reg_write(client, REG_BLUE, ctrl->value);
- if (!ret)
- priv->blue = ctrl->value;
- break;
- case V4L2_CID_RED_BALANCE:
- ret = ov6650_reg_write(client, REG_RED, ctrl->value);
- if (!ret)
- priv->red = ctrl->value;
- break;
+ ctrl->val ? COMB_AWB : 0, COMB_AWB);
+ if (!ret && !ctrl->val) {
+ ret = ov6650_reg_write(client, REG_BLUE, priv->blue->val);
+ if (!ret)
+ ret = ov6650_reg_write(client, REG_BLUE,
+ priv->red->val);
+ }
+ return ret;
case V4L2_CID_SATURATION:
- ret = ov6650_reg_rmw(client, REG_SAT, SET_SAT(ctrl->value),
+ return ov6650_reg_rmw(client, REG_SAT, SET_SAT(ctrl->val),
SAT_MASK);
- if (!ret)
- priv->saturation = ctrl->value;
- break;
case V4L2_CID_HUE:
- ret = ov6650_reg_rmw(client, REG_HUE, SET_HUE(ctrl->value),
+ return ov6650_reg_rmw(client, REG_HUE, SET_HUE(ctrl->val),
HUE_MASK);
- if (!ret)
- priv->hue = ctrl->value;
- break;
case V4L2_CID_BRIGHTNESS:
- ret = ov6650_reg_write(client, REG_BRT, ctrl->value);
- if (!ret)
- priv->brightness = ctrl->value;
- break;
+ return ov6650_reg_write(client, REG_BRT, ctrl->val);
case V4L2_CID_EXPOSURE_AUTO:
- switch (ctrl->value) {
- case V4L2_EXPOSURE_AUTO:
+ /* Force manual exposure if only the exposure was changed */
+ if (!ctrl->has_new)
+ ctrl->val = V4L2_EXPOSURE_MANUAL;
+ if (ctrl->val == V4L2_EXPOSURE_AUTO)
ret = ov6650_reg_rmw(client, REG_COMB, COMB_AEC, 0);
- break;
- default:
+ else
ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_AEC);
- break;
- }
- if (!ret)
- priv->aec = ctrl->value;
- break;
- case V4L2_CID_EXPOSURE:
- ret = ov6650_reg_write(client, REG_AECH, ctrl->value);
- if (!ret)
- priv->exposure = ctrl->value;
- break;
+ if (!ret && ctrl->val == V4L2_EXPOSURE_MANUAL)
+ ret = ov6650_reg_write(client, REG_AECH,
+ priv->exposure->val);
+ return ret;
case V4L2_CID_GAMMA:
- ret = ov6650_reg_write(client, REG_GAM1, ctrl->value);
- if (!ret)
- priv->gamma = ctrl->value;
- break;
+ return ov6650_reg_write(client, REG_GAM1, ctrl->val);
case V4L2_CID_VFLIP:
- ret = ov6650_reg_rmw(client, REG_COMB,
- ctrl->value ? COMB_FLIP_V : 0, COMB_FLIP_V);
- if (!ret)
- priv->vflip = ctrl->value;
- break;
+ return ov6650_reg_rmw(client, REG_COMB,
+ ctrl->val ? COMB_FLIP_V : 0, COMB_FLIP_V);
case V4L2_CID_HFLIP:
- ret = ov6650_reg_rmw(client, REG_COMB,
- ctrl->value ? COMB_FLIP_H : 0, COMB_FLIP_H);
- if (!ret)
- priv->hflip = ctrl->value;
- break;
+ return ov6650_reg_rmw(client, REG_COMB,
+ ctrl->val ? COMB_FLIP_H : 0, COMB_FLIP_H);
}
- return ret;
+ return -EINVAL;
}
/* Get chip identification */
@@ -1097,13 +939,14 @@ static int ov6650_video_probe(struct soc_camera_device *icd,
static struct soc_camera_ops ov6650_ops = {
.set_bus_param = ov6650_set_bus_param,
.query_bus_param = ov6650_query_bus_param,
- .controls = ov6650_controls,
- .num_controls = ARRAY_SIZE(ov6650_controls),
+};
+
+static const struct v4l2_ctrl_ops ov6550_ctrl_ops = {
+ .g_volatile_ctrl = ov6550_g_volatile_ctrl,
+ .s_ctrl = ov6550_s_ctrl,
};
static struct v4l2_subdev_core_ops ov6650_core_ops = {
- .g_ctrl = ov6650_g_ctrl,
- .s_ctrl = ov6650_s_ctrl,
.g_chip_ident = ov6650_g_chip_ident,
#ifdef CONFIG_VIDEO_ADV_DEBUG
.g_register = ov6650_get_register,
@@ -1159,6 +1002,45 @@ static int ov6650_probe(struct i2c_client *client,
}
v4l2_i2c_subdev_init(&priv->subdev, client, &ov6650_subdev_ops);
+ v4l2_ctrl_handler_init(&priv->hdl, 13);
+ v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ priv->autogain = v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+ priv->gain = v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_GAIN, 0, 0x3f, 1, DEF_GAIN);
+ priv->autowb = v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
+ priv->blue = v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_BLUE_BALANCE, 0, 0xff, 1, DEF_BLUE);
+ priv->red = v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_RED_BALANCE, 0, 0xff, 1, DEF_RED);
+ v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_SATURATION, 0, 0xf, 1, 0x8);
+ v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_HUE, 0, HUE_MASK, 1, DEF_HUE);
+ v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_BRIGHTNESS, 0, 0xff, 1, 0x80);
+ priv->autoexposure = v4l2_ctrl_new_std_menu(&priv->hdl,
+ &ov6550_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
+ V4L2_EXPOSURE_AUTO);
+ priv->exposure = v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_EXPOSURE, 0, 0xff, 1, DEF_AECH);
+ v4l2_ctrl_new_std(&priv->hdl, &ov6550_ctrl_ops,
+ V4L2_CID_GAMMA, 0, 0xff, 1, 0x12);
+
+ priv->subdev.ctrl_handler = &priv->hdl;
+ if (priv->hdl.error) {
+ int err = priv->hdl.error;
+
+ kfree(priv);
+ return err;
+ }
+ v4l2_ctrl_cluster(2, &priv->autogain);
+ v4l2_ctrl_cluster(3, &priv->autowb);
+ v4l2_ctrl_cluster(2, &priv->autoexposure);
icd->ops = &ov6650_ops;
@@ -1171,9 +1053,12 @@ static int ov6650_probe(struct i2c_client *client,
priv->colorspace = V4L2_COLORSPACE_JPEG;
ret = ov6650_video_probe(icd, client);
+ if (!ret)
+ ret = v4l2_ctrl_handler_setup(&priv->hdl);
if (ret) {
icd->ops = NULL;
+ v4l2_ctrl_handler_free(&priv->hdl);
kfree(priv);
}
@@ -1184,6 +1069,8 @@ static int ov6650_remove(struct i2c_client *client)
{
struct ov6650 *priv = to_ov6650(client);
+ v4l2_device_unregister_subdev(&priv->subdev);
+ v4l2_ctrl_handler_free(&priv->hdl);
kfree(priv);
return 0;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [RFC PATCH 12/12] soc_camera: remove the now obsolete controls/num_controls fields.
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (9 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 11/12] ov6550: " Hans Verkuil
@ 2011-01-11 23:06 ` Hans Verkuil
2011-01-19 17:49 ` [RFC PATCH 01/12] soc_camera: add control handler support Guennadi Liakhovetski
11 siblings, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-11 23:06 UTC (permalink / raw)
To: linux-media
Cc: Guennadi Liakhovetski, Magnus Damm, Kuninori Morimoto,
Alberto Panizzo, Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
include/media/soc_camera.h | 14 --------------
1 files changed, 0 insertions(+), 14 deletions(-)
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index b71b26e..361309b 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -190,8 +190,6 @@ struct soc_camera_ops {
unsigned long (*query_bus_param)(struct soc_camera_device *);
int (*set_bus_param)(struct soc_camera_device *, unsigned long);
int (*enum_input)(struct soc_camera_device *, struct v4l2_input *);
- const struct v4l2_queryctrl *controls;
- int num_controls;
};
#define SOCAM_SENSE_PCLK_CHANGED (1 << 0)
@@ -220,18 +218,6 @@ struct soc_camera_sense {
unsigned long pixel_clock;
};
-static inline struct v4l2_queryctrl const *soc_camera_find_qctrl(
- struct soc_camera_ops *ops, int id)
-{
- int i;
-
- for (i = 0; i < ops->num_controls; i++)
- if (ops->controls[i].id == id)
- return &ops->controls[i];
-
- return NULL;
-}
-
#define SOCAM_MASTER (1 << 0)
#define SOCAM_SLAVE (1 << 1)
#define SOCAM_HSYNC_ACTIVE_HIGH (1 << 2)
--
1.7.0.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH 01/12] soc_camera: add control handler support
2011-01-11 23:06 ` [RFC PATCH 01/12] soc_camera: add control handler support Hans Verkuil
` (10 preceding siblings ...)
2011-01-11 23:06 ` [RFC PATCH 12/12] soc_camera: remove the now obsolete controls/num_controls fields Hans Verkuil
@ 2011-01-19 17:49 ` Guennadi Liakhovetski
2011-01-23 19:44 ` Guennadi Liakhovetski
2011-01-25 7:34 ` Hans Verkuil
11 siblings, 2 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2011-01-19 17:49 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
Hi Hans
This is not a complete review yet, but just something that occurred to me,
while looking at the result of applying all these your patches, maybe you
could just explain, why I'm wrong, or this will be something to change in
the next version:
On Wed, 12 Jan 2011, Hans Verkuil wrote:
> The soc_camera framework is switched over to use the control framework.
> After this patch none of the controls in subdevs or host drivers are available,
> until those drivers are also converted to the control framework.
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
> drivers/media/video/soc_camera.c | 104 +++++++----------------------
> drivers/media/video/soc_camera_platform.c | 8 ++-
> include/media/soc_camera.h | 2 +
> 3 files changed, 33 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index a66811b..7de3fe2 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c
[snip]
> @@ -908,6 +840,8 @@ static int soc_camera_init_i2c(struct soc_camera_device *icd,
> icl->board_info, NULL);
> if (!subdev)
> goto ei2cnd;
> + if (v4l2_ctrl_add_handler(&icd->ctrl_handler, subdev->ctrl_handler))
> + goto ei2cnd;
>
> client = v4l2_get_subdevdata(subdev);
>
Is this really i2c-specific? You added this to soc_camera_init_i2c(),
which is only even built if I2C is configured, and is only used with i2c
subdevs. It is called from soc_camera_probe(), if the respective subdev
has i2c board-information. Otherwise a generic initialisation will take
place, as is, e.g., the case with the soc-camera-platform driver.
Shouldn't this call to add_handler be moved directly to soc_camera_probe()
ot be used for all - not only i2c - subdevs? And one more nitpick below:
> @@ -963,15 +897,15 @@ static int soc_camera_probe(struct device *dev)
> if (icl->reset)
> icl->reset(icd->pdev);
>
> - ret = ici->ops->add(icd);
> - if (ret < 0)
> - goto eadd;
> -
> /* Must have icd->vdev before registering the device */
> ret = video_dev_create(icd);
> if (ret < 0)
> goto evdc;
>
> + ret = ici->ops->add(icd);
> + if (ret < 0)
> + goto eadd;
> +
Yes, this is something, I'll have to think about / have a look at / test.
> /* Non-i2c cameras, e.g., soc_camera_platform, have no board_info */
> if (icl->board_info) {
> ret = soc_camera_init_i2c(icd, icl);
> @@ -1054,10 +988,10 @@ eiufmt:
> }
> enodrv:
> eadddev:
> - video_device_release(icd->vdev);
> -evdc:
> ici->ops->remove(icd);
> eadd:
> + video_device_release(icd->vdev);
> +evdc:
> soc_camera_power_set(icd, icl, 0);
> epower:
> regulator_bulk_free(icl->num_regulators, icl->regulators);
> @@ -1324,9 +1258,6 @@ static const struct v4l2_ioctl_ops soc_camera_ioctl_ops = {
> .vidioc_dqbuf = soc_camera_dqbuf,
> .vidioc_streamon = soc_camera_streamon,
> .vidioc_streamoff = soc_camera_streamoff,
> - .vidioc_queryctrl = soc_camera_queryctrl,
> - .vidioc_g_ctrl = soc_camera_g_ctrl,
> - .vidioc_s_ctrl = soc_camera_s_ctrl,
> .vidioc_cropcap = soc_camera_cropcap,
> .vidioc_g_crop = soc_camera_g_crop,
> .vidioc_s_crop = soc_camera_s_crop,
> @@ -1355,6 +1286,7 @@ static int video_dev_create(struct soc_camera_device *icd)
> vdev->ioctl_ops = &soc_camera_ioctl_ops;
> vdev->release = video_device_release;
> vdev->tvnorms = V4L2_STD_UNKNOWN;
> + vdev->ctrl_handler = &icd->ctrl_handler;
>
> icd->vdev = vdev;
>
> @@ -1402,13 +1334,24 @@ static int __devinit soc_camera_pdrv_probe(struct platform_device *pdev)
> if (!icd)
> return -ENOMEM;
>
> + /*
> + * Currently the subdev with the largest number of controls (13) is
> + * ov6550. So let's pick 16 as a hint for the control handler. Note
> + * that this is a hint only: too large and you waste some memory, too
> + * small and there is a (very) small performance hit when looking up
> + * controls in the internal hash.
> + */
> + ret = v4l2_ctrl_handler_init(&icd->ctrl_handler, 16);
> + if (ret < 0)
> + goto escdevreg;
> +
> icd->iface = icl->bus_id;
> icd->pdev = &pdev->dev;
> platform_set_drvdata(pdev, icd);
>
> ret = soc_camera_device_register(icd);
> if (ret < 0)
> - goto escdevreg;
> + goto eschdlinit;
hm, no, eXXX means in my notation XXX has failed. So, escdevreg means
"soc_camera_device_register() failed" and your eschdlinit should go to the
previous goto.
>
> soc_camera_device_init(&icd->dev, icl);
>
> @@ -1417,6 +1360,8 @@ static int __devinit soc_camera_pdrv_probe(struct platform_device *pdev)
>
> return 0;
>
> +eschdlinit:
> + v4l2_ctrl_handler_free(&icd->ctrl_handler);
Then this will change too, of course.
> escdevreg:
> kfree(icd);
>
> @@ -1437,6 +1382,7 @@ static int __devexit soc_camera_pdrv_remove(struct platform_device *pdev)
>
> soc_camera_device_unregister(icd);
>
> + v4l2_ctrl_handler_free(&icd->ctrl_handler);
> kfree(icd);
>
> return 0;
> diff --git a/drivers/media/video/soc_camera_platform.c b/drivers/media/video/soc_camera_platform.c
> index bf406e8..e319dda 100644
> --- a/drivers/media/video/soc_camera_platform.c
> +++ b/drivers/media/video/soc_camera_platform.c
> @@ -174,9 +174,13 @@ static int soc_camera_platform_probe(struct platform_device *pdev)
> ret = v4l2_device_register_subdev(&ici->v4l2_dev, &priv->subdev);
> if (ret)
> goto evdrs;
> + ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, priv->subdev.ctrl_handler);
> + if (ret)
> + goto evunreg;
and this will get a different name
> + return 0;
>
> - return ret;
> -
> +evunreg:
> + v4l2_device_unregister_subdev(&priv->subdev);
> evdrs:
> icd->ops = NULL;
> platform_set_drvdata(pdev, NULL);
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 9386db8..ee61ffb 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -18,6 +18,7 @@
> #include <linux/videodev2.h>
> #include <media/videobuf-core.h>
> #include <media/v4l2-device.h>
> +#include <media/v4l2-ctrls.h>
>
> extern struct bus_type soc_camera_bus_type;
>
> @@ -35,6 +36,7 @@ struct soc_camera_device {
> struct soc_camera_sense *sense; /* See comment in struct definition */
> struct soc_camera_ops *ops;
> struct video_device *vdev;
> + struct v4l2_ctrl_handler ctrl_handler;
> const struct soc_camera_format_xlate *current_fmt;
> struct soc_camera_format_xlate *user_formats;
> int num_user_formats;
> --
> 1.7.0.4
>
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH 01/12] soc_camera: add control handler support
2011-01-19 17:49 ` [RFC PATCH 01/12] soc_camera: add control handler support Guennadi Liakhovetski
@ 2011-01-23 19:44 ` Guennadi Liakhovetski
2011-01-25 7:34 ` Hans Verkuil
1 sibling, 0 replies; 27+ messages in thread
From: Guennadi Liakhovetski @ 2011-01-23 19:44 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
On Wed, 19 Jan 2011, Guennadi Liakhovetski wrote:
> > @@ -963,15 +897,15 @@ static int soc_camera_probe(struct device *dev)
> > if (icl->reset)
> > icl->reset(icd->pdev);
> >
> > - ret = ici->ops->add(icd);
> > - if (ret < 0)
> > - goto eadd;
> > -
> > /* Must have icd->vdev before registering the device */
> > ret = video_dev_create(icd);
> > if (ret < 0)
> > goto evdc;
> >
> > + ret = ici->ops->add(icd);
> > + if (ret < 0)
> > + goto eadd;
> > +
>
> Yes, this is something, I'll have to think about / have a look at / test.
Right, this looks harmless.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH 01/12] soc_camera: add control handler support
2011-01-19 17:49 ` [RFC PATCH 01/12] soc_camera: add control handler support Guennadi Liakhovetski
2011-01-23 19:44 ` Guennadi Liakhovetski
@ 2011-01-25 7:34 ` Hans Verkuil
1 sibling, 0 replies; 27+ messages in thread
From: Hans Verkuil @ 2011-01-25 7:34 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-media, Magnus Damm, Kuninori Morimoto, Alberto Panizzo,
Janusz Krzysztofik, Marek Vasut, Robert Jarzmik
On Wednesday, January 19, 2011 18:49:19 Guennadi Liakhovetski wrote:
> Hi Hans
>
> This is not a complete review yet, but just something that occurred to me,
> while looking at the result of applying all these your patches, maybe you
> could just explain, why I'm wrong, or this will be something to change in
> the next version:
>
> On Wed, 12 Jan 2011, Hans Verkuil wrote:
>
> > The soc_camera framework is switched over to use the control framework.
> > After this patch none of the controls in subdevs or host drivers are available,
> > until those drivers are also converted to the control framework.
> >
> > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> > ---
> > drivers/media/video/soc_camera.c | 104 +++++++----------------------
> > drivers/media/video/soc_camera_platform.c | 8 ++-
> > include/media/soc_camera.h | 2 +
> > 3 files changed, 33 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> > index a66811b..7de3fe2 100644
> > --- a/drivers/media/video/soc_camera.c
> > +++ b/drivers/media/video/soc_camera.c
>
> [snip]
>
> > @@ -908,6 +840,8 @@ static int soc_camera_init_i2c(struct soc_camera_device *icd,
> > icl->board_info, NULL);
> > if (!subdev)
> > goto ei2cnd;
> > + if (v4l2_ctrl_add_handler(&icd->ctrl_handler, subdev->ctrl_handler))
> > + goto ei2cnd;
> >
> > client = v4l2_get_subdevdata(subdev);
> >
>
> Is this really i2c-specific? You added this to soc_camera_init_i2c(),
> which is only even built if I2C is configured, and is only used with i2c
> subdevs. It is called from soc_camera_probe(), if the respective subdev
> has i2c board-information. Otherwise a generic initialisation will take
> place, as is, e.g., the case with the soc-camera-platform driver.
> Shouldn't this call to add_handler be moved directly to soc_camera_probe()
> ot be used for all - not only i2c - subdevs? And one more nitpick below:
Good point, I completely missed the fact that you can have non-i2c subdevs.
I'll move it to probe.
> > @@ -963,15 +897,15 @@ static int soc_camera_probe(struct device *dev)
> > if (icl->reset)
> > icl->reset(icd->pdev);
> >
> > - ret = ici->ops->add(icd);
> > - if (ret < 0)
> > - goto eadd;
> > -
> > /* Must have icd->vdev before registering the device */
> > ret = video_dev_create(icd);
> > if (ret < 0)
> > goto evdc;
> >
> > + ret = ici->ops->add(icd);
> > + if (ret < 0)
> > + goto eadd;
> > +
>
> Yes, this is something, I'll have to think about / have a look at / test.
>
> > /* Non-i2c cameras, e.g., soc_camera_platform, have no board_info */
> > if (icl->board_info) {
> > ret = soc_camera_init_i2c(icd, icl);
> > @@ -1054,10 +988,10 @@ eiufmt:
> > }
> > enodrv:
> > eadddev:
> > - video_device_release(icd->vdev);
> > -evdc:
> > ici->ops->remove(icd);
> > eadd:
> > + video_device_release(icd->vdev);
> > +evdc:
> > soc_camera_power_set(icd, icl, 0);
> > epower:
> > regulator_bulk_free(icl->num_regulators, icl->regulators);
> > @@ -1324,9 +1258,6 @@ static const struct v4l2_ioctl_ops soc_camera_ioctl_ops = {
> > .vidioc_dqbuf = soc_camera_dqbuf,
> > .vidioc_streamon = soc_camera_streamon,
> > .vidioc_streamoff = soc_camera_streamoff,
> > - .vidioc_queryctrl = soc_camera_queryctrl,
> > - .vidioc_g_ctrl = soc_camera_g_ctrl,
> > - .vidioc_s_ctrl = soc_camera_s_ctrl,
> > .vidioc_cropcap = soc_camera_cropcap,
> > .vidioc_g_crop = soc_camera_g_crop,
> > .vidioc_s_crop = soc_camera_s_crop,
> > @@ -1355,6 +1286,7 @@ static int video_dev_create(struct soc_camera_device *icd)
> > vdev->ioctl_ops = &soc_camera_ioctl_ops;
> > vdev->release = video_device_release;
> > vdev->tvnorms = V4L2_STD_UNKNOWN;
> > + vdev->ctrl_handler = &icd->ctrl_handler;
> >
> > icd->vdev = vdev;
> >
> > @@ -1402,13 +1334,24 @@ static int __devinit soc_camera_pdrv_probe(struct platform_device *pdev)
> > if (!icd)
> > return -ENOMEM;
> >
> > + /*
> > + * Currently the subdev with the largest number of controls (13) is
> > + * ov6550. So let's pick 16 as a hint for the control handler. Note
> > + * that this is a hint only: too large and you waste some memory, too
> > + * small and there is a (very) small performance hit when looking up
> > + * controls in the internal hash.
> > + */
> > + ret = v4l2_ctrl_handler_init(&icd->ctrl_handler, 16);
> > + if (ret < 0)
> > + goto escdevreg;
> > +
> > icd->iface = icl->bus_id;
> > icd->pdev = &pdev->dev;
> > platform_set_drvdata(pdev, icd);
> >
> > ret = soc_camera_device_register(icd);
> > if (ret < 0)
> > - goto escdevreg;
> > + goto eschdlinit;
>
> hm, no, eXXX means in my notation XXX has failed. So, escdevreg means
> "soc_camera_device_register() failed" and your eschdlinit should go to the
> previous goto.
Agreed.
>
> >
> > soc_camera_device_init(&icd->dev, icl);
> >
> > @@ -1417,6 +1360,8 @@ static int __devinit soc_camera_pdrv_probe(struct platform_device *pdev)
> >
> > return 0;
> >
> > +eschdlinit:
> > + v4l2_ctrl_handler_free(&icd->ctrl_handler);
>
> Then this will change too, of course.
>
> > escdevreg:
> > kfree(icd);
> >
> > @@ -1437,6 +1382,7 @@ static int __devexit soc_camera_pdrv_remove(struct platform_device *pdev)
> >
> > soc_camera_device_unregister(icd);
> >
> > + v4l2_ctrl_handler_free(&icd->ctrl_handler);
> > kfree(icd);
> >
> > return 0;
> > diff --git a/drivers/media/video/soc_camera_platform.c b/drivers/media/video/soc_camera_platform.c
> > index bf406e8..e319dda 100644
> > --- a/drivers/media/video/soc_camera_platform.c
> > +++ b/drivers/media/video/soc_camera_platform.c
> > @@ -174,9 +174,13 @@ static int soc_camera_platform_probe(struct platform_device *pdev)
> > ret = v4l2_device_register_subdev(&ici->v4l2_dev, &priv->subdev);
> > if (ret)
> > goto evdrs;
> > + ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, priv->subdev.ctrl_handler);
> > + if (ret)
> > + goto evunreg;
>
> and this will get a different name
Yes.
>
> > + return 0;
> >
> > - return ret;
> > -
> > +evunreg:
> > + v4l2_device_unregister_subdev(&priv->subdev);
> > evdrs:
> > icd->ops = NULL;
> > platform_set_drvdata(pdev, NULL);
> > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > index 9386db8..ee61ffb 100644
> > --- a/include/media/soc_camera.h
> > +++ b/include/media/soc_camera.h
> > @@ -18,6 +18,7 @@
> > #include <linux/videodev2.h>
> > #include <media/videobuf-core.h>
> > #include <media/v4l2-device.h>
> > +#include <media/v4l2-ctrls.h>
> >
> > extern struct bus_type soc_camera_bus_type;
> >
> > @@ -35,6 +36,7 @@ struct soc_camera_device {
> > struct soc_camera_sense *sense; /* See comment in struct definition */
> > struct soc_camera_ops *ops;
> > struct video_device *vdev;
> > + struct v4l2_ctrl_handler ctrl_handler;
> > const struct soc_camera_format_xlate *current_fmt;
> > struct soc_camera_format_xlate *user_formats;
> > int num_user_formats;
>
> Thanks
> Guennadi
> ---
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
^ permalink raw reply [flat|nested] 27+ messages in thread