linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: "Michele Sorcinelli" <michelesr@autistici.org>,
	"Pali Rohár" <pali.rohar@gmail.com>
Cc: Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org
Subject: Re: [PATCH] dell-smm-hwmon.c: Additional temperature sensors
Date: Fri, 7 Dec 2018 20:24:49 -0800	[thread overview]
Message-ID: <3f1b847e-2b70-70bb-f5e6-5f68ffbc63ed@roeck-us.net> (raw)
In-Reply-To: <20181208005603.11721-1-michelesr@autistici.org>

On 12/7/18 4:56 PM, Michele Sorcinelli wrote:
> The code has been extended to support up to 10 temperature sensors.
> 
> The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
> with i8k_get_temp() as in some laptop firmwares the related SMM procedure
> is not implemented correctly so i8k_get_temp_type() can't be used to
> discover temperature sensors.
> 
> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>

Please _always_ version your patches, and add a changelog, at least if you
would like to see it applied. I have now two patches which look almost the same,
meaning I would have to pull both versions, compare, and hope to apply the
correct one.

Anyway, I would like to get some feedback if this can cause regressions
on systems which don't support that many sensors and maybe report something
completely different if one tries to read the high-numbered sensors.
I seem to recall that there was a reason for checking the type and not just
trying to read sensor values.

Guenter

> ---
>   drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
>   1 file changed, 94 insertions(+), 19 deletions(-)
>   mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> old mode 100644
> new mode 100755
> index 367a8a617..b58f756ea
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>   #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
>   #define I8K_HWMON_HAVE_TEMP3	(1 << 2)
>   #define I8K_HWMON_HAVE_TEMP4	(1 << 3)
> -#define I8K_HWMON_HAVE_FAN1	(1 << 4)
> -#define I8K_HWMON_HAVE_FAN2	(1 << 5)
> -#define I8K_HWMON_HAVE_FAN3	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
> +#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
> +#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
> +#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
> +#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
> +#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
> +
> +#define I8K_HWMON_HAVE_FAN1	(1 << 10)
> +#define I8K_HWMON_HAVE_FAN2	(1 << 11)
> +#define I8K_HWMON_HAVE_FAN3	(1 << 12)
>   
>   MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>   MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>   static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>   static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>   			  3);
> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  4);
> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  5);
> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  6);
> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  7);
> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  8);
> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
> +			  9);
> +
>   static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, i8k_hwmon_show_fan, NULL, 0);
>   static SENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, i8k_hwmon_show_fan_label, NULL,
>   			  0);
> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>   	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
>   	&sensor_dev_attr_temp4_input.dev_attr.attr,	/* 6 */
>   	&sensor_dev_attr_temp4_label.dev_attr.attr,	/* 7 */
> -	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 8 */
> -	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 9 */
> -	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 10 */
> -	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 11 */
> -	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 12 */
> -	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 13 */
> -	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 14 */
> -	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 15 */
> -	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 16 */
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
> +	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
> +	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
> +	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
> +	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
> +	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
> +	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
> +	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
> +	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
> +	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
> +	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
> +	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
> +	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
> +	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 23 */
> +	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 24 */
> +	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 25 */
> +	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 26 */
> +	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 27 */
> +	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 28 */
>   	NULL
>   };
>   
> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>   	if (index >= 6 && index <= 7 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
>   		return 0;
> -	if (index >= 8 && index <= 10 &&
> +	if (index >= 8 && index <= 9 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP5))
> +		return 0;
> +	if (index >= 10 && index <= 11 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
> +		return 0;
> +	if (index >= 12 && index <= 13 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
> +		return 0;
> +	if (index >= 14 && index <= 15 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
> +		return 0;
> +	if (index >= 16 && index <= 17 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
> +		return 0;
> +	if (index >= 18 && index <= 19 &&
> +	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
> +		return 0;
> +
> +	if (index >= 20 && index <= 22 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>   		return 0;
> -	if (index >= 11 && index <= 13 &&
> +	if (index >= 23 && index <= 25 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>   		return 0;
> -	if (index >= 14 && index <= 16 &&
> +	if (index >= 26 && index <= 28 &&
>   	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>   		return 0;
>   
> @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
>   	i8k_hwmon_flags = 0;
>   
>   	/* CPU temperature attributes, if temperature type is OK */
> -	err = i8k_get_temp_type(0);
> +	err = i8k_get_temp(0);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
>   	/* check for additional temperature sensors */
> -	err = i8k_get_temp_type(1);
> +	err = i8k_get_temp(1);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
> -	err = i8k_get_temp_type(2);
> +	err = i8k_get_temp(2);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
> -	err = i8k_get_temp_type(3);
> +	err = i8k_get_temp(3);
>   	if (err >= 0)
>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
> +	err = i8k_get_temp(4);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
> +	err = i8k_get_temp(5);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
> +	err = i8k_get_temp(6);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
> +	err = i8k_get_temp(7);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
> +	err = i8k_get_temp(8);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
> +	err = i8k_get_temp(9);
> +	if (err >= 0)
> +		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
>   
>   	/* First fan attributes, if fan status or type is OK */
>   	err = i8k_get_fan_status(0);
> 

  reply	other threads:[~2018-12-08  4:24 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-27 23:06 [PATCH] dell-smm-hwmon.c: Add XPS 9570 to supported devices list Michele Sorcinelli
2018-11-28  8:14 ` Pali Rohár
2018-11-28 12:30 ` Michele Sorcinelli
2018-11-28 12:56   ` Pali Rohár
2018-11-28 13:01     ` Michele Sorcinelli
2018-11-28 23:23       ` Michele Sorcinelli
2018-11-29  9:48         ` Pali Rohár
2018-11-29 21:42           ` Michele Sorcinelli
2018-12-05  9:15             ` Pali Rohár
2018-12-07 20:29               ` [PATCH] dell-smm-hwmon.c: Add support for additional temperature sensors Michele Sorcinelli
2018-12-08  0:52                 ` Michele Sorcinelli
2018-12-08  0:56                 ` [PATCH] dell-smm-hwmon.c: Additional " Michele Sorcinelli
2018-12-08  4:24                   ` Guenter Roeck [this message]
2018-12-08 15:34                     ` Michele Sorcinelli
2018-12-08 15:43                       ` Michele Sorcinelli
2018-12-08 16:52                       ` Guenter Roeck
2018-12-10 10:58                     ` Pali Rohár
2018-12-10 11:53                       ` Michele Sorcinelli
2018-12-10 12:03                         ` Pali Rohár
2019-02-07 12:16                       ` Michele Sorcinelli
2019-02-07 12:40                         ` Pali Rohár
2019-04-21 17:00                           ` Michele Sorcinelli
2019-06-14 21:38                           ` Michele Sorcinelli
2018-12-08 17:02               ` [PATCH v2] " Michele Sorcinelli
2018-12-14 23:57               ` [PATCH v3] " Michele Sorcinelli
2018-12-21 23:40                 ` Guenter Roeck
2018-12-22 12:50                   ` Pali Rohár
2018-12-22 14:18                     ` [PATCH v3.1] " Michele Sorcinelli
2018-12-23 22:59                       ` Michele Sorcinelli
2018-12-24  1:16                         ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3f1b847e-2b70-70bb-f5e6-5f68ffbc63ed@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=jdelvare@suse.com \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=michelesr@autistici.org \
    --cc=pali.rohar@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).