On Mon, Jun 21, 2010 at 12:04:18PM -0400, Jonathan Cameron wrote: > On 06/21/10 16:06, Guenter Roeck wrote: > > On Mon, Jun 21, 2010 at 05:29:28AM -0400, Jonathan Cameron wrote: > >> On 06/20/10 20:19, Guenter Roeck wrote: > >>> This driver adds support for the monitoring features of the Summit > >>> Microelectronics SMM665 Six-Channel Active DC Output Controller/Monitor. > >>> > >>> Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx> > >> > >> Hi Guenter, > >> > >> Few more comments below. Please don't eat errors when there > >> is no reason to do so. > >> > >> Also I would hold this driver for a few days to see if anyone > >> has comments on the critical values attributes. It would be > >> good to see them supported as well (afterall you have pretty > >> much all the code here anyway!) > >> > > Sounds good to me. I'll be out travelling for the next few days anyway. > > > > Comments inline. > > > > Thanks a lot for the review! > > > > Guenter > > > >> Jonathan > >>> --- > >>> drivers/hwmon/Kconfig | 15 + > >>> drivers/hwmon/Makefile | 1 + > >>> drivers/hwmon/smm665.c | 681 ++++++++++++++++++++++++++++++++++++++++++++++++ > >>> 3 files changed, 697 insertions(+), 0 deletions(-) > >>> create mode 100644 drivers/hwmon/smm665.c > >>> > >>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > >>> index e19cf8e..c45c17e 100644 > >>> --- a/drivers/hwmon/Kconfig > >>> +++ b/drivers/hwmon/Kconfig > >>> @@ -739,6 +739,21 @@ config SENSORS_SIS5595 > >>> This driver can also be built as a module. If so, the module > >>> will be called sis5595. > >>> > >>> +config SENSORS_SMM665 > >>> + tristate "Summit Microelectronics SMM665" > >>> + depends on I2C && EXPERIMENTAL > >>> + default n > >>> + help > >>> + If you say yes here you get support for the hardware monitoring > >>> + features of the Summit Microelectronics SMM665/SMM665B Six-Channel > >>> + Active DC Output Controller / Monitor. > >>> + > >>> + Other supported chips are SMM465, SMM665C, SMM764, and SMM766. > >>> + Support for those chips is untested. > >>> + > >>> + This driver can also be built as a module. If so, the module will > >>> + be called smm665. > >>> + > >>> config SENSORS_DME1737 > >>> tristate "SMSC DME1737, SCH311x and compatibles" > >>> depends on I2C && EXPERIMENTAL > >>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > >>> index 2138ceb..55b0940 100644 > >>> --- a/drivers/hwmon/Makefile > >>> +++ b/drivers/hwmon/Makefile > >>> @@ -86,6 +86,7 @@ obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o > >>> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o > >>> obj-$(CONFIG_SENSORS_SHT15) += sht15.o > >>> obj-$(CONFIG_SENSORS_SIS5595) += sis5595.o > >>> +obj-$(CONFIG_SENSORS_SMM665) += smm665.o > >>> obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o > >>> obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o > >>> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o > >>> diff --git a/drivers/hwmon/smm665.c b/drivers/hwmon/smm665.c > >>> new file mode 100644 > >>> index 0000000..b4e7c8f > >>> --- /dev/null > >>> +++ b/drivers/hwmon/smm665.c > >>> @@ -0,0 +1,681 @@ > >>> +/* > >>> + * Driver for SMM665 Power Controller / Monitor > >>> + * > >>> + * Copyright (C) 2010 Ericsson AB. > >>> + * > >>> + * 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 > >>> + * the Free Software Foundation; version 2 of the License. > >>> + * > >>> + * This driver should also work for SMM465, SMM764, and SMM766, but is untested > >>> + * for those chips. Only monitoring functionality is implemented. > >>> + * > >>> + * Datasheets: > >>> + * http://www.summitmicro.com/prod_select/summary/SMM665/SMM665B_2089_20.pdf > >>> + * http://www.summitmicro.com/prod_select/summary/SMM766B/SMM766B_2122.pdf > >>> + */ > >>> + > >>> +#include <linux/kernel.h> > >>> +#include <linux/module.h> > >>> +#include <linux/init.h> > >>> +#include <linux/err.h> > >>> +#include <linux/slab.h> > >>> +#include <linux/i2c.h> > >>> +#include <linux/hwmon.h> > >>> +#include <linux/hwmon-sysfs.h> > >>> +#include <linux/delay.h> > >>> + > >>> +/* Internal reference voltage (VREF, x 1000 */ > >>> +#define SMM665_VREF_ADC_X1000 1250 > >>> + > >>> +/* module parameters */ > >>> +static int vref = SMM665_VREF_ADC_X1000; > >>> +module_param(vref, int, 0); > >>> +MODULE_PARM_DESC(vref, "Reference voltage in mV"); > >>> + > >>> +enum chips { smm465, smm665, smm665c, smm764, smm766 }; > >>> + > >>> +/* > >>> + * ADC channel addresses > >>> + */ > >>> +#define SMM665_MISC16_ADC_DATA_A 0x00 > >>> +#define SMM665_MISC16_ADC_DATA_B 0x01 > >>> +#define SMM665_MISC16_ADC_DATA_C 0x02 > >>> +#define SMM665_MISC16_ADC_DATA_D 0x03 > >>> +#define SMM665_MISC16_ADC_DATA_E 0x04 > >>> +#define SMM665_MISC16_ADC_DATA_F 0x05 > >>> +#define SMM665_MISC16_ADC_DATA_VDD 0x06 > >>> +#define SMM665_MISC16_ADC_DATA_12V 0x07 > >>> +#define SMM665_MISC16_ADC_DATA_INT_TEMP 0x08 > >>> +#define SMM665_MISC16_ADC_DATA_AIN1 0x09 > >>> +#define SMM665_MISC16_ADC_DATA_AIN2 0x0a > >>> + > >>> +/* > >>> + * Command registers > >>> + */ > >>> +#define SMM665_MISC8_CMD_STS 0x80 > >>> +#define SMM665_MISC8_STATUS1 0x81 > >>> +#define SMM665_MISC8_STATUSS2 0x82 > >>> +#define SMM665_MISC8_IO_POLARITY 0x83 > >>> +#define SMM665_MISC8_PUP_POLARITY 0x84 > >>> +#define SMM665_MISC8_ADOC_STATUS1 0x85 > >>> +#define SMM665_MISC8_ADOC_STATUS2 0x86 > >>> +#define SMM665_MISC8_WRITE_PROT 0x87 > >>> +#define SMM665_MISC8_STS_TRACK 0x88 > >>> + > >>> +/* > >>> + * Configuration registers and register groups > >>> + */ > >>> +#define SMM665_ADOC_ENABLE 0x0d > >>> +#define SMM665_LIMIT_BASE 0x80 /* First limit register */ > >>> + > >>> +/* > >>> + * Limit register bit masks > >>> + */ > >>> +#define SMM665_TRIGGER_RST 0x8000 > >>> +#define SMM665_TRIGGER_HEALTHY 0x4000 > >>> +#define SMM665_TRIGGER_POWEROFF 0x2000 > >>> +#define SMM665_TRIGGER_SHUTDOWN 0x1000 > >>> +#define SMM665_ADC_MASK 0x03ff > >>> + > >>> +#define smm665_is_critical(lim) ((lim) & (SMM665_TRIGGER_RST \ > >>> + | SMM665_TRIGGER_POWEROFF \ > >>> + | SMM665_TRIGGER_SHUTDOWN)) > >>> +/* > >>> + * Fault register bit definitions > >>> + * Values are merged from status registers 1/2, > >>> + * with status register 1 providing the upper 8 bits. > >>> + */ > >>> +#define SMM665_FAULT_A 0x0001 > >>> +#define SMM665_FAULT_B 0x0002 > >>> +#define SMM665_FAULT_C 0x0004 > >>> +#define SMM665_FAULT_D 0x0008 > >>> +#define SMM665_FAULT_E 0x0010 > >>> +#define SMM665_FAULT_F 0x0020 > >>> +#define SMM665_FAULT_VDD 0x0040 > >>> +#define SMM665_FAULT_12V 0x0080 > >>> +#define SMM665_FAULT_TEMP 0x0100 > >>> +#define SMM665_FAULT_AIN1 0x0200 > >>> +#define SMM665_FAULT_AIN2 0x0400 > >>> + > >>> +/* > >>> + * I2C Register addresses > >>> + * > >>> + * The configuration register needs to be the configured base register. > >>> + * The command/status register address is derived from it. > >>> + */ > >>> +#define SMM665_REGMASK 0x78 > >>> +#define SMM665_CMDREG_BASE 0x48 > >>> +#define SMM665_CONFREG_BASE 0x50 > >>> + > >>> +/* > >>> + * Equations given by chip manufacturer to calculate voltage/temperature values > >>> + * vref = Reference voltage on VREF_ADC pin (module parameter) > >>> + * adc = 10bit ADC value read back from registers > >>> + */ > >>> + > >>> +/* Voltage A-F and VDD */ > >>> +#define SMM665_VMON_ADC_TO_VOLTS(adc) ((adc) * vref / 256) > >>> + > >>> +/* Voltage 12VIN */ > >>> +#define SMM665_12VIN_ADC_TO_VOLTS(adc) ((adc) * vref * 3 / 256) > >>> + > >>> +/* Voltage AIN1, AIN2 */ > >>> +#define SMM665_AIN_ADC_TO_VOLTS(adc) ((adc) * vref / 512) > >>> + > >>> +/* Temp Sensor */ > >>> +#define SMM665_TEMP_ADC_TO_CELSIUS(adc) ((adc) <= 511) ? \ > >>> + ((int)(adc) * 1000 / 4) : \ > >>> + (((int)(adc) - 0x400) * 1000 / 4) > >>> + > >>> +#define SMM665_NUM_ADC 11 > >>> + > >>> +/* > >>> + * Chip dependent ADC conversion time, in uS > >>> + */ > >>> +#define SMM665_ADC_WAIT_SMM665 70 > >>> +#define SMM665_ADC_WAIT_SMM766 185 > >>> + > >>> +struct smm665_data { > >>> + enum chips type; > >>> + int conversion_time; /* ADC conversion time */ > >>> + struct device *hwmon_dev; > >>> + struct mutex update_lock; > >>> + bool valid; > >>> + unsigned long last_updated; /* in jiffies */ > >>> + u16 adc[SMM665_NUM_ADC]; /* adc values (raw) */ > >>> + u16 faults; /* fault status */ > >>> + /* The following values are in mV */ > >>> + int critical_under_limit[SMM665_NUM_ADC]; > >>> + int alarm_under_limit[SMM665_NUM_ADC]; > >>> + int critical_over_limit[SMM665_NUM_ADC]; > >>> + int alarm_over_limit[SMM665_NUM_ADC]; > >>> + struct i2c_client *cmdreg; > >>> +}; > >>> + > >>> +/* > >>> + * smm665_read16() > >>> + * > >>> + * Read 16 bit value from <reg>, <reg+1>. Upper 8 bits are in <reg>. > >>> + */ > >>> +static int smm665_read16(struct i2c_client *client, int reg) > >>> +{ > >>> + int rv, val; > >>> + > >>> + rv = i2c_smbus_read_byte_data(client, reg); > >>> + if (rv < 0) > >>> + return rv; > >>> + val = rv << 8; > >>> + rv = i2c_smbus_read_byte_data(client, reg + 1); > >>> + if (rv < 0) > >>> + return rv; > >>> + val |= rv; > >>> + return val; > >>> +} > >>> + > >>> +/* > >>> + * Read adc value. > >>> + */ > >>> +static int smm665_read_adc(struct smm665_data *data, int adc) > >>> +{ > >>> + struct i2c_client *client = data->cmdreg; > >>> + int rv; > >>> + int radc; > >>> + > >>> + /* > >>> + * Algorithm for reading ADC, per SMM665 datasheet > >>> + * > >>> + * {[S][addr][W][Ack]} {[offset][Ack]} {[S][addr][R][Nack]} > >>> + * [wait conversion time] > >>> + * {[S][addr][R][Ack]} {[datahi][Ack]} {[datalo][Ack][P]} > >>> + * > >>> + * To implement the first part of this exchange, > >>> + * do a full read transaction and expect a failure/Nack. > >>> + * This sets up the address pointer on the SMM665 > >>> + * and starts the ADC conversion. > >>> + * Then do a two-byte read transaction. > >>> + */ > >>> + rv = i2c_smbus_read_byte_data(client, adc << 3); > >>> + if (rv != -ENXIO) { > >>> + /* > >>> + * We expect ENXIO to reflect NACK > >>> + * (per Documentation/i2c/fault-codes). > >>> + * Everything else is an error. > >>> + */ > >>> + dev_dbg(&client->dev, > >>> + "Unexpected return code %d when setting ADC index", rv); > >> Better to pass on the return code rather than eat it like this. > > > > Code would then look like > > return (rv < 0) ? rv : -EIO; > > so it adds a little bit of complexity. Since this is an unlikely case, > > guess that should be ok. > Yup, not a lot of complexity for the gain in information if this > goes all the way to userspace. > > > >>> + return -1; > >>> + } > >>> + > >>> + udelay(data->conversion_time); > >>> + > >>> + /* > >>> + * Now read two bytes. > >>> + * > >>> + * Neither i2c_smbus_read_byte() nor > >>> + * i2c_smbus_read_block_data() worked here, > >>> + * so use i2c_smbus_read_word_data() instead. > >>> + * We could also try to use i2c_master_recv(), > >>> + * but that is not always supported. > >>> + */ > >>> + rv = i2c_smbus_read_word_data(client, 0); > >>> + if (rv < 0) { > >>> + dev_dbg(&client->dev, "Failed to read ADC value: error %d", rv); > >>> + return -1; > >> Again, don't eat return codes. Pass them on as far as you can > > > > Ok. > > > >>> + } > >>> + /* > >>> + * Validate/verify readback adc channel (in bit 11..14). > >>> + * High byte is in lower 8 bit of rv, so only shift by 3. > >>> + */ > >>> + radc = (rv >> 3) & 0x0f; > >>> + if (radc != adc) { > >>> + dev_dbg(&client->dev, "Unexpected RADC: Expected %d got %d", > >>> + adc, radc); > >> Not quite sure what the right error code here would be... > > > > I would assume -EAGAIN. The device reported an ADC value, but for the wrong ADC, > > so requesting a retry would probably be the best reaction. > -EAGAIN is typically used for a 'device is busy' circumstance rather than it failed > this time and might work next. Ok, I'll use -EIO, unless you have a better idea. After all, it should not fail in the first place unless something odd is going on. > > > >>> + return -1; > >>> + } > >>> + /* > >>> + * Chip replies with H/L, while SMBus expects L/H. > >>> + * Thus, byte order is reversed, and we have to swap > >>> + * the result. > >>> + */ > >>> + rv = swab16(rv) & SMM665_ADC_MASK; > >>> + > >>> + return rv; > >>> +} > >>> + > >>> +static struct smm665_data *smm665_update_device(struct device *dev) > >>> +{ > >>> + struct i2c_client *client = to_i2c_client(dev); > >>> + struct smm665_data *data = i2c_get_clientdata(client); > >>> + > >>> + mutex_lock(&data->update_lock); > >>> + > >>> + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { > >>> + int i, faults; > >>> + > >>> + /* > >>> + * read status registers > >>> + */ > >>> + faults = smm665_read16(client, SMM665_MISC8_STATUS1); > >>> + if (unlikely(faults < 0)) > >>> + data->faults = 0; > >>> + else > >>> + data->faults = faults; > >>> + > >>> + /* Read adc registers */ > >>> + for (i = 0; i < SMM665_NUM_ADC; i++) { > >>> + int val = smm665_read_adc(data, i); > >>> + if (unlikely(val < 0)) > >> So this is going to keep hammering a device that has already reported > >> an error. Ideally this would drop out and pass the error code all > >> the way up to user space. > > > > Yes, but do we know if this is a transient error ? Code would get quite > > complicated if I would try to sort out the reason for a failure and act on it. > If an error occurs then report it (via return from sysfs read). That way userspace > can know that 'all' data might be invalid. Doesn't matter if some of it is actually > fine. > Makes sense. I'll check if I can store the error in the adc variable and return the stored error when it is read via sysfs. > > > > Looking through other hwmon drivers, they either do the same (such as ltc4245, > > which I used as starting point), or ignore the error - up to the point > > where it is reported as ADC reading to the user (eg lm80). > Yup, there are occasionally patches to clean up error handling to ensure it goes > to userspace where possible. It is one of those things that is nice to get right > in new drivers but no one can be bothered to fix the older ones. If it adds a > lot of complexity then don't bother. I doubt anyone will be that fussy. > > > > > Ultimately, there is no API for reporting this kind of error to userland, > > so I rather stick with precedent and do nothing. > The passing them up to userspace is the api for reporting errors. Whether > user space acts on them is up to user space. So if one reads a sysfs file > and gets an error it should go all the way. Ok. > > > > One exception here is that we have a bit of vagueness in respect to the ADC reading code > > and the NACK mechanism. I think it makes sense to try reading an ADC value during device > > initialization (ie in the probe function), to make sure the code works as expected. > > This will catch problems if the bus driver does not return -ENXIO as response to NACK as > > expected. I'll add that code to the probe function. > I wouldn't bother unless you have reports of cases where it causes trouble. Just > pass any errors that occur anywhere to userspace if possible. If they happen during > probe then the probe should fail (again returning the error code). Ok. > > > >>> + data->adc[i] = 0; > >>> + else > >>> + data->adc[i] = val; > >>> + } > >>> + > >>> + data->last_updated = jiffies; > >>> + data->valid = 1; > >>> + } > >>> + > >>> + mutex_unlock(&data->update_lock); > >>> + > >>> + return data; > >>> +} > >>> + > >>> +/* Return converted value from given adc */ > >>> +static int smm665_convert(u16 adcval, int index) > >>> +{ > >>> + int val = 0; > >>> + > >>> + switch (index) { > >>> + case SMM665_MISC16_ADC_DATA_12V: > >>> + val = SMM665_12VIN_ADC_TO_VOLTS(adcval & SMM665_ADC_MASK); > >>> + break; > >>> + > >>> + case SMM665_MISC16_ADC_DATA_VDD: > >>> + case SMM665_MISC16_ADC_DATA_A: > >>> + case SMM665_MISC16_ADC_DATA_B: > >>> + case SMM665_MISC16_ADC_DATA_C: > >>> + case SMM665_MISC16_ADC_DATA_D: > >>> + case SMM665_MISC16_ADC_DATA_E: > >>> + case SMM665_MISC16_ADC_DATA_F: > >>> + val = SMM665_VMON_ADC_TO_VOLTS(adcval & SMM665_ADC_MASK); > >>> + break; > >>> + > >>> + case SMM665_MISC16_ADC_DATA_AIN1: > >>> + case SMM665_MISC16_ADC_DATA_AIN2: > >>> + val = SMM665_AIN_ADC_TO_VOLTS(adcval & SMM665_ADC_MASK); > >>> + break; > >>> + > >>> + case SMM665_MISC16_ADC_DATA_INT_TEMP: > >>> + val = SMM665_TEMP_ADC_TO_CELSIUS(adcval & SMM665_ADC_MASK); > >>> + break; > >>> + > >>> + default: > >>> + /* If we get here, the developer messed up */ > >>> + WARN_ON_ONCE(1); > >>> + break; > >>> + } > >>> + > >>> + return val; > >>> +} > >>> + > >>> +/* Return converted value from given adc */ > >>> +static int smm665_get_input(struct device *dev, int adc) > >>> +{ > >>> + struct smm665_data *data = smm665_update_device(dev); > >>> + > >>> + return smm665_convert(data->adc[adc], adc); > >>> +} > >>> + > >>> +static int smm665_get_min(struct device *dev, int index) > >>> +{ > >>> + struct i2c_client *client = to_i2c_client(dev); > >>> + struct smm665_data *data = i2c_get_clientdata(client); > >>> + > >>> + return data->alarm_under_limit[index]; > >>> +} > >>> + > >>> +static int smm665_get_max(struct device *dev, int index) > >>> +{ > >>> + struct i2c_client *client = to_i2c_client(dev); > >>> + struct smm665_data *data = i2c_get_clientdata(client); > >>> + > >>> + return data->alarm_over_limit[index]; > >>> +} > >>> + > >>> +static int smm665_get_crit(struct device *dev, int index) > >>> +{ > >>> + struct i2c_client *client = to_i2c_client(dev); > >>> + struct smm665_data *data = i2c_get_clientdata(client); > >>> + > >>> + return data->critical_over_limit[index]; > >>> +} > >>> + > >>> +static int smm665_get_fault(struct device *dev, int index) > >>> +{ > >>> + struct smm665_data *data = smm665_update_device(dev); > >>> + > >>> + if (data->faults & (1 << index)) > >>> + return 1; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +#define SMM665_SHOW(what) \ > >>> + static ssize_t smm665_show_##what(struct device *dev, \ > >>> + struct device_attribute *da, char *buf) \ > >>> +{ \ > >>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); \ > >>> + const int val = smm665_get_##what(dev, attr->index); \ > >>> + return snprintf(buf, PAGE_SIZE, "%d\n", val); \ > >>> +} > >>> + > >>> +SMM665_SHOW(input); > >>> +SMM665_SHOW(min); > >>> +SMM665_SHOW(max); > >>> +SMM665_SHOW(crit); > >>> +SMM665_SHOW(fault); > >>> + > >>> +/* These macros are used below in constructing device attribute objects > >>> + * for use with sysfs_create_group() to make a sysfs device file > >>> + * for each register. > >>> + */ > >>> + > >>> +#define SMM665_ATTR(name, type, cmd_idx) \ > >>> + static SENSOR_DEVICE_ATTR(name##_##type, S_IRUGO, \ > >>> + smm665_show_##type, NULL, cmd_idx) > >>> + > >>> +/* Construct a sensor_device_attribute structure for each register */ > >>> + > >>> +/* Input voltages */ > >>> +SMM665_ATTR(in1, input, SMM665_MISC16_ADC_DATA_12V); > >>> +SMM665_ATTR(in2, input, SMM665_MISC16_ADC_DATA_VDD); > >>> +SMM665_ATTR(in3, input, SMM665_MISC16_ADC_DATA_A); > >>> +SMM665_ATTR(in4, input, SMM665_MISC16_ADC_DATA_B); > >>> +SMM665_ATTR(in5, input, SMM665_MISC16_ADC_DATA_C); > >>> +SMM665_ATTR(in6, input, SMM665_MISC16_ADC_DATA_D); > >>> +SMM665_ATTR(in7, input, SMM665_MISC16_ADC_DATA_E); > >>> +SMM665_ATTR(in8, input, SMM665_MISC16_ADC_DATA_F); > >>> +SMM665_ATTR(in9, input, SMM665_MISC16_ADC_DATA_AIN1); > >>> +SMM665_ATTR(in10, input, SMM665_MISC16_ADC_DATA_AIN2); > >>> + > >>> +/* Input voltages min */ > >>> +SMM665_ATTR(in1, min, SMM665_MISC16_ADC_DATA_12V); > >>> +SMM665_ATTR(in2, min, SMM665_MISC16_ADC_DATA_VDD); > >>> +SMM665_ATTR(in3, min, SMM665_MISC16_ADC_DATA_A); > >>> +SMM665_ATTR(in4, min, SMM665_MISC16_ADC_DATA_B); > >>> +SMM665_ATTR(in5, min, SMM665_MISC16_ADC_DATA_C); > >>> +SMM665_ATTR(in6, min, SMM665_MISC16_ADC_DATA_D); > >>> +SMM665_ATTR(in7, min, SMM665_MISC16_ADC_DATA_E); > >>> +SMM665_ATTR(in8, min, SMM665_MISC16_ADC_DATA_F); > >>> +SMM665_ATTR(in9, min, SMM665_MISC16_ADC_DATA_AIN1); > >>> +SMM665_ATTR(in10, min, SMM665_MISC16_ADC_DATA_AIN2); > >>> + > >>> +/* Input voltages max */ > >>> +SMM665_ATTR(in1, max, SMM665_MISC16_ADC_DATA_12V); > >>> +SMM665_ATTR(in2, max, SMM665_MISC16_ADC_DATA_VDD); > >>> +SMM665_ATTR(in3, max, SMM665_MISC16_ADC_DATA_A); > >>> +SMM665_ATTR(in4, max, SMM665_MISC16_ADC_DATA_B); > >>> +SMM665_ATTR(in5, max, SMM665_MISC16_ADC_DATA_C); > >>> +SMM665_ATTR(in6, max, SMM665_MISC16_ADC_DATA_D); > >>> +SMM665_ATTR(in7, max, SMM665_MISC16_ADC_DATA_E); > >>> +SMM665_ATTR(in8, max, SMM665_MISC16_ADC_DATA_F); > >>> +SMM665_ATTR(in9, max, SMM665_MISC16_ADC_DATA_AIN1); > >>> +SMM665_ATTR(in10, max, SMM665_MISC16_ADC_DATA_AIN2); > >>> + > >>> +/* Faults */ > >>> +SMM665_ATTR(in1, fault, SMM665_FAULT_12V); > >>> +SMM665_ATTR(in2, fault, SMM665_FAULT_VDD); > >>> +SMM665_ATTR(in3, fault, SMM665_FAULT_A); > >>> +SMM665_ATTR(in4, fault, SMM665_FAULT_B); > >>> +SMM665_ATTR(in5, fault, SMM665_FAULT_C); > >>> +SMM665_ATTR(in6, fault, SMM665_FAULT_D); > >>> +SMM665_ATTR(in7, fault, SMM665_FAULT_E); > >>> +SMM665_ATTR(in8, fault, SMM665_FAULT_F); > >>> +SMM665_ATTR(in9, fault, SMM665_FAULT_AIN1); > >>> +SMM665_ATTR(in10, fault, SMM665_FAULT_AIN2); > >>> + > >>> +/* Temperature */ > >>> +SMM665_ATTR(temp1, input, SMM665_MISC16_ADC_DATA_INT_TEMP); > >>> +SMM665_ATTR(temp1, min, SMM665_MISC16_ADC_DATA_INT_TEMP); > >>> +SMM665_ATTR(temp1, max, SMM665_MISC16_ADC_DATA_INT_TEMP); > >> > >> Why have critical here and nowhere else? > > > > Because the API defines it for temperatures, but not for voltages. > > Also, it is "critical high", which makes sense to a point for temperatures, > > but would be insufficient/incomplete for other measurements. > > It would be nice to have "min_crit" and "max_crit" for temperatures > > as well, though. > Yup. Define them and post a patch adding them to the Documentation > (that's more likely to get peoples attention ;) Interesting idea. Might be worth a try ... maybe I should do the same for voltages. > > > >>> +SMM665_ATTR(temp1, crit, SMM665_MISC16_ADC_DATA_INT_TEMP); > >>> +SMM665_ATTR(temp1, fault, SMM665_FAULT_TEMP); > >>> + > >>> +/* > >>> + * Finally, construct an array of pointers to members of the above objects, > >>> + * as required for sysfs_create_group() > >>> + */ > >>> +static struct attribute *smm665_attributes[] = { > >>> + &sensor_dev_attr_in1_input.dev_attr.attr, > >>> + &sensor_dev_attr_in1_min.dev_attr.attr, > >>> + &sensor_dev_attr_in1_max.dev_attr.attr, > >>> + &sensor_dev_attr_in1_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in2_input.dev_attr.attr, > >>> + &sensor_dev_attr_in2_min.dev_attr.attr, > >>> + &sensor_dev_attr_in2_max.dev_attr.attr, > >>> + &sensor_dev_attr_in2_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in3_input.dev_attr.attr, > >>> + &sensor_dev_attr_in3_min.dev_attr.attr, > >>> + &sensor_dev_attr_in3_max.dev_attr.attr, > >>> + &sensor_dev_attr_in3_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in4_input.dev_attr.attr, > >>> + &sensor_dev_attr_in4_min.dev_attr.attr, > >>> + &sensor_dev_attr_in4_max.dev_attr.attr, > >>> + &sensor_dev_attr_in4_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in5_input.dev_attr.attr, > >>> + &sensor_dev_attr_in5_min.dev_attr.attr, > >>> + &sensor_dev_attr_in5_max.dev_attr.attr, > >>> + &sensor_dev_attr_in5_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in6_input.dev_attr.attr, > >>> + &sensor_dev_attr_in6_min.dev_attr.attr, > >>> + &sensor_dev_attr_in6_max.dev_attr.attr, > >>> + &sensor_dev_attr_in6_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in7_input.dev_attr.attr, > >>> + &sensor_dev_attr_in7_min.dev_attr.attr, > >>> + &sensor_dev_attr_in7_max.dev_attr.attr, > >>> + &sensor_dev_attr_in7_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in8_input.dev_attr.attr, > >>> + &sensor_dev_attr_in8_min.dev_attr.attr, > >>> + &sensor_dev_attr_in8_max.dev_attr.attr, > >>> + &sensor_dev_attr_in8_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in9_input.dev_attr.attr, > >>> + &sensor_dev_attr_in9_min.dev_attr.attr, > >>> + &sensor_dev_attr_in9_max.dev_attr.attr, > >>> + &sensor_dev_attr_in9_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_in10_input.dev_attr.attr, > >>> + &sensor_dev_attr_in10_min.dev_attr.attr, > >>> + &sensor_dev_attr_in10_max.dev_attr.attr, > >>> + &sensor_dev_attr_in10_fault.dev_attr.attr, > >>> + > >>> + &sensor_dev_attr_temp1_input.dev_attr.attr, > >>> + &sensor_dev_attr_temp1_min.dev_attr.attr, > >>> + &sensor_dev_attr_temp1_max.dev_attr.attr, > >>> + &sensor_dev_attr_temp1_crit.dev_attr.attr, > >>> + &sensor_dev_attr_temp1_fault.dev_attr.attr, > >>> + > >>> + NULL, > >>> +}; > >>> + > >>> +static const struct attribute_group smm665_group = { > >>> + .attrs = smm665_attributes, > >>> +}; > >>> + > >>> +static int smm665_probe(struct i2c_client *client, > >>> + const struct i2c_device_id *id) > >>> +{ > >>> + struct i2c_adapter *adapter = client->adapter; > >>> + struct smm665_data *data; > >>> + int i, ret; > >>> + > >>> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA > >>> + | I2C_FUNC_SMBUS_WORD_DATA)) > >>> + return -ENODEV; > >>> + > >>> + if (i2c_smbus_read_byte_data(client, SMM665_ADOC_ENABLE) < 0) > >>> + return -ENODEV; > >>> + > >>> + ret = -ENOMEM; > >>> + data = kzalloc(sizeof(*data), GFP_KERNEL); > >>> + if (!data) > >>> + goto out_kzalloc; > >>> + > >>> + i2c_set_clientdata(client, data); > >>> + mutex_init(&data->update_lock); > >>> + > >>> + data->type = id->driver_data; > >>> + data->cmdreg = i2c_new_dummy(adapter, (client->addr & ~SMM665_REGMASK) > >>> + | SMM665_CMDREG_BASE); > >>> + if (!data->cmdreg) > >>> + goto out_new_dummy; > >>> + > >>> + switch (data->type) { > >>> + case smm465: > >>> + case smm665: > >>> + data->conversion_time = SMM665_ADC_WAIT_SMM665; > >>> + break; > >>> + case smm665c: > >>> + case smm764: > >>> + case smm766: > >>> + data->conversion_time = SMM665_ADC_WAIT_SMM766; > >>> + break; > >>> + } > >>> + > >>> + ret = -ENODEV; > >>> + if (i2c_smbus_read_byte_data(data->cmdreg, SMM665_MISC8_CMD_STS) < 0) > >>> + goto out_sysfs_create_group; > >>> + > >>> + /* > >>> + * Read limits. > >>> + * > >>> + * Limit registers start with register SMM665_LIMIT_BASE. > >>> + * Each channel uses 8 registers, providing four limit values > >>> + * per channel. Each limit value requires two registers, with the > >>> + * high byte in the first register and the low byte in the second > >>> + * register. The first two limits are under limit values, followed > >>> + * by two over limit values. > >>> + * > >>> + * Limit register order matches the ADC register order, so we use > >>> + * ADC register defines throughout the code to index limit registers. > >>> + * > >>> + * We save the first retrieved value both as "critical" and "alarm" > >>> + * value. The second value overwrites either the critical or the > >>> + * alarm value, depending on its configuration. This ensures that both > >>> + * critical and alarm values are initialized, even if both registers are > >>> + * configured as critical or non-critical. > >>> + * > >>> + * Note: Critical values for voltage channels are saved, even though > >>> + * this information is currently not used by the driver. This is mostly > >>> + * for consistency, though it might eventually be useful if future APIs > >>> + * support reporting "critical" voltage values. > >>> + */ > >>> + for (i = 0; i < SMM665_NUM_ADC; i++) { > >>> + int val; > >>> + > >>> + val = smm665_read16(client, SMM665_LIMIT_BASE + i * 8); > >>> + if (unlikely(val < 0)) > >>> + goto out_sysfs_create_group; > >>> + data->critical_under_limit[i] = data->alarm_under_limit[i] > >>> + = smm665_convert(val, i); > >>> + val = smm665_read16(client, SMM665_LIMIT_BASE + i * 8 + 2); > >>> + if (unlikely(val < 0)) > >>> + goto out_sysfs_create_group; > >>> + if (smm665_is_critical(val)) > >>> + data->critical_under_limit[i] = smm665_convert(val, i); > >>> + else > >>> + data->alarm_under_limit[i] = smm665_convert(val, i); > >>> + val = smm665_read16(client, SMM665_LIMIT_BASE + i * 8 + 4); > >>> + if (unlikely(val < 0)) > >>> + goto out_sysfs_create_group; > >>> + data->critical_over_limit[i] = data->alarm_over_limit[i] > >>> + = smm665_convert(val, i); > >>> + val = smm665_read16(client, SMM665_LIMIT_BASE + i * 8 + 6); > >>> + if (unlikely(val < 0)) > >>> + goto out_sysfs_create_group; > >>> + if (smm665_is_critical(val)) > >>> + data->critical_over_limit[i] = smm665_convert(val, i); > >>> + else > >>> + data->alarm_over_limit[i] = smm665_convert(val, i); > >>> + } > >>> + > >>> + /* Register sysfs hooks */ > >>> + ret = sysfs_create_group(&client->dev.kobj, &smm665_group); > >>> + if (ret) > >>> + goto out_sysfs_create_group; > >>> + > >>> + data->hwmon_dev = hwmon_device_register(&client->dev); > >>> + if (IS_ERR(data->hwmon_dev)) { > >>> + ret = PTR_ERR(data->hwmon_dev); > >>> + goto out_hwmon_device_register; > >>> + } > >>> + > >>> + return 0; > >>> + > >>> +out_hwmon_device_register: > >>> + sysfs_remove_group(&client->dev.kobj, &smm665_group); > >>> +out_sysfs_create_group: > >>> + i2c_unregister_device(data->cmdreg); > >>> +out_new_dummy: > >>> + kfree(data); > >>> +out_kzalloc: > >>> + return ret; > >>> +} > >>> + > >>> +static int smm665_remove(struct i2c_client *client) > >>> +{ > >>> + struct smm665_data *data = i2c_get_clientdata(client); > >>> + > >>> + i2c_unregister_device(data->cmdreg); > >>> + hwmon_device_unregister(data->hwmon_dev); > >>> + sysfs_remove_group(&client->dev.kobj, &smm665_group); > >>> + > >>> + kfree(data); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static const struct i2c_device_id smm665_id[] = { > >>> + {"smm465", smm465}, > >>> + {"smm665", smm665}, > >>> + {"smm665c", smm665c}, > >>> + {"smm764", smm764}, > >>> + {"smm766", smm766}, > >>> + {} > >>> +}; > >>> + > >>> +MODULE_DEVICE_TABLE(i2c, smm665_id); > >>> + > >>> +/* This is the driver that will be inserted */ > >>> +static struct i2c_driver smm665_driver = { > >>> + .driver = { > >>> + .name = "smm665", > >>> + }, > >>> + .probe = smm665_probe, > >>> + .remove = smm665_remove, > >>> + .id_table = smm665_id, > >>> +}; > >>> + > >>> +static int __init smm665_init(void) > >>> +{ > >>> + return i2c_add_driver(&smm665_driver); > >>> +} > >>> + > >>> +static void __exit smm665_exit(void) > >>> +{ > >>> + i2c_del_driver(&smm665_driver); > >>> +} > >>> + > >>> +MODULE_AUTHOR("Guenter Roeck"); > >>> +MODULE_DESCRIPTION("SMM665 driver"); > >>> +MODULE_LICENSE("GPL"); > >>> + > >>> +module_init(smm665_init); > >>> +module_exit(smm665_exit); > >> > _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
