* [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()
@ 2017-03-20 10:59 Daeseok Youn
2017-03-20 12:04 ` walter harms
0 siblings, 1 reply; 5+ messages in thread
From: Daeseok Youn @ 2017-03-20 10:59 UTC (permalink / raw)
To: mchehab
Cc: gregkh, daeseok.youn, alan, singhalsimran0, dan.carpenter,
linux-media, devel, linux-kernel, kernel-janitors
If v4l2_subdev_call() gets the global frame interval values,
it returned 0 and it could be checked whether numerator is zero or not.
If the numerator is not zero, the fps could be calculated in this function.
If not, it just returns 0.
Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
---
.../media/atomisp/pci/atomisp2/atomisp_cmd.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 8bdb224..6bdd19e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct video_device *dev)
static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
{
- struct v4l2_subdev_frame_interval frame_interval;
+ struct v4l2_subdev_frame_interval fi;
struct atomisp_device *isp = asd->isp;
- unsigned short fps;
- if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
- video, g_frame_interval, &frame_interval)) {
- fps = 0;
- } else {
- if (frame_interval.interval.numerator)
- fps = frame_interval.interval.denominator /
- frame_interval.interval.numerator;
- else
- fps = 0;
- }
+ unsigned short fps = 0;
+ int ret;
+
+ ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
+ video, g_frame_interval, &fi);
+
+ if (!ret && fi.interval.numerator)
+ fps = fi.interval.denominator / fi.interval.numerator;
+
return fps;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()
2017-03-20 10:59 [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps() Daeseok Youn
@ 2017-03-20 12:04 ` walter harms
2017-03-20 12:51 ` DaeSeok Youn
0 siblings, 1 reply; 5+ messages in thread
From: walter harms @ 2017-03-20 12:04 UTC (permalink / raw)
To: Daeseok Youn
Cc: mchehab, gregkh, alan, singhalsimran0, dan.carpenter, linux-media,
devel, linux-kernel, kernel-janitors
Am 20.03.2017 11:59, schrieb Daeseok Youn:
> If v4l2_subdev_call() gets the global frame interval values,
> it returned 0 and it could be checked whether numerator is zero or not.
>
> If the numerator is not zero, the fps could be calculated in this function.
> If not, it just returns 0.
>
> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
> ---
> .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index 8bdb224..6bdd19e 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct video_device *dev)
>
> static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
> {
> - struct v4l2_subdev_frame_interval frame_interval;
> + struct v4l2_subdev_frame_interval fi;
> struct atomisp_device *isp = asd->isp;
> - unsigned short fps;
>
> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> - video, g_frame_interval, &frame_interval)) {
> - fps = 0;
> - } else {
> - if (frame_interval.interval.numerator)
> - fps = frame_interval.interval.denominator /
> - frame_interval.interval.numerator;
> - else
> - fps = 0;
> - }
> + unsigned short fps = 0;
> + int ret;
> +
> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> + video, g_frame_interval, &fi);
> +
> + if (!ret && fi.interval.numerator)
> + fps = fi.interval.denominator / fi.interval.numerator;
> +
> return fps;
> }
do you need to check ret at all ? if an error occurs can fi.interval.numerator
be something else than 0 ?
if ret is an ERRNO it would be wise to return ret not fps, but this may require
changes at other places also.
re,
wh
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()
2017-03-20 12:04 ` walter harms
@ 2017-03-20 12:51 ` DaeSeok Youn
2017-03-20 13:11 ` walter harms
0 siblings, 1 reply; 5+ messages in thread
From: DaeSeok Youn @ 2017-03-20 12:51 UTC (permalink / raw)
To: wharms
Cc: mchehab, Greg KH, Alan Cox, SIMRAN SINGHAL, Dan Carpenter,
linux-media, devel, linux-kernel, kernel-janitors
2017-03-20 21:04 GMT+09:00 walter harms <wharms@bfs.de>:
>
>
> Am 20.03.2017 11:59, schrieb Daeseok Youn:
>> If v4l2_subdev_call() gets the global frame interval values,
>> it returned 0 and it could be checked whether numerator is zero or not.
>>
>> If the numerator is not zero, the fps could be calculated in this function.
>> If not, it just returns 0.
>>
>> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>> ---
>> .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 22 ++++++++++------------
>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> index 8bdb224..6bdd19e 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct video_device *dev)
>>
>> static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
>> {
>> - struct v4l2_subdev_frame_interval frame_interval;
>> + struct v4l2_subdev_frame_interval fi;
>> struct atomisp_device *isp = asd->isp;
>> - unsigned short fps;
>>
>> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>> - video, g_frame_interval, &frame_interval)) {
>> - fps = 0;
>> - } else {
>> - if (frame_interval.interval.numerator)
>> - fps = frame_interval.interval.denominator /
>> - frame_interval.interval.numerator;
>> - else
>> - fps = 0;
>> - }
>> + unsigned short fps = 0;
>> + int ret;
>> +
>> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>> + video, g_frame_interval, &fi);
>> +
>> + if (!ret && fi.interval.numerator)
>> + fps = fi.interval.denominator / fi.interval.numerator;
>> +
>> return fps;
>> }
>
>
>
> do you need to check ret at all ? if an error occurs can fi.interval.numerator
> be something else than 0 ?
the return value from the v4l2_subdev_call() function is zero when it
is done without any error. and also I checked
the ret value whether is 0 or not. if the ret is 0 then the value of
numerator should be checked to avoid for dividing by 0.
>
> if ret is an ERRNO it would be wise to return ret not fps, but this may require
> changes at other places also.
hmm.., yes, you are right. but I think it is ok because the
atomisp_get_sensor_fps() function is needed to get fps value.
(originally, zero or calculated fps value was returned.)
>
> re,
> wh
>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()
2017-03-20 12:51 ` DaeSeok Youn
@ 2017-03-20 13:11 ` walter harms
2017-03-21 1:43 ` DaeSeok Youn
0 siblings, 1 reply; 5+ messages in thread
From: walter harms @ 2017-03-20 13:11 UTC (permalink / raw)
To: DaeSeok Youn
Cc: mchehab, Greg KH, Alan Cox, SIMRAN SINGHAL, Dan Carpenter,
linux-media, devel, linux-kernel, kernel-janitors
Am 20.03.2017 13:51, schrieb DaeSeok Youn:
> 2017-03-20 21:04 GMT+09:00 walter harms <wharms@bfs.de>:
>>
>>
>> Am 20.03.2017 11:59, schrieb Daeseok Youn:
>>> If v4l2_subdev_call() gets the global frame interval values,
>>> it returned 0 and it could be checked whether numerator is zero or not.
>>>
>>> If the numerator is not zero, the fps could be calculated in this function.
>>> If not, it just returns 0.
>>>
>>> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>>> ---
>>> .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 22 ++++++++++------------
>>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> index 8bdb224..6bdd19e 100644
>>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct video_device *dev)
>>>
>>> static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
>>> {
>>> - struct v4l2_subdev_frame_interval frame_interval;
>>> + struct v4l2_subdev_frame_interval fi;
>>> struct atomisp_device *isp = asd->isp;
>>> - unsigned short fps;
>>>
>>> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>> - video, g_frame_interval, &frame_interval)) {
>>> - fps = 0;
>>> - } else {
>>> - if (frame_interval.interval.numerator)
>>> - fps = frame_interval.interval.denominator /
>>> - frame_interval.interval.numerator;
>>> - else
>>> - fps = 0;
>>> - }
>>> + unsigned short fps = 0;
>>> + int ret;
>>> +
>>> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>> + video, g_frame_interval, &fi);
>>> +
>>> + if (!ret && fi.interval.numerator)
>>> + fps = fi.interval.denominator / fi.interval.numerator;
>>> +
>>> return fps;
>>> }
>>
>>
>>
>> do you need to check ret at all ? if an error occurs can fi.interval.numerator
>> be something else than 0 ?
> the return value from the v4l2_subdev_call() function is zero when it
> is done without any error. and also I checked
> the ret value whether is 0 or not. if the ret is 0 then the value of
> numerator should be checked to avoid for dividing by 0.
>>
>> if ret is an ERRNO it would be wise to return ret not fps, but this may require
>> changes at other places also.
> hmm.., yes, you are right. but I think it is ok because the
> atomisp_get_sensor_fps() function is needed to get fps value.
> (originally, zero or calculated fps value was returned.)
maybe its better to divide this in:
if (ret)
return 0; // error case
return (fi.interval.numerator>0)?fi.interval.denominator / fi.interval.numerator:0;
So there is a chance that someone will a) understand and b) fix the error return.
re,
wh
>
>>
>> re,
>> wh
>>
>>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()
2017-03-20 13:11 ` walter harms
@ 2017-03-21 1:43 ` DaeSeok Youn
0 siblings, 0 replies; 5+ messages in thread
From: DaeSeok Youn @ 2017-03-21 1:43 UTC (permalink / raw)
To: wharms
Cc: mchehab, Greg KH, Alan Cox, SIMRAN SINGHAL, Dan Carpenter,
linux-media, devel, linux-kernel, kernel-janitors
2017-03-20 22:11 GMT+09:00 walter harms <wharms@bfs.de>:
>
>
> Am 20.03.2017 13:51, schrieb DaeSeok Youn:
>> 2017-03-20 21:04 GMT+09:00 walter harms <wharms@bfs.de>:
>>>
>>>
>>> Am 20.03.2017 11:59, schrieb Daeseok Youn:
>>>> If v4l2_subdev_call() gets the global frame interval values,
>>>> it returned 0 and it could be checked whether numerator is zero or not.
>>>>
>>>> If the numerator is not zero, the fps could be calculated in this function.
>>>> If not, it just returns 0.
>>>>
>>>> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>>>> ---
>>>> .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 22 ++++++++++------------
>>>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>>> index 8bdb224..6bdd19e 100644
>>>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>>> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct video_device *dev)
>>>>
>>>> static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
>>>> {
>>>> - struct v4l2_subdev_frame_interval frame_interval;
>>>> + struct v4l2_subdev_frame_interval fi;
>>>> struct atomisp_device *isp = asd->isp;
>>>> - unsigned short fps;
>>>>
>>>> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>>> - video, g_frame_interval, &frame_interval)) {
>>>> - fps = 0;
>>>> - } else {
>>>> - if (frame_interval.interval.numerator)
>>>> - fps = frame_interval.interval.denominator /
>>>> - frame_interval.interval.numerator;
>>>> - else
>>>> - fps = 0;
>>>> - }
>>>> + unsigned short fps = 0;
>>>> + int ret;
>>>> +
>>>> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>>> + video, g_frame_interval, &fi);
>>>> +
>>>> + if (!ret && fi.interval.numerator)
>>>> + fps = fi.interval.denominator / fi.interval.numerator;
>>>> +
>>>> return fps;
>>>> }
>>>
>>>
>>>
>>> do you need to check ret at all ? if an error occurs can fi.interval.numerator
>>> be something else than 0 ?
>> the return value from the v4l2_subdev_call() function is zero when it
>> is done without any error. and also I checked
>> the ret value whether is 0 or not. if the ret is 0 then the value of
>> numerator should be checked to avoid for dividing by 0.
>>>
>>> if ret is an ERRNO it would be wise to return ret not fps, but this may require
>>> changes at other places also.
>> hmm.., yes, you are right. but I think it is ok because the
>> atomisp_get_sensor_fps() function is needed to get fps value.
>> (originally, zero or calculated fps value was returned.)
>
> maybe its better to divide this in:
> if (ret)
> return 0; // error case
>
> return (fi.interval.numerator>0)?fi.interval.denominator / fi.interval.numerator:0;
>
> So there is a chance that someone will a) understand and b) fix the error return.
yes, it looks better than mine. I will update it and resend it.
Thanks walter,
Regards,
Daeseok Youn.
>
> re,
> wh
>
>>
>>>
>>> re,
>>> wh
>>>
>>>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-21 1:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-20 10:59 [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps() Daeseok Youn
2017-03-20 12:04 ` walter harms
2017-03-20 12:51 ` DaeSeok Youn
2017-03-20 13:11 ` walter harms
2017-03-21 1:43 ` DaeSeok Youn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).