Re: [PATCH] i8k: Add support for temperature sensor labels

[Pali RohÃr]

On Saturday 29 November 2014 17:24:08 Guenter Roeck wrote:
> On 11/29/2014 08:04 AM, Pali RohÃr wrote:
> > +static bool __init i8k_check_temp(int sensor)
> > +{
> > +	int err;
> > +
> > +	/*
> > +	 * Check if temperature sensor type is valid.
> > +	 *
> > +	 * If it is valid then sensor should work. But some
> > sensors are not +	 * available at any time. E.g GPU sensor
> > on Optimus/PowerExpress/Enduro +	 * card does not work (or
> > return bogus value) when card is turned off. +	 * So this
> > function should not fail in this case. +	 */
> > +	err = i8k_get_temp_type(sensor);
> > +	if (err >= 0)
> > +		return true;
> > +
> 
> Are you sure this function is provided for all systems ?
> I am a bit concerned that we may wrongly disable sensors this
> way, especially on older systems.
> 

I do not know if that function is provided on all systems. But 
this code does not disable sensors. If function fail, then we 
fallback to temperature read down. Return true means that we 
enable sensor.

> It might be safer to create another set of flags and enable
> the labels only if the sensor is known to exist and if
> reading its type (label) works.
> 
> > +	/*
> > +	 * Check if temperatrue reading does not fail.
> > +	 *
> 
> temperature
> 
> > +	 * Sometimes detection of temperature sensor type does not
> > work but +	 * reading temperature working fine. Sometimes
> > temperature value is too +	 * high and i8k_get_temp()
> > returns -ERANGE. But there is no reason to +	 * ignore
> > these temperature sensors.
> > +	 */
> > +	err = i8k_get_temp(sensor);
> > +	if (err >= 0 || err == -ERANGE)
> > +		return true;
> > +
> 
> Please simplify to
> 	return err >= 0 || err == -ERANGE;
> 

I'm using style:

check_function_1();
if (not_failed)
	return true;

check_function_2();
if (not_failed)
	return true;

return false; // disable sensor

So I think it is better to have same style for both checks...

> > +	return false;
> > +}
> > +
> > 
> >   static int __init i8k_init_hwmon(void)
> >   {
> >   
> >   	int err;
> > 
> > @@ -613,18 +684,13 @@ static int __init i8k_init_hwmon(void)
> > 
> >   	i8k_hwmon_flags = 0;
> >   	
> >   	/* CPU temperature attributes, if temperature reading is
> >   	OK */
> > 
> > -	err = i8k_get_temp(0);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(0))
> 
> The introduction of i8k_check_temp() should be a separate
> patch.
> 
> Thanks,
> Guenter
> 

Ok.

> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
> > 
> > -	/* check for additional temperature sensors */
> > -	err = i8k_get_temp(1);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(1))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
> > 
> > -	err = i8k_get_temp(2);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(2))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
> > 
> > -	err = i8k_get_temp(3);
> > -	if (err >= 0 || err == -ERANGE)
> > +	if (i8k_check_temp(3))
> > 
> >   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> >   	
> >   	/* Left fan attributes, if left fan is present */

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.