Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors

[MichaÅ KÄpieÅ]

> On Thu, Jan 5, 2017 at 3:11 PM, MichaÅ KÄpieÅ <kernel@xxxxxxxxxx> wrote:
> > Potential errors returned by some call_fext_func() calls inside
> > logolamp_set() are currently ignored.  Rework logolamp_set() to properly
> > handle them.  This causes one more call_fext_func() call to be made in
> > the LED_OFF case, though one could argue that this is logically the
> > right thing to do (even though the extra call is not needed to shut the
> > LED off).
> >
> > Signed-off-by: MichaÅ KÄpieÅ <kernel@xxxxxxxxxx>
> > ---
> > This is a follow-up to a recent patch, "platform/x86: fujitsu-laptop:
> > use brightness_set_blocking for LED-setting callbacks" and thus needs
> > the latter to be applied first (currently it is already applied in
> > testing).
> >
> > Instead of sticking "if (ret < 0) return ret;" in two branches I decided
> > to restructure logolamp_set() for improved clarity and also to make it
> > resemble logolamp_get() a bit more.
> >
> >  drivers/platform/x86/fujitsu-laptop.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index b725a907a91f..12f7a8346dd0 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -271,15 +271,19 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
> >  static int logolamp_set(struct led_classdev *cdev,
> >                                enum led_brightness brightness)
> >  {
> > -       if (brightness >= LED_FULL) {
> > -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_ON);
> > -       } else if (brightness >= LED_HALF) {
> > -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_OFF);
> > -       } else {
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_OFF);
> 
> > +       int ret, poweron = FUNC_LED_OFF, always = FUNC_LED_OFF;
> > +
> > +       if (brightness >= LED_HALF) {
> > +               poweron = FUNC_LED_ON;
> > +               if (brightness >= LED_FULL)
> > +                       always = FUNC_LED_ON;
> >         }
> 
> But if you set ON by default the above could be written like
> 
>     if (brightness < LED_FULL)
>         always = FUNC_LED_OFF;
> 
>     if (brightness < LED_HALF)
>         poweron = FUNC_LED_OFF;

Ah, neat, thanks.  Jonathan, assuming you also like this, I will later
submit v2 using this approach.

-- 
Best regards,
MichaÅ KÄpieÅ