* [PATCH v3 0/2] add support for MCP998X
@ 2025-06-13 13:02 victor.duicu
2025-06-13 13:02 ` [PATCH v3 1/2] dt-bindings: iio: temperature: " victor.duicu
2025-06-13 13:02 ` [PATCH v3 2/2] " victor.duicu
0 siblings, 2 replies; 14+ messages in thread
From: victor.duicu @ 2025-06-13 13:02 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt
Cc: marius.cristea, victor.duicu, linux-iio, linux-kernel, devicetree
From: Victor Duicu <victor.duicu@microchip.com>
Add support for Microchip MCP998X/33 and MCP998XD/33D
Multichannel Automotive Temperature Monitor Family.
The chips in the family have different numbers of external
channels, ranging from 1 (MCP9982) to 4 channels (MCP9985).
Reading diodes in anti-parallel connection is supported
by MCP9984/85/33 and MCP9984D/85D/33D.
Dedicated hardware shutdown circuitry is present only
in MCP998XD and MCP9933D.
Current version of driver does not support interrupts, events and data
buffering.
Differences related to previous patch:
v3:
- move beta parameters to devicetree.
- change the name of the interrupts and add
check to match them to the device in yaml.
- remove label for device and remove "0x" from
channel registers in example in yaml.
- edit comments in yaml and driver.
- add minItems to interrupts in yaml.
- rename microchip,recd12 and microchip,recd34 to
microchip,resistance-comp-ch1-2-enable
and microchip,resistance-comp-ch3-4-enable.
- rename microchip,apdd-state to microchip,enable-anti-parallel.
- add static to mcp9982_3db_values_map_tbl to fix
kernel test robot warning.
- in mcp9982_init() add check to ensure that hardware
shutdown feature can't be overridden.
- replace div_u64_rem with do_div and add
asm/div64.h to includes.
- remove unused includes.
- add iio_chan_spec in the macro definition of MCP9982_CHAN.
- remove MCP9982_EXT_BETA_ENBL.
- in mcp9982_init() replace regmap_assign_bits
with regmap_write when setting beta compensation.
- remove custom attribute enable_extended_temp_range and
map it to IIO_CHAN_INFO_OFFSET.
- add unsigned to int variables that allow it.
- reorder parameters in mcp9982_priv, change some
from int to bool, add const to labels and add dev_name.
- add check for chips with "D" in the name to not
allow sampling frequencies lower than 1 to
prevent overriding of hardware shutdown.
- remove mcp9982_attributes.
- move mcp9982_calc_all_3db_values() to before
mcp9982_init().
- use MICRO instead of number constant.
- in mcp9982_write_raw replace ">=" with "==".
- rename index2 to idx in mcp9982_read_raw().
- remove i2c_set_clientdata() in mcp9982_probe().
- since there are no more custom ABI attributes
the testing file was removed.
v2:
- move hysteresis, extended temperature range and beta parameters
from devicetree into user space.
- edit comments in yaml and driver.
- remove "|" in descpriptions, remove "+" from PatternProperties in yaml.
- add default to microchip,ideality-factor, delete blank lines and wrap to
80 chars in yaml.
- remove variables with upper case.
- add check for microchip,apdd-state and microchip,recd34 in yaml.
- improve coding style in driver code.
- add includes for all functions used.
- rename MCP9982_INT_HIGH_BYTE_ADDR to MCP9982_INT_VALUE_ADDR and
MCP9982_INT_LOW_BYTE_ADDR to MCP9982_FRAC_VALUE_ADDR.
- remove custom attribute running_average_window and
running_average_window_available and map them to a low pass filter.
- update sysfs-bus-iio-temperature-mcp9982 to reflect current
driver attributes and point to next kernel version (6.17).
- use compound literal to define driver channels.
- replace device_property_read_string() with i2c_get_match_data() to read
chip name from devicetree.
- remove MCP9982_DEV_ATTR and mcp9982_prep_custom_attributes().
- remove client, chip_name, iio_info from mcp9982_priv.
- replace sprintf() with sysfs_emit().
- remove error messages which are triggered by keyboard input.
- replace devm_kzalloc() with devm_kcalloc(), array mcp9982_chip_config[] with
individual structures, device_property_present() with device_property_read_bool().
- reordered parameters in mcp9982_features and mcp9982_priv to optimize memory
allocation.
- remove .endianness from channel properties.
- change name of some parameters in mcp9982_priv.
- add check for reg value 0 from devicetree (channel 0 is for internal temperature
and can't be disabled).
v1:
- inital version.
Victor Duicu (2):
dt-bindings: iio: temperature: add support for MCP998X
iio: temperature: add support for MCP998X
.../iio/temperature/microchip,mcp9982.yaml | 211 +++++
MAINTAINERS | 7 +
drivers/iio/temperature/Kconfig | 10 +
drivers/iio/temperature/Makefile | 1 +
drivers/iio/temperature/mcp9982.c | 778 ++++++++++++++++++
5 files changed, 1007 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
create mode 100644 drivers/iio/temperature/mcp9982.c
base-commit: 0c86e33819785fe50616b6ee3fb35c1e4be406d5
--
2.48.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/2] dt-bindings: iio: temperature: add support for MCP998X
2025-06-13 13:02 [PATCH v3 0/2] add support for MCP998X victor.duicu
@ 2025-06-13 13:02 ` victor.duicu
2025-06-13 14:40 ` Conor Dooley
2025-07-28 13:01 ` Victor.Duicu
2025-06-13 13:02 ` [PATCH v3 2/2] " victor.duicu
1 sibling, 2 replies; 14+ messages in thread
From: victor.duicu @ 2025-06-13 13:02 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt
Cc: marius.cristea, victor.duicu, linux-iio, linux-kernel, devicetree
From: Victor Duicu <victor.duicu@microchip.com>
This is the devicetree schema for Microchip MCP998X/33 and
MCP998XD/33D Automotive Temperature Monitor Family.
Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
---
.../iio/temperature/microchip,mcp9982.yaml | 211 ++++++++++++++++++
1 file changed, 211 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
new file mode 100644
index 000000000000..ec939d463612
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
@@ -0,0 +1,211 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive
+ Temperature Monitor Family
+
+maintainers:
+ - Victor Duicu <victor.duicu@microchip.com>
+
+description: |
+ The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel
+ automotive temperature monitor.
+ The datasheet can be found here:
+ https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
+
+properties:
+ compatible:
+ enum:
+ - microchip,mcp9933
+ - microchip,mcp9933d
+ - microchip,mcp9982
+ - microchip,mcp9982d
+ - microchip,mcp9983
+ - microchip,mcp9983d
+ - microchip,mcp9984
+ - microchip,mcp9984d
+ - microchip,mcp9985
+ - microchip,mcp9985d
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ minItems: 2
+ maxItems: 2
+
+ interrupt-names:
+ description:
+ -alert-therm is used to handle a HIGH or LOW limit.
+ -therm-addr is used to handle a THERM limit on chips
+ without "D" in the name.
+ -sys-shutdown is used to handle a THERM limit on chips
+ with "D" in the name.
+ items:
+ - const: alert-therm
+ - const: therm-addr
+ - const: sys-shutdown
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ microchip,enable-anti-parallel:
+ description:
+ Enable anti-parallel diode mode operation.
+ MCP9984/84D/85/85D and MCP9933/33D support reading two external diodes
+ in anti-parallel connection on the same set of pins.
+ type: boolean
+
+ microchip,beta1:
+ description:
+ Set beta compensation value for external channel 1.
+ <0> 0.050
+ <1> 0.066
+ <2> 0.087
+ <3> 0.114
+ <4> 0.150
+ <5> 0.197
+ <6> 0.260
+ <7> 0.342
+ <8> 0.449
+ <9> 0.591
+ <10> 0.778
+ <11> 1.024
+ <12> 1.348
+ <13> 1.773
+ <14> 2.333
+ <15> Diode_Mode
+ <16> Auto
+ - Diode_Mode is used when measuring a discrete thermal diode
+ or a CPU diode that functions like a discrete thermal diode.
+ - Auto enables beta auto-detection. The chip monitors
+ external diode/transistor and determines the optimum
+ setting.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 16
+
+ microchip,beta2:
+ description:
+ Set beta compensation value for external channel 2.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 16
+
+ microchip,resistance-comp-ch1-2-enable:
+ description:
+ Enable resistance error correction(REC) for external channels 1 and 2.
+ The chip internal hardware counterbalances the parasitic resistance in
+ series with the external diodes. The compensation can be activated or
+ disabled in hardware for both channels 1 and 2 at the same time.
+ type: boolean
+
+ microchip,resistance-comp-ch3-4-enable:
+ description:
+ Enable resistance error correction(REC) for external channels 3 and 4.
+ The chip internal hardware counterbalances the parasitic resistance in
+ series with the external diodes. The compensation can be activated or
+ disabled in hardware for both channels 3 and 4 at the same time.
+ type: boolean
+
+
+ vdd-supply: true
+
+patternProperties:
+ "^channel@[1-4]$":
+ description:
+ Represents the external temperature channels to which
+ a remote diode is connected.
+ type: object
+
+ properties:
+ reg:
+ items:
+ minimum: 1
+ maximum: 4
+
+ microchip,ideality-factor:
+ description:
+ Each channel has an ideality factor.
+ Beta compensation and resistance error correction automatically
+ correct for most ideality errors. So ideality factor does not need
+ to be adjusted in general.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 18
+
+ label:
+ description: Unique name to identify which channel this is.
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - microchip,mcp9982d
+ - microchip,mcp9983d
+ - microchip,mcp9984d
+ - microchip,mcp9985d
+ - microchip,mcp9933d
+ then:
+ properties:
+ interrupts-names:
+ items:
+ - const: alert-therm
+ - const: sys-shutdown
+ else:
+ properties:
+ interrupts-names:
+ items:
+ - const: alert-therm
+ - const: therm-addr
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ temperature-sensor@4c {
+ compatible = "microchip,mcp9985";
+ reg = <0x4c>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ microchip,enable-anti-parallel;
+ microchip,resistance-comp-ch1-2-enable;
+ microchip,resistance-comp-ch3-4-enable;
+ microchip,beta1 = <16>;
+ microchip,beta2 = <16>;
+ vdd-supply = <&vdd>;
+
+ channel@1 {
+ reg = <1>;
+ label = "CPU Temperature";
+ };
+
+ channel@2 {
+ reg = <2>;
+ label = "GPU Temperature";
+ };
+ };
+ };
+
+...
--
2.48.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/2] iio: temperature: add support for MCP998X
2025-06-13 13:02 [PATCH v3 0/2] add support for MCP998X victor.duicu
2025-06-13 13:02 ` [PATCH v3 1/2] dt-bindings: iio: temperature: " victor.duicu
@ 2025-06-13 13:02 ` victor.duicu
2025-06-13 21:50 ` Andy Shevchenko
2025-06-14 13:18 ` Jonathan Cameron
1 sibling, 2 replies; 14+ messages in thread
From: victor.duicu @ 2025-06-13 13:02 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt
Cc: marius.cristea, victor.duicu, linux-iio, linux-kernel, devicetree
From: Victor Duicu <victor.duicu@microchip.com>
This is the driver for Microchip MCP998X/33 and MCP998XD/33D
Multichannel Automotive Temperature Monitor Family.
Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
---
MAINTAINERS | 7 +
drivers/iio/temperature/Kconfig | 10 +
drivers/iio/temperature/Makefile | 1 +
drivers/iio/temperature/mcp9982.c | 778 ++++++++++++++++++++++++++++++
4 files changed, 796 insertions(+)
create mode 100644 drivers/iio/temperature/mcp9982.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 86a2045ba62e..63573c517603 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15887,6 +15887,13 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
F: drivers/iio/adc/mcp3911.c
+MICROCHIP MCP9982 TEMPERATURE DRIVER
+M: Victor Duicu <victor.duicu@microchip.com>
+L: linux-iio@vger.kernel.org
+S: Supported
+F: Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
+F: drivers/iio/temperature/mcp9982.c
+
MICROCHIP MMC/SD/SDIO MCI DRIVER
M: Aubin Constans <aubin.constans@microchip.com>
S: Maintained
diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 1244d8e17d50..e72b49f95f86 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -182,4 +182,14 @@ config MCP9600
This driver can also be built as a module. If so, the module
will be called mcp9600.
+config MCP9982
+ tristate "Microchip Technology MCP9982 driver"
+ depends on I2C
+ help
+ Say yes here to build support for Microchip Technology's MCP998X/33
+ and MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
+
+ This driver can also be built as a module. If so, the module
+ will be called mcp9982.
+
endmenu
diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
index 07d6e65709f7..83f5f4bb4ff3 100644
--- a/drivers/iio/temperature/Makefile
+++ b/drivers/iio/temperature/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_MAX30208) += max30208.o
obj-$(CONFIG_MAX31856) += max31856.o
obj-$(CONFIG_MAX31865) += max31865.o
obj-$(CONFIG_MCP9600) += mcp9600.o
+obj-$(CONFIG_MCP9982) += mcp9982.o
obj-$(CONFIG_MLX90614) += mlx90614.o
obj-$(CONFIG_MLX90632) += mlx90632.o
obj-$(CONFIG_MLX90632) += mlx90635.o
diff --git a/drivers/iio/temperature/mcp9982.c b/drivers/iio/temperature/mcp9982.c
new file mode 100644
index 000000000000..b1ae77c6e691
--- /dev/null
+++ b/drivers/iio/temperature/mcp9982.c
@@ -0,0 +1,778 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IIO driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family
+ *
+ * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Victor Duicu <victor.duicu@microchip.com>
+ *
+ * Datasheet can be found here:
+ * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
+ */
+
+#include <asm/div64.h>
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device/devres.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <linux/units.h>
+
+/* MCP9982 Registers */
+#define MCP9982_INT_VALUE_ADDR(index) (2 * (index))
+#define MCP9982_FRAC_VALUE_ADDR(index) (2 * (index) + 1)
+#define MCP9982_ONE_SHOT_ADDR 0x0A
+#define MCP9982_INTERNAL_HIGH_LIMIT_ADDR 0x0B
+#define MCP9982_INTERNAL_LOW_LIMIT_ADDR 0x0C
+#define MCP9982_EXT1_HIGH_LIMIT_INT_VALUE_ADDR 0x0D
+#define MCP9982_EXT1_HIGH_LIMIT_FRAC_VALUE_ADDR 0x0E
+#define MCP9982_EXT1_LOW_LIMIT_INT_VALUE_ADDR 0x0F
+#define MCP9982_EXT1_LOW_LIMIT_FRAC_VALUE_ADDR 0x10
+#define MCP9982_INTERNAL_THERM_LIMIT_ADDR 0x1D
+#define MCP9982_EXT1_THERM_LIMIT_ADDR 0x1E
+#define MCP9982_CFG_ADDR 0x22
+#define MCP9982_CONV_ADDR 0x24
+#define MCP9982_HYS_ADDR 0x25
+#define MCP9982_CONSEC_ALRT_ADDR 0x26
+#define MCP9982_ALRT_CFG_ADDR 0x27
+#define MCP9982_RUNNING_AVG_ADDR 0x28
+#define MCP9982_HOTTEST_CFG_ADDR 0x29
+#define MCP9982_STATUS_ADDR 0x2A
+#define MCP9982_EXT_FAULT_STATUS_ADDR 0x2B
+#define MCP9982_HIGH_LIMIT_STATUS_ADDR 0x2C
+#define MCP9982_LOW_LIMIT_STATUS_ADDR 0x2D
+#define MCP9982_THERM_LIMIT_STATUS_ADDR 0x2E
+#define MCP9982_HOTTEST_INT_VALUE_ADDR 0x2F
+#define MCP9982_HOTTEST_FRAC_VALUE_ADDR 0x30
+#define MCP9982_HOTTEST_STATUS_ADDR 0x31
+#define MCP9982_THERM_SHTDWN_CFG_ADDR 0x32
+#define MCP9982_HRDW_THERM_SHTDWN_LIMIT_ADDR 0x33
+#define MCP9982_EXT_BETA_CFG_ADDR(index) ((index) + 52)
+#define MCP9982_EXT_IDEAL_ADDR(index) ((index) + 54)
+
+/* MCP9982 Bits */
+#define MCP9982_CFG_MSKAL BIT(7)
+#define MCP9982_CFG_RS BIT(6)
+#define MCP9982_CFG_ATTHM BIT(5)
+#define MCP9982_CFG_RECD12 BIT(4)
+#define MCP9982_CFG_RECD34 BIT(3)
+#define MCP9982_CFG_RANGE BIT(2)
+#define MCP9982_CFG_DA_ENA BIT(1)
+#define MCP9982_CFG_APDD BIT(0)
+
+/* The maximum number of channels a member of the family can have */
+#define MCP9982_MAX_NUM_CHANNELS 5
+
+#define MCP9982_CHAN(index, si, __address) ({ \
+ struct iio_chan_spec __chan = { \
+ .type = IIO_TEMP, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+ BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
+ BIT(IIO_CHAN_INFO_OFFSET), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+ BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
+ BIT(IIO_CHAN_INFO_HYSTERESIS) | \
+ BIT(IIO_CHAN_INFO_OFFSET), \
+ .channel = index, \
+ .address = __address, \
+ .scan_index = si, \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 8, \
+ .storagebits = 8, \
+ }, \
+ .indexed = 1, \
+ }; \
+ __chan; \
+})
+
+/**
+ * struct mcp9982_features - features of a mcp9982 instance
+ * @name: chip's name
+ * @phys_channels: number of physical channels supported by the chip
+ */
+struct mcp9982_features {
+ const char *name;
+ u8 phys_channels;
+};
+
+static const struct mcp9982_features mcp9933_chip_config = {
+ .name = "mcp9933",
+ .phys_channels = 3,
+};
+
+static const struct mcp9982_features mcp9933d_chip_config = {
+ .name = "mcp9933d",
+ .phys_channels = 3,
+};
+
+static const struct mcp9982_features mcp9982_chip_config = {
+ .name = "mcp9982",
+ .phys_channels = 2,
+};
+
+static const struct mcp9982_features mcp9982d_chip_config = {
+ .name = "mcp9982d",
+ .phys_channels = 2,
+};
+
+static const struct mcp9982_features mcp9983_chip_config = {
+ .name = "mcp9983",
+ .phys_channels = 3,
+};
+
+static const struct mcp9982_features mcp9983d_chip_config = {
+ .name = "mcp9983d",
+ .phys_channels = 3,
+};
+
+static const struct mcp9982_features mcp9984_chip_config = {
+ .name = "mcp9984",
+ .phys_channels = 4,
+};
+
+static const struct mcp9982_features mcp9984d_chip_config = {
+ .name = "mcp9984d",
+ .phys_channels = 4,
+};
+
+static const struct mcp9982_features mcp9985_chip_config = {
+ .name = "mcp9985",
+ .phys_channels = 5,
+};
+
+static const struct mcp9982_features mcp9985d_chip_config = {
+ .name = "mcp9985d",
+ .phys_channels = 5,
+};
+
+static const unsigned int mcp9982_fractional_values[] = {
+ 0,
+ 125000,
+ 250000,
+ 375000,
+ 500000,
+ 625000,
+ 750000,
+ 875000,
+};
+
+static const unsigned int mcp9982_conv_rate[][2] = {
+ { 0, 62500 },
+ { 0, 125000 },
+ { 0, 250000 },
+ { 0, 500000 },
+ { 1, 0 },
+ { 2, 0 },
+ { 4, 0 },
+ { 8, 0 },
+ { 16, 0 },
+ { 32, 0 },
+ { 64, 0 },
+};
+
+static unsigned int mcp9982_3db_values_map_tbl[11][3][2];
+static const unsigned int mcp9982_sampl_fr[][2] = {
+ { 1, 16 },
+ { 1, 8 },
+ { 1, 4 },
+ { 1, 2 },
+ { 1, 1 },
+ { 2, 1 },
+ { 4, 1 },
+ { 8, 1 },
+ { 16, 1 },
+ { 32, 1 },
+ { 64, 1 },
+};
+
+static const unsigned int mcp9982_window_size[3] = {1, 4, 8};
+
+/*
+ * (Sampling_Frequency * 1000000) / (Window_Size * 2)
+ */
+static unsigned int mcp9982_calc_all_3db_values(void)
+{
+ u32 denominator, remainder;
+ unsigned int i, j;
+ u64 numerator;
+
+ for (i = 0; i < ARRAY_SIZE(mcp9982_window_size); i++)
+ for (j = 0; j < ARRAY_SIZE(mcp9982_sampl_fr); j++) {
+ numerator = MICRO * mcp9982_sampl_fr[j][0];
+ denominator = 2 * mcp9982_window_size[i] * mcp9982_sampl_fr[j][1];
+ remainder = do_div(numerator, denominator);
+ remainder = do_div(numerator, MICRO);
+ mcp9982_3db_values_map_tbl[j][i][0] = numerator;
+ mcp9982_3db_values_map_tbl[j][i][1] = remainder;
+ }
+ return 0;
+}
+
+/* mcp9982 regmap configuration */
+static const struct regmap_range mcp9982_regmap_wr_ranges[] = {
+ regmap_reg_range(MCP9982_ONE_SHOT_ADDR, MCP9982_EXT1_LOW_LIMIT_FRAC_VALUE_ADDR),
+ regmap_reg_range(MCP9982_INTERNAL_THERM_LIMIT_ADDR, MCP9982_EXT1_THERM_LIMIT_ADDR),
+ regmap_reg_range(MCP9982_CFG_ADDR, MCP9982_CFG_ADDR),
+ regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_HOTTEST_CFG_ADDR),
+ regmap_reg_range(MCP9982_THERM_SHTDWN_CFG_ADDR, MCP9982_THERM_SHTDWN_CFG_ADDR),
+ regmap_reg_range(MCP9982_EXT_BETA_CFG_ADDR(0), MCP9982_EXT_IDEAL_ADDR(3)),
+};
+
+static const struct regmap_access_table mcp9982_regmap_wr_table = {
+ .yes_ranges = mcp9982_regmap_wr_ranges,
+ .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_wr_ranges),
+};
+
+static const struct regmap_range mcp9982_regmap_rd_ranges[] = {
+ regmap_reg_range(MCP9982_INT_VALUE_ADDR(0),
+ MCP9982_EXT1_LOW_LIMIT_FRAC_VALUE_ADDR),
+ regmap_reg_range(MCP9982_INTERNAL_THERM_LIMIT_ADDR, MCP9982_EXT1_THERM_LIMIT_ADDR),
+ regmap_reg_range(MCP9982_CFG_ADDR, MCP9982_CFG_ADDR),
+ regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_EXT_IDEAL_ADDR(3)),
+};
+
+static const struct regmap_access_table mcp9982_regmap_rd_table = {
+ .yes_ranges = mcp9982_regmap_rd_ranges,
+ .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_rd_ranges),
+};
+
+static bool mcp9982_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MCP9982_ONE_SHOT_ADDR:
+ case MCP9982_INTERNAL_HIGH_LIMIT_ADDR:
+ case MCP9982_INTERNAL_LOW_LIMIT_ADDR:
+ case MCP9982_EXT1_HIGH_LIMIT_INT_VALUE_ADDR:
+ case MCP9982_EXT1_HIGH_LIMIT_FRAC_VALUE_ADDR:
+ case MCP9982_EXT1_LOW_LIMIT_INT_VALUE_ADDR:
+ case MCP9982_EXT1_LOW_LIMIT_FRAC_VALUE_ADDR:
+ case MCP9982_INTERNAL_THERM_LIMIT_ADDR:
+ case MCP9982_EXT1_THERM_LIMIT_ADDR:
+ case MCP9982_CFG_ADDR:
+ case MCP9982_CONV_ADDR:
+ case MCP9982_HYS_ADDR:
+ case MCP9982_CONSEC_ALRT_ADDR:
+ case MCP9982_ALRT_CFG_ADDR:
+ case MCP9982_RUNNING_AVG_ADDR:
+ case MCP9982_HOTTEST_CFG_ADDR:
+ case MCP9982_THERM_SHTDWN_CFG_ADDR:
+ return false;
+ default:
+ return true;
+ }
+}
+
+static const struct regmap_config mcp9982_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .rd_table = &mcp9982_regmap_rd_table,
+ .wr_table = &mcp9982_regmap_wr_table,
+ .volatile_reg = mcp9982_is_volatile_reg,
+};
+
+/**
+ * struct mcp9992_priv - information about chip parameters
+ * @regmap: device register map
+ * @num_channels number of physical channels
+ * @extended_temp_range use extended temperature range or not
+ * @recd34_enable state of REC on channels 3 and 4
+ * @recd12_enable state of REC on channels 1 and 2
+ * @beta_values beta compensation value for external channel 1 and 2
+ * @lock synchronize access to driver's state members
+ * @iio_chan specifications of channels
+ * @labels labels of the channels
+ * @ideality_value ideality factor value for each external channel
+ * @sampl_idx index representing the current sampling frequency
+ * @dev_name name of the device
+ * @apdd_enable state of anti-parallel diode mode
+ */
+struct mcp9982_priv {
+ struct regmap *regmap;
+ u8 num_channels;
+ bool extended_temp_range;
+ bool recd34_enable;
+ bool recd12_enable;
+ unsigned int beta_values[2];
+ /*
+ * Synchronize access to private members, and ensure
+ * atomicity of consecutive regmap operations.
+ */
+ struct mutex lock;
+ struct iio_chan_spec *iio_chan;
+ const char *labels[MCP9982_MAX_NUM_CHANNELS];
+ unsigned int ideality_value[4];
+ unsigned int sampl_idx;
+ const char *dev_name;
+ bool apdd_enable;
+};
+
+static int mcp9982_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length, long mask)
+{
+ struct mcp9982_priv *priv = iio_priv(indio_dev);
+ static const int offset[2] = {0, -64};
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ if (strchr(priv->dev_name, 'd')) {
+ *vals = mcp9982_conv_rate[4];
+ *length = (ARRAY_SIZE(mcp9982_conv_rate) - 4) * 2;
+ } else {
+ *vals = mcp9982_conv_rate[0];
+ *length = ARRAY_SIZE(mcp9982_conv_rate) * 2;
+ }
+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ *vals = mcp9982_3db_values_map_tbl[priv->sampl_idx][0];
+ *length = ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]) * 2;
+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_OFFSET:
+ *type = IIO_VAL_INT;
+ *vals = offset;
+ *length = 2;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ unsigned int index, idx, tmp_reg;
+ struct mcp9982_priv *priv = iio_priv(indio_dev);
+ int ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&priv->lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = regmap_read(priv->regmap, MCP9982_INT_VALUE_ADDR(chan->channel), val);
+ if (ret)
+ return ret;
+
+ /* The extended temperature range is offset by 64 degrees C */
+ if (priv->extended_temp_range)
+ *val -= 64;
+
+ ret = regmap_read(priv->regmap, MCP9982_FRAC_VALUE_ADDR(chan->channel), val2);
+ if (ret)
+ return ret;
+
+ /* Only the 3 MSB in fractional registers are used */
+ *val2 = mcp9982_fractional_values[*val2 >> 5];
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = mcp9982_conv_rate[priv->sampl_idx][0];
+ *val2 = mcp9982_conv_rate[priv->sampl_idx][1];
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+
+ ret = regmap_read(priv->regmap, MCP9982_RUNNING_AVG_ADDR, &tmp_reg);
+ if (ret)
+ return ret;
+ /*
+ * In Filter Selection Register values 1 and 2
+ * are mapped to the same setting.
+ */
+ switch (tmp_reg) {
+ case 0:
+ idx = 0;
+ break;
+ case 1:
+ case 2:
+ idx = 1;
+ break;
+ default:
+ idx = 2;
+ break;
+ }
+
+ *val = mcp9982_3db_values_map_tbl[priv->sampl_idx][idx][0];
+ *val2 = mcp9982_3db_values_map_tbl[priv->sampl_idx][idx][1];
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_HYSTERESIS:
+ ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &index);
+ if (ret)
+ return ret;
+
+ *val = index;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_OFFSET:
+ if (priv->extended_temp_range)
+ *val = -64;
+ else
+ *val = 0;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mcp9982_read_label(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ char *label)
+{
+ struct mcp9982_priv *priv = iio_priv(indio_dev);
+
+ if (chan->channel < 0 || chan->channel > 4)
+ return -EINVAL;
+
+ return sysfs_emit(label, "%s\n", priv->labels[chan->channel]);
+}
+
+static int mcp9982_write_raw_get_fmt(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ long info)
+{
+ switch (info) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_HYSTERESIS:
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_OFFSET:
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct mcp9982_priv *priv = iio_priv(indio_dev);
+ int ret;
+ unsigned int i;
+ unsigned int start = 0;
+
+ guard(mutex)(&priv->lock);
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ /*
+ * For MCP998XD and MCP9933D sampling frequency can't
+ * be set lower than 1.
+ */
+ if (strchr(priv->dev_name, 'd'))
+ start = 4;
+ for (i = start; i < ARRAY_SIZE(mcp9982_conv_rate); i++)
+ if (val == mcp9982_conv_rate[i][0] && val2 == mcp9982_conv_rate[i][1])
+ break;
+
+ if (i == ARRAY_SIZE(mcp9982_conv_rate))
+ return -EINVAL;
+
+ ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, i);
+ if (ret)
+ return ret;
+
+ priv->sampl_idx = i;
+ return 0;
+ case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+ for (i = 0; i < ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]); i++)
+ if (val == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][0] &&
+ val2 == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][1])
+ break;
+
+ if (i == ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]))
+ return -EINVAL;
+
+ /*
+ * In mcp9982_3db_values_map_tbl the second index maps:
+ * 0 for filter off
+ * 1 for filter at level 1
+ * 2 for filter at level 2
+ */
+ if (i == 2)
+ i = 3;
+ ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, i);
+
+ return ret;
+ case IIO_CHAN_INFO_HYSTERESIS:
+ if (val < 0 || val > 255)
+ return -EINVAL;
+
+ ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, val);
+ return ret;
+ case IIO_CHAN_INFO_OFFSET:
+ if (val != 0 && val != -64)
+ return -EINVAL;
+ priv->extended_temp_range = !(val == 0);
+ ret = regmap_assign_bits(priv->regmap, MCP9982_CFG_ADDR, MCP9982_CFG_RANGE,
+ priv->extended_temp_range);
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info mcp9982_info = {
+ .read_raw = mcp9982_read_raw,
+ .read_label = mcp9982_read_label,
+ .read_avail = mcp9982_read_avail,
+ .write_raw_get_fmt = mcp9982_write_raw_get_fmt,
+ .write_raw = mcp9982_write_raw,
+};
+
+static int mcp9982_init(struct mcp9982_priv *priv)
+{
+ int ret;
+ unsigned int i;
+ u8 val;
+
+ /*
+ * For chips with "D" in the name
+ * set the below parameters to default to
+ * ensure that hardware shutdown feature
+ * can't be overridden.
+ */
+ if (strchr(priv->dev_name, 'd')) {
+ priv->recd12_enable = true;
+ priv->recd34_enable = true;
+ for (i = 0; i < 2; i++)
+ priv->beta_values[i] = 16;
+ for (i = 0; i < 4; i++)
+ priv->ideality_value[i] = 18;
+ }
+
+ /*
+ * Set default values in registers.
+ * APDD, RECD12 and RECD34 are active on 0.
+ */
+ val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) | FIELD_PREP(MCP9982_CFG_RS, 1) |
+ FIELD_PREP(MCP9982_CFG_ATTHM, 1) |
+ FIELD_PREP(MCP9982_CFG_RECD12, !priv->recd12_enable) |
+ FIELD_PREP(MCP9982_CFG_RECD34, !priv->recd34_enable) |
+ FIELD_PREP(MCP9982_CFG_RANGE, 0) | FIELD_PREP(MCP9982_CFG_DA_ENA, 0) |
+ FIELD_PREP(MCP9982_CFG_APDD, !priv->apdd_enable);
+
+ ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val);
+ if (ret)
+ return ret;
+ priv->extended_temp_range = false;
+
+ ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, 6);
+ if (ret)
+ return ret;
+ priv->sampl_idx = 6;
+
+ ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, 10);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, 112);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0);
+ if (ret)
+ return ret;
+
+ /* Set beta compensation for channels 1 and 2 */
+ for (i = 0; i < 2; i++) {
+ ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
+ priv->beta_values[i]);
+ if (ret)
+ return ret;
+ }
+ /* Set ideality factor for all external channels */
+ for (i = 0; i < 4; i++) {
+ ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
+ priv->ideality_value[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int mcp9982_parse_of_config(struct iio_dev *indio_dev, struct device *dev,
+ int device_nr_channels)
+{
+ unsigned int reg_nr, iio_idx;
+ struct mcp9982_priv *priv = iio_priv(indio_dev);
+
+ priv->apdd_enable = device_property_read_bool(dev, "microchip,enable-anti-parallel");
+
+ priv->recd12_enable = device_property_read_bool(dev,
+ "microchip,resistance-comp-ch1-2-enable");
+
+ priv->recd34_enable = device_property_read_bool(dev,
+ "microchip,resistance-comp-ch3-4-enable");
+
+ priv->beta_values[0] = 16;
+ priv->beta_values[1] = 16;
+ device_property_read_u32(dev, "microchip,beta1", &priv->beta_values[0]);
+ device_property_read_u32(dev, "microchip,beta2", &priv->beta_values[1]);
+ if (priv->beta_values[0] > 16 || priv->beta_values[1] > 16)
+ return -EINVAL;
+
+ priv->num_channels = device_get_child_node_count(dev) + 1;
+
+ if (priv->num_channels > device_nr_channels)
+ return dev_err_probe(dev, -EINVAL, "More channels than the chip supports\n");
+
+ priv->iio_chan = devm_kcalloc(dev, priv->num_channels, sizeof(*priv->iio_chan), GFP_KERNEL);
+ if (!priv->iio_chan)
+ return -ENOMEM;
+
+ priv->iio_chan[0] = MCP9982_CHAN(0, 0, MCP9982_INT_VALUE_ADDR(0));
+
+ priv->labels[0] = "internal diode";
+ iio_idx++;
+ device_for_each_child_node_scoped(dev, child) {
+ fwnode_property_read_u32(child, "reg", ®_nr);
+ if (!reg_nr || reg_nr >= device_nr_channels)
+ return dev_err_probe(dev, -EINVAL,
+ "The index of the channels does not match the chip\n");
+
+ priv->ideality_value[reg_nr - 1] = 18;
+ if (fwnode_property_present(child, "microchip,ideality-factor")) {
+ fwnode_property_read_u32(child, "microchip,ideality-factor",
+ &priv->ideality_value[reg_nr - 1]);
+ if (priv->ideality_value[reg_nr - 1] > 63)
+ return dev_err_probe(dev, -EINVAL,
+ "The ideality value is higher than maximum\n");
+ }
+
+ fwnode_property_read_string(child, "label",
+ &priv->labels[reg_nr]);
+
+ priv->iio_chan[iio_idx++] = MCP9982_CHAN(reg_nr, reg_nr,
+ MCP9982_INT_VALUE_ADDR(reg_nr));
+ }
+
+ return 0;
+}
+
+static int mcp9982_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct mcp9982_priv *priv;
+ struct iio_dev *indio_dev;
+ const struct mcp9982_features *chip;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ priv = iio_priv(indio_dev);
+ priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config);
+ if (IS_ERR(priv->regmap))
+ return dev_err_probe(dev, PTR_ERR(priv->regmap),
+ "Cannot initialize register map\n");
+
+ ret = devm_mutex_init(dev, &priv->lock);
+ if (ret)
+ return ret;
+
+ chip = i2c_get_match_data(client);
+ if (!chip)
+ return -EINVAL;
+ priv->dev_name = chip->name;
+
+ ret = mcp9982_parse_of_config(indio_dev, &client->dev, chip->phys_channels);
+ if (ret)
+ return dev_err_probe(dev, ret, "Parameter parsing error\n");
+
+ mcp9982_calc_all_3db_values();
+ ret = mcp9982_init(priv);
+ if (ret)
+ return dev_err_probe(dev, ret, "Cannot initialize device\n");
+
+ indio_dev->name = chip->name;
+ indio_dev->info = &mcp9982_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = priv->iio_chan;
+ indio_dev->num_channels = priv->num_channels;
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Cannot register IIO device\n");
+
+ return 0;
+}
+
+static const struct i2c_device_id mcp9982_id[] = {
+ { .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9933_chip_config },
+ { .name = "mcp9933d", .driver_data = (kernel_ulong_t)&mcp9933d_chip_config },
+ { .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config },
+ { .name = "mcp9982d", .driver_data = (kernel_ulong_t)&mcp9982d_chip_config },
+ { .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9983_chip_config },
+ { .name = "mcp9983d", .driver_data = (kernel_ulong_t)&mcp9983d_chip_config },
+ { .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9984_chip_config },
+ { .name = "mcp9984d", .driver_data = (kernel_ulong_t)&mcp9984d_chip_config },
+ { .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9985_chip_config },
+ { .name = "mcp9985d", .driver_data = (kernel_ulong_t)&mcp9985d_chip_config },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, mcp9982_id);
+
+static const struct of_device_id mcp9982_of_match[] = {
+ {
+ .compatible = "microchip,mcp9933",
+ .data = &mcp9933_chip_config
+ }, {
+ .compatible = "microchip,mcp9933d",
+ .data = &mcp9933d_chip_config
+ }, {
+ .compatible = "microchip,mcp9982",
+ .data = &mcp9982_chip_config
+ }, {
+ .compatible = "microchip,mcp9982d",
+ .data = &mcp9982d_chip_config
+ }, {
+ .compatible = "microchip,mcp9983",
+ .data = &mcp9983_chip_config
+ }, {
+ .compatible = "microchip,mcp9983d",
+ .data = &mcp9983d_chip_config
+ }, {
+ .compatible = "microchip,mcp9984",
+ .data = &mcp9984_chip_config
+ }, {
+ .compatible = "microchip,mcp9984d",
+ .data = &mcp9984d_chip_config
+ }, {
+ .compatible = "microchip,mcp9985",
+ .data = &mcp9985_chip_config
+ }, {
+ .compatible = "microchip,mcp9985d",
+ .data = &mcp9985d_chip_config
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mcp9982_of_match);
+
+static struct i2c_driver mcp9982_driver = {
+ .driver = {
+ .name = "mcp9982",
+ .of_match_table = mcp9982_of_match,
+ },
+ .probe = mcp9982_probe,
+ .id_table = mcp9982_id,
+};
+module_i2c_driver(mcp9982_driver);
+
+MODULE_AUTHOR("Victor Duicu <victor.duicu@microchip.com>");
+MODULE_DESCRIPTION("MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Driver");
+MODULE_LICENSE("GPL");
--
2.48.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: temperature: add support for MCP998X
2025-06-13 13:02 ` [PATCH v3 1/2] dt-bindings: iio: temperature: " victor.duicu
@ 2025-06-13 14:40 ` Conor Dooley
2025-06-23 13:07 ` Victor.Duicu
2025-07-28 13:01 ` Victor.Duicu
1 sibling, 1 reply; 14+ messages in thread
From: Conor Dooley @ 2025-06-13 14:40 UTC (permalink / raw)
To: victor.duicu
Cc: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt,
marius.cristea, linux-iio, linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 5031 bytes --]
On Fri, Jun 13, 2025 at 04:02:06PM +0300, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the devicetree schema for Microchip MCP998X/33 and
> MCP998XD/33D Automotive Temperature Monitor Family.
>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> ---
> .../iio/temperature/microchip,mcp9982.yaml | 211 ++++++++++++++++++
> 1 file changed, 211 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> new file mode 100644
> index 000000000000..ec939d463612
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> @@ -0,0 +1,211 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive
> + Temperature Monitor Family
> +
> +maintainers:
> + - Victor Duicu <victor.duicu@microchip.com>
> +
> +description: |
> + The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel
> + automotive temperature monitor.
> + The datasheet can be found here:
> + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - microchip,mcp9933
> + - microchip,mcp9933d
> + - microchip,mcp9982
> + - microchip,mcp9982d
> + - microchip,mcp9983
> + - microchip,mcp9983d
> + - microchip,mcp9984
> + - microchip,mcp9984d
> + - microchip,mcp9985
> + - microchip,mcp9985d
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + minItems: 2
> + maxItems: 2
> +
> + interrupt-names:
> + description:
> + -alert-therm is used to handle a HIGH or LOW limit.
> + -therm-addr is used to handle a THERM limit on chips
> + without "D" in the name.
> + -sys-shutdown is used to handle a THERM limit on chips
> + with "D" in the name.
> + items:
> + - const: alert-therm
> + - const: therm-addr
> + - const: sys-shutdown
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + microchip,enable-anti-parallel:
> + description:
> + Enable anti-parallel diode mode operation.
> + MCP9984/84D/85/85D and MCP9933/33D support reading two external diodes
> + in anti-parallel connection on the same set of pins.
> + type: boolean
> +
> + microchip,beta1:
> + description:
> + Set beta compensation value for external channel 1.
> + <0> 0.050
> + <1> 0.066
> + <2> 0.087
> + <3> 0.114
> + <4> 0.150
> + <5> 0.197
> + <6> 0.260
> + <7> 0.342
> + <8> 0.449
> + <9> 0.591
> + <10> 0.778
> + <11> 1.024
> + <12> 1.348
> + <13> 1.773
> + <14> 2.333
> + <15> Diode_Mode
> + <16> Auto
> + - Diode_Mode is used when measuring a discrete thermal diode
> + or a CPU diode that functions like a discrete thermal diode.
> + - Auto enables beta auto-detection. The chip monitors
> + external diode/transistor and determines the optimum
> + setting.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 16
Missing max/min constraints on the property.
> +
> + microchip,beta2:
> + description:
> + Set beta compensation value for external channel 2.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 16
> +
> + microchip,resistance-comp-ch1-2-enable:
> + description:
> + Enable resistance error correction(REC) for external channels 1 and 2.
> + The chip internal hardware counterbalances the parasitic resistance in
> + series with the external diodes. The compensation can be activated or
> + disabled in hardware for both channels 1 and 2 at the same time.
> + type: boolean
On the previous version I objected to this wording for the property,
where it is being used as an enable, and instead said that it should
indicate the presence of the parasitic resistance. Did I miss some sort
of new justification for it still talking about being an enable?
> + microchip,resistance-comp-ch3-4-enable:
> + description:
> + Enable resistance error correction(REC) for external channels 3 and 4.
> + The chip internal hardware counterbalances the parasitic resistance in
> + series with the external diodes. The compensation can be activated or
> + disabled in hardware for both channels 3 and 4 at the same time.
> + type: boolean
Cheers,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] iio: temperature: add support for MCP998X
2025-06-13 13:02 ` [PATCH v3 2/2] " victor.duicu
@ 2025-06-13 21:50 ` Andy Shevchenko
2025-06-19 7:22 ` Victor.Duicu
2025-06-14 13:18 ` Jonathan Cameron
1 sibling, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2025-06-13 21:50 UTC (permalink / raw)
To: victor.duicu
Cc: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt,
marius.cristea, linux-iio, linux-kernel, devicetree
On Fri, Jun 13, 2025 at 4:02 PM <victor.duicu@microchip.com> wrote:
>
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
...
> +MICROCHIP MCP9982 TEMPERATURE DRIVER
> +M: Victor Duicu <victor.duicu@microchip.com>
> +L: linux-iio@vger.kernel.org
> +S: Supported
> +F: Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> +F: drivers/iio/temperature/mcp9982.c
So, with the first patch only the dangling file will be present
without record in MAINTAINERS. Please, make sure that your DT schema
file is in MAINTAINERS.
> +config MCP9982
> + tristate "Microchip Technology MCP9982 driver"
> + depends on I2C
...
> +#include <asm/div64.h>
This needs to be linux/math64.h instead. The rule of thumb: prefer
linux/foo over asm/foo (with some exceptions, that are not the case
here).
...
> +#define MCP9982_INT_VALUE_ADDR(index) (2 * (index))
Maybe also ' + 0'? But I'm fine with this as well.
> +#define MCP9982_FRAC_VALUE_ADDR(index) (2 * (index) + 1)
...
> +#define MCP9982_EXT_IDEAL_ADDR(index) ((index) + 54)
What does 54 mean? What is the magic behind?
...
> +#define MCP9982_CHAN(index, si, __address) ({ \
> + struct iio_chan_spec __chan = { \
Why not compound literal?
> + .type = IIO_TEMP, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
> + BIT(IIO_CHAN_INFO_HYSTERESIS) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .channel = index, \
> + .address = __address, \
> + .scan_index = si, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 8, \
> + .storagebits = 8, \
> + }, \
> + .indexed = 1, \
> + }; \
> + __chan; \
> +})
...
> +static const unsigned int mcp9982_window_size[3] = {1, 4, 8};
Add surrounding spaces inside {}.
...
> +/*
> + * (Sampling_Frequency * 1000000) / (Window_Size * 2)
This comment needs more elaboration, i.e. units in use for frequency
and perhaps window size.
> + */
> +static unsigned int mcp9982_calc_all_3db_values(void)
> +{
> + u32 denominator, remainder;
> + unsigned int i, j;
> + u64 numerator;
> +
> + for (i = 0; i < ARRAY_SIZE(mcp9982_window_size); i++)
> + for (j = 0; j < ARRAY_SIZE(mcp9982_sampl_fr); j++) {
Have you considered making mcp9982_sampl_fr to be struct u64_fract?
Also using here on stack something like
struct u64_fract tmp;
> + numerator = MICRO * mcp9982_sampl_fr[j][0];
> + denominator = 2 * mcp9982_window_size[i] * mcp9982_sampl_fr[j][1];
> + remainder = do_div(numerator, denominator);
> + remainder = do_div(numerator, MICRO);
> + mcp9982_3db_values_map_tbl[j][i][0] = numerator;
> + mcp9982_3db_values_map_tbl[j][i][1] = remainder;
The proposed changes will clarify the meaning of [0] and [1] in such a table.
> + }
> + return 0;
> +}
...
> +struct mcp9982_priv {
> + struct regmap *regmap;
> + u8 num_channels;
> + bool extended_temp_range;
> + bool recd34_enable;
> + bool recd12_enable;
> + unsigned int beta_values[2];
> + /*
> + * Synchronize access to private members, and ensure
> + * atomicity of consecutive regmap operations.
> + */
> + struct mutex lock;
> + struct iio_chan_spec *iio_chan;
> + const char *labels[MCP9982_MAX_NUM_CHANNELS];
> + unsigned int ideality_value[4];
> + unsigned int sampl_idx;
> + const char *dev_name;
> + bool apdd_enable;
Wouldn't it be slightly better to group all booleans (and move u8 here)?
> +};
...
> + if (strchr(priv->dev_name, 'd')) {
> + *vals = mcp9982_conv_rate[4];
> + *length = (ARRAY_SIZE(mcp9982_conv_rate) - 4) * 2;
> + } else {
> + *vals = mcp9982_conv_rate[0];
> + *length = ARRAY_SIZE(mcp9982_conv_rate) * 2;
> + }
So, the length can be multiplied only once here...
...
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = mcp9982_conv_rate[priv->sampl_idx][0];
> + *val2 = mcp9982_conv_rate[priv->sampl_idx][1];
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +
Why this blank line?
> + ret = regmap_read(priv->regmap, MCP9982_RUNNING_AVG_ADDR, &tmp_reg);
> + if (ret)
> + return ret;
> + /*
> + * In Filter Selection Register values 1 and 2
> + * are mapped to the same setting.
> + */
> + switch (tmp_reg) {
> + case 0:
> + idx = 0;
> + break;
> + case 1:
> + case 2:
> + idx = 1;
> + break;
Instead of comment this can be regrouped like
case 0:
case 1:
idx = tmp_reg;
break;
case 2:
idx = 1;
break;
default:
...
> + default:
> + idx = 2;
> + break;
> + }
> +
> + *val = mcp9982_3db_values_map_tbl[priv->sampl_idx][idx][0];
> + *val2 = mcp9982_3db_values_map_tbl[priv->sampl_idx][idx][1];
> + return IIO_VAL_INT_PLUS_MICRO;
...
> +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + int ret;
> + unsigned int i;
> + unsigned int start = 0;
> +
> + guard(mutex)(&priv->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + /*
> + * For MCP998XD and MCP9933D sampling frequency can't
> + * be set lower than 1.
> + */
> + if (strchr(priv->dev_name, 'd'))
Why not simply have this in an additional field of chip_info structure?
> + start = 4;
> + for (i = start; i < ARRAY_SIZE(mcp9982_conv_rate); i++)
> + if (val == mcp9982_conv_rate[i][0] && val2 == mcp9982_conv_rate[i][1])
> + break;
> +
> + if (i == ARRAY_SIZE(mcp9982_conv_rate))
> + return -EINVAL;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, i);
> + if (ret)
> + return ret;
> +
> + priv->sampl_idx = i;
> + return 0;
> + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> + for (i = 0; i < ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]); i++)
> + if (val == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][0] &&
> + val2 == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][1])
> + break;
> +
> + if (i == ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]))
> + return -EINVAL;
> +
> + /*
> + * In mcp9982_3db_values_map_tbl the second index maps:
> + * 0 for filter off
> + * 1 for filter at level 1
> + * 2 for filter at level 2
> + */
> + if (i == 2)
> + i = 3;
> + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, i);
> +
> + return ret;
Why not
return regmap_write(...);
?
> + case IIO_CHAN_INFO_HYSTERESIS:
> + if (val < 0 || val > 255)
> + return -EINVAL;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, val);
> + return ret;
Ditto.
> + case IIO_CHAN_INFO_OFFSET:
> + if (val != 0 && val != -64)
> + return -EINVAL;
> + priv->extended_temp_range = !(val == 0);
> + ret = regmap_assign_bits(priv->regmap, MCP9982_CFG_ADDR, MCP9982_CFG_RANGE,
> + priv->extended_temp_range);
> + return ret;
Ditto.
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int mcp9982_init(struct mcp9982_priv *priv)
> +{
> + int ret;
> + unsigned int i;
> + u8 val;
> +
> + /*
> + * For chips with "D" in the name
> + * set the below parameters to default to
> + * ensure that hardware shutdown feature
> + * can't be overridden.
> + */
> + if (strchr(priv->dev_name, 'd')) {
> + priv->recd12_enable = true;
> + priv->recd34_enable = true;
> + for (i = 0; i < 2; i++)
> + priv->beta_values[i] = 16;
memset32() ?
> + for (i = 0; i < 4; i++)
> + priv->ideality_value[i] = 18;
Ditto.
> + }
> +
> + /*
> + * Set default values in registers.
> + * APDD, RECD12 and RECD34 are active on 0.
> + */
> + val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) | FIELD_PREP(MCP9982_CFG_RS, 1) |
> + FIELD_PREP(MCP9982_CFG_ATTHM, 1) |
> + FIELD_PREP(MCP9982_CFG_RECD12, !priv->recd12_enable) |
> + FIELD_PREP(MCP9982_CFG_RECD34, !priv->recd34_enable) |
> + FIELD_PREP(MCP9982_CFG_RANGE, 0) | FIELD_PREP(MCP9982_CFG_DA_ENA, 0) |
> + FIELD_PREP(MCP9982_CFG_APDD, !priv->apdd_enable);
> +
> + ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val);
> + if (ret)
> + return ret;
> + priv->extended_temp_range = false;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, 6);
> + if (ret)
> + return ret;
> + priv->sampl_idx = 6;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, 10);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, 112);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + /* Set beta compensation for channels 1 and 2 */
> + for (i = 0; i < 2; i++) {
ARRAY_SIZE()
> + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
> + priv->beta_values[i]);
> + if (ret)
> + return ret;
> + }
> + /* Set ideality factor for all external channels */
> + for (i = 0; i < 4; i++) {
Ditto.
> + ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
> + priv->ideality_value[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
...
> + priv->beta_values[0] = 16;
> + priv->beta_values[1] = 16;
memset32() ?
> + device_property_read_u32(dev, "microchip,beta1", &priv->beta_values[0]);
> + device_property_read_u32(dev, "microchip,beta2", &priv->beta_values[1]);
> + if (priv->beta_values[0] > 16 || priv->beta_values[1] > 16)
> + return -EINVAL;
...
> + if (priv->num_channels > device_nr_channels)
> + return dev_err_probe(dev, -EINVAL, "More channels than the chip supports\n");
Hmm... Perhaps -E2BIG?
...
> + priv->labels[0] = "internal diode";
> + iio_idx++;
> + device_for_each_child_node_scoped(dev, child) {
> + fwnode_property_read_u32(child, "reg", ®_nr);
> + if (!reg_nr || reg_nr >= device_nr_channels)
> + return dev_err_probe(dev, -EINVAL,
> + "The index of the channels does not match the chip\n");
> +
> + priv->ideality_value[reg_nr - 1] = 18;
> + if (fwnode_property_present(child, "microchip,ideality-factor")) {
> + fwnode_property_read_u32(child, "microchip,ideality-factor",
> + &priv->ideality_value[reg_nr - 1]);
> + if (priv->ideality_value[reg_nr - 1] > 63)
> + return dev_err_probe(dev, -EINVAL,
-EOVERFLOW?
> + "The ideality value is higher than maximum\n");
> + }
> +
> + fwnode_property_read_string(child, "label",
> + &priv->labels[reg_nr]);
> +
> + priv->iio_chan[iio_idx++] = MCP9982_CHAN(reg_nr, reg_nr,
> + MCP9982_INT_VALUE_ADDR(reg_nr));
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] iio: temperature: add support for MCP998X
2025-06-13 13:02 ` [PATCH v3 2/2] " victor.duicu
2025-06-13 21:50 ` Andy Shevchenko
@ 2025-06-14 13:18 ` Jonathan Cameron
2025-06-14 13:27 ` Jonathan Cameron
1 sibling, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2025-06-14 13:18 UTC (permalink / raw)
To: victor.duicu
Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, marius.cristea,
linux-iio, linux-kernel, devicetree
On Fri, 13 Jun 2025 16:02:07 +0300
<victor.duicu@microchip.com> wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
Hi Victor,
Some comments from me to add to what Andy called out.
Hopefully I've avoided too much duplication!
Thanks,
Jonathan
> diff --git a/drivers/iio/temperature/mcp9982.c b/drivers/iio/temperature/mcp9982.c
> new file mode 100644
> index 000000000000..b1ae77c6e691
> --- /dev/null
> +++ b/drivers/iio/temperature/mcp9982.c
> @@ -0,0 +1,778 @@
> +/**
> + * struct mcp9982_features - features of a mcp9982 instance
> + * @name: chip's name
> + * @phys_channels: number of physical channels supported by the chip
> + */
> +struct mcp9982_features {
> + const char *name;
> + u8 phys_channels;
As below. Add a few more fields for the stuff you were getting from string
matching.
> +};
> +static const unsigned int mcp9982_window_size[3] = {1, 4, 8};
{ 1, 4, 8, };
is style preference for IIO code and consistent with the
rest of this driver.
> +
> +/*
> + * (Sampling_Frequency * 1000000) / (Window_Size * 2)
> + */
Single line comment syntax
> +static unsigned int mcp9982_calc_all_3db_values(void)
> +{
> + u32 denominator, remainder;
> + unsigned int i, j;
> + u64 numerator;
> +
> + for (i = 0; i < ARRAY_SIZE(mcp9982_window_size); i++)
Coding style is a little ambiguous around what is a single statement vs multi statements
but here I think {} for the outer loop would help readability a tiny bit.
> + for (j = 0; j < ARRAY_SIZE(mcp9982_sampl_fr); j++) {
> + numerator = MICRO * mcp9982_sampl_fr[j][0];
> + denominator = 2 * mcp9982_window_size[i] * mcp9982_sampl_fr[j][1];
> + remainder = do_div(numerator, denominator);
> + remainder = do_div(numerator, MICRO);
> + mcp9982_3db_values_map_tbl[j][i][0] = numerator;
> + mcp9982_3db_values_map_tbl[j][i][1] = remainder;
> + }
> + return 0;
> +}
> +/**
> + * struct mcp9992_priv - information about chip parameters
> + * @regmap: device register map
> + * @num_channels number of physical channels
> + * @extended_temp_range use extended temperature range or not
> + * @recd34_enable state of REC on channels 3 and 4
> + * @recd12_enable state of REC on channels 1 and 2
> + * @beta_values beta compensation value for external channel 1 and 2
> + * @lock synchronize access to driver's state members
> + * @iio_chan specifications of channels
> + * @labels labels of the channels
> + * @ideality_value ideality factor value for each external channel
> + * @sampl_idx index representing the current sampling frequency
> + * @dev_name name of the device
> + * @apdd_enable state of anti-parallel diode mode
> + */
> +struct mcp9982_priv {
> + struct regmap *regmap;
> + u8 num_channels;
> + bool extended_temp_range;
> + bool recd34_enable;
> + bool recd12_enable;
> + unsigned int beta_values[2];
> + /*
> + * Synchronize access to private members, and ensure
> + * atomicity of consecutive regmap operations.
wrap at 80
> + */
> + struct mutex lock;
> + struct iio_chan_spec *iio_chan;
> + const char *labels[MCP9982_MAX_NUM_CHANNELS];
> + unsigned int ideality_value[4];
> + unsigned int sampl_idx;
> + const char *dev_name;
I'd store a pointer to chip in here rather than just the name.
Particularly as there are other flags to add to it.
> + bool apdd_enable;
> +};
> +static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + unsigned int index, idx, tmp_reg;
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&priv->lock);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = regmap_read(priv->regmap, MCP9982_INT_VALUE_ADDR(chan->channel), val);
> + if (ret)
> + return ret;
> +
> + /* The extended temperature range is offset by 64 degrees C */
> + if (priv->extended_temp_range)
> + *val -= 64;
Why is this here and in offset below? Userspace should be doing the maths for us.
> +
> + ret = regmap_read(priv->regmap, MCP9982_FRAC_VALUE_ADDR(chan->channel), val2);
> + if (ret)
> + return ret;
> +
> + /* Only the 3 MSB in fractional registers are used */
> + *val2 = mcp9982_fractional_values[*val2 >> 5];
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = mcp9982_conv_rate[priv->sampl_idx][0];
> + *val2 = mcp9982_conv_rate[priv->sampl_idx][1];
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +
> + ret = regmap_read(priv->regmap, MCP9982_RUNNING_AVG_ADDR, &tmp_reg);
> + if (ret)
> + return ret;
> + /*
> + * In Filter Selection Register values 1 and 2
> + * are mapped to the same setting.
> + */
> + switch (tmp_reg) {
> + case 0:
> + idx = 0;
> + break;
> + case 1:
> + case 2:
> + idx = 1;
> + break;
> + default:
> + idx = 2;
> + break;
> + }
> +
> + *val = mcp9982_3db_values_map_tbl[priv->sampl_idx][idx][0];
> + *val2 = mcp9982_3db_values_map_tbl[priv->sampl_idx][idx][1];
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_HYSTERESIS:
> + ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &index);
> + if (ret)
> + return ret;
> +
> + *val = index;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_OFFSET:
> + if (priv->extended_temp_range)
> + *val = -64;
As above. I'd expect to only see this -64 here - not in _RAW as well.
> + else
> + *val = 0;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + int ret;
> + unsigned int i;
> + unsigned int start = 0;
> +
> + guard(mutex)(&priv->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + /*
> + * For MCP998XD and MCP9933D sampling frequency can't
> + * be set lower than 1.
> + */
> + if (strchr(priv->dev_name, 'd'))
Encode this in the features structure as a specific field.
In general don't use string matching for this stuff as it ends up fragile
when new device support is added over time and people don't notice this
hiding in here.
> + start = 4;
> + for (i = start; i < ARRAY_SIZE(mcp9982_conv_rate); i++)
> + if (val == mcp9982_conv_rate[i][0] && val2 == mcp9982_conv_rate[i][1])
> + break;
> +
> + if (i == ARRAY_SIZE(mcp9982_conv_rate))
> + return -EINVAL;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, i);
> + if (ret)
> + return ret;
> +
> + priv->sampl_idx = i;
> + return 0;
> +static int mcp9982_init(struct mcp9982_priv *priv)
> +{
> + int ret;
> + unsigned int i;
> + u8 val;
> +
> + /*
> + * For chips with "D" in the name
Very short wrap. Aim for 80 chars.
> + * set the below parameters to default to
> + * ensure that hardware shutdown feature
> + * can't be overridden.
> + */
> + if (strchr(priv->dev_name, 'd')) {
As above. Don't match on strings - store the thing as a flag in the per
device type features structure.
> + priv->recd12_enable = true;
> + priv->recd34_enable = true;
> + for (i = 0; i < 2; i++)
> + priv->beta_values[i] = 16;
> + for (i = 0; i < 4; i++)
> + priv->ideality_value[i] = 18;
> + }
> +
> + /*
> + * Set default values in registers.
> + * APDD, RECD12 and RECD34 are active on 0.
> + */
> + val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) | FIELD_PREP(MCP9982_CFG_RS, 1) |
> + FIELD_PREP(MCP9982_CFG_ATTHM, 1) |
> + FIELD_PREP(MCP9982_CFG_RECD12, !priv->recd12_enable) |
> + FIELD_PREP(MCP9982_CFG_RECD34, !priv->recd34_enable) |
> + FIELD_PREP(MCP9982_CFG_RANGE, 0) | FIELD_PREP(MCP9982_CFG_DA_ENA, 0) |
> + FIELD_PREP(MCP9982_CFG_APDD, !priv->apdd_enable);
> +
> + ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val);
> + if (ret)
> + return ret;
> + priv->extended_temp_range = false;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, 6);
> + if (ret)
> + return ret;
> + priv->sampl_idx = 6;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, 10);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, 112);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + /* Set beta compensation for channels 1 and 2 */
> + for (i = 0; i < 2; i++) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
> + priv->beta_values[i]);
> + if (ret)
> + return ret;
> + }
> + /* Set ideality factor for all external channels */
> + for (i = 0; i < 4; i++) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
> + priv->ideality_value[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id mcp9982_id[] = {
> + { .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9933_chip_config },
> + { .name = "mcp9933d", .driver_data = (kernel_ulong_t)&mcp9933d_chip_config },
> + { .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config },
> + { .name = "mcp9982d", .driver_data = (kernel_ulong_t)&mcp9982d_chip_config },
> + { .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9983_chip_config },
> + { .name = "mcp9983d", .driver_data = (kernel_ulong_t)&mcp9983d_chip_config },
> + { .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9984_chip_config },
> + { .name = "mcp9984d", .driver_data = (kernel_ulong_t)&mcp9984d_chip_config },
> + { .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9985_chip_config },
> + { .name = "mcp9985d", .driver_data = (kernel_ulong_t)&mcp9985d_chip_config },
> + { }
David mentioned the other day that there is an effort to remove the need for
the kernel_ulong_t here but it relies on
{ "mcp9985d", &mcp9984d_chip_config },
style entries.
https://lore.kernel.org/all/1c7946f1-d712-4baa-8243-be6a55eec528@baylibre.com/
I wasn't aware of that effort but seems sensible to me!
> +};
> +MODULE_DEVICE_TABLE(i2c, mcp9982_id);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] iio: temperature: add support for MCP998X
2025-06-14 13:18 ` Jonathan Cameron
@ 2025-06-14 13:27 ` Jonathan Cameron
0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2025-06-14 13:27 UTC (permalink / raw)
To: victor.duicu
Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, marius.cristea,
linux-iio, linux-kernel, devicetree
>
> > +
> > +static const struct i2c_device_id mcp9982_id[] = {
> > + { .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9933_chip_config },
> > + { .name = "mcp9933d", .driver_data = (kernel_ulong_t)&mcp9933d_chip_config },
> > + { .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config },
> > + { .name = "mcp9982d", .driver_data = (kernel_ulong_t)&mcp9982d_chip_config },
> > + { .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9983_chip_config },
> > + { .name = "mcp9983d", .driver_data = (kernel_ulong_t)&mcp9983d_chip_config },
> > + { .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9984_chip_config },
> > + { .name = "mcp9984d", .driver_data = (kernel_ulong_t)&mcp9984d_chip_config },
> > + { .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9985_chip_config },
> > + { .name = "mcp9985d", .driver_data = (kernel_ulong_t)&mcp9985d_chip_config },
> > + { }
> David mentioned the other day that there is an effort to remove the need for
> the kernel_ulong_t here but it relies on
> { "mcp9985d", &mcp9984d_chip_config },
> style entries.
Oops. Still need the cast.
>
> https://lore.kernel.org/all/1c7946f1-d712-4baa-8243-be6a55eec528@baylibre.com/
>
> I wasn't aware of that effort but seems sensible to me!
>
>
> > +};
> > +MODULE_DEVICE_TABLE(i2c, mcp9982_id);
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] iio: temperature: add support for MCP998X
2025-06-13 21:50 ` Andy Shevchenko
@ 2025-06-19 7:22 ` Victor.Duicu
2025-06-19 8:29 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Victor.Duicu @ 2025-06-19 7:22 UTC (permalink / raw)
To: andy.shevchenko
Cc: dlechner, nuno.sa, linux-iio, devicetree, robh, jic23, andy,
krzk+dt, linux-kernel, Marius.Cristea, conor+dt
On Sat, 2025-06-14 at 00:50 +0300, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Fri, Jun 13, 2025 at 4:02 PM <victor.duicu@microchip.com> wrote:
> >
> > From: Victor Duicu <victor.duicu@microchip.com>
> >
> > This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> > Multichannel Automotive Temperature Monitor Family.
>
> ...
Hi Andy,
>
> > +MICROCHIP MCP9982 TEMPERATURE DRIVER
> > +M: Victor Duicu <victor.duicu@microchip.com>
> > +L: linux-iio@vger.kernel.org
> > +S: Supported
> > +F:
> > Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982
> > .yaml
> > +F: drivers/iio/temperature/mcp9982.c
>
> So, with the first patch only the dangling file will be present
> without record in MAINTAINERS. Please, make sure that your DT schema
> file is in MAINTAINERS.
>
Are you referring here to the file sysfs-bus-iio-temperature-mcp9982?
This file was in v2 where there were a few custom attributes. In v3
I removed them, so the driver currently doesn't have custom attributes.
Should I had added it to the files in MAINTAINERS?
Isn't the yaml file sufficient to describe the devicetree? Should I
also add a dts file?
...
>
>
> > +#define MCP9982_CHAN(index, si, __address)
> > ({ \
> > + struct iio_chan_spec __chan =
> > { \
>
> Why not compound literal?
>
In v2 I used compound literal, but Jonathan suggested to add
the struct in the macro. After describing the reasoning, we
agreed to code it like this.
[1]:https://lore.kernel.org/linux-iio/20250607182813.64230171@jic23-huawei/
> > + .type =
> > IIO_TEMP, \
> > + .info_mask_separate =
> > BIT(IIO_CHAN_INFO_RAW), \
> > + .info_mask_shared_by_all_available =
> > BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY)
> > | \
> > +
> > BIT(IIO_CHAN_INFO_OFFSET),
> > \
> > + .info_mask_shared_by_all =
> > BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY)
> > | \
> > + BIT(IIO_CHAN_INFO_HYSTERESIS)
> > | \
> > +
> > BIT(IIO_CHAN_INFO_OFFSET),
> > \
> > + .channel =
> > index, \
> > + .address =
> > __address, \
> > + .scan_index =
> > si, \
> > + .scan_type =
> > { \
> > + .sign =
> > 'u', \
> > + .realbits =
> > 8, \
> > + .storagebits =
> > 8, \
> > +
> > },
> > \
> > + .indexed =
> > 1, \
> > +
> > };
> > \
> > +
> > __chan;
> > \
> > +})
>
>
With Best Regards,
Victor Duicu
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] iio: temperature: add support for MCP998X
2025-06-19 7:22 ` Victor.Duicu
@ 2025-06-19 8:29 ` Andy Shevchenko
2025-06-21 17:19 ` Jonathan Cameron
0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2025-06-19 8:29 UTC (permalink / raw)
To: Victor.Duicu
Cc: dlechner, nuno.sa, linux-iio, devicetree, robh, jic23, andy,
krzk+dt, linux-kernel, Marius.Cristea, conor+dt
On Thu, Jun 19, 2025 at 10:22 AM <Victor.Duicu@microchip.com> wrote:
> On Sat, 2025-06-14 at 00:50 +0300, Andy Shevchenko wrote:
> > On Fri, Jun 13, 2025 at 4:02 PM <victor.duicu@microchip.com> wrote:
...
> > > +MICROCHIP MCP9982 TEMPERATURE DRIVER
> > > +M: Victor Duicu <victor.duicu@microchip.com>
> > > +L: linux-iio@vger.kernel.org
> > > +S: Supported
> > > +F:
> > > Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982
> > > .yaml
> > > +F: drivers/iio/temperature/mcp9982.c
> >
> > So, with the first patch only the dangling file will be present
> > without record in MAINTAINERS. Please, make sure that your DT schema
> > file is in MAINTAINERS.
>
> Are you referring here to the file sysfs-bus-iio-temperature-mcp9982?
> This file was in v2 where there were a few custom attributes. In v3
> I removed them, so the driver currently doesn't have custom attributes.
> Should I had added it to the files in MAINTAINERS?
You should have added the file to the MAINTAINERS in the same patch it
appears. Not in some arbitrary change afterwards.
> Isn't the yaml file sufficient to describe the devicetree? Should I
> also add a dts file?
No, this is not the point.
...
> > > +#define MCP9982_CHAN(index, si, __address)
> > > ({ \
> > > + struct iio_chan_spec __chan =
> > > { \
> >
> > Why not compound literal?
> >
> In v2 I used compound literal, but Jonathan suggested to add
> the struct in the macro. After describing the reasoning, we
> agreed to code it like this.
Neither of the versions has a compound literal.
> > > };
> > > \
> > > +
> > > __chan;
> > > \
> > > +})
https://gcc.gnu.org/onlinedocs/gcc/Compound-Literals.html
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] iio: temperature: add support for MCP998X
2025-06-19 8:29 ` Andy Shevchenko
@ 2025-06-21 17:19 ` Jonathan Cameron
2025-06-23 6:45 ` Andy Shevchenko
0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2025-06-21 17:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Victor.Duicu, dlechner, nuno.sa, linux-iio, devicetree, robh,
andy, krzk+dt, linux-kernel, Marius.Cristea, conor+dt
On Thu, 19 Jun 2025 11:29:30 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Jun 19, 2025 at 10:22 AM <Victor.Duicu@microchip.com> wrote:
> > On Sat, 2025-06-14 at 00:50 +0300, Andy Shevchenko wrote:
> > > On Fri, Jun 13, 2025 at 4:02 PM <victor.duicu@microchip.com> wrote:
>
> ...
>
> > > > +MICROCHIP MCP9982 TEMPERATURE DRIVER
> > > > +M: Victor Duicu <victor.duicu@microchip.com>
> > > > +L: linux-iio@vger.kernel.org
> > > > +S: Supported
> > > > +F:
> > > > Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982
> > > > .yaml
> > > > +F: drivers/iio/temperature/mcp9982.c
> > >
> > > So, with the first patch only the dangling file will be present
> > > without record in MAINTAINERS. Please, make sure that your DT schema
> > > file is in MAINTAINERS.
> >
> > Are you referring here to the file sysfs-bus-iio-temperature-mcp9982?
> > This file was in v2 where there were a few custom attributes. In v3
> > I removed them, so the driver currently doesn't have custom attributes.
> > Should I had added it to the files in MAINTAINERS?
>
> You should have added the file to the MAINTAINERS in the same patch it
> appears. Not in some arbitrary change afterwards.
>
Perhaps the confusion here is that Andy is talking about 2 lines above, not
the immediate line above this feedback. So the one with the dt-binding
file. If Victor was reading it as being about the .c file then
this whole cross discussion makes more sense!
Jonathan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/2] iio: temperature: add support for MCP998X
2025-06-21 17:19 ` Jonathan Cameron
@ 2025-06-23 6:45 ` Andy Shevchenko
0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2025-06-23 6:45 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andy Shevchenko, Victor.Duicu, dlechner, nuno.sa, linux-iio,
devicetree, robh, andy, krzk+dt, linux-kernel, Marius.Cristea,
conor+dt
On Sat, Jun 21, 2025 at 06:19:46PM +0100, Jonathan Cameron wrote:
> On Thu, 19 Jun 2025 11:29:30 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jun 19, 2025 at 10:22 AM <Victor.Duicu@microchip.com> wrote:
> > > On Sat, 2025-06-14 at 00:50 +0300, Andy Shevchenko wrote:
> > > > On Fri, Jun 13, 2025 at 4:02 PM <victor.duicu@microchip.com> wrote:
...
> > > > > +MICROCHIP MCP9982 TEMPERATURE DRIVER
> > > > > +M: Victor Duicu <victor.duicu@microchip.com>
> > > > > +L: linux-iio@vger.kernel.org
> > > > > +S: Supported
> > > > > +F:
> > > > > Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982
> > > > > .yaml
> > > > > +F: drivers/iio/temperature/mcp9982.c
> > > >
> > > > So, with the first patch only the dangling file will be present
> > > > without record in MAINTAINERS. Please, make sure that your DT schema
> > > > file is in MAINTAINERS.
> > >
> > > Are you referring here to the file sysfs-bus-iio-temperature-mcp9982?
> > > This file was in v2 where there were a few custom attributes. In v3
> > > I removed them, so the driver currently doesn't have custom attributes.
> > > Should I had added it to the files in MAINTAINERS?
> >
> > You should have added the file to the MAINTAINERS in the same patch it
> > appears. Not in some arbitrary change afterwards.
>
> Perhaps the confusion here is that Andy is talking about 2 lines above, not
> the immediate line above this feedback. So the one with the dt-binding
> file. If Victor was reading it as being about the .c file then
> this whole cross discussion makes more sense!
Yep, sorry for the confusion. My point is that the first patch in the series
added a file, it should start a MAINTAINERS at the same time.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: temperature: add support for MCP998X
2025-06-13 14:40 ` Conor Dooley
@ 2025-06-23 13:07 ` Victor.Duicu
0 siblings, 0 replies; 14+ messages in thread
From: Victor.Duicu @ 2025-06-23 13:07 UTC (permalink / raw)
To: conor
Cc: dlechner, nuno.sa, linux-iio, devicetree, robh, jic23, andy,
krzk+dt, linux-kernel, Marius.Cristea, conor+dt
On Fri, 2025-06-13 at 15:40 +0100, Conor Dooley wrote:
> On Fri, Jun 13, 2025 at 04:02:06PM +0300,
> victor.duicu@microchip.com wrote:
> > From: Victor Duicu <victor.duicu@microchip.com>
> >
Hi Conor,
> > This is the devicetree schema for Microchip MCP998X/33 and
> > MCP998XD/33D Automotive Temperature Monitor Family.
> >
> > Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> > ---
> > .../iio/temperature/microchip,mcp9982.yaml | 211
> > ++++++++++++++++++
> > 1 file changed, 211 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982
> > .yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp99
> > 82.yaml
> > b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp99
> > 82.yaml
> > new file mode 100644
> > index 000000000000..ec939d463612
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp99
> > 82.yaml
> > @@ -0,0 +1,211 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >
> > +
> > + microchip,resistance-comp-ch1-2-enable:
> > + description:
> > + Enable resistance error correction(REC) for external
> > channels 1 and 2.
> > + The chip internal hardware counterbalances the parasitic
> > resistance in
> > + series with the external diodes. The compensation can be
> > activated or
> > + disabled in hardware for both channels 1 and 2 at the same
> > time.
> > + type: boolean
>
> On the previous version I objected to this wording for the property,
> where it is being used as an enable, and instead said that it should
> indicate the presence of the parasitic resistance. Did I miss some
> sort
> of new justification for it still talking about being an enable?
>
My apologies, I forgot to modify the name of the variable.
I will change the variables to something like:
microchip,parasitic-res-on-channel1-2:
description:
Indicates that the chip and the diodes/transistors are sufficiently far
apart that a parasitic resistance is added to the wires, which can
affect the measurements. Due to the anti-parallel diode connections,
channels 1 and 2 are affected together.
>
> > + microchip,resistance-comp-ch3-4-enable:
> > + description:
> > + Enable resistance error correction(REC) for external
> > channels 3 and 4.
> > + The chip internal hardware counterbalances the parasitic
> > resistance in
> > + series with the external diodes. The compensation can be
> > activated or
> > + disabled in hardware for both channels 3 and 4 at the same
> > time.
> > + type: boolean
>
With Kind Regards,
Victor Duicu
> Cheers,
> Conor.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: temperature: add support for MCP998X
2025-06-13 13:02 ` [PATCH v3 1/2] dt-bindings: iio: temperature: " victor.duicu
2025-06-13 14:40 ` Conor Dooley
@ 2025-07-28 13:01 ` Victor.Duicu
2025-07-29 16:27 ` David Lechner
1 sibling, 1 reply; 14+ messages in thread
From: Victor.Duicu @ 2025-07-28 13:01 UTC (permalink / raw)
To: robh, krzk+dt, jic23, nuno.sa, dlechner, conor+dt, andy
Cc: Marius.Cristea, devicetree, linux-kernel, linux-iio
Hi everyone,
I am writing this message to ask your opinions regarding the placement
of temperature range property from the MCP998X/XD family in the
devicetree.
The reason why I am bringing back this topic is due to a limitation of
the chips. When the moving average filter is enabled, the old readings
are kept and new readings are added to the average. This is a problem
when changing the range of temperatures. The raw temperature values
change based on the range so the mixed values will give erroneous
results during averaging.
One possible workaround for this behavior is to set the temperature
range before runtime, to not allow the user to change it.
Initially, in the first patch, I have placed the property
microchip,extended-temp-range in the devicetree.
At that point I mistakenly did not include Conor, Krzysztof and Rob in
the discussion and I would like to ask for comments.
Below is the discussion between me and Jonathan from patch v1:
> > > > +
> > > > + microchip,extended-temp-range:
> > > > + description: |
> > > > + Set the chip to work in the extended temperature range
> > > > -64
> > > > degrees C to 191.875 degrees C.
> > > > + Omit this tag to set the default range 0 degrees C to
> > > > 127.875 degrees C
> > > > + type: boolean
> > >
> > > I'm curious. Why does this belong in the DT binding?
> > >
> >
> > Regarding microchip,extended-temp-range, my perspective is that the
> > user knows beforehand which specific range of temperatures he
> > needs.
> > For example, if the device to be measured is a freezer, the user
> > would
> > be interested in temperatures below 0 degrees C. If we monitor a
> > CPU,
> > the user would be interested in temperatures above 0 degrees C.
>
> Maybe - though also easy to control from userspace by making the
> offset writeable.
Thank you,
Victor Duicu
On Fri, 2025-06-13 at 16:02 +0300, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the devicetree schema for Microchip MCP998X/33 and
> MCP998XD/33D Automotive Temperature Monitor Family.
>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> ---
> .../iio/temperature/microchip,mcp9982.yaml | 211
> ++++++++++++++++++
> 1 file changed, 211 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.y
> aml
>
> diff --git
> a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982
> .yaml
> b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982
> .yaml
> new file mode 100644
> index 000000000000..ec939d463612
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982
> .yaml
> @@ -0,0 +1,211 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id:
> http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive
> + Temperature Monitor Family
> +
> +maintainers:
> + - Victor Duicu <victor.duicu@microchip.com>
> +
> +description: |
> + The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire
> multichannel
> + automotive temperature monitor.
> + The datasheet can be found here:
> +
> https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - microchip,mcp9933
> + - microchip,mcp9933d
> + - microchip,mcp9982
> + - microchip,mcp9982d
> + - microchip,mcp9983
> + - microchip,mcp9983d
> + - microchip,mcp9984
> + - microchip,mcp9984d
> + - microchip,mcp9985
> + - microchip,mcp9985d
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + minItems: 2
> + maxItems: 2
> +
> + interrupt-names:
> + description:
> + -alert-therm is used to handle a HIGH or LOW limit.
> + -therm-addr is used to handle a THERM limit on chips
> + without "D" in the name.
> + -sys-shutdown is used to handle a THERM limit on chips
> + with "D" in the name.
> + items:
> + - const: alert-therm
> + - const: therm-addr
> + - const: sys-shutdown
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + microchip,enable-anti-parallel:
> + description:
> + Enable anti-parallel diode mode operation.
> + MCP9984/84D/85/85D and MCP9933/33D support reading two
> external diodes
> + in anti-parallel connection on the same set of pins.
> + type: boolean
> +
> + microchip,beta1:
> + description:
> + Set beta compensation value for external channel 1.
> + <0> 0.050
> + <1> 0.066
> + <2> 0.087
> + <3> 0.114
> + <4> 0.150
> + <5> 0.197
> + <6> 0.260
> + <7> 0.342
> + <8> 0.449
> + <9> 0.591
> + <10> 0.778
> + <11> 1.024
> + <12> 1.348
> + <13> 1.773
> + <14> 2.333
> + <15> Diode_Mode
> + <16> Auto
> + - Diode_Mode is used when measuring a discrete thermal diode
> + or a CPU diode that functions like a discrete thermal diode.
> + - Auto enables beta auto-detection. The chip monitors
> + external diode/transistor and determines the optimum
> + setting.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 16
> +
> + microchip,beta2:
> + description:
> + Set beta compensation value for external channel 2.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 16
> +
> + microchip,resistance-comp-ch1-2-enable:
> + description:
> + Enable resistance error correction(REC) for external channels
> 1 and 2.
> + The chip internal hardware counterbalances the parasitic
> resistance in
> + series with the external diodes. The compensation can be
> activated or
> + disabled in hardware for both channels 1 and 2 at the same
> time.
> + type: boolean
> +
> + microchip,resistance-comp-ch3-4-enable:
> + description:
> + Enable resistance error correction(REC) for external channels
> 3 and 4.
> + The chip internal hardware counterbalances the parasitic
> resistance in
> + series with the external diodes. The compensation can be
> activated or
> + disabled in hardware for both channels 3 and 4 at the same
> time.
> + type: boolean
> +
> +
> + vdd-supply: true
> +
> +patternProperties:
> + "^channel@[1-4]$":
> + description:
> + Represents the external temperature channels to which
> + a remote diode is connected.
> + type: object
> +
> + properties:
> + reg:
> + items:
> + minimum: 1
> + maximum: 4
> +
> + microchip,ideality-factor:
> + description:
> + Each channel has an ideality factor.
> + Beta compensation and resistance error correction
> automatically
> + correct for most ideality errors. So ideality factor does
> not need
> + to be adjusted in general.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 18
> +
> + label:
> + description: Unique name to identify which channel this is.
> +
> + required:
> + - reg
> +
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - microchip,mcp9982d
> + - microchip,mcp9983d
> + - microchip,mcp9984d
> + - microchip,mcp9985d
> + - microchip,mcp9933d
> + then:
> + properties:
> + interrupts-names:
> + items:
> + - const: alert-therm
> + - const: sys-shutdown
> + else:
> + properties:
> + interrupts-names:
> + items:
> + - const: alert-therm
> + - const: therm-addr
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + temperature-sensor@4c {
> + compatible = "microchip,mcp9985";
> + reg = <0x4c>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + microchip,enable-anti-parallel;
> + microchip,resistance-comp-ch1-2-enable;
> + microchip,resistance-comp-ch3-4-enable;
> + microchip,beta1 = <16>;
> + microchip,beta2 = <16>;
> + vdd-supply = <&vdd>;
> +
> + channel@1 {
> + reg = <1>;
> + label = "CPU Temperature";
> + };
> +
> + channel@2 {
> + reg = <2>;
> + label = "GPU Temperature";
> + };
> + };
> + };
> +
> +...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: temperature: add support for MCP998X
2025-07-28 13:01 ` Victor.Duicu
@ 2025-07-29 16:27 ` David Lechner
0 siblings, 0 replies; 14+ messages in thread
From: David Lechner @ 2025-07-29 16:27 UTC (permalink / raw)
To: Victor.Duicu, robh, krzk+dt, jic23, nuno.sa, conor+dt, andy
Cc: Marius.Cristea, devicetree, linux-kernel, linux-iio
On 7/28/25 8:01 AM, Victor.Duicu@microchip.com wrote:
> Hi everyone,
>
> I am writing this message to ask your opinions regarding the placement
> of temperature range property from the MCP998X/XD family in the
> devicetree.
>
> The reason why I am bringing back this topic is due to a limitation of
> the chips. When the moving average filter is enabled, the old readings
> are kept and new readings are added to the average. This is a problem
> when changing the range of temperatures. The raw temperature values
> change based on the range so the mixed values will give erroneous
> results during averaging.
>
> One possible workaround for this behavior is to set the temperature
> range before runtime, to not allow the user to change it.
It looks like it is just a an average of the last 8 samples at most.
So if there isn't a way to reset the chip memory that holds those 8
samples, we could just read 8 samples and throw away the values before
giving data to userspace any time we start sampling.
Even without changing the temperature range, we would still have old
values and possibly the same issue of stale data possibly influencing
the measurements any time we stop sampling and start again. So I'm not
seeing that this temperature range setting should be a special case.
It still sounds like something better suited to be set at runtime.
>
> Initially, in the first patch, I have placed the property
> microchip,extended-temp-range in the devicetree.
> At that point I mistakenly did not include Conor, Krzysztof and Rob in
> the discussion and I would like to ask for comments.
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-07-29 16:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-13 13:02 [PATCH v3 0/2] add support for MCP998X victor.duicu
2025-06-13 13:02 ` [PATCH v3 1/2] dt-bindings: iio: temperature: " victor.duicu
2025-06-13 14:40 ` Conor Dooley
2025-06-23 13:07 ` Victor.Duicu
2025-07-28 13:01 ` Victor.Duicu
2025-07-29 16:27 ` David Lechner
2025-06-13 13:02 ` [PATCH v3 2/2] " victor.duicu
2025-06-13 21:50 ` Andy Shevchenko
2025-06-19 7:22 ` Victor.Duicu
2025-06-19 8:29 ` Andy Shevchenko
2025-06-21 17:19 ` Jonathan Cameron
2025-06-23 6:45 ` Andy Shevchenko
2025-06-14 13:18 ` Jonathan Cameron
2025-06-14 13:27 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).