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

Groups > linux.kernel > #1633664 > unrolled thread

Re: [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions

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 01/10] platform/x86: fujitsu-laptop: introduce fext_*()  helper functions Jonathan Woithe <jwoithe@just42.net> - 2017-05-01 15:20 +0200
    Re: [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*()  helper functions Michał Kępień <kernel@kempniu.pl> - 2017-05-02 15:30 +0200

#1633664 — Re: [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions

On Mon, Apr 24, 2017 at 03:33:25PM +0200, Micha?? K??pie?? wrote:
> Stop invoking call_fext_func() directly to improve code clarity and save
> some horizontal space.  Adjust whitespace to make checkpatch happy.

A comment: this patch in and of itself does not seem to be worthwhile.  In
particular, the saving of horzontal space seems academic.  The value comes
when later patches build on it.

> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 90 ++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 7f49d92914c9..3f232967af04 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -187,6 +187,26 @@ static int call_fext_func(int func, int op, int feature, int state)
>  	return value;
>  }
>  
> +static int fext_backlight(int op, int feature, int state)
> +{
> +	return call_fext_func(FUNC_BACKLIGHT, op, feature, state);
> +}
> +
> +static int fext_buttons(int op, int feature, int state)
> +{
> +	return call_fext_func(FUNC_BUTTONS, op, feature, state);
> +}
> +
> +static int fext_flags(int op, int feature, int state)
> +{
> +	return call_fext_func(FUNC_FLAGS, op, feature, state);
> +}
> +
> +static int fext_leds(int op, int feature, int state)
> +{
> +	return call_fext_func(FUNC_LEDS, op, feature, state);
> +}
> +
>  /* Hardware access for LCD brightness control */
>  
>  static int set_lcd_level(int level)
> @@ -272,9 +292,9 @@ static int bl_get_brightness(struct backlight_device *b)
>  static int bl_update_status(struct backlight_device *b)
>  {
>  	if (b->props.power == FB_BLANK_POWERDOWN)
> -		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
> +		fext_backlight(0x1, 0x4, 0x3);
>  	else
> -		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
> +		fext_backlight(0x1, 0x4, 0x0);
>  
>  	return set_lcd_level(b->props.brightness);
>  }
> @@ -610,22 +630,22 @@ static int logolamp_set(struct led_classdev *cdev,
>  	if (brightness < LED_FULL)
>  		always = FUNC_LED_OFF;
>  
> -	ret = call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
> +	ret = fext_leds(0x1, LOGOLAMP_POWERON, poweron);
>  	if (ret < 0)
>  		return ret;
>  
> -	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
> +	return fext_leds(0x1, LOGOLAMP_ALWAYS, always);
>  }

I've only just noticed this.  For the led calls we have symbolic identifiers
defined for the "features" parameter, but in the backlight case we are still
using arbitrary numeric constants.  Although not necessary for this patch
set, we should consider adding feature identifiers for the other fext_*() calls.
Similarly for the "op" parameter where it makes sense to do so.

Regards
  jonathan

[toc] | [next] | [standalone]

#1634408

> > @@ -272,9 +292,9 @@ static int bl_get_brightness(struct backlight_device *b)
> >  static int bl_update_status(struct backlight_device *b)
> >  {
> >  	if (b->props.power == FB_BLANK_POWERDOWN)
> > -		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
> > +		fext_backlight(0x1, 0x4, 0x3);
> >  	else
> > -		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
> > +		fext_backlight(0x1, 0x4, 0x0);
> >  
> >  	return set_lcd_level(b->props.brightness);
> >  }
> > @@ -610,22 +630,22 @@ static int logolamp_set(struct led_classdev *cdev,
> >  	if (brightness < LED_FULL)
> >  		always = FUNC_LED_OFF;
> >  
> > -	ret = call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
> > +	ret = fext_leds(0x1, LOGOLAMP_POWERON, poweron);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
> > +	return fext_leds(0x1, LOGOLAMP_ALWAYS, always);
> >  }
> 
> I've only just noticed this.  For the led calls we have symbolic identifiers
> defined for the "features" parameter, but in the backlight case we are still
> using arbitrary numeric constants.  Although not necessary for this patch
> set, we should consider adding feature identifiers for the other fext_*() calls.
> Similarly for the "op" parameter where it makes sense to do so.

Good point, I will keep that in mind for the next patch series.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web