Re: [PM-SR] [PATCH 5/7] omap3: sr: device: add unlikely checks

[Nishanth Menon <nm@ti.com>]

Artem Bityutskiy had written, on 07/09/2010 08:12 AM, the following:
> On Fri, 2010-06-25 at 16:46 -0500, Nishanth Menon wrote:
>> Add unlikely checks to better optimize the rare occurrance of
>> erroneous conditions.
>>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> Cc: Thara Gopinath <thara@ti.com>
>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
> 
> unlikely and friends make sens only in realy hot path places. In other
> places like you touch, they are pointless - better let gcc make a choice

> @@ -43,8 +43,9 @@ static void __init sr_read_efuse(struct omap_sr_dev_data *dev_data,
>  {
>  	int i;
>  
> -	if (!dev_data || !dev_data->volts_supported || !dev_data->volt_data ||
> -			!dev_data->efuse_nvalues_offs) {
> +	if (unlikely(!dev_data || !dev_data->volts_supported ||
> +			!dev_data->volt_data ||
> +			!dev_data->efuse_nvalues_offs)) {

> @@ -87,8 +88,8 @@ static void __init sr_set_testing_nvalues(struct omap_sr_dev_data *dev_data,
>  {
>  	int i;
>  
> -	if (!dev_data || !dev_data->volts_supported ||
> -			!dev_data->volt_data || !dev_data->test_nvalues) {
> +	if (unlikely(!dev_data || !dev_data->volts_supported ||
> +			!dev_data->volt_data || !dev_data->test_nvalues)) {

and other places, why do you think that these are checks that should be 
expected? - would be great if you can explain inline to the patch which 
unlikely checks dont make sense.

static functions such as these are helpers for the maincode, unless 
something horribly wrong occurs within the codepath calling these 
helpers, they are not expected to be invalid parameters. hence the 
rationale for adding unlikely.



> of how to arrange code. And they only make the code less readable by
> adding extra braces and making ifs longer.
> 
> Thus, NACK for this from my side.
> 


-- 
Regards,
Nishanth Menon