* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
@ 2007-09-03 18:30 ` Jean Delvare
2007-09-03 21:49 ` Charles Spirakis
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2007-09-03 18:30 UTC (permalink / raw)
To: lm-sensors
Hi Charles,
On Sun, 2 Sep 2007 15:41:16 -0700, Charles Spirakis wrote:
> This patch was made against the 2.6.23.rc1-git10 tree. Please review
> at your leisure and also please let me know if you would like the
> patch updated for a newer version of the kernel.
Thanks for working on this. Here's my review:
> Signed-off by: Charles Spirakis <bezaur@gmail.com>
Note: should be "Signed-off-by" with two dashes.
> 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-08-28 17:04:44.000000000 -0700
> @@ -115,6 +115,4 @@ often will do no harm, but will return '
>
> W83791D TODO:
> ---------------
> -Provide a patch for per-file alarms and beep enables as defined in the hwmon
> - documentation (Documentation/hwmon/sysfs-interface)
> Provide a patch for smart-fan control (still need appropriate motherboard/fans)
A few lines above in this document, is written:
*** NOTE: It is the responsibility of user-space code to handle the fact
that the beep enable and alarm bits are in different positions when using that
feature of the chip.
Maybe it would be good to mention that this problem doesn't exist when
using the new individual alarm and beep files?
> diff -urpN linux-2.6.23-rc1-git10/MAINTAINERS linux-2.6.23-rc1-git10_w83791d/MAINTAINERS
> --- linux-2.6.23-rc1-git10/MAINTAINERS 2007-08-05 23:18:27.000000000 -0700
> +++ linux-2.6.23-rc1-git10_w83791d/MAINTAINERS 2007-08-31 15:48:41.000000000 -0700
> @@ -4070,7 +4070,7 @@ W83791D HARDWARE MONITORING DRIVER
> P: Charles Spirakis
> M: bezaur@gmail.com
> L: lm-sensors@lm-sensors.org
> -S: Maintained
> +S: Odd Fixes
Thank you for maintaining this driver for a year, BTW. This was very
much appreciated.
> 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-01 17:10:32.000000000 -0700
> @@ -2,7 +2,7 @@
> w83791d.c - Part of lm_sensors, Linux kernel modules for hardware
> monitoring
>
> - Copyright (C) 2006 Charles Spirakis <bezaur@gmail.com>
> + Copyright (C) 2006-2007 Charles Spirakis <bezaur@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,88 @@ 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);
> +}
Some times ago, Mark M. Hoffman proposed a more compact alternative to
retrieve the index value:
int bitnr = to_sensor_dev_attr(attr)->index;
You may want to use this.
> +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 = w83791d_update_device(dev);
We normally don't call the big update function in write callbacks. This
function reads dozens of register values which you don't need here,
this will make the write very slow. The preferred way is to read just
the register(s) you need. This should be fairly easy in your case.
> + int bitnr = sensor_attr->index;
> + long val = simple_strtol(buf, NULL, 10) ? 1 : 0;
> + long beep_bit = val ? (1 << bitnr) : 0;
More simply written val << bitnr.
> + int i;
> +
> + mutex_lock(&data->update_lock);
> +
> + data->beep_mask &= ~(1 << bitnr);
> + data->beep_mask |= (beep_bit);
> +
> + val = data->beep_mask;
> +
> + /* In theory, based on the bit we changed we only need to update
> + one register, but why bother, this isn't time critical code... */
> + for (i = 0; i < 3; i++) {
> + w83791d_write(client, W83791D_REG_BEEP_CTRL[i], (val & 0xff));
> + val >>= 8;
> + }
I agree that this isn't time critical, but I2C transactions can be slow
and selecting the right byte is really easy:
int bytenr = bitnr / 8;
w83791d_write(client, W83791D_REG_BEEP_CTRL[bytenr],
(data->beep_mask >> (bytenr * 8)) & 0xff);
So maybe you can do it after all?
> +
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> +static ssize_t show_alarm(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->alarms >> bitnr) & 1);
> +}
> +
> +/* Note: The bitmask for the beep enable/disable is different than
> + the bitmask for the alarm. */
> +static struct sensor_device_attribute sda_in_beep[] = {
> + SENSOR_ATTR(in0_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 0),
> + SENSOR_ATTR(in1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 13),
> + SENSOR_ATTR(in2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 2),
> + SENSOR_ATTR(in3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 3),
> + SENSOR_ATTR(in4_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 8),
> + SENSOR_ATTR(in5_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 9),
> + SENSOR_ATTR(in6_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 10),
> + SENSOR_ATTR(in7_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 16),
> + SENSOR_ATTR(in8_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 17),
> + SENSOR_ATTR(in9_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 14),
> +};
> +
> +static struct sensor_device_attribute sda_in_alarm[] = {
> + SENSOR_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0),
> + SENSOR_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1),
> + SENSOR_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2),
> + SENSOR_ATTR(in3_alarm, S_IRUGO, show_alarm, NULL, 3),
> + SENSOR_ATTR(in4_alarm, S_IRUGO, show_alarm, NULL, 8),
> + SENSOR_ATTR(in5_alarm, S_IRUGO, show_alarm, NULL, 9),
> + SENSOR_ATTR(in6_alarm, S_IRUGO, show_alarm, NULL, 10),
> + SENSOR_ATTR(in7_alarm, S_IRUGO, show_alarm, NULL, 19),
> + SENSOR_ATTR(in8_alarm, S_IRUGO, show_alarm, NULL, 20),
> + SENSOR_ATTR(in9_alarm, S_IRUGO, show_alarm, NULL, 14),
> +};
> +
> #define show_fan_reg(reg) \
> static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
> char *buf) \
> @@ -536,6 +618,22 @@ static struct sensor_device_attribute sd
> show_fan_div, store_fan_div, 4),
> };
>
> +static struct sensor_device_attribute sda_fan_beep[] = {
> + SENSOR_ATTR(fan1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 6),
> + SENSOR_ATTR(fan2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 7),
> + SENSOR_ATTR(fan3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 11),
> + SENSOR_ATTR(fan4_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 21),
> + SENSOR_ATTR(fan5_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 22)
Please add a trailing comma, and below too (4 times total.)
> +};
> +
> +static struct sensor_device_attribute sda_fan_alarm[] = {
> + SENSOR_ATTR(fan1_alarm, S_IRUGO, show_alarm, NULL, 6),
> + SENSOR_ATTR(fan2_alarm, S_IRUGO, show_alarm, NULL, 7),
> + SENSOR_ATTR(fan3_alarm, S_IRUGO, show_alarm, NULL, 11),
> + SENSOR_ATTR(fan4_alarm, S_IRUGO, show_alarm, NULL, 21),
> + SENSOR_ATTR(fan5_alarm, S_IRUGO, show_alarm, NULL, 22)
> +};
> +
> /* read/write the temperature1, includes measured value and limits */
> static ssize_t show_temp1(struct device *dev, struct device_attribute *devattr,
> char *buf)
> @@ -618,6 +716,19 @@ static struct sensor_device_attribute_2
> show_temp23, store_temp23, 1, 2),
> };
>
> +/* Note: The bitmask for the beep enable/disable is different than
> + the bitmask for the alarm. */
> +static struct sensor_device_attribute sda_temp_beep[] = {
> + SENSOR_ATTR(temp1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 4),
> + SENSOR_ATTR(temp2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 5),
> + SENSOR_ATTR(temp3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 1)
> +};
> +
> +static struct sensor_device_attribute sda_temp_alarm[] = {
> + SENSOR_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, 4),
> + SENSOR_ATTR(temp2_alarm, S_IRUGO, show_alarm, NULL, 5),
> + SENSOR_ATTR(temp3_alarm, S_IRUGO, show_alarm, NULL, 13)
> +};
>
> /* get reatime status of all sensors items: voltage, temp, fan */
> static ssize_t show_alarms_reg(struct device *dev,
> @@ -749,17 +860,23 @@ static DEVICE_ATTR(vrm, S_IRUGO | S_IWUS
> #define IN_UNIT_ATTRS(X) \
> &sda_in_input[X].dev_attr.attr, \
> &sda_in_min[X].dev_attr.attr, \
> - &sda_in_max[X].dev_attr.attr
> + &sda_in_max[X].dev_attr.attr, \
> + &sda_in_beep[X].dev_attr.attr, \
> + &sda_in_alarm[X].dev_attr.attr
>
> #define FAN_UNIT_ATTRS(X) \
> &sda_fan_input[X].dev_attr.attr, \
> &sda_fan_min[X].dev_attr.attr, \
> - &sda_fan_div[X].dev_attr.attr
> + &sda_fan_div[X].dev_attr.attr, \
> + &sda_fan_beep[X].dev_attr.attr, \
> + &sda_fan_alarm[X].dev_attr.attr
>
> #define TEMP_UNIT_ATTRS(X) \
> &sda_temp_input[X].dev_attr.attr, \
> &sda_temp_max[X].dev_attr.attr, \
> - &sda_temp_max_hyst[X].dev_attr.attr
> + &sda_temp_max_hyst[X].dev_attr.attr, \
> + &sda_temp_beep[X].dev_attr.attr, \
> + &sda_temp_alarm[X].dev_attr.attr
>
> static struct attribute *w83791d_attributes[] = {
> IN_UNIT_ATTRS(0),
> @@ -1280,3 +1397,5 @@ MODULE_LICENSE("GPL");
>
> module_init(sensors_w83791d_init);
> module_exit(sensors_w83791d_exit);
> +
> +/* vim: set ts=8 sw=8 twx sts=0 noet ai cin cino= formatoptions=croql: */
I'd venture that you did not really want this to be part of your patch?
Overall it looks good, can you generate an updated patch based on my
comments? Then Mark can apply it.
Thanks,
--
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] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
2007-09-03 18:30 ` Jean Delvare
@ 2007-09-03 21:49 ` Charles Spirakis
2007-09-03 22:13 ` Jean Delvare
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Charles Spirakis @ 2007-09-03 21:49 UTC (permalink / raw)
To: lm-sensors
Jean --
Thank you for taking the time to review my code! I always find your
reviews to be very useful and appreciate your help in making the code
base better for everyone.
Below are some comments/questions:
>
> > Signed-off by: Charles Spirakis <bezaur@gmail.com>
>
> Note: should be "Signed-off-by" with two dashes.
>
Oops. Will do.
> >
> > W83791D TODO:
> > ---------------
> > -Provide a patch for per-file alarms and beep enables as defined in the hwmon
> > - documentation (Documentation/hwmon/sysfs-interface)
> > Provide a patch for smart-fan control (still need appropriate motherboard/fans)
>
> A few lines above in this document, is written:
>
> *** NOTE: It is the responsibility of user-space code to handle the fact
> that the beep enable and alarm bits are in different positions when using that
> feature of the chip.
>
> Maybe it would be good to mention that this problem doesn't exist when
> using the new individual alarm and beep files?
>
Yes. I'll clean that up to better describe the legacy vs.
new/preferred interface.
> > + 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);
> > +}
>
> Some times ago, Mark M. Hoffman proposed a more compact alternative to
> retrieve the index value:
>
> int bitnr = to_sensor_dev_attr(attr)->index;
>
> You may want to use this.
I will update. I'm assuming you want me to update all uses in the
driver and not just the new ones?
>
> > +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 = w83791d_update_device(dev);
>
> We normally don't call the big update function in write callbacks. This
> function reads dozens of register values which you don't need here,
> this will make the write very slow. The preferred way is to read just
> the register(s) you need. This should be fairly easy in your case.
The big update function (w83791d_update_device) allows itself to read
the hardware at most once every 3 seconds.
I initially put this call in each of the read/write routines so that
(1) it makes sure that we have recent values if it has been a long
time since the last call (2) it keeps the hardware read access
confined to one routine and (3) it allowed all of the other routines
to simply deal with driver variables instead of the actual hardware
and (4) regardless of the frequency or order of the calls from
user-mode, the values from the hardware would all be from roughly the
same point in time and grabbed, at most, once every 3 seconds.
Did you want me to update this for all of the code in the driver?
>
> > + int bitnr = sensor_attr->index;
> > + long val = simple_strtol(buf, NULL, 10) ? 1 : 0;
> > + long beep_bit = val ? (1 << bitnr) : 0;
>
> More simply written val << bitnr.
WIll do.
> > +
> > + /* In theory, based on the bit we changed we only need to update
> > + one register, but why bother, this isn't time critical code... */
> > + for (i = 0; i < 3; i++) {
> > + w83791d_write(client, W83791D_REG_BEEP_CTRL[i], (val & 0xff));
> > + val >>= 8;
> > + }
>
> I agree that this isn't time critical, but I2C transactions can be slow
> and selecting the right byte is really easy:
>
> int bytenr = bitnr / 8;
> w83791d_write(client, W83791D_REG_BEEP_CTRL[bytenr],
> (data->beep_mask >> (bytenr * 8)) & 0xff);
>
> So maybe you can do it after all?
Will do.
> >
> > +static struct sensor_device_attribute sda_fan_beep[] = {
> > + SENSOR_ATTR(fan1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 6),
> > + SENSOR_ATTR(fan2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 7),
> > + SENSOR_ATTR(fan3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 11),
> > + SENSOR_ATTR(fan4_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 21),
> > + SENSOR_ATTR(fan5_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 22)
>
> Please add a trailing comma, and below too (4 times total.)
Will do.
> > +
> > +/* vim: set ts=8 sw=8 twx sts=0 noet ai cin cino= formatoptions=croql: */
>
> I'd venture that you did not really want this to be part of your patch?
I use vi and I've seen it in some of the other drivers and it seemed
like a good idea at the time :) I will remove.
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
2007-09-03 18:30 ` Jean Delvare
2007-09-03 21:49 ` Charles Spirakis
@ 2007-09-03 22:13 ` Jean Delvare
2007-09-03 22:21 ` Charles Spirakis
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2007-09-03 22:13 UTC (permalink / raw)
To: lm-sensors
Hi Charles,
On Mon, 3 Sep 2007 14:49:54 -0700, Charles Spirakis wrote:
> Below are some comments/questions:
> (...)
> > > + 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);
> > > +}
> >
> > Some times ago, Mark M. Hoffman proposed a more compact alternative to
> > retrieve the index value:
> >
> > int bitnr = to_sensor_dev_attr(attr)->index;
> >
> > You may want to use this.
>
> I will update. I'm assuming you want me to update all uses in the
> driver and not just the new ones?
You're assuming wrong ;) Your patch shouldn't mix a new feature with
cleanups. If you want to update all the other uses, that's fine with
me, but that would be a separate patch. This is completely optional, of
course.
> > > +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 = w83791d_update_device(dev);
> >
> > We normally don't call the big update function in write callbacks. This
> > function reads dozens of register values which you don't need here,
> > this will make the write very slow. The preferred way is to read just
> > the register(s) you need. This should be fairly easy in your case.
> The big update function (w83791d_update_device) allows itself to read
> the hardware at most once every 3 seconds.
>
> I initially put this call in each of the read/write routines so that
> (1) it makes sure that we have recent values if it has been a long
> time since the last call (2) it keeps the hardware read access
> confined to one routine and (3) it allowed all of the other routines
> to simply deal with driver variables instead of the actual hardware
> and (4) regardless of the frequency or order of the calls from
> user-mode, the values from the hardware would all be from roughly the
> same point in time and grabbed, at most, once every 3 seconds.
>
> Did you want me to update this for all of the code in the driver?
No, just for store_beep(). I just double-checked your driver and you do
_not_ call the update function for any other write callback, only for
read callbacks (and that's OK).
The rationale is that writes typically happen at boot time ("sensors
-s" is run from an init script). At this point in time, the user
doesn't want to read the sensor values, so putting all the values in
the cache is not useful. We only have to write a few settings. In many
cases we can write the values directly to the registers. Only in a few
cases we need to read from the registers first (like is the case for
the beep mask.) We want to avoid increasing the boot time by one second
or so because we read a bunch of registers we don't even need.
> > > +
> > > +/* vim: set ts=8 sw=8 twx sts=0 noet ai cin cino= formatoptions=croql: */
> >
> > I'd venture that you did not really want this to be part of your patch?
>
> I use vi and I've seen it in some of the other drivers and it seemed
> like a good idea at the time :) I will remove.
Not everyone uses vi ;) And these settings should really be set at the
tree level, not for each file individually.
--
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] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
` (2 preceding siblings ...)
2007-09-03 22:13 ` Jean Delvare
@ 2007-09-03 22:21 ` Charles Spirakis
2007-09-04 2:55 ` Charles Spirakis
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Charles Spirakis @ 2007-09-03 22:21 UTC (permalink / raw)
To: lm-sensors
On Sep 3, 2007 3:13 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Charles,
>
> On Mon, 3 Sep 2007 14:49:54 -0700, Charles Spirakis wrote:
> > Below are some comments/questions:
> > (...)
> > > > + 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);
> > > > +}
> > >
> > > Some times ago, Mark M. Hoffman proposed a more compact alternative to
> > > retrieve the index value:
> > >
> > > int bitnr = to_sensor_dev_attr(attr)->index;
> > >
> > > You may want to use this.
> >
> > I will update. I'm assuming you want me to update all uses in the
> > driver and not just the new ones?
>
> You're assuming wrong ;) Your patch shouldn't mix a new feature with
> cleanups. If you want to update all the other uses, that's fine with
> me, but that would be a separate patch. This is completely optional, of
> course.
>
Ok. If you don't mind I'll use the old way then so the code is regular
and then update them all at once in a later patch.
> > > > +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 = w83791d_update_device(dev);
> > >
> > > We normally don't call the big update function in write callbacks. This
> > > function reads dozens of register values which you don't need here,
> > > this will make the write very slow. The preferred way is to read just
> > > the register(s) you need. This should be fairly easy in your case.
> > The big update function (w83791d_update_device) allows itself to read
> > the hardware at most once every 3 seconds.
> >
> > I initially put this call in each of the read/write routines so that
> > (1) it makes sure that we have recent values if it has been a long
> > time since the last call (2) it keeps the hardware read access
> > confined to one routine and (3) it allowed all of the other routines
> > to simply deal with driver variables instead of the actual hardware
> > and (4) regardless of the frequency or order of the calls from
> > user-mode, the values from the hardware would all be from roughly the
> > same point in time and grabbed, at most, once every 3 seconds.
> >
> > Did you want me to update this for all of the code in the driver?
>
> No, just for store_beep(). I just double-checked your driver and you do
> _not_ call the update function for any other write callback, only for
> read callbacks (and that's OK).
>
> The rationale is that writes typically happen at boot time ("sensors
> -s" is run from an init script). At this point in time, the user
> doesn't want to read the sensor values, so putting all the values in
> the cache is not useful. We only have to write a few settings. In many
> cases we can write the values directly to the registers. Only in a few
> cases we need to read from the registers first (like is the case for
> the beep mask.) We want to avoid increasing the boot time by one second
> or so because we read a bunch of registers we don't even need.
You're right. Will fix.
>
> > > > +
> > > > +/* vim: set ts=8 sw=8 twx sts=0 noet ai cin cino= formatoptions=croql: */
> > >
> > > I'd venture that you did not really want this to be part of your patch?
> >
> > I use vi and I've seen it in some of the other drivers and it seemed
> > like a good idea at the time :) I will remove.
>
> Not everyone uses vi ;) And these settings should really be set at the
> tree level, not for each file individually.
removed (running quickly to avoid the vi/non-vi religious war that could ensue)
>
> --
> 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] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
` (3 preceding siblings ...)
2007-09-03 22:21 ` Charles Spirakis
@ 2007-09-04 2:55 ` Charles Spirakis
2007-09-04 8:36 ` Jean Delvare
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Charles Spirakis @ 2007-09-04 2:55 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1: Type: text/plain, Size: 424 bytes --]
Updated based on the comments in this email thread.
Please take another look.
-- charles
---
Add new sysfs alarm methodology to w83791d driver
Signed-off-by: Charles Spirakis <bezaur@gmail.com>
---
Documentation/hwmon/w83791d | 40 +++++++++-----
MAINTAINERS | 2
drivers/hwmon/w83791d.c | 119 ++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 141 insertions(+), 20 deletions(-)
[-- Attachment #2: w83791d_alarms.patch --]
[-- Type: application/octet-stream, Size: 10333 bytes --]
Add new sysfs alarm methodology to w83791d driver
Signed-off-by: Charles Spirakis <bezaur@gmail.com>
---
Documentation/hwmon/w83791d | 40 +++++++++-----
MAINTAINERS | 2
drivers/hwmon/w83791d.c | 119 ++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 141 insertions(+), 20 deletions(-)
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
+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.
+
+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
+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
+------------------------------------
+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
-*** NOTE: It is the responsibility of user-space code to handle the fact
-that the beep enable and alarm bits are in different positions when using that
-feature of the chip.
-
-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.
-
W83791D TODO:
---------------
-Provide a patch for per-file alarms and beep enables as defined in the hwmon
- documentation (Documentation/hwmon/sysfs-interface)
Provide a patch for smart-fan control (still need appropriate motherboard/fans)
diff -urpN linux-2.6.23-rc1-git10/MAINTAINERS linux-2.6.23-rc1-git10_w83791d/MAINTAINERS
--- linux-2.6.23-rc1-git10/MAINTAINERS 2007-08-05 23:18:27.000000000 -0700
+++ linux-2.6.23-rc1-git10_w83791d/MAINTAINERS 2007-08-31 15:48:41.000000000 -0700
@@ -4070,7 +4070,7 @@ W83791D HARDWARE MONITORING DRIVER
P: Charles Spirakis
M: bezaur@gmail.com
L: lm-sensors@lm-sensors.org
-S: Maintained
+S: Odd Fixes
W83793 HARDWARE MONITORING DRIVER
P: Rudolf Marek
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@gmail.com>
+ Copyright (C) 2006-2007 Charles Spirakis <bezaur@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); \
+ 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);
+
+ data->beep_mask &= ~(1 << bitnr);
+ data->beep_mask |= (beep_bit);
+
+ w83791d_write(client, W83791D_REG_BEEP_CTRL[bytenr],
+ (data->beep_mask >> (bytenr * 8)) & 0xff);
+
+ mutex_unlock(&data->update_lock);
+
+ return count;
+}
+
+static ssize_t show_alarm(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->alarms >> bitnr) & 1);
+}
+
+/* Note: The bitmask for the beep enable/disable is different than
+ the bitmask for the alarm. */
+static struct sensor_device_attribute sda_in_beep[] = {
+ SENSOR_ATTR(in0_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 0),
+ SENSOR_ATTR(in1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 13),
+ SENSOR_ATTR(in2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 2),
+ SENSOR_ATTR(in3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 3),
+ SENSOR_ATTR(in4_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 8),
+ SENSOR_ATTR(in5_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 9),
+ SENSOR_ATTR(in6_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 10),
+ SENSOR_ATTR(in7_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 16),
+ SENSOR_ATTR(in8_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 17),
+ SENSOR_ATTR(in9_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 14),
+};
+
+static struct sensor_device_attribute sda_in_alarm[] = {
+ SENSOR_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0),
+ SENSOR_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1),
+ SENSOR_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2),
+ SENSOR_ATTR(in3_alarm, S_IRUGO, show_alarm, NULL, 3),
+ SENSOR_ATTR(in4_alarm, S_IRUGO, show_alarm, NULL, 8),
+ SENSOR_ATTR(in5_alarm, S_IRUGO, show_alarm, NULL, 9),
+ SENSOR_ATTR(in6_alarm, S_IRUGO, show_alarm, NULL, 10),
+ SENSOR_ATTR(in7_alarm, S_IRUGO, show_alarm, NULL, 19),
+ SENSOR_ATTR(in8_alarm, S_IRUGO, show_alarm, NULL, 20),
+ SENSOR_ATTR(in9_alarm, S_IRUGO, show_alarm, NULL, 14),
+};
+
#define show_fan_reg(reg) \
static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
char *buf) \
@@ -536,6 +612,22 @@ static struct sensor_device_attribute sd
show_fan_div, store_fan_div, 4),
};
+static struct sensor_device_attribute sda_fan_beep[] = {
+ SENSOR_ATTR(fan1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 6),
+ SENSOR_ATTR(fan2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 7),
+ SENSOR_ATTR(fan3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 11),
+ SENSOR_ATTR(fan4_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 21),
+ SENSOR_ATTR(fan5_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 22),
+};
+
+static struct sensor_device_attribute sda_fan_alarm[] = {
+ SENSOR_ATTR(fan1_alarm, S_IRUGO, show_alarm, NULL, 6),
+ SENSOR_ATTR(fan2_alarm, S_IRUGO, show_alarm, NULL, 7),
+ SENSOR_ATTR(fan3_alarm, S_IRUGO, show_alarm, NULL, 11),
+ SENSOR_ATTR(fan4_alarm, S_IRUGO, show_alarm, NULL, 21),
+ SENSOR_ATTR(fan5_alarm, S_IRUGO, show_alarm, NULL, 22),
+};
+
/* read/write the temperature1, includes measured value and limits */
static ssize_t show_temp1(struct device *dev, struct device_attribute *devattr,
char *buf)
@@ -618,6 +710,19 @@ static struct sensor_device_attribute_2
show_temp23, store_temp23, 1, 2),
};
+/* Note: The bitmask for the beep enable/disable is different than
+ the bitmask for the alarm. */
+static struct sensor_device_attribute sda_temp_beep[] = {
+ SENSOR_ATTR(temp1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 4),
+ SENSOR_ATTR(temp2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 5),
+ SENSOR_ATTR(temp3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 1),
+};
+
+static struct sensor_device_attribute sda_temp_alarm[] = {
+ SENSOR_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, 4),
+ SENSOR_ATTR(temp2_alarm, S_IRUGO, show_alarm, NULL, 5),
+ SENSOR_ATTR(temp3_alarm, S_IRUGO, show_alarm, NULL, 13),
+};
/* get reatime status of all sensors items: voltage, temp, fan */
static ssize_t show_alarms_reg(struct device *dev,
@@ -749,17 +854,23 @@ static DEVICE_ATTR(vrm, S_IRUGO | S_IWUS
#define IN_UNIT_ATTRS(X) \
&sda_in_input[X].dev_attr.attr, \
&sda_in_min[X].dev_attr.attr, \
- &sda_in_max[X].dev_attr.attr
+ &sda_in_max[X].dev_attr.attr, \
+ &sda_in_beep[X].dev_attr.attr, \
+ &sda_in_alarm[X].dev_attr.attr
#define FAN_UNIT_ATTRS(X) \
&sda_fan_input[X].dev_attr.attr, \
&sda_fan_min[X].dev_attr.attr, \
- &sda_fan_div[X].dev_attr.attr
+ &sda_fan_div[X].dev_attr.attr, \
+ &sda_fan_beep[X].dev_attr.attr, \
+ &sda_fan_alarm[X].dev_attr.attr
#define TEMP_UNIT_ATTRS(X) \
&sda_temp_input[X].dev_attr.attr, \
&sda_temp_max[X].dev_attr.attr, \
- &sda_temp_max_hyst[X].dev_attr.attr
+ &sda_temp_max_hyst[X].dev_attr.attr, \
+ &sda_temp_beep[X].dev_attr.attr, \
+ &sda_temp_alarm[X].dev_attr.attr
static struct attribute *w83791d_attributes[] = {
IN_UNIT_ATTRS(0),
[-- Attachment #3: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
` (4 preceding siblings ...)
2007-09-04 2:55 ` Charles Spirakis
@ 2007-09-04 8:36 ` Jean Delvare
2007-09-04 9:06 ` Jean Delvare
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2007-09-04 8:36 UTC (permalink / raw)
To: lm-sensors
On Mon, 3 Sep 2007 15:21:13 -0700, Charles Spirakis wrote:
> On Sep 3, 2007 3:13 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Mon, 3 Sep 2007 14:49:54 -0700, Charles Spirakis wrote:
> > > I will update. I'm assuming you want me to update all uses in the
> > > driver and not just the new ones?
> >
> > You're assuming wrong ;) Your patch shouldn't mix a new feature with
> > cleanups. If you want to update all the other uses, that's fine with
> > me, but that would be a separate patch. This is completely optional, of
> > course.
>
> Ok. If you don't mind I'll use the old way then so the code is regular
> and then update them all at once in a later patch.
Perfectly fine.
--
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] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
` (5 preceding siblings ...)
2007-09-04 8:36 ` Jean Delvare
@ 2007-09-04 9:06 ` Jean Delvare
2007-09-04 20:31 ` Charles Spirakis
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2007-09-04 9:06 UTC (permalink / raw)
To: lm-sensors
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@gmail.com>
> + Copyright (C) 2006-2007 Charles Spirakis <bezaur@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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
` (6 preceding siblings ...)
2007-09-04 9:06 ` Jean Delvare
@ 2007-09-04 20:31 ` Charles Spirakis
2007-09-06 9:30 ` Jean Delvare
2007-09-09 15:38 ` Mark M. Hoffman
9 siblings, 0 replies; 11+ messages in thread
From: Charles Spirakis @ 2007-09-04 20:31 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1: Type: text/plain, Size: 600 bytes --]
Significant documentation changes. Hopefully it reads better now and I
believe I've cleaned up the global beep enable vs. beep bitmask vs.
alarm bitmap aspect.
Also cleaned up the store_beep() per your earlier comment in this thread.
-- charles
---
Add new sysfs alarm methodology to w83791d driver
Signed-off-by: Charles Spirakis <bezaur@gmail.com>
---
Documentation/hwmon/w83791d | 96 ++++++++++++++++++++--------------
MAINTAINERS | 2
drivers/hwmon/w83791d.c | 122 ++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 176 insertions(+), 44 deletions(-)
[-- Attachment #2: w83791d_alarms.patch --]
[-- Type: application/octet-stream, Size: 13368 bytes --]
Add new sysfs alarm methodology to w83791d driver
Signed-off-by: Charles Spirakis <bezaur@gmail.com>
---
Documentation/hwmon/w83791d | 96 ++++++++++++++++++++--------------
MAINTAINERS | 2
drivers/hwmon/w83791d.c | 122 ++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 176 insertions(+), 44 deletions(-)
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-04 13:09:53.000000000 -0700
@@ -75,46 +75,64 @@ 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.
-
-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
-
-*** NOTE: It is the responsibility of user-space code to handle the fact
-that the beep enable and alarm bits are in different positions when using that
-feature of the chip.
-
-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.
+The w83791d has a global bit used to enable beeping from the speaker when an
+alarm is triggered as well as a bitmask to enable or disable the beep for
+specific alarms. You need both the global beep enable bit and the
+corresponding beep bit to be on for a triggered alarm to sound a beep.
+
+The sysfs interface to the gloabal enable is via the sysfs beep_enable file.
+This file is used for both legacy and new code.
+
+The sysfs interface to the beep bitmask has migrated from the original legacy
+method of a single sysfs beep_mask file to a newer method using multiple
+*_beep files as described in .../Documentation/hwmon/sysfs-interface.
+
+A similar change has occured for the bitmap corresponding to the alarms. The
+original legacy method used a single sysfs alarms file containing a bitmap
+of triggered alarms. The newer method uses multiple sysfs *_alarm files
+(again following the pattern described in sysfs-interface).
+
+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 alarms and beep_mask bitmaps
+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 bitmap problems and is the preferred interface
+going forward.
+
+The driver reads the hardware chip values at most once every three seconds.
+User mode code requesting values more often will receive cached values.
+
+Alarms bitmap vs. beep_mask bitmask
+------------------------------------
+For legacy code using the alarms and beep_mask files:
+
+in0 (VCORE) : alarms: 0x000001 beep_mask: 0x000001
+in1 (VINR0) : alarms: 0x000002 beep_mask: 0x002000 <== mismatch
+in2 (+3.3VIN): alarms: 0x000004 beep_mask: 0x000004
+in3 (5VDD) : alarms: 0x000008 beep_mask: 0x000008
+in4 (+12VIN) : alarms: 0x000100 beep_mask: 0x000100
+in5 (-12VIN) : alarms: 0x000200 beep_mask: 0x000200
+in6 (-5VIN) : alarms: 0x000400 beep_mask: 0x000400
+in7 (VSB) : alarms: 0x080000 beep_mask: 0x010000 <== mismatch
+in8 (VBAT) : alarms: 0x100000 beep_mask: 0x020000 <== mismatch
+in9 (VINR1) : alarms: 0x004000 beep_mask: 0x004000
+temp1 : alarms: 0x000010 beep_mask: 0x000010
+temp2 : alarms: 0x000020 beep_mask: 0x000020
+temp3 : alarms: 0x002000 beep_mask: 0x000002 <== mismatch
+fan1 : alarms: 0x000040 beep_mask: 0x000040
+fan2 : alarms: 0x000080 beep_mask: 0x000080
+fan3 : alarms: 0x000800 beep_mask: 0x000800
+fan4 : alarms: 0x200000 beep_mask: 0x200000
+fan5 : alarms: 0x400000 beep_mask: 0x400000
+tart1 : alarms: 0x010000 beep_mask: 0x040000 <== mismatch
+tart2 : alarms: 0x020000 beep_mask: 0x080000 <== mismatch
+tart3 : alarms: 0x040000 beep_mask: 0x100000 <== mismatch
+case_open : alarms: 0x001000 beep_mask: 0x001000
+global_enable: alarms: -------- beep_mask: 0x800000 (modified via beep_enable)
W83791D TODO:
---------------
-Provide a patch for per-file alarms and beep enables as defined in the hwmon
- documentation (Documentation/hwmon/sysfs-interface)
Provide a patch for smart-fan control (still need appropriate motherboard/fans)
diff -urpN linux-2.6.23-rc1-git10/MAINTAINERS linux-2.6.23-rc1-git10_w83791d/MAINTAINERS
--- linux-2.6.23-rc1-git10/MAINTAINERS 2007-08-05 23:18:27.000000000 -0700
+++ linux-2.6.23-rc1-git10_w83791d/MAINTAINERS 2007-08-31 15:48:41.000000000 -0700
@@ -4070,7 +4070,7 @@ W83791D HARDWARE MONITORING DRIVER
P: Charles Spirakis
M: bezaur@gmail.com
L: lm-sensors@lm-sensors.org
-S: Maintained
+S: Odd Fixes
W83793 HARDWARE MONITORING DRIVER
P: Rudolf Marek
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-04 11:11:04.000000000 -0700
@@ -2,7 +2,7 @@
w83791d.c - Part of lm_sensors, Linux kernel modules for hardware
monitoring
- Copyright (C) 2006 Charles Spirakis <bezaur@gmail.com>
+ Copyright (C) 2006-2007 Charles Spirakis <bezaur@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,85 @@ 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);
+ int bitnr = sensor_attr->index;
+ int bytenr = bitnr / 8;
+ long val = simple_strtol(buf, NULL, 10) ? 1 : 0;
+
+ mutex_lock(&data->update_lock);
+
+ 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 |= val << bitnr;
+
+ w83791d_write(client, W83791D_REG_BEEP_CTRL[bytenr],
+ (data->beep_mask >> (bytenr * 8)) & 0xff);
+
+ mutex_unlock(&data->update_lock);
+
+ return count;
+}
+
+static ssize_t show_alarm(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->alarms >> bitnr) & 1);
+}
+
+/* Note: The bitmask for the beep enable/disable is different than
+ the bitmask for the alarm. */
+static struct sensor_device_attribute sda_in_beep[] = {
+ SENSOR_ATTR(in0_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 0),
+ SENSOR_ATTR(in1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 13),
+ SENSOR_ATTR(in2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 2),
+ SENSOR_ATTR(in3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 3),
+ SENSOR_ATTR(in4_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 8),
+ SENSOR_ATTR(in5_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 9),
+ SENSOR_ATTR(in6_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 10),
+ SENSOR_ATTR(in7_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 16),
+ SENSOR_ATTR(in8_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 17),
+ SENSOR_ATTR(in9_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 14),
+};
+
+static struct sensor_device_attribute sda_in_alarm[] = {
+ SENSOR_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0),
+ SENSOR_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1),
+ SENSOR_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2),
+ SENSOR_ATTR(in3_alarm, S_IRUGO, show_alarm, NULL, 3),
+ SENSOR_ATTR(in4_alarm, S_IRUGO, show_alarm, NULL, 8),
+ SENSOR_ATTR(in5_alarm, S_IRUGO, show_alarm, NULL, 9),
+ SENSOR_ATTR(in6_alarm, S_IRUGO, show_alarm, NULL, 10),
+ SENSOR_ATTR(in7_alarm, S_IRUGO, show_alarm, NULL, 19),
+ SENSOR_ATTR(in8_alarm, S_IRUGO, show_alarm, NULL, 20),
+ SENSOR_ATTR(in9_alarm, S_IRUGO, show_alarm, NULL, 14),
+};
+
#define show_fan_reg(reg) \
static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
char *buf) \
@@ -536,6 +615,22 @@ static struct sensor_device_attribute sd
show_fan_div, store_fan_div, 4),
};
+static struct sensor_device_attribute sda_fan_beep[] = {
+ SENSOR_ATTR(fan1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 6),
+ SENSOR_ATTR(fan2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 7),
+ SENSOR_ATTR(fan3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 11),
+ SENSOR_ATTR(fan4_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 21),
+ SENSOR_ATTR(fan5_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 22),
+};
+
+static struct sensor_device_attribute sda_fan_alarm[] = {
+ SENSOR_ATTR(fan1_alarm, S_IRUGO, show_alarm, NULL, 6),
+ SENSOR_ATTR(fan2_alarm, S_IRUGO, show_alarm, NULL, 7),
+ SENSOR_ATTR(fan3_alarm, S_IRUGO, show_alarm, NULL, 11),
+ SENSOR_ATTR(fan4_alarm, S_IRUGO, show_alarm, NULL, 21),
+ SENSOR_ATTR(fan5_alarm, S_IRUGO, show_alarm, NULL, 22),
+};
+
/* read/write the temperature1, includes measured value and limits */
static ssize_t show_temp1(struct device *dev, struct device_attribute *devattr,
char *buf)
@@ -618,6 +713,19 @@ static struct sensor_device_attribute_2
show_temp23, store_temp23, 1, 2),
};
+/* Note: The bitmask for the beep enable/disable is different than
+ the bitmask for the alarm. */
+static struct sensor_device_attribute sda_temp_beep[] = {
+ SENSOR_ATTR(temp1_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 4),
+ SENSOR_ATTR(temp2_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 5),
+ SENSOR_ATTR(temp3_beep, S_IWUSR | S_IRUGO, show_beep, store_beep, 1),
+};
+
+static struct sensor_device_attribute sda_temp_alarm[] = {
+ SENSOR_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, 4),
+ SENSOR_ATTR(temp2_alarm, S_IRUGO, show_alarm, NULL, 5),
+ SENSOR_ATTR(temp3_alarm, S_IRUGO, show_alarm, NULL, 13),
+};
/* get reatime status of all sensors items: voltage, temp, fan */
static ssize_t show_alarms_reg(struct device *dev,
@@ -749,17 +857,23 @@ static DEVICE_ATTR(vrm, S_IRUGO | S_IWUS
#define IN_UNIT_ATTRS(X) \
&sda_in_input[X].dev_attr.attr, \
&sda_in_min[X].dev_attr.attr, \
- &sda_in_max[X].dev_attr.attr
+ &sda_in_max[X].dev_attr.attr, \
+ &sda_in_beep[X].dev_attr.attr, \
+ &sda_in_alarm[X].dev_attr.attr
#define FAN_UNIT_ATTRS(X) \
&sda_fan_input[X].dev_attr.attr, \
&sda_fan_min[X].dev_attr.attr, \
- &sda_fan_div[X].dev_attr.attr
+ &sda_fan_div[X].dev_attr.attr, \
+ &sda_fan_beep[X].dev_attr.attr, \
+ &sda_fan_alarm[X].dev_attr.attr
#define TEMP_UNIT_ATTRS(X) \
&sda_temp_input[X].dev_attr.attr, \
&sda_temp_max[X].dev_attr.attr, \
- &sda_temp_max_hyst[X].dev_attr.attr
+ &sda_temp_max_hyst[X].dev_attr.attr, \
+ &sda_temp_beep[X].dev_attr.attr, \
+ &sda_temp_alarm[X].dev_attr.attr
static struct attribute *w83791d_attributes[] = {
IN_UNIT_ATTRS(0),
[-- Attachment #3: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
` (7 preceding siblings ...)
2007-09-04 20:31 ` Charles Spirakis
@ 2007-09-06 9:30 ` Jean Delvare
2007-09-09 15:38 ` Mark M. Hoffman
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2007-09-06 9:30 UTC (permalink / raw)
To: lm-sensors
Hi Charles,
On Tue, 4 Sep 2007 13:31:56 -0700, Charles Spirakis wrote:
> Significant documentation changes. Hopefully it reads better now and I
> believe I've cleaned up the global beep enable vs. beep bitmask vs.
> alarm bitmap aspect.
>
> Also cleaned up the store_beep() per your earlier comment in this thread.
>
> -- charles
>
> ---
> Add new sysfs alarm methodology to w83791d driver
>
> Signed-off-by: Charles Spirakis <bezaur@gmail.com>
Looks good to me, thanks!
Acked-by: Jean Delvare <khali@linux-fr.org>
Mark, please apply.
--
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] 11+ messages in thread* Re: [lm-sensors] [PATCH] update w83791d driver with new sysfs
2007-09-02 22:41 [lm-sensors] [PATCH] update w83791d driver with new sysfs Charles Spirakis
` (8 preceding siblings ...)
2007-09-06 9:30 ` Jean Delvare
@ 2007-09-09 15:38 ` Mark M. Hoffman
9 siblings, 0 replies; 11+ messages in thread
From: Mark M. Hoffman @ 2007-09-09 15:38 UTC (permalink / raw)
To: lm-sensors
Hi:
* Jean Delvare <khali@linux-fr.org> [2007-09-06 11:30:47 +0200]:
> Hi Charles,
>
> On Tue, 4 Sep 2007 13:31:56 -0700, Charles Spirakis wrote:
> > Significant documentation changes. Hopefully it reads better now and I
> > believe I've cleaned up the global beep enable vs. beep bitmask vs.
> > alarm bitmap aspect.
> >
> > Also cleaned up the store_beep() per your earlier comment in this thread.
> >
> > -- charles
> >
> > ---
> > Add new sysfs alarm methodology to w83791d driver
> >
> > Signed-off-by: Charles Spirakis <bezaur@gmail.com>
>
> Looks good to me, thanks!
>
> Acked-by: Jean Delvare <khali@linux-fr.org>
Applied to hwmon-2.6.git/testing, thanks.
--
Mark M. Hoffman
mhoffman@lightlink.com
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread