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

[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