Re: [PATCH 2/3] hwmon: Add support for SPD5118 compliant temperature sensors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 30.05.24 um 19:33 schrieb Guenter Roeck:

On 5/30/24 10:03, Armin Wolf wrote:
Am 29.05.24 um 22:52 schrieb Guenter Roeck:

[ ... ]

+
+temp1_lcrit_alarm    Temperature low critical alarm
+temp1_min_alarm        Temperature low alarm
+temp1_max_alarm        Temperature high alarm
+temp1_crit_alarm    Temperature critical alarm

Maybe it would be a good idea to tell users that the alarm attributes
are sticky.


The driver auto-clears them after read, so they are only sticky in the
sense
that they will remain active until read. This is quite common for
hardware
monitoring devices. However, sure, I'll add a note.

[ ... ]

+static int spd5118_write_enable(struct regmap *regmap, u32 attr,
long val)
+{
+    if (val && val != 1)
+        return -EINVAL;
+
+    return regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG,
+                  SPD5118_TS_DISABLE,
+                  val ? 0 : SPD5118_TS_DISABLE);

The spd5118 spec says that we have to wait 10ms after enabling the
sensors before
we start reading temperature values, maybe we need a delay + locking
here?


I don't think that would add much if any value but a lot of complexity
for little gain. I find it acceptable that the sensor returns 0 for a
few ms
after enabling it. Pretty much all chips have the same problem, so I am
really not concerned about it.

+
+static struct i2c_driver spd5118_driver = {
+    .class        = I2C_CLASS_HWMON,
+    .driver = {
+        .name    = "spd5118",
+        .of_match_table = spd5118_of_ids,

The driver is missing suspend support, without it hibernation/S4
sleep will cause the
limit and config registers to be out of sync with the regmap cache.


Good point. Do you have a means to test this if I add suspend support ?
I have not been able to figure out how to put my system into suspend.

Thanks,
Guenter

I think so, at least i can verify if S3 sleep works, but S4 sleep should work fine too.

Thanks,
Armin Wolf






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux