Hi Martiros,
On 15/04/2021 02:51, Martiros Shakhzadyan wrote:
> Replace multiple return statements with goto in ia_css_init(), matching
> other functions.
>
> Signed-off-by: Martiros Shakhzadyan <vrzh@xxxxxxxx>
> ---
> drivers/staging/media/atomisp/pci/sh_css.c | 45 +++++++++-------------
> 1 file changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
> index bb752d47457c..4e3ef68014ec 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> @@ -1695,10 +1695,8 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> my_css.flush = flush_func;
>
> err = ia_css_rmgr_init();
> - if (err) {
> - IA_CSS_LEAVE_ERR(err);
> - return err;
> - }
> + if (err)
> + goto ERR;
Sorry, this doesn't work.
First a style issue: goto label are typically lowercase, not uppercase.
I do see that elsewhere in this source they use ERR as well, but that should
really all be changed to lowercase.
But more importantly, if you look up the definition of IA_CSS_LEAVE_ERR
you see:
#define IA_CSS_LEAVE_ERR(__err) \
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, \
"%s() %d: leave: return_err=%d\n", __func__, __LINE__, __err)
I.e., it is used to debug the code and print where the error is returned
(__func__ and __LINE__). By moving this to the end, the __LINE__ is always
the same for all 'error' cases, thus defeating the purpose of this debug line.
So I won't take this patch.
Regards,
Hans
>
> IA_CSS_LOG("init: %d", my_css_save_initialized);
>
> @@ -1730,27 +1728,23 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> gpio_reg_store(GPIO0_ID, _gpio_block_reg_do_0, 0);
>
> err = ia_css_refcount_init(REFCOUNT_SIZE);
> - if (err) {
> - IA_CSS_LEAVE_ERR(err);
> - return err;
> - }
> + if (err)
> + goto ERR;
> +
> err = sh_css_params_init();
> - if (err) {
> - IA_CSS_LEAVE_ERR(err);
> - return err;
> - }
> + if (err)
> + goto ERR;
> +
> if (fw) {
> ia_css_unload_firmware(); /* in case we already had firmware loaded */
> err = sh_css_load_firmware(dev, fw->data, fw->bytes);
> - if (err) {
> - IA_CSS_LEAVE_ERR(err);
> - return err;
> - }
> + if (err)
> + goto ERR;
> +
> err = ia_css_binary_init_infos();
> - if (err) {
> - IA_CSS_LEAVE_ERR(err);
> - return err;
> - }
> + if (err)
> + goto ERR;
> +
> fw_explicitly_loaded = false;
> #ifndef ISP2401
> my_css_save.loaded_fw = (struct ia_css_fw *)fw;
> @@ -1760,10 +1754,8 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> return -EINVAL;
>
> err = ia_css_spctrl_load_fw(SP0_ID, &spctrl_cfg);
> - if (err) {
> - IA_CSS_LEAVE_ERR(err);
> - return err;
> - }
> + if (err)
> + goto ERR;
>
> #if WITH_PC_MONITORING
> if (!thread_alive) {
> @@ -1774,8 +1766,8 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> }
> #endif
> if (!sh_css_hrt_system_is_idle()) {
> - IA_CSS_LEAVE_ERR(-EBUSY);
> - return -EBUSY;
> + err = -EBUSY;
> + goto ERR;
> }
> /* can be called here, queuing works, but:
> - when sp is started later, it will wipe queued items
> @@ -1801,6 +1793,7 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
>
> sh_css_params_map_and_store_default_gdc_lut();
>
> +ERR:
> IA_CSS_LEAVE_ERR(err);
> return err;
> }
>
