[PATCH v3 0/5] iio: adc: add AMD/Xilinx Versal SysMon driver

[PATCH v3 0/5] iio: adc: add AMD/Xilinx Versal SysMon driver

[Salih Erim]

This series adds a new IIO driver for the AMD/Xilinx Versal System
Monitor (SysMon), providing on-chip voltage and temperature monitoring.

The Versal SysMon measures up to 160 supply voltages and reads up to
64 temperature satellites distributed across the SoC. The hardware
also provides aggregated device temperature registers: the current
max and min across all active satellites, and peak/trough values
recorded since last hardware reset. The device can be accessed via
memory-mapped I/O or via an I2C interface.

The driver is split into a bus-agnostic core module using the regmap
API, an MMIO platform driver, and an I2C driver. This allows the
same IIO logic to be shared across different bus transports.

Previous submissions:
  v2: https://lore.kernel.org/all/cover.1746182670.git.salih.erim@amd.com/
  v1: https://lore.kernel.org/all/cover.1757061697.git.michal.simek@amd.com/

Changes in v3:
  - DT binding: single compatible, voltage-channels rename, single
    quotes, drop label/bipolar/xlnx,aie-temp (Krzysztof)
  - Core: IWYU throughout, __free(fwnode_handle), sign_extend32(),
    size_add(), dev_err_probe(), s16 param, remove (int) casts,
    drop SYSMON_MILLI in favor of (int)MILLI, rename _ext to _name
    in SYSMON_CHAN_TEMP macro (Andy, Jonathan)
  - Core: fwnode_irq_get() moved to core_probe, remove sysmon->dev/
    indio_dev/irq from struct, describe protected data in lock
    comment, add RAW+PROCESSED comment (Jonathan)
  - I2C: IWYU, remove wrapper struct, explicit enum values, sizeof()
    for buffers, = { } initializers, adapt to core_probe interface
    change (Andy, Krzysztof)
  - Events: IWYU, FIELD_GET/FIELD_PREP, regmap_set/clear_bits,
    clamp_t, !!, IRQ_RETVAL(), devm_delayed_work_autocancel,
    loop var scope, error checks, remove redundant else, logical
    param splits, spinlock safety comment (Andy)
  - Events: hysteresis rework -- store as millicelsius, hardcode
    ALARM_CONFIG to hysteresis mode, compute lower threshold from
    (upper - hysteresis), remove falling threshold for temperature,
    single event spec per channel with IIO_EV_DIR_RISING, push
    IIO_EV_DIR_RISING for temp and IIO_EV_DIR_EITHER for voltage
    (Jonathan)

Tested on VCK190 (single SLR, MMIO path, 7 supplies, 10 temperature
satellites) and VPK180 (System Controller, I2C path, 7 supplies).

A follow-up series will add thermal zone integration, secure firmware
access, and I2C remote monitoring.

Salih Erim (5):
  dt-bindings: iio: adc: add xlnx,versal-sysmon binding
  iio: adc: add Versal SysMon driver
  iio: adc: versal-sysmon: add I2C driver
  iio: adc: versal-sysmon: add threshold event support
  iio: adc: versal-sysmon: add oversampling support

 .../bindings/iio/adc/xlnx,versal-sysmon.yaml  |  154 +++
 MAINTAINERS                                   |    7 +
 drivers/iio/adc/Kconfig                       |   33 +
 drivers/iio/adc/Makefile                      |    3 +
 drivers/iio/adc/versal-sysmon-core.c          | 1094 +++++++++++++++++
 drivers/iio/adc/versal-sysmon-i2c.c           |  153 +++
 drivers/iio/adc/versal-sysmon.c               |   92 ++
 drivers/iio/adc/versal-sysmon.h               |  125 ++
 8 files changed, 1661 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
 create mode 100644 drivers/iio/adc/versal-sysmon-core.c
 create mode 100644 drivers/iio/adc/versal-sysmon-i2c.c
 create mode 100644 drivers/iio/adc/versal-sysmon.c
 create mode 100644 drivers/iio/adc/versal-sysmon.h

-- 
2.48.1

[PATCH v3 1/5] dt-bindings: iio: adc: add xlnx,versal-sysmon binding

[Salih Erim]

Add devicetree binding for the AMD/Xilinx Versal System Monitor (SysMon).

The Versal SysMon is the successor to the Zynq UltraScale+ AMS block,
providing on-chip voltage and temperature monitoring. The hardware
supports up to 160 supply voltage measurement points and up to 64
temperature satellites distributed across the SoC, with configurable
threshold alarms and oversampling. The device can be accessed via
memory-mapped I/O or via an I2C interface.

Supply and temperature channels are described as child nodes under
container nodes, referencing the standard adc.yaml binding for
channel properties.

Co-developed-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in v3:
  - Use single compatible (xlnx,versal-sysmon only), remove
    xlnx,versal-sysmon-i2c (Krzysztof)
  - Rename supply-channels container to voltage-channels (Krzysztof)
  - Use single quotes in patternProperties regex (Krzysztof)
  - Drop label description from channel properties (Krzysztof)
  - Drop bipolar from channel properties (Krzysztof)
  - Remove xlnx,aie-temp property from binding and example (Krzysztof)

Changes in v2:
  - Restructured to container nodes (supply-channels, temperature-channels)
    with channel@N children referencing adc.yaml
  - Added xlnx,versal-sysmon-i2c compatible
  - Descriptions rewritten to describe hardware only
  - Example simplified to #address-cells = <1>
  - Interrupt example uses GIC_SPI/IRQ_TYPE_LEVEL_HIGH constants
  - Commit description explains hardware context instead of schema layout
  - reg required for both MMIO and I2C, interrupts optional
  - Hex unit-addresses (channel@a not channel@10) per DTSpec
  - patternProperties regex updated to accept hex digits [0-9a-f]
  - Example trimmed to minimal variants (one basic + one bipolar supply,
    one AIE temperature channel)

 .../bindings/iio/adc/xlnx,versal-sysmon.yaml  | 154 ++++++++++++++++++
 1 file changed, 154 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml b/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
new file mode 100644
index 00000000000..1ad58e3d616
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
@@ -0,0 +1,154 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/xlnx,versal-sysmon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD/Xilinx Versal System Monitor
+
+maintainers:
+  - Salih Erim <salih.erim@amd.com>
+
+description:
+  The AMD/Xilinx Versal System Monitor (SysMon) is the successor to the
+  Zynq UltraScale+ AMS block. It provides on-chip voltage and temperature
+  monitoring with up to 160 voltage measurement points and up to
+  64 temperature satellites distributed across the SoC. The hardware
+  supports configurable threshold alarms and oversampling. The device
+  can be accessed via memory-mapped I/O or via an I2C interface.
+
+properties:
+  compatible:
+    const: xlnx,versal-sysmon
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#io-channel-cells':
+    const: 1
+
+  voltage-channels:
+    type: object
+    description:
+      Container for voltage measurement channels.
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      '^channel@([0-9a-f]|[1-9][0-9a-f])$':
+        $ref: adc.yaml
+
+        description:
+          Measures a voltage rail. The register index and rail
+          name are assigned by the hardware design tool (Vivado).
+
+        properties:
+          reg:
+            minimum: 0
+            maximum: 159
+            description:
+              Voltage measurement register index assigned by the hardware
+              design tool.
+
+        required:
+          - reg
+          - label
+
+        unevaluatedProperties: false
+
+    required:
+      - '#address-cells'
+      - '#size-cells'
+
+    additionalProperties: false
+
+  temperature-channels:
+    type: object
+    description:
+      Container for temperature satellite measurement channels.
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      '^channel@([1-9a-f]|[1-3][0-9a-f]|40)$':
+        $ref: adc.yaml
+
+        description:
+          Reads a temperature satellite sensor. Each satellite monitors
+          a specific region of the SoC die.
+
+        properties:
+          reg:
+            minimum: 1
+            maximum: 64
+            description:
+              Temperature satellite number (1-based hardware index).
+
+        required:
+          - reg
+          - label
+
+        unevaluatedProperties: false
+
+    required:
+      - '#address-cells'
+      - '#size-cells'
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    sysmon@f1270000 {
+        compatible = "xlnx,versal-sysmon";
+        reg = <0xf1270000 0x4000>;
+        interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>;
+        #io-channel-cells = <1>;
+
+        voltage-channels {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+                label = "vccaux";
+            };
+
+            channel@3 {
+                reg = <3>;
+                label = "vcc_ram";
+                bipolar;
+            };
+        };
+
+        temperature-channels {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@a {
+                reg = <10>;
+                label = "aie-temp-ch1";
+            };
+        };
+    };
-- 
2.48.1

[PATCH v3 2/5] iio: adc: add Versal SysMon driver

[Salih Erim]

Add the core driver and MMIO platform driver for the AMD/Xilinx Versal
System Monitor (SysMon) block.

The SysMon block resides in the platform management controller (PMC) and
provides on-chip voltage and temperature monitoring through a 10-bit,
200 kSPS ADC. It can monitor up to 160 voltage channels and 64
temperature satellites distributed across the SoC, with a consistent
sample rate of 8 kSPS per channel regardless of how many channels are
enabled.

The driver is split into three compilation units:
  - versal-sysmon-core: Channel parsing, IIO registration, read_raw
  - versal-sysmon: MMIO platform driver with custom regmap accessors

Voltage results are stored in a 19-bit modified floating-point format
and converted to millivolts. Temperature results are stored in Q8.7
signed fixed-point Celsius format and converted to millicelsius.

The MMIO regmap backend uses a custom reg_write accessor that
automatically unlocks the NPI (NoC programming interface) lock
register before each write, as required by the hardware. The regmap
is configured with fast_io since the underlying MMIO accessors are
safe to call from atomic context.

Co-developed-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in v3:
  - IWYU: add array_size.h, string.h, types.h to core; audit and
    fix header and MMIO driver includes (Andy)
  - Rename _ext to _name in SYSMON_CHAN_TEMP macro parameter (Andy,
    Jonathan)
  - Use .info_mask_separate = BIT() style in SYSMON_CHAN_TEMP (Andy)
  - Use s16 parameter in sysmon_q8p7_to_millicelsius (Andy)
  - Use sign_extend32() in sysmon_supply_rawtoprocessed (Andy)
  - Split sysmon_read_raw parameters logically across lines (Andy)
  - Remove redundant (int) casts on regval (Andy)
  - Split num_supply/num_temp initialization (Andy)
  - Use __free(fwnode_handle) cleanup, remove goto err_put (Andy)
  - Use size_add() for overflow-safe allocation (Andy)
  - Use dev_err_probe() in sysmon_parse_fw error paths (Jonathan)
  - Move fwnode_irq_get() to core_probe, remove irq parameter
    from bus driver interfaces (Jonathan)
  - Use (int)MILLI at call sites, drop SYSMON_MILLI define (Andy,
    Jonathan)
  - Remove sysmon->dev, sysmon->indio_dev, sysmon->irq from struct;
    pass as local variables or use regmap_get_device() (Jonathan)
  - Use struct device *dev local in sysmon_platform_probe (Andy)
  - Describe protected data in lock comment (Jonathan)
  - Add comment explaining RAW+PROCESSED co-exposure (Jonathan)

Changes in v2:
  - Split into core (versal-sysmon-core.c) + MMIO platform driver
    (versal-sysmon.c) + shared header (versal-sysmon.h)
  - Uses regmap API instead of direct readl/writel
  - MMIO regmap uses custom callbacks with NPI unlock in write path
  - Reverse Christmas Tree variable ordering throughout
  - Header include order fixed
  - MAINTAINERS entry folded in with wildcard F: pattern
  - Kconfig: hidden VERSAL_SYSMON_CORE + VERSAL_SYSMON selects it
  - Kconfig/Makefile: alphabetical ordering (VERSAL before VF610)
  - Bounds validation on DT reg values
  - Named constants replace magic numbers (SYSMON_REG_STRIDE,
    SYSMON_SUPPLY_MANTISSA_BITS, SYSMON_MILLI)
  - kernel-doc for exported sysmon_core_probe() and sysmon_parse_fw()
  - Supply voltage conversion uses proper two's complement sign
    extension (s16 cast) matching the hardware specification
  - Register offsets sorted by address in header
  - Each patch introduces only the defines, fields, and includes
    it uses (no dead code in any commit)
  - Removed unused linux/limits.h and linux/units.h includes
  - Renamed iio_dev_info to sysmon_iio_info
  - regmap_write return values checked in probe init path

 MAINTAINERS                          |   7 +
 drivers/iio/adc/Kconfig              |  20 ++
 drivers/iio/adc/Makefile             |   2 +
 drivers/iio/adc/versal-sysmon-core.c | 311 +++++++++++++++++++++++++++
 drivers/iio/adc/versal-sysmon.c      |  92 ++++++++
 drivers/iio/adc/versal-sysmon.h      |  64 ++++++
 6 files changed, 496 insertions(+)
 create mode 100644 drivers/iio/adc/versal-sysmon-core.c
 create mode 100644 drivers/iio/adc/versal-sysmon.c
 create mode 100644 drivers/iio/adc/versal-sysmon.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 2fb1c75afd1..46762c8496d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29216,6 +29216,13 @@ F:	Documentation/devicetree/bindings/memory-controllers/xlnx,versal-net-ddrmc5.y
 F:	drivers/edac/versalnet_edac.c
 F:	include/linux/cdx/edac_cdx_pcol.h
 
+XILINX VERSAL SYSMON DRIVER
+M:	Salih Erim <salih.erim@amd.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
+F:	drivers/iio/adc/versal-sysmon*
+
 XILINX WATCHDOG DRIVER
 M:	Srinivas Neeli <srinivas.neeli@amd.com>
 R:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a9dedbb8eb4..c7f19057484 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1943,6 +1943,26 @@ config TWL6030_GPADC
 	  This driver can also be built as a module. If so, the module will be
 	  called twl6030-gpadc.
 
+config VERSAL_SYSMON_CORE
+	tristate
+	select REGMAP
+
+config VERSAL_SYSMON
+	tristate "AMD Versal SysMon driver"
+	depends on ARCH_ZYNQMP || COMPILE_TEST
+	depends on HAS_IOMEM
+	select VERSAL_SYSMON_CORE
+	help
+	  Say yes here to have support for the AMD/Xilinx Versal System
+	  Monitor (SysMon). This driver provides voltage and temperature
+	  monitoring through the IIO subsystem.
+
+	  The SysMon measures up to 160 supply voltages and reads up to
+	  64 temperature satellites distributed across the SoC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called versal-sysmon.
+
 config VF610_ADC
 	tristate "Freescale vf610 ADC driver"
 	depends on HAS_IOMEM
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 097357d146b..d7696b1b157 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -167,6 +167,8 @@ obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
 obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o
 obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
 obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
+obj-$(CONFIG_VERSAL_SYSMON_CORE) += versal-sysmon-core.o
+obj-$(CONFIG_VERSAL_SYSMON) += versal-sysmon.o
 obj-$(CONFIG_VF610_ADC) += vf610_adc.o
 obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
 obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
new file mode 100644
index 00000000000..ebe052f6982
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -0,0 +1,311 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Versal SysMon core driver
+ *
+ * Copyright (C) 2019 - 2022, Xilinx, Inc.
+ * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+
+#include "versal-sysmon.h"
+
+/*
+ * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
+ * (which operate in hardware register format), PROCESSED gives userspace
+ * the converted millivolt or millicelsius value.
+ */
+#define SYSMON_CHAN_TEMP(_chan, _address, _name) {		\
+	.type = IIO_TEMP,					\
+	.indexed = 1,						\
+	.address = _address,					\
+	.channel = _chan,					\
+	.info_mask_separate =					\
+		BIT(IIO_CHAN_INFO_RAW) |				\
+		BIT(IIO_CHAN_INFO_PROCESSED),			\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = 15,					\
+		.storagebits = 16,				\
+		.endianness = IIO_CPU,				\
+	},							\
+	.datasheet_name = _name,				\
+}
+
+/* Static temperature channels (always present) */
+static const struct iio_chan_spec temp_channels[] = {
+	SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
+	SYSMON_CHAN_TEMP(1, SYSMON_TEMP_MIN, "min"),
+	SYSMON_CHAN_TEMP(2, SYSMON_TEMP_MAX_MAX, "max_max"),
+	SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
+};
+
+static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
+{
+	*val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;
+}
+
+static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
+{
+	int mantissa, format, exponent;
+
+	mantissa = FIELD_GET(SYSMON_MANTISSA_MASK, raw_data);
+	exponent = SYSMON_SUPPLY_MANTISSA_BITS - FIELD_GET(SYSMON_MODE_MASK, raw_data);
+	format = FIELD_GET(SYSMON_FMT_MASK, raw_data);
+	/*
+	 * When format bit is set the mantissa is two's complement
+	 * (per hardware spec); sign-extend to int for correct arithmetic.
+	 */
+	if (format)
+		mantissa = sign_extend32(mantissa, 15);
+
+	*val = (mantissa * (int)MILLI) >> exponent;
+}
+
+static int sysmon_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct sysmon *sysmon = iio_priv(indio_dev);
+	unsigned int regval;
+	int ret;
+
+	if (mask != IIO_CHAN_INFO_RAW && mask != IIO_CHAN_INFO_PROCESSED)
+		return -EINVAL;
+
+	guard(mutex)(&sysmon->lock);
+
+	switch (chan->type) {
+	case IIO_TEMP:
+		ret = regmap_read(sysmon->regmap, chan->address, &regval);
+		if (ret)
+			return ret;
+		if (mask == IIO_CHAN_INFO_PROCESSED)
+			sysmon_q8p7_to_millicelsius(regval, val);
+		else
+			*val = regval;
+		return IIO_VAL_INT;
+
+	case IIO_VOLTAGE:
+		ret = regmap_read(sysmon->regmap,
+				  (chan->address * SYSMON_REG_STRIDE) +
+				  SYSMON_SUPPLY_BASE, &regval);
+		if (ret)
+			return ret;
+		if (mask == IIO_CHAN_INFO_PROCESSED)
+			sysmon_supply_rawtoprocessed(regval, val);
+		else
+			*val = regval;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sysmon_read_label(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     char *label)
+{
+	if (chan->datasheet_name)
+		return sysfs_emit(label, "%s\n", chan->datasheet_name);
+
+	return -EINVAL;
+}
+
+static const struct iio_info sysmon_iio_info = {
+	.read_raw = sysmon_read_raw,
+	.read_label = sysmon_read_label,
+};
+
+/**
+ * sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
+ * @indio_dev: IIO device instance
+ * @dev: Parent device
+ *
+ * Reads voltage-channels and temperature-channels container nodes from
+ * firmware and builds the IIO channel array. Static temperature channels
+ * are prepended, followed by supply and satellite channels from DT.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
+{
+	struct fwnode_handle *supply_node __free(fwnode_handle) =
+		device_get_named_child_node(dev, "voltage-channels");
+	struct fwnode_handle *temp_node __free(fwnode_handle) =
+		device_get_named_child_node(dev, "temperature-channels");
+	unsigned int num_supply = 0, num_temp = 0;
+	unsigned int idx, temp_chan_idx, volt_chan_idx;
+	struct iio_chan_spec *sysmon_channels;
+	const char *label;
+	u32 reg;
+	int ret;
+
+	if (supply_node)
+		num_supply = fwnode_get_child_node_count(supply_node);
+	if (temp_node)
+		num_temp = fwnode_get_child_node_count(temp_node);
+
+	sysmon_channels = devm_kcalloc(dev,
+				       size_add(ARRAY_SIZE(temp_channels),
+						num_supply + num_temp),
+				       sizeof(*sysmon_channels), GFP_KERNEL);
+	if (!sysmon_channels)
+		return -ENOMEM;
+
+	/* Static temperature channels first (fixed indices) */
+	idx = 0;
+	memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
+	idx += ARRAY_SIZE(temp_channels);
+
+	/* Supply channels from DT */
+	fwnode_for_each_child_node_scoped(supply_node, child) {
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "missing reg for supply channel\n");
+
+		if (reg > SYSMON_SUPPLY_IDX_MAX)
+			return dev_err_probe(dev, -EINVAL,
+					     "supply reg %u exceeds max %u\n",
+					     reg, SYSMON_SUPPLY_IDX_MAX);
+
+		ret = fwnode_property_read_string(child, "label", &label);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "missing label for supply channel\n");
+
+		sysmon_channels[idx++] = (struct iio_chan_spec) {
+			.type = IIO_VOLTAGE,
+			.indexed = 1,
+			.address = reg,
+			.info_mask_separate =
+				BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_PROCESSED),
+			.scan_type = {
+				.realbits = 19,
+				.storagebits = 32,
+				.endianness = IIO_CPU,
+				.sign = fwnode_property_read_bool(child,
+					"bipolar") ? 's' : 'u',
+			},
+			.datasheet_name = label,
+		};
+	}
+
+	/* Temperature satellite channels from DT */
+	fwnode_for_each_child_node_scoped(temp_node, child) {
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "missing reg for temp channel\n");
+
+		if (reg < 1 || reg > SYSMON_TEMP_SAT_MAX)
+			return dev_err_probe(dev, -EINVAL,
+					     "temp reg %u out of range [1..%u]\n",
+					     reg, SYSMON_TEMP_SAT_MAX);
+
+		ret = fwnode_property_read_string(child, "label", &label);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "missing label for temp channel\n");
+
+		sysmon_channels[idx++] = (struct iio_chan_spec) {
+			.type = IIO_TEMP,
+			.indexed = 1,
+			.address = SYSMON_TEMP_SAT_BASE +
+				   ((reg - 1) * SYSMON_REG_STRIDE),
+			.info_mask_separate =
+				BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_PROCESSED),
+			.scan_type = {
+				.sign = 's',
+				.realbits = 15,
+				.storagebits = 16,
+				.endianness = IIO_CPU,
+			},
+			.datasheet_name = label,
+		};
+	}
+
+	indio_dev->num_channels = idx;
+	indio_dev->info = &sysmon_iio_info;
+
+	/*
+	 * Assign per-type sequential channel numbers.
+	 * IIO sysfs uses type prefix (in_tempN, in_voltageN)
+	 * so numbers only need to be unique within each type.
+	 */
+	temp_chan_idx = 0;
+	volt_chan_idx = 0;
+	for (idx = 0; idx < indio_dev->num_channels; idx++) {
+		if (sysmon_channels[idx].type == IIO_TEMP)
+			sysmon_channels[idx].channel = temp_chan_idx++;
+		else
+			sysmon_channels[idx].channel = volt_chan_idx++;
+	}
+
+	indio_dev->channels = sysmon_channels;
+
+	return 0;
+}
+
+/**
+ * sysmon_core_probe() - Initialize Versal SysMon core
+ * @dev: Parent device
+ * @regmap: Register map for hardware access
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int sysmon_core_probe(struct device *dev, struct regmap *regmap)
+{
+	struct iio_dev *indio_dev;
+	struct sysmon *sysmon;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*sysmon));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	sysmon = iio_priv(indio_dev);
+	sysmon->regmap = regmap;
+
+	ret = devm_mutex_init(dev, &sysmon->lock);
+	if (ret)
+		return ret;
+
+	/* Disable all interrupts and clear pending status */
+	ret = regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
+	if (ret)
+		return ret;
+	ret = regmap_write(sysmon->regmap, SYSMON_ISR, SYSMON_INTR_ALL_MASK);
+	if (ret)
+		return ret;
+
+	indio_dev->name = "versal-sysmon";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = sysmon_parse_fw(indio_dev, dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+EXPORT_SYMBOL_GPL(sysmon_core_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Versal SysMon Core Driver");
+MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");
diff --git a/drivers/iio/adc/versal-sysmon.c b/drivers/iio/adc/versal-sysmon.c
new file mode 100644
index 00000000000..8473288e7db
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Versal SysMon MMIO platform driver
+ *
+ * Copyright (C) 2019 - 2022, Xilinx, Inc.
+ * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "versal-sysmon.h"
+
+struct sysmon_mmio {
+	void __iomem *base;
+};
+
+static int sysmon_mmio_reg_read(void *context, unsigned int reg,
+				unsigned int *val)
+{
+	struct sysmon_mmio *mmio = context;
+
+	*val = readl(mmio->base + reg);
+	return 0;
+}
+
+static int sysmon_mmio_reg_write(void *context, unsigned int reg,
+				 unsigned int val)
+{
+	struct sysmon_mmio *mmio = context;
+
+	/* NPI must be unlocked before any register write except to NPI_LOCK */
+	if (reg != SYSMON_NPI_LOCK)
+		writel(SYSMON_NPI_UNLOCK_CODE, mmio->base + SYSMON_NPI_LOCK);
+	writel(val, mmio->base + reg);
+
+	return 0;
+}
+
+static const struct regmap_config sysmon_mmio_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = SYSMON_REG_STRIDE,
+	.max_register = SYSMON_MAX_REG,
+	.reg_read = sysmon_mmio_reg_read,
+	.reg_write = sysmon_mmio_reg_write,
+	.fast_io = true,
+};
+
+static int sysmon_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sysmon_mmio *mmio;
+	struct regmap *regmap;
+
+	mmio = devm_kzalloc(dev, sizeof(*mmio), GFP_KERNEL);
+	if (!mmio)
+		return -ENOMEM;
+
+	mmio->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mmio->base))
+		return PTR_ERR(mmio->base);
+
+	regmap = devm_regmap_init(dev, NULL, mmio,
+				  &sysmon_mmio_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return sysmon_core_probe(dev, regmap);
+}
+
+static const struct of_device_id sysmon_of_match_table[] = {
+	{ .compatible = "xlnx,versal-sysmon" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sysmon_of_match_table);
+
+static struct platform_driver sysmon_platform_driver = {
+	.probe = sysmon_platform_probe,
+	.driver = {
+		.name = "versal-sysmon",
+		.of_match_table = sysmon_of_match_table,
+	},
+};
+module_platform_driver(sysmon_platform_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Versal SysMon Platform Driver");
+MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");
diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
new file mode 100644
index 00000000000..d24d2481915
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * AMD Versal SysMon driver
+ *
+ * Copyright (C) 2019 - 2022, Xilinx, Inc.
+ * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#ifndef _VERSAL_SYSMON_H_
+#define _VERSAL_SYSMON_H_
+
+#include <linux/bits.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+struct device;
+struct iio_dev;
+struct regmap;
+
+/* Register offsets (sorted by address) */
+#define SYSMON_NPI_LOCK			0x000C
+#define SYSMON_ISR			0x0044
+#define SYSMON_IDR			0x0050
+#define SYSMON_TEMP_MAX			0x1030
+#define SYSMON_TEMP_MIN			0x1034
+#define SYSMON_SUPPLY_BASE		0x1040
+#define SYSMON_TEMP_MIN_MIN		0x1F8C
+#define SYSMON_TEMP_MAX_MAX		0x1F90
+#define SYSMON_TEMP_SAT_BASE		0x1FAC
+#define SYSMON_MAX_REG			0x24C0
+
+/* NPI unlock value written to SYSMON_NPI_LOCK */
+#define SYSMON_NPI_UNLOCK_CODE		0xF9E8D7C6
+
+/* Register stride: 4 bytes per 32-bit register */
+#define SYSMON_REG_STRIDE		4
+
+#define SYSMON_SUPPLY_IDX_MAX		159
+#define SYSMON_TEMP_SAT_MAX		64
+#define SYSMON_INTR_ALL_MASK		GENMASK(31, 0)
+
+/* Supply voltage conversion register fields */
+#define SYSMON_MANTISSA_MASK		GENMASK(15, 0)
+#define SYSMON_FMT_MASK			BIT(16)
+#define SYSMON_MODE_MASK		GENMASK(18, 17)
+
+/* Q8.7 fractional shift */
+#define SYSMON_FRACTIONAL_SHIFT		7U
+#define SYSMON_SUPPLY_MANTISSA_BITS	16
+
+/**
+ * struct sysmon - Driver data for Versal SysMon
+ * @regmap: register map for hardware access
+ * @lock: protects regmap access
+ */
+struct sysmon {
+	struct regmap *regmap;
+	/* Protects regmap access */
+	struct mutex lock;
+};
+
+int sysmon_core_probe(struct device *dev, struct regmap *regmap);
+
+#endif /* _VERSAL_SYSMON_H_ */
-- 
2.48.1

[PATCH v3 3/5] iio: adc: versal-sysmon: add I2C driver

[Salih Erim]

Add an I2C transport driver for the Versal SysMon block. The SysMon
provides an I2C slave interface that allows an external master to
read voltage and temperature measurements through the same register
map used by the MMIO path.

The I2C command frame is an 8-byte structure containing a 4-byte data
payload, a 2-byte register offset, and a 1-byte instruction field.
Read operations send the frame with a read instruction, then receive
a 4-byte response containing the register value.

Events are not supported on the I2C path because there is no
interrupt line and the I2C regmap backend cannot be called from
atomic context.

Co-developed-by: Conall O'Griofa <conall.ogriofa@amd.com>
Signed-off-by: Conall O'Griofa <conall.ogriofa@amd.com>
Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in v3:
  - IWYU: fix includes (Andy)
  - Enum: assign all values explicitly for HW-mapped fields (Andy)
  - Remove sysmon_i2c wrapper struct, pass i2c_client directly
    (Andy)
  - Use sizeof() for I2C buffer lengths instead of defines (Andy)
  - Use = { } instead of = { 0 } for initializers (Andy)
  - Use single compatible xlnx,versal-sysmon (Krzysztof)
  - Adapt to core_probe interface change: irq moved to core,
    remove irq parameter from bus driver (Jonathan)

Changes in v2:
  - New patch (I2C was deferred to Series B in v1)
  - Uses regmap API with custom I2C read/write callbacks
  - Shares core module with MMIO driver via sysmon_core_probe()
  - No event support (I2C has no interrupt line)
  - Separate VERSAL_SYSMON_I2C Kconfig symbol
  - Reverse Christmas Tree variable ordering in read/write functions

 drivers/iio/adc/Kconfig             |  13 +++
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/versal-sysmon-i2c.c | 153 ++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/iio/adc/versal-sysmon-i2c.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index c7f19057484..8f9fc9de74a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1963,6 +1963,19 @@ config VERSAL_SYSMON
 	  To compile this driver as a module, choose M here: the module
 	  will be called versal-sysmon.
 
+config VERSAL_SYSMON_I2C
+	tristate "AMD Versal SysMon I2C driver"
+	depends on I2C
+	select VERSAL_SYSMON_CORE
+	help
+	  Say yes here to have support for the AMD/Xilinx Versal System
+	  Monitor (SysMon) via I2C interface. This driver enables voltage
+	  and temperature monitoring when the Versal chip has SysMon
+	  configured with I2C access.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called versal-sysmon-i2c.
+
 config VF610_ADC
 	tristate "Freescale vf610 ADC driver"
 	depends on HAS_IOMEM
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d7696b1b157..5abb611fe46 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -169,6 +169,7 @@ obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
 obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
 obj-$(CONFIG_VERSAL_SYSMON_CORE) += versal-sysmon-core.o
 obj-$(CONFIG_VERSAL_SYSMON) += versal-sysmon.o
+obj-$(CONFIG_VERSAL_SYSMON_I2C) += versal-sysmon-i2c.o
 obj-$(CONFIG_VF610_ADC) += vf610_adc.o
 obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
 obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
new file mode 100644
index 00000000000..92d149f517e
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon-i2c.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Versal SysMon I2C driver
+ *
+ * Copyright (C) 2023 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "versal-sysmon.h"
+
+#define SYSMON_I2C_INSTR_READ	BIT(2)
+#define SYSMON_I2C_INSTR_WRITE	BIT(3)
+
+#define SYSMON_I2C_DATA0_MASK	GENMASK(7, 0)
+#define SYSMON_I2C_DATA1_MASK	GENMASK(15, 8)
+#define SYSMON_I2C_DATA2_MASK	GENMASK(23, 16)
+#define SYSMON_I2C_DATA3_MASK	GENMASK(31, 24)
+
+#define SYSMON_I2C_OFS_LOW_MASK		GENMASK(9, 2)
+#define SYSMON_I2C_OFS_HIGH_MASK	GENMASK(15, 10)
+
+/* Byte positions within the 8-byte I2C command frame (HW-defined) */
+enum sysmon_i2c_payload_idx {
+	SYSMON_I2C_DATA0_IDX = 0,
+	SYSMON_I2C_DATA1_IDX = 1,
+	SYSMON_I2C_DATA2_IDX = 2,
+	SYSMON_I2C_DATA3_IDX = 3,
+	SYSMON_I2C_OFS_LOW_IDX = 4,
+	SYSMON_I2C_OFS_HIGH_IDX = 5,
+	SYSMON_I2C_INSTR_IDX = 6,
+};
+
+static int sysmon_i2c_reg_read(void *context, unsigned int reg,
+			       unsigned int *val)
+{
+	struct i2c_client *client = context;
+	u8 write_buf[8] = { };
+	u8 read_buf[4];
+	int ret;
+
+	write_buf[SYSMON_I2C_OFS_LOW_IDX] =
+		FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
+	write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
+		FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);
+	write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_READ;
+
+	ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+	if (ret < 0)
+		return ret;
+	if (ret != sizeof(write_buf))
+		return -EIO;
+
+	ret = i2c_master_recv(client, read_buf, sizeof(read_buf));
+	if (ret < 0)
+		return ret;
+	if (ret != sizeof(read_buf))
+		return -EIO;
+
+	*val = FIELD_PREP(SYSMON_I2C_DATA0_MASK,
+			  read_buf[SYSMON_I2C_DATA0_IDX]) |
+	       FIELD_PREP(SYSMON_I2C_DATA1_MASK,
+			  read_buf[SYSMON_I2C_DATA1_IDX]) |
+	       FIELD_PREP(SYSMON_I2C_DATA2_MASK,
+			  read_buf[SYSMON_I2C_DATA2_IDX]) |
+	       FIELD_PREP(SYSMON_I2C_DATA3_MASK,
+			  read_buf[SYSMON_I2C_DATA3_IDX]);
+
+	return 0;
+}
+
+static int sysmon_i2c_reg_write(void *context, unsigned int reg,
+				unsigned int val)
+{
+	struct i2c_client *client = context;
+	u8 write_buf[8] = { };
+	int ret;
+
+	write_buf[SYSMON_I2C_DATA0_IDX] =
+		FIELD_GET(SYSMON_I2C_DATA0_MASK, val);
+	write_buf[SYSMON_I2C_DATA1_IDX] =
+		FIELD_GET(SYSMON_I2C_DATA1_MASK, val);
+	write_buf[SYSMON_I2C_DATA2_IDX] =
+		FIELD_GET(SYSMON_I2C_DATA2_MASK, val);
+	write_buf[SYSMON_I2C_DATA3_IDX] =
+		FIELD_GET(SYSMON_I2C_DATA3_MASK, val);
+	write_buf[SYSMON_I2C_OFS_LOW_IDX] =
+		FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
+	write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
+		FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);
+	write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_WRITE;
+
+	ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+	if (ret < 0)
+		return ret;
+	if (ret != sizeof(write_buf))
+		return -EIO;
+
+	return 0;
+}
+
+static const struct regmap_config sysmon_i2c_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = SYSMON_REG_STRIDE,
+	.max_register = SYSMON_MAX_REG,
+	.reg_read = sysmon_i2c_reg_read,
+	.reg_write = sysmon_i2c_reg_write,
+};
+
+static int sysmon_i2c_probe(struct i2c_client *client)
+{
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init(&client->dev, NULL, client,
+				  &sysmon_i2c_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	/* I2C has no IRQ connection; events are not supported */
+	return sysmon_core_probe(&client->dev, regmap);
+}
+
+static const struct of_device_id sysmon_i2c_of_match_table[] = {
+	{ .compatible = "xlnx,versal-sysmon" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sysmon_i2c_of_match_table);
+
+static const struct i2c_device_id sysmon_i2c_id_table[] = {
+	{ "versal-sysmon" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, sysmon_i2c_id_table);
+
+static struct i2c_driver sysmon_i2c_driver = {
+	.probe = sysmon_i2c_probe,
+	.driver = {
+		.name = "versal-sysmon-i2c",
+		.of_match_table = sysmon_i2c_of_match_table,
+	},
+	.id_table = sysmon_i2c_id_table,
+};
+module_i2c_driver(sysmon_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Versal SysMon I2C Driver");
+MODULE_AUTHOR("Conall O'Griofa <conall.ogriofa@amd.com>");
+MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");
-- 
2.48.1

[PATCH v3 4/5] iio: adc: versal-sysmon: add threshold event support

[Salih Erim]

Add threshold event support for temperature and supply voltage
channels.

Temperature events:
  - Rising threshold with configurable value
  - Over-temperature (OT) alarm with separate threshold
  - Per-channel hysteresis as a millicelsius value
  - Event direction is IIO_EV_DIR_RISING (hysteresis mode)

Supply voltage events:
  - Rising/falling threshold per supply channel
  - Per-channel alarm enable via alarm configuration registers

The hardware supports both window and hysteresis alarm modes for
temperature. This driver uses hysteresis mode, where the upper
threshold triggers the alarm and the lower threshold clears it
(re-arm point). The hardware has a single ISR bit per temperature
channel with no indication of which threshold was crossed, so
hysteresis mode is the natural fit. The lower threshold register
is computed internally as (upper - hysteresis).

Hysteresis is stored in the driver as a millicelsius value,
initialized from the hardware registers at probe. Writing the
rising threshold or hysteresis recomputes the lower register.
ALARM_CONFIG is hard-coded to hysteresis mode during init.

The interrupt handler masks active threshold interrupts (which are
level-sensitive) and schedules a delayed worker to poll for condition
clear before unmasking. When no hardware IRQ is available, event
channels are not created and interrupt init is skipped, since the
I2C regmap backend cannot be called from atomic context.

When disabling a supply channel alarm, the group interrupt remains
active if any other channel in the same alarm group still has an
alarm enabled.

Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in v3:
  - IWYU: add new includes, group iio headers with blank line (Andy)
  - Reduce casts in millicelsius_to_q8p7, consistent style with
    q8p7_to_millicelsius (Andy)
  - Use clamp_t with typed constants, remove tmp & U16_MAX (Andy)
  - Use !! to return 0/1 from read_alarm_config (Andy)
  - Use regmap_set_bits/clear_bits in write_alarm_config (Andy)
  - Add comment explaining spinlock is safe (I2C never reaches
    event code path) (Andy)
  - Add comment explaining IMR negation logic (Andy)
  - Split read_event_value/write_event_value parameters logically
    across lines (Andy)
  - Move mask/shift after regmap_read error check (Andy)
  - Remove redundant else in read_event_value and
    write_event_value (Andy)
  - Use named constant for hysteresis bit, if-else not ternary
    (Andy)
  - Loop variable declared in for() scope (Andy)
  - Add error checks in sysmon_handle_event (Andy)
  - Use IRQ_RETVAL() macro (Andy)
  - Use devm_delayed_work_autocancel instead of manual INIT +
    devm_add_action (Andy)
  - Use FIELD_GET/FIELD_PREP for hysteresis register bits
    (Jonathan)
  - Split OT vs TEMP handling with FIELD_GET (Jonathan)
  - Rework hysteresis: store as millicelsius value, hardcode
    ALARM_CONFIG to hysteresis mode, compute lower threshold
    from (upper - hysteresis), initialize from HW at probe
    (Jonathan)
  - Remove falling threshold for temperature; single event
    spec per channel with IIO_EV_DIR_RISING (Jonathan)
  - Push IIO_EV_DIR_RISING events for temperature,
    IIO_EV_DIR_EITHER for voltage (Jonathan)

Changes in v2:
  - Reverse Christmas Tree variable ordering in all functions
  - Named constants for hysteresis bits: SYSMON_OT_HYST_BIT,
    SYSMON_TEMP_HYST_BIT instead of magic 0x1/0x2
  - SYSMON_ALARM_BITS_PER_REG replaces magic number 32
  - SYSMON_ALARM_OFFSET() helper macro deduplicates alarm register
    offset computation
  - BIT() macro for shift expressions in conversion functions
  - Hysteresis input validated to single-bit range (0 or 1)
  - Event channels only created when irq > 0 (I2C safety)
  - Group alarm interrupt stays active while any channel in the
    group has an alarm enabled
  - write_event_value returns -EINVAL for unhandled types
  - IRQ_NONE returned for spurious interrupts
  - Q8.7 write path uses multiplication instead of left-shift
    to avoid undefined behavior with negative temperatures
  - (u16) mask prevents garbage in reserved register bits
  - regmap_write return values checked for IER/IDR writes
  - devm cleanup ordering: cancel_work before request_irq

 drivers/iio/adc/versal-sysmon-core.c | 655 ++++++++++++++++++++++++++-
 drivers/iio/adc/versal-sysmon.h      |  48 +-
 2 files changed, 697 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
index ebe052f6982..04977c9c887 100644
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -11,6 +11,8 @@
 #include <linux/bitops.h>
 #include <linux/cleanup.h>
 #include <linux/device.h>
+#include <linux/devm-helpers.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
@@ -18,10 +20,19 @@
 #include <linux/sysfs.h>
 #include <linux/units.h>
 
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 
 #include "versal-sysmon.h"
 
+/* OT and TEMP hysteresis mode bits in SYSMON_TEMP_EV_CFG */
+#define SYSMON_OT_HYST_MASK		BIT(0)
+#define SYSMON_TEMP_HYST_MASK		BIT(1)
+
+/* Compute alarm register offset from a channel address */
+#define SYSMON_ALARM_OFFSET(addr) \
+	(SYSMON_ALARM_REG + ((addr) / SYSMON_ALARM_BITS_PER_REG) * SYSMON_REG_STRIDE)
+
 /*
  * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
  * (which operate in hardware register format), PROCESSED gives userspace
@@ -44,6 +55,62 @@
 	.datasheet_name = _name,				\
 }
 
+#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _name, _events) {\
+	.type = IIO_TEMP,					\
+	.indexed = 1,						\
+	.address = _address,					\
+	.channel = _chan,					\
+	.event_spec = _events,					\
+	.num_event_specs = ARRAY_SIZE(_events),			\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = 15,					\
+		.storagebits = 16,				\
+		.endianness = IIO_CPU,				\
+	},							\
+	.datasheet_name = _name,				\
+}
+
+enum sysmon_alarm_bit {
+	SYSMON_BIT_ALARM0 = 0,
+	SYSMON_BIT_ALARM1 = 1,
+	SYSMON_BIT_ALARM2 = 2,
+	SYSMON_BIT_ALARM3 = 3,
+	SYSMON_BIT_ALARM4 = 4,
+	SYSMON_BIT_OT = 8,
+	SYSMON_BIT_TEMP = 9,
+};
+
+/* Temperature event specification: rising threshold + hysteresis only */
+static const struct iio_event_spec sysmon_temp_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+};
+
+/* Supply event specifications */
+static const struct iio_event_spec sysmon_supply_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
 /* Static temperature channels (always present) */
 static const struct iio_chan_spec temp_channels[] = {
 	SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
@@ -52,11 +119,24 @@ static const struct iio_chan_spec temp_channels[] = {
 	SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
 };
 
+/* Temperature event channels (threshold alarms) */
+static const struct iio_chan_spec temp_event_channels[] = {
+	SYSMON_CHAN_TEMP_EVENT(4, SYSMON_ADDR_TEMP_EVENT, "temp",
+			      sysmon_temp_events),
+	SYSMON_CHAN_TEMP_EVENT(5, SYSMON_ADDR_OT_EVENT, "ot",
+			      sysmon_temp_events),
+};
+
 static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
 {
 	*val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;
 }
 
+static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
+{
+	*raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / (int)MILLI;
+}
+
 static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
 {
 	int mantissa, format, exponent;
@@ -74,6 +154,49 @@ static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
 	*val = (mantissa * (int)MILLI) >> exponent;
 }
 
+static void sysmon_supply_processedtoraw(int val, u32 reg_val, u32 *raw_data)
+{
+	int exponent = FIELD_GET(SYSMON_MODE_MASK, reg_val);
+	int format = FIELD_GET(SYSMON_FMT_MASK, reg_val);
+	int scale, tmp;
+
+	scale = BIT(SYSMON_SUPPLY_MANTISSA_BITS - exponent);
+	tmp = (val * scale) / (int)MILLI;
+
+	if (format)
+		tmp = clamp_t(int, tmp, S16_MIN, S16_MAX);
+	else
+		tmp = clamp_t(int, tmp, 0, U16_MAX);
+
+	*raw_data = (u16)tmp;
+}
+
+static int sysmon_temp_thresh_offset(int address,
+				     enum iio_event_direction dir)
+{
+	switch (address) {
+	case SYSMON_ADDR_TEMP_EVENT:
+		return (dir == IIO_EV_DIR_RISING) ? SYSMON_TEMP_TH_UP :
+						    SYSMON_TEMP_TH_LOW;
+	case SYSMON_ADDR_OT_EVENT:
+		return (dir == IIO_EV_DIR_RISING) ? SYSMON_OT_TH_UP :
+						    SYSMON_OT_TH_LOW;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sysmon_supply_thresh_offset(int address,
+				       enum iio_event_direction dir)
+{
+	if (dir == IIO_EV_DIR_RISING)
+		return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_UP;
+	if (dir == IIO_EV_DIR_FALLING)
+		return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_LOW;
+
+	return -EINVAL;
+}
+
 static int sysmon_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long mask)
@@ -115,6 +238,258 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int sysmon_get_event_mask(unsigned long address)
+{
+	if (address == SYSMON_ADDR_TEMP_EVENT)
+		return BIT(SYSMON_BIT_TEMP);
+	if (address == SYSMON_ADDR_OT_EVENT)
+		return BIT(SYSMON_BIT_OT);
+
+	return BIT(address / SYSMON_ALARM_BITS_PER_REG);
+}
+
+static int sysmon_read_alarm_config(struct sysmon *sysmon,
+				    unsigned long address)
+{
+	u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
+	u32 offset = SYSMON_ALARM_OFFSET(address);
+	unsigned int reg_val;
+	int ret;
+
+	ret = regmap_read(sysmon->regmap, offset, &reg_val);
+	if (ret)
+		return ret;
+
+	return !!(reg_val & BIT(shift));
+}
+
+static int sysmon_write_alarm_config(struct sysmon *sysmon,
+				     unsigned long address, bool enable)
+{
+	u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
+	u32 offset = SYSMON_ALARM_OFFSET(address);
+
+	if (enable)
+		return regmap_set_bits(sysmon->regmap, offset, BIT(shift));
+
+	return regmap_clear_bits(sysmon->regmap, offset, BIT(shift));
+}
+
+static int sysmon_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	u32 alarm_event_mask = sysmon_get_event_mask(chan->address);
+	struct sysmon *sysmon = iio_priv(indio_dev);
+	unsigned int imr;
+	int config_value;
+	int ret;
+
+	ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+	if (ret)
+		return ret;
+
+	/* IMR bits are 1=masked, invert to get 1=enabled */
+	imr = ~imr;
+
+	if (chan->type == IIO_VOLTAGE) {
+		config_value = sysmon_read_alarm_config(sysmon, chan->address);
+		if (config_value < 0)
+			return config_value;
+		return config_value && (imr & alarm_event_mask);
+	}
+
+	return !!(imr & alarm_event_mask);
+}
+
+static int sysmon_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     bool state)
+{
+	u32 offset = SYSMON_ALARM_OFFSET(chan->address);
+	u32 ier = sysmon_get_event_mask(chan->address);
+	struct sysmon *sysmon = iio_priv(indio_dev);
+	unsigned int alarm_config;
+	int ret;
+
+	guard(mutex)(&sysmon->lock);
+
+	if (chan->type == IIO_VOLTAGE) {
+		ret = sysmon_write_alarm_config(sysmon, chan->address, state);
+		if (ret)
+			return ret;
+
+		ret = regmap_read(sysmon->regmap, offset, &alarm_config);
+		if (ret)
+			return ret;
+
+		if (alarm_config)
+			return regmap_write(sysmon->regmap, SYSMON_IER, ier);
+
+		return regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+	}
+
+	if (chan->type == IIO_TEMP) {
+		if (state) {
+			ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
+			if (ret)
+				return ret;
+			sysmon->temp_mask &= ~ier;
+		} else {
+			ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+			if (ret)
+				return ret;
+			sysmon->temp_mask |= ier;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Recompute the lower threshold register from upper threshold and
+ * cached hysteresis. Called when either upper threshold or hysteresis
+ * is written.
+ */
+static int sysmon_update_temp_lower(struct sysmon *sysmon, int address)
+{
+	unsigned int upper_reg;
+	int upper_mc, lower_mc, hysteresis;
+	u32 raw_val;
+	int upper_off, lower_off, ret;
+
+	upper_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_RISING);
+	if (upper_off < 0)
+		return upper_off;
+	lower_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_FALLING);
+	if (lower_off < 0)
+		return lower_off;
+
+	if (address == SYSMON_ADDR_OT_EVENT)
+		hysteresis = sysmon->ot_hysteresis;
+	else
+		hysteresis = sysmon->temp_hysteresis;
+
+	ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
+	if (ret)
+		return ret;
+	sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+
+	lower_mc = upper_mc - hysteresis;
+	sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
+
+	return regmap_write(sysmon->regmap, lower_off, raw_val);
+}
+
+static int sysmon_read_event_value(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   enum iio_event_type type,
+				   enum iio_event_direction dir,
+				   enum iio_event_info info,
+				   int *val, int *val2)
+{
+	struct sysmon *sysmon = iio_priv(indio_dev);
+	unsigned int reg_val;
+	int offset;
+	int ret;
+
+	guard(mutex)(&sysmon->lock);
+
+	if (chan->type == IIO_TEMP) {
+		if (info == IIO_EV_INFO_VALUE) {
+			/* Only rising threshold is exposed */
+			offset = sysmon_temp_thresh_offset(chan->address,
+							   IIO_EV_DIR_RISING);
+			if (offset < 0)
+				return offset;
+			ret = regmap_read(sysmon->regmap, offset, &reg_val);
+			if (ret)
+				return ret;
+			sysmon_q8p7_to_millicelsius(reg_val, val);
+			return IIO_VAL_INT;
+		}
+		if (info == IIO_EV_INFO_HYSTERESIS) {
+			if (chan->address == SYSMON_ADDR_OT_EVENT)
+				*val = sysmon->ot_hysteresis;
+			else
+				*val = sysmon->temp_hysteresis;
+			return IIO_VAL_INT;
+		}
+	}
+
+	if (chan->type == IIO_VOLTAGE) {
+		offset = sysmon_supply_thresh_offset(chan->address, dir);
+		if (offset < 0)
+			return offset;
+		ret = regmap_read(sysmon->regmap, offset, &reg_val);
+		if (ret)
+			return ret;
+		sysmon_supply_rawtoprocessed(reg_val, val);
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int sysmon_write_event_value(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info,
+				    int val, int val2)
+{
+	struct sysmon *sysmon = iio_priv(indio_dev);
+	unsigned int reg_val;
+	u32 raw_val;
+	int offset;
+	int ret;
+
+	guard(mutex)(&sysmon->lock);
+
+	if (chan->type == IIO_TEMP) {
+		if (info == IIO_EV_INFO_VALUE) {
+			/* Only rising threshold is exposed */
+			offset = sysmon_temp_thresh_offset(chan->address,
+							   IIO_EV_DIR_RISING);
+			if (offset < 0)
+				return offset;
+			sysmon_millicelsius_to_q8p7(&raw_val, val);
+			ret = regmap_write(sysmon->regmap, offset, raw_val);
+			if (ret)
+				return ret;
+			/* Recompute lower = upper - hysteresis */
+			return sysmon_update_temp_lower(sysmon,
+							chan->address);
+		}
+		if (info == IIO_EV_INFO_HYSTERESIS) {
+			if (val < 0)
+				return -EINVAL;
+			if (chan->address == SYSMON_ADDR_OT_EVENT)
+				sysmon->ot_hysteresis = val;
+			else
+				sysmon->temp_hysteresis = val;
+			return sysmon_update_temp_lower(sysmon,
+							chan->address);
+		}
+	}
+
+	if (chan->type == IIO_VOLTAGE) {
+		offset = sysmon_supply_thresh_offset(chan->address, dir);
+		if (offset < 0)
+			return offset;
+		ret = regmap_read(sysmon->regmap, offset, &reg_val);
+		if (ret)
+			return ret;
+		sysmon_supply_processedtoraw(val, reg_val, &raw_val);
+		return regmap_write(sysmon->regmap, offset, raw_val);
+	}
+
+	return -EINVAL;
+}
+
 static int sysmon_read_label(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     char *label)
@@ -128,20 +503,248 @@ static int sysmon_read_label(struct iio_dev *indio_dev,
 static const struct iio_info sysmon_iio_info = {
 	.read_raw = sysmon_read_raw,
 	.read_label = sysmon_read_label,
+	.read_event_config = sysmon_read_event_config,
+	.write_event_config = sysmon_write_event_config,
+	.read_event_value = sysmon_read_event_value,
+	.write_event_value = sysmon_write_event_value,
 };
 
+static void sysmon_push_event(struct iio_dev *indio_dev, u32 address)
+{
+	const struct iio_chan_spec *chan;
+	enum iio_event_direction dir;
+
+	for (unsigned int i = 0; i < indio_dev->num_channels; i++) {
+		if (indio_dev->channels[i].address != address)
+			continue;
+
+		chan = &indio_dev->channels[i];
+		/* Temp uses hysteresis mode (rising only), voltage uses window */
+		dir = (chan->type == IIO_TEMP) ? IIO_EV_DIR_RISING :
+						 IIO_EV_DIR_EITHER;
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(chan->type,
+						    chan->channel,
+						    IIO_EV_TYPE_THRESH,
+						    dir),
+			       iio_get_time_ns(indio_dev));
+	}
+}
+
+static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
+{
+	u32 alarm_flag_offset = SYSMON_ALARM_FLAG + (event * SYSMON_REG_STRIDE);
+	u32 alarm_reg_offset = SYSMON_ALARM_REG + (event * SYSMON_REG_STRIDE);
+	struct sysmon *sysmon = iio_priv(indio_dev);
+	unsigned long alarm_flag_reg;
+	unsigned int reg_val;
+	u32 address, bit;
+	int ret;
+
+	switch (event) {
+	case SYSMON_BIT_TEMP:
+		sysmon_push_event(indio_dev, SYSMON_ADDR_TEMP_EVENT);
+		ret = regmap_write(sysmon->regmap, SYSMON_IDR,
+				   BIT(SYSMON_BIT_TEMP));
+		if (ret)
+			return ret;
+		sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
+		break;
+
+	case SYSMON_BIT_OT:
+		sysmon_push_event(indio_dev, SYSMON_ADDR_OT_EVENT);
+		ret = regmap_write(sysmon->regmap, SYSMON_IDR,
+				   BIT(SYSMON_BIT_OT));
+		if (ret)
+			return ret;
+		sysmon->masked_temp |= BIT(SYSMON_BIT_OT);
+		break;
+
+	case SYSMON_BIT_ALARM0:
+	case SYSMON_BIT_ALARM1:
+	case SYSMON_BIT_ALARM2:
+	case SYSMON_BIT_ALARM3:
+	case SYSMON_BIT_ALARM4:
+		ret = regmap_read(sysmon->regmap, alarm_flag_offset, &reg_val);
+		if (ret)
+			return ret;
+		alarm_flag_reg = reg_val;
+
+		for_each_set_bit(bit, &alarm_flag_reg,
+				 SYSMON_ALARM_BITS_PER_REG) {
+			address = bit + (SYSMON_ALARM_BITS_PER_REG * event);
+			sysmon_push_event(indio_dev, address);
+			ret = regmap_update_bits(sysmon->regmap,
+						 alarm_reg_offset,
+						 BIT(bit), 0);
+			if (ret)
+				return ret;
+		}
+		ret = regmap_write(sysmon->regmap, alarm_flag_offset,
+				   alarm_flag_reg);
+		if (ret)
+			return ret;
+		break;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static void sysmon_handle_events(struct iio_dev *indio_dev,
+				 unsigned long events)
+{
+	unsigned int bit;
+
+	for_each_set_bit(bit, &events, SYSMON_NO_OF_EVENTS)
+		sysmon_handle_event(indio_dev, bit);
+}
+
+static void sysmon_unmask_temp(struct sysmon *sysmon, unsigned int isr)
+{
+	unsigned int unmask, status;
+
+	status = isr & SYSMON_TEMP_INTR_MASK;
+
+	unmask = (sysmon->masked_temp ^ status) & sysmon->masked_temp;
+	sysmon->masked_temp &= status;
+
+	unmask &= ~sysmon->temp_mask;
+
+	regmap_write(sysmon->regmap, SYSMON_IER, unmask);
+}
+
+/*
+ * Versal threshold interrupts are level-sensitive. Active threshold
+ * interrupts are masked in the handler and polled via delayed work
+ * until the condition clears, then unmasked.
+ */
+static void sysmon_unmask_worker(struct work_struct *work)
+{
+	struct sysmon *sysmon = container_of(work, struct sysmon,
+					     sysmon_unmask_work.work);
+	unsigned int isr;
+
+	spin_lock_irq(&sysmon->irq_lock);
+	regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
+	regmap_write(sysmon->regmap, SYSMON_ISR, isr);
+	sysmon_unmask_temp(sysmon, isr);
+	spin_unlock_irq(&sysmon->irq_lock);
+
+	if (sysmon->masked_temp)
+		schedule_delayed_work(&sysmon->sysmon_unmask_work,
+				      msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
+	else
+		regmap_write(sysmon->regmap, SYSMON_STATUS_RESET, 1);
+}
+
+static irqreturn_t sysmon_iio_irq(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;
+	struct sysmon *sysmon;
+	unsigned int isr, imr;
+
+	sysmon = iio_priv(indio_dev);
+	spin_lock(&sysmon->irq_lock);
+
+	regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
+	regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+
+	isr &= ~imr;
+	regmap_write(sysmon->regmap, SYSMON_ISR, isr);
+
+	if (isr) {
+		sysmon_handle_events(indio_dev, isr);
+		schedule_delayed_work(&sysmon->sysmon_unmask_work,
+				      msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
+	}
+
+	spin_unlock(&sysmon->irq_lock);
+
+	return IRQ_RETVAL(isr);
+}
+
+static int sysmon_init_interrupt(struct sysmon *sysmon,
+				 struct device *dev,
+				 struct iio_dev *indio_dev,
+				 int irq)
+{
+	unsigned int imr;
+	int ret;
+
+	/* Events not supported without IRQ (e.g. I2C path) */
+	if (!irq)
+		return 0;
+
+	ret = devm_delayed_work_autocancel(dev, &sysmon->sysmon_unmask_work,
+					   sysmon_unmask_worker);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+	if (ret)
+		return ret;
+	sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
+
+	return devm_request_irq(dev, irq, sysmon_iio_irq, 0,
+				"sysmon-irq", indio_dev);
+}
+
+/*
+ * Initialize the cached hysteresis for a temperature channel from the
+ * current hardware threshold registers: hysteresis = upper - lower.
+ */
+static int sysmon_init_hysteresis(struct sysmon *sysmon, int address,
+				  int *hysteresis)
+{
+	unsigned int upper_reg, lower_reg;
+	int upper_mc, lower_mc;
+	int upper_off, lower_off;
+	int ret;
+
+	upper_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_RISING);
+	if (upper_off < 0)
+		return upper_off;
+	lower_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_FALLING);
+	if (lower_off < 0)
+		return lower_off;
+
+	ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
+	if (ret)
+		return ret;
+	ret = regmap_read(sysmon->regmap, lower_off, &lower_reg);
+	if (ret)
+		return ret;
+
+	sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+	sysmon_q8p7_to_millicelsius(lower_reg, &lower_mc);
+	*hysteresis = upper_mc - lower_mc;
+
+	return 0;
+}
+
 /**
  * sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
  * @indio_dev: IIO device instance
  * @dev: Parent device
+ * @has_irq: true if an IRQ is available (enables event channels)
  *
  * Reads voltage-channels and temperature-channels container nodes from
  * firmware and builds the IIO channel array. Static temperature channels
- * are prepended, followed by supply and satellite channels from DT.
+ * and event channels are prepended, followed by supply and satellite
+ * channels from DT.
+ *
+ * Event channels and per-channel event specs are only added when the
+ * device has an IRQ. I2C devices have no interrupt line, and the I2C
+ * regmap cannot be called from atomic context, so events are not
+ * supported on that path.
  *
  * Return: 0 on success, negative errno on failure.
  */
-static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
+static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
+			   bool has_irq)
 {
 	struct fwnode_handle *supply_node __free(fwnode_handle) =
 		device_get_named_child_node(dev, "voltage-channels");
@@ -150,6 +753,7 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
 	unsigned int num_supply = 0, num_temp = 0;
 	unsigned int idx, temp_chan_idx, volt_chan_idx;
 	struct iio_chan_spec *sysmon_channels;
+	unsigned int num_events;
 	const char *label;
 	u32 reg;
 	int ret;
@@ -159,8 +763,11 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
 	if (temp_node)
 		num_temp = fwnode_get_child_node_count(temp_node);
 
+	num_events = has_irq ? ARRAY_SIZE(temp_event_channels) : 0;
+
 	sysmon_channels = devm_kcalloc(dev,
-				       size_add(ARRAY_SIZE(temp_channels),
+				       size_add(ARRAY_SIZE(temp_channels) +
+						num_events,
 						num_supply + num_temp),
 				       sizeof(*sysmon_channels), GFP_KERNEL);
 	if (!sysmon_channels)
@@ -171,6 +778,13 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
 	memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
 	idx += ARRAY_SIZE(temp_channels);
 
+	/* Temperature event channels (only when IRQ is available) */
+	if (has_irq) {
+		memcpy(sysmon_channels + idx, temp_event_channels,
+		       sizeof(temp_event_channels));
+		idx += ARRAY_SIZE(temp_event_channels);
+	}
+
 	/* Supply channels from DT */
 	fwnode_for_each_child_node_scoped(supply_node, child) {
 		ret = fwnode_property_read_u32(child, "reg", &reg);
@@ -195,6 +809,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
 			.info_mask_separate =
 				BIT(IIO_CHAN_INFO_RAW) |
 				BIT(IIO_CHAN_INFO_PROCESSED),
+			.event_spec = has_irq ?
+				sysmon_supply_events : NULL,
+			.num_event_specs = has_irq ?
+				ARRAY_SIZE(sysmon_supply_events) : 0,
 			.scan_type = {
 				.realbits = 19,
 				.storagebits = 32,
@@ -274,6 +892,8 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
 {
 	struct iio_dev *indio_dev;
 	struct sysmon *sysmon;
+	bool has_irq;
+	int irq;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*sysmon));
@@ -286,6 +906,7 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
 	ret = devm_mutex_init(dev, &sysmon->lock);
 	if (ret)
 		return ret;
+	spin_lock_init(&sysmon->irq_lock);
 
 	/* Disable all interrupts and clear pending status */
 	ret = regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
@@ -295,13 +916,39 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
 	if (ret)
 		return ret;
 
+	irq = fwnode_irq_get(dev_fwnode(dev), 0);
+	has_irq = irq > 0;
+
 	indio_dev->name = "versal-sysmon";
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = sysmon_parse_fw(indio_dev, dev);
+	ret = sysmon_parse_fw(indio_dev, dev, has_irq);
 	if (ret)
 		return ret;
 
+	if (has_irq) {
+		/* Set hysteresis mode for both temperature channels */
+		ret = regmap_set_bits(sysmon->regmap, SYSMON_TEMP_EV_CFG,
+				      SYSMON_OT_HYST_MASK |
+				      SYSMON_TEMP_HYST_MASK);
+		if (ret)
+			return ret;
+
+		/* Initialize cached hysteresis from hardware registers */
+		ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_TEMP_EVENT,
+					     &sysmon->temp_hysteresis);
+		if (ret)
+			return ret;
+		ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_OT_EVENT,
+					     &sysmon->ot_hysteresis);
+		if (ret)
+			return ret;
+
+		ret = sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
+		if (ret)
+			return ret;
+	}
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_GPL(sysmon_core_probe);
diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
index d24d2481915..a78362f95e6 100644
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -11,7 +11,9 @@
 
 #include <linux/bits.h>
 #include <linux/mutex.h>
+#include <linux/spinlock_types.h>
 #include <linux/types.h>
+#include <linux/workqueue.h>
 
 struct device;
 struct iio_dev;
@@ -20,12 +22,24 @@ struct regmap;
 /* Register offsets (sorted by address) */
 #define SYSMON_NPI_LOCK			0x000C
 #define SYSMON_ISR			0x0044
+#define SYSMON_IMR			0x0048
+#define SYSMON_IER			0x004C
 #define SYSMON_IDR			0x0050
 #define SYSMON_TEMP_MAX			0x1030
 #define SYSMON_TEMP_MIN			0x1034
 #define SYSMON_SUPPLY_BASE		0x1040
+#define SYSMON_ALARM_FLAG		0x1018
+#define SYSMON_ALARM_REG		0x1940
+#define SYSMON_TEMP_TH_LOW		0x1970
+#define SYSMON_TEMP_TH_UP		0x1974
+#define SYSMON_OT_TH_LOW		0x1978
+#define SYSMON_OT_TH_UP		0x197C
+#define SYSMON_SUPPLY_TH_LOW		0x1980
+#define SYSMON_SUPPLY_TH_UP		0x1C80
+#define SYSMON_TEMP_EV_CFG		0x1F84
 #define SYSMON_TEMP_MIN_MIN		0x1F8C
 #define SYSMON_TEMP_MAX_MAX		0x1F90
+#define SYSMON_STATUS_RESET		0x1F94
 #define SYSMON_TEMP_SAT_BASE		0x1FAC
 #define SYSMON_MAX_REG			0x24C0
 
@@ -37,8 +51,12 @@ struct regmap;
 
 #define SYSMON_SUPPLY_IDX_MAX		159
 #define SYSMON_TEMP_SAT_MAX		64
+#define SYSMON_NO_OF_EVENTS		32
 #define SYSMON_INTR_ALL_MASK		GENMASK(31, 0)
 
+/* ISR/IMR temperature and OT alarm mask (bits 9:8) */
+#define SYSMON_TEMP_INTR_MASK		GENMASK(9, 8)
+
 /* Supply voltage conversion register fields */
 #define SYSMON_MANTISSA_MASK		GENMASK(15, 0)
 #define SYSMON_FMT_MASK			BIT(16)
@@ -48,15 +66,41 @@ struct regmap;
 #define SYSMON_FRACTIONAL_SHIFT		7U
 #define SYSMON_SUPPLY_MANTISSA_BITS	16
 
+/* Event address IDs for temp event channels */
+#define SYSMON_ADDR_TEMP_EVENT		160
+#define SYSMON_ADDR_OT_EVENT		161
+
+/* Bits per alarm register */
+#define SYSMON_ALARM_BITS_PER_REG	32
+
+#define SYSMON_UNMASK_WORK_DELAY_MS	500
+
 /**
  * struct sysmon - Driver data for Versal SysMon
  * @regmap: register map for hardware access
- * @lock: protects regmap access
+ * @lock: protects regmap access and cached state
+ * @irq_lock: protects interrupt mask register updates (MMIO path only)
+ * @masked_temp: currently masked temperature alarm bits
+ * @temp_mask: temperature interrupt configuration mask
+ * @temp_hysteresis: cached DEVICE_TEMP hysteresis in millicelsius
+ * @ot_hysteresis: cached OT hysteresis in millicelsius
+ * @sysmon_unmask_work: re-enables events after alarm condition clears
  */
 struct sysmon {
 	struct regmap *regmap;
-	/* Protects regmap access */
+	/* Protects regmap access and cached state */
 	struct mutex lock;
+	/*
+	 * Protects interrupt mask register updates.  Only used on the
+	 * MMIO path (fast_io regmap); I2C has no IRQ and never reaches
+	 * the event code that takes this lock.
+	 */
+	spinlock_t irq_lock;
+	unsigned int masked_temp;
+	unsigned int temp_mask;
+	int temp_hysteresis;
+	int ot_hysteresis;
+	struct delayed_work sysmon_unmask_work;
 };
 
 int sysmon_core_probe(struct device *dev, struct regmap *regmap);
-- 
2.48.1

[PATCH v3 5/5] iio: adc: versal-sysmon: add oversampling support

[Salih Erim]

Add support for reading and writing the oversampling ratio through
the IIO oversampling_ratio attribute. The hardware supports averaging
2, 4, 8, or 16 samples, plus a ratio of 1 (no averaging).

Temperature and supply channels share oversampling configuration at
the type level (all temperature channels share one ratio, all supply
channels share another), exposed through info_mask_shared_by_type.

The hardware encoding uses sample_count / 2 in a 4-bit field within
the CONFIG register. Per-channel averaging enable registers must also
be updated to activate or deactivate averaging.

Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in v3:
  - No changes

Changes in v2:
  - EN_AVG per-channel bitmask registers written with all-ones
    instead of boolean 1 when oversampling is enabled
  - EN_AVG write errors propagated to userspace
  - Oversampling limited to satellite temp and supply channels;
    static temp channels do not participate
  - Oversampling exposes actual sample counts (1,2,4,8,16) to
    userspace with internal HW register translation
  - write_raw_get_fmt returns IIO_VAL_INT for oversampling ratio
  - HW encoding documented (sample_count/2, not log2)
  - oversampling_avail is const int[] (type match fix)

 drivers/iio/adc/versal-sysmon-core.c | 136 +++++++++++++++++++++++++++
 drivers/iio/adc/versal-sysmon.h      |  17 ++++
 2 files changed, 153 insertions(+)

diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
index 04977c9c887..7775a4cfa6d 100644
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -25,6 +25,12 @@
 
 #include "versal-sysmon.h"
 
+/*
+ * Oversampling ratio values exposed to userspace via IIO.
+ * Actual number of samples averaged: 1=none, 2=2x, 4=4x, 8=8x, 16=16x.
+ */
+static const int sysmon_oversampling_avail[] = { 1, 2, 4, 8, 16 };
+
 /* OT and TEMP hysteresis mode bits in SYSMON_TEMP_EV_CFG */
 #define SYSMON_OT_HYST_MASK		BIT(0)
 #define SYSMON_TEMP_HYST_MASK		BIT(1)
@@ -205,6 +211,12 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
 	unsigned int regval;
 	int ret;
 
+	if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO) {
+		*val = (chan->type == IIO_TEMP) ? sysmon->temp_oversampling :
+						 sysmon->supply_oversampling;
+		return IIO_VAL_INT;
+	}
+
 	if (mask != IIO_CHAN_INFO_RAW && mask != IIO_CHAN_INFO_PROCESSED)
 		return -EINVAL;
 
@@ -490,6 +502,117 @@ static int sysmon_write_event_value(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int sysmon_set_avg_enable(struct sysmon *sysmon,
+				 u32 base, u32 count, u32 val)
+{
+	int ret;
+
+	for (unsigned int i = 0; i < count; i++) {
+		ret = regmap_write(sysmon->regmap,
+				   base + (i * SYSMON_REG_STRIDE), val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int sysmon_osr_write(struct sysmon *sysmon, int channel_type, int val)
+{
+	/*
+	 * HW register encoding is sample_count / 2:
+	 * 0=none, 1=2x, 2=4x, 4=8x, 8=16x (not log2-based).
+	 */
+	int hw_val = val >> 1;
+	int ret;
+
+	if (channel_type == IIO_TEMP) {
+		ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
+					SYSMON_TEMP_SAT_CONFIG_MASK,
+					FIELD_PREP(SYSMON_TEMP_SAT_CONFIG_MASK,
+						   hw_val));
+		if (ret)
+			return ret;
+		ret = sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
+					    SYSMON_TEMP_EN_AVG_COUNT,
+					    hw_val ? ~0U : 0);
+		if (ret)
+			return ret;
+	} else if (channel_type == IIO_VOLTAGE) {
+		ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
+					SYSMON_SUPPLY_CONFIG_MASK,
+					FIELD_PREP(SYSMON_SUPPLY_CONFIG_MASK,
+						   hw_val));
+		if (ret)
+			return ret;
+		ret = sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
+					    SYSMON_SUPPLY_EN_AVG_COUNT,
+					    hw_val ? ~0U : 0);
+		if (ret)
+			return ret;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int sysmon_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct sysmon *sysmon = iio_priv(indio_dev);
+	int i, ret;
+
+	if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(sysmon_oversampling_avail); i++) {
+		if (val == sysmon_oversampling_avail[i])
+			break;
+	}
+	if (i == ARRAY_SIZE(sysmon_oversampling_avail))
+		return -EINVAL;
+
+	guard(mutex)(&sysmon->lock);
+
+	ret = sysmon_osr_write(sysmon, chan->type, val);
+	if (ret)
+		return ret;
+
+	if (chan->type == IIO_TEMP)
+		sysmon->temp_oversampling = val;
+	else
+		sysmon->supply_oversampling = val;
+
+	return 0;
+}
+
+static int sysmon_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan,
+				    long mask)
+{
+	if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+		return IIO_VAL_INT;
+
+	return -EINVAL;
+}
+
+static int sysmon_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type,
+			     int *length, long mask)
+{
+	if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+		return -EINVAL;
+
+	*vals = sysmon_oversampling_avail;
+	*type = IIO_VAL_INT;
+	*length = ARRAY_SIZE(sysmon_oversampling_avail);
+
+	return IIO_AVAIL_LIST;
+}
+
 static int sysmon_read_label(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     char *label)
@@ -502,6 +625,9 @@ static int sysmon_read_label(struct iio_dev *indio_dev,
 
 static const struct iio_info sysmon_iio_info = {
 	.read_raw = sysmon_read_raw,
+	.write_raw = sysmon_write_raw,
+	.write_raw_get_fmt = sysmon_write_raw_get_fmt,
+	.read_avail = sysmon_read_avail,
 	.read_label = sysmon_read_label,
 	.read_event_config = sysmon_read_event_config,
 	.write_event_config = sysmon_write_event_config,
@@ -809,6 +935,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
 			.info_mask_separate =
 				BIT(IIO_CHAN_INFO_RAW) |
 				BIT(IIO_CHAN_INFO_PROCESSED),
+			.info_mask_shared_by_type =
+				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+			.info_mask_shared_by_type_available =
+				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 			.event_spec = has_irq ?
 				sysmon_supply_events : NULL,
 			.num_event_specs = has_irq ?
@@ -849,6 +979,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
 			.info_mask_separate =
 				BIT(IIO_CHAN_INFO_RAW) |
 				BIT(IIO_CHAN_INFO_PROCESSED),
+			.info_mask_shared_by_type =
+				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+			.info_mask_shared_by_type_available =
+				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 			.scan_type = {
 				.sign = 's',
 				.realbits = 15,
@@ -902,6 +1036,8 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
 
 	sysmon = iio_priv(indio_dev);
 	sysmon->regmap = regmap;
+	sysmon->temp_oversampling = 1;
+	sysmon->supply_oversampling = 1;
 
 	ret = devm_mutex_init(dev, &sysmon->lock);
 	if (ret)
diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
index a78362f95e6..cf69be62709 100644
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -25,11 +25,13 @@ struct regmap;
 #define SYSMON_IMR			0x0048
 #define SYSMON_IER			0x004C
 #define SYSMON_IDR			0x0050
+#define SYSMON_CONFIG			0x0100
 #define SYSMON_TEMP_MAX			0x1030
 #define SYSMON_TEMP_MIN			0x1034
 #define SYSMON_SUPPLY_BASE		0x1040
 #define SYSMON_ALARM_FLAG		0x1018
 #define SYSMON_ALARM_REG		0x1940
+#define SYSMON_SUPPLY_EN_AVG_BASE	0x1958
 #define SYSMON_TEMP_TH_LOW		0x1970
 #define SYSMON_TEMP_TH_UP		0x1974
 #define SYSMON_OT_TH_LOW		0x1978
@@ -41,6 +43,7 @@ struct regmap;
 #define SYSMON_TEMP_MAX_MAX		0x1F90
 #define SYSMON_STATUS_RESET		0x1F94
 #define SYSMON_TEMP_SAT_BASE		0x1FAC
+#define SYSMON_TEMP_EN_AVG_BASE		0x24B4
 #define SYSMON_MAX_REG			0x24C0
 
 /* NPI unlock value written to SYSMON_NPI_LOCK */
@@ -57,6 +60,16 @@ struct regmap;
 /* ISR/IMR temperature and OT alarm mask (bits 9:8) */
 #define SYSMON_TEMP_INTR_MASK		GENMASK(9, 8)
 
+/* Config register: supply oversampling field (bits 17:14) */
+#define SYSMON_SUPPLY_CONFIG_MASK	GENMASK(17, 14)
+
+/* Config register: temp satellite oversampling field (bits 27:24) */
+#define SYSMON_TEMP_SAT_CONFIG_MASK	GENMASK(27, 24)
+
+/* Per-channel averaging enable register counts */
+#define SYSMON_SUPPLY_EN_AVG_COUNT	5
+#define SYSMON_TEMP_EN_AVG_COUNT	2
+
 /* Supply voltage conversion register fields */
 #define SYSMON_MANTISSA_MASK		GENMASK(15, 0)
 #define SYSMON_FMT_MASK			BIT(16)
@@ -85,6 +98,8 @@ struct regmap;
  * @temp_hysteresis: cached DEVICE_TEMP hysteresis in millicelsius
  * @ot_hysteresis: cached OT hysteresis in millicelsius
  * @sysmon_unmask_work: re-enables events after alarm condition clears
+ * @temp_oversampling: current temp oversampling ratio
+ * @supply_oversampling: current supply oversampling ratio
  */
 struct sysmon {
 	struct regmap *regmap;
@@ -101,6 +116,8 @@ struct sysmon {
 	int temp_hysteresis;
 	int ot_hysteresis;
 	struct delayed_work sysmon_unmask_work;
+	unsigned int temp_oversampling;
+	unsigned int supply_oversampling;
 };
 
 int sysmon_core_probe(struct device *dev, struct regmap *regmap);
-- 
2.48.1

Re: [PATCH v3 2/5] iio: adc: add Versal SysMon driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The Kconfig dependency prevents building the driver on its intended architecture.
- [High] Raw temperature reads lack sign extension, and there is a mismatch in bit width definitions causing negative temperatures to be corrupted.
- [Medium] Exposing IIO_CHAN_INFO_RAW for packed floating-point hardware formats violates the IIO UAPI.
--

commit 084edfc78fc9fd37f78ac260bcd2da4ecb832e8a
Author: Salih Erim <salih.erim@amd.com>

iio: adc: add Versal SysMon driver

Adds the core driver and MMIO platform driver for the AMD/Xilinx Versal
System Monitor (SysMon) block, providing voltage and temperature monitoring.

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig

[ ... ]

> @@ -1943,6 +1943,26 @@ config TWL6030_GPADC
>  	  This driver can also be built as a module. If so, the module will be
>  	  called twl6030-gpadc.
>  
> +config VERSAL_SYSMON_CORE
> +	tristate
> +	select REGMAP
> +
> +config VERSAL_SYSMON
> +	tristate "AMD Versal SysMon driver"
> +	depends on ARCH_ZYNQMP || COMPILE_TEST

[Severity: High]
Could this Kconfig dependency prevent the driver from being built on its
intended architecture? Since this driver targets the AMD Versal platform,
should it depend on ARCH_VERSAL instead of ARCH_ZYNQMP?

> +	depends on HAS_IOMEM
> +	select VERSAL_SYSMON_CORE

[ ... ]

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon-core.c

[ ... ]

> +/*
> + * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
> + * (which operate in hardware register format), PROCESSED gives userspace
> + * the converted millivolt or millicelsius value.
> + */

[Severity: Medium]
Does exposing RAW for packed floating-point formats violate the IIO UAPI?

The IIO ABI dictates that _raw attributes must represent a simple, unscaled
linear integer value. Exporting a packed format containing mantissa, exponent,
and format bits could break generic userspace clients expecting linear values.

Since event thresholds use their own dedicated sysfs attributes, is it
necessary to expose _raw on the main channel?

> +#define SYSMON_CHAN_TEMP(_chan, _address, _name) {		\
> +	.type = IIO_TEMP,					\
> +	.indexed = 1,						\
> +	.address = _address,					\
> +	.channel = _chan,					\
> +	.info_mask_separate =					\
> +		BIT(IIO_CHAN_INFO_RAW) |				\
> +		BIT(IIO_CHAN_INFO_PROCESSED),			\
> +	.scan_type = {						\
> +		.sign = 's',					\
> +		.realbits = 15,					\

[Severity: High]
There appears to be a mismatch between the realbits defined here and the
parameter type used in the conversion function below.

If the hardware format is actually 15-bit, will casting to s16 in
sysmon_q8p7_to_millicelsius() fail to sign-extend negative values? Or if
the hardware format is actually 16-bit, should realbits be set to 16?

> +		.storagebits = 16,				\
> +		.endianness = IIO_CPU,				\
> +	},							\
> +	.datasheet_name = _name,				\
> +}

[ ... ]

> +static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
> +{
> +	*val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;
> +}

[ ... ]

> +static int sysmon_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int regval;
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_RAW && mask != IIO_CHAN_INFO_PROCESSED)
> +		return -EINVAL;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	switch (chan->type) {
> +	case IIO_TEMP:
> +		ret = regmap_read(sysmon->regmap, chan->address, &regval);
> +		if (ret)
> +			return ret;
> +		if (mask == IIO_CHAN_INFO_PROCESSED)
> +			sysmon_q8p7_to_millicelsius(regval, val);
> +		else
> +			*val = regval;

[Severity: High]
Can this direct assignment corrupt negative temperature reads?

Since regval is unsigned int, assigning it directly to *val without
sign-extending the hardware value could result in a large positive integer
being exported to sysfs when the temperature is negative.

> +		return IIO_VAL_INT;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim@amd.com?part=2

Re: [PATCH v3 3/5] iio: adc: versal-sysmon: add I2C driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The I2C register read operation splits the write and read phases into two independent API calls, breaking bus atomicity and introducing an unsafe STOP condition.
- [Low] The VERSAL_SYSMON_I2C config is missing the architecture dependency present in the related VERSAL_SYSMON config.
--

commit ce68d28f1b162fe7dac253345bd2e5c0af9ee334
Author: Salih Erim <salih.erim@amd.com>

iio: adc: versal-sysmon: add I2C driver

This commit adds an I2C transport driver for the Versal SysMon block,
allowing reading voltage and temperature measurements over I2C.

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index c7f1905748497..8f9fc9de74a54 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
[ ... ]
> @@ -1963,6 +1963,19 @@ config VERSAL_SYSMON
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called versal-sysmon.
>  
> +config VERSAL_SYSMON_I2C
> +	tristate "AMD Versal SysMon I2C driver"
> +	depends on I2C
> +	select VERSAL_SYSMON_CORE

[Severity: Low]
Does this config need an architecture dependency?

The related VERSAL_SYSMON config restricts visibility with
"depends on ARCH_ZYNQMP || COMPILE_TEST". By omitting this dependency here,
will VERSAL_SYSMON_I2C be incorrectly presented as an available driver on
all I2C-enabled architectures?

> +	help
> +	  Say yes here to have support for the AMD/Xilinx Versal System
[ ... ]
> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
> new file mode 100644
> index 0000000000000..92d149f517e57
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon-i2c.c
[ ... ]
> +static int sysmon_i2c_reg_read(void *context, unsigned int reg,
> +			       unsigned int *val)
> +{
> +	struct i2c_client *client = context;
> +	u8 write_buf[8] = { };
> +	u8 read_buf[4];
> +	int ret;
> +
> +	write_buf[SYSMON_I2C_OFS_LOW_IDX] =
> +		FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
> +	write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
> +		FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);
> +	write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_READ;
> +
> +	ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(write_buf))
> +		return -EIO;
> +
> +	ret = i2c_master_recv(client, read_buf, sizeof(read_buf));

[Severity: Medium]
Will this sequence of independent send and receive calls break bus atomicity?

Since i2c_master_send() and i2c_master_recv() independently acquire and
release the I2C adapter lock, this approach generates a STOP condition on
the bus after the write phase.

On a shared I2C bus, couldn't this allow transactions for other devices to
interleave and disrupt the device state or abort the pending read sequence?

Would it be safer to use i2c_transfer() with an array of two i2c_msg
structures to ensure a Repeated START (Sr) condition?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim@amd.com?part=3

Re: [PATCH v3 4/5] iio: adc: versal-sysmon: add threshold event support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Data race and missing synchronization when updating `sysmon->temp_mask` and hardware interrupt mask registers.
- [High] Voltage threshold alarms act as one-shot events and become permanently disabled upon triggering.
- [High] Signed integer overflow in `sysmon_supply_processedtoraw` when scaling unconstrained user-supplied thresholds.
- [High] Unhandled regmap error during voltage alarm processing leads to an IRQ storm.
- [High] `sysmon_millicelsius_to_q8p7()` triggers undefined behavior and writes sign-extended garbage to reserved upper bits.
- [High] Requesting a hardirq handler for I2C devices will trigger a 'scheduling while atomic' kernel panic.
- [High] Lockless read of `sysmon->masked_temp` causes a TOCTOU race, and undocumented write to `SYSMON_STATUS_RESET` destroys telemetry data.
- [High] Voltage threshold writes clobber the format and mode bits in the hardware register.
- [High] `sysmon_update_temp_lower` is vulnerable to a signed integer underflow when calculating the lower threshold.
- [Medium] Reading the event configuration of a temperature channel during an active alarm incorrectly returns 0 (disabled).
- [Medium] Device interrupts are not disabled in the hardware during driver teardown.
--

commit bf3f4a0490a50d3240978d97650f909908cd4a27
Author: Salih Erim <salih.erim@amd.com>

iio: adc: versal-sysmon: add threshold event support

Add threshold event support for temperature and supply voltage channels.

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index ebe052f69820f..04977c9c8871f 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
[ ... ]
> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
> +{
> +	*raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / (int)MILLI;
> +}

[Severity: High]
Will left-shifting a negative val cause undefined behavior in C? 

Additionally, when the division produces a negative integer, does the
implicit cast to u32 sign-extend and set the upper 16 bits to 1? Could
this clobber reserved upper bits when written to the threshold registers?

[ ... ]
> +static void sysmon_supply_processedtoraw(int val, u32 reg_val, u32 *raw_data)
> +{
> +	int exponent = FIELD_GET(SYSMON_MODE_MASK, reg_val);
> +	int format = FIELD_GET(SYSMON_FMT_MASK, reg_val);
> +	int scale, tmp;
> +
> +	scale = BIT(SYSMON_SUPPLY_MANTISSA_BITS - exponent);
> +	tmp = (val * scale) / (int)MILLI;

[Severity: High]
Can val * scale silently overflow a 32-bit signed integer if userspace
provides a large threshold value (e.g., > 32767)? 

Would it be safer to use a 64-bit cast here to avoid undefined behavior
before clamping?

[ ... ]
> +static int sysmon_read_event_config(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir)
> +{
> +	u32 alarm_event_mask = sysmon_get_event_mask(chan->address);
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int imr;
> +	int config_value;
> +	int ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
> +	if (ret)
> +		return ret;

[Severity: Medium]
When a temperature alarm triggers, the interrupt handler temporarily masks
the interrupt in hardware to prevent an IRQ storm.

If userspace reads the sysfs _en attribute during this period, will it
incorrectly read 0 (disabled) instead of the logical administrative state
of the event?

[ ... ]
> +static int sysmon_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     bool state)
> +{
> +	u32 offset = SYSMON_ALARM_OFFSET(chan->address);
> +	u32 ier = sysmon_get_event_mask(chan->address);
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int alarm_config;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);
[ ... ]
> +	if (chan->type == IIO_TEMP) {
> +		if (state) {
> +			ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
> +			if (ret)
> +				return ret;
> +			sysmon->temp_mask &= ~ier;

[Severity: High]
sysmon_write_event_config() updates sysmon->temp_mask and modifies the
hardware interrupt registers while holding sysmon->lock (a mutex).

However, sysmon_unmask_worker() reads sysmon->temp_mask and modifies the
hardware interrupt registers while holding sysmon->irq_lock (a spinlock).

Since these paths use completely different locks, can they execute
concurrently and create a data race that leaves the hardware interrupt
mask permanently out of sync with the software state?

> +		} else {
> +			ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
> +			if (ret)
> +				return ret;
> +			sysmon->temp_mask |= ier;
> +		}
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int sysmon_update_temp_lower(struct sysmon *sysmon, int address)
> +{
> +	unsigned int upper_reg;
> +	int upper_mc, lower_mc, hysteresis;
> +	u32 raw_val;
> +	int upper_off, lower_off, ret;
[ ... ]
> +	ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
> +	if (ret)
> +		return ret;
> +	sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
> +
> +	lower_mc = upper_mc - hysteresis;

[Severity: High]
If userspace configures an exceptionally large hysteresis value while upper_mc
is negative, can this subtraction underflow a 32-bit signed integer?

This might produce a large positive value, making the lower threshold higher
than the upper threshold and breaking the alarm logic.

> +	sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
> +
> +	return regmap_write(sysmon->regmap, lower_off, raw_val);
> +}
[ ... ]
> +static int sysmon_write_event_value(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info,
> +				    int val, int val2)
> +{
[ ... ]
> +	if (chan->type == IIO_VOLTAGE) {
> +		offset = sysmon_supply_thresh_offset(chan->address, dir);
> +		if (offset < 0)
> +			return offset;
> +		ret = regmap_read(sysmon->regmap, offset, &reg_val);
> +		if (ret)
> +			return ret;
> +		sysmon_supply_processedtoraw(val, reg_val, &raw_val);
> +		return regmap_write(sysmon->regmap, offset, raw_val);

[Severity: High]
sysmon_supply_processedtoraw() zero-extends the 16-bit threshold.

Does calling regmap_write() here overwrite the format and mode bits
(bits 16-18) with 0s? Should this be a regmap_update_bits() call instead
to preserve those configuration bits?

> +	}
> +
> +	return -EINVAL;
> +}
[ ... ]
> +static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
> +{
[ ... ]
> +	case SYSMON_BIT_ALARM0:
> +	case SYSMON_BIT_ALARM1:
> +	case SYSMON_BIT_ALARM2:
> +	case SYSMON_BIT_ALARM3:
> +	case SYSMON_BIT_ALARM4:
> +		ret = regmap_read(sysmon->regmap, alarm_flag_offset, &reg_val);
> +		if (ret)
> +			return ret;
> +		alarm_flag_reg = reg_val;
> +
> +		for_each_set_bit(bit, &alarm_flag_reg,
> +				 SYSMON_ALARM_BITS_PER_REG) {
> +			address = bit + (SYSMON_ALARM_BITS_PER_REG * event);
> +			sysmon_push_event(indio_dev, address);
> +			ret = regmap_update_bits(sysmon->regmap,
> +						 alarm_reg_offset,
> +						 BIT(bit), 0);

[Severity: High]
When a voltage threshold triggers, the driver clears the corresponding bit
in SYSMON_ALARM_REG to prevent an interrupt storm.

Is there a mechanism to ever re-enable this alarm later? If not, does this
make the voltage threshold alarms act as one-shot events, violating the
continuous nature of IIO events?

> +			if (ret)
> +				return ret;

[Severity: High]
If regmap_update_bits() fails here, the function returns early and skips the
mandatory regmap_write() to alarm_flag_offset that clears the hardware
interrupt status.

Since sysmon_handle_events() ignores the return value and sysmon_iio_irq()
returns IRQ_HANDLED, will this cause the interrupt controller to immediately
re-trigger the handler, resulting in an infinite IRQ storm?

> +		}
> +		ret = regmap_write(sysmon->regmap, alarm_flag_offset,
> +				   alarm_flag_reg);
> +		if (ret)
> +			return ret;
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static void sysmon_unmask_worker(struct work_struct *work)
> +{
> +	struct sysmon *sysmon = container_of(work, struct sysmon,
> +					     sysmon_unmask_work.work);
> +	unsigned int isr;
> +
> +	spin_lock_irq(&sysmon->irq_lock);
> +	regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
> +	regmap_write(sysmon->regmap, SYSMON_ISR, isr);
> +	sysmon_unmask_temp(sysmon, isr);
> +	spin_unlock_irq(&sysmon->irq_lock);
> +
> +	if (sysmon->masked_temp)
> +		schedule_delayed_work(&sysmon->sysmon_unmask_work,
> +				      msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
> +	else
> +		regmap_write(sysmon->regmap, SYSMON_STATUS_RESET, 1);

[Severity: High]
Is it safe to read sysmon->masked_temp after dropping sysmon->irq_lock?
Could the hardware interrupt handler concurrently update it, leading to a
TOCTOU race?

Additionally, if it evaluates to false, the worker writes to
SYSMON_STATUS_RESET. Doesn't this register clear the hardware Min/Max
status registers (e.g., SYSMON_TEMP_MAX)? Could this permanently destroy
historical telemetry data simply because an alarm cleared?

> +}
[ ... ]
> +static int sysmon_init_interrupt(struct sysmon *sysmon,
> +				 struct device *dev,
> +				 struct iio_dev *indio_dev,
> +				 int irq)
> +{
[ ... ]
> +	return devm_request_irq(dev, irq, sysmon_iio_irq, 0,
> +				"sysmon-irq", indio_dev);

[Severity: High]
The driver unconditionally uses devm_request_irq() to register a hardirq
handler if an interrupt is present in the Device Tree.

If the device is connected via I2C, the hardirq handler will execute
regmap_read() on the I2C regmap, which requires sleeping locks. Will
sleeping inside the hardirq context trigger a scheduling while atomic
kernel panic?

Would it be better to check !regmap_might_sleep() or use
devm_request_threaded_irq() here?

> +}
[ ... ]
>  int sysmon_core_probe(struct device *dev, struct regmap *regmap)
>  {
[ ... ]
> +		ret = sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);

[Severity: Medium]
Does the driver need a teardown hook to physically disable the device's
interrupt generation on module unload?

While devm_request_irq deregisters the kernel handler, the hardware may
continue asserting the physical IRQ line to the interrupt controller,
potentially causing issues if the IRQ is shared.

>  }
>  EXPORT_SYMBOL_GPL(sysmon_core_probe);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim@amd.com?part=4

Re: [PATCH v3 5/5] iio: adc: versal-sysmon: add oversampling support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Oversampling ratio is incorrectly exposed as a type-wide attribute for all temperature channels, while hardware only supports it for satellite channels.
- [Medium] Software oversampling state is initialized to 1 but not synchronized with the hardware during probe.
- [Medium] Unlocked read of oversampling ratios causes a data race with concurrent writes.
--

commit 747d3ecadc20e43c2e94e634cd186da1e49aea29
Author: Salih Erim <salih.erim@amd.com>

iio: adc: versal-sysmon: add oversampling support

This commit adds support for reading and writing the oversampling ratio
through the IIO oversampling_ratio attribute. It configures averaging
for temperature and supply channels and updates the hardware registers.

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 04977c9c8871f..7775a4cfa6deb 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c

[ ... ]

> @@ -205,6 +211,12 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
>  	unsigned int regval;
>  	int ret;
>  
> +	if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO) {
> +		*val = (chan->type == IIO_TEMP) ? sysmon->temp_oversampling :
> +						 sysmon->supply_oversampling;
> +		return IIO_VAL_INT;
> +	}
> +

[Severity: Medium]
Is it safe to read sysmon->temp_oversampling and sysmon->supply_oversampling
without holding the mutex here?

Since sysmon_write_raw() modifies these values under sysmon->lock,
could reading them before the guard(mutex) call introduce a data race
regression?

>  	if (mask != IIO_CHAN_INFO_RAW && mask != IIO_CHAN_INFO_PROCESSED)
>  		return -EINVAL;
>  
>  	guard(mutex)(&sysmon->lock);

[ ... ]

> @@ -849,6 +979,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
>  			.info_mask_separate =
>  				BIT(IIO_CHAN_INFO_RAW) |
>  				BIT(IIO_CHAN_INFO_PROCESSED),
> +			.info_mask_shared_by_type =
> +				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> +			.info_mask_shared_by_type_available =
> +				BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),

[Severity: Medium]
Is it correct to use info_mask_shared_by_type for the satellite temperature
channels?

The hardware writes in sysmon_osr_write() only configure the satellite
channels via SYSMON_TEMP_SAT_CONFIG_MASK, leaving the main core temperature
channel unaffected. Exposing this as a type-wide attribute might incorrectly
imply to userspace that the setting applies to all temperature channels.

Would it be better to use info_mask_separate to avoid a regression in how
userspace configures the main core temperature channel?

>  			.scan_type = {
>  				.sign = 's',
>  				.realbits = 15,

[ ... ]

> @@ -902,6 +1036,8 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
>  
>  	sysmon = iio_priv(indio_dev);
>  	sysmon->regmap = regmap;
> +	sysmon->temp_oversampling = 1;
> +	sysmon->supply_oversampling = 1;

[Severity: Medium]
Are the initial oversampling values synchronized with the hardware state?

The software state is initialized to 1 here, but it doesn't appear to write
these defaults to the device or read the initial state from SYSMON_CONFIG.

Could this cause a regression where the driver and hardware state silently
mismatch if a bootloader left the device in a different state?

>  
>  	ret = devm_mutex_init(dev, &sysmon->lock);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim@amd.com?part=5

Re: [PATCH v3 1/5] dt-bindings: iio: adc: add xlnx,versal-sysmon binding

[Krzysztof Kozlowski]

On Wed, May 27, 2026 at 12:42:07PM +0100, Salih Erim wrote:
> Add devicetree binding for the AMD/Xilinx Versal System Monitor (SysMon).
> 
> The Versal SysMon is the successor to the Zynq UltraScale+ AMS block,
> providing on-chip voltage and temperature monitoring. The hardware
> supports up to 160 supply voltage measurement points and up to 64
> temperature satellites distributed across the SoC, with configurable
> threshold alarms and oversampling. The device can be accessed via
> memory-mapped I/O or via an I2C interface.
> 
> Supply and temperature channels are described as child nodes under
> container nodes, referencing the standard adc.yaml binding for
> channel properties.
> 
> Co-developed-by: Michal Simek <michal.simek@amd.com>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> Signed-off-by: Salih Erim <salih.erim@amd.com>
> ---
> Changes in v3:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [PATCH v3 0/5] iio: adc: add AMD/Xilinx Versal SysMon driver

[Jonathan Cameron]

On Wed, 27 May 2026 12:42:06 +0100
Salih Erim <salih.erim@amd.com> wrote:

> This series adds a new IIO driver for the AMD/Xilinx Versal System
> Monitor (SysMon), providing on-chip voltage and temperature monitoring.
> 
> The Versal SysMon measures up to 160 supply voltages and reads up to
> 64 temperature satellites distributed across the SoC. The hardware
> also provides aggregated device temperature registers: the current
> max and min across all active satellites, and peak/trough values
> recorded since last hardware reset. The device can be accessed via
> memory-mapped I/O or via an I2C interface.
> 
> The driver is split into a bus-agnostic core module using the regmap
> API, an MMIO platform driver, and an I2C driver. This allows the
> same IIO logic to be shared across different bus transports.
> 
> Previous submissions:
>   v2: https://lore.kernel.org/all/cover.1746182670.git.salih.erim@amd.com/
>   v1: https://lore.kernel.org/all/cover.1757061697.git.michal.simek@amd.com/
> 
https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim%40amd.com
Quite a bit of feedback.  Some of which is clearly garbage, like the
ARCH_VERSAL suggestion, but take a close look as it does tend to pick up on
stuff that humans miss.

Re: [PATCH v3 2/5] iio: adc: add Versal SysMon driver

[Jonathan Cameron]

On Wed, 27 May 2026 12:42:08 +0100
Salih Erim <salih.erim@amd.com> wrote:

> Add the core driver and MMIO platform driver for the AMD/Xilinx Versal
> System Monitor (SysMon) block.
> 
> The SysMon block resides in the platform management controller (PMC) and
> provides on-chip voltage and temperature monitoring through a 10-bit,
> 200 kSPS ADC. It can monitor up to 160 voltage channels and 64
> temperature satellites distributed across the SoC, with a consistent
> sample rate of 8 kSPS per channel regardless of how many channels are
> enabled.
> 
> The driver is split into three compilation units:
>   - versal-sysmon-core: Channel parsing, IIO registration, read_raw
>   - versal-sysmon: MMIO platform driver with custom regmap accessors
> 
> Voltage results are stored in a 19-bit modified floating-point format
> and converted to millivolts. Temperature results are stored in Q8.7
> signed fixed-point Celsius format and converted to millicelsius.
> 
> The MMIO regmap backend uses a custom reg_write accessor that
> automatically unlocks the NPI (NoC programming interface) lock
> register before each write, as required by the hardware. The regmap
> is configured with fast_io since the underlying MMIO accessors are
> safe to call from atomic context.
> 
> Co-developed-by: Michal Simek <michal.simek@amd.com>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> Signed-off-by: Salih Erim <salih.erim@amd.com>

Various comments inline.


> ---
> Changes in v3:
>   - IWYU: add array_size.h, string.h, types.h to core; audit and
>     fix header and MMIO driver includes (Andy)
>   - Rename _ext to _name in SYSMON_CHAN_TEMP macro parameter (Andy,
>     Jonathan)
>   - Use .info_mask_separate = BIT() style in SYSMON_CHAN_TEMP (Andy)
>   - Use s16 parameter in sysmon_q8p7_to_millicelsius (Andy)
>   - Use sign_extend32() in sysmon_supply_rawtoprocessed (Andy)
>   - Split sysmon_read_raw parameters logically across lines (Andy)
>   - Remove redundant (int) casts on regval (Andy)
>   - Split num_supply/num_temp initialization (Andy)
>   - Use __free(fwnode_handle) cleanup, remove goto err_put (Andy)
>   - Use size_add() for overflow-safe allocation (Andy)
>   - Use dev_err_probe() in sysmon_parse_fw error paths (Jonathan)
>   - Move fwnode_irq_get() to core_probe, remove irq parameter
>     from bus driver interfaces (Jonathan)
>   - Use (int)MILLI at call sites, drop SYSMON_MILLI define (Andy,
>     Jonathan)
>   - Remove sysmon->dev, sysmon->indio_dev, sysmon->irq from struct;
>     pass as local variables or use regmap_get_device() (Jonathan)
>   - Use struct device *dev local in sysmon_platform_probe (Andy)
>   - Describe protected data in lock comment (Jonathan)
>   - Add comment explaining RAW+PROCESSED co-exposure (Jonathan)

Looking at this again, I'm thinking we don't need them both.
In particular it makes it ambiguous for what scaling of events is
so best to use one or the other.

> 
> Changes in v2:
>   - Split into core (versal-sysmon-core.c) + MMIO platform driver
>     (versal-sysmon.c) + shared header (versal-sysmon.h)
>   - Uses regmap API instead of direct readl/writel
>   - MMIO regmap uses custom callbacks with NPI unlock in write path
>   - Reverse Christmas Tree variable ordering throughout
>   - Header include order fixed
>   - MAINTAINERS entry folded in with wildcard F: pattern
>   - Kconfig: hidden VERSAL_SYSMON_CORE + VERSAL_SYSMON selects it
>   - Kconfig/Makefile: alphabetical ordering (VERSAL before VF610)
>   - Bounds validation on DT reg values
>   - Named constants replace magic numbers (SYSMON_REG_STRIDE,
>     SYSMON_SUPPLY_MANTISSA_BITS, SYSMON_MILLI)
>   - kernel-doc for exported sysmon_core_probe() and sysmon_parse_fw()
>   - Supply voltage conversion uses proper two's complement sign
>     extension (s16 cast) matching the hardware specification
>   - Register offsets sorted by address in header
>   - Each patch introduces only the defines, fields, and includes
>     it uses (no dead code in any commit)
>   - Removed unused linux/limits.h and linux/units.h includes
>   - Renamed iio_dev_info to sysmon_iio_info
>   - regmap_write return values checked in probe init path

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> new file mode 100644
> index 00000000000..ebe052f6982
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Versal SysMon core driver
> + *
> + * Copyright (C) 2019 - 2022, Xilinx, Inc.
> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#include "versal-sysmon.h"
> +
> +/*
> + * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
> + * (which operate in hardware register format), PROCESSED gives userspace
> + * the converted millivolt or millicelsius value.
The conversion seems to be linear.  So you should be providing _SCALE and
maybe _OFFSET to let the users work out any necessary conversion.

I don't yet see a reason to provide PROCESSED for this channel type.

> + */
> +#define SYSMON_CHAN_TEMP(_chan, _address, _name) {		\
> +	.type = IIO_TEMP,					\
> +	.indexed = 1,						\
> +	.address = _address,					\
> +	.channel = _chan,					\
> +	.info_mask_separate =					\
> +		BIT(IIO_CHAN_INFO_RAW) |				\
> +		BIT(IIO_CHAN_INFO_PROCESSED),			\
> +	.scan_type = {						\

This is mainly there for buffered interfaces (chardev) but I don't think
you yet support that so drop it.

> +		.sign = 's',					\
> +		.realbits = 15,					\
> +		.storagebits = 16,				\
> +		.endianness = IIO_CPU,				\
> +	},							\
> +	.datasheet_name = _name,				\
> +}
> +
> +/* Static temperature channels (always present) */
> +static const struct iio_chan_spec temp_channels[] = {
> +	SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
> +	SYSMON_CHAN_TEMP(1, SYSMON_TEMP_MIN, "min"),
> +	SYSMON_CHAN_TEMP(2, SYSMON_TEMP_MAX_MAX, "max_max"),
> +	SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
> +};
> +
> +static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
> +{
> +	*val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;

That's a very simple linear scaling so provide _RAW and get rid of this.

> +}
> +
> +static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
> +{
> +	int mantissa, format, exponent;
> +
> +	mantissa = FIELD_GET(SYSMON_MANTISSA_MASK, raw_data);
> +	exponent = SYSMON_SUPPLY_MANTISSA_BITS - FIELD_GET(SYSMON_MODE_MASK, raw_data);
> +	format = FIELD_GET(SYSMON_FMT_MASK, raw_data);
> +	/*
> +	 * When format bit is set the mantissa is two's complement
> +	 * (per hardware spec); sign-extend to int for correct arithmetic.
> +	 */
> +	if (format)
> +		mantissa = sign_extend32(mantissa, 15);
> +
> +	*val = (mantissa * (int)MILLI) >> exponent;
> +}


> +
> +/**
> + * sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
> + * @indio_dev: IIO device instance
> + * @dev: Parent device
> + *
> + * Reads voltage-channels and temperature-channels container nodes from
> + * firmware and builds the IIO channel array. Static temperature channels
> + * are prepended, followed by supply and satellite channels from DT.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
> +{
> +	struct fwnode_handle *supply_node __free(fwnode_handle) =
> +		device_get_named_child_node(dev, "voltage-channels");
> +	struct fwnode_handle *temp_node __free(fwnode_handle) =
> +		device_get_named_child_node(dev, "temperature-channels");

Move these down to just above each check. Note that when using __free()
it is fine not to have all declarations at the top.

> +	unsigned int num_supply = 0, num_temp = 0;
> +	unsigned int idx, temp_chan_idx, volt_chan_idx;
> +	struct iio_chan_spec *sysmon_channels;
> +	const char *label;
> +	u32 reg;
> +	int ret;
> +
	struct fwnode_handle *supply_node __free(fwnode_handle) =
		device_get_named_child_node(dev, "voltage-channels");
	if (supply_node)
> +	if (supply_node)
> +		num_supply = fwnode_get_child_node_count(supply_node);
	struct fwnode_handle *temp_node __free(fwnode_handle) =
		device_get_named_child_node(dev, "temperature-channels");
	if (temp_node)

> +	if (temp_node)
> +		num_temp = fwnode_get_child_node_count(temp_node);
> +
> +	sysmon_channels = devm_kcalloc(dev,
> +				       size_add(ARRAY_SIZE(temp_channels),
> +						num_supply + num_temp),
> +				       sizeof(*sysmon_channels), GFP_KERNEL);
> +	if (!sysmon_channels)
> +		return -ENOMEM;
> +
> +	/* Static temperature channels first (fixed indices) */
> +	idx = 0;
> +	memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
> +	idx += ARRAY_SIZE(temp_channels);
> +
> +	/* Supply channels from DT */
> +	fwnode_for_each_child_node_scoped(supply_node, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "missing reg for supply channel\n");
> +
> +		if (reg > SYSMON_SUPPLY_IDX_MAX)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "supply reg %u exceeds max %u\n",
> +					     reg, SYSMON_SUPPLY_IDX_MAX);
> +
> +		ret = fwnode_property_read_string(child, "label", &label);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "missing label for supply channel\n");
> +
> +		sysmon_channels[idx++] = (struct iio_chan_spec) {
> +			.type = IIO_VOLTAGE,
> +			.indexed = 1,
> +			.address = reg,
> +			.info_mask_separate =
> +				BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_PROCESSED),
Add a similar comment to the one you have for temperature channels here.
This is tricky because I have no idea how a user would set a floating point
threshold via raw.  Their expectation is that is simple and linear.

How hard is it to take a _PROCESSED event value and convert it back to
a format that can be used for setting the register values? To me that
seems like a much more intuitive interface

From a quick look at the event patch it seems you are doing that? In which case drop raw.

> +			.scan_type = {
> +				.realbits = 19,
> +				.storagebits = 32,
> +				.endianness = IIO_CPU,
> +				.sign = fwnode_property_read_bool(child,
> +					"bipolar") ? 's' : 'u',

As above, this only gets exposed when buffered interfaces are added, so for
now don't set it unless you are using them for some internal purposes.
Also .sign is being replace with .format.

> +			},
> +			.datasheet_name = label,
> +		};
> +	}
> +
> +	/* Temperature satellite channels from DT */
> +	fwnode_for_each_child_node_scoped(temp_node, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "missing reg for temp channel\n");
> +
> +		if (reg < 1 || reg > SYSMON_TEMP_SAT_MAX)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "temp reg %u out of range [1..%u]\n",
> +					     reg, SYSMON_TEMP_SAT_MAX);
> +
> +		ret = fwnode_property_read_string(child, "label", &label);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "missing label for temp channel\n");
> +
> +		sysmon_channels[idx++] = (struct iio_chan_spec) {
> +			.type = IIO_TEMP,
> +			.indexed = 1,
> +			.address = SYSMON_TEMP_SAT_BASE +
> +				   ((reg - 1) * SYSMON_REG_STRIDE),
> +			.info_mask_separate =
> +				BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_PROCESSED),

As above, add a comment on why both (or drop one of them)

> +			.scan_type = {
> +				.sign = 's',
> +				.realbits = 15,
> +				.storagebits = 16,
> +				.endianness = IIO_CPU,
> +			},
> +			.datasheet_name = label,
> +		};
> +	}
> +
> +	indio_dev->num_channels = idx;
> +	indio_dev->info = &sysmon_iio_info;
> +
> +	/*
> +	 * Assign per-type sequential channel numbers.
> +	 * IIO sysfs uses type prefix (in_tempN, in_voltageN)
> +	 * so numbers only need to be unique within each type.
> +	 */
> +	temp_chan_idx = 0;
> +	volt_chan_idx = 0;
> +	for (idx = 0; idx < indio_dev->num_channels; idx++) {
> +		if (sysmon_channels[idx].type == IIO_TEMP)
> +			sysmon_channels[idx].channel = temp_chan_idx++;
> +		else
> +			sysmon_channels[idx].channel = volt_chan_idx++;
> +	}
> +
> +	indio_dev->channels = sysmon_channels;
> +
> +	return 0;
> +}

> diff --git a/drivers/iio/adc/versal-sysmon.c b/drivers/iio/adc/versal-sysmon.c
> new file mode 100644
> index 00000000000..8473288e7db
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Versal SysMon MMIO platform driver
> + *
> + * Copyright (C) 2019 - 2022, Xilinx, Inc.
> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "versal-sysmon.h"
> +
> +struct sysmon_mmio {
> +	void __iomem *base;
> +};
> +
> +static int sysmon_mmio_reg_read(void *context, unsigned int reg,
> +				unsigned int *val)
> +{
> +	struct sysmon_mmio *mmio = context;
> +
> +	*val = readl(mmio->base + reg);

Blank line before simple returns slightly helps readabilty.

> +	return 0;
> +}

> +static int sysmon_platform_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sysmon_mmio *mmio;
> +	struct regmap *regmap;
> +
> +	mmio = devm_kzalloc(dev, sizeof(*mmio), GFP_KERNEL);
> +	if (!mmio)
> +		return -ENOMEM;
> +
> +	mmio->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mmio->base))
> +		return PTR_ERR(mmio->base);
> +
> +	regmap = devm_regmap_init(dev, NULL, mmio,
> +				  &sysmon_mmio_regmap_config);

Fits on one line I think even with a strict 80 char limit (which we relax
when readabilty is hurt)

> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	return sysmon_core_probe(dev, regmap);
> +}

> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
> new file mode 100644
> index 00000000000..d24d2481915
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon.h

...

> +
> +/* Q8.7 fractional shift */
> +#define SYSMON_FRACTIONAL_SHIFT		7U

This should perhaps be a mask then use FIELD_GET() to extract the value.

> +#define SYSMON_SUPPLY_MANTISSA_BITS	16
> +
> +/**
> + * struct sysmon - Driver data for Versal SysMon
> + * @regmap: register map for hardware access
> + * @lock: protects regmap access
> + */
> +struct sysmon {
> +	struct regmap *regmap;
> +	/* Protects regmap access */

regmap has it's own internal locks. So this comment needs to explain
in more detail what is being protected.  I assume read modify write
or long related sequences that must not be interrupted and aren't encapsulated
in single regmap calls?

> +	struct mutex lock;
> +};

Re: [PATCH v3 3/5] iio: adc: versal-sysmon: add I2C driver

[Jonathan Cameron]

On Wed, 27 May 2026 12:42:09 +0100
Salih Erim <salih.erim@amd.com> wrote:

> Add an I2C transport driver for the Versal SysMon block. The SysMon
> provides an I2C slave interface that allows an external master to
> read voltage and temperature measurements through the same register
> map used by the MMIO path.
> 
> The I2C command frame is an 8-byte structure containing a 4-byte data
> payload, a 2-byte register offset, and a 1-byte instruction field.
> Read operations send the frame with a read instruction, then receive
> a 4-byte response containing the register value.
> 
> Events are not supported on the I2C path because there is no
> interrupt line and the I2C regmap backend cannot be called from
> atomic context.
> 
> Co-developed-by: Conall O'Griofa <conall.ogriofa@amd.com>
> Signed-off-by: Conall O'Griofa <conall.ogriofa@amd.com>
> Signed-off-by: Salih Erim <salih.erim@amd.com>
A few minor things inline.

> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
> new file mode 100644
> index 00000000000..92d149f517e
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon-i2c.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Versal SysMon I2C driver
> + *
> + * Copyright (C) 2023 - 2026, Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include "versal-sysmon.h"
> +
> +#define SYSMON_I2C_INSTR_READ	BIT(2)
> +#define SYSMON_I2C_INSTR_WRITE	BIT(3)
> +
> +#define SYSMON_I2C_DATA0_MASK	GENMASK(7, 0)
> +#define SYSMON_I2C_DATA1_MASK	GENMASK(15, 8)
> +#define SYSMON_I2C_DATA2_MASK	GENMASK(23, 16)
> +#define SYSMON_I2C_DATA3_MASK	GENMASK(31, 24)
> +
> +#define SYSMON_I2C_OFS_LOW_MASK		GENMASK(9, 2)
> +#define SYSMON_I2C_OFS_HIGH_MASK	GENMASK(15, 10)
> +
> +/* Byte positions within the 8-byte I2C command frame (HW-defined) */
> +enum sysmon_i2c_payload_idx {
> +	SYSMON_I2C_DATA0_IDX = 0,
> +	SYSMON_I2C_DATA1_IDX = 1,
> +	SYSMON_I2C_DATA2_IDX = 2,
> +	SYSMON_I2C_DATA3_IDX = 3,
> +	SYSMON_I2C_OFS_LOW_IDX = 4,
> +	SYSMON_I2C_OFS_HIGH_IDX = 5,

With changes suggested below I think you only need the two base
offsets and the final one.  As such maybe 3 defines makes more sense
than an enum

> +	SYSMON_I2C_INSTR_IDX = 6,
If you do keep an enum, it would be good to add an entry for the final
byte to give some indication of why it is 8 bytes. Even if that is
reserved0
> +};
> +
> +static int sysmon_i2c_reg_read(void *context, unsigned int reg,
> +			       unsigned int *val)
> +{
> +	struct i2c_client *client = context;
> +	u8 write_buf[8] = { };
> +	u8 read_buf[4];
> +	int ret;
> +
> +	write_buf[SYSMON_I2C_OFS_LOW_IDX] =
> +		FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
> +	write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
> +		FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);

I'd guess an unaligned put works here a well? though you'll need
to do a FIELD_GET() to extract the slightly shifted content.

> +	write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_READ;
> +
> +	ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(write_buf))
> +		return -EIO;
> +
> +	ret = i2c_master_recv(client, read_buf, sizeof(read_buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(read_buf))
> +		return -EIO;
> +
> +	*val = FIELD_PREP(SYSMON_I2C_DATA0_MASK,
> +			  read_buf[SYSMON_I2C_DATA0_IDX]) |
> +	       FIELD_PREP(SYSMON_I2C_DATA1_MASK,
> +			  read_buf[SYSMON_I2C_DATA1_IDX]) |
> +	       FIELD_PREP(SYSMON_I2C_DATA2_MASK,
> +			  read_buf[SYSMON_I2C_DATA2_IDX]) |
> +	       FIELD_PREP(SYSMON_I2C_DATA3_MASK,
> +			  read_buf[SYSMON_I2C_DATA3_IDX]);
Very complex way to express what I think is
	*val = get_unaligned_le32(&read_buf[0]);
> +
> +	return 0;
> +}
> +
> +static int sysmon_i2c_reg_write(void *context, unsigned int reg,
> +				unsigned int val)
> +{
> +	struct i2c_client *client = context;
> +	u8 write_buf[8] = { };

> +	int ret;
> +
> +	write_buf[SYSMON_I2C_DATA0_IDX] =
> +		FIELD_GET(SYSMON_I2C_DATA0_MASK, val);
> +	write_buf[SYSMON_I2C_DATA1_IDX] =
> +		FIELD_GET(SYSMON_I2C_DATA1_MASK, val);
> +	write_buf[SYSMON_I2C_DATA2_IDX] =
> +		FIELD_GET(SYSMON_I2C_DATA2_MASK, val);
> +	write_buf[SYSMON_I2C_DATA3_IDX] =
> +		FIELD_GET(SYSMON_I2C_DATA3_MASK, val);

That's a put_unaligned_le32() I think?

> +	write_buf[SYSMON_I2C_OFS_LOW_IDX] =
> +		FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
> +	write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
> +		FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);

I'd guess put_unaligned_le16()? Will need a a FIELD_PREP()
for the full thing though.

> +	write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_WRITE;
> +
> +	ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(write_buf))
> +		return -EIO;
> +
> +	return 0;
> +}

> +static const struct of_device_id sysmon_i2c_of_match_table[] = {
> +	{ .compatible = "xlnx,versal-sysmon" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sysmon_i2c_of_match_table);
> +
> +static const struct i2c_device_id sysmon_i2c_id_table[] = {
> +	{ "versal-sysmon" },

Named initializer for this.  Uwe is cleaning these up across IIO;
let us not add another one!

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, sysmon_i2c_id_table);
> +
> +static struct i2c_driver sysmon_i2c_driver = {
> +	.probe = sysmon_i2c_probe,
> +	.driver = {
> +		.name = "versal-sysmon-i2c",
> +		.of_match_table = sysmon_i2c_of_match_table,
> +	},
> +	.id_table = sysmon_i2c_id_table,
> +};
> +module_i2c_driver(sysmon_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("AMD Versal SysMon I2C Driver");
> +MODULE_AUTHOR("Conall O'Griofa <conall.ogriofa@amd.com>");
> +MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");

Re: [PATCH v3 4/5] iio: adc: versal-sysmon: add threshold event support

[Jonathan Cameron]

On Wed, 27 May 2026 12:42:10 +0100
Salih Erim <salih.erim@amd.com> wrote:

> Add threshold event support for temperature and supply voltage
> channels.
> 
> Temperature events:
>   - Rising threshold with configurable value
>   - Over-temperature (OT) alarm with separate threshold
>   - Per-channel hysteresis as a millicelsius value
>   - Event direction is IIO_EV_DIR_RISING (hysteresis mode)
> 
> Supply voltage events:
>   - Rising/falling threshold per supply channel
>   - Per-channel alarm enable via alarm configuration registers
> 
> The hardware supports both window and hysteresis alarm modes for
> temperature. This driver uses hysteresis mode, where the upper
> threshold triggers the alarm and the lower threshold clears it
> (re-arm point). The hardware has a single ISR bit per temperature
> channel with no indication of which threshold was crossed, so
> hysteresis mode is the natural fit. The lower threshold register
> is computed internally as (upper - hysteresis).
> 
> Hysteresis is stored in the driver as a millicelsius value,
> initialized from the hardware registers at probe. Writing the
> rising threshold or hysteresis recomputes the lower register.
> ALARM_CONFIG is hard-coded to hysteresis mode during init.
> 
> The interrupt handler masks active threshold interrupts (which are
> level-sensitive) and schedules a delayed worker to poll for condition
> clear before unmasking. When no hardware IRQ is available, event
> channels are not created and interrupt init is skipped, since the
> I2C regmap backend cannot be called from atomic context.
> 
> When disabling a supply channel alarm, the group interrupt remains
> active if any other channel in the same alarm group still has an
> alarm enabled.
> 
> Signed-off-by: Salih Erim <salih.erim@amd.com>
Hi Salih

Main thing in here is related to earlier question on _PROCESSED + _RAW.
I can't see a reason to have _RAW.

Various minor things inline. In many case they apply in other places I haven't
called out so look for repeats

Jonathan

>  drivers/iio/adc/versal-sysmon-core.c | 655 ++++++++++++++++++++++++++-
>  drivers/iio/adc/versal-sysmon.h      |  48 +-
>  2 files changed, 697 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index ebe052f6982..04977c9c887 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> @@ -11,6 +11,8 @@
>  #include <linux/bitops.h>
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
> +#include <linux/devm-helpers.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
> @@ -18,10 +20,19 @@
>  #include <linux/sysfs.h>
>  #include <linux/units.h>
>  
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  
>  #include "versal-sysmon.h"
>  
> +/* OT and TEMP hysteresis mode bits in SYSMON_TEMP_EV_CFG */
> +#define SYSMON_OT_HYST_MASK		BIT(0)
> +#define SYSMON_TEMP_HYST_MASK		BIT(1)
> +
> +/* Compute alarm register offset from a channel address */
> +#define SYSMON_ALARM_OFFSET(addr) \
> +	(SYSMON_ALARM_REG + ((addr) / SYSMON_ALARM_BITS_PER_REG) * SYSMON_REG_STRIDE)
> +
>  /*
>   * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
>   * (which operate in hardware register format), PROCESSED gives userspace
> @@ -44,6 +55,62 @@
>  	.datasheet_name = _name,				\
>  }
>  
> +#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _name, _events) {\
> +	.type = IIO_TEMP,					\
> +	.indexed = 1,						\

Why do you needs separate channel for events?  Can't we add the event
spec to existing channels? For the constant ones you may need to
have two arrays to pick between depending on whether the irq is available or not.


> +	.address = _address,					\
> +	.channel = _chan,					\
> +	.event_spec = _events,					\
> +	.num_event_specs = ARRAY_SIZE(_events),			\
> +	.scan_type = {						\
> +		.sign = 's',					\
> +		.realbits = 15,					\
> +		.storagebits = 16,				\
> +		.endianness = IIO_CPU,				\
> +	},							\
> +	.datasheet_name = _name,				\
> +}
> +
	*raw_data = (u16)tmp;
> +}

> +
> +/*
> + * Recompute the lower threshold register from upper threshold and
> + * cached hysteresis. Called when either upper threshold or hysteresis
> + * is written.
> + */
> +static int sysmon_update_temp_lower(struct sysmon *sysmon, int address)
> +{
> +	unsigned int upper_reg;
> +	int upper_mc, lower_mc, hysteresis;
> +	u32 raw_val;
> +	int upper_off, lower_off, ret;
> +
> +	upper_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_RISING);
> +	if (upper_off < 0)
> +		return upper_off;
> +	lower_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_FALLING);
> +	if (lower_off < 0)
> +		return lower_off;
> +
> +	if (address == SYSMON_ADDR_OT_EVENT)
> +		hysteresis = sysmon->ot_hysteresis;
> +	else
> +		hysteresis = sysmon->temp_hysteresis;
> +
> +	ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
> +	if (ret)
> +		return ret;
> +	sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
> +
> +	lower_mc = upper_mc - hysteresis;
> +	sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);

So all the manipulation is in the units of _PROCESSED. Hence
I'd drop _RAW.

> +
> +	return regmap_write(sysmon->regmap, lower_off, raw_val);
> +}
> +
> +static int sysmon_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info,
> +				   int *val, int *val2)
> +{
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	int offset;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	if (chan->type == IIO_TEMP) {
> +		if (info == IIO_EV_INFO_VALUE) {
> +			/* Only rising threshold is exposed */
> +			offset = sysmon_temp_thresh_offset(chan->address,
> +							   IIO_EV_DIR_RISING);
> +			if (offset < 0)
> +				return offset;
I'd add a blank line here.
> +			ret = regmap_read(sysmon->regmap, offset, &reg_val);
> +			if (ret)
> +				return ret;
here
> +			sysmon_q8p7_to_millicelsius(reg_val, val);
and here

Generally keep block of call + error check for one thing separate from code
before and after.  Slightly nicer to read.

Lots of other places this applies but it's a very minor thing.

> +			return IIO_VAL_INT;
> +		}
> +		if (info == IIO_EV_INFO_HYSTERESIS) {
> +			if (chan->address == SYSMON_ADDR_OT_EVENT)
> +				*val = sysmon->ot_hysteresis;
> +			else
> +				*val = sysmon->temp_hysteresis;
> +			return IIO_VAL_INT;
> +		}
> +	}
> +
> +	if (chan->type == IIO_VOLTAGE) {
> +		offset = sysmon_supply_thresh_offset(chan->address, dir);
> +		if (offset < 0)
> +			return offset;
> +		ret = regmap_read(sysmon->regmap, offset, &reg_val);
> +		if (ret)
> +			return ret;
> +		sysmon_supply_rawtoprocessed(reg_val, val);
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int sysmon_write_event_value(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info,
> +				    int val, int val2)
> +{
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	u32 raw_val;
> +	int offset;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);

> +
> +	if (chan->type == IIO_TEMP) {
> +		if (info == IIO_EV_INFO_VALUE) {
> +			/* Only rising threshold is exposed */
> +			offset = sysmon_temp_thresh_offset(chan->address,
> +							   IIO_EV_DIR_RISING);
> +			if (offset < 0)
> +				return offset;
> +			sysmon_millicelsius_to_q8p7(&raw_val, val);
> +			ret = regmap_write(sysmon->regmap, offset, raw_val);
> +			if (ret)
> +				return ret;
> +			/* Recompute lower = upper - hysteresis */
> +			return sysmon_update_temp_lower(sysmon,
> +							chan->address);

Under 80 chars on one line.  Check for cases of this and feel free to go a bit
over if it helps readability.

> +			return sysmon_update_temp_lower(sysmon, chan->address);

> +		}
> +		if (info == IIO_EV_INFO_HYSTERESIS) {
> +			if (val < 0)
> +				return -EINVAL;
> +			if (chan->address == SYSMON_ADDR_OT_EVENT)
> +				sysmon->ot_hysteresis = val;
> +			else
> +				sysmon->temp_hysteresis = val;
> +			return sysmon_update_temp_lower(sysmon,
> +							chan->address);

As above.

> +		}
> +	}
> +
> +	if (chan->type == IIO_VOLTAGE) {
> +		offset = sysmon_supply_thresh_offset(chan->address, dir);
> +		if (offset < 0)
> +			return offset;
> +		ret = regmap_read(sysmon->regmap, offset, &reg_val);
> +		if (ret)
> +			return ret;
> +		sysmon_supply_processedtoraw(val, reg_val, &raw_val);
> +		return regmap_write(sysmon->regmap, offset, raw_val);
> +	}
> +
> +	return -EINVAL;
> +}

> +static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
> +{
> +	u32 alarm_flag_offset = SYSMON_ALARM_FLAG + (event * SYSMON_REG_STRIDE);
> +	u32 alarm_reg_offset = SYSMON_ALARM_REG + (event * SYSMON_REG_STRIDE);
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned long alarm_flag_reg;
> +	unsigned int reg_val;
> +	u32 address, bit;
> +	int ret;
> +
> +	switch (event) {
> +	case SYSMON_BIT_TEMP:
> +		sysmon_push_event(indio_dev, SYSMON_ADDR_TEMP_EVENT);
> +		ret = regmap_write(sysmon->regmap, SYSMON_IDR,
> +				   BIT(SYSMON_BIT_TEMP));
> +		if (ret)
> +			return ret;
> +		sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
> +		break;
> +
> +	case SYSMON_BIT_OT:
> +		sysmon_push_event(indio_dev, SYSMON_ADDR_OT_EVENT);
> +		ret = regmap_write(sysmon->regmap, SYSMON_IDR,
> +				   BIT(SYSMON_BIT_OT));
> +		if (ret)
> +			return ret;
> +		sysmon->masked_temp |= BIT(SYSMON_BIT_OT);
> +		break;
> +
> +	case SYSMON_BIT_ALARM0:
> +	case SYSMON_BIT_ALARM1:
> +	case SYSMON_BIT_ALARM2:
> +	case SYSMON_BIT_ALARM3:
> +	case SYSMON_BIT_ALARM4:
> +		ret = regmap_read(sysmon->regmap, alarm_flag_offset, &reg_val);
> +		if (ret)
> +			return ret;
> +		alarm_flag_reg = reg_val;
> +
> +		for_each_set_bit(bit, &alarm_flag_reg,
> +				 SYSMON_ALARM_BITS_PER_REG) {
> +			address = bit + (SYSMON_ALARM_BITS_PER_REG * event);
> +			sysmon_push_event(indio_dev, address);
> +			ret = regmap_update_bits(sysmon->regmap,
> +						 alarm_reg_offset,
> +						 BIT(bit), 0);
> +			if (ret)
> +				return ret;
> +		}
> +		ret = regmap_write(sysmon->regmap, alarm_flag_offset,
> +				   alarm_flag_reg);
> +		if (ret)
> +			return ret;
> +		break;
		return regmap_write();
> +
> +	default:
> +		break;

Why is this not an error?

> +	}
> +
> +	return 0;
Might as well return early in the various paths.
> +}
...

> +static irqreturn_t sysmon_iio_irq(int irq, void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct sysmon *sysmon;
> +	unsigned int isr, imr;
> +
> +	sysmon = iio_priv(indio_dev);
> +	spin_lock(&sysmon->irq_lock);

guard() here would eman you can just return if (!isr)

> +
> +	regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
> +	regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
> +
> +	isr &= ~imr;
> +	regmap_write(sysmon->regmap, SYSMON_ISR, isr);
> +
> +	if (isr) {
> +		sysmon_handle_events(indio_dev, isr);
> +		schedule_delayed_work(&sysmon->sysmon_unmask_work,
> +				      msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
> +	}
> +
> +	spin_unlock(&sysmon->irq_lock);
> +
> +	return IRQ_RETVAL(isr);
> +}

Re: [PATCH v3 5/5] iio: adc: versal-sysmon: add oversampling support

[Jonathan Cameron]

On Wed, 27 May 2026 12:42:11 +0100
Salih Erim <salih.erim@amd.com> wrote:

> Add support for reading and writing the oversampling ratio through
> the IIO oversampling_ratio attribute. The hardware supports averaging
> 2, 4, 8, or 16 samples, plus a ratio of 1 (no averaging).
> 
> Temperature and supply channels share oversampling configuration at
> the type level (all temperature channels share one ratio, all supply
> channels share another), exposed through info_mask_shared_by_type.
> 
> The hardware encoding uses sample_count / 2 in a 4-bit field within
> the CONFIG register. Per-channel averaging enable registers must also
> be updated to activate or deactivate averaging.
> 
> Signed-off-by: Salih Erim <salih.erim@amd.com>
Really minor stuff from a reread

> +static int sysmon_osr_write(struct sysmon *sysmon, int channel_type, int val)
> +{
> +	/*
> +	 * HW register encoding is sample_count / 2:
> +	 * 0=none, 1=2x, 2=4x, 4=8x, 8=16x (not log2-based).
> +	 */
> +	int hw_val = val >> 1;
> +	int ret;
> +
> +	if (channel_type == IIO_TEMP) {
> +		ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
> +					SYSMON_TEMP_SAT_CONFIG_MASK,
> +					FIELD_PREP(SYSMON_TEMP_SAT_CONFIG_MASK,
> +						   hw_val));
> +		if (ret)
> +			return ret;
blank line
> +		ret = sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
> +					    SYSMON_TEMP_EN_AVG_COUNT,
> +					    hw_val ? ~0U : 0);
> +		if (ret)
> +			return ret;
		return sysmon_set...

> +	} else if (channel_type == IIO_VOLTAGE) {
Won't need the else if returned already.
> +		ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
> +					SYSMON_SUPPLY_CONFIG_MASK,
> +					FIELD_PREP(SYSMON_SUPPLY_CONFIG_MASK,
> +						   hw_val));
> +		if (ret)
> +			return ret;
blank line
> +		ret = sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
> +					    SYSMON_SUPPLY_EN_AVG_COUNT,
> +					    hw_val ? ~0U : 0);
> +		if (ret)
> +			return ret;
		return sysmon_set...

> +	} else {
No else needed here either

> +		return -EINVAL;
> +	}
> +
> +	return 0
And this isn't needed at all.

> +}

> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
> index a78362f95e6..cf69be62709 100644
> --- a/drivers/iio/adc/versal-sysmon.h
> +++ b/drivers/iio/adc/versal-sysmon.h
> @@ -25,11 +25,13 @@ struct regmap;
>  #define SYSMON_IMR			0x0048
>  #define SYSMON_IER			0x004C
>  #define SYSMON_IDR			0x0050
> +#define SYSMON_CONFIG			0x0100
>  #define SYSMON_TEMP_MAX			0x1030
>  #define SYSMON_TEMP_MIN			0x1034
>  #define SYSMON_SUPPLY_BASE		0x1040
>  #define SYSMON_ALARM_FLAG		0x1018
>  #define SYSMON_ALARM_REG		0x1940
> +#define SYSMON_SUPPLY_EN_AVG_BASE	0x1958
>  #define SYSMON_TEMP_TH_LOW		0x1970
>  #define SYSMON_TEMP_TH_UP		0x1974
>  #define SYSMON_OT_TH_LOW		0x1978
> @@ -41,6 +43,7 @@ struct regmap;
>  #define SYSMON_TEMP_MAX_MAX		0x1F90
>  #define SYSMON_STATUS_RESET		0x1F94
>  #define SYSMON_TEMP_SAT_BASE		0x1FAC
> +#define SYSMON_TEMP_EN_AVG_BASE		0x24B4
>  #define SYSMON_MAX_REG			0x24C0
>  
>  /* NPI unlock value written to SYSMON_NPI_LOCK */
> @@ -57,6 +60,16 @@ struct regmap;
>  /* ISR/IMR temperature and OT alarm mask (bits 9:8) */
>  #define SYSMON_TEMP_INTR_MASK		GENMASK(9, 8)
>  
> +/* Config register: supply oversampling field (bits 17:14) */
> +#define SYSMON_SUPPLY_CONFIG_MASK	GENMASK(17, 14)
> +
> +/* Config register: temp satellite oversampling field (bits 27:24) */

I missed this before, but given the GENMASK just below the bits part
of these comments is pointles. Drop it.

Ideally also name them in a way that makes it clear what register
they are fields of.

> +#define SYSMON_TEMP_SAT_CONFIG_MASK	GENMASK(27, 24)

Re: [PATCH v3 0/5] iio: adc: add AMD/Xilinx Versal SysMon driver

[Erim, Salih]

Hi Jonathan,

On 28/05/2026 13:06, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:06 +0100
> Salih Erim <salih.erim@amd.com> wrote:
> 
>> This series adds a new IIO driver for the AMD/Xilinx Versal System
>> Monitor (SysMon), providing on-chip voltage and temperature monitoring.
>>
>> The Versal SysMon measures up to 160 supply voltages and reads up to
>> 64 temperature satellites distributed across the SoC. The hardware
>> also provides aggregated device temperature registers: the current
>> max and min across all active satellites, and peak/trough values
>> recorded since last hardware reset. The device can be accessed via
>> memory-mapped I/O or via an I2C interface.
>>
>> The driver is split into a bus-agnostic core module using the regmap
>> API, an MMIO platform driver, and an I2C driver. This allows the
>> same IIO logic to be shared across different bus transports.
>>
>> Previous submissions:
>>    v2: https://lore.kernel.org/all/cover.1746182670.git.salih.erim@amd.com/
>>    v1: https://lore.kernel.org/all/cover.1757061697.git.michal.simek@amd.com/
>>
> https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim%40amd.com
> Quite a bit of feedback.  Some of which is clearly garbage, like the
> ARCH_VERSAL suggestion, but take a close look as it does tend to pick up on
> stuff that humans miss.

Thanks Jonathan. I've gone through all the Sashiko findings.

False positives:
- ARCH_VERSAL: doesn't exist in mainline; all Versal drivers use
   ARCH_ZYNQMP (EDAC_VERSAL, Versal TRNG, etc.)
- I2C bus atomicity: the SysMon I2C slave protocol requires a
   STOP between command and response; the vendor driver uses the
   same send/recv sequence
- Hardirq on I2C: IRQ handler is never registered on the I2C
   path (has_irq is false when fwnode_irq_get returns negative)
- Voltage threshold clobbering format bits: threshold registers
   only use the mantissa (bits 15:0); vendor driver does the same
- IRQ storm on regmap error: ISR is cleared before handle_events
   is called, so the interrupt won't re-fire
- VERSAL_SYSMON_I2C missing ARCH: no I2C ADC driver in the
   kernel has an ARCH dependency; I2C clients are bus-agnostic

Reviewed but keeping as-is:
- Left-shift of negative in millicelsius_to_q8p7: GCC defines
   this behavior and it's consistent with the read direction
   (right-shift); Andy asked for this symmetry in v2
- Oversampling read without mutex: reading a single int is
   atomic on arm64; adding contention for no practical benefit
- Voltage alarms one-shot: follows the IIO event model where
   userspace re-arms via write_event_config
- Event config reads 0 during masked alarm: standard behavior,
   xilinx-ams does the same
- TOCTOU on masked_temp after spinlock drop: benign; worst case
   the worker reschedules one extra cycle
- Interrupts not disabled on teardown: devm_request_irq frees
   the IRQ handler; probe already disables all interrupts via
   IDR at init, and LIFO devm ordering is correct
- Oversampling shared_by_type for all temp: this is a per-type
   HW setting; static channels report the aggregate result
- val*scale overflow in processedtoraw: realistic voltages
   (< 3.3V) are well within int32 range; overflow requires
   > 32V input which is beyond the ADC range
- sysmon_update_temp_lower underflow: hysteresis is validated
   as non-negative and realistic thresholds keep the subtraction
   well within Q8.7 range

Will address in v4:
- scan_type: dropping entirely (no buffered support), which
   also resolves the realbits question
- RAW + PROCESSED: dropping RAW for voltage (PROCESSED only),
   and using RAW + SCALE for temperature (linear Q8.7)
- temp_mask: take irq_lock in write_event_config to
   synchronize with the unmask worker

Happy to fold these into v4 along with your review feedback.

Thanks,
Salih

Re: [PATCH v3 2/5] iio: adc: add Versal SysMon driver

[Erim, Salih]

Hi Jonathan,

On 28/05/2026 13:24, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:08 +0100
> Salih Erim <salih.erim@amd.com> wrote:
> 
>> Add the core driver and MMIO platform driver for the AMD/Xilinx Versal
>> System Monitor (SysMon) block.
>>
>> The SysMon block resides in the platform management controller (PMC) and
>> provides on-chip voltage and temperature monitoring through a 10-bit,
>> 200 kSPS ADC. It can monitor up to 160 voltage channels and 64
>> temperature satellites distributed across the SoC, with a consistent
>> sample rate of 8 kSPS per channel regardless of how many channels are
>> enabled.
>>
>> The driver is split into three compilation units:
>>    - versal-sysmon-core: Channel parsing, IIO registration, read_raw
>>    - versal-sysmon: MMIO platform driver with custom regmap accessors
>>
>> Voltage results are stored in a 19-bit modified floating-point format
>> and converted to millivolts. Temperature results are stored in Q8.7
>> signed fixed-point Celsius format and converted to millicelsius.
>>
>> The MMIO regmap backend uses a custom reg_write accessor that
>> automatically unlocks the NPI (NoC programming interface) lock
>> register before each write, as required by the hardware. The regmap
>> is configured with fast_io since the underlying MMIO accessors are
>> safe to call from atomic context.
>>
>> Co-developed-by: Michal Simek <michal.simek@amd.com>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
> 
> Various comments inline.
> 
> 
>> ---
>> Changes in v3:
>>    - IWYU: add array_size.h, string.h, types.h to core; audit and
>>      fix header and MMIO driver includes (Andy)
>>    - Rename _ext to _name in SYSMON_CHAN_TEMP macro parameter (Andy,
>>      Jonathan)
>>    - Use .info_mask_separate = BIT() style in SYSMON_CHAN_TEMP (Andy)
>>    - Use s16 parameter in sysmon_q8p7_to_millicelsius (Andy)
>>    - Use sign_extend32() in sysmon_supply_rawtoprocessed (Andy)
>>    - Split sysmon_read_raw parameters logically across lines (Andy)
>>    - Remove redundant (int) casts on regval (Andy)
>>    - Split num_supply/num_temp initialization (Andy)
>>    - Use __free(fwnode_handle) cleanup, remove goto err_put (Andy)
>>    - Use size_add() for overflow-safe allocation (Andy)
>>    - Use dev_err_probe() in sysmon_parse_fw error paths (Jonathan)
>>    - Move fwnode_irq_get() to core_probe, remove irq parameter
>>      from bus driver interfaces (Jonathan)
>>    - Use (int)MILLI at call sites, drop SYSMON_MILLI define (Andy,
>>      Jonathan)
>>    - Remove sysmon->dev, sysmon->indio_dev, sysmon->irq from struct;
>>      pass as local variables or use regmap_get_device() (Jonathan)
>>    - Use struct device *dev local in sysmon_platform_probe (Andy)
>>    - Describe protected data in lock comment (Jonathan)
>>    - Add comment explaining RAW+PROCESSED co-exposure (Jonathan)
> 
> Looking at this again, I'm thinking we don't need them both.
> In particular it makes it ambiguous for what scaling of events is
> so best to use one or the other.

Agreed.

> 
>>
>> Changes in v2:
>>    - Split into core (versal-sysmon-core.c) + MMIO platform driver
>>      (versal-sysmon.c) + shared header (versal-sysmon.h)
>>    - Uses regmap API instead of direct readl/writel
>>    - MMIO regmap uses custom callbacks with NPI unlock in write path
>>    - Reverse Christmas Tree variable ordering throughout
>>    - Header include order fixed
>>    - MAINTAINERS entry folded in with wildcard F: pattern
>>    - Kconfig: hidden VERSAL_SYSMON_CORE + VERSAL_SYSMON selects it
>>    - Kconfig/Makefile: alphabetical ordering (VERSAL before VF610)
>>    - Bounds validation on DT reg values
>>    - Named constants replace magic numbers (SYSMON_REG_STRIDE,
>>      SYSMON_SUPPLY_MANTISSA_BITS, SYSMON_MILLI)
>>    - kernel-doc for exported sysmon_core_probe() and sysmon_parse_fw()
>>    - Supply voltage conversion uses proper two's complement sign
>>      extension (s16 cast) matching the hardware specification
>>    - Register offsets sorted by address in header
>>    - Each patch introduces only the defines, fields, and includes
>>      it uses (no dead code in any commit)
>>    - Removed unused linux/limits.h and linux/units.h includes
>>    - Renamed iio_dev_info to sysmon_iio_info
>>    - regmap_write return values checked in probe init path
> 
>> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
>> new file mode 100644
>> index 00000000000..ebe052f6982
>> --- /dev/null
>> +++ b/drivers/iio/adc/versal-sysmon-core.c
>> @@ -0,0 +1,311 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD Versal SysMon core driver
>> + *
>> + * Copyright (C) 2019 - 2022, Xilinx, Inc.
>> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/string.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/units.h>
>> +
>> +#include <linux/iio/iio.h>
>> +
>> +#include "versal-sysmon.h"
>> +
>> +/*
>> + * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
>> + * (which operate in hardware register format), PROCESSED gives userspace
>> + * the converted millivolt or millicelsius value.
> The conversion seems to be linear.  So you should be providing _SCALE and
> maybe _OFFSET to let the users work out any necessary conversion.
> 
> I don't yet see a reason to provide PROCESSED for this channel type.

Accepted. Will switch temperature to RAW + SCALE.

> 
>> + */
>> +#define SYSMON_CHAN_TEMP(_chan, _address, _name) {           \
>> +     .type = IIO_TEMP,                                       \
>> +     .indexed = 1,                                           \
>> +     .address = _address,                                    \
>> +     .channel = _chan,                                       \
>> +     .info_mask_separate =                                   \
>> +             BIT(IIO_CHAN_INFO_RAW) |                                \
>> +             BIT(IIO_CHAN_INFO_PROCESSED),                   \
>> +     .scan_type = {                                          \
> 
> This is mainly there for buffered interfaces (chardev) but I don't think
> you yet support that so drop it.

Accepted.

> 
>> +             .sign = 's',                                    \
>> +             .realbits = 15,                                 \
>> +             .storagebits = 16,                              \
>> +             .endianness = IIO_CPU,                          \
>> +     },                                                      \
>> +     .datasheet_name = _name,                                \
>> +}
>> +
>> +/* Static temperature channels (always present) */
>> +static const struct iio_chan_spec temp_channels[] = {
>> +     SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
>> +     SYSMON_CHAN_TEMP(1, SYSMON_TEMP_MIN, "min"),
>> +     SYSMON_CHAN_TEMP(2, SYSMON_TEMP_MAX_MAX, "max_max"),
>> +     SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
>> +};
>> +
>> +static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
>> +{
>> +     *val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;
> 
> That's a very simple linear scaling so provide _RAW and get rid of this.

Accepted.

> 
>> +}
>> +
>> +static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
>> +{
>> +     int mantissa, format, exponent;
>> +
>> +     mantissa = FIELD_GET(SYSMON_MANTISSA_MASK, raw_data);
>> +     exponent = SYSMON_SUPPLY_MANTISSA_BITS - FIELD_GET(SYSMON_MODE_MASK, raw_data);
>> +     format = FIELD_GET(SYSMON_FMT_MASK, raw_data);
>> +     /*
>> +      * When format bit is set the mantissa is two's complement
>> +      * (per hardware spec); sign-extend to int for correct arithmetic.
>> +      */
>> +     if (format)
>> +             mantissa = sign_extend32(mantissa, 15);
>> +
>> +     *val = (mantissa * (int)MILLI) >> exponent;
>> +}
> 
> 
>> +
>> +/**
>> + * sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
>> + * @indio_dev: IIO device instance
>> + * @dev: Parent device
>> + *
>> + * Reads voltage-channels and temperature-channels container nodes from
>> + * firmware and builds the IIO channel array. Static temperature channels
>> + * are prepended, followed by supply and satellite channels from DT.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
>> +{
>> +     struct fwnode_handle *supply_node __free(fwnode_handle) =
>> +             device_get_named_child_node(dev, "voltage-channels");
>> +     struct fwnode_handle *temp_node __free(fwnode_handle) =
>> +             device_get_named_child_node(dev, "temperature-channels");
> 
> Move these down to just above each check. Note that when using __free()
> it is fine not to have all declarations at the top.

Accepted.

> 
>> +     unsigned int num_supply = 0, num_temp = 0;
>> +     unsigned int idx, temp_chan_idx, volt_chan_idx;
>> +     struct iio_chan_spec *sysmon_channels;
>> +     const char *label;
>> +     u32 reg;
>> +     int ret;
>> +
>          struct fwnode_handle *supply_node __free(fwnode_handle) =
>                  device_get_named_child_node(dev, "voltage-channels");
>          if (supply_node)
>> +     if (supply_node)
>> +             num_supply = fwnode_get_child_node_count(supply_node);
>          struct fwnode_handle *temp_node __free(fwnode_handle) =
>                  device_get_named_child_node(dev, "temperature-channels");
>          if (temp_node)
> 
>> +     if (temp_node)
>> +             num_temp = fwnode_get_child_node_count(temp_node);
>> +
>> +     sysmon_channels = devm_kcalloc(dev,
>> +                                    size_add(ARRAY_SIZE(temp_channels),
>> +                                             num_supply + num_temp),
>> +                                    sizeof(*sysmon_channels), GFP_KERNEL);
>> +     if (!sysmon_channels)
>> +             return -ENOMEM;
>> +
>> +     /* Static temperature channels first (fixed indices) */
>> +     idx = 0;
>> +     memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
>> +     idx += ARRAY_SIZE(temp_channels);
>> +
>> +     /* Supply channels from DT */
>> +     fwnode_for_each_child_node_scoped(supply_node, child) {
>> +             ret = fwnode_property_read_u32(child, "reg", &reg);
>> +             if (ret < 0)
>> +                     return dev_err_probe(dev, ret,
>> +                                          "missing reg for supply channel\n");
>> +
>> +             if (reg > SYSMON_SUPPLY_IDX_MAX)
>> +                     return dev_err_probe(dev, -EINVAL,
>> +                                          "supply reg %u exceeds max %u\n",
>> +                                          reg, SYSMON_SUPPLY_IDX_MAX);
>> +
>> +             ret = fwnode_property_read_string(child, "label", &label);
>> +             if (ret < 0)
>> +                     return dev_err_probe(dev, ret,
>> +                                          "missing label for supply channel\n");
>> +
>> +             sysmon_channels[idx++] = (struct iio_chan_spec) {
>> +                     .type = IIO_VOLTAGE,
>> +                     .indexed = 1,
>> +                     .address = reg,
>> +                     .info_mask_separate =
>> +                             BIT(IIO_CHAN_INFO_RAW) |
>> +                             BIT(IIO_CHAN_INFO_PROCESSED),
> Add a similar comment to the one you have for temperature channels here.
> This is tricky because I have no idea how a user would set a floating point
> threshold via raw.  Their expectation is that is simple and linear.

Accepted.

> 
> How hard is it to take a _PROCESSED event value and convert it back to
> a format that can be used for setting the register values? To me that
> seems like a much more intuitive interface
> 
>  From a quick look at the event patch it seems you are doing that? In which case drop raw.

Yes, the event code in P4 already converts between millivolts and
the raw register format internally. Will drop RAW for voltage,
keep PROCESSED only.

> 
>> +                     .scan_type = {
>> +                             .realbits = 19,
>> +                             .storagebits = 32,
>> +                             .endianness = IIO_CPU,
>> +                             .sign = fwnode_property_read_bool(child,
>> +                                     "bipolar") ? 's' : 'u',
> 
> As above, this only gets exposed when buffered interfaces are added, so for
> now don't set it unless you are using them for some internal purposes.
> Also .sign is being replace with .format.

Accepted. Noted re .format for future buffered support.

> 
>> +                     },
>> +                     .datasheet_name = label,
>> +             };
>> +     }
>> +
>> +     /* Temperature satellite channels from DT */
>> +     fwnode_for_each_child_node_scoped(temp_node, child) {
>> +             ret = fwnode_property_read_u32(child, "reg", &reg);
>> +             if (ret < 0)
>> +                     return dev_err_probe(dev, ret,
>> +                                          "missing reg for temp channel\n");
>> +
>> +             if (reg < 1 || reg > SYSMON_TEMP_SAT_MAX)
>> +                     return dev_err_probe(dev, -EINVAL,
>> +                                          "temp reg %u out of range [1..%u]\n",
>> +                                          reg, SYSMON_TEMP_SAT_MAX);
>> +
>> +             ret = fwnode_property_read_string(child, "label", &label);
>> +             if (ret < 0)
>> +                     return dev_err_probe(dev, ret,
>> +                                          "missing label for temp channel\n");
>> +
>> +             sysmon_channels[idx++] = (struct iio_chan_spec) {
>> +                     .type = IIO_TEMP,
>> +                     .indexed = 1,
>> +                     .address = SYSMON_TEMP_SAT_BASE +
>> +                                ((reg - 1) * SYSMON_REG_STRIDE),
>> +                     .info_mask_separate =
>> +                             BIT(IIO_CHAN_INFO_RAW) |
>> +                             BIT(IIO_CHAN_INFO_PROCESSED),
> 
> As above, add a comment on why both (or drop one of them)

Dropping RAW for voltage as discussed above.

> 
>> +                     .scan_type = {
>> +                             .sign = 's',
>> +                             .realbits = 15,
>> +                             .storagebits = 16,
>> +                             .endianness = IIO_CPU,
>> +                     },
>> +                     .datasheet_name = label,
>> +             };
>> +     }
>> +
>> +     indio_dev->num_channels = idx;
>> +     indio_dev->info = &sysmon_iio_info;
>> +
>> +     /*
>> +      * Assign per-type sequential channel numbers.
>> +      * IIO sysfs uses type prefix (in_tempN, in_voltageN)
>> +      * so numbers only need to be unique within each type.
>> +      */
>> +     temp_chan_idx = 0;
>> +     volt_chan_idx = 0;
>> +     for (idx = 0; idx < indio_dev->num_channels; idx++) {
>> +             if (sysmon_channels[idx].type == IIO_TEMP)
>> +                     sysmon_channels[idx].channel = temp_chan_idx++;
>> +             else
>> +                     sysmon_channels[idx].channel = volt_chan_idx++;
>> +     }
>> +
>> +     indio_dev->channels = sysmon_channels;
>> +
>> +     return 0;
>> +}
> 
>> diff --git a/drivers/iio/adc/versal-sysmon.c b/drivers/iio/adc/versal-sysmon.c
>> new file mode 100644
>> index 00000000000..8473288e7db
>> --- /dev/null
>> +++ b/drivers/iio/adc/versal-sysmon.c
>> @@ -0,0 +1,92 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD Versal SysMon MMIO platform driver
>> + *
>> + * Copyright (C) 2019 - 2022, Xilinx, Inc.
>> + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "versal-sysmon.h"
>> +
>> +struct sysmon_mmio {
>> +     void __iomem *base;
>> +};
>> +
>> +static int sysmon_mmio_reg_read(void *context, unsigned int reg,
>> +                             unsigned int *val)
>> +{
>> +     struct sysmon_mmio *mmio = context;
>> +
>> +     *val = readl(mmio->base + reg);
> 
> Blank line before simple returns slightly helps readabilty.

Accepted.
> 
>> +     return 0;
>> +}
> 
>> +static int sysmon_platform_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct sysmon_mmio *mmio;
>> +     struct regmap *regmap;
>> +
>> +     mmio = devm_kzalloc(dev, sizeof(*mmio), GFP_KERNEL);
>> +     if (!mmio)
>> +             return -ENOMEM;
>> +
>> +     mmio->base = devm_platform_ioremap_resource(pdev, 0);
>> +     if (IS_ERR(mmio->base))
>> +             return PTR_ERR(mmio->base);
>> +
>> +     regmap = devm_regmap_init(dev, NULL, mmio,
>> +                               &sysmon_mmio_regmap_config);
> 
> Fits on one line I think even with a strict 80 char limit (which we relax
> when readabilty is hurt)

Accepted.

> 
>> +     if (IS_ERR(regmap))
>> +             return PTR_ERR(regmap);
>> +
>> +     return sysmon_core_probe(dev, regmap);
>> +}
> 
>> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
>> new file mode 100644
>> index 00000000000..d24d2481915
>> --- /dev/null
>> +++ b/drivers/iio/adc/versal-sysmon.h
> 
> ...
> 
>> +
>> +/* Q8.7 fractional shift */
>> +#define SYSMON_FRACTIONAL_SHIFT              7U
> 
> This should perhaps be a mask then use FIELD_GET() to extract the value.

Accepted.

> 
>> +#define SYSMON_SUPPLY_MANTISSA_BITS  16
>> +
>> +/**
>> + * struct sysmon - Driver data for Versal SysMon
>> + * @regmap: register map for hardware access
>> + * @lock: protects regmap access
>> + */
>> +struct sysmon {
>> +     struct regmap *regmap;
>> +     /* Protects regmap access */
> 
> regmap has it's own internal locks. So this comment needs to explain
> in more detail what is being protected.  I assume read modify write
> or long related sequences that must not be interrupted and aren't encapsulated
> in single regmap calls?

Accepted. The mutex protects read-modify-write sequences on
threshold registers and cached state (oversampling ratios,
hysteresis values) that span multiple regmap calls.

> 
>> +     struct mutex lock;
>> +};
> 

All items will be addressed in v4.

Salih

Re: [PATCH v3 3/5] iio: adc: versal-sysmon: add I2C driver

[Erim, Salih]

Hi Jonathan,

On 28/05/2026 13:42, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:09 +0100
> Salih Erim <salih.erim@amd.com> wrote:
> 
>> Add an I2C transport driver for the Versal SysMon block. The SysMon
>> provides an I2C slave interface that allows an external master to
>> read voltage and temperature measurements through the same register
>> map used by the MMIO path.
>>
>> The I2C command frame is an 8-byte structure containing a 4-byte data
>> payload, a 2-byte register offset, and a 1-byte instruction field.
>> Read operations send the frame with a read instruction, then receive
>> a 4-byte response containing the register value.
>>
>> Events are not supported on the I2C path because there is no
>> interrupt line and the I2C regmap backend cannot be called from
>> atomic context.
>>
>> Co-developed-by: Conall O'Griofa <conall.ogriofa@amd.com>
>> Signed-off-by: Conall O'Griofa <conall.ogriofa@amd.com>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
> A few minor things inline.
> 
>> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
>> new file mode 100644
>> index 00000000000..92d149f517e
>> --- /dev/null
>> +++ b/drivers/iio/adc/versal-sysmon-i2c.c
>> @@ -0,0 +1,153 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD Versal SysMon I2C driver
>> + *
>> + * Copyright (C) 2023 - 2026, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "versal-sysmon.h"
>> +
>> +#define SYSMON_I2C_INSTR_READ        BIT(2)
>> +#define SYSMON_I2C_INSTR_WRITE       BIT(3)
>> +
>> +#define SYSMON_I2C_DATA0_MASK        GENMASK(7, 0)
>> +#define SYSMON_I2C_DATA1_MASK        GENMASK(15, 8)
>> +#define SYSMON_I2C_DATA2_MASK        GENMASK(23, 16)
>> +#define SYSMON_I2C_DATA3_MASK        GENMASK(31, 24)
>> +
>> +#define SYSMON_I2C_OFS_LOW_MASK              GENMASK(9, 2)
>> +#define SYSMON_I2C_OFS_HIGH_MASK     GENMASK(15, 10)
>> +
>> +/* Byte positions within the 8-byte I2C command frame (HW-defined) */
>> +enum sysmon_i2c_payload_idx {
>> +     SYSMON_I2C_DATA0_IDX = 0,
>> +     SYSMON_I2C_DATA1_IDX = 1,
>> +     SYSMON_I2C_DATA2_IDX = 2,
>> +     SYSMON_I2C_DATA3_IDX = 3,
>> +     SYSMON_I2C_OFS_LOW_IDX = 4,
>> +     SYSMON_I2C_OFS_HIGH_IDX = 5,
> 
> With changes suggested below I think you only need the two base
> offsets and the final one.  As such maybe 3 defines makes more sense
> than an enum

Accepted. Will switch to defines.

> 
>> +     SYSMON_I2C_INSTR_IDX = 6,
> If you do keep an enum, it would be good to add an entry for the final
> byte to give some indication of why it is 8 bytes. Even if that is
> reserved0

Switching to defines as above.

>> +};
>> +
>> +static int sysmon_i2c_reg_read(void *context, unsigned int reg,
>> +                            unsigned int *val)
>> +{
>> +     struct i2c_client *client = context;
>> +     u8 write_buf[8] = { };
>> +     u8 read_buf[4];
>> +     int ret;
>> +
>> +     write_buf[SYSMON_I2C_OFS_LOW_IDX] =
>> +             FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
>> +     write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
>> +             FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);
> 
> I'd guess an unaligned put works here a well? though you'll need
> to do a FIELD_GET() to extract the slightly shifted content.

Accepted.

> 
>> +     write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_READ;
>> +
>> +     ret = i2c_master_send(client, write_buf, sizeof(write_buf));
>> +     if (ret < 0)
>> +             return ret;
>> +     if (ret != sizeof(write_buf))
>> +             return -EIO;
>> +
>> +     ret = i2c_master_recv(client, read_buf, sizeof(read_buf));
>> +     if (ret < 0)
>> +             return ret;
>> +     if (ret != sizeof(read_buf))
>> +             return -EIO;
>> +
>> +     *val = FIELD_PREP(SYSMON_I2C_DATA0_MASK,
>> +                       read_buf[SYSMON_I2C_DATA0_IDX]) |
>> +            FIELD_PREP(SYSMON_I2C_DATA1_MASK,
>> +                       read_buf[SYSMON_I2C_DATA1_IDX]) |
>> +            FIELD_PREP(SYSMON_I2C_DATA2_MASK,
>> +                       read_buf[SYSMON_I2C_DATA2_IDX]) |
>> +            FIELD_PREP(SYSMON_I2C_DATA3_MASK,
>> +                       read_buf[SYSMON_I2C_DATA3_IDX]);
> Very complex way to express what I think is
>          *val = get_unaligned_le32(&read_buf[0]);

Accepted.

>> +
>> +     return 0;
>> +}
>> +
>> +static int sysmon_i2c_reg_write(void *context, unsigned int reg,
>> +                             unsigned int val)
>> +{
>> +     struct i2c_client *client = context;
>> +     u8 write_buf[8] = { };
> 
>> +     int ret;
>> +
>> +     write_buf[SYSMON_I2C_DATA0_IDX] =
>> +             FIELD_GET(SYSMON_I2C_DATA0_MASK, val);
>> +     write_buf[SYSMON_I2C_DATA1_IDX] =
>> +             FIELD_GET(SYSMON_I2C_DATA1_MASK, val);
>> +     write_buf[SYSMON_I2C_DATA2_IDX] =
>> +             FIELD_GET(SYSMON_I2C_DATA2_MASK, val);
>> +     write_buf[SYSMON_I2C_DATA3_IDX] =
>> +             FIELD_GET(SYSMON_I2C_DATA3_MASK, val);
> 
> That's a put_unaligned_le32() I think?

Accepted.

> 
>> +     write_buf[SYSMON_I2C_OFS_LOW_IDX] =
>> +             FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
>> +     write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
>> +             FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);
> 
> I'd guess put_unaligned_le16()? Will need a a FIELD_PREP()
> for the full thing though.

Accepted.

> 
>> +     write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_WRITE;
>> +
>> +     ret = i2c_master_send(client, write_buf, sizeof(write_buf));
>> +     if (ret < 0)
>> +             return ret;
>> +     if (ret != sizeof(write_buf))
>> +             return -EIO;
>> +
>> +     return 0;
>> +}
> 
>> +static const struct of_device_id sysmon_i2c_of_match_table[] = {
>> +     { .compatible = "xlnx,versal-sysmon" },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(of, sysmon_i2c_of_match_table);
>> +
>> +static const struct i2c_device_id sysmon_i2c_id_table[] = {
>> +     { "versal-sysmon" },
> 
> Named initializer for this.  Uwe is cleaning these up across IIO;
> let us not add another one!

Accepted. Will use { .name = "versal-sysmon" }.

All items will be addressed in v4.

Salih
> 
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, sysmon_i2c_id_table);
>> +
>> +static struct i2c_driver sysmon_i2c_driver = {
>> +     .probe = sysmon_i2c_probe,
>> +     .driver = {
>> +             .name = "versal-sysmon-i2c",
>> +             .of_match_table = sysmon_i2c_of_match_table,
>> +     },
>> +     .id_table = sysmon_i2c_id_table,
>> +};
>> +module_i2c_driver(sysmon_i2c_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("AMD Versal SysMon I2C Driver");
>> +MODULE_AUTHOR("Conall O'Griofa <conall.ogriofa@amd.com>");
>> +MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");
> 

Re: [PATCH v3 4/5] iio: adc: versal-sysmon: add threshold event support

[Erim, Salih]

Hi Jonathan,

On 28/05/2026 14:01, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:10 +0100
> Salih Erim <salih.erim@amd.com> wrote:
> 
>> Add threshold event support for temperature and supply voltage
>> channels.
>>
>> Temperature events:
>>    - Rising threshold with configurable value
>>    - Over-temperature (OT) alarm with separate threshold
>>    - Per-channel hysteresis as a millicelsius value
>>    - Event direction is IIO_EV_DIR_RISING (hysteresis mode)
>>
>> Supply voltage events:
>>    - Rising/falling threshold per supply channel
>>    - Per-channel alarm enable via alarm configuration registers
>>
>> The hardware supports both window and hysteresis alarm modes for
>> temperature. This driver uses hysteresis mode, where the upper
>> threshold triggers the alarm and the lower threshold clears it
>> (re-arm point). The hardware has a single ISR bit per temperature
>> channel with no indication of which threshold was crossed, so
>> hysteresis mode is the natural fit. The lower threshold register
>> is computed internally as (upper - hysteresis).
>>
>> Hysteresis is stored in the driver as a millicelsius value,
>> initialized from the hardware registers at probe. Writing the
>> rising threshold or hysteresis recomputes the lower register.
>> ALARM_CONFIG is hard-coded to hysteresis mode during init.
>>
>> The interrupt handler masks active threshold interrupts (which are
>> level-sensitive) and schedules a delayed worker to poll for condition
>> clear before unmasking. When no hardware IRQ is available, event
>> channels are not created and interrupt init is skipped, since the
>> I2C regmap backend cannot be called from atomic context.
>>
>> When disabling a supply channel alarm, the group interrupt remains
>> active if any other channel in the same alarm group still has an
>> alarm enabled.
>>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
> Hi Salih
> 
> Main thing in here is related to earlier question on _PROCESSED + _RAW.
> I can't see a reason to have _RAW.

Agreed. Will drop RAW, keep PROCESSED only.

> 
> Various minor things inline. In many case they apply in other places I haven't
> called out so look for repeats
> 
> Jonathan
> 
>>   drivers/iio/adc/versal-sysmon-core.c | 655 ++++++++++++++++++++++++++-
>>   drivers/iio/adc/versal-sysmon.h      |  48 +-
>>   2 files changed, 697 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
>> index ebe052f6982..04977c9c887 100644
>> --- a/drivers/iio/adc/versal-sysmon-core.c
>> +++ b/drivers/iio/adc/versal-sysmon-core.c
>> @@ -11,6 +11,8 @@
>>   #include <linux/bitops.h>
>>   #include <linux/cleanup.h>
>>   #include <linux/device.h>
>> +#include <linux/devm-helpers.h>
>> +#include <linux/interrupt.h>
>>   #include <linux/module.h>
>>   #include <linux/property.h>
>>   #include <linux/regmap.h>
>> @@ -18,10 +20,19 @@
>>   #include <linux/sysfs.h>
>>   #include <linux/units.h>
>>
>> +#include <linux/iio/events.h>
>>   #include <linux/iio/iio.h>
>>
>>   #include "versal-sysmon.h"
>>
>> +/* OT and TEMP hysteresis mode bits in SYSMON_TEMP_EV_CFG */
>> +#define SYSMON_OT_HYST_MASK          BIT(0)
>> +#define SYSMON_TEMP_HYST_MASK                BIT(1)
>> +
>> +/* Compute alarm register offset from a channel address */
>> +#define SYSMON_ALARM_OFFSET(addr) \
>> +     (SYSMON_ALARM_REG + ((addr) / SYSMON_ALARM_BITS_PER_REG) * SYSMON_REG_STRIDE)
>> +
>>   /*
>>    * Both RAW and PROCESSED are exposed: RAW is needed for event thresholds
>>    * (which operate in hardware register format), PROCESSED gives userspace
>> @@ -44,6 +55,62 @@
>>        .datasheet_name = _name,                                \
>>   }
>>
>> +#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _name, _events) {\
>> +     .type = IIO_TEMP,                                       \
>> +     .indexed = 1,                                           \
> 
> Why do you needs separate channel for events?  Can't we add the event
> spec to existing channels? For the constant ones you may need to
> have two arrays to pick between depending on whether the irq is available or not.

Accepted. Will merge event specs into the static temp channels
and use two arrays (with/without events) based on has_irq.

> 
> 
>> +     .address = _address,                                    \
>> +     .channel = _chan,                                       \
>> +     .event_spec = _events,                                  \
>> +     .num_event_specs = ARRAY_SIZE(_events),                 \
>> +     .scan_type = {                                          \
>> +             .sign = 's',                                    \
>> +             .realbits = 15,                                 \
>> +             .storagebits = 16,                              \
>> +             .endianness = IIO_CPU,                          \
>> +     },                                                      \
>> +     .datasheet_name = _name,                                \
>> +}
>> +
>          *raw_data = (u16)tmp;
>> +}
> 
>> +
>> +/*
>> + * Recompute the lower threshold register from upper threshold and
>> + * cached hysteresis. Called when either upper threshold or hysteresis
>> + * is written.
>> + */
>> +static int sysmon_update_temp_lower(struct sysmon *sysmon, int address)
>> +{
>> +     unsigned int upper_reg;
>> +     int upper_mc, lower_mc, hysteresis;
>> +     u32 raw_val;
>> +     int upper_off, lower_off, ret;
>> +
>> +     upper_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_RISING);
>> +     if (upper_off < 0)
>> +             return upper_off;
>> +     lower_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_FALLING);
>> +     if (lower_off < 0)
>> +             return lower_off;
>> +
>> +     if (address == SYSMON_ADDR_OT_EVENT)
>> +             hysteresis = sysmon->ot_hysteresis;
>> +     else
>> +             hysteresis = sysmon->temp_hysteresis;
>> +
>> +     ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
>> +     if (ret)
>> +             return ret;
>> +     sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
>> +
>> +     lower_mc = upper_mc - hysteresis;
>> +     sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
> 
> So all the manipulation is in the units of _PROCESSED. Hence
> I'd drop _RAW.

Accepted.
> 
>> +
>> +     return regmap_write(sysmon->regmap, lower_off, raw_val);
>> +}
>> +
>> +static int sysmon_read_event_value(struct iio_dev *indio_dev,
>> +                                const struct iio_chan_spec *chan,
>> +                                enum iio_event_type type,
>> +                                enum iio_event_direction dir,
>> +                                enum iio_event_info info,
>> +                                int *val, int *val2)
>> +{
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     unsigned int reg_val;
>> +     int offset;
>> +     int ret;
>> +
>> +     guard(mutex)(&sysmon->lock);
>> +
>> +     if (chan->type == IIO_TEMP) {
>> +             if (info == IIO_EV_INFO_VALUE) {
>> +                     /* Only rising threshold is exposed */
>> +                     offset = sysmon_temp_thresh_offset(chan->address,
>> +                                                        IIO_EV_DIR_RISING);
>> +                     if (offset < 0)
>> +                             return offset;
> I'd add a blank line here.
>> +                     ret = regmap_read(sysmon->regmap, offset, &reg_val);
>> +                     if (ret)
>> +                             return ret;
> here
>> +                     sysmon_q8p7_to_millicelsius(reg_val, val);
> and here
> 
> Generally keep block of call + error check for one thing separate from code
> before and after.  Slightly nicer to read.
> 
> Lots of other places this applies but it's a very minor thing.

Accepted. Will add blank lines throughout between call+error-check
blocks.
> 
>> +                     return IIO_VAL_INT;
>> +             }
>> +             if (info == IIO_EV_INFO_HYSTERESIS) {
>> +                     if (chan->address == SYSMON_ADDR_OT_EVENT)
>> +                             *val = sysmon->ot_hysteresis;
>> +                     else
>> +                             *val = sysmon->temp_hysteresis;
>> +                     return IIO_VAL_INT;
>> +             }
>> +     }
>> +
>> +     if (chan->type == IIO_VOLTAGE) {
>> +             offset = sysmon_supply_thresh_offset(chan->address, dir);
>> +             if (offset < 0)
>> +                     return offset;
>> +             ret = regmap_read(sysmon->regmap, offset, &reg_val);
>> +             if (ret)
>> +                     return ret;
>> +             sysmon_supply_rawtoprocessed(reg_val, val);
>> +             return IIO_VAL_INT;
>> +     }
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +static int sysmon_write_event_value(struct iio_dev *indio_dev,
>> +                                 const struct iio_chan_spec *chan,
>> +                                 enum iio_event_type type,
>> +                                 enum iio_event_direction dir,
>> +                                 enum iio_event_info info,
>> +                                 int val, int val2)
>> +{
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     unsigned int reg_val;
>> +     u32 raw_val;
>> +     int offset;
>> +     int ret;
>> +
>> +     guard(mutex)(&sysmon->lock);
> 
>> +
>> +     if (chan->type == IIO_TEMP) {
>> +             if (info == IIO_EV_INFO_VALUE) {
>> +                     /* Only rising threshold is exposed */
>> +                     offset = sysmon_temp_thresh_offset(chan->address,
>> +                                                        IIO_EV_DIR_RISING);
>> +                     if (offset < 0)
>> +                             return offset;
>> +                     sysmon_millicelsius_to_q8p7(&raw_val, val);
>> +                     ret = regmap_write(sysmon->regmap, offset, raw_val);
>> +                     if (ret)
>> +                             return ret;
>> +                     /* Recompute lower = upper - hysteresis */
>> +                     return sysmon_update_temp_lower(sysmon,
>> +                                                     chan->address);
> 
> Under 80 chars on one line.  Check for cases of this and feel free to go a bit
> over if it helps readability.

Accepted.

> 
>> +                     return sysmon_update_temp_lower(sysmon, chan->address);
> 
>> +             }
>> +             if (info == IIO_EV_INFO_HYSTERESIS) {
>> +                     if (val < 0)
>> +                             return -EINVAL;
>> +                     if (chan->address == SYSMON_ADDR_OT_EVENT)
>> +                             sysmon->ot_hysteresis = val;
>> +                     else
>> +                             sysmon->temp_hysteresis = val;
>> +                     return sysmon_update_temp_lower(sysmon,
>> +                                                     chan->address);
> 
> As above.

Accepted.

> 
>> +             }
>> +     }
>> +
>> +     if (chan->type == IIO_VOLTAGE) {
>> +             offset = sysmon_supply_thresh_offset(chan->address, dir);
>> +             if (offset < 0)
>> +                     return offset;
>> +             ret = regmap_read(sysmon->regmap, offset, &reg_val);
>> +             if (ret)
>> +                     return ret;
>> +             sysmon_supply_processedtoraw(val, reg_val, &raw_val);
>> +             return regmap_write(sysmon->regmap, offset, raw_val);
>> +     }
>> +
>> +     return -EINVAL;
>> +}
> 
>> +static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
>> +{
>> +     u32 alarm_flag_offset = SYSMON_ALARM_FLAG + (event * SYSMON_REG_STRIDE);
>> +     u32 alarm_reg_offset = SYSMON_ALARM_REG + (event * SYSMON_REG_STRIDE);
>> +     struct sysmon *sysmon = iio_priv(indio_dev);
>> +     unsigned long alarm_flag_reg;
>> +     unsigned int reg_val;
>> +     u32 address, bit;
>> +     int ret;
>> +
>> +     switch (event) {
>> +     case SYSMON_BIT_TEMP:
>> +             sysmon_push_event(indio_dev, SYSMON_ADDR_TEMP_EVENT);
>> +             ret = regmap_write(sysmon->regmap, SYSMON_IDR,
>> +                                BIT(SYSMON_BIT_TEMP));
>> +             if (ret)
>> +                     return ret;
>> +             sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
>> +             break;
>> +
>> +     case SYSMON_BIT_OT:
>> +             sysmon_push_event(indio_dev, SYSMON_ADDR_OT_EVENT);
>> +             ret = regmap_write(sysmon->regmap, SYSMON_IDR,
>> +                                BIT(SYSMON_BIT_OT));
>> +             if (ret)
>> +                     return ret;
>> +             sysmon->masked_temp |= BIT(SYSMON_BIT_OT);
>> +             break;
>> +
>> +     case SYSMON_BIT_ALARM0:
>> +     case SYSMON_BIT_ALARM1:
>> +     case SYSMON_BIT_ALARM2:
>> +     case SYSMON_BIT_ALARM3:
>> +     case SYSMON_BIT_ALARM4:
>> +             ret = regmap_read(sysmon->regmap, alarm_flag_offset, &reg_val);
>> +             if (ret)
>> +                     return ret;
>> +             alarm_flag_reg = reg_val;
>> +
>> +             for_each_set_bit(bit, &alarm_flag_reg,
>> +                              SYSMON_ALARM_BITS_PER_REG) {
>> +                     address = bit + (SYSMON_ALARM_BITS_PER_REG * event);
>> +                     sysmon_push_event(indio_dev, address);
>> +                     ret = regmap_update_bits(sysmon->regmap,
>> +                                              alarm_reg_offset,
>> +                                              BIT(bit), 0);
>> +                     if (ret)
>> +                             return ret;
>> +             }
>> +             ret = regmap_write(sysmon->regmap, alarm_flag_offset,
>> +                                alarm_flag_reg);
>> +             if (ret)
>> +                     return ret;
>> +             break;
>                  return regmap_write();
>> +
>> +     default:
>> +             break;
> 
> Why is this not an error?

Good point. Will return -EINVAL from the default case.

> 
>> +     }
>> +
>> +     return 0;
> Might as well return early in the various paths.

Accepted. Will restructure to return from each case directly.

>> +}
> ...
> 
>> +static irqreturn_t sysmon_iio_irq(int irq, void *data)
>> +{
>> +     struct iio_dev *indio_dev = data;
>> +     struct sysmon *sysmon;
>> +     unsigned int isr, imr;
>> +
>> +     sysmon = iio_priv(indio_dev);
>> +     spin_lock(&sysmon->irq_lock);
> 
> guard() here would eman you can just return if (!isr)

Accepted.

All items will be addressed in v4.

Salih

> 
>> +
>> +     regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
>> +     regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
>> +
>> +     isr &= ~imr;
>> +     regmap_write(sysmon->regmap, SYSMON_ISR, isr);
>> +
>> +     if (isr) {
>> +             sysmon_handle_events(indio_dev, isr);
>> +             schedule_delayed_work(&sysmon->sysmon_unmask_work,
>> +                                   msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
>> +     }
>> +
>> +     spin_unlock(&sysmon->irq_lock);
>> +
>> +     return IRQ_RETVAL(isr);
>> +}

Re: [PATCH v3 5/5] iio: adc: versal-sysmon: add oversampling support

[Erim, Salih]

Hi Jonathan,

On 28/05/2026 14:05, Jonathan Cameron wrote:
> 
> 
> On Wed, 27 May 2026 12:42:11 +0100
> Salih Erim <salih.erim@amd.com> wrote:
> 
>> Add support for reading and writing the oversampling ratio through
>> the IIO oversampling_ratio attribute. The hardware supports averaging
>> 2, 4, 8, or 16 samples, plus a ratio of 1 (no averaging).
>>
>> Temperature and supply channels share oversampling configuration at
>> the type level (all temperature channels share one ratio, all supply
>> channels share another), exposed through info_mask_shared_by_type.
>>
>> The hardware encoding uses sample_count / 2 in a 4-bit field within
>> the CONFIG register. Per-channel averaging enable registers must also
>> be updated to activate or deactivate averaging.
>>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
> Really minor stuff from a reread

Thanks for all reviews, it has been really teachful every time.

> 
>> +static int sysmon_osr_write(struct sysmon *sysmon, int channel_type, int val)
>> +{
>> +     /*
>> +      * HW register encoding is sample_count / 2:
>> +      * 0=none, 1=2x, 2=4x, 4=8x, 8=16x (not log2-based).
>> +      */
>> +     int hw_val = val >> 1;
>> +     int ret;
>> +
>> +     if (channel_type == IIO_TEMP) {
>> +             ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
>> +                                     SYSMON_TEMP_SAT_CONFIG_MASK,
>> +                                     FIELD_PREP(SYSMON_TEMP_SAT_CONFIG_MASK,
>> +                                                hw_val));
>> +             if (ret)
>> +                     return ret;
> blank line

Accepted.

>> +             ret = sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
>> +                                         SYSMON_TEMP_EN_AVG_COUNT,
>> +                                         hw_val ? ~0U : 0);
>> +             if (ret)
>> +                     return ret;
>                  return sysmon_set...

Accepted. Will return directly.

> 
>> +     } else if (channel_type == IIO_VOLTAGE) {
> Won't need the else if returned already.

Accepted.

>> +             ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
>> +                                     SYSMON_SUPPLY_CONFIG_MASK,
>> +                                     FIELD_PREP(SYSMON_SUPPLY_CONFIG_MASK,
>> +                                                hw_val));
>> +             if (ret)
>> +                     return ret;
> blank line

Accepted.

>> +             ret = sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
>> +                                         SYSMON_SUPPLY_EN_AVG_COUNT,
>> +                                         hw_val ? ~0U : 0);
>> +             if (ret)
>> +                     return ret;
>                  return sysmon_set...

Accepted.

> 
>> +     } else {
> No else needed here either

Accepted.

> 
>> +             return -EINVAL;
>> +     }
>> +
>> +     return 0
> And this isn't needed at all.

Accepted.

> 
>> +}
> 
>> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
>> index a78362f95e6..cf69be62709 100644
>> --- a/drivers/iio/adc/versal-sysmon.h
>> +++ b/drivers/iio/adc/versal-sysmon.h
>> @@ -25,11 +25,13 @@ struct regmap;
>>   #define SYSMON_IMR                   0x0048
>>   #define SYSMON_IER                   0x004C
>>   #define SYSMON_IDR                   0x0050
>> +#define SYSMON_CONFIG                        0x0100
>>   #define SYSMON_TEMP_MAX                      0x1030
>>   #define SYSMON_TEMP_MIN                      0x1034
>>   #define SYSMON_SUPPLY_BASE           0x1040
>>   #define SYSMON_ALARM_FLAG            0x1018
>>   #define SYSMON_ALARM_REG             0x1940
>> +#define SYSMON_SUPPLY_EN_AVG_BASE    0x1958
>>   #define SYSMON_TEMP_TH_LOW           0x1970
>>   #define SYSMON_TEMP_TH_UP            0x1974
>>   #define SYSMON_OT_TH_LOW             0x1978
>> @@ -41,6 +43,7 @@ struct regmap;
>>   #define SYSMON_TEMP_MAX_MAX          0x1F90
>>   #define SYSMON_STATUS_RESET          0x1F94
>>   #define SYSMON_TEMP_SAT_BASE         0x1FAC
>> +#define SYSMON_TEMP_EN_AVG_BASE              0x24B4
>>   #define SYSMON_MAX_REG                       0x24C0
>>
>>   /* NPI unlock value written to SYSMON_NPI_LOCK */
>> @@ -57,6 +60,16 @@ struct regmap;
>>   /* ISR/IMR temperature and OT alarm mask (bits 9:8) */
>>   #define SYSMON_TEMP_INTR_MASK                GENMASK(9, 8)
>>
>> +/* Config register: supply oversampling field (bits 17:14) */
>> +#define SYSMON_SUPPLY_CONFIG_MASK    GENMASK(17, 14)
>> +
>> +/* Config register: temp satellite oversampling field (bits 27:24) */
> 
> I missed this before, but given the GENMASK just below the bits part
> of these comments is pointles. Drop it.
> 
> Ideally also name them in a way that makes it clear what register
> they are fields of.

Accepted. Will drop the "bits X:Y" comments and rename to
make the register association clear.

All items will be addressed in v4.

Salih

> 
>> +#define SYSMON_TEMP_SAT_CONFIG_MASK  GENMASK(27, 24)
> 
> 

Re: [PATCH v3 0/5] iio: adc: add AMD/Xilinx Versal SysMon driver

[Jonathan Cameron]

On Thu, 28 May 2026 22:46:58 +0100
"Erim, Salih" <salih.erim@amd.com> wrote:

> Hi Jonathan,
> 
> On 28/05/2026 13:06, Jonathan Cameron wrote:
> > 
> > 
> > On Wed, 27 May 2026 12:42:06 +0100
> > Salih Erim <salih.erim@amd.com> wrote:
> >   
> >> This series adds a new IIO driver for the AMD/Xilinx Versal System
> >> Monitor (SysMon), providing on-chip voltage and temperature monitoring.
> >>
> >> The Versal SysMon measures up to 160 supply voltages and reads up to
> >> 64 temperature satellites distributed across the SoC. The hardware
> >> also provides aggregated device temperature registers: the current
> >> max and min across all active satellites, and peak/trough values
> >> recorded since last hardware reset. The device can be accessed via
> >> memory-mapped I/O or via an I2C interface.
> >>
> >> The driver is split into a bus-agnostic core module using the regmap
> >> API, an MMIO platform driver, and an I2C driver. This allows the
> >> same IIO logic to be shared across different bus transports.
> >>
> >> Previous submissions:
> >>    v2: https://lore.kernel.org/all/cover.1746182670.git.salih.erim@amd.com/
> >>    v1: https://lore.kernel.org/all/cover.1757061697.git.michal.simek@amd.com/
> >>  
> > https://sashiko.dev/#/patchset/20260527114211.174288-1-salih.erim%40amd.com
> > Quite a bit of feedback.  Some of which is clearly garbage, like the
> > ARCH_VERSAL suggestion, but take a close look as it does tend to pick up on
> > stuff that humans miss.  
> 
> Thanks Jonathan. I've gone through all the Sashiko findings.
Thanks,
> 
> Reviewed but keeping as-is:
> - Left-shift of negative in millicelsius_to_q8p7: GCC defines
>    this behavior and it's consistent with the read direction
>    (right-shift); Andy asked for this symmetry in v2
> - Oversampling read without mutex: reading a single int is
>    atomic on arm64; adding contention for no practical benefit

This is potentially a bit messier if the compiler gets creative
(see Will Deacon's various talks on this for instance).  Still
we neglect this for most IIO drivers today.  One day maybe we'll
fix all that up - lots of careful READ_ONCE()/WRITE_ONCE() markings.