diff mbox series

[v2] hwmon:Add MEC172x Micro Chip driver for Lenovo motherboards

Message ID 20231031120942.4404-1-dober6023@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series [v2] hwmon:Add MEC172x Micro Chip driver for Lenovo motherboards | expand

Commit Message

David Ober Oct. 31, 2023, 12:09 p.m. UTC 
This addition adds in the ability for the system to scan the
MEC172x EC chip in Lenovo ThinkStation systems to get the
current fan RPM speeds and the Maximum speed value for each
fan also provides the current CPU and DIMM thermal status

Signed-off-by: David Ober <dober6023@gmail.com>

v2 fixed mixcased naming
v2 add mutex protection
v2 removed references to ACPI as it is not used
v2 added comment to explain why returning a -1 is needed
---
 drivers/hwmon/lenovo-ec-sensors.c | 81 ++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 34 deletions(-)

Comments

Guenter Roeck Oct. 31, 2023, 2:51 p.m. UTC | #1 
On 10/31/23 05:09, David Ober wrote:
> This addition adds in the ability for the system to scan the
> MEC172x EC chip in Lenovo ThinkStation systems to get the
> current fan RPM speeds and the Maximum speed value for each
> fan also provides the current CPU and DIMM thermal status
> 
> Signed-off-by: David Ober <dober6023@gmail.com>
> 
> v2 fixed mixcased naming
> v2 add mutex protection
> v2 removed references to ACPI as it is not used
> v2 added comment to explain why returning a -1 is needed
> ---
>   drivers/hwmon/lenovo-ec-sensors.c | 81 ++++++++++++++++++-------------

What is this patch based on ? I don't see that driver in
drivers/hwmon/, and based on the patch I would be surprised
if I had accepted it.

Guenter
Guenter Roeck Oct. 31, 2023, 3:22 p.m. UTC | #2 
On 10/31/23 08:14, David Ober wrote:
> That was the changes from the original commit, my bad.
> 
> Do I submit the entire patch again and will it replace this, not sure how to fix this
> 

Please

- do not top-post
- never drop the mailing list from your replies

"git rebase -i <parent>", where <parent> is the baseline SHA,
lets you edit the list of patches. Select "squash" to squash
a patch into a previous one.

Guenter


> David
> 
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, October 31, 2023 10:52 AM
> To: David Ober <dober6023@gmail.com>; linux-hwmon@vger.kernel.org
> Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; jdelvare@suse.com; corbet@lwn.net; David Ober <dober@lenovo.com>; Mark Pearson <mpearson@lenovo.com>
> Subject: [External] Re: [PATCH v2] hwmon:Add MEC172x Micro Chip driver for Lenovo motherboards
> 
> On 10/31/23 05:09, David Ober wrote:
>> This addition adds in the ability for the system to scan the MEC172x
>> EC chip in Lenovo ThinkStation systems to get the current fan RPM
>> speeds and the Maximum speed value for each fan also provides the
>> current CPU and DIMM thermal status
>>
>> Signed-off-by: David Ober <dober6023@gmail.com>
>>
>> v2 fixed mixcased naming
>> v2 add mutex protection
>> v2 removed references to ACPI as it is not used
>> v2 added comment to explain why returning a -1 is needed
>> ---
>>    drivers/hwmon/lenovo-ec-sensors.c | 81
>> ++++++++++++++++++-------------
> 
> What is this patch based on ? I don't see that driver in drivers/hwmon/, and based on the patch I would be surprised if I had accepted it.
> 
> Guenter
>
diff mbox series

Patch

diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index acf26ed4c96b..e2b14b3aea08 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -32,8 +32,8 @@ 
 #define MCHP_EMI0_EC_DATA_BYTE2		0x0906
 #define MCHP_EMI0_EC_DATA_BYTE3		0x0907
 
-#define IoWrite8(a, b)	outb_p(b, a)
-#define IoRead8(a)	inb_p(a)
+#define io_write8(a, b)	outb_p(b, a)
+#define io_read8(a)	inb_p(a)
 
 static inline uint8_t
 get_ec_reg(unsigned char page, unsigned char index)
@@ -42,31 +42,31 @@  get_ec_reg(unsigned char page, unsigned char index)
 	unsigned short m_index;
 	unsigned short phy_index = page * 256 + index;
 
-	if (IoRead8(MCHP_EMI0_APPLICATION_ID) != 0) /* EMI access locked */
-		return false;
+	if (io_read8(MCHP_EMI0_APPLICATION_ID) != 0) /* EMI access locked */
+		return -1;
 
-	IoWrite8(MCHP_EMI0_APPLICATION_ID, 0x01);
+	io_write8(MCHP_EMI0_APPLICATION_ID, 0x01);
 
 	m_index = phy_index & 0x7FFC;
-	IoWrite8(MCHP_EMI0_EC_ADDRESS_LSB, m_index);
-	IoWrite8(MCHP_EMI0_EC_ADDRESS_MSB, m_index >> 8);
+	io_write8(MCHP_EMI0_EC_ADDRESS_LSB, m_index);
+	io_write8(MCHP_EMI0_EC_ADDRESS_MSB, m_index >> 8);
 
 	switch (phy_index & 0x0003) {
 	case 0:
-		onebyte = IoRead8(MCHP_EMI0_EC_DATA_BYTE0);
+		onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE0);
 		break;
 	case 1:
-		onebyte = IoRead8(MCHP_EMI0_EC_DATA_BYTE1);
+		onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE1);
 		break;
 	case 2:
-		onebyte = IoRead8(MCHP_EMI0_EC_DATA_BYTE2);
+		onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE2);
 		break;
 	case 3:
-		onebyte = IoRead8(MCHP_EMI0_EC_DATA_BYTE3);
+		onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE3);
 		break;
 	}
 
-	IoWrite8(MCHP_EMI0_APPLICATION_ID, 0x01);  /* write same data to clean */
+	io_write8(MCHP_EMI0_APPLICATION_ID, 0x01);  /* write same data to clean */
 	return onebyte;
 }
 
@@ -190,19 +190,22 @@  static const char * const p7_amd_ec_fan_label[] = {
 };
 
 struct ec_sensors_data {
+	struct mutex mec_mutex; /* lock for sensors write */
 	u8 platform_id;
 	const char *const *fan_labels;
 	const char *const *temp_labels;
 };
 
 static int
-lenovo_ec_do_read_temp(u32 attr, int channel, long *val)
+lenovo_ec_do_read_temp(struct ec_sensors_data *data, u32 attr, int channel, long *val)
 {
 	u8   LSB;
 
 	switch (attr) {
 	case hwmon_temp_input:
+		mutex_lock(&data->mec_mutex);
 		LSB = get_ec_reg(2, 0x81 + channel);
+		mutex_unlock(&data->mec_mutex);
 		if (LSB > 0x40) {
 			*val = (LSB - 0x40) * 1000;
 		} else {
@@ -217,28 +220,36 @@  lenovo_ec_do_read_temp(u32 attr, int channel, long *val)
 }
 
 static int
-lenovo_ec_do_read_fan(u32 attr, int channel, long *val)
+lenovo_ec_do_read_fan(struct ec_sensors_data *data, u32 attr, int channel, long *val)
 {
 	u8    LSB, MSB;
 
 	channel *= 2;
 	switch (attr) {
 	case hwmon_fan_input:
+		mutex_lock(&data->mec_mutex);
 		LSB = get_ec_reg(4, 0x60 + channel);
 		MSB = get_ec_reg(4, 0x61 + channel);
+		mutex_unlock(&data->mec_mutex);
 		if ((MSB << 8) + LSB != 0) {
+			mutex_lock(&data->mec_mutex);
 			LSB = get_ec_reg(4, 0x20 + channel);
 			MSB = get_ec_reg(4, 0x21 + channel);
+			mutex_unlock(&data->mec_mutex);
 			*val = (MSB << 8) + LSB;
 			return 0;
 		}
-		return -1;
+		return -1; /* Returning -1 here has the sensors tool mark the FAN speed as N/A */
 	case hwmon_fan_max:
+		mutex_lock(&data->mec_mutex);
 		LSB = get_ec_reg(4, 0x60 + channel);
 		MSB = get_ec_reg(4, 0x61 + channel);
+		mutex_unlock(&data->mec_mutex);
 		if ((MSB << 8) + LSB != 0) {
+			mutex_lock(&data->mec_mutex);
 			LSB = get_ec_reg(4, 0x40 + channel);
 			MSB = get_ec_reg(4, 0x41 + channel);
+			mutex_unlock(&data->mec_mutex);
 			*val = (MSB << 8) + LSB;
 		} else {
 			*val = 0;
@@ -254,14 +265,16 @@  lenovo_ec_do_read_fan(u32 attr, int channel, long *val)
 	return -EOPNOTSUPP;
 }
 
-static int get_platform(void)
+static int get_platform(struct ec_sensors_data *data)
 {
 	char system_type[6];
 	int ret = -1;
 	int idx;
 
 	for (idx = 0 ; idx < 6 ; idx++)
+		mutex_lock(&data->mec_mutex);
 		system_type[idx] = get_ec_reg(0xC, (0x10 + idx));
+		mutex_unlock(&data->mec_mutex);
 
 	for (idx = 0 ; idx < 4 ; idx++) {
 		if (strcmp(systems[idx], system_type) == 0) {
@@ -296,11 +309,13 @@  static int
 lenovo_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 		     u32 attr, int channel, long *val)
 {
+	struct ec_sensors_data *data = dev_get_drvdata(dev);
+
 	switch (type) {
 	case hwmon_temp:
-		return lenovo_ec_do_read_temp(attr, channel, val);
+		return lenovo_ec_do_read_temp(data, attr, channel, val);
 	case hwmon_fan:
-		return lenovo_ec_do_read_fan(attr, channel, val);
+		return lenovo_ec_do_read_fan(data, attr, channel, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -312,9 +327,6 @@  static umode_t
 lenovo_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
 			   u32 attr, int channel)
 {
-	//if (type != hwmon_fan)
-//		return 0;
-
 	switch (type) {
 	case hwmon_temp:
 		if (attr == hwmon_temp_input || attr == hwmon_temp_label)
@@ -393,17 +405,19 @@  static int lenovo_ec_probe(struct platform_device *pdev)
 
 	chip_info = &lenovo_ec_chip_info;
 
-	if (IoRead8(0x90C) != 0) {               /* check EMI Application BIT */
-		IoWrite8(0x90C, IoRead8(0x90C)); /* set EMI Application BIT to 0 */
+	mutex_lock(&ec_data->mec_mutex);
+	if (io_read8(MCHP_EMI0_APPLICATION_ID) != 0) { /* check EMI Application BIT */
+		io_write8(0x90C, io_read8(0x90C)); /* set EMI Application BIT to 0 */
 	}
-	IoWrite8(MCHP_EMI0_EC_ADDRESS_LSB, MCHP_SING_IDX);
-	IoWrite8(MCHP_EMI0_EC_ADDRESS_MSB, MCHP_SING_IDX >> 8);
-
-	if ((IoRead8(MCHP_EMI0_EC_DATA_BYTE0) == 'M') &&
-	    (IoRead8(MCHP_EMI0_EC_DATA_BYTE1) == 'C') &&
-	    (IoRead8(MCHP_EMI0_EC_DATA_BYTE2) == 'H') &&
-	    (IoRead8(MCHP_EMI0_EC_DATA_BYTE3) == 'P')) {
-		ec_data->platform_id = get_platform();
+	io_write8(MCHP_EMI0_EC_ADDRESS_LSB, MCHP_SING_IDX);
+	io_write8(MCHP_EMI0_EC_ADDRESS_MSB, MCHP_SING_IDX >> 8);
+	mutex_unlock(&ec_data->mec_mutex);
+
+	if ((io_read8(MCHP_EMI0_EC_DATA_BYTE0) == 'M') &&
+	    (io_read8(MCHP_EMI0_EC_DATA_BYTE1) == 'C') &&
+	    (io_read8(MCHP_EMI0_EC_DATA_BYTE2) == 'H') &&
+	    (io_read8(MCHP_EMI0_EC_DATA_BYTE3) == 'P')) {
+		ec_data->platform_id = get_platform(ec_data);
 		switch (ec_data->platform_id) {
 		case 0:
 			ec_data->fan_labels = px_ec_fan_label;
@@ -431,9 +445,8 @@  static int lenovo_ec_probe(struct platform_device *pdev)
 							     chip_info, NULL);
 
 		return PTR_ERR_OR_ZERO(hwdev);
-	} else {
-		return -ENODEV;
 	}
+	return -ENODEV;
 }
 
 static struct platform_driver lenovo_ec_sensors_platform_driver = {
@@ -467,5 +480,5 @@  module_init(lenovo_ec_init);
 module_exit(lenovo_ec_exit);
 
 MODULE_AUTHOR("David Ober <dober@lenovo.com>");
-MODULE_DESCRIPTION("HWMON driver for MEC172x EC sensors accessible via ACPI on LENOVO motherboards");
+MODULE_DESCRIPTION("HWMON driver for MEC172x EC sensors on LENOVO motherboards");
 MODULE_LICENSE("GPL");