Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1317058 > unrolled thread

Re: [PATCH] [media] xc5000: Faster result reporting in xc_load_fw_and_init_tuner()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] [media] xc5000: Faster result reporting in  xc_load_fw_and_init_tuner() Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-01-25 18:10 +0100
    Re: [PATCH] [media] xc5000: Faster result reporting in  xc_load_fw_and_init_tuner() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-25 19:30 +0100
      Re: [PATCH] [media] xc5000: Faster result reporting in xc_load_fw_and_init_tuner() Devin Heitmueller <dheitmueller@kernellabs.com> - 2016-01-25 19:40 +0100

#1317058 — Re: [PATCH] [media] xc5000: Faster result reporting in xc_load_fw_and_init_tuner()

Em Mon, 28 Dec 2015 20:20:27 +0100
SF Markus Elfring <elfring@users.sourceforge.net> escreveu:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 28 Dec 2015 20:10:30 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> Split the previous if statement at the end so that each final log statement
> will eventually be performed by a direct jump to these labels.
> * report_failure
> * report_success
> 
> A check repetition can be excluded for the variable "ret" at the end then.
> 
> 
> Apply also two recommendations from the script "checkpatch.pl".
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/tuners/xc5000.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
> index e6e5e90..1360677 100644
> --- a/drivers/media/tuners/xc5000.c
> +++ b/drivers/media/tuners/xc5000.c
> @@ -1166,7 +1166,7 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
>  
>  		ret = xc5000_fwupload(fe, desired_fw, fw);
>  		if (ret != 0)
> -			goto err;
> +			goto report_failure;
>  
>  		msleep(20);
>  
> @@ -1229,18 +1229,16 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
>  		/* Default to "CABLE" mode */
>  		ret = xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE);
>  		if (!ret)
> -			break;
> +			goto report_success;
>  		printk(KERN_ERR "xc5000: can't set to cable mode.");

It sounds worth to avoid adding a goto here.

>  	}
>  
> -err:
> -	if (!ret)
> -		printk(KERN_INFO "xc5000: Firmware %s loaded and running.\n",
> -		       desired_fw->name);
> -	else
> -		printk(KERN_CONT " - too many retries. Giving up\n");
> -
> +report_failure:
> +	pr_cont(" - too many retries. Giving up\n");
>  	return ret;
> +report_success:
> +	pr_info("xc5000: Firmware %s loaded and running.\n", desired_fw->name);
> +	return 0;
>  }
>  
>  static void xc5000_do_timer_sleep(struct work_struct *timer_sleep)

[toc] | [next] | [standalone]

#1317144

>> This issue was detected by using the Coccinelle software.
>>
>> Split the previous if statement at the end so that each final log statement
>> will eventually be performed by a direct jump to these labels.
>> * report_failure
>> * report_success
>>
>> A check repetition can be excluded for the variable "ret" at the end then.
>>
>>
>> Apply also two recommendations from the script "checkpatch.pl".
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  drivers/media/tuners/xc5000.c | 16 +++++++---------
>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
>> index e6e5e90..1360677 100644
>> --- a/drivers/media/tuners/xc5000.c
>> +++ b/drivers/media/tuners/xc5000.c
>> @@ -1166,7 +1166,7 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
>>  
>>  		ret = xc5000_fwupload(fe, desired_fw, fw);
>>  		if (ret != 0)
>> -			goto err;
>> +			goto report_failure;
>>  
>>  		msleep(20);
>>  
>> @@ -1229,18 +1229,16 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
>>  		/* Default to "CABLE" mode */
>>  		ret = xc_write_reg(priv, XREG_SIGNALSOURCE, XC_RF_MODE_CABLE);
>>  		if (!ret)
>> -			break;
>> +			goto report_success;
>>  		printk(KERN_ERR "xc5000: can't set to cable mode.");
> 
> It sounds worth to avoid adding a goto here.

Are you interested in a bit of software optimisation for the implementation
of the function "xc_load_fw_and_init_tuner"?


>>  	}
>>  
>> -err:
>> -	if (!ret)
>> -		printk(KERN_INFO "xc5000: Firmware %s loaded and running.\n",
>> -		       desired_fw->name);
>> -	else
>> -		printk(KERN_CONT " - too many retries. Giving up\n");
>> -
>> +report_failure:
>> +	pr_cont(" - too many retries. Giving up\n");
>>  	return ret;
>> +report_success:
>> +	pr_info("xc5000: Firmware %s loaded and running.\n", desired_fw->name);
>> +	return 0;
>>  }
>>  
>>  static void xc5000_do_timer_sleep(struct work_struct *timer_sleep)


Is the proposed source code restructuring interesting?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1317167 — Re: [PATCH] [media] xc5000: Faster result reporting in xc_load_fw_and_init_tuner()

> Are you interested in a bit of software optimisation for the implementation
> of the function "xc_load_fw_and_init_tuner"?

To be clear, absolutely none of the code in question is performance
sensitive (i.e. saving a couple of extra CPU cycles has no value in
this case).  Hence given that I'm assuming you have no intention to
actually test any of these patches with a real device I would
recommend you do the bare minimum to prevent Coccinelle from
complaining and not restructure any of the core business logic unless
you plan to also do actual testing.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web