LinuxLists.cc - [PATCH 0/3] hwmon: (nct6775) Support lock by ACPI mutex

[permalink] [raw]

Subject: [PATCH 1/3] hwmon: (nct6775) Use nct6775_*() lock function pointers in nct6775_data.

Prepare for platform specific callbacks usage:
* Use nct6775 lock function pointers in struct nct6775_data instead
direct calls.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <[email protected]>
---
drivers/hwmon/nct6775.c | 195 +++++++++++++++++++++++++++++-----------
1 file changed, 143 insertions(+), 52 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 93dca471972e..049c42ea66bb 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -1326,6 +1326,8 @@ struct nct6775_data {
/* nct6775_*() callbacks */
u16 (*read_value)(struct nct6775_data *data, u16 reg);
int (*write_value)(struct nct6775_data *data, u16 reg, u16 value);
+ int (*lock)(struct nct6775_data *data);
+ void (*unlock)(struct nct6775_data *data, struct device *dev);
};

struct sensor_device_template {
@@ -1918,12 +1920,26 @@ static void nct6775_update_pwm_limits(struct device *dev)
}
}

+static int nct6775_lock(struct nct6775_data *data)
+{
+ mutex_lock(&data->update_lock);
+
+ return 0;
+}
+
+static void nct6775_unlock(struct nct6775_data *data, struct device *dev)
+{
+ mutex_unlock(&data->update_lock);
+}
+
static struct nct6775_data *nct6775_update_device(struct device *dev)
{
struct nct6775_data *data = dev_get_drvdata(dev);
- int i, j;
+ int i, j, err;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return data;

if (time_after(jiffies, data->last_updated + HZ + HZ / 2)
|| !data->valid) {
@@ -2011,7 +2027,7 @@ static struct nct6775_data *nct6775_update_device(struct device *dev)
data->valid = true;
}

- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return data;
}

@@ -2043,11 +2059,15 @@ store_in_reg(struct device *dev, struct device_attribute *attr, const char *buf,
err = kstrtoul(buf, 10, &val);
if (err < 0)
return err;
- mutex_lock(&data->update_lock);
+
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->in[nr][index] = in_to_reg(val, nr);
data->write_value(data, data->REG_IN_MINMAX[index - 1][nr],
data->in[nr][index]);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -2127,14 +2147,17 @@ store_beep(struct device *dev, struct device_attribute *attr, const char *buf,
if (val > 1)
return -EINVAL;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
if (val)
data->beeps |= (1ULL << nr);
else
data->beeps &= ~(1ULL << nr);
data->write_value(data, data->REG_BEEP[regindex],
(data->beeps >> (regindex << 3)) & 0xff);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -2183,14 +2206,17 @@ store_temp_beep(struct device *dev, struct device_attribute *attr,
bit = data->BEEP_BITS[nr + TEMP_ALARM_BASE];
regindex = bit >> 3;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
if (val)
data->beeps |= (1ULL << bit);
else
data->beeps &= ~(1ULL << bit);
data->write_value(data, data->REG_BEEP[regindex],
(data->beeps >> (regindex << 3)) & 0xff);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);

return count;
}
@@ -2284,7 +2310,10 @@ store_fan_min(struct device *dev, struct device_attribute *attr,
if (err < 0)
return err;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
if (!data->has_fan_div) {
/* NCT6776F or NCT6779D; we know this is a 13 bit register */
if (!val) {
@@ -2357,7 +2386,7 @@ store_fan_min(struct device *dev, struct device_attribute *attr,

write_min:
data->write_value(data, data->REG_FAN_MIN[nr], data->fan_min[nr]);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);

return count;
}
@@ -2390,13 +2419,16 @@ store_fan_pulses(struct device *dev, struct device_attribute *attr,
if (val > 4)
return -EINVAL;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->fan_pulses[nr] = val & 3;
reg = data->read_value(data, data->REG_FAN_PULSES[nr]);
reg &= ~(0x03 << data->FAN_PULSE_SHIFT[nr]);
reg |= (val & 3) << data->FAN_PULSE_SHIFT[nr];
data->write_value(data, data->REG_FAN_PULSES[nr], reg);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);

return count;
}
@@ -2494,11 +2526,14 @@ store_temp(struct device *dev, struct device_attribute *attr, const char *buf,
if (err < 0)
return err;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->temp[index][nr] = LM75_TEMP_TO_REG(val);
nct6775_write_temp(data, data->reg_temp[index][nr],
data->temp[index][nr]);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -2527,10 +2562,13 @@ store_temp_offset(struct device *dev, struct device_attribute *attr,

val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->temp_offset[nr] = val;
data->write_value(data, data->REG_TEMP_OFFSET[nr], val);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);

return count;
}
@@ -2563,7 +2601,9 @@ store_temp_type(struct device *dev, struct device_attribute *attr,
if (val != 1 && val != 3 && val != 4)
return -EINVAL;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;

data->temp_type[nr] = val;
vbit = 0x02 << nr;
@@ -2584,7 +2624,7 @@ store_temp_type(struct device *dev, struct device_attribute *attr,
data->write_value(data, data->REG_VBAT, vbat);
data->write_value(data, data->REG_DIODE, diode);

- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -2704,14 +2744,17 @@ store_pwm_mode(struct device *dev, struct device_attribute *attr,
return count;
}

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->pwm_mode[nr] = val;
reg = data->read_value(data, data->REG_PWM_MODE[nr]);
reg &= ~data->PWM_MODE_MASK[nr];
if (!val)
reg |= data->PWM_MODE_MASK[nr];
data->write_value(data, data->REG_PWM_MODE[nr], reg);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -2756,7 +2799,10 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
return err;
val = clamp_val(val, minval[index], maxval[index]);

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->pwm[index][nr] = val;
data->write_value(data, data->REG_PWM[index][nr], val);
if (index == 2) { /* floor: disable if val == 0 */
@@ -2766,7 +2812,7 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
reg |= 0x80;
data->write_value(data, data->REG_TEMP_SEL[nr], reg);
}
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -2866,7 +2912,10 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
return -EINVAL;
}

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->pwm_enable[nr] = val;
if (val == off) {
/*
@@ -2880,7 +2929,7 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
reg &= 0x0f;
reg |= pwm_enable_to_reg(val) << 4;
data->write_value(data, data->REG_FAN_MODE[nr], reg);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -2929,14 +2978,17 @@ store_pwm_temp_sel(struct device *dev, struct device_attribute *attr,
if (!(data->have_temp & BIT(val - 1)) || !data->temp_src[val - 1])
return -EINVAL;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
src = data->temp_src[val - 1];
data->pwm_temp_sel[nr] = src;
reg = data->read_value(data, data->REG_TEMP_SEL[nr]);
reg &= 0xe0;
reg |= src;
data->write_value(data, data->REG_TEMP_SEL[nr], reg);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);

return count;
}
@@ -2973,7 +3025,10 @@ store_pwm_weight_temp_sel(struct device *dev, struct device_attribute *attr,
!data->temp_src[val - 1]))
return -EINVAL;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
if (val) {
src = data->temp_src[val - 1];
data->pwm_weight_temp_sel[nr] = src;
@@ -2987,7 +3042,7 @@ store_pwm_weight_temp_sel(struct device *dev, struct device_attribute *attr,
reg &= 0x7f;
data->write_value(data, data->REG_WEIGHT_TEMP_SEL[nr], reg);
}
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);

return count;
}
@@ -3018,10 +3073,13 @@ store_target_temp(struct device *dev, struct device_attribute *attr,
val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), 0,
data->target_temp_mask);

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->target_temp[nr] = val;
pwm_update_registers(data, nr);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3055,10 +3113,13 @@ store_target_speed(struct device *dev, struct device_attribute *attr,
val = clamp_val(val, 0, 1350000U);
speed = fan_to_reg(val, data->fan_div[nr]);

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->target_speed[nr] = speed;
pwm_update_registers(data, nr);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3092,7 +3153,10 @@ store_temp_tolerance(struct device *dev, struct device_attribute *attr,
/* Limit tolerance as needed */
val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), 0, data->tolerance_mask);

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->temp_tolerance[index][nr] = val;
if (index)
pwm_update_registers(data, nr);
@@ -3100,7 +3164,7 @@ store_temp_tolerance(struct device *dev, struct device_attribute *attr,
data->write_value(data,
data->REG_CRITICAL_TEMP_TOLERANCE[nr],
val);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3169,10 +3233,13 @@ store_speed_tolerance(struct device *dev, struct device_attribute *attr,
/* Limit tolerance as needed */
val = clamp_val(val, 0, data->speed_tolerance_limit);

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->target_speed_tolerance[nr] = val;
pwm_update_registers(data, nr);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3220,10 +3287,13 @@ store_weight_temp(struct device *dev, struct device_attribute *attr,

val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), 0, 255);

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->weight_temp[index][nr] = val;
data->write_value(data, data->REG_WEIGHT_TEMP[index][nr], val);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3269,10 +3339,13 @@ store_fan_time(struct device *dev, struct device_attribute *attr,
return err;

val = step_time_to_reg(val, data->pwm_mode[nr]);
- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->fan_time[index][nr] = val;
data->write_value(data, data->REG_FAN_TIME[index][nr], val);
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3310,7 +3383,10 @@ store_auto_pwm(struct device *dev, struct device_attribute *attr,
val = 0xff;
}

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->auto_pwm[nr][point] = val;
if (point < data->auto_pwm_num) {
data->write_value(data,
@@ -3355,7 +3431,7 @@ store_auto_pwm(struct device *dev, struct device_attribute *attr,
break;
}
}
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3391,7 +3467,10 @@ store_auto_temp(struct device *dev, struct device_attribute *attr,
if (val > 255000)
return -EINVAL;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->auto_temp[nr][point] = DIV_ROUND_CLOSEST(val, 1000);
if (point < data->auto_pwm_num) {
data->write_value(data,
@@ -3401,7 +3480,7 @@ store_auto_temp(struct device *dev, struct device_attribute *attr,
data->write_value(data, data->REG_CRITICAL_TEMP[nr],
data->auto_temp[nr][point]);
}
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3570,7 +3649,9 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,
if (kstrtoul(buf, 10, &val) || val != 0)
return -EINVAL;

- mutex_lock(&data->update_lock);
+ ret = data->lock(data);
+ if (ret)
+ return ret;

/*
* Use CR registers to clear caseopen status.
@@ -3593,7 +3674,7 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,

data->valid = false; /* Force cache refresh */
error:
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);
return count;
}

@@ -3981,6 +4062,9 @@ static int nct6775_probe(struct platform_device *pdev)
}

mutex_init(&data->update_lock);
+ data->lock = nct6775_lock;
+ data->unlock = nct6775_unlock;
+
data->name = nct6775_device_names[data->kind];
data->bank = 0xff; /* Force initial bank selection */
platform_set_drvdata(pdev, data);
@@ -4790,14 +4874,18 @@ static void nct6791_enable_io_mapping(struct nct6775_sio_data *sio_data)
static int __maybe_unused nct6775_suspend(struct device *dev)
{
struct nct6775_data *data = nct6775_update_device(dev);
+ int err;
+
+ err = data->lock(data);
+ if (err)
+ return err;

- mutex_lock(&data->update_lock);
data->vbat = data->read_value(data, data->REG_VBAT);
if (data->kind == nct6775) {
data->fandiv1 = data->read_value(data, NCT6775_REG_FANDIV1);
data->fandiv2 = data->read_value(data, NCT6775_REG_FANDIV2);
}
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);

return 0;
}
@@ -4806,10 +4894,13 @@ static int __maybe_unused nct6775_resume(struct device *dev)
{
struct nct6775_data *data = dev_get_drvdata(dev);
struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
- int i, j, err = 0;
+ int i, j, err;
u8 reg;

- mutex_lock(&data->update_lock);
+ err = data->lock(data);
+ if (err)
+ return err;
+
data->bank = 0xff; /* Force initial bank selection */

err = sio_data->sio_enter(sio_data);
@@ -4868,7 +4959,7 @@ static int __maybe_unused nct6775_resume(struct device *dev)
abort:
/* Force re-reading all values */
data->valid = false;
- mutex_unlock(&data->update_lock);
+ data->unlock(data, dev);

return err;
}
--
2.33.0

2021-11-22 21:29:17

[permalink] [raw]

Subject: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

Use ACPI lock when board has separate lock for monitoring IO.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <[email protected]>
---
drivers/hwmon/nct6775.c | 46 +++++++++++++++++++++++++++++++++--------
1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 049c42ea66bb..3aeb32093c35 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -140,6 +140,7 @@ struct nct6775_sio_data {
int ld;
enum kinds kind;
enum sensor_access access;
+ acpi_handle acpi_wmi_mutex;

/* superio_() callbacks */
void (*sio_outb)(struct nct6775_sio_data *sio_data, int reg, int val);
@@ -155,6 +156,7 @@ struct nct6775_sio_data {
#define ASUSWMI_METHODID_RHWM 0x5248574D
#define ASUSWMI_METHODID_WHWM 0x5748574D
#define ASUSWMI_UNSUPPORTED_METHOD 0xFFFFFFFE
+#define ASUSWMI_DELAY_MSEC_LOCK 500 /* Wait 0.5 s max. to get the lock */

static int nct6775_asuswmi_evaluate_method(u32 method_id, u8 bank, u8 reg, u8 val, u32 *retval)
{
@@ -1243,7 +1245,9 @@ struct nct6775_data {
unsigned int (*fan_from_reg)(u16 reg, unsigned int divreg);
unsigned int (*fan_from_reg_min)(u16 reg, unsigned int divreg);

- struct mutex update_lock;
+ struct mutex update_lock; /* non ACPI lock */
+ acpi_handle acpi_wmi_mutex; /* ACPI lock */
+
bool valid; /* true if following fields are valid */
unsigned long last_updated; /* In jiffies */

@@ -1563,6 +1567,20 @@ static int nct6775_wmi_write_value(struct nct6775_data *data, u16 reg, u16 value
return res;
}

+static int nct6775_wmi_lock(struct nct6775_data *data)
+{
+ if (ACPI_FAILURE(acpi_acquire_mutex(data->acpi_wmi_mutex, NULL, ASUSWMI_DELAY_MSEC_LOCK)))
+ return -EIO;
+
+ return 0;
+}
+
+static void nct6775_wmi_unlock(struct nct6775_data *data, struct device *dev)
+{
+ if (ACPI_FAILURE(acpi_release_mutex(data->acpi_wmi_mutex, NULL)))
+ dev_err(dev, "Failed to release mutex.");
+}
+
/*
* On older chips, only registers 0x50-0x5f are banked.
* On more recent chips, all registers are banked.
@@ -4051,6 +4069,7 @@ static int nct6775_probe(struct platform_device *pdev)

data->kind = sio_data->kind;
data->sioreg = sio_data->sioreg;
+ data->acpi_wmi_mutex = sio_data->acpi_wmi_mutex;

if (sio_data->access == access_direct) {
data->addr = res->start;
@@ -4061,9 +4080,14 @@ static int nct6775_probe(struct platform_device *pdev)
data->write_value = nct6775_wmi_write_value;
}

- mutex_init(&data->update_lock);
- data->lock = nct6775_lock;
- data->unlock = nct6775_unlock;
+ if (data->acpi_wmi_mutex) {
+ data->lock = nct6775_wmi_lock;
+ data->unlock = nct6775_wmi_unlock;
+ } else {
+ mutex_init(&data->update_lock);
+ data->lock = nct6775_lock;
+ data->unlock = nct6775_unlock;
+ }

data->name = nct6775_device_names[data->kind];
data->bank = 0xff; /* Force initial bank selection */
@@ -5114,6 +5138,7 @@ static int __init sensors_nct6775_init(void)
int sioaddr[2] = { 0x2e, 0x4e };
enum sensor_access access = access_direct;
const char *board_vendor, *board_name;
+ acpi_handle acpi_wmi_mutex = NULL;
u8 tmp;

err = platform_driver_register(&nct6775_driver);
@@ -5159,6 +5184,7 @@ static int __init sensors_nct6775_init(void)
found = true;

sio_data.access = access;
+ sio_data.acpi_wmi_mutex = acpi_wmi_mutex;

if (access == access_asuswmi) {
sio_data.sio_outb = superio_wmi_outb;
@@ -5186,11 +5212,13 @@ static int __init sensors_nct6775_init(void)
res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
res.flags = IORESOURCE_IO;

- err = acpi_check_resource_conflict(&res);
- if (err) {
- platform_device_put(pdev[i]);
- pdev[i] = NULL;
- continue;
+ if (!acpi_wmi_mutex) {
+ err = acpi_check_resource_conflict(&res);
+ if (err) {
+ platform_device_put(pdev[i]);
+ pdev[i] = NULL;
+ continue;
+ }
}

err = platform_device_add_resources(pdev[i], &res, 1);
--
2.33.0

2021-11-22 21:29:26

[permalink] [raw]

Subject: [PATCH 3/3] hwmon: (nct6775) add MAXIMUS VII HERO.

ASUS MAXIMUS VII HERO board has got an nct6775 chip, but by default
there's no use of it because of resource conflict with WMI method.

This commit adds MAXIMUS VII HERO to the list of boards and provides
ACPI mutex name that can be used as shared lock with ASUS WMI.

Logic checks that mutex is available. If mutex is not available
tries to get chip version by ACPI WMI interface.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <[email protected]>
Tested-by: Olli Asikainen <[email protected]>
---
drivers/hwmon/nct6775.c | 121 +++++++++++++++++++++++++++-------------
1 file changed, 81 insertions(+), 40 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 3aeb32093c35..c7a476bc4693 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -5100,34 +5100,60 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
*/
static struct platform_device *pdev[2];

-static const char * const asus_wmi_boards[] = {
- "ProArt X570-CREATOR WIFI",
- "Pro WS X570-ACE",
- "PRIME B360-PLUS",
- "PRIME B460-PLUS",
- "PRIME X570-PRO",
- "ROG CROSSHAIR VIII DARK HERO",
- "ROG CROSSHAIR VIII FORMULA",
- "ROG CROSSHAIR VIII HERO",
- "ROG CROSSHAIR VIII IMPACT",
- "ROG STRIX B550-E GAMING",
- "ROG STRIX B550-F GAMING",
- "ROG STRIX B550-F GAMING (WI-FI)",
- "ROG STRIX B550-I GAMING",
- "ROG STRIX X570-F GAMING",
- "ROG STRIX Z390-E GAMING",
- "ROG STRIX Z490-I GAMING",
- "TUF GAMING B550M-PLUS",
- "TUF GAMING B550M-PLUS (WI-FI)",
- "TUF GAMING B550-PLUS",
- "TUF GAMING B550-PRO",
- "TUF GAMING X570-PLUS",
- "TUF GAMING X570-PLUS (WI-FI)",
- "TUF GAMING X570-PRO (WI-FI)",
- "TUF GAMING Z490-PLUS",
- "TUF GAMING Z490-PLUS (WI-FI)",
+struct acpi_board_info {
+ char *acpi_mutex_name;
};

+#define DMI_ASUS_BOARD_INFO(name, mutex_name) \
+static struct acpi_board_info name = { \
+ .acpi_mutex_name = mutex_name, \
+}
+
+DMI_ASUS_BOARD_INFO(acpi_board_ANY, NULL);
+DMI_ASUS_BOARD_INFO(acpi_board_MAXIMUS_VII_HERO, "\\_SB_.PCI0.LPCB.SIO1.MUT0");
+DMI_ASUS_BOARD_INFO(acpi_board_ROG_STRIX_B550_E_GAMING, "\\_SB.PCI0.SBRG.SIO1.MUT0");
+
+#define DMI_EXACT_MATCH_ASUS_BOARD_NAME(name, info) { \
+ .matches = { \
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."), \
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, name), \
+ }, \
+ .driver_data = info, \
+}
+
+static const struct dmi_system_id asus_wmi_info_table[] = {
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("MAXIMUS VII HERO", &acpi_board_MAXIMUS_VII_HERO),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B360-PLUS", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B460-PLUS", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B550M-A (WI-FI)", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME X570-PRO", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("Pro WS X570-ACE", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ProArt X570-CREATOR WIFI", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII DARK HERO", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII FORMULA", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII HERO", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII IMPACT", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-E GAMING",
+ &acpi_board_ROG_STRIX_B550_E_GAMING),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING (WI-FI)", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-I GAMING", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX X570-F GAMING", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z390-E GAMING", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z490-I GAMING", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PLUS", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PRO", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS (WI-FI)", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS (WI-FI)", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PRO (WI-FI)", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS", &acpi_board_ANY),
+ DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS (WI-FI)", &acpi_board_ANY),
+ {}
+};
+MODULE_DEVICE_TABLE(dmi, asus_wmi_info_table);
+
static int __init sensors_nct6775_init(void)
{
int i, err;
@@ -5137,30 +5163,45 @@ static int __init sensors_nct6775_init(void)
struct nct6775_sio_data sio_data;
int sioaddr[2] = { 0x2e, 0x4e };
enum sensor_access access = access_direct;
- const char *board_vendor, *board_name;
+ const struct dmi_system_id *dmi_id;
+ struct acpi_board_info *board_info;
acpi_handle acpi_wmi_mutex = NULL;
- u8 tmp;
+ acpi_status status;
+ u8 tmp = 0;

err = platform_driver_register(&nct6775_driver);
if (err)
return err;

- board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
- board_name = dmi_get_system_info(DMI_BOARD_NAME);
+ dmi_id = dmi_first_match(asus_wmi_info_table);
+ if (dmi_id && dmi_id->driver_data) {
+ board_info = dmi_id->driver_data;
+ access = access_asuswmi;

- if (board_name && board_vendor &&
- !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
- err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
- board_name);
- if (err >= 0) {
- /* if reading chip id via WMI succeeds, use WMI */
- if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
- pr_info("Using Asus WMI to access %#x chip.\n", tmp);
- access = access_asuswmi;
+ if (board_info->acpi_mutex_name) {
+ status = acpi_get_handle(NULL, board_info->acpi_mutex_name,
+ &acpi_wmi_mutex);
+ if (ACPI_FAILURE(status)) {
+ pr_err("Could not get hardware access guard mutex.\n");
} else {
- pr_err("Can't read ChipID by Asus WMI.\n");
+ pr_info("Using Asus WMI mutex: %s\n", board_info->acpi_mutex_name);
+ access = access_direct;
}
}
+
+ /* if reading chip id via WMI succeeds, use WMI */
+ if (access == access_asuswmi &&
+ nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
+ access = access_direct;
+ pr_err("Can't read ChipID by Asus WMI.\n");
+ }
+
+ if (access == access_asuswmi) {
+ if (tmp)
+ pr_info("Using Asus WMI to access %#x chip.\n", tmp);
+ else
+ access = access_direct;
+ }
}

/*
--
2.33.0

2021-11-24 16:04:07

[permalink] [raw]

Subject: Re: [PATCH 1/3] hwmon: (nct6775) Use nct6775_*() lock function pointers in nct6775_data.

On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <[email protected]> wrote:

Better subject line (after prefix): Use lock function pointers in nct6775_data
(note no period and drop of redundancy)

> Prepare for platform specific callbacks usage:
> * Use nct6775 lock function pointers in struct nct6775_data instead
> direct calls.

...

> +static int nct6775_lock(struct nct6775_data *data)
> +{
> + mutex_lock(&data->update_lock);
> +
> + return 0;
> +}
> +
> +static void nct6775_unlock(struct nct6775_data *data, struct device *dev)
> +{
> + mutex_unlock(&data->update_lock);
> +}

Have you run `sparse` against this?
Install `sparse` in your distribution and make kernel with
`make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`

It might require using special annotations to these functions to make
static analysers happy.

--
With Best Regards,
Andy Shevchenko

2021-11-24 16:11:27

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

> + if (ACPI_FAILURE(acpi_acquire_mutex(data->acpi_wmi_mutex, NULL, ASUSWMI_DELAY_MSEC_LOCK)))

On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <[email protected]> wrote:

No period in the Subject.

> Use ACPI lock when board has separate lock for monitoring IO.

the board
a separate

...

> +#define ASUSWMI_DELAY_MSEC_LOCK 500 /* Wait 0.5 s max. to get the lock */

Units are the last in the names, hence (also check the comment's
location and English)

/* Wait for up to 0.5 s to acquire the lock */
#define ASUSWMI_LOCK_TIMEOUT_MS 500

...

> - struct mutex update_lock;
> + struct mutex update_lock; /* non ACPI lock */
> + acpi_handle acpi_wmi_mutex; /* ACPI lock */

Couldn't it be an anonymous union?

...

> +static int nct6775_wmi_lock(struct nct6775_data *data)
> +{
> + if (ACPI_FAILURE(acpi_acquire_mutex(data->acpi_wmi_mutex, NULL, ASUSWMI_DELAY_MSEC_LOCK)))

Please, use a temporary variable here and in the similar cases.

acpi_status status;

status = acpi_acquire_mutex(data->acpi_wmi_mutex, NULL,
ASUSWMI_LOCK_TIMEOUT_MS));
if (ACPI_FAILURE(status))

> + return -EIO;
> +
> + return 0;
> +}

--
With Best Regards,
Andy Shevchenko

2021-11-24 16:24:07

[permalink] [raw]

Subject: Re: [PATCH 3/3] hwmon: (nct6775) add MAXIMUS VII HERO.

On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <[email protected]> wrote:
>
> ASUS MAXIMUS VII HERO board has got an nct6775 chip, but by default
> there's no use of it because of resource conflict with WMI method.
>
> This commit adds MAXIMUS VII HERO to the list of boards and provides
> ACPI mutex name that can be used as shared lock with ASUS WMI.
>
> Logic checks that mutex is available. If mutex is not available
> tries to get chip version by ACPI WMI interface.

a chip

...

> +struct acpi_board_info {
> + char *acpi_mutex_name;

Looking below the name of the "name" should be rather "path".

> };

...

> +static const struct dmi_system_id asus_wmi_info_table[] = {
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("MAXIMUS VII HERO", &acpi_board_MAXIMUS_VII_HERO),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B360-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B460-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B550M-A (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME X570-PRO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("Pro WS X570-ACE", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ProArt X570-CREATOR WIFI", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII DARK HERO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII FORMULA", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII HERO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII IMPACT", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-E GAMING",
> + &acpi_board_ROG_STRIX_B550_E_GAMING),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-I GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX X570-F GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z390-E GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z490-I GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PRO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PRO (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS (WI-FI)", &acpi_board_ANY),

So, is it possible to eliminate acpi_board_ANY and use some default in
the code instead?

> + {}
> +};

....

> - if (board_name && board_vendor &&
> - !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
> - err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
> - board_name);

Do you need string_helpers.h after this change?

> - if (err >= 0) {
> - /* if reading chip id via WMI succeeds, use WMI */
> - if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
> - pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> - access = access_asuswmi;
> + if (board_info->acpi_mutex_name) {

> + status = acpi_get_handle(NULL, board_info->acpi_mutex_name,
> + &acpi_wmi_mutex);

One line?

> + if (ACPI_FAILURE(status)) {
> + pr_err("Could not get hardware access guard mutex.\n");
> } else {
> - pr_err("Can't read ChipID by Asus WMI.\n");
> + pr_info("Using Asus WMI mutex: %s\n", board_info->acpi_mutex_name);
> + access = access_direct;
> }
> }

...

> + /* if reading chip id via WMI succeeds, use WMI */

Be consistent with how you spell "ChipID" / "chip id" / etc everywhere
in the code.

...

> + if (access == access_asuswmi &&
> + nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
> + access = access_direct;
> + pr_err("Can't read ChipID by Asus WMI.\n");
> + }
> +
> + if (access == access_asuswmi) {
> + if (tmp)
> + pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> + else
> + access = access_direct;

Why not:

if (access == access_asuswmi) {
access = access_direct;
if (nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp))
pr_err("Can't read ChipID by Asus WMI.\n");
if (tmp) {
pr_info("Using Asus WMI to access %#x chip.\n", tmp);
access = access_...; // do you have this?
}
...
}

?

--
With Best Regards,
Andy Shevchenko

2021-11-24 16:25:34

[permalink] [raw]

Subject: Re: [PATCH 3/3] hwmon: (nct6775) add MAXIMUS VII HERO.

On Wed, Nov 24, 2021 at 6:21 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <[email protected]> wrote:

...

> > + if (access == access_asuswmi &&
> > + nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
> > + access = access_direct;
> > + pr_err("Can't read ChipID by Asus WMI.\n");
> > + }
> > +
> > + if (access == access_asuswmi) {
> > + if (tmp)
> > + pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> > + else
> > + access = access_direct;
>
> Why not:

> if (access == access_asuswmi) {
> access = access_direct;

Oh, just noticed above... Looks not good due to possible confusion
which means this part needs to be thought through and refactored,
perhaps by intermediate variable that defines the access and then you
assign access= to it if it satisfies the conditions.

> if (nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp))
> pr_err("Can't read ChipID by Asus WMI.\n");
> if (tmp) {
> pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> access = access_...; // do you have this?
> }
> ...
> }
>
> ?

--
With Best Regards,
Andy Shevchenko

2021-11-25 13:21:04

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

On Thu, 25 Nov 2021 at 14:14, Eugene Shalygin <[email protected]> wrote:
> Denis worked on my code with the first attempt to read EC sensors from
> ASUS motherboards and submitted it as a driver named
> "asus_wmi_ec_sensors", which is not in hwmon-next.

Sorry, "which is NOW in hwmon-next".

2021-11-25 13:21:16

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

Dear Andy, Denis, and Günter,

Denis worked on my code with the first attempt to read EC sensors from
ASUS motherboards and submitted it as a driver named
"asus_wmi_ec_sensors", which is not in hwmon-next. Now he adds the
ACPI lock feature to support other motherboards, and I have another
iteration of the EC sensor driver under development (needs some
polishment) that utilizes the same concept (ACPI lock instead of WMI
methods), which is smaller, cleaner and faster than the WMI-based one.
I'm going to submit it to the mainline too. I think it should replace
the WMI one. In anticipation of that, can we change the name of the
accepted driver (asus_wmi_ec_sensors -> asus_ec_sensors) now, in order
to not confuse users in the next version and to remove implementation
detail from the module name? The drivers provide indistinguishable
data to HWMON.

Regards,
Eugene

On Wed, 24 Nov 2021 at 17:11, Andy Shevchenko <[email protected]> wrote:
>
> > + if (ACPI_FAILURE(acpi_acquire_mutex(data->acpi_wmi_mutex, NULL, ASUSWMI_DELAY_MSEC_LOCK)))
>
>
> On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <[email protected]> wrote:
>
> No period in the Subject.
>
> > Use ACPI lock when board has separate lock for monitoring IO.
>
> the board
> a separate
>
> ...
>
> > +#define ASUSWMI_DELAY_MSEC_LOCK 500 /* Wait 0.5 s max. to get the lock */
>
> Units are the last in the names, hence (also check the comment's
> location and English)
>
> /* Wait for up to 0.5 s to acquire the lock */
> #define ASUSWMI_LOCK_TIMEOUT_MS 500
>
> ...
>
> > - struct mutex update_lock;
> > + struct mutex update_lock; /* non ACPI lock */
> > + acpi_handle acpi_wmi_mutex; /* ACPI lock */
>
> Couldn't it be an anonymous union?
>
> ...
>
> > +static int nct6775_wmi_lock(struct nct6775_data *data)
> > +{
> > + if (ACPI_FAILURE(acpi_acquire_mutex(data->acpi_wmi_mutex, NULL, ASUSWMI_DELAY_MSEC_LOCK)))
>
> Please, use a temporary variable here and in the similar cases.
>
> acpi_status status;
>
> status = acpi_acquire_mutex(data->acpi_wmi_mutex, NULL,
> ASUSWMI_LOCK_TIMEOUT_MS));
> if (ACPI_FAILURE(status))
>
> > + return -EIO;
> > +
> > + return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko

2021-11-25 13:56:50

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

On 11/25/21 5:14 AM, Eugene Shalygin wrote:
> Dear Andy, Denis, and Günter,
>
> Denis worked on my code with the first attempt to read EC sensors from
> ASUS motherboards and submitted it as a driver named
> "asus_wmi_ec_sensors", which is not in hwmon-next. Now he adds the
> ACPI lock feature to support other motherboards, and I have another
> iteration of the EC sensor driver under development (needs some
> polishment) that utilizes the same concept (ACPI lock instead of WMI
> methods), which is smaller, cleaner and faster than the WMI-based one.
> I'm going to submit it to the mainline too. I think it should replace
> the WMI one. In anticipation of that, can we change the name of the
> accepted driver (asus_wmi_ec_sensors -> asus_ec_sensors) now, in order
> to not confuse users in the next version and to remove implementation
> detail from the module name? The drivers provide indistinguishable
> data to HWMON.
>

Two drivers with the same functionality ? What would be the benefit of that ?
Why not drop the current one entirely instead ?

Guenter

2021-11-25 13:58:47

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

On Thu, Nov 25, 2021 at 3:15 PM Eugene Shalygin
<[email protected]> wrote:
>
> Dear Andy, Denis, and Günter,

Please, do not top post in the mailing lists!

> Denis worked on my code with the first attempt to read EC sensors from
> ASUS motherboards and submitted it as a driver named
> "asus_wmi_ec_sensors", which is not in hwmon-next. Now he adds the
> ACPI lock feature to support other motherboards, and I have another
> iteration of the EC sensor driver under development (needs some
> polishment) that utilizes the same concept (ACPI lock instead of WMI
> methods), which is smaller, cleaner and faster than the WMI-based one.
> I'm going to submit it to the mainline too. I think it should replace
> the WMI one. In anticipation of that, can we change the name of the
> accepted driver (asus_wmi_ec_sensors -> asus_ec_sensors) now, in order
> to not confuse users in the next version and to remove implementation
> detail from the module name? The drivers provide indistinguishable
> data to HWMON.

I'm not sure I have got the above correctly, do you mean that the just
submitted asus_wmi_sensors HWMON driver will be replaced by your
changes? If so, you guys, need a lot to improve communication between
each other before submitting anything upstream.

--
With Best Regards,
Andy Shevchenko

2021-11-25 14:00:47

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

> Two drivers with the same functionality ? What would be the benefit of that ?
> Why not drop the current one entirely instead ?

Exactly that: I want to replace the WMI one with the new one, which
relies on ACPI lock and directly talks to the EC, and I want to keep
the driver name unchanged. Thus asking to rename the WMI driver before
the 5.16 release.

2021-11-25 14:04:50

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

> Please, do not top post in the mailing lists!

Well, it was almost a new topic...

> I'm not sure I have got the above correctly, do you mean that the just
> submitted asus_wmi_sensors HWMON driver will be replaced by your
> changes? If so, you guys, need a lot to improve communication between
> each other before submitting anything upstream.

Yes, you get it right. Sorry for that, it was a long story and I
worked on the subject
only occasionally, so that when Denis took the code and submitted it
to the mainline
I was not sure which approach is better, and so I did not stop him.

Best regards,
Eugene

2021-11-25 19:56:17

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

On 11/25/21 6:00 AM, Eugene Shalygin wrote:
>> Please, do not top post in the mailing lists!
>
> Well, it was almost a new topic...
>
>> I'm not sure I have got the above correctly, do you mean that the just
>> submitted asus_wmi_sensors HWMON driver will be replaced by your
>> changes? If so, you guys, need a lot to improve communication between
>> each other before submitting anything upstream.
>
> Yes, you get it right. Sorry for that, it was a long story and I
> worked on the subject
> only occasionally, so that when Denis took the code and submitted it
> to the mainline
> I was not sure which approach is better, and so I did not stop him.
>

We won't be heving two drivers with the same functionality. Give me one
reason why I should not drop the wmi driver (or both of them; I am not
sure which one we are talking about here).

On top of all that, this submission isn't about any of the wmi drivers,
but for the nct6775 driver, which just adds to the confusion.

Guenter

2021-11-25 20:08:28

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

On Thu, 25 Nov 2021 at 20:54, Guenter Roeck <[email protected]> wrote:
> We won't be heving two drivers with the same functionality. Give me one
> reason why I should not drop the wmi driver (or both of them; I am not
> sure which one we are talking about here).
>
> On top of all that, this submission isn't about any of the wmi drivers,
> but for the nct6775 driver, which just adds to the confusion.

Let me try to explain once again, please. I began to dig into the ASUS
sensors and how to read their values and at first found the WMI
function to do that, wrote a driver and Denis submitted it as
asus_wmi_ec_sensors. Now, I know a better and simpler way to read
those sensors, which uses ACPI mutex directly. I suggested Denis to
use the mutex way for the nct6775 driver for motherboards without WMI
functions to read from the nct chip. With that change entering the nct
driver, I want to submit my updated driver for EC sensors, replacing
the asus_wmi_ec_sensors code (which is essentially my old driver).

So, now I ask to rename asus_wmi_sensors to asus_ec_sensors (source
file and KBuild options) already before 5.16 is released, because the
updated code, which I will submit later, and which will replace that
in asus_wmi_ec_sensors.c, does not use WMI.

Hope this clarifies my request and intention.

Best regards,
Eugene

2021-11-25 20:14:45

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

On Thu, Nov 25, 2021 at 10:05 PM Eugene Shalygin
<[email protected]> wrote:
>
> On Thu, 25 Nov 2021 at 20:54, Guenter Roeck <[email protected]> wrote:
> > We won't be heving two drivers with the same functionality. Give me one
> > reason why I should not drop the wmi driver (or both of them; I am not
> > sure which one we are talking about here).
> >
> > On top of all that, this submission isn't about any of the wmi drivers,
> > but for the nct6775 driver, which just adds to the confusion.
>
> Let me try to explain once again, please. I began to dig into the ASUS
> sensors and how to read their values and at first found the WMI
> function to do that, wrote a driver and Denis submitted it as
> asus_wmi_ec_sensors. Now, I know a better and simpler way to read
> those sensors, which uses ACPI mutex directly. I suggested Denis to
> use the mutex way for the nct6775 driver for motherboards without WMI
> functions to read from the nct chip. With that change entering the nct
> driver, I want to submit my updated driver for EC sensors, replacing
> the asus_wmi_ec_sensors code (which is essentially my old driver).
>
> So, now I ask to rename asus_wmi_sensors to asus_ec_sensors (source
> file and KBuild options) already before 5.16 is released, because the
> updated code, which I will submit later, and which will replace that
> in asus_wmi_ec_sensors.c, does not use WMI.
>
> Hope this clarifies my request and intention.

Since you are talking about something which is not ready yet (as I
read "will" above), I propose a compromise variant, i.e. let's leave
current driver(s) as is for v5.17 and after v5.17-rc1 you submit your
proposal for review. Okay?

--
With Best Regards,
Andy Shevchenko

2021-11-25 20:30:35

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

Hi,

On Thu, 25 Nov 2021 22:09:46 +0200
Andy Shevchenko <[email protected]> wrote:

> On Thu, Nov 25, 2021 at 10:05 PM Eugene Shalygin
> <[email protected]> wrote:
> >
> > On Thu, 25 Nov 2021 at 20:54, Guenter Roeck <[email protected]>
> > wrote:
> > > We won't be heving two drivers with the same functionality. Give
> > > me one reason why I should not drop the wmi driver (or both of
> > > them; I am not sure which one we are talking about here).
> > >
> > > On top of all that, this submission isn't about any of the wmi
> > > drivers, but for the nct6775 driver, which just adds to the
> > > confusion.
> >
> > Let me try to explain once again, please. I began to dig into the
> > ASUS sensors and how to read their values and at first found the WMI
> > function to do that, wrote a driver and Denis submitted it as
> > asus_wmi_ec_sensors. Now, I know a better and simpler way to read
> > those sensors, which uses ACPI mutex directly. I suggested Denis to
> > use the mutex way for the nct6775 driver for motherboards without
> > WMI functions to read from the nct chip. With that change entering
> > the nct driver, I want to submit my updated driver for EC sensors,
> > replacing the asus_wmi_ec_sensors code (which is essentially my old
> > driver).
> >
> > So, now I ask to rename asus_wmi_sensors to asus_ec_sensors (source
> > file and KBuild options) already before 5.16 is released, because
> > the updated code, which I will submit later, and which will replace
> > that in asus_wmi_ec_sensors.c, does not use WMI.
> >
> > Hope this clarifies my request and intention.
>
> Since you are talking about something which is not ready yet (as I
> read "will" above), I propose a compromise variant, i.e. let's leave
> current driver(s) as is for v5.17 and after v5.17-rc1 you submit your
> proposal for review. Okay?
>

I would like to propose to leave the current name of the driver and add
the same logic as in the current patch. So when we know the exact name
of acpi mutex - code will use such mutex for lock and directly read EC
memory region. In case if we don't know the exact mutex name/path or for
some reason ASUS decides to change UEFI code - code will use WMI
methods. In such a case, adding or checking a new motherboard will
require only adding a minimal list of well known registers without
disassembling UEFI code.

What do you think?

Best regards,
Denis.

2021-11-25 20:32:50

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

On Thu, 25 Nov 2021 at 21:10, Andy Shevchenko <[email protected]> wrote:

> Since you are talking about something which is not ready yet (as I
> read "will" above), I propose a compromise variant, i.e. let's leave
> current driver(s) as is for v5.17 and after v5.17-rc1 you submit your
> proposal for review. Okay?
>

Andy, I'm pretty sure the submission will happen and my only intention
is to save possible users from driver name change right after the
driver is introduced. I see no harm in renaming it.

Eugene

2021-11-25 20:40:33

[permalink] [raw]

Subject: Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

On Thu, 25 Nov 2021 at 21:25, Denis Pauk <[email protected]> wrote:

> I would like to propose to leave the current name of the driver and add
> the same logic as in the current patch. So when we know the exact name
> of acpi mutex - code will use such mutex for lock and directly read EC
> memory region. In case if we don't know the exact mutex name/path or for
> some reason ASUS decides to change UEFI code - code will use WMI
> methods. In such a case, adding or checking a new motherboard will
> require only adding a minimal list of well known registers without
> disassembling UEFI code.
>
> What do you think?

Sounds reasonable to me, but nevertheless I think dropping the "wmi"
part from the driver name would make the name clearer with the
proposed functional change.

Best regards,
Eugene

2021-11-25 21:11:29

[permalink] [raw]

Subject: Re: [PATCH 1/3] hwmon: (nct6775) Use nct6775_*() lock function pointers in nct6775_data.

On Wed, 24 Nov 2021 18:03:25 +0200
Andy Shevchenko <[email protected]> wrote:

> On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <[email protected]>
> wrote:
>
> Better subject line (after prefix): Use lock function pointers in
> nct6775_data (note no period and drop of redundancy)
>
> > Prepare for platform specific callbacks usage:
> > * Use nct6775 lock function pointers in struct nct6775_data instead
> > direct calls.
>
> ...
>
> > +static int nct6775_lock(struct nct6775_data *data)
> > +{
> > + mutex_lock(&data->update_lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void nct6775_unlock(struct nct6775_data *data, struct
> > device *dev) +{
> > + mutex_unlock(&data->update_lock);
> > +}
>
> Have you run `sparse` against this?
> Install `sparse` in your distribution and make kernel with
> `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`
>
> It might require using special annotations to these functions to make
> static analysers happy.
>

Thank you, I will validate my patches before sending with sparse also.

I have tried with sparse==0.6.4:
---
$ make CC="ccache gcc" W=1 C=2 CF=-D__CHECK_ENDIAN__ 2>&1 | grep
nct6775 -n5
....
27219- CHECK drivers/hwmon/nct6683.c
27220: CHECK drivers/hwmon/nct6775.c
27221- CHECK drivers/hwmon/nct7802.c
....
---

It has not showed any warnings. Have I missed some flag?

Best regards,
Denis.

2021-11-25 21:57:31

[permalink] [raw]

Subject: Re: [PATCH 1/3] hwmon: (nct6775) Use nct6775_*() lock function pointers in nct6775_data.

On Thu, Nov 25, 2021 at 11:07 PM Denis Pauk <[email protected]> wrote:
> On Wed, 24 Nov 2021 18:03:25 +0200
> Andy Shevchenko <[email protected]> wrote:
> > On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <[email protected]>
> > wrote:

...

> > > +static int nct6775_lock(struct nct6775_data *data)
> > > +{
> > > + mutex_lock(&data->update_lock);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void nct6775_unlock(struct nct6775_data *data, struct
> > > device *dev) +{
> > > + mutex_unlock(&data->update_lock);
> > > +}
> >
> > Have you run `sparse` against this?
> > Install `sparse` in your distribution and make kernel with
> > `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`
> >
> > It might require using special annotations to these functions to make
> > static analysers happy.
>
> Thank you, I will validate my patches before sending with sparse also.
>
> I have tried with sparse==0.6.4:
> ---
> $ make CC="ccache gcc" W=1 C=2 CF=-D__CHECK_ENDIAN__ 2>&1 | grep
> nct6775 -n5
> ....
> 27219- CHECK drivers/hwmon/nct6683.c
> 27220: CHECK drivers/hwmon/nct6775.c
> 27221- CHECK drivers/hwmon/nct7802.c
> ....
> ---
>
> It has not showed any warnings. Have I missed some flag?

No, you are all good!

--
With Best Regards,
Andy Shevchenko

2021-11-25 21:59:20