Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754332AbdCTNN7 (ORCPT ); Mon, 20 Mar 2017 09:13:59 -0400 Received: from mx02-sz.bfs.de ([194.94.69.103]:44778 "EHLO mx02-sz.bfs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753304AbdCTNMm (ORCPT ); Mon, 20 Mar 2017 09:12:42 -0400 Message-ID: <58CFD505.60201@bfs.de> Date: Mon, 20 Mar 2017 14:11:33 +0100 From: walter harms Reply-To: wharms@bfs.de User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.1.16) Gecko/20101125 SUSE/3.0.11 Thunderbird/3.0.11 MIME-Version: 1.0 To: DaeSeok Youn CC: mchehab@kernel.org, Greg KH , Alan Cox , SIMRAN SINGHAL , Dan Carpenter , linux-media@vger.kernel.org, devel , linux-kernel , kernel-janitors Subject: Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps() References: <20170320105940.GA17472@SEL-JYOUN-D1> <58CFC561.8090104@bfs.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3051 Lines: 86 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, &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 >> >>> >