Re: [PATCH 09/13] hwmon: Driver for TI TMP103 temperature sensor
From: Heiko Schocher
Date: Thu Jun 12 2014 - 05:49:39 EST
Hello Guenter,
Am 11.06.2014 19:11, schrieb Guenter Roeck:
On 06/10/2014 11:28 PM, Heiko Schocher wrote:
Driver for the TI TMP103.
The TI TMP103 is similar to the TMP102. It differs from the TMP102
by having only 8 bit registers.
Signed-off-by: Heiko Schocher <hs@xxxxxxx>
Cc: Jean Delvare <khali@xxxxxxxxxxxx>
Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx
Hi Heiko,
You don't need those Cc: in the header.
Ok, I remove them.
Why patch 09/13 ? I'd expect to see 12 more patches in this series,
Hups, sorry, my mistake, there is just this patch...
but there is only one (even on kernel.org). If you sent those
other patches to various other mailing lists, everyone will wonder
where the remaining patches are. If there is just one patch,
it is just confusing.
---
Documentation/devicetree/bindings/hwmon/tmp103 | 30 +++
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/tmp103.c | 281 +++++++++++++++++++++++++
4 files changed, 322 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/tmp103
create mode 100644 drivers/hwmon/tmp103.c
diff --git a/Documentation/devicetree/bindings/hwmon/tmp103 b/Documentation/devicetree/bindings/hwmon/tmp103
new file mode 100644
index 0000000..36d7b36
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/tmp103
Should be Documentation/hwmon/tmp103.
Hmm.. I did this first there, but "scripts/checkpatch.pl"
warns (if using a DT based board with compatible = "ti,tmp103"):
WARNING: DT compatible string "ti,tmp103" appears un-documented -- check ./Documentation/devicetree/bindings/
#324: FILE: arch/arm/boot/dts/imx6qdl-aristainetos.dtsi:107:
+ compatible = "ti,tmp103";
@@ -0,0 +1,30 @@
+Kernel driver tmp103
+====================
+
+Supported chips:
+ * Texas Instruments TMP103
+ Prefix: 'tmp103'
+ Addresses scanned: none
+ Product info and datasheet: http://www.ti.com/product/tmp103
+
+Author:
+ Heiko Schocher <hs@xxxxxxx>
+
+Description
+-----------
+
+The TMP103 is a digital output temperature sensor in a four-ball
+wafer chip-scale package (WCSP). The TMP103 is capable of reading
+temperatures to a resolution of 1ÂC. The TMP103 is specified for
+operation over a temperature range of â40ÂC to +125ÂC.
+
+Resolution: 8 Bits
+Accuracy: Â1ÂC Typ (â10ÂC to +100ÂC)
+
+The driver provides the common sysfs-interface for temperatures (see
+Documentation/hwmon/sysfs-interface under Temperatures).
+
+Required node properties:
+- compatible: manufacturer and chip name
+ "ti,tmp103"
+- reg: I2C bus address of the device
You don't need this part, and it is not really correct (there are
other ways to instantiate the device, devicetree is just one of them).
Ok.
I would suggest to refer to Documentation/i2c/instantiating-devices
instead. It might also make sense to update
Documentation/devicetree/bindings/i2c/trivial-devices.txt.
Ah, ok, good idea, this removes also the checkpatch warning, thanks!
Changed.
Also, whenever you touch any of the the dt files, you need to copy
the DT maintainers. scripts/get_maintainer.pl will tell you who needs
to be copied.
Ok.
[...]
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 11798ad..8e2f6a2 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -134,6 +134,7 @@ obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
obj-$(CONFIG_SENSORS_TMP102) += tmp102.o
+obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
diff --git a/drivers/hwmon/tmp103.c b/drivers/hwmon/tmp103.c
new file mode 100644
index 0000000..0bd33d6
--- /dev/null
+++ b/drivers/hwmon/tmp103.c
@@ -0,0 +1,281 @@
+/*
+ * Texas Instruments TMP103 SMBus temperature sensor driver
+ * Copyright (C) 2014 Heiko Schocher <hs@xxxxxxx>
+ *
+ * Based on:
+ * Texas Instruments TMP102 SMBus temperature sensor driver
+ *
+ * Copyright (C) 2010 Steven King <sfking@xxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/jiffies.h>
+
+#define DRIVER_NAME "tmp103"
+
+#define TMP103_TEMP_REG 0x00
+#define TMP103_CONF_REG 0x01
+#define TMP103_TLOW_REG 0x02
+#define TMP103_THIGH_REG 0x03
+
+#define TMP103_CONF_M0 0x01
+#define TMP103_CONF_M1 0x02
+#define TMP103_CONF_LC 0x04
+#define TMP103_CONF_FL 0x08
+#define TMP103_CONF_FH 0x10
+#define TMP103_CONF_CR0 0x20
+#define TMP103_CONF_CR1 0x40
+#define TMP103_CONF_ID 0x80
+#define TMP103_CONF_SD (TMP103_CONF_M0 | TMP103_CONF_M1)
+
+struct tmp103 {
+ struct device *hwmon_dev;
+ struct mutex lock;
+ u16 config_orig;
+ unsigned long last_update;
+ int temp[3];
+};
+
+static inline int tmp103_reg_to_mC(s8 val)
+{
+ return val * 1000;
+}
+
+static inline u8 tmp103_mC_to_reg(int val)
+{
+ return val / 1000;
DIV_ROUND_CLOSEST() ?
No camelCase, please.
Ok ... hmm, wondering that checkpatch.pl not claimed this.
removed.
+}
+
+static const u8 tmp103_reg[] = {
+ TMP103_TEMP_REG,
+ TMP103_TLOW_REG,
+ TMP103_THIGH_REG,
+};
+
+static struct tmp103 *tmp103_update_device(struct i2c_client *client)
+{
+ struct tmp103 *tmp103 = i2c_get_clientdata(client);
+
+ mutex_lock(&tmp103->lock);
+ if (time_after(jiffies, tmp103->last_update + HZ / 3)) {
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(tmp103->temp); ++i) {
+ int status = i2c_smbus_read_byte_data(client,
+ tmp103_reg[i]);
+ if (status > -1)
+ tmp103->temp[i] = tmp103_reg_to_mC(status);
I understand this is from the tmp102 driver, but it is kind of unusual
because it does not report the error back to the user. Please consider
doing that, ie return PTR_ERR(status) on error and check/return the error
from the calling functions. Irrelevant if you use regmap (see below).
you mean ERR_PTR() ?
Ok, add this.
+ }
+ tmp103->last_update = jiffies;
+ }
+ mutex_unlock(&tmp103->lock);
+ return tmp103;
+}
Overall you might consider dropping the update function and using regmap instead.
It would be an excellent fit for this driver; while it doesn't do timed caching
it supports per-register caching which is ultimately more useful anyway.
Some of the drivers in the hwmon directory use it, so you could use that
as example. Not mandatory, though; just a suggestion.
Could you give me an example?
+
+static ssize_t tmp103_show_temp(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+ struct tmp103 *tmp103 = tmp103_update_device(to_i2c_client(dev));
+
+ return sprintf(buf, "%d\n", tmp103->temp[sda->index]);
+}
+
+static ssize_t tmp103_set_temp(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tmp103 *tmp103 = i2c_get_clientdata(client);
+ long val;
+ int status;
+
+ if (kstrtol(buf, 10, &val) < 0)
+ return -EINVAL;
+ val = clamp_val(val, -55000, 128000);
Max should be 127000.
Ok, change this.
+
+ mutex_lock(&tmp103->lock);
+ tmp103->temp[sda->index] = val;
You can not do that since it is the non-rounded value. You have to convert to
the register value, then convert back to the display value.
If you use regmap, the problem goes away, since you would not cache converted
data but convert in the show function. Alternatively, you could store register
values and convert in the show function.
I take a look at this.
+ status = i2c_smbus_write_byte_data(client, tmp103_reg[sda->index],
+ tmp103_mC_to_reg(val));
Please make sure that continuation lines match the '(' in the line above.
Alignments are inconsistent, otherwise I would not mention it.
checkpatch --strict tells you which ones are misaligned.
Ah, did a checkpatch but without the "--strict" option !
I fix this globally.
+ mutex_unlock(&tmp103->lock);
+ return status ? : count;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp103_show_temp, NULL , 0);
+
If you use regmap you can store the register directly here and you would not
need tmp103_reg[].
Ok.
+static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, tmp103_show_temp,
+ tmp103_set_temp, 1);
+
Per the datasheet this should be temp1_min. The register definition is different to TMP102.
changed.
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, tmp103_show_temp,
+ tmp103_set_temp, 2);
+
+static struct attribute *tmp103_attributes[] = {
+ &sensor_dev_attr_temp1_input.dev_attr.attr,
+ &sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
+ &sensor_dev_attr_temp1_max.dev_attr.attr,
You might also consider adding temp1_min_alarm and temp1_max_alarm to inform
user space if limits are exceeded. The status is reported with the FL and FH
bits in the configuration register.
Add this if I find time, or in an another patch...
+ NULL
+};
+
+static const struct attribute_group tmp103_attr_group = {
+ .attrs = tmp103_attributes,
+};
+
Please use the ATTRIBUTE_GROUPS() macro.
Ok.
+#define TMP103_CONFIG (TMP103_CONF_CR1 | TMP103_CONF_M1)
+#define TMP103_CONFIG_RD_ONLY (TMP103_CONF_CR1 | TMP103_CONF_M1)
Latter define is not used as far as I can see. Since it is the same
it is unnecessary anyway.
removed.
+
+static int tmp103_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct tmp103 *tmp103;
+ int status;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA)) {
+ dev_err(&client->dev,
+ "adapter doesn't support SMBus word transactions\n");
Good that the chip doesn't support any ;-). Check for byte transactions instead.
Heh... changed.
+ return -ENODEV;
+ }
+
+ tmp103 = devm_kzalloc(&client->dev, sizeof(*tmp103), GFP_KERNEL);
+ if (!tmp103)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, tmp103);
+
+ status = i2c_smbus_read_byte_data(client, TMP103_CONF_REG);
+ if (status < 0) {
+ dev_err(&client->dev, "error reading config register\n");
+ return status;
+ }
+ tmp103->config_orig = status;
You don't use config_orig outside the probe function,
so storing it in private data doesn't add any value.
Alternatively, you could restore the original configuration on exit.
I add "restoring it on exit."
+ status = i2c_smbus_write_byte_data(client, TMP103_CONF_REG,
+ TMP103_CONFIG);
+ if (status < 0) {
+ dev_err(&client->dev, "error writing config register\n");
+ goto fail_restore_config;
+ }
+ status = i2c_smbus_read_byte_data(client, TMP103_CONF_REG);
+ if (status < 0) {
+ dev_err(&client->dev, "error reading config register\n");
+ goto fail_restore_config;
+ }
+ if (status != TMP103_CONFIG) {
+ dev_err(&client->dev, "config settings did not stick\n");
+ status = -ENODEV;
+ goto fail_restore_config;
+ }
+ tmp103->last_update = jiffies - HZ;
+ mutex_init(&tmp103->lock);
+
+ status = sysfs_create_group(&client->dev.kobj, &tmp103_attr_group);
+ if (status) {
+ dev_dbg(&client->dev, "could not create sysfs files\n");
+ goto fail_restore_config;
+ }
+ tmp103->hwmon_dev = hwmon_device_register(&client->dev);
+ if (IS_ERR(tmp103->hwmon_dev)) {
+ dev_dbg(&client->dev, "unable to register hwmon device\n");
+ status = PTR_ERR(tmp103->hwmon_dev);
+ goto fail_remove_sysfs;
+ }
Please use devm_hwmon_device_register_with_groups().
Ok.
+
+ dev_info(&client->dev, "initialized\n");
Please drop this message.
Ok.
+
+ return 0;
+
+fail_remove_sysfs:
+ sysfs_remove_group(&client->dev.kobj, &tmp103_attr_group);
+fail_restore_config:
+ i2c_smbus_write_word_swapped(client, TMP103_CONF_REG,
+ tmp103->config_orig);
i2c_smbus_write_byte_data() ?
Yes, changed!
+ return status;
+}
+
+static int tmp103_remove(struct i2c_client *client)
+{
+ struct tmp103 *tmp103 = i2c_get_clientdata(client);
+
+ hwmon_device_unregister(tmp103->hwmon_dev);
+ sysfs_remove_group(&client->dev.kobj, &tmp103_attr_group);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int tmp103_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int config;
+
+ config = i2c_smbus_read_word_swapped(client, TMP103_CONF_REG);
byte
+ if (config < 0)
+ return config;
+
+ config &= ~TMP103_CONF_SD;
+ return i2c_smbus_write_word_swapped(client, TMP103_CONF_REG, config);
byte
+}
+
+static int tmp103_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int config;
+
+ config = i2c_smbus_read_word_swapped(client, TMP103_CONF_REG);
byte
+ if (config < 0)
+ return config;
+
+ config |= TMP103_CONF_SD;
+ return i2c_smbus_write_word_swapped(client, TMP103_CONF_REG, config);
byte
changed.
+}
+
+static const struct dev_pm_ops tmp103_dev_pm_ops = {
+ .suspend = tmp103_suspend,
+ .resume = tmp103_resume,
+};
+
+#define TMP103_DEV_PM_OPS (&tmp103_dev_pm_ops)
+#else
+#define TMP103_DEV_PM_OPS NULL
+#endif /* CONFIG_PM */
+
+static const struct i2c_device_id tmp103_id[] = {
+ { DRIVER_NAME, 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, tmp103_id);
+
+static struct i2c_driver tmp103_driver = {
+ .driver.name = DRIVER_NAME,
+ .driver.pm = TMP103_DEV_PM_OPS,
.driver = {
.name = DRIVER_NAME,
.pm = TMP103_DEV_PM_OPS,
},
would be a bit nicer. I won't mandate it, though.
changed.
+ .probe = tmp103_probe,
+ .remove = tmp103_remove,
+ .id_table = tmp103_id,
+};
+
+module_i2c_driver(tmp103_driver);
+
+MODULE_AUTHOR("Heiko Schocher <hs@xxxxxxx>");
+MODULE_DESCRIPTION("Texas Instruments TMP103 temperature sensor driver");
+MODULE_LICENSE("GPL");
Thanks for this great review!
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/