[PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Guenter Roeck]

By registering a cleanup function with devm_add_action(), we can
simplify the error path in the probe function and drop the remove
function entirely.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/tmp102.c | 54 ++++++++++++++++++--------------------------------
 1 file changed, 19 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index f1e96fd7f445..befd06b6f3a5 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -52,7 +52,6 @@
 
 struct tmp102 {
 	struct i2c_client *client;
-	struct device *hwmon_dev;
 	struct mutex lock;
 	u16 config_orig;
 	unsigned long last_update;
@@ -173,6 +172,15 @@ static const struct thermal_zone_of_device_ops tmp102_of_thermal_ops = {
 	.get_temp = tmp102_read_temp,
 };
 
+static void tmp102_restore_config(void *data)
+{
+	struct tmp102 *tmp102 = data;
+	struct i2c_client *client = tmp102->client;
+
+	i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
+				     tmp102->config_orig);
+}
+
 static int tmp102_probe(struct i2c_client *client,
 				  const struct i2c_device_id *id)
 {
@@ -201,66 +209,43 @@ static int tmp102_probe(struct i2c_client *client,
 		return status;
 	}
 	tmp102->config_orig = status;
+
+	devm_add_action(dev, tmp102_restore_config, tmp102);
+
 	status = i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
 					      TMP102_CONFIG);
 	if (status < 0) {
 		dev_err(dev, "error writing config register\n");
-		goto fail_restore_config;
+		return status;
 	}
 	status = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
 	if (status < 0) {
 		dev_err(dev, "error reading config register\n");
-		goto fail_restore_config;
+		return status;
 	}
 	status &= ~TMP102_CONFIG_RD_ONLY;
 	if (status != TMP102_CONFIG) {
 		dev_err(dev, "config settings did not stick\n");
-		status = -ENODEV;
-		goto fail_restore_config;
+		return -ENODEV;
 	}
 	tmp102->last_update = jiffies;
 	/* Mark that we are not ready with data until conversion is complete */
 	tmp102->first_time = true;
 	mutex_init(&tmp102->lock);
 
-	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
-						      tmp102, tmp102_groups);
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+							   tmp102,
+							   tmp102_groups);
 	if (IS_ERR(hwmon_dev)) {
 		dev_dbg(dev, "unable to register hwmon device\n");
-		status = PTR_ERR(hwmon_dev);
-		goto fail_restore_config;
+		return PTR_ERR(hwmon_dev);
 	}
-	tmp102->hwmon_dev = hwmon_dev;
 	devm_thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
 					     &tmp102_of_thermal_ops);
 
 	dev_info(dev, "initialized\n");
 
 	return 0;
-
-fail_restore_config:
-	i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
-				     tmp102->config_orig);
-	return status;
-}
-
-static int tmp102_remove(struct i2c_client *client)
-{
-	struct tmp102 *tmp102 = i2c_get_clientdata(client);
-
-	hwmon_device_unregister(tmp102->hwmon_dev);
-
-	/* Stop monitoring if device was stopped originally */
-	if (tmp102->config_orig & TMP102_CONF_SD) {
-		int config;
-
-		config = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
-		if (config >= 0)
-			i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
-						     config | TMP102_CONF_SD);
-	}
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -303,7 +288,6 @@ static struct i2c_driver tmp102_driver = {
 	.driver.name	= DRIVER_NAME,
 	.driver.pm	= &tmp102_dev_pm_ops,
 	.probe		= tmp102_probe,
-	.remove		= tmp102_remove,
 	.id_table	= tmp102_id,
 };
 
-- 
2.5.0

[PATCH 2/5] hwmon: (tmp102) Drop FSF address

[Guenter Roeck]

The FSF address can change, so drop it from the driver.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/tmp102.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index befd06b6f3a5..5bdf262e6a0e 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -11,10 +11,6 @@
  * 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 St - Fifth Floor, Boston, MA 02110-1301 USA
  */
 
 #include <linux/module.h>
-- 
2.5.0

[PATCH 3/5] hwmon: (tmp102) Improve handling of initial read delay

[Guenter Roeck]

If the chip was in shutdown mode when the driver was loaded, the first
conversion is ready no more than 35 milli-seconds after the chip was
taken out of shutdown. The driver delay was so far set to 333 ms (HZ / 3),
which is much higher than the maximum time needed by the chip.

Reduce the time to 35 milli-seconds. Instead of returning -EAGAIN if data
is not yet available, go to sleep for the remaining time.

Introduce a 'valid' flag to ensure that sensor data is actually read
even if requested less than 333 ms after the driver was loaded.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/tmp102.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index 5bdf262e6a0e..615f9c176aaf 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -13,6 +13,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
@@ -46,13 +47,16 @@
 #define	TMP102_TLOW_REG			0x02
 #define	TMP102_THIGH_REG		0x03
 
+#define CONVERSION_TIME_MS		35	/* in milli-seconds */
+
 struct tmp102 {
 	struct i2c_client *client;
 	struct mutex lock;
 	u16 config_orig;
 	unsigned long last_update;
+	unsigned long ready_time;
+	bool valid;
 	int temp[3];
-	bool first_time;
 };
 
 /* convert left adjusted 13-bit TMP102 register value to milliCelsius */
@@ -78,8 +82,16 @@ static struct tmp102 *tmp102_update_device(struct device *dev)
 	struct tmp102 *tmp102 = dev_get_drvdata(dev);
 	struct i2c_client *client = tmp102->client;
 
+	/* Is it too early to return a conversion ? */
+	if (time_before(jiffies, tmp102->ready_time)) {
+		unsigned long sleeptime = tmp102->ready_time - jiffies;
+
+		msleep(jiffies_to_msecs(sleeptime));
+	}
+
 	mutex_lock(&tmp102->lock);
-	if (time_after(jiffies, tmp102->last_update + HZ / 3)) {
+	if (!tmp102->valid ||
+	    time_after(jiffies, tmp102->last_update + HZ / 3)) {
 		int i;
 		for (i = 0; i < ARRAY_SIZE(tmp102->temp); ++i) {
 			int status = i2c_smbus_read_word_swapped(client,
@@ -88,7 +100,7 @@ static struct tmp102 *tmp102_update_device(struct device *dev)
 				tmp102->temp[i] = tmp102_reg_to_mC(status);
 		}
 		tmp102->last_update = jiffies;
-		tmp102->first_time = false;
+		tmp102->valid = true;
 	}
 	mutex_unlock(&tmp102->lock);
 	return tmp102;
@@ -98,12 +110,6 @@ static int tmp102_read_temp(void *dev, int *temp)
 {
 	struct tmp102 *tmp102 = tmp102_update_device(dev);
 
-	/* Is it too early even to return a conversion? */
-	if (tmp102->first_time) {
-		dev_dbg(dev, "%s: Conversion not ready yet..\n", __func__);
-		return -EAGAIN;
-	}
-
 	*temp = tmp102->temp[0];
 
 	return 0;
@@ -116,10 +122,6 @@ static ssize_t tmp102_show_temp(struct device *dev,
 	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
 	struct tmp102 *tmp102 = tmp102_update_device(dev);
 
-	/* Is it too early even to return a read? */
-	if (tmp102->first_time)
-		return -EAGAIN;
-
 	return sprintf(buf, "%d\n", tmp102->temp[sda->index]);
 }
 
@@ -224,11 +226,18 @@ static int tmp102_probe(struct i2c_client *client,
 		dev_err(dev, "config settings did not stick\n");
 		return -ENODEV;
 	}
-	tmp102->last_update = jiffies;
-	/* Mark that we are not ready with data until conversion is complete */
-	tmp102->first_time = true;
+
 	mutex_init(&tmp102->lock);
 
+	tmp102->ready_time = jiffies;
+	if (tmp102->config_orig & TMP102_CONF_SD) {
+		/*
+		 * Mark that we are not ready with data until the first
+		 * conversion is complete
+		 */
+		tmp102->ready_time += msecs_to_jiffies(CONVERSION_TIME_MS);
+	}
+
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
 							   tmp102,
 							   tmp102_groups);
@@ -261,12 +270,15 @@ static int tmp102_suspend(struct device *dev)
 static int tmp102_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct tmp102 *tmp102 = i2c_get_clientdata(client);
 	int config;
 
 	config = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
 	if (config < 0)
 		return config;
 
+	tmp102->ready_time = jiffies + msecs_to_jiffies(CONVERSION_TIME_MS);
+
 	config &= ~TMP102_CONF_SD;
 	return i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, config);
 }
-- 
2.5.0

[PATCH 4/5] hwmon: (tmp102) Rework chip configuration

[Guenter Roeck]

So far the chip was forced into polarity 0, even if it was preconfigured
differently. Do not touch the polarity when configuring the chip.

Also, the configuration register was read beack to check if the
configuration 'sticks'. Ultimately, that is similar to checking if the
chip is a tmp102 in the first place. Checking if a write into the
configuration register was successful is really not the way to do it,
and quite risky if the chip is not a tmp102, so drop that check.
Instead, verify if the configuration register has unexpected bits set
before writing into it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/tmp102.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index 615f9c176aaf..487b4ef5992c 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -47,6 +47,17 @@
 #define	TMP102_TLOW_REG			0x02
 #define	TMP102_THIGH_REG		0x03
 
+#define TMP102_CONFREG_MASK	(TMP102_CONF_SD | TMP102_CONF_TM | \
+				 TMP102_CONF_POL | TMP102_CONF_F0 | \
+				 TMP102_CONF_F1 | TMP102_CONF_OS | \
+				 TMP102_CONF_EM | TMP102_CONF_AL | \
+				 TMP102_CONF_CR0 | TMP102_CONF_CR1)
+
+#define TMP102_CONFIG_CLEAR	(TMP102_CONF_SD | TMP102_CONF_OS | \
+				 TMP102_CONF_CR0)
+#define TMP102_CONFIG_SET	(TMP102_CONF_TM | TMP102_CONF_EM | \
+				 TMP102_CONF_CR1)
+
 #define CONVERSION_TIME_MS		35	/* in milli-seconds */
 
 struct tmp102 {
@@ -163,9 +174,6 @@ static struct attribute *tmp102_attrs[] = {
 };
 ATTRIBUTE_GROUPS(tmp102);
 
-#define TMP102_CONFIG  (TMP102_CONF_TM | TMP102_CONF_EM | TMP102_CONF_CR1)
-#define TMP102_CONFIG_RD_ONLY (TMP102_CONF_R0 | TMP102_CONF_R1 | TMP102_CONF_AL)
-
 static const struct thermal_zone_of_device_ops tmp102_of_thermal_ops = {
 	.get_temp = tmp102_read_temp,
 };
@@ -206,26 +214,25 @@ static int tmp102_probe(struct i2c_client *client,
 		dev_err(dev, "error reading config register\n");
 		return status;
 	}
+
+	if ((status & ~TMP102_CONFREG_MASK) !=
+	    (TMP102_CONF_R0 | TMP102_CONF_R1)) {
+		dev_err(dev, "unexpected config register value\n");
+		return -ENODEV;
+	}
+
 	tmp102->config_orig = status;
 
 	devm_add_action(dev, tmp102_restore_config, tmp102);
 
-	status = i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
-					      TMP102_CONFIG);
+	status &= ~TMP102_CONFIG_CLEAR;
+	status |= TMP102_CONFIG_SET;
+
+	status = i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, status);
 	if (status < 0) {
 		dev_err(dev, "error writing config register\n");
 		return status;
 	}
-	status = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
-	if (status < 0) {
-		dev_err(dev, "error reading config register\n");
-		return status;
-	}
-	status &= ~TMP102_CONFIG_RD_ONLY;
-	if (status != TMP102_CONFIG) {
-		dev_err(dev, "config settings did not stick\n");
-		return -ENODEV;
-	}
 
 	mutex_init(&tmp102->lock);
 
-- 
2.5.0

[PATCH 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache

[Guenter Roeck]

By concerting the driver to regmap, we can use regmap to cache non-volatile
registers. Stop caching the temperature register; while potentially reading
it more often can result in reading it more often than necessary, this is
offset by the gain due to not re-reading the limit registers.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/tmp102.c | 158 +++++++++++++++++++++++++------------------------
 1 file changed, 81 insertions(+), 77 deletions(-)

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index 487b4ef5992c..0c6eee7b7b80 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -24,6 +24,7 @@
 #include <linux/mutex.h>
 #include <linux/device.h>
 #include <linux/jiffies.h>
+#include <linux/regmap.h>
 #include <linux/thermal.h>
 #include <linux/of.h>
 
@@ -47,6 +48,8 @@
 #define	TMP102_TLOW_REG			0x02
 #define	TMP102_THIGH_REG		0x03
 
+#define TMP102_NUM_REG			0x04
+
 #define TMP102_CONFREG_MASK	(TMP102_CONF_SD | TMP102_CONF_TM | \
 				 TMP102_CONF_POL | TMP102_CONF_F0 | \
 				 TMP102_CONF_F1 | TMP102_CONF_OS | \
@@ -61,13 +64,9 @@
 #define CONVERSION_TIME_MS		35	/* in milli-seconds */
 
 struct tmp102 {
-	struct i2c_client *client;
-	struct mutex lock;
+	struct regmap *regmap;
 	u16 config_orig;
-	unsigned long last_update;
 	unsigned long ready_time;
-	bool valid;
-	int temp[3];
 };
 
 /* convert left adjusted 13-bit TMP102 register value to milliCelsius */
@@ -82,16 +81,11 @@ static inline u16 tmp102_mC_to_reg(int val)
 	return (val * 128) / 1000;
 }
 
-static const u8 tmp102_reg[] = {
-	TMP102_TEMP_REG,
-	TMP102_TLOW_REG,
-	TMP102_THIGH_REG,
-};
-
-static struct tmp102 *tmp102_update_device(struct device *dev)
+static int tmp102_read_temp(void *dev, int *temp)
 {
 	struct tmp102 *tmp102 = dev_get_drvdata(dev);
-	struct i2c_client *client = tmp102->client;
+	unsigned int reg;
+	int ret;
 
 	/* Is it too early to return a conversion ? */
 	if (time_before(jiffies, tmp102->ready_time)) {
@@ -100,28 +94,11 @@ static struct tmp102 *tmp102_update_device(struct device *dev)
 		msleep(jiffies_to_msecs(sleeptime));
 	}
 
-	mutex_lock(&tmp102->lock);
-	if (!tmp102->valid ||
-	    time_after(jiffies, tmp102->last_update + HZ / 3)) {
-		int i;
-		for (i = 0; i < ARRAY_SIZE(tmp102->temp); ++i) {
-			int status = i2c_smbus_read_word_swapped(client,
-								 tmp102_reg[i]);
-			if (status > -1)
-				tmp102->temp[i] = tmp102_reg_to_mC(status);
-		}
-		tmp102->last_update = jiffies;
-		tmp102->valid = true;
-	}
-	mutex_unlock(&tmp102->lock);
-	return tmp102;
-}
+	ret = regmap_read(tmp102->regmap, TMP102_TEMP_REG, &reg);
+	if (ret < 0)
+		return ret;
 
-static int tmp102_read_temp(void *dev, int *temp)
-{
-	struct tmp102 *tmp102 = tmp102_update_device(dev);
-
-	*temp = tmp102->temp[0];
+	*temp = tmp102_reg_to_mC(reg);
 
 	return 0;
 }
@@ -131,9 +108,24 @@ static ssize_t tmp102_show_temp(struct device *dev,
 				char *buf)
 {
 	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
-	struct tmp102 *tmp102 = tmp102_update_device(dev);
+	struct tmp102 *tmp102 = dev_get_drvdata(dev);
+	int regaddr = sda->index;
+	unsigned int reg;
+	int err;
 
-	return sprintf(buf, "%d\n", tmp102->temp[sda->index]);
+	/* Is it too early to return a conversion ? */
+	if (regaddr == TMP102_TEMP_REG &&
+	    time_before(jiffies, tmp102->ready_time)) {
+		unsigned long sleeptime = tmp102->ready_time - jiffies;
+
+		msleep(jiffies_to_msecs(sleeptime));
+	}
+
+	err = regmap_read(tmp102->regmap, regaddr, &reg);
+	if (err < 0)
+		return err;
+
+	return sprintf(buf, "%d\n", tmp102_reg_to_mC(reg));
 }
 
 static ssize_t tmp102_set_temp(struct device *dev,
@@ -142,29 +134,26 @@ static ssize_t tmp102_set_temp(struct device *dev,
 {
 	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
 	struct tmp102 *tmp102 = dev_get_drvdata(dev);
-	struct i2c_client *client = tmp102->client;
+	int reg = sda->index;
 	long val;
-	int status;
+	int err;
 
 	if (kstrtol(buf, 10, &val) < 0)
 		return -EINVAL;
 	val = clamp_val(val, -256000, 255000);
 
-	mutex_lock(&tmp102->lock);
-	tmp102->temp[sda->index] = val;
-	status = i2c_smbus_write_word_swapped(client, tmp102_reg[sda->index],
-					      tmp102_mC_to_reg(val));
-	mutex_unlock(&tmp102->lock);
-	return status ? : count;
+	err = regmap_write(tmp102->regmap, reg, tmp102_mC_to_reg(val));
+	return err ? : count;
 }
 
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp102_show_temp, NULL , 0);
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp102_show_temp, NULL,
+			  TMP102_TEMP_REG);
 
 static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, tmp102_show_temp,
-			  tmp102_set_temp, 1);
+			  tmp102_set_temp, TMP102_TLOW_REG);
 
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, tmp102_show_temp,
-			  tmp102_set_temp, 2);
+			  tmp102_set_temp, TMP102_THIGH_REG);
 
 static struct attribute *tmp102_attrs[] = {
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
@@ -181,19 +170,39 @@ static const struct thermal_zone_of_device_ops tmp102_of_thermal_ops = {
 static void tmp102_restore_config(void *data)
 {
 	struct tmp102 *tmp102 = data;
-	struct i2c_client *client = tmp102->client;
 
-	i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
-				     tmp102->config_orig);
+	regmap_write(tmp102->regmap, TMP102_CONF_REG, tmp102->config_orig);
+}
+
+static bool tmp102_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return reg != TMP102_TEMP_REG;
+}
+
+static bool tmp102_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return reg == TMP102_TEMP_REG;
 }
 
+static const struct regmap_config tmp102_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = TMP102_THIGH_REG,
+	.num_reg_defaults_raw = TMP102_NUM_REG,
+	.writeable_reg = tmp102_is_writeable_reg,
+	.volatile_reg = tmp102_is_volatile_reg,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.cache_type = REGCACHE_FLAT,
+};
+
 static int tmp102_probe(struct i2c_client *client,
 				  const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct tmp102 *tmp102;
-	int status;
+	unsigned int regval;
+	int err;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_WORD_DATA)) {
@@ -207,35 +216,36 @@ static int tmp102_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	i2c_set_clientdata(client, tmp102);
-	tmp102->client = client;
 
-	status = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
-	if (status < 0) {
+	tmp102->regmap = devm_regmap_init_i2c(client, &tmp102_regmap_config);
+	if (IS_ERR(tmp102->regmap))
+		return PTR_ERR(tmp102->regmap);
+
+	err = regmap_read(tmp102->regmap, TMP102_CONF_REG, &regval);
+	if (err < 0) {
 		dev_err(dev, "error reading config register\n");
-		return status;
+		return err;
 	}
 
-	if ((status & ~TMP102_CONFREG_MASK) !=
+	if ((regval & ~TMP102_CONFREG_MASK) !=
 	    (TMP102_CONF_R0 | TMP102_CONF_R1)) {
 		dev_err(dev, "unexpected config register value\n");
 		return -ENODEV;
 	}
 
-	tmp102->config_orig = status;
+	tmp102->config_orig = regval;
 
 	devm_add_action(dev, tmp102_restore_config, tmp102);
 
-	status &= ~TMP102_CONFIG_CLEAR;
-	status |= TMP102_CONFIG_SET;
+	regval &= ~TMP102_CONFIG_CLEAR;
+	regval |= TMP102_CONFIG_SET;
 
-	status = i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, status);
-	if (status < 0) {
+	err = regmap_write(tmp102->regmap, TMP102_CONF_REG, regval);
+	if (err < 0) {
 		dev_err(dev, "error writing config register\n");
-		return status;
+		return err;
 	}
 
-	mutex_init(&tmp102->lock);
-
 	tmp102->ready_time = jiffies;
 	if (tmp102->config_orig & TMP102_CONF_SD) {
 		/*
@@ -264,30 +274,24 @@ static int tmp102_probe(struct i2c_client *client,
 static int tmp102_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	int config;
-
-	config = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
-	if (config < 0)
-		return config;
+	struct tmp102 *tmp102 = i2c_get_clientdata(client);
 
-	config |= TMP102_CONF_SD;
-	return i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, config);
+	return regmap_update_bits(tmp102->regmap, TMP102_CONF_REG,
+				  TMP102_CONF_SD, TMP102_CONF_SD);
 }
 
 static int tmp102_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct tmp102 *tmp102 = i2c_get_clientdata(client);
-	int config;
+	int err;
 
-	config = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
-	if (config < 0)
-		return config;
+	err = regmap_update_bits(tmp102->regmap, TMP102_CONF_REG,
+				 TMP102_CONF_SD, 0);
 
 	tmp102->ready_time = jiffies + msecs_to_jiffies(CONVERSION_TIME_MS);
 
-	config &= ~TMP102_CONF_SD;
-	return i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, config);
+	return err;
 }
 #endif /* CONFIG_PM */
 
-- 
2.5.0

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Nishanth Menon]

On 06/23/2016 07:28 PM, Guenter Roeck wrote:
> By registering a cleanup function with devm_add_action(), we can
> simplify the error path in the probe function and drop the remove
> function entirely.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

I dont seem to have a cover letter to reply to... but anyways..

Before: http://pastebin.ubuntu.com/17801376/
After all 5 patches: http://pastebin.ubuntu.com/17801824/

Fails on beagleboard-X15 with:
[   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
[   36.795940] tmp102 0-0048: unexpected config register value

I have'nt bisected down on the specific patch in the series. Have you 
had a chance to test the series?


-- 
Regards,
Nishanth Menon

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Guenter Roeck]

Hi Nishanth,

On 06/24/2016 07:13 AM, Nishanth Menon wrote:
> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>> By registering a cleanup function with devm_add_action(), we can
>> simplify the error path in the probe function and drop the remove
>> function entirely.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> I dont seem to have a cover letter to reply to... but anyways..
>
> Before: http://pastebin.ubuntu.com/17801376/
> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>
> Fails on beagleboard-X15 with:
> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
> [   36.795940] tmp102 0-0048: unexpected config register value
>
> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>
>

Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
initial config register values and I messed up there. Can you send me the output
of i2cdump (word wide) ?

Thanks,
Guenter

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Guenter Roeck]

On 06/24/2016 07:30 AM, Guenter Roeck wrote:
> Hi Nishanth,
>
> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>> By registering a cleanup function with devm_add_action(), we can
>>> simplify the error path in the probe function and drop the remove
>>> function entirely.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>
>> I dont seem to have a cover letter to reply to... but anyways..
>>
>> Before: http://pastebin.ubuntu.com/17801376/
>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>
>> Fails on beagleboard-X15 with:
>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>> [   36.795940] tmp102 0-0048: unexpected config register value
>>
>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>
>>
>
> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
> initial config register values and I messed up there. Can you send me the output
> of i2cdump (word wide) ?
>

Also, beagleboard uses the omap i2c controller, correct ? I'll have to check
if regmap handles endianness conversion properly for controllers supporting
I2C_FUNC_I2C. If some other i2c controller is used, please let me know (or
sene me the output of i2cdetect -l). Recent kernels have a bug in regmap
which affects chips with 16-bit registers if I2C_FUNC_I2C is not supported.

Maybe I should get a beagleboard. The X15 isn't available yet, though. Do the
available boards (eg Rev C) use the same i2c controller ?

Thanks,
Guenter

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Nishanth Menon]

On 06/24/2016 09:54 AM, Guenter Roeck wrote:
> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>> Hi Nishanth,
>>
>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>> By registering a cleanup function with devm_add_action(), we can
>>>> simplify the error path in the probe function and drop the remove
>>>> function entirely.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>
>>> I dont seem to have a cover letter to reply to... but anyways..
>>>
>>> Before: http://pastebin.ubuntu.com/17801376/
>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>
>>> Fails on beagleboard-X15 with:
>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>
>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>
>>>
>>
>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>> initial config register values and I messed up there. Can you send me the output
>> of i2cdump (word wide) ?
>>
> 
> Before 5 patches:
>> # i2cdump -f 0 0x48 w
>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 08: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 10: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 18: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 20: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 28: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 30: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 38: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 40: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 48: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 50: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 58: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 60: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 68: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 70: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 78: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 80: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 88: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 90: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 98: 7912 b062 004b 0050 c018 e006 0000 0000 
>> a0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> a8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> b0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> b8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> c0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> c8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> d0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> d8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> e0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> e8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> f0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> f8: 7912 b062 004b 0050 c018 e006 0000 0000 
> 
> After 5 patches:
>>  i2cdump -y 0 0x48 w
>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 08: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 10: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 18: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 20: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 28: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 30: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 38: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 40: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 48: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 50: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 58: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 60: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 68: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 70: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 78: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 80: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 88: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 90: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 98: 5024 a060 004b 0050 c018 e006 0000 0000 
>> a0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> a8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> b0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> b8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> c0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> c8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> d0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> d8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> e0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> e8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> f0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> f8: 5024 a060 004b 0050 c018 e006 0000 0000 
> 



> Also, beagleboard uses the omap i2c controller, correct ? I'll have to check
> if regmap handles endianness conversion properly for controllers supporting
> I2C_FUNC_I2C. If some other i2c controller is used, please let me know (or
> sene me the output of i2cdetect -l). Recent kernels have a bug in regmap
> which affects chips with 16-bit registers if I2C_FUNC_I2C is not supported.
> 
> Maybe I should get a beagleboard. The X15 isn't available yet, though. Do the
> available boards (eg Rev C) use the same i2c controller ?

yeah there are variants of omap-i2c controllers, but basically they
work in the same way.

I can try and debug the series once I get some spare time, might be
over the weekend or next week.
-- 
Regards,
Nishanth Menon

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Nishanth Menon]

On 06/24/2016 10:23 AM, Nishanth Menon wrote:
> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>>> Hi Nishanth,
>>>
>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>>> By registering a cleanup function with devm_add_action(), we can
>>>>> simplify the error path in the probe function and drop the remove
>>>>> function entirely.
>>>>>
>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>
>>>> I dont seem to have a cover letter to reply to... but anyways..
>>>>
>>>> Before: http://pastebin.ubuntu.com/17801376/
>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>>
>>>> Fails on beagleboard-X15 with:
>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>>
>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>>
>>>>
>>>
>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>>> initial config register values and I messed up there. Can you send me the output
>>> of i2cdump (word wide) ?
>>>
>>
>> Before 5 patches:
>>> # i2cdump -f 0 0x48 w
>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 08: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 10: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 18: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 20: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 28: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 30: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 38: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 40: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 48: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 50: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 58: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 60: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 68: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 70: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 78: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 80: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 88: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 90: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 98: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> a0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> a8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> b0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> b8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> c0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> c8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> d0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> d8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> e0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> e8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> f0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> f8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>
>> After 5 patches:
>>>  i2cdump -y 0 0x48 w
>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 08: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 10: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 18: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 20: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 28: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 30: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 38: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 40: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 48: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 50: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 58: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 60: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 68: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 70: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 78: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 80: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 88: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 90: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 98: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> a0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> a8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> b0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> b8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> c0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> c8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> d0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> d8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> e0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> e8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> f0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> f8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>
> 
> 
> 
>> Also, beagleboard uses the omap i2c controller, correct ? I'll have to check
>> if regmap handles endianness conversion properly for controllers supporting
>> I2C_FUNC_I2C. If some other i2c controller is used, please let me know (or
>> sene me the output of i2cdetect -l). Recent kernels have a bug in regmap
>> which affects chips with 16-bit registers if I2C_FUNC_I2C is not supported.
Forgot to send you -l output:
root@BeagleBoard-X15:~# i2cdetect -l
i2c-0   i2c             OMAP I2C adapter                        I2C
adapter
i2c-2   i2c             OMAP I2C adapter                        I2C
adapter
root@BeagleBoard-X15:~# i2cdetect -F 0
Functionalities implemented by /dev/i2c-0:
I2C                              yes
SMBus Quick Command              no
SMBus Send Byte                  yes
SMBus Receive Byte               yes
SMBus Write Byte                 yes
SMBus Read Byte                  yes
SMBus Write Word                 yes
SMBus Read Word                  yes
SMBus Process Call               yes
SMBus Block Write                yes
SMBus Block Read                 no
SMBus Block Process Call         no
SMBus PEC                        yes
I2C Block Write                  yes
I2C Block Read                   yes


>>
>> Maybe I should get a beagleboard. The X15 isn't available yet, though. Do the
>> available boards (eg Rev C) use the same i2c controller ?
> 
> yeah there are variants of omap-i2c controllers, but basically they
> work in the same way.
> 
> I can try and debug the series once I get some spare time, might be
> over the weekend or next week.
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Guenter Roeck]

On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
> > On 06/24/2016 07:30 AM, Guenter Roeck wrote:
> >> Hi Nishanth,
> >>
> >> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
> >>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
> >>>> By registering a cleanup function with devm_add_action(), we can
> >>>> simplify the error path in the probe function and drop the remove
> >>>> function entirely.
> >>>>
> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>>
> >>> I dont seem to have a cover letter to reply to... but anyways..
> >>>
> >>> Before: http://pastebin.ubuntu.com/17801376/
> >>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
> >>>
> >>> Fails on beagleboard-X15 with:
> >>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
> >>> [   36.795940] tmp102 0-0048: unexpected config register value
> >>>
> >>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
> >>>
> >>>
> >>
> >> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
> >> initial config register values and I messed up there. Can you send me the output
> >> of i2cdump (word wide) ?
> >>
> > 
> > Before 5 patches:
> >> # i2cdump -f 0 0x48 w
> >>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> >> 00: 7912 b062 004b 0050 c018 e006 0000 0000 

[ ... ]
> > 
> > After 5 patches:
> >>  i2cdump -y 0 0x48 w
> >>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> >> 00: 5024 a060 004b 0050 c018 e006 0000 0000 

[ .... ]

> I can try and debug the series once I get some spare time, might be
> over the weekend or next week.

The register map, at least the initial one, is pretty much the same as mine
and as expected. The configuration register in the second map is messed up,
possible because of a write with wrong endianness.

I bet the regmap_read() of the configuration register returns 0xa060 (or
0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
it would be great if you can confirm by printing the register value with
the "unexpected config register value" message (guess I should have left
that in place - I used it during testing ;-).

If that is the case, I'll probably have to drop the regmap changes, at least
for now. It would mean that regmap is broken for chips like the LM75 (ie
for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.

Thanks,
Guenter

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Nishanth Menon]

On 06/24/2016 11:36 AM, Guenter Roeck wrote:
> On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
>> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
>>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>>>> Hi Nishanth,
>>>>
>>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>>>> By registering a cleanup function with devm_add_action(), we can
>>>>>> simplify the error path in the probe function and drop the remove
>>>>>> function entirely.
>>>>>>
>>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>>
>>>>> I dont seem to have a cover letter to reply to... but anyways..
>>>>>
>>>>> Before: http://pastebin.ubuntu.com/17801376/
>>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>>>
>>>>> Fails on beagleboard-X15 with:
>>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>>>
>>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>>>
>>>>>
>>>>
>>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>>>> initial config register values and I messed up there. Can you send me the output
>>>> of i2cdump (word wide) ?
>>>>
>>>
>>> Before 5 patches:
>>>> # i2cdump -f 0 0x48 w
>>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
> 
> [ ... ]
>>>
>>> After 5 patches:
>>>>  i2cdump -y 0 0x48 w
>>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
> 
> [ .... ]
> 
>> I can try and debug the series once I get some spare time, might be
>> over the weekend or next week.
> 
> The register map, at least the initial one, is pretty much the same as mine
> and as expected. The configuration register in the second map is messed up,
> possible because of a write with wrong endianness.

Got a few mins skipping lunch.. ;)

I did a quick bisect, and it is indeed the patch #5 that breaks.
added http://pastebin.ubuntu.com/17812044/ and got:

tmp102 0-0048: regval = 0x0000ffff

That was weird. Does'nt look like endian-ness swap thingy

So, did the following hack to see all messages flowing in and out from
0x48 at bus controller driver level: http://pastebin.ubuntu.com/17813093/
/ # dmesg|grep XXX
/ #

Before patch #5 (and on next-20160624):
the same diff gave:
http://pastebin.ubuntu.com/17813303/



> I bet the regmap_read() of the configuration register returns 0xa060 (or
> 0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
> it would be great if you can confirm by printing the register value with
> the "unexpected config register value" message (guess I should have left
> that in place - I used it during testing ;-).
> 
> If that is the case, I'll probably have to drop the regmap changes, at least
> for now. It would mean that regmap is broken for chips like the LM75 (ie
> for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.

It does look like everything is getting cached out and no actual reads
are actually getting through to the bus controller driver even.

I tested on next-20160624 and used omap2plus_defconfig for the test.


-- 
Regards,
Nishanth Menon

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Guenter Roeck]

On Fri, Jun 24, 2016 at 01:02:32PM -0500, Nishanth Menon wrote:
> On 06/24/2016 11:36 AM, Guenter Roeck wrote:
> > On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
> >> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
> >>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
> >>>> Hi Nishanth,
> >>>>
> >>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
> >>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
> >>>>>> By registering a cleanup function with devm_add_action(), we can
> >>>>>> simplify the error path in the probe function and drop the remove
> >>>>>> function entirely.
> >>>>>>
> >>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>>>>
> >>>>> I dont seem to have a cover letter to reply to... but anyways..
> >>>>>
> >>>>> Before: http://pastebin.ubuntu.com/17801376/
> >>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
> >>>>>
> >>>>> Fails on beagleboard-X15 with:
> >>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
> >>>>> [   36.795940] tmp102 0-0048: unexpected config register value
> >>>>>
> >>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
> >>>>>
> >>>>>
> >>>>
> >>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
> >>>> initial config register values and I messed up there. Can you send me the output
> >>>> of i2cdump (word wide) ?
> >>>>
> >>>
> >>> Before 5 patches:
> >>>> # i2cdump -f 0 0x48 w
> >>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> >>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
> > 
> > [ ... ]
> >>>
> >>> After 5 patches:
> >>>>  i2cdump -y 0 0x48 w
> >>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> >>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
> > 
> > [ .... ]
> > 
> >> I can try and debug the series once I get some spare time, might be
> >> over the weekend or next week.
> > 
> > The register map, at least the initial one, is pretty much the same as mine
> > and as expected. The configuration register in the second map is messed up,
> > possible because of a write with wrong endianness.
> 
> Got a few mins skipping lunch.. ;)
> 
> I did a quick bisect, and it is indeed the patch #5 that breaks.
> added http://pastebin.ubuntu.com/17812044/ and got:
> 
> tmp102 0-0048: regval = 0x0000ffff
> 
> That was weird. Does'nt look like endian-ness swap thingy
> 
> So, did the following hack to see all messages flowing in and out from
> 0x48 at bus controller driver level: http://pastebin.ubuntu.com/17813093/
> / # dmesg|grep XXX
> / #
> 
> Before patch #5 (and on next-20160624):
> the same diff gave:
> http://pastebin.ubuntu.com/17813303/
> 
> 
> 
> > I bet the regmap_read() of the configuration register returns 0xa060 (or
> > 0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
> > it would be great if you can confirm by printing the register value with
> > the "unexpected config register value" message (guess I should have left
> > that in place - I used it during testing ;-).
> > 
> > If that is the case, I'll probably have to drop the regmap changes, at least
> > for now. It would mean that regmap is broken for chips like the LM75 (ie
> > for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.
> 
> It does look like everything is getting cached out and no actual reads
> are actually getting through to the bus controller driver even.
> 
Yes, that is really weird. It also seems to fill the cache with 0xffff,
which is even more weird.

Ok, looks like converting drivers to regmap isn't a good idea. I'll need
to get a system with an adapter supporting I2C_FUNC_I2C and do some more
testing.

> I tested on next-20160624 and used omap2plus_defconfig for the test.
> 

Thanks a lot for the information, and for testing this with your system.

Guenter

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Nishanth Menon]

On 06/24/2016 01:18 PM, Guenter Roeck wrote:
> On Fri, Jun 24, 2016 at 01:02:32PM -0500, Nishanth Menon wrote:
>> On 06/24/2016 11:36 AM, Guenter Roeck wrote:
>>> On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
>>>> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
>>>>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>>>>>> Hi Nishanth,
>>>>>>
>>>>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>>>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>>>>>> By registering a cleanup function with devm_add_action(), we can
>>>>>>>> simplify the error path in the probe function and drop the remove
>>>>>>>> function entirely.
>>>>>>>>
>>>>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>>>>
>>>>>>> I dont seem to have a cover letter to reply to... but anyways..
>>>>>>>
>>>>>>> Before: http://pastebin.ubuntu.com/17801376/
>>>>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>>>>>
>>>>>>> Fails on beagleboard-X15 with:
>>>>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>>>>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>>>>>
>>>>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>>>>>> initial config register values and I messed up there. Can you send me the output
>>>>>> of i2cdump (word wide) ?
>>>>>>
>>>>>
>>>>> Before 5 patches:
>>>>>> # i2cdump -f 0 0x48 w
>>>>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
>>>
>>> [ ... ]
>>>>>
>>>>> After 5 patches:
>>>>>>  i2cdump -y 0 0x48 w
>>>>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
>>>
>>> [ .... ]
>>>
>>>> I can try and debug the series once I get some spare time, might be
>>>> over the weekend or next week.
>>>
>>> The register map, at least the initial one, is pretty much the same as mine
>>> and as expected. The configuration register in the second map is messed up,
>>> possible because of a write with wrong endianness.
>>
>> Got a few mins skipping lunch.. ;)
>>
>> I did a quick bisect, and it is indeed the patch #5 that breaks.
>> added http://pastebin.ubuntu.com/17812044/ and got:
>>
>> tmp102 0-0048: regval = 0x0000ffff
>>
>> That was weird. Does'nt look like endian-ness swap thingy
>>
>> So, did the following hack to see all messages flowing in and out from
>> 0x48 at bus controller driver level: http://pastebin.ubuntu.com/17813093/
>> / # dmesg|grep XXX
>> / #
>>
>> Before patch #5 (and on next-20160624):
>> the same diff gave:
>> http://pastebin.ubuntu.com/17813303/
>>
>>
>>
>>> I bet the regmap_read() of the configuration register returns 0xa060 (or
>>> 0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
>>> it would be great if you can confirm by printing the register value with
>>> the "unexpected config register value" message (guess I should have left
>>> that in place - I used it during testing ;-).
>>>
>>> If that is the case, I'll probably have to drop the regmap changes, at least
>>> for now. It would mean that regmap is broken for chips like the LM75 (ie
>>> for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.
>>
>> It does look like everything is getting cached out and no actual reads
>> are actually getting through to the bus controller driver even.
>>
> Yes, that is really weird. It also seems to fill the cache with 0xffff,
> which is even more weird.
> 
> Ok, looks like converting drivers to regmap isn't a good idea. I'll need
> to get a system with an adapter supporting I2C_FUNC_I2C and do some more
> testing.
> 
>> I tested on next-20160624 and used omap2plus_defconfig for the test.
>>
> 
> Thanks a lot for the information, and for testing this with your system.


here is more:
http://pastebin.ubuntu.com/17814261/

Looks like return for is_writable etc should probably be true or false

[   32.609935] at24 0-0050: 4096 byte 24c32 EEPROM, writable, 1
bytes/write
[   32.751560] rtc-ds1307 2-006f: SET TIME!
[   32.857593] palmas_pwrbutton
48070000.i2c:tps659038@58:tps659038_pwr_button: h/w controlled
shutdown duration=12 s
econds
[   33.047265] rtc-ds1307 2-006f: rtc core: registered mcp7941x as rtc0
[   33.135774] tmp102 0-0048: No cache defaults, reading back from HW
[   33.538655] rtc-ds1307 2-006f: 64 bytes nvram
[   34.202107] omap_rng 48090000.rng: _od_fail_runtime_resume: FIXME:
missing hwmod/omap_dev info
[   34.211191] omap_rng 48090000.rng: Failed to runtime_get device: -19
[   34.217869] omap_rng 48090000.rng: initialization failed.
[   34.229190] omap_rtc 48838000.rtc: rtc core: registered
48838000.rtc as rtc2
[   34.371375] omap_i2c 48070000.i2c: XXX MSG[0]: add=0x0048, len: 1,
flags: 0x0
[   34.378893] omap_i2c 48070000.i2c: XXX:[0] 0x00
/ # [   34.457000] omap_i2c 48070000.i2c: XXX MSG[1]: add=0x0048, len:
8, flags: 0x1
[   34.464476] omap_i2c 48070000.i2c: XXX:[0] 0x23
[   34.469255] omap_i2c 48070000.i2c: XXX:[1] 0xa0
[   34.473999] omap_i2c 48070000.i2c: XXX:[2] 0xff
[   34.478775] omap_i2c 48070000.i2c: XXX:[3] 0xff
[   34.483518] omap_i2c 48070000.i2c: XXX:[4] 0xff
[   34.488282] omap_i2c 48070000.i2c: XXX:[5] 0xff
[   34.493022] omap_i2c 48070000.i2c: XXX:[6] 0xff
[   34.497788] omap_i2c 48070000.i2c: XXX:[7] 0xff


I probably have to stop now, since I am behind on an internal
deadline. Do let me know if you want me to dig further, I can try at a
later time..
-- 
Regards,
Nishanth Menon

Re: [PATCH 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

[Guenter Roeck]

On 06/24/2016 11:21 AM, Nishanth Menon wrote:
> On 06/24/2016 01:18 PM, Guenter Roeck wrote:
>> On Fri, Jun 24, 2016 at 01:02:32PM -0500, Nishanth Menon wrote:
>>> On 06/24/2016 11:36 AM, Guenter Roeck wrote:
>>>> On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
>>>>> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
>>>>>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>>>>>>> Hi Nishanth,
>>>>>>>
>>>>>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>>>>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>>>>>>> By registering a cleanup function with devm_add_action(), we can
>>>>>>>>> simplify the error path in the probe function and drop the remove
>>>>>>>>> function entirely.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>>>>>
>>>>>>>> I dont seem to have a cover letter to reply to... but anyways..
>>>>>>>>
>>>>>>>> Before: http://pastebin.ubuntu.com/17801376/
>>>>>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>>>>>>
>>>>>>>> Fails on beagleboard-X15 with:
>>>>>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>>>>>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>>>>>>
>>>>>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>>>>>>> initial config register values and I messed up there. Can you send me the output
>>>>>>> of i2cdump (word wide) ?
>>>>>>>
>>>>>>
>>>>>> Before 5 patches:
>>>>>>> # i2cdump -f 0 0x48 w
>>>>>>>       0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>>>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000
>>>>
>>>> [ ... ]
>>>>>>
>>>>>> After 5 patches:
>>>>>>>   i2cdump -y 0 0x48 w
>>>>>>>       0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>>>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000
>>>>
>>>> [ .... ]
>>>>
>>>>> I can try and debug the series once I get some spare time, might be
>>>>> over the weekend or next week.
>>>>
>>>> The register map, at least the initial one, is pretty much the same as mine
>>>> and as expected. The configuration register in the second map is messed up,
>>>> possible because of a write with wrong endianness.
>>>
>>> Got a few mins skipping lunch.. ;)
>>>
>>> I did a quick bisect, and it is indeed the patch #5 that breaks.
>>> added http://pastebin.ubuntu.com/17812044/ and got:
>>>
>>> tmp102 0-0048: regval = 0x0000ffff
>>>
>>> That was weird. Does'nt look like endian-ness swap thingy
>>>
>>> So, did the following hack to see all messages flowing in and out from
>>> 0x48 at bus controller driver level: http://pastebin.ubuntu.com/17813093/
>>> / # dmesg|grep XXX
>>> / #
>>>
>>> Before patch #5 (and on next-20160624):
>>> the same diff gave:
>>> http://pastebin.ubuntu.com/17813303/
>>>
>>>
>>>
>>>> I bet the regmap_read() of the configuration register returns 0xa060 (or
>>>> 0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
>>>> it would be great if you can confirm by printing the register value with
>>>> the "unexpected config register value" message (guess I should have left
>>>> that in place - I used it during testing ;-).
>>>>
>>>> If that is the case, I'll probably have to drop the regmap changes, at least
>>>> for now. It would mean that regmap is broken for chips like the LM75 (ie
>>>> for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.
>>>
>>> It does look like everything is getting cached out and no actual reads
>>> are actually getting through to the bus controller driver even.
>>>
>> Yes, that is really weird. It also seems to fill the cache with 0xffff,
>> which is even more weird.
>>
>> Ok, looks like converting drivers to regmap isn't a good idea. I'll need
>> to get a system with an adapter supporting I2C_FUNC_I2C and do some more
>> testing.
>>
>>> I tested on next-20160624 and used omap2plus_defconfig for the test.
>>>
>>
>> Thanks a lot for the information, and for testing this with your system.
>
>
> here is more:
> http://pastebin.ubuntu.com/17814261/
>
> Looks like return for is_writable etc should probably be true or false
>

But it is. The result of "reg != TMP102_TEMP_REG" is a boolean. Even
if that was not the case, the function return value is bool, so per
C standard a non-boolean return value would be auto-converted to 0 / 1.

> [   32.609935] at24 0-0050: 4096 byte 24c32 EEPROM, writable, 1
> bytes/write
> [   32.751560] rtc-ds1307 2-006f: SET TIME!
> [   32.857593] palmas_pwrbutton
> 48070000.i2c:tps659038@58:tps659038_pwr_button: h/w controlled
> shutdown duration=12 s
> econds
> [   33.047265] rtc-ds1307 2-006f: rtc core: registered mcp7941x as rtc0
> [   33.135774] tmp102 0-0048: No cache defaults, reading back from HW
> [   33.538655] rtc-ds1307 2-006f: 64 bytes nvram
> [   34.202107] omap_rng 48090000.rng: _od_fail_runtime_resume: FIXME:
> missing hwmod/omap_dev info
> [   34.211191] omap_rng 48090000.rng: Failed to runtime_get device: -19
> [   34.217869] omap_rng 48090000.rng: initialization failed.
> [   34.229190] omap_rtc 48838000.rtc: rtc core: registered
> 48838000.rtc as rtc2
> [   34.371375] omap_i2c 48070000.i2c: XXX MSG[0]: add=0x0048, len: 1,
> flags: 0x0
> [   34.378893] omap_i2c 48070000.i2c: XXX:[0] 0x00jjjj
> / # [   34.457000] omap_i2c 48070000.i2c: XXX MSG[1]: add=0x0048, len:
> 8, flags: 0x1
> [   34.464476] omap_i2c 48070000.i2c: XXX:[0] 0x23
> [   34.469255] omap_i2c 48070000.i2c: XXX:[1] 0xa0
> [   34.473999] omap_i2c 48070000.i2c: XXX:[2] 0xff
> [   34.478775] omap_i2c 48070000.i2c: XXX:[3] 0xff
> [   34.483518] omap_i2c 48070000.i2c: XXX:[4] 0xff
> [   34.488282] omap_i2c 48070000.i2c: XXX:[5] 0xff
> [   34.493022] omap_i2c 48070000.i2c: XXX:[6] 0xff
> [   34.497788] omap_i2c 48070000.i2c: XXX:[7] 0xff
>

The problem is that the read is a multi-byte read across more than one
register (length 8). That won't work. If you look closely at the result
from the above, the read returns two bytes (probably the temperature),
and then lots of 0xff, probably because the _chip_ does not understand
the i2c protocol but only the smbus protocol. This explains the configuration
register value 0xffff, which is index 2 and 3 from the read. The chip
specification mentions nothing about reads or writes crossing register
boundaries.

Ultimately, it means that regmap-i2c is broken for chips with a register
value length of 16 bit on adapters supporting I2C_FUNC_I2C. If you want to
hack regmap, you could fix the problem by changing regmap_get_i2c_bus()
as follows.

--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -257,7 +257,8 @@ static struct regmap_bus regmap_i2c_smbus_i2c_block = {
  static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
                                         const struct regmap_config *config)
  {
-       if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
+       if (config->val_bits == 8 &&
+           i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
                 return &regmap_i2c;
         else if (config->val_bits == 8 && config->reg_bits == 8 &&
                  i2c_check_functionality(i2c->adapter,
                                          I2C_FUNC_SMBUS_WORD_DATA))

Note that there is another bug in upstream regmap - I2C_FUNC_SMBUS_WORD_DATA
only works if val_bits == 8. The patch to fix that problem may not yet be
in -next. See https://patchwork.kernel.org/patch/9191185/.

> I probably have to stop now, since I am behind on an internal
> deadline. Do let me know if you want me to dig further, I can try at a

Sorry ... but thanks a lot for your help!

Guenter