* [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml
@ 2022-02-09 13:36 Agathe Porte
2022-02-09 13:36 ` [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip Agathe Porte
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Agathe Porte @ 2022-02-09 13:36 UTC (permalink / raw)
To: linux-hwmon; +Cc: Guenter Roeck, Jean Delvare, Krzysztof Adamski, Agathe Porte
Add basic description of the tmp464 driver DT bindings.
Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
---
.../devicetree/bindings/hwmon/ti,tmp464.yaml | 106 ++++++++++++++++++
1 file changed, 106 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
new file mode 100644
index 000000000000..aaee652c0067
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
@@ -0,0 +1,106 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ti,tmp464.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TMP464 temperature sensor
+
+maintainers:
+ - Guenter Roeck <linux@roeck-us.net>
+
+description: |
+ ±0.0625°C Remote and Local temperature sensor
+ https://www.ti.com/lit/ds/symlink/tmp464.pdf
+
+properties:
+ compatible:
+ enum:
+ - ti,tmp464
+ reg:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+patternProperties:
+ "^channel@([0-3])$":
+ type: object
+ description: |
+ Represents channels of the device and their specific configuration.
+
+ properties:
+ reg:
+ description: |
+ The channel number. 0 is local channel, 1-3 are remote channels
+ items:
+ minimum: 0
+ maximum: 3
+
+ label:
+ description: |
+ A descriptive name for this channel, like "ambient" or "psu".
+
+ ti,n-factor:
+ description: |
+ The value (two's complement) to be programmed in the channel specific N correction register.
+ For remote channels only.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ items:
+ minimum: 0
+ maximum: 255
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor@4b {
+ compatible = "ti,tmp464";
+ reg = <0x4b>;
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor@4b {
+ compatible = "ti,tmp464";
+ reg = <0x4b>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 {
+ reg = <0x0>;
+ ti,n-factor = <0x1>;
+ label = "local";
+ };
+
+ channel@1 {
+ reg = <0x1>;
+ ti,n-factor = <0x0>;
+ label = "somelabel";
+ };
+
+ channel@2 {
+ reg = <0x2>;
+ status = "disabled";
+ };
+ };
+ };
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-09 13:36 [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml Agathe Porte
@ 2022-02-09 13:36 ` Agathe Porte
2022-02-09 14:46 ` Guenter Roeck
2022-02-09 15:11 ` [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml Guenter Roeck
2022-02-09 15:53 ` [PATCH v2 " Agathe Porte
2 siblings, 1 reply; 14+ messages in thread
From: Agathe Porte @ 2022-02-09 13:36 UTC (permalink / raw)
To: linux-hwmon; +Cc: Guenter Roeck, Jean Delvare, Krzysztof Adamski, Agathe Porte
Add support for Texas Instruments TMP464 temperature sensor IC.
TI's TMP464 is an I2C temperature sensor chip. This chip is
similar to TI's TMP421 chip, but with 16bit-wide registers (instead
of 8bit-wide registers). The chip have one local sensor and four
remote sensors.
Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
Acked-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
Documentation/hwmon/tmp464.rst | 43 +++
MAINTAINERS | 7 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/tmp464.c | 472 +++++++++++++++++++++++++++++++++
5 files changed, 533 insertions(+)
create mode 100644 Documentation/hwmon/tmp464.rst
create mode 100644 drivers/hwmon/tmp464.c
diff --git a/Documentation/hwmon/tmp464.rst b/Documentation/hwmon/tmp464.rst
new file mode 100644
index 000000000000..8990554194de
--- /dev/null
+++ b/Documentation/hwmon/tmp464.rst
@@ -0,0 +1,43 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver tmp421
+====================
+
+Supported chips:
+
+ * Texas Instruments TMP464
+
+ Prefix: 'tmp464'
+
+ Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
+
+ Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp464.html
+
+Authors:
+
+ Agathe Porte <agathe.porte@nokia.com>
+
+Description
+-----------
+
+This driver implements support for Texas Instruments TMP464
+temperature sensor chip. This chip implement one local four remote
+sensors. Temperature is measured in degrees Celsius. The chips are
+wired over I2C/SMBus and specified over a temperature range of -40 to
++125 degrees Celsius. Resolution for both the local and remote
+channels is 0.0625 degree C.
+
+The chips support only temperature measurement. The driver exports
+the temperature values via the following sysfs files:
+
+**temp[1-5]_input**
+
+Each sensor can be individually disabled via Devicetree or from sysfs
+via:
+
+**temp[1-5]_enable**
+
+If labels were specified in Devicetree, additional sysfs files will
+be present:
+
+**temp[1-5]_label**
diff --git a/MAINTAINERS b/MAINTAINERS
index 3e461db9cd91..fa0b27a8fe28 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19457,6 +19457,13 @@ S: Maintained
F: Documentation/hwmon/tmp401.rst
F: drivers/hwmon/tmp401.c
+TMP464 HARDWARE MONITOR DRIVER
+M: Agathe Porte <agathe.porte@nokia.com>
+L: linux-hwmon@vger.kernel.org
+S: Maintained
+F: Documentation/hwmon/tmp464.rst
+F: drivers/hwmon/tmp464.c
+
TMP513 HARDWARE MONITOR DRIVER
M: Eric Tremblay <etremblay@distech-controls.com>
L: linux-hwmon@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 8df25f1079ba..52b4f5688b45 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1979,6 +1979,16 @@ config SENSORS_TMP421
This driver can also be built as a module. If so, the module
will be called tmp421.
+config SENSORS_TMP464
+ tristate "Texas Instruments TMP464 and compatible"
+ depends on I2C
+ help
+ If you say yes here you get support for Texas Instruments TMP464
+ temperature sensor chip.
+
+ This driver can also be built as a module. If so, the module
+ will be called tmp464.
+
config SENSORS_TMP513
tristate "Texas Instruments TMP513 and compatibles"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 185f946d698b..a1f2d6686227 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
obj-$(CONFIG_SENSORS_TMP108) += tmp108.o
obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
+obj-$(CONFIG_SENSORS_TMP464) += tmp464.o
obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
obj-$(CONFIG_SENSORS_VEXPRESS) += vexpress-hwmon.o
obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
new file mode 100644
index 000000000000..f294e12b1e39
--- /dev/null
+++ b/drivers/hwmon/tmp464.c
@@ -0,0 +1,472 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
+ * Supported models: TMP464
+
+ * Copyright (C) 2022 Agathe Porte <agathe.porte@nokia.com>
+ * Preliminary support by:
+ * Lionel Pouliquen <lionel.lp.pouliquen@nokia.com>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
+ I2C_CLIENT_END };
+
+enum chips { tmp464 };
+
+#define MAX_CHANNELS 5 /* chan 0 is internal, 1-4 are remote */
+
+/* TMP464 registers */
+#define TMP464_REG_THERM_STATUS 0x21
+#define TMP464_REG_THERM2_STATUS 0x22
+
+static const u8 TMP464_TEMP[MAX_CHANNELS] = { 0x00, 0x01, 0x02, 0x03, 0x04 };
+static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = { 0x39, 0x42, 0x4A, 0x52, 0x5A };
+static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = { 0x3A, 0x43, 0x4B, 0x53, 0x5B };
+static const u8 TMP464_OFFSET_REMOTE[MAX_CHANNELS] = { 0x40, 0x48, 0x50, 0x58 };
+#define TMP464_N_FACTOR_REG_1 0x41
+#define TMP464_CONFIG_REG 0x30
+
+/* alarm offset in THERM_STATUS/THERM2_STATUS register */
+#define TMP464_ALARM_BASE_NUMBER 7
+
+#define TMP464_REG_THERM_HYSTERESIS 0x38
+
+#define TMP464_MANUFACTURER_ID_REG 0xFE
+#define TMP464_DEVICE_ID_REG 0xFF
+
+/* Flags */
+#define TMP464_CONFIG_SHUTDOWN BIT(5)
+#define TMP464_CONFIG_RANGE 0x04
+#define TMP464_CONFIG_REG_REN(x) (BIT(7 + (x)))
+#define TMP464_CONFIG_REG_REN_MASK GENMASK(11, 7)
+#define TMP464_CONFIG_CONVERSION_RATE_B0 2
+#define TMP464_CONFIG_CONVERSION_RATE_MASK GENMASK(TMP464_CONFIG_CONVERSION_RATE_B0, \
+ TMP464_CONFIG_CONVERSION_RATE_B0 + 2)
+
+/* Manufacturer / Device ID's */
+#define TMP464_MANUFACTURER_ID 0x5449
+#define TMP464_DEVICE_ID 0x1468
+
+
+static const struct i2c_device_id tmp464_id[] = {
+ { "tmp464", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, tmp464_id);
+
+static const struct of_device_id __maybe_unused tmp464_of_match[] = {
+ {
+ .compatible = "ti,tmp464",
+ .data = (void *)5
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, tmp464_of_match);
+
+
+struct tmp464_channel {
+ const char *label;
+ bool enabled;
+ s16 temp;
+};
+
+struct tmp464_data {
+ struct i2c_client *client;
+ struct mutex update_lock;
+ u32 temp_config[MAX_CHANNELS + 1];
+ struct hwmon_channel_info temp_info;
+ const struct hwmon_channel_info *info[2];
+ struct hwmon_chip_info chip;
+ bool valid;
+ unsigned long last_updated;
+ unsigned long channels;
+ u16 config;
+ struct tmp464_channel channel[MAX_CHANNELS];
+};
+
+
+/* funcs */
+
+static int temp_from_raw(u16 reg)
+{
+ int temp = 0;
+
+ temp = (reg >> 8) * 2000;
+ temp += ((reg & 0xFF) >> 3) * 625 / 10;
+
+ return temp;
+}
+
+static int tmp464_update_device(struct tmp464_data *data)
+{
+ struct i2c_client *client = data->client;
+ int ret = 0;
+ int i;
+
+ mutex_lock(&data->update_lock);
+
+ if (time_after(jiffies, data->last_updated + (HZ / 2)) ||
+ !data->valid) {
+ ret = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
+ if (ret < 0)
+ goto exit;
+ data->config = ret;
+
+ for (i = 0; i < data->channels; i++) {
+ ret = i2c_smbus_read_word_swapped(client, TMP464_TEMP[i]);
+ if (ret < 0)
+ goto exit;
+ data->channel[i].temp = ret;
+ }
+ data->last_updated = jiffies;
+ data->valid = true;
+ }
+
+exit:
+ mutex_unlock(&data->update_lock);
+
+ if (ret < 0) {
+ data->valid = false;
+ return ret;
+ }
+
+ return 0;
+}
+
+static int tmp464_enable_channels(struct tmp464_data *data)
+{
+ int err;
+ struct i2c_client *client = data->client;
+ struct device *dev = &client->dev;
+ int old = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
+ int new, i;
+
+ if (old < 0) {
+ dev_err(dev, "error reading register, can't disable channels\n");
+ return old;
+ }
+
+ new = old & ~TMP464_CONFIG_REG_REN_MASK;
+ for (i = 0; i < data->channels; i++)
+ if (data->channel[i].enabled)
+ new |= TMP464_CONFIG_REG_REN(i);
+
+ if (new == old)
+ return 0;
+
+ err = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, new);
+ if (err < 0)
+ dev_err(dev, "error writing register, can't disable channels\n");
+
+ return err;
+}
+
+static int tmp464_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct tmp464_data *tmp464 = dev_get_drvdata(dev);
+ int ret = 0;
+
+ ret = tmp464_update_device(tmp464);
+ if (ret)
+ return ret;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ if (!tmp464->channel[channel].enabled)
+ return -ENODATA;
+ *val = temp_from_raw(tmp464->channel[channel].temp);
+ return 0;
+ case hwmon_temp_enable:
+ *val = tmp464->channel[channel].enabled;
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+}
+
+static int tmp464_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **str)
+{
+ struct tmp464_data *data = dev_get_drvdata(dev);
+
+ *str = data->channel[channel].label;
+
+ return 0;
+}
+
+static int tmp464_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ struct tmp464_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ switch (attr) {
+ case hwmon_temp_enable:
+ data->channel[channel].enabled = val;
+ ret = tmp464_enable_channels(data);
+ break;
+ default:
+ ret = -EOPNOTSUPP;
+ }
+
+ return ret;
+}
+
+static umode_t tmp464_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (attr) {
+ case hwmon_temp_input:
+ return 0444;
+ case hwmon_temp_label:
+ return 0444;
+ case hwmon_temp_enable:
+ return 0644;
+ default:
+ return 0;
+ }
+}
+
+static int tmp464_init_client(struct tmp464_data *data)
+{
+ int config, config_orig;
+ struct i2c_client *client = data->client;
+
+ /* Set the conversion rate to 2 Hz */
+ config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
+ if (config < 0) {
+ dev_err(&client->dev,
+ "Could not read configuration register (%d)\n", config);
+ return config;
+ }
+
+ config_orig = config;
+ config &= ~TMP464_CONFIG_CONVERSION_RATE_MASK;
+ config |= (0x05) << TMP464_CONFIG_CONVERSION_RATE_B0;
+
+ if (config != config_orig) {
+ dev_info(&client->dev, "Set conversion rate to 2 Hz\n");
+ i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
+ }
+
+ /* Start conversions (disable shutdown if necessary) */
+ config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
+ if (config < 0) {
+ dev_err(&client->dev,
+ "Could not read configuration register (%d)\n", config);
+ return config;
+ }
+
+ config_orig = config;
+ config &= ~TMP464_CONFIG_SHUTDOWN;
+
+ if (config != config_orig) {
+ dev_info(&client->dev, "Enable monitoring chip\n");
+ i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
+ }
+
+ return tmp464_enable_channels(data);
+}
+
+static int tmp464_detect(struct i2c_client *client,
+ struct i2c_board_info *info)
+{
+ enum chips kind;
+ struct i2c_adapter *adapter = client->adapter;
+ static const char * const names[] = {
+ "TMP464"
+ };
+ u8 reg;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
+ return -ENODEV;
+
+ reg = i2c_smbus_read_word_swapped(client, TMP464_MANUFACTURER_ID_REG);
+ if (reg != TMP464_MANUFACTURER_ID)
+ return -ENODEV;
+
+ /* Check for "always return zero" bits */
+ reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM_STATUS);
+ if (reg & 0x1f)
+ return -ENODEV;
+ reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM2_STATUS);
+ if (reg & 0x1f)
+ return -ENODEV;
+
+ reg = i2c_smbus_read_word_swapped(client, TMP464_DEVICE_ID_REG);
+ if (reg < 0)
+ return reg;
+ switch (reg) {
+ case TMP464_DEVICE_ID:
+ kind = tmp464;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ strscpy(info->type, tmp464_id[kind].name, I2C_NAME_SIZE);
+ dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
+ names[kind], client->addr);
+
+ return 0;
+}
+
+static int tmp464_probe_child_from_dt(struct i2c_client *client,
+ struct device_node *child,
+ struct tmp464_data *data)
+
+{
+ struct device *dev = &client->dev;
+ u32 i;
+ s32 val;
+ int err;
+
+ err = of_property_read_u32(child, "reg", &i);
+ if (err) {
+ dev_err(dev, "missing reg property of %pOFn\n", child);
+ return err;
+ }
+
+ if (i >= data->channels) {
+ dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
+ return -EINVAL;
+ }
+
+ of_property_read_string(child, "label", &data->channel[i].label);
+ if (data->channel[i].label)
+ data->temp_config[i] |= HWMON_T_LABEL;
+
+ data->channel[i].enabled = of_device_is_available(child);
+
+ err = of_property_read_s32(child, "ti,n-factor", &val);
+ if (!err) {
+ if (i == 0) {
+ dev_err(dev, "n-factor can't be set for internal channel\n");
+ return -EINVAL;
+ }
+
+ if (val > 127 || val < -128) {
+ dev_err(dev, "n-factor for channel %d invalid (%d)\n",
+ i, val);
+ return -EINVAL;
+ }
+ i2c_smbus_write_word_data(client, TMP464_N_FACTOR_REG_1 + i - 1,
+ val);
+ }
+
+ return 0;
+}
+
+static int tmp464_probe_from_dt(struct i2c_client *client, struct tmp464_data *data)
+{
+ struct device *dev = &client->dev;
+ const struct device_node *np = dev->of_node;
+ struct device_node *child;
+ int err;
+
+ for_each_child_of_node(np, child) {
+ if (strcmp(child->name, "channel"))
+ continue;
+
+ err = tmp464_probe_child_from_dt(client, child, data);
+ if (err) {
+ of_node_put(child);
+ return err;
+ }
+ }
+
+ return 0;
+}
+
+static const struct hwmon_ops tmp464_ops = {
+ .is_visible = tmp464_is_visible,
+ .read = tmp464_read,
+ .read_string = tmp464_read_string,
+ .write = tmp464_write,
+};
+
+static int tmp464_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct device *hwmon_dev;
+ struct tmp464_data *data;
+ int i, err;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_WORD_DATA)) {
+ dev_err(&client->dev, "i2c functionality check failed\n");
+ return -ENODEV;
+ }
+ data = devm_kzalloc(dev, sizeof(struct tmp464_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ mutex_init(&data->update_lock);
+ if (client->dev.of_node)
+ data->channels = (unsigned long)
+ of_device_get_match_data(&client->dev);
+ else
+ data->channels = i2c_match_id(tmp464_id, client)->driver_data;
+ data->client = client;
+
+ for (i = 0; i < data->channels; i++) {
+ data->temp_config[i] = HWMON_T_INPUT | HWMON_T_ENABLE;
+ data->channel[i].enabled = true;
+ }
+
+ err = tmp464_probe_from_dt(client, data);
+ if (err)
+ return err;
+
+ err = tmp464_init_client(data);
+ if (err)
+ return err;
+
+ data->chip.ops = &tmp464_ops;
+ data->chip.info = data->info;
+
+ data->info[0] = &data->temp_info;
+
+ data->temp_info.type = hwmon_temp;
+ data->temp_info.config = data->temp_config;
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+ data,
+ &data->chip,
+ NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct i2c_driver tmp464_driver = {
+ .class = I2C_CLASS_HWMON,
+ .driver = {
+ .name = "tmp464",
+ .of_match_table = of_match_ptr(tmp464_of_match),
+ },
+ .probe_new = tmp464_probe,
+ .id_table = tmp464_id,
+ .detect = tmp464_detect,
+ .address_list = normal_i2c,
+};
+
+module_i2c_driver(tmp464_driver);
+
+MODULE_AUTHOR("Agathe Porte <agathe.porte@nokia.com>");
+MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor driver");
+MODULE_LICENSE("GPL");
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-09 13:36 ` [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip Agathe Porte
@ 2022-02-09 14:46 ` Guenter Roeck
2022-02-09 15:45 ` Agathe Porte
0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2022-02-09 14:46 UTC (permalink / raw)
To: Agathe Porte, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
On 2/9/22 05:36, Agathe Porte wrote:
> Add support for Texas Instruments TMP464 temperature sensor IC.
>
> TI's TMP464 is an I2C temperature sensor chip. This chip is
> similar to TI's TMP421 chip, but with 16bit-wide registers (instead
> of 8bit-wide registers). The chip have one local sensor and four
> remote sensors.
>
> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
> Acked-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
> Documentation/hwmon/tmp464.rst | 43 +++
> MAINTAINERS | 7 +
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tmp464.c | 472 +++++++++++++++++++++++++++++++++
> 5 files changed, 533 insertions(+)
> create mode 100644 Documentation/hwmon/tmp464.rst
> create mode 100644 drivers/hwmon/tmp464.c
>
> diff --git a/Documentation/hwmon/tmp464.rst b/Documentation/hwmon/tmp464.rst
Needs to be added to Documentation/hwmon/index.rst.
> new file mode 100644
> index 000000000000..8990554194de
> --- /dev/null
> +++ b/Documentation/hwmon/tmp464.rst
> @@ -0,0 +1,43 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver tmp421
> +====================
> +
> +Supported chips:
> +
> + * Texas Instruments TMP464
> +
> + Prefix: 'tmp464'
> +
> + Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
> +
> + Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp464.html
> +
> +Authors:
> +
> + Agathe Porte <agathe.porte@nokia.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for Texas Instruments TMP464
> +temperature sensor chip. This chip implement one local four remote
> +sensors. Temperature is measured in degrees Celsius. The chips are
> +wired over I2C/SMBus and specified over a temperature range of -40 to
> ++125 degrees Celsius. Resolution for both the local and remote
> +channels is 0.0625 degree C.
> +
> +The chips support only temperature measurement. The driver exports
> +the temperature values via the following sysfs files:
> +
> +**temp[1-5]_input**
> +
> +Each sensor can be individually disabled via Devicetree or from sysfs
> +via:
> +
> +**temp[1-5]_enable**
> +
> +If labels were specified in Devicetree, additional sysfs files will
> +be present:
> +
> +**temp[1-5]_label**
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3e461db9cd91..fa0b27a8fe28 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19457,6 +19457,13 @@ S: Maintained
> F: Documentation/hwmon/tmp401.rst
> F: drivers/hwmon/tmp401.c
>
> +TMP464 HARDWARE MONITOR DRIVER
> +M: Agathe Porte <agathe.porte@nokia.com>
> +L: linux-hwmon@vger.kernel.org
> +S: Maintained
> +F: Documentation/hwmon/tmp464.rst
> +F: drivers/hwmon/tmp464.c
> +
> TMP513 HARDWARE MONITOR DRIVER
> M: Eric Tremblay <etremblay@distech-controls.com>
> L: linux-hwmon@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 8df25f1079ba..52b4f5688b45 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1979,6 +1979,16 @@ config SENSORS_TMP421
> This driver can also be built as a module. If so, the module
> will be called tmp421.
>
> +config SENSORS_TMP464
> + tristate "Texas Instruments TMP464 and compatible"
> + depends on I2C
> + help
> + If you say yes here you get support for Texas Instruments TMP464
> + temperature sensor chip.
> +
> + This driver can also be built as a module. If so, the module
> + will be called tmp464.
> +
> config SENSORS_TMP513
> tristate "Texas Instruments TMP513 and compatibles"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 185f946d698b..a1f2d6686227 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
> obj-$(CONFIG_SENSORS_TMP108) += tmp108.o
> obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
> obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
> +obj-$(CONFIG_SENSORS_TMP464) += tmp464.o
> obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
> obj-$(CONFIG_SENSORS_VEXPRESS) += vexpress-hwmon.o
> obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
> diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
> new file mode 100644
> index 000000000000..f294e12b1e39
> --- /dev/null
> +++ b/drivers/hwmon/tmp464.c
> @@ -0,0 +1,472 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
> + * Supported models: TMP464
> +
> + * Copyright (C) 2022 Agathe Porte <agathe.porte@nokia.com>
> + * Preliminary support by:
> + * Lionel Pouliquen <lionel.lp.pouliquen@nokia.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Unnecessary include.
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/sysfs.h>
Is this include necessary ?
Include files in alphabetic order, please.
> +
> +/* Addresses to scan */
> +static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
> + I2C_CLIENT_END };
Unnecessary continuation line.
> +
> +enum chips { tmp464 };
> +
> +#define MAX_CHANNELS 5 /* chan 0 is internal, 1-4 are remote */
> +
> +/* TMP464 registers */
> +#define TMP464_REG_THERM_STATUS 0x21
> +#define TMP464_REG_THERM2_STATUS 0x22
> +
> +static const u8 TMP464_TEMP[MAX_CHANNELS] = { 0x00, 0x01, 0x02, 0x03, 0x04 };
> +static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = { 0x39, 0x42, 0x4A, 0x52, 0x5A };
> +static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = { 0x3A, 0x43, 0x4B, 0x53, 0x5B };
> +static const u8 TMP464_OFFSET_REMOTE[MAX_CHANNELS] = { 0x40, 0x48, 0x50, 0x58 };
Most of those are unused (which would result in static analyzer warnings).
More on that below.
> +#define TMP464_N_FACTOR_REG_1 0x41
> +#define TMP464_CONFIG_REG 0x30
> +
> +/* alarm offset in THERM_STATUS/THERM2_STATUS register */
> +#define TMP464_ALARM_BASE_NUMBER 7
> +
> +#define TMP464_REG_THERM_HYSTERESIS 0x38
> +
> +#define TMP464_MANUFACTURER_ID_REG 0xFE
> +#define TMP464_DEVICE_ID_REG 0xFF
> +
> +/* Flags */
> +#define TMP464_CONFIG_SHUTDOWN BIT(5)
> +#define TMP464_CONFIG_RANGE 0x04
> +#define TMP464_CONFIG_REG_REN(x) (BIT(7 + (x)))
> +#define TMP464_CONFIG_REG_REN_MASK GENMASK(11, 7)
> +#define TMP464_CONFIG_CONVERSION_RATE_B0 2
> +#define TMP464_CONFIG_CONVERSION_RATE_MASK GENMASK(TMP464_CONFIG_CONVERSION_RATE_B0, \
> + TMP464_CONFIG_CONVERSION_RATE_B0 + 2)
> +
> +/* Manufacturer / Device ID's */
> +#define TMP464_MANUFACTURER_ID 0x5449
> +#define TMP464_DEVICE_ID 0x1468
> +
> +
No double empty lines, please.
> +static const struct i2c_device_id tmp464_id[] = {
> + { "tmp464", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, tmp464_id);
> +
> +static const struct of_device_id __maybe_unused tmp464_of_match[] = {
> + {
> + .compatible = "ti,tmp464",
> + .data = (void *)5
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, tmp464_of_match);
> +
> +
No double empty lines, please. checkpatch --strict should tell you that. Please
run and fix what it reports.
> +struct tmp464_channel {
> + const char *label;
> + bool enabled;
> + s16 temp;
> +};
> +
> +struct tmp464_data {
> + struct i2c_client *client;
> + struct mutex update_lock;
> + u32 temp_config[MAX_CHANNELS + 1];
> + struct hwmon_channel_info temp_info;
> + const struct hwmon_channel_info *info[2];
> + struct hwmon_chip_info chip;
> + bool valid;
> + unsigned long last_updated;
> + unsigned long channels;
> + u16 config;
> + struct tmp464_channel channel[MAX_CHANNELS];
> +};
> +
> +
> +/* funcs */
> +
Useless comment. Please drop.
> +static int temp_from_raw(u16 reg)
> +{
> + int temp = 0;
> +
> + temp = (reg >> 8) * 2000;
> + temp += ((reg & 0xFF) >> 3) * 625 / 10;
> +
This doesn't correctly handle negative temperatures. reg needs to be s16
to be able to handle those. Also, why not just
return (reg >> 3) * 625 / 10;
or
return DIV_ROUND_CLOSEST((reg >> 3) * 625, 10);
?
> + return temp;
> +}
> +
> +static int tmp464_update_device(struct tmp464_data *data)
> +{
> + struct i2c_client *client = data->client;
> + int ret = 0;
> + int i;
> +
> + mutex_lock(&data->update_lock);
> +
> + if (time_after(jiffies, data->last_updated + (HZ / 2)) ||
> + !data->valid) {
> + ret = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
> + if (ret < 0)
> + goto exit;
> + data->config = ret;
> +
> + for (i = 0; i < data->channels; i++) {
> + ret = i2c_smbus_read_word_swapped(client, TMP464_TEMP[i]);
> + if (ret < 0)
> + goto exit;
> + data->channel[i].temp = ret;
> + }
> + data->last_updated = jiffies;
> + data->valid = true;
> + }
> +
> +exit:
> + mutex_unlock(&data->update_lock);
> +
> + if (ret < 0) {
> + data->valid = false;
> + return ret;
> + }
> +
> + return 0;
> +}
> +
We are moving away from local caching. Please consider using regmap instead.
> +static int tmp464_enable_channels(struct tmp464_data *data)
> +{
> + int err;
> + struct i2c_client *client = data->client;
> + struct device *dev = &client->dev;
> + int old = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
> + int new, i;
> +
> + if (old < 0) {
> + dev_err(dev, "error reading register, can't disable channels\n");
> + return old;
> + }
> +
> + new = old & ~TMP464_CONFIG_REG_REN_MASK;
> + for (i = 0; i < data->channels; i++)
> + if (data->channel[i].enabled)
> + new |= TMP464_CONFIG_REG_REN(i);
> +
> + if (new == old)
> + return 0;
> +
> + err = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, new);
> + if (err < 0)
> + dev_err(dev, "error writing register, can't disable channels\n");
> +
> + return err;
> +}
> +
> +static int tmp464_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct tmp464_data *tmp464 = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + ret = tmp464_update_device(tmp464);
> + if (ret)
> + return ret;
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + if (!tmp464->channel[channel].enabled)
> + return -ENODATA;
> + *val = temp_from_raw(tmp464->channel[channel].temp);
> + return 0;
> + case hwmon_temp_enable:
> + *val = tmp464->channel[channel].enabled;
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> +}
> +
> +static int tmp464_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **str)
> +{
> + struct tmp464_data *data = dev_get_drvdata(dev);
> +
> + *str = data->channel[channel].label;
> +
> + return 0;
> +}
> +
> +static int tmp464_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + struct tmp464_data *data = dev_get_drvdata(dev);
> + int ret;
> +
> + switch (attr) {
> + case hwmon_temp_enable:
> + data->channel[channel].enabled = val;
> + ret = tmp464_enable_channels(data);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static umode_t tmp464_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + switch (attr) {
> + case hwmon_temp_input:
> + return 0444;
> + case hwmon_temp_label:
> + return 0444;
> + case hwmon_temp_enable:
> + return 0644;
> + default:
> + return 0;
> + }
> +}
> +
> +static int tmp464_init_client(struct tmp464_data *data)
> +{
> + int config, config_orig;
> + struct i2c_client *client = data->client;
> +
> + /* Set the conversion rate to 2 Hz */
> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
> + if (config < 0) {
> + dev_err(&client->dev,
> + "Could not read configuration register (%d)\n", config);
> + return config;
> + }
> +
> + config_orig = config;
> + config &= ~TMP464_CONFIG_CONVERSION_RATE_MASK;
> + config |= (0x05) << TMP464_CONFIG_CONVERSION_RATE_B0;
> +
> + if (config != config_orig) {
> + dev_info(&client->dev, "Set conversion rate to 2 Hz\n");
> + i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
> + }
> +
> + /* Start conversions (disable shutdown if necessary) */
> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
Why read this register twice ?
> + if (config < 0) {
> + dev_err(&client->dev,
> + "Could not read configuration register (%d)\n", config);
> + return config;
> + }
> +
> + config_orig = config;
> + config &= ~TMP464_CONFIG_SHUTDOWN;
> +
> + if (config != config_orig) {
> + dev_info(&client->dev, "Enable monitoring chip\n");
Please only write the configuration register once, and drop the messages.
> + i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
> + }
> +
> + return tmp464_enable_channels(data);
> +}
> +
> +static int tmp464_detect(struct i2c_client *client,
> + struct i2c_board_info *info)
> +{
> + enum chips kind;
> + struct i2c_adapter *adapter = client->adapter;
> + static const char * const names[] = {
> + "TMP464"
> + };
> + u8 reg;
Should be int to enable error checks.
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
> + return -ENODEV;
> +
> + reg = i2c_smbus_read_word_swapped(client, TMP464_MANUFACTURER_ID_REG);
> + if (reg != TMP464_MANUFACTURER_ID)
> + return -ENODEV;
> +
> + /* Check for "always return zero" bits */
> + reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM_STATUS);
> + if (reg & 0x1f)
should also check for < 0.
> + return -ENODEV;
> + reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM2_STATUS);
> + if (reg & 0x1f)
> + return -ENODEV;
Should also check for < 0.
> +
> + reg = i2c_smbus_read_word_swapped(client, TMP464_DEVICE_ID_REG);
> + if (reg < 0)
> + return reg;
That won't happen with an u8 variable.
> + switch (reg) {
> + case TMP464_DEVICE_ID:
> + kind = tmp464;
> + break;
Are there more devices with the same register set ? If not it is pointless to
have/prepare support for it.
> + default:
> + return -ENODEV;
> + }
> +
> + strscpy(info->type, tmp464_id[kind].name, I2C_NAME_SIZE);
> + dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
> + names[kind], client->addr);
> +
> + return 0;
> +}
> +
> +static int tmp464_probe_child_from_dt(struct i2c_client *client,
> + struct device_node *child,
> + struct tmp464_data *data)
> +
> +{
> + struct device *dev = &client->dev;
> + u32 i;
> + s32 val;
> + int err;
> +
> + err = of_property_read_u32(child, "reg", &i);
> + if (err) {
> + dev_err(dev, "missing reg property of %pOFn\n", child);
> + return err;
> + }
> +
> + if (i >= data->channels) {
> + dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
> + return -EINVAL;
> + }
> +
> + of_property_read_string(child, "label", &data->channel[i].label);
> + if (data->channel[i].label)
> + data->temp_config[i] |= HWMON_T_LABEL;
> +
> + data->channel[i].enabled = of_device_is_available(child);
> +
> + err = of_property_read_s32(child, "ti,n-factor", &val);
> + if (!err) {
> + if (i == 0) {
> + dev_err(dev, "n-factor can't be set for internal channel\n");
> + return -EINVAL;
> + }
> +
> + if (val > 127 || val < -128) {
> + dev_err(dev, "n-factor for channel %d invalid (%d)\n",
> + i, val);
> + return -EINVAL;
> + }
> + i2c_smbus_write_word_data(client, TMP464_N_FACTOR_REG_1 + i - 1,
> + val);
needs error check
> + }
> +
> + return 0;
> +}
> +
> +static int tmp464_probe_from_dt(struct i2c_client *client, struct tmp464_data *data)
> +{
> + struct device *dev = &client->dev;
> + const struct device_node *np = dev->of_node;
> + struct device_node *child;
> + int err;
> +
> + for_each_child_of_node(np, child) {
> + if (strcmp(child->name, "channel"))
> + continue;
> +
> + err = tmp464_probe_child_from_dt(client, child, data);
> + if (err) {
> + of_node_put(child);
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct hwmon_ops tmp464_ops = {
> + .is_visible = tmp464_is_visible,
> + .read = tmp464_read,
> + .read_string = tmp464_read_string,
> + .write = tmp464_write,
> +};
> +
> +static int tmp464_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct device *hwmon_dev;
> + struct tmp464_data *data;
> + int i, err;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_WORD_DATA)) {
> + dev_err(&client->dev, "i2c functionality check failed\n");
> + return -ENODEV;
> + }
> + data = devm_kzalloc(dev, sizeof(struct tmp464_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + mutex_init(&data->update_lock);
> + if (client->dev.of_node)
> + data->channels = (unsigned long)
> + of_device_get_match_data(&client->dev);
Unnecessary continuation line.
> + else
> + data->channels = i2c_match_id(tmp464_id, client)->driver_data;
> + data->client = client;
> +
> + for (i = 0; i < data->channels; i++) {
> + data->temp_config[i] = HWMON_T_INPUT | HWMON_T_ENABLE;
Why not support limits, temperature offsets, alarms, hysteresis, and conversion
rate configuration ? It is kind of off to support n-factor correction but
nothing else.
Overall it looks like some of the support was dropped since defines for
limit and offset registers are there, and the driver seems to be prepared
to support more chips. Why ?
> + data->channel[i].enabled = true;
> + }
> +
> + err = tmp464_probe_from_dt(client, data);
> + if (err)
> + return err;
> +
> + err = tmp464_init_client(data);
> + if (err)
> + return err;
> +
> + data->chip.ops = &tmp464_ops;
> + data->chip.info = data->info;
> +
> + data->info[0] = &data->temp_info;
> +
> + data->temp_info.type = hwmon_temp;
> + data->temp_info.config = data->temp_config;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> + data,
> + &data->chip,
> + NULL);
Please drop some of those continuation lines. We can go up to 100 columns and
should do so in situations like this.
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct i2c_driver tmp464_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = "tmp464",
> + .of_match_table = of_match_ptr(tmp464_of_match),
> + },
> + .probe_new = tmp464_probe,
> + .id_table = tmp464_id,
> + .detect = tmp464_detect,
> + .address_list = normal_i2c,
> +};
> +
> +module_i2c_driver(tmp464_driver);
> +
> +MODULE_AUTHOR("Agathe Porte <agathe.porte@nokia.com>");
> +MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml
2022-02-09 13:36 [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml Agathe Porte
2022-02-09 13:36 ` [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip Agathe Porte
@ 2022-02-09 15:11 ` Guenter Roeck
2022-02-09 15:53 ` [PATCH v2 " Agathe Porte
2 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2022-02-09 15:11 UTC (permalink / raw)
To: Agathe Porte, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
On 2/9/22 05:36, Agathe Porte wrote:
> Add basic description of the tmp464 driver DT bindings.
>
> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
> ---
> .../devicetree/bindings/hwmon/ti,tmp464.yaml | 106 ++++++++++++++++++
> 1 file changed, 106 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
> new file mode 100644
> index 000000000000..aaee652c0067
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
> @@ -0,0 +1,106 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ti,tmp464.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TMP464 temperature sensor
> +
> +maintainers:
> + - Guenter Roeck <linux@roeck-us.net>
> +
> +description: |
> + ±0.0625°C Remote and Local temperature sensor
> + https://www.ti.com/lit/ds/symlink/tmp464.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - ti,tmp464
> + reg:
> + maxItems: 1
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +patternProperties:
> + "^channel@([0-3])$":
> + type: object
> + description: |
> + Represents channels of the device and their specific configuration.
> +
> + properties:
> + reg:
> + description: |
> + The channel number. 0 is local channel, 1-3 are remote channels
> + items:
> + minimum: 0
> + maximum: 3
The chip has 1 local and 4 remote channels.
> +
> + label:
> + description: |
> + A descriptive name for this channel, like "ambient" or "psu".
> +
> + ti,n-factor:
> + description: |
> + The value (two's complement) to be programmed in the channel specific N correction register.
> + For remote channels only.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + items:
> + minimum: 0
> + maximum: 255
> +
This does not seem to match the implementation, which calls
of_property_read_s32() and accepts values from -128 .. 127.
It seems unlikely that 255 is interpreted as -1.
Hmm, is that a bug in the tmp421 driver ? I wonder if anyone
checked that, and if there is an official means to express
negative numbers in devicetree. I see other drivers use
of_property_read_s32(), but I did not find a clear explanation
how to describe negative numbers in devicetree files.
Also, I think other chip configuration properties need to be
described here as well for completeness.
- temperature offsets
- hysteresis
- temperature limits
- conversion rate
> + required:
> + - reg
> +
> + additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + sensor@4b {
> + compatible = "ti,tmp464";
> + reg = <0x4b>;
> + };
> + };
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + sensor@4b {
> + compatible = "ti,tmp464";
> + reg = <0x4b>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + channel@0 {
> + reg = <0x0>;
> + ti,n-factor = <0x1>;
> + label = "local";
> + };
> +
> + channel@1 {
> + reg = <0x1>;
> + ti,n-factor = <0x0>;
> + label = "somelabel";
> + };
> +
> + channel@2 {
> + reg = <0x2>;
> + status = "disabled";
> + };
> + };
> + };
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-09 14:46 ` Guenter Roeck
@ 2022-02-09 15:45 ` Agathe Porte
2022-02-09 17:37 ` Guenter Roeck
0 siblings, 1 reply; 14+ messages in thread
From: Agathe Porte @ 2022-02-09 15:45 UTC (permalink / raw)
To: Guenter Roeck, Agathe Porte, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
Hi Guenter,
Le 09/02/2022 à 15:46, Guenter Roeck a écrit :
> On 2/9/22 05:36, Agathe Porte wrote:
>> Add support for Texas Instruments TMP464 temperature sensor IC.
>>
>> TI's TMP464 is an I2C temperature sensor chip. This chip is
>> similar to TI's TMP421 chip, but with 16bit-wide registers (instead
>> of 8bit-wide registers). The chip have one local sensor and four
>> remote sensors.
>>
>> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
>> Acked-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> ---
>> Documentation/hwmon/tmp464.rst | 43 +++
>> MAINTAINERS | 7 +
>> drivers/hwmon/Kconfig | 10 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/tmp464.c | 472 +++++++++++++++++++++++++++++++++
>> 5 files changed, 533 insertions(+)
>> create mode 100644 Documentation/hwmon/tmp464.rst
>> create mode 100644 drivers/hwmon/tmp464.c
>>
>> diff --git a/Documentation/hwmon/tmp464.rst
>> b/Documentation/hwmon/tmp464.rst
>
> Needs to be added to Documentation/hwmon/index.rst.
ACK.
>
>> new file mode 100644
>> index 000000000000..8990554194de
>> --- /dev/null
>> +++ b/Documentation/hwmon/tmp464.rst
>> @@ -0,0 +1,43 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +Kernel driver tmp421
>> +====================
>> +
>> +Supported chips:
>> +
>> + * Texas Instruments TMP464
>> +
>> + Prefix: 'tmp464'
>> +
>> + Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
>> +
>> + Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp464.html
>> +
>> +Authors:
>> +
>> + Agathe Porte <agathe.porte@nokia.com>
>> +
>> +Description
>> +-----------
>> +
>> +This driver implements support for Texas Instruments TMP464
>> +temperature sensor chip. This chip implement one local four remote
>> +sensors. Temperature is measured in degrees Celsius. The chips are
>> +wired over I2C/SMBus and specified over a temperature range of -40 to
>> ++125 degrees Celsius. Resolution for both the local and remote
>> +channels is 0.0625 degree C.
>> +
>> +The chips support only temperature measurement. The driver exports
>> +the temperature values via the following sysfs files:
>> +
>> +**temp[1-5]_input**
>> +
>> +Each sensor can be individually disabled via Devicetree or from sysfs
>> +via:
>> +
>> +**temp[1-5]_enable**
>> +
>> +If labels were specified in Devicetree, additional sysfs files will
>> +be present:
>> +
>> +**temp[1-5]_label**
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3e461db9cd91..fa0b27a8fe28 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19457,6 +19457,13 @@ S: Maintained
>> F: Documentation/hwmon/tmp401.rst
>> F: drivers/hwmon/tmp401.c
>> +TMP464 HARDWARE MONITOR DRIVER
>> +M: Agathe Porte <agathe.porte@nokia.com>
>> +L: linux-hwmon@vger.kernel.org
>> +S: Maintained
>> +F: Documentation/hwmon/tmp464.rst
>> +F: drivers/hwmon/tmp464.c
>> +
>> TMP513 HARDWARE MONITOR DRIVER
>> M: Eric Tremblay <etremblay@distech-controls.com>
>> L: linux-hwmon@vger.kernel.org
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 8df25f1079ba..52b4f5688b45 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1979,6 +1979,16 @@ config SENSORS_TMP421
>> This driver can also be built as a module. If so, the module
>> will be called tmp421.
>> +config SENSORS_TMP464
>> + tristate "Texas Instruments TMP464 and compatible"
>> + depends on I2C
>> + help
>> + If you say yes here you get support for Texas Instruments TMP464
>> + temperature sensor chip.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called tmp464.
>> +
>> config SENSORS_TMP513
>> tristate "Texas Instruments TMP513 and compatibles"
>> depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 185f946d698b..a1f2d6686227 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
>> obj-$(CONFIG_SENSORS_TMP108) += tmp108.o
>> obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
>> obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
>> +obj-$(CONFIG_SENSORS_TMP464) += tmp464.o
>> obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
>> obj-$(CONFIG_SENSORS_VEXPRESS) += vexpress-hwmon.o
>> obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>> diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
>> new file mode 100644
>> index 000000000000..f294e12b1e39
>> --- /dev/null
>> +++ b/drivers/hwmon/tmp464.c
>> @@ -0,0 +1,472 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
>> + * Supported models: TMP464
>> +
>> + * Copyright (C) 2022 Agathe Porte <agathe.porte@nokia.com>
>> + * Preliminary support by:
>> + * Lionel Pouliquen <lionel.lp.pouliquen@nokia.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/i2c.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>
> Unnecessary include.
ACK.
>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of_device.h>
>> +#include <linux/sysfs.h>
>
> Is this include necessary ?
Removed.
>
> Include files in alphabetic order, please.
ACK.
>
>> +
>> +/* Addresses to scan */
>> +static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
>> + I2C_CLIENT_END };
>
> Unnecessary continuation line.
ACK.
>
>> +
>> +enum chips { tmp464 };
>> +
>> +#define MAX_CHANNELS 5 /* chan 0 is internal, 1-4 are
>> remote */
>> +
>> +/* TMP464 registers */
>> +#define TMP464_REG_THERM_STATUS 0x21
>> +#define TMP464_REG_THERM2_STATUS 0x22
>> +
>> +static const u8 TMP464_TEMP[MAX_CHANNELS] = { 0x00, 0x01, 0x02,
>> 0x03, 0x04 };
>> +static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = { 0x39, 0x42,
>> 0x4A, 0x52, 0x5A };
>> +static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = { 0x3A, 0x43,
>> 0x4B, 0x53, 0x5B };
>> +static const u8 TMP464_OFFSET_REMOTE[MAX_CHANNELS] = { 0x40, 0x48,
>> 0x50, 0x58 };
>
> Most of those are unused (which would result in static analyzer
> warnings).
> More on that below.
>
>> +#define TMP464_N_FACTOR_REG_1 0x41
>> +#define TMP464_CONFIG_REG 0x30
>> +
>> +/* alarm offset in THERM_STATUS/THERM2_STATUS register */
>> +#define TMP464_ALARM_BASE_NUMBER 7
>> +
>> +#define TMP464_REG_THERM_HYSTERESIS 0x38
>> +
>> +#define TMP464_MANUFACTURER_ID_REG 0xFE
>> +#define TMP464_DEVICE_ID_REG 0xFF
>> +
>> +/* Flags */
>> +#define TMP464_CONFIG_SHUTDOWN BIT(5)
>> +#define TMP464_CONFIG_RANGE 0x04
>> +#define TMP464_CONFIG_REG_REN(x) (BIT(7 + (x)))
>> +#define TMP464_CONFIG_REG_REN_MASK GENMASK(11, 7)
>> +#define TMP464_CONFIG_CONVERSION_RATE_B0 2
>> +#define TMP464_CONFIG_CONVERSION_RATE_MASK
>> GENMASK(TMP464_CONFIG_CONVERSION_RATE_B0, \
>> + TMP464_CONFIG_CONVERSION_RATE_B0 + 2)
>> +
>> +/* Manufacturer / Device ID's */
>> +#define TMP464_MANUFACTURER_ID 0x5449
>> +#define TMP464_DEVICE_ID 0x1468
>> +
>> +
> No double empty lines, please.
ACK.
>
>> +static const struct i2c_device_id tmp464_id[] = {
>> + { "tmp464", 0 },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, tmp464_id);
>> +
>> +static const struct of_device_id __maybe_unused tmp464_of_match[] = {
>> + {
>> + .compatible = "ti,tmp464",
>> + .data = (void *)5
>> + },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, tmp464_of_match);
>> +
>> +
> No double empty lines, please. checkpatch --strict should tell you
> that. Please
> run and fix what it reports.
ACK. I only ran checkpatch.pl without the --strict version before. I
will fix all --strict warnings in v2 patchset
>
>> +struct tmp464_channel {
>> + const char *label;
>> + bool enabled;
>> + s16 temp;
>> +};
>> +
>> +struct tmp464_data {
>> + struct i2c_client *client;
>> + struct mutex update_lock;
>> + u32 temp_config[MAX_CHANNELS + 1];
>> + struct hwmon_channel_info temp_info;
>> + const struct hwmon_channel_info *info[2];
>> + struct hwmon_chip_info chip;
>> + bool valid;
>> + unsigned long last_updated;
>> + unsigned long channels;
>> + u16 config;
>> + struct tmp464_channel channel[MAX_CHANNELS];
>> +};
>> +
>> +
>> +/* funcs */
>> +
> Useless comment. Please drop.
ACK.
>
>> +static int temp_from_raw(u16 reg)
>> +{
>> + int temp = 0;
>> +
>> + temp = (reg >> 8) * 2000;
>> + temp += ((reg & 0xFF) >> 3) * 625 / 10;
>> +
>
> This doesn't correctly handle negative temperatures. reg needs to be s16
> to be able to handle those. Also, why not just
>
> return (reg >> 3) * 625 / 10;
> or
> return DIV_ROUND_CLOSEST((reg >> 3) * 625, 10);
>
> ?
ACK. I did not think about simplifying the math. Next patchset will use
DIV_ROUND_CLOSEST and s16.
>
>> + return temp;
>> +}
>> +
>> +static int tmp464_update_device(struct tmp464_data *data)
>> +{
>> + struct i2c_client *client = data->client;
>> + int ret = 0;
>> + int i;
>> +
>> + mutex_lock(&data->update_lock);
>> +
>> + if (time_after(jiffies, data->last_updated + (HZ / 2)) ||
>> + !data->valid) {
>> + ret = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>> + if (ret < 0)
>> + goto exit;
>> + data->config = ret;
>> +
>> + for (i = 0; i < data->channels; i++) {
>> + ret = i2c_smbus_read_word_swapped(client, TMP464_TEMP[i]);
>> + if (ret < 0)
>> + goto exit;
>> + data->channel[i].temp = ret;
>> + }
>> + data->last_updated = jiffies;
>> + data->valid = true;
>> + }
>> +
>> +exit:
>> + mutex_unlock(&data->update_lock);
>> +
>> + if (ret < 0) {
>> + data->valid = false;
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> We are moving away from local caching. Please consider using regmap
> instead.
Seems like I was wrong to base my work on the tmp421.c driver. Do you
have a driver in mind in hwmon that is state-of-the-art and uses regmap
so that I can inspire myself? Will propose v2 patchset without regmap first.
>
>> +static int tmp464_enable_channels(struct tmp464_data *data)
>> +{
>> + int err;
>> + struct i2c_client *client = data->client;
>> + struct device *dev = &client->dev;
>> + int old = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>> + int new, i;
>> +
>> + if (old < 0) {
>> + dev_err(dev, "error reading register, can't disable
>> channels\n");
>> + return old;
>> + }
>> +
>> + new = old & ~TMP464_CONFIG_REG_REN_MASK;
>> + for (i = 0; i < data->channels; i++)
>> + if (data->channel[i].enabled)
>> + new |= TMP464_CONFIG_REG_REN(i);
>> +
>> + if (new == old)
>> + return 0;
>> +
>> + err = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, new);
>> + if (err < 0)
>> + dev_err(dev, "error writing register, can't disable
>> channels\n");
>> +
>> + return err;
>> +}
>> +
>> +static int tmp464_read(struct device *dev, enum hwmon_sensor_types
>> type,
>> + u32 attr, int channel, long *val)
>> +{
>> + struct tmp464_data *tmp464 = dev_get_drvdata(dev);
>> + int ret = 0;
>> +
>> + ret = tmp464_update_device(tmp464);
>> + if (ret)
>> + return ret;
>> +
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + if (!tmp464->channel[channel].enabled)
>> + return -ENODATA;
>> + *val = temp_from_raw(tmp464->channel[channel].temp);
>> + return 0;
>> + case hwmon_temp_enable:
>> + *val = tmp464->channel[channel].enabled;
>> + return 0;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> +}
>> +
>> +static int tmp464_read_string(struct device *dev, enum
>> hwmon_sensor_types type,
>> + u32 attr, int channel, const char **str)
>> +{
>> + struct tmp464_data *data = dev_get_drvdata(dev);
>> +
>> + *str = data->channel[channel].label;
>> +
>> + return 0;
>> +}
>> +
>> +static int tmp464_write(struct device *dev, enum hwmon_sensor_types
>> type,
>> + u32 attr, int channel, long val)
>> +{
>> + struct tmp464_data *data = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + switch (attr) {
>> + case hwmon_temp_enable:
>> + data->channel[channel].enabled = val;
>> + ret = tmp464_enable_channels(data);
>> + break;
>> + default:
>> + ret = -EOPNOTSUPP;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static umode_t tmp464_is_visible(const void *data, enum
>> hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + return 0444;
>> + case hwmon_temp_label:
>> + return 0444;
>> + case hwmon_temp_enable:
>> + return 0644;
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> +static int tmp464_init_client(struct tmp464_data *data)
>> +{
>> + int config, config_orig;
>> + struct i2c_client *client = data->client;
>> +
>> + /* Set the conversion rate to 2 Hz */
>> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>> + if (config < 0) {
>> + dev_err(&client->dev,
>> + "Could not read configuration register (%d)\n", config);
>> + return config;
>> + }
>> +
>> + config_orig = config;
>> + config &= ~TMP464_CONFIG_CONVERSION_RATE_MASK;
>> + config |= (0x05) << TMP464_CONFIG_CONVERSION_RATE_B0;
>> +
>> + if (config != config_orig) {
>> + dev_info(&client->dev, "Set conversion rate to 2 Hz\n");
>> + i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
>> + }
>> +
>> + /* Start conversions (disable shutdown if necessary) */
>> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>
> Why read this register twice ?
>
>> + if (config < 0) {
>> + dev_err(&client->dev,
>> + "Could not read configuration register (%d)\n", config);
>> + return config;
>> + }
>> +
>> + config_orig = config;
>> + config &= ~TMP464_CONFIG_SHUTDOWN;
>> +
>> + if (config != config_orig) {
>> + dev_info(&client->dev, "Enable monitoring chip\n");
>
> Please only write the configuration register once, and drop the messages.
ACK. This was inspired from the tmp421.c driver, but there should be no
reason to read twice.
>
>> + i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
>> + }
>> +
>> + return tmp464_enable_channels(data);
>> +}
>> +
>> +static int tmp464_detect(struct i2c_client *client,
>> + struct i2c_board_info *info)
>> +{
>> + enum chips kind;
>> + struct i2c_adapter *adapter = client->adapter;
>> + static const char * const names[] = {
>> + "TMP464"
>> + };
>> + u8 reg;
>
> Should be int to enable error checks.
ACK.
>
>> +
>> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
>> + return -ENODEV;
>> +
>> + reg = i2c_smbus_read_word_swapped(client,
>> TMP464_MANUFACTURER_ID_REG);
>> + if (reg != TMP464_MANUFACTURER_ID)
>> + return -ENODEV;
>> +
>> + /* Check for "always return zero" bits */
>> + reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM_STATUS);
>> + if (reg & 0x1f)
>
> should also check for < 0.
ACK.
>
>> + return -ENODEV;
>> + reg = i2c_smbus_read_word_swapped(client,
>> TMP464_REG_THERM2_STATUS);
>> + if (reg & 0x1f)
>> + return -ENODEV;
>
> Should also check for < 0.
ACK.
>
>> +
>> + reg = i2c_smbus_read_word_swapped(client, TMP464_DEVICE_ID_REG);
>> + if (reg < 0)
>> + return reg;
>
> That won't happen with an u8 variable.
ACK.
>
>> + switch (reg) {
>> + case TMP464_DEVICE_ID:
>> + kind = tmp464;
>> + break;
>
> Are there more devices with the same register set ? If not it is
> pointless to
> have/prepare support for it.
The tmp468 is the same device with 1+8 channels (tmp464 is 1+4
channels). Adding TMP468 support would be fairly easy with the current
structure, but we do not have the hardware to validate it unfortunately.
>
>> + default:
>> + return -ENODEV;
>> + }
>> +
>> + strscpy(info->type, tmp464_id[kind].name, I2C_NAME_SIZE);
>> + dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
>> + names[kind], client->addr);
>> +
>> + return 0;
>> +}
>> +
>> +static int tmp464_probe_child_from_dt(struct i2c_client *client,
>> + struct device_node *child,
>> + struct tmp464_data *data)
>> +
>> +{
>> + struct device *dev = &client->dev;
>> + u32 i;
>> + s32 val;
>> + int err;
>> +
>> + err = of_property_read_u32(child, "reg", &i);
>> + if (err) {
>> + dev_err(dev, "missing reg property of %pOFn\n", child);
>> + return err;
>> + }
>> +
>> + if (i >= data->channels) {
>> + dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
>> + return -EINVAL;
>> + }
>> +
>> + of_property_read_string(child, "label", &data->channel[i].label);
>> + if (data->channel[i].label)
>> + data->temp_config[i] |= HWMON_T_LABEL;
>> +
>> + data->channel[i].enabled = of_device_is_available(child);
>> +
>> + err = of_property_read_s32(child, "ti,n-factor", &val);
>> + if (!err) {
>> + if (i == 0) {
>> + dev_err(dev, "n-factor can't be set for internal
>> channel\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (val > 127 || val < -128) {
>> + dev_err(dev, "n-factor for channel %d invalid (%d)\n",
>> + i, val);
>> + return -EINVAL;
>> + }
>> + i2c_smbus_write_word_data(client, TMP464_N_FACTOR_REG_1 + i
>> - 1,
>> + val);
>
> needs error check
ACK.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tmp464_probe_from_dt(struct i2c_client *client, struct
>> tmp464_data *data)
>> +{
>> + struct device *dev = &client->dev;
>> + const struct device_node *np = dev->of_node;
>> + struct device_node *child;
>> + int err;
>> +
>> + for_each_child_of_node(np, child) {
>> + if (strcmp(child->name, "channel"))
>> + continue;
>> +
>> + err = tmp464_probe_child_from_dt(client, child, data);
>> + if (err) {
>> + of_node_put(child);
>> + return err;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct hwmon_ops tmp464_ops = {
>> + .is_visible = tmp464_is_visible,
>> + .read = tmp464_read,
>> + .read_string = tmp464_read_string,
>> + .write = tmp464_write,
>> +};
>> +
>> +static int tmp464_probe(struct i2c_client *client)
>> +{
>> + struct device *dev = &client->dev;
>> + struct device *hwmon_dev;
>> + struct tmp464_data *data;
>> + int i, err;
>> +
>> + if (!i2c_check_functionality(client->adapter,
>> + I2C_FUNC_SMBUS_WORD_DATA)) {
>> + dev_err(&client->dev, "i2c functionality check failed\n");
>> + return -ENODEV;
>> + }
>> + data = devm_kzalloc(dev, sizeof(struct tmp464_data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + mutex_init(&data->update_lock);
>> + if (client->dev.of_node)
>> + data->channels = (unsigned long)
>> + of_device_get_match_data(&client->dev);
>
> Unnecessary continuation line.
ACK.
>
>> + else
>> + data->channels = i2c_match_id(tmp464_id, client)->driver_data;
>> + data->client = client;
>> +
>> + for (i = 0; i < data->channels; i++) {
>> + data->temp_config[i] = HWMON_T_INPUT | HWMON_T_ENABLE;
>
> Why not support limits, temperature offsets, alarms, hysteresis, and
> conversion
> rate configuration ? It is kind of off to support n-factor correction but
> nothing else.
>
> Overall it looks like some of the support was dropped since defines for
> limit and offset registers are there, and the driver seems to be prepared
> to support more chips. Why ?
The history of this driver is that it was using older hwmon framework
practise. During the port to the new framework for upstreaming, we found
out that we did not have the use of {limits,offsets,alarms} sysfs
entries, so these features were not ported. They can be added later,
though. I will drop the #defines for the unused registers meanwhile.
>
>> + data->channel[i].enabled = true;
>> + }
>> +
>> + err = tmp464_probe_from_dt(client, data);
>> + if (err)
>> + return err;
>> +
>> + err = tmp464_init_client(data);
>> + if (err)
>> + return err;
>> +
>> + data->chip.ops = &tmp464_ops;
>> + data->chip.info = data->info;
>> +
>> + data->info[0] = &data->temp_info;
>> +
>> + data->temp_info.type = hwmon_temp;
>> + data->temp_info.config = data->temp_config;
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
>> + data,
>> + &data->chip,
>> + NULL);
>
> Please drop some of those continuation lines. We can go up to 100
> columns and
> should do so in situations like this.
ACK.
>
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static struct i2c_driver tmp464_driver = {
>> + .class = I2C_CLASS_HWMON,
>> + .driver = {
>> + .name = "tmp464",
>> + .of_match_table = of_match_ptr(tmp464_of_match),
>> + },
>> + .probe_new = tmp464_probe,
>> + .id_table = tmp464_id,
>> + .detect = tmp464_detect,
>> + .address_list = normal_i2c,
>> +};
>> +
>> +module_i2c_driver(tmp464_driver);
>> +
>> +MODULE_AUTHOR("Agathe Porte <agathe.porte@nokia.com>");
>> +MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor
>> driver");
>> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] dt-bindings: hwmon: add tmp464.yaml
2022-02-09 13:36 [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml Agathe Porte
2022-02-09 13:36 ` [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip Agathe Porte
2022-02-09 15:11 ` [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml Guenter Roeck
@ 2022-02-09 15:53 ` Agathe Porte
2022-02-09 15:53 ` [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip Agathe Porte
2 siblings, 1 reply; 14+ messages in thread
From: Agathe Porte @ 2022-02-09 15:53 UTC (permalink / raw)
To: linux-hwmon; +Cc: Guenter Roeck, Jean Delvare, Krzysztof Adamski, Agathe Porte
Add basic description of the tmp464 driver DT bindings.
Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
---
.../devicetree/bindings/hwmon/ti,tmp464.yaml | 106 ++++++++++++++++++
MAINTAINERS | 6 +
2 files changed, 112 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
new file mode 100644
index 000000000000..8eb43e82f5ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
@@ -0,0 +1,106 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ti,tmp464.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TMP464 temperature sensor
+
+maintainers:
+ - Agathe Porte <agathe.porte@nokia.com>
+
+description: |
+ ±0.0625°C Remote and Local temperature sensor
+ https://www.ti.com/lit/ds/symlink/tmp464.pdf
+
+properties:
+ compatible:
+ enum:
+ - ti,tmp464
+ reg:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+patternProperties:
+ "^channel@([0-3])$":
+ type: object
+ description: |
+ Represents channels of the device and their specific configuration.
+
+ properties:
+ reg:
+ description: |
+ The channel number. 0 is local channel, 1-3 are remote channels
+ items:
+ minimum: 0
+ maximum: 3
+
+ label:
+ description: |
+ A descriptive name for this channel, like "ambient" or "psu".
+
+ ti,n-factor:
+ description: |
+ The value (two's complement) to be programmed in the channel specific N correction register.
+ For remote channels only.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ items:
+ minimum: 0
+ maximum: 255
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor@4b {
+ compatible = "ti,tmp464";
+ reg = <0x4b>;
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor@4b {
+ compatible = "ti,tmp464";
+ reg = <0x4b>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 {
+ reg = <0x0>;
+ ti,n-factor = <0x1>;
+ label = "local";
+ };
+
+ channel@1 {
+ reg = <0x1>;
+ ti,n-factor = <0x0>;
+ label = "somelabel";
+ };
+
+ channel@2 {
+ reg = <0x2>;
+ status = "disabled";
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3e461db9cd91..136cd34be715 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19457,6 +19457,12 @@ S: Maintained
F: Documentation/hwmon/tmp401.rst
F: drivers/hwmon/tmp401.c
+TMP464 HARDWARE MONITOR DRIVER
+M: Agathe Porte <agathe.porte@nokia.com>
+L: linux-hwmon@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
+
TMP513 HARDWARE MONITOR DRIVER
M: Eric Tremblay <etremblay@distech-controls.com>
L: linux-hwmon@vger.kernel.org
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-09 15:53 ` [PATCH v2 " Agathe Porte
@ 2022-02-09 15:53 ` Agathe Porte
2022-02-11 5:16 ` Guenter Roeck
0 siblings, 1 reply; 14+ messages in thread
From: Agathe Porte @ 2022-02-09 15:53 UTC (permalink / raw)
To: linux-hwmon; +Cc: Guenter Roeck, Jean Delvare, Krzysztof Adamski, Agathe Porte
Add support for Texas Instruments TMP464 temperature sensor IC.
TI's TMP464 is an I2C temperature sensor chip. This chip is
similar to TI's TMP421 chip, but with 16bit-wide registers (instead
of 8bit-wide registers). The chip have one local sensor and four
remote sensors.
Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
Acked-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/tmp464.rst | 43 ++++
MAINTAINERS | 2 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/tmp464.c | 447 +++++++++++++++++++++++++++++++++
6 files changed, 504 insertions(+)
create mode 100644 Documentation/hwmon/tmp464.rst
create mode 100644 drivers/hwmon/tmp464.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index df20022c741f..37590db85e65 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -193,6 +193,7 @@ Hardware Monitoring Kernel Drivers
tmp108
tmp401
tmp421
+ tmp464
tmp513
tps23861
tps40422
diff --git a/Documentation/hwmon/tmp464.rst b/Documentation/hwmon/tmp464.rst
new file mode 100644
index 000000000000..8990554194de
--- /dev/null
+++ b/Documentation/hwmon/tmp464.rst
@@ -0,0 +1,43 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver tmp421
+====================
+
+Supported chips:
+
+ * Texas Instruments TMP464
+
+ Prefix: 'tmp464'
+
+ Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
+
+ Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp464.html
+
+Authors:
+
+ Agathe Porte <agathe.porte@nokia.com>
+
+Description
+-----------
+
+This driver implements support for Texas Instruments TMP464
+temperature sensor chip. This chip implement one local four remote
+sensors. Temperature is measured in degrees Celsius. The chips are
+wired over I2C/SMBus and specified over a temperature range of -40 to
++125 degrees Celsius. Resolution for both the local and remote
+channels is 0.0625 degree C.
+
+The chips support only temperature measurement. The driver exports
+the temperature values via the following sysfs files:
+
+**temp[1-5]_input**
+
+Each sensor can be individually disabled via Devicetree or from sysfs
+via:
+
+**temp[1-5]_enable**
+
+If labels were specified in Devicetree, additional sysfs files will
+be present:
+
+**temp[1-5]_label**
diff --git a/MAINTAINERS b/MAINTAINERS
index 136cd34be715..7fa2796adbef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19462,6 +19462,8 @@ M: Agathe Porte <agathe.porte@nokia.com>
L: linux-hwmon@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
+F: Documentation/hwmon/tmp464.rst
+F: drivers/hwmon/tmp464.c
TMP513 HARDWARE MONITOR DRIVER
M: Eric Tremblay <etremblay@distech-controls.com>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 8df25f1079ba..52b4f5688b45 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1979,6 +1979,16 @@ config SENSORS_TMP421
This driver can also be built as a module. If so, the module
will be called tmp421.
+config SENSORS_TMP464
+ tristate "Texas Instruments TMP464 and compatible"
+ depends on I2C
+ help
+ If you say yes here you get support for Texas Instruments TMP464
+ temperature sensor chip.
+
+ This driver can also be built as a module. If so, the module
+ will be called tmp464.
+
config SENSORS_TMP513
tristate "Texas Instruments TMP513 and compatibles"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 185f946d698b..a1f2d6686227 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
obj-$(CONFIG_SENSORS_TMP108) += tmp108.o
obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
+obj-$(CONFIG_SENSORS_TMP464) += tmp464.o
obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
obj-$(CONFIG_SENSORS_VEXPRESS) += vexpress-hwmon.o
obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
new file mode 100644
index 000000000000..564090929ad4
--- /dev/null
+++ b/drivers/hwmon/tmp464.c
@@ -0,0 +1,447 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
+ * Supported models: TMP464
+
+ * Copyright (C) 2022 Agathe Porte <agathe.porte@nokia.com>
+ * Preliminary support by:
+ * Lionel Pouliquen <lionel.lp.pouliquen@nokia.com>
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b, I2C_CLIENT_END };
+
+enum chips { tmp464 };
+
+#define MAX_CHANNELS 5 /* chan 0 is internal, 1-4 are remote */
+
+/* TMP464 registers */
+static const u8 TMP464_TEMP[MAX_CHANNELS] = { 0x00, 0x01, 0x02, 0x03, 0x04 };
+static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = { 0x39, 0x42, 0x4A, 0x52, 0x5A };
+static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = { 0x3A, 0x43, 0x4B, 0x53, 0x5B };
+static const u8 TMP464_OFFSET_REMOTE[MAX_CHANNELS] = { 0x40, 0x48, 0x50, 0x58 };
+#define TMP464_N_FACTOR_REG_1 0x41
+#define TMP464_CONFIG_REG 0x30
+
+/* Identification */
+#define TMP464_MANUFACTURER_ID_REG 0xFE
+#define TMP464_DEVICE_ID_REG 0xFF
+
+/* Flags */
+#define TMP464_CONFIG_SHUTDOWN BIT(5)
+#define TMP464_CONFIG_RANGE 0x04
+#define TMP464_CONFIG_REG_REN(x) (BIT(7 + (x)))
+#define TMP464_CONFIG_REG_REN_MASK GENMASK(11, 7)
+#define TMP464_CONFIG_CONVERSION_RATE_B0 2
+#define TMP464_CONFIG_CONVERSION_RATE_MASK GENMASK(TMP464_CONFIG_CONVERSION_RATE_B0, \
+ TMP464_CONFIG_CONVERSION_RATE_B0 + 2)
+
+/* Manufacturer / Device ID's */
+#define TMP464_MANUFACTURER_ID 0x5449
+#define TMP464_DEVICE_ID 0x1468
+
+static const struct i2c_device_id tmp464_id[] = {
+ { "tmp464", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, tmp464_id);
+
+static const struct of_device_id __maybe_unused tmp464_of_match[] = {
+ {
+ .compatible = "ti,tmp464",
+ .data = (void *)5
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, tmp464_of_match);
+
+struct tmp464_channel {
+ const char *label;
+ bool enabled;
+ s16 temp;
+};
+
+struct tmp464_data {
+ struct i2c_client *client;
+ struct mutex update_lock; /* used to update the struct values */
+ u32 temp_config[MAX_CHANNELS + 1];
+ struct hwmon_channel_info temp_info;
+ const struct hwmon_channel_info *info[2];
+ struct hwmon_chip_info chip;
+ bool valid;
+ unsigned long last_updated;
+ unsigned long channels;
+ u16 config;
+ struct tmp464_channel channel[MAX_CHANNELS];
+};
+
+static int temp_from_raw(s16 reg)
+{
+ return DIV_ROUND_CLOSEST((reg >> 3) * 625, 10);
+}
+
+static int tmp464_update_device(struct tmp464_data *data)
+{
+ struct i2c_client *client = data->client;
+ int ret = 0;
+ int i;
+
+ mutex_lock(&data->update_lock);
+
+ if (time_after(jiffies, data->last_updated + (HZ / 2)) ||
+ !data->valid) {
+ ret = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
+ if (ret < 0)
+ goto exit;
+ data->config = ret;
+
+ for (i = 0; i < data->channels; i++) {
+ ret = i2c_smbus_read_word_swapped(client, TMP464_TEMP[i]);
+ if (ret < 0)
+ goto exit;
+ data->channel[i].temp = ret;
+ }
+ data->last_updated = jiffies;
+ data->valid = true;
+ }
+
+exit:
+ mutex_unlock(&data->update_lock);
+
+ if (ret < 0) {
+ data->valid = false;
+ return ret;
+ }
+
+ return 0;
+}
+
+static int tmp464_enable_channels(struct tmp464_data *data)
+{
+ int err;
+ struct i2c_client *client = data->client;
+ struct device *dev = &client->dev;
+ int old = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
+ int new, i;
+
+ if (old < 0) {
+ dev_err(dev, "error reading register, can't disable channels\n");
+ return old;
+ }
+
+ new = old & ~TMP464_CONFIG_REG_REN_MASK;
+ for (i = 0; i < data->channels; i++)
+ if (data->channel[i].enabled)
+ new |= TMP464_CONFIG_REG_REN(i);
+
+ if (new == old)
+ return 0;
+
+ err = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, new);
+ if (err < 0)
+ dev_err(dev, "error writing register, can't disable channels\n");
+
+ return err;
+}
+
+static int tmp464_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct tmp464_data *tmp464 = dev_get_drvdata(dev);
+ int ret = 0;
+
+ ret = tmp464_update_device(tmp464);
+ if (ret)
+ return ret;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ if (!tmp464->channel[channel].enabled)
+ return -ENODATA;
+ *val = temp_from_raw(tmp464->channel[channel].temp);
+ return 0;
+ case hwmon_temp_enable:
+ *val = tmp464->channel[channel].enabled;
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int tmp464_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **str)
+{
+ struct tmp464_data *data = dev_get_drvdata(dev);
+
+ *str = data->channel[channel].label;
+
+ return 0;
+}
+
+static int tmp464_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ struct tmp464_data *data = dev_get_drvdata(dev);
+ int ret;
+
+ switch (attr) {
+ case hwmon_temp_enable:
+ data->channel[channel].enabled = val;
+ ret = tmp464_enable_channels(data);
+ break;
+ default:
+ ret = -EOPNOTSUPP;
+ }
+
+ return ret;
+}
+
+static umode_t tmp464_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (attr) {
+ case hwmon_temp_input:
+ return 0444;
+ case hwmon_temp_label:
+ return 0444;
+ case hwmon_temp_enable:
+ return 0644;
+ default:
+ return 0;
+ }
+}
+
+static int tmp464_init_client(struct tmp464_data *data)
+{
+ int err;
+ int config, config_orig;
+ struct i2c_client *client = data->client;
+
+ config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
+ if (config < 0) {
+ dev_err(&client->dev,
+ "Could not read configuration register (%d)\n", config);
+ return config;
+ }
+
+ /* Set the conversion rate to 2 Hz */
+ config_orig = config;
+ config &= ~TMP464_CONFIG_CONVERSION_RATE_MASK;
+ config |= (0x05) << TMP464_CONFIG_CONVERSION_RATE_B0;
+
+ /* Start conversions (disable shutdown if necessary) */
+ config_orig = config;
+ config &= ~TMP464_CONFIG_SHUTDOWN;
+
+ if (config != config_orig) {
+ config = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
+ if (config < 0) {
+ dev_err(&client->dev,
+ "Could not write configuration register (%d)\n", err);
+ return config;
+ }
+ }
+
+ return tmp464_enable_channels(data);
+}
+
+static int tmp464_detect(struct i2c_client *client,
+ struct i2c_board_info *info)
+{
+ enum chips kind;
+ struct i2c_adapter *adapter = client->adapter;
+ static const char * const names[] = {
+ "TMP464"
+ };
+ int reg;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
+ return -ENODEV;
+
+ reg = i2c_smbus_read_word_swapped(client, TMP464_MANUFACTURER_ID_REG);
+ if (reg < 0)
+ return reg;
+ if (reg != TMP464_MANUFACTURER_ID)
+ return -ENODEV;
+
+ /* Check for "always return zero" bits */
+ reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM_STATUS);
+ if (reg < 0)
+ return reg;
+ if (reg & 0x1f)
+ return -ENODEV;
+ reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM2_STATUS);
+ if (reg < 0)
+ return reg;
+ if (reg & 0x1f)
+ return -ENODEV;
+
+ reg = i2c_smbus_read_word_swapped(client, TMP464_DEVICE_ID_REG);
+ if (reg < 0)
+ return reg;
+ switch (reg) {
+ case TMP464_DEVICE_ID:
+ kind = tmp464;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ strscpy(info->type, tmp464_id[kind].name, I2C_NAME_SIZE);
+ dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
+ names[kind], client->addr);
+
+ return 0;
+}
+
+static int tmp464_probe_child_from_dt(struct i2c_client *client,
+ struct device_node *child,
+ struct tmp464_data *data)
+
+{
+ struct device *dev = &client->dev;
+ u32 i;
+ s32 val;
+ int err;
+
+ err = of_property_read_u32(child, "reg", &i);
+ if (err) {
+ dev_err(dev, "missing reg property of %pOFn\n", child);
+ return err;
+ }
+
+ if (i >= data->channels) {
+ dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
+ return -EINVAL;
+ }
+
+ of_property_read_string(child, "label", &data->channel[i].label);
+ if (data->channel[i].label)
+ data->temp_config[i] |= HWMON_T_LABEL;
+
+ data->channel[i].enabled = of_device_is_available(child);
+
+ err = of_property_read_s32(child, "ti,n-factor", &val);
+ if (!err) {
+ if (i == 0) {
+ dev_err(dev, "n-factor can't be set for internal channel\n");
+ return -EINVAL;
+ }
+
+ if (val > 127 || val < -128) {
+ dev_err(dev, "n-factor for channel %d invalid (%d)\n",
+ i, val);
+ return -EINVAL;
+ }
+ err = i2c_smbus_write_word_data(client, TMP464_N_FACTOR_REG_1 + i - 1, val);
+ if (err < 0)
+ return err;
+ }
+
+ return 0;
+}
+
+static int tmp464_probe_from_dt(struct i2c_client *client, struct tmp464_data *data)
+{
+ struct device *dev = &client->dev;
+ const struct device_node *np = dev->of_node;
+ struct device_node *child;
+ int err;
+
+ for_each_child_of_node(np, child) {
+ if (strcmp(child->name, "channel"))
+ continue;
+
+ err = tmp464_probe_child_from_dt(client, child, data);
+ if (err) {
+ of_node_put(child);
+ return err;
+ }
+ }
+
+ return 0;
+}
+
+static const struct hwmon_ops tmp464_ops = {
+ .is_visible = tmp464_is_visible,
+ .read = tmp464_read,
+ .read_string = tmp464_read_string,
+ .write = tmp464_write,
+};
+
+static int tmp464_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct device *hwmon_dev;
+ struct tmp464_data *data;
+ int i, err;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+ dev_err(&client->dev, "i2c functionality check failed\n");
+ return -ENODEV;
+ }
+ data = devm_kzalloc(dev, sizeof(struct tmp464_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ mutex_init(&data->update_lock);
+ if (client->dev.of_node)
+ data->channels = (unsigned long)of_device_get_match_data(&client->dev);
+ else
+ data->channels = i2c_match_id(tmp464_id, client)->driver_data;
+ data->client = client;
+
+ for (i = 0; i < data->channels; i++) {
+ data->temp_config[i] = HWMON_T_INPUT | HWMON_T_ENABLE;
+ data->channel[i].enabled = true;
+ }
+
+ err = tmp464_probe_from_dt(client, data);
+ if (err)
+ return err;
+
+ err = tmp464_init_client(data);
+ if (err)
+ return err;
+
+ data->chip.ops = &tmp464_ops;
+ data->chip.info = data->info;
+
+ data->info[0] = &data->temp_info;
+
+ data->temp_info.type = hwmon_temp;
+ data->temp_info.config = data->temp_config;
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+ data, &data->chip, NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct i2c_driver tmp464_driver = {
+ .class = I2C_CLASS_HWMON,
+ .driver = {
+ .name = "tmp464",
+ .of_match_table = of_match_ptr(tmp464_of_match),
+ },
+ .probe_new = tmp464_probe,
+ .id_table = tmp464_id,
+ .detect = tmp464_detect,
+ .address_list = normal_i2c,
+};
+
+module_i2c_driver(tmp464_driver);
+
+MODULE_AUTHOR("Agathe Porte <agathe.porte@nokia.com>");
+MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor driver");
+MODULE_LICENSE("GPL");
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-09 15:45 ` Agathe Porte
@ 2022-02-09 17:37 ` Guenter Roeck
0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2022-02-09 17:37 UTC (permalink / raw)
To: Agathe Porte, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
On 2/9/22 07:45, Agathe Porte wrote:
> Hi Guenter,
>
> Le 09/02/2022 à 15:46, Guenter Roeck a écrit :
>> On 2/9/22 05:36, Agathe Porte wrote:
>>> Add support for Texas Instruments TMP464 temperature sensor IC.
>>>
>>> TI's TMP464 is an I2C temperature sensor chip. This chip is
>>> similar to TI's TMP421 chip, but with 16bit-wide registers (instead
>>> of 8bit-wide registers). The chip have one local sensor and four
>>> remote sensors.
>>>
>>> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
>>> Acked-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>>> ---
>>> Documentation/hwmon/tmp464.rst | 43 +++
>>> MAINTAINERS | 7 +
>>> drivers/hwmon/Kconfig | 10 +
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/tmp464.c | 472 +++++++++++++++++++++++++++++++++
>>> 5 files changed, 533 insertions(+)
>>> create mode 100644 Documentation/hwmon/tmp464.rst
>>> create mode 100644 drivers/hwmon/tmp464.c
>>>
>>> diff --git a/Documentation/hwmon/tmp464.rst b/Documentation/hwmon/tmp464.rst
>>
>> Needs to be added to Documentation/hwmon/index.rst.
> ACK.
>>
>>> new file mode 100644
>>> index 000000000000..8990554194de
>>> --- /dev/null
>>> +++ b/Documentation/hwmon/tmp464.rst
>>> @@ -0,0 +1,43 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
>>> +Kernel driver tmp421
>>> +====================
>>> +
>>> +Supported chips:
>>> +
>>> + * Texas Instruments TMP464
>>> +
>>> + Prefix: 'tmp464'
>>> +
>>> + Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
>>> +
>>> + Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp464.html
>>> +
>>> +Authors:
>>> +
>>> + Agathe Porte <agathe.porte@nokia.com>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +This driver implements support for Texas Instruments TMP464
>>> +temperature sensor chip. This chip implement one local four remote
>>> +sensors. Temperature is measured in degrees Celsius. The chips are
>>> +wired over I2C/SMBus and specified over a temperature range of -40 to
>>> ++125 degrees Celsius. Resolution for both the local and remote
>>> +channels is 0.0625 degree C.
>>> +
>>> +The chips support only temperature measurement. The driver exports
>>> +the temperature values via the following sysfs files:
>>> +
>>> +**temp[1-5]_input**
>>> +
>>> +Each sensor can be individually disabled via Devicetree or from sysfs
>>> +via:
>>> +
>>> +**temp[1-5]_enable**
>>> +
>>> +If labels were specified in Devicetree, additional sysfs files will
>>> +be present:
>>> +
>>> +**temp[1-5]_label**
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 3e461db9cd91..fa0b27a8fe28 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -19457,6 +19457,13 @@ S: Maintained
>>> F: Documentation/hwmon/tmp401.rst
>>> F: drivers/hwmon/tmp401.c
>>> +TMP464 HARDWARE MONITOR DRIVER
>>> +M: Agathe Porte <agathe.porte@nokia.com>
>>> +L: linux-hwmon@vger.kernel.org
>>> +S: Maintained
>>> +F: Documentation/hwmon/tmp464.rst
>>> +F: drivers/hwmon/tmp464.c
>>> +
>>> TMP513 HARDWARE MONITOR DRIVER
>>> M: Eric Tremblay <etremblay@distech-controls.com>
>>> L: linux-hwmon@vger.kernel.org
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 8df25f1079ba..52b4f5688b45 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1979,6 +1979,16 @@ config SENSORS_TMP421
>>> This driver can also be built as a module. If so, the module
>>> will be called tmp421.
>>> +config SENSORS_TMP464
>>> + tristate "Texas Instruments TMP464 and compatible"
>>> + depends on I2C
>>> + help
>>> + If you say yes here you get support for Texas Instruments TMP464
>>> + temperature sensor chip.
>>> +
>>> + This driver can also be built as a module. If so, the module
>>> + will be called tmp464.
>>> +
>>> config SENSORS_TMP513
>>> tristate "Texas Instruments TMP513 and compatibles"
>>> depends on I2C
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 185f946d698b..a1f2d6686227 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
>>> obj-$(CONFIG_SENSORS_TMP108) += tmp108.o
>>> obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
>>> obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
>>> +obj-$(CONFIG_SENSORS_TMP464) += tmp464.o
>>> obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
>>> obj-$(CONFIG_SENSORS_VEXPRESS) += vexpress-hwmon.o
>>> obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>>> diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
>>> new file mode 100644
>>> index 000000000000..f294e12b1e39
>>> --- /dev/null
>>> +++ b/drivers/hwmon/tmp464.c
>>> @@ -0,0 +1,472 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
>>> + * Supported models: TMP464
>>> +
>>> + * Copyright (C) 2022 Agathe Porte <agathe.porte@nokia.com>
>>> + * Preliminary support by:
>>> + * Lionel Pouliquen <lionel.lp.pouliquen@nokia.com>
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/jiffies.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/hwmon-sysfs.h>
>>
>> Unnecessary include.
> ACK.
>>
>>> +#include <linux/err.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/sysfs.h>
>>
>> Is this include necessary ?
> Removed.
>>
>> Include files in alphabetic order, please.
> ACK.
>>
>>> +
>>> +/* Addresses to scan */
>>> +static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
>>> + I2C_CLIENT_END };
>>
>> Unnecessary continuation line.
> ACK.
>>
>>> +
>>> +enum chips { tmp464 };
>>> +
>>> +#define MAX_CHANNELS 5 /* chan 0 is internal, 1-4 are remote */
>>> +
>>> +/* TMP464 registers */
>>> +#define TMP464_REG_THERM_STATUS 0x21
>>> +#define TMP464_REG_THERM2_STATUS 0x22
>>> +
>>> +static const u8 TMP464_TEMP[MAX_CHANNELS] = { 0x00, 0x01, 0x02, 0x03, 0x04 };
>>> +static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = { 0x39, 0x42, 0x4A, 0x52, 0x5A };
>>> +static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = { 0x3A, 0x43, 0x4B, 0x53, 0x5B };
>>> +static const u8 TMP464_OFFSET_REMOTE[MAX_CHANNELS] = { 0x40, 0x48, 0x50, 0x58 };
>>
>> Most of those are unused (which would result in static analyzer warnings).
>> More on that below.
>>
>>> +#define TMP464_N_FACTOR_REG_1 0x41
>>> +#define TMP464_CONFIG_REG 0x30
>>> +
>>> +/* alarm offset in THERM_STATUS/THERM2_STATUS register */
>>> +#define TMP464_ALARM_BASE_NUMBER 7
>>> +
>>> +#define TMP464_REG_THERM_HYSTERESIS 0x38
>>> +
>>> +#define TMP464_MANUFACTURER_ID_REG 0xFE
>>> +#define TMP464_DEVICE_ID_REG 0xFF
>>> +
>>> +/* Flags */
>>> +#define TMP464_CONFIG_SHUTDOWN BIT(5)
>>> +#define TMP464_CONFIG_RANGE 0x04
>>> +#define TMP464_CONFIG_REG_REN(x) (BIT(7 + (x)))
>>> +#define TMP464_CONFIG_REG_REN_MASK GENMASK(11, 7)
>>> +#define TMP464_CONFIG_CONVERSION_RATE_B0 2
>>> +#define TMP464_CONFIG_CONVERSION_RATE_MASK GENMASK(TMP464_CONFIG_CONVERSION_RATE_B0, \
>>> + TMP464_CONFIG_CONVERSION_RATE_B0 + 2)
>>> +
>>> +/* Manufacturer / Device ID's */
>>> +#define TMP464_MANUFACTURER_ID 0x5449
>>> +#define TMP464_DEVICE_ID 0x1468
>>> +
>>> +
>> No double empty lines, please.
> ACK.
>>
>>> +static const struct i2c_device_id tmp464_id[] = {
>>> + { "tmp464", 0 },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, tmp464_id);
>>> +
>>> +static const struct of_device_id __maybe_unused tmp464_of_match[] = {
>>> + {
>>> + .compatible = "ti,tmp464",
>>> + .data = (void *)5
>>> + },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, tmp464_of_match);
>>> +
>>> +
>> No double empty lines, please. checkpatch --strict should tell you that. Please
>> run and fix what it reports.
> ACK. I only ran checkpatch.pl without the --strict version before. I will fix all --strict warnings in v2 patchset
>>
>>> +struct tmp464_channel {
>>> + const char *label;
>>> + bool enabled;
>>> + s16 temp;
>>> +};
>>> +
>>> +struct tmp464_data {
>>> + struct i2c_client *client;
>>> + struct mutex update_lock;
>>> + u32 temp_config[MAX_CHANNELS + 1];
>>> + struct hwmon_channel_info temp_info;
>>> + const struct hwmon_channel_info *info[2];
>>> + struct hwmon_chip_info chip;
>>> + bool valid;
>>> + unsigned long last_updated;
>>> + unsigned long channels;
>>> + u16 config;
>>> + struct tmp464_channel channel[MAX_CHANNELS];
>>> +};
>>> +
>>> +
>>> +/* funcs */
>>> +
>> Useless comment. Please drop.
> ACK.
>>
>>> +static int temp_from_raw(u16 reg)
>>> +{
>>> + int temp = 0;
>>> +
>>> + temp = (reg >> 8) * 2000;
>>> + temp += ((reg & 0xFF) >> 3) * 625 / 10;
>>> +
>>
>> This doesn't correctly handle negative temperatures. reg needs to be s16
>> to be able to handle those. Also, why not just
>>
>> return (reg >> 3) * 625 / 10;
>> or
>> return DIV_ROUND_CLOSEST((reg >> 3) * 625, 10);
>>
>> ?
> ACK. I did not think about simplifying the math. Next patchset will use DIV_ROUND_CLOSEST and s16.
>>
>>> + return temp;
>>> +}
>>> +
>>> +static int tmp464_update_device(struct tmp464_data *data)
>>> +{
>>> + struct i2c_client *client = data->client;
>>> + int ret = 0;
>>> + int i;
>>> +
>>> + mutex_lock(&data->update_lock);
>>> +
>>> + if (time_after(jiffies, data->last_updated + (HZ / 2)) ||
>>> + !data->valid) {
>>> + ret = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>>> + if (ret < 0)
>>> + goto exit;
>>> + data->config = ret;
>>> +
>>> + for (i = 0; i < data->channels; i++) {
>>> + ret = i2c_smbus_read_word_swapped(client, TMP464_TEMP[i]);
>>> + if (ret < 0)
>>> + goto exit;
>>> + data->channel[i].temp = ret;
>>> + }
>>> + data->last_updated = jiffies;
>>> + data->valid = true;
>>> + }
>>> +
>>> +exit:
>>> + mutex_unlock(&data->update_lock);
>>> +
>>> + if (ret < 0) {
>>> + data->valid = false;
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>> We are moving away from local caching. Please consider using regmap instead.
> Seems like I was wrong to base my work on the tmp421.c driver. Do you have a driver in mind in hwmon that is state-of-the-art and uses regmap so that I can inspire myself? Will propose v2 patchset without regmap first.
Any driver using regmap would be fine. A couple of driver conversions
to use regmap are queued in linux-next, and there are a few earlier
conversions as well.
495da5954e15 hwmon: (adt7x10) Convert to use regmap
a166d8e6f5b7 hwmon: (lm83) Use regmap
50152fb6c1a1 hwmon: (tmp401) Use regmap
ef67959c4253 hwmon: (adt7470) Convert to use regmap
df885d912f67 hwmon: (adm9240) Convert to regmap
>>
>>> +static int tmp464_enable_channels(struct tmp464_data *data)
>>> +{
>>> + int err;
>>> + struct i2c_client *client = data->client;
>>> + struct device *dev = &client->dev;
>>> + int old = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>>> + int new, i;
>>> +
>>> + if (old < 0) {
>>> + dev_err(dev, "error reading register, can't disable channels\n");
>>> + return old;
>>> + }
>>> +
>>> + new = old & ~TMP464_CONFIG_REG_REN_MASK;
>>> + for (i = 0; i < data->channels; i++)
>>> + if (data->channel[i].enabled)
>>> + new |= TMP464_CONFIG_REG_REN(i);
>>> +
>>> + if (new == old)
>>> + return 0;
>>> +
>>> + err = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, new);
>>> + if (err < 0)
>>> + dev_err(dev, "error writing register, can't disable channels\n");
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static int tmp464_read(struct device *dev, enum hwmon_sensor_types type,
>>> + u32 attr, int channel, long *val)
>>> +{
>>> + struct tmp464_data *tmp464 = dev_get_drvdata(dev);
>>> + int ret = 0;
>>> +
>>> + ret = tmp464_update_device(tmp464);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + switch (attr) {
>>> + case hwmon_temp_input:
>>> + if (!tmp464->channel[channel].enabled)
>>> + return -ENODATA;
>>> + *val = temp_from_raw(tmp464->channel[channel].temp);
>>> + return 0;
>>> + case hwmon_temp_enable:
>>> + *val = tmp464->channel[channel].enabled;
>>> + return 0;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> +}
>>> +
>>> +static int tmp464_read_string(struct device *dev, enum hwmon_sensor_types type,
>>> + u32 attr, int channel, const char **str)
>>> +{
>>> + struct tmp464_data *data = dev_get_drvdata(dev);
>>> +
>>> + *str = data->channel[channel].label;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tmp464_write(struct device *dev, enum hwmon_sensor_types type,
>>> + u32 attr, int channel, long val)
>>> +{
>>> + struct tmp464_data *data = dev_get_drvdata(dev);
>>> + int ret;
>>> +
>>> + switch (attr) {
>>> + case hwmon_temp_enable:
>>> + data->channel[channel].enabled = val;
>>> + ret = tmp464_enable_channels(data);
>>> + break;
>>> + default:
>>> + ret = -EOPNOTSUPP;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static umode_t tmp464_is_visible(const void *data, enum hwmon_sensor_types type,
>>> + u32 attr, int channel)
>>> +{
>>> + switch (attr) {
>>> + case hwmon_temp_input:
>>> + return 0444;
>>> + case hwmon_temp_label:
>>> + return 0444;
>>> + case hwmon_temp_enable:
>>> + return 0644;
>>> + default:
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static int tmp464_init_client(struct tmp464_data *data)
>>> +{
>>> + int config, config_orig;
>>> + struct i2c_client *client = data->client;
>>> +
>>> + /* Set the conversion rate to 2 Hz */
>>> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>>> + if (config < 0) {
>>> + dev_err(&client->dev,
>>> + "Could not read configuration register (%d)\n", config);
>>> + return config;
>>> + }
>>> +
>>> + config_orig = config;
>>> + config &= ~TMP464_CONFIG_CONVERSION_RATE_MASK;
>>> + config |= (0x05) << TMP464_CONFIG_CONVERSION_RATE_B0;
>>> +
>>> + if (config != config_orig) {
>>> + dev_info(&client->dev, "Set conversion rate to 2 Hz\n");
>>> + i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
>>> + }
>>> +
>>> + /* Start conversions (disable shutdown if necessary) */
>>> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>>
>> Why read this register twice ?
>>
>>> + if (config < 0) {
>>> + dev_err(&client->dev,
>>> + "Could not read configuration register (%d)\n", config);
>>> + return config;
>>> + }
>>> +
>>> + config_orig = config;
>>> + config &= ~TMP464_CONFIG_SHUTDOWN;
>>> +
>>> + if (config != config_orig) {
>>> + dev_info(&client->dev, "Enable monitoring chip\n");
>>
>> Please only write the configuration register once, and drop the messages.
> ACK. This was inspired from the tmp421.c driver, but there should be no reason to read twice.
>>
>>> + i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
>>> + }
>>> +
>>> + return tmp464_enable_channels(data);
>>> +}
>>> +
>>> +static int tmp464_detect(struct i2c_client *client,
>>> + struct i2c_board_info *info)
>>> +{
>>> + enum chips kind;
>>> + struct i2c_adapter *adapter = client->adapter;
>>> + static const char * const names[] = {
>>> + "TMP464"
>>> + };
>>> + u8 reg;
>>
>> Should be int to enable error checks.
> ACK.
>>
>>> +
>>> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
>>> + return -ENODEV;
>>> +
>>> + reg = i2c_smbus_read_word_swapped(client, TMP464_MANUFACTURER_ID_REG);
>>> + if (reg != TMP464_MANUFACTURER_ID)
>>> + return -ENODEV;
>>> +
>>> + /* Check for "always return zero" bits */
>>> + reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM_STATUS);
>>> + if (reg & 0x1f)
>>
>> should also check for < 0.
> ACK.
>>
>>> + return -ENODEV;
>>> + reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM2_STATUS);
>>> + if (reg & 0x1f)
>>> + return -ENODEV;
>>
>> Should also check for < 0.
> ACK.
>>
>>> +
>>> + reg = i2c_smbus_read_word_swapped(client, TMP464_DEVICE_ID_REG);
>>> + if (reg < 0)
>>> + return reg;
>>
>> That won't happen with an u8 variable.
> ACK.
>>
>>> + switch (reg) {
>>> + case TMP464_DEVICE_ID:
>>> + kind = tmp464;
>>> + break;
>>
>> Are there more devices with the same register set ? If not it is pointless to
>> have/prepare support for it.
> The tmp468 is the same device with 1+8 channels (tmp464 is 1+4 channels). Adding TMP468 support would be fairly easy with the current structure, but we do not have the hardware to validate it unfortunately.
Evaluation board for TI468 is $75 over at TI. Unfortunately it looks
like TMP464 is sold out everywhere, or I'd order a few.
>>
>>> + default:
>>> + return -ENODEV;
>>> + }
>>> +
>>> + strscpy(info->type, tmp464_id[kind].name, I2C_NAME_SIZE);
>>> + dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
>>> + names[kind], client->addr);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tmp464_probe_child_from_dt(struct i2c_client *client,
>>> + struct device_node *child,
>>> + struct tmp464_data *data)
>>> +
>>> +{
>>> + struct device *dev = &client->dev;
>>> + u32 i;
>>> + s32 val;
>>> + int err;
>>> +
>>> + err = of_property_read_u32(child, "reg", &i);
>>> + if (err) {
>>> + dev_err(dev, "missing reg property of %pOFn\n", child);
>>> + return err;
>>> + }
>>> +
>>> + if (i >= data->channels) {
>>> + dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + of_property_read_string(child, "label", &data->channel[i].label);
>>> + if (data->channel[i].label)
>>> + data->temp_config[i] |= HWMON_T_LABEL;
>>> +
>>> + data->channel[i].enabled = of_device_is_available(child);
>>> +
>>> + err = of_property_read_s32(child, "ti,n-factor", &val);
>>> + if (!err) {
>>> + if (i == 0) {
>>> + dev_err(dev, "n-factor can't be set for internal channel\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (val > 127 || val < -128) {
>>> + dev_err(dev, "n-factor for channel %d invalid (%d)\n",
>>> + i, val);
>>> + return -EINVAL;
>>> + }
>>> + i2c_smbus_write_word_data(client, TMP464_N_FACTOR_REG_1 + i - 1,
>>> + val);
>>
>> needs error check
> ACK.
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tmp464_probe_from_dt(struct i2c_client *client, struct tmp464_data *data)
>>> +{
>>> + struct device *dev = &client->dev;
>>> + const struct device_node *np = dev->of_node;
>>> + struct device_node *child;
>>> + int err;
>>> +
>>> + for_each_child_of_node(np, child) {
>>> + if (strcmp(child->name, "channel"))
>>> + continue;
>>> +
>>> + err = tmp464_probe_child_from_dt(client, child, data);
>>> + if (err) {
>>> + of_node_put(child);
>>> + return err;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct hwmon_ops tmp464_ops = {
>>> + .is_visible = tmp464_is_visible,
>>> + .read = tmp464_read,
>>> + .read_string = tmp464_read_string,
>>> + .write = tmp464_write,
>>> +};
>>> +
>>> +static int tmp464_probe(struct i2c_client *client)
>>> +{
>>> + struct device *dev = &client->dev;
>>> + struct device *hwmon_dev;
>>> + struct tmp464_data *data;
>>> + int i, err;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_WORD_DATA)) {
>>> + dev_err(&client->dev, "i2c functionality check failed\n");
>>> + return -ENODEV;
>>> + }
>>> + data = devm_kzalloc(dev, sizeof(struct tmp464_data), GFP_KERNEL);
>>> + if (!data)
>>> + return -ENOMEM;
>>> +
>>> + mutex_init(&data->update_lock);
>>> + if (client->dev.of_node)
>>> + data->channels = (unsigned long)
>>> + of_device_get_match_data(&client->dev);
>>
>> Unnecessary continuation line.
> ACK.
>>
>>> + else
>>> + data->channels = i2c_match_id(tmp464_id, client)->driver_data;
>>> + data->client = client;
>>> +
>>> + for (i = 0; i < data->channels; i++) {
>>> + data->temp_config[i] = HWMON_T_INPUT | HWMON_T_ENABLE;
>>
>> Why not support limits, temperature offsets, alarms, hysteresis, and conversion
>> rate configuration ? It is kind of off to support n-factor correction but
>> nothing else.
>>
>> Overall it looks like some of the support was dropped since defines for
>> limit and offset registers are there, and the driver seems to be prepared
>> to support more chips. Why ?
>
> The history of this driver is that it was using older hwmon framework practise. During the port to the new framework for upstreaming, we found out that we did not have the use of {limits,offsets,alarms} sysfs entries, so these features were not ported. They can be added later, though. I will drop the #defines for the unused registers meanwhile.
>
Hmm, that sounds less than perfect to me, to say it mildly.
Even if you don't use those attributes, they should still be
supported. It is not as if providing that support is
super-complicated to add (or keep), and testing it is quite
straightforward.
Can you possibly send me a couple of samples of tmp464 ? I'd rather
deal with that myself now instead of having to review the code later
again. I already ordered an evaluation board for TMP468, so that
part is covered, but I'd want to test the code with a real TMP464 as well.
Thanks,
Guenter
>>
>>> + data->channel[i].enabled = true;
>>> + }
>>> +
>>> + err = tmp464_probe_from_dt(client, data);
>>> + if (err)
>>> + return err;
>>> +
>>> + err = tmp464_init_client(data);
>>> + if (err)
>>> + return err;
>>> +
>>> + data->chip.ops = &tmp464_ops;
>>> + data->chip.info = data->info;
>>> +
>>> + data->info[0] = &data->temp_info;
>>> +
>>> + data->temp_info.type = hwmon_temp;
>>> + data->temp_info.config = data->temp_config;
>>> +
>>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
>>> + data,
>>> + &data->chip,
>>> + NULL);
>>
>> Please drop some of those continuation lines. We can go up to 100 columns and
>> should do so in situations like this.
> ACK.
>>
>>> +
>>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>>> +}
>>> +
>>> +static struct i2c_driver tmp464_driver = {
>>> + .class = I2C_CLASS_HWMON,
>>> + .driver = {
>>> + .name = "tmp464",
>>> + .of_match_table = of_match_ptr(tmp464_of_match),
>>> + },
>>> + .probe_new = tmp464_probe,
>>> + .id_table = tmp464_id,
>>> + .detect = tmp464_detect,
>>> + .address_list = normal_i2c,
>>> +};
>>> +
>>> +module_i2c_driver(tmp464_driver);
>>> +
>>> +MODULE_AUTHOR("Agathe Porte <agathe.porte@nokia.com>");
>>> +MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor driver");
>>> +MODULE_LICENSE("GPL");
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-09 15:53 ` [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip Agathe Porte
@ 2022-02-11 5:16 ` Guenter Roeck
2022-02-11 10:11 ` Agathe Porte
0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2022-02-11 5:16 UTC (permalink / raw)
To: Agathe Porte, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
On 2/9/22 07:53, Agathe Porte wrote:
> Add support for Texas Instruments TMP464 temperature sensor IC.
>
> TI's TMP464 is an I2C temperature sensor chip. This chip is
> similar to TI's TMP421 chip, but with 16bit-wide registers (instead
> of 8bit-wide registers). The chip have one local sensor and four
> remote sensors.
>
> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
> Acked-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
First, please _never_ send a new version of a patch or patch series
as response to an older version. I almost missed this version of the series.
Second, I really dislike continuing this driver without all the attibutes
making it valuable as hwmon driver, and I really want to drop local caching
and use regmap instead. I can not get a TMP464 (they appear to be sold out
until 2023 everywhere I checked, and I can not even get a sample from TI
either). However, I ordered and - according to Fedex - should get an
evaluation board for TMP468 tomorrow. Before we keep going back and
forth, I'd prefer to use that chip to add support for all attributes (or
in other words take the driver from here). Would that be ok with you ?
Additional comment inline.
Thanks,
Guenter
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/tmp464.rst | 43 ++++
> MAINTAINERS | 2 +
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tmp464.c | 447 +++++++++++++++++++++++++++++++++
> 6 files changed, 504 insertions(+)
> create mode 100644 Documentation/hwmon/tmp464.rst
> create mode 100644 drivers/hwmon/tmp464.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index df20022c741f..37590db85e65 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -193,6 +193,7 @@ Hardware Monitoring Kernel Drivers
> tmp108
> tmp401
> tmp421
> + tmp464
> tmp513
> tps23861
> tps40422
> diff --git a/Documentation/hwmon/tmp464.rst b/Documentation/hwmon/tmp464.rst
> new file mode 100644
> index 000000000000..8990554194de
> --- /dev/null
> +++ b/Documentation/hwmon/tmp464.rst
> @@ -0,0 +1,43 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver tmp421
> +====================
> +
> +Supported chips:
> +
> + * Texas Instruments TMP464
> +
> + Prefix: 'tmp464'
> +
> + Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
> +
> + Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp464.html
> +
> +Authors:
> +
> + Agathe Porte <agathe.porte@nokia.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for Texas Instruments TMP464
> +temperature sensor chip. This chip implement one local four remote
> +sensors. Temperature is measured in degrees Celsius. The chips are
> +wired over I2C/SMBus and specified over a temperature range of -40 to
> ++125 degrees Celsius. Resolution for both the local and remote
> +channels is 0.0625 degree C.
> +
> +The chips support only temperature measurement. The driver exports
> +the temperature values via the following sysfs files:
> +
> +**temp[1-5]_input**
> +
> +Each sensor can be individually disabled via Devicetree or from sysfs
> +via:
> +
> +**temp[1-5]_enable**
> +
> +If labels were specified in Devicetree, additional sysfs files will
> +be present:
> +
> +**temp[1-5]_label**
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 136cd34be715..7fa2796adbef 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19462,6 +19462,8 @@ M: Agathe Porte <agathe.porte@nokia.com>
> L: linux-hwmon@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
> +F: Documentation/hwmon/tmp464.rst
> +F: drivers/hwmon/tmp464.c
>
> TMP513 HARDWARE MONITOR DRIVER
> M: Eric Tremblay <etremblay@distech-controls.com>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 8df25f1079ba..52b4f5688b45 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1979,6 +1979,16 @@ config SENSORS_TMP421
> This driver can also be built as a module. If so, the module
> will be called tmp421.
>
> +config SENSORS_TMP464
> + tristate "Texas Instruments TMP464 and compatible"
> + depends on I2C
> + help
> + If you say yes here you get support for Texas Instruments TMP464
> + temperature sensor chip.
> +
> + This driver can also be built as a module. If so, the module
> + will be called tmp464.
> +
> config SENSORS_TMP513
> tristate "Texas Instruments TMP513 and compatibles"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 185f946d698b..a1f2d6686227 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
> obj-$(CONFIG_SENSORS_TMP108) += tmp108.o
> obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
> obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
> +obj-$(CONFIG_SENSORS_TMP464) += tmp464.o
> obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
> obj-$(CONFIG_SENSORS_VEXPRESS) += vexpress-hwmon.o
> obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
> diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
> new file mode 100644
> index 000000000000..564090929ad4
> --- /dev/null
> +++ b/drivers/hwmon/tmp464.c
> @@ -0,0 +1,447 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
> + * Supported models: TMP464
> +
> + * Copyright (C) 2022 Agathe Porte <agathe.porte@nokia.com>
> + * Preliminary support by:
> + * Lionel Pouliquen <lionel.lp.pouliquen@nokia.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +
> +/* Addresses to scan */
> +static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b, I2C_CLIENT_END };
> +
> +enum chips { tmp464 };
> +
> +#define MAX_CHANNELS 5 /* chan 0 is internal, 1-4 are remote */
> +
> +/* TMP464 registers */
> +static const u8 TMP464_TEMP[MAX_CHANNELS] = { 0x00, 0x01, 0x02, 0x03, 0x04 };
> +static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = { 0x39, 0x42, 0x4A, 0x52, 0x5A };
> +static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = { 0x3A, 0x43, 0x4B, 0x53, 0x5B };
> +static const u8 TMP464_OFFSET_REMOTE[MAX_CHANNELS] = { 0x40, 0x48, 0x50, 0x58 };
> +#define TMP464_N_FACTOR_REG_1 0x41
> +#define TMP464_CONFIG_REG 0x30
> +
> +/* Identification */
> +#define TMP464_MANUFACTURER_ID_REG 0xFE
> +#define TMP464_DEVICE_ID_REG 0xFF
> +
> +/* Flags */
> +#define TMP464_CONFIG_SHUTDOWN BIT(5)
> +#define TMP464_CONFIG_RANGE 0x04
> +#define TMP464_CONFIG_REG_REN(x) (BIT(7 + (x)))
> +#define TMP464_CONFIG_REG_REN_MASK GENMASK(11, 7)
> +#define TMP464_CONFIG_CONVERSION_RATE_B0 2
> +#define TMP464_CONFIG_CONVERSION_RATE_MASK GENMASK(TMP464_CONFIG_CONVERSION_RATE_B0, \
> + TMP464_CONFIG_CONVERSION_RATE_B0 + 2)
> +
> +/* Manufacturer / Device ID's */
> +#define TMP464_MANUFACTURER_ID 0x5449
> +#define TMP464_DEVICE_ID 0x1468
> +
> +static const struct i2c_device_id tmp464_id[] = {
> + { "tmp464", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, tmp464_id);
> +
> +static const struct of_device_id __maybe_unused tmp464_of_match[] = {
> + {
> + .compatible = "ti,tmp464",
> + .data = (void *)5
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, tmp464_of_match);
> +
> +struct tmp464_channel {
> + const char *label;
> + bool enabled;
> + s16 temp;
> +};
> +
> +struct tmp464_data {
> + struct i2c_client *client;
> + struct mutex update_lock; /* used to update the struct values */
> + u32 temp_config[MAX_CHANNELS + 1];
> + struct hwmon_channel_info temp_info;
> + const struct hwmon_channel_info *info[2];
> + struct hwmon_chip_info chip;
There is really no good reason to have the above structures dynamically
allocated. They can as well be declared statically. The only dynamic
attribute is the label, and the visibility of that attribute can be
controlled with the is_visible callback. After all, that is what
the callback is for.
> + bool valid;
> + unsigned long last_updated;
> + unsigned long channels;
Not sure why this should be unsigned long.
> + u16 config;
> + struct tmp464_channel channel[MAX_CHANNELS];
> +};
> +
> +static int temp_from_raw(s16 reg)
> +{
> + return DIV_ROUND_CLOSEST((reg >> 3) * 625, 10);
> +}
> +
> +static int tmp464_update_device(struct tmp464_data *data)
> +{
> + struct i2c_client *client = data->client;
> + int ret = 0;
> + int i;
> +
> + mutex_lock(&data->update_lock);
> +
> + if (time_after(jiffies, data->last_updated + (HZ / 2)) ||
> + !data->valid) {
> + ret = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
> + if (ret < 0)
> + goto exit;
> + data->config = ret;
> +
> + for (i = 0; i < data->channels; i++) {
> + ret = i2c_smbus_read_word_swapped(client, TMP464_TEMP[i]);
> + if (ret < 0)
> + goto exit;
> + data->channel[i].temp = ret;
> + }
> + data->last_updated = jiffies;
> + data->valid = true;
> + }
> +
> +exit:
> + mutex_unlock(&data->update_lock);
> +
> + if (ret < 0) {
> + data->valid = false;
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tmp464_enable_channels(struct tmp464_data *data)
> +{
> + int err;
> + struct i2c_client *client = data->client;
> + struct device *dev = &client->dev;
> + int old = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
> + int new, i;
> +
> + if (old < 0) {
> + dev_err(dev, "error reading register, can't disable channels\n");
> + return old;
> + }
> +
> + new = old & ~TMP464_CONFIG_REG_REN_MASK;
> + for (i = 0; i < data->channels; i++)
> + if (data->channel[i].enabled)
> + new |= TMP464_CONFIG_REG_REN(i);
> +
> + if (new == old)
> + return 0;
> +
> + err = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, new);
> + if (err < 0)
> + dev_err(dev, "error writing register, can't disable channels\n");
> +
> + return err;
> +}
> +
> +static int tmp464_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct tmp464_data *tmp464 = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + ret = tmp464_update_device(tmp464);
> + if (ret)
> + return ret;
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + if (!tmp464->channel[channel].enabled)
> + return -ENODATA;
> + *val = temp_from_raw(tmp464->channel[channel].temp);
> + return 0;
> + case hwmon_temp_enable:
> + *val = tmp464->channel[channel].enabled;
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int tmp464_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **str)
> +{
> + struct tmp464_data *data = dev_get_drvdata(dev);
> +
> + *str = data->channel[channel].label;
> +
> + return 0;
> +}
> +
> +static int tmp464_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + struct tmp464_data *data = dev_get_drvdata(dev);
> + int ret;
> +
> + switch (attr) {
> + case hwmon_temp_enable:
> + data->channel[channel].enabled = val;
> + ret = tmp464_enable_channels(data);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static umode_t tmp464_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + switch (attr) {
> + case hwmon_temp_input:
> + return 0444;
> + case hwmon_temp_label:
> + return 0444;
> + case hwmon_temp_enable:
> + return 0644;
> + default:
> + return 0;
> + }
> +}
> +
> +static int tmp464_init_client(struct tmp464_data *data)
> +{
> + int err;
> + int config, config_orig;
> + struct i2c_client *client = data->client;
> +
> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
> + if (config < 0) {
> + dev_err(&client->dev,
> + "Could not read configuration register (%d)\n", config);
> + return config;
> + }
> +
> + /* Set the conversion rate to 2 Hz */
> + config_orig = config;
> + config &= ~TMP464_CONFIG_CONVERSION_RATE_MASK;
> + config |= (0x05) << TMP464_CONFIG_CONVERSION_RATE_B0;
> +
> + /* Start conversions (disable shutdown if necessary) */
> + config_orig = config;
> + config &= ~TMP464_CONFIG_SHUTDOWN;
> +
> + if (config != config_orig) {
> + config = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
> + if (config < 0) {
> + dev_err(&client->dev,
> + "Could not write configuration register (%d)\n", err);
> + return config;
> + }
> + }
> +
> + return tmp464_enable_channels(data);
> +}
> +
> +static int tmp464_detect(struct i2c_client *client,
> + struct i2c_board_info *info)
> +{
> + enum chips kind;
> + struct i2c_adapter *adapter = client->adapter;
> + static const char * const names[] = {
> + "TMP464"
> + };
> + int reg;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
> + return -ENODEV;
> +
> + reg = i2c_smbus_read_word_swapped(client, TMP464_MANUFACTURER_ID_REG);
> + if (reg < 0)
> + return reg;
> + if (reg != TMP464_MANUFACTURER_ID)
> + return -ENODEV;
> +
> + /* Check for "always return zero" bits */
> + reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM_STATUS);
> + if (reg < 0)
> + return reg;
> + if (reg & 0x1f)
> + return -ENODEV;
> + reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM2_STATUS);
> + if (reg < 0)
> + return reg;
> + if (reg & 0x1f)
> + return -ENODEV;
> +
> + reg = i2c_smbus_read_word_swapped(client, TMP464_DEVICE_ID_REG);
> + if (reg < 0)
> + return reg;
> + switch (reg) {
> + case TMP464_DEVICE_ID:
> + kind = tmp464;
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + strscpy(info->type, tmp464_id[kind].name, I2C_NAME_SIZE);
> + dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
> + names[kind], client->addr);
> +
> + return 0;
> +}
> +
> +static int tmp464_probe_child_from_dt(struct i2c_client *client,
> + struct device_node *child,
> + struct tmp464_data *data)
> +
> +{
> + struct device *dev = &client->dev;
> + u32 i;
> + s32 val;
> + int err;
> +
> + err = of_property_read_u32(child, "reg", &i);
> + if (err) {
> + dev_err(dev, "missing reg property of %pOFn\n", child);
> + return err;
> + }
> +
> + if (i >= data->channels) {
> + dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
> + return -EINVAL;
> + }
> +
> + of_property_read_string(child, "label", &data->channel[i].label);
> + if (data->channel[i].label)
> + data->temp_config[i] |= HWMON_T_LABEL;
> +
> + data->channel[i].enabled = of_device_is_available(child);
> +
> + err = of_property_read_s32(child, "ti,n-factor", &val);
> + if (!err) {
> + if (i == 0) {
> + dev_err(dev, "n-factor can't be set for internal channel\n");
> + return -EINVAL;
> + }
> +
> + if (val > 127 || val < -128) {
> + dev_err(dev, "n-factor for channel %d invalid (%d)\n",
> + i, val);
> + return -EINVAL;
> + }
> + err = i2c_smbus_write_word_data(client, TMP464_N_FACTOR_REG_1 + i - 1, val);
> + if (err < 0)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int tmp464_probe_from_dt(struct i2c_client *client, struct tmp464_data *data)
> +{
> + struct device *dev = &client->dev;
> + const struct device_node *np = dev->of_node;
> + struct device_node *child;
> + int err;
> +
> + for_each_child_of_node(np, child) {
> + if (strcmp(child->name, "channel"))
> + continue;
> +
> + err = tmp464_probe_child_from_dt(client, child, data);
> + if (err) {
> + of_node_put(child);
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct hwmon_ops tmp464_ops = {
> + .is_visible = tmp464_is_visible,
> + .read = tmp464_read,
> + .read_string = tmp464_read_string,
> + .write = tmp464_write,
> +};
> +
> +static int tmp464_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct device *hwmon_dev;
> + struct tmp464_data *data;
> + int i, err;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> + dev_err(&client->dev, "i2c functionality check failed\n");
> + return -ENODEV;
> + }
> + data = devm_kzalloc(dev, sizeof(struct tmp464_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + mutex_init(&data->update_lock);
> + if (client->dev.of_node)
> + data->channels = (unsigned long)of_device_get_match_data(&client->dev);
> + else
> + data->channels = i2c_match_id(tmp464_id, client)->driver_data;
> + data->client = client;
> +
> + for (i = 0; i < data->channels; i++) {
> + data->temp_config[i] = HWMON_T_INPUT | HWMON_T_ENABLE;
> + data->channel[i].enabled = true;
> + }
> +
> + err = tmp464_probe_from_dt(client, data);
> + if (err)
> + return err;
> +
> + err = tmp464_init_client(data);
> + if (err)
> + return err;
> +
> + data->chip.ops = &tmp464_ops;
> + data->chip.info = data->info;
> +
> + data->info[0] = &data->temp_info;
> +
> + data->temp_info.type = hwmon_temp;
> + data->temp_info.config = data->temp_config;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> + data, &data->chip, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct i2c_driver tmp464_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = "tmp464",
> + .of_match_table = of_match_ptr(tmp464_of_match),
> + },
> + .probe_new = tmp464_probe,
> + .id_table = tmp464_id,
> + .detect = tmp464_detect,
> + .address_list = normal_i2c,
> +};
> +
> +module_i2c_driver(tmp464_driver);
> +
> +MODULE_AUTHOR("Agathe Porte <agathe.porte@nokia.com>");
> +MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-11 5:16 ` Guenter Roeck
@ 2022-02-11 10:11 ` Agathe Porte
2022-02-11 14:20 ` Guenter Roeck
2022-02-11 23:53 ` Guenter Roeck
0 siblings, 2 replies; 14+ messages in thread
From: Agathe Porte @ 2022-02-11 10:11 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
Hi Guenter,
Le 11/02/2022 à 06:16, Guenter Roeck a écrit :
> On 2/9/22 07:53, Agathe Porte wrote:
>> Add support for Texas Instruments TMP464 temperature sensor IC.
>>
>> TI's TMP464 is an I2C temperature sensor chip. This chip is
>> similar to TI's TMP421 chip, but with 16bit-wide registers (instead
>> of 8bit-wide registers). The chip have one local sensor and four
>> remote sensors.
>>
>> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
>> Acked-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>
> First, please _never_ send a new version of a patch or patch series
> as response to an older version. I almost missed this version of the
> series.
I should have sent it without In-reply-to to the ML?
I have used the following example from git send-email --help as
reference, but I was probably wrong:
> So for example when --thread and --no-chain-reply-to are
> specified, the second and
> subsequent patches will be replies to the first one like in
> the illustration below
> where [PATCH v2 0/3] is in reply to [PATCH 0/2]:
>
> [PATCH 0/2] Here is what I did...
> [PATCH 1/2] Clean up and tests
> [PATCH 2/2] Implementation
> [PATCH v2 0/3] Here is a reroll
> [PATCH v2 1/3] Clean up
> [PATCH v2 2/3] New tests
> [PATCH v2 3/3] Implementation
>
> Second, I really dislike continuing this driver without all the attibutes
> making it valuable as hwmon driver, and I really want to drop local
> caching
> and use regmap instead. I can not get a TMP464 (they appear to be sold
> out
> until 2023 everywhere I checked, and I can not even get a sample from TI
> either). However, I ordered and - according to Fedex - should get an
> evaluation board for TMP468 tomorrow. Before we keep going back and
> forth, I'd prefer to use that chip to add support for all attributes (or
> in other words take the driver from here). Would that be ok with you ?
This looks fine for me, as long as copyright is kept if you plan to
reuse what I have proposed here. I have asked a colleague in the
hardware team to unsolder a few of them from dismissed boards since they
are hard to find due to the component shortage. I will ask you a
delivery address using my personal email with a proper GPG key when I
will be in possession of the chips. Would that be OK to you?
>
> Additional comment inline.
Since you are planning on taking over this driver I think I will not
propose a v3 driver. However I will try to fix the comments in our local
version as a learning opportunity. Please Cc me and Krzysztof when
posting your implementation so that we can backport it in our system.
BRs,
Agathe.
>
> Thanks,
> Guenter
>
>> ---
>> Documentation/hwmon/index.rst | 1 +
>> Documentation/hwmon/tmp464.rst | 43 ++++
>> MAINTAINERS | 2 +
>> drivers/hwmon/Kconfig | 10 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/tmp464.c | 447 +++++++++++++++++++++++++++++++++
>> 6 files changed, 504 insertions(+)
>> create mode 100644 Documentation/hwmon/tmp464.rst
>> create mode 100644 drivers/hwmon/tmp464.c
>>
>> diff --git a/Documentation/hwmon/index.rst
>> b/Documentation/hwmon/index.rst
>> index df20022c741f..37590db85e65 100644
>> --- a/Documentation/hwmon/index.rst
>> +++ b/Documentation/hwmon/index.rst
>> @@ -193,6 +193,7 @@ Hardware Monitoring Kernel Drivers
>> tmp108
>> tmp401
>> tmp421
>> + tmp464
>> tmp513
>> tps23861
>> tps40422
>> diff --git a/Documentation/hwmon/tmp464.rst
>> b/Documentation/hwmon/tmp464.rst
>> new file mode 100644
>> index 000000000000..8990554194de
>> --- /dev/null
>> +++ b/Documentation/hwmon/tmp464.rst
>> @@ -0,0 +1,43 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +Kernel driver tmp421
>> +====================
>> +
>> +Supported chips:
>> +
>> + * Texas Instruments TMP464
>> +
>> + Prefix: 'tmp464'
>> +
>> + Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
>> +
>> + Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp464.html
>> +
>> +Authors:
>> +
>> + Agathe Porte <agathe.porte@nokia.com>
>> +
>> +Description
>> +-----------
>> +
>> +This driver implements support for Texas Instruments TMP464
>> +temperature sensor chip. This chip implement one local four remote
>> +sensors. Temperature is measured in degrees Celsius. The chips are
>> +wired over I2C/SMBus and specified over a temperature range of -40 to
>> ++125 degrees Celsius. Resolution for both the local and remote
>> +channels is 0.0625 degree C.
>> +
>> +The chips support only temperature measurement. The driver exports
>> +the temperature values via the following sysfs files:
>> +
>> +**temp[1-5]_input**
>> +
>> +Each sensor can be individually disabled via Devicetree or from sysfs
>> +via:
>> +
>> +**temp[1-5]_enable**
>> +
>> +If labels were specified in Devicetree, additional sysfs files will
>> +be present:
>> +
>> +**temp[1-5]_label**
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 136cd34be715..7fa2796adbef 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19462,6 +19462,8 @@ M: Agathe Porte <agathe.porte@nokia.com>
>> L: linux-hwmon@vger.kernel.org
>> S: Maintained
>> F: Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
>> +F: Documentation/hwmon/tmp464.rst
>> +F: drivers/hwmon/tmp464.c
>>
>> TMP513 HARDWARE MONITOR DRIVER
>> M: Eric Tremblay <etremblay@distech-controls.com>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 8df25f1079ba..52b4f5688b45 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1979,6 +1979,16 @@ config SENSORS_TMP421
>> This driver can also be built as a module. If so, the module
>> will be called tmp421.
>>
>> +config SENSORS_TMP464
>> + tristate "Texas Instruments TMP464 and compatible"
>> + depends on I2C
>> + help
>> + If you say yes here you get support for Texas Instruments TMP464
>> + temperature sensor chip.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called tmp464.
>> +
>> config SENSORS_TMP513
>> tristate "Texas Instruments TMP513 and compatibles"
>> depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 185f946d698b..a1f2d6686227 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
>> obj-$(CONFIG_SENSORS_TMP108) += tmp108.o
>> obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
>> obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
>> +obj-$(CONFIG_SENSORS_TMP464) += tmp464.o
>> obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
>> obj-$(CONFIG_SENSORS_VEXPRESS) += vexpress-hwmon.o
>> obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>> diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
>> new file mode 100644
>> index 000000000000..564090929ad4
>> --- /dev/null
>> +++ b/drivers/hwmon/tmp464.c
>> @@ -0,0 +1,447 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
>> + * Supported models: TMP464
>> +
>> + * Copyright (C) 2022 Agathe Porte <agathe.porte@nokia.com>
>> + * Preliminary support by:
>> + * Lionel Pouliquen <lionel.lp.pouliquen@nokia.com>
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +
>> +/* Addresses to scan */
>> +static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
>> I2C_CLIENT_END };
>> +
>> +enum chips { tmp464 };
>> +
>> +#define MAX_CHANNELS 5 /* chan 0 is internal, 1-4 are
>> remote */
>> +
>> +/* TMP464 registers */
>> +static const u8 TMP464_TEMP[MAX_CHANNELS] = { 0x00, 0x01, 0x02,
>> 0x03, 0x04 };
>> +static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = { 0x39, 0x42,
>> 0x4A, 0x52, 0x5A };
>> +static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = { 0x3A, 0x43,
>> 0x4B, 0x53, 0x5B };
>> +static const u8 TMP464_OFFSET_REMOTE[MAX_CHANNELS] = { 0x40, 0x48,
>> 0x50, 0x58 };
>> +#define TMP464_N_FACTOR_REG_1 0x41
>> +#define TMP464_CONFIG_REG 0x30
>> +
>> +/* Identification */
>> +#define TMP464_MANUFACTURER_ID_REG 0xFE
>> +#define TMP464_DEVICE_ID_REG 0xFF
>> +
>> +/* Flags */
>> +#define TMP464_CONFIG_SHUTDOWN BIT(5)
>> +#define TMP464_CONFIG_RANGE 0x04
>> +#define TMP464_CONFIG_REG_REN(x) (BIT(7 + (x)))
>> +#define TMP464_CONFIG_REG_REN_MASK GENMASK(11, 7)
>> +#define TMP464_CONFIG_CONVERSION_RATE_B0 2
>> +#define TMP464_CONFIG_CONVERSION_RATE_MASK
>> GENMASK(TMP464_CONFIG_CONVERSION_RATE_B0, \
>> + TMP464_CONFIG_CONVERSION_RATE_B0 + 2)
>> +
>> +/* Manufacturer / Device ID's */
>> +#define TMP464_MANUFACTURER_ID 0x5449
>> +#define TMP464_DEVICE_ID 0x1468
>> +
>> +static const struct i2c_device_id tmp464_id[] = {
>> + { "tmp464", 0 },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, tmp464_id);
>> +
>> +static const struct of_device_id __maybe_unused tmp464_of_match[] = {
>> + {
>> + .compatible = "ti,tmp464",
>> + .data = (void *)5
>> + },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, tmp464_of_match);
>> +
>> +struct tmp464_channel {
>> + const char *label;
>> + bool enabled;
>> + s16 temp;
>> +};
>> +
>> +struct tmp464_data {
>> + struct i2c_client *client;
>> + struct mutex update_lock; /* used to update the struct values */
>> + u32 temp_config[MAX_CHANNELS + 1];
>> + struct hwmon_channel_info temp_info;
>> + const struct hwmon_channel_info *info[2];
>> + struct hwmon_chip_info chip;
>
> There is really no good reason to have the above structures dynamically
> allocated. They can as well be declared statically. The only dynamic
> attribute is the label, and the visibility of that attribute can be
> controlled with the is_visible callback. After all, that is what
> the callback is for.
>
>> + bool valid;
>> + unsigned long last_updated;
>> + unsigned long channels;
>
> Not sure why this should be unsigned long.
>
>> + u16 config;
>> + struct tmp464_channel channel[MAX_CHANNELS];
>> +};
>> +
>> +static int temp_from_raw(s16 reg)
>> +{
>> + return DIV_ROUND_CLOSEST((reg >> 3) * 625, 10);
>> +}
>> +
>> +static int tmp464_update_device(struct tmp464_data *data)
>> +{
>> + struct i2c_client *client = data->client;
>> + int ret = 0;
>> + int i;
>> +
>> + mutex_lock(&data->update_lock);
>> +
>> + if (time_after(jiffies, data->last_updated + (HZ / 2)) ||
>> + !data->valid) {
>> + ret = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>> + if (ret < 0)
>> + goto exit;
>> + data->config = ret;
>> +
>> + for (i = 0; i < data->channels; i++) {
>> + ret = i2c_smbus_read_word_swapped(client, TMP464_TEMP[i]);
>> + if (ret < 0)
>> + goto exit;
>> + data->channel[i].temp = ret;
>> + }
>> + data->last_updated = jiffies;
>> + data->valid = true;
>> + }
>> +
>> +exit:
>> + mutex_unlock(&data->update_lock);
>> +
>> + if (ret < 0) {
>> + data->valid = false;
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tmp464_enable_channels(struct tmp464_data *data)
>> +{
>> + int err;
>> + struct i2c_client *client = data->client;
>> + struct device *dev = &client->dev;
>> + int old = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>> + int new, i;
>> +
>> + if (old < 0) {
>> + dev_err(dev, "error reading register, can't disable
>> channels\n");
>> + return old;
>> + }
>> +
>> + new = old & ~TMP464_CONFIG_REG_REN_MASK;
>> + for (i = 0; i < data->channels; i++)
>> + if (data->channel[i].enabled)
>> + new |= TMP464_CONFIG_REG_REN(i);
>> +
>> + if (new == old)
>> + return 0;
>> +
>> + err = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, new);
>> + if (err < 0)
>> + dev_err(dev, "error writing register, can't disable
>> channels\n");
>> +
>> + return err;
>> +}
>> +
>> +static int tmp464_read(struct device *dev, enum hwmon_sensor_types
>> type,
>> + u32 attr, int channel, long *val)
>> +{
>> + struct tmp464_data *tmp464 = dev_get_drvdata(dev);
>> + int ret = 0;
>> +
>> + ret = tmp464_update_device(tmp464);
>> + if (ret)
>> + return ret;
>> +
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + if (!tmp464->channel[channel].enabled)
>> + return -ENODATA;
>> + *val = temp_from_raw(tmp464->channel[channel].temp);
>> + return 0;
>> + case hwmon_temp_enable:
>> + *val = tmp464->channel[channel].enabled;
>> + return 0;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> +static int tmp464_read_string(struct device *dev, enum
>> hwmon_sensor_types type,
>> + u32 attr, int channel, const char **str)
>> +{
>> + struct tmp464_data *data = dev_get_drvdata(dev);
>> +
>> + *str = data->channel[channel].label;
>> +
>> + return 0;
>> +}
>> +
>> +static int tmp464_write(struct device *dev, enum hwmon_sensor_types
>> type,
>> + u32 attr, int channel, long val)
>> +{
>> + struct tmp464_data *data = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + switch (attr) {
>> + case hwmon_temp_enable:
>> + data->channel[channel].enabled = val;
>> + ret = tmp464_enable_channels(data);
>> + break;
>> + default:
>> + ret = -EOPNOTSUPP;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static umode_t tmp464_is_visible(const void *data, enum
>> hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + return 0444;
>> + case hwmon_temp_label:
>> + return 0444;
>> + case hwmon_temp_enable:
>> + return 0644;
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> +static int tmp464_init_client(struct tmp464_data *data)
>> +{
>> + int err;
>> + int config, config_orig;
>> + struct i2c_client *client = data->client;
>> +
>> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>> + if (config < 0) {
>> + dev_err(&client->dev,
>> + "Could not read configuration register (%d)\n", config);
>> + return config;
>> + }
>> +
>> + /* Set the conversion rate to 2 Hz */
>> + config_orig = config;
>> + config &= ~TMP464_CONFIG_CONVERSION_RATE_MASK;
>> + config |= (0x05) << TMP464_CONFIG_CONVERSION_RATE_B0;
>> +
>> + /* Start conversions (disable shutdown if necessary) */
>> + config_orig = config;
>> + config &= ~TMP464_CONFIG_SHUTDOWN;
>> +
>> + if (config != config_orig) {
>> + config = i2c_smbus_write_word_data(client,
>> TMP464_CONFIG_REG, config);
>> + if (config < 0) {
>> + dev_err(&client->dev,
>> + "Could not write configuration register (%d)\n", err);
>> + return config;
>> + }
>> + }
>> +
>> + return tmp464_enable_channels(data);
>> +}
>> +
>> +static int tmp464_detect(struct i2c_client *client,
>> + struct i2c_board_info *info)
>> +{
>> + enum chips kind;
>> + struct i2c_adapter *adapter = client->adapter;
>> + static const char * const names[] = {
>> + "TMP464"
>> + };
>> + int reg;
>> +
>> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
>> + return -ENODEV;
>> +
>> + reg = i2c_smbus_read_word_swapped(client,
>> TMP464_MANUFACTURER_ID_REG);
>> + if (reg < 0)
>> + return reg;
>> + if (reg != TMP464_MANUFACTURER_ID)
>> + return -ENODEV;
>> +
>> + /* Check for "always return zero" bits */
>> + reg = i2c_smbus_read_word_swapped(client, TMP464_REG_THERM_STATUS);
>> + if (reg < 0)
>> + return reg;
>> + if (reg & 0x1f)
>> + return -ENODEV;
>> + reg = i2c_smbus_read_word_swapped(client,
>> TMP464_REG_THERM2_STATUS);
>> + if (reg < 0)
>> + return reg;
>> + if (reg & 0x1f)
>> + return -ENODEV;
>> +
>> + reg = i2c_smbus_read_word_swapped(client, TMP464_DEVICE_ID_REG);
>> + if (reg < 0)
>> + return reg;
>> + switch (reg) {
>> + case TMP464_DEVICE_ID:
>> + kind = tmp464;
>> + break;
>> + default:
>> + return -ENODEV;
>> + }
>> +
>> + strscpy(info->type, tmp464_id[kind].name, I2C_NAME_SIZE);
>> + dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
>> + names[kind], client->addr);
>> +
>> + return 0;
>> +}
>> +
>> +static int tmp464_probe_child_from_dt(struct i2c_client *client,
>> + struct device_node *child,
>> + struct tmp464_data *data)
>> +
>> +{
>> + struct device *dev = &client->dev;
>> + u32 i;
>> + s32 val;
>> + int err;
>> +
>> + err = of_property_read_u32(child, "reg", &i);
>> + if (err) {
>> + dev_err(dev, "missing reg property of %pOFn\n", child);
>> + return err;
>> + }
>> +
>> + if (i >= data->channels) {
>> + dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
>> + return -EINVAL;
>> + }
>> +
>> + of_property_read_string(child, "label", &data->channel[i].label);
>> + if (data->channel[i].label)
>> + data->temp_config[i] |= HWMON_T_LABEL;
>> +
>> + data->channel[i].enabled = of_device_is_available(child);
>> +
>> + err = of_property_read_s32(child, "ti,n-factor", &val);
>> + if (!err) {
>> + if (i == 0) {
>> + dev_err(dev, "n-factor can't be set for internal
>> channel\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (val > 127 || val < -128) {
>> + dev_err(dev, "n-factor for channel %d invalid (%d)\n",
>> + i, val);
>> + return -EINVAL;
>> + }
>> + err = i2c_smbus_write_word_data(client,
>> TMP464_N_FACTOR_REG_1 + i - 1, val);
>> + if (err < 0)
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tmp464_probe_from_dt(struct i2c_client *client, struct
>> tmp464_data *data)
>> +{
>> + struct device *dev = &client->dev;
>> + const struct device_node *np = dev->of_node;
>> + struct device_node *child;
>> + int err;
>> +
>> + for_each_child_of_node(np, child) {
>> + if (strcmp(child->name, "channel"))
>> + continue;
>> +
>> + err = tmp464_probe_child_from_dt(client, child, data);
>> + if (err) {
>> + of_node_put(child);
>> + return err;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct hwmon_ops tmp464_ops = {
>> + .is_visible = tmp464_is_visible,
>> + .read = tmp464_read,
>> + .read_string = tmp464_read_string,
>> + .write = tmp464_write,
>> +};
>> +
>> +static int tmp464_probe(struct i2c_client *client)
>> +{
>> + struct device *dev = &client->dev;
>> + struct device *hwmon_dev;
>> + struct tmp464_data *data;
>> + int i, err;
>> +
>> + if (!i2c_check_functionality(client->adapter,
>> I2C_FUNC_SMBUS_WORD_DATA)) {
>> + dev_err(&client->dev, "i2c functionality check failed\n");
>> + return -ENODEV;
>> + }
>> + data = devm_kzalloc(dev, sizeof(struct tmp464_data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + mutex_init(&data->update_lock);
>> + if (client->dev.of_node)
>> + data->channels = (unsigned
>> long)of_device_get_match_data(&client->dev);
>> + else
>> + data->channels = i2c_match_id(tmp464_id, client)->driver_data;
>> + data->client = client;
>> +
>> + for (i = 0; i < data->channels; i++) {
>> + data->temp_config[i] = HWMON_T_INPUT | HWMON_T_ENABLE;
>> + data->channel[i].enabled = true;
>> + }
>> +
>> + err = tmp464_probe_from_dt(client, data);
>> + if (err)
>> + return err;
>> +
>> + err = tmp464_init_client(data);
>> + if (err)
>> + return err;
>> +
>> + data->chip.ops = &tmp464_ops;
>> + data->chip.info = data->info;
>> +
>> + data->info[0] = &data->temp_info;
>> +
>> + data->temp_info.type = hwmon_temp;
>> + data->temp_info.config = data->temp_config;
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
>> + data, &data->chip, NULL);
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static struct i2c_driver tmp464_driver = {
>> + .class = I2C_CLASS_HWMON,
>> + .driver = {
>> + .name = "tmp464",
>> + .of_match_table = of_match_ptr(tmp464_of_match),
>> + },
>> + .probe_new = tmp464_probe,
>> + .id_table = tmp464_id,
>> + .detect = tmp464_detect,
>> + .address_list = normal_i2c,
>> +};
>> +
>> +module_i2c_driver(tmp464_driver);
>> +
>> +MODULE_AUTHOR("Agathe Porte <agathe.porte@nokia.com>");
>> +MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor
>> driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.34.1
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-11 10:11 ` Agathe Porte
@ 2022-02-11 14:20 ` Guenter Roeck
2022-02-11 23:53 ` Guenter Roeck
1 sibling, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2022-02-11 14:20 UTC (permalink / raw)
To: Agathe Porte, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
On 2/11/22 02:11, Agathe Porte wrote:
> Hi Guenter,
>
> Le 11/02/2022 à 06:16, Guenter Roeck a écrit :
>> On 2/9/22 07:53, Agathe Porte wrote:
>>> Add support for Texas Instruments TMP464 temperature sensor IC.
>>>
>>> TI's TMP464 is an I2C temperature sensor chip. This chip is
>>> similar to TI's TMP421 chip, but with 16bit-wide registers (instead
>>> of 8bit-wide registers). The chip have one local sensor and four
>>> remote sensors.
>>>
>>> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
>>> Acked-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>>
>> First, please _never_ send a new version of a patch or patch series
>> as response to an older version. I almost missed this version of the series.
>
> I should have sent it without In-reply-to to the ML?
>
> I have used the following example from git send-email --help as reference, but I was probably wrong:
>
Please see Documentation/process/submitting-patches.rst,
"Explicit In-Reply-To headers".
>> So for example when --thread and --no-chain-reply-to are specified, the second and
>> subsequent patches will be replies to the first one like in the illustration below
>> where [PATCH v2 0/3] is in reply to [PATCH 0/2]:
>>
>> [PATCH 0/2] Here is what I did...
>> [PATCH 1/2] Clean up and tests
>> [PATCH 2/2] Implementation
>> [PATCH v2 0/3] Here is a reroll
>> [PATCH v2 1/3] Clean up
>> [PATCH v2 2/3] New tests
>> [PATCH v2 3/3] Implementation
>
>>
>> Second, I really dislike continuing this driver without all the attibutes
>> making it valuable as hwmon driver, and I really want to drop local caching
>> and use regmap instead. I can not get a TMP464 (they appear to be sold out
>> until 2023 everywhere I checked, and I can not even get a sample from TI
>> either). However, I ordered and - according to Fedex - should get an
>> evaluation board for TMP468 tomorrow. Before we keep going back and
>> forth, I'd prefer to use that chip to add support for all attributes (or
>> in other words take the driver from here). Would that be ok with you ?
> This looks fine for me, as long as copyright is kept if you plan to reuse what I have proposed here. I have asked a colleague in the hardware team to unsolder a few of them from dismissed boards since they are hard to find due to the component shortage. I will ask you a delivery address using my personal email with a proper GPG key when I will be in possession of the chips. Would that be OK to you?
Sounds good.
>>
>> Additional comment inline.
>
> Since you are planning on taking over this driver I think I will not propose a v3 driver. However I will try to fix the comments in our local version as a learning opportunity. Please Cc me and Krzysztof when posting your implementation so that we can backport it in our system.
Sure, will do.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-11 10:11 ` Agathe Porte
2022-02-11 14:20 ` Guenter Roeck
@ 2022-02-11 23:53 ` Guenter Roeck
2022-02-15 13:40 ` Agathe Porte
1 sibling, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2022-02-11 23:53 UTC (permalink / raw)
To: Agathe Porte, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
Hi Agathe,
On 2/11/22 02:11, Agathe Porte wrote:
[ ... ]
>>> +
>>> +static int tmp464_init_client(struct tmp464_data *data)
>>> +{
>>> + int err;
>>> + int config, config_orig;
>>> + struct i2c_client *client = data->client;
>>> +
>>> + config = i2c_smbus_read_word_swapped(client, TMP464_CONFIG_REG);
>>> + if (config < 0) {
>>> + dev_err(&client->dev,
>>> + "Could not read configuration register (%d)\n", config);
>>> + return config;
>>> + }
>>> +
>>> + /* Set the conversion rate to 2 Hz */
>>> + config_orig = config;
>>> + config &= ~TMP464_CONFIG_CONVERSION_RATE_MASK;
>>> + config |= (0x05) << TMP464_CONFIG_CONVERSION_RATE_B0;
>>> +
>>> + /* Start conversions (disable shutdown if necessary) */
>>> + config_orig = config;
>>> + config &= ~TMP464_CONFIG_SHUTDOWN;
>>> +
>>> + if (config != config_orig) {
>>> + config = i2c_smbus_write_word_data(client, TMP464_CONFIG_REG, config);
>>> + if (config < 0) {
>>> + dev_err(&client->dev,
>>> + "Could not write configuration register (%d)\n", err);
>>> + return config;
>>> + }
>>> + }
>>> +
>>> + return tmp464_enable_channels(data);
>>> +}
>>> +
Turns out the chip is by default locked, meaning all those configuration writes
fail unless I explicitly unlock the chip. How would you suggest to handle that
situation, and how do you handle it in your application ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-11 23:53 ` Guenter Roeck
@ 2022-02-15 13:40 ` Agathe Porte
2022-02-15 14:13 ` Guenter Roeck
0 siblings, 1 reply; 14+ messages in thread
From: Agathe Porte @ 2022-02-15 13:40 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
Hi Guenter,
Le 12/02/2022 à 00:53, Guenter Roeck a écrit :
> Turns out the chip is by default locked, meaning all those
> configuration writes
> fail unless I explicitly unlock the chip. How would you suggest to
> handle that
> situation, and how do you handle it in your application ?
>
After reading the TMP464 datasheet, the device seems locked by default
indeed:
> All of the configuration and limit registers may be locked for writes
> (making the registers write-protected), which
> decreases the chance of software runaway from issuing false changes to
> these registers. The Lock column in
> Table 3 identifies which registers may be locked. Lock mode does not
> effect read operations. To activate the lock
> mode, Lock Register C4h must be set to 0x5CA6. The lock only remains
> active while the TMP464 device is
> powered up. Because the TMP464 device does not contain nonvolatile
> memory, the settings of the configuration
> and limit registers are lost once a power cycle occurs regardless if
> the registers are locked or unlocked.
> In lock mode, the TMP464 device ignores a write operation to
> configuration and limit registers except for Lock
> Register C4h. The TMP464 device does not acknowledge the data bytes
> during a write operation to a locked
> register. To unlock the TMP464 registers, write 0xEB19 to register
> C4h. The TMP464 device powers up in locked
> mode, so the registers must be unlocked before the registers accept
> writes of new data.
From my deduction, since we do not unlock it with software, I guess
that our local FPGA is doing the job of disabling the lock during
power-on procedure.
I guess we could read and unlock the device when probing, and relock it
after rmmod if it was locked during probing (store initial lock state).
We do not want to relock it if it was unlocked before probe — like in
our usecase — because other applications may use this "already unlocked"
assumption (bootloader, FPGA code, …).
What do you think?
Best regards,
Agathe.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip
2022-02-15 13:40 ` Agathe Porte
@ 2022-02-15 14:13 ` Guenter Roeck
0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2022-02-15 14:13 UTC (permalink / raw)
To: Agathe Porte, linux-hwmon; +Cc: Jean Delvare, Krzysztof Adamski
Hi Agathe,
On 2/15/22 05:40, Agathe Porte wrote:
> Hi Guenter,
>
> Le 12/02/2022 à 00:53, Guenter Roeck a écrit :
>> Turns out the chip is by default locked, meaning all those configuration writes
>> fail unless I explicitly unlock the chip. How would you suggest to handle that
>> situation, and how do you handle it in your application ?
>>
> After reading the TMP464 datasheet, the device seems locked by default indeed:
>
>> All of the configuration and limit registers may be locked for writes (making the registers write-protected), which
>> decreases the chance of software runaway from issuing false changes to these registers. The Lock column in
>> Table 3 identifies which registers may be locked. Lock mode does not effect read operations. To activate the lock
>> mode, Lock Register C4h must be set to 0x5CA6. The lock only remains active while the TMP464 device is
>> powered up. Because the TMP464 device does not contain nonvolatile memory, the settings of the configuration
>> and limit registers are lost once a power cycle occurs regardless if the registers are locked or unlocked.
>> In lock mode, the TMP464 device ignores a write operation to configuration and limit registers except for Lock
>> Register C4h. The TMP464 device does not acknowledge the data bytes during a write operation to a locked
>> register. To unlock the TMP464 registers, write 0xEB19 to register C4h. The TMP464 device powers up in locked
>> mode, so the registers must be unlocked before the registers accept writes of new data.
>
> From my deduction, since we do not unlock it with software, I guess that our local FPGA is doing the job of disabling the lock during power-on procedure.
>
> I guess we could read and unlock the device when probing, and relock it after rmmod if it was locked during probing (store initial lock state). We do not want to relock it if it was unlocked before probe — like in our usecase — because other applications may use this "already unlocked" assumption (bootloader, FPGA code, …).
>
> What do you think?
>
Yes, that is exactly what I ended up doing.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-02-15 14:13 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-09 13:36 [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml Agathe Porte
2022-02-09 13:36 ` [PATCH 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip Agathe Porte
2022-02-09 14:46 ` Guenter Roeck
2022-02-09 15:45 ` Agathe Porte
2022-02-09 17:37 ` Guenter Roeck
2022-02-09 15:11 ` [PATCH 1/2] dt-bindings: hwmon: add tmp464.yaml Guenter Roeck
2022-02-09 15:53 ` [PATCH v2 " Agathe Porte
2022-02-09 15:53 ` [PATCH v2 2/2] hwmon: Add driver for Texas Instruments TMP464 sensor chip Agathe Porte
2022-02-11 5:16 ` Guenter Roeck
2022-02-11 10:11 ` Agathe Porte
2022-02-11 14:20 ` Guenter Roeck
2022-02-11 23:53 ` Guenter Roeck
2022-02-15 13:40 ` Agathe Porte
2022-02-15 14:13 ` Guenter Roeck
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.