* [PATCH] mt9v032: Correct the logic for the auto-exposure setting
@ 2012-05-02 22:19 Kartik Mohta
2012-05-08 11:12 ` Laurent Pinchart
0 siblings, 1 reply; 4+ messages in thread
From: Kartik Mohta @ 2012-05-02 22:19 UTC (permalink / raw)
To: linux-media; +Cc: Kartik Mohta
The driver uses the ctrl value passed in as a bool to determine whether
to enable auto-exposure, but the auto-exposure setting is defined as an
enum where AUTO has a value of 0 and MANUAL has a value of 1. This leads
to a reversed logic where if you send in AUTO, it actually sets manual
exposure and vice-versa.
Signed-off-by: Kartik Mohta <kartikmohta@gmail.com>
---
drivers/media/video/mt9v032.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
index 75e253a..8ea8737 100644
--- a/drivers/media/video/mt9v032.c
+++ b/drivers/media/video/mt9v032.c
@@ -470,6 +470,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
container_of(ctrl->handler, struct mt9v032, ctrls);
struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
u16 data;
+ int aec_value;
switch (ctrl->id) {
case V4L2_CID_AUTOGAIN:
@@ -480,8 +481,13 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
return mt9v032_write(client, MT9V032_ANALOG_GAIN, ctrl->val);
case V4L2_CID_EXPOSURE_AUTO:
+ if(ctrl->val == V4L2_EXPOSURE_MANUAL)
+ aec_value = 0;
+ else
+ aec_value = 1;
+
return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
- ctrl->val);
+ aec_value);
case V4L2_CID_EXPOSURE:
return mt9v032_write(client, MT9V032_TOTAL_SHUTTER_WIDTH,
--
1.7.10.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] mt9v032: Correct the logic for the auto-exposure setting
2012-05-02 22:19 [PATCH] mt9v032: Correct the logic for the auto-exposure setting Kartik Mohta
@ 2012-05-08 11:12 ` Laurent Pinchart
2012-05-08 15:51 ` Kartik Mohta
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2012-05-08 11:12 UTC (permalink / raw)
To: Kartik Mohta; +Cc: linux-media
Hi Kartik,
Thank you for the patch.
On Wednesday 02 May 2012 18:19:08 Kartik Mohta wrote:
> The driver uses the ctrl value passed in as a bool to determine whether
> to enable auto-exposure, but the auto-exposure setting is defined as an
> enum where AUTO has a value of 0 and MANUAL has a value of 1. This leads
> to a reversed logic where if you send in AUTO, it actually sets manual
> exposure and vice-versa.
>
> Signed-off-by: Kartik Mohta <kartikmohta@gmail.com>
> ---
> drivers/media/video/mt9v032.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
> index 75e253a..8ea8737 100644
> --- a/drivers/media/video/mt9v032.c
> +++ b/drivers/media/video/mt9v032.c
> @@ -470,6 +470,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
> container_of(ctrl->handler, struct mt9v032, ctrls);
> struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> u16 data;
> + int aec_value;
>
> switch (ctrl->id) {
> case V4L2_CID_AUTOGAIN:
> @@ -480,8 +481,13 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
> return mt9v032_write(client, MT9V032_ANALOG_GAIN, ctrl->val);
>
> case V4L2_CID_EXPOSURE_AUTO:
> + if(ctrl->val == V4L2_EXPOSURE_MANUAL)
> + aec_value = 0;
> + else
> + aec_value = 1;
> +
> return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
> - ctrl->val);
> + aec_value);
What about just
return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
!ctrl->val);
If you're fine with that change I'll modify the patch accordingly, there's no
need to resubmit (I'll of course keep the patch attribution).
>
> case V4L2_CID_EXPOSURE:
> return mt9v032_write(client, MT9V032_TOTAL_SHUTTER_WIDTH,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mt9v032: Correct the logic for the auto-exposure setting
2012-05-08 11:12 ` Laurent Pinchart
@ 2012-05-08 15:51 ` Kartik Mohta
2012-05-08 17:30 ` Laurent Pinchart
0 siblings, 1 reply; 4+ messages in thread
From: Kartik Mohta @ 2012-05-08 15:51 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
Hi,
On Tue, May 8, 2012 at 7:12 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Kartik,
>
> Thank you for the patch.
>
> On Wednesday 02 May 2012 18:19:08 Kartik Mohta wrote:
>> The driver uses the ctrl value passed in as a bool to determine whether
>> to enable auto-exposure, but the auto-exposure setting is defined as an
>> enum where AUTO has a value of 0 and MANUAL has a value of 1. This leads
>> to a reversed logic where if you send in AUTO, it actually sets manual
>> exposure and vice-versa.
>>
>> Signed-off-by: Kartik Mohta <kartikmohta@gmail.com>
>> ---
>> drivers/media/video/mt9v032.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
>> index 75e253a..8ea8737 100644
>> --- a/drivers/media/video/mt9v032.c
>> +++ b/drivers/media/video/mt9v032.c
>> @@ -470,6 +470,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
>> container_of(ctrl->handler, struct mt9v032, ctrls);
>> struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
>> u16 data;
>> + int aec_value;
>>
>> switch (ctrl->id) {
>> case V4L2_CID_AUTOGAIN:
>> @@ -480,8 +481,13 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
>> return mt9v032_write(client, MT9V032_ANALOG_GAIN, ctrl->val);
>>
>> case V4L2_CID_EXPOSURE_AUTO:
>> + if(ctrl->val == V4L2_EXPOSURE_MANUAL)
>> + aec_value = 0;
>> + else
>> + aec_value = 1;
>> +
>> return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
>> - ctrl->val);
>> + aec_value);
>
> What about just
>
> return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
> !ctrl->val);
>
> If you're fine with that change I'll modify the patch accordingly, there's no
> need to resubmit (I'll of course keep the patch attribution).
>
That should work since the only supported exposure modes are auto and
manual with enum values 0 and 1 respectively, but then aren't you
depending on the values of the enum to not change in the future? Also
the change gives an impression that the value is a bool which it is
not. If that is fine, you can change it.
--
Kartik
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mt9v032: Correct the logic for the auto-exposure setting
2012-05-08 15:51 ` Kartik Mohta
@ 2012-05-08 17:30 ` Laurent Pinchart
0 siblings, 0 replies; 4+ messages in thread
From: Laurent Pinchart @ 2012-05-08 17:30 UTC (permalink / raw)
To: Kartik Mohta; +Cc: linux-media
Hi Kartik,
On Tuesday 08 May 2012 11:51:29 Kartik Mohta wrote:
> On Tue, May 8, 2012 at 7:12 AM, Laurent Pinchart wrote:
> > On Wednesday 02 May 2012 18:19:08 Kartik Mohta wrote:
> >> The driver uses the ctrl value passed in as a bool to determine whether
> >> to enable auto-exposure, but the auto-exposure setting is defined as an
> >> enum where AUTO has a value of 0 and MANUAL has a value of 1. This leads
> >> to a reversed logic where if you send in AUTO, it actually sets manual
> >> exposure and vice-versa.
> >>
> >> Signed-off-by: Kartik Mohta <kartikmohta@gmail.com>
> >> ---
> >> drivers/media/video/mt9v032.c | 8 +++++++-
> >> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/video/mt9v032.c
> >> b/drivers/media/video/mt9v032.c
> >> index 75e253a..8ea8737 100644
> >> --- a/drivers/media/video/mt9v032.c
> >> +++ b/drivers/media/video/mt9v032.c
> >> @@ -470,6 +470,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
> >> container_of(ctrl->handler, struct mt9v032, ctrls);
> >> struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> >> u16 data;
> >> + int aec_value;
> >>
> >> switch (ctrl->id) {
> >> case V4L2_CID_AUTOGAIN:
> >> @@ -480,8 +481,13 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
> >> return mt9v032_write(client, MT9V032_ANALOG_GAIN,
> >> ctrl->val);
> >>
> >> case V4L2_CID_EXPOSURE_AUTO:
> >> + if(ctrl->val == V4L2_EXPOSURE_MANUAL)
> >> + aec_value = 0;
> >> + else
> >> + aec_value = 1;
> >> +
> >> return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
> >> - ctrl->val);
> >> + aec_value);
> >
> > What about just
> >
> > return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
> > !ctrl->val);
> >
> > If you're fine with that change I'll modify the patch accordingly, there's
> > no need to resubmit (I'll of course keep the patch attribution).
>
> That should work since the only supported exposure modes are auto and manual
> with enum values 0 and 1 respectively, but then aren't you depending on the
> values of the enum to not change in the future?
The values are part of the V4L2 public API so they can't change.
> Also the change gives an impression that the value is a bool which it is
> not. If that is fine, you can change it.
OK thank you.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-05-08 17:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-02 22:19 [PATCH] mt9v032: Correct the logic for the auto-exposure setting Kartik Mohta
2012-05-08 11:12 ` Laurent Pinchart
2012-05-08 15:51 ` Kartik Mohta
2012-05-08 17:30 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox