Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[DaeSeok Youn]

2017-03-20 22:11 GMT+09:00 walter harms :
>
>
> Am 20.03.2017 13:51, schrieb DaeSeok Youn:
>> 2017-03-20 21:04 GMT+09:00 walter harms :
>>>
>>>
>>> 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 
 ---
  .../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, _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, );
 +
 + 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
>>>

>>

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[walter harms]

Am 20.03.2017 13:51, schrieb DaeSeok Youn:
> 2017-03-20 21:04 GMT+09:00 walter harms :
>>
>>
>> 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 
>>> ---
>>>  .../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, _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, );
>>> +
>>> + 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
>>
>>>
> 

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[DaeSeok Youn]

2017-03-20 21:04 GMT+09:00 walter harms :
>
>
> 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 
>> ---
>>  .../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, _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, );
>> +
>> + 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
>
>>

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[walter harms]

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 
> ---
>  .../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, _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, );
> +
> + 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

>