Linux-Hwmon Archive on lore.kernel.org
 help / color / 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
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 index

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 publically 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

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox