* [PATCH 1/1] mt9v032: Provide pixel rate control
@ 2012-03-15 21:01 Sakari Ailus
2012-03-16 11:58 ` Laurent Pinchart
0 siblings, 1 reply; 7+ messages in thread
From: Sakari Ailus @ 2012-03-15 21:01 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart
Provide pixel rate control calculated from external clock and horizontal
binning factor.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
drivers/media/video/mt9v032.c | 35 ++++++++++++++++++++++++++++++++++-
1 files changed, 34 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
index 75e253a..e530e8d 100644
--- a/drivers/media/video/mt9v032.c
+++ b/drivers/media/video/mt9v032.c
@@ -122,6 +122,7 @@ struct mt9v032 {
struct v4l2_mbus_framefmt format;
struct v4l2_rect crop;
+ struct v4l2_ctrl *pixel_rate;
struct v4l2_ctrl_handler ctrls;
struct mutex power_lock;
@@ -187,13 +188,15 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable)
return 0;
}
+#define EXT_CLK 25000000
+
static int mt9v032_power_on(struct mt9v032 *mt9v032)
{
struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
int ret;
if (mt9v032->pdata->set_clock) {
- mt9v032->pdata->set_clock(&mt9v032->subdev, 25000000);
+ mt9v032->pdata->set_clock(&mt9v032->subdev, EXT_CLK);
udelay(1);
}
@@ -365,6 +368,27 @@ static int mt9v032_get_format(struct v4l2_subdev *subdev,
return 0;
}
+static void mt9v032_configure_pixel_rate(struct v4l2_subdev *subdev,
+ unsigned int hratio)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(subdev);
+ struct mt9v032 *mt9v032 = to_mt9v032(subdev);
+ struct v4l2_ext_controls ctrls;
+ struct v4l2_ext_control ctrl;
+
+ memset(&ctrls, 0, sizeof(ctrls));
+ memset(&ctrl, 0, sizeof(ctrl));
+
+ ctrls.count = 1;
+ ctrls.controls = &ctrl;
+
+ ctrl.id = V4L2_CID_PIXEL_RATE;
+ ctrl.value64 = EXT_CLK / hratio;
+
+ if (v4l2_s_ext_ctrls(mt9v032->pixel_rate->ctrl_handler, &ctrls) < 0)
+ dev_warn(&client->dev, "bug: failed to set pixel rate\n");
+}
+
static int mt9v032_set_format(struct v4l2_subdev *subdev,
struct v4l2_subdev_fh *fh,
struct v4l2_subdev_format *format)
@@ -395,6 +419,8 @@ static int mt9v032_set_format(struct v4l2_subdev *subdev,
format->which);
__format->width = __crop->width / hratio;
__format->height = __crop->height / vratio;
+ if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ mt9v032_configure_pixel_rate(subdev, hratio);
format->format = *__format;
@@ -450,6 +476,8 @@ static int mt9v032_set_crop(struct v4l2_subdev *subdev,
crop->which);
__format->width = rect.width;
__format->height = rect.height;
+ if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ mt9v032_configure_pixel_rate(subdev, 1);
}
*__crop = rect;
@@ -695,6 +723,9 @@ static int mt9v032_probe(struct i2c_client *client,
V4L2_CID_EXPOSURE, MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
MT9V032_TOTAL_SHUTTER_WIDTH_MAX, 1,
MT9V032_TOTAL_SHUTTER_WIDTH_DEF);
+ mt9v032->pixel_rate =
+ v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
+ V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
for (i = 0; i < ARRAY_SIZE(mt9v032_ctrls); ++i)
v4l2_ctrl_new_custom(&mt9v032->ctrls, &mt9v032_ctrls[i], NULL);
@@ -716,6 +747,8 @@ static int mt9v032_probe(struct i2c_client *client,
mt9v032->format.field = V4L2_FIELD_NONE;
mt9v032->format.colorspace = V4L2_COLORSPACE_SRGB;
+ mt9v032_configure_pixel_rate(subdev, 1);
+
mt9v032->aec_agc = MT9V032_AEC_ENABLE | MT9V032_AGC_ENABLE;
v4l2_i2c_subdev_init(&mt9v032->subdev, client, &mt9v032_subdev_ops);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] mt9v032: Provide pixel rate control
2012-03-15 21:01 [PATCH 1/1] mt9v032: Provide pixel rate control Sakari Ailus
@ 2012-03-16 11:58 ` Laurent Pinchart
2012-03-16 12:12 ` Sakari Ailus
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2012-03-16 11:58 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
Thanks for the patch.
On Thursday 15 March 2012 23:01:39 Sakari Ailus wrote:
> Provide pixel rate control calculated from external clock and horizontal
> binning factor.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> drivers/media/video/mt9v032.c | 35 ++++++++++++++++++++++++++++++++++-
> 1 files changed, 34 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
> index 75e253a..e530e8d 100644
> --- a/drivers/media/video/mt9v032.c
> +++ b/drivers/media/video/mt9v032.c
> @@ -122,6 +122,7 @@ struct mt9v032 {
> struct v4l2_mbus_framefmt format;
> struct v4l2_rect crop;
>
> + struct v4l2_ctrl *pixel_rate;
> struct v4l2_ctrl_handler ctrls;
>
> struct mutex power_lock;
> @@ -187,13 +188,15 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16
> which, int enable) return 0;
> }
>
> +#define EXT_CLK 25000000
> +
> static int mt9v032_power_on(struct mt9v032 *mt9v032)
> {
> struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> int ret;
>
> if (mt9v032->pdata->set_clock) {
> - mt9v032->pdata->set_clock(&mt9v032->subdev, 25000000);
> + mt9v032->pdata->set_clock(&mt9v032->subdev, EXT_CLK);
> udelay(1);
> }
>
> @@ -365,6 +368,27 @@ static int mt9v032_get_format(struct v4l2_subdev
> *subdev, return 0;
> }
>
> +static void mt9v032_configure_pixel_rate(struct v4l2_subdev *subdev,
> + unsigned int hratio)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> + struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> + struct v4l2_ext_controls ctrls;
> + struct v4l2_ext_control ctrl;
> +
> + memset(&ctrls, 0, sizeof(ctrls));
> + memset(&ctrl, 0, sizeof(ctrl));
> +
> + ctrls.count = 1;
> + ctrls.controls = &ctrl;
> +
> + ctrl.id = V4L2_CID_PIXEL_RATE;
> + ctrl.value64 = EXT_CLK / hratio;
> +
> + if (v4l2_s_ext_ctrls(mt9v032->pixel_rate->ctrl_handler, &ctrls) < 0)
> + dev_warn(&client->dev, "bug: failed to set pixel rate\n");
What about just calling v4l2_ctrl_s_ctrl() ?
> +}
> +
> static int mt9v032_set_format(struct v4l2_subdev *subdev,
> struct v4l2_subdev_fh *fh,
> struct v4l2_subdev_format *format)
> @@ -395,6 +419,8 @@ static int mt9v032_set_format(struct v4l2_subdev
> *subdev, format->which);
> __format->width = __crop->width / hratio;
> __format->height = __crop->height / vratio;
> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + mt9v032_configure_pixel_rate(subdev, hratio);
>
> format->format = *__format;
>
> @@ -450,6 +476,8 @@ static int mt9v032_set_crop(struct v4l2_subdev *subdev,
> crop->which);
> __format->width = rect.width;
> __format->height = rect.height;
> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + mt9v032_configure_pixel_rate(subdev, 1);
> }
>
> *__crop = rect;
> @@ -695,6 +723,9 @@ static int mt9v032_probe(struct i2c_client *client,
> V4L2_CID_EXPOSURE, MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
> MT9V032_TOTAL_SHUTTER_WIDTH_MAX, 1,
> MT9V032_TOTAL_SHUTTER_WIDTH_DEF);
> + mt9v032->pixel_rate =
> + v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
> + V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
Shouldn't you set the bounds to [EXT_CLK/4..EXT_CLK] ? Otherwise the set
control call will likely fail. We probably need a new control framework
function to modify the bounds.
>
> for (i = 0; i < ARRAY_SIZE(mt9v032_ctrls); ++i)
> v4l2_ctrl_new_custom(&mt9v032->ctrls, &mt9v032_ctrls[i], NULL);
> @@ -716,6 +747,8 @@ static int mt9v032_probe(struct i2c_client *client,
> mt9v032->format.field = V4L2_FIELD_NONE;
> mt9v032->format.colorspace = V4L2_COLORSPACE_SRGB;
>
> + mt9v032_configure_pixel_rate(subdev, 1);
> +
You could just initialize the control value to EXT_CLK when creating the
control.
> mt9v032->aec_agc = MT9V032_AEC_ENABLE | MT9V032_AGC_ENABLE;
>
> v4l2_i2c_subdev_init(&mt9v032->subdev, client, &mt9v032_subdev_ops);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] mt9v032: Provide pixel rate control
2012-03-16 11:58 ` Laurent Pinchart
@ 2012-03-16 12:12 ` Sakari Ailus
2012-03-16 12:31 ` Laurent Pinchart
0 siblings, 1 reply; 7+ messages in thread
From: Sakari Ailus @ 2012-03-16 12:12 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
Hi Laurent,
Thanks for the review!
On Fri, Mar 16, 2012 at 12:58:30PM +0100, Laurent Pinchart wrote:
> On Thursday 15 March 2012 23:01:39 Sakari Ailus wrote:
> > Provide pixel rate control calculated from external clock and horizontal
> > binning factor.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> > drivers/media/video/mt9v032.c | 35 ++++++++++++++++++++++++++++++++++-
> > 1 files changed, 34 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
> > index 75e253a..e530e8d 100644
> > --- a/drivers/media/video/mt9v032.c
> > +++ b/drivers/media/video/mt9v032.c
> > @@ -122,6 +122,7 @@ struct mt9v032 {
> > struct v4l2_mbus_framefmt format;
> > struct v4l2_rect crop;
> >
> > + struct v4l2_ctrl *pixel_rate;
> > struct v4l2_ctrl_handler ctrls;
> >
> > struct mutex power_lock;
> > @@ -187,13 +188,15 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16
> > which, int enable) return 0;
> > }
> >
> > +#define EXT_CLK 25000000
> > +
> > static int mt9v032_power_on(struct mt9v032 *mt9v032)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> > int ret;
> >
> > if (mt9v032->pdata->set_clock) {
> > - mt9v032->pdata->set_clock(&mt9v032->subdev, 25000000);
> > + mt9v032->pdata->set_clock(&mt9v032->subdev, EXT_CLK);
> > udelay(1);
> > }
> >
> > @@ -365,6 +368,27 @@ static int mt9v032_get_format(struct v4l2_subdev
> > *subdev, return 0;
> > }
> >
> > +static void mt9v032_configure_pixel_rate(struct v4l2_subdev *subdev,
> > + unsigned int hratio)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > + struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> > + struct v4l2_ext_controls ctrls;
> > + struct v4l2_ext_control ctrl;
> > +
> > + memset(&ctrls, 0, sizeof(ctrls));
> > + memset(&ctrl, 0, sizeof(ctrl));
> > +
> > + ctrls.count = 1;
> > + ctrls.controls = &ctrl;
> > +
> > + ctrl.id = V4L2_CID_PIXEL_RATE;
> > + ctrl.value64 = EXT_CLK / hratio;
> > +
> > + if (v4l2_s_ext_ctrls(mt9v032->pixel_rate->ctrl_handler, &ctrls) < 0)
> > + dev_warn(&client->dev, "bug: failed to set pixel rate\n");
>
> What about just calling v4l2_ctrl_s_ctrl() ?
It's a 64-bit integer control, so it has to be set using v4l2_s_ext_ctrls().
> > +}
> > +
> > static int mt9v032_set_format(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_fh *fh,
> > struct v4l2_subdev_format *format)
> > @@ -395,6 +419,8 @@ static int mt9v032_set_format(struct v4l2_subdev
> > *subdev, format->which);
> > __format->width = __crop->width / hratio;
> > __format->height = __crop->height / vratio;
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + mt9v032_configure_pixel_rate(subdev, hratio);
> >
> > format->format = *__format;
> >
> > @@ -450,6 +476,8 @@ static int mt9v032_set_crop(struct v4l2_subdev *subdev,
> > crop->which);
> > __format->width = rect.width;
> > __format->height = rect.height;
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + mt9v032_configure_pixel_rate(subdev, 1);
> > }
> >
> > *__crop = rect;
> > @@ -695,6 +723,9 @@ static int mt9v032_probe(struct i2c_client *client,
> > V4L2_CID_EXPOSURE, MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
> > MT9V032_TOTAL_SHUTTER_WIDTH_MAX, 1,
> > MT9V032_TOTAL_SHUTTER_WIDTH_DEF);
> > + mt9v032->pixel_rate =
> > + v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
> > + V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
>
> Shouldn't you set the bounds to [EXT_CLK/4..EXT_CLK] ? Otherwise the set
> control call will likely fail. We probably need a new control framework
> function to modify the bounds.
Same here. 64-bit controls don't have min/max.
> >
> > for (i = 0; i < ARRAY_SIZE(mt9v032_ctrls); ++i)
> > v4l2_ctrl_new_custom(&mt9v032->ctrls, &mt9v032_ctrls[i], NULL);
> > @@ -716,6 +747,8 @@ static int mt9v032_probe(struct i2c_client *client,
> > mt9v032->format.field = V4L2_FIELD_NONE;
> > mt9v032->format.colorspace = V4L2_COLORSPACE_SRGB;
> >
> > + mt9v032_configure_pixel_rate(subdev, 1);
> > +
>
> You could just initialize the control value to EXT_CLK when creating the
> control.
Good point; I'll do that and resend.
> > mt9v032->aec_agc = MT9V032_AEC_ENABLE | MT9V032_AGC_ENABLE;
> >
> > v4l2_i2c_subdev_init(&mt9v032->subdev, client, &mt9v032_subdev_ops);
Cheers,
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] mt9v032: Provide pixel rate control
2012-03-16 12:12 ` Sakari Ailus
@ 2012-03-16 12:31 ` Laurent Pinchart
2012-03-16 12:36 ` Sakari Ailus
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2012-03-16 12:31 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
On Friday 16 March 2012 14:12:11 Sakari Ailus wrote:
> On Fri, Mar 16, 2012 at 12:58:30PM +0100, Laurent Pinchart wrote:
> > On Thursday 15 March 2012 23:01:39 Sakari Ailus wrote:
> > > Provide pixel rate control calculated from external clock and horizontal
> > > binning factor.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > ---
> > >
> > > drivers/media/video/mt9v032.c | 35 ++++++++++++++++++++++++++++++++-
> > > 1 files changed, 34 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/media/video/mt9v032.c
> > > b/drivers/media/video/mt9v032.c
> > > index 75e253a..e530e8d 100644
> > > --- a/drivers/media/video/mt9v032.c
> > > +++ b/drivers/media/video/mt9v032.c
[snip]
> > > +static void mt9v032_configure_pixel_rate(struct v4l2_subdev *subdev,
> > > + unsigned int hratio)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > > + struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> > > + struct v4l2_ext_controls ctrls;
> > > + struct v4l2_ext_control ctrl;
> > > +
> > > + memset(&ctrls, 0, sizeof(ctrls));
> > > + memset(&ctrl, 0, sizeof(ctrl));
> > > +
> > > + ctrls.count = 1;
> > > + ctrls.controls = &ctrl;
> > > +
> > > + ctrl.id = V4L2_CID_PIXEL_RATE;
> > > + ctrl.value64 = EXT_CLK / hratio;
> > > +
> > > + if (v4l2_s_ext_ctrls(mt9v032->pixel_rate->ctrl_handler, &ctrls) < 0)
> > > + dev_warn(&client->dev, "bug: failed to set pixel rate\n");
> >
> > What about just calling v4l2_ctrl_s_ctrl() ?
>
> It's a 64-bit integer control, so it has to be set using v4l2_s_ext_ctrls().
What about extending v4l2_ctrl_s_ctrl() to support 64-bit integer controls
then ?
> > > +}
[snip]
> > > @@ -695,6 +723,9 @@ static int mt9v032_probe(struct i2c_client *client,
> > >
> > > V4L2_CID_EXPOSURE, MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
> > > MT9V032_TOTAL_SHUTTER_WIDTH_MAX, 1,
> > > MT9V032_TOTAL_SHUTTER_WIDTH_DEF);
> > >
> > > + mt9v032->pixel_rate =
> > > + v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
> > > + V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
> >
> > Shouldn't you set the bounds to [EXT_CLK/4..EXT_CLK] ? Otherwise the set
> > control call will likely fail. We probably need a new control framework
> > function to modify the bounds.
>
> Same here. 64-bit controls don't have min/max.
Right, my bad.
> > > for (i = 0; i < ARRAY_SIZE(mt9v032_ctrls); ++i)
> > >
> > > v4l2_ctrl_new_custom(&mt9v032->ctrls, &mt9v032_ctrls[i], NULL);
> > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] mt9v032: Provide pixel rate control
2012-03-16 12:31 ` Laurent Pinchart
@ 2012-03-16 12:36 ` Sakari Ailus
2012-03-16 12:55 ` Laurent Pinchart
0 siblings, 1 reply; 7+ messages in thread
From: Sakari Ailus @ 2012-03-16 12:36 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
Hi Laurent,
On Fri, Mar 16, 2012 at 01:31:39PM +0100, Laurent Pinchart wrote:
> On Friday 16 March 2012 14:12:11 Sakari Ailus wrote:
> > On Fri, Mar 16, 2012 at 12:58:30PM +0100, Laurent Pinchart wrote:
> > > On Thursday 15 March 2012 23:01:39 Sakari Ailus wrote:
> > > > Provide pixel rate control calculated from external clock and horizontal
> > > > binning factor.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > ---
> > > >
> > > > drivers/media/video/mt9v032.c | 35 ++++++++++++++++++++++++++++++++-
> > > > 1 files changed, 34 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/drivers/media/video/mt9v032.c
> > > > b/drivers/media/video/mt9v032.c
> > > > index 75e253a..e530e8d 100644
> > > > --- a/drivers/media/video/mt9v032.c
> > > > +++ b/drivers/media/video/mt9v032.c
>
> [snip]
>
> > > > +static void mt9v032_configure_pixel_rate(struct v4l2_subdev *subdev,
> > > > + unsigned int hratio)
> > > > +{
> > > > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > > > + struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> > > > + struct v4l2_ext_controls ctrls;
> > > > + struct v4l2_ext_control ctrl;
> > > > +
> > > > + memset(&ctrls, 0, sizeof(ctrls));
> > > > + memset(&ctrl, 0, sizeof(ctrl));
> > > > +
> > > > + ctrls.count = 1;
> > > > + ctrls.controls = &ctrl;
> > > > +
> > > > + ctrl.id = V4L2_CID_PIXEL_RATE;
> > > > + ctrl.value64 = EXT_CLK / hratio;
> > > > +
> > > > + if (v4l2_s_ext_ctrls(mt9v032->pixel_rate->ctrl_handler, &ctrls) < 0)
> > > > + dev_warn(&client->dev, "bug: failed to set pixel rate\n");
> > >
> > > What about just calling v4l2_ctrl_s_ctrl() ?
> >
> > It's a 64-bit integer control, so it has to be set using v4l2_s_ext_ctrls().
>
> What about extending v4l2_ctrl_s_ctrl() to support 64-bit integer controls
> then ?
The second argument to that function is struct v4l2_control:
struct v4l2_control {
__u32 id;
__s32 value;
};
So there's no chance to extend it. This must also be the reason why 64-bit
controls require using extended controls.
What we could do is to introduce v4l2_s_ext_ctrl without the "s" in the end
to just set one extended control. I think that would be a reasonable
approach, as it seems we need the functionality in multiple places.
Regards,
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] mt9v032: Provide pixel rate control
2012-03-16 12:36 ` Sakari Ailus
@ 2012-03-16 12:55 ` Laurent Pinchart
2012-03-16 13:07 ` Sakari Ailus
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2012-03-16 12:55 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
Hi Sakari,
On Friday 16 March 2012 14:36:53 Sakari Ailus wrote:
> On Fri, Mar 16, 2012 at 01:31:39PM +0100, Laurent Pinchart wrote:
> > On Friday 16 March 2012 14:12:11 Sakari Ailus wrote:
> > > On Fri, Mar 16, 2012 at 12:58:30PM +0100, Laurent Pinchart wrote:
> > > > On Thursday 15 March 2012 23:01:39 Sakari Ailus wrote:
> > > > > Provide pixel rate control calculated from external clock and
> > > > > horizontal
> > > > > binning factor.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > ---
> > > > >
> > > > > drivers/media/video/mt9v032.c | 35
> > > > > ++++++++++++++++++++++++++++++++-
> > > > > 1 files changed, 34 insertions(+), 1 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/video/mt9v032.c
> > > > > b/drivers/media/video/mt9v032.c
> > > > > index 75e253a..e530e8d 100644
> > > > > --- a/drivers/media/video/mt9v032.c
> > > > > +++ b/drivers/media/video/mt9v032.c
> >
> > [snip]
> >
> > > > > +static void mt9v032_configure_pixel_rate(struct v4l2_subdev
> > > > > *subdev,
> > > > > + unsigned int hratio)
> > > > > +{
> > > > > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > > > > + struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> > > > > + struct v4l2_ext_controls ctrls;
> > > > > + struct v4l2_ext_control ctrl;
> > > > > +
> > > > > + memset(&ctrls, 0, sizeof(ctrls));
> > > > > + memset(&ctrl, 0, sizeof(ctrl));
> > > > > +
> > > > > + ctrls.count = 1;
> > > > > + ctrls.controls = &ctrl;
> > > > > +
> > > > > + ctrl.id = V4L2_CID_PIXEL_RATE;
> > > > > + ctrl.value64 = EXT_CLK / hratio;
> > > > > +
> > > > > + if (v4l2_s_ext_ctrls(mt9v032->pixel_rate->ctrl_handler, &ctrls)
<
> > > > > 0)
> > > > > + dev_warn(&client->dev, "bug: failed to set pixel rate\n");
> > > >
> > > > What about just calling v4l2_ctrl_s_ctrl() ?
> > >
> > > It's a 64-bit integer control, so it has to be set using
> > > v4l2_s_ext_ctrls().>
> > What about extending v4l2_ctrl_s_ctrl() to support 64-bit integer controls
> > then ?
>
> The second argument to that function is struct v4l2_control:
>
> struct v4l2_control {
> __u32 id;
> __s32 value;
> };
>
> So there's no chance to extend it. This must also be the reason why 64-bit
> controls require using extended controls.
>
> What we could do is to introduce v4l2_s_ext_ctrl without the "s" in the end
> to just set one extended control. I think that would be a reasonable
> approach, as it seems we need the functionality in multiple places.
We're talking about different functions.
int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
Extending that one wouldn't break the userspace API. We could use s64 instead
of s32, or add a new function such as v4l2_ctrl_s_ctrl64().
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] mt9v032: Provide pixel rate control
2012-03-16 12:55 ` Laurent Pinchart
@ 2012-03-16 13:07 ` Sakari Ailus
0 siblings, 0 replies; 7+ messages in thread
From: Sakari Ailus @ 2012-03-16 13:07 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
On Fri, Mar 16, 2012 at 01:55:13PM +0100, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Friday 16 March 2012 14:36:53 Sakari Ailus wrote:
> > On Fri, Mar 16, 2012 at 01:31:39PM +0100, Laurent Pinchart wrote:
> > > On Friday 16 March 2012 14:12:11 Sakari Ailus wrote:
> > > > On Fri, Mar 16, 2012 at 12:58:30PM +0100, Laurent Pinchart wrote:
> > > > > On Thursday 15 March 2012 23:01:39 Sakari Ailus wrote:
> > > > > > Provide pixel rate control calculated from external clock and
> > > > > > horizontal
> > > > > > binning factor.
> > > > > >
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > > ---
> > > > > >
> > > > > > drivers/media/video/mt9v032.c | 35
> > > > > > ++++++++++++++++++++++++++++++++-
> > > > > > 1 files changed, 34 insertions(+), 1 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/video/mt9v032.c
> > > > > > b/drivers/media/video/mt9v032.c
> > > > > > index 75e253a..e530e8d 100644
> > > > > > --- a/drivers/media/video/mt9v032.c
> > > > > > +++ b/drivers/media/video/mt9v032.c
> > >
> > > [snip]
> > >
> > > > > > +static void mt9v032_configure_pixel_rate(struct v4l2_subdev
> > > > > > *subdev,
> > > > > > + unsigned int hratio)
> > > > > > +{
> > > > > > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > > > > > + struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> > > > > > + struct v4l2_ext_controls ctrls;
> > > > > > + struct v4l2_ext_control ctrl;
> > > > > > +
> > > > > > + memset(&ctrls, 0, sizeof(ctrls));
> > > > > > + memset(&ctrl, 0, sizeof(ctrl));
> > > > > > +
> > > > > > + ctrls.count = 1;
> > > > > > + ctrls.controls = &ctrl;
> > > > > > +
> > > > > > + ctrl.id = V4L2_CID_PIXEL_RATE;
> > > > > > + ctrl.value64 = EXT_CLK / hratio;
> > > > > > +
> > > > > > + if (v4l2_s_ext_ctrls(mt9v032->pixel_rate->ctrl_handler, &ctrls)
> <
> > > > > > 0)
> > > > > > + dev_warn(&client->dev, "bug: failed to set pixel rate\n");
> > > > >
> > > > > What about just calling v4l2_ctrl_s_ctrl() ?
> > > >
> > > > It's a 64-bit integer control, so it has to be set using
> > > > v4l2_s_ext_ctrls().>
> > > What about extending v4l2_ctrl_s_ctrl() to support 64-bit integer controls
> > > then ?
> >
> > The second argument to that function is struct v4l2_control:
> >
> > struct v4l2_control {
> > __u32 id;
> > __s32 value;
> > };
> >
> > So there's no chance to extend it. This must also be the reason why 64-bit
> > controls require using extended controls.
> >
> > What we could do is to introduce v4l2_s_ext_ctrl without the "s" in the end
> > to just set one extended control. I think that would be a reasonable
> > approach, as it seems we need the functionality in multiple places.
>
> We're talking about different functions.
Oh, I noticed that now. :-)
> int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
>
> Extending that one wouldn't break the userspace API. We could use s64 instead
> of s32, or add a new function such as v4l2_ctrl_s_ctrl64().
I think I'd go for another function.
Regards,
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-03-16 13:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-15 21:01 [PATCH 1/1] mt9v032: Provide pixel rate control Sakari Ailus
2012-03-16 11:58 ` Laurent Pinchart
2012-03-16 12:12 ` Sakari Ailus
2012-03-16 12:31 ` Laurent Pinchart
2012-03-16 12:36 ` Sakari Ailus
2012-03-16 12:55 ` Laurent Pinchart
2012-03-16 13:07 ` Sakari Ailus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.