* Re: [PATCH v3 1/2] dt-bindings: spi: nuvoton,ma35d1-qspi: Add Nuvoton MA35D1 QSPI
From: Chi-Wen Weng @ 2026-06-15 1:18 UTC (permalink / raw)
To: Conor Dooley
Cc: broonie, robh, krzk+dt, conor+dt, linux-arm-kernel, linux-spi,
devicetree, linux-kernel, cwweng
In-Reply-To: <20260612-diagram-florist-01a0e8f923d8@spud>
Hi Conor,
Thanks for the clarification.
I will make the driver read num-cs in v4 and fall back to the hardware
default of 2 when the property is not present. I will also keep the
binding default in sync with that behavior.
Best regards,
Chi-Wen
Conor Dooley 於 2026/6/12 下午 11:48 寫道:
> On Fri, Jun 12, 2026 at 08:33:01AM +0800, Chi-Wen Weng wrote:
>> Hi Conor,
>>
>> Thanks for the review.
>>
>> I will add a default value for num-cs in v4:
>>
>> num-cs:
>> maximum: 2
>> default: 2
>>
>> The controller has two native chip selects and the driver currently uses
>> that hardware default.
> The driver should handle the property and fall back to the default.
> It's not complex to support, so surely there's no reason not to?
>
> Cheers,
> Conor.
>
>> Best regards,
>> Chi-Wen
>>
>> Conor Dooley 於 2026/6/12 上午 01:34 寫道:
>>> On Thu, Jun 11, 2026 at 05:12:45PM +0800, Chi-Wen Weng wrote:
>>>> From: Chi-Wen Weng <cwweng@nuvoton.com>
>>>>
>>>> Add a devicetree binding for the Quad SPI controller found in
>>>> Nuvoton MA35D1 SoCs.
>>>>
>>>> The controller supports SPI memory devices such as SPI NOR and SPI NAND
>>>> flashes. It has one register range, one clock input and one reset line,
>>>> and supports up to two chip selects.
>>>>
>>>> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
>>>> ---
>>>> .../bindings/spi/nuvoton,ma35d1-qspi.yaml | 62 +++++++++++++++++++
>>>> 1 file changed, 62 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,ma35d1-qspi.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/nuvoton,ma35d1-qspi.yaml b/Documentation/devicetree/bindings/spi/nuvoton,ma35d1-qspi.yaml
>>>> new file mode 100644
>>>> index 000000000000..d3b36e612eb0
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/nuvoton,ma35d1-qspi.yaml
>>>> @@ -0,0 +1,62 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/spi/nuvoton,ma35d1-qspi.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Nuvoton MA35D1 Quad SPI Controller
>>>> +
>>>> +maintainers:
>>>> + - Chi-Wen Weng <cwweng@nuvoton.com>
>>>> +
>>>> +allOf:
>>>> + - $ref: /schemas/spi/spi-controller.yaml#
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + const: nuvoton,ma35d1-qspi
>>>> +
>>>> + reg:
>>>> + maxItems: 1
>>>> +
>>>> + interrupts:
>>>> + maxItems: 1
>>>> +
>>>> + clocks:
>>>> + maxItems: 1
>>>> +
>>>> + resets:
>>>> + maxItems: 1
>>>> +
>>>> + num-cs:
>>>> + maximum: 2
>>> Missing a default of 2, unless you make the property required.
>>> FWIW, your driver doesn't appear to read this value.
>>>
>>> pw-bot: changes-requested
>>>
>>> Cheers,
>>> Conor.
>>>
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> + - clocks
>>>> + - resets
>>>> +
>>>> +unevaluatedProperties: false
>>>> +
>>>> +examples:
>>>> + - |
>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>> + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
>>>> + #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
>>>> +
>>>> + soc {
>>>> + #address-cells = <2>;
>>>> + #size-cells = <2>;
>>>> +
>>>> + spi@40680000 {
>>>> + compatible = "nuvoton,ma35d1-qspi";
>>>> + reg = <0 0x40680000 0 0x100>;
>>>> + interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
>>>> + clocks = <&clk QSPI0_GATE>;
>>>> + resets = <&sys MA35D1_RESET_QSPI0>;
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + };
>>>> + };
>>>> +
>>>> --
>>>> 2.25.1
>>>>
^ permalink raw reply
* Re: [PATCH v3] riscv: dts: spacemit: Use symbolic PDMA request numbers on K1
From: Yixun Lan @ 2026-06-15 1:15 UTC (permalink / raw)
To: Guodong Xu
Cc: Vinod Koul, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, linux-kernel, dmaengine, devicetree, linux-riscv,
spacemit
In-Reply-To: <20260611-b4-k1-pdma-req-macros-v3-1-ae8416052571@gmail.com>
Hi Guodong,
The patch itself looks good to me, only few minor comments
On 09:17 Thu 11 Jun , Guodong Xu wrote:
> Add a local DTS header, k1-pdma.h, that gives symbolic names to the K1
> PDMA request numbers. These request numbers are hardware-fixed; their
> allocation can be found in K1 manual.
>
..
> Replace the hard-coded numbers in the SPI3 "dmas" property with the
> K1_PDMA_SPI3_RX/TX macros.
>
As it's too obvious that people can tell from the diff, then I feel it's
unnecessary to repeat in commit message..
> Signed-off-by: Guodong Xu <docular.xu@gmail.com>
> ---
> Add a local DTS header naming the K1 PDMA request lines and convert the
> current user (the K1 SPI3 node) to the new K1_PDMA_* macros.
..
> The request
> numbers come from the SpacemiT K1 User Manual [1], Chapter 9.4.3 DMA
> Connectivity & Assignments.
>
I'd suggest to put above into commit message, as more detailed description.
Here is my attempt to slightly reconstruct the commit message:
The PDMA request numbers (DRQ) are fixed values specific to the SoC from a
hardware perspective. The detailed definition can be found in K1 User Manual,
Chapter 9.4.3 DMA Connectivity & Assignments. Add a DTS header file to define
the symbolic names for the DRQs of non-secure DMA peripherals.
> [1]: https://www.spacemit.com/community/document/info?lang=en&nodepath=hardware/key_stone/k1/k1_docs/k1_usermanual/9.Top_System.md
>
> Changes in v3:
> - Move the request-number macros from include/dt-bindings/dma/ to a local
> DTS header arch/riscv/boot/dts/spacemit/k1-pdma.h (Conor).
> - Squash the header and its user into a single patch.
> - Link to v2: https://patch.msgid.link/20260609-b4-k1-pdma-req-macros-v2-0-5d5d7b997b54@gmail.com
>
> Changes in v2:
> - Drop the #dma-cells description change in spacemit,k1-pdma.yaml; the request
> numbers are hardware-fixed and unused by the driver (Conor)
> - Link to v1: https://patch.msgid.link/20260607-b4-k1-pdma-req-macros-v1-0-5b2a3955007c@gmail.com
>
> BR,
> Guodong Xu
> ---
> arch/riscv/boot/dts/spacemit/k1-pdma.h | 56 ++++++++++++++++++++++++++++++++++
> arch/riscv/boot/dts/spacemit/k1.dtsi | 4 ++-
> 2 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/boot/dts/spacemit/k1-pdma.h b/arch/riscv/boot/dts/spacemit/k1-pdma.h
> new file mode 100644
> index 0000000000000..65112d5847add
> --- /dev/null
> +++ b/arch/riscv/boot/dts/spacemit/k1-pdma.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * This header provides DMA request number for non-secure peripherals of
> + * SpacemiT K1 PDMA.
slightly less redundant,
DMA request number (DRQ) defintion for non-secure peripherals
> + *
> + * Copyright (c) 2026 Guodong Xu <docular.xu@gmail.com>
> + */
> +
> +#ifndef _DTS_SPACEMIT_K1_PDMA_H
> +#define _DTS_SPACEMIT_K1_PDMA_H
> +
> +#define K1_PDMA_UART0_TX 3
> +#define K1_PDMA_UART0_RX 4
> +#define K1_PDMA_UART2_TX 5
> +#define K1_PDMA_UART2_RX 6
> +#define K1_PDMA_UART3_TX 7
> +#define K1_PDMA_UART3_RX 8
> +#define K1_PDMA_UART4_TX 9
> +#define K1_PDMA_UART4_RX 10
> +#define K1_PDMA_I2C0_TX 11
> +#define K1_PDMA_I2C0_RX 12
> +#define K1_PDMA_I2C1_TX 13
> +#define K1_PDMA_I2C1_RX 14
> +#define K1_PDMA_I2C2_TX 15
> +#define K1_PDMA_I2C2_RX 16
> +#define K1_PDMA_I2C4_TX 17
> +#define K1_PDMA_I2C4_RX 18
> +#define K1_PDMA_SPI3_TX 19
> +#define K1_PDMA_SPI3_RX 20
> +#define K1_PDMA_I2S0_TX 21
> +#define K1_PDMA_I2S0_RX 22
> +#define K1_PDMA_I2S1_TX 23
> +#define K1_PDMA_I2S1_RX 24
> +#define K1_PDMA_UART5_TX 25
> +#define K1_PDMA_UART5_RX 26
> +#define K1_PDMA_UART6_TX 27
> +#define K1_PDMA_UART6_RX 28
> +#define K1_PDMA_UART7_TX 29
> +#define K1_PDMA_UART7_RX 30
> +#define K1_PDMA_UART8_TX 31
> +#define K1_PDMA_UART8_RX 32
> +#define K1_PDMA_UART9_TX 33
> +#define K1_PDMA_UART9_RX 34
> +#define K1_PDMA_I2C5_TX 35
> +#define K1_PDMA_I2C5_RX 36
> +#define K1_PDMA_I2C6_TX 37
> +#define K1_PDMA_I2C6_RX 38
> +#define K1_PDMA_I2C7_TX 39
> +#define K1_PDMA_I2C7_RX 40
> +#define K1_PDMA_I2C8_TX 41
> +#define K1_PDMA_I2C8_RX 42
> +#define K1_PDMA_CAN0_RX 43
> +#define K1_PDMA_QSPI_RX 44
> +#define K1_PDMA_QSPI_TX 45
> +
> +#endif /* _DTS_SPACEMIT_K1_PDMA_H */
> diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
> index 08a0f28d011fe..7d414e15d2cc2 100644
> --- a/arch/riscv/boot/dts/spacemit/k1.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
> @@ -6,6 +6,8 @@
> #include <dt-bindings/clock/spacemit,k1-syscon.h>
> #include <dt-bindings/phy/phy.h>
>
> +#include "k1-pdma.h"
> +
> /dts-v1/;
> / {
> #address-cells = <2>;
> @@ -1094,7 +1096,7 @@ spi3: spi@d401c000 {
> clock-names = "core", "bus";
> resets = <&syscon_apbc RESET_SSP3>;
> interrupts = <55>;
> - dmas = <&pdma 20>, <&pdma 19>;
> + dmas = <&pdma K1_PDMA_SPI3_RX>, <&pdma K1_PDMA_SPI3_TX>;
> dma-names = "rx", "tx";
> status = "disabled";
> };
>
> ---
> base-commit: 793cc54475b49b5b558902b5c13e4bfe66530a50
> change-id: 20260607-b4-k1-pdma-req-macros-8d276d0126df
>
> Best regards,
> --
> Guodong Xu <docular.xu@gmail.com>
>
--
Yixun Lan (dlan)
^ permalink raw reply
* RE: [PATCH v9 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver
From: Escala, Edelweise @ 2026-06-15 1:12 UTC (permalink / raw)
To: Lee Jones
Cc: Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20260611114741.GP4151951@google.com>
Hi Lee,
Yes I'll take care of these.
> -----Original Message-----
> From: Lee Jones <lee@kernel.org>
> Sent: Thursday, June 11, 2026 7:48 PM
> To: Escala, Edelweise <Edelweise.Escala@analog.com>
> Cc: Pavel Machek <pavel@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; linux-
> leds@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v9 2/2] leds: ltc3220: Add Support for LTC3220 18 channel
> LED Driver
>
> [External]
>
> /* Sashiko Automation: Issues Found (10 Findings) */
>
> Would you mind taking care of these before I conduct my next review please?
>
> On Thu, 28 May 2026, Edelweise Escala wrote:
>
> > Add driver for the LTC3220 18-channel LED driver with I2C interface,
> > individual brightness control, and hardware-assisted blink/gradation
> > features.
> >
> > Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/leds/Kconfig | 13 ++
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-ltc3220.c | 440
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 455 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > c8a242577d2f..0f553ada61d9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15229,6 +15229,7 @@ L: linux-leds@vger.kernel.org
> > S: Maintained
> > W: https://ez.analog.com/linux-software-drivers
> > F: Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
> > +F: drivers/leds/leds-ltc3220.c
> >
> > LTC4282 HARDWARE MONITOR DRIVER
> > M: Nuno Sa <nuno.sa@analog.com>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index
> > f4a0a3c8c870..31b1e3ff094c 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -1000,6 +1000,19 @@ config LEDS_ST1202
> > Say Y to enable support for LEDs connected to LED1202
> > LED driver chips accessed via the I2C bus.
> >
> > +config LEDS_LTC3220
> > + tristate "LED Driver for Analog Devices Inc. LTC3220"
> > + depends on I2C && LEDS_CLASS
> > + select REGMAP_I2C
> > + help
> > + Say Y to enable support for the Analog Devices LTC3220
> > + 18-channel LED controller with I2C interface.
> > + The driver supports individual LED brightness control (64 steps),
> > + hardware-assisted blinking and gradation effects.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called leds-ltc3220.
> > +
> > config LEDS_TPS6105X
> > tristate "LED support for TI TPS6105X"
> > depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index
> > 7db3768912ca..a68244bd50fb 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_LEDS_LP8788) += leds-
> lp8788.o
> > obj-$(CONFIG_LEDS_LP8860) += leds-lp8860.o
> > obj-$(CONFIG_LEDS_LP8864) += leds-lp8864.o
> > obj-$(CONFIG_LEDS_LT3593) += leds-lt3593.o
> > +obj-$(CONFIG_LEDS_LTC3220) += leds-ltc3220.o
> > obj-$(CONFIG_LEDS_MAX5970) += leds-max5970.o
> > obj-$(CONFIG_LEDS_MAX77650) += leds-max77650.o
> > obj-$(CONFIG_LEDS_MAX77705) += leds-max77705.o
> > diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> > new file mode 100644 index 000000000000..a633ac5ca6ae
> > --- /dev/null
> > +++ b/drivers/leds/leds-ltc3220.c
> > @@ -0,0 +1,440 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LTC3220 18-Channel LED Driver
> > + *
> > + * Copyright 2026 Analog Devices Inc.
> > + *
> > + * Author: Edelweise Escala <edelweise.escala@analog.com> */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
>
> [Severity: Low]
> Is this include needed?
>
> It doesn't appear that any delay functions like msleep() or usleep_range() are
> used in the driver implementation.
>
I'll probably need delay again for the reset comment later.
>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/leds.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
>
> [Severity: Low]
> Should this file include <linux/property.h>?
>
> The driver uses fwnode property APIs such as
> device_for_each_child_node_scoped(),
> fwnode_property_read_u32(), and fwnode_property_present(), which are
> defined in <linux/property.h>.
>
>
I'll check this.
> > +
> > +/* LTC3220 Registers */
> > +#define LTC3220_COMMAND_REG 0x00
> > +#define LTC3220_QUICK_WRITE_MASK BIT(0)
> > +#define LTC3220_SHUTDOWN_MASK BIT(3)
> > +
> > +#define LTC3220_ULED_REG(x) (0x01 + (x))
> > +#define LTC3220_LED_CURRENT_MASK GENMASK(5,
> 0)
> > +#define LTC3220_LED_MODE_MASK GENMASK(7,
> 6)
> > +
> > +#define LTC3220_GRAD_BLINK_REG 0x13
> > +#define LTC3220_GRADATION_MASK GENMASK(2, 0)
> > +#define LTC3220_GRADATION_DIRECTION_MASK BIT(0)
> > +#define LTC3220_GRADATION_PERIOD_MASK
> GENMASK(2, 1)
> > +#define LTC3220_BLINK_MASK GENMASK(4,
> 3)
> > +
> > +#define LTC3220_NUM_LEDS 18
> > +
> > +#define LTC3220_GRADATION_START_VALUE 128
>
> [Severity: Low]
> Is this macro used anywhere in the driver?
>
> It appears to be dead code as it is never referenced.
>
>
Will remove
> > +#define LTC3220_GRADATION_RAMP_TIME_240MS 240
> > +#define LTC3220_GRADATION_RAMP_TIME_480MS 480
> > +
> > +#define LTC3220_BLINK_ON_156MS 156
> > +#define LTC3220_BLINK_ON_625MS 625
> > +#define LTC3220_BLINK_PERIOD_1250MS 1250
> > +#define LTC3220_BLINK_PERIOD_2500MS 2500
> > +
> > +#define LTC3220_BLINK_SHORT_ON_TIME BIT(0)
> > +#define LTC3220_BLINK_LONG_PERIOD BIT(1)
> > +
> > +enum ltc3220_led_mode {
> > + LTC3220_NORMAL_MODE,
> > + LTC3220_BLINK_MODE,
> > + LTC3220_GRADATION_MODE,
> > +};
> > +
> > +enum ltc3220_blink_mode {
> > + LTC3220_BLINK_MODE_625MS_1250MS,
> > + LTC3220_BLINK_MODE_156MS_1250MS,
> > + LTC3220_BLINK_MODE_625MS_2500MS,
> > + LTC3220_BLINK_MODE_156MS_2500MS
> > +};
> > +
> > +enum ltc3220_gradation_mode {
> > + LTC3220_GRADATION_MODE_DISABLED,
> > + LTC3220_GRADATION_MODE_240MS_RAMP_TIME,
> > + LTC3220_GRADATION_MODE_480MS_RAMP_TIME,
> > + LTC3220_GRADATION_MODE_960MS_RAMP_TIME
> > +};
> > +
> > +static bool ltc3220_volatile_reg(struct device *dev, unsigned int
> > +reg) {
> > + return reg == LTC3220_GRAD_BLINK_REG; }
>
> [Severity: High]
> Does this register need to be marked volatile?
>
> Marking a register as volatile tells regmap to bypass the cache and read from the
> physical hardware. If the LTC3220 is a write-only I2C device, the physical read will
> fail, causing regmap_update_bits() to fail entirely in
> ltc3220_pattern_set() and ltc3220_blink_set().
>
> Even if the device supports reads, this register is exclusively controlled by
> software and does not change autonomously, so could it be cached normally to
> avoid unnecessary I2C reads?
>
>
Will check if it will work if I remove this, I had an issue with the gradation and blink register when I did not add volatile regisgters
> > +
> > +static const struct regmap_config ltc3220_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = LTC3220_GRAD_BLINK_REG,
> > + .cache_type = REGCACHE_FLAT_S,
> > + .volatile_reg = ltc3220_volatile_reg, };
> > +
> > +struct ltc3220_uled_cfg {
> > + struct led_classdev led_cdev;
> > + u8 reg_value;
> > + u8 led_index;
> > +};
> > +
> > +struct ltc3220 {
> > + struct ltc3220_uled_cfg uled_cfg[LTC3220_NUM_LEDS];
> > + struct regmap *regmap;
> > + bool is_aggregated;
> > +};
> > +
> > +/*
> > + * Set LED brightness. Hardware supports 0-63 brightness levels.
> > + * Mode switching (blink/gradation) is handled through dedicated callbacks.
> > + */
> > +static int ltc3220_set_led_data(struct led_classdev *led_cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> > + uled_cfg[uled_cfg->led_index]);
>
> [Severity: Low]
> Will this dynamic variable index inside container_of() cause a build failure?
>
> The driver attempts to retrieve the parent structure using uled_cfg[uled_cfg-
> >led_index]. The container_of macro relies on offsetof(), which strictly requires
> the member argument to be a constant expression.
>
> Since uled_cfg->led_index is a runtime variable, this violates the C standard and
> can abort compilation on compilers like Clang with an error about a non-constant
> integer expression.
>
>
Will fix this as well as other container of issues
> > + int ret;
> > +
> > + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg-
> >led_index),
> > + brightness);
> > + if (ret)
> > + return ret;
> > +
> > + uled_cfg->reg_value = brightness;
> > +
> > + /*
> > + * When aggregated LED mode is enabled, writing to LED 1 updates all
> > + * LEDs simultaneously via quick-write mode. Update cached values for
> > + * all LEDs to reflect the synchronized state.
> > + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for
> how
> > + * to configure aggregated LED mode.
> > + */
> > + if (ltc3220->is_aggregated && uled_cfg->led_index == 0) {
> > + for (int i = 0; i < LTC3220_NUM_LEDS; i++)
> > + ltc3220->uled_cfg[i].reg_value = brightness;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static enum led_brightness ltc3220_get_led_data(struct led_classdev
> > +*led_cdev) {
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > +
> > + return uled_cfg->reg_value;
> > +}
> > +
> > +/*
> > + * LTC3220 pattern support for hardware-assisted breathing/gradation.
> > + * The hardware supports 3 gradation ramp time 240ms, 480ms, 960ms)
> > + * and can ramp up or down.
> > + *
> > + * Pattern array interpretation:
> > + * pattern[0].brightness = start brightness (0-63)
> > + * pattern[0].delta_t = ramp time in milliseconds
> > + * pattern[1].brightness = end brightness (0-63)
> > + * pattern[1].delta_t = (optional, can be 0 or same as pattern[0].delta_t)
> > + */
> > +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> > + struct led_pattern *pattern,
> > + u32 len, int repeat)
> > +{
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> > + uled_cfg[uled_cfg->led_index]);
>
> [Severity: Low]
> Will this dynamic variable index inside container_of() cause a build failure?
>
> The driver attempts to retrieve the parent structure using uled_cfg[uled_cfg-
> >led_index]. The container_of macro relies on offsetof(), which strictly requires
> the member argument to be a constant expression.
>
> Since uled_cfg->led_index is a runtime variable, this violates the C standard and
> can abort compilation on compilers like Clang with an error about a non-constant
> integer expression.
>
>
> > + u8 gradation_period;
> > + u8 start_brightness;
> > + u8 end_brightness;
> > + u8 gradation_val;
> > + bool is_increasing;
> > + int ret;
> > +
> > + if (len != 2)
> > + return -EINVAL;
> > +
> > + start_brightness = pattern[0].brightness &
> LTC3220_LED_CURRENT_MASK;
> > + end_brightness = pattern[1].brightness &
> LTC3220_LED_CURRENT_MASK;
> > +
> > + is_increasing = end_brightness > start_brightness;
> > +
> > + if (pattern[0].delta_t == 0)
> > + gradation_period = LTC3220_GRADATION_MODE_DISABLED;
> > + else if (pattern[0].delta_t <=
> LTC3220_GRADATION_RAMP_TIME_240MS)
> > + gradation_period =
> LTC3220_GRADATION_MODE_240MS_RAMP_TIME;
> > + else if (pattern[0].delta_t <=
> LTC3220_GRADATION_RAMP_TIME_480MS)
> > + gradation_period =
> LTC3220_GRADATION_MODE_480MS_RAMP_TIME;
> > + else
> > + gradation_period =
> LTC3220_GRADATION_MODE_960MS_RAMP_TIME;
> > +
> > + gradation_val = FIELD_PREP(LTC3220_GRADATION_PERIOD_MASK,
> gradation_period);
> > + gradation_val |= FIELD_PREP(LTC3220_GRADATION_DIRECTION_MASK,
> > +is_increasing);
> > +
> > + ret = regmap_update_bits(ltc3220->regmap,
> LTC3220_GRAD_BLINK_REG,
> > + LTC3220_GRADATION_MASK, gradation_val);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ltc3220_set_led_data(led_cdev, start_brightness);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg-
> >led_index),
> > + FIELD_PREP(LTC3220_LED_MODE_MASK,
> LTC3220_GRADATION_MODE) |
> > + end_brightness);
> > + if (ret)
> > + return ret;
> > +
> > + uled_cfg->reg_value = end_brightness;
> > +
> > + return 0;
> > +}
> > +
> > +static int ltc3220_pattern_clear(struct led_classdev *led_cdev) {
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> > + uled_cfg[uled_cfg->led_index]);
>
> [Severity: Low]
> Will this dynamic variable index inside container_of() cause a build failure?
>
> The driver attempts to retrieve the parent structure using uled_cfg[uled_cfg-
> >led_index]. The container_of macro relies on offsetof(), which strictly requires
> the member argument to be a constant expression.
>
> Since uled_cfg->led_index is a runtime variable, this violates the C standard and
> can abort compilation on compilers like Clang with an error about a non-constant
> integer expression.
>
>
> > + int ret;
> > +
> > + ret = regmap_update_bits(ltc3220->regmap,
> LTC3220_ULED_REG(uled_cfg->led_index),
> > + LTC3220_LED_MODE_MASK,
> LTC3220_NORMAL_MODE);
> > + if (ret)
> > + return ret;
> > +
> > + return ltc3220_set_led_data(led_cdev, LED_OFF); }
> > +
> > +/*
> > + * LTC3220 has a global blink configuration that affects all LEDs.
> > + * This implementation allows per-LED blink requests, but the blink
> > +timing
> > + * will be shared across all LEDs. The delay values are mapped to the
> > + * hardware's discrete blink rates.
> > + */
> > +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off)
> > +{
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> > + uled_cfg[uled_cfg->led_index]);
>
> [Severity: Low]
> Will this dynamic variable index inside container_of() cause a build failure?
>
> The driver attempts to retrieve the parent structure using uled_cfg[uled_cfg-
> >led_index]. The container_of macro relies on offsetof(), which strictly requires
> the member argument to be a constant expression.
>
> Since uled_cfg->led_index is a runtime variable, this violates the C standard and
> can abort compilation on compilers like Clang with an error about a non-constant
> integer expression.
>
>
> > + u8 blink_mode = 0;
> > + int ret;
> > +
> > + if (*delay_on <= LTC3220_BLINK_ON_156MS)
> > + blink_mode = LTC3220_BLINK_SHORT_ON_TIME;
> > +
> > + if (*delay_on + *delay_off > LTC3220_BLINK_PERIOD_1250MS)
> > + blink_mode |= LTC3220_BLINK_LONG_PERIOD;
> > +
> > + switch (blink_mode) {
> > + case LTC3220_BLINK_MODE_625MS_1250MS:
> > + *delay_on = LTC3220_BLINK_ON_625MS;
> > + *delay_off = LTC3220_BLINK_PERIOD_1250MS -
> LTC3220_BLINK_ON_625MS;
> > + break;
> > + case LTC3220_BLINK_MODE_156MS_1250MS:
> > + *delay_on = LTC3220_BLINK_ON_156MS;
> > + *delay_off = LTC3220_BLINK_PERIOD_1250MS -
> LTC3220_BLINK_ON_156MS;
> > + break;
> > + case LTC3220_BLINK_MODE_625MS_2500MS:
> > + *delay_on = LTC3220_BLINK_ON_625MS;
> > + *delay_off = LTC3220_BLINK_PERIOD_2500MS -
> LTC3220_BLINK_ON_625MS;
> > + break;
> > + case LTC3220_BLINK_MODE_156MS_2500MS:
> > + *delay_on = LTC3220_BLINK_ON_156MS;
> > + *delay_off = LTC3220_BLINK_PERIOD_2500MS -
> LTC3220_BLINK_ON_156MS;
> > + break;
> > + }
> > +
> > + ret = regmap_update_bits(ltc3220->regmap,
> LTC3220_GRAD_BLINK_REG,
> > + LTC3220_BLINK_MASK,
> FIELD_PREP(LTC3220_BLINK_MASK, blink_mode));
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_update_bits(ltc3220->regmap,
> LTC3220_ULED_REG(uled_cfg->led_index),
> > + LTC3220_LED_MODE_MASK,
> > + FIELD_PREP(LTC3220_LED_MODE_MASK,
> LTC3220_BLINK_MODE)); }
>
> [Severity: Medium]
> Will the hardware blink activate at 0 brightness if the LED was previously off?
>
> When the LED is off, its current bits in LTC3220_ULED_REG are 0. The driver
> updates the mode bits to LTC3220_BLINK_MODE using regmap_update_bits() but
> does not update the brightness bits.
>
> Because the core expects the driver to handle brightness configuration for
> hardware blinking, could this hardware blink execute at 0 brightness, leaving the
> LED unlit?
>
Will make sure to have brightness when setting blinking
>
> > +
> > +static void ltc3220_reset_gpio_action(void *data) {
> > + struct gpio_desc *reset_gpio = data;
> > +
> > + gpiod_set_value_cansleep(reset_gpio, 1); }
> > +
> > +static int ltc3220_reset(struct ltc3220 *ltc3220, struct i2c_client
> > +*client) {
> > + struct gpio_desc *reset_gpio;
> > + int ret;
> > +
> > + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> GPIOD_OUT_HIGH);
> > + if (IS_ERR(reset_gpio))
> > + return dev_err_probe(&client->dev, PTR_ERR(reset_gpio),
> "Failed on
> > +reset GPIO\n");
> > +
> > + if (reset_gpio) {
> > + gpiod_set_value_cansleep(reset_gpio, 0);
>
> [Severity: High]
> Does this sequence lack the necessary delays to ensure a successful reset?
>
> The driver asserts the reset GPIO by requesting it with GPIOD_OUT_HIGH and
> immediately de-asserts it with gpiod_set_value_cansleep(reset_gpio, 0). There is
> no delay between these calls, creating a near-zero width pulse that may fail to
> meet the IC's minimum reset pulse width requirement.
>
> It also lacks a wakeup delay before subsequent I2C commands might be sent.
>
Will add delays
>
> > +
> > + return devm_add_action_or_reset(&client->dev,
> ltc3220_reset_gpio_action,
> > + reset_gpio);
>
> [Severity: High]
> Does returning early here skip software initialization and regmap cache setup?
>
> If the reset_gpio is successfully acquired, this function immediately returns 0,
> entirely skipping the subsequent regmap_write() calls intended to initialize the
> hardware registers.
>
> Because the driver does not define a .reg_defaults table, skipping these writes
> leaves the regmap cache uninitialized, which can break subsequent read-modify-
> write logic via regmap_update_bits().
>
>
> > + }
> > +
> > + ret = regmap_write(ltc3220->regmap, LTC3220_COMMAND_REG, 0);
> > + if (ret)
> > + return ret;
> > +
> > + for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
> > + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(i), 0);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return regmap_write(ltc3220->regmap, LTC3220_GRAD_BLINK_REG, 0); }
> > +
> > +static int ltc3220_suspend(struct device *dev) {
> > + struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> > +
> > + return regmap_update_bits(ltc3220->regmap,
> LTC3220_COMMAND_REG,
> > + LTC3220_SHUTDOWN_MASK,
> LTC3220_SHUTDOWN_MASK); }
> > +
> > +static int ltc3220_resume(struct device *dev) {
> > + struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> > +
> > + return regmap_update_bits(ltc3220->regmap,
> LTC3220_COMMAND_REG,
> > + LTC3220_SHUTDOWN_MASK, 0);
> > +}
>
> [Severity: High]
> Are the hardware registers left permanently off after resume?
>
> In ltc3220_suspend(), the driver sets the LTC3220_SHUTDOWN_MASK bit. On
> many such chips, asserting the shutdown bit resets internal registers. Additionally,
> power to the IC may be lost during system suspend.
>
> In ltc3220_resume(), the driver clears the shutdown bit but does not call
> regcache_mark_dirty() and regcache_sync(). Could this leave the physical LED
> outputs permanently off, while the kernel's regmap cache and LED classdev state
> incorrectly reflect that the LEDs are still configured and illuminated?
>
Will add regcache_sync() call to ltc3220_resume()
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(ltc3220_pm_ops, ltc3220_suspend,
> > +ltc3220_resume);
> > +
> > +static int ltc3220_probe(struct i2c_client *client) {
> > + struct ltc3220 *ltc3220;
> > + bool aggregated_led_found = false;
> > + int num_leds = 0;
> > + u8 led_index = 0;
> > + int ret;
> > +
> > + ltc3220 = devm_kzalloc(&client->dev, sizeof(*ltc3220), GFP_KERNEL);
> > + if (!ltc3220)
> > + return -ENOMEM;
> > +
> > + ltc3220->regmap = devm_regmap_init_i2c(client,
> <c3220_regmap_config);
> > + if (IS_ERR(ltc3220->regmap))
> > + return dev_err_probe(&client->dev, PTR_ERR(ltc3220->regmap),
> > + "Failed to initialize regmap\n");
> > +
> > + i2c_set_clientdata(client, ltc3220);
> > +
> > + ret = ltc3220_reset(ltc3220, client);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret, "Failed to reset
> > +device\n");
> > +
> > + device_for_each_child_node_scoped(&client->dev, child) {
> > + struct led_init_data init_data = {};
> > + struct ltc3220_uled_cfg *led;
> > + u32 source;
> > +
> > + ret = fwnode_property_read_u32(child, "reg", &source);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret, "Couldn't read
> LED
> > +address\n");
> > +
> > + if (!source || source > LTC3220_NUM_LEDS)
> > + return dev_err_probe(&client->dev, -EINVAL, "LED
> address out of
> > +range\n");
> > +
> > + init_data.fwnode = child;
> > + init_data.devicename = "ltc3220";
> > +
> > + if (fwnode_property_present(child, "led-sources")) {
> > + if (source != 1)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "Aggregated LED out of
> range\n");
> > +
> > + if (aggregated_led_found)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "One Aggregated LED
> only\n");
> > +
> > + aggregated_led_found = true;
> > + ltc3220->is_aggregated = true;
> > +
> > + ret = regmap_update_bits(ltc3220->regmap,
> > + LTC3220_COMMAND_REG,
> > +
> LTC3220_QUICK_WRITE_MASK,
> > +
> LTC3220_QUICK_WRITE_MASK);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret,
> > + "Failed to set quick write
> mode\n");
> > + }
> > +
> > + num_leds++;
> > +
> > + /* LED node reg/index/address goes from 1 to 18 */
> > + led_index = source - 1;
> > + led = <c3220->uled_cfg[led_index];
> > +
> > + if (led->led_cdev.dev)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "Duplicate LED reg %u found",
> source);
> > +
> > + led->led_index = led_index;
> > + led->reg_value = 0;
> > + led->led_cdev.brightness_set_blocking = ltc3220_set_led_data;
> > + led->led_cdev.brightness_get = ltc3220_get_led_data;
> > + led->led_cdev.max_brightness = 63;
> > + led->led_cdev.blink_set = ltc3220_blink_set;
> > + led->led_cdev.pattern_set = ltc3220_pattern_set;
> > + led->led_cdev.pattern_clear = ltc3220_pattern_clear;
> > +
> > + ret = devm_led_classdev_register_ext(&client->dev, &led-
> >led_cdev, &init_data);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret, "Failed to
> register LED class\n");
> > + }
> > +
> > + /*
> > + * Aggregated LED mode uses hardware quick-write to control all 18 LEDs
> > + * simultaneously. This is mutually exclusive with individual LED control.
> > + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for
> details
> > + * on how to configure aggregated LED mode.
> > + */
> > + if (aggregated_led_found && num_leds > 1)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "Aggregated LED must be the only LED
> node\n");
>
> [Severity: Medium]
> Are the LED sysfs interfaces exposed to userspace before the driver probe is fully
> validated?
>
> Inside the loop above, devm_led_classdev_register_ext() is called, which
> immediately exposes sysfs interfaces. If the validation check here fails and
> returns an error, userspace could have already discovered and interacted with the
> LED devices before they are unexpectedly torn down by devres.
>
> Could these validation checks be completed before registering the interfaces?
>
Restructured ltc3220_probe() to use two passes:
- First pass: Validate all device tree configuration and set up LED structures
- Second pass: Register LED class devices only after validation succeeds
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id ltc3220_of_match[] = {
> > + { .compatible = "adi,ltc3220" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ltc3220_of_match);
> > +
> > +static struct i2c_driver ltc3220_led_driver = {
> > + .driver = {
> > + .name = "ltc3220",
> > + .of_match_table = ltc3220_of_match,
> > + .pm = pm_sleep_ptr(<c3220_pm_ops),
> > + },
> > + .probe = ltc3220_probe,
> > +};
> > +module_i2c_driver(ltc3220_led_driver);
> > +
> > +MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
> > +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.43.0
> >
>
> --
> Lee Jones
Thank you!
Best Regards,
Edelweise Escala
^ permalink raw reply
* Re: [PATCH v2 2/7] ASoC: loongson: Add Loongson-2K0300 I2S controller support
From: Binbin Zhou @ 2026-06-15 1:06 UTC (permalink / raw)
To: Huacai Chen
Cc: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Xuerui Wang, loongarch, devicetree, linux-sound
In-Reply-To: <CAAhV-H6G5vTH_58_pKvBnvjdr2eVTWTctUqmPAX3aMjsFDjsQg@mail.gmail.com>
On Sat, Jun 13, 2026 at 11:18 AM Huacai Chen <chenhuacai@kernel.org> wrote:
>
> Hi, Binbin,
>
> On Thu, Jun 4, 2026 at 10:11 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
> >
> > The Loongson-2K0300 I2S interface differs significantly from the
> > Loongson-2K1000. Although both utilize external DMA controllers, the
> > Loongson-2K0300 does not require additional registers for routing
> > configuration.
> >
> > Due to hardware design flaw, an extra controller reset sequence is
> > required during probe.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> > sound/soc/loongson/loongson_i2s_plat.c | 42 +++++++++++++++++++++-----
> > 1 file changed, 35 insertions(+), 7 deletions(-)
> >
> > diff --git a/sound/soc/loongson/loongson_i2s_plat.c b/sound/soc/loongson/loongson_i2s_plat.c
> > index ac054b6ce632..b4d807ee7f8f 100644
> > --- a/sound/soc/loongson/loongson_i2s_plat.c
> > +++ b/sound/soc/loongson/loongson_i2s_plat.c
> > @@ -2,7 +2,7 @@
> > //
> > // Loongson I2S controller master mode dirver(platform device)
> > //
> > -// Copyright (C) 2023-2024 Loongson Technology Corporation Limited
> > +// Copyright (C) 2023-2026 Loongson Technology Corporation Limited
> > //
> > // Author: Yingkun Meng <mengyingkun@loongson.cn>
> > // Binbin Zhou <zhoubinbin@loongson.cn>
> > @@ -21,6 +21,7 @@
> > #include "loongson_i2s.h"
> > #include "loongson_dma.h"
> >
> > +/* Loongson-2K1000 APBDMA routing */
> > #define LOONGSON_I2S_RX_DMA_OFFSET 21
> > #define LOONGSON_I2S_TX_DMA_OFFSET 18
> >
> > @@ -30,6 +31,11 @@
> > #define LOONGSON_DMA3_CONF 0x3
> > #define LOONGSON_DMA4_CONF 0x4
> >
> > +struct loongson_i2s_plat_config {
> > + int rev_id;
> > + int (*i2s_dma_config)(struct platform_device *pdev);
> > +};
> > +
> > static int loongson_i2s_apbdma_config(struct platform_device *pdev)
> > {
> > int val;
> > @@ -47,8 +53,18 @@ static int loongson_i2s_apbdma_config(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +static struct loongson_i2s_plat_config ls2k1000_i2s_plat_config = {
> > + .rev_id = 0,
> > + .i2s_dma_config = loongson_i2s_apbdma_config,
> > +};
> > +
> > +static struct loongson_i2s_plat_config ls2k0300_i2s_plat_config = {
> > + .rev_id = 1,
> > +};
> Put ls2k0300_i2s_plat_config before ls2k1000_i2s_plat_config?
OK, I will do it in the next version.
>
> Huacai
>
> > +
> > static int loongson_i2s_plat_probe(struct platform_device *pdev)
> > {
> > + const struct loongson_i2s_plat_config *plat_config;
> > struct device *dev = &pdev->dev;
> > struct loongson_i2s *i2s;
> > struct resource *res;
> > @@ -59,12 +75,17 @@ static int loongson_i2s_plat_probe(struct platform_device *pdev)
> > if (!i2s)
> > return -ENOMEM;
> >
> > - ret = loongson_i2s_apbdma_config(pdev);
> > - if (ret)
> > - return ret;
> > + plat_config = device_get_match_data(dev);
> > + if (!plat_config)
> > + return -EINVAL;
> >
> > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > - i2s->reg_base = devm_ioremap_resource(&pdev->dev, res);
> > + if (plat_config->i2s_dma_config) {
> > + ret = plat_config->i2s_dma_config(pdev);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + i2s->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > if (IS_ERR(i2s->reg_base))
> > return dev_err_probe(dev, PTR_ERR(i2s->reg_base),
> > "devm_ioremap_resource failed\n");
> > @@ -87,11 +108,17 @@ static int loongson_i2s_plat_probe(struct platform_device *pdev)
> > if (IS_ERR(i2s_clk))
> > return dev_err_probe(dev, PTR_ERR(i2s_clk), "clock property invalid\n");
> > i2s->clk_rate = clk_get_rate(i2s_clk);
> > + i2s->rev_id = plat_config->rev_id;
> >
> > dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > dev_set_name(dev, LS_I2S_DRVNAME);
> > dev_set_drvdata(dev, i2s);
> >
> > + if (i2s->rev_id == 1) {
> > + regmap_update_bits(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_RESET, I2S_CTRL_RESET);
> > + fsleep(200);
> > + }
> > +
> > ret = devm_snd_soc_register_component(dev, &loongson_i2s_edma_component,
> > &loongson_i2s_dai, 1);
> > if (ret)
> > @@ -102,7 +129,8 @@ static int loongson_i2s_plat_probe(struct platform_device *pdev)
> > }
> >
> > static const struct of_device_id loongson_i2s_ids[] = {
> > - { .compatible = "loongson,ls2k1000-i2s" },
> > + { .compatible = "loongson,ls2k0300-i2s", .data = &ls2k0300_i2s_plat_config },
> > + { .compatible = "loongson,ls2k1000-i2s", .data = &ls2k1000_i2s_plat_config },
> > { /* sentinel */ },
> > };
> > MODULE_DEVICE_TABLE(of, loongson_i2s_ids);
> > --
> > 2.52.0
> >
> >
--
Thanks.
Binbin
^ permalink raw reply
* Re: [PATCH v2 6/7] ASoC: dt-bindings: loongson,ls-audio-card: Add ATK-DL2K0300B compatible
From: Binbin Zhou @ 2026-06-15 1:04 UTC (permalink / raw)
To: Rob Herring
Cc: Binbin Zhou, Huacai Chen, Krzysztof Kozlowski, Conor Dooley,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Huacai Chen, Xuerui Wang, loongarch, devicetree, linux-sound
In-Reply-To: <20260612160134.GA1116138-robh@kernel.org>
Hi Rob:
Thanks for your review.
On Sat, Jun 13, 2026 at 12:01 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jun 04, 2026 at 10:11:46AM +0800, Binbin Zhou wrote:
> > Add new compatible for the ATK-DL2K0300B development board based on
> > Loongson-2K0300.
> >
> > Unlike others, this board features GPIO-controlled headphone detection,
> > headphone mute, and speaker enable.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> > .../sound/loongson,ls-audio-card.yaml | 37 +++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml b/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
> > index 8c214e5d04b1..e9b248e8246c 100644
> > --- a/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
> > +++ b/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
> > @@ -23,6 +23,7 @@ properties:
> > enum:
> > - loongson,ls-audio-card # Loongson-2K1000/Loongson-2K2000/LS7A
> > - loongson,ls2k0300-forever-pi-audio-card # CTCISZ Forever Pi
> > + - loongson,ls2k0300-dl2k0300b-audio-card # ATK-DL2K0300B
> >
> > mclk-fs:
> > $ref: simple-card.yaml#/definitions/mclk-fs
> > @@ -47,6 +48,18 @@ properties:
> > required:
> > - sound-dai
> >
> > + loongson,spkr-en-gpios:
> > + maxItems: 1
> > + description: The GPIO that enables the speakers
> > +
> > + loongson,hp-mute-gpios:
> > + maxItems: 1
> > + description: The GPIO that mutes the headphones
> > +
> > + loongson,hp-det-gpios:
> > + maxItems: 1
> > + description: The GPIO that detect headphones are plugged in
>
> We have standard properties for at least this one. If you see multiple
> $vendor,foo-bar properties, don't add yet another vendor foo-bar
> property.
Yes, the `hp-det-gpios` property can be found in
`audio-graph-card2.yaml` or `audio-graph-card.yaml`, but I don't seem
to be able to reference it directly.
I’m not sure if I understood your suggestion correctly—to remove the
`vendor` prefix? If so, could I use the `fsl sound card`[1] as a
reference?
Also, the description for `hp-mute-gpios` is inaccurate; it actually
controls the enable state of the hp. I will rename it to
`hp-ctl-gpios`, as follows:
spkr-en-gpios:
maxItems: 1
description: The GPIO that enables the speakers
hp-ctl-gpios:
maxItems: 1
description: The GPIO that controls the headphones
hp-det-gpios:
maxItems: 1
description: The GPIO that detects when headphones are plugged in
[1]: https://elixir.bootlin.com/linux/v7.0/source/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml#L133
>
> Rob
--
Thanks.
Binbin
^ permalink raw reply
* Re: [PATCH] riscv: dts: spacemit: orangepi-rv2: Add cpu scaling for K1, SoC
From: Yixun Lan @ 2026-06-15 0:41 UTC (permalink / raw)
To: Vincent Legoll
Cc: Shuwei Wu, Rafael J. Wysocki, Viresh Kumar, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, linux-pm, linux-kernel, linux-riscv,
spacemit, devicetree
In-Reply-To: <f031e13e-d9f5-414a-b90e-a3d5a60b8d05@online.fr>
Hi Vincent,
Please send the patch directly instead of attached file, it's more
easy for people to review and apply, suggest to use b4, or
"git send-email" (old way)
On 08:50 Sun 14 Jun , Vincent Legoll wrote:
> [RESEND] as I cannot see yesterday's
>
>
> Hello,
>
>
>
> I tested (on OrangePi RV2) the attached patch applied over
>
> spacemit/for-next + Shuwei Wu's V3 series.
>
>
> This is a copy/paste of Shuwei Wu's work, so I don't know
>
> if I can submit this with my SoB or if it should be done
>
It's ok to add your SoB while adding support for new board
> differently, please advise.
>
>
> Anyways, you can add my:
>
> Tested-by: Vincent Legoll <vincent.legoll@gmail.com> # OrangePi-RV2
>
>
> To the relevant patches from your series, if that's useful,
>
> because it looks like it is working properly:
>
>
> On Void linux (musl libc):
>
>
> uname -a
> Linux opirv2 7.1.0-rc1-00043-gb860bca13be4 #14 SMP PREEMPT Sat Jun 13
> 11:02:13 CEST 2026 riscv64 GNU/Linux
>
> awk --version | head -1
> GNU Awk 5.3.2, API 4.0
>
> echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
>
> echo 1600000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
> time awk 'BEGIN{for(i=0;i<1000000;i++){}}'
> real 0m0.300s
> user 0m0.299s
> sys 0m0.001s
>
> echo 1228800 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
> time awk 'BEGIN{for(i=0;i<1000000;i++){}}'
> real 0m0.432s
> user 0m0.429s
> sys 0m0.004s
>
> echo 1000000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
> time awk 'BEGIN{for(i=0;i<1000000;i++){}}'
> real 0m0.476s
> user 0m0.476s
> sys 0m0.001s
>
> echo 819000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
> time awk 'BEGIN{for(i=0;i<1000000;i++){}}'
> real 0m0.582s
> user 0m0.581s
> sys 0m0.001s
>
> echo 614400 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
> time awk 'BEGIN{for(i=0;i<1000000;i++){}}'
> real 0m0.778s
> user 0m0.773s
> sys 0m0.005s
>
> --
>
> Vincent Legoll
>
> From 9924789655ae5f4f98e7a5cdc3df3f34e7bfb659 Mon Sep 17 00:00:00 2001
> From: Vincent Legoll <vincent.legoll@gmail.com>
> Date: Sat, 13 Jun 2026 10:49:34 +0200
> Subject: [PATCH] riscv: dts: spacemit: orangepi-rv2: Add cpu scaling for K1
> SoC
>
> Enable CPU DVFS on OrangePi RV2
>
..
> This is a copy/paste from Shuwei Wu's BPI-F3 DTS modifications
>
It's quite common that people do copy/paste for additional board
support, no need to mention it in commit message which I think it
isn't really useful
> Signed-off-by: Vincent Legoll <vincent.legoll@gmail.com>
> ---
> .../boot/dts/spacemit/k1-orangepi-rv2.dts | 35 ++++++++++++++++++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts b/arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts
> index 7c49bce427f3..62d25f579af6 100644
> --- a/arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts
> +++ b/arch/riscv/boot/dts/spacemit/k1-orangepi-rv2.dts
> @@ -8,6 +8,7 @@
>
> #include "k1.dtsi"
> #include "k1-pinctrl.dtsi"
> +#include "k1-opp.dtsi"
keep alphabet sorted
>
> / {
> model = "OrangePi RV2";
> @@ -80,6 +81,38 @@ &combo_phy {
> status = "okay";
> };
>
> +&cpu_0 {
> + cpu-supply = <&buck1_0v9>;
> +};
> +
> +&cpu_1 {
> + cpu-supply = <&buck1_0v9>;
> +};
> +
> +&cpu_2 {
> + cpu-supply = <&buck1_0v9>;
> +};
> +
> +&cpu_3 {
> + cpu-supply = <&buck1_0v9>;
> +};
> +
> +&cpu_4 {
> + cpu-supply = <&buck1_0v9>;
> +};
> +
> +&cpu_5 {
> + cpu-supply = <&buck1_0v9>;
> +};
> +
> +&cpu_6 {
> + cpu-supply = <&buck1_0v9>;
> +};
> +
> +&cpu_7 {
> + cpu-supply = <&buck1_0v9>;
> +};
> +
> &emmc {
> bus-width = <8>;
> mmc-hs400-1_8v;
> @@ -162,7 +195,7 @@ pmic@41 {
> dldoin2-supply = <&buck5>;
>
> regulators {
> - buck1 {
> + buck1_0v9: buck1 {
> regulator-min-microvolt = <500000>;
> regulator-max-microvolt = <3450000>;
> regulator-ramp-delay = <5000>;
> --
> 2.54.0
>
--
Yixun Lan (dlan)
^ permalink raw reply
* Re: [PATCH v3 1/4] drm: panel: Add LG LH609QH1 Panel with SW49410 controller
From: sashiko-bot @ 2026-06-15 0:18 UTC (permalink / raw)
To: Paul Sajna; +Cc: conor+dt, dri-devel, robh, devicetree
In-Reply-To: <20260614-judyln-panel-v3-1-07f4134441bd@postmarketos.org>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing `drm_panel_remove()` in the `mipi_dsi_attach()` error path causes a global list corruption and Use-After-Free.
- [High] DSI commands are sent while the panel is held in reset in `sw49410_panel_unprepare()`.
- [Medium] Misplaced `MIPI_DSI_MODE_LPM` flag manipulation around a software memory packing function.
- [Medium] The panel is published to the system before its DSC configuration is fully initialized.
--
commit 1a6b069af275d9965692f4acaa02ebd1a8517da9
Author: Amir Dahan <system64fumo@tuta.io>
Subject: drm: panel: Add LG LH609QH1 Panel with SW49410 controller
This commit adds a new panel driver for the LG SW49410 controller used
in the LG G7 ThinQ. It configures the panel, sets up the DSI commands,
and initializes the DSC parameters.
> diff --git a/drivers/gpu/drm/panel/panel-lg-sw49410.c b/drivers/gpu/drm/panel/panel-lg-sw49410.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-lg-sw49410.c
[ ... ]
> +static int sw49410_panel_program(struct sw49410_panel *ctx)
> +{
[ ... ]
> + ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> + drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
> +
> + ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> + mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
[Severity: Medium]
Does this code incorrectly clear and restore the MIPI_DSI_MODE_LPM flag?
The drm_dsc_pps_payload_pack() function merely formats a local structure
in memory and performs no hardware DSI transactions, so manipulating DSI
hardware transmission flags around it seems unnecessary.
By restoring the flag immediately after packing, does it cause the
subsequent mipi_dsi_picture_parameter_set_multi() call to execute in Low
Power Mode rather than the intended High Speed mode?
[ ... ]
> +static int sw49410_panel_unprepare(struct drm_panel *panel)
> +{
> + struct sw49410_panel *ctx = to_sw49410_panel(panel);
> +
> + gpiod_set_value(ctx->reset_gpio, 1);
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
[Severity: High]
Is it intentional to assert the reset GPIO before sending the sleep
commands?
Because the display controller is held in hardware reset, it cannot receive
or process the display off and sleep commands, which could cause the DSI
host's transmissions to fail or time out.
Looking further down in this function, the reset GPIO is asserted a second
time, which suggests this first call might be misplaced.
[ ... ]
> +static int sw49410_panel_probe(struct mipi_dsi_device *dsi)
> +{
[ ... ]
> + drm_panel_add(&ctx->panel);
> +
> + /* This panel only supports DSC; unconditionally enable it */
> + dsi->dsc = &ctx->dsc;
> +
> + ctx->dsc.dsc_version_major = 1;
> + ctx->dsc.dsc_version_minor = 1;
[Severity: Medium]
Could this cause a race condition where a concurrently probing display
driver reads uninitialized DSC settings?
Since drm_panel_add() makes the panel visible globally, another driver
could find it before the DSC parameters like slice_width and
bits_per_component are fully populated.
Should drm_panel_add() be called as the final step of initialization?
[ ... ]
> + ctx->dsc.slice_count = 1440 / ctx->dsc.slice_width;
> + ctx->dsc.bits_per_component = 8;
> + ctx->dsc.bits_per_pixel = 8 << 4; /* 4 fractional bits */
> + ctx->dsc.block_pred_enable = true;
> +
> + return mipi_dsi_attach(dsi);
> +}
[Severity: High]
Does returning the error from mipi_dsi_attach() without calling
drm_panel_remove() leave a dangling pointer in the global panel list?
If mipi_dsi_attach() fails, the probe function returns the error directly.
Because the panel context is allocated via devm_drm_panel_alloc(), it will
be automatically freed by the driver core.
However, since drm_panel_add() was already called, this freed memory would
remain linked in the global DRM panel list, leading to a use-after-free
the next time the list is traversed.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-judyln-panel-v3-0-07f4134441bd@postmarketos.org?part=1
^ permalink raw reply
* Re: [PATCH v3 2/4] dt-bindings: display: panel: Add documentation for lg,sw49410-lh609qh1
From: sashiko-bot @ 2026-06-15 0:17 UTC (permalink / raw)
To: Paul Sajna; +Cc: dri-devel, devicetree, robh, conor+dt
In-Reply-To: <20260614-judyln-panel-v3-2-07f4134441bd@postmarketos.org>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sending DSI commands in `unprepare` after asserting the reset GPIO will cause DSI transmission timeouts.
- [High] The `MIPI_DSI_MODE_LPM` flag toggle for the PPS transfer is wrapped around the wrong function, rendering it ineffective.
- [Medium] The `compatible` property definition enforces an incorrect generic-first string order and mismatches the device tree example, causing validation to fail.
--
commit faf055f392f23751b6b923b1d2e80556b912da18
Author: Paul Sajna <sajattack@postmarketos.org>
dt-bindings: display: panel: Add documentation for lg,sw49410-lh609qh1
This commit documents the bindings for the LG LH609QH1 MIPI DSI panel. It
also introduces the DRM panel driver for the SW49410 controller found in the
LG G7 ThinQ smartphone.
> diff --git a/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml b/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml
> new file mode 100644
> index 0000000000000..4c4bf4d89353f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml
> @@ -0,0 +1,79 @@
[ ... ]
> +properties:
> + compatible:
> + items:
> + - const: lg,sw49410
> + - const: lg,sw49410-lh609qh1
[Severity: Medium]
Does the compatible array enforce a generic-to-specific order?
Device tree standard typically dictates specific-to-generic ordering.
Additionally, since the device tree example further down only provides a
single string:
compatible = "lg,sw49410-lh609qh1";
Will this single-string example fail validation against the two-item schema
constraint defined here?
> +
> + reg:
[ ... ]
> diff --git a/drivers/gpu/drm/panel/panel-lg-sw49410.c b/drivers/gpu/drm/panel/panel-lg-sw49410.c
> new file mode 100644
> index 0000000000000..02d1b85c3affb
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-lg-sw49410.c
> @@ -0,0 +1,528 @@
[ ... ]
> +static int sw49410_panel_program(struct sw49410_panel *ctx)
> +{
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> + struct drm_dsc_picture_parameter_set pps;
[ ... ]
> + ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> + drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
> +
> + ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> + mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
[Severity: High]
Are the MIPI_DSI_MODE_LPM flags being toggled around the right function?
This code wraps the local memory packing function drm_dsc_pps_payload_pack()
instead of the actual transmission function
mipi_dsi_picture_parameter_set_multi().
Does this mean the PPS command is still incorrectly sent in low-power mode?
> +
> + mipi_dsi_compression_mode_ext_multi(&dsi_ctx, true, MIPI_DSI_COMPRESSION_DSC, 1);
[ ... ]
> +static int sw49410_panel_unprepare(struct drm_panel *panel)
> +{
> + struct sw49410_panel *ctx = to_sw49410_panel(panel);
> +
> + gpiod_set_value(ctx->reset_gpio, 1);
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
[Severity: High]
Could asserting the reset GPIO before sending DSI commands lead to
transmission timeouts?
Because the panel is already put into reset by gpiod_set_value(..., 1),
it seems like it won't be able to acknowledge the display_off and
enter_sleep_mode commands sent immediately after.
> + mipi_dsi_msleep(&dsi_ctx, 100);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-judyln-panel-v3-0-07f4134441bd@postmarketos.org?part=2
^ permalink raw reply
* Re: [PATCH v3 4/4] Revert "dt-bindings: display: panel: panel-simple: Add lg,sw49410 compatible"
From: sashiko-bot @ 2026-06-15 0:17 UTC (permalink / raw)
To: Paul Sajna; +Cc: conor+dt, dri-devel, robh, devicetree
In-Reply-To: <20260614-judyln-panel-v3-4-07f4134441bd@postmarketos.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing drm_panel_remove() on the mipi_dsi_attach() error path in probe causes a Use-After-Free.
- [High] Panel reset is erroneously asserted before sending DSI sleep commands in the unprepare sequence.
--
commit 591497f012912dfaa02eb5ec6c5c1e0d8e61f126
Author: Paul Sajna <sajattack@postmarketos.org>
Revert "dt-bindings: display: panel: panel-simple: Add lg,sw49410 compatible"
This reverts commit a74c2e55ab66519ffa2069ac9ae83cd937bff4c4. The panel
isn't actually panel-simple-compatible.
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> index 3e41ed0ef5d51..1abdc8698a865 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> @@ -200,8 +200,6 @@ properties:
> - lemaker,bl035-rgb-002
> # LG 7" (800x480 pixels) TFT LCD panel
> - lg,lb070wv8
> - # LG 6.1" (1440x3120) IPS LCD panel
> - - lg,sw49410
[Severity: High]
This isn't a bug introduced by this patch, but while looking at the sw49410
panel driver code, I noticed a missing drm_panel_remove() on the
mipi_dsi_attach() error path in probe which causes a use-after-free:
drivers/gpu/drm/panel/panel-lg-sw49410.c:sw49410_panel_probe() {
drm_panel_add(&ctx->panel);
/* This panel only supports DSC; unconditionally enable it */
dsi->dsc = &ctx->dsc;
...
return mipi_dsi_attach(dsi);
}
If mipi_dsi_attach() fails, the function returns an error without calling
drm_panel_remove(). Because ctx was allocated with devm_drm_panel_alloc(),
the kernel devres mechanism will free ctx upon probe failure.
This leaves a dangling pointer in the global panel_list. Any subsequent
operations iterating over panel_list (such as another driver calling
drm_panel_get()) will dereference the freed memory, causing a
use-after-free kernel panic.
Could we add a call to drm_panel_remove() to the error path here?
[Severity: High]
This is a pre-existing issue, but the panel reset is erroneously asserted
before sending DSI sleep commands in the unprepare sequence:
drivers/gpu/drm/panel/panel-lg-sw49410.c:sw49410_panel_unprepare() {
gpiod_set_value(ctx->reset_gpio, 1);
struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
}
The reset GPIO is asserted (gpiod_set_value(ctx->reset_gpio, 1)) before
executing the MIPI DCS sleep commands.
Attempting to send MIPI DCS commands to a panel that is held in hardware
reset will cause the DSI host to fail to communicate, resulting in severe
command timeouts and error logs. The unprepare sequence is functionally
broken because the panel cannot gracefully enter sleep mode.
Should the panel reset be delayed until after the DSI sleep commands are
sent?
> # Logic Technologies LT161010-2NHC 7" WVGA TFT Cap Touch Module
> - logictechno,lt161010-2nhc
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-judyln-panel-v3-0-07f4134441bd@postmarketos.org?part=4
^ permalink raw reply
* [PATCH v3 4/4] Revert "dt-bindings: display: panel: panel-simple: Add lg,sw49410 compatible"
From: Paul Sajna @ 2026-06-15 0:08 UTC (permalink / raw)
To: Neil Armstrong, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thierry Reding, Sam Ravnborg,
Jessica Zhang, Jessica Zhang
Cc: linux-kernel, dri-devel, devicetree, David Heidelberg,
phone-devel, Amir Dahan, Paul Sajna
In-Reply-To: <20260614-judyln-panel-v3-0-07f4134441bd@postmarketos.org>
This reverts commit a74c2e55ab66519ffa2069ac9ae83cd937bff4c4.
It isn't actually panel-simple-compatible, and we knew as much by the
time this merged, but I guess maintainers needed something to prevent
the dt-bindings error going off while I was working on these patches.
Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
Documentation/devicetree/bindings/display/panel/panel-simple.yaml | 2 --
1 file changed, 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
index 21453f0d7ce2..58303a767a36 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
@@ -204,8 +204,6 @@ properties:
- lemaker,bl035-rgb-002
# LG 7" (800x480 pixels) TFT LCD panel
- lg,lb070wv8
- # LG 6.1" (1440x3120) IPS LCD panel
- - lg,sw49410
# Logic Technologies LT161010-2NHC 7" WVGA TFT Cap Touch Module
- logictechno,lt161010-2nhc
# Logic Technologies LT161010-2NHR 7" WVGA TFT Resistive Touch Module
--
2.54.0
^ permalink raw reply related
* [PATCH v3 3/4] MAINTAINERS: add Paul Sajna as maintainer for lg,sw49410-lh609qh1
From: Paul Sajna @ 2026-06-15 0:08 UTC (permalink / raw)
To: Neil Armstrong, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thierry Reding, Sam Ravnborg,
Jessica Zhang, Jessica Zhang
Cc: linux-kernel, dri-devel, devicetree, David Heidelberg,
phone-devel, Amir Dahan, Paul Sajna
In-Reply-To: <20260614-judyln-panel-v3-0-07f4134441bd@postmarketos.org>
Add myself (Paul Sajna) as maintainer for lg,sw49410-lh609qh1
Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index c9f2780b2d31..5c043fb66af0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8134,6 +8134,12 @@ T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
F: Documentation/devicetree/bindings/display/panel/lg,sw43408.yaml
F: drivers/gpu/drm/panel/panel-lg-sw43408.c
+DRM DRIVER FOR LG SW49410 PANELS
+M: Paul Sajna <sajattack@postmarketos.org>
+S: Maintained
+F: Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml
+F: drivers/gpu/drm/panel/panel-lg-sw49410.c
+
DRM DRIVER FOR LOGICVC DISPLAY CONTROLLER
M: Paul Kocialkowski <paulk@sys-base.io>
S: Supported
--
2.54.0
^ permalink raw reply related
* [PATCH v3 0/4] Add DRM driver for LG LH609QH1 Panel with SiliconWorks SW49410 DDIC
From: Paul Sajna @ 2026-06-15 0:07 UTC (permalink / raw)
To: Neil Armstrong, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thierry Reding, Sam Ravnborg,
Jessica Zhang, Jessica Zhang
Cc: linux-kernel, dri-devel, devicetree, David Heidelberg,
phone-devel, Amir Dahan, Paul Sajna
This patch series adds a drm panel driver for the LG SW49410 panel found
in the LG G7 ThinQ (codename judyln).
The basic driver skeleton was generated by https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator
from the vendor device-tree.
There seems to still be some power supply issues, the bottom-left corner
of the screen is dark, and the rest of the screen develops shadow-y
burn-in-like patterns when resumed after being left off for a while.
https://i.imgur.com/oJZSHzE.jpeg
Comments were added explaining magic numbers, MAINTAINERS updated, and devicetree
documentation added
Co-developed-by: Amir Dahan <system64fumo@tuta.io>
Signed-off-by: Amir Dahan <system64fumo@tuta.io>
Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
Changes in v3:
- Add power supplies
- Use full panel name including panel and DDIC
- Update email addresses
- Various improvements copied from other similar upstream panels
- Switch back to not using panel-simple due to power supply properties
- Revert incorrect merged panel-simple bindings
- Link to v2: https://lore.kernel.org/r/20250915-judyln-panel-v2-0-01ab2199fea5@postmarketos.org
Changes in v2:
- use "multi" versions of functions
- remove DRM_DISPLAY_DP_HELPER
- change dt-bindings to panel-simple
- Link to v1: https://lore.kernel.org/r/20250910-judyln-panel-v1-0-825c74403bbb@postmarketos.org
---
Amir Dahan (1):
drm: panel: Add LG LH609QH1 Panel with SW49410 controller
Paul Sajna (3):
dt-bindings: display: panel: Add documentation for lg,sw49410-lh609qh1
MAINTAINERS: add Paul Sajna as maintainer for lg,sw49410-lh609qh1
Revert "dt-bindings: display: panel: panel-simple: Add lg,sw49410 compatible"
.../bindings/display/panel/lg,sw49410.yaml | 79 +++
.../bindings/display/panel/panel-simple.yaml | 2 -
MAINTAINERS | 6 +
drivers/gpu/drm/panel/Kconfig | 15 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-lg-sw49410.c | 528 +++++++++++++++++++++
6 files changed, 629 insertions(+), 2 deletions(-)
---
base-commit: c9b2552e124828f40d5102b146dc72b506b65de9
change-id: 20250910-judyln-panel-948f177c5c5c
Best regards,
--
Paul Sajna <sajattack@postmarketos.org>
^ permalink raw reply
* [PATCH v3 2/4] dt-bindings: display: panel: Add documentation for lg,sw49410-lh609qh1
From: Paul Sajna @ 2026-06-15 0:07 UTC (permalink / raw)
To: Neil Armstrong, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thierry Reding, Sam Ravnborg,
Jessica Zhang, Jessica Zhang
Cc: linux-kernel, dri-devel, devicetree, David Heidelberg,
phone-devel, Amir Dahan, Paul Sajna
In-Reply-To: <20260614-judyln-panel-v3-0-07f4134441bd@postmarketos.org>
Document how to use lg,sw49410-lh609qh1 in a devicetree
Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
.../bindings/display/panel/lg,sw49410.yaml | 79 ++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml b/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml
new file mode 100644
index 000000000000..4c4bf4d89353
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/lg,sw49410.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LG LH609QH1 MIPI-DSI panel with SW49410 controller
+
+maintainers:
+ - Paul Sajna <sajattack@postmarketos.org>
+
+description:
+ LG LH609QH1 6.1" 1440x3120 MIPI DSI panel with SW49410 controller found in LG G7 ThinQ smartphone.
+
+allOf:
+ - $ref: panel-common.yaml#
+
+properties:
+ compatible:
+ items:
+ - const: lg,sw49410
+ - const: lg,sw49410-lh609qh1
+
+ reg:
+ maxItems: 1
+
+ backlight:
+ description: Backlight device reference
+
+ reset-gpios:
+ description: Reset pin reference
+
+ vsp-supply:
+ description: Positive voltage supply
+
+ vsn-supply:
+ description: Negative voltage supply
+
+required:
+ - compatible
+ - reg
+ - reset-gpios
+ - port
+ - vsp-supply
+ - vsn-supply
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ dsi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ panel@0 {
+ reg = <0>;
+ compatible = "lg,sw49410-lh609qh1";
+
+ backlight = <&pmi8998_wled>;
+ reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
+ width-mm = <65>;
+ height-mm = <140>;
+
+ vsp-supply = <&lab>;
+ vsn-supply = <&ibb>;
+
+ pinctrl-0 = <&sde_dsi_active &sde_te_active_sleep>;
+ pinctrl-1 = <&sde_dsi_sleep &sde_te_active_sleep>;
+ pinctrl-names = "default", "sleep";
+
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <&mdss_dsi0_out>;
+ };
+ };
+ };
+ };
--
2.54.0
^ permalink raw reply related
* [PATCH v3 1/4] drm: panel: Add LG LH609QH1 Panel with SW49410 controller
From: Paul Sajna @ 2026-06-15 0:07 UTC (permalink / raw)
To: Neil Armstrong, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thierry Reding, Sam Ravnborg,
Jessica Zhang, Jessica Zhang
Cc: linux-kernel, dri-devel, devicetree, David Heidelberg,
phone-devel, Amir Dahan, Paul Sajna
In-Reply-To: <20260614-judyln-panel-v3-0-07f4134441bd@postmarketos.org>
From: Amir Dahan <system64fumo@tuta.io>
Add panel driver used by LG G7 ThinQ (judyln)
Signed-off-by: Amir Dahan <system64fumo@tuta.io>
Co-developed-by: Paul Sajna <sajattack@postmarketos.org>
Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
drivers/gpu/drm/panel/Kconfig | 15 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-lg-sw49410.c | 528 +++++++++++++++++++++++++++++++
3 files changed, 544 insertions(+)
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 7450b27622a2..ecf6a45224d3 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -498,6 +498,21 @@ config DRM_PANEL_LG_SW43408
pixel. It provides a MIPI DSI interface to the host and has a
built-in LED backlight.
+config DRM_PANEL_LG_SW49410
+ tristate "LG SW49410 panel"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ depends on BACKLIGHT_CLASS_DEVICE
+ select DRM_DISPLAY_DSC_HELPER
+ select DRM_DISPLAY_HELPER
+ help
+ Say Y here if you want to enable support for LG/SiliconWorks SW49410 controller
+ (found in LG G7 ThinQ).
+ The LH609QH1 panel has a 1440x3120@60Hz resolution and uses 24 bit RGB per
+ pixel. It provides a MIPI DSI interface to the host and has a
+ built-in LED backlight.
+ To compile this driver as a module, choose M here.
+
config DRM_PANEL_LXD_M9189A
tristate "LXD M9189A MIPI-DSI LCD panel"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index c2c5cf817116..153970480269 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
obj-$(CONFIG_DRM_PANEL_LG_LD070WX3) += panel-lg-ld070wx3.o
obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
obj-$(CONFIG_DRM_PANEL_LG_SW43408) += panel-lg-sw43408.o
+obj-$(CONFIG_DRM_PANEL_LG_SW49410) += panel-lg-sw49410.o
obj-$(CONFIG_DRM_PANEL_LXD_M9189A) += panel-lxd-m9189a.o
obj-$(CONFIG_DRM_PANEL_MAGNACHIP_D53E6EA8966) += panel-magnachip-d53e6ea8966.o
obj-$(CONFIG_DRM_PANEL_MOTOROLA_MOT) += panel-motorola-mot.o
diff --git a/drivers/gpu/drm/panel/panel-lg-sw49410.c b/drivers/gpu/drm/panel/panel-lg-sw49410.c
new file mode 100644
index 000000000000..02d1b85c3aff
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-lg-sw49410.c
@@ -0,0 +1,528 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Generated with linux-mdss-dsi-panel-driver-generator from vendor device tree:
+// Copyright (c) 2025, The Linux Foundation. All rights reserved.
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/display/drm_dsc.h>
+#include <drm/display/drm_dsc_helper.h>
+
+static const struct regulator_bulk_data sw49410_supplies[] = {
+ { .supply = "vsp"},
+ { .supply = "vsn"},
+};
+
+
+struct sw49410_panel {
+ struct drm_panel panel;
+ struct mipi_dsi_device *dsi;
+ struct drm_dsc_config dsc;
+
+ struct regulator_bulk_data *supplies;
+
+ struct gpio_desc *reset_gpio;
+};
+
+static inline
+struct sw49410_panel *to_sw49410_panel(struct drm_panel *panel)
+{
+ return container_of(panel, struct sw49410_panel, panel);
+}
+
+static void sw49410_panel_reset(struct sw49410_panel *ctx)
+{
+ gpiod_set_value(ctx->reset_gpio, 0);
+ usleep_range(9000, 10000);
+ gpiod_set_value(ctx->reset_gpio, 1);
+ usleep_range(1000, 2000);
+ gpiod_set_value(ctx->reset_gpio, 0);
+ usleep_range(9000, 10000);
+}
+
+static int sw49410_panel_program(struct sw49410_panel *ctx)
+{
+ struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
+ struct drm_dsc_picture_parameter_set pps;
+
+
+ mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+ mipi_dsi_dcs_set_page_address_multi(&dsi_ctx, 0x0000, 0x0c2f);
+ mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, 0x00ff);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_CONTROL_DISPLAY,
+ 0x2c);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_CABC_MIN_BRIGHTNESS,
+ 0x00);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_POWER_SAVE, 0x81);
+
+ /* Manufacturer protection */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0xac);
+
+ /* Source Control */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb3,
+ 0x04, 0x04, 0x28, 0x08, 0x5a, 0x12, 0x23,
+ 0x02);
+
+ /* Gate & Mux Control */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb4,
+ 0x11, 0x04, 0x02, 0x02, 0x02, 0x02, 0x02,
+ 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0xd0,
+ 0xe4, 0xe4, 0xe4, 0x93, 0x4e, 0x39, 0x0a,
+ 0x10, 0x18, 0x25, 0x24, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00);
+
+ /* Sync Setup */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb5,
+ 0x2e, 0x0f, 0x10, 0xc0, 0x00, 0x10, 0xc0,
+ 0x00);
+
+ /* Panel Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6,
+ 0x03, 0x05, 0x0b, 0xb3, 0x30);
+
+ /* Touch Timing Control */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb8,
+ 0x57, 0x02, 0x90, 0x40, 0x5d, 0xd0, 0x05,
+ 0x00, 0x00, 0x18, 0x22, 0x04, 0x01, 0x02,
+ 0x90, 0x40, 0x4c, 0xc0, 0x04, 0x00, 0x00,
+ 0x18, 0x22, 0x04, 0x01, 0x08, 0x00, 0x3a,
+ 0x86, 0x83, 0x00);
+
+ /* Touch Source Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb9,
+ 0x64, 0x64, 0x2a, 0x3f, 0xee);
+
+ /* DSC Configuration */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xba,
+ 0x3d, 0x1f, 0x01, 0xff, 0x01, 0x3c, 0x1f,
+ 0x01, 0xff, 0x01, 0x00);
+
+ /* Low Rate Refresh Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xbc, 0x00, 0x00, 0x00, 0x90);
+
+ /* Black Frame Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xbd, 0x00, 0x00);
+
+ /* U2 Corner Down */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xbf, 0x4f, 0x02);
+
+ /* Internal Oscillator Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xc0,
+ 0x00, 0x04, 0x18, 0x07, 0x11, 0x11, 0x3c,
+ 0x00, 0x0a, 0x0a);
+
+ /* Power Control1 */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xc1,
+ 0x01, 0x00, 0xf0, 0xc2, 0xcf, 0x0c);
+
+ /* Power Control2 */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xc2,
+ 0xcc, 0x44, 0x44, 0x20, 0x22, 0x26, 0x21,
+ 0x00);
+
+ /* Power Control3 */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xc3,
+ 0x92, 0x11, 0x09, 0x09, 0x11, 0xcc, 0x02,
+ 0x02, 0xa4, 0xa4, 0x02, 0xa2, 0x38, 0x28,
+ 0x14, 0x40, 0x38, 0xc0);
+
+ /* Vcom Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xc4, 0x26, 0x00);
+
+ /* Power Sequence Option Configuration */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xc9,
+ 0x05, 0x5d, 0x03, 0x04, 0x00);
+
+ /* Abrupt Power Off Control */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca, 0x9b, 0x10);
+
+ /* LFD Control */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcb,
+ 0xf3, 0x90, 0x3d, 0x30, 0xcc);
+
+ /* Tail TFT Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc,
+ 0x00, 0x40, 0x50, 0x90, 0x41);
+
+ /* U2 Option */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xce, 0x00, 0x00);
+
+ /* Gamma */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xd0,
+ 0x12, 0x05, 0x20, 0x1b, 0x2c, 0x28, 0x3f,
+ 0x3d, 0x4f, 0x4f, 0x66, 0x66, 0x6e, 0x6e,
+ 0x76, 0x76, 0x80, 0x80, 0x88, 0x88, 0x95,
+ 0x95, 0x3f, 0x3f, 0xa2, 0xa2, 0x94, 0x94,
+ 0x8b, 0x8b, 0x81, 0x81, 0x75, 0x75, 0x66,
+ 0x66, 0x47, 0x47, 0x2d, 0x2d, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2c, 0x28, 0x3f,
+ 0x3d, 0x4f, 0x4f, 0x66, 0x66, 0x6e, 0x6e,
+ 0x76, 0x76, 0x80, 0x80, 0x88, 0x88, 0x95,
+ 0x95, 0x3f, 0x3f, 0xa2, 0xa2, 0x94, 0x94,
+ 0x8b, 0x8b, 0x81, 0x81, 0x75, 0x75, 0x66,
+ 0x66, 0x47, 0x47, 0x2d, 0x2d, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2c, 0x28, 0x3f,
+ 0x3d, 0x4f, 0x4f, 0x66, 0x66, 0x6e, 0x6e,
+ 0x76, 0x76, 0x80, 0x80, 0x88, 0x88, 0x95,
+ 0x95, 0x3f, 0x3f, 0xa2, 0xa2, 0x94, 0x94,
+ 0x8b, 0x8b, 0x81, 0x81, 0x75, 0x75, 0x66,
+ 0x66, 0x47, 0x47, 0x2d, 0x2d, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2c, 0x28, 0x3f,
+ 0x3d, 0x4f, 0x4f, 0x66, 0x66, 0x6e, 0x6e,
+ 0x76, 0x76, 0x80, 0x80, 0x88, 0x88, 0x94,
+ 0x94, 0x3f, 0x3f, 0xa4, 0xa4, 0x95, 0x95,
+ 0x8b, 0x8b, 0x81, 0x81, 0x75, 0x75, 0x66,
+ 0x66, 0x47, 0x47, 0x2d, 0x2d, 0x00, 0x01);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xd1,
+ 0x12, 0x05, 0x20, 0x1b, 0x2e, 0x29, 0x41,
+ 0x3f, 0x52, 0x52, 0x6a, 0x6a, 0x72, 0x72,
+ 0x7a, 0x7a, 0x84, 0x84, 0x8c, 0x8c, 0x9a,
+ 0x9a, 0x3f, 0x3f, 0x9b, 0x9b, 0x8d, 0x8d,
+ 0x84, 0x84, 0x7a, 0x7a, 0x6e, 0x6e, 0x5f,
+ 0x5f, 0x41, 0x41, 0x2a, 0x2a, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2e, 0x29, 0x41,
+ 0x3f, 0x52, 0x52, 0x6a, 0x6a, 0x72, 0x72,
+ 0x7a, 0x7a, 0x84, 0x84, 0x8c, 0x8c, 0x9a,
+ 0x9a, 0x3f, 0x3f, 0x9b, 0x9b, 0x8d, 0x8d,
+ 0x84, 0x84, 0x7a, 0x7a, 0x6e, 0x6e, 0x5f,
+ 0x5f, 0x41, 0x41, 0x2a, 0x2a, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2e, 0x29, 0x41,
+ 0x3f, 0x52, 0x52, 0x6a, 0x6a, 0x72, 0x72,
+ 0x7a, 0x7a, 0x84, 0x84, 0x8c, 0x8c, 0x9a,
+ 0x9a, 0x3f, 0x3f, 0x9b, 0x9b, 0x8d, 0x8d,
+ 0x84, 0x84, 0x7a, 0x7a, 0x6e, 0x6e, 0x5f,
+ 0x5f, 0x41, 0x41, 0x2a, 0x2a, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2e, 0x29, 0x41,
+ 0x3f, 0x52, 0x52, 0x6a, 0x6a, 0x72, 0x72,
+ 0x7a, 0x7a, 0x84, 0x84, 0x8c, 0x8c, 0x9a,
+ 0x9a, 0x3f, 0x3f, 0x9b, 0x9b, 0x8d, 0x8d,
+ 0x84, 0x84, 0x7a, 0x7a, 0x6e, 0x6e, 0x5f,
+ 0x5f, 0x41, 0x41, 0x2a, 0x2a, 0x00, 0x01);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xd2,
+ 0x12, 0x05, 0x20, 0x1b, 0x2f, 0x2a, 0x43,
+ 0x41, 0x55, 0x55, 0x6e, 0x6e, 0x76, 0x76,
+ 0x7e, 0x7e, 0x88, 0x88, 0x90, 0x90, 0x9f,
+ 0x9f, 0x3f, 0x3f, 0x95, 0x95, 0x86, 0x86,
+ 0x7d, 0x7d, 0x74, 0x74, 0x68, 0x68, 0x59,
+ 0x59, 0x3c, 0x3c, 0x26, 0x26, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2f, 0x2a, 0x43,
+ 0x41, 0x55, 0x55, 0x6e, 0x6e, 0x76, 0x76,
+ 0x7e, 0x7e, 0x88, 0x88, 0x90, 0x90, 0x9f,
+ 0x9f, 0x3f, 0x3f, 0x95, 0x95, 0x86, 0x86,
+ 0x7d, 0x7d, 0x74, 0x74, 0x68, 0x68, 0x59,
+ 0x59, 0x3c, 0x3c, 0x26, 0x26, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2f, 0x2a, 0x43,
+ 0x41, 0x55, 0x55, 0x6e, 0x6e, 0x76, 0x76,
+ 0x7e, 0x7e, 0x88, 0x88, 0x90, 0x90, 0x9f,
+ 0x9f, 0x3f, 0x3f, 0x95, 0x95, 0x86, 0x86,
+ 0x7d, 0x7d, 0x74, 0x74, 0x68, 0x68, 0x59,
+ 0x59, 0x3c, 0x3c, 0x26, 0x26, 0x00, 0x01,
+ 0x12, 0x05, 0x20, 0x1b, 0x2f, 0x2a, 0x43,
+ 0x41, 0x55, 0x55, 0x6e, 0x6e, 0x76, 0x76,
+ 0x7e, 0x7e, 0x88, 0x88, 0x90, 0x90, 0x9f,
+ 0x9f, 0x3f, 0x3f, 0x95, 0x95, 0x86, 0x86,
+ 0x7d, 0x7d, 0x74, 0x74, 0x68, 0x68, 0x59,
+ 0x59, 0x3c, 0x3c, 0x26, 0x26, 0x00, 0x01);
+
+ /* MPLUS Control */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xd3, 0x12, 0x01, 0x00, 0x00);
+
+ /* MPLUS Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xd4,
+ 0xdc, 0x5f, 0x9c, 0xbe, 0x39, 0x39, 0x39,
+ 0x47, 0x48, 0x48, 0x48, 0x3a, 0x00, 0x03,
+ 0x6d, 0x80, 0x00, 0x00, 0x8c, 0x66, 0x00,
+ 0x00, 0x8c, 0x66, 0x00, 0x00, 0x8c, 0x66,
+ 0x00, 0x0a, 0x48, 0x80, 0x00, 0x0a, 0x48,
+ 0x80, 0x00, 0x0a, 0x48, 0x80, 0x00, 0x0a,
+ 0x48, 0x80, 0x20, 0x0a, 0x14, 0x0a, 0x18,
+ 0x00, 0x1c, 0xcc, 0x23, 0x9e, 0x23, 0x9e,
+ 0x01, 0x01, 0x01, 0x01, 0x04, 0x04, 0x04,
+ 0x04, 0x01, 0x00, 0x02, 0x80, 0x00, 0x10,
+ 0x00, 0x10, 0x00, 0x10, 0x13, 0x9e, 0x13,
+ 0x9e, 0x13, 0x9e, 0x13, 0x9e, 0x05, 0x05,
+ 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x23,
+ 0x9e, 0xff, 0xff, 0x13, 0x33, 0x18, 0x00,
+ 0x16, 0x66, 0x10, 0x00, 0xff, 0x01, 0x00,
+ 0x02, 0x00, 0x03, 0x00, 0x04, 0x00, 0x05,
+ 0x00, 0x06, 0x00, 0x07, 0x00, 0x08, 0x00,
+ 0x09, 0x00, 0x0a, 0x00, 0x0b, 0x00, 0x0c,
+ 0x00, 0x0d, 0x00, 0x0e, 0x00, 0x0f, 0x00,
+ 0x1b, 0x25, 0xdc, 0x18, 0x00, 0x20, 0x00,
+ 0x1c, 0xe1, 0x00, 0xff, 0xe0, 0xc8, 0xc8,
+ 0x41, 0x8f);
+
+ /* Notch Up Gradation */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xad,
+ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x06,
+ 0x06, 0x06, 0x06, 0x06, 0x06, 0x06, 0x06,
+ 0x06, 0x06, 0x20, 0x40, 0x60, 0x90, 0xc0,
+ 0xf0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff);
+
+ /* Notch Down Gradation */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xae,
+ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x06,
+ 0x06, 0x06, 0x06, 0x06, 0x06, 0x06, 0x06,
+ 0x06, 0x06, 0x20, 0x40, 0x60, 0x90, 0xc0,
+ 0xf0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff);
+
+ /* GIP Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xe5,
+ 0x0b, 0x0a, 0x0c, 0x00, 0x02, 0x04, 0x06,
+ 0x08, 0x0f, 0x1b, 0x02, 0x1a, 0x1a, 0x0b,
+ 0x0a, 0x0c, 0x01, 0x03, 0x05, 0x07, 0x09,
+ 0x10, 0x1b, 0x03, 0x1a, 0x1a);
+
+ /* Mux Setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xe6,
+ 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17,
+ 0x18, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16,
+ 0x17, 0x18);
+
+ /* Test1 */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xed,
+ 0x21, 0x49, 0x00, 0x00, 0x00, 0x00);
+
+ /* BLU Control */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_POWER_SAVE, 0x81);
+
+ /* Sharpness */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf3,
+ 0x00, 0x01, 0x00, 0x0d, 0x00);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf4,
+ 0x00, 0x00, 0x40, 0x83, 0xc5, 0x00, 0x01,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfb,
+ 0x20, 0x40, 0x60, 0x80, 0xa0, 0xc0, 0xe0,
+ 0x13, 0x18, 0x18, 0x18, 0x16, 0x0d, 0x0d,
+ 0x00, 0xc7, 0xcf, 0xd8, 0xe1, 0xea, 0xf3,
+ 0xf9, 0xff);
+
+ /* Gamma Correction */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf5, 0x00);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf6,
+ 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40,
+ 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7,
+ 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40,
+ 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf8,
+ 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40,
+ 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00);
+
+ /* BLU PWM Control */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc,
+ 0x13, 0x70, 0xd0, 0x26, 0x30, 0x7c, 0x02,
+ 0xff, 0x12, 0x22, 0x22, 0x10, 0x00);
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_ENTER_NORMAL_MODE);
+ mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
+ mipi_dsi_msleep(&dsi_ctx, 135);
+
+ /* Black frame setting */
+ mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xbd, 0x01, 0x05);
+
+ mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
+ mipi_dsi_msleep(&dsi_ctx, 50);
+
+ ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+ drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
+
+ ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+ mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
+
+ mipi_dsi_compression_mode_ext_multi(&dsi_ctx, true, MIPI_DSI_COMPRESSION_DSC, 1);
+
+ return dsi_ctx.accum_err;
+}
+
+static int sw49410_panel_disable(struct drm_panel *panel)
+{
+ struct sw49410_panel *ctx = to_sw49410_panel(panel);
+ struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
+
+ ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+ mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+ mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
+ mipi_dsi_msleep(&dsi_ctx, 128);
+
+ return dsi_ctx.accum_err;
+}
+
+static int sw49410_panel_prepare(struct drm_panel *panel)
+{
+ struct sw49410_panel *ctx = to_sw49410_panel(panel);
+ int ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(sw49410_supplies), ctx->supplies);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(5000, 6000);
+
+ sw49410_panel_reset(ctx);
+
+ ret = sw49410_panel_program(ctx);
+ if (ret)
+ goto poweroff;
+
+ return 0;
+
+poweroff:
+ gpiod_set_value(ctx->reset_gpio, 1);
+ regulator_bulk_disable(ARRAY_SIZE(sw49410_supplies), ctx->supplies);
+ return ret;
+}
+
+static int sw49410_panel_unprepare(struct drm_panel *panel)
+{
+ struct sw49410_panel *ctx = to_sw49410_panel(panel);
+
+ gpiod_set_value(ctx->reset_gpio, 1);
+ struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
+
+ mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+ mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
+ mipi_dsi_msleep(&dsi_ctx, 100);
+
+ gpiod_set_value(ctx->reset_gpio, 1);
+
+ regulator_bulk_disable(ARRAY_SIZE(sw49410_supplies), ctx->supplies);
+
+ return dsi_ctx.accum_err;
+}
+
+static const struct drm_display_mode sw49410_panel_mode = {
+ .clock = (1440 + 168 + 4 + 84) * (3120 + 2 + 18 + 18) * 60 / 1000,
+ .hdisplay = 1440,
+ .hsync_start = 1440 + 168,
+ .hsync_end = 1440 + 168 + 4,
+ .htotal = 1440 + 168 + 4 + 84,
+ .vdisplay = 3120,
+ .vsync_start = 3120 + 2,
+ .vsync_end = 3120 + 2 + 18,
+ .vtotal = 3120 + 2 + 18 + 18,
+ .width_mm = 65,
+ .height_mm = 140,
+ .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static int sw49410_panel_get_modes(struct drm_panel *panel, struct drm_connector *connector)
+{
+ return drm_connector_helper_get_modes_fixed(connector, &sw49410_panel_mode);
+}
+
+static const struct drm_panel_funcs sw49410_panel_funcs = {
+ .disable = sw49410_panel_disable,
+ .prepare = sw49410_panel_prepare,
+ .unprepare = sw49410_panel_unprepare,
+ .get_modes = sw49410_panel_get_modes,
+};
+
+static int sw49410_panel_probe(struct mipi_dsi_device *dsi)
+{
+ struct device *dev = &dsi->dev;
+ struct sw49410_panel *ctx;
+ int ret;
+
+ ctx = devm_drm_panel_alloc(&dsi->dev, __typeof(*ctx), panel,
+ &sw49410_panel_funcs, DRM_MODE_CONNECTOR_DSI);
+
+ if (IS_ERR(ctx))
+ return PTR_ERR(ctx);
+
+ ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(sw49410_supplies),
+ sw49410_supplies,
+ &ctx->supplies
+ );
+
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
+ ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(ctx->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
+ "Failed to get reset-gpios\n");
+
+ ctx->dsi = dsi;
+ mipi_dsi_set_drvdata(dsi, ctx);
+
+ dsi->lanes = 4;
+ dsi->format = MIPI_DSI_FMT_RGB888;
+ dsi->mode_flags = MIPI_DSI_MODE_VIDEO_BURST |
+ MIPI_DSI_CLOCK_NON_CONTINUOUS;
+
+ ctx->panel.prepare_prev_first = true;
+
+ ret = drm_panel_of_backlight(&ctx->panel);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to get backlight\n");
+
+ drm_panel_add(&ctx->panel);
+
+ /* This panel only supports DSC; unconditionally enable it */
+ dsi->dsc = &ctx->dsc;
+
+ ctx->dsc.dsc_version_major = 1;
+ ctx->dsc.dsc_version_minor = 1;
+
+ ctx->dsc.slice_height = 60;
+ ctx->dsc.slice_width = 720;
+
+ WARN_ON(1440 % ctx->dsc.slice_width);
+ ctx->dsc.slice_count = 1440 / ctx->dsc.slice_width;
+ ctx->dsc.bits_per_component = 8;
+ ctx->dsc.bits_per_pixel = 8 << 4; /* 4 fractional bits */
+ ctx->dsc.block_pred_enable = true;
+
+ return mipi_dsi_attach(dsi);
+}
+
+static void sw49410_panel_remove(struct mipi_dsi_device *dsi)
+{
+ struct sw49410_panel *ctx = mipi_dsi_get_drvdata(dsi);
+ int ret;
+
+ ret = mipi_dsi_detach(dsi);
+ if (ret < 0)
+ dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
+
+ drm_panel_remove(&ctx->panel);
+}
+
+static const struct of_device_id sw49410_of_match[] = {
+ { .compatible = "lg,sw49410" },
+ { .compatible = "lg,sw49410-lh609qh1" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sw49410_of_match);
+
+static struct mipi_dsi_driver sw49410_panel_driver = {
+ .driver = {
+ .name = "panel-lg-sw49410",
+ .of_match_table = sw49410_of_match,
+ },
+ .probe = sw49410_panel_probe,
+ .remove = sw49410_panel_remove,
+};
+module_mipi_dsi_driver(sw49410_panel_driver);
+
+MODULE_AUTHOR("Amir Dahan <system64fumo@tuta.io>");
+MODULE_DESCRIPTION("DRM driver for LG DSI Panel with SW49410 controller");
+MODULE_LICENSE("GPL");
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 1/5] dt-bindings: iio: adc: Add TI ADS126x ADC family
From: David Lechner @ 2026-06-15 0:06 UTC (permalink / raw)
To: Kurt Borja, Krzysztof Kozlowski
Cc: Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, Nuno Sá, Andy Shevchenko,
linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <DJ93WSYC3HTT.3NXQW390CLQ82@gmail.com>
On 6/14/26 4:57 PM, Kurt Borja wrote:
> On Sun Jun 14, 2026 at 4:37 PM -05, David Lechner wrote:
>> On 6/14/26 3:53 PM, Kurt Borja wrote:
>>
>> ...
>>
>>>> Not a separate device node. Fold into the parent... or explain in
>>>> commit msg. You have entire commit msg to explain odd things.
>>>>
>>>> In that binding description you call it "independent", so it should have
>>>> its own SPI chip select? Why "independent" and part of this binding?
>>>> Maybe not independent, so basically part of this device?
>>>
>>> It's independent in the sense that it is a proper subdevice on the same
>>> chip. It shares the serial interface but operates completely in
>>> parallel.
>>>
>>> I decided to add a subnode because other devices might request their
>>> io-channels and most importantly a different voltage reference might be
>>> connected to it.
>>>
>>> I'll clarify this in the commmit message on the next version. Although
>>> after seeing this submitted bindings [1], I wonder if it's a better
>>> approach to do something like
>>>
>>> spi@0 {
>>> mydevice@0 {
>>> ...
>>> adc@0 { ... };
>>> adc@1 { ... };
>>> };
>>> };
>>>
>>> Any thoughts?
>>
>> I don't see how this relates to the linked patch at all. The linked
>> patch looks just like a normal DAC binding.
>
> Ah, wrong link. This is the correct one [1]. The suggestion just at the
> end.
>
>>
>> What is the point of the 2nd ADC in this chip? Is it just to be able
>> to do simultaneous sampling of two different measurements at the same
>> time? We have other simultaneous sampling ADC chips and just model them
>> as a single device.
>
> It does simultaneous sampling of the same channel, as well as different
> channels. Also the secondary ADC is only 24 bit instead of 32 bit, has a
> different noise profile and has a different PGA configuration (goes up
> to 128 gain, instead of 32).
>
> Taken from the datasheet (Section 9.3.15):
>
> Use ADC2 to perform main channel (ADC1) cross-checking
> measurements (for example, diagnostics purposes and redundant
> channel measurements), system background measurements, or
> temperature compensation of the primary sensor (such as
> thermocouple cold junction compensation). Using data rates of
> 10, 100, and 400 SPS for both ADCs, ADC2 performs virtual
> parallel conversions with ADC1 on the same input channel.
>
Ah, that is the kind of info I was looking for.
>>
>> Since everything can be muxed to either ADC at runtime, I don't see
>> any reason the devicetree should care about it. Forcing certain pins
>> to be assigned to a certain ADC seems overly restrictive.
>>
>> And unless you have an application that specifically needs it, I
>> wouldn't bother trying to implement the 2nd ADC in the IIO driver.
>> I didn't see any hints in the datasheet as to when it would actually
>> make sense to use this 2nd ADC. My first thought is that it might
>> make sense to use the 2nd ADC for a 2nd buffer so that you can do
>> 2 buffered reads at the same time. But without knowing why this chip
>> was designed this way, I don't know if that is the right idea or not.
>
> I myself don't have an application for this feature. But I don't see why
> not adding support for this feature, given that I already implemented a
> driver (Patch 5) and is capable, as you said, of 2 buffered reads at the
> same time.
>
> I do believe I have to explain all this better in commit messages
> though.
I still think we don't need anything special in the devicetree
though. Other than #io-channels-cells = <2>; where the 2nd cell
would be which ADC the channel is routed through when the consumer
reads it.
Otherwise, we would just have to duplicate all channels exactly
in both the adc@0 and adc@1 node (otherwise we would just be
making artificial limitations).
>
>>
>>
>>> Ack to the rest of comments.
>>>
>>> [1] https://lore.kernel.org/linux-iio/20260519-ad5529r-driver-v3-1-267c0731aa68@analog.com/
>>>
>
> [1] https://lore.kernel.org/linux-iio/25mh6grzh7zh3b4uytcqnusyv5zjuf6ia4if3ce3oqzqz56ehi@le72iqv7ye3d/
>
^ permalink raw reply
* Re: [PATCH v7 4/5] iio: adc: versal-sysmon: add threshold event support
From: sashiko-bot @ 2026-06-14 23:48 UTC (permalink / raw)
To: Salih Erim; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260614233722.2603459-5-salih.erim@amd.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect devm teardown sequence allows the delayed worker to re-enable hardware interrupts after the IRQ handler is freed, causing an unhandled interrupt storm.
- [High] Writing a supply voltage threshold replaces the entire hardware register with the 16-bit mantissa, silently clearing the channel's format and mode bits.
- [High] Voltage threshold events are silently disabled in hardware after they fire, making them unexpectedly one-shot.
- [Medium] Unbounded sysfs threshold values cause signed integer overflows and undefined behavior during conversion.
- [Medium] Converting negative temperature thresholds writes 1s to the reserved upper 16 bits of the hardware threshold registers.
--
commit 4db3b58bd3561251c48f5d1c527c846b332e37fd
Author: Salih Erim <salih.erim@amd.com>
iio: adc: versal-sysmon: add threshold event support
This commit adds threshold event support for temperature and supply voltage
channels in the versal-sysmon driver. It enables rising thresholds and
hysteresis for temperature channels, and rising/falling thresholds for supply
voltage channels. It also sets up interrupt handling, including a delayed
worker for polling and re-arming temperature events.
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 5b7439e443fca..16feb373bc704 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
[ ... ]
> @@ -51,6 +100,16 @@ static const struct iio_chan_spec temp_channels[] = {
> 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_millicelsius_to_q8p7(u32 *raw_data, int val)
> +{
> + *raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / (int)MILLI;
[Severity: Medium]
Will this 32-bit left shift overflow for large sysfs input values?
Since val is a 32-bit signed integer, an overflow could lead to undefined
behavior during the conversion. Is a cast to a 64-bit integer needed before
the arithmetic operation?
[Severity: Medium]
If a negative temperature threshold is provided, does this calculation
sign-extend the negative integer and write 1s to the upper 16 bits?
The negative 32-bit integer is stored in the u32 pointer. Later in
sysmon_write_event_value(), this unmasked value is written directly to the
threshold register:
sysmon_write_event_value() {
...
sysmon_millicelsius_to_q8p7(&raw_val, val);
ret = regmap_write(sysmon->regmap, SYSMON_TEMP_TH_UP, raw_val);
...
}
Could this overwrite reserved hardware fields or adjacent configuration bits
in the upper 16 bits of the register?
> +}
> +
> +static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
> +{
[ ... ]
> @@ -68,6 +127,34 @@ 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;
[Severity: Medium]
Can this 32-bit multiplication overflow when val is a large sysfs input?
Since scale can be up to 65536, multiplying it by the 32-bit signed integer
val might overflow. Should this use a 64-bit cast before the multiplication
to prevent undefined behavior?
> +
> + if (format)
> + tmp = clamp(tmp, S16_MIN, S16_MAX);
> + else
> + tmp = clamp(tmp, 0, U16_MAX);
> +
> + *raw_data = (u16)tmp;
[Severity: High]
Does this discard the extracted mode and format bits by returning only the
16-bit mantissa?
When sysmon_write_event_value() writes this value back to the hardware, it uses
regmap_write() which overwrites the entire register:
sysmon_write_event_value() {
...
sysmon_supply_processedtoraw(val, reg_val, &raw_val);
return regmap_write(sysmon->regmap, offset, raw_val);
}
Will this overwrite the entire 32-bit threshold register and silently clear
the channel's format and mode bits?
> +}
> +
> +static int sysmon_supply_thresh_offset(int address,
[ ... ]
> @@ -127,24 +473,244 @@ static int sysmon_read_label(struct iio_dev *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_TEMP_MAX);
> +
> + ret = regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_TEMP));
> + if (ret)
> + return ret;
> +
> + sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
> + return 0;
> +
> + 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, ®_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_clear_bits(sysmon->regmap, alarm_reg_offset, BIT(bit));
[Severity: High]
Does clearing the alarm configuration bit here permanently disable the voltage
event after it fires?
Unlike the temperature alarms which use a delayed worker to poll and unmask,
there doesn't appear to be a mechanism to re-arm voltage alarms once the
condition clears. Does this make the voltage threshold events unexpectedly
one-shot, rather than remaining enabled until explicitly disabled by the user?
> + if (ret)
> + return ret;
> + }
> +
> + return regmap_write(sysmon->regmap, alarm_flag_offset, alarm_flag_reg);
> +
> + default:
> + return -EINVAL;
> + }
> +}
[ ... ]
> +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;
> +
> + ret = devm_request_irq(dev, irq, sysmon_iio_irq, 0, "sysmon-irq", indio_dev);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(dev, sysmon_disable_interrupts, sysmon);
[Severity: High]
Does this devm teardown sequence allow the delayed worker to execute after the
IRQ handler has been freed?
Since devm actions execute in reverse order, the IRQ is freed before the
delayed worker is canceled. If sysmon_unmask_worker() runs during this
window, it can unmask the hardware interrupt by calling sysmon_unmask_temp():
sysmon_unmask_worker() -> sysmon_unmask_temp():
...
regmap_write(sysmon->regmap, SYSMON_IER, unmask);
Could this trigger an unhandled interrupt storm since the IRQ handler is no
longer registered?
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260614233722.2603459-1-salih.erim@amd.com?part=4
^ permalink raw reply
* Re: [PATCH v7 5/5] iio: adc: versal-sysmon: add oversampling support
From: sashiko-bot @ 2026-06-14 23:47 UTC (permalink / raw)
To: Salih Erim; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260614233722.2603459-6-salih.erim@amd.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Software cache for oversampling ratio is not synchronized with hardware state.
--
commit 10aa21fe8c20849d9568e9f27df70f27c183249f
Author: Salih Erim <salih.erim@amd.com>
iio: adc: versal-sysmon: add oversampling support
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).
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 16feb373bc704..d9599a7bc8978 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
[ ... ]
> @@ -839,6 +988,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]
Will this cause the software cache to be out of sync with the hardware if the
bootloader or firmware previously enabled oversampling?
Since these defaults are not written to the hardware registers during probe,
and the actual hardware state is not read back to populate the cache, the IIO
sysfs interface might incorrectly report a ratio of 1 while the hardware is
actually averaging samples.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260614233722.2603459-1-salih.erim@amd.com?part=5
^ permalink raw reply
* [PATCH v7 5/5] iio: adc: versal-sysmon: add oversampling support
From: Salih Erim @ 2026-06-14 23:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
In-Reply-To: <20260614233722.2603459-1-salih.erim@amd.com>
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 v7:
- Split sysmon_osr_write into sysmon_osr_write_temp and
sysmon_osr_write_supply; caller dispatches with if/else
on chan->type (Jonathan)
- Restore HW encoding comment in both helpers; fix
cross-reference in sysmon_osr_write_supply
Changes in v6:
- Fix FIELD_PREP indentation in sysmon_osr_write (Andy)
- unsigned int for loop index in sysmon_write_raw (Andy)
Changes in v5:
- Remove unneeded parentheses in i * SYSMON_REG_STRIDE (Andy)
- Use struct regmap *map local variable in
sysmon_set_avg_enable (Andy)
- switch instead of redundant if/if on channel_type (Andy)
- Add CONFIG register readback fence after oversampling update
to prevent NoC bus hang from posted writes (found during
hardware stress testing)
Changes in v4:
- Return directly from sysmon_set_avg_enable calls, remove
else after early returns, drop unreachable return 0 (Jonathan)
- Rename mask defines to SYSMON_CONFIG_SUPPLY_OSR and
SYSMON_CONFIG_TEMP_SAT_OSR (Jonathan)
- Drop "bits X:Y" from GENMASK comments (Jonathan)
- Blank lines after if (ret) return ret blocks (Jonathan)
- Move oversampling read inside guard(mutex) scope
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 | 153 ++++++++++++++++++++++++++-
drivers/iio/adc/versal-sysmon.h | 17 +++
2 files changed, 169 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
index 16feb373bc7..d9599a7bc89 100644
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -26,6 +26,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 };
+
/* TEMP hysteresis mode bit in SYSMON_TEMP_EV_CFG */
#define SYSMON_TEMP_HYST_MASK BIT(1)
@@ -165,6 +171,12 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
guard(mutex)(&sysmon->lock);
+ if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO) {
+ *val = (chan->type == IIO_TEMP) ? sysmon->temp_oversampling :
+ sysmon->supply_oversampling;
+ return IIO_VAL_INT;
+ }
+
switch (chan->type) {
case IIO_TEMP:
if (mask == IIO_CHAN_INFO_SCALE) {
@@ -460,6 +472,132 @@ static int sysmon_write_event_value(struct iio_dev *indio_dev,
}
}
+static int sysmon_set_avg_enable(struct sysmon *sysmon,
+ u32 base, u32 count, u32 val)
+{
+ struct regmap *map = sysmon->regmap;
+ int ret;
+
+ for (unsigned int i = 0; i < count; i++) {
+ ret = regmap_write(map, base + i * SYSMON_REG_STRIDE, val);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int sysmon_osr_write_temp(struct sysmon *sysmon, 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;
+ unsigned int readback;
+ int ret;
+
+ ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
+ SYSMON_CONFIG_TEMP_SAT_OSR,
+ FIELD_PREP(SYSMON_CONFIG_TEMP_SAT_OSR, hw_val));
+ if (ret)
+ return ret;
+
+ /*
+ * Readback fence: the SysMon CONFIG register resides in the
+ * PMC domain behind the NoC. A posted write may not reach the
+ * hardware before the next MMIO access. Reading the register
+ * back forces the interconnect to complete the write, preventing
+ * a bus hang on the subsequent access.
+ */
+ regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
+
+ return sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
+ SYSMON_TEMP_EN_AVG_COUNT,
+ hw_val ? ~0U : 0);
+}
+
+static int sysmon_osr_write_supply(struct sysmon *sysmon, int val)
+{
+ /* HW encoding: sample_count / 2 (see sysmon_osr_write_temp) */
+ int hw_val = val >> 1;
+ unsigned int readback;
+ int ret;
+
+ ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
+ SYSMON_CONFIG_SUPPLY_OSR,
+ FIELD_PREP(SYSMON_CONFIG_SUPPLY_OSR, hw_val));
+ if (ret)
+ return ret;
+
+ /* Readback fence -- see sysmon_osr_write_temp for details */
+ regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
+
+ return sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
+ SYSMON_SUPPLY_EN_AVG_COUNT,
+ hw_val ? ~0U : 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);
+ unsigned int i;
+ int 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);
+
+ if (chan->type == IIO_TEMP) {
+ ret = sysmon_osr_write_temp(sysmon, val);
+ if (ret)
+ return ret;
+ sysmon->temp_oversampling = val;
+ } else {
+ ret = sysmon_osr_write_supply(sysmon, val);
+ if (ret)
+ return ret;
+ 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)
@@ -472,6 +610,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,
@@ -761,6 +902,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
.indexed = 1,
.address = reg,
.info_mask_separate = 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 = irq > 0 ?
sysmon_supply_events : NULL,
.num_event_specs = irq > 0 ?
@@ -792,7 +937,11 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
.address = SYSMON_TEMP_SAT_BASE +
(reg - 1) * SYSMON_REG_STRIDE,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
- .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_type =
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+ .info_mask_shared_by_type_available =
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
.datasheet_name = label,
};
}
@@ -839,6 +988,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 ca3df017dbb..409dd9298fb 100644
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -23,11 +23,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_SUPPLY_TH_LOW 0x1980
@@ -37,6 +39,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 */
@@ -53,6 +56,16 @@ struct regmap;
/* ISR/IMR temperature alarm mask (bit 9) */
#define SYSMON_TEMP_INTR_MASK BIT(9)
+/* SYSMON_CONFIG: supply oversampling ratio */
+#define SYSMON_CONFIG_SUPPLY_OSR GENMASK(17, 14)
+
+/* SYSMON_CONFIG: temperature satellite oversampling ratio */
+#define SYSMON_CONFIG_TEMP_SAT_OSR 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)
@@ -77,6 +90,8 @@ struct regmap;
* @temp_mask: temperature interrupt configuration mask
* @temp_hysteresis: cached DEVICE_TEMP 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;
@@ -96,6 +111,8 @@ struct sysmon {
unsigned int temp_mask;
int temp_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
^ permalink raw reply related
* [PATCH v7 4/5] iio: adc: versal-sysmon: add threshold event support
From: Salih Erim @ 2026-06-14 23:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
In-Reply-To: <20260614233722.2603459-1-salih.erim@amd.com>
Add threshold event support for temperature and supply voltage
channels.
Temperature events:
- Rising threshold with configurable value on the device
temperature channel (current max across all satellites)
- 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 hardware also provides a separate over-temperature (OT)
threshold, but it is not exposed through IIO as it serves as a
hardware safety mechanism for platform shutdown. OT will be
exposed through the thermal framework in a follow-up series.
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
specs are not attached 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.
A devm cleanup action masks all interrupts on driver unbind to
prevent unhandled interrupt storms after the IRQ handler is freed.
Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in v7:
- Move TEMP threshold event onto channel 0; drop OT as
separate IIO channel -- OT is a hardware safety mechanism
better suited for the thermal framework follow-up (Jonathan)
- Use single temp_channels array; attach event spec to
channel 0 at runtime when IRQ is available, matching the
pattern used for supply channels (Jonathan)
- Remove sysmon_temp_thresh_offset; use SYSMON_TEMP_TH_UP
and SYSMON_TEMP_TH_LOW defines directly at call sites
- Return administrative state from temp_mask in
read_event_config instead of transient hardware IMR
(Jonathan, Sashiko)
- Add devm_add_action_or_reset to mask all HW interrupts
on driver unbind (Sashiko)
- Remove SYSMON_CHAN_TEMP_EVENT macro, SYSMON_ADDR_TEMP_EVENT,
SYSMON_ADDR_OT_EVENT, SYSMON_BIT_OT, SYSMON_OT_HYST_MASK,
OT_TH_LOW/UP registers, ot_hysteresis from struct
- Simplify sysmon_get_event_mask, sysmon_update_temp_lower,
sysmon_init_hysteresis -- all now operate on single TEMP
channel only
Changes in v6:
- Remove types.h from header (not needed at any stage) (Andy)
- Macro brace on separate line for SYSMON_CHAN_TEMP_EVENT (Andy)
- switch(chan->type) in all event functions instead of cascading
if statements (Andy)
- switch(info) in read/write_event_value for nested
dispatch (Andy)
- Reversed xmas tree in sysmon_update_temp_lower and
sysmon_init_hysteresis (Andy)
- scoped_guard(spinlock_irq) with error check in
sysmon_unmask_worker (Andy)
- Combined regmap_read error check with || in
sysmon_iio_irq (Andy)
- Join devm_request_irq on one line (Andy)
- Fix fwnode_irq_get() to propagate only -EPROBE_DEFER;
treating all negatives as fatal broke probe on I2C nodes
without interrupts property
Changes in v5:
- clamp() instead of clamp_t() (Andy)
- regmap_assign_bits() instead of separate set/clear (Andy)
- Remove unneeded parentheses (2 places) (Andy)
- for_each_set_bit on single line (Andy)
- regmap_clear_bits() instead of regmap_update_bits() (Andy)
- Simplify unmask XOR to ~status & masked_temp (Andy)
- Add comment explaining unmask &= ~temp_mask logic (Andy)
- Split container_of across two lines (Andy)
- Move ISR write after !isr check to avoid writing 0 (Andy)
- unsigned int for init_hysteresis address param (Andy)
- Add comment explaining error check policy in worker/IRQ (Andy)
- Nested size_add() for overflow-safe allocation (Andy)
- Propagate negative from fwnode_irq_get() for
EPROBE_DEFER (Andy)
- Pass irq instead of has_irq to sysmon_parse_fw (Andy)
Changes in v4:
- Merge event channels into static temp array; two arrays
(with/without events) selected by has_irq (Jonathan)
- Event-only channels have no info_mask; their addresses are
logical identifiers, not readable registers
- Drop RAW for voltage events, keep PROCESSED only (Jonathan)
- Drop scan_type from event channel macro (Jonathan)
- Blank lines between call+error-check blocks (Jonathan)
- Fit under 80 chars on one line where possible (Jonathan)
- default case returns -EINVAL instead of break (Jonathan)
- sysmon_handle_event: return early in each case (Jonathan)
- guard(spinlock) in sysmon_iio_irq, return IRQ_NONE/IRQ_HANDLED
directly (Jonathan)
- Take irq_lock in write_event_config for temp_mask updates to
synchronize with unmask worker (Sashiko)
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 | 616 ++++++++++++++++++++++++++-
drivers/iio/adc/versal-sysmon.h | 36 ++
2 files changed, 644 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
index 5b7439e443f..16feb373bc7 100644
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -11,7 +11,9 @@
#include <linux/bitops.h>
#include <linux/cleanup.h>
#include <linux/device.h>
+#include <linux/devm-helpers.h>
#include <linux/err.h>
+#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/property.h>
#include <linux/regmap.h>
@@ -19,10 +21,18 @@
#include <linux/sysfs.h>
#include <linux/units.h>
+#include <linux/iio/events.h>
#include <linux/iio/iio.h>
#include "versal-sysmon.h"
+/* TEMP hysteresis mode bit in SYSMON_TEMP_EV_CFG */
+#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)
+
#define SYSMON_CHAN_TEMP(_chan, _address, _name) \
{ \
.type = IIO_TEMP, \
@@ -34,6 +44,45 @@
.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_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).
*
@@ -51,6 +100,16 @@ static const struct iio_chan_spec temp_channels[] = {
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_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;
@@ -68,6 +127,34 @@ 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(tmp, S16_MIN, S16_MAX);
+ else
+ tmp = clamp(tmp, 0, U16_MAX);
+
+ *raw_data = (u16)tmp;
+}
+
+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)
@@ -114,6 +201,265 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
}
}
+static u32 sysmon_get_event_mask(const struct iio_chan_spec *chan)
+{
+ if (chan->type == IIO_TEMP)
+ return BIT(SYSMON_BIT_TEMP);
+
+ return BIT(chan->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, ®_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);
+
+ return regmap_assign_bits(sysmon->regmap, offset, BIT(shift), enable);
+}
+
+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)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int imr;
+ int config_value;
+ u32 mask;
+ int ret;
+
+ mask = sysmon_get_event_mask(chan);
+
+ ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+ if (ret)
+ return ret;
+
+ /* IMR bits are 1=masked, invert to get 1=enabled */
+ imr = ~imr;
+
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ config_value = sysmon_read_alarm_config(sysmon, chan->address);
+ if (config_value < 0)
+ return config_value;
+ return config_value && (imr & mask);
+
+ case IIO_TEMP:
+ /*
+ * Return the administrative state, not the hardware IMR.
+ * The IRQ handler temporarily masks the interrupt during
+ * the polling window; reading IMR would show it as disabled.
+ * temp_mask bit is set when administratively disabled.
+ */
+ return !(sysmon->temp_mask & mask);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+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);
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ u32 ier = sysmon_get_event_mask(chan);
+ unsigned int alarm_config;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ switch (chan->type) {
+ case 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);
+
+ case IIO_TEMP:
+ if (state) {
+ ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->temp_mask &= ~ier;
+ } else {
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->temp_mask |= ier;
+ }
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+/*
+ * 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)
+{
+ unsigned int upper_reg;
+ int upper_mc, lower_mc;
+ u32 raw_val;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &upper_reg);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+
+ lower_mc = upper_mc - sysmon->temp_hysteresis;
+ sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
+
+ return regmap_write(sysmon->regmap, SYSMON_TEMP_TH_LOW, 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);
+
+ switch (chan->type) {
+ case IIO_TEMP:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(reg_val, val);
+
+ return IIO_VAL_INT;
+
+ case IIO_EV_INFO_HYSTERESIS:
+ *val = sysmon->temp_hysteresis;
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+
+ case IIO_VOLTAGE:
+ offset = sysmon_supply_thresh_offset(chan->address, dir);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_rawtoprocessed(reg_val, val);
+
+ return IIO_VAL_INT;
+
+ default:
+ 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);
+
+ switch (chan->type) {
+ case IIO_TEMP:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ sysmon_millicelsius_to_q8p7(&raw_val, val);
+
+ ret = regmap_write(sysmon->regmap, SYSMON_TEMP_TH_UP, raw_val);
+ if (ret)
+ return ret;
+
+ /* Recompute lower = upper - hysteresis */
+ return sysmon_update_temp_lower(sysmon);
+
+ case IIO_EV_INFO_HYSTERESIS:
+ if (val < 0)
+ return -EINVAL;
+
+ sysmon->temp_hysteresis = val;
+
+ return sysmon_update_temp_lower(sysmon);
+
+ default:
+ return -EINVAL;
+ }
+
+ case IIO_VOLTAGE:
+ offset = sysmon_supply_thresh_offset(chan->address, dir);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_processedtoraw(val, reg_val, &raw_val);
+
+ return regmap_write(sysmon->regmap, offset, raw_val);
+
+ default:
+ return -EINVAL;
+ }
+}
+
static int sysmon_read_label(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
char *label)
@@ -127,24 +473,244 @@ 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_TEMP_MAX);
+
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_TEMP));
+ if (ret)
+ return ret;
+
+ sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
+ return 0;
+
+ 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, ®_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_clear_bits(sysmon->regmap, alarm_reg_offset, BIT(bit));
+ if (ret)
+ return ret;
+ }
+
+ return regmap_write(sysmon->regmap, alarm_flag_offset, alarm_flag_reg);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+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 = ~status & sysmon->masked_temp;
+ sysmon->masked_temp &= status;
+
+ /* Only unmask if not administratively disabled by userspace */
+ 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;
+
+ /*
+ * If the ISR read fails, skip processing to avoid acting
+ * on undefined data.
+ */
+ scoped_guard(spinlock_irq, &sysmon->irq_lock) {
+ if (regmap_read(sysmon->regmap, SYSMON_ISR, &isr))
+ break;
+ regmap_write(sysmon->regmap, SYSMON_ISR, isr);
+ sysmon_unmask_temp(sysmon, isr);
+ }
+
+ 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 = iio_priv(indio_dev);
+ unsigned int isr, imr;
+
+ guard(spinlock)(&sysmon->irq_lock);
+
+ if (regmap_read(sysmon->regmap, SYSMON_ISR, &isr) ||
+ regmap_read(sysmon->regmap, SYSMON_IMR, &imr))
+ return IRQ_NONE;
+
+ isr &= ~imr;
+ if (!isr)
+ return IRQ_NONE;
+
+ regmap_write(sysmon->regmap, SYSMON_ISR, isr);
+
+ sysmon_handle_events(indio_dev, isr);
+ schedule_delayed_work(&sysmon->sysmon_unmask_work,
+ msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
+
+ return IRQ_HANDLED;
+}
+
+static void sysmon_disable_interrupts(void *data)
+{
+ struct sysmon *sysmon = data;
+
+ regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
+}
+
+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;
+
+ ret = devm_request_irq(dev, irq, sysmon_iio_irq, 0, "sysmon-irq", indio_dev);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, sysmon_disable_interrupts, sysmon);
+}
+
+/*
+ * 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 *hysteresis)
+{
+ unsigned int upper_reg, lower_reg;
+ int upper_mc, lower_mc;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &upper_reg);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_LOW, &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
+ * @irq: IRQ number (positive enables event channels, 0 disables)
*
* 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,
+ int irq)
{
- unsigned int num_chan, idx, temp_chan_idx, volt_chan_idx;
- unsigned int num_supply, num_temp;
+ unsigned int num_chan, num_static, idx, temp_chan_idx, volt_chan_idx;
struct iio_chan_spec *sysmon_channels;
+ unsigned int num_supply, num_temp;
const char *label;
u32 reg;
int ret;
@@ -157,14 +723,21 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
device_get_named_child_node(dev, "temperature-channels");
num_temp = fwnode_get_child_node_count(temp_node);
- num_chan = size_add(num_temp, size_add(ARRAY_SIZE(temp_channels), num_supply));
+ num_static = ARRAY_SIZE(temp_channels);
+ num_chan = size_add(num_temp, size_add(num_static, num_supply));
sysmon_channels = devm_kcalloc(dev, num_chan, sizeof(*sysmon_channels), GFP_KERNEL);
if (!sysmon_channels)
return -ENOMEM;
- /* Static temperature channels first */
memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
- idx = ARRAY_SIZE(temp_channels);
+
+ /* Attach event spec to channel 0 when IRQ is available */
+ if (irq > 0) {
+ sysmon_channels[0].event_spec = sysmon_temp_events;
+ sysmon_channels[0].num_event_specs = ARRAY_SIZE(sysmon_temp_events);
+ }
+
+ idx = num_static;
/* Supply channels from DT */
fwnode_for_each_child_node_scoped(supply_node, child) {
@@ -188,6 +761,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
.indexed = 1,
.address = reg,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .event_spec = irq > 0 ?
+ sysmon_supply_events : NULL,
+ .num_event_specs = irq > 0 ?
+ ARRAY_SIZE(sysmon_supply_events) : 0,
.datasheet_name = label,
};
}
@@ -253,6 +830,7 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
{
struct iio_dev *indio_dev;
struct sysmon *sysmon;
+ int irq;
int ret;
indio_dev = devm_iio_device_alloc(dev, sizeof(*sysmon));
@@ -265,6 +843,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);
@@ -274,13 +853,34 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
if (ret)
return ret;
+ irq = fwnode_irq_get(dev_fwnode(dev), 0);
+ if (irq == -EPROBE_DEFER)
+ return dev_err_probe(dev, irq, "failed to get IRQ\n");
+
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, irq);
if (ret)
return ret;
+ if (irq > 0) {
+ /* Set hysteresis mode for temperature threshold */
+ ret = regmap_set_bits(sysmon->regmap, SYSMON_TEMP_EV_CFG,
+ SYSMON_TEMP_HYST_MASK);
+ if (ret)
+ return ret;
+
+ /* Initialize cached hysteresis from hardware registers */
+ ret = sysmon_init_hysteresis(sysmon, &sysmon->temp_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 0c0c89f6e0c..ca3df017dbb 100644
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -11,6 +11,8 @@
#include <linux/bits.h>
#include <linux/mutex.h>
+#include <linux/spinlock_types.h>
+#include <linux/workqueue.h>
struct device;
struct regmap;
@@ -18,12 +20,22 @@ 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_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
@@ -35,8 +47,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 alarm mask (bit 9) */
+#define SYSMON_TEMP_INTR_MASK BIT(9)
+
/* Supply voltage conversion register fields */
#define SYSMON_MANTISSA_MASK GENMASK(15, 0)
#define SYSMON_FMT_MASK BIT(16)
@@ -46,11 +62,21 @@ struct regmap;
#define SYSMON_FRACTIONAL_SHIFT 7U
#define SYSMON_SUPPLY_MANTISSA_BITS 16
+/* 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 read-modify-write sequences on threshold registers
* and cached state that spans multiple regmap calls
+ * @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
+ * @sysmon_unmask_work: re-enables events after alarm condition clears
*/
struct sysmon {
struct regmap *regmap;
@@ -60,6 +86,16 @@ struct sysmon {
* that spans multiple regmap calls.
*/
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;
+ struct delayed_work sysmon_unmask_work;
};
int sysmon_core_probe(struct device *dev, struct regmap *regmap);
--
2.48.1
^ permalink raw reply related
* [PATCH v7 3/5] iio: adc: versal-sysmon: add I2C driver
From: Salih Erim @ 2026-06-14 23:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
In-Reply-To: <20260614233722.2603459-1-salih.erim@amd.com>
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 v7:
- No code changes
Changes in v6:
- Add types.h include (IWYU) (Andy)
- Add local struct device *dev, join devm_regmap_init on
one line (Andy)
Changes in v5:
- Add err.h, mod_devicetable.h includes (IWYU) (Andy)
Changes in v4:
- Replace enum with defines for I2C frame offsets (Jonathan)
- Use get_unaligned_le32() for read data reassembly (Jonathan)
- Use put_unaligned_le32/le16() for write data and register offset
packing (Jonathan)
- Named initializer in i2c_device_id (Jonathan)
- Drop bitfield.h, add unaligned.h (FIELD_GET/FIELD_PREP replaced
by unaligned accessors)
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 | 128 ++++++++++++++++++++++++++++
3 files changed, 142 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..1bc969597b4
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon-i2c.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Versal SysMon I2C driver
+ *
+ * Copyright (C) 2023 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+
+#include "versal-sysmon.h"
+
+#define SYSMON_I2C_INSTR_READ BIT(2)
+#define SYSMON_I2C_INSTR_WRITE BIT(3)
+
+/*
+ * I2C command frame layout (8 bytes):
+ * [0..3] data payload (little-endian u32)
+ * [4..5] register offset >> 2 (little-endian u16)
+ * [6] instruction (read/write)
+ * [7] reserved
+ */
+#define SYSMON_I2C_DATA_OFS 0
+#define SYSMON_I2C_REG_OFS 4
+#define SYSMON_I2C_INSTR_OFS 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;
+
+ put_unaligned_le16(reg >> 2, &write_buf[SYSMON_I2C_REG_OFS]);
+ write_buf[SYSMON_I2C_INSTR_OFS] = 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 = get_unaligned_le32(read_buf);
+
+ 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;
+
+ put_unaligned_le32(val, &write_buf[SYSMON_I2C_DATA_OFS]);
+ put_unaligned_le16(reg >> 2, &write_buf[SYSMON_I2C_REG_OFS]);
+ write_buf[SYSMON_I2C_INSTR_OFS] = 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 device *dev = &client->dev;
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init(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(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[] = {
+ { .name = "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
^ permalink raw reply related
* [PATCH v7 2/5] iio: adc: add Versal SysMon driver
From: Salih Erim @ 2026-06-14 23:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
In-Reply-To: <20260614233722.2603459-1-salih.erim@amd.com>
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 hardware also provides four aggregate temperature registers that
are always present regardless of the device tree configuration: the
current max and min across all active satellites, and the peak and
trough values recorded since the last hardware reset.
The driver is split into two 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 v7:
- Add documentation comment explaining the four static aggregate
temperature registers (Jonathan)
Changes in v6:
- Macro brace on separate line for SYSMON_CHAN_TEMP (Andy)
- Remove unneeded = 0 init on num_supply, num_temp (Andy)
- Use temporary variable for channel count allocation (Andy)
- Initialize idx directly after memcpy (Andy)
- Join info_mask_separate on one line (Andy)
- Join info_mask_shared_by_type on one line (Andy)
- Join devm_kcalloc on one line (Andy)
- Reversed xmas tree ordering in sysmon_parse_fw
Changes in v5:
- Add err.h include to core (IWYU) (Andy)
- Drop (int) cast on MILLI in scale assignment (Andy)
- sign_extend32() instead of (s16) cast for temperature raw (Andy)
- Remove unneeded parentheses in voltage address calculation (Andy)
- Drop NULL checks before fwnode_get_child_node_count (Andy)
- Nested size_add() for overflow-safe allocation (Andy)
- if (ret) instead of if (ret < 0) for fwnode property reads (Andy)
- Remove outer parentheses in satellite address calculation (Andy)
- Loop index declared in for() scope (Andy)
- MMIO: add err.h, types.h includes (IWYU) (Andy)
- Header: remove unused types.h include and struct iio_dev
forward declaration (Andy)
Changes in v4:
- Temperature: RAW + SCALE (IIO_VAL_FRACTIONAL, 1000/128) instead
of PROCESSED (Jonathan)
- Voltage: PROCESSED only, drop RAW (Jonathan)
- Drop scan_type from all channel macros (Jonathan)
- Move __free(fwnode_handle) declarations down to just above use
(Jonathan)
- devm_regmap_init() on one line (Jonathan)
- Lock comment: describe RMW sequences and cached state (Jonathan)
- Remove sysmon_q8p7_to_millicelsius() from this patch; the function
is now introduced in P4 where it is first used
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 | 290 +++++++++++++++++++++++++++
drivers/iio/adc/versal-sysmon.c | 93 +++++++++
drivers/iio/adc/versal-sysmon.h | 67 +++++++
6 files changed, 479 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..5b7439e443f
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -0,0 +1,290 @@
+// 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/err.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"
+
+#define SYSMON_CHAN_TEMP(_chan, _address, _name) \
+{ \
+ .type = IIO_TEMP, \
+ .indexed = 1, \
+ .address = _address, \
+ .channel = _chan, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .datasheet_name = _name, \
+}
+
+/*
+ * Static temperature channels (always present).
+ *
+ * These are hardware-computed aggregate registers across all active
+ * temperature satellites:
+ * temp: current max temperature across all active satellites
+ * min: current min temperature across all active satellites
+ * max_max: highest peak recorded since last hardware reset
+ * min_min: lowest trough recorded since last hardware reset
+ */
+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_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;
+
+ guard(mutex)(&sysmon->lock);
+
+ switch (chan->type) {
+ case IIO_TEMP:
+ if (mask == IIO_CHAN_INFO_SCALE) {
+ /* Q8.7 to millicelsius: raw * 1000 / 128 */
+ *val = MILLI;
+ *val2 = BIT(SYSMON_FRACTIONAL_SHIFT);
+ return IIO_VAL_FRACTIONAL;
+ }
+ if (mask != IIO_CHAN_INFO_RAW)
+ return -EINVAL;
+
+ ret = regmap_read(sysmon->regmap, chan->address, ®val);
+ if (ret)
+ return ret;
+
+ *val = sign_extend32(regval, 15);
+ return IIO_VAL_INT;
+
+ case IIO_VOLTAGE:
+ if (mask != IIO_CHAN_INFO_PROCESSED)
+ return -EINVAL;
+
+ ret = regmap_read(sysmon->regmap,
+ chan->address * SYSMON_REG_STRIDE +
+ SYSMON_SUPPLY_BASE, ®val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_rawtoprocessed(regval, val);
+ 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)
+{
+ unsigned int num_chan, idx, temp_chan_idx, volt_chan_idx;
+ unsigned int num_supply, num_temp;
+ 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");
+ 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");
+ num_temp = fwnode_get_child_node_count(temp_node);
+
+ num_chan = size_add(num_temp, size_add(ARRAY_SIZE(temp_channels), num_supply));
+ sysmon_channels = devm_kcalloc(dev, num_chan, sizeof(*sysmon_channels), GFP_KERNEL);
+ if (!sysmon_channels)
+ return -ENOMEM;
+
+ /* Static temperature channels first */
+ 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", ®);
+ if (ret)
+ 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)
+ 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_PROCESSED),
+ .datasheet_name = label,
+ };
+ }
+
+ /* Temperature satellite channels from DT */
+ fwnode_for_each_child_node_scoped(temp_node, child) {
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret)
+ 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)
+ 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),
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+ .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 (unsigned int 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..0c2704326a4
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon.c
@@ -0,0 +1,93 @@
+// 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/err.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/types.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..0c0c89f6e0c
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -0,0 +1,67 @@
+/* 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>
+
+struct device;
+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 read-modify-write sequences on threshold registers
+ * and cached state that spans multiple regmap calls
+ */
+struct sysmon {
+ struct regmap *regmap;
+ /*
+ * Protects read-modify-write sequences on threshold registers
+ * and cached state (oversampling ratios, hysteresis values)
+ * that spans multiple regmap calls.
+ */
+ struct mutex lock;
+};
+
+int sysmon_core_probe(struct device *dev, struct regmap *regmap);
+
+#endif /* _VERSAL_SYSMON_H_ */
--
2.48.1
^ permalink raw reply related
* [PATCH v7 1/5] dt-bindings: iio: adc: add xlnx,versal-sysmon binding
From: Salih Erim @ 2026-06-14 23:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim, Krzysztof Kozlowski
In-Reply-To: <20260614233722.2603459-1-salih.erim@amd.com>
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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
Changes in v7:
- No code changes
Changes in v6:
- No code changes
Changes in v5:
- No code changes
Changes in v4:
- Add Reviewed-by tag from Krzysztof Kozlowski
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
^ permalink raw reply related
* [PATCH v7 0/5] iio: adc: add AMD/Xilinx Versal SysMon driver
From: Salih Erim @ 2026-06-14 23:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, 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:
v6: https://lore.kernel.org/all/20260611222738.2035062-1-salih.erim@amd.com/
v5: https://lore.kernel.org/all/20260608183801.1257051-1-salih.erim@amd.com/
v4: https://lore.kernel.org/all/20260606051707.535281-1-salih.erim@amd.com/
v3: https://lore.kernel.org/all/20260527114211.174288-1-salih.erim@amd.com/
v2: https://lore.kernel.org/all/20260502111951.538488-1-salih.erim@amd.com/
v1: https://lore.kernel.org/all/cover.1757061697.git.michal.simek@amd.com/
Changes in v7:
- Move TEMP event onto channel 0; drop OT as separate IIO
channel; use single temp_channels array with runtime event
attachment (Jonathan)
- Return administrative state from temp_mask in read_event_config
instead of transient hardware IMR (Jonathan, Sashiko)
- Add devm cleanup to mask HW interrupts on driver unbind (Sashiko)
- Add documentation comment for static aggregate temp channels
(Jonathan)
- Split sysmon_osr_write into per-type helpers (Jonathan)
Changes in v6:
- Address all Andy v5 review items: switch(chan->type) in event
functions, reversed xmas tree ordering, macro brace placement,
scoped_guard in unmask worker, combined regmap error checks,
join single-line constructs, IWYU header fixes (Andy)
- Fix fwnode_irq_get() to propagate only -EPROBE_DEFER; treating
all negatives as fatal broke probe on I2C nodes without
interrupts property
Changes in v5:
- Core: add err.h include (IWYU) (Andy)
- Core: drop (int) cast on MILLI in scale assignment (Andy)
- Core: sign_extend32() instead of (s16) cast (Andy)
- Core: remove unneeded parentheses in voltage address
calculation (Andy)
- Core: drop NULL checks before fwnode_get_child_node_count
(NULL-aware) (Andy)
- Core: nested size_add() for overflow-safe allocation (Andy)
- Core: if (ret) instead of if (ret < 0) for fwnode property
reads (Andy)
- Core: remove outer parentheses in satellite address
calculation (Andy)
- Core: loop index declared in for() scope (Andy)
- MMIO: add err.h, types.h includes (IWYU) (Andy)
- Header: remove unused types.h include and struct iio_dev
forward declaration at P2 stage (Andy)
- I2C: add err.h, mod_devicetable.h includes (IWYU) (Andy)
- Events: clamp() instead of clamp_t() (Andy)
- Events: regmap_assign_bits() instead of separate set/clear (Andy)
- Events: remove unneeded parentheses (2 places) (Andy)
- Events: for_each_set_bit on single line (Andy)
- Events: regmap_clear_bits() instead of regmap_update_bits() (Andy)
- Events: simplify unmask XOR to ~status & masked_temp (Andy)
- Events: add comment explaining unmask &= ~temp_mask logic (Andy)
- Events: split container_of across two lines (Andy)
- Events: move ISR write after !isr check (Andy)
- Events: unsigned int for init_hysteresis address param (Andy)
- Events: add comment explaining error check policy in
worker/IRQ (Andy)
- Events: nested size_add() for overflow-safe allocation (Andy)
- Events: propagate negative from fwnode_irq_get() for
EPROBE_DEFER (Andy)
- Events: pass irq instead of has_irq to sysmon_parse_fw (Andy)
- Oversampling: remove unneeded parentheses (Andy)
- Oversampling: use struct regmap *map local variable (Andy)
- Oversampling: switch instead of redundant if/if on
channel_type (Andy)
- Oversampling: add CONFIG register readback fence after
oversampling update to prevent NoC bus hang from posted
writes (found during hardware stress testing)
Changes in v4:
- Core: temperature channels use RAW + SCALE (IIO_VAL_FRACTIONAL,
1000/128) instead of PROCESSED; voltage channels use PROCESSED
only, drop RAW; drop scan_type from all channel macros (Jonathan)
- Core: move __free(fwnode_handle) declarations down to just
above use; devm_regmap_init() on one line; lock comment
describes RMW sequences and cached state (Jonathan)
- Events: merge event channels into static temp array -- two
arrays (with/without events) selected by has_irq; event-only
channels have no info_mask (Jonathan)
- Events: blank lines, fit under 80 chars, default returns error,
return early in each case, guard(spinlock) in IRQ handler
(Jonathan)
- Events: take irq_lock in write_event_config for temp_mask
updates (Sashiko)
- I2C: replace enum with defines, use unaligned accessors for
data and register offset packing, named initializer in
i2c_device_id (Jonathan)
- I2C: drop bitfield.h, add unaligned.h
- Oversampling: return directly, remove else after early returns,
rename mask defines, blank lines (Jonathan)
- Oversampling: move oversampling read inside guard(mutex) scope
- Fix v2 lore link in cover letter
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). I2C compile-tested.
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 | 1041 +++++++++++++++++
drivers/iio/adc/versal-sysmon-i2c.c | 128 ++
drivers/iio/adc/versal-sysmon.c | 93 ++
drivers/iio/adc/versal-sysmon.h | 120 ++
8 files changed, 1579 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
^ permalink raw reply
* Re: [PATCH RFC v4 2/6] Documentation: iio: add Open Sensor Fusion driver overview
From: Kim Jinseob @ 2026-06-14 23:10 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Shuah Khan, devicetree, linux-kernel, linux-doc
In-Reply-To: <20260614185914.6c14d376@jic23-huawei>
> Same issue on needing this series to be against a suitable
> tree, not on top of your previous version.
Understood, thanks.
I will regenerate the next revision as a full standalone series against a
clean upstream base, not on top of the previous version.
I will also check the final diffstat before sending so files from earlier
versions are not removed, and fold the MAINTAINERS cleanup into the correct
patch.
Thanks,
Jinseob
^ permalink raw reply
* Re: [PATCH RFC v4 1/6] dt-bindings: iio: add Open Sensor Fusion device
From: Kim Jinseob @ 2026-06-14 23:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Conor Dooley, linux-iio, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Corbet, Shuah Khan, devicetree, linux-kernel, linux-doc
In-Reply-To: <20260614185551.10ca9e0f@jic23-huawei>
2026년 6월 15일 (월) 오전 2:55, Jonathan Cameron <jic23@kernel.org>님이 작성:
>
> On Wed, 10 Jun 2026 18:33:54 +0900
> Kim Jinseob <kimjinseob88@gmail.com> wrote:
> When a device needs power, the regulator is required, not optional from a binding
> point of view. If it is always one people can use a fixed regulator to represent it.
>
> Now from a driver point of view, the regulator framework in linux provides stub regulators
> for missing ones - on assumption they are always on. So we can just request the
> regulators in the driver. Keep it simple for now and use a
> devm_regulator_get_enable() in probe so we have power on for all the time
> the driver is loaded. Can do fancy stuff later when you have a board where the
> power is controlled.
Understood, thanks.
I will make vcc-supply a required property in the binding and use
devm_regulator_get_enable() in probe in the next revision, keeping the
power handling simple for now.
Jinseob
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox