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
