* [PATCH v2 0/5] dmaengine: sun6i-dma: Add support for Allwinner A733 DMA controller
From: Yuanshen Cao @ 2026-06-21 21:40 UTC (permalink / raw)
To: Vinod Koul, Frank Li, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maxime Ripard
Cc: Yuanshen Cao, dmaengine, linux-arm-kernel, linux-sunxi,
linux-kernel, devicetree
Hi everyone,
This patch series introduces support for the Allwinner A733 DMA
controller in the `sun6i-dma` driver.
The A733 DMA controller differs from previous generations in several key
ways:
1. It supports higher address (up to 32G).
2. It uses a different interrupt register layout and mapping.
3. It has a different number of channels per interrupt register.
To support these differences without introducing complex conditional
logic throughout the driver, this series first refactors the
`sun6i_dma_config` structure. By moving interrupt handling, register
dumping, and address configuration into function pointers within the
configuration structure. This allows the driver to support the A733
and future hardware revisions. It also aligns with the DMA drivers in
Radxa BSP Package[1].
The series is organized as follows:
- Refactors the configuration structure to include function pointers for
interrupt and register operations.
- Moves address setting logic into the configuration structure to handle
varying address widths.
- Adds support for variable channels per interrupt register.
- Updates the device tree bindings documentation.
- Implements the A733-specific configuration and register mappings.
Tested on Radxa Cubie A7Z.
[1] https://github.com/radxa/allwinner-bsp/blob/cubie-aiot-v1.4.8/drivers/dma/sunxi-dma.c
Thanks!
Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
---
Changes in v2:
- Implement SUN6I_DMA_IRQ_A31_COMMON_OPS macro to avoid duplicate.
- Move set_addr into helper function and revert back sun6i_dma_set_addr.
- Rename chan_num to irq_req to avoid misleading name as suggested by
sashiko.
- Reorder and reword the dtbinding patch for more clarity.
- Link to v1: https://patch.msgid.link/20260619-sun60i-a733-dma-v1-0-da4b649fc72a@gmail.com
To: Vinod Koul <vkoul@kernel.org>
To: Frank Li <Frank.Li@kernel.org>
To: Chen-Yu Tsai <wens@kernel.org>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: Samuel Holland <samuel@sholland.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Maxime Ripard <mripard@kernel.org>
Cc: dmaengine@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-sunxi@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
Yuanshen Cao (5):
dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling
dmaengine: sun6i-dma: Add set_addr function pointer for variable address widths
dmaengine: sun6i-dma: Add num_channels_per_reg for flexible interrupt mapping
dt-bindings: dma: sun50i-a64-dma: Add allwinner,sun60i-a733-dma compatible string
dmaengine: sun6i-dma: Implement support for Allwinner A733 DMA controller
.../bindings/dma/allwinner,sun50i-a64-dma.yaml | 2 +
drivers/dma/sun6i-dma.c | 197 +++++++++++++++++++--
2 files changed, 181 insertions(+), 18 deletions(-)
---
base-commit: 8cd9520d35a6c38db6567e97dd93b1f11f185dc6
change-id: 20260619-sun60i-a733-dma-c2455149165d
Best regards,
--
Yuanshen Cao <alex.caoys@gmail.com>
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: iio: adc: add ti,ads122c14
From: David Lechner @ 2026-06-21 21:14 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Kurt Borja, Nguyen Minh Tien, linux-iio, devicetree,
linux-kernel
In-Reply-To: <20260621194102.08d7fdd6@jic23-huawei>
On 6/21/26 1:41 PM, Jonathan Cameron wrote:
> On Mon, 15 Jun 2026 16:59:59 -0500
> "David Lechner (TI)" <dlechner@baylibre.com> wrote:
>
>> Add new bindings for ti,ads122c14 and similar devices.
>>
>> This is an ADC that is primarily intended for use with temperature
>> sensors. There are a few unusual properties because of this. In
>> particular, the reference voltage source and current output requirements
>> can be different for each measurement, so these are included in the
>> channel bindings.
>>
>> The REFP/REFN reference voltage is usually just connected to a resistor
>> that is being driven by the ADC's current outputs, so there is special
>> property for this case rather than requiring a regulator to be defined
>> to represent that.
>>
>> ti,vref-source is reused from ti,tlv320adcx140.yaml (otherwise might
>> have preferred an enum of strings).
>>
>> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
>
> A few queries inline though I'm only just starting to get my head
> around this device...
>
> Thanks
>
> Jonathan
>
>> ---
>> .../devicetree/bindings/iio/adc/ti,ads112c14.yaml | 224 +++++++++++++++++++++
>> MAINTAINERS | 7 +
>> include/dt-bindings/iio/adc/ti,ads112c14.h | 11 +
>> 3 files changed, 242 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml
>> new file mode 100644
>> index 000000000000..dc7f37cad772
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml
>> @@ -0,0 +1,224 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/ti,ads112c14.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments' ADS112C14 and similar ADC chips
>> +
>> +description: |
>> + Supports the following Texas Instruments' ADC chips:
>> + - ADS112C14 (16-bit)
>> + - ADS122C14 (24-bit)
>> +
>> + https://www.ti.com/lit/ds/symlink/ads122c14.pdf
>> +
>> + These chips are primarily designed for use with temperature sensors such as
>> + RTDs and thermocouples. The channel bindings reflect this in that each channel
>> + represents the conditions required to make a measurement rather than strictly
>> + just the physical input channels.
>> +
>> +maintainers:
>> + - David Lechner <dlechner@baylibre.com>
>> +
>> +unevaluatedProperties: false
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - ti,ads112c14
>> + - ti,ads122c14
>> +
>> + reg:
>> + items:
>> + - minimum: 0x40
>> + maximum: 0x47
>> +
>> + clocks:
>> + maxItems: 1
>> + description: Optional external clock connected to GPIO3 pin.
>> +
>> + avdd-supply: true
>> + dvdd-supply: true
>> +
>> + refp-supply: true
>> + refn-supply: true
>> +
>> + refp-refn-resistor-ohms:
>> + description:
>> + The resistance of the external resistor between REFP and REFN when using
>> + resistor bridge driven by current outputs for RTD measurements.
>> +
>> + interrupts:
>> + minItems: 1
>> + items:
>> + - description: FAULT interrupt (GPIO2 pin)
>> + - description: DRDY interrupt (GPIO3 pin)
>> +
>> + interrupt-names:
>> + minItems: 1
>> + maxItems: 2
>> + items:
>> + enum: [fault, drdy]
>> +
>> + gpio-controller: true
>> + '#gpio-cells':
>> + const: 2
>> +
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 0
>> +
>> +patternProperties:
>> + ^channel@[0-7]$:
>> + $ref: adc.yaml
>> +
>> + unevaluatedProperties: false
>> +
>> + properties:
>> + reg:
>> + maximum: 16 # arbitrary limit, channel@ can be any combination of AIN0-AIN7
>> +
>> + single-channel:
>> + maximum: 7
>> +
>> + diff-channels:
>> + items:
>> + maximum: 7
>> +
>> + bipolar:
>> + description:
>> + Set this flag if the differential input can be negative.
>
> I'd leave that description to adc.yaml Maybe that doc could be improved though
> given it basically says bipolar == bipolar mode ;)
It seems not always obvious to me which properties from adc.yaml apply
and which ones don't to a given ADC that makes use of it. So I was
hoping to have some way of saying that bipolar is applicable to this
chip.
>
>> +
>> + excitation-channels:
>> + description: AINx pins used as current output.
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + minItems: 1
>> + maxItems: 2
>> + items:
>> + maximum: 7
>> +
>> + excitation-current-microamp:
>
> There seem to be separate controls. Are their usecases where this needs
> to be in array?
I'll have to ask, but probably yes since there are separate controls
so `maxItems: 2` would be appropriate.
>
>> + description: The current output of the excitation channels in microamps.
>> + minimum: 1
>> + maximum: 1000
>> +
>> + current-chopping:
>> + $ref: /schemas/types.yaml#/definitions/flag
>> + description:
>> + If provided, the two excitation channels are to be used with current
>> + chopping enabled.
>
> Can I have a reference for that? My initial read suggests it's the input channels
No. :-)
I must have got two ideas mixed together in my head to come up with
this. Clearly this should be `input-channel-rotation` or something like
that (we discussed in another thread already). Also curious if you thing
any of these properties are common enough to promote to adc.yaml or if we
should just make them e.g. `ti,input-channel-rotation` (you might not have
had time to read the threads on that yet).
> that are chopped. For GC_EN
> "When enabled, the device automatically swaps
> the analog inputs and takes the average of two consecutive conversions to
> cancel the internal offset voltage"
>
>
>> +
>> + ti,vref-source:
>> + description: |
>> + Indicates the source for the reference voltage for this channel.
>> + 0 - Internal 2.5V reference
>> + 1 - Internal 1.25V reference
>> + 2 - External reference (REFP-REFN)
>> + 3 - AVDD as reference
>> +
>> + For convenience, macros for these values are available in
>> + dt-bindings/iio/adc/ti,ads112c14.h.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + maximum: 3
>> + default: 0
>> +
>> + dependencies:
>> + excitation-channels: [ excitation-current-microamp ]
>> + excitation-current-microamp: [ excitation-channels ]
>> + current-chopping: [ excitation-channels ]
>> +
>> + oneOf:
>> + - required: [ single-channel ]
>> + - required: [ diff-channels ]
>
>> +examples:
>> + - |
>> + #include <dt-bindings/iio/adc/ti,ads112c14.h>
>> +
>> + i2c {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + adc@40 {
>> + compatible = "ti,ads112c14";
>> + reg = <0x40>;
>> +
>> + avdd-supply = <&avdd>;
>> + dvdd-supply = <&dvdd>;
>> +
>> + /* 3-Wire RTD: Two IDACs, One Measurement (AIN1-AIN2) */
>> +
>> + refp-refn-resistor-ohms = <500>;
>> +
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + channel@0 {
>> + reg = <0>;
>> + diff-channels = <1>, <2>;
>> + excitation-channels = <0>, <3>;
>> + excitation-current-microamp = <500>;
>> + current-chopping;
>> + ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
>> + label = "rtd";
>> + };
>> + };
>> + };
>> + - |
>> + #include <dt-bindings/iio/adc/ti,ads112c14.h>
>> +
>> + i2c {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + adc@40 {
>> + compatible = "ti,ads112c14";
>> + reg = <0x40>;
>> +
>> + avdd-supply = <&avdd>;
>> + dvdd-supply = <&dvdd>;
>> +
>> + /* Resistive Bridge Measurement With a Thermistor for Temperature Compensation*/
>> +
>> + refp-supply = <&avdd>;
>> +
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + channel@0 {
>> + reg = <0>;
>> + diff-channels = <6>, <7>;
>> + bipolar;
>> + ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
>> + label = "bridge";
>> + };
>> +
>> + channel@1 {
>> + reg = <1>;
>> + diff-channels = <1>, <2>;
>> + ti,vref-source = <ADS112C14_VREF_SOURCE_INTERNAL_2_5V>;
>> + label = "thermistor";
>
> Hmm. I'm interested to see where this goes, but generally when we have
> a thermistor we attempt to ultimately convert it to a temperature
> channel and I'm not seeing info here to allow us to do that.
Since the hardware doesn't have any special features for handling
specific sensor types, it seems like a case of the driver trying to
do things that the hardware doesn't do, which we generally try to
avoid in the kernel.
For cases where we want a quick and easy (and not necessarily accurate)
temperature conversion done in the kernel, we could make a generic thermistor
analog front end binding and driver like we already have for RTDs and
(linear) temperature transducers. This seems more sensible to me rather
than having to re-implement such a thing in each ADC that could be used
with a thermistor.
>
>> + };
>> + };
>> + };
>
>
^ permalink raw reply
* Re: [PATCH] arm64: dts: broadcom: bcm2712: Remove non-functional EL2 virtual timer
From: Daniel Drake @ 2026-06-21 20:58 UTC (permalink / raw)
To: Florian Fainelli, Marc Zyngier
Cc: robh, krzk+dt, conor+dt, bcm-kernel-feedback-list, devicetree,
linux-rpi-kernel, linux-arm-kernel, m.szyprowski, andrea.porta
In-Reply-To: <223cd514-41b3-45b9-8617-b54d379d5091@broadcom.com>
On 21/06/2026 21:03, Florian Fainelli wrote:
> Daniel, do you happen to know which 2712 SoC revision you have, whether
> this is a C0 or D0 stepping?
>
> We have an internal bug tracker item pertaining exactly to the virtual
> timer interrupt connection however it affected a sister chip (77122) and
> not 2712 AFAICT, now checking with the design team whether the same
> happened on 2712.
Thanks for looking into this! I am using Raspberry Pi 500 with D0 stepping.
Daniel
^ permalink raw reply
* Re: [PATCH] arm64: dts: broadcom: bcm2712: Remove non-functional EL2 virtual timer
From: Florian Fainelli @ 2026-06-21 20:03 UTC (permalink / raw)
To: Marc Zyngier, Daniel Drake
Cc: robh, krzk+dt, conor+dt, bcm-kernel-feedback-list, devicetree,
linux-rpi-kernel, linux-arm-kernel, m.szyprowski, andrea.porta
In-Reply-To: <878q898ulx.wl-maz@kernel.org>
On 6/20/2026 9:49 AM, Marc Zyngier wrote:
> Hi Daniel,
>
> Thanks for posting this.
>
> On Fri, 19 Jun 2026 21:48:32 +0100,
> Daniel Drake <dan@reactivated.net> wrote:
>>
>> Commit d87773de9efe1 ("clocksource/drivers/arm_arch_timer: Default to
>> EL2 virtual timer when running VHE") causes boot to hang on
>> Raspberry Pi 5. The newly-selected EL2 virtual timer does not generate
>> any interrupts, even though the GIC_DIST_ENABLE_SET flag has been
>> confirmed set via readback.
>>
>> The reasons for this failure are unknown, however it is likely that
>> this timer was never tested. Raspberry Pi's original devicetree did
>
> The timer is part of the CPU, and there are enough A76 implementations
> around to prove that it actually works. The same can be said for the
> GIC400 this is (supposedly) attached to.
> >> not include this timer interrupt; it was only introduced via a
>> suggestion[1] made in code review as part of the upstreaming process.
>> (Current RPi firmware versions do include this timer, but only because
>> they rebased on top of the upstreamed devicetree starting with
>> Linux 6.12)
>>
>> Until more is known about this non-firing timer interrupt, remove
>> the devicetree entry to enable RPi5 devices to boot.
>
> I'd like to understand the reason why the timer interrupt isn't being
> delivered *before* we paper over it, and not the other way
> around. Each of the CPUs definitely have an EL2 virtual timer, the GIC
> has a per-CPU interrupt, but somehow the two don't seem to be linked.
>
> Since DT is supposed to describe the HW, I'd expect someone from
> Broadcom or RPi to shine a light on this issue. Integration mistakes
> happen, and we work around them (see the handful of Samsung SoCs where
> the timer interrupt was simply not wired). But we absolutely need to
> know what we are dealing with beforehand.
>
> Finally, just hacking the DT is not enough. Assuming that the timer is
> indeed unusable, we need to cope with the fact that there are DTs
> describing it in the wild, as nobody should be forced to upgrade their
> DT in lockstep with the kernel. For that, you'd also need something
> like the patch below (untested, and in need of a proper commit
> message, which I expect the SoC vendor to provide).
Daniel, do you happen to know which 2712 SoC revision you have, whether
this is a C0 or D0 stepping?
We have an internal bug tracker item pertaining exactly to the virtual
timer interrupt connection however it affected a sister chip (77122) and
not 2712 AFAICT, now checking with the design team whether the same
happened on 2712.
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH 0/4] iio: adc: new ti-ads112c14 driver
From: Jonathan Cameron @ 2026-06-21 19:14 UTC (permalink / raw)
To: David Lechner
Cc: Kurt Borja, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nguyen Minh Tien, linux-iio,
devicetree, linux-kernel
In-Reply-To: <9b8d5cfc-e392-45aa-9adc-867c364dd36e@baylibre.com>
On Tue, 16 Jun 2026 13:16:46 -0500
David Lechner <dlechner@baylibre.com> wrote:
> On 6/16/26 12:26 PM, Kurt Borja wrote:
> > On Tue Jun 16, 2026 at 10:21 AM -05, David Lechner wrote:
> >> On 6/15/26 7:18 PM, Kurt Borja wrote:
> >>> On Mon Jun 15, 2026 at 4:59 PM -05, David Lechner (TI) wrote:
>
> ...
>
> >>>> All of these chips have in common that they are designed for use with
> >>>> RTDs and thermocouples and so they look very similar to each other in
> >>>> terms of wiring and feature set, even if the register maps are
> >>>> different. They are in the gray area where we could either keep them
> >>>> separate because they are just different enough, or we could do like
> >>>> we've done before with ad_sigma_delta and have a bit of an abstraction
> >>>> layer for the register differences and otherwise try to share as much
> >>>> code as possible. Normally, I would lean towards keeping them separate,
> >>>> but in this case, I'm considering trying to share code because the
> >>>> devicetree bindings for the inputs is complex and is going to be mostly
> >>>> the same across all of these chips.
> >>>
> >>> The channel configuration is indeed very similar for the three chips.
> >>> All three have IDAC, BOC and VREF configurations.
> >>
> >> Hmm... I forgot to include the burnout current in the DT bindings. Following
> >> the channel = "conditions for measurement" pattern that I have set out here
> >> I guess that would mean that we would need to have the same inputs twice
> >> when using the burnout. One "channel" would be the one used to do a "precision"
> >> measurement and the other would be the one to do open/short circuit detection.
> >>
> >>
> >> i2c {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >>
> >> adc@40 {
> >> compatible = "ti,ads112c14";
> >> reg = <0x40>;
> >>
> >> avdd-supply = <&avdd>;
> >> dvdd-supply = <&dvdd>;
> >>
> >> refp-supply = <&avdd>;
> >>
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >>
> >> channel@0 {
> >> reg = <0>;
> >> diff-channels = <1>, <2>;
> >> excitation-channels = <0>, <3>;
> >> excitation-current-microamp = <500>;
> >> current-chopping;
> >> ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
> >> label = "rtd-precision";
> >> };
> >>
> >> channel@1 {
> >> reg = <0>;
> >> diff-channels = <1>, <2>;
> >> excitation-channels = <0>, <3>;
> >> excitation-current-microamp = <500>;
Maybe use an example with more stuff changing? Do we want same excitation
for burn out? I've no idea.
> >> burnout-current-nanoamp = <1000>;
> >> ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
> >> label = "rtd-diagnostic";
> >> };
> >
> > This would mean we wouldn't be able to use iio_chan_spec .channel and
> > .channel2 to describe inputs because of duplicate sysfs attributes, no?
> >
>
> Yes, that is a bit unfortunate. At least there the labels to tell them
> apart. I guess we would just need to use consecutive channel and channel2
> when dynamically allocating the channels to avoid conflict.
From a very initial look, maybe do something similar to the folk have
been looking at for the more complex DDS devices where we have lots
of channels that are on the same 'wires'. Basically add a numbering
scheme to keep them reasonably separate - channel numbers are cheap.
Maybe first channel is 10->1f, second 20-2f etc. They are differential
so it will get ugly. Perhaps have a play around and see if there is
a reasonable channel naming scheme for this 'same inputs, different thing
being measured case'
I'm not yet sure I'm convinced that a separate channel model makes sense.
Even less in the DT given these are different settings for one channel.
That doesn't mean we don't split them up in the driver if channels
are the best implementation / ABI to userspace.
Basically nothing actually says diff-channels numbers match the
userspace ABI, so break that link.
>
> >>>> This makes things more flexible, but does make the driver a bit more
> >>>> complex. For example, knowing when the current output needs to be
> >>>> enabled or disabled. For now, I have chosen a lazy-enable where they
> >>>> are not turned on until the first measurement is taken that requires
> >>>> them, but then they stay on until another measurement is taken that
> >>>> doesn't require them. This can lead to some oddness with the diagnostic
> >>>> channels that may be measuring something that indirectly requires the
> >>>> current output (i.e. the external reference voltage when it is connected
> >>>> to a resistor rather than a power supply). This means you need to take
> >>>> a measurement that requires the current output to be enabled before the
> >>>> diagnostic channels will give accurate readings.
> >>>
> >>> This is the same approach I took around the BOC, it feels kinda hacky
> >>> but it makes sense. Just an idea I thought about just now: What if we
> >>> have an additional write-only "_enable" sysfs attribute for these
> >>> channels?
> >>
> >> I would not want to make a write-only attribute, we always want to be
> >> able to read back what the current state is.
> >
> > Yeah, I don't know why I said WO. Reading would be fine too.
> >
> >>
> >> Do you mean an _enable for just the BOC? I think I would do it like I
> >> suggested above instead.
> >
> > No, no just the BOC. The BOC, IDAC and rest of side effects. Thinking
> > about it some more, it would be a bit redundant but clearer if proper
> > documentation is provided.
> >
> I would be interested to see what Jonathan has to say about this too.
> Generally, his advice has been to avoid attributes that power things
> on and off if we can help it.
>
This is again a bit similar to the DDS case, but there we have more than
one on at a time (as controlling different parts of the signal - freq
/ phase / amplitude). Here we at least avoid that complexity.
I didn't really like that solution but we didn't come up with any other
way to support the complex stuff going on. Hence I'm not necessarily
suggesting to go that way here.
I think this may be a case of laying out different options in an ABI
doc then reviewing that to make sure we spot corner cases etc.
Thanks,
Jonathan
p.s. Sometimes it feels like the world just keeps getting more complex!
^ permalink raw reply
* Re: [PATCH 4/4] iio: adc: ti-ads112c14: add measurement channel support
From: Jonathan Cameron @ 2026-06-21 19:08 UTC (permalink / raw)
To: David Lechner (TI)
Cc: Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Kurt Borja, Nguyen Minh Tien, linux-iio, devicetree,
linux-kernel
In-Reply-To: <20260615-iio-adc-ti-ads122c14-v1-4-e6bdadf7cb2b@baylibre.com>
On Mon, 15 Jun 2026 17:00:02 -0500
"David Lechner (TI)" <dlechner@baylibre.com> wrote:
> Add support for parsing devicetree properties for measurement channels
> and doing direct reads on these.
>
> There are quite a lot of conditions that have to be met for each
> measurement to be made, so quite a bit of state and algorithms are
> required to handle it.
>
> Channels are created dynamically since the number of possibilities is
> unreasonably large.
>
> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
Trivial stuff. Seems the bulk of discussion here is in the dt vs
channel mappings area. I'll probably reply to that in a few mins.
> @@ -449,25 +599,31 @@ static int ads112c14_write_raw(struct iio_dev *indio_dev,
> int val2, long mask)
> {
> struct ads112c14_data *data = iio_priv(indio_dev);
> + const int (*scale_avail)[2];
> + u8 *gain_val;
> +
> + if (chan->channel == ADS112C14_SYS_MON_CHANNEL_SHORT) {
> + scale_avail = data->sys_mon_chan_short_scale_available;
> + gain_val = &data->sys_mon_chan_short_gain_val;
> + } else if (chan->channel < 100) {
> + scale_avail = data->measurements[chan->scan_index].scale_available;
> + gain_val = &data->measurements[chan->scan_index].gain_val;
> + } else {
> + return -EINVAL;
> + }
>
> - switch (chan->channel) {
> - case ADS112C14_SYS_MON_CHANNEL_SHORT: {
> - IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> - if (IIO_DEV_ACQUIRE_FAILED(claim))
> - return -EBUSY;
> + IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> + if (IIO_DEV_ACQUIRE_FAILED(claim))
> + return -EBUSY;
Anything stop you doing this in the earlier patch?
>
> - for (u32 i = 0; i < ARRAY_SIZE(data->sys_mon_chan_short_scale_available); i++) {
> - if (val == data->sys_mon_chan_short_scale_available[i][0] &&
> - val2 == data->sys_mon_chan_short_scale_available[i][1]) {
> - data->sys_mon_chan_short_gain_val = i;
> - return 0;
> - }
> + for (u32 i = 0; i < ARRAY_SIZE(ads112c14_pga_gains_x10); i++) {
> + if (val == scale_avail[i][0] && val2 == scale_avail[i][1]) {
> + *gain_val = i;
> + return 0;
> }
> - return -EINVAL;
> - }
> - default:
> - return -EINVAL;
> }
> +
> + return -EINVAL;
> }
>
>
> static int ads112c14_probe(struct i2c_client *client)
> @@ -547,6 +876,9 @@ static int ads112c14_probe(struct i2c_client *client)
> const struct ads112c14_chip_info *info;
> struct iio_dev *indio_dev;
> struct ads112c14_data *data;
> + bool need_avdd_ref, need_ext_ref;
> + u32 refp_uV = 0;
> + u32 refn_uV = 0;
> u32 reg_val;
> int ret;
>
>
> + if (device_property_present(dev, "refp-supply")) {
> + ret = devm_regulator_get_enable_read_voltage(&client->dev, "refp");
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to get refp voltage\n");
> +
> + refp_uV = ret;
> +
> + struct fwnode_handle *refp_fwnode __free(fwnode_handle) =
> + fwnode_find_reference(dev->fwnode, "refp-supply", 0);
> + if (IS_ERR(refp_fwnode))
> + return dev_err_probe(dev, PTR_ERR(refp_fwnode),
> + "failed to get refp fwnode\n");
> +
> + struct fwnode_handle *avdd_fwnode __free(fwnode_handle) =
> + fwnode_find_reference(dev->fwnode, "avdd-supply", 0);
> + if (IS_ERR(avdd_fwnode))
> + return dev_err_probe(dev, PTR_ERR(avdd_fwnode),
> + "failed to get avdd fwnode\n");
> +
> + data->refp_is_avdd = refp_fwnode == avdd_fwnode;
Add a comment somewhere on why we care. I wonder how common this is. Maybe
a generic helper? Even if it isn't common lets have a helper here!
fwnode_same_reference() or something like that.
> + }
> +
> + if (device_property_present(dev, "refn-supply")) {
> + ret = devm_regulator_get_enable_read_voltage(&client->dev, "refn");
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to get refn voltage\n");
> +
> + refn_uV = ret;
> + } else {
> + data->refn_is_gnd = true;
> + }
> +
> + data->ext_ref_uV = refp_uV - refn_uV;
> +
> + if (device_property_present(dev, "refp-refn-resistor-ohms")) {
> + if (refp_uV != 0 || refn_uV != 0)
> + return dev_err_probe(dev, -EINVAL,
> + "refp-refn-resistor-ohms property should not be present when refp-supply or refn-supply is present\n");
> +
> + ret = device_property_read_u32(dev, "refp-refn-resistor-ohms",
> + &data->ext_ref_ohms);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to read refp-refn-resistor-ohms property\n");
> + } else {
> + if (need_ext_ref && data->ext_ref_uV == 0)
> + return dev_err_probe(dev, -EINVAL,
> + "external reference measurements require either refp-supply or refp-refn-resistor-ohms property\n");
> + }
^ permalink raw reply
* Re: [PATCH RFC v4 01/12] dt-bindings: clk: zte: Add zx297520v3 top clock and reset bindings
From: Conor Dooley @ 2026-06-21 18:58 UTC (permalink / raw)
To: Stefan Dösinger
Cc: Michael Turquette, Stephen Boyd, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Philipp Zabel, Brian Masney, linux-clk, devicetree,
linux-kernel, linux-arm-kernel
In-Reply-To: <vYm1twErR8mp-Fjgbvf-MQ@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3772 bytes --]
On Sat, Jun 20, 2026 at 08:28:03PM +0300, Stefan Dösinger wrote:
> Hi Conor,
> Am Donnerstag, 18. Juni 2026, 22:54:53 Ostafrikanische Zeit schrieb Conor
> Dooley:
>
> I think I get the gist of your suggestions. I have a few follow-up questions
> to make sure I understand things right:
>
> > I think aux bus makes perfect sense when you have a clock/reset
> > controller, but once you start expanding past that and you have reboot
> > or hwmon or hwspinlock then mfd starts to make sense.
>
> At what point does it make sense to move the bindings from bindings/clock to
> bindings/mfd? The controllers are still very clock-heavy. allwinner,*-
> prcm.yaml look like clock, reset, misc controllers in mfd/ whereas
> ingenic,cgu.yaml, sprd,sc9863a-clk.yaml and da8xx-cfgchip.txt are clock + misc
> drivers in clock/.
Yeah, to bindings/mfd or bindings/soc/<vendor>. Which I think is mostly
a judgement call. Two of your devices have at least three functions,
which I think is enough to make the claim that it's not just a clock
controller.
> Likewise for the node names: syscon@ or clock-controller@?
If you have syscon in the compatible, then I think it should be syscon
in the node name as it's more general and makes it clear the device
isn't just a clock controller.
> > You'd then have topclock that is a syscon + simple-mfd, matrixclk that is
> > a syscon and lsp that's using the aux bus. The topclock and matrixclock
> > would have dedicated and trivial drivers somewhere that have the mfd_cells
> > and call mfd_add_devices().
>
> Do I even need simple-mfd? It seems I can add the syscon-reboot node via
> mfd_cells too by setting .of_compatible. It seems once it has a driver (even a
> very short one) simple-mfd is misplaced.
If you don't need child nodes in dt, you don't need simple-mfd.
Whether setting of_compatible is a correct thing to do, I do not know,
sorry.
> What about syscon? Topclk needs it for syscon-reboot and the watchdog
> controls. For the other two I only want a regmap. Afaiu device_node_to_regmap
> works without a "syscon" compatible. There's also regmap_init_mmio, but afaics
> I only want this when my driver is the only one using the regmap.
If it is a miscellaneous system register region, then it should be a
syscon. These devices that perform multiple functions like hwspinlock,
clocks and resets fit that bill. Whether or not you "need" it for linux to
work, if it is a correct description of your hardware you need to use
that compatible.
>
> > Probably the compatibles you've chosen start to make less sense at this
> > point though, but probably "topclk" and "matrixclk" are not what the
> > documentation for this device calls these register regions?
>
> Yeah I'll rename them top topcrm / matrixcrm / lspcrm. I just stuck to the old
> names for this email.
>
> > I think the priority is having something that reflects the hardware
> > accurately, I wouldn't compromise on that just to have the same design
> > for all three drivers.
>
> As far as I can see the primary difference between mfd_add_devices and simple-
> mfd + child nodes is that the latter makes the MFD composition visible in the
> device tree and the former keeps it a driver implementation detail. My sense
> is that the latter is preferred unless a subcomponent of the MFD might be
> reused in other components - e.g. an ADC is used in PMIC-abc and PMIC-xyz and
> thus the driver can be reused as well.
Correct. The other reason for doing it the devicetree way is if there
are conflicting property requirements. E.g. two of the same class of
device, like a pair of pin control functions or devices that use
different #address-cells to one another.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] iio: adc: add ti-ads112c14 driver
From: Jonathan Cameron @ 2026-06-21 18:52 UTC (permalink / raw)
To: David Lechner (TI)
Cc: Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Kurt Borja, Nguyen Minh Tien, linux-iio, devicetree,
linux-kernel
In-Reply-To: <20260615-iio-adc-ti-ads122c14-v1-2-e6bdadf7cb2b@baylibre.com>
On Mon, 15 Jun 2026 17:00:00 -0500
"David Lechner (TI)" <dlechner@baylibre.com> wrote:
> Add a new driver for the TI ADS112C14/ADS122C14 ADC chips.
>
> This first step is adding a very basic driver that only supports power
> on/reset and reading the system monitor channels.
>
> ADS112C14_SYS_MON_CHANNEL_SHORT is the last channel rather than being in
> logical order by address to keep the voltage channels together and in
> case we find we need to add variants of this channel with different
> voltage reference later.
>
> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
> ---
>
> A few other notes for review that didn't seem worth putting in the
> commit message:
> * I intentionally did not use bulk regmap because later we may need to
> get the voltage of the avdd supply.
> * I left some comments in the code where the code might look funny (e.g.
> to reduce future diff) or does not exactly match the datasheet, in
> which case later changes will address that.
A few really small things inline.
> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> new file mode 100644
> index 000000000000..97097ae2a487
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c14.c
> @@ -0,0 +1,536 @@
> +
> +#define ADS112C14_REG_IDAC_MUX_CFG 0x0E
> +#define ADS112C14_IDAC_MUX_CFG_IUNIT BIT(7)
> +#define ADS112C14_IDAC_MUX_CFG_I2MUX GENMASK(6, 4)
> +#define ADS112C14_IDAC_MUX_CFG_I1MUX GENMASK(2, 0)
Maybe tweak the indent so the register vs fields is slightly easier
to see.
#define ADS112C14_REG_IDAC_MUX_CFG 0x0E
#define ADS112C14_IDAC_MUX_CFG_IUNIT BIT(7)
#define ADS112C14_IDAC_MUX_CFG_I2MUX GENMASK(6, 4)
#define ADS112C14_IDAC_MUX_CFG_I1MUX GENMASK(2, 0)
And deeper still for values where appropriate.
> +static const struct reg_default ads112c14_reg_defaults[] = {
> + { ADS112C14_REG_DEVICE_CFG, 0x00 },
> + { ADS112C14_REG_DATA_RATE_CFG, 0x00 },
> + { ADS112C14_REG_MUX_CFG, 0x00 },
> + { ADS112C14_REG_GAIN_CFG, 0x01 },
> + { ADS112C14_REG_REFERENCE_CFG, 0x00 },
> + { ADS112C14_REG_DIGITAL_CFG, 0x00 },
> + { ADS112C14_REG_GPIO_CFG, 0x00 },
> + { ADS112C14_REG_GPIO_DATA_OUTPUT, 0x00 },
> + { ADS112C14_REG_IDAC_MAG_CFG, 0x00 },
> + { ADS112C14_REG_IDAC_MUX_CFG, 0x10 },
If it is plausible to define these in terms of fields that make them
up I'd prefer that. It isn't always sensible to do so though as
they can get very messy.
> +};
>
> +
> +static int ads112c14_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct ads112c14_data *data = iio_priv(indio_dev);
> + u32 vref_uV, fsr_bits;
> +
> + /* Selecting V_REF source is not implemented yet. */
> + vref_uV = ads112c14_internal_ref_uV[ADS112C14_REFERENCE_CFG_REF_VAL_2_5V];
> +
> + /* Currently, everything is using signed data. */
> + fsr_bits = data->chip_info->resolution_bits - 1;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW: {
> + u8 buf[3];
> + int ret;
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> +
> + ret = ads112c14_single_conversion(data, chan, buf);
> + iio_device_release_direct(indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + switch (data->chip_info->resolution_bits) {
> + case 16:
> + *val = get_unaligned_be16(buf);
> + break;
> + case 24:
> + *val = get_unaligned_be24(buf);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + *val = sign_extend32(*val, fsr_bits);
> +
> + return IIO_VAL_INT;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + if (chan->type == IIO_TEMP) {
> + /* TS_TC (typical) = 405 uV/°C */
> + *val = MILLI * vref_uV / 405;
> + *val2 = fsr_bits;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + }
> +
> + *val = vref_uV / (MICRO / MILLI);
> + /*
> + * Last 3 SYS_MON channels (ext ref, AVDD, DVDD) need to be
> + * multiplied by 8 to account for internal attenuation of / 8.
> + */
> + *val2 = fsr_bits - (chan->address >= 3 ? 3 : 0);
> + return IIO_VAL_FRACTIONAL_LOG2;
> + case IIO_CHAN_INFO_OFFSET:
> + /* Only the temperature channel has an offset. */
> + if (chan->type != IIO_TEMP)
> + return -EINVAL;
> + /* Die temperature [°C] = 25°C + (Measured voltage – TS_Offset) / TS_TC */
> + /* TS_TC (typical) = 405 uV/°C */
> + /* TS_Offset (typical) = 119.5 mV */
Trivial but make that a multiline comment block given it is all related stuff.
> + *val = div_s64((s64)(25 * 405 - 119500) * BIT(fsr_bits), vref_uV);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ads112c14_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + const struct ads112c14_chip_info *info;
> + struct iio_dev *indio_dev;
> + struct ads112c14_data *data;
> + u32 reg_val;
> + int ret;
> +
> + info = i2c_get_match_data(client);
We currently (I think) still need to protect against a manual driver
override and that being NULL. Just error out if that happens as we
don't care about trying to support that.
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: iio: adc: add ti,ads122c14
From: Jonathan Cameron @ 2026-06-21 18:41 UTC (permalink / raw)
To: David Lechner (TI)
Cc: Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Kurt Borja, Nguyen Minh Tien, linux-iio, devicetree,
linux-kernel
In-Reply-To: <20260615-iio-adc-ti-ads122c14-v1-1-e6bdadf7cb2b@baylibre.com>
On Mon, 15 Jun 2026 16:59:59 -0500
"David Lechner (TI)" <dlechner@baylibre.com> wrote:
> Add new bindings for ti,ads122c14 and similar devices.
>
> This is an ADC that is primarily intended for use with temperature
> sensors. There are a few unusual properties because of this. In
> particular, the reference voltage source and current output requirements
> can be different for each measurement, so these are included in the
> channel bindings.
>
> The REFP/REFN reference voltage is usually just connected to a resistor
> that is being driven by the ADC's current outputs, so there is special
> property for this case rather than requiring a regulator to be defined
> to represent that.
>
> ti,vref-source is reused from ti,tlv320adcx140.yaml (otherwise might
> have preferred an enum of strings).
>
> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
A few queries inline though I'm only just starting to get my head
around this device...
Thanks
Jonathan
> ---
> .../devicetree/bindings/iio/adc/ti,ads112c14.yaml | 224 +++++++++++++++++++++
> MAINTAINERS | 7 +
> include/dt-bindings/iio/adc/ti,ads112c14.h | 11 +
> 3 files changed, 242 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml
> new file mode 100644
> index 000000000000..dc7f37cad772
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml
> @@ -0,0 +1,224 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads112c14.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments' ADS112C14 and similar ADC chips
> +
> +description: |
> + Supports the following Texas Instruments' ADC chips:
> + - ADS112C14 (16-bit)
> + - ADS122C14 (24-bit)
> +
> + https://www.ti.com/lit/ds/symlink/ads122c14.pdf
> +
> + These chips are primarily designed for use with temperature sensors such as
> + RTDs and thermocouples. The channel bindings reflect this in that each channel
> + represents the conditions required to make a measurement rather than strictly
> + just the physical input channels.
> +
> +maintainers:
> + - David Lechner <dlechner@baylibre.com>
> +
> +unevaluatedProperties: false
> +
> +properties:
> + compatible:
> + enum:
> + - ti,ads112c14
> + - ti,ads122c14
> +
> + reg:
> + items:
> + - minimum: 0x40
> + maximum: 0x47
> +
> + clocks:
> + maxItems: 1
> + description: Optional external clock connected to GPIO3 pin.
> +
> + avdd-supply: true
> + dvdd-supply: true
> +
> + refp-supply: true
> + refn-supply: true
> +
> + refp-refn-resistor-ohms:
> + description:
> + The resistance of the external resistor between REFP and REFN when using
> + resistor bridge driven by current outputs for RTD measurements.
> +
> + interrupts:
> + minItems: 1
> + items:
> + - description: FAULT interrupt (GPIO2 pin)
> + - description: DRDY interrupt (GPIO3 pin)
> +
> + interrupt-names:
> + minItems: 1
> + maxItems: 2
> + items:
> + enum: [fault, drdy]
> +
> + gpio-controller: true
> + '#gpio-cells':
> + const: 2
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> +patternProperties:
> + ^channel@[0-7]$:
> + $ref: adc.yaml
> +
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + maximum: 16 # arbitrary limit, channel@ can be any combination of AIN0-AIN7
> +
> + single-channel:
> + maximum: 7
> +
> + diff-channels:
> + items:
> + maximum: 7
> +
> + bipolar:
> + description:
> + Set this flag if the differential input can be negative.
I'd leave that description to adc.yaml Maybe that doc could be improved though
given it basically says bipolar == bipolar mode ;)
> +
> + excitation-channels:
> + description: AINx pins used as current output.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 1
> + maxItems: 2
> + items:
> + maximum: 7
> +
> + excitation-current-microamp:
There seem to be separate controls. Are their usecases where this needs
to be in array?
> + description: The current output of the excitation channels in microamps.
> + minimum: 1
> + maximum: 1000
> +
> + current-chopping:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description:
> + If provided, the two excitation channels are to be used with current
> + chopping enabled.
Can I have a reference for that? My initial read suggests it's the input channels
that are chopped. For GC_EN
"When enabled, the device automatically swaps
the analog inputs and takes the average of two consecutive conversions to
cancel the internal offset voltage"
> +
> + ti,vref-source:
> + description: |
> + Indicates the source for the reference voltage for this channel.
> + 0 - Internal 2.5V reference
> + 1 - Internal 1.25V reference
> + 2 - External reference (REFP-REFN)
> + 3 - AVDD as reference
> +
> + For convenience, macros for these values are available in
> + dt-bindings/iio/adc/ti,ads112c14.h.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + maximum: 3
> + default: 0
> +
> + dependencies:
> + excitation-channels: [ excitation-current-microamp ]
> + excitation-current-microamp: [ excitation-channels ]
> + current-chopping: [ excitation-channels ]
> +
> + oneOf:
> + - required: [ single-channel ]
> + - required: [ diff-channels ]
> +examples:
> + - |
> + #include <dt-bindings/iio/adc/ti,ads112c14.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adc@40 {
> + compatible = "ti,ads112c14";
> + reg = <0x40>;
> +
> + avdd-supply = <&avdd>;
> + dvdd-supply = <&dvdd>;
> +
> + /* 3-Wire RTD: Two IDACs, One Measurement (AIN1-AIN2) */
> +
> + refp-refn-resistor-ohms = <500>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + channel@0 {
> + reg = <0>;
> + diff-channels = <1>, <2>;
> + excitation-channels = <0>, <3>;
> + excitation-current-microamp = <500>;
> + current-chopping;
> + ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
> + label = "rtd";
> + };
> + };
> + };
> + - |
> + #include <dt-bindings/iio/adc/ti,ads112c14.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adc@40 {
> + compatible = "ti,ads112c14";
> + reg = <0x40>;
> +
> + avdd-supply = <&avdd>;
> + dvdd-supply = <&dvdd>;
> +
> + /* Resistive Bridge Measurement With a Thermistor for Temperature Compensation*/
> +
> + refp-supply = <&avdd>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + channel@0 {
> + reg = <0>;
> + diff-channels = <6>, <7>;
> + bipolar;
> + ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
> + label = "bridge";
> + };
> +
> + channel@1 {
> + reg = <1>;
> + diff-channels = <1>, <2>;
> + ti,vref-source = <ADS112C14_VREF_SOURCE_INTERNAL_2_5V>;
> + label = "thermistor";
Hmm. I'm interested to see where this goes, but generally when we have
a thermistor we attempt to ultimately convert it to a temperature
channel and I'm not seeing info here to allow us to do that.
> + };
> + };
> + };
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: iio: dac: Add AD5529R
From: Conor Dooley @ 2026-06-21 18:35 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Nuno Sá, Janani Sunil, Rodrigo Alencar, Janani Sunil,
Lars-Peter Clausen, Michael Hennerich, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Philipp Zabel, Jonathan Corbet, Shuah Khan,
linux-iio, devicetree, linux-kernel, linux-doc, Mark Brown
In-Reply-To: <20260621153330.79b6600c@jic23-huawei>
[-- Attachment #1: Type: text/plain, Size: 15710 bytes --]
On Sun, Jun 21, 2026 at 03:33:40PM +0100, Jonathan Cameron wrote:
> On Fri, 19 Jun 2026 16:54:11 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Fri, Jun 19, 2026 at 03:12:07PM +0100, Conor Dooley wrote:
> > > On Fri, Jun 19, 2026 at 02:01:08PM +0100, Nuno Sá wrote:
> > > > On Fri, Jun 19, 2026 at 12:40:54PM +0100, Conor Dooley wrote:
> > > > > On Fri, Jun 19, 2026 at 12:36:55PM +0100, Conor Dooley wrote:
> > > > > > On Fri, Jun 19, 2026 at 12:33:11PM +0200, Janani Sunil wrote:
> > > > > > >
> > > > > > > On 6/14/26 21:44, Jonathan Cameron wrote:
> > > > > > > > On Tue, 9 Jun 2026 16:47:23 +0200
> > > > > > > > Janani Sunil <jan.sun97@gmail.com> wrote:
> > > > > > > >
> > > > > > > > > On 5/26/26 15:11, Rodrigo Alencar wrote:
> > > > > > > > > > On 26/05/19 05:42PM, Janani Sunil wrote:
> > > > > > > > > > > Devicetree bindings for AD5529R 16 channel 12/16 bit high voltage,
> > > > > > > > > > > buffered voltage output digital-to-analog converter (DAC) with an
> > > > > > > > > > > integrated precision reference.
> > > > > > > > > > ...
> > > > > > > > > > Probably others may comment on that, but...
> > > > > > > > > >
> > > > > > > > > > This parent node may support device addressing for multi-device support through
> > > > > > > > > > those ID pins. I suppose that each device may have its own power supplies or
> > > > > > > > > > other resources like the toggle pins or reset and enable.
> > > > > > > > > >
> > > > > > > > > > That way I suppose that an example would look like...
> > > > > > > > > > > +
> > > > > > > > > > > +patternProperties:
> > > > > > > > > > > + "^channel@([0-9]|1[0-5])$":
> > > > > > > > > > > + type: object
> > > > > > > > > > > + description: Child nodes for individual channel configuration
> > > > > > > > > > > +
> > > > > > > > > > > + properties:
> > > > > > > > > > > + reg:
> > > > > > > > > > > + description: Channel number.
> > > > > > > > > > > + minimum: 0
> > > > > > > > > > > + maximum: 15
> > > > > > > > > > > +
> > > > > > > > > > > + adi,output-range-microvolt:
> > > > > > > > > > > + description: |
> > > > > > > > > > > + Output voltage range for this channel as [min, max] in microvolts.
> > > > > > > > > > > + If not specified, defaults to 0V to 5V range.
> > > > > > > > > > > + oneOf:
> > > > > > > > > > > + - items:
> > > > > > > > > > > + - const: 0
> > > > > > > > > > > + - enum: [5000000, 10000000, 20000000, 40000000]
> > > > > > > > > > > + - items:
> > > > > > > > > > > + - const: -5000000
> > > > > > > > > > > + - const: 5000000
> > > > > > > > > > > + - items:
> > > > > > > > > > > + - const: -10000000
> > > > > > > > > > > + - const: 10000000
> > > > > > > > > > > + - items:
> > > > > > > > > > > + - const: -15000000
> > > > > > > > > > > + - const: 15000000
> > > > > > > > > > > + - items:
> > > > > > > > > > > + - const: -20000000
> > > > > > > > > > > + - const: 20000000
> > > > > > > > > > > +
> > > > > > > > > > > + required:
> > > > > > > > > > > + - reg
> > > > > > > > > > > +
> > > > > > > > > > > + additionalProperties: false
> > > > > > > > > > > +
> > > > > > > > > > > +required:
> > > > > > > > > > > + - compatible
> > > > > > > > > > > + - reg
> > > > > > > > > > > + - vdd-supply
> > > > > > > > > > > + - avdd-supply
> > > > > > > > > > > + - hvdd-supply
> > > > > > > > > > > +
> > > > > > > > > > > +dependencies:
> > > > > > > > > > > + spi-cpha: [ spi-cpol ]
> > > > > > > > > > > + spi-cpol: [ spi-cpha ]
> > > > > > > > > > > +
> > > > > > > > > > > +allOf:
> > > > > > > > > > > + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > > > > > > > +
> > > > > > > > > > > +unevaluatedProperties: false
> > > > > > > > > > > +
> > > > > > > > > > > +examples:
> > > > > > > > > > > + - |
> > > > > > > > > > > + #include <dt-bindings/gpio/gpio.h>
> > > > > > > > > > > +
> > > > > > > > > > > + spi {
> > > > > > > > > > > + #address-cells = <1>;
> > > > > > > > > > > + #size-cells = <0>;
> > > > > > > > > > > +
> > > > > > > > > > > + dac@0 {
> > > > > > > > > > > + compatible = "adi,ad5529r-16";
> > > > > > > > > > > + reg = <0>;
> > > > > > > > > > > + spi-max-frequency = <25000000>;
> > > > > > > > > > > +
> > > > > > > > > > > + vdd-supply = <&vdd_regulator>;
> > > > > > > > > > > + avdd-supply = <&avdd_regulator>;
> > > > > > > > > > > + hvdd-supply = <&hvdd_regulator>;
> > > > > > > > > > > + hvss-supply = <&hvss_regulator>;
> > > > > > > > > > > +
> > > > > > > > > > > + reset-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>;
> > > > > > > > > > > +
> > > > > > > > > > > + #address-cells = <1>;
> > > > > > > > > > > + #size-cells = <0>;
> > > > > > > > > > > +
> > > > > > > > > > > + channel@0 {
> > > > > > > > > > > + reg = <0>;
> > > > > > > > > > > + adi,output-range-microvolt = <0 5000000>;
> > > > > > > > > > > + };
> > > > > > > > > > > +
> > > > > > > > > > > + channel@1 {
> > > > > > > > > > > + reg = <1>;
> > > > > > > > > > > + adi,output-range-microvolt = <(-10000000) 10000000>;
> > > > > > > > > > > + };
> > > > > > > > > > > +
> > > > > > > > > > > + channel@2 {
> > > > > > > > > > > + reg = <2>;
> > > > > > > > > > > + adi,output-range-microvolt = <0 40000000>;
> > > > > > > > > > > + };
> > > > > > > > > > > + };
> > > > > > > > > > > + };
> > > > > > > > > > ...
> > > > > > > > > >
> > > > > > > > > > spi {
> > > > > > > > > > #address-cells = <1>;
> > > > > > > > > > #size-cells = <0>;
> > > > > > > > > >
> > > > > > > > > > multi-dac@0 {
> > > > > > > > > > compatible = "adi,ad5529r-16";
> > > > > > > > > > reg = <0>;
> > > > > > > > > > spi-max-frequency = <25000000>;
> > > > > > > > > >
> > > > > > > > > > #address-cells = <1>;
> > > > > > > > > > #size-cells = <0>;
> > > > > > > > > >
> > > > > > > > > > dac@0 {
> > > > > > > > > > reg = <0>;
> > > > > > > > > > vdd-supply = <&vdd_regulator>;
> > > > > > > > > > avdd-supply = <&avdd_regulator>;
> > > > > > > > > > hvdd-supply = <&hvdd_regulator>;
> > > > > > > > > > hvss-supply = <&hvss_regulator>;
> > > > > > > > > >
> > > > > > > > > > reset-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>;
> > > > > > > > > >
> > > > > > > > > > #address-cells = <1>;
> > > > > > > > > > #size-cells = <0>;
> > > > > > > > > >
> > > > > > > > > > channel@0 {
> > > > > > > > > > reg = <0>;
> > > > > > > > > > adi,output-range-microvolt = <0 5000000>;
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > channel@1 {
> > > > > > > > > > reg = <1>;
> > > > > > > > > > adi,output-range-microvolt = <(-10000000) 10000000>;
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > channel@2 {
> > > > > > > > > > reg = <2>;
> > > > > > > > > > adi,output-range-microvolt = <0 40000000>;
> > > > > > > > > > };
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > dac@1 {
> > > > > > > > > > reg = <1>;
> > > > > > > > > > vdd-supply = <&vdd_regulator>;
> > > > > > > > > > avdd-supply = <&avdd_regulator>;
> > > > > > > > > > hvdd-supply = <&hvdd_regulator>;
> > > > > > > > > > hvss-supply = <&hvss_regulator>;
> > > > > > > > > >
> > > > > > > > > > reset-gpios = <&gpio0 88 GPIO_ACTIVE_LOW>;
> > > > > > > > > >
> > > > > > > > > > #address-cells = <1>;
> > > > > > > > > > #size-cells = <0>;
> > > > > > > > > >
> > > > > > > > > > channel@0 {
> > > > > > > > > > reg = <0>;
> > > > > > > > > > adi,output-range-microvolt = <0 5000000>;
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > channel@1 {
> > > > > > > > > > reg = <1>;
> > > > > > > > > > adi,output-range-microvolt = <(-10000000) 10000000>;
> > > > > > > > > > };
> > > > > > > > > > }
> > > > > > > > > > };
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > then you might need something like:
> > > > > > > > > >
> > > > > > > > > > patternProperties:
> > > > > > > > > > "^dac@[0-3]$":
> > > > > > > > > >
> > > > > > > > > > and put most of the things under this node pattern.
> > > > > > > > > >
> > > > > > > > > > So the main driver that you're putting together might need to handle up to four instances.
> > > > > > > > > > Even if your current driver cannot handle this, the dt-bindings might need cover that.
> > > > > > > > > >
> > > > > > > > > > Need to double check if each dac node needs a separate compatible, so you would maybe populate
> > > > > > > > > > a platform data to be shared with the child nodes, which would be a separate driver.
> > > > > > > > > > (not sure if it would make sense to mix and match ad5529r-16 and ad5529r-12).
> > > > > > > > > Hi Rodrigo,
> > > > > > > > >
> > > > > > > > > Thank you for looking at this.
> > > > > > > > >
> > > > > > > > > For now, I would prefer to keep the binding scoped to a single AD5529R device instance. The current
> > > > > > > > > hardware/use case we have only needs one device node and the driver is written around that model as well.
> > > > > > > > > While the device addressing pins could allow multi-device topology, we do not have an actual platform using
> > > > > > > > > that configuration at the moment, so I would prefer not to introduce an extra parent/child binding structure
> > > > > > > > > speculatively without a validating use case.
> > > > > > > > Interesting feature - kind of similar to address control on a typical i2c bus device, or
> > > > > > > > looking at it another way a kind of distributed SPI mux.
> > > > > > > >
> > > > > > > > Challenge of a binding is we need to anticipate the future. So I think we do need something
> > > > > > > > like Rodrigo is suggesting even if we only (for now) support a single instance in the driver.
> > > > > > > > That would leave the path open to supporting the addressing at a later date.
> > > > > > > > An alternative might be to look at it like a chained device setup. In those we pretend there
> > > > > > > > is just one device with a lot of channels etc. The snag is that here things are more loosely
> > > > > > > > coupled whereas for those devices it tends to be you have to read / write the same register
> > > > > > > > in all devices in the chain as one big SPI message.
> > > > > > > >
> > > > > > > > +CC Mark Brown as he may know of some precedence for this feature. For his reference..
> > > > > > > > - Each of these device has 2 ID pins. The SPI transfers have to contain the 2 bit
> > > > > > > > value that matches that or they are ignored. Thus a single bus + 1 chip select can
> > > > > > > > be used to talk to 4 devices. Question is what that looks like in device tree + I guess
> > > > > > > > longer term how to support it cleanly in SPI.
> > > > > >
> > > > > > I'd swear I have seen this before, from some Microchip devices. Let me
> > > > > > see if I can find what I am thinking of...
> > > > >
> > > > >
> > > > > microchip,mcp3911 and microchip,mcp3564 both seem to do this with
> > > > > slightly different properties.
> > > > >
> > > > > microchip,device-addr:
> > > > > description: Device address when multiple MCP3911 chips are present on the same SPI bus.
> > > > > $ref: /schemas/types.yaml#/definitions/uint32
> > > > > enum: [0, 1, 2, 3]
> > > > > default: 0
> > > > >
> > > > > and
> > > > >
> > > > >
> > > > > microchip,hw-device-address:
> > > > > $ref: /schemas/types.yaml#/definitions/uint32
> > > > > minimum: 0
> > > > > maximum: 3
> > > > > description:
> > > > > The address is set on a per-device basis by fuses in the factory,
> > > > > configured on request. If not requested, the fuses are set for 0x1.
> > > > > The device address is part of the device markings to avoid
> > > > > potential confusion. This address is coded on two bits, so four possible
> > > > > addresses are available when multiple devices are present on the same
> > > > > SPI bus with only one Chip Select line for all devices.
> > > > > Each device communication starts by a CS falling edge, followed by the
> > > > > clocking of the device address (BITS[7:6] - top two bits of COMMAND BYTE
> > > > > which is first one on the wire).
> > > > >
> > > > > This sounds exactly like the sort of feature that you're dealing with
> > > > > here?
> > > > >
> > > >
> > > > The core idea yes but for this chip, things are a bit more annoying (but
> > > > Janani can correct me if I'm wrong). Here, each device can, in theory,
> > > > have it's own supplies, pins and at the very least, channels with maybe
> > > > different scales. That is why Janani is proposing dac nodes. Given I
> > > > honestly don't like much of that "adi,ad5529r-bus" compatible I wondered
> > > > about solving this at the spi level.
> > > >
> > > > Ah and to make it more annoying, we can also mix 12 and 16 bits variants
> > > > together in the same bus.
> > >
> > > I'm definitely missing something, because that property for the
> > > microchip devices is not impacted what else is on the bus. AFAICT, you
> > > could have an mcp3911 and an mcp3564 on the same bus even though both
> > > are completely different devices with different drivers. They have
> > > individual device nodes and their own supplies etc etc. These aren't
> > > per-channel properties on an adc or dac, they're per child device on a
> > > spi bus.
> >
> > Maybe I'm the one missing something :). IIRC, spi would not allow two
> > devices on the same CS right? Because for this chip we would need
> > something like:
> >
> > spi {
> > dac@0 {
> > reg = <0>;
> > adi,pin-id = <0>;
> > };
> >
> > dac@1 {
> > reg = <0>; // which seems already problematic?
> > adi,pin-id <1>;
> > };
> >
> > ...
> >
> > //up to 4
> > };
> Yeah. It's not clear to me how that works for the microchip devices
> (I suspect it doesn't!)
>
> Just thinking as I type, but could we do something a bit nasty with
> a gpio mux that doesn't actually switch but represents the GPIO being
> shared? Given this is all tied to the spi bus that should all happen
> under serializing locks.
>
> Agreed though that this would be nicer as an SPI thing that let
> us specify that a single CS is share by multiple devices and their
> is some other signal acting to select which one we are talking to.
Whether it works or not, I think it is the more correct approach. Messing
with gpio muxes seems completely wrong, given the chip select may not be
a gpio at all.
Why do you think the microchip devices won't work? Does the spi core
reject multiple devices with the same chip select being registered or
something like that?
Certainly seems like an opportunity though to do something common
property wise, if both ADI and Microchip are trying to do the same thing
here with multiple users of the same chip select.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3] dt-bindings: misc: add binding for Xilinx AXI-Stream FIFO
From: Krzysztof Kozlowski @ 2026-06-21 18:33 UTC (permalink / raw)
To: Aditya Chari, robh, krzk+dt, conor+dt, gregkh
Cc: jacobsfeder, devicetree, linux-staging, linux-kernel
In-Reply-To: <20260621094312.53655-1-adi25charis@gmail.com>
On 21/06/2026 11:43, Aditya Chari wrote:
> The axis-fifo driver's compatible strings were undocumented, flagged
> by checkpatch.pl as UNDOCUMENTED_DT_STRING. Add a YAML devicetree
> binding document for drivers/staging/axis-fifo, converted from and
> replacing the existing free-form text binding (axis-fifo.txt), which
> this patch removes.
>
> Constrain xlnx,tx-fifo-depth to a minimum of 4, since the driver
> subtracts 4 from this value in its transmit bounds check and a
> smaller value would underflow that check.
>
> Signed-off-by: Aditya Chari <adi25charis@gmail.com>
> ---
>
> Changes since v2:
> - Added $ref: /schemas/types.yaml#/definitions/string to the three
> AXI-Stream protocol enum properties (xlnx,axi-str-rxd-protocol,
> xlnx,axi-str-txd-protocol, xlnx,axi-str-txc-protocol) for explicit
> type consistency with the rest of the schema.
> - Added minimum: 4 to xlnx,tx-fifo-depth, since the driver subtracts
> 4 from this value in its transmit bounds check
> (axis_fifo_write()) and a smaller configured value would underflow
> that unsigned check, bypassing the oversized-packet guard.
>
> Changes since v1:
> - Fixed xlnx,rx/tx-fifo-depth: depth is in 32-bit words, not bytes,
> matching the driver's overflow check in axis_fifo_write() and the
> wording of the original text binding.
> - Restored the full set of hardware-generated properties (interrupt-
> names, AXI-Stream protocol/width properties, has-axis-t* feature
> flags, fifo threshold properties, etc.) so that additionalProperties:
> false does not reject valid device trees generated for real hardware.
> - Removed the now-superseded axis-fifo.txt text binding.
Please slow down. Three versions within 1 hour! Why sending something
and immediately sending fixes to it?
>
> .../bindings/misc/xlnx,axi-fifo-mm-s.yaml | 227 ++++++++++++++++++
> drivers/staging/axis-fifo/axis-fifo.txt | 96 --------
Why are you touching staging binding?
https://lore.kernel.org/all/?q=dfn%3Aaxis-fifo.txt
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 2/2] iio: temperature: Add STS30 temperature sensor driver
From: Joshua Crofts @ 2026-06-21 18:33 UTC (permalink / raw)
To: Maxwell Doose
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
open list:IIO SUBSYSTEM AND DRIVERS,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
In-Reply-To: <20260621004626.66629-3-m32285159@gmail.com>
On Sat, 20 Jun 2026 19:46:24 -0500
Maxwell Doose <m32285159@gmail.com> wrote:
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/i2c.h>
I am a numpty as I also forgot to mention a missing
mod_devicetable.h header.
--
Kind regards
CJD
^ permalink raw reply
* Re: [PATCH 1/3] dt-bindings: arm: qcom: Add HP EliteBook X G2q 14 AI
From: Krzysztof Kozlowski @ 2026-06-21 18:29 UTC (permalink / raw)
To: Jason Pettit, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, devicetree, linux-kernel, Akhil P Oommen,
Mahadevan P, Sibi Sankar, Jingyi Wang, Ananthu C V
In-Reply-To: <20260620-glymur-send-v1-1-fc4a2cfd107c@oss.qualcomm.com>
On 21/06/2026 06:50, Jason Pettit wrote:
> The HP EliteBook X G2q 14 AI is a Snapdragon X2 Elite (Glymur) laptop.
> Document its top-level "hp,elitebook-x-g2q" compatible, falling back to
> the existing "qcom,glymur" SoC compatible.
Drop last sentence. It's pointless to explain what the diff is doing.
>
> Signed-off-by: Jason Pettit <jason.pettit@oss.qualcomm.com>
> Assisted-by: Claude:claude-opus-4-8
Your tag is supposed to be the last one. And really, making such commit
with Claude feels odd. This is a single one liner.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH V13 7/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607
From: Jonathan Cameron @ 2026-06-21 17:36 UTC (permalink / raw)
To: Chris Morgan
Cc: linux-iio, andy, nuno.sa, dlechner, jean-baptiste.maneyrol,
linux-rockchip, devicetree, heiko, conor+dt, krzk+dt, robh,
andriy.shevchenko, Chris Morgan
In-Reply-To: <20260615172554.160910-8-macroalpha82@gmail.com>
On Mon, 15 Jun 2026 12:25:50 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add icm42607 accelerometer sensor for icm42607.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
FWIW I think this has the same issue as the gyro patch around
management of the cache of what is enabled, but
this one sashiko gave a clean bill of health...
:)
^ permalink raw reply
* Re: [PATCH V13 8/9] iio: imu: inv_icm42607: Add Gyroscope to icm42607
From: Jonathan Cameron @ 2026-06-21 17:34 UTC (permalink / raw)
To: Chris Morgan
Cc: linux-iio, andy, nuno.sa, dlechner, jean-baptiste.maneyrol,
linux-rockchip, devicetree, heiko, conor+dt, krzk+dt, robh,
andriy.shevchenko, Chris Morgan
In-Reply-To: <20260615172554.160910-9-macroalpha82@gmail.com>
On Mon, 15 Jun 2026 12:25:51 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add gyroscope functions to the icm42607 driver.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
There is another bit of sashiko stuff in here. Please
take a look
https://sashiko.dev/#/patchset/20260615172554.160910-1-macroalpha82%40gmail.com
I think it is correct about there being a path in which the
gyro ends up always enabled along side anything else after
the first read.
In general there seems to be a bit of mix on whether the caller
or the power management function should be responsible for the caching
of state.
I know you look at Sashiko so I could just have waited a bit, but
I was reviewing anyway so took a look and having done that might
as well highlight some stuff!
Jonathan
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index eb239987a1ce..23ca7529825c 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -218,6 +218,49 @@ int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
> st->conf.temp_en, sleep_ms);
> }
>
> +int inv_icm42607_set_gyro_conf(struct inv_icm42607_state *st,
> + struct inv_icm42607_sensor_conf *conf,
> + unsigned int *sleep_ms)
> +{
> + struct inv_icm42607_sensor_conf *oldconf = &st->conf.gyro;
> + unsigned int val;
> + int ret;
> +
> + if (conf->mode < 0)
> + conf->mode = oldconf->mode;
> + if (conf->fs < 0)
> + conf->fs = oldconf->fs;
> + if (conf->odr < 0)
> + conf->odr = oldconf->odr;
> + if (conf->filter < 0)
> + conf->filter = oldconf->filter;
> +
> + if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> + val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG0_FS_SEL_MASK,
> + conf->fs);
> + val |= FIELD_PREP(INV_ICM42607_GYRO_CONFIG0_ODR_MASK,
> + conf->odr);
> + ret = regmap_write(st->map, INV_ICM42607_REG_GYRO_CONFIG0, val);
> + if (ret)
> + return ret;
> + oldconf->fs = conf->fs;
> + oldconf->odr = conf->odr;
> + }
> +
> + if (conf->filter != oldconf->filter) {
> + val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG1_FILTER_MASK,
> + conf->filter);
> + ret = regmap_update_bits(st->map, INV_ICM42607_REG_GYRO_CONFIG1,
> + INV_ICM42607_GYRO_CONFIG1_FILTER_MASK, val);
> + if (ret)
> + return ret;
> + oldconf->filter = conf->filter;
> + }
> +
> + return inv_icm42607_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode,
> + st->conf.temp_en, sleep_ms);
> +}
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
> new file mode 100644
> index 000000000000..ef73560b39d7
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
> +static int inv_icm42607_gyro_read_sensor(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + s16 *val)
> +{
> + struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> + struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42607_sensor_state *gyro_st = iio_priv(indio_dev);
> + struct device *dev = regmap_get_device(st->map);
> + unsigned int reg;
> + u8 data[2];
> + int ret;
> +
> + if (chan->type != IIO_ANGL_VEL)
> + return -EINVAL;
> +
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + reg = INV_ICM42607_REG_GYRO_DATA_X1;
> + break;
> + case IIO_MOD_Y:
> + reg = INV_ICM42607_REG_GYRO_DATA_Y1;
> + break;
> + case IIO_MOD_Z:
> + reg = INV_ICM42607_REG_GYRO_DATA_Z1;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> + ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
> +
> + /* enable gyro sensor */
> + conf.mode = gyro_st->power_mode;
> + conf.filter = gyro_st->filter;
> + ret = inv_icm42607_set_gyro_conf(st, &conf, NULL);
The sashiko report is basically:
This turns it on, and runtime pm will turn it off but the
state cached ends up such that it is turned on again for
any runtime pm resume.
> + if (ret)
> + return ret;
> +
> + /* read gyro register data */
> + ret = regmap_bulk_read(st->map, reg, data, sizeof(data));
> + if (ret)
> + return ret;
> +
> + *val = get_unaligned_be16(data);
> + if (*val == INV_ICM42607_DATA_INVALID)
> + return -EINVAL;
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH V13 6/9] iio: imu: inv_icm42607: Add Temp Support in icm42607
From: Jonathan Cameron @ 2026-06-21 17:26 UTC (permalink / raw)
To: Chris Morgan
Cc: linux-iio, andy, nuno.sa, dlechner, jean-baptiste.maneyrol,
linux-rockchip, devicetree, heiko, conor+dt, krzk+dt, robh,
andriy.shevchenko, Chris Morgan
In-Reply-To: <20260615172554.160910-7-macroalpha82@gmail.com>
On Mon, 15 Jun 2026 12:25:49 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add functions for reading temperature sensor data.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Another sashiko reported thing. I'd definitely have missed
this one and I think it is correct.
When I get caught up I'll post a thread to see if people
feel we should generally just ask for Sashiko to reply on
list.
Jonathan
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 64f5d263de4f..644cd7f821b9 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -162,6 +162,24 @@ static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
> return 0;
> }
>
> +int inv_icm42607_set_temp_conf(struct inv_icm42607_state *st, bool enable,
> + unsigned int *sleep_ms)
> +{
> + unsigned int val;
> + int ret;
> +
> + val = FIELD_PREP(INV_ICM42607_TEMP_CONFIG0_FILTER_MASK,
> + INV_ICM42607_FILTER_BW_34HZ);
> + ret = regmap_update_bits(st->map, INV_ICM42607_REG_TEMP_CONFIG0,
> + INV_ICM42607_TEMP_CONFIG0_FILTER_MASK, val);
> + if (ret)
> + return ret;
> +
> + return inv_icm42607_set_pwr_mgmt0(st, st->conf.gyro.mode,
> + st->conf.accel.mode, enable,
> + sleep_ms);
> +}
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
> new file mode 100644
> index 000000000000..9a60e1a478b0
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
> +static int inv_icm42607_temp_read(struct inv_icm42607_state *st, s16 *temp)
> +{
> + struct device *dev = regmap_get_device(st->map);
> + u8 raw[2];
> + int ret;
> +
> + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> + ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
> +
> + st->conf.temp_en = true;
Sashiko points out (seems right) that this sets the internal state
before the power management routine expects it to be set. So why
is this here as opposed to just passing true into the
function that follows?
> + ret = inv_icm42607_set_temp_conf(st, st->conf.temp_en, NULL);
> + st->conf.temp_en = false;
> + if (ret)
> + return ret;
> +
> + ret = regmap_bulk_read(st->map, INV_ICM42607_REG_TEMP_DATA1,
> + raw, sizeof(raw));
> + if (ret)
> + return ret;
> +
> + *temp = get_unaligned_be16(raw);
> + if (*temp == INV_ICM42607_DATA_INVALID)
> + return -EINVAL;
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH V13 5/9] iio: imu: inv_icm42607: Add PM support for icm42607
From: Jonathan Cameron @ 2026-06-21 17:19 UTC (permalink / raw)
To: Chris Morgan
Cc: linux-iio, andy, nuno.sa, dlechner, jean-baptiste.maneyrol,
linux-rockchip, devicetree, heiko, conor+dt, krzk+dt, robh,
andriy.shevchenko, Chris Morgan
In-Reply-To: <20260615172554.160910-6-macroalpha82@gmail.com>
On Mon, 15 Jun 2026 12:25:48 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add power management support for the ICM42607 device driver.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
A few things from taking a look at the sashiko report:
https://sashiko.dev/#/patchset/20260615172554.160910-1-macroalpha82%40gmail.com
> ---
> drivers/iio/imu/inv_icm42607/inv_icm42607.h | 18 +++
> .../iio/imu/inv_icm42607/inv_icm42607_core.c | 139 ++++++++++++++++++
> .../iio/imu/inv_icm42607/inv_icm42607_i2c.c | 1 +
> .../iio/imu/inv_icm42607/inv_icm42607_spi.c | 1 +
> 4 files changed, 159 insertions(+)
>
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> index a6a58571935f..4f4f541027dc 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> @@ -334,11 +345,18 @@ struct inv_icm42607_state {
> #define INV_ICM42607_GYRO_STOP_TIME_MS 45
> #define INV_ICM42607_TEMP_STARTUP_TIME_MS 77
>
> +/*
> + * Suspend delay assumed from other icm42600 series device, not
> + * documented in datasheet.
> + */
> +#define INV_ICM42607_SUSPEND_DELAY_MS (2 * USEC_PER_MSEC)
Sashiko had a valid comment on this. MSEC_PER_SEC seems more
appropriate given this is 2 seconds in milli seconds.
> +
> typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
>
> extern const struct regmap_config inv_icm42607_regmap_config;
> extern const struct inv_icm42607_hw inv_icm42607_hw_data;
> extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
> +extern const struct dev_pm_ops inv_icm42607_pm_ops;
>
> int inv_icm42607_core_probe(struct regmap *regmap,
> const struct inv_icm42607_hw *hw,
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 4b8e19091786..64f5d263de4f 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -4,6 +4,7 @@
> */
>
> #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> #include <linux/delay.h>
> #include <linux/dev_printk.h>
> #include <linux/device/devres.h>
> @@ -11,6 +12,7 @@
> #include <linux/iio/iio.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/time.h>
> @@ -103,6 +105,63 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = {
> };
> EXPORT_SYMBOL_NS_GPL(inv_icm42607p_hw_data, "IIO_ICM42607");
>
> +static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
> + enum inv_icm42607_sensor_mode gyro,
> + enum inv_icm42607_sensor_mode accel,
> + bool temp, unsigned int *sleep_ms)
> +{
> + enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
> + enum inv_icm42607_sensor_mode oldgyro = st->conf.gyro.mode;
> + bool oldtemp = st->conf.temp_en;
> + unsigned int sleepval_ms;
> + unsigned int val;
> + int ret;
> +
> + if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
> + return 0;
> +
> + /*
> + * Datasheet on page 14.26 says we need to ensure the gyro sensor is on
> + * for a minimum of 45ms. So if we transition from an on state to an
> + * off state wait 45ms to ensure a sufficient pause before power off.
Sashiko commented on this.. I think what we could do with adding to the
comment is what the path is that didn't pass through this function which would
ensure we have been on for 30 of this msecs already.
> + */
> + if (!gyro && oldgyro)
> + fsleep(INV_ICM42607_GYRO_STOP_TIME_MS * USEC_PER_MSEC);
> +
> + val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> + val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
> + ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
> + if (ret)
> + return ret;
> +
> + st->conf.gyro.mode = gyro;
> + st->conf.accel.mode = accel;
> + st->conf.temp_en = temp;
> +
> + /*
> + * If a state change occurs from off to on, sleep for the startup
> + * time of the sensor, unless a sleep_ms is specified. Since more
> + * than one sensor can be transitioned from off to on, select the
> + * maximum time from each of the sensors changing from off to on.
> + */
> + sleepval_ms = 0;
> + if (temp && !oldtemp)
> + sleepval_ms = max(sleepval_ms, INV_ICM42607_TEMP_STARTUP_TIME_MS);
> +
> + if (accel && !oldaccel)
> + sleepval_ms = max(sleepval_ms, INV_ICM42607_ACCEL_STARTUP_TIME_MS);
> +
> + if (gyro && !oldgyro)
> + sleepval_ms = max(sleepval_ms, INV_ICM42607_GYRO_STARTUP_TIME_MS);
> +
> + if (sleep_ms)
> + *sleep_ms = sleepval_ms;
> + else if (sleepval_ms)
> + fsleep(sleepval_ms * USEC_PER_MSEC);
> +
> + return 0;
> +}
>
> int inv_icm42607_core_probe(struct regmap *regmap,
> @@ -236,6 +305,8 @@ int inv_icm42607_core_probe(struct regmap *regmap,
> if (!st)
> return -ENOMEM;
>
> + dev_set_drvdata(dev, st);
> +
> ret = devm_mutex_init(dev, &st->lock);
> if (ret)
> return ret;
> @@ -271,10 +342,78 @@ int inv_icm42607_core_probe(struct regmap *regmap,
> if (ret)
> return ret;
>
> + ret = devm_pm_runtime_set_active_enabled(dev);
> + if (ret)
> + return ret;
> +
> + pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
> + pm_runtime_use_autosuspend(dev);
Sashiko does put out some stuff here. Please take a look and work out or
test if it is right (I think not but haven't checked that carefully!)
From a quick look I think that the auto disabling of autosuspend does a
rpm_idle() that should result in it suspending...
> +
> return 0;
> }
> EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");
^ permalink raw reply
* Re: [PATCH V13 2/9] dt-bindings: iio: imu: icm42600: Add icm42607
From: Jonathan Cameron @ 2026-06-21 17:18 UTC (permalink / raw)
To: Chris Morgan
Cc: linux-iio, andy, nuno.sa, dlechner, jean-baptiste.maneyrol,
linux-rockchip, devicetree, heiko, conor+dt, krzk+dt, robh,
andriy.shevchenko, Chris Morgan, Krzysztof Kozlowski
In-Reply-To: <20260615172554.160910-3-macroalpha82@gmail.com>
On Mon, 15 Jun 2026 12:25:45 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add the ICM42607 and ICM42607P inertial measurement unit.
>
> This device is functionally very similar to the icm42600 series with a
> very different register layout. The driver does not require an
> interrupt for these specific chip revisions.
This talks about how they are different from other parts already
supported, but I think we need something on how they are different
from each other.
I'm kind of assuming one is a subset of the other? If that is the
case the ideal might be the more complex part falling back to the
simpler one.
Thanks,
Jonathan
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> ---
> .../bindings/iio/imu/invensense,icm42600.yaml | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml b/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
> index 9b2af104f186..81b6e85decd5 100644
> --- a/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
> +++ b/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
> @@ -30,6 +30,8 @@ properties:
> - invensense,icm42600
> - invensense,icm42602
> - invensense,icm42605
> + - invensense,icm42607
> + - invensense,icm42607p
> - invensense,icm42622
> - invensense,icm42631
> - invensense,icm42686
> @@ -67,10 +69,24 @@ properties:
> required:
> - compatible
> - reg
> - - interrupts
>
> allOf:
> - $ref: /schemas/spi/spi-peripheral-props.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - invensense,icm42600
> + - invensense,icm42602
> + - invensense,icm42605
> + - invensense,icm42622
> + - invensense,icm42631
> + - invensense,icm42686
> + - invensense,icm42688
> + then:
> + required:
> + - interrupts
>
> unevaluatedProperties: false
>
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64: dts: qcom: sdm845-oneplus: Update compatible to include model
From: David Heidelberg @ 2026-06-21 17:11 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jason A. Donenfeld, Matthias Schiffer, Vincent Huang,
Bjorn Andersson, Konrad Dybcio, linux-input, devicetree,
linux-kernel, linux-arm-msm, phone-devel, Krzysztof Kozlowski,
Konrad Dybcio
In-Reply-To: <1d0e7e31-f808-4347-955a-7246dea208f5@ixit.cz>
On 28/05/2026 00:13, David Heidelberg wrote:
> On 27/05/2026 23:56, Dmitry Torokhov wrote:
>> Hi David,
>>
>> On Sat, May 23, 2026 at 11:45:35AM +0200, David Heidelberg via B4 Relay wrote:
>>> From: David Heidelberg <david@ixit.cz>
>>>
>>> We know the driver is reporting s3706b, introduce the compatible so we
>>> can more easily introduce quirks for weird touchscreen replacements in
>>> followup series.
>>>
>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>> Signed-off-by: David Heidelberg <david@ixit.cz>
>>> ---
>>> arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/
>>> arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> index 6b7378cf4d493..148164d456a5a 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> @@ -475,17 +475,17 @@ bq27441_fg: bq27441-battery@55 {
>>> };
>>> };
>>> &i2c12 {
>>> status = "okay";
>>> clock-frequency = <400000>;
>>> synaptics-rmi4-i2c@20 {
>>> - compatible = "syna,rmi4-i2c";
>>> + compatible = "syna,rmi4-s3706b", "syna,rmi4-i2c";
>>
>> So I believe we established that this device (s3706b) does not in fact
>> implement rmi4 protocol properly. Why do we have "syna,rmi4-i2c" as a
>> fallback? Shouldn't it be just "syna,rmi4-s3706b"?
>
> The vendor supplies s3706b which does implement the RMI4 properly.
>
> The 3rd party replacement impersonating original parts may not implement it
> properly, but I don't address this issue in this initial submission.
>
> With this compatible we know which original part is used by the vendor and
> installed in the phones, so later we can deduct specific sequences for the
> replacement aftermarket parts to keep phone touchscreen working same as they do
> on Android without affecting other devices.
Hello Dmitry.
May I ask what is currently preventing this series from moving forward?
The first version was posted in 2023 [1]. I picked it up again in 2025 [2] and
am now on the 9th iteration (this patchset). At this point, the series has been
under discussion for well over a year, with relatively little feedback and
increasingly long gaps between review rounds.
The current approach is based on the guidance I have received so far, including
suggestions from the device-tree maintainers. When concerns were raised, I tried
to address them and rework the series accordingly.
What I am struggling with is understanding what specific issue still needs to be
resolved before these patches can be accepted. If there are remaining
requirements, objections to the approach, or technical concerns that I have not
addressed, I would appreciate having them stated explicitly so I can work on them.
I also split out the straightforward, self-contained changes in the hope that at
least those could progress independently while I continued working on any
follow-up requirements. However, even those patches do not appear to be moving
forward.
Could you please clarify what outcome you would like to see from this series,
and what concrete changes would be required to get it accepted?
Thank you,
David
[1]
https://lore.kernel.org/all/20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org/
[2] https://lore.kernel.org/all/20250402-synaptics-rmi4-v4-0-1bb95959e564@ixit.cz/
>
> David
>
>>
>> Thanks.
>>
>
--
David Heidelberg
^ permalink raw reply
* Re: [PATCH v2 3/4] irqchip/gic-v3: Add Renesas R-Car Gen4 erratum workaround
From: Marc Zyngier @ 2026-06-21 17:00 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pci, Yoshihiro Shimoda, Krzysztof Wilczyński,
Bjorn Helgaas, Catalin Marinas, Conor Dooley, Geert Uytterhoeven,
Krzysztof Kozlowski, Lorenzo Pieralisi, Manivannan Sadhasivam,
Rob Herring, devicetree, linux-arm-kernel, linux-doc,
linux-kernel, linux-renesas-soc
In-Reply-To: <20260618220427.14325-4-marek.vasut+renesas@mailbox.org>
On Thu, 18 Jun 2026 23:02:01 +0100,
Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:
>
> Renesas R-Car S4/V4H/V4M GIC600 integration has address width for AXI
> or APB interface configured to 32 bit, it can therefore access only
> the first 4 GiB of physical address space. This information comes from
> R-Car V4H Interface Specification sheet, there is currently no technical
> update number assigned to this limitation. Further input from hardware
> engineer indicates that this limitation also applies to R-Car S4 and V4M.
> Name the limitation GEN4GICITS1, and add a driver quirk to mitigate this
> limitation.
>
> The quirk is keyed on the combination of the GIC implementation
> and the platform identification in the device tree.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
With the SoB chain issue addressed:
Acked-by: Marc Zyngier <maz@kernel.org>
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply
* Re: [PATCH v2 2/4] irqchip/gic-v3: Refactor GIC600 limited to 32bit PA erratum handling
From: Marc Zyngier @ 2026-06-21 16:59 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pci, Krzysztof Wilczyński, Bjorn Helgaas,
Catalin Marinas, Conor Dooley, Geert Uytterhoeven,
Krzysztof Kozlowski, Lorenzo Pieralisi, Manivannan Sadhasivam,
Rob Herring, Yoshihiro Shimoda, devicetree, linux-arm-kernel,
linux-doc, linux-kernel, linux-renesas-soc
In-Reply-To: <20260618220427.14325-3-marek.vasut+renesas@mailbox.org>
On Thu, 18 Jun 2026 23:02:00 +0100,
Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:
>
> The GIC600 implementation is now known to be used on multiple 64-bit
> SoCs, where it has address width for AXI or APB interface configured
> to 32 bit, and it can access only the first 4GiB of physical address
> space.
>
> Rework the handling of the quirk to work around this limitation such
> that new entries can be added purely as new compatible strings, with
> no need to add additional functions or new its_quirk array entries.
>
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Acked-by: Marc Zyngier <maz@kernel.org>
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply
* Re: [PATCH v2 1/4] iio: dac: ad3530r: Refactor setup to table-driven register bank approach
From: Jonathan Cameron @ 2026-06-21 16:49 UTC (permalink / raw)
To: Kim Seer Paller
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Michael Hennerich,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio,
linux-kernel, linux, devicetree
In-Reply-To: <20260615-iio-ad3532r-support-v2-1-84a0af8b83fa@analog.com>
On Mon, 15 Jun 2026 14:20:15 +0800
Kim Seer Paller <kimseer.paller@analog.com> wrote:
> Replace direct register calls in ad3530r_setup() with per-chip register
> address arrays and bank helpers (ad3530r_set_reg_bank_bits,
> ad3530r_write_reg_banks). Convert sw_ldac_trig_reg from a static
> register address to a function pointer for per-bank LDAC trigger
> register selection. Switch spi_device_id to named initializers.
>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
>
> static const struct spi_device_id ad3530r_id[] = {
> - { "ad3530", (kernel_ulong_t)&ad3530_chip },
> - { "ad3530r", (kernel_ulong_t)&ad3530r_chip },
> - { "ad3531", (kernel_ulong_t)&ad3531_chip },
> - { "ad3531r", (kernel_ulong_t)&ad3531r_chip },
> + { .name = "ad3530", .driver_data = (kernel_ulong_t)&ad3530_chip },
> + { .name = "ad3530r", .driver_data = (kernel_ulong_t)&ad3530r_chip },
> + { .name = "ad3531", .driver_data = (kernel_ulong_t)&ad3531_chip },
> + { .name = "ad3531r", .driver_data = (kernel_ulong_t)&ad3531r_chip },
There is a high chance this will cross with Uwe's series that does
this change for all SPI drivers. Before you send each version have
a quick look to see if I've already picked that up. Or perhaps
better to just pick that up your self as a dependency to make
life easy. Just say you have done that in the cover letter.
> { }
> };
> MODULE_DEVICE_TABLE(spi, ad3530r_id);
>
^ permalink raw reply
* Re: [PATCH v2 4/4] iio: dac: ad3530r: Add support for AD3532R/AD3532
From: Jonathan Cameron @ 2026-06-21 16:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kim Seer Paller, David Lechner, Nuno Sá, Andy Shevchenko,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-iio, linux-kernel, linux, devicetree
In-Reply-To: <ai_OeEegWavHcNF1@ashevche-desk.local>
On Mon, 15 Jun 2026 13:05:44 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Mon, Jun 15, 2026 at 02:20:18PM +0800, Kim Seer Paller wrote:
> > The AD3532R/AD3532 is a 16-channel, 16-bit voltage output DAC with a
> > dual-bank register architecture (bank 0 at 0x1000 for channels 0-7,
> > bank 1 at 0x3000 for channels 8-15). It shares similar functionality
> > with AD3530R (channel configuration, LDAC triggering, powerdown control),
> > the main difference being the register address map due to the dual-bank
> > architecture, handled by table-driven helpers.
> >
> > Add AD3532R-specific register definitions, channel specs, per-bank
> > register arrays, a dedicated ad3532r_set_dac_powerdown(), and per-chip
> > regmap_config to limit debugfs-exposed register space to each variant's
> > actual address range.
>
> ...
>
>
> > help
> > - Say yes here to build support for Analog Devices AD3530R, AD3531R
> > - Digital to Analog Converter.
> > + Say yes here to build support for Analog Devices AD3530/AD3530R,
> > + AD3531/AD3531R, and AD3532/AD3532R Digital to Analog Converters.
>
> This just shows how unscalable the above text is. That's why we usually
> recommend to make the list explicit and separated.
>
> Say yes here to build support for the following Analog Devices
> Digital to Analog Converters:
> - AD3530/AD3530R (8-channel)
> - AD3531/AD3531R (4-channel)
> - AD3532/AD3532R (16-channel)
>
> (and looking into the C-file change, perhaps add here as well distinctive
> information, such as number of channels, in the parentheses).
>
> > To compile this driver as a module, choose M here: the
> > module will be called ad3530r.
>
> ...
>
> > +#define AD3532R_INTERFACE_CONFIG_A_0 0x1000
> > +#define AD3532R_INTERFACE_CONFIG_A_1 0x3000
> > +#define AD3532R_OUTPUT_OPERATING_MODE_0 0x1020
> > +#define AD3532R_OUTPUT_OPERATING_MODE_1 0x1021
> > +#define AD3532R_OUTPUT_OPERATING_MODE_2 0x3020
> > +#define AD3532R_OUTPUT_OPERATING_MODE_3 0x3021
> > +#define AD3532R_OUTPUT_CONTROL_0 0x102A
> > +#define AD3532R_OUTPUT_CONTROL_1 0x302A
> > +#define AD3532R_REFERENCE_CONTROL_0 0x103C
> > +#define AD3532R_REFERENCE_CONTROL_1 0x303C
> > +#define AD3532R_SW_LDAC_TRIG_0 0x10E5
> > +#define AD3532R_SW_LDAC_TRIG_1 0x30E5
> > +#define AD3532R_INPUT_CH_0 0x10EB
> > +#define AD3532R_INPUT_CH_1 0x30EB
> > +#define AD3532R_MAX_REG_ADDR 0x30F9
Whilst we are here, Sashiko thinks there is an off by one on that value
as it's the lower of the two registers that make up channel 15.
https://sashiko.dev/#/patchset/20260615-iio-ad3532r-support-v2-0-84a0af8b83fa%40analog.com
It also suggests an existing bug that it would be good to look into.
>
> Hmm... I dunno if it's better to sort by values (so the "bank" 0 goes together
> followed by "bank" 1). Jonathan, what's your preference here? Nuno, David?
That is how people will typically check them vs the datasheet so I
agree with numeric order. Maybe with a comment at the top about there
effectively being two banks. Many of the registers are effectively copies
for the new channels but not all of them, so a macro approach would
probably be even more confusing.
Jonathan
^ permalink raw reply
* Re: [PATCH v10 5/5] iio: adc: versal-sysmon: add oversampling support
From: Jonathan Cameron @ 2026-06-21 16:28 UTC (permalink / raw)
To: Salih Erim
Cc: andy, dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Andy Shevchenko
In-Reply-To: <20260618101414.3462934-6-salih.erim@amd.com>
On Thu, 18 Jun 2026 11:14:14 +0100
Salih Erim <salih.erim@amd.com> wrote:
> Add support for reading and writing the oversampling ratio through
> the IIO oversampling_ratio attribute. The hardware supports averaging
> 2, 4, 8, or 16 samples, plus a ratio of 1 (no averaging).
>
> Temperature and supply channels share oversampling configuration at
> the type level (all temperature channels share one ratio, all supply
> channels share another), exposed through info_mask_shared_by_type.
>
> The hardware encoding uses sample_count / 2 in a 4-bit field within
> the CONFIG register. Per-channel averaging enable registers must also
> be updated to activate or deactivate averaging.
>
> Signed-off-by: Salih Erim <salih.erim@amd.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Sashiko I think correctly calls out that you should have
the oversampling related bits set in info masks for the statically
declared temp channels as well.
Otherwise looks good to me.
I will note that sashiko was busy complaining about at least
some of these in earlier versions. Please always take a look
before sending next version. If it is spouting garbage (which
it does for I2C dma safety for instance) then best option I think
is to reply to your own posting saying what is wrong and what is
correct and to be addressed.
Thanks,
Jonathan
> ---
> Changes in v10:
> - No code changes
>
> Changes in v9:
> - Add Reviewed-by tag from Andy Shevchenko
> - No code changes
>
> Changes in v8:
> - Use unsigned int for val parameter and hw_val in both
> osr_write helpers (Andy)
> - Use ~0 instead of ~0U for avg enable bitmask (Andy)
>
> 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 50b5228aa22..bae229f27c6 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> +
> static int sysmon_read_label(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> char *label)
> @@ -464,6 +602,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,
> @@ -755,6 +896,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev, int ir
> .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 ?
> @@ -786,7 +931,11 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev, int ir
> .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,
This stuff also needs to be set for the static channels as it applies
to them as well. Obviously it only makes a practical difference if
you have no satellite sensors (in which case it makes little sense anyway)
but it is the 'right' thing to do to document they apply there as well.
> };
> }
> @@ -833,6 +982,8 @@ int devm_versal_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;
>
^ permalink raw reply
* Re: [PATCH v10 4/5] iio: adc: versal-sysmon: add threshold event support
From: Jonathan Cameron @ 2026-06-21 16:22 UTC (permalink / raw)
To: Salih Erim
Cc: andy, dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Andy Shevchenko
In-Reply-To: <20260618101414.3462934-5-salih.erim@amd.com>
On Thu, 18 Jun 2026 11:14:13 +0100
Salih Erim <salih.erim@amd.com> wrote:
> Add threshold event support for temperature and supply voltage
> channels.
>
> Temperature events:
> - Rising threshold with configurable value 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>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
There is some stuff from Sashiko that looks plausible.
https://sashiko.dev/#/patchset/20260618101414.3462934-1-salih.erim%40amd.com
Whilst the out of range temp thresholds might not be a bug
that causes anything particular bad to happen it would be nice
to report an error to userspace if a write is for something we
can't support.
There are some things that I can't figure out without data sheet
diving so I'll leave those for you to sanity check + some I think
we addressed in earlier discussions.
For a few of the things it raises I talk about them inline.
Note I didn't spot anything else (and probably wouldn't have
spotted these :(
Jonathan
> ---
> Changes in v10:
> - Add Reviewed-by tag from Andy Shevchenko
> - Add limits.h include for U16_MAX, S16_MIN, S16_MAX (Andy)
>
> Changes in v9:
> - Add minmax.h include for clamp() (Andy)
> - Join sysmon_supply_thresh_offset to one line, change address
> parameter to unsigned long for consistency (Andy)
> - Combine mask declaration with initialization in
> sysmon_read_event_config (Andy)
> - Rename ier to mask in sysmon_write_event_config for
> consistency with sysmon_read_event_config (Andy)
> - Remove blank line in sysmon_update_temp_lower between
> semantically coupled lines (Andy)
> - Rename unmask to ier (u32) in sysmon_unmask_temp (Andy)
> - Variable name and type consistency audit across all
> event functions (Andy)
>
> Changes in v8:
> - Use MILLIDEGREE_PER_DEGREE in q8p7 conversion functions (Andy)
> - Use regmap_test_bits() in sysmon_read_alarm_config (Andy)
> - Join sysmon_parse_fw signature onto one line (Andy)
> - Fix devm teardown race: replace devm_delayed_work_autocancel
> with INIT_DELAYED_WORK; fold cancel_delayed_work_sync into
> sysmon_disable_interrupts to prevent the worker from
> re-enabling interrupts after the IRQ handler is freed (Sashiko)
> - Drop devm-helpers.h include (no longer needed)
>
> 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 | 600 ++++++++++++++++++++++++++-
> drivers/iio/adc/versal-sysmon.h | 36 ++
> 2 files changed, 632 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 03a745d3fb4..50b5228aa22 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> /*
> * Static temperature channels (always present).
> *
> @@ -52,6 +102,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 * MILLIDEGREE_PER_DEGREE) >> SYSMON_FRACTIONAL_SHIFT;
> +}
> +
> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
> +{
> + *raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / MILLIDEGREE_PER_DEGREE;
> +}
> +
> static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
> {
> int mantissa, format, exponent;
> @@ -69,6 +129,33 @@ 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;
See below. Overflow issue is if val is large enough that this overflows
before tmp is clamped, possibly giving unexpected values.
> +
> + if (format)
> + tmp = clamp(tmp, S16_MIN, S16_MAX);
> + else
> + tmp = clamp(tmp, 0, U16_MAX);
> +
> + *raw_data = (u16)tmp;
> +}
...
> +
> +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);
In this path, sashiko is asking whether it is possible for val to be sufficiently large
or negative that the calculation is going to given rather unexpected results.
Given in the read direction you assume it is suitable for passing in a U16, should we
have a check here? + error out if it is out of range?
> +
> + 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);
There is another sashiko report about potential out of range val
here. Probably also want to check for that just as a way to improve
useability.
> +
> + return regmap_write(sysmon->regmap, offset, raw_val);
There is also a comment on whether this is wiping out the fields in the
upper bits of the register. If it isn't (maybe they are read only?)
then a comment here would be good.
> +
> + default:
> + return -EINVAL;
> + }
> +}
^ 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