[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Charles Spirakis]

From: Charles Spirakis <bezaur at gmail.com>

Modified patch from Sven Anders to enhance w83791d support. Partial
support was already implemented, but it was missing IN9/FAN4/FAN5 output.

Signed-off by: Sven Anders <anders at anduras.de>
Signed-off by: Charles Spirakis <bezaur at gmail.com>

---

 kernel/chips/w83781d.c |   16 ++++++++++-----
 prog/sensors/chips.c   |   50
+++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 59
insertions(+), 7 deletions(-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lm-sensors-r4079-20060730_sven_changes.patch
Type: text/x-patch
Size: 6216 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060803/920c40e7/attachment-0001.bin 

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Jean Delvare]

Hi Charles, Sven,

Sorry for the late answer.

> Modified patch from Sven Anders to enhance w83791d support. Partial
> support was already implemented, but it was missing IN9/FAN4/FAN5 output.

>  /* PWM 782D (1-4) and 783S (1-2) only */
> @@ -385,16 +385,22 @@ static struct i2c_driver w83781d_driver 
>  #define W83781D_ALARM_IN4 0x0100
>  #define W83781D_ALARM_IN5 0x0200
>  #define W83781D_ALARM_IN6 0x0400
> -#define W83782D_ALARM_IN7 0x10000
> -#define W83782D_ALARM_IN8 0x20000
> +#define W83782D_ALARM_IN7 0x10000	/* 782D/791D */
> +#define W83782D_ALARM_IN8 0x20000	/* 782D/791D */
> +#define W83791D_ALARM_IN9 0x40000	/* 791D only */

This does NOT match the W83791D datasheet I have here. These bits are
OK for the 782D, but the 791D seems to use a different mapping:

5VSB = in7 = register 0xAB bit 3 -> 0x80000
VBAT = in8 = register 0xAB bit 4 -> 0x100000
VINR1 = in9 = register 0xAA bit 6 -> 0x4000

>  #define W83781D_ALARM_FAN1 0x0040
>  #define W83781D_ALARM_FAN2 0x0080
>  #define W83781D_ALARM_FAN3 0x0800
> +#define W83791D_ALARM_FAN4 0x200000	/* 791D only */
> +#define W83791D_ALARM_FAN5 0x400000	/* 791D only */

I'm OK with these ones.

>  #define W83781D_ALARM_TEMP1 0x0010
>  #define W83781D_ALARM_TEMP23 0x0020	/* 781D only */
> -#define W83781D_ALARM_TEMP2 0x0020	/* 782D/783S */
> -#define W83781D_ALARM_TEMP3 0x2000	/* 782D only */
> +#define W83781D_ALARM_TEMP2 0x0020	/* 782D/783S/791D */
> +#define W83781D_ALARM_TEMP3 0x2000	/* 782D/791D */

I'm OK here as well.

>  #define W83781D_ALARM_CHAS 0x1000
> +#define W83791D_ALARM_VINR1 0x04000	/* 782D/791D */
> +#define W83791D_ALARM_VSB 0x80000	/* 791D only */
> +#define W83791D_ALARM_VBAT 0x100000	/* 791D only */

And here you have the same values I have above. These are the ones we
want to use! But I'd rather name them W83791D_ALARM_IN{7,8,9} to keep
some consistency in the naming.

> diff -urpN lm-sensors-r4079-20060730/prog/sensors/chips.c lm-sensors-r4079-20060730_sven_changes/prog/sensors/chips.c
> --- lm-sensors-r4079-20060730/prog/sensors/chips.c	2006-07-29 21:10:45.000000000 -0700
> +++ lm-sensors-r4079-20060730_sven_changes/prog/sensors/chips.c	2006-07-30 22:26:05.000000000 -0700
> @@ -2187,7 +2187,7 @@ void print_w83781d(const sensors_chip_na
>    char *label;
>    double cur,min,max,fdiv,sens;
>    int alarms,beeps;
> -  int is81d, is82d, is83s, is697hf, is627thf, valid;
> +  int is81d, is82d, is83s, is91d, is697hf, is627thf, valid;
>  
>    is81d = !strcmp(name->prefix,"w83781d");
>    is82d = (!strcmp(name->prefix,"w83782d")) ||
> @@ -2196,6 +2196,7 @@ void print_w83781d(const sensors_chip_na
>            (!strcmp(name->prefix, "w83627thf")) ||
>            (!strcmp(name->prefix, "w83687thf"));
>    is83s = !strcmp(name->prefix,"w83783s");
> +  is91d = !strcmp(name->prefix,"w83791d");
>    is627thf = (!strcmp(name->prefix,"w83627thf")) ||
>               (!strcmp(name->prefix, "w83637hf")) ||
>               (!strcmp(name->prefix, "w83687thf"));
> @@ -2310,7 +2311,7 @@ void print_w83781d(const sensors_chip_na
>        printf("ERROR: Can't get IN6 data!\n");
>      free(label);
>    } /* !is627thf */
> -  if (is82d || is697hf || is627thf) {
> +  if (is82d || is697hf || is627thf || is91d) {
>      if (!sensors_get_label_and_valid(*name,SENSORS_W83782D_IN7,&label,&valid) &&
>          !sensors_get_feature(*name,SENSORS_W83782D_IN7,&cur) &&
>          !sensors_get_feature(*name,SENSORS_W83782D_IN7_MIN,&min) &&
> @@ -2339,6 +2340,22 @@ void print_w83781d(const sensors_chip_na
>      free(label);
>    }
>  
> +  if (is91d) {
> +    if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_IN9,&label,&valid) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_IN9,&cur) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_IN9_MIN,&min) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_IN9_MAX,&max)) {
> +      if (valid) {
> +        print_label(label,10);
> +        printf("%+6.2f V  (min = %+6.2f V, max = %+6.2f V)       %s  %s\n",
> +             cur,min,max,alarms&W83791D_ALARM_IN9?"ALARM":"     ",
> +             beeps&W83791D_ALARM_IN9?"(beep)":"");

OK if and only if W83791D_ALARM_IN9 is set to the right value.

> +      }
> +    } else
> +      printf("ERROR: Can't get IN9 data!\n");
> +    free(label);
> +  }
> +
>    if (!sensors_get_label_and_valid(*name,SENSORS_W83781D_FAN1,&label,&valid) &&
>        !sensors_get_feature(*name,SENSORS_W83781D_FAN1,&cur) &&
>        !sensors_get_feature(*name,SENSORS_W83781D_FAN1_DIV,&fdiv) &&
> @@ -2382,6 +2399,35 @@ void print_w83781d(const sensors_chip_na
>      free(label);
>    }
>  
> +  if(is91d) {
> +    if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_FAN4,&label,&valid) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_FAN4,&cur) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_FAN4_DIV,&fdiv) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_FAN4_MIN,&min)) {
> +      if (valid) {
> +        print_label(label,10);
> +        printf("%4.0f RPM  (min = %4.0f RPM, div = %1.0f)              %s  %s\n",
> +             cur,min,fdiv, alarms&W83791D_ALARM_FAN4?"ALARM":"     ",
> +             beeps&W83791D_ALARM_FAN4?"(beep)":"");
> +      }
> +    } else
> +      printf("ERROR: Can't get FAN4 data!\n");
> +    free(label);
> +    if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_FAN5,&label,&valid) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_FAN5,&cur) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_FAN5_DIV,&fdiv) &&
> +        !sensors_get_feature(*name,SENSORS_W83791D_FAN5_MIN,&min)) {
> +      if (valid) {
> +        print_label(label,10);
> +        printf("%4.0f RPM  (min = %4.0f RPM, div = %1.0f)              %s  %s\n",
> +             cur,min,fdiv, alarms&W83791D_ALARM_FAN5?"ALARM":"     ",
> +             beeps&W83791D_ALARM_FAN5?"(beep)":"");
> +      }
> +    } else
> +      printf("ERROR: Can't get FAN5 data!\n");
> +    free(label);
> +  }
> +
>    if (!sensors_get_label_and_valid(*name,SENSORS_W83781D_TEMP1,&label,&valid) &&
>        !sensors_get_feature(*name,SENSORS_W83781D_TEMP1,&cur) &&
>        !sensors_get_feature(*name,SENSORS_W83781D_TEMP1_HYST,&min) &&

This is OK but not sufficient as far as I can see. Because of the
difference in alarm bits for in7 and in8, you also need to add specific
code for the w83791d in the in7 and in8 cases.

I'd like to have these improvements merged soon now, can you please
provide a new patch?

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Charles Spirakis]

Jean --

I will try to get an update to you in the next few days. However,
there is some ambiguity in the documentation which is causing some of
the confusion...

If you look at the 791d beep enable register (0xa3) and compare those
bits to the alarm status register (0xab) you will see a discrepancy in
the bit ordering.

Sven and I tried to figure out if the documentation was correct or if
one of the bit orderings provided were correct and it was hard to
tell. It seemed like the bits for the enable did correspond to the
bits for the alarm and we had reason to believe the enable bits were
correct (which puts in7 and in8 at 0x10000 and 0x20000 respectively -
the same as the w83782d). In a side note, if you look at the interrupt
mask bits (0x9d) they seem to follow the alarm status bit pattern and
not the enable bit pattern but since the driver isn't using that
register it's not overly significant other than interesting
information.

I will go back and test this again just to verify what we think we saw
is what we actually saw (or heard as the case may be).

If anyone from winbond is reading this, would it be possible to get a
definitive answer as to whether the documentation is right and (if
not), to let us know what the correct bit patterns are for the alarm
vs. the enable?

Along these same lines, the bit position for bit 1 and bit 13 are also
swapped between what is shown for the enable vs. alarm. In that case,
the documentation seems to be correct and the patch for the 2.6 driver
is updated to reflect this.

Which leads to a different question. The assumption that was made for
the driver patch was that we wanted the alarm and enable bits to match
in user-space (and the driver would hide the problem if the chip
didn't do this). Is this the correct assumption? Or should userspace
get the values directly from the chip (so it matches the chip
documentation) and let the user-space code deal with the bit
twiddling?

Oh, and I just noticed the inconsistency in the 2.6 w83791d driver
patch in the Documentation file - needs to be:

14 - in9 (VINR1)

Depending on your answer to the previous question (how do we handle
the situation where the enable bit ordering is different from the
alarm bit ordering), I will update or remove the patch... And yes, I
understand this is yet another argument for updating this code to the
standardized sysfs beep/alarm model. :)

-- charles


On 8/12/06, Jean Delvare <khali at linux-fr.org> wrote:
> Hi Charles, Sven,
>
> Sorry for the late answer.
>
> > Modified patch from Sven Anders to enhance w83791d support. Partial
> > support was already implemented, but it was missing IN9/FAN4/FAN5 output.
>
> >  /* PWM 782D (1-4) and 783S (1-2) only */
> > @@ -385,16 +385,22 @@ static struct i2c_driver w83781d_driver
> >  #define W83781D_ALARM_IN4 0x0100
> >  #define W83781D_ALARM_IN5 0x0200
> >  #define W83781D_ALARM_IN6 0x0400
> > -#define W83782D_ALARM_IN7 0x10000
> > -#define W83782D_ALARM_IN8 0x20000
> > +#define W83782D_ALARM_IN7 0x10000    /* 782D/791D */
> > +#define W83782D_ALARM_IN8 0x20000    /* 782D/791D */
> > +#define W83791D_ALARM_IN9 0x40000    /* 791D only */
>
> This does NOT match the W83791D datasheet I have here. These bits are
> OK for the 782D, but the 791D seems to use a different mapping:
>
> 5VSB = in7 = register 0xAB bit 3 -> 0x80000
> VBAT = in8 = register 0xAB bit 4 -> 0x100000
> VINR1 = in9 = register 0xAA bit 6 -> 0x4000
>
> >  #define W83781D_ALARM_FAN1 0x0040
> >  #define W83781D_ALARM_FAN2 0x0080
> >  #define W83781D_ALARM_FAN3 0x0800
> > +#define W83791D_ALARM_FAN4 0x200000  /* 791D only */
> > +#define W83791D_ALARM_FAN5 0x400000  /* 791D only */
>
> I'm OK with these ones.
>
> >  #define W83781D_ALARM_TEMP1 0x0010
> >  #define W83781D_ALARM_TEMP23 0x0020  /* 781D only */
> > -#define W83781D_ALARM_TEMP2 0x0020   /* 782D/783S */
> > -#define W83781D_ALARM_TEMP3 0x2000   /* 782D only */
> > +#define W83781D_ALARM_TEMP2 0x0020   /* 782D/783S/791D */
> > +#define W83781D_ALARM_TEMP3 0x2000   /* 782D/791D */
>
> I'm OK here as well.
>
> >  #define W83781D_ALARM_CHAS 0x1000
> > +#define W83791D_ALARM_VINR1 0x04000  /* 782D/791D */
> > +#define W83791D_ALARM_VSB 0x80000    /* 791D only */
> > +#define W83791D_ALARM_VBAT 0x100000  /* 791D only */
>
> And here you have the same values I have above. These are the ones we
> want to use! But I'd rather name them W83791D_ALARM_IN{7,8,9} to keep
> some consistency in the naming.
>
> > diff -urpN lm-sensors-r4079-20060730/prog/sensors/chips.c lm-sensors-r4079-20060730_sven_changes/prog/sensors/chips.c
> > --- lm-sensors-r4079-20060730/prog/sensors/chips.c    2006-07-29 21:10:45.000000000 -0700
> > +++ lm-sensors-r4079-20060730_sven_changes/prog/sensors/chips.c       2006-07-30 22:26:05.000000000 -0700
> > @@ -2187,7 +2187,7 @@ void print_w83781d(const sensors_chip_na
> >    char *label;
> >    double cur,min,max,fdiv,sens;
> >    int alarms,beeps;
> > -  int is81d, is82d, is83s, is697hf, is627thf, valid;
> > +  int is81d, is82d, is83s, is91d, is697hf, is627thf, valid;
> >
> >    is81d = !strcmp(name->prefix,"w83781d");
> >    is82d = (!strcmp(name->prefix,"w83782d")) ||
> > @@ -2196,6 +2196,7 @@ void print_w83781d(const sensors_chip_na
> >            (!strcmp(name->prefix, "w83627thf")) ||
> >            (!strcmp(name->prefix, "w83687thf"));
> >    is83s = !strcmp(name->prefix,"w83783s");
> > +  is91d = !strcmp(name->prefix,"w83791d");
> >    is627thf = (!strcmp(name->prefix,"w83627thf")) ||
> >               (!strcmp(name->prefix, "w83637hf")) ||
> >               (!strcmp(name->prefix, "w83687thf"));
> > @@ -2310,7 +2311,7 @@ void print_w83781d(const sensors_chip_na
> >        printf("ERROR: Can't get IN6 data!\n");
> >      free(label);
> >    } /* !is627thf */
> > -  if (is82d || is697hf || is627thf) {
> > +  if (is82d || is697hf || is627thf || is91d) {
> >      if (!sensors_get_label_and_valid(*name,SENSORS_W83782D_IN7,&label,&valid) &&
> >          !sensors_get_feature(*name,SENSORS_W83782D_IN7,&cur) &&
> >          !sensors_get_feature(*name,SENSORS_W83782D_IN7_MIN,&min) &&
> > @@ -2339,6 +2340,22 @@ void print_w83781d(const sensors_chip_na
> >      free(label);
> >    }
> >
> > +  if (is91d) {
> > +    if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_IN9,&label,&valid) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_IN9,&cur) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_IN9_MIN,&min) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_IN9_MAX,&max)) {
> > +      if (valid) {
> > +        print_label(label,10);
> > +        printf("%+6.2f V  (min = %+6.2f V, max = %+6.2f V)       %s  %s\n",
> > +             cur,min,max,alarms&W83791D_ALARM_IN9?"ALARM":"     ",
> > +             beeps&W83791D_ALARM_IN9?"(beep)":"");
>
> OK if and only if W83791D_ALARM_IN9 is set to the right value.
>
> > +      }
> > +    } else
> > +      printf("ERROR: Can't get IN9 data!\n");
> > +    free(label);
> > +  }
> > +
> >    if (!sensors_get_label_and_valid(*name,SENSORS_W83781D_FAN1,&label,&valid) &&
> >        !sensors_get_feature(*name,SENSORS_W83781D_FAN1,&cur) &&
> >        !sensors_get_feature(*name,SENSORS_W83781D_FAN1_DIV,&fdiv) &&
> > @@ -2382,6 +2399,35 @@ void print_w83781d(const sensors_chip_na
> >      free(label);
> >    }
> >
> > +  if(is91d) {
> > +    if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_FAN4,&label,&valid) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_FAN4,&cur) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_FAN4_DIV,&fdiv) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_FAN4_MIN,&min)) {
> > +      if (valid) {
> > +        print_label(label,10);
> > +        printf("%4.0f RPM  (min = %4.0f RPM, div = %1.0f)              %s  %s\n",
> > +             cur,min,fdiv, alarms&W83791D_ALARM_FAN4?"ALARM":"     ",
> > +             beeps&W83791D_ALARM_FAN4?"(beep)":"");
> > +      }
> > +    } else
> > +      printf("ERROR: Can't get FAN4 data!\n");
> > +    free(label);
> > +    if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_FAN5,&label,&valid) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_FAN5,&cur) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_FAN5_DIV,&fdiv) &&
> > +        !sensors_get_feature(*name,SENSORS_W83791D_FAN5_MIN,&min)) {
> > +      if (valid) {
> > +        print_label(label,10);
> > +        printf("%4.0f RPM  (min = %4.0f RPM, div = %1.0f)              %s  %s\n",
> > +             cur,min,fdiv, alarms&W83791D_ALARM_FAN5?"ALARM":"     ",
> > +             beeps&W83791D_ALARM_FAN5?"(beep)":"");
> > +      }
> > +    } else
> > +      printf("ERROR: Can't get FAN5 data!\n");
> > +    free(label);
> > +  }
> > +
> >    if (!sensors_get_label_and_valid(*name,SENSORS_W83781D_TEMP1,&label,&valid) &&
> >        !sensors_get_feature(*name,SENSORS_W83781D_TEMP1,&cur) &&
> >        !sensors_get_feature(*name,SENSORS_W83781D_TEMP1_HYST,&min) &&
>
> This is OK but not sufficient as far as I can see. Because of the
> difference in alarm bits for in7 and in8, you also need to add specific
> code for the w83791d in the in7 and in8 cases.
>
> I'd like to have these improvements merged soon now, can you please
> provide a new patch?
>
> Thanks,
> --
> Jean Delvare
>

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Jean Delvare]

Charles,

> I will try to get an update to you in the next few days. However,
> there is some ambiguity in the documentation which is causing some of
> the confusion...
> 
> If you look at the 791d beep enable register (0xa3) and compare those
> bits to the alarm status register (0xab) you will see a discrepancy in
> the bit ordering.
> 
> Sven and I tried to figure out if the documentation was correct or if
> one of the bit orderings provided were correct and it was hard to
> tell. It seemed like the bits for the enable did correspond to the
> bits for the alarm and we had reason to believe the enable bits were
> correct (which puts in7 and in8 at 0x10000 and 0x20000 respectively -
> the same as the w83782d). In a side note, if you look at the interrupt
> mask bits (0x9d) they seem to follow the alarm status bit pattern and
> not the enable bit pattern but since the driver isn't using that
> register it's not overly significant other than interesting
> information.
> 
> I will go back and test this again just to verify what we think we saw
> is what we actually saw (or heard as the case may be).

Yeah I agree that there's some confusion in the datasheet. I don't have
a chip to test myself. All I want is that the bits we define in the 2.4
driver and use in sensors/chips.c match the hardware reality. Else they
are quite useless.

> If anyone from winbond is reading this, would it be possible to get a
> definitive answer as to whether the documentation is right and (if
> not), to let us know what the correct bit patterns are for the alarm
> vs. the enable?

We have people from Winbond reading the list and helping where they
can, but they certainly don't have the time to read everything. Please
prepare precise questions, then we can ask them. Keep in mind though
that the W83791D is a relatively old chip, and not very successful from
a commercial perspective as far as can see, so Winbond must have it at
low priority on their lists.

> Along these same lines, the bit position for bit 1 and bit 13 are also
> swapped between what is shown for the enable vs. alarm. In that case,
> the documentation seems to be correct and the patch for the 2.6 driver
> is updated to reflect this.
>
> Which leads to a different question. The assumption that was made for
> the driver patch was that we wanted the alarm and enable bits to match
> in user-space (and the driver would hide the problem if the chip
> didn't do this). Is this the correct assumption? Or should userspace
> get the values directly from the chip (so it matches the chip
> documentation) and let the user-space code deal with the bit
> twiddling?

That's probably the first time we encounter this problem so we have to
decide now. The rule for alarms and beeps in 2.4 was indeed to export
them in the most simple way for the driver (only masking out irrelevant
bits.) And so far it happened that beep registers always matched alarm
registers so it just happened that a single set of bit definitions
would fit them both. If this ain't true anymore, we have to break one
of the rules, i.e. either introduce new bit definitions for beeps, or
cook the beep register values in the driver.

The most important thing to keep in mind here is that in the future, we
want individual alarm and beep files anyway, so the decision we make
here only matters for the 2.4 kernel drivers in the long run. In other
words, it doesn't really matter. I'd go for the most simple solution,
i.e. keep exporting the raw register values for both alarms and beeps,
and introduce new defines for beeps where needed.

> Oh, and I just noticed the inconsistency in the 2.6 w83791d driver
> patch in the Documentation file - needs to be:
> 
> 14 - in9 (VINR1)

You're right.

> Depending on your answer to the previous question (how do we handle
> the situation where the enable bit ordering is different from the
> alarm bit ordering), I will update or remove the patch... And yes, I
> understand this is yet another argument for updating this code to the
> standardized sysfs beep/alarm model. :)

Definitely. The w83791d currently only creates the old style alarm
files, I would welcome a patch adding individual alarm and beep files.

-- 
Jean Delvare

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Charles Spirakis]

I wrote an ugly little python script to cause each alarm to trigger
one at a time (I can provide it if anyone is interested). Based on the
script, what I found was that the documentation was completely correct
for the values I tested (in0-9, temp1-3, fan1-5). My board doesn't
have the pwm stuff and the driver doesn't have that support, so I
didn't validate the tart1-3 alarms/enables, but since everything else
was correct, it seems likely those are as well.

Sven - Can you confirm this result?

Based on this result and the comment earlier that the driver should do
a pass through of the values, I will create a documentation only
change for the 2.6 driver and clean up the lm_sensors patch that
started this thread.

Does this sound reasonable?

-- charles

in0 (VCORE)  :  alarms: 0x000001 beep_enable: 0x000001
in1 (VINR0)  :  alarms: 0x000002 beep_enable: 0x002000 <= mismatch
in2 (+3.3VIN):  alarms: 0x000004 beep_enable: 0x000004
in3 (5VDD)   :  alarms: 0x000008 beep_enable: 0x000008
in4 (+12VIN) :  alarms: 0x000100 beep_enable: 0x000100
in5 (-12VIN) :  alarms: 0x000200 beep_enable: 0x000200
in6 (-5VIN)  :  alarms: 0x000400 beep_enable: 0x000400
in7 (VSB)    :  alarms: 0x080000 beep_enable: 0x010000 <= mismatch
in8 (VBAT)   :  alarms: 0x100000 beep_enable: 0x020000 <= mismatch
in9 (VINR1)  :  alarms: 0x004000 beep_enable: 0x004000
temp1        :  alarms: 0x000010 beep_enable: 0x000010
temp2        :  alarms: 0x000020 beep_enable: 0x000020
temp3        :  alarms: 0x002000 beep_enable: 0x000002 <= mismatch
fan1         :  alarms: 0x000040 beep_enable: 0x000040
fan2         :  alarms: 0x000080 beep_enable: 0x000080
fan3         :  alarms: 0x000800 beep_enable: 0x000800
fan4         :  alarms: 0x200000 beep_enable: 0x200000
fan5         :  alarms: 0x400000 beep_enable: 0x400000
tart1        :  alarms: 0x010000 beep_enable: 0x040000 <= mismatch
tart2        :  alarms: 0x020000 beep_enable: 0x080000 <= mismatch
tart3        :  alarms: 0x040000 beep_enable: 0x100000 <= mismatch
case open    :  alarms: 0x001000 beep_enable: 0x001000
user enable  :  alarms: -------- beep_enable: 0x800000
alarm_rsvd   :  alarms: 0x800000 beep_enable: --------
reserved     :  alarms: 0x008000 beep_enable: 0x008000


On 8/14/06, Jean Delvare <khali at linux-fr.org> wrote:
> Charles,
>
> > I will try to get an update to you in the next few days. However,
> > there is some ambiguity in the documentation which is causing some of
> > the confusion...
> >
> > If you look at the 791d beep enable register (0xa3) and compare those
> > bits to the alarm status register (0xab) you will see a discrepancy in
> > the bit ordering.
> >
> > Sven and I tried to figure out if the documentation was correct or if
> > one of the bit orderings provided were correct and it was hard to
> > tell. It seemed like the bits for the enable did correspond to the
> > bits for the alarm and we had reason to believe the enable bits were
> > correct (which puts in7 and in8 at 0x10000 and 0x20000 respectively -
> > the same as the w83782d). In a side note, if you look at the interrupt
> > mask bits (0x9d) they seem to follow the alarm status bit pattern and
> > not the enable bit pattern but since the driver isn't using that
> > register it's not overly significant other than interesting
> > information.
> >
> > I will go back and test this again just to verify what we think we saw
> > is what we actually saw (or heard as the case may be).
>
> Yeah I agree that there's some confusion in the datasheet. I don't have
> a chip to test myself. All I want is that the bits we define in the 2.4
> driver and use in sensors/chips.c match the hardware reality. Else they
> are quite useless.
>
> > If anyone from winbond is reading this, would it be possible to get a
> > definitive answer as to whether the documentation is right and (if
> > not), to let us know what the correct bit patterns are for the alarm
> > vs. the enable?
>
> We have people from Winbond reading the list and helping where they
> can, but they certainly don't have the time to read everything. Please
> prepare precise questions, then we can ask them. Keep in mind though
> that the W83791D is a relatively old chip, and not very successful from
> a commercial perspective as far as can see, so Winbond must have it at
> low priority on their lists.
>
> > Along these same lines, the bit position for bit 1 and bit 13 are also
> > swapped between what is shown for the enable vs. alarm. In that case,
> > the documentation seems to be correct and the patch for the 2.6 driver
> > is updated to reflect this.
> >
> > Which leads to a different question. The assumption that was made for
> > the driver patch was that we wanted the alarm and enable bits to match
> > in user-space (and the driver would hide the problem if the chip
> > didn't do this). Is this the correct assumption? Or should userspace
> > get the values directly from the chip (so it matches the chip
> > documentation) and let the user-space code deal with the bit
> > twiddling?
>
> That's probably the first time we encounter this problem so we have to
> decide now. The rule for alarms and beeps in 2.4 was indeed to export
> them in the most simple way for the driver (only masking out irrelevant
> bits.) And so far it happened that beep registers always matched alarm
> registers so it just happened that a single set of bit definitions
> would fit them both. If this ain't true anymore, we have to break one
> of the rules, i.e. either introduce new bit definitions for beeps, or
> cook the beep register values in the driver.
>
> The most important thing to keep in mind here is that in the future, we
> want individual alarm and beep files anyway, so the decision we make
> here only matters for the 2.4 kernel drivers in the long run. In other
> words, it doesn't really matter. I'd go for the most simple solution,
> i.e. keep exporting the raw register values for both alarms and beeps,
> and introduce new defines for beeps where needed.
>
> > Oh, and I just noticed the inconsistency in the 2.6 w83791d driver
> > patch in the Documentation file - needs to be:
> >
> > 14 - in9 (VINR1)
>
> You're right.
>
> > Depending on your answer to the previous question (how do we handle
> > the situation where the enable bit ordering is different from the
> > alarm bit ordering), I will update or remove the patch... And yes, I
> > understand this is yet another argument for updating this code to the
> > standardized sysfs beep/alarm model. :)
>
> Definitely. The w83791d currently only creates the old style alarm
> files, I would welcome a patch adding individual alarm and beep files.
>
> --
> Jean Delvare
>

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Jean Delvare]

Hi Charles,

> I wrote an ugly little python script to cause each alarm to trigger
> one at a time (I can provide it if anyone is interested). Based on the
> script, what I found was that the documentation was completely correct
> for the values I tested (in0-9, temp1-3, fan1-5). My board doesn't
> have the pwm stuff and the driver doesn't have that support, so I
> didn't validate the tart1-3 alarms/enables, but since everything else
> was correct, it seems likely those are as well.
> 
> Sven - Can you confirm this result?

Maybe you can share your script with Sven so that he can test quickly.
A confirmation would indeed be welcome, as I know there are different
revisions of the W83791D chip.

> Based on this result and the comment earlier that the driver should do
> a pass through of the values, I will create a documentation only
> change for the 2.6 driver and clean up the lm_sensors patch that
> started this thread.
> 
> Does this sound reasonable?

Yes, sounds like a good plan to me. I'm waiting for your patches :)

-- 
Jean Delvare

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Charles Spirakis]

From: Charles Spirakis <bezaur at gmail.com>

Modified patch from Sven Anders to enhance w83791d support. Adds output
for in7, in8, in9, fan4 and fan5. Also updated to display the (beep)
properly due to the fact that the w83791d beep enable mask is different
than the alarm mask.

Signed-off by: Sven Anders <anders at anduras.de>
Signed-off by: Charles Spirakis <bezaur at gmail.com>

---

 kernel/chips/w83781d.c |   18 +++++++--
 prog/sensors/chips.c   |   94
++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 103
insertions(+), 9 deletions(-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lm-sensors-r4079-20060730_sven_changes.patch
Type: text/x-patch
Size: 8721 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060816/43b7b6d7/attachment.bin 

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Jean Delvare]

Hi Charles, Sven,

> From: Charles Spirakis <bezaur at gmail.com>
> 
> Modified patch from Sven Anders to enhance w83791d support. Adds output
> for in7, in8, in9, fan4 and fan5. Also updated to display the (beep)
> properly due to the fact that the w83791d beep enable mask is different
> than the alarm mask.
> 
> Signed-off by: Sven Anders <anders at anduras.de>
> Signed-off by: Charles Spirakis <bezaur at gmail.com>

Applied, thanks.

-- 
Jean Delvare

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Sven Anders]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Charles Spirakis schrieb:
> I wrote an ugly little python script to cause each alarm to trigger
> one at a time (I can provide it if anyone is interested). Based on the
> script, what I found was that the documentation was completely correct
> for the values I tested (in0-9, temp1-3, fan1-5). My board doesn't
> have the pwm stuff and the driver doesn't have that support, so I
> didn't validate the tart1-3 alarms/enables, but since everything else
> was correct, it seems likely those are as well.
> 
> Sven - Can you confirm this result?

Hello Charles!

I can confirm in1 and temp3, but for in7/in8 the beeps comes on the first
"enabling beep"...

Hope this helps.

Regards
 Sven

- --8X-------------------------------------------------------------------

me-fw-int-01:/root # ./check_beep.py

       in0_input: alarm: 0x000001 enable: 0x000001
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
       in1_input: alarm: 0x000002 enable: 0x002000 <= mismatch
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds
disabling beep via alarm bit
       in2_input: alarm: 0x000004 enable: 0x000004
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
       in3_input: alarm: 0x000008 enable: 0x000008
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
       in4_input: alarm: 0x000100 enable: 0x000100
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
       in5_input: alarm: 0x000200 enable: 0x000200
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
       in6_input: alarm: 0x000400 enable: 0x000400
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
       in7_input: alarm: 0x080000 enable: 0x010000 <= mismatch
enabling beep via enable bit for 10 seconds   ** <= here it beeps!
disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds
disabling beep via alarm bit
       in8_input: alarm: 0x100000 enable: 0x020000 <= mismatch
enabling beep via enable bit for 10 seconds   ** <== here it beeps!
disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds
disabling beep via alarm bit
       in9_input: alarm: 0x004000 enable: 0x004000
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
     temp1_input: alarm: 0x000010 enable: 0x000010
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
     temp2_input: alarm: 0x000020 enable: 0x000020
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
     temp3_input: alarm: 0x002000 enable: 0x000002 <= mismatch
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds
disabling beep via alarm bit
      fan1_input: alarm: 0x000040 enable: 0x000040
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
      fan1_input: alarm: 0x000040 enable: 0x000040
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
      fan2_input: alarm: 0x000080 enable: 0x000080
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
      fan3_input: alarm: 0x000800 enable: 0x000800
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
      fan4_input: alarm: 0x200000 enable: 0x200000
enabling beep via enable bit for 10 seconds  ?? <== Strange beep here!
disabling beep via enable bit
      fan5_input: alarm: 0x400000 enable: 0x400000
enabling beep via enable bit for 10 seconds
skipping tart1_input, enable bit is 18 (262144 = 0x040000)
skipping tart2_input, enable bit is 19 (524288 = 0x080000)
skipping tart3_input, enable bit is 20 (1048576 = 0x100000)
skipping case_open, enable bit is 12 (4096 = 0x001000)
skipping user_enable, enable bit is 23 (8388608 = 0x800000)


- --
 Sven Anders <anders at anduras.de>                 () Ascii Ribbon Campaign
                                                 /\ Support plain text e-mail
 ANDURAS service solutions AG
 Innstra?e 71 - 94036 Passau - Germany
 Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55

Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032
Mitglieder des Vorstands: Sven Anders, Marcus Junker, Michael Sch?n
Vorsitzender des Aufsichtsrats: Dipl. Kfm. Thomas Tr?ger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFE7bLG5lKZ7Feg4EcRApgwAJ9/SdxWJi3ebaMXaTlkiJWPNTtIDwCeOnwD
GtCbEfQGpGUy2FGc40nSdJs=La5G
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: anders.vcf
Type: text/x-vcard
Size: 339 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060824/e089f1b9/attachment-0001.vcf 

[lm-sensors] [PATCH] Update lm_sensors to handle more of the

[Charles Spirakis]

Beeping on "enable bit" means that the enable bit position in the document
turned on the beep noise vs. "alarm bit" which tries using the alarm bit
position. So it sounds like you are confirming the documentation is correct.

Thanks!

-- charles


On 8/24/06, Sven Anders <anders at anduras.de> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Charles Spirakis schrieb:
> > I wrote an ugly little python script to cause each alarm to trigger
> > one at a time (I can provide it if anyone is interested). Based on the
> > script, what I found was that the documentation was completely correct
> > for the values I tested (in0-9, temp1-3, fan1-5). My board doesn't
> > have the pwm stuff and the driver doesn't have that support, so I
> > didn't validate the tart1-3 alarms/enables, but since everything else
> > was correct, it seems likely those are as well.
> >
> > Sven - Can you confirm this result?
>
> Hello Charles!
>
> I can confirm in1 and temp3, but for in7/in8 the beeps comes on the first
> "enabling beep"...
>
> Hope this helps.
>
> Regards
> Sven
>
> - --8X-------------------------------------------------------------------
>
> me-fw-int-01:/root # ./check_beep.py
>
>        in0_input: alarm: 0x000001 enable: 0x000001
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>        in1_input: alarm: 0x000002 enable: 0x002000 <= mismatch
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
> due to misatch, enabling beep via alarm bit for 10 seconds
> disabling beep via alarm bit
>        in2_input: alarm: 0x000004 enable: 0x000004
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>        in3_input: alarm: 0x000008 enable: 0x000008
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>        in4_input: alarm: 0x000100 enable: 0x000100
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>        in5_input: alarm: 0x000200 enable: 0x000200
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>        in6_input: alarm: 0x000400 enable: 0x000400
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>        in7_input: alarm: 0x080000 enable: 0x010000 <= mismatch
> enabling beep via enable bit for 10 seconds   ** <= here it beeps!
> disabling beep via enable bit
> due to misatch, enabling beep via alarm bit for 10 seconds
> disabling beep via alarm bit
>        in8_input: alarm: 0x100000 enable: 0x020000 <= mismatch
> enabling beep via enable bit for 10 seconds   ** <== here it beeps!
> disabling beep via enable bit
> due to misatch, enabling beep via alarm bit for 10 seconds
> disabling beep via alarm bit
>        in9_input: alarm: 0x004000 enable: 0x004000
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>      temp1_input: alarm: 0x000010 enable: 0x000010
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>      temp2_input: alarm: 0x000020 enable: 0x000020
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>      temp3_input: alarm: 0x002000 enable: 0x000002 <= mismatch
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
> due to misatch, enabling beep via alarm bit for 10 seconds
> disabling beep via alarm bit
>       fan1_input: alarm: 0x000040 enable: 0x000040
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>       fan1_input: alarm: 0x000040 enable: 0x000040
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>       fan2_input: alarm: 0x000080 enable: 0x000080
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>       fan3_input: alarm: 0x000800 enable: 0x000800
> enabling beep via enable bit for 10 seconds
> disabling beep via enable bit
>       fan4_input: alarm: 0x200000 enable: 0x200000
> enabling beep via enable bit for 10 seconds  ?? <== Strange beep here!
> disabling beep via enable bit
>       fan5_input: alarm: 0x400000 enable: 0x400000
> enabling beep via enable bit for 10 seconds
> skipping tart1_input, enable bit is 18 (262144 = 0x040000)
> skipping tart2_input, enable bit is 19 (524288 = 0x080000)
> skipping tart3_input, enable bit is 20 (1048576 = 0x100000)
> skipping case_open, enable bit is 12 (4096 = 0x001000)
> skipping user_enable, enable bit is 23 (8388608 = 0x800000)
>
>
> - --
> Sven Anders <anders at anduras.de>                 () Ascii Ribbon Campaign
>                                                  /\ Support plain text
> e-mail
> ANDURAS service solutions AG
> Innstra?e 71 - 94036 Passau - Germany
> Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90
> 50-55
>
> Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB
> 6032
> Mitglieder des Vorstands: Sven Anders, Marcus Junker, Michael Sch?n
> Vorsitzender des Aufsichtsrats: Dipl. Kfm. Thomas Tr?ger
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.2.1 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFE7bLG5lKZ7Feg4EcRApgwAJ9/SdxWJi3ebaMXaTlkiJWPNTtIDwCeOnwD
> GtCbEfQGpGUy2FGc40nSdJs> =La5G
> -----END PGP SIGNATURE-----
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060824/6c4713b1/attachment-0001.html