* [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion state
@ 2010-11-07 15:46 Jean Delvare
2010-11-07 16:53 ` [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion Guenter Roeck
2010-11-07 20:55 ` Jean Delvare
0 siblings, 2 replies; 3+ messages in thread
From: Jean Delvare @ 2010-11-07 15:46 UTC (permalink / raw)
To: lm-sensors
We can't read the intrusion state from the real-time alarm registers
as we do for all other alarm flags, because real-time alarm bits don't
stick (by definition) and the intrusion state has to stick until
explicitly cleared (otherwise it has little value.)
So we have to use the interrupt status register instead, which is read
from the same address but with a configuration bit flipped in another
register.
Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
w83795.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
--- a/drivers/hwmon/w83795.c 2010-11-05 22:12:32.000000000 +0100
+++ b/drivers/hwmon/w83795.c 2010-11-06 22:37:55.000000000 +0100
@@ -165,10 +165,11 @@ static const u8 IN_LSB_SHIFT_IDX[][2]
#define W83795_REG_VID_CTRL 0x6A
+#define W83795_REG_ALARM_CTRL 0x40
+#define ALARM_CTRL_RTSACS (1 << 7)
#define W83795_REG_ALARM(index) (0x41 + (index))
-#define W83795_REG_BEEP(index) (0x50 + (index))
-
#define W83795_REG_CLR_CHASSIS 0x4D
+#define W83795_REG_BEEP(index) (0x50 + (index))
#define W83795_REG_FCMS1 0x201
@@ -585,6 +586,7 @@ static struct w83795_data *w83795_update
struct i2c_client *client = to_i2c_client(dev);
struct w83795_data *data = i2c_get_clientdata(client);
u16 tmp;
+ u8 intrusion;
int i;
mutex_lock(&data->update_lock);
@@ -656,9 +658,24 @@ static struct w83795_data *w83795_update
w83795_read(client, W83795_REG_PWM(i, PWM_OUTPUT));
}
- /* update alarm */
+ /* Update intrusion and alarms
+ * It is important to read intrusion first, because reading from
+ * register SMI STS6 clears the interrupt status temporarily. */
+ tmp = w83795_read(client, W83795_REG_ALARM_CTRL);
+ /* Switch to interrupt status for intrusion if needed */
+ if (tmp & ALARM_CTRL_RTSACS)
+ w83795_write(client, W83795_REG_ALARM_CTRL,
+ tmp & ~ALARM_CTRL_RTSACS);
+ intrusion = w83795_read(client, W83795_REG_ALARM(5)) & (1 << 6);
+ /* Switch to real-time alarms */
+ w83795_write(client, W83795_REG_ALARM_CTRL, tmp | ALARM_CTRL_RTSACS);
for (i = 0; i < ARRAY_SIZE(data->alarms); i++)
data->alarms[i] = w83795_read(client, W83795_REG_ALARM(i));
+ data->alarms[5] |= intrusion;
+ /* Restore original configuration if needed */
+ if (!(tmp & ALARM_CTRL_RTSACS))
+ w83795_write(client, W83795_REG_ALARM_CTRL,
+ tmp & ~ALARM_CTRL_RTSACS);
data->last_updated = jiffies;
data->valid = 1;
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion
2010-11-07 15:46 [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion state Jean Delvare
@ 2010-11-07 16:53 ` Guenter Roeck
2010-11-07 20:55 ` Jean Delvare
1 sibling, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2010-11-07 16:53 UTC (permalink / raw)
To: lm-sensors
On Sun, Nov 07, 2010 at 10:46:37AM -0500, Jean Delvare wrote:
> We can't read the intrusion state from the real-time alarm registers
> as we do for all other alarm flags, because real-time alarm bits don't
> stick (by definition) and the intrusion state has to stick until
> explicitly cleared (otherwise it has little value.)
>
> So we have to use the interrupt status register instead, which is read
> from the same address but with a configuration bit flipped in another
> register.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
> w83795.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> --- a/drivers/hwmon/w83795.c 2010-11-05 22:12:32.000000000 +0100
> +++ b/drivers/hwmon/w83795.c 2010-11-06 22:37:55.000000000 +0100
> @@ -165,10 +165,11 @@ static const u8 IN_LSB_SHIFT_IDX[][2] >
> #define W83795_REG_VID_CTRL 0x6A
>
> +#define W83795_REG_ALARM_CTRL 0x40
> +#define ALARM_CTRL_RTSACS (1 << 7)
> #define W83795_REG_ALARM(index) (0x41 + (index))
> -#define W83795_REG_BEEP(index) (0x50 + (index))
> -
> #define W83795_REG_CLR_CHASSIS 0x4D
> +#define W83795_REG_BEEP(index) (0x50 + (index))
>
>
> #define W83795_REG_FCMS1 0x201
> @@ -585,6 +586,7 @@ static struct w83795_data *w83795_update
> struct i2c_client *client = to_i2c_client(dev);
> struct w83795_data *data = i2c_get_clientdata(client);
> u16 tmp;
> + u8 intrusion;
> int i;
>
> mutex_lock(&data->update_lock);
> @@ -656,9 +658,24 @@ static struct w83795_data *w83795_update
> w83795_read(client, W83795_REG_PWM(i, PWM_OUTPUT));
> }
>
> - /* update alarm */
> + /* Update intrusion and alarms
> + * It is important to read intrusion first, because reading from
> + * register SMI STS6 clears the interrupt status temporarily. */
> + tmp = w83795_read(client, W83795_REG_ALARM_CTRL);
> + /* Switch to interrupt status for intrusion if needed */
> + if (tmp & ALARM_CTRL_RTSACS)
> + w83795_write(client, W83795_REG_ALARM_CTRL,
> + tmp & ~ALARM_CTRL_RTSACS);
> + intrusion = w83795_read(client, W83795_REG_ALARM(5)) & (1 << 6);
> + /* Switch to real-time alarms */
> + w83795_write(client, W83795_REG_ALARM_CTRL, tmp | ALARM_CTRL_RTSACS);
> for (i = 0; i < ARRAY_SIZE(data->alarms); i++)
> data->alarms[i] = w83795_read(client, W83795_REG_ALARM(i));
> + data->alarms[5] |= intrusion;
> + /* Restore original configuration if needed */
> + if (!(tmp & ALARM_CTRL_RTSACS))
> + w83795_write(client, W83795_REG_ALARM_CTRL,
> + tmp & ~ALARM_CTRL_RTSACS);
>
Does this make a difference, ie is it needed ?
If not, might be simpler to just switch back and forth w/o checking
if ALARM_CTRL_RTSACS is set.
Anyway,
Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion
2010-11-07 15:46 [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion state Jean Delvare
2010-11-07 16:53 ` [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion Guenter Roeck
@ 2010-11-07 20:55 ` Jean Delvare
1 sibling, 0 replies; 3+ messages in thread
From: Jean Delvare @ 2010-11-07 20:55 UTC (permalink / raw)
To: lm-sensors
On Sun, 7 Nov 2010 08:53:28 -0800, Guenter Roeck wrote:
> On Sun, Nov 07, 2010 at 10:46:37AM -0500, Jean Delvare wrote:
> > We can't read the intrusion state from the real-time alarm registers
> > as we do for all other alarm flags, because real-time alarm bits don't
> > stick (by definition) and the intrusion state has to stick until
> > explicitly cleared (otherwise it has little value.)
> >
> > So we have to use the interrupt status register instead, which is read
> > from the same address but with a configuration bit flipped in another
> > register.
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > ---
> > w83795.c | 23 ++++++++++++++++++++---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > --- a/drivers/hwmon/w83795.c 2010-11-05 22:12:32.000000000 +0100
> > +++ b/drivers/hwmon/w83795.c 2010-11-06 22:37:55.000000000 +0100
> > @@ -165,10 +165,11 @@ static const u8 IN_LSB_SHIFT_IDX[][2] > >
> > #define W83795_REG_VID_CTRL 0x6A
> >
> > +#define W83795_REG_ALARM_CTRL 0x40
> > +#define ALARM_CTRL_RTSACS (1 << 7)
> > #define W83795_REG_ALARM(index) (0x41 + (index))
> > -#define W83795_REG_BEEP(index) (0x50 + (index))
> > -
> > #define W83795_REG_CLR_CHASSIS 0x4D
> > +#define W83795_REG_BEEP(index) (0x50 + (index))
> >
> >
> > #define W83795_REG_FCMS1 0x201
> > @@ -585,6 +586,7 @@ static struct w83795_data *w83795_update
> > struct i2c_client *client = to_i2c_client(dev);
> > struct w83795_data *data = i2c_get_clientdata(client);
> > u16 tmp;
> > + u8 intrusion;
> > int i;
> >
> > mutex_lock(&data->update_lock);
> > @@ -656,9 +658,24 @@ static struct w83795_data *w83795_update
> > w83795_read(client, W83795_REG_PWM(i, PWM_OUTPUT));
> > }
> >
> > - /* update alarm */
> > + /* Update intrusion and alarms
> > + * It is important to read intrusion first, because reading from
> > + * register SMI STS6 clears the interrupt status temporarily. */
> > + tmp = w83795_read(client, W83795_REG_ALARM_CTRL);
> > + /* Switch to interrupt status for intrusion if needed */
> > + if (tmp & ALARM_CTRL_RTSACS)
> > + w83795_write(client, W83795_REG_ALARM_CTRL,
> > + tmp & ~ALARM_CTRL_RTSACS);
> > + intrusion = w83795_read(client, W83795_REG_ALARM(5)) & (1 << 6);
> > + /* Switch to real-time alarms */
> > + w83795_write(client, W83795_REG_ALARM_CTRL, tmp | ALARM_CTRL_RTSACS);
> > for (i = 0; i < ARRAY_SIZE(data->alarms); i++)
>
> > data->alarms[i] = w83795_read(client, W83795_REG_ALARM(i));
> > + data->alarms[5] |= intrusion;
> > + /* Restore original configuration if needed */
> > + if (!(tmp & ALARM_CTRL_RTSACS))
> > + w83795_write(client, W83795_REG_ALARM_CTRL,
> > + tmp & ~ALARM_CTRL_RTSACS);
> >
> Does this make a difference, ie is it needed ?
> If not, might be simpler to just switch back and forth w/o checking
> if ALARM_CTRL_RTSACS is set.
Experience taught me that it's always a good practice to leave
configuration register in the same state we found them in. It would
matter if the BIOS has SMM code which processes SMIs and wants to check
these registers (although if this is the case, we are in trouble
anyway, as there's no synchronization of SMBus access.) It can also
matter on warm reboot if the BIOS makes some assumptions on
configuration settings. We've seen this in the past. Of course we could
restore the configuration only at driver unbind/shutdown time, but this
would require no less code than we have with my approach. With my code,
you also have exactly two register writes to W83795_REG_ALARM_CTRL with
each update (the 1st and 3rd are mutually exclusive) so it is only
marginally slower than your proposal (two comparisons).
So I've pondered all this and came to the conclusion that I'd play it
safe because it doesn't cost much.
>
> Anyway,
> Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-11-07 20:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-07 15:46 [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion state Jean Delvare
2010-11-07 16:53 ` [lm-sensors] [PATCH 5/7] hwmon: (w83795) Read the intrusion Guenter Roeck
2010-11-07 20:55 ` Jean Delvare
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.