From: Frank Crawford <frank@crawford.emu.id.au>
To: Jean Delvare <jdelvare@suse.com>, Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org, Frank Crawford <frank@crawford.emu.id.au>
Subject: [PATCH v2 3/4] hwmon: (it87) Add calls to smbus_enable/smbus_disable as required
Date: Sun, 16 Apr 2023 14:25:09 +1000 [thread overview]
Message-ID: <20230416042510.1929077-4-frank@crawford.emu.id.au> (raw)
In-Reply-To: <20230416042510.1929077-1-frank@crawford.emu.id.au>
Disable/re-enable access through SMBus for chip registers when they are
are being read or written.
For simple cases this is done at the same time as when a mutex is set,
however, within loops or during initialisation it is done separately.
Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
v2:
* Reorder patches within patchset.
* Remove spaces before tab.
---
drivers/hwmon/it87.c | 181 ++++++++++++++++++++++++++++++++-----------
1 file changed, 135 insertions(+), 46 deletions(-)
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 920fb9b6b2e2..2922f551b717 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -746,6 +746,7 @@ static int smbus_enable(struct it87_data *data)
/*
* Must be called with data->update_lock held, except during initialization.
+ * Must be called with SMBus accesses disabled.
* We ignore the IT87 BUSY flag at this moment - it could lead to deadlocks,
* would slow down the IT87 access and should not be necessary.
*/
@@ -757,6 +758,7 @@ static int it87_read_value(struct it87_data *data, u8 reg)
/*
* Must be called with data->update_lock held, except during initialization.
+ * Must be called with SMBus accesses disabled.
* We ignore the IT87 BUSY flag at this moment - it could lead to deadlocks,
* would slow down the IT87 access and should not be necessary.
*/
@@ -816,15 +818,39 @@ static void it87_update_pwm_ctrl(struct it87_data *data, int nr)
}
}
+static int it87_lock(struct it87_data *data)
+{
+ int err;
+
+ mutex_lock(&data->update_lock);
+ err = smbus_disable(data);
+ if (err)
+ mutex_unlock(&data->update_lock);
+ return err;
+}
+
+static void it87_unlock(struct it87_data *data)
+{
+ smbus_enable(data);
+ mutex_unlock(&data->update_lock);
+}
+
static struct it87_data *it87_update_device(struct device *dev)
{
struct it87_data *data = dev_get_drvdata(dev);
+ struct it87_data *ret = data;
+ int err;
int i;
mutex_lock(&data->update_lock);
if (time_after(jiffies, data->last_updated + HZ + HZ / 2) ||
- !data->valid) {
+ !data->valid) {
+ err = smbus_disable(data);
+ if (err) {
+ ret = ERR_PTR(err);
+ goto unlock;
+ }
if (update_vbat) {
/*
* Cleared after each update, so reenable. Value
@@ -927,11 +953,11 @@ static struct it87_data *it87_update_device(struct device *dev)
}
data->last_updated = jiffies;
data->valid = true;
+ smbus_enable(data);
}
-
+unlock:
mutex_unlock(&data->update_lock);
-
- return data;
+ return ret;
}
static ssize_t show_in(struct device *dev, struct device_attribute *attr,
@@ -956,17 +982,21 @@ static ssize_t set_in(struct device *dev, struct device_attribute *attr,
int index = sattr->index;
int nr = sattr->nr;
unsigned long val;
+ int err;
if (kstrtoul(buf, 10, &val) < 0)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
data->in[nr][index] = in_to_reg(data, nr, val);
it87_write_value(data,
index == 1 ? IT87_REG_VIN_MIN(nr)
: IT87_REG_VIN_MAX(nr),
data->in[nr][index]);
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1048,11 +1078,14 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
struct it87_data *data = dev_get_drvdata(dev);
long val;
u8 reg, regval;
+ int err;
if (kstrtol(buf, 10, &val) < 0)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
switch (index) {
default:
@@ -1075,7 +1108,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
data->temp[nr][index] = TEMP_TO_REG(val);
it87_write_value(data, reg, data->temp[nr][index]);
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1135,10 +1168,15 @@ static ssize_t set_temp_type(struct device *dev, struct device_attribute *attr,
struct it87_data *data = dev_get_drvdata(dev);
long val;
u8 reg, extra;
+ int err;
if (kstrtol(buf, 10, &val) < 0)
return -EINVAL;
+ err = it87_lock(data);
+ if (err)
+ return err;
+
reg = it87_read_value(data, IT87_REG_TEMP_ENABLE);
reg &= ~(1 << nr);
reg &= ~(8 << nr);
@@ -1161,17 +1199,19 @@ static ssize_t set_temp_type(struct device *dev, struct device_attribute *attr,
reg |= (nr + 1) << 6;
else if (has_temp_old_peci(data, nr) && val == 6)
extra |= 0x80;
- else if (val != 0)
- return -EINVAL;
+ else if (val != 0) {
+ count = -EINVAL;
+ goto unlock;
+ }
- mutex_lock(&data->update_lock);
data->sensor = reg;
data->extra = extra;
it87_write_value(data, IT87_REG_TEMP_ENABLE, data->sensor);
if (has_temp_old_peci(data, nr))
it87_write_value(data, IT87_REG_TEMP_EXTRA, data->extra);
data->valid = false; /* Force cache refresh */
- mutex_unlock(&data->update_lock);
+unlock:
+ it87_unlock(data);
return count;
}
@@ -1287,12 +1327,15 @@ static ssize_t set_fan(struct device *dev, struct device_attribute *attr,
struct it87_data *data = dev_get_drvdata(dev);
long val;
+ int err;
u8 reg;
if (kstrtol(buf, 10, &val) < 0)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
if (has_16bit_fans(data)) {
data->fan[nr][index] = FAN16_TO_REG(val);
@@ -1319,7 +1362,7 @@ static ssize_t set_fan(struct device *dev, struct device_attribute *attr,
data->fan[nr][index]);
}
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1330,13 +1373,16 @@ static ssize_t set_fan_div(struct device *dev, struct device_attribute *attr,
struct it87_data *data = dev_get_drvdata(dev);
int nr = sensor_attr->index;
unsigned long val;
- int min;
+ int min, err;
u8 old;
if (kstrtoul(buf, 10, &val) < 0)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
old = it87_read_value(data, IT87_REG_FAN_DIV);
/* Save fan min limit */
@@ -1364,7 +1410,7 @@ static ssize_t set_fan_div(struct device *dev, struct device_attribute *attr,
data->fan[nr][1] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
it87_write_value(data, IT87_REG_FAN_MIN[nr], data->fan[nr][1]);
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1405,6 +1451,7 @@ static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
struct it87_data *data = dev_get_drvdata(dev);
int nr = sensor_attr->index;
long val;
+ int err;
if (kstrtol(buf, 10, &val) < 0 || val < 0 || val > 2)
return -EINVAL;
@@ -1415,7 +1462,9 @@ static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
return -EINVAL;
}
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
if (val == 0) {
if (nr < 3 && data->type != it8603) {
@@ -1466,7 +1515,7 @@ static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
}
}
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1477,11 +1526,15 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
struct it87_data *data = dev_get_drvdata(dev);
int nr = sensor_attr->index;
long val;
+ int err;
if (kstrtol(buf, 10, &val) < 0 || val < 0 || val > 255)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
it87_update_pwm_ctrl(data, nr);
if (has_newer_autopwm(data)) {
/*
@@ -1489,8 +1542,8 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
* is read-only so we can't write the value.
*/
if (data->pwm_ctrl[nr] & 0x80) {
- mutex_unlock(&data->update_lock);
- return -EBUSY;
+ count = -EBUSY;
+ goto unlock;
}
data->pwm_duty[nr] = pwm_to_reg(data, val);
it87_write_value(data, IT87_REG_PWM_DUTY[nr],
@@ -1507,7 +1560,8 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
data->pwm_ctrl[nr]);
}
}
- mutex_unlock(&data->update_lock);
+unlock:
+ it87_unlock(data);
return count;
}
@@ -1518,6 +1572,7 @@ static ssize_t set_pwm_freq(struct device *dev, struct device_attribute *attr,
struct it87_data *data = dev_get_drvdata(dev);
int nr = sensor_attr->index;
unsigned long val;
+ int err;
int i;
if (kstrtoul(buf, 10, &val) < 0)
@@ -1532,7 +1587,10 @@ static ssize_t set_pwm_freq(struct device *dev, struct device_attribute *attr,
break;
}
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
if (nr == 0) {
data->fan_ctl = it87_read_value(data, IT87_REG_FAN_CTL) & 0x8f;
data->fan_ctl |= i << 4;
@@ -1542,7 +1600,7 @@ static ssize_t set_pwm_freq(struct device *dev, struct device_attribute *attr,
data->extra |= i << 4;
it87_write_value(data, IT87_REG_TEMP_EXTRA, data->extra);
}
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1575,6 +1633,7 @@ static ssize_t set_pwm_temp_map(struct device *dev,
struct it87_data *data = dev_get_drvdata(dev);
int nr = sensor_attr->index;
long val;
+ int err;
u8 reg;
if (kstrtol(buf, 10, &val) < 0)
@@ -1597,7 +1656,10 @@ static ssize_t set_pwm_temp_map(struct device *dev,
return -EINVAL;
}
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
it87_update_pwm_ctrl(data, nr);
data->pwm_temp_map[nr] = reg;
/*
@@ -1609,7 +1671,7 @@ static ssize_t set_pwm_temp_map(struct device *dev,
data->pwm_temp_map[nr];
it87_write_value(data, IT87_REG_PWM[nr], data->pwm_ctrl[nr]);
}
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1639,18 +1701,22 @@ static ssize_t set_auto_pwm(struct device *dev, struct device_attribute *attr,
int point = sensor_attr->index;
int regaddr;
long val;
+ int err;
if (kstrtol(buf, 10, &val) < 0 || val < 0 || val > 255)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
data->auto_pwm[nr][point] = pwm_to_reg(data, val);
if (has_newer_autopwm(data))
regaddr = IT87_REG_AUTO_TEMP(nr, 3);
else
regaddr = IT87_REG_AUTO_PWM(nr, point);
it87_write_value(data, regaddr, data->auto_pwm[nr][point]);
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1675,15 +1741,19 @@ static ssize_t set_auto_pwm_slope(struct device *dev,
struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
int nr = sensor_attr->index;
unsigned long val;
+ int err;
if (kstrtoul(buf, 10, &val) < 0 || val > 127)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
data->auto_pwm[nr][1] = (data->auto_pwm[nr][1] & 0x80) | val;
it87_write_value(data, IT87_REG_AUTO_TEMP(nr, 4),
data->auto_pwm[nr][1]);
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1718,11 +1788,15 @@ static ssize_t set_auto_temp(struct device *dev, struct device_attribute *attr,
int point = sensor_attr->index;
long val;
int reg;
+ int err;
if (kstrtol(buf, 10, &val) < 0 || val < -128000 || val > 127000)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
if (has_newer_autopwm(data) && !point) {
reg = data->auto_temp[nr][1] - TEMP_TO_REG(val);
reg = clamp_val(reg, 0, 0x1f) | (data->auto_temp[nr][0] & 0xe0);
@@ -1735,7 +1809,7 @@ static ssize_t set_auto_temp(struct device *dev, struct device_attribute *attr,
point--;
it87_write_value(data, IT87_REG_AUTO_TEMP(nr, point), reg);
}
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -1944,13 +2018,16 @@ static ssize_t clear_intrusion(struct device *dev,
size_t count)
{
struct it87_data *data = dev_get_drvdata(dev);
- int config;
+ int err, config;
long val;
if (kstrtol(buf, 10, &val) < 0 || val != 0)
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
config = it87_read_value(data, IT87_REG_CONFIG);
if (config < 0) {
count = config;
@@ -1960,8 +2037,7 @@ static ssize_t clear_intrusion(struct device *dev,
/* Invalidate cache to force re-read */
data->valid = false;
}
- mutex_unlock(&data->update_lock);
-
+ it87_unlock(data);
return count;
}
@@ -2003,18 +2079,22 @@ static ssize_t set_beep(struct device *dev, struct device_attribute *attr,
int bitnr = to_sensor_dev_attr(attr)->index;
struct it87_data *data = dev_get_drvdata(dev);
long val;
+ int err;
if (kstrtol(buf, 10, &val) < 0 || (val != 0 && val != 1))
return -EINVAL;
- mutex_lock(&data->update_lock);
+ err = it87_lock(data);
+ if (err)
+ return err;
+
data->beeps = it87_read_value(data, IT87_REG_BEEP_ENABLE);
if (val)
data->beeps |= BIT(bitnr);
else
data->beeps &= ~BIT(bitnr);
it87_write_value(data, IT87_REG_BEEP_ENABLE, data->beeps);
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
return count;
}
@@ -3177,6 +3257,7 @@ static int it87_probe(struct platform_device *pdev)
struct it87_sio_data *sio_data = dev_get_platdata(dev);
int enable_pwm_interface;
struct device *hwmon_dev;
+ int err;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
if (!devm_request_region(&pdev->dev, res->start, IT87_EC_EXTENT,
@@ -3222,15 +3303,21 @@ static int it87_probe(struct platform_device *pdev)
break;
}
- /* Now, we do the remaining detection. */
- if ((it87_read_value(data, IT87_REG_CONFIG) & 0x80) ||
- it87_read_value(data, IT87_REG_CHIPID) != 0x90)
- return -ENODEV;
-
platform_set_drvdata(pdev, data);
mutex_init(&data->update_lock);
+ err = smbus_disable(data);
+ if (err)
+ return err;
+
+ /* Now, we do the remaining detection. */
+ if ((it87_read_value(data, IT87_REG_CONFIG) & 0x80) ||
+ it87_read_value(data, IT87_REG_CHIPID) != 0x90) {
+ smbus_enable(data);
+ return -ENODEV;
+ }
+
/* Check PWM configuration */
enable_pwm_interface = it87_check_pwm(dev);
if (!enable_pwm_interface)
@@ -3291,6 +3378,8 @@ static int it87_probe(struct platform_device *pdev)
/* Initialize the IT87 chip */
it87_init_device(pdev);
+ smbus_enable(data);
+
if (!sio_data->skip_vid) {
data->has_vid = true;
data->vrm = vid_which_vrm();
@@ -3358,7 +3447,7 @@ static int it87_resume(struct device *dev)
it87_resume_sio(pdev);
- mutex_lock(&data->update_lock);
+ it87_lock(data);
it87_check_pwm(dev);
it87_check_limit_regs(data);
@@ -3371,7 +3460,7 @@ static int it87_resume(struct device *dev)
/* force update */
data->valid = false;
- mutex_unlock(&data->update_lock);
+ it87_unlock(data);
it87_update_device(dev);
--
2.39.2
next prev parent reply other threads:[~2023-04-16 4:25 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-16 4:25 [PATCH v2 0/4] hwmon: (it87) Add access control for SMBus Frank Crawford
2023-04-16 4:25 ` [PATCH v2 1/4] hwmon: (it87) Disable SMBus access for environmental controller registers Frank Crawford
2023-04-16 4:25 ` [PATCH v2 2/4] hwmon: (it87) Test for error in it87_update_device Frank Crawford
2023-04-16 4:25 ` Frank Crawford [this message]
2023-04-16 4:25 ` [PATCH v2 4/4] hwmon: (it87) Disable/enable SMBus access for IT8622E chipset Frank Crawford
2023-04-16 14:51 [PATCH v2 3/4] hwmon: (it87) Add calls to smbus_enable/smbus_disable as required Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230416042510.1929077-4-frank@crawford.emu.id.au \
--to=frank@crawford.emu.id.au \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux@roeck-us.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.