Re: [PATCH v8 5/7] platform/x86/dell-laptop: Refactor kbd_led_triggers_store()

["Pali Rohár" <pali.rohar@gmail.com>]

On Thursday 09 February 2017 16:44:15 Hans de Goede wrote:
> Return -EINVAL immediately on invalid input, rather then doing
> the straight path in an if block and returning -EINVAL at the end
> of the function.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Ok, you can add my Reviewed-by.

> ---
> Changes in v8:
> -New patch in v8 of this patch-set
> ---
>  drivers/platform/x86/dell-laptop.c | 63 +++++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 14392a0..a2913a5 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -1660,38 +1660,39 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
>  		}
>  	}
>  
> -	if (trigger_bit != -1) {
> -		new_state = state;
> -		if (trigger[0] == '+')
> -			new_state.triggers |= BIT(trigger_bit);
> -		else {
> -			new_state.triggers &= ~BIT(trigger_bit);
> -			/* NOTE: trackstick bit (2) must be disabled when
> -			 *       disabling touchpad bit (1), otherwise touchpad
> -			 *       bit (1) will not be disabled */
> -			if (trigger_bit == 1)
> -				new_state.triggers &= ~BIT(2);
> -		}
> -		if ((kbd_info.triggers & new_state.triggers) !=
> -		    new_state.triggers)
> -			return -EINVAL;
> -		if (new_state.triggers && !triggers_enabled) {
> -			new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> -			kbd_set_level(&new_state, kbd_previous_level);
> -		} else if (new_state.triggers == 0) {
> -			kbd_set_level(&new_state, 0);
> -		}
> -		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> -			return -EINVAL;
> -		ret = kbd_set_state_safe(&new_state, &state);
> -		if (ret)
> -			return ret;
> -		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
> -			kbd_previous_mode_bit = new_state.mode_bit;
> -		return count;
> -	}
> +	if (trigger_bit == -1)
> +		return -EINVAL;
>  
> -	return -EINVAL;
> +	new_state = state;
> +	if (trigger[0] == '+')
> +		new_state.triggers |= BIT(trigger_bit);
> +	else {
> +		new_state.triggers &= ~BIT(trigger_bit);
> +		/*
> +		 * NOTE: trackstick bit (2) must be disabled when
> +		 *       disabling touchpad bit (1), otherwise touchpad
> +		 *       bit (1) will not be disabled
> +		 */
> +		if (trigger_bit == 1)
> +			new_state.triggers &= ~BIT(2);
> +	}
> +	if ((kbd_info.triggers & new_state.triggers) !=
> +	    new_state.triggers)
> +		return -EINVAL;
> +	if (new_state.triggers && !triggers_enabled) {
> +		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> +		kbd_set_level(&new_state, kbd_previous_level);
> +	} else if (new_state.triggers == 0) {
> +		kbd_set_level(&new_state, 0);
> +	}
> +	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> +		return -EINVAL;
> +	ret = kbd_set_state_safe(&new_state, &state);
> +	if (ret)
> +		return ret;
> +	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
> +		kbd_previous_mode_bit = new_state.mode_bit;
> +	return count;
>  }
>  
>  static ssize_t kbd_led_triggers_show(struct device *dev,

-- 
Pali Rohár
pali.rohar@gmail.com