Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

[Jean Delvare <khali@linux-fr.org>]

Hi Alan,

On Mon, 26 Apr 2010 11:09:13 +0100, Alan Cox wrote:
> > > It turns the device on or off. I can't find an obvious general hwmon
> > > path for this ?
> > 
> > No, we don't have any, this doesn't belong to the hwmon framework,
> > this should be common to all devices and handled by the power
> > subsystem.
> 
> Nice in theory but in some cases we do actually want to be able to power
> up/down individual sensors and it's not therefore a matter of a
> suspend/resume method.

I have no problem with individual power up/down of devices. But this
doesn't make it a hwmon-specific requirement, so I insist that it
doesn't belong to the hwmon framework.

> > > any follow on stuff where the chip is being used for thermal
> > > management of an x86 SoC platform (non PC)
> > 
> > Does it really have to be a user-space controllable setting in sysfs?
> 
> Probably it needs to be user settable, but equally that I imagine will
> belong in the monitor/power logic for the platform not the generic
> stuff.
> 
> > All other drivers use one mutex per device. I don't quite see how
> > overkill this is, and it's difficult to justify blocking access to one
> > I2C device because another one is in use. They may be on different
> > buses.
> 
> If you prefer.
> 
> 
> 
> > No store_hyst()? Note that the hysteresis being a shared register, the
> > traditional way to handle it is to make the sysfs attribute writable
> > only for one of the temperature channels, and read-only for the
> > remainder.
> 
> Done
> 
> 
> Other bits done.. I'll try and get it tested but my dev board is being
> recalcitrant right now.

Almost alright now, see my few remaining comments below.

> 
> --
> 
> commit ae8a34b81f05836db754aa9feb318a443d9fd5fa
> Author: Kalhan Trisal <kalhan.trisal at intel.com>
> Date:   Mon Apr 26 11:08:38 2010 +0100
> 
>     emc1403: thermal sensor support
>     
>     Provides support for the emc1403 thermal sensor. Only reporting of values
>     is supported. The various Moorestown specific extras to do with thermal
>     alerts and the like are not in this version of the driver.
> 
>     (Much clean up by Alan Cox)
>     
>     Signed-off-by: Kalhan Trisal <kalhan.trisal at intel.com>
>     Signed-off-by: Alan Cox <alan@linux.intel.com>
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d38447f..0bd3206 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -750,6 +750,16 @@ config SENSORS_DME1737
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called dme1737.
>  
> +config SENSORS_EMC1403
> +	tristate "SMSC EMC1403 thermal sensor"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for the SMSC EMC1403
> +	  temperature monitoring chip.
> +
> +	  Threshold values can be configured using sysfs.
> +	  Data from the different diodes are accessible via sysfs.
> +
>  config SENSORS_SMSC47M1
>  	tristate "SMSC LPC47M10x and compatibles"
>  	help
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 4aa1a3d..1e6b083 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
>  obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
>  obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
> +obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
>  obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
>  obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
>  obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
> diff --git a/drivers/hwmon/emc1403.c b/drivers/hwmon/emc1403.c
> new file mode 100644
> index 0000000..3e50325
> --- /dev/null
> +++ b/drivers/hwmon/emc1403.c
> @@ -0,0 +1,366 @@
> +/*
> + * emc1403.c - SMSC Thermal Driver
> + *
> + * Copyright (C) 2008 Intel Corp
> + *
> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * 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 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.,
> + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/sysfs.h>
> +#include <linux/mutex.h>
> +
> +#define THERMAL_PID_REG		0xfd
> +#define THERMAL_SMSC_ID_REG	0xfe
> +#define THERMAL_REVISION_REG	0xff
> +
> +struct thermal_data {
> +	struct device *hwmon_dev;
> +	struct mutex mutex;
> +	/* Cache the hyst value so we don't keep re-reading it. In theory
> +	   we could cache it forever as nobody else should be writing it. */
> +	u8 cached_hyst;
> +	unsigned long hyst_valid;

This is good, but might need to be extended to other registers. In
particular the alarm registers are read 3 times, and the critical limit
registers are read twice. This can be left for later though if you
don't feel like implementing it now.

> +};
> +
> +static ssize_t show_temp(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
> +	int retval = i2c_smbus_read_byte_data(client, sda->index);
> +
> +	if (retval < 0)
> +		return retval;
> +	return sprintf(buf, "%d000\n", retval);
> +}
> +
> +static ssize_t show_bit(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sensor_device_attribute_2 *sda = to_sensor_dev_attr_2(attr);
> +	int retval = i2c_smbus_read_byte_data(client, sda->nr);
> +
> +	if (retval < 0)
> +		return retval;
> +	retval &= sda->index;
> +	return sprintf(buf, "%d\n", retval ? 1 : 0);
> +}
> +
> +static ssize_t store_temp(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned long val;
> +	int retval;
> +
> +	if (strict_strtoul(buf, 10, &val))
> +		return -EINVAL;
> +	retval = i2c_smbus_write_byte_data(client, sda->index,
> +					DIV_ROUND_CLOSEST(val, 1000));
> +	if (retval < 0)
> +		return retval;
> +	return count;
> +}
> +
> +static ssize_t store_bit(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct thermal_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute_2 *sda = to_sensor_dev_attr_2(attr);
> +	unsigned long val;
> +	int retval;
> +
> +	if (strict_strtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->mutex);
> +	retval = i2c_smbus_read_byte_data(client, sda->nr);
> +	if (retval < 0)
> +		goto fail;
> +
> +	retval &= sda->index;

Missing negation?

> +	if (val)
> +		retval |= sda->index;
> +
> +	retval = i2c_smbus_write_byte_data(client, sda->index, retval);
> +	if (retval = 0)
> +		retval = count;
> +fail:
> +	mutex_unlock(&data->mutex);
> +	return retval;
> +}
> +
> +static ssize_t show_hyst(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct thermal_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
> +	int retval;
> +	int hyst;
> +
> +	retval = i2c_smbus_read_byte_data(client, sda->index);
> +	if (retval < 0)
> +		return retval;
> +
> +	if (time_after(jiffies, data->hyst_valid)) {
> +		hyst = i2c_smbus_read_byte_data(client, 0x21);
> +		if (hyst < 0)
> +			return retval;
> +		data->cached_hyst = hyst;
> +		data->hyst_valid = jiffies + HZ;
> +	}
> +	return sprintf(buf, "%d000\n", retval - data->cached_hyst);
> +}
> +
> +static ssize_t store_hyst(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct thermal_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
> +	int retval;
> +	int hyst;
> +	unsigned long val;
> +
> +	if (strict_strtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->mutex);
> +	retval = i2c_smbus_read_byte_data(client, sda->index);
> +	if (retval < 0)
> +		goto fail;
> +
> +	hyst = val - retval * 1000;
> +	hyst = DIV_ROUND_CLOSEST(hyst, 1000);
> +	if (hyst < 0 || hyst > 255) {
> +		retval = -ERANGE;
> +		goto fail;
> +	}
> +
> +	retval = i2c_smbus_write_byte_data(client, 0x21, hyst);
> +	if (retval = 0) {
> +		retval = count;
> +		data->cached_hyst = hyst;
> +		data->hyst_valid = jiffies + HZ;
> +	}
> +fail:
> +	mutex_unlock(&data->mutex);
> +	return retval;
> +}
> +
> +/*
> + *	Sensors. We pass the actual i2c register to the methods.
> + */
> +
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x06);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x05);
> +static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x20);
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0x00);
> +static SENSOR_DEVICE_ATTR_2(temp1_min_alarm, S_IRUGO,
> +	show_bit, NULL, 0x36, 0x01);
> +static SENSOR_DEVICE_ATTR_2(temp1_max_alarm, S_IRUGO,
> +	show_bit, NULL, 0x35, 0x01);
> +static SENSOR_DEVICE_ATTR_2(temp1_crit_alarm, S_IRUGO,
> +	show_bit, NULL, 0x37, 0x01);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IRUGO | S_IWUSR,
> +	show_hyst, store_hyst, 0x20);
> +
> +static SENSOR_DEVICE_ATTR(temp2_min, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x08);
> +static SENSOR_DEVICE_ATTR(temp2_max, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x07);
> +static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x19);
> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 0x01);
> +static SENSOR_DEVICE_ATTR_2(temp2_min_alarm, S_IRUGO,
> +	show_bit, NULL, 0x36, 0x02);
> +static SENSOR_DEVICE_ATTR_2(temp2_max_alarm, S_IRUGO,
> +	show_bit, NULL, 0x35, 0x02);
> +static SENSOR_DEVICE_ATTR_2(temp2_crit_alarm, S_IRUGO,
> +	show_bit, NULL, 0x37, 0x02);
> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO | S_IWUSR,
> +	show_hyst, store_hyst, 0x19);
> +
> +static SENSOR_DEVICE_ATTR(temp3_min, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x16);
> +static SENSOR_DEVICE_ATTR(temp3_max, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x15);
> +static SENSOR_DEVICE_ATTR(temp3_crit, S_IRUGO | S_IWUSR,
> +	show_temp, store_temp, 0x1A);
> +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp, NULL, 0x23);
> +static SENSOR_DEVICE_ATTR_2(temp3_min_alarm, S_IRUGO,
> +	show_bit, NULL, 0x36, 0x04);
> +static SENSOR_DEVICE_ATTR_2(temp3_max_alarm, S_IRUGO,
> +	show_bit, NULL, 0x35, 0x04);
> +static SENSOR_DEVICE_ATTR_2(temp3_crit_alarm, S_IRUGO,
> +	show_bit, NULL, 0x37, 0x04);
> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO | S_IWUSR,
> +	show_hyst, store_hyst, 0x1A);
> +
> +static SENSOR_DEVICE_ATTR_2(power_state, S_IRUGO | S_IWUSR,
> +	show_bit, store_bit, 0x03, 0x40);
> +
> +static struct attribute *mid_att_thermal[] = {
> +	&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_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp2_min.dev_attr.attr,
> +	&sensor_dev_attr_temp2_max.dev_attr.attr,
> +	&sensor_dev_attr_temp2_crit.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp3_min.dev_attr.attr,
> +	&sensor_dev_attr_temp3_max.dev_attr.attr,
> +	&sensor_dev_attr_temp3_crit.dev_attr.attr,
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
> +	&sensor_dev_attr_power_state.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group m_thermal_gr = {
> +	.attrs = mid_att_thermal
> +};
> +
> +static int emc1403_detect(struct i2c_client *client,
> +			struct i2c_board_info *info)
> +{
> +	int id;
> +	/* Check if thermal chip is SMSC and EMC1403 */
> +
> +	id = i2c_smbus_read_byte_data(client, THERMAL_SMSC_ID_REG);
> +	if (id != 0x5d)
> +		return -ENODEV;
> +
> +	/* Note: 0x25 is the 1404 which is very similar and this
> +	   driver could be extended */
> +	id = i2c_smbus_read_byte_data(client, THERMAL_PID_REG);
> +	if (id != 0x21)
> +		return -ENODEV;
> +
> +	id = i2c_smbus_read_byte_data(client, THERMAL_REVISION_REG);
> +	if (id != 0x01)
> +		return -ENODEV;
> +
> +	strlcpy(info->type, "emc1403", I2C_NAME_SIZE);
> +	return 0;
> +}
> +
> +static int emc1403_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	int res;
> +	struct thermal_data *data;
> +
> +	data = kzalloc(sizeof(struct thermal_data), GFP_KERNEL);
> +	if (data = NULL) {
> +		dev_warn(&client->dev, "out of memory");
> +		return -ENOMEM;
> +	}
> +
> +	i2c_set_clientdata(client, data);
> +	mutex_init(&data->mutex);
> +	data->hyst_valid = jiffies - 1;		/* Expired */
> +
> +	res = sysfs_create_group(&client->dev.kobj, &m_thermal_gr);
> +	if (res) {
> +		dev_warn(&client->dev, "create group failed\n");
> +		hwmon_device_unregister(data->hwmon_dev);
> +		goto thermal_error1;
> +	}
> +	data->hwmon_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		res = PTR_ERR(data->hwmon_dev);
> +		dev_warn(&client->dev, "register hwmon dev failed\n");
> +		goto thermal_error2;
> +	}
> +	dev_info(&client->dev, "EMC1403 Thermal chip found\n");
> +	return res;

An extra blank line here would be appreciated.

> +thermal_error2:
> +	sysfs_remove_group(&client->dev.kobj, &m_thermal_gr);
> +thermal_error1:
> +	kfree(data);
> +	return res;
> +}
> +
> +static int emc1403_remove(struct i2c_client *client)
> +{
> +	struct thermal_data *data = i2c_get_clientdata(client);
> +
> +	hwmon_device_unregister(data->hwmon_dev);
> +	sysfs_remove_group(&client->dev.kobj, &m_thermal_gr);
> +	kfree(data);
> +	return 0;
> +}
> +
> +const static struct i2c_device_id emc1403_idtable[] = {
> +	{ "emc1403", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, emc1403_idtable);
> +
> +static struct i2c_driver sensor_emc1403 = {
> +	.driver = {
> +		.name = "emc1403",
> +	},
> +	.detect = emc1403_detect,
> +	.probe = emc1403_probe,
> +	.remove = emc1403_remove,
> +	.id_table = emc1403_idtable,
> +};
> +
> +static int __init sensor_emc1403_init(void)
> +{
> +	return i2c_add_driver(&sensor_emc1403);
> +}
> +
> +static void  __exit sensor_emc1403_exit(void)
> +{
> +	i2c_del_driver(&sensor_emc1403);
> +}
> +
> +module_init(sensor_emc1403_init);
> +module_exit(sensor_emc1403_exit);
> +
> +MODULE_AUTHOR("Kalhan Trisal <kalhan.trisal@intel.com");
> +MODULE_DESCRIPTION("emc1403 Thermal Driver");
> +MODULE_LICENSE("GPL v2");

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors