All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function
@ 2016-06-26  2:40 Guenter Roeck
  2016-06-26  2:40 ` [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address Guenter Roeck
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26  2:40 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Nishanth Menon, linux-hwmon, linux-kernel, 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>
---
v2: No change

 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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address
  2016-06-26  2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
@ 2016-06-26  2:40 ` Guenter Roeck
  2016-06-26 13:32   ` Nishanth Menon
  2016-06-26  2:40 ` [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay Guenter Roeck
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26  2:40 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Nishanth Menon, linux-hwmon, linux-kernel, Guenter Roeck

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

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: No change

 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay
  2016-06-26  2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
  2016-06-26  2:40 ` [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address Guenter Roeck
@ 2016-06-26  2:40 ` Guenter Roeck
  2016-06-26 13:31   ` Nishanth Menon
  2016-06-26  2:40 ` [PATCH v2 4/5] hwmon: (tmp102) Rework chip configuration Guenter Roeck
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26  2:40 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Nishanth Menon, linux-hwmon, linux-kernel, 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>
---
v2: No change

 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 4/5] hwmon: (tmp102) Rework chip configuration
  2016-06-26  2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
  2016-06-26  2:40 ` [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address Guenter Roeck
  2016-06-26  2:40 ` [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay Guenter Roeck
@ 2016-06-26  2:40 ` Guenter Roeck
  2016-06-26  2:40 ` [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache Guenter Roeck
  2016-06-26 13:32 ` [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Nishanth Menon
  4 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26  2:40 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Nishanth Menon, linux-hwmon, linux-kernel, 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>
---
v2: No change

 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache
  2016-06-26  2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
                   ` (2 preceding siblings ...)
  2016-06-26  2:40 ` [PATCH v2 4/5] hwmon: (tmp102) Rework chip configuration Guenter Roeck
@ 2016-06-26  2:40 ` Guenter Roeck
  2016-06-26 13:27   ` Nishanth Menon
  2016-06-26 13:32 ` [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Nishanth Menon
  4 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26  2:40 UTC (permalink / raw)
  To: Jean Delvare
  Cc: Nishanth Menon, linux-hwmon, linux-kernel, Guenter Roeck, Mark Brown

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.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Set use_single_rw to indicate that the chip can not perform
    read operations crossing register boundaries.
    Use REGCACHE_RBTREE instead REGCACHE_FLAT. REGCACHE_FLAT does not
    initialize the cache from the chip unless num_reg_defaults_raw is
    provided. If it is provided without actual cache values, it complains
    with 'No cache defaults, reading back from HW'. REGCACHE_RBTREE
    automatically initializes register values from the chip if no
    defaults are provided, and does not complain about it.

Note: I'll drop the Cc: to Mark later.

 drivers/hwmon/tmp102.c | 156 +++++++++++++++++++++++++------------------------
 1 file changed, 79 insertions(+), 77 deletions(-)

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index 487b4ef5992c..e4a2314a2c49 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>
 
@@ -61,13 +62,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 +79,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 +92,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;
-}
-
-static int tmp102_read_temp(void *dev, int *temp)
-{
-	struct tmp102 *tmp102 = tmp102_update_device(dev);
+	ret = regmap_read(tmp102->regmap, TMP102_TEMP_REG, &reg);
+	if (ret < 0)
+		return ret;
 
-	*temp = tmp102->temp[0];
+	*temp = tmp102_reg_to_mC(reg);
 
 	return 0;
 }
@@ -131,9 +106,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;
+
+	/* 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->temp[sda->index]);
+	return sprintf(buf, "%d\n", tmp102_reg_to_mC(reg));
 }
 
 static ssize_t tmp102_set_temp(struct device *dev,
@@ -142,29 +132,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 +168,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,
+	.writeable_reg = tmp102_is_writeable_reg,
+	.volatile_reg = tmp102_is_volatile_reg,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.cache_type = REGCACHE_RBTREE,
+	.use_single_rw = true,
+};
+
 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 +214,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 +272,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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache
  2016-06-26  2:40 ` [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache Guenter Roeck
@ 2016-06-26 13:27   ` Nishanth Menon
  2016-06-26 14:17     ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Nishanth Menon @ 2016-06-26 13:27 UTC (permalink / raw)
  To: Guenter Roeck, Jean Delvare; +Cc: linux-hwmon, linux-kernel, Mark Brown

On 06/25/2016 09:40 PM, Guenter Roeck wrote:
> 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.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Set use_single_rw to indicate that the chip can not perform
>     read operations crossing register boundaries.
>     Use REGCACHE_RBTREE instead REGCACHE_FLAT. REGCACHE_FLAT does not
>     initialize the cache from the chip unless num_reg_defaults_raw is
>     provided. If it is provided without actual cache values, it complains
>     with 'No cache defaults, reading back from HW'. REGCACHE_RBTREE
>     automatically initializes register values from the chip if no
>     defaults are provided, and does not complain about it.
> 
> Note: I'll drop the Cc: to Mark later.
> 
>  drivers/hwmon/tmp102.c | 156 +++++++++++++++++++++++++------------------------
>  1 file changed, 79 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
> index 487b4ef5992c..e4a2314a2c49 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>
>  
> @@ -61,13 +62,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 +79,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 +92,11 @@ static struct tmp102 *tmp102_update_device(struct device *dev)
>  		msleep(jiffies_to_msecs(sleeptime));
>  	}


Note, at this point:
We have set CR0=0, CR1=1 (4HZ conversion rate). we do indeed have a
26typical (worst case 35ms) conversion time, but if we read register
before 250 ms, we are not getting a new data, instead, we are just
reading the same old register data.

for lowering potential i2c ops, I suggest:
if < conversion_rate + CONVERSION_TIME_MS, provide a cached data, if
after that, do a read.

we could do a patch over this ofcourse -> best will  be to let us do a
configurable conversion rate. We could get upto 8Hz conversion rate
with this chip (CR0,1=1).



Otherwise, this series worked just fine..
http://pastebin.ubuntu.com/17907605/

-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay
  2016-06-26  2:40 ` [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay Guenter Roeck
@ 2016-06-26 13:31   ` Nishanth Menon
  2016-06-26 14:02     ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Nishanth Menon @ 2016-06-26 13:31 UTC (permalink / raw)
  To: Guenter Roeck, Jean Delvare
  Cc: linux-hwmon, linux-kernel, Eduardo Valentin, Zhang Rui

On 06/25/2016 09:40 PM, Guenter Roeck wrote:
[...]
>  /* 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));
> +	}
> +

While msleep can indeed work and simplify, in case of usage for
example with thermal framework, if the data is not ready and we return
-EAGAIN, it lets the thermal framework go and read other sensors
instead of being blocked on the tmp102 conversion of data.

Eduardo, Rui: what is your view on this approach?
Patch: https://patchwork.kernel.org/patch/9198961/

-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function
  2016-06-26  2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
                   ` (3 preceding siblings ...)
  2016-06-26  2:40 ` [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache Guenter Roeck
@ 2016-06-26 13:32 ` Nishanth Menon
  4 siblings, 0 replies; 11+ messages in thread
From: Nishanth Menon @ 2016-06-26 13:32 UTC (permalink / raw)
  To: Guenter Roeck, Jean Delvare; +Cc: linux-hwmon, linux-kernel

On 06/25/2016 09:40 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>
Acked-by: Nishanth Menon <nm@ti.com>


-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address
  2016-06-26  2:40 ` [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address Guenter Roeck
@ 2016-06-26 13:32   ` Nishanth Menon
  0 siblings, 0 replies; 11+ messages in thread
From: Nishanth Menon @ 2016-06-26 13:32 UTC (permalink / raw)
  To: Guenter Roeck, Jean Delvare; +Cc: linux-hwmon, linux-kernel

On 06/25/2016 09:40 PM, Guenter Roeck wrote:
> The FSF address can change, so drop it from the driver.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Acked-by: Nishanth Menon <nm@ti.com>


-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay
  2016-06-26 13:31   ` Nishanth Menon
@ 2016-06-26 14:02     ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26 14:02 UTC (permalink / raw)
  To: Nishanth Menon, Jean Delvare
  Cc: linux-hwmon, linux-kernel, Eduardo Valentin, Zhang Rui

On 06/26/2016 06:31 AM, Nishanth Menon wrote:
> On 06/25/2016 09:40 PM, Guenter Roeck wrote:
> [...]
>>   /* 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));
>> +	}
>> +
>
> While msleep can indeed work and simplify, in case of usage for
> example with thermal framework, if the data is not ready and we return
> -EAGAIN, it lets the thermal framework go and read other sensors
> instead of being blocked on the tmp102 conversion of data.
>

My thought was that returning -EAGAIN was more appropriate for the
1/3s wait time we had earlier. With 35ms, it is much more questionable
if we hit this condition in the first place than it was before,
and the impact if waiting is much less severe. Furthermore,
it is no longer unconditional but only occurs if the sensor
was in fact disabled.

But, yes, you have a point. I do prefer to wait, but if Rui and you
both agree that we should return -EAGAIN, I'll be happy to change
the code back to the old behavior.

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache
  2016-06-26 13:27   ` Nishanth Menon
@ 2016-06-26 14:17     ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26 14:17 UTC (permalink / raw)
  To: Nishanth Menon, Jean Delvare; +Cc: linux-hwmon, linux-kernel, Mark Brown

Hi Nishanth,

On 06/26/2016 06:27 AM, Nishanth Menon wrote:
>
> Note, at this point:
> We have set CR0=0, CR1=1 (4HZ conversion rate). we do indeed have a
> 26typical (worst case 35ms) conversion time, but if we read register
> before 250 ms, we are not getting a new data, instead, we are just
> reading the same old register data.
>
Yes, I know. As I said in the description of patch 5, there is a
trade-off between caching provided by regmap for non-volatile
registers and internally managed caching.

Question for me is the practical impact. It is quite uncommon for
applications to actually read the attribute that often.

> for lowering potential i2c ops, I suggest:
> if < conversion_rate + CONVERSION_TIME_MS, provide a cached data, if
> after that, do a read.
>
That would be a possibility (we could specifically cache only the
temperature register), but I am not sure if it is worth it.

Caching was more important when the driver kept reading all registers,
which was more time consuming and also happened if only a single attribute
was read. Since the non-volatile registers are now cached by regmap,
this won't happen anymore, and reading a single register is not as
expensive as reading four. In practice, that means the amount of i2c
reads is only larger than before if an application polls the temperature
more than 10 times per second.

Do you think there is an application which does poll the temperature
that often ?

> we could do a patch over this ofcourse -> best will  be to let us do a
> configurable conversion rate. We could get upto 8Hz conversion rate
> with this chip (CR0,1=1).
>
Adding such an attribute might be worthwhile, if for nothing else to
reduce power consumption if high speed conversions are not needed.
I actually thought about it, but my ultimate goal was elsewhere -
the series served as baseline to test the new registration API
(http://www.spinics.net/lists/kernel/msg2288309.html), and I didn't
want to get too distracted.

If we add that attribute, it might be easier to do it on top of the
driver converted to the new API, though.

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-06-26 14:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-26  2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
2016-06-26  2:40 ` [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address Guenter Roeck
2016-06-26 13:32   ` Nishanth Menon
2016-06-26  2:40 ` [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay Guenter Roeck
2016-06-26 13:31   ` Nishanth Menon
2016-06-26 14:02     ` Guenter Roeck
2016-06-26  2:40 ` [PATCH v2 4/5] hwmon: (tmp102) Rework chip configuration Guenter Roeck
2016-06-26  2:40 ` [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache Guenter Roeck
2016-06-26 13:27   ` Nishanth Menon
2016-06-26 14:17     ` Guenter Roeck
2016-06-26 13:32 ` [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Nishanth Menon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.