diff mbox series

[RFC,v2,4/4] hwmon: (jc42) Don't cache the temperature register

Message ID 20221020210320.1624617-5-martin.blumenstingl@googlemail.com
State New
Headers
Series hwmon: (jc42) regmap conversion and resume fix |

Commit Message

Martin Blumenstingl Oct. 20, 2022, 9:03 p.m. UTC 
  Now that we're utilizing regmap and it's regcache for the
minimum/maximum/critical temperature registers the only cached register
that's left is the actual temperature register. Drop the custom cache
implementation as it just complicates things.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/hwmon/jc42.c | 59 ++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 38 deletions(-)

Comments

Guenter Roeck Oct. 20, 2022, 10:14 p.m. UTC | #1 
On Thu, Oct 20, 2022 at 11:03:20PM +0200, Martin Blumenstingl wrote:
> Now that we're utilizing regmap and it's regcache for the
> minimum/maximum/critical temperature registers the only cached register
> that's left is the actual temperature register. Drop the custom cache
> implementation as it just complicates things.
> 

Ah, you got there eventually. Just combine this patch into the first
patch of the series. No need to keep separate patches, especially since
a lot of the code changed in patch 1 and 2 is just thrown away here.

That reminds me, though: Make sure that the alarm bits are not dropped
after reading the temperature (running the 'sensors' command with
alarms active should do). I have some JC42 chips here and will do the
same.

Thanks,
Guenter
Martin Blumenstingl Oct. 20, 2022, 10:22 p.m. UTC | #2 
Hi Guenter,

On Fri, Oct 21, 2022 at 12:14 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Oct 20, 2022 at 11:03:20PM +0200, Martin Blumenstingl wrote:
> > Now that we're utilizing regmap and it's regcache for the
> > minimum/maximum/critical temperature registers the only cached register
> > that's left is the actual temperature register. Drop the custom cache
> > implementation as it just complicates things.
> >
>
> Ah, you got there eventually. Just combine this patch into the first
> patch of the series. No need to keep separate patches, especially since
> a lot of the code changed in patch 1 and 2 is just thrown away here.
Thanks again for the quick response and for the great feedback. I'll
combine the patches tomorrow and send a v3!

> That reminds me, though: Make sure that the alarm bits are not dropped
> after reading the temperature (running the 'sensors' command with
> alarms active should do). I have some JC42 chips here and will do the
> same.
I configured below ambient high and crit temperatures:
  jc42-i2c-0-1a
  Adapter: SMBus PIIX4 adapter port 0 at 0b00
  temp1:        +35.0°C  (low  =  +0.0°C)                  ALARM (HIGH, CRIT)
                        (high = +25.0°C, hyst = +25.0°C)
                        (crit = +30.0°C, hyst = +30.0°C)

Then I ran "sensors" three times in a row.
The output of all "sensors" commands is the same, meaning all of them
show the ALARM (HIGH, CRIT) part.

Do you want me to mention this somewhere (for example in the
cover-letter or the new patch #1)?


Best regards,
Martin
Guenter Roeck Oct. 20, 2022, 10:37 p.m. UTC | #3 
On 10/20/22 15:22, Martin Blumenstingl wrote:
> Hi Guenter,
> 
> On Fri, Oct 21, 2022 at 12:14 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Thu, Oct 20, 2022 at 11:03:20PM +0200, Martin Blumenstingl wrote:
>>> Now that we're utilizing regmap and it's regcache for the
>>> minimum/maximum/critical temperature registers the only cached register
>>> that's left is the actual temperature register. Drop the custom cache
>>> implementation as it just complicates things.
>>>
>>
>> Ah, you got there eventually. Just combine this patch into the first
>> patch of the series. No need to keep separate patches, especially since
>> a lot of the code changed in patch 1 and 2 is just thrown away here.
> Thanks again for the quick response and for the great feedback. I'll
> combine the patches tomorrow and send a v3!
> 
>> That reminds me, though: Make sure that the alarm bits are not dropped
>> after reading the temperature (running the 'sensors' command with
>> alarms active should do). I have some JC42 chips here and will do the
>> same.
> I configured below ambient high and crit temperatures:
>    jc42-i2c-0-1a
>    Adapter: SMBus PIIX4 adapter port 0 at 0b00
>    temp1:        +35.0°C  (low  =  +0.0°C)                  ALARM (HIGH, CRIT)
>                          (high = +25.0°C, hyst = +25.0°C)
>                          (crit = +30.0°C, hyst = +30.0°C)
> 
> Then I ran "sensors" three times in a row.
> The output of all "sensors" commands is the same, meaning all of them
> show the ALARM (HIGH, CRIT) part.
> 

Excellent!

> Do you want me to mention this somewhere (for example in the
> cover-letter or the new patch #1)?
> 
Yes, please mention it in the cover letter.

Background for the question: Some older sensor chips (granted, typically
20+ years old) tend to reset the alarm status after reading it, only
to set it again after the next measurement cycle.

Thanks,
Guenter
diff mbox series

Patch

diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 61311483a5c6..52a60eb0791b 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -203,13 +203,10 @@  static struct jc42_chips jc42_chips[] = {
 /* Each client has this additional data */
 struct jc42_data {
 	struct regmap	*regmap;
-	struct mutex	update_lock;	/* protect register access */
 	bool		extended;	/* true if extended range supported */
 	bool		valid;
-	unsigned long	last_updated;	/* In jiffies */
 	u16		orig_config;	/* original configuration */
 	u16		config;		/* current configuration */
-	u16		temp;		/* Cached temperature register value */
 };
 
 #define JC42_TEMP_MIN_EXTENDED	(-40000)
@@ -234,41 +231,20 @@  static int jc42_temp_from_reg(s16 reg)
 	return reg * 125 / 2;
 }
 
-static struct jc42_data *jc42_update_device(struct device *dev)
-{
-	struct jc42_data *data = dev_get_drvdata(dev);
-	unsigned int val;
-	int ret;
-
-	mutex_lock(&data->update_lock);
-
-	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		ret = regmap_read(data->regmap, JC42_REG_TEMP, &val);
-		if (ret)
-			goto abort;
-
-		data->temp = val;
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-abort:
-	mutex_unlock(&data->update_lock);
-	return ret ? ERR_PTR(ret) : data;
-}
-
 static int jc42_read(struct device *dev, enum hwmon_sensor_types type,
 		     u32 attr, int channel, long *val)
 {
-	struct jc42_data *data = jc42_update_device(dev);
+	struct jc42_data *data = dev_get_drvdata(dev);
 	unsigned int regval;
 	int ret, temp, hyst;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
 	switch (attr) {
 	case hwmon_temp_input:
-		*val = jc42_temp_from_reg(data->temp);
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			return ret;
+
+		*val = jc42_temp_from_reg(regval);
 		return 0;
 	case hwmon_temp_min:
 		ret = regmap_read(data->regmap, JC42_REG_TEMP_LOWER, &regval);
@@ -314,13 +290,25 @@  static int jc42_read(struct device *dev, enum hwmon_sensor_types type,
 		*val = temp - hyst;
 		return 0;
 	case hwmon_temp_min_alarm:
-		*val = (data->temp >> JC42_ALARM_MIN_BIT) & 1;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			return ret;
+
+		*val = (regval >> JC42_ALARM_MIN_BIT) & 1;
 		return 0;
 	case hwmon_temp_max_alarm:
-		*val = (data->temp >> JC42_ALARM_MAX_BIT) & 1;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			return ret;
+
+		*val = (regval >> JC42_ALARM_MAX_BIT) & 1;
 		return 0;
 	case hwmon_temp_crit_alarm:
-		*val = (data->temp >> JC42_ALARM_CRIT_BIT) & 1;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			return ret;
+
+		*val = (regval >> JC42_ALARM_CRIT_BIT) & 1;
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -335,8 +323,6 @@  static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 	int diff, hyst;
 	int ret;
 
-	mutex_lock(&data->update_lock);
-
 	switch (attr) {
 	case hwmon_temp_min:
 		ret = regmap_write(data->regmap, JC42_REG_TEMP_LOWER,
@@ -383,8 +369,6 @@  static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 		break;
 	}
 
-	mutex_unlock(&data->update_lock);
-
 	return ret;
 }
 
@@ -521,7 +505,6 @@  static int jc42_probe(struct i2c_client *client)
 		return PTR_ERR(data->regmap);
 
 	i2c_set_clientdata(client, data);
-	mutex_init(&data->update_lock);
 
 	ret = regmap_read(data->regmap, JC42_REG_CAP, &cap);
 	if (ret)