Bug 208693 - i5k_amb should use hwmon_device_register
Summary: i5k_amb should use hwmon_device_register
Status: NEW
Alias: None
Product: Drivers
Classification: Unclassified
Component: Hardware Monitoring (show other bugs)
Hardware: x86-64 Linux
: P1 low
Assignee: Jean Delvare
URL:
Keywords: trivial
Depends on:
Blocks:
 
Reported: 2020-07-25 20:20 UTC by Rob MacKinnon
Modified: 2025-01-30 20:24 UTC (History)
2 users (show)

See Also:
Kernel Version: 5.6.14
Subsystem:
Regression: No
Bisected commit-id:


Attachments
Prototype patch (16.47 KB, application/mbox)
2025-01-30 20:24 UTC, Armin Wolf
Details

Description Rob MacKinnon 2020-07-25 20:20:35 UTC
Receive the following warning while inserting module `i5k_amb`.  A check of the 5.8.0-rc8 source tree shows this still in effect and 44 other modules with the same usage would return the same warnings. If the fix is similar to bug #192201 (which it seems to be semi-related to), then this should be a trivial fix and has been floating around since 4.4.x?

```
Jul 24 00:17:06 host systemd-modules-load[676]: Inserted module 'i5k_amb'
Jul 24 00:17:06 host kernel: i5k_amb i5k_amb.0: hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
```

Cheers,
--Rob
Comment 1 Armin Wolf 2021-10-07 14:26:27 UTC
Have you still access to the hardware?
Comment 2 Guenter Roeck 2021-10-07 16:30:03 UTC
Note that the "trivial fix" is unacceptable here. It is a kludge/workaround that was accepted for the thermal subsystem because that subsystem registers with hwmon without providing a parent device and because it generates sysfs attributes on the fly, not while registering with the hwmon subsystem. That is not the case here.
Comment 3 Armin Wolf 2021-10-07 17:55:59 UTC
This patch should fix the warning, however it is compile-tested only.
Can you apply it with git to hwmon-next and report if everything works?

From e0dd025664290700f88cd00b2a798426b1be5b18 Mon Sep 17 00:00:00 2001
From: Armin Wolf <W_Armin@gmx.de>
Date: Sat, 2 Oct 2021 21:54:18 +0200
Subject: [PATCH] hwmon: (i5k_amb) Use new hwmon registration api

Use devm_hwmon_device_register_with_info() to close bug
bugzilla.kernel.org/show_bug.cgi?id=208693, which includes
renaming tempX_mid/_max to tempX_max/_crit to comply with
the hwmon sysfs standards.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/i5k_amb.c | 519 +++++++++++++++++++---------------------
 1 file changed, 245 insertions(+), 274 deletions(-)

diff --git a/drivers/hwmon/i5k_amb.c b/drivers/hwmon/i5k_amb.c
index 783fa936e4d1..e706f533b0e1 100644
--- a/drivers/hwmon/i5k_amb.c
+++ b/drivers/hwmon/i5k_amb.c
@@ -9,7 +9,6 @@
 
 #include <linux/module.h>
 #include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/log2.h>
@@ -77,39 +76,21 @@ static unsigned long amb_reg_temp(unsigned int amb)
  * might prevent us from seeing the 16th DIMM in the channel.
  */
 #define REAL_MAX_AMBS_PER_CHANNEL	15
-#define KNOBS_PER_AMB			6
 
-static unsigned long amb_num_from_reg(unsigned int byte_num, unsigned int bit)
+static unsigned int amb_from_channel(int channel)
 {
-	return byte_num * MAX_AMBS_PER_CHANNEL + bit;
-}
+	unsigned int mem_channel = channel / REAL_MAX_AMBS_PER_CHANNEL;
 
-#define AMB_SYSFS_NAME_LEN		16
-struct i5k_device_attribute {
-	struct sensor_device_attribute s_attr;
-	char name[AMB_SYSFS_NAME_LEN];
-};
+	return channel + mem_channel;
+}
 
 struct i5k_amb_data {
-	struct device *hwmon_dev;
-
 	unsigned long amb_base;
 	unsigned long amb_len;
 	u16 amb_present[MAX_MEM_CHANNELS];
 	void __iomem *amb_mmio;
-	struct i5k_device_attribute *attrs;
-	unsigned int num_attrs;
 };
 
-static ssize_t name_show(struct device *dev, struct device_attribute *devattr,
-			 char *buf)
-{
-	return sprintf(buf, "%s\n", DRVNAME);
-}
-
-
-static DEVICE_ATTR_RO(name);
-
 static struct platform_device *amb_pdev;
 
 static u8 amb_read_byte(struct i5k_amb_data *data, unsigned long offset)
@@ -123,276 +104,270 @@ static void amb_write_byte(struct i5k_amb_data *data, unsigned long offset,
 	iowrite8(val, data->amb_mmio + offset);
 }
 
-static ssize_t show_amb_alarm(struct device *dev,
-			     struct device_attribute *devattr,
-			     char *buf)
+static umode_t i5k_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
+			      int channel)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct i5k_amb_data *data = dev_get_drvdata(dev);
+	unsigned int amb = amb_from_channel(channel);
+	const struct i5k_amb_data *data = drvdata;
 
-	if (!(amb_read_byte(data, amb_reg_temp_status(attr->index)) & 0x20) &&
-	     (amb_read_byte(data, amb_reg_temp_status(attr->index)) & 0x8))
-		return sprintf(buf, "1\n");
-	else
-		return sprintf(buf, "0\n");
-}
-
-static ssize_t store_amb_min(struct device *dev,
-			     struct device_attribute *devattr,
-			     const char *buf,
-			     size_t count)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct i5k_amb_data *data = dev_get_drvdata(dev);
-	unsigned long temp;
-	int ret = kstrtoul(buf, 10, &temp);
-	if (ret < 0)
-		return ret;
-
-	temp = temp / 500;
-	if (temp > 255)
-		temp = 255;
-
-	amb_write_byte(data, amb_reg_temp_min(attr->index), temp);
-	return count;
-}
-
-static ssize_t store_amb_mid(struct device *dev,
-			     struct device_attribute *devattr,
-			     const char *buf,
-			     size_t count)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct i5k_amb_data *data = dev_get_drvdata(dev);
-	unsigned long temp;
-	int ret = kstrtoul(buf, 10, &temp);
-	if (ret < 0)
-		return ret;
+	switch (type) {
+	case hwmon_temp:
+		if (data->amb_present[amb >> CHANNEL_SHIFT] & BIT(amb & DIMM_MASK))
+			break;
 
-	temp = temp / 500;
-	if (temp > 255)
-		temp = 255;
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_alarm:
+		case hwmon_temp_label:
+			return 0444;
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+		case hwmon_temp_crit:
+			return 0644;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
 
-	amb_write_byte(data, amb_reg_temp_mid(attr->index), temp);
-	return count;
+	return 0;
 }
 
-static ssize_t store_amb_max(struct device *dev,
-			     struct device_attribute *devattr,
-			     const char *buf,
-			     size_t count)
+static int i5k_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+		    long *val)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct i5k_amb_data *data = dev_get_drvdata(dev);
-	unsigned long temp;
-	int ret = kstrtoul(buf, 10, &temp);
-	if (ret < 0)
-		return ret;
-
-	temp = temp / 500;
-	if (temp > 255)
-		temp = 255;
+	unsigned int amb = amb_from_channel(channel);
+
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			*val = 500 * amb_read_byte(data, amb_reg_temp(amb));
+
+			return 0;
+		case hwmon_temp_min:
+			*val = 500 * amb_read_byte(data, amb_reg_temp_min(amb));
+
+			return 0;
+		case hwmon_temp_max:
+			*val = 500 * amb_read_byte(data, amb_reg_temp_mid(amb));
+
+			return 0;
+		case hwmon_temp_crit:
+			*val = 500 * amb_read_byte(data, amb_reg_temp_max(amb));
+
+			return 0;
+		case hwmon_temp_alarm:
+			if (!(amb_read_byte(data, amb_reg_temp_status(amb)) & 0x20) &&
+			    amb_read_byte(data, amb_reg_temp_status(amb)) & 0x8)
+				*val = 1;
+			else
+				*val = 0;
+
+			return 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
 
-	amb_write_byte(data, amb_reg_temp_max(attr->index), temp);
-	return count;
+	return -EOPNOTSUPP;
 }
 
-static ssize_t show_amb_min(struct device *dev,
-			     struct device_attribute *devattr,
-			     char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct i5k_amb_data *data = dev_get_drvdata(dev);
-	return sprintf(buf, "%d\n",
-		500 * amb_read_byte(data, amb_reg_temp_min(attr->index)));
-}
+static const char * const channel_name[] = {
+	"Ch. 0 DIMM 0",
+	"Ch. 0 DIMM 1",
+	"Ch. 0 DIMM 2",
+	"Ch. 0 DIMM 3",
+	"Ch. 0 DIMM 4",
+	"Ch. 0 DIMM 5",
+	"Ch. 0 DIMM 6",
+	"Ch. 0 DIMM 7",
+	"Ch. 0 DIMM 8",
+	"Ch. 0 DIMM 9",
+	"Ch. 0 DIMM 10",
+	"Ch. 0 DIMM 11",
+	"Ch. 0 DIMM 12",
+	"Ch. 0 DIMM 13",
+	"Ch. 0 DIMM 14",
+	"Ch. 1 DIMM 0",
+	"Ch. 1 DIMM 1",
+	"Ch. 1 DIMM 2",
+	"Ch. 1 DIMM 3",
+	"Ch. 1 DIMM 4",
+	"Ch. 1 DIMM 5",
+	"Ch. 1 DIMM 6",
+	"Ch. 1 DIMM 7",
+	"Ch. 1 DIMM 8",
+	"Ch. 1 DIMM 9",
+	"Ch. 1 DIMM 10",
+	"Ch. 1 DIMM 11",
+	"Ch. 1 DIMM 12",
+	"Ch. 1 DIMM 13",
+	"Ch. 1 DIMM 14",
+	"Ch. 2 DIMM 0",
+	"Ch. 2 DIMM 1",
+	"Ch. 2 DIMM 2",
+	"Ch. 2 DIMM 3",
+	"Ch. 2 DIMM 4",
+	"Ch. 2 DIMM 5",
+	"Ch. 2 DIMM 6",
+	"Ch. 2 DIMM 7",
+	"Ch. 2 DIMM 8",
+	"Ch. 2 DIMM 9",
+	"Ch. 2 DIMM 10",
+	"Ch. 2 DIMM 11",
+	"Ch. 2 DIMM 12",
+	"Ch. 2 DIMM 13",
+	"Ch. 2 DIMM 14",
+	"Ch. 3 DIMM 0",
+	"Ch. 3 DIMM 1",
+	"Ch. 3 DIMM 2",
+	"Ch. 3 DIMM 3",
+	"Ch. 3 DIMM 4",
+	"Ch. 3 DIMM 5",
+	"Ch. 3 DIMM 6",
+	"Ch. 3 DIMM 7",
+	"Ch. 3 DIMM 8",
+	"Ch. 3 DIMM 9",
+	"Ch. 3 DIMM 10",
+	"Ch. 3 DIMM 11",
+	"Ch. 3 DIMM 12",
+	"Ch. 3 DIMM 13",
+	"Ch. 3 DIMM 14",
+};
 
-static ssize_t show_amb_mid(struct device *dev,
-			     struct device_attribute *devattr,
-			     char *buf)
+static int i5k_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+			   const char **str)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct i5k_amb_data *data = dev_get_drvdata(dev);
-	return sprintf(buf, "%d\n",
-		500 * amb_read_byte(data, amb_reg_temp_mid(attr->index)));
-}
+	*str = channel_name[channel];
 
-static ssize_t show_amb_max(struct device *dev,
-			     struct device_attribute *devattr,
-			     char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct i5k_amb_data *data = dev_get_drvdata(dev);
-	return sprintf(buf, "%d\n",
-		500 * amb_read_byte(data, amb_reg_temp_max(attr->index)));
+	return 0;
 }
 
-static ssize_t show_amb_temp(struct device *dev,
-			     struct device_attribute *devattr,
-			     char *buf)
+static int i5k_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+		     long val)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct i5k_amb_data *data = dev_get_drvdata(dev);
-	return sprintf(buf, "%d\n",
-		500 * amb_read_byte(data, amb_reg_temp(attr->index)));
-}
+	unsigned int amb = amb_from_channel(channel);
+	u8 temp = clamp_val(val / 500, 0, 255);
 
-static ssize_t show_label(struct device *dev,
-			  struct device_attribute *devattr,
-			  char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_min:
+			amb_write_byte(data, amb_reg_temp_min(amb), temp);
 
-	return sprintf(buf, "Ch. %d DIMM %d\n", attr->index >> CHANNEL_SHIFT,
-		       attr->index & DIMM_MASK);
-}
+			return 0;
+		case hwmon_temp_max:
+			amb_write_byte(data, amb_reg_temp_mid(amb), temp);
 
-static int i5k_amb_hwmon_init(struct platform_device *pdev)
-{
-	int i, j, k, d = 0;
-	u16 c;
-	int res = 0;
-	int num_ambs = 0;
-	struct i5k_amb_data *data = platform_get_drvdata(pdev);
+			return 0;
+		case hwmon_temp_crit:
+			amb_write_byte(data, amb_reg_temp_max(amb), temp);
 
-	/* Count the number of AMBs found */
-	/* ignore the high-order bit, see "Ugly hack" comment above */
-	for (i = 0; i < MAX_MEM_CHANNELS; i++)
-		num_ambs += hweight16(data->amb_present[i] & 0x7fff);
-
-	/* Set up sysfs stuff */
-	data->attrs = kzalloc(array3_size(num_ambs, KNOBS_PER_AMB,
-					  sizeof(*data->attrs)),
-			      GFP_KERNEL);
-	if (!data->attrs)
-		return -ENOMEM;
-	data->num_attrs = 0;
-
-	for (i = 0; i < MAX_MEM_CHANNELS; i++) {
-		c = data->amb_present[i];
-		for (j = 0; j < REAL_MAX_AMBS_PER_CHANNEL; j++, c >>= 1) {
-			struct i5k_device_attribute *iattr;
-
-			k = amb_num_from_reg(i, j);
-			if (!(c & 0x1))
-				continue;
-			d++;
-
-			/* sysfs label */
-			iattr = data->attrs + data->num_attrs;
-			snprintf(iattr->name, AMB_SYSFS_NAME_LEN,
-				 "temp%d_label", d);
-			iattr->s_attr.dev_attr.attr.name = iattr->name;
-			iattr->s_attr.dev_attr.attr.mode = 0444;
-			iattr->s_attr.dev_attr.show = show_label;
-			iattr->s_attr.index = k;
-			sysfs_attr_init(&iattr->s_attr.dev_attr.attr);
-			res = device_create_file(&pdev->dev,
-						 &iattr->s_attr.dev_attr);
-			if (res)
-				goto exit_remove;
-			data->num_attrs++;
-
-			/* Temperature sysfs knob */
-			iattr = data->attrs + data->num_attrs;
-			snprintf(iattr->name, AMB_SYSFS_NAME_LEN,
-				 "temp%d_input", d);
-			iattr->s_attr.dev_attr.attr.name = iattr->name;
-			iattr->s_attr.dev_attr.attr.mode = 0444;
-			iattr->s_attr.dev_attr.show = show_amb_temp;
-			iattr->s_attr.index = k;
-			sysfs_attr_init(&iattr->s_attr.dev_attr.attr);
-			res = device_create_file(&pdev->dev,
-						 &iattr->s_attr.dev_attr);
-			if (res)
-				goto exit_remove;
-			data->num_attrs++;
-
-			/* Temperature min sysfs knob */
-			iattr = data->attrs + data->num_attrs;
-			snprintf(iattr->name, AMB_SYSFS_NAME_LEN,
-				 "temp%d_min", d);
-			iattr->s_attr.dev_attr.attr.name = iattr->name;
-			iattr->s_attr.dev_attr.attr.mode = 0644;
-			iattr->s_attr.dev_attr.show = show_amb_min;
-			iattr->s_attr.dev_attr.store = store_amb_min;
-			iattr->s_attr.index = k;
-			sysfs_attr_init(&iattr->s_attr.dev_attr.attr);
-			res = device_create_file(&pdev->dev,
-						 &iattr->s_attr.dev_attr);
-			if (res)
-				goto exit_remove;
-			data->num_attrs++;
-
-			/* Temperature mid sysfs knob */
-			iattr = data->attrs + data->num_attrs;
-			snprintf(iattr->name, AMB_SYSFS_NAME_LEN,
-				 "temp%d_mid", d);
-			iattr->s_attr.dev_attr.attr.name = iattr->name;
-			iattr->s_attr.dev_attr.attr.mode = 0644;
-			iattr->s_attr.dev_attr.show = show_amb_mid;
-			iattr->s_attr.dev_attr.store = store_amb_mid;
-			iattr->s_attr.index = k;
-			sysfs_attr_init(&iattr->s_attr.dev_attr.attr);
-			res = device_create_file(&pdev->dev,
-						 &iattr->s_attr.dev_attr);
-			if (res)
-				goto exit_remove;
-			data->num_attrs++;
-
-			/* Temperature max sysfs knob */
-			iattr = data->attrs + data->num_attrs;
-			snprintf(iattr->name, AMB_SYSFS_NAME_LEN,
-				 "temp%d_max", d);
-			iattr->s_attr.dev_attr.attr.name = iattr->name;
-			iattr->s_attr.dev_attr.attr.mode = 0644;
-			iattr->s_attr.dev_attr.show = show_amb_max;
-			iattr->s_attr.dev_attr.store = store_amb_max;
-			iattr->s_attr.index = k;
-			sysfs_attr_init(&iattr->s_attr.dev_attr.attr);
-			res = device_create_file(&pdev->dev,
-						 &iattr->s_attr.dev_attr);
-			if (res)
-				goto exit_remove;
-			data->num_attrs++;
-
-			/* Temperature alarm sysfs knob */
-			iattr = data->attrs + data->num_attrs;
-			snprintf(iattr->name, AMB_SYSFS_NAME_LEN,
-				 "temp%d_alarm", d);
-			iattr->s_attr.dev_attr.attr.name = iattr->name;
-			iattr->s_attr.dev_attr.attr.mode = 0444;
-			iattr->s_attr.dev_attr.show = show_amb_alarm;
-			iattr->s_attr.index = k;
-			sysfs_attr_init(&iattr->s_attr.dev_attr.attr);
-			res = device_create_file(&pdev->dev,
-						 &iattr->s_attr.dev_attr);
-			if (res)
-				goto exit_remove;
-			data->num_attrs++;
+			return 0;
+		default:
+			break;
 		}
+		break;
+	default:
+		break;
 	}
 
-	res = device_create_file(&pdev->dev, &dev_attr_name);
-	if (res)
-		goto exit_remove;
-
-	data->hwmon_dev = hwmon_device_register(&pdev->dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		res = PTR_ERR(data->hwmon_dev);
-		goto exit_remove;
-	}
+	return -EOPNOTSUPP;
+}
 
-	return res;
+static const struct hwmon_ops i5k_ops = {
+	.is_visible = i5k_is_visible,
+	.read = i5k_read,
+	.read_string = i5k_read_string,
+	.write = i5k_write,
+};
 
-exit_remove:
-	device_remove_file(&pdev->dev, &dev_attr_name);
-	for (i = 0; i < data->num_attrs; i++)
-		device_remove_file(&pdev->dev, &data->attrs[i].s_attr.dev_attr);
-	kfree(data->attrs);
+#define DIMM_FLAGS	(HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_ALARM \
+			 | HWMON_T_LABEL)
+
+static const struct hwmon_channel_info *i5k_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp,
+			   /* Channel 1 */
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   /* Channel 2 */
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   /* Channel 3 */
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   /* Channel 4 */
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS,
+			   DIMM_FLAGS
+			   ),
+	NULL
+};
 
-	return res;
-}
+static const struct hwmon_chip_info i5k_chip_info = {
+	.ops = &i5k_ops,
+	.info = i5k_info,
+};
 
 static int i5k_amb_add(void)
 {
@@ -413,8 +388,7 @@ static int i5k_amb_add(void)
 	return res;
 }
 
-static int i5k_find_amb_registers(struct i5k_amb_data *data,
-					    unsigned long devid)
+static int i5k_find_amb_registers(struct i5k_amb_data *data, unsigned long devid)
 {
 	struct pci_dev *pcidev;
 	u32 val32;
@@ -498,6 +472,7 @@ MODULE_DEVICE_TABLE(pci, i5k_amb_ids);
 static int i5k_amb_probe(struct platform_device *pdev)
 {
 	struct i5k_amb_data *data;
+	struct device *hwmon_dev;
 	struct resource *reso;
 	int i, res;
 
@@ -540,7 +515,9 @@ static int i5k_amb_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
-	res = i5k_amb_hwmon_init(pdev);
+	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, DRVNAME, data, &i5k_chip_info,
+							 NULL);
+	res = PTR_ERR(hwmon_dev);
 	if (res)
 		goto err_init_failed;
 
@@ -557,14 +534,8 @@ static int i5k_amb_probe(struct platform_device *pdev)
 
 static int i5k_amb_remove(struct platform_device *pdev)
 {
-	int i;
 	struct i5k_amb_data *data = platform_get_drvdata(pdev);
 
-	hwmon_device_unregister(data->hwmon_dev);
-	device_remove_file(&pdev->dev, &dev_attr_name);
-	for (i = 0; i < data->num_attrs; i++)
-		device_remove_file(&pdev->dev, &data->attrs[i].s_attr.dev_attr);
-	kfree(data->attrs);
 	iounmap(data->amb_mmio);
 	release_mem_region(data->amb_base, data->amb_len);
 	kfree(data);
-- 
2.20.1
Comment 4 Jean Delvare 2025-01-15 08:58:08 UTC
Rob, are you still around and would you be able to test the patch?

Armin, I suggest attaching patches instead of pasting them into comments. Attachments can be downloaded and applied much more easily.

If Rob is no long around or no longer has the hardware, I suggest you simply post the patch to the linux-hwmon list and see if anyone there can test it, or review it.
Comment 5 Rob MacKinnon 2025-01-16 06:52:35 UTC
@Jean, Indeed I'm still around (honestly I'd sorta forgotten about this bug), and pretty sure that I have that Dell server still kicking around. I'll take look in the next couple of days and see if I can find it.  Depending on how things play out in the next week, I should be able to get it running again, and see about testing this under the 6.x kernel (and/or manually patching things and attaching a patch).

@Armin, thank you for doing the heavy lifting and coding a patch. My C/C++ skills aren't anywhere good enough to write fresh code.  But I can patch, build, and troubleshoot more than well enough. I'll report here any issues with the patch.
Comment 6 Armin Wolf 2025-01-30 20:24:05 UTC
Created attachment 307556 [details]
Prototype patch

Compile-tested only!

Note You need to log in before you can comment on or make changes to this bug.