[PATCH 0/5] platform/x86: dell-ddv: Various driver updates

[PATCH 0/5] platform/x86: dell-ddv: Various driver updates

[Armin Wolf]

Thanks to bugreport 216655 on bugzilla, the contents of the
fan/thermal sensor buffers could be almost completely decoded.
It was also found out that newer Dell notebooks support a new
DDV WMI interface version, which just adds a additional method.

The first patch adds support for this new DDV WMI interface version,
while the second patch simplifies error handling then dealing with
empty buffers. The fourth patch adds a module param useful for testing
the driver on hardware exposing an unsupported DDV WMI interface
version. The last patch finally adds support for exposing the sensor
values over a standard hwmon interface.

The patch series was tested on a Dell Inspiron 3505, with the hwmon
patch being tested by multiple people over bugzilla and email. Those
who tested the final version of the hwmon patch are credited with
Tested-by tags.

Armin Wolf (5):
  platform/x86: dell-ddv: Add support for interface version 3
  platform/x86: dell-ddv: Return error if buffer is empty
  platform/x86: dell-ddv: Replace EIO with ENOMSG
  platform/x86: dell-ddv: Add "force" module param
  platform/x86: dell-ddv: Add hwmon support

 .../admin-guide/kernel-parameters.txt         |   3 +
 drivers/platform/x86/dell/Kconfig             |   1 +
 drivers/platform/x86/dell/dell-wmi-ddv.c      | 471 +++++++++++++++++-
 3 files changed, 465 insertions(+), 10 deletions(-)

--
2.30.2

[PATCH 1/5] platform/x86: dell-ddv: Add support for interface version 3

[Armin Wolf]

While trying to solve a bugreport on bugzilla, i learned that
some devices (for example the Dell XPS 17 9710) provide a more
recent DDV WMI interface (version 3).
Since the new interface version just adds an additional method,
no code changes are necessary apart from whitelisting the version.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 2bb449845d14..9cb6ae42dbdc 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -26,7 +26,8 @@

 #define DRIVER_NAME	"dell-wmi-ddv"

-#define DELL_DDV_SUPPORTED_INTERFACE 2
+#define DELL_DDV_SUPPORTED_VERSION_MIN	2
+#define DELL_DDV_SUPPORTED_VERSION_MAX	3
 #define DELL_DDV_GUID	"8A42EA14-4F2A-FD45-6422-0087F7A7E608"

 #define DELL_EPPID_LENGTH	20
@@ -49,6 +50,7 @@ enum dell_ddv_method {
 	DELL_DDV_BATTERY_RAW_ANALYTICS_START	= 0x0E,
 	DELL_DDV_BATTERY_RAW_ANALYTICS		= 0x0F,
 	DELL_DDV_BATTERY_DESIGN_VOLTAGE		= 0x10,
+	DELL_DDV_BATTERY_RAW_ANALYTICS_A_BLOCK	= 0x11, /* version 3 */

 	DELL_DDV_INTERFACE_VERSION		= 0x12,

@@ -340,7 +342,7 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
 		return ret;

 	dev_dbg(&wdev->dev, "WMI interface version: %d\n", version);
-	if (version != DELL_DDV_SUPPORTED_INTERFACE)
+	if (version < DELL_DDV_SUPPORTED_VERSION_MIN || version > DELL_DDV_SUPPORTED_VERSION_MAX)
 		return -ENODEV;

 	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
--
2.30.2

Re: [PATCH 1/5] platform/x86: dell-ddv: Add support for interface version 3

[Hans de Goede]

Hi,

On 1/26/23 20:40, Armin Wolf wrote:
> While trying to solve a bugreport on bugzilla, i learned that
> some devices (for example the Dell XPS 17 9710) provide a more
> recent DDV WMI interface (version 3).
> Since the new interface version just adds an additional method,
> no code changes are necessary apart from whitelisting the version.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> ---
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 2bb449845d14..9cb6ae42dbdc 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -26,7 +26,8 @@
> 
>  #define DRIVER_NAME	"dell-wmi-ddv"
> 
> -#define DELL_DDV_SUPPORTED_INTERFACE 2
> +#define DELL_DDV_SUPPORTED_VERSION_MIN	2
> +#define DELL_DDV_SUPPORTED_VERSION_MAX	3
>  #define DELL_DDV_GUID	"8A42EA14-4F2A-FD45-6422-0087F7A7E608"
> 
>  #define DELL_EPPID_LENGTH	20
> @@ -49,6 +50,7 @@ enum dell_ddv_method {
>  	DELL_DDV_BATTERY_RAW_ANALYTICS_START	= 0x0E,
>  	DELL_DDV_BATTERY_RAW_ANALYTICS		= 0x0F,
>  	DELL_DDV_BATTERY_DESIGN_VOLTAGE		= 0x10,
> +	DELL_DDV_BATTERY_RAW_ANALYTICS_A_BLOCK	= 0x11, /* version 3 */
> 
>  	DELL_DDV_INTERFACE_VERSION		= 0x12,
> 
> @@ -340,7 +342,7 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
>  		return ret;
> 
>  	dev_dbg(&wdev->dev, "WMI interface version: %d\n", version);
> -	if (version != DELL_DDV_SUPPORTED_INTERFACE)
> +	if (version < DELL_DDV_SUPPORTED_VERSION_MIN || version > DELL_DDV_SUPPORTED_VERSION_MAX)
>  		return -ENODEV;
> 
>  	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
> --
> 2.30.2
> 

[PATCH 2/5] platform/x86: dell-ddv: Return error if buffer is empty

[Armin Wolf]

In several cases, the DDV WMI interface can return buffers
with a length of zero. Return -ENODATA in such a case for
proper error handling. Also replace some -EIO errors with
more specialized ones.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 9cb6ae42dbdc..f99c4cb686fd 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -11,6 +11,7 @@
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/dev_printk.h>
+#include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/kstrtox.h>
 #include <linux/math.h>
@@ -125,21 +126,27 @@ static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_meth
 	if (ret < 0)
 		return ret;

-	if (obj->package.count != 2)
-		goto err_free;
+	if (obj->package.count != 2 ||
+	    obj->package.elements[0].type != ACPI_TYPE_INTEGER ||
+	    obj->package.elements[1].type != ACPI_TYPE_BUFFER) {
+		ret = -ENOMSG;

-	if (obj->package.elements[0].type != ACPI_TYPE_INTEGER)
 		goto err_free;
+	}

 	buffer_size = obj->package.elements[0].integer.value;

-	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
+	if (!buffer_size) {
+		ret = -ENODATA;
+
 		goto err_free;
+	}

 	if (buffer_size > obj->package.elements[1].buffer.length) {
 		dev_warn(&wdev->dev,
 			 FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer size (%d)\n",
 			 buffer_size, obj->package.elements[1].buffer.length);
+		ret = -EMSGSIZE;

 		goto err_free;
 	}
@@ -151,7 +158,7 @@ static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_meth
 err_free:
 	kfree(obj);

-	return -EIO;
+	return ret;
 }

 static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
--
2.30.2

Re: [PATCH 2/5] platform/x86: dell-ddv: Return error if buffer is empty

[Hans de Goede]

Hi,

On 1/26/23 20:40, Armin Wolf wrote:
> In several cases, the DDV WMI interface can return buffers
> with a length of zero. Return -ENODATA in such a case for
> proper error handling. Also replace some -EIO errors with
> more specialized ones.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 9cb6ae42dbdc..f99c4cb686fd 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -11,6 +11,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/dev_printk.h>
> +#include <linux/errno.h>
>  #include <linux/kernel.h>
>  #include <linux/kstrtox.h>
>  #include <linux/math.h>
> @@ -125,21 +126,27 @@ static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_meth
>  	if (ret < 0)
>  		return ret;
> 
> -	if (obj->package.count != 2)
> -		goto err_free;
> +	if (obj->package.count != 2 ||
> +	    obj->package.elements[0].type != ACPI_TYPE_INTEGER ||
> +	    obj->package.elements[1].type != ACPI_TYPE_BUFFER) {
> +		ret = -ENOMSG;
> 
> -	if (obj->package.elements[0].type != ACPI_TYPE_INTEGER)
>  		goto err_free;
> +	}
> 
>  	buffer_size = obj->package.elements[0].integer.value;
> 
> -	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
> +	if (!buffer_size) {
> +		ret = -ENODATA;
> +
>  		goto err_free;
> +	}
> 
>  	if (buffer_size > obj->package.elements[1].buffer.length) {
>  		dev_warn(&wdev->dev,
>  			 FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer size (%d)\n",
>  			 buffer_size, obj->package.elements[1].buffer.length);
> +		ret = -EMSGSIZE;
> 
>  		goto err_free;
>  	}
> @@ -151,7 +158,7 @@ static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_meth
>  err_free:
>  	kfree(obj);
> 
> -	return -EIO;
> +	return ret;
>  }
> 
>  static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
> --
> 2.30.2
> 

[PATCH 3/5] platform/x86: dell-ddv: Replace EIO with ENOMSG

[Armin Wolf]

When the ACPI WMI interface returns a valid ACPI object
which has the wrong type, then ENOMSG instead of EIO
should be returned, since the WMI method was still
successfully evaluated.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index f99c4cb686fd..58fadb74e86a 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -87,7 +87,7 @@ static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method

 	if (obj->type != type) {
 		kfree(obj);
-		return -EIO;
+		return -ENOMSG;
 	}

 	*result = obj;
--
2.30.2

Re: [PATCH 3/5] platform/x86: dell-ddv: Replace EIO with ENOMSG

[Hans de Goede]

Hi,

On 1/26/23 20:40, Armin Wolf wrote:
> When the ACPI WMI interface returns a valid ACPI object
> which has the wrong type, then ENOMSG instead of EIO
> should be returned, since the WMI method was still
> successfully evaluated.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index f99c4cb686fd..58fadb74e86a 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -87,7 +87,7 @@ static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method
> 
>  	if (obj->type != type) {
>  		kfree(obj);
> -		return -EIO;
> +		return -ENOMSG;
>  	}
> 
>  	*result = obj;
> --
> 2.30.2
> 

[PATCH 4/5] platform/x86: dell-ddv: Add "force" module param

[Armin Wolf]

Until now, the dell-wmi-ddv driver needs to be manually
patched and compiled to test compatibility with unknown
DDV WMI interface versions.
Add a module param to allow users to force loading even
when a unknown interface version was detected. Since this
might cause various unwanted side effects, the module param
is marked as unsafe.
Also update kernel-parameters.txt.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/admin-guide/kernel-parameters.txt |  3 +++
 drivers/platform/x86/dell/dell-wmi-ddv.c        | 13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 6cfa6e3996cf..9bbff5113427 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1024,6 +1024,9 @@
 	dell_smm_hwmon.fan_max=
 			[HW] Maximum configurable fan speed.

+	dell_wmi_ddv.force=
+			[HW] Do not check for supported WMI interface versions.
+
 	dfltcc=		[HW,S390]
 			Format: { on | off | def_only | inf_only | always }
 			on:       s390 zlib hardware support for compression on
diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 58fadb74e86a..9695bf493ea6 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -34,6 +34,10 @@
 #define DELL_EPPID_LENGTH	20
 #define DELL_EPPID_EXT_LENGTH	23

+static bool force;
+module_param_unsafe(force, bool, 0);
+MODULE_PARM_DESC(force, "Force loading without checking for supported WMI interface versions");
+
 enum dell_ddv_method {
 	DELL_DDV_BATTERY_DESIGN_CAPACITY	= 0x01,
 	DELL_DDV_BATTERY_FULL_CHARGE_CAPACITY	= 0x02,
@@ -349,8 +353,13 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
 		return ret;

 	dev_dbg(&wdev->dev, "WMI interface version: %d\n", version);
-	if (version < DELL_DDV_SUPPORTED_VERSION_MIN || version > DELL_DDV_SUPPORTED_VERSION_MAX)
-		return -ENODEV;
+	if (version < DELL_DDV_SUPPORTED_VERSION_MIN || version > DELL_DDV_SUPPORTED_VERSION_MAX) {
+		if (!force)
+			return -ENODEV;
+
+		dev_warn(&wdev->dev, "Loading despite unsupported WMI interface version (%u)\n",
+			 version);
+	}

 	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
--
2.30.2

Re: [PATCH 4/5] platform/x86: dell-ddv: Add "force" module param

[Hans de Goede]

Hi,

On 1/26/23 20:40, Armin Wolf wrote:
> Until now, the dell-wmi-ddv driver needs to be manually
> patched and compiled to test compatibility with unknown
> DDV WMI interface versions.
> Add a module param to allow users to force loading even
> when a unknown interface version was detected. Since this
> might cause various unwanted side effects, the module param
> is marked as unsafe.
> Also update kernel-parameters.txt.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  Documentation/admin-guide/kernel-parameters.txt |  3 +++
>  drivers/platform/x86/dell/dell-wmi-ddv.c        | 13 +++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 6cfa6e3996cf..9bbff5113427 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1024,6 +1024,9 @@
>  	dell_smm_hwmon.fan_max=
>  			[HW] Maximum configurable fan speed.
> 
> +	dell_wmi_ddv.force=
> +			[HW] Do not check for supported WMI interface versions.
> +
>  	dfltcc=		[HW,S390]
>  			Format: { on | off | def_only | inf_only | always }
>  			on:       s390 zlib hardware support for compression on
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 58fadb74e86a..9695bf493ea6 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -34,6 +34,10 @@
>  #define DELL_EPPID_LENGTH	20
>  #define DELL_EPPID_EXT_LENGTH	23
> 
> +static bool force;
> +module_param_unsafe(force, bool, 0);
> +MODULE_PARM_DESC(force, "Force loading without checking for supported WMI interface versions");
> +
>  enum dell_ddv_method {
>  	DELL_DDV_BATTERY_DESIGN_CAPACITY	= 0x01,
>  	DELL_DDV_BATTERY_FULL_CHARGE_CAPACITY	= 0x02,
> @@ -349,8 +353,13 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
>  		return ret;
> 
>  	dev_dbg(&wdev->dev, "WMI interface version: %d\n", version);
> -	if (version < DELL_DDV_SUPPORTED_VERSION_MIN || version > DELL_DDV_SUPPORTED_VERSION_MAX)
> -		return -ENODEV;
> +	if (version < DELL_DDV_SUPPORTED_VERSION_MIN || version > DELL_DDV_SUPPORTED_VERSION_MAX) {
> +		if (!force)
> +			return -ENODEV;
> +
> +		dev_warn(&wdev->dev, "Loading despite unsupported WMI interface version (%u)\n",
> +			 version);
> +	}
> 
>  	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
> --
> 2.30.2
> 

Re: [PATCH 4/5] platform/x86: dell-ddv: Add "force" module param

[Hans de Goede]

Hi again,

On 1/26/23 20:40, Armin Wolf wrote:
> Until now, the dell-wmi-ddv driver needs to be manually
> patched and compiled to test compatibility with unknown
> DDV WMI interface versions.
> Add a module param to allow users to force loading even
> when a unknown interface version was detected. Since this
> might cause various unwanted side effects, the module param
> is marked as unsafe.
> Also update kernel-parameters.txt.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  3 +++
>  drivers/platform/x86/dell/dell-wmi-ddv.c        | 13 +++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 6cfa6e3996cf..9bbff5113427 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1024,6 +1024,9 @@
>  	dell_smm_hwmon.fan_max=
>  			[HW] Maximum configurable fan speed.
> 
> +	dell_wmi_ddv.force=
> +			[HW] Do not check for supported WMI interface versions.
> +
>  	dfltcc=		[HW,S390]
>  			Format: { on | off | def_only | inf_only | always }
>  			on:       s390 zlib hardware support for compression on

In my previous email I forgot to add that I have dropped this bit. I appreciate
the effort to document this parameter, but if we add documentation for all
existing parameters to Documentation/admin-guide/kernel-parameters.txt then
the file will become quite unyielding / unusable.

So in general we only add new parameters which we expect to be important for
a large group of users or necessary to debug serious problems like machines
not booting.

I realize that a bunch of parameters in there do not match this, like
e.g. dell_smm_hwmon.fan_max, these are just from older times when
there were just less parameters, so listing them all was still ok.

So I have merged this patch, but with the Documentation/admin-guide/kernel-parameters.txt
bit dropped.

Regards,

Hans






> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 58fadb74e86a..9695bf493ea6 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -34,6 +34,10 @@
>  #define DELL_EPPID_LENGTH	20
>  #define DELL_EPPID_EXT_LENGTH	23
> 
> +static bool force;
> +module_param_unsafe(force, bool, 0);
> +MODULE_PARM_DESC(force, "Force loading without checking for supported WMI interface versions");
> +
>  enum dell_ddv_method {
>  	DELL_DDV_BATTERY_DESIGN_CAPACITY	= 0x01,
>  	DELL_DDV_BATTERY_FULL_CHARGE_CAPACITY	= 0x02,
> @@ -349,8 +353,13 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
>  		return ret;
> 
>  	dev_dbg(&wdev->dev, "WMI interface version: %d\n", version);
> -	if (version < DELL_DDV_SUPPORTED_VERSION_MIN || version > DELL_DDV_SUPPORTED_VERSION_MAX)
> -		return -ENODEV;
> +	if (version < DELL_DDV_SUPPORTED_VERSION_MIN || version > DELL_DDV_SUPPORTED_VERSION_MAX) {
> +		if (!force)
> +			return -ENODEV;
> +
> +		dev_warn(&wdev->dev, "Loading despite unsupported WMI interface version (%u)\n",
> +			 version);
> +	}
> 
>  	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
> --
> 2.30.2
> 

Re: [PATCH 4/5] platform/x86: dell-ddv: Add "force" module param

[Hans de Goede]

Hi again...

On 1/30/23 16:09, Hans de Goede wrote:
> Hi again,
> 
> On 1/26/23 20:40, Armin Wolf wrote:
>> Until now, the dell-wmi-ddv driver needs to be manually
>> patched and compiled to test compatibility with unknown
>> DDV WMI interface versions.
>> Add a module param to allow users to force loading even
>> when a unknown interface version was detected. Since this
>> might cause various unwanted side effects, the module param
>> is marked as unsafe.
>> Also update kernel-parameters.txt.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt |  3 +++
>>  drivers/platform/x86/dell/dell-wmi-ddv.c        | 13 +++++++++++--
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 6cfa6e3996cf..9bbff5113427 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1024,6 +1024,9 @@
>>  	dell_smm_hwmon.fan_max=
>>  			[HW] Maximum configurable fan speed.
>>
>> +	dell_wmi_ddv.force=
>> +			[HW] Do not check for supported WMI interface versions.
>> +
>>  	dfltcc=		[HW,S390]
>>  			Format: { on | off | def_only | inf_only | always }
>>  			on:       s390 zlib hardware support for compression on
> 
> In my previous email I forgot to add that I have dropped this bit. I appreciate
> the effort to document this parameter, but if we add documentation for all
> existing parameters to Documentation/admin-guide/kernel-parameters.txt then
> the file will become quite unyielding / unusable.
> 
> So in general we only add new parameters which we expect to be important for
> a large group of users or necessary to debug serious problems like machines
> not booting.
> 
> I realize that a bunch of parameters in there do not match this, like
> e.g. dell_smm_hwmon.fan_max, these are just from older times when
> there were just less parameters, so listing them all was still ok.
> 
> So I have merged this patch, but with the Documentation/admin-guide/kernel-parameters.txt
> bit dropped.

I forgot to add: and these days it is really easy to find all the supported parameters
for a module by just doing "modinfo <modulename>

Regards,

Hans

[PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[Armin Wolf]

Thanks to bugreport 216655 on bugzilla triggered by the
dell-smm-hwmon driver, the contents of the sensor buffers
could be almost completely decoded.
Add an hwmon interface for exposing the fan and thermal
sensor values. The debugfs interface remains in place to
aid in reverse-engineering of unknown sensor types
and the thermal buffer.

Tested-by: Antonín Skala <skala.antonin@gmail.com>
Tested-by: Gustavo Walbon <gustavowalbon@gmail.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/Kconfig        |   1 +
 drivers/platform/x86/dell/dell-wmi-ddv.c | 435 ++++++++++++++++++++++-
 2 files changed, 435 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index d319de8f2132..21a74b63d9b1 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -194,6 +194,7 @@ config DELL_WMI_DDV
 	default m
 	depends on ACPI_BATTERY
 	depends on ACPI_WMI
+	depends on HWMON
 	help
 	  This option adds support for WMI-based sensors like
 	  battery temperature sensors found on some Dell notebooks.
diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 9695bf493ea6..5b30bb85199e 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -13,6 +13,7 @@
 #include <linux/dev_printk.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
+#include <linux/hwmon.h>
 #include <linux/kstrtox.h>
 #include <linux/math.h>
 #include <linux/module.h>
@@ -21,10 +22,13 @@
 #include <linux/printk.h>
 #include <linux/seq_file.h>
 #include <linux/sysfs.h>
+#include <linux/types.h>
 #include <linux/wmi.h>

 #include <acpi/battery.h>

+#include <asm/unaligned.h>
+
 #define DRIVER_NAME	"dell-wmi-ddv"

 #define DELL_DDV_SUPPORTED_VERSION_MIN	2
@@ -63,6 +67,29 @@ enum dell_ddv_method {
 	DELL_DDV_THERMAL_SENSOR_INFORMATION	= 0x22,
 };

+struct fan_sensor_entry {
+	u8 type;
+	__le16 rpm;
+} __packed;
+
+struct thermal_sensor_entry {
+	u8 type;
+	s8 now;
+	s8 min;
+	s8 max;
+	u8 unknown;
+} __packed;
+
+struct combined_channel_info {
+	struct hwmon_channel_info info;
+	u32 config[];
+};
+
+struct combined_chip_info {
+	struct hwmon_chip_info chip;
+	const struct hwmon_channel_info *info[];
+};
+
 struct dell_wmi_ddv_data {
 	struct acpi_battery_hook hook;
 	struct device_attribute temp_attr;
@@ -70,6 +97,24 @@ struct dell_wmi_ddv_data {
 	struct wmi_device *wdev;
 };

+static const char * const fan_labels[] = {
+	"CPU Fan",
+	"Chassis Motherboard Fan",
+	"Video Fan",
+	"Power Supply Fan",
+	"Chipset Fan",
+	"Memory Fan",
+	"PCI Fan",
+	"HDD Fan",
+};
+
+static const char * const fan_dock_labels[] = {
+	"Docking Chassis/Motherboard Fan",
+	"Docking Video Fan",
+	"Docking Power Supply Fan",
+	"Docking Chipset Fan",
+};
+
 static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
 				   union acpi_object **result, acpi_object_type type)
 {
@@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
 	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
 }

+static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
+				      size_t entry_size, union acpi_object **result, u64 *count)
+{
+	union acpi_object *obj;
+	u64 buffer_size;
+	u8 *buffer;
+	int ret;
+
+	ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
+	if (ret < 0)
+		return ret;
+
+	buffer_size = obj->package.elements[0].integer.value;
+	buffer = obj->package.elements[1].buffer.pointer;
+	if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != 0xff) {
+		kfree(obj);
+
+		return -ENOMSG;
+	}
+
+	*count = (buffer_size - 1) / entry_size;
+	*result = obj;
+
+	return 0;
+}
+
+static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
+				       int channel)
+{
+	return 0444;
+}
+
+static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
+					 long *val)
+{
+	struct fan_sensor_entry *entry;
+	union acpi_object *obj;
+	u64 count;
+	int ret;
+
+	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
+					 sizeof(*entry), &obj, &count);
+	if (ret < 0)
+		return ret;
+
+	entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
+	if (count > channel) {
+		switch (attr) {
+		case hwmon_fan_input:
+			*val = get_unaligned_le16(&entry[channel].rpm);
+
+			break;
+		default:
+			ret = -EOPNOTSUPP;
+		}
+	} else {
+		ret = -ENXIO;
+	}
+
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
+					  long *val)
+{
+	struct thermal_sensor_entry *entry;
+	union acpi_object *obj;
+	u64 count;
+	int ret;
+
+	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
+					 sizeof(*entry), &obj, &count);
+	if (ret < 0)
+		return ret;
+
+	entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
+	if (count > channel) {
+		switch (attr) {
+		case hwmon_temp_input:
+			*val = entry[channel].now * 1000;
+
+			break;
+		case hwmon_temp_min:
+			*val = entry[channel].min * 1000;
+
+			break;
+		case hwmon_temp_max:
+			*val = entry[channel].max * 1000;
+
+			break;
+		default:
+			ret = -EOPNOTSUPP;
+		}
+	} else {
+		ret = -ENXIO;
+	}
+
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+			     int channel, long *val)
+{
+	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_fan:
+		return dell_wmi_ddv_fan_read_channel(data, attr, channel, val);
+	case hwmon_temp:
+		return dell_wmi_ddv_temp_read_channel(data, attr, channel, val);
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int channel,
+					const char **str)
+{
+	struct fan_sensor_entry *entry;
+	union acpi_object *obj;
+	u64 count;
+	u8 type;
+	int ret;
+
+	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
+					 sizeof(*entry), &obj, &count);
+	if (ret < 0)
+		return ret;
+
+	entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
+	if (count > channel) {
+		type = entry[channel].type;
+
+		switch (type) {
+		case 0x00 ... 0x07:
+			*str = fan_labels[type];
+
+			break;
+		case 0x11 ... 0x14:
+			*str = fan_dock_labels[type - 0x11];
+
+			break;
+		default:
+			*str = "Unknown Fan";
+		}
+	} else {
+		ret = -ENXIO;
+	}
+
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int channel,
+					 const char **str)
+{
+	struct thermal_sensor_entry *entry;
+	union acpi_object *obj;
+	u64 count;
+	int ret;
+
+	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
+					 sizeof(*entry), &obj, &count);
+	if (ret < 0)
+		return ret;
+
+	entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
+	if (count > channel) {
+		switch (entry[channel].type) {
+		case 0x00:
+			*str = "CPU";
+
+			break;
+		case 0x11:
+			*str = "Video";
+
+			break;
+		case 0x22:
+			*str = "Memory"; // sometimes called DIMM
+
+			break;
+		case 0x33:
+			*str = "Other";
+
+			break;
+		case 0x44:
+			*str = "Ambient"; // sometimes called SKIN
+
+			break;
+		case 0x52:
+			*str = "SODIMM";
+
+			break;
+		case 0x55:
+			*str = "HDD";
+
+			break;
+		case 0x62:
+			*str = "SODIMM 2";
+
+			break;
+		case 0x73:
+			*str = "NB";
+
+			break;
+		case 0x83:
+			*str = "Charger";
+
+			break;
+		case 0xbb:
+			*str = "Memory 3";
+
+			break;
+		default:
+			*str = "Unknown";
+		}
+	} else {
+		ret = -ENXIO;
+	}
+
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+				    int channel, const char **str)
+{
+	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_label:
+			return dell_wmi_ddv_fan_read_string(data, channel, str);
+		default:
+			break;
+		}
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_label:
+			return dell_wmi_ddv_temp_read_string(data, channel, str);
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops dell_wmi_ddv_ops = {
+	.is_visible = dell_wmi_ddv_is_visible,
+	.read = dell_wmi_ddv_read,
+	.read_string = dell_wmi_ddv_read_string,
+};
+
+static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev, u64 count,
+							      enum hwmon_sensor_types type,
+							      u32 config)
+{
+	struct combined_channel_info *cinfo;
+	int i;
+
+	cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + 1), GFP_KERNEL);
+	if (!cinfo)
+		return ERR_PTR(-ENOMEM);
+
+	cinfo->info.type = type;
+	cinfo->info.config = cinfo->config;
+
+	for (i = 0; i < count; i++)
+		cinfo->config[i] = config;
+
+	return &cinfo->info;
+}
+
+static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
+							    enum dell_ddv_method method,
+							    size_t entry_size,
+							    enum hwmon_sensor_types type,
+							    u32 config)
+{
+	union acpi_object *obj;
+	u64 count;
+	int ret;
+
+	ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, &obj, &count);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	kfree(obj);
+
+	if (!count)
+		return ERR_PTR(-ENODEV);
+
+	return dell_wmi_ddv_channel_create(&wdev->dev, count, type, config);
+}
+
+static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data)
+{
+	struct wmi_device *wdev = data->wdev;
+	struct combined_chip_info *cinfo;
+	struct device *hdev;
+	int index = 0;
+	int ret;
+
+	if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, GFP_KERNEL))
+		return -ENOMEM;
+
+	cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), GFP_KERNEL);
+	if (!cinfo) {
+		ret = -ENOMEM;
+
+		goto err_release;
+	}
+
+	cinfo->chip.ops = &dell_wmi_ddv_ops;
+	cinfo->chip.info = cinfo->info;
+
+	cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, hwmon_chip,
+							 HWMON_C_REGISTER_TZ);
+
+	if (IS_ERR(cinfo->info[index])) {
+		ret = PTR_ERR(cinfo->info[index]);
+
+		goto err_release;
+	}
+
+	index++;
+
+	cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
+						       sizeof(struct fan_sensor_entry), hwmon_fan,
+						       (HWMON_F_INPUT | HWMON_F_LABEL));
+	if (!IS_ERR(cinfo->info[index]))
+		index++;
+
+	cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
+						       sizeof(struct thermal_sensor_entry),
+						       hwmon_temp, (HWMON_T_INPUT | HWMON_T_MIN |
+						       HWMON_T_MAX | HWMON_T_LABEL));
+	if (!IS_ERR(cinfo->info[index]))
+		index++;
+
+	if (!index) {
+		ret = -ENODEV;
+
+		goto err_release;
+	}
+
+	cinfo->info[index] = NULL;
+
+	hdev = devm_hwmon_device_register_with_info(&wdev->dev, "dell_ddv", data, &cinfo->chip,
+						    NULL);
+	if (IS_ERR(hdev)) {
+		ret = PTR_ERR(hdev);
+
+		goto err_release;
+	}
+
+	devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
+
+	return 0;
+
+err_release:
+	devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
+
+	return ret;
+}
+
 static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
 {
 	const char *uid_str;
@@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)

 	dell_wmi_ddv_debugfs_init(wdev);

-	return dell_wmi_ddv_battery_add(data);
+	ret = dell_wmi_ddv_hwmon_add(data);
+	if (ret < 0)
+		dev_dbg(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
+
+	ret = dell_wmi_ddv_battery_add(data);
+	if (ret < 0)
+		dev_dbg(&wdev->dev, "Unable to register acpi battery hook: %d\n", ret);
+
+	return 0;
 }

 static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
--
2.30.2

Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[Guenter Roeck]

On Thu, Jan 26, 2023 at 08:40:21PM +0100, Armin Wolf wrote:
> Thanks to bugreport 216655 on bugzilla triggered by the
> dell-smm-hwmon driver, the contents of the sensor buffers
> could be almost completely decoded.
> Add an hwmon interface for exposing the fan and thermal
> sensor values. The debugfs interface remains in place to
> aid in reverse-engineering of unknown sensor types
> and the thermal buffer.
> 
> Tested-by: Antonín Skala <skala.antonin@gmail.com>
> Tested-by: Gustavo Walbon <gustavowalbon@gmail.com>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/dell/Kconfig        |   1 +
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 435 ++++++++++++++++++++++-
>  2 files changed, 435 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index d319de8f2132..21a74b63d9b1 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -194,6 +194,7 @@ config DELL_WMI_DDV
>  	default m
>  	depends on ACPI_BATTERY
>  	depends on ACPI_WMI
> +	depends on HWMON

Not sure if adding such a dependency is a good idea.
Up to the maintainer to decide. Personally I would prefer
something like
	depends on HWMON || HWMON=n
and some conditionals in the code, as it is done with other drivers
outside the hwmon directory.

>  	help
>  	  This option adds support for WMI-based sensors like
>  	  battery temperature sensors found on some Dell notebooks.
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 9695bf493ea6..5b30bb85199e 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -13,6 +13,7 @@
>  #include <linux/dev_printk.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
> +#include <linux/hwmon.h>
>  #include <linux/kstrtox.h>
>  #include <linux/math.h>
>  #include <linux/module.h>
> @@ -21,10 +22,13 @@
>  #include <linux/printk.h>
>  #include <linux/seq_file.h>
>  #include <linux/sysfs.h>
> +#include <linux/types.h>
>  #include <linux/wmi.h>
> 
>  #include <acpi/battery.h>
> 
> +#include <asm/unaligned.h>
> +
>  #define DRIVER_NAME	"dell-wmi-ddv"
> 
>  #define DELL_DDV_SUPPORTED_VERSION_MIN	2
> @@ -63,6 +67,29 @@ enum dell_ddv_method {
>  	DELL_DDV_THERMAL_SENSOR_INFORMATION	= 0x22,
>  };
> 
> +struct fan_sensor_entry {
> +	u8 type;
> +	__le16 rpm;
> +} __packed;
> +
> +struct thermal_sensor_entry {
> +	u8 type;
> +	s8 now;
> +	s8 min;
> +	s8 max;
> +	u8 unknown;
> +} __packed;
> +
> +struct combined_channel_info {
> +	struct hwmon_channel_info info;
> +	u32 config[];
> +};
> +
> +struct combined_chip_info {
> +	struct hwmon_chip_info chip;
> +	const struct hwmon_channel_info *info[];
> +};
> +
>  struct dell_wmi_ddv_data {
>  	struct acpi_battery_hook hook;
>  	struct device_attribute temp_attr;
> @@ -70,6 +97,24 @@ struct dell_wmi_ddv_data {
>  	struct wmi_device *wdev;
>  };
> 
> +static const char * const fan_labels[] = {
> +	"CPU Fan",
> +	"Chassis Motherboard Fan",
> +	"Video Fan",
> +	"Power Supply Fan",
> +	"Chipset Fan",
> +	"Memory Fan",
> +	"PCI Fan",
> +	"HDD Fan",
> +};
> +
> +static const char * const fan_dock_labels[] = {
> +	"Docking Chassis/Motherboard Fan",
> +	"Docking Video Fan",
> +	"Docking Power Supply Fan",
> +	"Docking Chipset Fan",
> +};
> +
>  static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
>  				   union acpi_object **result, acpi_object_type type)
>  {
> @@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
>  	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
>  }
> 
> +static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
> +				      size_t entry_size, union acpi_object **result, u64 *count)
> +{
> +	union acpi_object *obj;
> +	u64 buffer_size;
> +	u8 *buffer;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
> +	if (ret < 0)
> +		return ret;
> +
> +	buffer_size = obj->package.elements[0].integer.value;
> +	buffer = obj->package.elements[1].buffer.pointer;
> +	if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != 0xff) {
> +		kfree(obj);
> +

Stray empty line

> +		return -ENOMSG;
> +	}
> +
> +	*count = (buffer_size - 1) / entry_size;
> +	*result = obj;
> +
> +	return 0;
> +}
> +
> +static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
> +				       int channel)
> +{
> +	return 0444;
> +}
> +
> +static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
> +					 long *val)
> +{
> +	struct fan_sensor_entry *entry;
> +	union acpi_object *obj;
> +	u64 count;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
> +					 sizeof(*entry), &obj, &count);
> +	if (ret < 0)
> +		return ret;
> +
> +	entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
> +	if (count > channel) {
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			*val = get_unaligned_le16(&entry[channel].rpm);
> +

Another stray empty line. I see that "empty line before return or break"
is common. Looks odd to me, and I don't see the point (it confuses
the code flow for me and lets my brain focus on the empty line instead
of the code in question), but I guess that is PoV. I won't comment on
it further and let the maintainer decide.

> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}

Error handling should come first.

> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
> +					  long *val)
> +{
> +	struct thermal_sensor_entry *entry;
> +	union acpi_object *obj;
> +	u64 count;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
> +					 sizeof(*entry), &obj, &count);
> +	if (ret < 0)
> +		return ret;
> +
> +	entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
> +	if (count > channel) {

This is a bit of Joda programming. It is really "channel < count",
ie the requested channel number is in the range of channels reported
by the WMI code. PoV, of course, but I find that the above makes the
code more difficult to read.

> +		switch (attr) {
> +		case hwmon_temp_input:
> +			*val = entry[channel].now * 1000;
> +
> +			break;
> +		case hwmon_temp_min:
> +			*val = entry[channel].min * 1000;
> +
> +			break;
> +		case hwmon_temp_max:
> +			*val = entry[channel].max * 1000;
> +
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;

break; missing

> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}

Same as above - error handling should come first.

> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			     int channel, long *val)
> +{
> +	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		return dell_wmi_ddv_fan_read_channel(data, attr, channel, val);
> +	case hwmon_temp:
> +		return dell_wmi_ddv_temp_read_channel(data, attr, channel, val);
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int channel,
> +					const char **str)
> +{
> +	struct fan_sensor_entry *entry;
> +	union acpi_object *obj;
> +	u64 count;
> +	u8 type;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
> +					 sizeof(*entry), &obj, &count);
> +	if (ret < 0)
> +		return ret;
> +
> +	entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
> +	if (count > channel) {
> +		type = entry[channel].type;
> +
> +		switch (type) {
> +		case 0x00 ... 0x07:
> +			*str = fan_labels[type];
> +
> +			break;
> +		case 0x11 ... 0x14:
> +			*str = fan_dock_labels[type - 0x11];
> +
> +			break;
> +		default:
> +			*str = "Unknown Fan";

break; missing.

> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}

And again.

> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int channel,
> +					 const char **str)
> +{
> +	struct thermal_sensor_entry *entry;
> +	union acpi_object *obj;
> +	u64 count;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
> +					 sizeof(*entry), &obj, &count);
> +	if (ret < 0)
> +		return ret;
> +
> +	entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
> +	if (count > channel) {
> +		switch (entry[channel].type) {
> +		case 0x00:
> +			*str = "CPU";
> +
> +			break;
> +		case 0x11:
> +			*str = "Video";
> +
> +			break;
> +		case 0x22:
> +			*str = "Memory"; // sometimes called DIMM

Personally I don't permit C++ style comments in a hwmon driver
unless _all_ comments are C++ style. Just a remark here.

> +
> +			break;
> +		case 0x33:
> +			*str = "Other";
> +
> +			break;
> +		case 0x44:
> +			*str = "Ambient"; // sometimes called SKIN
> +
> +			break;
> +		case 0x52:
> +			*str = "SODIMM";
> +
> +			break;
> +		case 0x55:
> +			*str = "HDD";
> +
> +			break;
> +		case 0x62:
> +			*str = "SODIMM 2";
> +
> +			break;
> +		case 0x73:
> +			*str = "NB";
> +
> +			break;
> +		case 0x83:
> +			*str = "Charger";
> +
> +			break;
> +		case 0xbb:
> +			*str = "Memory 3";
> +
> +			break;
> +		default:
> +			*str = "Unknown";

break; missing
Ok, I guess this is on purpose. I personally don't permit
that since it always leaves the question if it was on purpose or not.

> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}
> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +				    int channel, const char **str)
> +{
> +	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_label:
> +			return dell_wmi_ddv_fan_read_string(data, channel, str);
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_label:
> +			return dell_wmi_ddv_temp_read_string(data, channel, str);
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops dell_wmi_ddv_ops = {
> +	.is_visible = dell_wmi_ddv_is_visible,
> +	.read = dell_wmi_ddv_read,
> +	.read_string = dell_wmi_ddv_read_string,
> +};
> +
> +static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev, u64 count,
> +							      enum hwmon_sensor_types type,
> +							      u32 config)
> +{
> +	struct combined_channel_info *cinfo;
> +	int i;
> +
> +	cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + 1), GFP_KERNEL);
> +	if (!cinfo)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cinfo->info.type = type;
> +	cinfo->info.config = cinfo->config;
> +
> +	for (i = 0; i < count; i++)
> +		cinfo->config[i] = config;
> +
> +	return &cinfo->info;
> +}
> +
> +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
> +							    enum dell_ddv_method method,
> +							    size_t entry_size,
> +							    enum hwmon_sensor_types type,
> +							    u32 config)
> +{
> +	union acpi_object *obj;
> +	u64 count;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, &obj, &count);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	kfree(obj);
> +
> +	if (!count)
> +		return ERR_PTR(-ENODEV);
> +
> +	return dell_wmi_ddv_channel_create(&wdev->dev, count, type, config);
> +}
> +
> +static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data)
> +{
> +	struct wmi_device *wdev = data->wdev;
> +	struct combined_chip_info *cinfo;
> +	struct device *hdev;
> +	int index = 0;
> +	int ret;
> +
> +	if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), GFP_KERNEL);
> +	if (!cinfo) {
> +		ret = -ENOMEM;
> +
> +		goto err_release;
> +	}
> +
> +	cinfo->chip.ops = &dell_wmi_ddv_ops;
> +	cinfo->chip.info = cinfo->info;
> +
> +	cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, hwmon_chip,
> +							 HWMON_C_REGISTER_TZ);
> +
> +	if (IS_ERR(cinfo->info[index])) {
> +		ret = PTR_ERR(cinfo->info[index]);
> +
> +		goto err_release;
> +	}
> +
> +	index++;
> +
> +	cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
> +						       sizeof(struct fan_sensor_entry), hwmon_fan,
> +						       (HWMON_F_INPUT | HWMON_F_LABEL));
> +	if (!IS_ERR(cinfo->info[index]))
> +		index++;
> +
> +	cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
> +						       sizeof(struct thermal_sensor_entry),
> +						       hwmon_temp, (HWMON_T_INPUT | HWMON_T_MIN |
> +						       HWMON_T_MAX | HWMON_T_LABEL));
> +	if (!IS_ERR(cinfo->info[index]))
> +		index++;
> +
> +	if (!index) {
> +		ret = -ENODEV;
> +
> +		goto err_release;
> +	}
> +
> +	cinfo->info[index] = NULL;
> +
> +	hdev = devm_hwmon_device_register_with_info(&wdev->dev, "dell_ddv", data, &cinfo->chip,
> +						    NULL);
> +	if (IS_ERR(hdev)) {
> +		ret = PTR_ERR(hdev);
> +
> +		goto err_release;
> +	}
> +
> +	devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
> +
> +	return 0;
> +
> +err_release:
> +	devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
> +
> +	return ret;
> +}
> +
>  static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
>  {
>  	const char *uid_str;
> @@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
> 
>  	dell_wmi_ddv_debugfs_init(wdev);
> 
> -	return dell_wmi_ddv_battery_add(data);
> +	ret = dell_wmi_ddv_hwmon_add(data);
> +	if (ret < 0)
> +		dev_dbg(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
> +
> +	ret = dell_wmi_ddv_battery_add(data);
> +	if (ret < 0)
> +		dev_dbg(&wdev->dev, "Unable to register acpi battery hook: %d\n", ret);
> +

This used to be an error, but no longer is. Not my call to make
if this is acceptable, just pointing it out.

> +	return 0;
>  }
> 
>  static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
> --
> 2.30.2
> 

Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[Armin Wolf]

Am 27.01.23 um 14:08 schrieb Guenter Roeck:

> On Thu, Jan 26, 2023 at 08:40:21PM +0100, Armin Wolf wrote:
>> Thanks to bugreport 216655 on bugzilla triggered by the
>> dell-smm-hwmon driver, the contents of the sensor buffers
>> could be almost completely decoded.
>> Add an hwmon interface for exposing the fan and thermal
>> sensor values. The debugfs interface remains in place to
>> aid in reverse-engineering of unknown sensor types
>> and the thermal buffer.
>>
>> Tested-by: Antonín Skala <skala.antonin@gmail.com>
>> Tested-by: Gustavo Walbon <gustavowalbon@gmail.com>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/dell/Kconfig        |   1 +
>>   drivers/platform/x86/dell/dell-wmi-ddv.c | 435 ++++++++++++++++++++++-
>>   2 files changed, 435 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>> index d319de8f2132..21a74b63d9b1 100644
>> --- a/drivers/platform/x86/dell/Kconfig
>> +++ b/drivers/platform/x86/dell/Kconfig
>> @@ -194,6 +194,7 @@ config DELL_WMI_DDV
>>   	default m
>>   	depends on ACPI_BATTERY
>>   	depends on ACPI_WMI
>> +	depends on HWMON
> Not sure if adding such a dependency is a good idea.
> Up to the maintainer to decide. Personally I would prefer
> something like
> 	depends on HWMON || HWMON=n
> and some conditionals in the code, as it is done with other drivers
> outside the hwmon directory.
>
Good point, i will include this in the next patch revision.

>>   	help
>>   	  This option adds support for WMI-based sensors like
>>   	  battery temperature sensors found on some Dell notebooks.
>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
>> index 9695bf493ea6..5b30bb85199e 100644
>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/dev_printk.h>
>>   #include <linux/errno.h>
>>   #include <linux/kernel.h>
>> +#include <linux/hwmon.h>
>>   #include <linux/kstrtox.h>
>>   #include <linux/math.h>
>>   #include <linux/module.h>
>> @@ -21,10 +22,13 @@
>>   #include <linux/printk.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/sysfs.h>
>> +#include <linux/types.h>
>>   #include <linux/wmi.h>
>>
>>   #include <acpi/battery.h>
>>
>> +#include <asm/unaligned.h>
>> +
>>   #define DRIVER_NAME	"dell-wmi-ddv"
>>
>>   #define DELL_DDV_SUPPORTED_VERSION_MIN	2
>> @@ -63,6 +67,29 @@ enum dell_ddv_method {
>>   	DELL_DDV_THERMAL_SENSOR_INFORMATION	= 0x22,
>>   };
>>
>> +struct fan_sensor_entry {
>> +	u8 type;
>> +	__le16 rpm;
>> +} __packed;
>> +
>> +struct thermal_sensor_entry {
>> +	u8 type;
>> +	s8 now;
>> +	s8 min;
>> +	s8 max;
>> +	u8 unknown;
>> +} __packed;
>> +
>> +struct combined_channel_info {
>> +	struct hwmon_channel_info info;
>> +	u32 config[];
>> +};
>> +
>> +struct combined_chip_info {
>> +	struct hwmon_chip_info chip;
>> +	const struct hwmon_channel_info *info[];
>> +};
>> +
>>   struct dell_wmi_ddv_data {
>>   	struct acpi_battery_hook hook;
>>   	struct device_attribute temp_attr;
>> @@ -70,6 +97,24 @@ struct dell_wmi_ddv_data {
>>   	struct wmi_device *wdev;
>>   };
>>
>> +static const char * const fan_labels[] = {
>> +	"CPU Fan",
>> +	"Chassis Motherboard Fan",
>> +	"Video Fan",
>> +	"Power Supply Fan",
>> +	"Chipset Fan",
>> +	"Memory Fan",
>> +	"PCI Fan",
>> +	"HDD Fan",
>> +};
>> +
>> +static const char * const fan_dock_labels[] = {
>> +	"Docking Chassis/Motherboard Fan",
>> +	"Docking Video Fan",
>> +	"Docking Power Supply Fan",
>> +	"Docking Chipset Fan",
>> +};
>> +
>>   static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
>>   				   union acpi_object **result, acpi_object_type type)
>>   {
>> @@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
>>   	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
>>   }
>>
>> +static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
>> +				      size_t entry_size, union acpi_object **result, u64 *count)
>> +{
>> +	union acpi_object *obj;
>> +	u64 buffer_size;
>> +	u8 *buffer;
>> +	int ret;
>> +
>> +	ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	buffer_size = obj->package.elements[0].integer.value;
>> +	buffer = obj->package.elements[1].buffer.pointer;
>> +	if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != 0xff) {
>> +		kfree(obj);
>> +
> Stray empty line
>
>> +		return -ENOMSG;
>> +	}
>> +
>> +	*count = (buffer_size - 1) / entry_size;
>> +	*result = obj;
>> +
>> +	return 0;
>> +}
>> +
>> +static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
>> +				       int channel)
>> +{
>> +	return 0444;
>> +}
>> +
>> +static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
>> +					 long *val)
>> +{
>> +	struct fan_sensor_entry *entry;
>> +	union acpi_object *obj;
>> +	u64 count;
>> +	int ret;
>> +
>> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>> +					 sizeof(*entry), &obj, &count);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
>> +	if (count > channel) {
>> +		switch (attr) {
>> +		case hwmon_fan_input:
>> +			*val = get_unaligned_le16(&entry[channel].rpm);
>> +
> Another stray empty line. I see that "empty line before return or break"
> is common. Looks odd to me, and I don't see the point (it confuses
> the code flow for me and lets my brain focus on the empty line instead
> of the code in question), but I guess that is PoV. I won't comment on
> it further and let the maintainer decide.
>
>> +			break;
>> +		default:
>> +			ret = -EOPNOTSUPP;
>> +		}
>> +	} else {
>> +		ret = -ENXIO;
>> +	}
> Error handling should come first.
>> +
>> +	kfree(obj);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
>> +					  long *val)
>> +{
>> +	struct thermal_sensor_entry *entry;
>> +	union acpi_object *obj;
>> +	u64 count;
>> +	int ret;
>> +
>> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>> +					 sizeof(*entry), &obj, &count);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
>> +	if (count > channel) {
> This is a bit of Joda programming. It is really "channel < count",
> ie the requested channel number is in the range of channels reported
> by the WMI code. PoV, of course, but I find that the above makes the
> code more difficult to read.
>> +		switch (attr) {
>> +		case hwmon_temp_input:
>> +			*val = entry[channel].now * 1000;
>> +
>> +			break;
>> +		case hwmon_temp_min:
>> +			*val = entry[channel].min * 1000;
>> +
>> +			break;
>> +		case hwmon_temp_max:
>> +			*val = entry[channel].max * 1000;
>> +
>> +			break;
>> +		default:
>> +			ret = -EOPNOTSUPP;
> break; missing
>
>> +		}
>> +	} else {
>> +		ret = -ENXIO;
>> +	}
> Same as above - error handling should come first.
>
>> +
>> +	kfree(obj);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dell_wmi_ddv_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +			     int channel, long *val)
>> +{
>> +	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>> +
>> +	switch (type) {
>> +	case hwmon_fan:
>> +		return dell_wmi_ddv_fan_read_channel(data, attr, channel, val);
>> +	case hwmon_temp:
>> +		return dell_wmi_ddv_temp_read_channel(data, attr, channel, val);
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int channel,
>> +					const char **str)
>> +{
>> +	struct fan_sensor_entry *entry;
>> +	union acpi_object *obj;
>> +	u64 count;
>> +	u8 type;
>> +	int ret;
>> +
>> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>> +					 sizeof(*entry), &obj, &count);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
>> +	if (count > channel) {
>> +		type = entry[channel].type;
>> +
>> +		switch (type) {
>> +		case 0x00 ... 0x07:
>> +			*str = fan_labels[type];
>> +
>> +			break;
>> +		case 0x11 ... 0x14:
>> +			*str = fan_dock_labels[type - 0x11];
>> +
>> +			break;
>> +		default:
>> +			*str = "Unknown Fan";
> break; missing.
>
>> +		}
>> +	} else {
>> +		ret = -ENXIO;
>> +	}
> And again.
>
>> +
>> +	kfree(obj);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int channel,
>> +					 const char **str)
>> +{
>> +	struct thermal_sensor_entry *entry;
>> +	union acpi_object *obj;
>> +	u64 count;
>> +	int ret;
>> +
>> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>> +					 sizeof(*entry), &obj, &count);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
>> +	if (count > channel) {
>> +		switch (entry[channel].type) {
>> +		case 0x00:
>> +			*str = "CPU";
>> +
>> +			break;
>> +		case 0x11:
>> +			*str = "Video";
>> +
>> +			break;
>> +		case 0x22:
>> +			*str = "Memory"; // sometimes called DIMM
> Personally I don't permit C++ style comments in a hwmon driver
> unless _all_ comments are C++ style. Just a remark here.
>
>> +
>> +			break;
>> +		case 0x33:
>> +			*str = "Other";
>> +
>> +			break;
>> +		case 0x44:
>> +			*str = "Ambient"; // sometimes called SKIN
>> +
>> +			break;
>> +		case 0x52:
>> +			*str = "SODIMM";
>> +
>> +			break;
>> +		case 0x55:
>> +			*str = "HDD";
>> +
>> +			break;
>> +		case 0x62:
>> +			*str = "SODIMM 2";
>> +
>> +			break;
>> +		case 0x73:
>> +			*str = "NB";
>> +
>> +			break;
>> +		case 0x83:
>> +			*str = "Charger";
>> +
>> +			break;
>> +		case 0xbb:
>> +			*str = "Memory 3";
>> +
>> +			break;
>> +		default:
>> +			*str = "Unknown";
> break; missing
> Ok, I guess this is on purpose. I personally don't permit
> that since it always leaves the question if it was on purpose or not.
>
>> +		}
>> +	} else {
>> +		ret = -ENXIO;
>> +	}
>> +
>> +	kfree(obj);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dell_wmi_ddv_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +				    int channel, const char **str)
>> +{
>> +	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>> +
>> +	switch (type) {
>> +	case hwmon_fan:
>> +		switch (attr) {
>> +		case hwmon_fan_label:
>> +			return dell_wmi_ddv_fan_read_string(data, channel, str);
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	case hwmon_temp:
>> +		switch (attr) {
>> +		case hwmon_temp_label:
>> +			return dell_wmi_ddv_temp_read_string(data, channel, str);
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_ops dell_wmi_ddv_ops = {
>> +	.is_visible = dell_wmi_ddv_is_visible,
>> +	.read = dell_wmi_ddv_read,
>> +	.read_string = dell_wmi_ddv_read_string,
>> +};
>> +
>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev, u64 count,
>> +							      enum hwmon_sensor_types type,
>> +							      u32 config)
>> +{
>> +	struct combined_channel_info *cinfo;
>> +	int i;
>> +
>> +	cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + 1), GFP_KERNEL);
>> +	if (!cinfo)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	cinfo->info.type = type;
>> +	cinfo->info.config = cinfo->config;
>> +
>> +	for (i = 0; i < count; i++)
>> +		cinfo->config[i] = config;
>> +
>> +	return &cinfo->info;
>> +}
>> +
>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
>> +							    enum dell_ddv_method method,
>> +							    size_t entry_size,
>> +							    enum hwmon_sensor_types type,
>> +							    u32 config)
>> +{
>> +	union acpi_object *obj;
>> +	u64 count;
>> +	int ret;
>> +
>> +	ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, &obj, &count);
>> +	if (ret < 0)
>> +		return ERR_PTR(ret);
>> +
>> +	kfree(obj);
>> +
>> +	if (!count)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	return dell_wmi_ddv_channel_create(&wdev->dev, count, type, config);
>> +}
>> +
>> +static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data)
>> +{
>> +	struct wmi_device *wdev = data->wdev;
>> +	struct combined_chip_info *cinfo;
>> +	struct device *hdev;
>> +	int index = 0;
>> +	int ret;
>> +
>> +	if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, GFP_KERNEL))
>> +		return -ENOMEM;
>> +
>> +	cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), GFP_KERNEL);
>> +	if (!cinfo) {
>> +		ret = -ENOMEM;
>> +
>> +		goto err_release;
>> +	}
>> +
>> +	cinfo->chip.ops = &dell_wmi_ddv_ops;
>> +	cinfo->chip.info = cinfo->info;
>> +
>> +	cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, hwmon_chip,
>> +							 HWMON_C_REGISTER_TZ);
>> +
>> +	if (IS_ERR(cinfo->info[index])) {
>> +		ret = PTR_ERR(cinfo->info[index]);
>> +
>> +		goto err_release;
>> +	}
>> +
>> +	index++;
>> +
>> +	cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>> +						       sizeof(struct fan_sensor_entry), hwmon_fan,
>> +						       (HWMON_F_INPUT | HWMON_F_LABEL));
>> +	if (!IS_ERR(cinfo->info[index]))
>> +		index++;
>> +
>> +	cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>> +						       sizeof(struct thermal_sensor_entry),
>> +						       hwmon_temp, (HWMON_T_INPUT | HWMON_T_MIN |
>> +						       HWMON_T_MAX | HWMON_T_LABEL));
>> +	if (!IS_ERR(cinfo->info[index]))
>> +		index++;
>> +
>> +	if (!index) {
>> +		ret = -ENODEV;
>> +
>> +		goto err_release;
>> +	}
>> +
>> +	cinfo->info[index] = NULL;
>> +
>> +	hdev = devm_hwmon_device_register_with_info(&wdev->dev, "dell_ddv", data, &cinfo->chip,
>> +						    NULL);
>> +	if (IS_ERR(hdev)) {
>> +		ret = PTR_ERR(hdev);
>> +
>> +		goto err_release;
>> +	}
>> +
>> +	devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>> +
>> +	return 0;
>> +
>> +err_release:
>> +	devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>> +
>> +	return ret;
>> +}
>> +
>>   static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
>>   {
>>   	const char *uid_str;
>> @@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
>>
>>   	dell_wmi_ddv_debugfs_init(wdev);
>>
>> -	return dell_wmi_ddv_battery_add(data);
>> +	ret = dell_wmi_ddv_hwmon_add(data);
>> +	if (ret < 0)
>> +		dev_dbg(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
>> +
>> +	ret = dell_wmi_ddv_battery_add(data);
>> +	if (ret < 0)
>> +		dev_dbg(&wdev->dev, "Unable to register acpi battery hook: %d\n", ret);
>> +
> This used to be an error, but no longer is. Not my call to make
> if this is acceptable, just pointing it out.

I decided to not treat the battery hook as essential function anymore because
the battery hook and hwmon functionality are more or less disconnected from
each other, so having the driver abort probing just because one functionality
could not be initialized seemed unreasonable to me.

I already thought about putting the battery hook and hwmon functionality into
separate drivers, with the main driver registering a MFD device or something similar.
Because apart from some generic routines, both functions are not connected in any way.

Is it acceptable to split the driver for such a thing?

Armin Wolf

>> +	return 0;
>>   }
>>
>>   static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
>> --
>> 2.30.2
>>

Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[Armin Wolf]

Am 27.01.23 um 17:09 schrieb Armin Wolf:

> Am 27.01.23 um 14:08 schrieb Guenter Roeck:
>
>> On Thu, Jan 26, 2023 at 08:40:21PM +0100, Armin Wolf wrote:
>>> Thanks to bugreport 216655 on bugzilla triggered by the
>>> dell-smm-hwmon driver, the contents of the sensor buffers
>>> could be almost completely decoded.
>>> Add an hwmon interface for exposing the fan and thermal
>>> sensor values. The debugfs interface remains in place to
>>> aid in reverse-engineering of unknown sensor types
>>> and the thermal buffer.
>>>
>>> Tested-by: Antonín Skala <skala.antonin@gmail.com>
>>> Tested-by: Gustavo Walbon <gustavowalbon@gmail.com>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>>   drivers/platform/x86/dell/Kconfig        |   1 +
>>>   drivers/platform/x86/dell/dell-wmi-ddv.c | 435 
>>> ++++++++++++++++++++++-
>>>   2 files changed, 435 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/x86/dell/Kconfig 
>>> b/drivers/platform/x86/dell/Kconfig
>>> index d319de8f2132..21a74b63d9b1 100644
>>> --- a/drivers/platform/x86/dell/Kconfig
>>> +++ b/drivers/platform/x86/dell/Kconfig
>>> @@ -194,6 +194,7 @@ config DELL_WMI_DDV
>>>       default m
>>>       depends on ACPI_BATTERY
>>>       depends on ACPI_WMI
>>> +    depends on HWMON
>> Not sure if adding such a dependency is a good idea.
>> Up to the maintainer to decide. Personally I would prefer
>> something like
>>     depends on HWMON || HWMON=n
>> and some conditionals in the code, as it is done with other drivers
>> outside the hwmon directory.
>>
> Good point, i will include this in the next patch revision.
>
>>>       help
>>>         This option adds support for WMI-based sensors like
>>>         battery temperature sensors found on some Dell notebooks.
>>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c 
>>> b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> index 9695bf493ea6..5b30bb85199e 100644
>>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> @@ -13,6 +13,7 @@
>>>   #include <linux/dev_printk.h>
>>>   #include <linux/errno.h>
>>>   #include <linux/kernel.h>
>>> +#include <linux/hwmon.h>
>>>   #include <linux/kstrtox.h>
>>>   #include <linux/math.h>
>>>   #include <linux/module.h>
>>> @@ -21,10 +22,13 @@
>>>   #include <linux/printk.h>
>>>   #include <linux/seq_file.h>
>>>   #include <linux/sysfs.h>
>>> +#include <linux/types.h>
>>>   #include <linux/wmi.h>
>>>
>>>   #include <acpi/battery.h>
>>>
>>> +#include <asm/unaligned.h>
>>> +
>>>   #define DRIVER_NAME    "dell-wmi-ddv"
>>>
>>>   #define DELL_DDV_SUPPORTED_VERSION_MIN    2
>>> @@ -63,6 +67,29 @@ enum dell_ddv_method {
>>>       DELL_DDV_THERMAL_SENSOR_INFORMATION    = 0x22,
>>>   };
>>>
>>> +struct fan_sensor_entry {
>>> +    u8 type;
>>> +    __le16 rpm;
>>> +} __packed;
>>> +
>>> +struct thermal_sensor_entry {
>>> +    u8 type;
>>> +    s8 now;
>>> +    s8 min;
>>> +    s8 max;
>>> +    u8 unknown;
>>> +} __packed;
>>> +
>>> +struct combined_channel_info {
>>> +    struct hwmon_channel_info info;
>>> +    u32 config[];
>>> +};
>>> +
>>> +struct combined_chip_info {
>>> +    struct hwmon_chip_info chip;
>>> +    const struct hwmon_channel_info *info[];
>>> +};
>>> +
>>>   struct dell_wmi_ddv_data {
>>>       struct acpi_battery_hook hook;
>>>       struct device_attribute temp_attr;
>>> @@ -70,6 +97,24 @@ struct dell_wmi_ddv_data {
>>>       struct wmi_device *wdev;
>>>   };
>>>
>>> +static const char * const fan_labels[] = {
>>> +    "CPU Fan",
>>> +    "Chassis Motherboard Fan",
>>> +    "Video Fan",
>>> +    "Power Supply Fan",
>>> +    "Chipset Fan",
>>> +    "Memory Fan",
>>> +    "PCI Fan",
>>> +    "HDD Fan",
>>> +};
>>> +
>>> +static const char * const fan_dock_labels[] = {
>>> +    "Docking Chassis/Motherboard Fan",
>>> +    "Docking Video Fan",
>>> +    "Docking Power Supply Fan",
>>> +    "Docking Chipset Fan",
>>> +};
>>> +
>>>   static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum 
>>> dell_ddv_method method, u32 arg,
>>>                      union acpi_object **result, acpi_object_type type)
>>>   {
>>> @@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct 
>>> wmi_device *wdev, enum dell_ddv_meth
>>>       return dell_wmi_ddv_query_type(wdev, method, arg, result, 
>>> ACPI_TYPE_STRING);
>>>   }
>>>
>>> +static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum 
>>> dell_ddv_method method,
>>> +                      size_t entry_size, union acpi_object 
>>> **result, u64 *count)
>>> +{
>>> +    union acpi_object *obj;
>>> +    u64 buffer_size;
>>> +    u8 *buffer;
>>> +    int ret;
>>> +
>>> +    ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    buffer_size = obj->package.elements[0].integer.value;
>>> +    buffer = obj->package.elements[1].buffer.pointer;
>>> +    if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != 
>>> 0xff) {
>>> +        kfree(obj);
>>> +
>> Stray empty line
>>
>>> +        return -ENOMSG;
>>> +    }
>>> +
>>> +    *count = (buffer_size - 1) / entry_size;
>>> +    *result = obj;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum 
>>> hwmon_sensor_types type, u32 attr,
>>> +                       int channel)
>>> +{
>>> +    return 0444;
>>> +}
>>> +
>>> +static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data 
>>> *data, u32 attr, int channel,
>>> +                     long *val)
>>> +{
>>> +    struct fan_sensor_entry *entry;
>>> +    union acpi_object *obj;
>>> +    u64 count;
>>> +    int ret;
>>> +
>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, 
>>> DELL_DDV_FAN_SENSOR_INFORMATION,
>>> +                     sizeof(*entry), &obj, &count);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    entry = (struct fan_sensor_entry 
>>> *)obj->package.elements[1].buffer.pointer;
>>> +    if (count > channel) {
>>> +        switch (attr) {
>>> +        case hwmon_fan_input:
>>> +            *val = get_unaligned_le16(&entry[channel].rpm);
>>> +
>> Another stray empty line. I see that "empty line before return or break"
>> is common. Looks odd to me, and I don't see the point (it confuses
>> the code flow for me and lets my brain focus on the empty line instead
>> of the code in question), but I guess that is PoV. I won't comment on
>> it further and let the maintainer decide.
>>
>>> +            break;
>>> +        default:
>>> +            ret = -EOPNOTSUPP;
>>> +        }
>>> +    } else {
>>> +        ret = -ENXIO;
>>> +    }
>> Error handling should come first.
>>> +
>>> +    kfree(obj);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data 
>>> *data, u32 attr, int channel,
>>> +                      long *val)
>>> +{
>>> +    struct thermal_sensor_entry *entry;
>>> +    union acpi_object *obj;
>>> +    u64 count;
>>> +    int ret;
>>> +
>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, 
>>> DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>> +                     sizeof(*entry), &obj, &count);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    entry = (struct thermal_sensor_entry 
>>> *)obj->package.elements[1].buffer.pointer;
>>> +    if (count > channel) {
>> This is a bit of Joda programming. It is really "channel < count",
>> ie the requested channel number is in the range of channels reported
>> by the WMI code. PoV, of course, but I find that the above makes the
>> code more difficult to read.
>>> +        switch (attr) {
>>> +        case hwmon_temp_input:
>>> +            *val = entry[channel].now * 1000;
>>> +
>>> +            break;
>>> +        case hwmon_temp_min:
>>> +            *val = entry[channel].min * 1000;
>>> +
>>> +            break;
>>> +        case hwmon_temp_max:
>>> +            *val = entry[channel].max * 1000;
>>> +
>>> +            break;
>>> +        default:
>>> +            ret = -EOPNOTSUPP;
>> break; missing
>>
>>> +        }
>>> +    } else {
>>> +        ret = -ENXIO;
>>> +    }
>> Same as above - error handling should come first.
>>
>>> +
>>> +    kfree(obj);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int dell_wmi_ddv_read(struct device *dev, enum 
>>> hwmon_sensor_types type, u32 attr,
>>> +                 int channel, long *val)
>>> +{
>>> +    struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>> +
>>> +    switch (type) {
>>> +    case hwmon_fan:
>>> +        return dell_wmi_ddv_fan_read_channel(data, attr, channel, 
>>> val);
>>> +    case hwmon_temp:
>>> +        return dell_wmi_ddv_temp_read_channel(data, attr, channel, 
>>> val);
>>> +    default:
>>> +        break;
>>> +    }
>>> +
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data 
>>> *data, int channel,
>>> +                    const char **str)
>>> +{
>>> +    struct fan_sensor_entry *entry;
>>> +    union acpi_object *obj;
>>> +    u64 count;
>>> +    u8 type;
>>> +    int ret;
>>> +
>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, 
>>> DELL_DDV_FAN_SENSOR_INFORMATION,
>>> +                     sizeof(*entry), &obj, &count);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    entry = (struct fan_sensor_entry 
>>> *)obj->package.elements[1].buffer.pointer;
>>> +    if (count > channel) {
>>> +        type = entry[channel].type;
>>> +
>>> +        switch (type) {
>>> +        case 0x00 ... 0x07:
>>> +            *str = fan_labels[type];
>>> +
>>> +            break;
>>> +        case 0x11 ... 0x14:
>>> +            *str = fan_dock_labels[type - 0x11];
>>> +
>>> +            break;
>>> +        default:
>>> +            *str = "Unknown Fan";
>> break; missing.
>>
>>> +        }
>>> +    } else {
>>> +        ret = -ENXIO;
>>> +    }
>> And again.
>>
>>> +
>>> +    kfree(obj);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data 
>>> *data, int channel,
>>> +                     const char **str)
>>> +{
>>> +    struct thermal_sensor_entry *entry;
>>> +    union acpi_object *obj;
>>> +    u64 count;
>>> +    int ret;
>>> +
>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, 
>>> DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>> +                     sizeof(*entry), &obj, &count);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    entry = (struct thermal_sensor_entry 
>>> *)obj->package.elements[1].buffer.pointer;
>>> +    if (count > channel) {
>>> +        switch (entry[channel].type) {
>>> +        case 0x00:
>>> +            *str = "CPU";
>>> +
>>> +            break;
>>> +        case 0x11:
>>> +            *str = "Video";
>>> +
>>> +            break;
>>> +        case 0x22:
>>> +            *str = "Memory"; // sometimes called DIMM
>> Personally I don't permit C++ style comments in a hwmon driver
>> unless _all_ comments are C++ style. Just a remark here.
>>
>>> +
>>> +            break;
>>> +        case 0x33:
>>> +            *str = "Other";
>>> +
>>> +            break;
>>> +        case 0x44:
>>> +            *str = "Ambient"; // sometimes called SKIN
>>> +
>>> +            break;
>>> +        case 0x52:
>>> +            *str = "SODIMM";
>>> +
>>> +            break;
>>> +        case 0x55:
>>> +            *str = "HDD";
>>> +
>>> +            break;
>>> +        case 0x62:
>>> +            *str = "SODIMM 2";
>>> +
>>> +            break;
>>> +        case 0x73:
>>> +            *str = "NB";
>>> +
>>> +            break;
>>> +        case 0x83:
>>> +            *str = "Charger";
>>> +
>>> +            break;
>>> +        case 0xbb:
>>> +            *str = "Memory 3";
>>> +
>>> +            break;
>>> +        default:
>>> +            *str = "Unknown";
>> break; missing
>> Ok, I guess this is on purpose. I personally don't permit
>> that since it always leaves the question if it was on purpose or not.
>>
>>> +        }
>>> +    } else {
>>> +        ret = -ENXIO;
>>> +    }
>>> +
>>> +    kfree(obj);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int dell_wmi_ddv_read_string(struct device *dev, enum 
>>> hwmon_sensor_types type, u32 attr,
>>> +                    int channel, const char **str)
>>> +{
>>> +    struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>> +
>>> +    switch (type) {
>>> +    case hwmon_fan:
>>> +        switch (attr) {
>>> +        case hwmon_fan_label:
>>> +            return dell_wmi_ddv_fan_read_string(data, channel, str);
>>> +        default:
>>> +            break;
>>> +        }
>>> +        break;
>>> +    case hwmon_temp:
>>> +        switch (attr) {
>>> +        case hwmon_temp_label:
>>> +            return dell_wmi_ddv_temp_read_string(data, channel, str);
>>> +        default:
>>> +            break;
>>> +        }
>>> +        break;
>>> +    default:
>>> +        break;
>>> +    }
>>> +
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static const struct hwmon_ops dell_wmi_ddv_ops = {
>>> +    .is_visible = dell_wmi_ddv_is_visible,
>>> +    .read = dell_wmi_ddv_read,
>>> +    .read_string = dell_wmi_ddv_read_string,
>>> +};
>>> +
>>> +static struct hwmon_channel_info 
>>> *dell_wmi_ddv_channel_create(struct device *dev, u64 count,
>>> +                                  enum hwmon_sensor_types type,
>>> +                                  u32 config)
>>> +{
>>> +    struct combined_channel_info *cinfo;
>>> +    int i;
>>> +
>>> +    cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + 
>>> 1), GFP_KERNEL);
>>> +    if (!cinfo)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    cinfo->info.type = type;
>>> +    cinfo->info.config = cinfo->config;
>>> +
>>> +    for (i = 0; i < count; i++)
>>> +        cinfo->config[i] = config;
>>> +
>>> +    return &cinfo->info;
>>> +}
>>> +
>>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct 
>>> wmi_device *wdev,
>>> +                                enum dell_ddv_method method,
>>> +                                size_t entry_size,
>>> +                                enum hwmon_sensor_types type,
>>> +                                u32 config)
>>> +{
>>> +    union acpi_object *obj;
>>> +    u64 count;
>>> +    int ret;
>>> +
>>> +    ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, 
>>> &obj, &count);
>>> +    if (ret < 0)
>>> +        return ERR_PTR(ret);
>>> +
>>> +    kfree(obj);
>>> +
>>> +    if (!count)
>>> +        return ERR_PTR(-ENODEV);
>>> +
>>> +    return dell_wmi_ddv_channel_create(&wdev->dev, count, type, 
>>> config);
>>> +}
>>> +
>>> +static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data)
>>> +{
>>> +    struct wmi_device *wdev = data->wdev;
>>> +    struct combined_chip_info *cinfo;
>>> +    struct device *hdev;
>>> +    int index = 0;
>>> +    int ret;
>>> +
>>> +    if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, 
>>> GFP_KERNEL))
>>> +        return -ENOMEM;
>>> +
>>> +    cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), 
>>> GFP_KERNEL);
>>> +    if (!cinfo) {
>>> +        ret = -ENOMEM;
>>> +
>>> +        goto err_release;
>>> +    }
>>> +
>>> +    cinfo->chip.ops = &dell_wmi_ddv_ops;
>>> +    cinfo->chip.info = cinfo->info;
>>> +
>>> +    cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, 
>>> hwmon_chip,
>>> +                             HWMON_C_REGISTER_TZ);
>>> +
>>> +    if (IS_ERR(cinfo->info[index])) {
>>> +        ret = PTR_ERR(cinfo->info[index]);
>>> +
>>> +        goto err_release;
>>> +    }
>>> +
>>> +    index++;
>>> +
>>> +    cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, 
>>> DELL_DDV_FAN_SENSOR_INFORMATION,
>>> +                               sizeof(struct fan_sensor_entry), 
>>> hwmon_fan,
>>> +                               (HWMON_F_INPUT | HWMON_F_LABEL));
>>> +    if (!IS_ERR(cinfo->info[index]))
>>> +        index++;
>>> +
>>> +    cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, 
>>> DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>> +                               sizeof(struct thermal_sensor_entry),
>>> +                               hwmon_temp, (HWMON_T_INPUT | 
>>> HWMON_T_MIN |
>>> +                               HWMON_T_MAX | HWMON_T_LABEL));
>>> +    if (!IS_ERR(cinfo->info[index]))
>>> +        index++;
>>> +
>>> +    if (!index) {
>>> +        ret = -ENODEV;
>>> +
>>> +        goto err_release;
>>> +    }
>>> +
>>> +    cinfo->info[index] = NULL;
>>> +
>>> +    hdev = devm_hwmon_device_register_with_info(&wdev->dev, 
>>> "dell_ddv", data, &cinfo->chip,
>>> +                            NULL);
>>> +    if (IS_ERR(hdev)) {
>>> +        ret = PTR_ERR(hdev);
>>> +
>>> +        goto err_release;
>>> +    }
>>> +
>>> +    devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>>> +
>>> +    return 0;
>>> +
>>> +err_release:
>>> +    devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static int dell_wmi_ddv_battery_index(struct acpi_device 
>>> *acpi_dev, u32 *index)
>>>   {
>>>       const char *uid_str;
>>> @@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device 
>>> *wdev, const void *context)
>>>
>>>       dell_wmi_ddv_debugfs_init(wdev);
>>>
>>> -    return dell_wmi_ddv_battery_add(data);
>>> +    ret = dell_wmi_ddv_hwmon_add(data);
>>> +    if (ret < 0)
>>> +        dev_dbg(&wdev->dev, "Unable to register hwmon interface: 
>>> %d\n", ret);
>>> +
>>> +    ret = dell_wmi_ddv_battery_add(data);
>>> +    if (ret < 0)
>>> +        dev_dbg(&wdev->dev, "Unable to register acpi battery hook: 
>>> %d\n", ret);
>>> +
>> This used to be an error, but no longer is. Not my call to make
>> if this is acceptable, just pointing it out.
>
> I decided to not treat the battery hook as essential function anymore 
> because
> the battery hook and hwmon functionality are more or less disconnected 
> from
> each other, so having the driver abort probing just because one 
> functionality
> could not be initialized seemed unreasonable to me.
>
> I already thought about putting the battery hook and hwmon 
> functionality into
> separate drivers, with the main driver registering a MFD device or 
> something similar.
> Because apart from some generic routines, both functions are not 
> connected in any way.
>
> Is it acceptable to split the driver for such a thing?
>
> Armin Wolf
>
Any thoughts about this? Otherwise i will just use conditionals.

Armin Wolf

>>> +    return 0;
>>>   }
>>>
>>>   static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
>>> -- 
>>> 2.30.2
>>>

Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[Hans de Goede]

Hi,

On 2/2/23 22:12, Armin Wolf wrote:
> Am 27.01.23 um 17:09 schrieb Armin Wolf:
> 
>> Am 27.01.23 um 14:08 schrieb Guenter Roeck:
>>
>>> On Thu, Jan 26, 2023 at 08:40:21PM +0100, Armin Wolf wrote:
>>>> Thanks to bugreport 216655 on bugzilla triggered by the
>>>> dell-smm-hwmon driver, the contents of the sensor buffers
>>>> could be almost completely decoded.
>>>> Add an hwmon interface for exposing the fan and thermal
>>>> sensor values. The debugfs interface remains in place to
>>>> aid in reverse-engineering of unknown sensor types
>>>> and the thermal buffer.
>>>>
>>>> Tested-by: Antonín Skala <skala.antonin@gmail.com>
>>>> Tested-by: Gustavo Walbon <gustavowalbon@gmail.com>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> ---
>>>>   drivers/platform/x86/dell/Kconfig        |   1 +
>>>>   drivers/platform/x86/dell/dell-wmi-ddv.c | 435 ++++++++++++++++++++++-
>>>>   2 files changed, 435 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>>>> index d319de8f2132..21a74b63d9b1 100644
>>>> --- a/drivers/platform/x86/dell/Kconfig
>>>> +++ b/drivers/platform/x86/dell/Kconfig
>>>> @@ -194,6 +194,7 @@ config DELL_WMI_DDV
>>>>       default m
>>>>       depends on ACPI_BATTERY
>>>>       depends on ACPI_WMI
>>>> +    depends on HWMON
>>> Not sure if adding such a dependency is a good idea.
>>> Up to the maintainer to decide. Personally I would prefer
>>> something like
>>>     depends on HWMON || HWMON=n
>>> and some conditionals in the code, as it is done with other drivers
>>> outside the hwmon directory.
>>>
>> Good point, i will include this in the next patch revision.
>>
>>>>       help
>>>>         This option adds support for WMI-based sensors like
>>>>         battery temperature sensors found on some Dell notebooks.
>>>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>> index 9695bf493ea6..5b30bb85199e 100644
>>>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>> @@ -13,6 +13,7 @@
>>>>   #include <linux/dev_printk.h>
>>>>   #include <linux/errno.h>
>>>>   #include <linux/kernel.h>
>>>> +#include <linux/hwmon.h>
>>>>   #include <linux/kstrtox.h>
>>>>   #include <linux/math.h>
>>>>   #include <linux/module.h>
>>>> @@ -21,10 +22,13 @@
>>>>   #include <linux/printk.h>
>>>>   #include <linux/seq_file.h>
>>>>   #include <linux/sysfs.h>
>>>> +#include <linux/types.h>
>>>>   #include <linux/wmi.h>
>>>>
>>>>   #include <acpi/battery.h>
>>>>
>>>> +#include <asm/unaligned.h>
>>>> +
>>>>   #define DRIVER_NAME    "dell-wmi-ddv"
>>>>
>>>>   #define DELL_DDV_SUPPORTED_VERSION_MIN    2
>>>> @@ -63,6 +67,29 @@ enum dell_ddv_method {
>>>>       DELL_DDV_THERMAL_SENSOR_INFORMATION    = 0x22,
>>>>   };
>>>>
>>>> +struct fan_sensor_entry {
>>>> +    u8 type;
>>>> +    __le16 rpm;
>>>> +} __packed;
>>>> +
>>>> +struct thermal_sensor_entry {
>>>> +    u8 type;
>>>> +    s8 now;
>>>> +    s8 min;
>>>> +    s8 max;
>>>> +    u8 unknown;
>>>> +} __packed;
>>>> +
>>>> +struct combined_channel_info {
>>>> +    struct hwmon_channel_info info;
>>>> +    u32 config[];
>>>> +};
>>>> +
>>>> +struct combined_chip_info {
>>>> +    struct hwmon_chip_info chip;
>>>> +    const struct hwmon_channel_info *info[];
>>>> +};
>>>> +
>>>>   struct dell_wmi_ddv_data {
>>>>       struct acpi_battery_hook hook;
>>>>       struct device_attribute temp_attr;
>>>> @@ -70,6 +97,24 @@ struct dell_wmi_ddv_data {
>>>>       struct wmi_device *wdev;
>>>>   };
>>>>
>>>> +static const char * const fan_labels[] = {
>>>> +    "CPU Fan",
>>>> +    "Chassis Motherboard Fan",
>>>> +    "Video Fan",
>>>> +    "Power Supply Fan",
>>>> +    "Chipset Fan",
>>>> +    "Memory Fan",
>>>> +    "PCI Fan",
>>>> +    "HDD Fan",
>>>> +};
>>>> +
>>>> +static const char * const fan_dock_labels[] = {
>>>> +    "Docking Chassis/Motherboard Fan",
>>>> +    "Docking Video Fan",
>>>> +    "Docking Power Supply Fan",
>>>> +    "Docking Chipset Fan",
>>>> +};
>>>> +
>>>>   static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
>>>>                      union acpi_object **result, acpi_object_type type)
>>>>   {
>>>> @@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
>>>>       return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
>>>>   }
>>>>
>>>> +static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
>>>> +                      size_t entry_size, union acpi_object **result, u64 *count)
>>>> +{
>>>> +    union acpi_object *obj;
>>>> +    u64 buffer_size;
>>>> +    u8 *buffer;
>>>> +    int ret;
>>>> +
>>>> +    ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    buffer_size = obj->package.elements[0].integer.value;
>>>> +    buffer = obj->package.elements[1].buffer.pointer;
>>>> +    if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != 0xff) {
>>>> +        kfree(obj);
>>>> +
>>> Stray empty line
>>>
>>>> +        return -ENOMSG;
>>>> +    }
>>>> +
>>>> +    *count = (buffer_size - 1) / entry_size;
>>>> +    *result = obj;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
>>>> +                       int channel)
>>>> +{
>>>> +    return 0444;
>>>> +}
>>>> +
>>>> +static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
>>>> +                     long *val)
>>>> +{
>>>> +    struct fan_sensor_entry *entry;
>>>> +    union acpi_object *obj;
>>>> +    u64 count;
>>>> +    int ret;
>>>> +
>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>> +                     sizeof(*entry), &obj, &count);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>> +    if (count > channel) {
>>>> +        switch (attr) {
>>>> +        case hwmon_fan_input:
>>>> +            *val = get_unaligned_le16(&entry[channel].rpm);
>>>> +
>>> Another stray empty line. I see that "empty line before return or break"
>>> is common. Looks odd to me, and I don't see the point (it confuses
>>> the code flow for me and lets my brain focus on the empty line instead
>>> of the code in question), but I guess that is PoV. I won't comment on
>>> it further and let the maintainer decide.
>>>
>>>> +            break;
>>>> +        default:
>>>> +            ret = -EOPNOTSUPP;
>>>> +        }
>>>> +    } else {
>>>> +        ret = -ENXIO;
>>>> +    }
>>> Error handling should come first.
>>>> +
>>>> +    kfree(obj);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
>>>> +                      long *val)
>>>> +{
>>>> +    struct thermal_sensor_entry *entry;
>>>> +    union acpi_object *obj;
>>>> +    u64 count;
>>>> +    int ret;
>>>> +
>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>> +                     sizeof(*entry), &obj, &count);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>> +    if (count > channel) {
>>> This is a bit of Joda programming. It is really "channel < count",
>>> ie the requested channel number is in the range of channels reported
>>> by the WMI code. PoV, of course, but I find that the above makes the
>>> code more difficult to read.
>>>> +        switch (attr) {
>>>> +        case hwmon_temp_input:
>>>> +            *val = entry[channel].now * 1000;
>>>> +
>>>> +            break;
>>>> +        case hwmon_temp_min:
>>>> +            *val = entry[channel].min * 1000;
>>>> +
>>>> +            break;
>>>> +        case hwmon_temp_max:
>>>> +            *val = entry[channel].max * 1000;
>>>> +
>>>> +            break;
>>>> +        default:
>>>> +            ret = -EOPNOTSUPP;
>>> break; missing
>>>
>>>> +        }
>>>> +    } else {
>>>> +        ret = -ENXIO;
>>>> +    }
>>> Same as above - error handling should come first.
>>>
>>>> +
>>>> +    kfree(obj);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int dell_wmi_ddv_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>> +                 int channel, long *val)
>>>> +{
>>>> +    struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>>> +
>>>> +    switch (type) {
>>>> +    case hwmon_fan:
>>>> +        return dell_wmi_ddv_fan_read_channel(data, attr, channel, val);
>>>> +    case hwmon_temp:
>>>> +        return dell_wmi_ddv_temp_read_channel(data, attr, channel, val);
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>> +static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int channel,
>>>> +                    const char **str)
>>>> +{
>>>> +    struct fan_sensor_entry *entry;
>>>> +    union acpi_object *obj;
>>>> +    u64 count;
>>>> +    u8 type;
>>>> +    int ret;
>>>> +
>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>> +                     sizeof(*entry), &obj, &count);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>> +    if (count > channel) {
>>>> +        type = entry[channel].type;
>>>> +
>>>> +        switch (type) {
>>>> +        case 0x00 ... 0x07:
>>>> +            *str = fan_labels[type];
>>>> +
>>>> +            break;
>>>> +        case 0x11 ... 0x14:
>>>> +            *str = fan_dock_labels[type - 0x11];
>>>> +
>>>> +            break;
>>>> +        default:
>>>> +            *str = "Unknown Fan";
>>> break; missing.
>>>
>>>> +        }
>>>> +    } else {
>>>> +        ret = -ENXIO;
>>>> +    }
>>> And again.
>>>
>>>> +
>>>> +    kfree(obj);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int channel,
>>>> +                     const char **str)
>>>> +{
>>>> +    struct thermal_sensor_entry *entry;
>>>> +    union acpi_object *obj;
>>>> +    u64 count;
>>>> +    int ret;
>>>> +
>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>> +                     sizeof(*entry), &obj, &count);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>> +    if (count > channel) {
>>>> +        switch (entry[channel].type) {
>>>> +        case 0x00:
>>>> +            *str = "CPU";
>>>> +
>>>> +            break;
>>>> +        case 0x11:
>>>> +            *str = "Video";
>>>> +
>>>> +            break;
>>>> +        case 0x22:
>>>> +            *str = "Memory"; // sometimes called DIMM
>>> Personally I don't permit C++ style comments in a hwmon driver
>>> unless _all_ comments are C++ style. Just a remark here.
>>>
>>>> +
>>>> +            break;
>>>> +        case 0x33:
>>>> +            *str = "Other";
>>>> +
>>>> +            break;
>>>> +        case 0x44:
>>>> +            *str = "Ambient"; // sometimes called SKIN
>>>> +
>>>> +            break;
>>>> +        case 0x52:
>>>> +            *str = "SODIMM";
>>>> +
>>>> +            break;
>>>> +        case 0x55:
>>>> +            *str = "HDD";
>>>> +
>>>> +            break;
>>>> +        case 0x62:
>>>> +            *str = "SODIMM 2";
>>>> +
>>>> +            break;
>>>> +        case 0x73:
>>>> +            *str = "NB";
>>>> +
>>>> +            break;
>>>> +        case 0x83:
>>>> +            *str = "Charger";
>>>> +
>>>> +            break;
>>>> +        case 0xbb:
>>>> +            *str = "Memory 3";
>>>> +
>>>> +            break;
>>>> +        default:
>>>> +            *str = "Unknown";
>>> break; missing
>>> Ok, I guess this is on purpose. I personally don't permit
>>> that since it always leaves the question if it was on purpose or not.
>>>
>>>> +        }
>>>> +    } else {
>>>> +        ret = -ENXIO;
>>>> +    }
>>>> +
>>>> +    kfree(obj);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int dell_wmi_ddv_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>> +                    int channel, const char **str)
>>>> +{
>>>> +    struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>>> +
>>>> +    switch (type) {
>>>> +    case hwmon_fan:
>>>> +        switch (attr) {
>>>> +        case hwmon_fan_label:
>>>> +            return dell_wmi_ddv_fan_read_string(data, channel, str);
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +        break;
>>>> +    case hwmon_temp:
>>>> +        switch (attr) {
>>>> +        case hwmon_temp_label:
>>>> +            return dell_wmi_ddv_temp_read_string(data, channel, str);
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +        break;
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>> +static const struct hwmon_ops dell_wmi_ddv_ops = {
>>>> +    .is_visible = dell_wmi_ddv_is_visible,
>>>> +    .read = dell_wmi_ddv_read,
>>>> +    .read_string = dell_wmi_ddv_read_string,
>>>> +};
>>>> +
>>>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev, u64 count,
>>>> +                                  enum hwmon_sensor_types type,
>>>> +                                  u32 config)
>>>> +{
>>>> +    struct combined_channel_info *cinfo;
>>>> +    int i;
>>>> +
>>>> +    cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + 1), GFP_KERNEL);
>>>> +    if (!cinfo)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    cinfo->info.type = type;
>>>> +    cinfo->info.config = cinfo->config;
>>>> +
>>>> +    for (i = 0; i < count; i++)
>>>> +        cinfo->config[i] = config;
>>>> +
>>>> +    return &cinfo->info;
>>>> +}
>>>> +
>>>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
>>>> +                                enum dell_ddv_method method,
>>>> +                                size_t entry_size,
>>>> +                                enum hwmon_sensor_types type,
>>>> +                                u32 config)
>>>> +{
>>>> +    union acpi_object *obj;
>>>> +    u64 count;
>>>> +    int ret;
>>>> +
>>>> +    ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, &obj, &count);
>>>> +    if (ret < 0)
>>>> +        return ERR_PTR(ret);
>>>> +
>>>> +    kfree(obj);
>>>> +
>>>> +    if (!count)
>>>> +        return ERR_PTR(-ENODEV);
>>>> +
>>>> +    return dell_wmi_ddv_channel_create(&wdev->dev, count, type, config);
>>>> +}
>>>> +
>>>> +static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data)
>>>> +{
>>>> +    struct wmi_device *wdev = data->wdev;
>>>> +    struct combined_chip_info *cinfo;
>>>> +    struct device *hdev;
>>>> +    int index = 0;
>>>> +    int ret;
>>>> +
>>>> +    if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, GFP_KERNEL))
>>>> +        return -ENOMEM;
>>>> +
>>>> +    cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), GFP_KERNEL);
>>>> +    if (!cinfo) {
>>>> +        ret = -ENOMEM;
>>>> +
>>>> +        goto err_release;
>>>> +    }
>>>> +
>>>> +    cinfo->chip.ops = &dell_wmi_ddv_ops;
>>>> +    cinfo->chip.info = cinfo->info;
>>>> +
>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, hwmon_chip,
>>>> +                             HWMON_C_REGISTER_TZ);
>>>> +
>>>> +    if (IS_ERR(cinfo->info[index])) {
>>>> +        ret = PTR_ERR(cinfo->info[index]);
>>>> +
>>>> +        goto err_release;
>>>> +    }
>>>> +
>>>> +    index++;
>>>> +
>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>> +                               sizeof(struct fan_sensor_entry), hwmon_fan,
>>>> +                               (HWMON_F_INPUT | HWMON_F_LABEL));
>>>> +    if (!IS_ERR(cinfo->info[index]))
>>>> +        index++;
>>>> +
>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>> +                               sizeof(struct thermal_sensor_entry),
>>>> +                               hwmon_temp, (HWMON_T_INPUT | HWMON_T_MIN |
>>>> +                               HWMON_T_MAX | HWMON_T_LABEL));
>>>> +    if (!IS_ERR(cinfo->info[index]))
>>>> +        index++;
>>>> +
>>>> +    if (!index) {
>>>> +        ret = -ENODEV;
>>>> +
>>>> +        goto err_release;
>>>> +    }
>>>> +
>>>> +    cinfo->info[index] = NULL;
>>>> +
>>>> +    hdev = devm_hwmon_device_register_with_info(&wdev->dev, "dell_ddv", data, &cinfo->chip,
>>>> +                            NULL);
>>>> +    if (IS_ERR(hdev)) {
>>>> +        ret = PTR_ERR(hdev);
>>>> +
>>>> +        goto err_release;
>>>> +    }
>>>> +
>>>> +    devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_release:
>>>> +    devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>   static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
>>>>   {
>>>>       const char *uid_str;
>>>> @@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
>>>>
>>>>       dell_wmi_ddv_debugfs_init(wdev);
>>>>
>>>> -    return dell_wmi_ddv_battery_add(data);
>>>> +    ret = dell_wmi_ddv_hwmon_add(data);
>>>> +    if (ret < 0)
>>>> +        dev_dbg(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
>>>> +
>>>> +    ret = dell_wmi_ddv_battery_add(data);
>>>> +    if (ret < 0)
>>>> +        dev_dbg(&wdev->dev, "Unable to register acpi battery hook: %d\n", ret);
>>>> +
>>> This used to be an error, but no longer is. Not my call to make
>>> if this is acceptable, just pointing it out.
>>
>> I decided to not treat the battery hook as essential function anymore because
>> the battery hook and hwmon functionality are more or less disconnected from
>> each other, so having the driver abort probing just because one functionality
>> could not be initialized seemed unreasonable to me.
>>
>> I already thought about putting the battery hook and hwmon functionality into
>> separate drivers, with the main driver registering a MFD device or something similar.
>> Because apart from some generic routines, both functions are not connected in any way.
>>
>> Is it acceptable to split the driver for such a thing?
>>
>> Armin Wolf
>>
> Any thoughts about this? Otherwise i will just use conditionals.

I addressed this already in my earlier review of this (5/5) patch:

"""
I'm fine with not making either _add failing an error, but can we make this a dev_warn,
dev_dbg is a bit too low of a log-level for something which is not supposed to happen.

E.g. change this to:

	ret = dell_wmi_ddv_hwmon_add(data);
	if (ret && ret != -ENODEV)
		dev_warn(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
"""

IOW I agree to not have one of the _add() calls failing making probe() fail,
because as you say there are 2 independent calls and just because one does
not work does not mean we don't still want the other.

But as mentioned please change the logging to a warning (and make it
silent when ret == -ENODEV).

Regards,

Hans

Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[Armin Wolf]

Am 02.02.23 um 22:29 schrieb Hans de Goede:

> Hi,
>
> On 2/2/23 22:12, Armin Wolf wrote:
>> Am 27.01.23 um 17:09 schrieb Armin Wolf:
>>
>>> Am 27.01.23 um 14:08 schrieb Guenter Roeck:
>>>
>>>> On Thu, Jan 26, 2023 at 08:40:21PM +0100, Armin Wolf wrote:
>>>>> Thanks to bugreport 216655 on bugzilla triggered by the
>>>>> dell-smm-hwmon driver, the contents of the sensor buffers
>>>>> could be almost completely decoded.
>>>>> Add an hwmon interface for exposing the fan and thermal
>>>>> sensor values. The debugfs interface remains in place to
>>>>> aid in reverse-engineering of unknown sensor types
>>>>> and the thermal buffer.
>>>>>
>>>>> Tested-by: Antonín Skala <skala.antonin@gmail.com>
>>>>> Tested-by: Gustavo Walbon <gustavowalbon@gmail.com>
>>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>>> ---
>>>>>    drivers/platform/x86/dell/Kconfig        |   1 +
>>>>>    drivers/platform/x86/dell/dell-wmi-ddv.c | 435 ++++++++++++++++++++++-
>>>>>    2 files changed, 435 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>>>>> index d319de8f2132..21a74b63d9b1 100644
>>>>> --- a/drivers/platform/x86/dell/Kconfig
>>>>> +++ b/drivers/platform/x86/dell/Kconfig
>>>>> @@ -194,6 +194,7 @@ config DELL_WMI_DDV
>>>>>        default m
>>>>>        depends on ACPI_BATTERY
>>>>>        depends on ACPI_WMI
>>>>> +    depends on HWMON
>>>> Not sure if adding such a dependency is a good idea.
>>>> Up to the maintainer to decide. Personally I would prefer
>>>> something like
>>>>      depends on HWMON || HWMON=n
>>>> and some conditionals in the code, as it is done with other drivers
>>>> outside the hwmon directory.
>>>>
>>> Good point, i will include this in the next patch revision.
>>>
>>>>>        help
>>>>>          This option adds support for WMI-based sensors like
>>>>>          battery temperature sensors found on some Dell notebooks.
>>>>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>>> index 9695bf493ea6..5b30bb85199e 100644
>>>>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>>> @@ -13,6 +13,7 @@
>>>>>    #include <linux/dev_printk.h>
>>>>>    #include <linux/errno.h>
>>>>>    #include <linux/kernel.h>
>>>>> +#include <linux/hwmon.h>
>>>>>    #include <linux/kstrtox.h>
>>>>>    #include <linux/math.h>
>>>>>    #include <linux/module.h>
>>>>> @@ -21,10 +22,13 @@
>>>>>    #include <linux/printk.h>
>>>>>    #include <linux/seq_file.h>
>>>>>    #include <linux/sysfs.h>
>>>>> +#include <linux/types.h>
>>>>>    #include <linux/wmi.h>
>>>>>
>>>>>    #include <acpi/battery.h>
>>>>>
>>>>> +#include <asm/unaligned.h>
>>>>> +
>>>>>    #define DRIVER_NAME    "dell-wmi-ddv"
>>>>>
>>>>>    #define DELL_DDV_SUPPORTED_VERSION_MIN    2
>>>>> @@ -63,6 +67,29 @@ enum dell_ddv_method {
>>>>>        DELL_DDV_THERMAL_SENSOR_INFORMATION    = 0x22,
>>>>>    };
>>>>>
>>>>> +struct fan_sensor_entry {
>>>>> +    u8 type;
>>>>> +    __le16 rpm;
>>>>> +} __packed;
>>>>> +
>>>>> +struct thermal_sensor_entry {
>>>>> +    u8 type;
>>>>> +    s8 now;
>>>>> +    s8 min;
>>>>> +    s8 max;
>>>>> +    u8 unknown;
>>>>> +} __packed;
>>>>> +
>>>>> +struct combined_channel_info {
>>>>> +    struct hwmon_channel_info info;
>>>>> +    u32 config[];
>>>>> +};
>>>>> +
>>>>> +struct combined_chip_info {
>>>>> +    struct hwmon_chip_info chip;
>>>>> +    const struct hwmon_channel_info *info[];
>>>>> +};
>>>>> +
>>>>>    struct dell_wmi_ddv_data {
>>>>>        struct acpi_battery_hook hook;
>>>>>        struct device_attribute temp_attr;
>>>>> @@ -70,6 +97,24 @@ struct dell_wmi_ddv_data {
>>>>>        struct wmi_device *wdev;
>>>>>    };
>>>>>
>>>>> +static const char * const fan_labels[] = {
>>>>> +    "CPU Fan",
>>>>> +    "Chassis Motherboard Fan",
>>>>> +    "Video Fan",
>>>>> +    "Power Supply Fan",
>>>>> +    "Chipset Fan",
>>>>> +    "Memory Fan",
>>>>> +    "PCI Fan",
>>>>> +    "HDD Fan",
>>>>> +};
>>>>> +
>>>>> +static const char * const fan_dock_labels[] = {
>>>>> +    "Docking Chassis/Motherboard Fan",
>>>>> +    "Docking Video Fan",
>>>>> +    "Docking Power Supply Fan",
>>>>> +    "Docking Chipset Fan",
>>>>> +};
>>>>> +
>>>>>    static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
>>>>>                       union acpi_object **result, acpi_object_type type)
>>>>>    {
>>>>> @@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
>>>>>        return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
>>>>>    }
>>>>>
>>>>> +static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
>>>>> +                      size_t entry_size, union acpi_object **result, u64 *count)
>>>>> +{
>>>>> +    union acpi_object *obj;
>>>>> +    u64 buffer_size;
>>>>> +    u8 *buffer;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    buffer_size = obj->package.elements[0].integer.value;
>>>>> +    buffer = obj->package.elements[1].buffer.pointer;
>>>>> +    if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != 0xff) {
>>>>> +        kfree(obj);
>>>>> +
>>>> Stray empty line
>>>>
>>>>> +        return -ENOMSG;
>>>>> +    }
>>>>> +
>>>>> +    *count = (buffer_size - 1) / entry_size;
>>>>> +    *result = obj;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
>>>>> +                       int channel)
>>>>> +{
>>>>> +    return 0444;
>>>>> +}
>>>>> +
>>>>> +static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
>>>>> +                     long *val)
>>>>> +{
>>>>> +    struct fan_sensor_entry *entry;
>>>>> +    union acpi_object *obj;
>>>>> +    u64 count;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>>> +                     sizeof(*entry), &obj, &count);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>>> +    if (count > channel) {
>>>>> +        switch (attr) {
>>>>> +        case hwmon_fan_input:
>>>>> +            *val = get_unaligned_le16(&entry[channel].rpm);
>>>>> +
>>>> Another stray empty line. I see that "empty line before return or break"
>>>> is common. Looks odd to me, and I don't see the point (it confuses
>>>> the code flow for me and lets my brain focus on the empty line instead
>>>> of the code in question), but I guess that is PoV. I won't comment on
>>>> it further and let the maintainer decide.
>>>>
>>>>> +            break;
>>>>> +        default:
>>>>> +            ret = -EOPNOTSUPP;
>>>>> +        }
>>>>> +    } else {
>>>>> +        ret = -ENXIO;
>>>>> +    }
>>>> Error handling should come first.
>>>>> +
>>>>> +    kfree(obj);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
>>>>> +                      long *val)
>>>>> +{
>>>>> +    struct thermal_sensor_entry *entry;
>>>>> +    union acpi_object *obj;
>>>>> +    u64 count;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>>> +                     sizeof(*entry), &obj, &count);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>>> +    if (count > channel) {
>>>> This is a bit of Joda programming. It is really "channel < count",
>>>> ie the requested channel number is in the range of channels reported
>>>> by the WMI code. PoV, of course, but I find that the above makes the
>>>> code more difficult to read.
>>>>> +        switch (attr) {
>>>>> +        case hwmon_temp_input:
>>>>> +            *val = entry[channel].now * 1000;
>>>>> +
>>>>> +            break;
>>>>> +        case hwmon_temp_min:
>>>>> +            *val = entry[channel].min * 1000;
>>>>> +
>>>>> +            break;
>>>>> +        case hwmon_temp_max:
>>>>> +            *val = entry[channel].max * 1000;
>>>>> +
>>>>> +            break;
>>>>> +        default:
>>>>> +            ret = -EOPNOTSUPP;
>>>> break; missing
>>>>
>>>>> +        }
>>>>> +    } else {
>>>>> +        ret = -ENXIO;
>>>>> +    }
>>>> Same as above - error handling should come first.
>>>>
>>>>> +
>>>>> +    kfree(obj);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int dell_wmi_ddv_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>>> +                 int channel, long *val)
>>>>> +{
>>>>> +    struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>>>> +
>>>>> +    switch (type) {
>>>>> +    case hwmon_fan:
>>>>> +        return dell_wmi_ddv_fan_read_channel(data, attr, channel, val);
>>>>> +    case hwmon_temp:
>>>>> +        return dell_wmi_ddv_temp_read_channel(data, attr, channel, val);
>>>>> +    default:
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>> +    return -EOPNOTSUPP;
>>>>> +}
>>>>> +
>>>>> +static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int channel,
>>>>> +                    const char **str)
>>>>> +{
>>>>> +    struct fan_sensor_entry *entry;
>>>>> +    union acpi_object *obj;
>>>>> +    u64 count;
>>>>> +    u8 type;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>>> +                     sizeof(*entry), &obj, &count);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>>> +    if (count > channel) {
>>>>> +        type = entry[channel].type;
>>>>> +
>>>>> +        switch (type) {
>>>>> +        case 0x00 ... 0x07:
>>>>> +            *str = fan_labels[type];
>>>>> +
>>>>> +            break;
>>>>> +        case 0x11 ... 0x14:
>>>>> +            *str = fan_dock_labels[type - 0x11];
>>>>> +
>>>>> +            break;
>>>>> +        default:
>>>>> +            *str = "Unknown Fan";
>>>> break; missing.
>>>>
>>>>> +        }
>>>>> +    } else {
>>>>> +        ret = -ENXIO;
>>>>> +    }
>>>> And again.
>>>>
>>>>> +
>>>>> +    kfree(obj);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int channel,
>>>>> +                     const char **str)
>>>>> +{
>>>>> +    struct thermal_sensor_entry *entry;
>>>>> +    union acpi_object *obj;
>>>>> +    u64 count;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>>> +                     sizeof(*entry), &obj, &count);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>>> +    if (count > channel) {
>>>>> +        switch (entry[channel].type) {
>>>>> +        case 0x00:
>>>>> +            *str = "CPU";
>>>>> +
>>>>> +            break;
>>>>> +        case 0x11:
>>>>> +            *str = "Video";
>>>>> +
>>>>> +            break;
>>>>> +        case 0x22:
>>>>> +            *str = "Memory"; // sometimes called DIMM
>>>> Personally I don't permit C++ style comments in a hwmon driver
>>>> unless _all_ comments are C++ style. Just a remark here.
>>>>
>>>>> +
>>>>> +            break;
>>>>> +        case 0x33:
>>>>> +            *str = "Other";
>>>>> +
>>>>> +            break;
>>>>> +        case 0x44:
>>>>> +            *str = "Ambient"; // sometimes called SKIN
>>>>> +
>>>>> +            break;
>>>>> +        case 0x52:
>>>>> +            *str = "SODIMM";
>>>>> +
>>>>> +            break;
>>>>> +        case 0x55:
>>>>> +            *str = "HDD";
>>>>> +
>>>>> +            break;
>>>>> +        case 0x62:
>>>>> +            *str = "SODIMM 2";
>>>>> +
>>>>> +            break;
>>>>> +        case 0x73:
>>>>> +            *str = "NB";
>>>>> +
>>>>> +            break;
>>>>> +        case 0x83:
>>>>> +            *str = "Charger";
>>>>> +
>>>>> +            break;
>>>>> +        case 0xbb:
>>>>> +            *str = "Memory 3";
>>>>> +
>>>>> +            break;
>>>>> +        default:
>>>>> +            *str = "Unknown";
>>>> break; missing
>>>> Ok, I guess this is on purpose. I personally don't permit
>>>> that since it always leaves the question if it was on purpose or not.
>>>>
>>>>> +        }
>>>>> +    } else {
>>>>> +        ret = -ENXIO;
>>>>> +    }
>>>>> +
>>>>> +    kfree(obj);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int dell_wmi_ddv_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>>> +                    int channel, const char **str)
>>>>> +{
>>>>> +    struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>>>> +
>>>>> +    switch (type) {
>>>>> +    case hwmon_fan:
>>>>> +        switch (attr) {
>>>>> +        case hwmon_fan_label:
>>>>> +            return dell_wmi_ddv_fan_read_string(data, channel, str);
>>>>> +        default:
>>>>> +            break;
>>>>> +        }
>>>>> +        break;
>>>>> +    case hwmon_temp:
>>>>> +        switch (attr) {
>>>>> +        case hwmon_temp_label:
>>>>> +            return dell_wmi_ddv_temp_read_string(data, channel, str);
>>>>> +        default:
>>>>> +            break;
>>>>> +        }
>>>>> +        break;
>>>>> +    default:
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>> +    return -EOPNOTSUPP;
>>>>> +}
>>>>> +
>>>>> +static const struct hwmon_ops dell_wmi_ddv_ops = {
>>>>> +    .is_visible = dell_wmi_ddv_is_visible,
>>>>> +    .read = dell_wmi_ddv_read,
>>>>> +    .read_string = dell_wmi_ddv_read_string,
>>>>> +};
>>>>> +
>>>>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev, u64 count,
>>>>> +                                  enum hwmon_sensor_types type,
>>>>> +                                  u32 config)
>>>>> +{
>>>>> +    struct combined_channel_info *cinfo;
>>>>> +    int i;
>>>>> +
>>>>> +    cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + 1), GFP_KERNEL);
>>>>> +    if (!cinfo)
>>>>> +        return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> +    cinfo->info.type = type;
>>>>> +    cinfo->info.config = cinfo->config;
>>>>> +
>>>>> +    for (i = 0; i < count; i++)
>>>>> +        cinfo->config[i] = config;
>>>>> +
>>>>> +    return &cinfo->info;
>>>>> +}
>>>>> +
>>>>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
>>>>> +                                enum dell_ddv_method method,
>>>>> +                                size_t entry_size,
>>>>> +                                enum hwmon_sensor_types type,
>>>>> +                                u32 config)
>>>>> +{
>>>>> +    union acpi_object *obj;
>>>>> +    u64 count;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, &obj, &count);
>>>>> +    if (ret < 0)
>>>>> +        return ERR_PTR(ret);
>>>>> +
>>>>> +    kfree(obj);
>>>>> +
>>>>> +    if (!count)
>>>>> +        return ERR_PTR(-ENODEV);
>>>>> +
>>>>> +    return dell_wmi_ddv_channel_create(&wdev->dev, count, type, config);
>>>>> +}
>>>>> +
>>>>> +static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data)
>>>>> +{
>>>>> +    struct wmi_device *wdev = data->wdev;
>>>>> +    struct combined_chip_info *cinfo;
>>>>> +    struct device *hdev;
>>>>> +    int index = 0;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, GFP_KERNEL))
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), GFP_KERNEL);
>>>>> +    if (!cinfo) {
>>>>> +        ret = -ENOMEM;
>>>>> +
>>>>> +        goto err_release;
>>>>> +    }
>>>>> +
>>>>> +    cinfo->chip.ops = &dell_wmi_ddv_ops;
>>>>> +    cinfo->chip.info = cinfo->info;
>>>>> +
>>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, hwmon_chip,
>>>>> +                             HWMON_C_REGISTER_TZ);
>>>>> +
>>>>> +    if (IS_ERR(cinfo->info[index])) {
>>>>> +        ret = PTR_ERR(cinfo->info[index]);
>>>>> +
>>>>> +        goto err_release;
>>>>> +    }
>>>>> +
>>>>> +    index++;
>>>>> +
>>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>>> +                               sizeof(struct fan_sensor_entry), hwmon_fan,
>>>>> +                               (HWMON_F_INPUT | HWMON_F_LABEL));
>>>>> +    if (!IS_ERR(cinfo->info[index]))
>>>>> +        index++;
>>>>> +
>>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>>> +                               sizeof(struct thermal_sensor_entry),
>>>>> +                               hwmon_temp, (HWMON_T_INPUT | HWMON_T_MIN |
>>>>> +                               HWMON_T_MAX | HWMON_T_LABEL));
>>>>> +    if (!IS_ERR(cinfo->info[index]))
>>>>> +        index++;
>>>>> +
>>>>> +    if (!index) {
>>>>> +        ret = -ENODEV;
>>>>> +
>>>>> +        goto err_release;
>>>>> +    }
>>>>> +
>>>>> +    cinfo->info[index] = NULL;
>>>>> +
>>>>> +    hdev = devm_hwmon_device_register_with_info(&wdev->dev, "dell_ddv", data, &cinfo->chip,
>>>>> +                            NULL);
>>>>> +    if (IS_ERR(hdev)) {
>>>>> +        ret = PTR_ERR(hdev);
>>>>> +
>>>>> +        goto err_release;
>>>>> +    }
>>>>> +
>>>>> +    devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>>>>> +
>>>>> +    return 0;
>>>>> +
>>>>> +err_release:
>>>>> +    devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>    static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
>>>>>    {
>>>>>        const char *uid_str;
>>>>> @@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
>>>>>
>>>>>        dell_wmi_ddv_debugfs_init(wdev);
>>>>>
>>>>> -    return dell_wmi_ddv_battery_add(data);
>>>>> +    ret = dell_wmi_ddv_hwmon_add(data);
>>>>> +    if (ret < 0)
>>>>> +        dev_dbg(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
>>>>> +
>>>>> +    ret = dell_wmi_ddv_battery_add(data);
>>>>> +    if (ret < 0)
>>>>> +        dev_dbg(&wdev->dev, "Unable to register acpi battery hook: %d\n", ret);
>>>>> +
>>>> This used to be an error, but no longer is. Not my call to make
>>>> if this is acceptable, just pointing it out.
>>> I decided to not treat the battery hook as essential function anymore because
>>> the battery hook and hwmon functionality are more or less disconnected from
>>> each other, so having the driver abort probing just because one functionality
>>> could not be initialized seemed unreasonable to me.
>>>
>>> I already thought about putting the battery hook and hwmon functionality into
>>> separate drivers, with the main driver registering a MFD device or something similar.
>>> Because apart from some generic routines, both functions are not connected in any way.
>>>
>>> Is it acceptable to split the driver for such a thing?
>>>
>>> Armin Wolf
>>>
>> Any thoughts about this? Otherwise i will just use conditionals.
> I addressed this already in my earlier review of this (5/5) patch:
>
> """
> I'm fine with not making either _add failing an error, but can we make this a dev_warn,
> dev_dbg is a bit too low of a log-level for something which is not supposed to happen.
>
> E.g. change this to:
>
> 	ret = dell_wmi_ddv_hwmon_add(data);
> 	if (ret && ret != -ENODEV)
> 		dev_warn(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
> """
>
> IOW I agree to not have one of the _add() calls failing making probe() fail,
> because as you say there are 2 independent calls and just because one does
> not work does not mean we don't still want the other.
>
> But as mentioned please change the logging to a warning (and make it
> silent when ret == -ENODEV).
>
> Regards,
>
> Hans
>
>
I was referring to my proposal of splitting the battery and hwmon functionality into separate drivers.
If splitting the driver is undesirable, i will just use conditionals to allow for enabling/disabling
the battery/hwmon part and change the probing as you suggested previously.

Armin Wolf

Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[Hans de Goede]

Hi,

On 2/3/23 02:07, Armin Wolf wrote:
> Am 02.02.23 um 22:29 schrieb Hans de Goede:
> 
>> Hi,
>>
>> On 2/2/23 22:12, Armin Wolf wrote:
>>> Am 27.01.23 um 17:09 schrieb Armin Wolf:
>>>
>>>> Am 27.01.23 um 14:08 schrieb Guenter Roeck:
>>>>
>>>>> On Thu, Jan 26, 2023 at 08:40:21PM +0100, Armin Wolf wrote:
>>>>>> Thanks to bugreport 216655 on bugzilla triggered by the
>>>>>> dell-smm-hwmon driver, the contents of the sensor buffers
>>>>>> could be almost completely decoded.
>>>>>> Add an hwmon interface for exposing the fan and thermal
>>>>>> sensor values. The debugfs interface remains in place to
>>>>>> aid in reverse-engineering of unknown sensor types
>>>>>> and the thermal buffer.
>>>>>>
>>>>>> Tested-by: Antonín Skala <skala.antonin@gmail.com>
>>>>>> Tested-by: Gustavo Walbon <gustavowalbon@gmail.com>
>>>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>>>> ---
>>>>>>    drivers/platform/x86/dell/Kconfig        |   1 +
>>>>>>    drivers/platform/x86/dell/dell-wmi-ddv.c | 435 ++++++++++++++++++++++-
>>>>>>    2 files changed, 435 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>>>>>> index d319de8f2132..21a74b63d9b1 100644
>>>>>> --- a/drivers/platform/x86/dell/Kconfig
>>>>>> +++ b/drivers/platform/x86/dell/Kconfig
>>>>>> @@ -194,6 +194,7 @@ config DELL_WMI_DDV
>>>>>>        default m
>>>>>>        depends on ACPI_BATTERY
>>>>>>        depends on ACPI_WMI
>>>>>> +    depends on HWMON
>>>>> Not sure if adding such a dependency is a good idea.
>>>>> Up to the maintainer to decide. Personally I would prefer
>>>>> something like
>>>>>      depends on HWMON || HWMON=n
>>>>> and some conditionals in the code, as it is done with other drivers
>>>>> outside the hwmon directory.
>>>>>
>>>> Good point, i will include this in the next patch revision.
>>>>
>>>>>>        help
>>>>>>          This option adds support for WMI-based sensors like
>>>>>>          battery temperature sensors found on some Dell notebooks.
>>>>>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>>>> index 9695bf493ea6..5b30bb85199e 100644
>>>>>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>>>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>>>> @@ -13,6 +13,7 @@
>>>>>>    #include <linux/dev_printk.h>
>>>>>>    #include <linux/errno.h>
>>>>>>    #include <linux/kernel.h>
>>>>>> +#include <linux/hwmon.h>
>>>>>>    #include <linux/kstrtox.h>
>>>>>>    #include <linux/math.h>
>>>>>>    #include <linux/module.h>
>>>>>> @@ -21,10 +22,13 @@
>>>>>>    #include <linux/printk.h>
>>>>>>    #include <linux/seq_file.h>
>>>>>>    #include <linux/sysfs.h>
>>>>>> +#include <linux/types.h>
>>>>>>    #include <linux/wmi.h>
>>>>>>
>>>>>>    #include <acpi/battery.h>
>>>>>>
>>>>>> +#include <asm/unaligned.h>
>>>>>> +
>>>>>>    #define DRIVER_NAME    "dell-wmi-ddv"
>>>>>>
>>>>>>    #define DELL_DDV_SUPPORTED_VERSION_MIN    2
>>>>>> @@ -63,6 +67,29 @@ enum dell_ddv_method {
>>>>>>        DELL_DDV_THERMAL_SENSOR_INFORMATION    = 0x22,
>>>>>>    };
>>>>>>
>>>>>> +struct fan_sensor_entry {
>>>>>> +    u8 type;
>>>>>> +    __le16 rpm;
>>>>>> +} __packed;
>>>>>> +
>>>>>> +struct thermal_sensor_entry {
>>>>>> +    u8 type;
>>>>>> +    s8 now;
>>>>>> +    s8 min;
>>>>>> +    s8 max;
>>>>>> +    u8 unknown;
>>>>>> +} __packed;
>>>>>> +
>>>>>> +struct combined_channel_info {
>>>>>> +    struct hwmon_channel_info info;
>>>>>> +    u32 config[];
>>>>>> +};
>>>>>> +
>>>>>> +struct combined_chip_info {
>>>>>> +    struct hwmon_chip_info chip;
>>>>>> +    const struct hwmon_channel_info *info[];
>>>>>> +};
>>>>>> +
>>>>>>    struct dell_wmi_ddv_data {
>>>>>>        struct acpi_battery_hook hook;
>>>>>>        struct device_attribute temp_attr;
>>>>>> @@ -70,6 +97,24 @@ struct dell_wmi_ddv_data {
>>>>>>        struct wmi_device *wdev;
>>>>>>    };
>>>>>>
>>>>>> +static const char * const fan_labels[] = {
>>>>>> +    "CPU Fan",
>>>>>> +    "Chassis Motherboard Fan",
>>>>>> +    "Video Fan",
>>>>>> +    "Power Supply Fan",
>>>>>> +    "Chipset Fan",
>>>>>> +    "Memory Fan",
>>>>>> +    "PCI Fan",
>>>>>> +    "HDD Fan",
>>>>>> +};
>>>>>> +
>>>>>> +static const char * const fan_dock_labels[] = {
>>>>>> +    "Docking Chassis/Motherboard Fan",
>>>>>> +    "Docking Video Fan",
>>>>>> +    "Docking Power Supply Fan",
>>>>>> +    "Docking Chipset Fan",
>>>>>> +};
>>>>>> +
>>>>>>    static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
>>>>>>                       union acpi_object **result, acpi_object_type type)
>>>>>>    {
>>>>>> @@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
>>>>>>        return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
>>>>>>    }
>>>>>>
>>>>>> +static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
>>>>>> +                      size_t entry_size, union acpi_object **result, u64 *count)
>>>>>> +{
>>>>>> +    union acpi_object *obj;
>>>>>> +    u64 buffer_size;
>>>>>> +    u8 *buffer;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    buffer_size = obj->package.elements[0].integer.value;
>>>>>> +    buffer = obj->package.elements[1].buffer.pointer;
>>>>>> +    if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != 0xff) {
>>>>>> +        kfree(obj);
>>>>>> +
>>>>> Stray empty line
>>>>>
>>>>>> +        return -ENOMSG;
>>>>>> +    }
>>>>>> +
>>>>>> +    *count = (buffer_size - 1) / entry_size;
>>>>>> +    *result = obj;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
>>>>>> +                       int channel)
>>>>>> +{
>>>>>> +    return 0444;
>>>>>> +}
>>>>>> +
>>>>>> +static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
>>>>>> +                     long *val)
>>>>>> +{
>>>>>> +    struct fan_sensor_entry *entry;
>>>>>> +    union acpi_object *obj;
>>>>>> +    u64 count;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>>>> +                     sizeof(*entry), &obj, &count);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>>>> +    if (count > channel) {
>>>>>> +        switch (attr) {
>>>>>> +        case hwmon_fan_input:
>>>>>> +            *val = get_unaligned_le16(&entry[channel].rpm);
>>>>>> +
>>>>> Another stray empty line. I see that "empty line before return or break"
>>>>> is common. Looks odd to me, and I don't see the point (it confuses
>>>>> the code flow for me and lets my brain focus on the empty line instead
>>>>> of the code in question), but I guess that is PoV. I won't comment on
>>>>> it further and let the maintainer decide.
>>>>>
>>>>>> +            break;
>>>>>> +        default:
>>>>>> +            ret = -EOPNOTSUPP;
>>>>>> +        }
>>>>>> +    } else {
>>>>>> +        ret = -ENXIO;
>>>>>> +    }
>>>>> Error handling should come first.
>>>>>> +
>>>>>> +    kfree(obj);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
>>>>>> +                      long *val)
>>>>>> +{
>>>>>> +    struct thermal_sensor_entry *entry;
>>>>>> +    union acpi_object *obj;
>>>>>> +    u64 count;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>>>> +                     sizeof(*entry), &obj, &count);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>>>> +    if (count > channel) {
>>>>> This is a bit of Joda programming. It is really "channel < count",
>>>>> ie the requested channel number is in the range of channels reported
>>>>> by the WMI code. PoV, of course, but I find that the above makes the
>>>>> code more difficult to read.
>>>>>> +        switch (attr) {
>>>>>> +        case hwmon_temp_input:
>>>>>> +            *val = entry[channel].now * 1000;
>>>>>> +
>>>>>> +            break;
>>>>>> +        case hwmon_temp_min:
>>>>>> +            *val = entry[channel].min * 1000;
>>>>>> +
>>>>>> +            break;
>>>>>> +        case hwmon_temp_max:
>>>>>> +            *val = entry[channel].max * 1000;
>>>>>> +
>>>>>> +            break;
>>>>>> +        default:
>>>>>> +            ret = -EOPNOTSUPP;
>>>>> break; missing
>>>>>
>>>>>> +        }
>>>>>> +    } else {
>>>>>> +        ret = -ENXIO;
>>>>>> +    }
>>>>> Same as above - error handling should come first.
>>>>>
>>>>>> +
>>>>>> +    kfree(obj);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int dell_wmi_ddv_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>>>> +                 int channel, long *val)
>>>>>> +{
>>>>>> +    struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>>>>> +
>>>>>> +    switch (type) {
>>>>>> +    case hwmon_fan:
>>>>>> +        return dell_wmi_ddv_fan_read_channel(data, attr, channel, val);
>>>>>> +    case hwmon_temp:
>>>>>> +        return dell_wmi_ddv_temp_read_channel(data, attr, channel, val);
>>>>>> +    default:
>>>>>> +        break;
>>>>>> +    }
>>>>>> +
>>>>>> +    return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +
>>>>>> +static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int channel,
>>>>>> +                    const char **str)
>>>>>> +{
>>>>>> +    struct fan_sensor_entry *entry;
>>>>>> +    union acpi_object *obj;
>>>>>> +    u64 count;
>>>>>> +    u8 type;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>>>> +                     sizeof(*entry), &obj, &count);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>>>> +    if (count > channel) {
>>>>>> +        type = entry[channel].type;
>>>>>> +
>>>>>> +        switch (type) {
>>>>>> +        case 0x00 ... 0x07:
>>>>>> +            *str = fan_labels[type];
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x11 ... 0x14:
>>>>>> +            *str = fan_dock_labels[type - 0x11];
>>>>>> +
>>>>>> +            break;
>>>>>> +        default:
>>>>>> +            *str = "Unknown Fan";
>>>>> break; missing.
>>>>>
>>>>>> +        }
>>>>>> +    } else {
>>>>>> +        ret = -ENXIO;
>>>>>> +    }
>>>>> And again.
>>>>>
>>>>>> +
>>>>>> +    kfree(obj);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int channel,
>>>>>> +                     const char **str)
>>>>>> +{
>>>>>> +    struct thermal_sensor_entry *entry;
>>>>>> +    union acpi_object *obj;
>>>>>> +    u64 count;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>>>> +                     sizeof(*entry), &obj, &count);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
>>>>>> +    if (count > channel) {
>>>>>> +        switch (entry[channel].type) {
>>>>>> +        case 0x00:
>>>>>> +            *str = "CPU";
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x11:
>>>>>> +            *str = "Video";
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x22:
>>>>>> +            *str = "Memory"; // sometimes called DIMM
>>>>> Personally I don't permit C++ style comments in a hwmon driver
>>>>> unless _all_ comments are C++ style. Just a remark here.
>>>>>
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x33:
>>>>>> +            *str = "Other";
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x44:
>>>>>> +            *str = "Ambient"; // sometimes called SKIN
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x52:
>>>>>> +            *str = "SODIMM";
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x55:
>>>>>> +            *str = "HDD";
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x62:
>>>>>> +            *str = "SODIMM 2";
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x73:
>>>>>> +            *str = "NB";
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0x83:
>>>>>> +            *str = "Charger";
>>>>>> +
>>>>>> +            break;
>>>>>> +        case 0xbb:
>>>>>> +            *str = "Memory 3";
>>>>>> +
>>>>>> +            break;
>>>>>> +        default:
>>>>>> +            *str = "Unknown";
>>>>> break; missing
>>>>> Ok, I guess this is on purpose. I personally don't permit
>>>>> that since it always leaves the question if it was on purpose or not.
>>>>>
>>>>>> +        }
>>>>>> +    } else {
>>>>>> +        ret = -ENXIO;
>>>>>> +    }
>>>>>> +
>>>>>> +    kfree(obj);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int dell_wmi_ddv_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>>>>> +                    int channel, const char **str)
>>>>>> +{
>>>>>> +    struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>>>>> +
>>>>>> +    switch (type) {
>>>>>> +    case hwmon_fan:
>>>>>> +        switch (attr) {
>>>>>> +        case hwmon_fan_label:
>>>>>> +            return dell_wmi_ddv_fan_read_string(data, channel, str);
>>>>>> +        default:
>>>>>> +            break;
>>>>>> +        }
>>>>>> +        break;
>>>>>> +    case hwmon_temp:
>>>>>> +        switch (attr) {
>>>>>> +        case hwmon_temp_label:
>>>>>> +            return dell_wmi_ddv_temp_read_string(data, channel, str);
>>>>>> +        default:
>>>>>> +            break;
>>>>>> +        }
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        break;
>>>>>> +    }
>>>>>> +
>>>>>> +    return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct hwmon_ops dell_wmi_ddv_ops = {
>>>>>> +    .is_visible = dell_wmi_ddv_is_visible,
>>>>>> +    .read = dell_wmi_ddv_read,
>>>>>> +    .read_string = dell_wmi_ddv_read_string,
>>>>>> +};
>>>>>> +
>>>>>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev, u64 count,
>>>>>> +                                  enum hwmon_sensor_types type,
>>>>>> +                                  u32 config)
>>>>>> +{
>>>>>> +    struct combined_channel_info *cinfo;
>>>>>> +    int i;
>>>>>> +
>>>>>> +    cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + 1), GFP_KERNEL);
>>>>>> +    if (!cinfo)
>>>>>> +        return ERR_PTR(-ENOMEM);
>>>>>> +
>>>>>> +    cinfo->info.type = type;
>>>>>> +    cinfo->info.config = cinfo->config;
>>>>>> +
>>>>>> +    for (i = 0; i < count; i++)
>>>>>> +        cinfo->config[i] = config;
>>>>>> +
>>>>>> +    return &cinfo->info;
>>>>>> +}
>>>>>> +
>>>>>> +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
>>>>>> +                                enum dell_ddv_method method,
>>>>>> +                                size_t entry_size,
>>>>>> +                                enum hwmon_sensor_types type,
>>>>>> +                                u32 config)
>>>>>> +{
>>>>>> +    union acpi_object *obj;
>>>>>> +    u64 count;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, &obj, &count);
>>>>>> +    if (ret < 0)
>>>>>> +        return ERR_PTR(ret);
>>>>>> +
>>>>>> +    kfree(obj);
>>>>>> +
>>>>>> +    if (!count)
>>>>>> +        return ERR_PTR(-ENODEV);
>>>>>> +
>>>>>> +    return dell_wmi_ddv_channel_create(&wdev->dev, count, type, config);
>>>>>> +}
>>>>>> +
>>>>>> +static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data)
>>>>>> +{
>>>>>> +    struct wmi_device *wdev = data->wdev;
>>>>>> +    struct combined_chip_info *cinfo;
>>>>>> +    struct device *hdev;
>>>>>> +    int index = 0;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, GFP_KERNEL))
>>>>>> +        return -ENOMEM;
>>>>>> +
>>>>>> +    cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), GFP_KERNEL);
>>>>>> +    if (!cinfo) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +
>>>>>> +        goto err_release;
>>>>>> +    }
>>>>>> +
>>>>>> +    cinfo->chip.ops = &dell_wmi_ddv_ops;
>>>>>> +    cinfo->chip.info = cinfo->info;
>>>>>> +
>>>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, hwmon_chip,
>>>>>> +                             HWMON_C_REGISTER_TZ);
>>>>>> +
>>>>>> +    if (IS_ERR(cinfo->info[index])) {
>>>>>> +        ret = PTR_ERR(cinfo->info[index]);
>>>>>> +
>>>>>> +        goto err_release;
>>>>>> +    }
>>>>>> +
>>>>>> +    index++;
>>>>>> +
>>>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
>>>>>> +                               sizeof(struct fan_sensor_entry), hwmon_fan,
>>>>>> +                               (HWMON_F_INPUT | HWMON_F_LABEL));
>>>>>> +    if (!IS_ERR(cinfo->info[index]))
>>>>>> +        index++;
>>>>>> +
>>>>>> +    cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
>>>>>> +                               sizeof(struct thermal_sensor_entry),
>>>>>> +                               hwmon_temp, (HWMON_T_INPUT | HWMON_T_MIN |
>>>>>> +                               HWMON_T_MAX | HWMON_T_LABEL));
>>>>>> +    if (!IS_ERR(cinfo->info[index]))
>>>>>> +        index++;
>>>>>> +
>>>>>> +    if (!index) {
>>>>>> +        ret = -ENODEV;
>>>>>> +
>>>>>> +        goto err_release;
>>>>>> +    }
>>>>>> +
>>>>>> +    cinfo->info[index] = NULL;
>>>>>> +
>>>>>> +    hdev = devm_hwmon_device_register_with_info(&wdev->dev, "dell_ddv", data, &cinfo->chip,
>>>>>> +                            NULL);
>>>>>> +    if (IS_ERR(hdev)) {
>>>>>> +        ret = PTR_ERR(hdev);
>>>>>> +
>>>>>> +        goto err_release;
>>>>>> +    }
>>>>>> +
>>>>>> +    devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +
>>>>>> +err_release:
>>>>>> +    devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>>    static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
>>>>>>    {
>>>>>>        const char *uid_str;
>>>>>> @@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
>>>>>>
>>>>>>        dell_wmi_ddv_debugfs_init(wdev);
>>>>>>
>>>>>> -    return dell_wmi_ddv_battery_add(data);
>>>>>> +    ret = dell_wmi_ddv_hwmon_add(data);
>>>>>> +    if (ret < 0)
>>>>>> +        dev_dbg(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
>>>>>> +
>>>>>> +    ret = dell_wmi_ddv_battery_add(data);
>>>>>> +    if (ret < 0)
>>>>>> +        dev_dbg(&wdev->dev, "Unable to register acpi battery hook: %d\n", ret);
>>>>>> +
>>>>> This used to be an error, but no longer is. Not my call to make
>>>>> if this is acceptable, just pointing it out.
>>>> I decided to not treat the battery hook as essential function anymore because
>>>> the battery hook and hwmon functionality are more or less disconnected from
>>>> each other, so having the driver abort probing just because one functionality
>>>> could not be initialized seemed unreasonable to me.
>>>>
>>>> I already thought about putting the battery hook and hwmon functionality into
>>>> separate drivers, with the main driver registering a MFD device or something similar.
>>>> Because apart from some generic routines, both functions are not connected in any way.
>>>>
>>>> Is it acceptable to split the driver for such a thing?
>>>>
>>>> Armin Wolf
>>>>
>>> Any thoughts about this? Otherwise i will just use conditionals.
>> I addressed this already in my earlier review of this (5/5) patch:
>>
>> """
>> I'm fine with not making either _add failing an error, but can we make this a dev_warn,
>> dev_dbg is a bit too low of a log-level for something which is not supposed to happen.
>>
>> E.g. change this to:
>>
>>     ret = dell_wmi_ddv_hwmon_add(data);
>>     if (ret && ret != -ENODEV)
>>         dev_warn(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);
>> """
>>
>> IOW I agree to not have one of the _add() calls failing making probe() fail,
>> because as you say there are 2 independent calls and just because one does
>> not work does not mean we don't still want the other.
>>
>> But as mentioned please change the logging to a warning (and make it
>> silent when ret == -ENODEV).
>>
>> Regards,
>>
>> Hans
>>
>>
> I was referring to my proposal of splitting the battery and hwmon functionality into separate drivers.
> If splitting the driver is undesirable, i will just use conditionals to allow for enabling/disabling
> the battery/hwmon part and change the probing as you suggested previously.

Both parts bind / talk to the same WMI device / GUID, right ?

In that case I would not split the driver.

Regards,

Hans

Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[kernel test robot]

Hi Armin,

I love your patch! Yet something to improve:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.2-rc5 next-20230127]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Armin-Wolf/platform-x86-dell-ddv-Add-support-for-interface-version-3/20230128-153324
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230126194021.381092-6-W_Armin%40gmx.de
patch subject: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support
config: i386-randconfig-c021 (https://download.01.org/0day-ci/archive/20230129/202301291705.JVYNKBnd-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/36be415c22eab400b1546b2c5a6737b28e847774
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Armin-Wolf/platform-x86-dell-ddv-Add-support-for-interface-version-3/20230128-153324
        git checkout 36be415c22eab400b1546b2c5a6737b28e847774
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__umoddi3" [drivers/platform/x86/dell/dell-wmi-ddv.ko] undefined!
>> ERROR: modpost: "__udivdi3" [drivers/platform/x86/dell/dell-wmi-ddv.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 5/5] platform/x86: dell-ddv: Add hwmon support

[Hans de Goede]

Hi Armin,

On 1/26/23 20:40, Armin Wolf wrote:
> Thanks to bugreport 216655 on bugzilla triggered by the
> dell-smm-hwmon driver, the contents of the sensor buffers
> could be almost completely decoded.
> Add an hwmon interface for exposing the fan and thermal
> sensor values. The debugfs interface remains in place to
> aid in reverse-engineering of unknown sensor types
> and the thermal buffer.
> 
> Tested-by: Antonín Skala <skala.antonin@gmail.com>
> Tested-by: Gustavo Walbon <gustavowalbon@gmail.com>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/dell/Kconfig        |   1 +
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 435 ++++++++++++++++++++++-
>  2 files changed, 435 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index d319de8f2132..21a74b63d9b1 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -194,6 +194,7 @@ config DELL_WMI_DDV
>  	default m
>  	depends on ACPI_BATTERY
>  	depends on ACPI_WMI
> +	depends on HWMON
>  	help
>  	  This option adds support for WMI-based sensors like
>  	  battery temperature sensors found on some Dell notebooks.
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 9695bf493ea6..5b30bb85199e 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -13,6 +13,7 @@
>  #include <linux/dev_printk.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
> +#include <linux/hwmon.h>
>  #include <linux/kstrtox.h>
>  #include <linux/math.h>
>  #include <linux/module.h>
> @@ -21,10 +22,13 @@
>  #include <linux/printk.h>
>  #include <linux/seq_file.h>
>  #include <linux/sysfs.h>
> +#include <linux/types.h>
>  #include <linux/wmi.h>
> 
>  #include <acpi/battery.h>
> 
> +#include <asm/unaligned.h>
> +
>  #define DRIVER_NAME	"dell-wmi-ddv"
> 
>  #define DELL_DDV_SUPPORTED_VERSION_MIN	2
> @@ -63,6 +67,29 @@ enum dell_ddv_method {
>  	DELL_DDV_THERMAL_SENSOR_INFORMATION	= 0x22,
>  };
> 
> +struct fan_sensor_entry {
> +	u8 type;
> +	__le16 rpm;
> +} __packed;
> +
> +struct thermal_sensor_entry {
> +	u8 type;
> +	s8 now;
> +	s8 min;
> +	s8 max;
> +	u8 unknown;
> +} __packed;
> +
> +struct combined_channel_info {
> +	struct hwmon_channel_info info;
> +	u32 config[];
> +};
> +
> +struct combined_chip_info {
> +	struct hwmon_chip_info chip;
> +	const struct hwmon_channel_info *info[];
> +};
> +
>  struct dell_wmi_ddv_data {
>  	struct acpi_battery_hook hook;
>  	struct device_attribute temp_attr;
> @@ -70,6 +97,24 @@ struct dell_wmi_ddv_data {
>  	struct wmi_device *wdev;
>  };
> 
> +static const char * const fan_labels[] = {
> +	"CPU Fan",
> +	"Chassis Motherboard Fan",
> +	"Video Fan",
> +	"Power Supply Fan",
> +	"Chipset Fan",
> +	"Memory Fan",
> +	"PCI Fan",
> +	"HDD Fan",
> +};
> +
> +static const char * const fan_dock_labels[] = {
> +	"Docking Chassis/Motherboard Fan",
> +	"Docking Video Fan",
> +	"Docking Power Supply Fan",
> +	"Docking Chipset Fan",
> +};
> +
>  static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
>  				   union acpi_object **result, acpi_object_type type)
>  {
> @@ -171,6 +216,386 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
>  	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
>  }
> 
> +static int dell_wmi_ddv_query_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
> +				      size_t entry_size, union acpi_object **result, u64 *count)
> +{
> +	union acpi_object *obj;
> +	u64 buffer_size;
> +	u8 *buffer;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
> +	if (ret < 0)
> +		return ret;
> +
> +	buffer_size = obj->package.elements[0].integer.value;
> +	buffer = obj->package.elements[1].buffer.pointer;
> +	if (buffer_size % entry_size != 1 || buffer[buffer_size - 1] != 0xff) {
> +		kfree(obj);
> +
> +		return -ENOMSG;
> +	}
> +
> +	*count = (buffer_size - 1) / entry_size;
> +	*result = obj;
> +
> +	return 0;
> +}
> +
> +static umode_t dell_wmi_ddv_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
> +				       int channel)
> +{
> +	return 0444;
> +}
> +
> +static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
> +					 long *val)
> +{
> +	struct fan_sensor_entry *entry;
> +	union acpi_object *obj;
> +	u64 count;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
> +					 sizeof(*entry), &obj, &count);
> +	if (ret < 0)
> +		return ret;
> +
> +	entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
> +	if (count > channel) {
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			*val = get_unaligned_le16(&entry[channel].rpm);
> +
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}
> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 attr, int channel,
> +					  long *val)
> +{
> +	struct thermal_sensor_entry *entry;
> +	union acpi_object *obj;
> +	u64 count;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
> +					 sizeof(*entry), &obj, &count);

Previously we discussed how expensive some of these calls are. I assume that this
call also is not really "free" to make. Even if it does a fast read from some hw
buffer it still needs to go through the ACPI interpreter and then actually read
from the hw.

It seems that there are just 2 sort of calls made to dell_wmi_ddv_query_sensors:

1. DELL_DDV_THERMAL_SENSOR_INFORMATION
2. DELL_DDV_FAN_SENSOR_INFORMATION

Userspace apps using hwmon will typically have some periodically refreshing
display and when the refresh timer expires they will read all hwmon
attributes in one go.

To avoid this causing unnecessary overhead many hwmon drivers use a driver
based cache and they then refresh the cache if the cache is older then 30 seconds
when userspace does its next read of an attribute.

See e.g. drivers/hwmon/f71882fg.c and the use of the update_lock, valid and
last_updated members of struct f71882fg_data.

Below I see that the return value of a single dell_wmi_ddv_query_sensors() call
returns data for 3 different attributes * count-channels = 3, 6 or 9 read
calls. So to me this sounds like it is worthwhile caching the result.

You can just store a pointer to the returned ACPI-obj and free the old
ACPI obj when it is time to refresh the cache, instead of immidiately
free-ing the obj when the read function returns.

You can use either separate last_updated timestamps for 
DELL_DDV_THERMAL_SENSOR_INFORMATION + DELL_DDV_FAN_SENSOR_INFORMATION,
or just update both at once when the cache is stale. Either way works for me.

This way we gain a significant amount of efficiency wrt not doing the
expensive WMI call multiple times for naught and we avoid userspace
being able to "hammer" the underlying hw with repeated requests (userspace
can still burn 100% CPU on one core of course).









> +	if (ret < 0)
> +		return ret;
> +
> +	entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
> +	if (count > channel) {

As Guenter already sorta said, please do error handling first,
change this to something like this:

	if (channel >= count) {
		ret = -ENXIO;
		goto out_free;
	}


And then reduce the indentation the switch case by 1 tab / level.

Generally speaking we want the straight (no errors) path to be
indentend by only 1 tab so that if you just read all the lines at
that 1 tab indentation level you are actually reading the normal /
no-errors code path.

Also note that if you switch to a cache as suggested above,
you no longer need the kfree() since that is now done in your
cache-refresh helper.

And then the error check changes to just:

	if (channel >= data->thermal_sensor_count)
		return -ENXIO;




> +		switch (attr) {
> +		case hwmon_temp_input:
> +			*val = entry[channel].now * 1000;
> +
> +			break;
> +		case hwmon_temp_min:
> +			*val = entry[channel].min * 1000;
> +
> +			break;
> +		case hwmon_temp_max:
> +			*val = entry[channel].max * 1000;
> +
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}
> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			     int channel, long *val)
> +{
> +	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		return dell_wmi_ddv_fan_read_channel(data, attr, channel, val);
> +	case hwmon_temp:
> +		return dell_wmi_ddv_temp_read_channel(data, attr, channel, val);
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int channel,
> +					const char **str)
> +{
> +	struct fan_sensor_entry *entry;
> +	union acpi_object *obj;
> +	u64 count;
> +	u8 type;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
> +					 sizeof(*entry), &obj, &count);
> +	if (ret < 0)
> +		return ret;
> +
> +	entry = (struct fan_sensor_entry *)obj->package.elements[1].buffer.pointer;
> +	if (count > channel) {
> +		type = entry[channel].type;
> +
> +		switch (type) {
> +		case 0x00 ... 0x07:
> +			*str = fan_labels[type];
> +
> +			break;
> +		case 0x11 ... 0x14:
> +			*str = fan_dock_labels[type - 0x11];
> +
> +			break;
> +		default:
> +			*str = "Unknown Fan";
> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}
> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int channel,
> +					 const char **str)
> +{
> +	struct thermal_sensor_entry *entry;
> +	union acpi_object *obj;
> +	u64 count;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(data->wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
> +					 sizeof(*entry), &obj, &count);
> +	if (ret < 0)
> +		return ret;
> +
> +	entry = (struct thermal_sensor_entry *)obj->package.elements[1].buffer.pointer;
> +	if (count > channel) {
> +		switch (entry[channel].type) {
> +		case 0x00:
> +			*str = "CPU";
> +
> +			break;
> +		case 0x11:
> +			*str = "Video";
> +
> +			break;
> +		case 0x22:
> +			*str = "Memory"; // sometimes called DIMM
> +
> +			break;
> +		case 0x33:
> +			*str = "Other";
> +
> +			break;
> +		case 0x44:
> +			*str = "Ambient"; // sometimes called SKIN
> +
> +			break;
> +		case 0x52:
> +			*str = "SODIMM";
> +
> +			break;
> +		case 0x55:
> +			*str = "HDD";
> +
> +			break;
> +		case 0x62:
> +			*str = "SODIMM 2";
> +
> +			break;
> +		case 0x73:
> +			*str = "NB";
> +
> +			break;
> +		case 0x83:
> +			*str = "Charger";
> +
> +			break;
> +		case 0xbb:
> +			*str = "Memory 3";
> +
> +			break;
> +		default:
> +			*str = "Unknown";
> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}
> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +				    int channel, const char **str)
> +{
> +	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_label:
> +			return dell_wmi_ddv_fan_read_string(data, channel, str);
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_label:
> +			return dell_wmi_ddv_temp_read_string(data, channel, str);
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops dell_wmi_ddv_ops = {
> +	.is_visible = dell_wmi_ddv_is_visible,
> +	.read = dell_wmi_ddv_read,
> +	.read_string = dell_wmi_ddv_read_string,
> +};
> +
> +static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev, u64 count,
> +							      enum hwmon_sensor_types type,
> +							      u32 config)
> +{
> +	struct combined_channel_info *cinfo;
> +	int i;
> +
> +	cinfo = devm_kzalloc(dev, struct_size(cinfo, config, count + 1), GFP_KERNEL);
> +	if (!cinfo)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cinfo->info.type = type;
> +	cinfo->info.config = cinfo->config;
> +
> +	for (i = 0; i < count; i++)
> +		cinfo->config[i] = config;
> +
> +	return &cinfo->info;
> +}
> +
> +static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
> +							    enum dell_ddv_method method,
> +							    size_t entry_size,
> +							    enum hwmon_sensor_types type,
> +							    u32 config)
> +{
> +	union acpi_object *obj;
> +	u64 count;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_sensors(wdev, method, entry_size, &obj, &count);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	kfree(obj);
> +
> +	if (!count)
> +		return ERR_PTR(-ENODEV);
> +
> +	return dell_wmi_ddv_channel_create(&wdev->dev, count, type, config);
> +}
> +
> +static int dell_wmi_ddv_hwmon_add(struct dell_wmi_ddv_data *data)
> +{
> +	struct wmi_device *wdev = data->wdev;
> +	struct combined_chip_info *cinfo;
> +	struct device *hdev;
> +	int index = 0;
> +	int ret;
> +
> +	if (!devres_open_group(&wdev->dev, dell_wmi_ddv_hwmon_add, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	cinfo = devm_kzalloc(&wdev->dev, struct_size(cinfo, info, 4), GFP_KERNEL);
> +	if (!cinfo) {
> +		ret = -ENOMEM;
> +
> +		goto err_release;
> +	}
> +
> +	cinfo->chip.ops = &dell_wmi_ddv_ops;
> +	cinfo->chip.info = cinfo->info;
> +
> +	cinfo->info[index] = dell_wmi_ddv_channel_create(&wdev->dev, 1, hwmon_chip,
> +							 HWMON_C_REGISTER_TZ);
> +
> +	if (IS_ERR(cinfo->info[index])) {
> +		ret = PTR_ERR(cinfo->info[index]);
> +
> +		goto err_release;
> +	}
> +
> +	index++;
> +
> +	cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_FAN_SENSOR_INFORMATION,
> +						       sizeof(struct fan_sensor_entry), hwmon_fan,
> +						       (HWMON_F_INPUT | HWMON_F_LABEL));
> +	if (!IS_ERR(cinfo->info[index]))
> +		index++;
> +
> +	cinfo->info[index] = dell_wmi_ddv_channel_init(wdev, DELL_DDV_THERMAL_SENSOR_INFORMATION,
> +						       sizeof(struct thermal_sensor_entry),
> +						       hwmon_temp, (HWMON_T_INPUT | HWMON_T_MIN |
> +						       HWMON_T_MAX | HWMON_T_LABEL));
> +	if (!IS_ERR(cinfo->info[index]))
> +		index++;
> +
> +	if (!index) {
> +		ret = -ENODEV;
> +
> +		goto err_release;
> +	}
> +
> +	cinfo->info[index] = NULL;
> +
> +	hdev = devm_hwmon_device_register_with_info(&wdev->dev, "dell_ddv", data, &cinfo->chip,
> +						    NULL);
> +	if (IS_ERR(hdev)) {
> +		ret = PTR_ERR(hdev);
> +
> +		goto err_release;
> +	}
> +
> +	devres_close_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
> +
> +	return 0;
> +
> +err_release:
> +	devres_release_group(&wdev->dev, dell_wmi_ddv_hwmon_add);
> +
> +	return ret;
> +}
> +
>  static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
>  {
>  	const char *uid_str;
> @@ -370,7 +795,15 @@ static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
> 
>  	dell_wmi_ddv_debugfs_init(wdev);
> 
> -	return dell_wmi_ddv_battery_add(data);
> +	ret = dell_wmi_ddv_hwmon_add(data);
> +	if (ret < 0)
> +		dev_dbg(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);

I'm fine with not making either _add failing an error, but can we make this a dev_warn,
dev_dbg is a bit too low of a log-level for something which is not supposed to happen.

E.g. change this to:

	ret = dell_wmi_ddv_hwmon_add(data);
	if (ret && ret != -ENODEV)
		dev_warn(&wdev->dev, "Unable to register hwmon interface: %d\n", ret);



> +
> +	ret = dell_wmi_ddv_battery_add(data);
> +	if (ret < 0)
> +		dev_dbg(&wdev->dev, "Unable to register acpi battery hook: %d\n", ret);

And the same here.

Regards,

Hans


> +
> +	return 0;
>  }
> 
>  static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
> --
> 2.30.2
>