From: Agathe Porte <[email protected]>
Add basic description of the tmp464 driver DT bindings.
Signed-off-by: Agathe Porte <[email protected]>
Cc: Krzysztof Adamski <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
v5:
- Dropped ti,n-factor from channel@0 example. Added additional
channel to examples to show positive ti,n-factor property.
v4:
- No changes
v3:
- Addedd support for TMP468.
- Changed number of channels from 0..3 (which was wrong anyway) to 0..8.
- Changed value range for ti,n-factor to int8, with an example for
a negative value.
- Added myself as driver maintainer.
.../devicetree/bindings/hwmon/ti,tmp464.yaml | 114 ++++++++++++++++++
MAINTAINERS | 7 ++
2 files changed, 121 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..14f6a3412b8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
@@ -0,0 +1,114 @@
+# 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 and TMP468 temperature sensors
+
+maintainers:
+ - Agathe Porte <[email protected]>
+
+description: |
+ ±0.0625°C Remote and Local temperature sensor
+ https://www.ti.com/lit/ds/symlink/tmp464.pdf
+ https://www.ti.com/lit/ds/symlink/tmp468.pdf
+
+properties:
+ compatible:
+ enum:
+ - ti,tmp464
+ - ti,tmp468
+
+ reg:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+patternProperties:
+ "^channel@([0-8])$":
+ type: object
+ description: |
+ Represents channels of the device and their specific configuration.
+
+ properties:
+ reg:
+ description: |
+ The channel number. 0 is local channel, 1-8 are remote channels.
+ items:
+ minimum: 0
+ maximum: 8
+
+ 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/int8
+ items:
+ minimum: -128
+ maximum: 127
+
+ 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>;
+ label = "local";
+ };
+
+ channel@1 {
+ reg = <0x1>;
+ ti,n-factor = /bits/ 8 <(-10)>;
+ label = "external";
+ };
+
+ channel@2 {
+ reg = <0x2>;
+ ti,n-factor = /bits/ 8 <0x10>;
+ label = "somelabel";
+ };
+
+ channel@3 {
+ reg = <0x3>;
+ status = "disabled";
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index fca970a46e77..f51bc7c7e439 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19489,6 +19489,13 @@ S: Maintained
F: Documentation/hwmon/tmp401.rst
F: drivers/hwmon/tmp401.c
+TMP464 HARDWARE MONITOR DRIVER
+M: Agathe Porte <[email protected]>
+M: Guenter Roeck <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
+
TMP513 HARDWARE MONITOR DRIVER
M: Eric Tremblay <[email protected]>
L: [email protected]
--
2.35.1
Add support for Texas Instruments TMP464 and TMP468 temperature sensor
ICs.
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 has one local sensor and four remote
sensors. TMP468 is similar to TMP464 but has one local and eight
remote sensors.
Originally-from: Agathe Porte <[email protected]>
Cc: Agathe Porte <[email protected]>
Cc: Krzysztof Adamski <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
v5:
- Dropped unused variable
v4:
- Fixed reading n-factor information from devicetree
[Use of_property_read_u8 instead of of_property_read_s32 to read the
property value, and write n-factor value into the upper 8 bit of the
n-factor register]
- Fixed displaying label attributes
v3:
- Added support for TMP468
- Added support for various limits, temperature hysteresis, alarm attributes,
and update interval
- Use regmap instead of local caching
- Use static chip configuration
- Unlock check if needed when loading driver, and lock it when unloading it
- Call tmp464_init_client() before calling tmp464_probe_from_dt()
since the latter changes registers, which requires the chip to be
unlocked.
- Restore configuration register when unloading driver
- ti,n-factor is optional, so don't fail if the property is not present
Notes:
- Tested with real TMP468. Module tested for TMP464.
Devicetree code tested with rudimentary qemu implementation.
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/tmp464.rst | 73 ++++
MAINTAINERS | 2 +
drivers/hwmon/Kconfig | 11 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/tmp464.c | 689 +++++++++++++++++++++++++++++++++
6 files changed, 777 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..7596e7623d06
--- /dev/null
+++ b/Documentation/hwmon/tmp464.rst
@@ -0,0 +1,73 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver tmp464
+====================
+
+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
+
+ * Texas Instruments TMP468
+
+ Prefix: 'tmp468'
+
+ Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
+
+ Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp468.html
+
+Authors:
+
+ Agathe Porte <[email protected]>
+ Guenter Roeck <[email protected]>
+
+Description
+-----------
+
+This driver implements support for Texas Instruments TMP464 and TMP468
+temperature sensor chips. TMP464 provides one local and four remote
+sensors. TMP468 provides one local and eight 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 measurements. The driver exports
+temperature values, limits, and alarms via the following sysfs files:
+
+**temp[1-9]_input**
+
+**temp[1-9]_max**
+
+**temp[1-9]_max_hyst**
+
+**temp[1-9]_max_alarm**
+
+**temp[1-9]_crit**
+
+**temp[1-9]_crit_alarm**
+
+**temp[1-9]_crit_hyst**
+
+**temp[2-9]_offset**
+
+**temp[2-9]_fault**
+
+Each sensor can be individually disabled via Devicetree or from sysfs
+via:
+
+**temp[1-9]_enable**
+
+If labels were specified in Devicetree, additional sysfs files will
+be present:
+
+**temp[1-9]_label**
+
+The update interval is configurable with the following sysfs attribute.
+
+**update_interval**
diff --git a/MAINTAINERS b/MAINTAINERS
index f51bc7c7e439..9b51bf5ca3b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19495,6 +19495,8 @@ M: Guenter Roeck <[email protected]>
L: [email protected]
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 <[email protected]>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 8df25f1079ba..b84932fd0c13 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1979,6 +1979,17 @@ 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
+ select REGMAP_I2C
+ help
+ If you say yes here you get support for Texas Instruments TMP464
+ and TMP468 temperature sensor chips.
+
+ 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..438ed9d9aa00
--- /dev/null
+++ b/drivers/hwmon/tmp464.c
@@ -0,0 +1,689 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
+ * Supported models: TMP464, TMP468
+
+ * Copyright (C) 2022 Agathe Porte <[email protected]>
+ * Preliminary support by:
+ * Lionel Pouliquen <[email protected]>
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b, I2C_CLIENT_END };
+
+#define TMP464_NUM_CHANNELS 5 /* chan 0 is internal, 1-4 are remote */
+#define TMP468_NUM_CHANNELS 9 /* chan 0 is internal, 1-8 are remote */
+
+#define MAX_CHANNELS 9
+
+#define TMP464_TEMP_REG(channel) (channel)
+#define TMP464_TEMP_OFFSET_REG(channel) (0x40 + ((channel) - 1) * 8)
+#define TMP464_N_FACTOR_REG(channel) (0x41 + ((channel) - 1) * 8)
+
+static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = {
+ 0x39, 0x42, 0x4A, 0x52, 0x5A, 0x62, 0x6a, 0x72, 0x7a };
+static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = {
+ 0x3A, 0x43, 0x4B, 0x53, 0x5B, 0x63, 0x6b, 0x73, 0x7b };
+
+#define TMP464_THERM_STATUS_REG 0x21
+#define TMP464_THERM2_STATUS_REG 0x22
+#define TMP464_REMOTE_OPEN_REG 0x23
+#define TMP464_CONFIG_REG 0x30
+#define TMP464_TEMP_HYST_REG 0x38
+#define TMP464_LOCK_REG 0xc4
+
+/* 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(15, 7)
+#define TMP464_CONFIG_CONVERSION_RATE_B0 2
+#define TMP464_CONFIG_CONVERSION_RATE_B2 4
+#define TMP464_CONFIG_CONVERSION_RATE_MASK GENMASK(TMP464_CONFIG_CONVERSION_RATE_B2, \
+ TMP464_CONFIG_CONVERSION_RATE_B0)
+
+#define TMP464_UNLOCK_VAL 0xeb19
+#define TMP464_LOCK_VAL 0x5ca6
+#define TMP464_LOCKED 0x8000
+
+/* Manufacturer / Device ID's */
+#define TMP464_MANUFACTURER_ID 0x5449
+#define TMP464_DEVICE_ID 0x1468
+#define TMP468_DEVICE_ID 0x0468
+
+static const struct i2c_device_id tmp464_id[] = {
+ { "tmp464", TMP464_NUM_CHANNELS },
+ { "tmp468", TMP468_NUM_CHANNELS },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, tmp464_id);
+
+static const struct of_device_id __maybe_unused tmp464_of_match[] = {
+ {
+ .compatible = "ti,tmp464",
+ .data = (void *)TMP464_NUM_CHANNELS
+ },
+ {
+ .compatible = "ti,tmp468",
+ .data = (void *)TMP468_NUM_CHANNELS
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, tmp464_of_match);
+
+struct tmp464_channel {
+ const char *label;
+ bool enabled;
+};
+
+struct tmp464_data {
+ struct regmap *regmap;
+ struct mutex update_lock;
+ int channels;
+ s16 config_orig;
+ struct tmp464_channel channel[MAX_CHANNELS];
+};
+
+static int temp_from_reg(s16 reg)
+{
+ return DIV_ROUND_CLOSEST((reg >> 3) * 625, 10);
+}
+
+static s16 temp_to_limit_reg(long temp)
+{
+ return DIV_ROUND_CLOSEST(temp, 500) << 6;
+}
+
+static s16 temp_to_offset_reg(long temp)
+{
+ return DIV_ROUND_CLOSEST(temp * 10, 625) << 3;
+}
+
+static int tmp464_enable_channels(struct tmp464_data *data)
+{
+ struct regmap *regmap = data->regmap;
+ u16 enable = 0;
+ int i;
+
+ for (i = 0; i < data->channels; i++)
+ if (data->channel[i].enabled)
+ enable |= TMP464_CONFIG_REG_REN(i);
+
+ return regmap_update_bits(regmap, TMP464_CONFIG_REG, TMP464_CONFIG_REG_REN_MASK, enable);
+}
+
+static int tmp464_chip_read(struct device *dev, u32 attr, int channel, long *val)
+{
+ struct tmp464_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
+ unsigned int status;
+ int err;
+
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ err = regmap_read(regmap, TMP464_CONFIG_REG, &status);
+ if (err)
+ return err;
+ status = (status & TMP464_CONFIG_CONVERSION_RATE_MASK) >>
+ TMP464_CONFIG_CONVERSION_RATE_B0;
+ *val = 125 << (7 - status);
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val)
+{
+ struct tmp464_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
+ unsigned int regval, regval2;
+ int err = 0;
+
+ switch (attr) {
+ case hwmon_temp_max_alarm:
+ err = regmap_read(regmap, TMP464_THERM_STATUS_REG, ®val);
+ if (err < 0)
+ break;
+ *val = !!(regval & BIT(channel + 7));
+ break;
+ case hwmon_temp_crit_alarm:
+ err = regmap_read(regmap, TMP464_THERM2_STATUS_REG, ®val);
+ if (err < 0)
+ break;
+ *val = !!(regval & BIT(channel + 7));
+ break;
+ case hwmon_temp_fault:
+ err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, ®val);
+ if (err < 0)
+ break;
+ *val = !!(regval & BIT(channel + 7));
+ break;
+ case hwmon_temp_max_hyst:
+ mutex_lock(&data->update_lock);
+ err = regmap_read(regmap, TMP464_THERM_LIMIT[channel], ®val);
+ if (err < 0)
+ goto unlock_max;
+ err = regmap_read(regmap, TMP464_TEMP_HYST_REG, ®val2);
+ if (err < 0)
+ goto unlock_max;
+ regval -= regval2;
+ *val = temp_from_reg(regval);
+unlock_max:
+ mutex_unlock(&data->update_lock);
+ break;
+ case hwmon_temp_max:
+ err = regmap_read(regmap, TMP464_THERM_LIMIT[channel], ®val);
+ if (err < 0)
+ break;
+ *val = temp_from_reg(regval);
+ break;
+ case hwmon_temp_crit_hyst:
+ mutex_lock(&data->update_lock);
+ err = regmap_read(regmap, TMP464_THERM2_LIMIT[channel], ®val);
+ if (err < 0)
+ goto unlock_crit;
+ err = regmap_read(regmap, TMP464_TEMP_HYST_REG, ®val2);
+ if (err < 0)
+ goto unlock_crit;
+ regval -= regval2;
+ *val = temp_from_reg(regval);
+unlock_crit:
+ mutex_unlock(&data->update_lock);
+ break;
+ case hwmon_temp_crit:
+ err = regmap_read(regmap, TMP464_THERM2_LIMIT[channel], ®val);
+ if (err < 0)
+ break;
+ *val = temp_from_reg(regval);
+ break;
+ case hwmon_temp_offset:
+ err = regmap_read(regmap, TMP464_TEMP_OFFSET_REG(channel), ®val);
+ if (err < 0)
+ break;
+ *val = temp_from_reg(regval);
+ break;
+ case hwmon_temp_input:
+ if (!data->channel[channel].enabled)
+ return -ENODATA;
+ err = regmap_read(regmap, TMP464_TEMP_REG(channel), ®val);
+ if (err < 0)
+ break;
+ *val = temp_from_reg(regval);
+ break;
+ case hwmon_temp_enable:
+ *val = data->channel[channel].enabled;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ return err;
+}
+
+static int tmp464_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ switch (type) {
+ case hwmon_chip:
+ return tmp464_chip_read(dev, attr, channel, val);
+ case hwmon_temp:
+ return tmp464_temp_read(dev, attr, channel, val);
+ 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_set_convrate(struct regmap *regmap, long interval)
+{
+ int rate;
+
+ /*
+ * For valid rates, interval in milli-seconds can be calculated as
+ * interval = 125 << (7 - rate);
+ * or
+ * interval = (1 << (7 - rate)) * 125;
+ * The rate is therefore
+ * rate = 7 - __fls(interval / 125);
+ * and the rounded rate is
+ * rate = 7 - __fls(interval * 4 / (125 * 3));
+ * Use clamp_val() to avoid overflows, and to ensure valid input
+ * for __fls.
+ */
+ interval = clamp_val(interval, 125, 16000);
+ rate = 7 - __fls(interval * 4 / (125 * 3));
+
+ return regmap_update_bits(regmap, TMP464_CONFIG_REG,
+ TMP464_CONFIG_CONVERSION_RATE_MASK,
+ rate << TMP464_CONFIG_CONVERSION_RATE_B0);
+}
+
+static int tmp464_chip_write(struct device *dev, u32 attr, int channel, long val)
+{
+ struct tmp464_data *data = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ return tmp464_set_convrate(data->regmap, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int tmp464_temp_write(struct device *dev, u32 attr, int channel, long val)
+{
+ struct tmp464_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
+ unsigned int regval;
+ int err = 0;
+
+ switch (attr) {
+ case hwmon_temp_max_hyst:
+ mutex_lock(&data->update_lock);
+ err = regmap_read(regmap, TMP464_THERM_LIMIT[0], ®val);
+ if (err < 0)
+ goto unlock;
+ val = clamp_val(val, -256000, 256000); /* prevent overflow/underflow */
+ val = clamp_val(temp_from_reg(regval) - val, 0, 255000);
+ err = regmap_write(regmap, TMP464_TEMP_HYST_REG,
+ DIV_ROUND_CLOSEST(val, 1000) << 7);
+unlock:
+ mutex_unlock(&data->update_lock);
+ break;
+ case hwmon_temp_max:
+ val = temp_to_limit_reg(clamp_val(val, -255000, 255500));
+ err = regmap_write(regmap, TMP464_THERM_LIMIT[channel], val);
+ break;
+ case hwmon_temp_crit:
+ val = temp_to_limit_reg(clamp_val(val, -255000, 255500));
+ err = regmap_write(regmap, TMP464_THERM2_LIMIT[channel], val);
+ break;
+ case hwmon_temp_offset:
+ val = temp_to_offset_reg(clamp_val(val, -128000, 127937));
+ err = regmap_write(regmap, TMP464_TEMP_OFFSET_REG(channel), val);
+ break;
+ case hwmon_temp_enable:
+ mutex_lock(&data->update_lock);
+ data->channel[channel].enabled = !!val;
+ err = tmp464_enable_channels(data);
+ mutex_unlock(&data->update_lock);
+ break;
+ default:
+ err = -EOPNOTSUPP;
+ break;
+ }
+
+ return err;
+}
+
+static int tmp464_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ switch (type) {
+ case hwmon_chip:
+ return tmp464_chip_write(dev, attr, channel, val);
+ case hwmon_temp:
+ return tmp464_temp_write(dev, attr, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t tmp464_is_visible(const void *_data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ const struct tmp464_data *data = _data;
+
+ if (channel >= data->channels)
+ return 0;
+
+ if (type == hwmon_chip) {
+ if (attr == hwmon_chip_update_interval)
+ return 0644;
+ return 0;
+ }
+
+ switch (attr) {
+ case hwmon_temp_input:
+ case hwmon_temp_max_alarm:
+ case hwmon_temp_crit_alarm:
+ case hwmon_temp_crit_hyst:
+ return 0444;
+ case hwmon_temp_enable:
+ case hwmon_temp_max:
+ case hwmon_temp_crit:
+ return 0644;
+ case hwmon_temp_max_hyst:
+ if (!channel)
+ return 0644;
+ return 0444;
+ case hwmon_temp_label:
+ if (data->channel[channel].label)
+ return 0444;
+ return 0;
+ case hwmon_temp_fault:
+ if (channel)
+ return 0444;
+ return 0;
+ case hwmon_temp_offset:
+ if (channel)
+ return 0644;
+ return 0;
+ default:
+ return 0;
+ }
+}
+
+static void tmp464_restore_lock(void *regmap)
+{
+ regmap_write(regmap, TMP464_LOCK_REG, TMP464_LOCK_VAL);
+}
+
+static void tmp464_restore_config(void *_data)
+{
+ struct tmp464_data *data = _data;
+
+ regmap_write(data->regmap, TMP464_CONFIG_REG, data->config_orig);
+}
+
+static int tmp464_init_client(struct device *dev, struct tmp464_data *data)
+{
+ struct regmap *regmap = data->regmap;
+ unsigned int regval;
+ int err;
+
+ err = regmap_read(regmap, TMP464_LOCK_REG, ®val);
+ if (err)
+ return err;
+ if (regval == TMP464_LOCKED) {
+ /* Explicitly unlock chip if it is locked */
+ err = regmap_write(regmap, TMP464_LOCK_REG, TMP464_UNLOCK_VAL);
+ if (err)
+ return err;
+ /* and lock it again when unloading the driver */
+ err = devm_add_action_or_reset(dev, tmp464_restore_lock, regmap);
+ if (err)
+ return err;
+ }
+
+ err = regmap_read(regmap, TMP464_CONFIG_REG, ®val);
+ if (err)
+ return err;
+ data->config_orig = regval;
+ err = devm_add_action_or_reset(dev, tmp464_restore_config, data);
+ if (err)
+ return err;
+
+ /* Default to 500 ms update interval */
+ err = regmap_update_bits(regmap, TMP464_CONFIG_REG,
+ TMP464_CONFIG_CONVERSION_RATE_MASK | TMP464_CONFIG_SHUTDOWN,
+ BIT(TMP464_CONFIG_CONVERSION_RATE_B0) |
+ BIT(TMP464_CONFIG_CONVERSION_RATE_B2));
+ if (err)
+ return err;
+
+ return tmp464_enable_channels(data);
+}
+
+static int tmp464_detect(struct i2c_client *client,
+ struct i2c_board_info *info)
+{
+ struct i2c_adapter *adapter = client->adapter;
+ char *name, *chip;
+ 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_THERM_STATUS_REG);
+ if (reg < 0)
+ return reg;
+ if (reg & 0x1f)
+ return -ENODEV;
+ reg = i2c_smbus_read_word_swapped(client, TMP464_THERM2_STATUS_REG);
+ 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:
+ name = "tmp464";
+ chip = "TMP464";
+ break;
+ case TMP468_DEVICE_ID:
+ name = "tmp468";
+ chip = "TMP468";
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ strscpy(info->type, name, I2C_NAME_SIZE);
+ dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n", chip, client->addr);
+
+ return 0;
+}
+
+static int tmp464_probe_child_from_dt(struct device *dev,
+ struct device_node *child,
+ struct tmp464_data *data)
+
+{
+ struct regmap *regmap = data->regmap;
+ u32 channel;
+ u8 nfactor;
+ int err;
+
+ err = of_property_read_u32(child, "reg", &channel);
+ if (err) {
+ dev_err(dev, "missing reg property of %pOFn\n", child);
+ return err;
+ }
+
+ if (channel >= data->channels) {
+ dev_err(dev, "invalid reg %d of %pOFn\n", channel, child);
+ return -EINVAL;
+ }
+
+ of_property_read_string(child, "label", &data->channel[channel].label);
+
+ data->channel[channel].enabled = of_device_is_available(child);
+
+ err = of_property_read_u8(child, "ti,n-factor", &nfactor);
+ if (err && err != -EINVAL)
+ return err;
+ if (!err) {
+ if (channel == 0) {
+ dev_err(dev, "n-factor can't be set for internal channel\n");
+ return -EINVAL;
+ }
+ err = regmap_write(regmap, TMP464_N_FACTOR_REG(channel), nfactor << 8);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static int tmp464_probe_from_dt(struct device *dev, struct tmp464_data *data)
+{
+ 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(dev, 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 const struct hwmon_channel_info *tmp464_info[] = {
+ HWMON_CHANNEL_INFO(chip,
+ HWMON_C_UPDATE_INTERVAL),
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST | HWMON_T_CRIT |
+ HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+ HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+ HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+ HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+ HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+ HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+ HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+ HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+ HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+ HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+ HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL),
+ NULL
+};
+
+static const struct hwmon_chip_info tmp464_chip_info = {
+ .ops = &tmp464_ops,
+ .info = tmp464_info,
+};
+
+/* regmap */
+
+static bool tmp464_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+ return (reg < TMP464_TEMP_REG(TMP468_NUM_CHANNELS) ||
+ reg == TMP464_THERM_STATUS_REG ||
+ reg == TMP464_THERM2_STATUS_REG ||
+ reg == TMP464_REMOTE_OPEN_REG);
+}
+
+static const struct regmap_config tmp464_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 16,
+ .max_register = TMP464_DEVICE_ID_REG,
+ .volatile_reg = tmp464_is_volatile_reg,
+ .val_format_endian = REGMAP_ENDIAN_BIG,
+ .cache_type = REGCACHE_RBTREE,
+ .use_single_read = true,
+ .use_single_write = true,
+};
+
+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 (dev->of_node)
+ data->channels = (int)(unsigned long)of_device_get_match_data(&client->dev);
+ else
+ data->channels = i2c_match_id(tmp464_id, client)->driver_data;
+
+ data->regmap = devm_regmap_init_i2c(client, &tmp464_regmap_config);
+ if (IS_ERR(data->regmap))
+ return PTR_ERR(data->regmap);
+
+ for (i = 0; i < data->channels; i++)
+ data->channel[i].enabled = true;
+
+ err = tmp464_init_client(dev, data);
+ if (err)
+ return err;
+
+ if (dev->of_node) {
+ err = tmp464_probe_from_dt(dev, data);
+ if (err)
+ return err;
+ }
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+ data, &tmp464_chip_info, 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 <[email protected]>");
+MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor driver");
+MODULE_LICENSE("GPL");
--
2.35.1
Dnia Fri, Feb 18, 2022 at 07:09:07AM -0800, Guenter Roeck napisał(a):
>From: Agathe Porte <[email protected]>
>
>Add basic description of the tmp464 driver DT bindings.
>
>Signed-off-by: Agathe Porte <[email protected]>
>Cc: Krzysztof Adamski <[email protected]>
>Signed-off-by: Guenter Roeck <[email protected]>
>---
>v5:
>- Dropped ti,n-factor from channel@0 example. Added additional
> channel to examples to show positive ti,n-factor property.
>
>v4:
>- No changes
>
>v3:
>- Addedd support for TMP468.
>- Changed number of channels from 0..3 (which was wrong anyway) to 0..8.
>- Changed value range for ti,n-factor to int8, with an example for
> a negative value.
>- Added myself as driver maintainer.
>
> .../devicetree/bindings/hwmon/ti,tmp464.yaml | 114 ++++++++++++++++++
> MAINTAINERS | 7 ++
> 2 files changed, 121 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..14f6a3412b8c
>--- /dev/null
>+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
>@@ -0,0 +1,114 @@
>+# 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 and TMP468 temperature sensors
>+
>+maintainers:
>+ - Agathe Porte <[email protected]>
>+
>+description: |
>+ ±0.0625°C Remote and Local temperature sensor
>+ https://www.ti.com/lit/ds/symlink/tmp464.pdf
>+ https://www.ti.com/lit/ds/symlink/tmp468.pdf
>+
>+properties:
>+ compatible:
>+ enum:
>+ - ti,tmp464
>+ - ti,tmp468
>+
>+ reg:
>+ maxItems: 1
>+
>+ '#address-cells':
>+ const: 1
>+
>+ '#size-cells':
>+ const: 0
>+
>+required:
>+ - compatible
>+ - reg
>+
>+additionalProperties: false
>+
>+patternProperties:
>+ "^channel@([0-8])$":
>+ type: object
>+ description: |
>+ Represents channels of the device and their specific configuration.
>+
>+ properties:
>+ reg:
>+ description: |
>+ The channel number. 0 is local channel, 1-8 are remote channels.
>+ items:
>+ minimum: 0
>+ maximum: 8
>+
>+ 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/int8
>+ items:
>+ minimum: -128
>+ maximum: 127
I still thing we should have the same format here and in tmp421, for
consistency. If use the same property name, "ti,n-factor" but on tmp421
you have use 32bit value while here you have to use 8bit (which is weird
in DT, BTW), it might be confusing.
Back when we did this for TMP421, there was some discussion and we
settled on this approach, why do it differently now?
Krzysztof
Dnia Mon, Feb 21, 2022 at 08:16:15AM -0800, Guenter Roeck napisał(a):
>>I still thing we should have the same format here and in tmp421, for
>>consistency. If use the same property name, "ti,n-factor" but on tmp421
>>you have use 32bit value while here you have to use 8bit (which is weird
>>in DT, BTW), it might be confusing.
>>Back when we did this for TMP421, there was some discussion and we
>>settled on this approach, why do it differently now?
>>
>
>I seem to recall from that discussion that there was supposedly no way to
>express negative numbers in devicetree. Obviously that is incorrect.
Well, I would still argue it *is* correct. DT only support unsigned
numbers and, really, only 32 or 64 bit. See the chapter 2.2.4 Properties
in:
https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4-rc1/devicetree-specification-v0.4-rc1.pdf
Devicetree also supports array of bytes, and this is how we get the
/bits/ magic but this is just a syntactic suggar. The same is true about
negative values. Just decompile your compiled DTB and you will see.
To put it in other words - DTS does support negative values, DTB don't.j
>In addition to that, I strongly suspect that the tmp421 code as written
>does not work. Its value range is specified as 0..255, but it is read with
> err = of_property_read_s32(child, "ti,n-factor", &val);
>and range checked with
> if (val > 127 || val < -128) {
> dev_err(dev, "n-factor for channel %d invalid (%d)\n",
> i, val);
> return -EINVAL;
> }
>
>That just looks wrong. Either the value range is 0..255 and checked
>as 0 .. 255, or it is -128 .. 127 and must be both checked and specified
>accordingly. This made me look into the code and I found how negative
>numbers are supposed to be handled.
It worked for me when I tested that. I could redo the test, if you are
unsure. The code also looks good to me. I wasn't convinced for this
format in yaml but after the whole discussion we had, we settled on
that, with Robs blessing :)
Here's the actual discussion where all this was considered:
https://patchwork.kernel.org/project/linux-hwmon/patch/3ff7b4cc57dab2073fa091072366c1e524631729.1632984254.git.krzysztof.adamski@nokia.com/
I'm not saying the way we do this for tmp421 is better or even right,
all I say is that it would make sense to be consistent and not redo this
discussion every time we have this problem.
Krzysztof
On 2/21/22 13:24, Krzysztof Adamski wrote:
> Dnia Mon, Feb 21, 2022 at 08:16:15AM -0800, Guenter Roeck napisał(a):
>>> I still thing we should have the same format here and in tmp421, for
>>> consistency. If use the same property name, "ti,n-factor" but on tmp421
>>> you have use 32bit value while here you have to use 8bit (which is weird
>>> in DT, BTW), it might be confusing.
>>> Back when we did this for TMP421, there was some discussion and we
>>> settled on this approach, why do it differently now?
>>>
>>
>> I seem to recall from that discussion that there was supposedly no way to
>> express negative numbers in devicetree. Obviously that is incorrect.
>
> Well, I would still argue it *is* correct. DT only support unsigned
> numbers and, really, only 32 or 64 bit. See the chapter 2.2.4 Properties
> in:
> https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4-rc1/devicetree-specification-v0.4-rc1.pdf
>
> Devicetree also supports array of bytes, and this is how we get the
> /bits/ magic but this is just a syntactic suggar. The same is true about
> negative values. Just decompile your compiled DTB and you will see.
> To put it in other words - DTS does support negative values, DTB don't.j
>
>> In addition to that, I strongly suspect that the tmp421 code as written
>> does not work. Its value range is specified as 0..255, but it is read with
>> err = of_property_read_s32(child, "ti,n-factor", &val);
>> and range checked with
>> if (val > 127 || val < -128) {
>> dev_err(dev, "n-factor for channel %d invalid (%d)\n",
>> i, val);
>> return -EINVAL;
>> }
>>
>> That just looks wrong. Either the value range is 0..255 and checked
>> as 0 .. 255, or it is -128 .. 127 and must be both checked and specified
>> accordingly. This made me look into the code and I found how negative
>> numbers are supposed to be handled.
>
> It worked for me when I tested that. I could redo the test, if you are
> unsure. The code also looks good to me. I wasn't convinced for this
> format in yaml but after the whole discussion we had, we settled on
> that, with Robs blessing :)
>
It is hard for me to believe that you can enter, say, 255 into the dts file
and of_property_read_s32() reads it as -1. If so, of_property_read_s32()
could never support values of 128 and above, which seems unlikely.
Now, I can imagine that one can enter 0xffffffff and of_property_read_s32()
returns -1, but that isn't what is documented for tmp421.
Guenter
> Here's the actual discussion where all this was considered:
> https://patchwork.kernel.org/project/linux-hwmon/patch/3ff7b4cc57dab2073fa091072366c1e524631729.1632984254.git.krzysztof.adamski@nokia.com/
>
> I'm not saying the way we do this for tmp421 is better or even right,
> all I say is that it would make sense to be consistent and not redo this
> discussion every time we have this problem.
>
> Krzysztof
On 2/21/22 01:07, Krzysztof Adamski wrote:
> Dnia Fri, Feb 18, 2022 at 07:09:07AM -0800, Guenter Roeck napisał(a):
>> From: Agathe Porte <[email protected]>
>>
>> Add basic description of the tmp464 driver DT bindings.
>>
>> Signed-off-by: Agathe Porte <[email protected]>
>> Cc: Krzysztof Adamski <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> v5:
>> - Dropped ti,n-factor from channel@0 example. Added additional
>> channel to examples to show positive ti,n-factor property.
>>
>> v4:
>> - No changes
>>
>> v3:
>> - Addedd support for TMP468.
>> - Changed number of channels from 0..3 (which was wrong anyway) to 0..8.
>> - Changed value range for ti,n-factor to int8, with an example for
>> a negative value.
>> - Added myself as driver maintainer.
>>
>> .../devicetree/bindings/hwmon/ti,tmp464.yaml | 114 ++++++++++++++++++
>> MAINTAINERS | 7 ++
>> 2 files changed, 121 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..14f6a3412b8c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
>> @@ -0,0 +1,114 @@
>> +# 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 and TMP468 temperature sensors
>> +
>> +maintainers:
>> + - Agathe Porte <[email protected]>
>> +
>> +description: |
>> + ±0.0625°C Remote and Local temperature sensor
>> + https://www.ti.com/lit/ds/symlink/tmp464.pdf
>> + https://www.ti.com/lit/ds/symlink/tmp468.pdf
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - ti,tmp464
>> + - ti,tmp468
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 0
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +patternProperties:
>> + "^channel@([0-8])$":
>> + type: object
>> + description: |
>> + Represents channels of the device and their specific configuration.
>> +
>> + properties:
>> + reg:
>> + description: |
>> + The channel number. 0 is local channel, 1-8 are remote channels.
>> + items:
>> + minimum: 0
>> + maximum: 8
>> +
>> + 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/int8
>> + items:
>> + minimum: -128
>> + maximum: 127
>
> I still thing we should have the same format here and in tmp421, for
> consistency. If use the same property name, "ti,n-factor" but on tmp421
> you have use 32bit value while here you have to use 8bit (which is weird
> in DT, BTW), it might be confusing.
> Back when we did this for TMP421, there was some discussion and we
> settled on this approach, why do it differently now?
>
I seem to recall from that discussion that there was supposedly no way to
express negative numbers in devicetree. Obviously that is incorrect.
In addition to that, I strongly suspect that the tmp421 code as written
does not work. Its value range is specified as 0..255, but it is read with
err = of_property_read_s32(child, "ti,n-factor", &val);
and range checked with
if (val > 127 || val < -128) {
dev_err(dev, "n-factor for channel %d invalid (%d)\n",
i, val);
return -EINVAL;
}
That just looks wrong. Either the value range is 0..255 and checked
as 0 .. 255, or it is -128 .. 127 and must be both checked and specified
accordingly. This made me look into the code and I found how negative
numbers are supposed to be handled.
We can go either way, but whatever it is should be correct and be confirmed
to work. Rob, any thoughts ?
Guenter
Dnia Mon, Feb 21, 2022 at 02:11:17PM -0800, Guenter Roeck napisał(a):
>On 2/21/22 13:24, Krzysztof Adamski wrote:
>>Dnia Mon, Feb 21, 2022 at 08:16:15AM -0800, Guenter Roeck napisał(a):
>>>>I still thing we should have the same format here and in tmp421, for
>>>>consistency. If use the same property name, "ti,n-factor" but on tmp421
>>>>you have use 32bit value while here you have to use 8bit (which is weird
>>>>in DT, BTW), it might be confusing.
>>>>Back when we did this for TMP421, there was some discussion and we
>>>>settled on this approach, why do it differently now?
>>>>
>>>
>>>I seem to recall from that discussion that there was supposedly no way to
>>>express negative numbers in devicetree. Obviously that is incorrect.
>>
>>Well, I would still argue it *is* correct. DT only support unsigned
>>numbers and, really, only 32 or 64 bit. See the chapter 2.2.4 Properties
>>in:
>>https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4-rc1/devicetree-specification-v0.4-rc1.pdf
>>
>>Devicetree also supports array of bytes, and this is how we get the
>>/bits/ magic but this is just a syntactic suggar. The same is true about
>>negative values. Just decompile your compiled DTB and you will see.
>>To put it in other words - DTS does support negative values, DTB don't.j
>>
>>>In addition to that, I strongly suspect that the tmp421 code as written
>>>does not work. Its value range is specified as 0..255, but it is read with
>>> err = of_property_read_s32(child, "ti,n-factor", &val);
>>>and range checked with
>>> if (val > 127 || val < -128) {
>>> dev_err(dev, "n-factor for channel %d invalid (%d)\n",
>>> i, val);
>>> return -EINVAL;
>>> }
>>>
>>>That just looks wrong. Either the value range is 0..255 and checked
>>>as 0 .. 255, or it is -128 .. 127 and must be both checked and specified
>>>accordingly. This made me look into the code and I found how negative
>>>numbers are supposed to be handled.
>>
>>It worked for me when I tested that. I could redo the test, if you are
>>unsure. The code also looks good to me. I wasn't convinced for this
>>format in yaml but after the whole discussion we had, we settled on
>>that, with Robs blessing :)
>>
>
>It is hard for me to believe that you can enter, say, 255 into the dts file
>and of_property_read_s32() reads it as -1. If so, of_property_read_s32()
>could never support values of 128 and above, which seems unlikely.
>
>Now, I can imagine that one can enter 0xffffffff and of_property_read_s32()
>returns -1, but that isn't what is documented for tmp421.
>
Yes, you are correct, you have to enter either <(-1)> or <0xffffffff>
(which is the same thing). I was quite confused on how to specify that
in DT bindings and apparently maybe we did not understand each other
well enough back when the patch was submitted. The code and the
description is correct, though, so the question is how to properly set
"minimum" and "maximum" value.
I think I should at least update the example of tmp421 in the binding to
use <(-1)> notation to make it obvious that it works this way. But I
guess we need Rob to help us understand how this should be specified.
In any case, if you drop usage of /bits 8/ and keep the property naitive
32 bit, both tmp421 and tmp464 bindings will be compatible with each
other.
@Rob, if I want a property ti,n-factor be in in range from <(-128)> to
<127>, so that we can use of_property_read_s32() and then use just one
byte of that, how to specify that in yaml file? For TMP421, we currently
have:
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
which is confusing.
Guenter is proposing to use
$ref: /schemas/types.yaml#/definitions/int8
items:
minimum: -128
maximum: 127
and of_property_read_u8() for the same property on another driver, so
usage of /bits/ 8 is required. Which approach is better in your opinion?
Krzysztof
Krzysztof
On Tue, Feb 22, 2022 at 08:23:35AM +0100, Krzysztof Adamski wrote:
> Dnia Mon, Feb 21, 2022 at 02:11:17PM -0800, Guenter Roeck napisał(a):
> > On 2/21/22 13:24, Krzysztof Adamski wrote:
> > > Dnia Mon, Feb 21, 2022 at 08:16:15AM -0800, Guenter Roeck napisał(a):
> > > > > I still thing we should have the same format here and in tmp421, for
> > > > > consistency. If use the same property name, "ti,n-factor" but on tmp421
> > > > > you have use 32bit value while here you have to use 8bit (which is weird
> > > > > in DT, BTW), it might be confusing.
> > > > > Back when we did this for TMP421, there was some discussion and we
> > > > > settled on this approach, why do it differently now?
> > > > >
> > > >
> > > > I seem to recall from that discussion that there was supposedly no way to
> > > > express negative numbers in devicetree. Obviously that is incorrect.
> > >
> > > Well, I would still argue it *is* correct. DT only support unsigned
> > > numbers and, really, only 32 or 64 bit. See the chapter 2.2.4 Properties
> > > in:
> > > https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4-rc1/devicetree-specification-v0.4-rc1.pdf
> > >
> > > Devicetree also supports array of bytes, and this is how we get the
> > > /bits/ magic but this is just a syntactic suggar. The same is true about
> > > negative values. Just decompile your compiled DTB and you will see.
> > > To put it in other words - DTS does support negative values, DTB don't.j
> > >
> > > > In addition to that, I strongly suspect that the tmp421 code as written
> > > > does not work. Its value range is specified as 0..255, but it is read with
> > > > err = of_property_read_s32(child, "ti,n-factor", &val);
> > > > and range checked with
> > > > if (val > 127 || val < -128) {
> > > > dev_err(dev, "n-factor for channel %d invalid (%d)\n",
> > > > i, val);
> > > > return -EINVAL;
> > > > }
> > > >
> > > > That just looks wrong. Either the value range is 0..255 and checked
> > > > as 0 .. 255, or it is -128 .. 127 and must be both checked and specified
> > > > accordingly. This made me look into the code and I found how negative
> > > > numbers are supposed to be handled.
> > >
> > > It worked for me when I tested that. I could redo the test, if you are
> > > unsure. The code also looks good to me. I wasn't convinced for this
> > > format in yaml but after the whole discussion we had, we settled on
> > > that, with Robs blessing :)
> > >
> >
> > It is hard for me to believe that you can enter, say, 255 into the dts file
> > and of_property_read_s32() reads it as -1. If so, of_property_read_s32()
> > could never support values of 128 and above, which seems unlikely.
> >
> > Now, I can imagine that one can enter 0xffffffff and of_property_read_s32()
> > returns -1, but that isn't what is documented for tmp421.
> >
>
> Yes, you are correct, you have to enter either <(-1)> or <0xffffffff>
> (which is the same thing). I was quite confused on how to specify that
> in DT bindings and apparently maybe we did not understand each other
> well enough back when the patch was submitted. The code and the
> description is correct, though, so the question is how to properly set
Here is where we disagree. The bindings say:
items:
minimum: 0
maximum: 255
Based on this, the following devicetree configuration should be correct.
tmp423a@4c {
compatible = "ti,tmp423";
reg = <0x4c>;
#address-cells = <1>;
#size-cells = <0>;
channel@1 {
reg = <1>;
ti,n-factor = <255>;
};
};
However, it results in:
tmp421 4-004c: n-factor for channel 1 invalid (255)
tmp421: probe of 4-004c failed with error -22
Either the range is 0 ... 255 and we need to accept 0 ... 255,
or the range is -128 ... 127 and we need to accept -128 ... 127.
> "minimum" and "maximum" value.
>
> I think I should at least update the example of tmp421 in the binding to
> use <(-1)> notation to make it obvious that it works this way. But I
> guess we need Rob to help us understand how this should be specified.
>
> In any case, if you drop usage of /bits 8/ and keep the property naitive
> 32 bit, both tmp421 and tmp464 bindings will be compatible with each
> other.
>
> @Rob, if I want a property ti,n-factor be in in range from <(-128)> to
> <127>, so that we can use of_property_read_s32() and then use just one
> byte of that, how to specify that in yaml file? For TMP421, we currently
> have:
>
> 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
>
> which is confusing.
>
> Guenter is proposing to use
> $ref: /schemas/types.yaml#/definitions/int8
> items:
> minimum: -128
> maximum: 127
>
> and of_property_read_u8() for the same property on another driver, so
> usage of /bits/ 8 is required. Which approach is better in your opinion?
>
I could declare the property as int32, use of_property_read_s32, and
validate the range in the driver. However, the range still needs
to match the documentation.
Guenter