| Message ID | f358329ff1dd3c3c272cadb4a358a5587cb28e18.1374093761.git.luto@amacapital.net |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jul 17, 2013 at 01:53:07PM -0700, Andy Lutomirski wrote: > Signed-off-by: Andy Lutomirski <luto@amacapital.net> > --- Why don't you just use the existing jc42 driver ? Guenter > drivers/hwmon/Kconfig | 10 +++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/tsod.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 206 insertions(+) > create mode 100644 drivers/hwmon/tsod.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 89ac1cb..96edb87 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1462,6 +1462,16 @@ config SENSORS_MC13783_ADC > help > Support for the A/D converter on MC13783 and MC13892 PMIC. > > +config SENSORS_TSOD > + tristate "Temperature Sensor On DIMM (TSOD)" > + depends on I2C > + help > + If you say yes here you get support for the integrated temperature > + sensors on newer DIMMs that comply with JESD21-C. > + > + This driver can also be built as a module. If so, the module > + will be called tsod. > + > if ACPI > > comment "ACPI drivers" > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 8d6d97e..439ef1f 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -134,6 +134,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o > obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o > obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o > obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o > +obj-$(CONFIG_SENSORS_TSOD) += tsod.o > > obj-$(CONFIG_PMBUS) += pmbus/ > > diff --git a/drivers/hwmon/tsod.c b/drivers/hwmon/tsod.c > new file mode 100644 > index 0000000..f7bb070 > --- /dev/null > +++ b/drivers/hwmon/tsod.c > @@ -0,0 +1,195 @@ > +/* > + * drivers/hwmon/tsod.c - Temperaure Sensor On DIMM > + * > + * Copyright (C) 2013 Andrew Lutomirski <luto@amacapital.net> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License v2 as published by the > + * Free Software Foundation. > + * > + * 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. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA > + * > + * The official reference for these devices is JEDEC Standard No. 21-C, > + * which is available for free from www.jedec.org. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/slab.h> > + > +/* Registers */ > +#define TSOD_CURRENT_TEMP 5 > +#define TSOD_VENDOR 6 > +#define TSOD_DEVICE 7 > + > +/* > + * This driver does not program the trip points, etc. -- this is done by > + * firmware, and the memory controller probably wants the defaults preserved. > + */ > + > +struct tsod_priv { > + struct i2c_client *client; > + struct device *hwmondev; > +}; > + > +static ssize_t show_name(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "TSOD\n"); > +} > + > +static ssize_t show_label(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "DIMM Temperature\n"); > +} > + > +static ssize_t show_temperature(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct tsod_priv *priv = dev_get_drvdata(dev->parent); > + int temp, raw; > + > + raw = i2c_smbus_read_word_swapped(priv->client, TSOD_CURRENT_TEMP); > + if (raw < 0) > + return raw; > + > + /* > + * The three high bits are undefined and the rest is twos-complement. > + * Use a sign-extending right shift to propagate the sign bit. > + */ > + temp = ((s16)((s16)raw << 3) >> 3); > + > + /* > + * The value is in units of 0.0625 degrees, but we want it in > + * units of 0.001 degrees. > + */ > + return sprintf(buf, "%d\n", DIV_ROUND_CLOSEST(temp * 625, 10)); > +} > + > +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL); > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temperature, NULL, 0); > +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL, 0); > + > +static struct attribute *tsod_hwmon_attributes[] = { > + &dev_attr_name.attr, > + &sensor_dev_attr_temp1_input.dev_attr.attr, > + &sensor_dev_attr_temp1_label.dev_attr.attr, > + > + NULL, > +}; > + > +static const struct attribute_group tsod_hwmon_attr_group = { > + .attrs = tsod_hwmon_attributes, > +}; > + > +static int tsod_detect(struct i2c_client *client, struct i2c_board_info *info) > +{ > + struct i2c_adapter *adapter = client->adapter; > + > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_READ_WORD_DATA)) > + return -ENODEV; > + > + strlcpy(info->type, "tsod", I2C_NAME_SIZE); > + return 0; > +} > + > +static int tsod_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int ret; > + struct tsod_priv *priv; > + > + /* Sanity check the address */ > + if ((client->addr & 0x78) != 0x18) > + return -ENODEV; > + > + /* Sanity check: make sure we can read the temperature. */ > + ret = i2c_smbus_read_word_swapped(client, TSOD_CURRENT_TEMP); > + if (ret < 0) > + return -ENODEV; > + > + priv = kzalloc(sizeof(struct tsod_priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->client = client; > + > + priv->hwmondev = hwmon_device_register(&client->dev); > + if (IS_ERR(priv->hwmondev)) { > + ret = PTR_ERR(priv->hwmondev); > + goto err_free; > + } > + > + i2c_set_clientdata(client, priv); > + > + ret = sysfs_create_group(&priv->hwmondev->kobj, &tsod_hwmon_attr_group); > + if (ret) > + goto err_unreg; > + > + return 0; > + > +err_unreg: > + hwmon_device_unregister(&client->dev); > + > +err_free: > + kfree(priv); > + i2c_set_clientdata(client, 0); > + return ret; > +} > + > +static int tsod_remove(struct i2c_client *client) > +{ > + struct tsod_priv *priv = i2c_get_clientdata(client); > + > + sysfs_remove_group(&priv->hwmondev->kobj, &tsod_hwmon_attr_group); > + hwmon_device_unregister(priv->hwmondev); > + kfree(priv); > + return 0; > +} > + > +static const unsigned short tsod_addresses[] = { > + 0x18, 0x19, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, I2C_CLIENT_END > +}; > + > +static const struct i2c_device_id tsod_id[] = { > + { "tsod", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, tsod_id); > + > +static struct i2c_driver tsod_driver = { > + .driver = { > + .name = "tsod", > + .owner = THIS_MODULE, > + }, > + .probe = tsod_probe, > + .remove = tsod_remove, > + .id_table = tsod_id, > + > + /* > + * We do not claim I2C_CLASS_SPD -- there are other devices > + * on, e.g., the i2c_i801 bus that have these addresses. > + * Instead we let the dimm-bus code instantiate us. > + */ > + > + .detect = tsod_detect, > + .address_list = tsod_addresses, > +}; > + > +module_i2c_driver(tsod_driver); > + > +MODULE_AUTHOR("Andrew Lutomirski <luto@amacapital.net>"); > +MODULE_DESCRIPTION("Temperaure Sensor On DIMM"); > +MODULE_LICENSE("GPL"); > -- > 1.8.1.4 > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 17, 2013 at 3:19 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Wed, Jul 17, 2013 at 01:53:07PM -0700, Andy Lutomirski wrote: >> Signed-off-by: Andy Lutomirski <luto@amacapital.net> >> --- > > Why don't you just use the existing jc42 driver ? Failure to notice it. *sigh*. I assumed that any driver for this device would have "tsod" or "TSOD" somewhere greppable. I'll at least send a patch to improve the description. That being said, the jc42 driver *writes*. The iMC controller (AFAICS) uses the data from the TSOD for things like dynamically adjusting the DRAM refresh interval and IMO the kernel has no business at all writing to any registers on the TSOD on this platform. (FWIW, I tried fiddling with the critical threshold and my box didn't blow up or report thermal trip events. But still, this scares me a bit.) If I modify the i2c_imc driver to add .class = I2C_CLASS_SPD and get rid of the TSOD probing and load the jc42 module, then it can't actually find my TSODs (because the i2c core isn't capable of probing on the iMC bus). If I manually add the device, it works. But I'd like to keep the enumeration code I wrote around, since the main point of this exercise is to enumerate things that aren't actually hwmon devices. But I don't want random sensors scripts reprogramming the hardware. Is the some magic I can work (or should add) with i2c_board_info to force the jc42 driver into a read-only mode? In any case, am I doing the right thing by not setting I2C_CLASS_SPD on my bus? --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 17, 2013 at 03:49:24PM -0700, Andy Lutomirski wrote: > On Wed, Jul 17, 2013 at 3:19 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Jul 17, 2013 at 01:53:07PM -0700, Andy Lutomirski wrote: > >> Signed-off-by: Andy Lutomirski <luto@amacapital.net> > >> --- > > > > Why don't you just use the existing jc42 driver ? > > Failure to notice it. *sigh*. I assumed that any driver for this > device would have "tsod" or "TSOD" somewhere greppable. I'll at least > send a patch to improve the description. > > That being said, the jc42 driver *writes*. The iMC controller > (AFAICS) uses the data from the TSOD for things like dynamically > adjusting the DRAM refresh interval and IMO the kernel has no business > at all writing to any registers on the TSOD on this platform. (FWIW, > I tried fiddling with the critical threshold and my box didn't blow up > or report thermal trip events. But still, this scares me a bit.) > I don't see a problem with that. If the i2c controller supports writes, one can perform the write from user space with i2cset, so what is the problem ? Besides, jc42 compliant temperature sensors expect to be written to in order to set the temperature limits. If you don't like that, you don't have to do it. > If I modify the i2c_imc driver to add .class = I2C_CLASS_SPD and get > rid of the TSOD probing and load the jc42 module, then it can't > actually find my TSODs (because the i2c core isn't capable of probing > on the iMC bus). If I manually add the device, it works. > You mean it doesn't support the SMBus quick command ? Did you set I2C_CLASS_HWMON as well, or just I2C_CLASS_SPD ? If you have the ability to use i2c_board_info, the failure to auto-detect the devices should not really matter. In that case you can just use one of the other methods to instantiate the i2c devices. > But I'd like to keep the enumeration code I wrote around, since the > main point of this exercise is to enumerate things that aren't > actually hwmon devices. But I don't want random sensors scripts > reprogramming the hardware. Is the some magic I can work (or should > add) with i2c_board_info to force the jc42 driver into a read-only > mode? > Again, that is user space code doing the write, which you can control without reverting to a "private" driver. That the driver permits the write doesn't mean you have to use it. The only write it always performs is to enable the sensor if it is disabled .. which presumably should be ok even for you, since otherwise loading the driver doesn't make any sense in the first place. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 17, 2013 at 4:09 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Wed, Jul 17, 2013 at 03:49:24PM -0700, Andy Lutomirski wrote: >> On Wed, Jul 17, 2013 at 3:19 PM, Guenter Roeck <linux@roeck-us.net> wrote: >> > On Wed, Jul 17, 2013 at 01:53:07PM -0700, Andy Lutomirski wrote: >> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net> >> >> --- >> > >> > Why don't you just use the existing jc42 driver ? >> >> Failure to notice it. *sigh*. I assumed that any driver for this >> device would have "tsod" or "TSOD" somewhere greppable. I'll at least >> send a patch to improve the description. >> >> That being said, the jc42 driver *writes*. The iMC controller >> (AFAICS) uses the data from the TSOD for things like dynamically >> adjusting the DRAM refresh interval and IMO the kernel has no business >> at all writing to any registers on the TSOD on this platform. (FWIW, >> I tried fiddling with the critical threshold and my box didn't blow up >> or report thermal trip events. But still, this scares me a bit.) >> > I don't see a problem with that. If the i2c controller supports writes, one can > perform the write from user space with i2cset, so what is the problem ? Besides, > jc42 compliant temperature sensors expect to be written to in order to set the > temperature limits. If you don't like that, you don't have to do it. Fair enough. > >> If I modify the i2c_imc driver to add .class = I2C_CLASS_SPD and get >> rid of the TSOD probing and load the jc42 module, then it can't >> actually find my TSODs (because the i2c core isn't capable of probing >> on the iMC bus). If I manually add the device, it works. >> > You mean it doesn't support the SMBus quick command ? Did you set > I2C_CLASS_HWMON as well, or just I2C_CLASS_SPD ? I didn't set I2C_CLASS_HWMON (and I won't -- jc42 is the only hwmon driver that should ever probe this bus). The adapter only supports read byte/word data, write byte/word data, and write byte. (I'm pretty sure that the hardware is capable of initiating a read byte transaction, but not under software control. I have no idea why write byte is there without read byte being available, so I didn't implement it. I think it's so that you can reset the pointer register after reading something else, but this is pointless because you don't know what to reset it to. This hardware is not exactly a thing of beauty, but I'm stuck with it.) > > If you have the ability to use i2c_board_info, the failure to auto-detect the > devices should not really matter. In that case you can just use one of the other > methods to instantiate the i2c devices. See my other post about dimm_bus. > >> But I'd like to keep the enumeration code I wrote around, since the >> main point of this exercise is to enumerate things that aren't >> actually hwmon devices. But I don't want random sensors scripts >> reprogramming the hardware. Is the some magic I can work (or should >> add) with i2c_board_info to force the jc42 driver into a read-only >> mode? >> > Again, that is user space code doing the write, which you can control without > reverting to a "private" driver. That the driver permits the write doesn't mean > you have to use it. The only write it always performs is to enable the sensor if > it is disabled .. which presumably should be ok even for you, since otherwise > loading the driver doesn't make any sense in the first place. > OK, I'm convinced. I'll scrap my tsod driver. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 89ac1cb..96edb87 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1462,6 +1462,16 @@ config SENSORS_MC13783_ADC help Support for the A/D converter on MC13783 and MC13892 PMIC. +config SENSORS_TSOD + tristate "Temperature Sensor On DIMM (TSOD)" + depends on I2C + help + If you say yes here you get support for the integrated temperature + sensors on newer DIMMs that comply with JESD21-C. + + This driver can also be built as a module. If so, the module + will be called tsod. + if ACPI comment "ACPI drivers" diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 8d6d97e..439ef1f 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -134,6 +134,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o +obj-$(CONFIG_SENSORS_TSOD) += tsod.o obj-$(CONFIG_PMBUS) += pmbus/ diff --git a/drivers/hwmon/tsod.c b/drivers/hwmon/tsod.c new file mode 100644 index 0000000..f7bb070 --- /dev/null +++ b/drivers/hwmon/tsod.c @@ -0,0 +1,195 @@ +/* + * drivers/hwmon/tsod.c - Temperaure Sensor On DIMM + * + * Copyright (C) 2013 Andrew Lutomirski <luto@amacapital.net> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License v2 as published by the + * Free Software Foundation. + * + * 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. + * + * You should have received a copy of the GNU General Public License along with + * this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + * + * The official reference for these devices is JEDEC Standard No. 21-C, + * which is available for free from www.jedec.org. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/slab.h> + +/* Registers */ +#define TSOD_CURRENT_TEMP 5 +#define TSOD_VENDOR 6 +#define TSOD_DEVICE 7 + +/* + * This driver does not program the trip points, etc. -- this is done by + * firmware, and the memory controller probably wants the defaults preserved. + */ + +struct tsod_priv { + struct i2c_client *client; + struct device *hwmondev; +}; + +static ssize_t show_name(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "TSOD\n"); +} + +static ssize_t show_label(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "DIMM Temperature\n"); +} + +static ssize_t show_temperature(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct tsod_priv *priv = dev_get_drvdata(dev->parent); + int temp, raw; + + raw = i2c_smbus_read_word_swapped(priv->client, TSOD_CURRENT_TEMP); + if (raw < 0) + return raw; + + /* + * The three high bits are undefined and the rest is twos-complement. + * Use a sign-extending right shift to propagate the sign bit. + */ + temp = ((s16)((s16)raw << 3) >> 3); + + /* + * The value is in units of 0.0625 degrees, but we want it in + * units of 0.001 degrees. + */ + return sprintf(buf, "%d\n", DIV_ROUND_CLOSEST(temp * 625, 10)); +} + +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL); +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temperature, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL, 0); + +static struct attribute *tsod_hwmon_attributes[] = { + &dev_attr_name.attr, + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_temp1_label.dev_attr.attr, + + NULL, +}; + +static const struct attribute_group tsod_hwmon_attr_group = { + .attrs = tsod_hwmon_attributes, +}; + +static int tsod_detect(struct i2c_client *client, struct i2c_board_info *info) +{ + struct i2c_adapter *adapter = client->adapter; + + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_READ_WORD_DATA)) + return -ENODEV; + + strlcpy(info->type, "tsod", I2C_NAME_SIZE); + return 0; +} + +static int tsod_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int ret; + struct tsod_priv *priv; + + /* Sanity check the address */ + if ((client->addr & 0x78) != 0x18) + return -ENODEV; + + /* Sanity check: make sure we can read the temperature. */ + ret = i2c_smbus_read_word_swapped(client, TSOD_CURRENT_TEMP); + if (ret < 0) + return -ENODEV; + + priv = kzalloc(sizeof(struct tsod_priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->client = client; + + priv->hwmondev = hwmon_device_register(&client->dev); + if (IS_ERR(priv->hwmondev)) { + ret = PTR_ERR(priv->hwmondev); + goto err_free; + } + + i2c_set_clientdata(client, priv); + + ret = sysfs_create_group(&priv->hwmondev->kobj, &tsod_hwmon_attr_group); + if (ret) + goto err_unreg; + + return 0; + +err_unreg: + hwmon_device_unregister(&client->dev); + +err_free: + kfree(priv); + i2c_set_clientdata(client, 0); + return ret; +} + +static int tsod_remove(struct i2c_client *client) +{ + struct tsod_priv *priv = i2c_get_clientdata(client); + + sysfs_remove_group(&priv->hwmondev->kobj, &tsod_hwmon_attr_group); + hwmon_device_unregister(priv->hwmondev); + kfree(priv); + return 0; +} + +static const unsigned short tsod_addresses[] = { + 0x18, 0x19, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, I2C_CLIENT_END +}; + +static const struct i2c_device_id tsod_id[] = { + { "tsod", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, tsod_id); + +static struct i2c_driver tsod_driver = { + .driver = { + .name = "tsod", + .owner = THIS_MODULE, + }, + .probe = tsod_probe, + .remove = tsod_remove, + .id_table = tsod_id, + + /* + * We do not claim I2C_CLASS_SPD -- there are other devices + * on, e.g., the i2c_i801 bus that have these addresses. + * Instead we let the dimm-bus code instantiate us. + */ + + .detect = tsod_detect, + .address_list = tsod_addresses, +}; + +module_i2c_driver(tsod_driver); + +MODULE_AUTHOR("Andrew Lutomirski <luto@amacapital.net>"); +MODULE_DESCRIPTION("Temperaure Sensor On DIMM"); +MODULE_LICENSE("GPL");
Signed-off-by: Andy Lutomirski <luto@amacapital.net> --- drivers/hwmon/Kconfig | 10 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/tsod.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 206 insertions(+) create mode 100644 drivers/hwmon/tsod.c