Re: [PATCH] platform/x86: thinkpad_acpi: Add Thinkpad Edge E531 fan support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

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@matthias-fetzer.de>

> > > @@ -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.