Hi Charles,
On Mon, 3 Sep 2007 19:55:52 -0700, Charles Spirakis wrote:
> Updated based on the comments in this email thread.
>
> Please take another look.
OK, we're getting there, but I have a few more comments:
> diff -urpN linux-2.6.23-rc1-git10/Documentation/hwmon/w83791d linux-2.6.23-rc1-git10_w83791d/Documentation/hwmon/w83791d
> --- linux-2.6.23-rc1-git10/Documentation/hwmon/w83791d 2007-07-22 13:41:00.000000000 -0700
> +++ linux-2.6.23-rc1-git10_w83791d/Documentation/hwmon/w83791d 2007-09-03 14:59:18.000000000 -0700
> @@ -75,8 +75,31 @@ Voltage sensors (also known as IN sensor
> An alarm is triggered if the voltage has crossed a programmable minimum
> or maximum limit.
>
> -The bit ordering for the alarm "realtime status register" and the
> -"beep enable registers" are different.
> +The w83791d driver has two ways to access the beep_enable and alarm
There seems to be a confusion between beep_enable and beep_mask. The
feature that is replaced by individual files is beep_mask. beep_enable
is still there. The same confusion is present in the bit position table
below.
This makes me realize that the new libsensors doesn't support
beep_enable. I need to add it.
> +functionality of the chip. The first way is via legacy bitmask files in
> +sysfs named alarms and beep_mask. The second way is via the new xxx_alarm
> +and xxx_beep files as describe in the .../Documentation/hwmon/sysfs-interface.
"xxx" is probably better written "*".
s/describe/described/
s/in the/in/
> +
> +Since both methods read and write the underlying hardware, they can be used
> +interchangeably and changes in one will automatically be reflected by
> +the other. If you use the legacy bitmask method, your user-space code is
> +responsible for handling the fact that the alarm and beep_enable bitmasks
beep_mask
> +are not the same (see the table below).
> +
> +NOTE: All new code should be written to use the newer sysfs-interface
> +specification as that avoids this problem and is the preferred interface
> +going forward.
> +
> +When an alarm goes off, you can be warned by a beeping signal through your
> +computer speaker. It is possible to enable all beeping globally, or only
> +the beeping for some alarms.
> +
> +The driver only reads the chip values each 3 seconds; reading them more
> +often will do no harm, but will return 'old' values.
> +
> +Alarm bitmask vs. beep_enable bitmask
> +------------------------------------
beep_mask
> +For legacy code using the legacy alarms and beep_enable bitmask files:
>
> in0 (VCORE) : alarms: 0x000001 beep_enable: 0x000001
> in1 (VINR0) : alarms: 0x000002 beep_enable: 0x002000 <== mismatch
> @@ -102,19 +125,6 @@ tart3 : alarms: 0x040000 beep_en
> case_open : alarms: 0x001000 beep_enable: 0x001000
> user_enable : alarms: -------- beep_enable: 0x800000
Thanks for the documentation update.
> diff -urpN linux-2.6.23-rc1-git10/drivers/hwmon/w83791d.c linux-2.6.23-rc1-git10_w83791d/drivers/hwmon/w83791d.c
> --- linux-2.6.23-rc1-git10/drivers/hwmon/w83791d.c 2007-07-22 13:41:00.000000000 -0700
> +++ linux-2.6.23-rc1-git10_w83791d/drivers/hwmon/w83791d.c 2007-09-03 18:07:27.000000000 -0700
> @@ -2,7 +2,7 @@
> w83791d.c - Part of lm_sensors, Linux kernel modules for hardware
> monitoring
>
> - Copyright (C) 2006 Charles Spirakis <bezaur at gmail.com>
> + Copyright (C) 2006-2007 Charles Spirakis <bezaur at gmail.com>
>
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> @@ -384,6 +384,82 @@ static struct sensor_device_attribute sd
> SENSOR_ATTR(in9_max, S_IWUSR | S_IRUGO, show_in_max, store_in_max, 9),
> };
>
> +
> +static ssize_t show_beep(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct sensor_device_attribute *sensor_attr =
> + to_sensor_dev_attr(attr);
> + struct w83791d_data *data = w83791d_update_device(dev);
> + int bitnr = sensor_attr->index;
> +
> + return sprintf(buf, "%d\n", (data->beep_mask >> bitnr) & 1);
> +}
> +
> +static ssize_t store_beep(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct sensor_device_attribute *sensor_attr =
> + to_sensor_dev_attr(attr);
> + struct i2c_client *client = to_i2c_client(dev);
> + struct w83791d_data *data = i2c_get_clientdata(client); \
Stray trailing backslash.
> + int bitnr = sensor_attr->index;
> + int bytenr = bitnr / 8;
> + long val = simple_strtol(buf, NULL, 10) ? 1 : 0;
> + long beep_bit = val << bitnr;
> +
> + mutex_lock(&data->update_lock);
> +
You're missing a register read here, to refresh the value of
data->beep_mask which may be old (or even uninitialized):
data->beep_mask &= ~(0xff << (bytenr * 8));
data->beep_mask |= w83791d_read(client, W83791D_REG_BEEP_CTRL[bytenr])
<< (bytenr * 8);
> + data->beep_mask &= ~(1 << bitnr);
> + data->beep_mask |= (beep_bit);
Parentheses are not needed. BTW this is the only place where you use
beep_bit so I'm not sure if you need a local variable.
> +
> + w83791d_write(client, W83791D_REG_BEEP_CTRL[bytenr],
> + (data->beep_mask >> (bytenr * 8)) & 0xff);
> +
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
The rest is all OK.
Thanks,
--
Jean Delvare
