On Fri, 9 Aug 2024, Matthias Fetzer wrote:
>
> Thanks for the review!
>
> Am 08.08.24 um 15:14 schrieb Ilpo Järvinen:
> > On Sun, 14 Jul 2024, Matthias Fetzer wrote:
> >
> > > Fan control on the E531 is done using the ACPI methods FANG and
> > > FANW. The correct parameters and register values were found by
> > > analyzing EC firmware as well as DSDT. This has been tested on
> > > my Thinkpad Edge E531 (6885CTO, BIOS HEET52WW 1.33).
> > >
> > > Signed-off-by: Matthias Fetzer <kontakt@xxxxxxxxxxxxxxxxxx>
> > > @@ -8157,6 +8213,7 @@ static int fan2_get_speed(unsigned int *speed)
> > > static int fan_set_level(int level)
> > > {
> > > + int rc;
> > > if (!fan_control_allowed)
> > > return -EPERM;
> > > @@ -8206,6 +8263,36 @@ static int fan_set_level(int level)
> > > tp_features.fan_ctrl_status_undef = 0;
> > > break;
> > > + case TPACPI_FAN_WR_ACPI_FANW:
> > > + if ((!(level & TP_EC_FAN_AUTO) &&
> > > + ((level < 0) || (level > 7))) ||
> > > + (level & TP_EC_FAN_FULLSPEED))
> > > + return -EINVAL;
> >
> > I'd split this into two to make it more readable:
> >
> > if (!(level & TP_EC_FAN_AUTO) && (level < 0 || level > 7))
> > return -EINVAL;
> > if (level & TP_EC_FAN_FULLSPEED)
> > return -EINVAL;
>
> This is much better, thanks.
>
> >
> > I'm not sure if -EINVAL is really the right code to return though in these
> > cases.
> >
>
> I thought that since those are invalid values/parameters the return code of
> -EINVAL
> would be a good choice. What do you suggest to use instead?
Actually, now that I look into it more carefully, forget what I said.
I think -EINVAL is correct to return in these cases because the input
value is invalid (I previously assumed something else based on the
define names).
--
i.
