Hi Ben, On Mon, 9 Jun 2008 12:13:53 +0100, Ben Hutchings wrote: > > Signed-off-by: Ben Hutchings <bhutchings at solarflare.com> > --- > drivers/hwmon/Kconfig | 10 ++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/max664x.c | 355 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/max664x.h | 92 ++++++++++++ > 4 files changed, 458 insertions(+), 0 deletions(-) > create mode 100644 drivers/hwmon/max664x.c > create mode 100644 include/linux/max664x.h First of all: please rename the driver to max6646. We don't use "x" in hwmon driver names, because this tends to get confusing: your driver doesn't support the MAX6640, MAX6641, etc. chips while the max664x "mask" suggests it would. So the rule is to pick the name of the first supported chip as the driver name. Second preliminary note: these chips look suspiciously similar to the LM90, ADM1032 and MAX6657 chips which are supported by the lm90 driver. Are you certain that you need a new driver for them? I've still reviewed your driver, but my feeling is that it may be less work for you to add support for the new chips to the lm90 driver, than to fix your new driver. As far as I can see the only significant difference is the encoding of the temperatures (signed vs. unsigned), and support for that kind of difference has been added to the lm90 driver a few weeks ago. You can look at my pending lm90 patches here: http://jdelvare.pck.nerim.net/sensors/lm90/ I also have parallel work to support new-style i2c devices with this driver (same as for the lm87 driver, this will be merged in 2.6.27-rc1): http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/hwmon-lm90-convert-to-new-style.patch Can you please also send me a dump of one of these chips? I keep a collection of hwmon chip dumps, it helps me when reviewing a driver or testing changes I make to the driver later on. You can take a dump of the chip using the i2c-dev driver and the i2cdump user-space tool, part of the i2c-tools package. > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 00ff533..fad29c0 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -512,6 +512,16 @@ config SENSORS_MAX1619 > This driver can also be built as a module. If so, the module > will be called max1619. > > +config SENSORS_MAX664X > + tristate "Maxim MAX6646/6647/6649 sensor chips" > + depends on I2C > + help > + If you say yes here you get support for MAX6646/6647/6649 Please spell out MAX6647 and MAX6649 in full in the help text. This makes it possible for users to search for part names while configuring their kernel. > + sensor chips. > + > + This driver can also be built as a module. If so, the module > + will be called max664x. > + > config SENSORS_MAX6650 > tristate "Maxim MAX6650 sensor chip" > depends on I2C && EXPERIMENTAL > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index d098677..24b907e 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -58,6 +58,7 @@ obj-$(CONFIG_SENSORS_LM90) += lm90.o > obj-$(CONFIG_SENSORS_LM92) += lm92.o > obj-$(CONFIG_SENSORS_LM93) += lm93.o > obj-$(CONFIG_SENSORS_MAX1619) += max1619.o > +obj-$(CONFIG_SENSORS_MAX664X) += max664x.o > obj-$(CONFIG_SENSORS_MAX6650) += max6650.o > obj-$(CONFIG_SENSORS_PC87360) += pc87360.o > obj-$(CONFIG_SENSORS_PC87427) += pc87427.o > diff --git a/drivers/hwmon/max664x.c b/drivers/hwmon/max664x.c > new file mode 100644 > index 0000000..c5cd365 > --- /dev/null > +++ b/drivers/hwmon/max664x.c > @@ -0,0 +1,355 @@ > +/**************************************************************************** > + * Driver for Maxim MAX6646/6647/6649 sensor chips > + * Copyright 2008 Solarflare Communications Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published > + * by the Free Software Foundation, incorporated herein by reference. > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/err.h> > +#include <linux/jiffies.h> > +#include <linux/i2c.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/max664x.h> I don't think we want hardware-specific header files to go directly in include/linux. There could virtually be 45 such header files in the future, that would be a big mess. It's probably time to create include/linux/hwmon or even include/hwmon? I don't know what others think? > + > +/* Data sheet: <http://datasheets.maxim-ic.com/en/ds/MAX6646-MAX6649.pdf> */ > + > +#define MAX664X_REG_RLTS 0x00 > +#define MAX664X_REG_RRTE 0x01 > +#define MAX664X_REG_RSL 0x02 > +#define MAX664X_REG_RCL 0x03 > +#define MAX664X_REG_RCRA 0x04 > +#define MAX664X_REG_RLHN 0x05 > +#define MAX664X_REG_RLLI 0x06 > +#define MAX664X_REG_RRHI 0x07 > +#define MAX664X_REG_RRLS 0x08 > +#define MAX664X_REG_WCA 0x09 > +#define MAX664X_REG_WCRW 0x0a > +#define MAX664X_REG_WLHO 0x0b > +#define MAX664X_REG_WLLM 0x0c > +#define MAX664X_REG_WRHA 0x0d > +#define MAX664X_REG_WRLN 0x0e > +#define MAX664X_REG_OSHT 0x0f > +#define MAX664X_REG_REET 0x10 > +#define MAX664X_REG_RIET 0x11 > +#define MAX664X_REG_RWOE 0x19 > +#define MAX664X_REG_RWOI 0x20 > +#define MAX664X_REG_HYS 0x21 > +#define MAX664X_REG_QUEUE 0x22 > +#define MAX664X_REG_MFID 0xfe > +#define MAX664X_REG_REVID 0xff You apparently used tabs instead of spaces for the last 8 defines. Also, it looks like many of these defines aren't used in your driver. > + > +struct max664x_data { > + struct device *hwmon_dev; > + struct mutex update_lock; > + int valid; > + unsigned long last_updated; /* in jiffies */ > + struct max664x_settings set; > + struct max664x_values val; > +}; > + > +#define TEMP_FROM_REG(val) ((val) * 1000) > +#define TEMP_TO_REG(val) ((val) / 1000) This lacks proper rounding. > +#define PERIOD_FROM_RATE_REG(val) (16000 >> min_t(int, val, 6)) > +#define PERIOD_TO_RATE_REG(val) SENSORS_LIMIT(7 - ffs(val / 250), 0, 6) Note that in general we try to move away from macros. Using (possibly inline) functions for these conversions is preferred, as it is more readable and lets the compiler do type checks. And less error-prone: your macros lack some parentheses. > + > +static struct max664x_data *max664x_update_device(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct max664x_data *data = i2c_get_clientdata(client); > + > + mutex_lock(&data->update_lock); > + > + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { > + data->set.rate = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RCRA); > + data->val.temp[0] = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RLTS); > + data->set.temp_overt[0] = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RWOI); > + data->set.temp_high_alert[0] = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RLHN); > + data->set.temp_low_alert[0] = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RLLI); > + data->val.temp[1] = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RRTE); > + data->set.temp_overt[1] = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RWOE); > + data->set.temp_high_alert[1] = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RRHI); > + data->set.temp_low_alert[1] = > + i2c_smbus_read_byte_data(client, MAX664X_REG_RRLS); > + data->set.temp_overt_hyst = > + i2c_smbus_read_byte_data(client, MAX664X_REG_HYS); > + > + data->last_updated = jiffies; > + data->valid = 1; > + } > + > + mutex_unlock(&data->update_lock); > + > + return data; > +} > + > +struct max664x_values *max664x_update_values(struct i2c_client *client) > +{ > + struct max664x_data *data = max664x_update_device(&client->dev); > + return &data->val; > +} > +EXPORT_SYMBOL(max664x_update_values); This should rather be named max6646_get_values. "Update" makes sense in the driver because we know we are caching the values and "update" refers to the cache, but from the user's perspective, all the function does is reading register values. > + > +static void > +set_temp_limit(struct device *dev, const char *buf, int offset, int reg) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct max664x_data *data = i2c_get_clientdata(client); > + long val = simple_strtol(buf, NULL, 10); > + u8 reg_val = TEMP_TO_REG(val); > + > + mutex_lock(&data->update_lock); > + ((u8 *)&data->set)[offset] = reg_val; > + i2c_smbus_write_byte_data(client, reg, reg_val); > + mutex_unlock(&data->update_lock); > +} > + > +#define TEMP_LIMIT_ATTR(offset, limit, array_name, reg_name) \ > + static ssize_t \ > + show_temp##offset##_##limit(struct device *dev, \ > + struct device_attribute *attr, char *buf) \ > + { \ > + struct max664x_data *data = max664x_update_device(dev); \ > + return sprintf(buf, "%u\n", \ > + TEMP_FROM_REG(data->set.array_name[offset - 1])); \ > + } \ > + static ssize_t \ > + set_temp##offset##_##limit(struct device *dev, \ > + struct device_attribute *attr, \ > + const char *buf, size_t count) \ > + { \ > + set_temp_limit(dev, buf, \ > + offsetof(struct max664x_settings, \ > + array_name[offset - 1]), \ > + MAX664X_REG_##reg_name); \ > + return count; \ > + } \ > + static DEVICE_ATTR(temp##offset##_##limit, S_IRUGO | S_IWUSR, \ > + show_temp##offset##_##limit, \ > + set_temp##offset##_##limit); Argh, no, please, not these macro-generated functions again :( It has been a lot of work to get rid of almost all the these ugly constructs in the hwmon drivers, let's not reintroduce them in new drivers. Please make use of SENSOR_DEVICE_ATTR() and to_sensor_dev_attr(attr) to pass private data from sysfs attributes to their callbacks. The adm1025 driver is one of many drivers doing this properly if you want to get an idea how this works. This will let you get rid of all macro-generated functions, which is very desirable for code readability, code maintainability, compilation time and binary size. > + > +static ssize_t show_temp_crit_hyst(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct max664x_data *data = max664x_update_device(dev); > + return sprintf(buf, "%u\n", TEMP_FROM_REG(data->set.temp_overt_hyst)); > +} > + > +static ssize_t > +set_temp_crit_hyst(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct max664x_data *data = i2c_get_clientdata(client); > + long val = simple_strtol(buf, NULL, 10); > + u8 reg_val = TEMP_TO_REG(val); > + > + mutex_lock(&data->update_lock); > + data->set.temp_overt_hyst = reg_val; > + i2c_smbus_write_byte_data(client, MAX664X_REG_HYS, reg_val); > + mutex_unlock(&data->update_lock); > + return count; > +} This violates the standard. All temperature values in sysfs should be absolute, not relative. > + > +#define TEMP_ATTRS(offset, overt_reg, high_alert_reg, low_alert_reg) \ > + static ssize_t \ > + show_temp##offset##_input(struct device *dev, \ > + struct device_attribute *attr, char *buf) \ > + { \ > + struct max664x_data *data = max664x_update_device(dev); \ > + return sprintf(buf, "%u\n", \ > + TEMP_FROM_REG(data->val.temp[offset - 1])); \ > + } \ > + static DEVICE_ATTR(temp##offset##_input, S_IRUGO, \ > + show_temp##offset##_input, NULL); \ > + TEMP_LIMIT_ATTR(offset, crit, temp_overt, overt_reg) \ > + static DEVICE_ATTR(temp##offset##_crit_hyst, S_IRUGO, \ > + show_temp_crit_hyst, set_temp_crit_hyst); \ > + TEMP_LIMIT_ATTR(offset, max, temp_high_alert, high_alert_reg) \ > + TEMP_LIMIT_ATTR(offset, min, temp_low_alert, low_alert_reg) > + > +TEMP_ATTRS(1, RWOI, WLHO, WLLM) > +TEMP_ATTRS(2, RWOE, WRHA, WRLN) Please define static const arrays with these registers, for example: static const u8 MAX6646_REG_ROI[2] = { MAX6646_REG_RWOI, MAX6646_REG_RWOE }; Then you can just use an index value to select the channel. This will allow for code refactoring all over the driver. > + > +static ssize_t > +show_period(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + struct max664x_data *data = max664x_update_device(dev); > + return sprintf(buf, "%u\n", PERIOD_FROM_RATE_REG(data->set.rate)); > +} > +static ssize_t set_period(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct max664x_data *data = i2c_get_clientdata(client); > + long val = simple_strtol(buf, NULL, 10); > + u8 reg_val = PERIOD_TO_RATE_REG(val); > + > + mutex_lock(&data->update_lock); > + data->set.rate = reg_val; > + i2c_smbus_write_byte_data(client, MAX664X_REG_WCRW, reg_val); > + mutex_unlock(&data->update_lock); > + return count; > +} > +static DEVICE_ATTR(period, S_IRUGO | S_IWUSR, show_period, set_period); Not a standard hwmon attribute, and "period" is a pretty vague term. Other drivers don't bother exposing this value to user-space, as the default setting is usually fine and people don't really care. > + > +/* > + * Status is read-to-clear, so it is not world-readable and is not > + * updated with the other fields. > + */ > +static ssize_t show_status(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + int val = i2c_smbus_read_byte_data(client, MAX664X_REG_RSL); > + return val >= 0 ? sprintf(buf, "0x%x\n", val) : val; > +} > +static DEVICE_ATTR(status, S_IRUSR, show_status, NULL); Alarms bit are to be split into individual files according to Documentation/hwmon/sysfs-interface. And other drivers make this information publicly readable - but you can't do that safely without caching the value. > + > +int max664x_read_status(struct i2c_client *client) > +{ > + return i2c_smbus_read_byte_data(client, MAX664X_REG_RSL); > +} > +EXPORT_SYMBOL(max664x_read_status); I fail to see the point of making this separate from max6646_get_values (or whatever it ends up being called)? > + > +static struct attribute *max664x_attributes[] = { > + &dev_attr_temp1_input.attr, > + &dev_attr_temp1_crit.attr, > + &dev_attr_temp1_crit_hyst.attr, > + &dev_attr_temp1_max.attr, > + &dev_attr_temp1_min.attr, > + &dev_attr_temp2_input.attr, > + &dev_attr_temp2_crit.attr, > + &dev_attr_temp2_crit_hyst.attr, > + &dev_attr_temp2_max.attr, > + &dev_attr_temp2_min.attr, > + > + &dev_attr_period.attr, > + &dev_attr_status.attr, > + > + NULL > +}; > + > +static const struct attribute_group max664x_group = { > + .attrs = max664x_attributes, > +}; > + > +static int > +max664x_probe(struct i2c_client *client, const struct i2c_device_id *id) > +{ > + const struct max664x_platform_data *plat_data = > + client->dev.platform_data; > + struct max664x_data *data; > + int err; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_BYTE_DATA)) > + return -EIO; That's not an I/O error. -EOPNOTSUPP would be more suitable, methinks. > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->valid = 0; Pointless initialization, given that kzalloc() zero'd the whole structure already. > + mutex_init(&data->update_lock); > + if (plat_data) > + data->set = plat_data->set; These values will never be used, except by yourself in this function. It would be more efficient to not set data->set, and use plat_data->set instead. It would also make more sense, as right now you write to data->set values which you may not write to the chip (if any of set_rate or set_limits isn't set) so the chip and cache are out of sync. > + i2c_set_clientdata(client, data); > + > + if (plat_data && plat_data->set_rate) > + i2c_smbus_write_byte_data(client, MAX664X_REG_WCRW, > + data->set.rate); > + if (plat_data && plat_data->set_limits) { > + i2c_smbus_write_byte_data(client, MAX664X_REG_RWOI, > + data->set.temp_overt[0]); > + i2c_smbus_write_byte_data(client, MAX664X_REG_WLHO, > + data->set.temp_high_alert[0]); > + i2c_smbus_write_byte_data(client, MAX664X_REG_WLLM, > + data->set.temp_low_alert[0]); > + i2c_smbus_write_byte_data(client, MAX664X_REG_RWOE, > + data->set.temp_overt[1]); > + i2c_smbus_write_byte_data(client, MAX664X_REG_WRHA, > + data->set.temp_high_alert[1]); > + i2c_smbus_write_byte_data(client, MAX664X_REG_WRLN, > + data->set.temp_low_alert[1]); > + i2c_smbus_write_byte_data(client, MAX664X_REG_HYS, > + data->set.temp_overt_hyst); > + } > + > + err = sysfs_create_group(&client->dev.kobj, &max664x_group); > + if (err) > + goto exit_free; > + > + data->hwmon_dev = hwmon_device_register(&client->dev); > + if (IS_ERR(data->hwmon_dev)) { > + err = PTR_ERR(data->hwmon_dev); > + goto exit_remove; > + } > + > + return 0; > + > +exit_remove: > + sysfs_remove_group(&client->dev.kobj, &max664x_group); > +exit_free: > + kfree(data); > + i2c_set_clientdata(client, NULL); > + return err; > +} > + > +static int max664x_remove(struct i2c_client *client) > +{ > + struct max664x_data *data = i2c_get_clientdata(client); > + > + hwmon_device_unregister(data->hwmon_dev); > + sysfs_remove_group(&client->dev.kobj, &max664x_group); > + > + kfree(data); > + i2c_set_clientdata(client, NULL); > + return 0; > +} > + > +static const struct i2c_device_id max664x_id[] = { > + { "max6646" }, > + { "max6647" }, > + { "max6649" }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, max664x_id); > + > +struct i2c_driver max664x_driver = { > + .driver = { > + .name = "max664x", > + }, > + .probe = max664x_probe, > + .remove = max664x_remove, > + .id_table = max664x_id, > +}; > + > +static int __init max664x_init(void) > +{ > + return i2c_add_driver(&max664x_driver); > +} > + > +static void __exit max664x_exit(void) > +{ > + i2c_del_driver(&max664x_driver); > +} > + > +MODULE_AUTHOR("Solarflare Communications"); > +MODULE_DESCRIPTION("MAX6646/6647/6649 driver"); > +MODULE_LICENSE("GPL v2"); > + > +module_init(max664x_init); > +module_exit(max664x_exit); > diff --git a/include/linux/max664x.h b/include/linux/max664x.h > new file mode 100644 > index 0000000..0b05903 > --- /dev/null > +++ b/include/linux/max664x.h > @@ -0,0 +1,92 @@ > +/**************************************************************************** > + * Interface to max664x driver > + * Copyright 2008 Solarflare Communications Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published > + * by the Free Software Foundation, incorporated herein by reference. > + */ > + > +#ifndef _MAX664X_H_ > +#define _MAX664X_H_ > + > +#ifdef __KERNEL__ > + > +#include <linux/types.h> > +#include <linux/i2c.h> > + > +/* Status register */ > +#define MAX664X_STATUS_IOT (1 << 0) > +#define MAX664X_STATUS_EOT (1 << 1) > +#define MAX664X_STATUS_FAULT (1 << 2) > +#define MAX664X_STATUS_RLOW (1 << 3) > +#define MAX664X_STATUS_RHIGH (1 << 4) > +#define MAX664X_STATUS_LLOW (1 << 5) > +#define MAX664X_STATUS_LHIGH (1 << 6) > +#define MAX664X_STATUS_BUSY (1 << 7) > + > +#define MAX664X_TEMP_INT 0 > +#define MAX664X_TEMP_EXT 1 Nice defines... but not used in your driver. This raises the question of how useful they are. Not much IMHO, as each channel can be used for something different depending on how the chip is used. > + > +/** > + * struct max664x_settings - settings for MAX6646/6647/6649 hardware monitor > + * @rate: Conversion rate setting > + * @temp_overt: High temperature to set OVERT > + * @temp_high_alert: High temperature to set ALERT > + * @temp_low_alert: Low temperature to set ALERT > + * @temp_overt_hyst: Hysteresis for resetting OVERT > + * > + * All values are raw register values. ... which doesn't sound good (see below.) > + */ > +struct max664x_settings { > + u8 rate; > + u8 temp_overt[2]; > + u8 temp_high_alert[2]; > + u8 temp_low_alert[2]; > + u8 temp_overt_hyst; > +}; > + > +/** > + * struct max664x_values - values from MAX6646/6647/6649 hardware monitor > + * @temp: Current temperature > + * > + * All values are raw register values. > + */ > +struct max664x_values { > + u8 temp[2]; > +}; > + > +/** > + * struct max664x_platform_data - platform data for MAX6646/6647/6649 hardware monitor > + * @set_rate: Flag for whether to set rate register > + * @set_limits: Flag for whether to set limit registers > + * @set: Settings to be applied depending on the above flags > + * > + * This platform data is optional. If not provided, the driver will > + * assume that the MAX6646/6647/6649 was properly configured by firmware > + * or the power-on-reset defaults. > + */ > +struct max664x_platform_data { > + unsigned set_rate : 1; > + unsigned set_limits : 1; > + struct max664x_settings set; > +}; > + > +/** > + * max664x_update_values() - update and return current values > + * @client: I2C client to which the max664x driver is bound > + */ > +struct max664x_values *max664x_update_values(struct i2c_client *client); I think you should return a const pointer. The caller really shouldn't be allowed to change the values. > + > +/** > + * max664x_read_status() - read status register > + * @client: I2C client to which the max664x driver is bound > + * > + * Read the status register, which automatically clears any stuck alarm > + * bits. Why "stuck"? They are simply set. > + */ > +int max664x_read_status(struct i2c_client *client); > + > +#endif > + > +#endif Please add comments on what each #endif closes. Note that I don't expect this API you just defined to live long. It's completely hardware-specific and will show up its limitations quickly as more drivers will offer similar interfaces, and I suspect the users will ask for some level of abstraction. We have a nice, standard sysfs interface by now, so going back to hardware-specific definitions on the kernel side sounds pretty wrong. That being said, I do not have the time to work on such a standard specification at the moment, nor do I think that it makes sense to specify anything until we have at least 3 drivers offering a similar interface and 3 users thereof. So I am fine with taking your API for now, as long as you realize that it is very likely temporary only. -- Jean Delvare
