[lm-sensors] [PATCH v3 1/1] hwmon: added support for the max6650 alarm stati
Jean Delvare
khali at linux-fr.org
Sat May 30 16:20:19 CEST 2009
Hi Christian,
On Sun, 24 May 2009 22:38:15 +0200, Engelmayer Christian wrote:
> From: Christian Engelmayer <christian.engelmayer at frequentis.com>
>
> Exported the alarm stati provided by the MAX6650/MAX6651 fan-speed regulator
> and monitor chips via sysfs.
>
> Signed-off-by: Christian Engelmayer <christian.engelmayer at frequentis.com>
> ---
> Tested with the MAX6651 chip. This obsoletes the previously proposed patch
> 'support for the max6650 GPIO and alarm features' after discussion on sane
> gpio handling.
>
> Revised after remarks by Hans de Goede. Changed the alarm attributes to
> sensor
> device attributes thus eliminating the need for pointer comparison in show
> function get_alarm(). Left the alarm behaviour unchanged. In case the sysfs
> interface does not prefer 'real time' over 'latched' alarms I prefer to keep
> the style used by the device.
>
> Incorporated additional remarks by Hans de Goede. Changed a temporary
> variable
> from 'u8' to 'int'. Changed the order of alarms within the code from MSB
> first
> to LSB first as occuring within the registers.
Note that the patch is corrupted by your e-mail client, I can't apply
it. Please fix and resend - in attachment if you can't get inline to
work.
>
> --- drivers/hwmon/max6650.c.orig 2009-05-20 12:36:21.000000000 +0200
> +++ drivers/hwmon/max6650.c 2009-05-24 22:13:40.000000000 +0200
> @@ -12,7 +12,7 @@
> * also work with the MAX6651. It does not distinguish max6650 and max6651
> * chips.
> *
> - * Tha datasheet was last seen at:
> + * The datasheet was last seen at:
> *
> * http://pdfserv.maxim-ic.com/en/ds/MAX6650-MAX6651.pdf
> *
> @@ -98,6 +98,17 @@ I2C_CLIENT_INSMOD_1(max6650);
> #define MAX6650_CFG_MODE_OPEN_LOOP 0x30
> #define MAX6650_COUNT_MASK 0x03
>
> +/*
> + * Alarm status register bits
> + */
> +
> +#define MAX6650_ALRM_MAX 0x01
> +#define MAX6650_ALRM_MIN 0x02
> +#define MAX6650_ALRM_TACH 0x04
> +#define MAX6650_ALRM_GPIO1 0x08
> +#define MAX6650_ALRM_GPIO2 0x10
> +#define MAX6650_ALRM_MASK 0x1F
> +
> /* Minimum and maximum values of the FAN-RPM */
> #define FAN_RPM_MIN 240
> #define FAN_RPM_MAX 30000
> @@ -151,6 +162,8 @@ struct max6650_data
> u8 tach[4];
> u8 count;
> u8 dac;
> + u8 alarm_en;
> + u8 alarm;
> };
>
> static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
> @@ -418,6 +431,54 @@ static ssize_t set_div(struct device *de
> return count;
> }
>
> +/*
> + * Get alarm stati:
> + * Possible values:
> + * 0 = no alarm
> + * 1 = alarm
> + */
> +
> +static ssize_t get_alarm(struct device *dev, struct device_attribute
> *devattr,
> + char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + struct max6650_data *data = max6650_update_device(dev);
> + struct i2c_client *client = to_i2c_client(dev);
> + u8 mask = 0x00;
> + int alarm = 0;
> +
> + switch (attr->index) {
> + case 0:
> + mask = MAX6650_ALRM_MAX;
> + break;
> + case 1:
> + mask = MAX6650_ALRM_MIN;
> + break;
> + case 2:
> + mask = MAX6650_ALRM_TACH;
> + break;
> + case 3:
> + mask = MAX6650_ALRM_GPIO1;
> + break;
> + case 4:
> + mask = MAX6650_ALRM_GPIO2;
> + break;
> + default:
> + return 0;
> + }
It would be much easier to pass the mask directly as attr->index. Just
because it is named "index" doesn't mean you can't use it for whatever
you want ;)
> +
> + if (data->alarm & mask) {
> + mutex_lock(&data->update_lock);
> + alarm = 1;
> + data->alarm &= ~mask;
> + data->alarm |= i2c_smbus_read_byte_data(client,
> + MAX6650_REG_ALARM);
> + mutex_unlock(&data->update_lock);
> + }
> +
> + return sprintf(buf, "%d\n", alarm);
> +}
> +
> static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, get_fan, NULL, 0);
> static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, get_fan, NULL, 1);
> static SENSOR_DEVICE_ATTR(fan3_input, S_IRUGO, get_fan, NULL, 2);
> @@ -426,7 +487,41 @@ static DEVICE_ATTR(fan1_target, S_IWUSR
> static DEVICE_ATTR(fan1_div, S_IWUSR | S_IRUGO, get_div, set_div);
> static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO, get_enable, set_enable);
> static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, get_pwm, set_pwm);
> +static SENSOR_DEVICE_ATTR(fan1_max_alarm, S_IRUGO, get_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan1_min_alarm, S_IRUGO, get_alarm, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan1_fault, S_IRUGO, get_alarm, NULL, 2);
> +static SENSOR_DEVICE_ATTR(gpio1_alarm, S_IRUGO, get_alarm, NULL, 3);
> +static SENSOR_DEVICE_ATTR(gpio2_alarm, S_IRUGO, get_alarm, NULL, 4);
>
> +static mode_t max6650_attrs_visible(struct kobject *kobj, struct attribute
> *a,
> + int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct max6650_data *data = max6650_update_device(dev);
> +
> + /*
> + * Hide the alarms that have not been enabled by the firmware
> + */
> +
> + if (a == &sensor_dev_attr_fan1_max_alarm.dev_attr.attr) {
> + if (!(data->alarm_en & MAX6650_ALRM_MAX))
> + return 0;
> + } else if (a == &sensor_dev_attr_fan1_min_alarm.dev_attr.attr) {
> + if (!(data->alarm_en & MAX6650_ALRM_MIN))
> + return 0;
> + } else if (a == &sensor_dev_attr_fan1_fault.dev_attr.attr) {
> + if (!(data->alarm_en & MAX6650_ALRM_TACH))
> + return 0;
> + } else if (a == &sensor_dev_attr_gpio1_alarm.dev_attr.attr) {
> + if (!(data->alarm_en & MAX6650_ALRM_GPIO1))
> + return 0;
> + } else if (a == &sensor_dev_attr_gpio2_alarm.dev_attr.attr) {
> + if (!(data->alarm_en & MAX6650_ALRM_GPIO2))
> + return 0;
> + }
I believe this code could be made somewhat more simple if you store the
mask in ->index as suggested above.
> +
> + return a->mode;
> +}
>
> static struct attribute *max6650_attrs[] = {
> &sensor_dev_attr_fan1_input.dev_attr.attr,
> @@ -437,11 +532,17 @@ static struct attribute *max6650_attrs[]
> &dev_attr_fan1_div.attr,
> &dev_attr_pwm1_enable.attr,
> &dev_attr_pwm1.attr,
> + &sensor_dev_attr_fan1_max_alarm.dev_attr.attr,
> + &sensor_dev_attr_fan1_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_fan1_fault.dev_attr.attr,
> + &sensor_dev_attr_gpio1_alarm.dev_attr.attr,
> + &sensor_dev_attr_gpio2_alarm.dev_attr.attr,
> NULL
> };
>
> static struct attribute_group max6650_attr_grp = {
> .attrs = max6650_attrs,
> + .is_visible = max6650_attrs_visible,
Nice trick, I didn't know about it :)
> };
>
> /*
> @@ -658,6 +759,14 @@ static struct max6650_data *max6650_upda
> data->count = i2c_smbus_read_byte_data(client,
> MAX6650_REG_COUNT);
> data->dac = i2c_smbus_read_byte_data(client,
> MAX6650_REG_DAC);
> + data->alarm_en = i2c_smbus_read_byte_data(client,
> +
> MAX6650_REG_ALARM_EN);
As far as I can see this is used at device initialization time and only
there. This means that reading the value in the update function is
overkill - as is calling it from max6650_attrs_visible (will make the
drivers loading somewhat slower.) You should simply read the value you
need in max6650_attrs_visible() directly. Then you don't even need to
store it in the data structure.
> +
> + /* Alarms are cleared on read in case the condition that
> + * caused the alarm is removed. Keep the value latched here
> + * for providing the register through different alarm files.
> */
> + data->alarm |= i2c_smbus_read_byte_data(client,
> + MAX6650_REG_ALARM);
>
> data->last_updated = jiffies;
> data->valid = 1;
>
--
Jean Delvare
More information about the lm-sensors
mailing list