On Sat, 6 May 2023, Luke Jones wrote:
> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen
> <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> > On Fri, 5 May 2023, Luke D. Jones wrote:
> >
> > > Add support for the WMI methods used to turn off and adjust the
> > > brightness of the secondary "screenpad" device found on some high-end
> > > ASUS laptops like the GX650P series and others.
> > >
> > > These methods are utilised in a new backlight device named:
> > > - asus_screenpad
> > >
> > > Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
> > > ---
> > > @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
> > > return 0;
> > > }
> > >
> > > +/* Screenpad backlight */
> > > +
> > > +static int read_screenpad_backlight_power(struct asus_wmi *asus)
> > > +{
> > > + int ret = asus_wmi_get_devstate_simple(asus,
> > > ASUS_WMI_DEVID_SCREENPAD_POWER);
> >
> > Please move this to own line because now you have the extra newline
> > in between the call and error handling.
>
> I don't understand what you mean sorry. Remove the new line or:
> int ret;
> ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
Don't do:
int func()
{
int ret = func();
if (ret < 0)
return ret;
...
}
But do:
int func()
{
int ret;
ret = func();
if (ret < 0)
return ret;
...
}
This keeps the error handling next to the actual call following the usual
error handling pattern (natural logic grouping). The added clarity is well
worth the one extra line required.
> > > +static int update_screenpad_bl_status(struct backlight_device *bd)
> > > +{
> > > + struct asus_wmi *asus = bl_get_data(bd);
> > > + int power, err = 0;
> > > + u32 ctrl_param;
> > > +
> > > + power = read_screenpad_backlight_power(asus);
> > > + if (power == -ENODEV)
> > > + return err;
> >
> > Just return 0. Or is there perhaps something wrong/missing here?
>
> I thought the correct thing was to return any possible error state (here,
> anything less than 0 would be an error, right?)
I think this was covered already but here what I meant, use either:
return 0;
because err is always 0 at that point, or:
return power;
Depending on which is correct, I wasn't sure because you had this after
it:
> > > + else if (power < 0)
> > > + return power;
...So I thought you might really intentionally wanted to return 0 there.
--
i.
