On Mon, Jul 04, 2016 at 05:27:04PM +0200, Thilo Cestonaro wrote: > From: Thilo Cestonaro <thilo@xxxxxxxxxx> > > This driver implements support for the FTS BMC Chip "Teutates". > > Signed-off-by: Thilo Cestonaro <thilo@xxxxxxxxxx> > --- > Documentation/hwmon/ftsteutates | 23 ++ > drivers/hwmon/Kconfig | 11 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/ftsteutates.c | 834 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 869 insertions(+) > create mode 100644 Documentation/hwmon/ftsteutates > create mode 100644 drivers/hwmon/ftsteutates.c > > diff --git a/Documentation/hwmon/ftsteutates b/Documentation/hwmon/ftsteutates > new file mode 100644 > index 0000000..2a1bf69 > --- /dev/null > +++ b/Documentation/hwmon/ftsteutates > @@ -0,0 +1,23 @@ > +Kernel driver ftsteutates > +===================== > + > +Supported chips: > + * FTS Teutates > + Prefix: 'ftsteutates' > + Addresses scanned: I2C 0x73 (7-Bit) > + > +Author: Thilo Cestonaro <thilo.cestonaro@xxxxxxxxxxxxxx> > + > + > +Description > +----------- > +The BMC Teutates is the Eleventh generation of Superior System > +monitoring and thermal management solution. It is builds on the basic > +functionality of the BMC Theseus and contains several new features and > +enhancements. It can monitor up to 4 voltages, 16 temperatures and > +8 fans. It also contains an integrated watchdog which is currently > +implemented in this driver. > + > +Specification of the chip can be found here: > +ftp:///pub/Mainboard-OEM-Sales/Services/Software&Tools/Linux_SystemMonitoring&Watchdog&GPIO/BMC-Teutates_Specification_V1.21.pdf > +ftp:///pub/Mainboard-OEM-Sales/Services/Software&Tools/Linux_SystemMonitoring&Watchdog&GPIO/Fujitsu_mainboards-1-Sensors_HowTo-en-US.pdf > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index ff94007..8f5d30d 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -486,6 +486,17 @@ config SENSORS_FSCHMD > This driver can also be built as a module. If so, the module > will be called fschmd. > > +config SENSORS_FTSTEUTATES > + tristate "Fujitsu Technology Solutions sensor chip Teutates" > + depends on I2C > + help > + If you say yes here you get support for the Fujitsu Technology > + Solutions (FTS) sensor chip "Teutates" including support for > + the integrated watchdog. Did you try to enable this ? Probably not, because if I apply your patch and try to run "make allmodconfig" I get drivers/hwmon/Kconfig:495: syntax error drivers/hwmon/Kconfig:494: unknown option "Solutions" drivers/hwmon/Kconfig:495: unknown option "the" drivers/hwmon/Kconfig:498: syntax error > + > + This driver can also be built as a module. If so, the module > + will be called ftsteutates. > + > config SENSORS_GL518SM > tristate "Genesys Logic GL518SM" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 2ef5b7c..dcad5f7 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -62,6 +62,7 @@ obj-$(CONFIG_SENSORS_F71882FG) += f71882fg.o > obj-$(CONFIG_SENSORS_F75375S) += f75375s.o > obj-$(CONFIG_SENSORS_FAM15H_POWER) += fam15h_power.o > obj-$(CONFIG_SENSORS_FSCHMD) += fschmd.o > +obj-$(CONFIG_SENSORS_FTSTEUTATES) += ftsteutates.o > obj-$(CONFIG_SENSORS_G760A) += g760a.o > obj-$(CONFIG_SENSORS_G762) += g762.o > obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o > diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c > new file mode 100644 > index 0000000..19e0906 > --- /dev/null > +++ b/drivers/hwmon/ftsteutates.c > @@ -0,0 +1,834 @@ > +/* > + * fts.c, Support for the FTS Systemmonitoring Chip "Teutates" > + * > + * Copyright (C) 2016 Fujitsu Technology Solutions GmbH, > + * Thilo Cestonaro <thilo.cestonaro@xxxxxxxxxxxxxx> > + * > + * 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; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#include <linux/err.h> > +#include <linux/fs.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/jiffies.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > +#include <linux/sysfs.h> > +#include <linux/uaccess.h> > +#include <linux/watchdog.h> > + > +#define FTS_DEVICE_ID_REG 0x0000 > +#define FTS_DEVICE_REVISION_REG 0x0001 > +#define FTS_DEVICE_STATUS_REG 0x0004 > +#define FTS_SATELLITE_STATUS_REG 0x0005 > +#define FTS_EVENT_STATUS_REG 0x0006 > +#define FTS_GLOBAL_CONTROL_REG 0x0007 > + > +#define FTS_SENSOR_EVENT_REG 0x0010 > + > +#define FTS_FAN_EVENT_REG 0x0014 > +#define FTS_FAN_PRESENT_REG 0x0015 > + > +#define FTS_POWER_ON_TIME_COUNTER_A 0x007A > +#define FTS_POWER_ON_TIME_COUNTER_B 0x007B > +#define FTS_POWER_ON_TIME_COUNTER_C 0x007C > + > +#define FTS_PAGE_SELECT_REG 0x007F > + > +#define FTS_WATCHDOG_TIME_PRESET 0x000B > +#define FTS_WATCHDOG_CONTROL 0x5081 > + > +#define FTS_NO_FAN_SENSORS 0x08 > +#define FTS_NO_TEMP_SENSORS 0x10 > +#define FTS_NO_VOLT_SENSORS 0x04 > + > +/* possible addresses */ > +static const unsigned short normal_i2c[] = { 0x73, I2C_CLIENT_END }; > + > +static struct i2c_device_id fts_id[] = { > + { "ftsteutates", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, fts_id); > + > +struct fts_data { > + struct i2c_client *client; > + struct device *hwmon_dev; This variable is not used outside the probe function and thus not needed in the data structure. > + struct mutex update_lock; > + bool valid; /* zero until following fields are valid */ false > + unsigned long last_updated; /* in jiffies */ > + struct mutex access_lock; > + u8 watchdog_resolution; > + > + /* voltage */ > + u8 volt[FTS_NO_VOLT_SENSORS]; > + > + /* temprature */ temperature That seems obvious from the variable names, though. > + u8 temp_input[FTS_NO_TEMP_SENSORS]; /* value */ > + u8 temp_alarm; /* alarm */ > + > + /* fan */ > + u8 fan_present; /* presence */ > + u8 fan_input[FTS_NO_FAN_SENSORS]; /* revolutions per second */ > + u8 fan_source[FTS_NO_FAN_SENSORS]; /* sensor source */ > + u8 fan_alarm; /* alarm */ Those comments add no value (it appears obvious that fan_alarm refers to an alarm). > +}; > + > +#define FTS_REG_FAN_INPUT(idx) (idx + 0x20) > +#define FTS_REG_FAN_SOURCE(idx) (idx + 0x30) > +#define FTS_REG_FAN_CONTROL(idx) ((idx<<16) + 0x4881) Space before and after operators, please > + > +#define FTS_REG_TEMP_INPUT(idx) (idx + 0x40) > +#define FTS_REG_TEMP_CONTROL(idx) ((idx<<16) + 0x0681) > + All macro parameters should be in ( ), such as #define FTS_REG_TEMP_CONTROL(idx) (((idx) << 16) + 0x0681) > +/* voltage registers */ > +static const u16 FTS_REG_VOLT[FTS_NO_VOLT_SENSORS] = { > + 0x001A, 0x0018, 0x0019, 0x001B > +}; > + > +/*****************************************************************************/ > +/* I2C Helper functions */ > +/*****************************************************************************/ > +int fts_read_byte(struct i2c_client *client, unsigned short reg) static > +{ > + int ret = -1; Unnecessary initialization. > + unsigned char page = reg>>8; reg << 8 checkpatch --strict helps finding such places. > + struct fts_data *data = dev_get_drvdata(&client->dev); > + > + mutex_lock(&data->access_lock); > + > + dev_dbg(&client->dev, "page select - page: 0x%.02x\n", page); > + ret = i2c_smbus_write_byte_data(client, FTS_PAGE_SELECT_REG, page); > + Please no empty line between functions and return value checks. > + if (ret < 0) > + goto error; > + > + reg &= 0xFF; > + ret = i2c_smbus_read_byte_data(client, reg); > + dev_dbg(&client->dev, "read - reg: 0x%.02x: val: 0x%.02x\n", reg, ret); > + > +error: > + mutex_unlock(&data->access_lock); > + return ret; > +} > + > +int fts_write_byte(struct i2c_client *client, unsigned short reg, > + unsigned char value) static Please align continuation lines with '('. > +{ > + int ret; > + unsigned char page = reg>>8; > + struct fts_data *data = dev_get_drvdata(&client->dev); > + > + mutex_lock(&data->access_lock); > + > + dev_dbg(&client->dev, "page select - page: 0x%.02x\n", page); > + ret = i2c_smbus_write_byte_data(client, FTS_PAGE_SELECT_REG, > + page); > + Please no empty line here. > + if (ret < 0) > + goto error; > + > + reg &= 0xFF; > + dev_dbg(&client->dev, > + "write - reg: 0x%.02x: val: 0x%.02x\n", reg, value); > + ret = i2c_smbus_write_byte_data(client, reg, value); > + > +error: > + mutex_unlock(&data->access_lock); > + return ret; > +} > + > +/*****************************************************************************/ > +/* Data Updater Helper function */ > +/*****************************************************************************/ > +static int fts_update_device(struct fts_data *data) > +{ > + int i; > + int err = 0; > + > + mutex_lock(&data->update_lock); > + if (!time_after(jiffies, data->last_updated + 2 * HZ) && data->valid) > + goto exit; > + > + err = fts_read_byte(data->client, FTS_DEVICE_STATUS_REG); > + if (err < 0) > + goto exit; > + > + data->valid = !!(err & 0x02); /* Data not ready yet */ > + if (unlikely(!data->valid)) { > + err = -EAGAIN; > + goto exit; > + } > + > + err = fts_read_byte(data->client, FTS_FAN_PRESENT_REG); > + if (err < 0) > + goto exit; > + data->fan_present = err; > + > + err = fts_read_byte(data->client, FTS_FAN_EVENT_REG); > + if (err < 0) > + goto exit; > + data->fan_alarm = err; > + > + for (i = 0; i < FTS_NO_FAN_SENSORS; i++) { > + if (!!(data->fan_present & BIT(i))) { Unnecessary !! > + err = fts_read_byte(data->client, FTS_REG_FAN_INPUT(i)); > + if (err < 0) > + goto exit; > + data->fan_input[i] = err; > + > + err = fts_read_byte(data->client, > + FTS_REG_FAN_SOURCE(i)); > + if (err < 0) > + goto exit; > + data->fan_source[i] = err; > + } else { > + data->fan_input[i] = 0; > + data->fan_source[i] = 0; > + } > + } > + > + err = fts_read_byte(data->client, FTS_SENSOR_EVENT_REG); > + if (err < 0) > + goto exit; > + data->temp_alarm = err; > + > + for (i = 0; i < FTS_NO_TEMP_SENSORS; i++) { > + err = fts_read_byte(data->client, FTS_REG_TEMP_INPUT(i)); > + if (err < 0) > + goto exit; > + data->temp_input[i] = err; > + } > + > + for (i = 0; i < FTS_NO_VOLT_SENSORS; i++) { > + err = fts_read_byte(data->client, FTS_REG_VOLT[i]); > + if (err < 0) > + goto exit; > + data->volt[i] = err; > + } > + data->last_updated = jiffies; > + err = 0; > +exit: > + mutex_unlock(&data->update_lock); > + return err; > +} > + > +/*****************************************************************************/ > +/* Watchdog functions */ > +/*****************************************************************************/ > +static int fts_wdt_set_resolution(struct watchdog_device *wdd, int resolution) > +{ You might as well pass struct fts_data * to this function. > + struct fts_data *data; > + int ret; > + > + if (resolution != 1 && resolution != 60) > + return -EINVAL; > + The resolution variable passed to this function is always 1, so this check and ... > + data = watchdog_get_drvdata(wdd); > + ret = fts_read_byte(data->client, FTS_WATCHDOG_CONTROL); > + if (ret < 0) > + return ret; > + > + ret = fts_write_byte(data->client, FTS_WATCHDOG_CONTROL, > + resolution == 1 ? ret | BIT(1) : ret & ~BIT(1)); ... the resolution == 1 check here are unnecessary. > + if (ret < 0) > + return ret; > + > + data->watchdog_resolution = resolution; The calling code already sets data->watchdog_resolution. Besides, data->watchdog_resolution is not used anywhere. What is the point of keeping it ? > + return ret; return 0; > +} > + > +static int fts_wdt_start(struct watchdog_device *wdd) > +{ > + struct fts_data *data; > + > + data = watchdog_get_drvdata(wdd); > + return fts_write_byte(data->client, FTS_WATCHDOG_TIME_PRESET, > + wdd->timeout); > +} > + > +static int fts_wdt_stop(struct watchdog_device *wdd) > +{ > + struct fts_data *data; > + > + data = watchdog_get_drvdata(wdd); > + return fts_write_byte(data->client, FTS_WATCHDOG_TIME_PRESET, 0); > +} > + > +static const struct watchdog_info fts_wdt_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, > + .identity = "FTS Teutates Hardware Watchdog", > +}; > + > +static const struct watchdog_ops fts_wdt_ops = { > + .owner = THIS_MODULE, > + .start = fts_wdt_start, > + .stop = fts_wdt_stop, > +}; > + > +static struct watchdog_device fts_wdt = { > + .info = &fts_wdt_info, > + .ops = &fts_wdt_ops, > +}; > + > +static int fts_watchdog_init(struct fts_data *data) > +{ > + int ret, timeout; > + > + watchdog_set_drvdata(&fts_wdt, data); > + > + ret = fts_read_byte(data->client, FTS_WATCHDOG_TIME_PRESET); > + if (ret < 0) > + return ret; > + timeout = ret; > + Just assign to timeout directly. No need to use 'ret' as interim variable. > + /* watchdog not running, set timeout to a default of 60 sec. */ > + if (timeout == 0) { > + data->watchdog_resolution = 1; > + ret = fts_wdt_set_resolution(&fts_wdt, > + data->watchdog_resolution); > + if (ret < 0) > + return ret; > + fts_wdt.timeout = 60; > + Unecessary empty line. > + } else { > + ret = fts_read_byte(data->client, FTS_WATCHDOG_CONTROL); > + if (ret < 0) > + return ret; > + data->watchdog_resolution = !!(ret & BIT(1)) ? 1 : 60; Unnecessary !! > + fts_wdt.timeout = data->watchdog_resolution * timeout; > + fts_wdt.status |= BIT(WDOG_ACTIVE); WDOG_ACTIVE indicates that the watchdog device is opened. You need to set WDOG_HW_RUNNING instead. > + } > + > + /* Register our watchdog part */ > + fts_wdt.parent = &data->client->dev; > + fts_wdt.min_timeout = 1; > + fts_wdt.max_timeout = 0xFF; max_timeout is not necessarly correct, isn't it ? If the watchdog is already running, and the resolution is set to 60 seconds, that should be 60 * 255 seconds. The static nature of fts_wdt is problematic. If the driver is instantiated on multiple addresses, if a chip is there, and if the code above does not detect that the chip isn't really the chip it is supposed to be (it could be an eeprom or nvram with the right values in its registers), everything will be messed up. You'll either need to add more checks (such as checking the i2c address, and possibly a flag to indicate if the driver is already instantiated), or keep all variables local, ie allocated with struct fts_data. > + ret = watchdog_register_device(&fts_wdt); > + return ret; > +} > + > +/*****************************************************************************/ > +/* SysFS handler functions */ > +/*****************************************************************************/ > +static ssize_t show_in_value(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + /* got from the systemboard specification */ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + int err; > + > + err = fts_update_device(data); > + if (err < 0) > + return err; > + > + return sprintf(buf, "%u\n", data->volt[index]); > +} > + > +static ssize_t show_temp_value(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + int err; > + > + err = fts_update_device(data); > + if (err < 0) > + return err; > + > + return sprintf(buf, "%u\n", data->temp_input[index]); > +} > + > +static ssize_t show_temp_fault(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + int err; > + > + err = fts_update_device(data); > + if (err < 0) > + return err; > + > + /* 00h Temperature = Sensor Error */ > + return sprintf(buf, "%d\n", data->temp_input[index] == 0); > +} > + > +static ssize_t show_temp_alarm(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + int err; > + > + err = fts_update_device(data); > + if (err < 0) > + return err; > + > + Please no double empty lines. > + return sprintf(buf, "%u\n", !!(data->temp_alarm & BIT(index))); > +} > + > +static ssize_t > +clear_temp_alarm(struct device *dev, struct device_attribute *devattr, > + const char *buf, size_t count) > +{ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + unsigned long val; > + unsigned char reg; > + int err; > + > + if (!data) { > + dev_err(dev, "no dev drvdata available"); > + return -EINVAL; > + } This won't happen. Please no unnecessary error checks. > + > + err = fts_update_device(data); > + if (err < 0) { > + count = err; > + return err; > + } > + > + if (kstrtoul(buf, 10, &val) || val != 0) { > + count = -EINVAL; > + return count; return -EINVAL; accomplishes exactly the same without assigning a negative value to an unsigned variable. > + } > + > + mutex_lock(&data->update_lock); > + reg = fts_read_byte(data->client, FTS_REG_TEMP_CONTROL(index)); reg is declared as unsigned char, and thus will never be < 0. > + if (reg < 0) { > + count = reg; > + goto error; > + } > + > + val = fts_write_byte(data->client, FTS_REG_TEMP_CONTROL(index), > + reg | 0x1); > + if (val < 0) { val is declared as unsigned long. > + count = val; count is size_t which is unsigned long. Granted, the return value is ssize_t and it will thus be converted back to signed, but it is still a bad idea to rely on that. > + goto error; > + } > + data->valid = false; > + > +error: > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +static ssize_t show_fan_value(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + int err; > + > + err = fts_update_device(data); > + if (err < 0) > + return err; > + > + return sprintf(buf, "%u\n", data->fan_input[index]); > +} > + > +static ssize_t show_fan_source(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + int err; > + > + err = fts_update_device(data); > + if (err < 0) > + return err; > + > + return sprintf(buf, "%u\n", data->fan_source[index]); > +} > + > +static ssize_t show_fan_alarm(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + int err; > + > + err = fts_update_device(data); > + if (err < 0) > + return err; > + > + return sprintf(buf, "%d\n", !!(data->fan_alarm & BIT(index))); > +} > + > +static ssize_t > +clear_fan_alarm(struct device *dev, struct device_attribute *devattr, > + const char *buf, size_t count) > +{ > + struct fts_data *data = dev_get_drvdata(dev); > + int index = to_sensor_dev_attr(devattr)->index; > + unsigned long val; > + unsigned char reg; > + int err; > + > + if (!data) { > + dev_err(dev, "no dev drvdata available"); > + return -EINVAL; > + } Please no unnecessary error checks. > + > + err = fts_update_device(data); > + if (err < 0) { > + count = err; count is size_t (unsigned long). > + return err; > + } > + > + if (kstrtoul(buf, 10, &val) || val != 0) { > + count = -EINVAL; > + return count; return -EINVAL; > + } > + > + mutex_lock(&data->update_lock); > + reg = fts_read_byte(data->client, FTS_REG_FAN_CONTROL(index)); > + if (reg < 0) { reg is declared as unsigned char. > + count = reg; > + goto error; > + } > + > + val = fts_write_byte(data->client, FTS_REG_FAN_CONTROL(index), > + reg | 0x1); > + if (val < 0) { val is unsigned long. Why not use ret here ? Please build your code with W=1 to see (and hopefully eliminate) all those errors. > + count = val; > + goto error; > + } > + data->valid = false; > + > +error: > + mutex_unlock(&data->update_lock); > + return count; > +} > + > +/*****************************************************************************/ > +/* SysFS structs */ > +/*****************************************************************************/ > + > +/* Temprature sensors */ > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0); > +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1); > +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp_value, NULL, 2); > +static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp_value, NULL, 3); > +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, show_temp_value, NULL, 4); > +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, show_temp_value, NULL, 5); > +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, show_temp_value, NULL, 6); > +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, show_temp_value, NULL, 7); > +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, show_temp_value, NULL, 8); > +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, show_temp_value, NULL, 9); > +static SENSOR_DEVICE_ATTR(temp11_input, S_IRUGO, show_temp_value, NULL, 10); > +static SENSOR_DEVICE_ATTR(temp12_input, S_IRUGO, show_temp_value, NULL, 11); > +static SENSOR_DEVICE_ATTR(temp13_input, S_IRUGO, show_temp_value, NULL, 12); > +static SENSOR_DEVICE_ATTR(temp14_input, S_IRUGO, show_temp_value, NULL, 13); > +static SENSOR_DEVICE_ATTR(temp15_input, S_IRUGO, show_temp_value, NULL, 14); > +static SENSOR_DEVICE_ATTR(temp16_input, S_IRUGO, show_temp_value, NULL, 15); > + > +static SENSOR_DEVICE_ATTR(temp1_fault, S_IRUGO, show_temp_fault, NULL, 0); > +static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_temp_fault, NULL, 1); > +static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_temp_fault, NULL, 2); > +static SENSOR_DEVICE_ATTR(temp4_fault, S_IRUGO, show_temp_fault, NULL, 3); > +static SENSOR_DEVICE_ATTR(temp5_fault, S_IRUGO, show_temp_fault, NULL, 4); > +static SENSOR_DEVICE_ATTR(temp6_fault, S_IRUGO, show_temp_fault, NULL, 5); > +static SENSOR_DEVICE_ATTR(temp7_fault, S_IRUGO, show_temp_fault, NULL, 6); > +static SENSOR_DEVICE_ATTR(temp8_fault, S_IRUGO, show_temp_fault, NULL, 7); > +static SENSOR_DEVICE_ATTR(temp9_fault, S_IRUGO, show_temp_fault, NULL, 8); > +static SENSOR_DEVICE_ATTR(temp10_fault, S_IRUGO, show_temp_fault, NULL, 9); > +static SENSOR_DEVICE_ATTR(temp11_fault, S_IRUGO, show_temp_fault, NULL, 10); > +static SENSOR_DEVICE_ATTR(temp12_fault, S_IRUGO, show_temp_fault, NULL, 11); > +static SENSOR_DEVICE_ATTR(temp13_fault, S_IRUGO, show_temp_fault, NULL, 12); > +static SENSOR_DEVICE_ATTR(temp14_fault, S_IRUGO, show_temp_fault, NULL, 13); > +static SENSOR_DEVICE_ATTR(temp15_fault, S_IRUGO, show_temp_fault, NULL, 14); > +static SENSOR_DEVICE_ATTR(temp16_fault, S_IRUGO, show_temp_fault, NULL, 15); > + > +static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 0); > +static SENSOR_DEVICE_ATTR(temp2_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 1); > +static SENSOR_DEVICE_ATTR(temp3_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 2); > +static SENSOR_DEVICE_ATTR(temp4_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 3); > +static SENSOR_DEVICE_ATTR(temp5_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 4); > +static SENSOR_DEVICE_ATTR(temp6_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 5); > +static SENSOR_DEVICE_ATTR(temp7_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 6); > +static SENSOR_DEVICE_ATTR(temp8_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 7); > +static SENSOR_DEVICE_ATTR(temp9_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 8); > +static SENSOR_DEVICE_ATTR(temp10_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 9); > +static SENSOR_DEVICE_ATTR(temp11_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 10); > +static SENSOR_DEVICE_ATTR(temp12_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 11); > +static SENSOR_DEVICE_ATTR(temp13_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 12); > +static SENSOR_DEVICE_ATTR(temp14_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 13); > +static SENSOR_DEVICE_ATTR(temp15_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 14); > +static SENSOR_DEVICE_ATTR(temp16_alarm, S_IRUGO | S_IWUSR, show_temp_alarm, > + clear_temp_alarm, 15); > + > +static struct attribute *fts_temp_attrs[] = { > + &sensor_dev_attr_temp1_input.dev_attr.attr, > + &sensor_dev_attr_temp2_input.dev_attr.attr, > + &sensor_dev_attr_temp3_input.dev_attr.attr, > + &sensor_dev_attr_temp4_input.dev_attr.attr, > + &sensor_dev_attr_temp5_input.dev_attr.attr, > + &sensor_dev_attr_temp6_input.dev_attr.attr, > + &sensor_dev_attr_temp7_input.dev_attr.attr, > + &sensor_dev_attr_temp8_input.dev_attr.attr, > + &sensor_dev_attr_temp9_input.dev_attr.attr, > + &sensor_dev_attr_temp10_input.dev_attr.attr, > + &sensor_dev_attr_temp11_input.dev_attr.attr, > + &sensor_dev_attr_temp12_input.dev_attr.attr, > + &sensor_dev_attr_temp13_input.dev_attr.attr, > + &sensor_dev_attr_temp14_input.dev_attr.attr, > + &sensor_dev_attr_temp15_input.dev_attr.attr, > + &sensor_dev_attr_temp16_input.dev_attr.attr, > + > + &sensor_dev_attr_temp1_fault.dev_attr.attr, > + &sensor_dev_attr_temp2_fault.dev_attr.attr, > + &sensor_dev_attr_temp3_fault.dev_attr.attr, > + &sensor_dev_attr_temp4_fault.dev_attr.attr, > + &sensor_dev_attr_temp5_fault.dev_attr.attr, > + &sensor_dev_attr_temp6_fault.dev_attr.attr, > + &sensor_dev_attr_temp7_fault.dev_attr.attr, > + &sensor_dev_attr_temp8_fault.dev_attr.attr, > + &sensor_dev_attr_temp9_fault.dev_attr.attr, > + &sensor_dev_attr_temp10_fault.dev_attr.attr, > + &sensor_dev_attr_temp11_fault.dev_attr.attr, > + &sensor_dev_attr_temp12_fault.dev_attr.attr, > + &sensor_dev_attr_temp13_fault.dev_attr.attr, > + &sensor_dev_attr_temp14_fault.dev_attr.attr, > + &sensor_dev_attr_temp15_fault.dev_attr.attr, > + &sensor_dev_attr_temp16_fault.dev_attr.attr, > + > + &sensor_dev_attr_temp1_alarm.dev_attr.attr, > + &sensor_dev_attr_temp2_alarm.dev_attr.attr, > + &sensor_dev_attr_temp3_alarm.dev_attr.attr, > + &sensor_dev_attr_temp4_alarm.dev_attr.attr, > + &sensor_dev_attr_temp5_alarm.dev_attr.attr, > + &sensor_dev_attr_temp6_alarm.dev_attr.attr, > + &sensor_dev_attr_temp7_alarm.dev_attr.attr, > + &sensor_dev_attr_temp8_alarm.dev_attr.attr, > + &sensor_dev_attr_temp9_alarm.dev_attr.attr, > + &sensor_dev_attr_temp10_alarm.dev_attr.attr, > + &sensor_dev_attr_temp11_alarm.dev_attr.attr, > + &sensor_dev_attr_temp12_alarm.dev_attr.attr, > + &sensor_dev_attr_temp13_alarm.dev_attr.attr, > + &sensor_dev_attr_temp14_alarm.dev_attr.attr, > + &sensor_dev_attr_temp15_alarm.dev_attr.attr, > + &sensor_dev_attr_temp16_alarm.dev_attr.attr, > + NULL > +}; > + > +/* Fans */ > +static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_fan_value, NULL, 0); > +static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, show_fan_value, NULL, 1); > +static SENSOR_DEVICE_ATTR(fan3_input, S_IRUGO, show_fan_value, NULL, 2); > +static SENSOR_DEVICE_ATTR(fan4_input, S_IRUGO, show_fan_value, NULL, 3); > +static SENSOR_DEVICE_ATTR(fan5_input, S_IRUGO, show_fan_value, NULL, 4); > +static SENSOR_DEVICE_ATTR(fan6_input, S_IRUGO, show_fan_value, NULL, 5); > +static SENSOR_DEVICE_ATTR(fan7_input, S_IRUGO, show_fan_value, NULL, 6); > +static SENSOR_DEVICE_ATTR(fan8_input, S_IRUGO, show_fan_value, NULL, 7); > + > +static SENSOR_DEVICE_ATTR(fan1_source, S_IRUGO, show_fan_source, NULL, 0); > +static SENSOR_DEVICE_ATTR(fan2_source, S_IRUGO, show_fan_source, NULL, 1); > +static SENSOR_DEVICE_ATTR(fan3_source, S_IRUGO, show_fan_source, NULL, 2); > +static SENSOR_DEVICE_ATTR(fan4_source, S_IRUGO, show_fan_source, NULL, 3); > +static SENSOR_DEVICE_ATTR(fan5_source, S_IRUGO, show_fan_source, NULL, 4); > +static SENSOR_DEVICE_ATTR(fan6_source, S_IRUGO, show_fan_source, NULL, 5); > +static SENSOR_DEVICE_ATTR(fan7_source, S_IRUGO, show_fan_source, NULL, 6); > +static SENSOR_DEVICE_ATTR(fan8_source, S_IRUGO, show_fan_source, NULL, 7); > + > +static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO | S_IWUSR, > + show_fan_alarm, clear_fan_alarm, 0); > +static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO | S_IWUSR, > + show_fan_alarm, clear_fan_alarm, 1); > +static SENSOR_DEVICE_ATTR(fan3_alarm, S_IRUGO | S_IWUSR, > + show_fan_alarm, clear_fan_alarm, 2); > +static SENSOR_DEVICE_ATTR(fan4_alarm, S_IRUGO | S_IWUSR, > + show_fan_alarm, clear_fan_alarm, 3); > +static SENSOR_DEVICE_ATTR(fan5_alarm, S_IRUGO | S_IWUSR, > + show_fan_alarm, clear_fan_alarm, 4); > +static SENSOR_DEVICE_ATTR(fan6_alarm, S_IRUGO | S_IWUSR, > + show_fan_alarm, clear_fan_alarm, 5); > +static SENSOR_DEVICE_ATTR(fan7_alarm, S_IRUGO | S_IWUSR, > + show_fan_alarm, clear_fan_alarm, 6); > +static SENSOR_DEVICE_ATTR(fan8_alarm, S_IRUGO | S_IWUSR, > + show_fan_alarm, clear_fan_alarm, 7); > + > +static struct attribute *fts_fan_attrs[] = { > + &sensor_dev_attr_fan1_input.dev_attr.attr, > + &sensor_dev_attr_fan2_input.dev_attr.attr, > + &sensor_dev_attr_fan3_input.dev_attr.attr, > + &sensor_dev_attr_fan4_input.dev_attr.attr, > + &sensor_dev_attr_fan5_input.dev_attr.attr, > + &sensor_dev_attr_fan6_input.dev_attr.attr, > + &sensor_dev_attr_fan7_input.dev_attr.attr, > + &sensor_dev_attr_fan8_input.dev_attr.attr, > + > + &sensor_dev_attr_fan1_source.dev_attr.attr, > + &sensor_dev_attr_fan2_source.dev_attr.attr, > + &sensor_dev_attr_fan3_source.dev_attr.attr, > + &sensor_dev_attr_fan4_source.dev_attr.attr, > + &sensor_dev_attr_fan5_source.dev_attr.attr, > + &sensor_dev_attr_fan6_source.dev_attr.attr, > + &sensor_dev_attr_fan7_source.dev_attr.attr, > + &sensor_dev_attr_fan8_source.dev_attr.attr, > + > + &sensor_dev_attr_fan1_alarm.dev_attr.attr, > + &sensor_dev_attr_fan2_alarm.dev_attr.attr, > + &sensor_dev_attr_fan3_alarm.dev_attr.attr, > + &sensor_dev_attr_fan4_alarm.dev_attr.attr, > + &sensor_dev_attr_fan5_alarm.dev_attr.attr, > + &sensor_dev_attr_fan6_alarm.dev_attr.attr, > + &sensor_dev_attr_fan7_alarm.dev_attr.attr, > + &sensor_dev_attr_fan8_alarm.dev_attr.attr, > + NULL > +}; > + > +/* Voltages */ > +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, show_in_value, NULL, 0); > +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, show_in_value, NULL, 1); > +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, show_in_value, NULL, 2); > +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, show_in_value, NULL, 3); > +static struct attribute *fts_voltage_attrs[] = { > + &sensor_dev_attr_in1_input.dev_attr.attr, > + &sensor_dev_attr_in2_input.dev_attr.attr, > + &sensor_dev_attr_in3_input.dev_attr.attr, > + &sensor_dev_attr_in4_input.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group fts_voltage_attr_group = { > + .attrs = fts_voltage_attrs > +}; > + > +static const struct attribute_group fts_temp_attr_group = { > + .attrs = fts_temp_attrs > +}; > + > +static const struct attribute_group fts_fan_attr_group = { > + .attrs = fts_fan_attrs > +}; > + > +static const struct attribute_group *fts_attr_groups[] = { > + &fts_voltage_attr_group, > + &fts_temp_attr_group, > + &fts_fan_attr_group, > + NULL > +}; > + > +/*****************************************************************************/ > +/* Module initialization / remove functions */ > +/*****************************************************************************/ > +static int fts_remove(struct i2c_client *client) > +{ > + fts_wdt_stop(&fts_wdt); If the watchdog is running, the watchdog core should prevent the driver from being unloaded, so this should not be necessary. > + watchdog_unregister_device(&fts_wdt); > + return 0; > +} > + > +static int fts_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + u8 revision; > + struct fts_data *data; > + long err; Why long and not int ? > + s8 deviceid; > + > + /* Baseboard Management Controller check */ > + deviceid = i2c_smbus_read_byte_data(client, FTS_DEVICE_ID_REG); > + if (deviceid > 0 && (deviceid & 0xF0) == 0x10) { > + switch (deviceid & 0x0F) { > + case 0x01: > + break; > + default: > + dev_dbg(&client->dev, "No aseboard Management Controller\n"); Baseboard > + return -ENODEV; > + } > + } else { > + dev_dbg(&client->dev, "No fujitsu board\n"); > + return -ENODEV; > + } > + > + data = devm_kzalloc(&client->dev, sizeof(struct fts_data), > + GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + mutex_init(&data->update_lock); > + mutex_init(&data->access_lock); > + data->client = client; > + dev_set_drvdata(&client->dev, data); > + > + err = i2c_smbus_read_byte_data(client, FTS_DEVICE_REVISION_REG); > + if (err < 0) { > + dev_err(&client->dev, > + "couldn't read device revision register\n"); > + return err; > + } > + revision = err; > + > + data->hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev, > + "ftsteutates", > + data, > + fts_attr_groups > + ); > + if (IS_ERR(data->hwmon_dev)) { > + err = PTR_ERR(data->hwmon_dev); > + goto exit_detach; Those gotos (and the cleanup) are not necessary. There is no data to be freed, and the watchdog device is not registered. The only possible effect is that the watchdog is stopped if it happened to be running, but one could argue that it should continue to run in this case. Obviously, something must be badly wrong if this happens, and reacting by stopping the watchdog seems to be the wrong solution. > + } > + > + err = fts_watchdog_init(data); > + if (err) > + goto exit_detach; > + > + dev_info(&client->dev, "Detected FTS Teutates chip, revision: %d.%d\n", > + (revision & 0xF0)>>4, revision & 0x0F); > + return 0; > + > +exit_detach: > + fts_remove(client); /* will also free data for us */ > + return err; > +} > + > +/*****************************************************************************/ > +/* Module Details */ > +/*****************************************************************************/ > +static struct i2c_driver fts_driver = { > + .driver = { > + .name = "ftsteutates", > + }, > + .id_table = fts_id, > + .probe = fts_probe, > + .remove = fts_remove, > + .address_list = normal_i2c, Unnecessary without detect function. > +}; > + > +module_i2c_driver(fts_driver); > + > +MODULE_AUTHOR("Thilo Cestonaro <thilo.cestonaro@xxxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("FTS Teutates driver"); > +MODULE_LICENSE("GPL"); > -- > 2.8.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
