Re: [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Jan 24, 2017 at 04:02:20PM +0100, Andrea Merello wrote:
> This patch adds a HWMON driver for ST Microelectronics STTS751
> temperature sensors.
> 
> It does support manual-triggered conversions as well as automatic
> conversions. The latter is used when the "event" or "therm" function
> is present (declaring the physical wire is attached in the DT).
> 
> Thanks-to: LABBE Corentin [for suggestions]
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> ---

No change log, meaning we have to review the entire driver again or look up
older versions and comments ourselves. This will take a while. For now, just a
few quick commnents. For the future, please consider providing a changelog.

>  drivers/hwmon/Kconfig   |  10 +
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/stts751.c | 809 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 820 insertions(+)
>  create mode 100644 drivers/hwmon/stts751.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d1f82f2..8fdd241 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called sch5636.
>  
> +config SENSORS_STTS751
> +	tristate "ST Microelectronics STTS751"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for STTS751
> +	  temperature sensor chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called stts751.
> +
>  config SENSORS_SMM665
>  	tristate "Summit Microelectronics SMM665"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 7476b22..1114130 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>  obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> +obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
>  obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
>  obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
> new file mode 100644
> index 0000000..0d4716f
> --- /dev/null
> +++ b/drivers/hwmon/stts751.c
> @@ -0,0 +1,809 @@
> +/*
> + * STTS751 sensor driver
> + *
> + * Copyright (C) 2016-2017 Istituto Italiano di Tecnologia - RBCS - EDL
> + * Robotics, Brain and Cognitive Sciences department
> + * Electronic Design Laboratory
> + *
> + * Written by Andrea Merello <andrea.merello@gmail.com>
> + *
> + * Based on  LM95241 driver and LM90 driver
> + *
> + * 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/bitops.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/util_macros.h>
> +
> +#define DEVNAME "stts751"
> +
> +static const unsigned short normal_i2c[] = {
> +	0x48, 0x49, 0x38, 0x39,  /* STTS751-0 */
> +	0x4A, 0x4B, 0x3A, 0x3B,  /* STTS751-1 */
> +	I2C_CLIENT_END };
> +
> +#define STTS751_REG_TEMP_H	0x00
> +#define STTS751_REG_STATUS	0x01
> +#define STTS751_STATUS_TRIPT	BIT(0)
> +#define STTS751_STATUS_TRIPL	BIT(5)
> +#define STTS751_STATUS_TRIPH	BIT(6)
> +#define STTS751_REG_TEMP_L	0x02
> +#define STTS751_REG_CONF	0x03
> +#define STTS751_CONF_RES_MASK	0x0C
> +#define STTS751_CONF_RES_SHIFT  2
> +#define STTS751_CONF_EVENT_DIS  BIT(7)
> +#define STTS751_CONF_STOP	BIT(6)
> +#define STTS751_REG_RATE	0x04
> +#define STTS751_REG_HLIM_H	0x05
> +#define STTS751_REG_HLIM_L	0x06
> +#define STTS751_REG_LLIM_H	0x07
> +#define STTS751_REG_LLIM_L	0x08
> +#define STTS751_REG_TLIM	0x20
> +#define STTS751_REG_HYST	0x21
> +#define STTS751_REG_SMBUS_TO	0x22
> +
> +#define STTS751_REG_PROD_ID	0xFD
> +#define STTS751_REG_MAN_ID	0xFE
> +#define STTS751_REG_REV_ID	0xFF
> +
> +#define STTS751_0_PROD_ID	0x00
> +#define STTS751_1_PROD_ID	0x01
> +#define ST_MAN_ID		0x53
> +
> +/*
> + * Possible update intervals are (in mS):
> + * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
> + * However we are not going to complicate things too much and we stick to the
> + * approx value in mS.
> + */
> +static const int stts751_intervals[] = {
> +	16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
> +};
> +
> +static const struct i2c_device_id stts751_id[] = {
> +	{ "stts751", 0 },
> +	{ }
> +};
> +
> +struct stts751_priv {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	struct mutex access_lock;
> +	u8 interval;
> +	int res;
> +	int event_max, event_min;
> +	int therm;
> +	int hyst;
> +	bool smbus_timeout;
> +	int temp;
> +	unsigned long last_update, last_alert_update;
> +	u8 config;
> +	bool min_alert, max_alert, therm_trip;
> +	bool data_valid, alert_valid;
> +};
> +
> +/*
> + * These functions converts temperature from HW format to integer format and
> + * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
> + */
> +static int stts751_to_deg(s32 hw_val)
> +{
> +	return hw_val * 125 / 32;
> +}
> +
> +static s32 stts751_to_hw(int val)
> +{
> +	s32 hw_val;
> +
> +	if (val < 0)
> +		hw_val = (val - 62) / 125 * 32;
> +	else
> +		hw_val = (val + 62) / 125 * 32;
> +

How about "return DIV_ROUND_CLOSEST(val, 125) * 32;" ?

> +	return hw_val;
> +}
> +
> +static int stts751_adjust_resolution(struct stts751_priv *priv)
> +{
> +	u8 res;
> +
> +	switch (priv->interval) {
> +	case 9:
> +		/* 10 bits */
> +		res = 0;
> +		break;
> +	case 8:
> +		/* 11 bits */
> +		res = 1;
> +		break;
> +	default:
> +		/* 12 bits */
> +		res = 3;
> +		break;
> +	}
> +
> +	if (priv->res == res)
> +		return 0;
> +
> +	priv->config &= ~STTS751_CONF_RES_MASK;
> +	priv->config |= res << STTS751_CONF_RES_SHIFT;
> +
> +	return i2c_smbus_write_byte_data(priv->client,
> +				STTS751_REG_CONF, priv->config);
> +}
> +
> +static int stts751_update_temp(struct stts751_priv *priv)
> +{
> +	s32 integer1, integer2, frac;
> +	int ret = 0;
> +
> +	/*
> +	 * There is a trick here, like in the lm90 driver. We have to read two
> +	 * registers to get the sensor temperature, but we have to beware a
> +	 * conversion could occur between the readings. We could use the
> +	 * one-shot conversion register, but we don't want to do this (disables
> +	 * hardware monitoring). So the solution used here is to read the high
> +	 * byte once, then the low byte, then the high byte again. If the new
> +	 * high byte matches the old one, then we have a valid reading. Else we
> +	 * have to read the low byte again, and now we believe we have a correct
> +	 * reading.
> +	 */
> +	integer1 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
> +	if (integer1 < 0) {
> +		dev_dbg(&priv->client->dev,
> +			"I2C read failed (temp H). ret: %x\n", ret);
> +		ret = integer1;
> +		goto exit;
> +	}
> +
> +	frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
> +	if (frac < 0) {
> +		dev_dbg(&priv->client->dev,
> +			"I2C read failed (temp L). ret: %x\n", ret);
> +		ret = frac;
> +		goto exit;
> +	}
> +
> +	integer2 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
> +	if (integer2 < 0) {
> +		dev_dbg(&priv->client->dev,
> +			"I2C 2nd read failed (temp H). ret: %x\n", ret);
> +		ret = integer2;
> +		goto exit;
> +	}
> +
> +	if (integer1 != integer2) {
> +		frac = i2c_smbus_read_byte_data(priv->client,
> +						STTS751_REG_TEMP_L);
> +		if (frac < 0) {
> +			dev_dbg(&priv->client->dev,
> +				"I2C 2nd read failed (temp L). ret: %x\n", ret);
> +			ret = frac;
> +			goto exit;
> +		}
> +	}
> +
> +exit:
> +	if (ret)
> +		return ret;

If the code can return immediately, please do so.

> +
> +	priv->temp = stts751_to_deg((integer1 << 8) | frac);
> +	return ret;
> +}
> +
> +static int stts751_set_temp_reg16(struct stts751_priv *priv, int temp,
> +				u8 hreg, u8 lreg)
> +{
> +	s32 hwval;
> +	int ret;
> +
> +	hwval = stts751_to_hw(temp);
> +
> +	mutex_lock(&priv->access_lock);
> +	ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
> +	if (!ret)
> +		ret = i2c_smbus_write_byte_data(priv->client, lreg,
> +						hwval & 0xff);
> +	mutex_unlock(&priv->access_lock);
> +
> +	return ret;
> +}
> +
> +static int stts751_set_temp_reg8(struct stts751_priv *priv, int temp, u8 reg)
> +{
> +	s32 hwval;
> +	int ret;
> +
> +	hwval = stts751_to_hw(temp);
> +
> +	mutex_lock(&priv->access_lock);
> +	ret = i2c_smbus_write_byte_data(priv->client, reg, hwval >> 8);
> +	mutex_unlock(&priv->access_lock);

The lock is in the wrong location. It needs to be in the calling code,
since the calling code updates context variables. The same is true for
stts751_set_temp_reg16().

> +
> +	return ret;
> +}
> +
> +static int stts751_read_reg16(struct stts751_priv *priv, int *temp,
> +				u8 hreg, u8 lreg)
> +{
> +	int integer, frac;
> +
> +	integer = i2c_smbus_read_byte_data(priv->client, hreg);
> +	if (integer < 0)
> +		return integer;
> +
> +	frac = i2c_smbus_read_byte_data(priv->client, lreg);
> +	if (frac < 0)
> +		return frac;
> +
> +	*temp = stts751_to_deg((integer << 8) | frac);
> +
> +	return 0;
> +}
> +
> +static int stts751_read_reg8(struct stts751_priv *priv, int *temp, u8 reg)
> +{
> +	int integer;
> +
> +	integer = i2c_smbus_read_byte_data(priv->client, reg);
> +	if (integer < 0)
> +		return integer;
> +
> +	*temp = stts751_to_deg(integer << 8);
> +
> +	return 0;
> +}
> +
> +/*
> + * Update alert flags without waiting for cache to expire. We detects alerts
> + * immediately for the sake of the alert handler; we still need to deal with
> + * caching to workaround the fact that the status register gets cleared when

Did we have this before ? The datasheet claims that the status is only cleared
if the condition no longer exists. If that is incorrect, please explain here,
or it will come up again.

> + * reading it.
> + */
> +static int stts751_update_alert(struct stts751_priv *priv)
> +{
> +	int ret;
> +	bool conv_done;
> +	int cache_time =
> +		DIV_ROUND_UP(stts751_intervals[priv->interval] * HZ, 1000);

How about using msecs_to_jiffies() ?

> +
> +	/*
> +	 * Add another 10% because if we run faster than the HW conversion
> +	 * rate we will end up in reporting incorrectly alarms.
> +	 */
> +	cache_time += cache_time / 10;
> +
> +	ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_dbg(priv->dev, "status reg %x\n", ret);
> +	conv_done = ret & (STTS751_STATUS_TRIPH | STTS751_STATUS_TRIPL);
> +	/*
> +	 * Reset the cache if the cache time expired, or if we are sure
> +	 * we have valid data from a device conversion, or if we know
> +	 * our cache has been never written.
> +	 *
> +	 * Note that when the cache has been never written the point is
> +	 * to correctly initialize the timestamp, rather than clearing
> +	 * the cache values.
> +	 *
> +	 * Note that updating the cache timestamp when we get an alarm flag
> +	 * is required, otherwise we could incorrectly report alarms to be zero.
> +	 */
> +	if (time_after(jiffies,	priv->last_alert_update + cache_time) ||
> +		conv_done || !priv->alert_valid) {
> +		priv->max_alert = false;
> +		priv->min_alert = false;
> +		priv->alert_valid = true;
> +		priv->last_alert_update = jiffies;
> +		dev_dbg(priv->dev, "invalidating alert cache\n");
> +	}
> +
> +	priv->max_alert |= !!(ret & STTS751_STATUS_TRIPH);
> +	priv->min_alert |= !!(ret & STTS751_STATUS_TRIPL);
> +	priv->therm_trip = !!(ret & STTS751_STATUS_TRIPT);
> +
> +	dev_dbg(priv->dev, "max_alert: %d, min_alert: %d, therm_trip: %d\n",
> +		priv->max_alert, priv->min_alert, priv->therm_trip);
> +
> +	return 0;
> +}
> +
> +static void stts751_alert(struct i2c_client *client,
> +			enum i2c_alert_protocol type, unsigned int data)
> +{
> +	int ret;
> +	struct stts751_priv *priv = i2c_get_clientdata(client);
> +
> +	if (type != I2C_PROTOCOL_SMBUS_ALERT)
> +		return;
> +
> +	dev_dbg(&client->dev, "alert!");
> +
> +	mutex_lock(&priv->access_lock);
> +	ret = stts751_update_alert(priv);
> +	if (ret < 0) {
> +		/* default to worst case */
> +		priv->max_alert = true;
> +		priv->min_alert = true;
> +
> +		dev_warn(&priv->client->dev,
> +			"Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!");
> +	}
> +
> +	if (priv->max_alert) {
> +		dev_notice(&client->dev, "got alert for HIGH temperature");
> +
> +		/* unblock alert poll */
> +		sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_max_alert");
> +		kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
> +	}
> +
> +	if (priv->min_alert) {
> +		dev_notice(&client->dev, "got alert for LOW temperature");
> +
> +		/* unblock alert poll */
> +		sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_min_alert");
> +		kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);

This way you can get two uevents, which is really undesirable.
Maybe move uevent creation into a separate if().

> +	}
> +	mutex_unlock(&priv->access_lock);
> +}
> +
> +static int stts751_update(struct stts751_priv *priv)
> +{
> +	int ret = 0;
> +	int cache_time = stts751_intervals[priv->interval] / 1000;

jiffies and the parameter for time_after() are in HZ. Does this
assume that HZ=1000 ? msecs_to_jiffies() might be more appropriate.

> +
> +	mutex_lock(&priv->access_lock);
> +	if (time_after(jiffies,	priv->last_update + cache_time) ||
> +		!priv->data_valid) {
> +		priv->data_valid = true;

Not really, only after stts751_update_temp() succeeded.

> +		priv->last_update = jiffies;

Same here.

> +
> +		ret = stts751_update_temp(priv);
> +		if (ret)
> +			goto exit;
> +
> +		ret = stts751_update_alert(priv);

		if (!ret)
			ret = stts751_update_alert(priv);

would avoid the goto.

> +	}
> +exit:
> +	mutex_unlock(&priv->access_lock);
> +
> +	return ret;
> +}
> +
> +static ssize_t show_max_alarm(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	int ret;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	ret = stts751_update(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
> +}
> +
> +static ssize_t show_min_alarm(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	int ret;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	ret = stts751_update(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
> +}
> +
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	int ret;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	ret = stts751_update(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
> +}
> +
> +static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
> +}
> +
> +static int do_set_hyst(struct stts751_priv *priv, int temp)
> +{
> +	/* HW works in range -64C to +127.937C */
> +	temp = clamp_val(temp, -64000, priv->therm);
> +	priv->hyst = temp;
> +	dev_dbg(priv->dev, "setting hyst %d", temp);
> +	temp = priv->therm - temp;
> +
> +	return stts751_set_temp_reg8(priv, temp, STTS751_REG_HYST);
> +}
> +
> +static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	int ret;
> +	long temp;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +	/* HW works in range -64C to +127.937C */
> +	temp = clamp_val(temp, -64000, 127937);
> +	ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_TLIM);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "setting therm %ld", temp);
> +	priv->therm = temp;
> +
> +	/*
> +	 * hysteresis reg is relative to therm, so we need to update
> +	 * it as well.
> +	 */
> +	ret = do_set_hyst(priv, priv->hyst);

The basic expectation here is that the hysteresis value changes automatically.
If the old hysteresis was 8 degrees below the limit, it should still be 8
degrees below the new limit. This code tries to restore the old (absolute)
value, which hardly ever makes any sense.

> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t show_hyst(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
> +}
> +
> +static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	int ret;
> +	long temp;
> +
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +	ret = do_set_hyst(priv, temp);
> +	if (ret)
> +		return ret;
> +	return count;
> +}
> +
> +static ssize_t show_therm_trip(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	ret = stts751_update(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm_trip);
> +}
> +
> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
> +}
> +
> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	int ret;
> +	long temp;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +	/* HW works in range -64C to +127.937C */
> +	temp = clamp_val(temp, priv->event_min, 127937);
> +	ret = stts751_set_temp_reg16(priv, temp,
> +				STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "setting event max %ld", temp);
> +	priv->event_max = temp;

As mentioned above, the writes are all racy. Two writes in parallel may
result in inconsistent register values vs. values written into event_max etc.

> +	return count;
> +}
> +
> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
> +}
> +
> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	int ret;
> +	long temp;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +	/* HW works in range -64C to +127.937C */
> +	temp = clamp_val(temp, -64000, priv->event_max);
> +	ret = stts751_set_temp_reg16(priv, temp,
> +				STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "setting event min %ld", temp);
> +
> +	priv->event_min = temp;
> +	return count;
> +}
> +
> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
> +			stts751_intervals[priv->interval]);
> +}
> +
> +static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	unsigned long val;
> +	int idx;
> +	int ret = 0;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtoul(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	idx = find_closest_descending(val, stts751_intervals,
> +				ARRAY_SIZE(stts751_intervals));
> +
> +	dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, idx,
> +		stts751_intervals[idx]);
> +
> +	if (priv->interval == idx)
> +		return count;
> +
> +	mutex_lock(&priv->access_lock);
> +
> +	/*
> +	 * In early development stages I've become suspicious about the chip
> +	 * starting to misbehave if I ever set, even briefly, an invalid
> +	 * configuration. While I'm not sure this is really needed, be
> +	 * conservative and set rate/resolution in such an order that avoids
> +	 * passing through an invalid configuration.
> +	 */
> +
> +	/* speed up: lower the resolution, then modify convrate */
> +	if (priv->interval < idx) {
> +		priv->interval = idx;
> +		ret = stts751_adjust_resolution(priv);
> +		if (ret)
> +			goto exit;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, idx);
> +	if (ret)
> +		goto exit;
> +	/* slow down: modify convrate, then raise resolution */
> +	if (priv->interval != idx) {
> +		priv->interval = idx;
> +		ret = stts751_adjust_resolution(priv);
> +		if (ret)
> +			goto exit;
> +	}
> +exit:
> +	mutex_unlock(&priv->access_lock);
> +
> +	return count;
> +}
> +
> +static int stts751_detect(struct i2c_client *new_client,
> +			  struct i2c_board_info *info)
> +{
> +	struct i2c_adapter *adapter = new_client->adapter;
> +	const char *name;
> +	int mfg_id, prod_id, rev_id;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> +
> +	mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
> +	if (mfg_id != ST_MAN_ID)
> +		return -ENODEV;
> +
> +	prod_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
> +
> +	switch (prod_id) {
> +	case STTS751_0_PROD_ID:
> +		name = "STTS751-0";
> +		break;
> +	case STTS751_1_PROD_ID:
> +		name = "STTS751-1";
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +	dev_dbg(&new_client->dev, "Chip %s detected!", name);

I'll never understand what the "!" in such messages is for ;-).

> +
> +	rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
> +	if (rev_id < 0)
> +		return -ENODEV;
> +	if (rev_id != 0x1) {
> +		dev_notice(&new_client->dev,
> +			"Chip revision 0x%x is untested\nPlease report whether it works to andrea.merello@gmail.com",
> +			rev_id);
> +	}
> +
> +	strlcpy(info->type, stts751_id[0].name, I2C_NAME_SIZE);
> +	return 0;
> +}
> +
> +static int stts751_read_chip_config(struct stts751_priv *priv)
> +{
> +	int ret;
> +	int tmp;
> +
> +	ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_CONF);
> +	if (ret < 0)
> +		return ret;
> +	priv->config = ret;
> +	priv->res = (ret & STTS751_CONF_RES_MASK) >> STTS751_CONF_RES_SHIFT;
> +
> +	ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_RATE);
> +	if (ret < 0)
> +		return ret;
> +	priv->interval = ret;
> +
> +	ret = stts751_read_reg16(priv, &priv->event_max,
> +				STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
> +	if (ret)
> +		return ret;
> +
> +	ret = stts751_read_reg16(priv, &priv->event_min,
> +				STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
> +	if (ret)
> +		return ret;
> +
> +	ret = stts751_read_reg8(priv, &priv->therm, STTS751_REG_TLIM);
> +	if (ret)
> +		return ret;
> +
> +	ret = stts751_read_reg8(priv, &tmp, STTS751_REG_HYST);
> +	if (ret)
> +		return ret;
> +	priv->hyst = priv->therm - tmp;
> +
> +	return ret;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_min, set_min, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_max, set_max, 0);
> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_min_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_max_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_therm,
> +			set_therm, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_hyst,
> +			set_hyst, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_therm_trip, NULL, 0);
> +static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
> +			show_interval, set_interval, 0);
> +
> +static struct attribute *stts751_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
> +	&sensor_dev_attr_update_interval.dev_attr.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(stts751);
> +
> +static int stts751_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct stts751_priv *priv;
> +	int ret;
> +	bool smbus_to;
> +
> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +	i2c_set_clientdata(client, priv);
> +	mutex_init(&priv->access_lock);
> +
> +	if (device_property_present(&client->dev,
> +					"smbus-timeout-disable")) {
> +		smbus_to = !device_property_read_bool(&client->dev,
> +						"smbus-timeout-disable");
> +
> +		ret = i2c_smbus_write_byte_data(priv->client,
> +						STTS751_REG_SMBUS_TO,
> +						smbus_to ? 0x80 : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = stts751_read_chip_config(priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->config &= ~(STTS751_CONF_STOP | STTS751_CONF_EVENT_DIS);
> +	ret = i2c_smbus_write_byte_data(priv->client,
> +					STTS751_REG_CONF, priv->config);
> +	if (ret)
> +		return ret;
> +
> +	priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
> +							client->name, priv,
> +							stts751_groups);
> +	return PTR_ERR_OR_ZERO(priv->dev);
> +}
> +
> +MODULE_DEVICE_TABLE(i2c, stts751_id);
> +
> +static struct i2c_driver stts751_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= DEVNAME,
> +	},
> +	.probe		= stts751_probe,
> +	.id_table	= stts751_id,
> +	.detect		= stts751_detect,
> +	.alert		= stts751_alert,
> +	.address_list	= normal_i2c,
> +};
> +
> +module_i2c_driver(stts751_driver);
> +
> +MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
> +MODULE_DESCRIPTION("STTS751 sensor driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4
>