* Re: [PATCH] iio: stm32-dfsdm: Treat flags as booleans
From: Olivier MOYSAN @ 2026-06-23 9:43 UTC (permalink / raw)
To: Jonathan Cameron, Andy Shevchenko
Cc: Rob Herring (Arm), David Lechner, Nuno Sá, Andy Shevchenko,
Maxime Coquelin, Alexandre Torgue, linux-iio, linux-stm32,
linux-arm-kernel, linux-kernel
In-Reply-To: <20260621151026.69714694@jic23-huawei>
Hi Andy, Jonathan,
Sorry for the late answer.
On 6/21/26 16:10, Jonathan Cameron wrote:
> On Sat, 13 Jun 2026 16:39:16 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>
>> On Fri, Jun 12, 2026 at 04:51:50PM -0500, Rob Herring (Arm) wrote:
>>> The "st,adc-alt-channel" and "st,filter0-sync" properties are
>>> documented as boolean flags. The legacy parser read them as integer
>>> cells, unlike the child-node parser which already checks only for
>>> presence.
>>>
>>> Use presence and boolean helpers so both parsers follow the binding and
>>> the property type checker no longer reports the flags.
>>
>> For the patch
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>>
>> However one interesting remark below.
>>
>> ...
>>
>>> - ret = of_property_read_u32_index(indio_dev->dev.of_node,
>>> - "st,adc-alt-channel", chan_idx,
>>> - &df_ch->alt_si);
>>
>>> + df_ch->alt_si = of_property_present(indio_dev->dev.of_node,
>>
>> I believe it still has another (serious?) issue. We usually don't use indio_dev
>> for device properties. It's not a device that is described in DT.
>> It seems the only driver in IIO that does that. Note, I haven't conducted any
>> deeper research, it might be (however I'm quite in doubt) that this is correct
>> use and one device registers a few indio_dev:s.
>
> It is curious. The registration sequence in this driver is complex, but I'm not
> seeing anything that sets the fwnode for the struct iio_dev->dev before calling
> the init() callbacks that end up in this code. It is set later by iio_device_register()
> (iirc that has something to do with consumers turning up later).
>
> St folk could you take a look at this and see what we are missing
> if it does currently work?
>
> For now I'll apply this patch but might need to drop it if a fix clashes
> with it.
>
> Thanks,
>
> Jonathan
>
>
I confirm that the current legacy path is functional
(With the st,adc-alt-channel property fix applied)
It currently works because the driver initializes np from dev->of_node
in probe, and that value is then used in init callbacks.
I agree that this approach is not robust, as it depends on
initialization sequencing and on using an IIO object that is not the DT
owner object. I will prepare a patch to use the DT device directly as
the single source for DT properties.
I also suggest keeping a fallback path for st,adc-alt-channel so we do
not break legacy DTs that have not yet migrated to the new binding.
I prepare this also.
BRs
Olivier
>
>>
>>> + "st,adc-alt-channel");
>>
>
>
^ permalink raw reply
* Re: [PATCH v4 1/3] iio: adc: Fix incorrect reading when datarate changed in single mode
From: Andy Shevchenko @ 2026-06-23 9:39 UTC (permalink / raw)
To: Joshua Crofts
Cc: Jakub Szczudlo, linux-iio, andy, antoniu.miclaus, conor+dt,
devicetree, dlechner, duje, jic23, jishnu.prakash, jorge.marques,
krzk+dt, linusw, linux-kernel, marcelo.schmitt, mazziesaccount,
mike.looijmans, nuno.sa, robh, sakari.ailus, wens
In-Reply-To: <20260623112953.000066cc@gmail.com>
On Tue, Jun 23, 2026 at 11:29:53AM +0200, Joshua Crofts wrote:
> On Tue, 23 Jun 2026 12:16:39 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
...
> > > + return read_poll_timeout(ads1100_new_data_not_ready, data_ready,
> > > + !data_ready, wait_time,
>
> I'd actually be all for using `data_ready != 0`, to make the condition more
> readable.
I am okay with either. It might be slightly clearer if the comparison is done
for some dynamic counting or so, when 0 is not special.
...
> > > + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);
> >
> > > +
> >
> > This blank line is not needed as they are coupled, but I don't know if we have
> > an agreed style in IIO for this.
>
> I'd be surprised if there was an agreed style, as there aren't any IIO drivers
> that use this specific macro (not in mainline at least). Additionally, might I
> suggest using `PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND` as it is more generic?
We have other PM_ACQUIRE_*() macros in the drivers in IIO, so we have some style,
but I haven't checked what is that.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 1/3] iio: adc: Fix incorrect reading when datarate changed in single mode
From: Joshua Crofts @ 2026-06-23 9:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jakub Szczudlo, linux-iio, andy, antoniu.miclaus, conor+dt,
devicetree, dlechner, duje, jic23, jishnu.prakash, jorge.marques,
krzk+dt, linusw, linux-kernel, marcelo.schmitt, mazziesaccount,
mike.looijmans, nuno.sa, robh, sakari.ailus, wens
In-Reply-To: <ajpO9zaZbIl3x1uC@ashevche-desk.local>
On Tue, 23 Jun 2026 12:16:39 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > + return read_poll_timeout(ads1100_new_data_not_ready, data_ready,
> > + !data_ready, wait_time,
I'd actually be all for using `data_ready != 0`, to make the condition more
readable.
...
> > + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);
>
> > +
>
> This blank line is not needed as they are coupled, but I don't know if we have
> an agreed style in IIO for this.
I'd be surprised if there was an agreed style, as there aren't any IIO drivers
that use this specific macro (not in mainline at least). Additionally, might I
suggest using `PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND` as it is more generic?
--
Kind regards
CJD
^ permalink raw reply
* Re: [PATCH v4 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver
From: Andy Shevchenko @ 2026-06-23 9:23 UTC (permalink / raw)
To: Jakub Szczudlo
Cc: linux-iio, andy, antoniu.miclaus, conor+dt, devicetree, dlechner,
duje, jic23, jishnu.prakash, jorge.marques, joshua.crofts1,
krzk+dt, linusw, linux-kernel, marcelo.schmitt, mazziesaccount,
mike.looijmans, nuno.sa, robh, sakari.ailus, wens
In-Reply-To: <20260622221550.374235-4-jakubszczudlo40@gmail.com>
On Tue, Jun 23, 2026 at 12:15:50AM +0200, Jakub Szczudlo wrote:
> Add ADS1110 support that have faster datarate than ADS1100, it also uses
> internal voltage reference of 2.048V for measurement.
...
> config TI_ADS1100
> - tristate "Texas Instruments ADS1100 and ADS1000 ADC"
> + tristate "Texas Instruments ADS1100 and similar single channel I2C ADC"
> depends on I2C
> help
> - If you say yes here you get support for Texas Instruments ADS1100 and
> - ADS1000 ADC chips.
> + If you say yes here you get support TI ADS1100 and similar single
> + channel I2C Analog to Digital Converters.
User won't know what similar are really supported. The rule of thumb is to add
the list of supported here as
- ADS1000 (...perhaps some very short spec info...)
- ADS1100 (...perhaps some very short spec info...)
> This driver can also be built as a module. If so, the module will be
> called ti-ads1100.
...
> +static int ads1100_get_vref_milivolts(struct ads1100_data *data)
> +{
> + if (data->ads_config->has_internal_vref_only)
> + return ADS1110_INTERNAL_REF_mV;
> +
> + return regulator_get_voltage(data->reg_vdd) / MILLI;
For now we used "(MICRO / MILLI)" instead of "MILLI", to show the unit
conversion.
> +}
...
> if (ret < 0) {
> dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> return ret;
> + } else if (ret < 2) {
Redundant 'else'.
> + dev_err(&data->client->dev, "Short I2C read\n");
> + return -EIO;
> }
...
> - microvolts = regulator_get_voltage(data->reg_vdd);
> + microvolts = ads1100_get_vref_milivolts(data) * (MICRO / MILLI);
See above, here you correctly used the existing pattern, the above is
inconsistent and needs to be addressed.
...
> + model = i2c_get_match_data(client);
> + if (!model)
> + return dev_err_probe(dev, -EINVAL,
> + "Can't get device data from firmware\n");
> +
> + data->ads_config = (struct ads1100_config *)model;
You can't drop const like this. If you need to apply modification,
use devm_kmemdup(). Otherwise it won't work correctly if you have two different
sensors of the same driver in the system.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 1/3] iio: adc: Fix incorrect reading when datarate changed in single mode
From: Andy Shevchenko @ 2026-06-23 9:16 UTC (permalink / raw)
To: Jakub Szczudlo
Cc: linux-iio, andy, antoniu.miclaus, conor+dt, devicetree, dlechner,
duje, jic23, jishnu.prakash, jorge.marques, joshua.crofts1,
krzk+dt, linusw, linux-kernel, marcelo.schmitt, mazziesaccount,
mike.looijmans, nuno.sa, robh, sakari.ailus, wens
In-Reply-To: <20260622221550.374235-2-jakubszczudlo40@gmail.com>
On Tue, Jun 23, 2026 at 12:15:48AM +0200, Jakub Szczudlo wrote:
> When device is suspended and it is in single mode then changing
> datarate doesn't make it actual wait for new measurement, so to
> be sure that read after change is correct functions that changes
> datarate and gain will wait for new data.
...
> +/* Timeout based on the minimum sample rate of 8 SPS (7.5s) */
> +#define ADS1100_MAX_DRDY_TIMEOUT_US 7500000
Not sure if the multiplier will look good here
#define ADS1100_MAX_DRDY_TIMEOUT_US (7500 * USEC_PER_MSEC)
(comment might need an update to use 7500 ms, but see above).
...
> +static bool ads1100_new_data_not_ready(struct ads1100_data *data)
> +{
> + int ret;
> + u8 buffer[3];
> +
> + ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> + if (ret < 0) {
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return true;
> + } else if (ret < 3) {
sizeof()
> + dev_err(&data->client->dev, "Short I2C read\n");
> + return true;
> + }
> +
> + return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +}
...
> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{
> + int ret;
> + u8 buffer[3];
> + bool data_ready;
> + int datarate = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> + /* To be sure we wait 5 times more than datarate */
> + unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
First of all, reversed xmas tree order can be better (especially
when something is assigned). Second, use units in the variable name,
wait_time_us. And at last, use USEC_PER_SEC instead of MICRO
(this will need time.h to be included if not yet).
> + /* To be sure that polled value will have value after config change */
> + ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> + if (ret < 0) {
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return ret;
> + }
> + return read_poll_timeout(ads1100_new_data_not_ready, data_ready,
> + !data_ready, wait_time,
> + ADS1100_MAX_DRDY_TIMEOUT_US, false, data);
Why not readx_poll_timeout()? It's a short cut for the one-argument "read"
function.
> +}
...
> ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>
> - return 0;
> + ret = ads1100_poll_data_ready(data);
> +
> + return ret;
Is it specifically done due to next patch? But this one is marked as Fix and
will go deep back in the releases. For them this will look unjustified. Just
use
return ads1100_...;
here.
> static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
> {
> unsigned int i;
> unsigned int size;
> + int ret;
>
> size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
> for (i = 0; i < size; i++) {
> - if (ads1100_data_rate[i] == rate)
> - return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> - FIELD_PREP(ADS1100_DR_MASK, i));
> + if (ads1100_data_rate[i] != rate)
> + continue;
This will look better if you break here and add a check
if (i == size)
return -EINVAL;
... then your new code...
return ads1100_...;
> + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);
> +
This blank line is not needed as they are coupled, but I don't know if we have
an agreed style in IIO for this.
> + ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> + if (ret)
> + return ret;
> +
> + ret = ads1100_set_config_bits(data, ADS1100_DR_MASK,
> + FIELD_PREP(ADS1100_DR_MASK, i));
> + if (ret)
> + return ret;
> + ret = ads1100_poll_data_ready(data);
> +
> + return ret;
As per above.
> }
>
> return -EINVAL;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH 2/2] iio: adc: ad4080: configure backend data size
From: Antoniu Miclaus @ 2026-06-23 9:07 UTC (permalink / raw)
To: Nuno Sá, Michael Hennerich, Antoniu Miclaus,
Jonathan Cameron, David Lechner, linux, linux-iio, linux-kernel
In-Reply-To: <20260623090717.3296-1-antoniu.miclaus@analog.com>
The AXI backend needs to know the ADC word width in order to pack the
sample data correctly on the bus. During channel setup, program the
backend packet format via iio_backend_data_size_set() using the channel
resolution, so the data is transferred according to the device's
realbits.
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
drivers/iio/adc/ad4080.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
index 8d2953341b15..8feb0c7f7fcb 100644
--- a/drivers/iio/adc/ad4080.c
+++ b/drivers/iio/adc/ad4080.c
@@ -698,6 +698,11 @@ static int ad4080_setup_channel(struct ad4080_state *st, unsigned int ch)
if (ret)
return ret;
+ ret = iio_backend_data_size_set(st->back[ch],
+ st->info->channels[0].scan_type.realbits);
+ if (ret)
+ return ret;
+
if (!st->lvds_cnv_en)
return 0;
--
2.43.0
^ permalink raw reply related
* [PATCH 1/2] iio: adc: adi-axi-adc: add data size support for AD408X backend
From: Antoniu Miclaus @ 2026-06-23 9:07 UTC (permalink / raw)
To: Nuno Sá, Michael Hennerich, Antoniu Miclaus,
Jonathan Cameron, David Lechner, linux, linux-iio, linux-kernel
In-Reply-To: <20260623090717.3296-1-antoniu.miclaus@analog.com>
The AD408X AXI core can pack the sample data on the bus using different
word widths. Expose this through the data_size_set backend operation so
that frontends can program the packet format field (bits 3:2 of the
CNTRL_3 register) according to the ADC resolution: 20-bit, 16-bit and
14-bit map to packet format values 0, 1 and 2 respectively.
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
drivers/iio/adc/adi-axi-adc.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index ced0a2321ecf..aac6f4a0705e 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -54,6 +54,10 @@
#define AXI_AD485X_PACKET_FORMAT_24BIT 0x1
#define AXI_AD485X_PACKET_FORMAT_32BIT 0x2
#define AXI_AD408X_CNTRL_3_FILTER_EN_MSK BIT(0)
+#define AXI_AD408X_CNTRL_3_PACKET_FORMAT_MSK GENMASK(3, 2)
+#define AXI_AD408X_PACKET_FORMAT_20BIT 0x0
+#define AXI_AD408X_PACKET_FORMAT_16BIT 0x1
+#define AXI_AD408X_PACKET_FORMAT_14BIT 0x2
#define ADI_AXI_ADC_REG_SYNC_STATUS 0x0068
#define ADI_AXI_ADC_SYNC_STATUS_ADC_SYNC_MSK BIT(0)
@@ -437,6 +441,31 @@ static int axi_adc_ad408x_filter_type_set(struct iio_backend *back,
AXI_AD408X_CNTRL_3_FILTER_EN_MSK);
}
+static int axi_adc_ad408x_data_size_set(struct iio_backend *back,
+ unsigned int size)
+{
+ struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+ unsigned int val;
+
+ switch (size) {
+ case 20:
+ val = AXI_AD408X_PACKET_FORMAT_20BIT;
+ break;
+ case 16:
+ val = AXI_AD408X_PACKET_FORMAT_16BIT;
+ break;
+ case 14:
+ val = AXI_AD408X_PACKET_FORMAT_14BIT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return regmap_update_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
+ AXI_AD408X_CNTRL_3_PACKET_FORMAT_MSK,
+ FIELD_PREP(AXI_AD408X_CNTRL_3_PACKET_FORMAT_MSK, val));
+}
+
static int axi_adc_ad408x_interface_data_align(struct iio_backend *back,
u32 timeout_us)
{
@@ -660,6 +689,7 @@ static const struct iio_backend_ops adi_ad408x_ops = {
.free_buffer = axi_adc_free_buffer,
.data_sample_trigger = axi_adc_data_sample_trigger,
.filter_type_set = axi_adc_ad408x_filter_type_set,
+ .data_size_set = axi_adc_ad408x_data_size_set,
.interface_data_align = axi_adc_ad408x_interface_data_align,
.num_lanes_set = axi_adc_num_lanes_set,
.debugfs_reg_access = iio_backend_debugfs_ptr(axi_adc_reg_access),
--
2.43.0
^ permalink raw reply related
* [PATCH 0/2] iio: adc: ad4080: add backend data size configuration
From: Antoniu Miclaus @ 2026-06-23 9:07 UTC (permalink / raw)
To: Nuno Sá, Michael Hennerich, Antoniu Miclaus,
Jonathan Cameron, David Lechner, linux, linux-iio, linux-kernel
The AD408X AXI core can pack sample data on the bus using different word
widths (20-bit, 16-bit and 14-bit), selected through the packet format
field of the CNTRL_3 register. Until now this was left at its default,
which only matched the AD4080 20-bit resolution.
This series exposes the packet format through a new data_size_set backend
operation and has the ad4080 driver program it during channel setup based
on the channel resolution, so the bus packing always matches the device
realbits.
Patch 1 adds the data_size_set operation to the adi-axi-adc backend.
Patch 2 calls iio_backend_data_size_set() from the ad4080 channel setup.
Link: https://analogdevicesinc.github.io/hdl/library/axi_ad408x/index.html
Antoniu Miclaus (2):
iio: adc: adi-axi-adc: add data size support for AD408X backend
iio: adc: ad4080: configure backend data size
drivers/iio/adc/ad4080.c | 5 +++++
drivers/iio/adc/adi-axi-adc.c | 30 ++++++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
--
2.43.0
^ permalink raw reply
* Re: [PATCH V13 2/9] dt-bindings: iio: imu: icm42600: Add icm42607
From: Jean-Baptiste Maneyrol @ 2026-06-23 8:14 UTC (permalink / raw)
To: Chris Morgan
Cc: Jonathan Cameron, Chris Morgan, linux-iio@vger.kernel.org,
andy@kernel.org, nuno.sa@analog.com, dlechner@baylibre.com,
linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org,
heiko@sntech.de, conor+dt@kernel.org, krzk+dt@kernel.org,
robh@kernel.org, andriy.shevchenko@intel.com, Krzysztof Kozlowski
In-Reply-To: <PH0PR19MB997338ED05370F27B730FEE60CA5EE2@PH0PR19MB997338.namprd19.prod.outlook.com>
>From: Chris Morgan <macromorgan@hotmail.com>
>Sent: Tuesday, June 23, 2026 02:06
>To: Jean-Baptiste Maneyrol
>Cc: Jonathan Cameron; Chris Morgan; linux-iio@vger.kernel.org; andy@kernel.org; nuno.sa@analog.com; dlechner@baylibre.com; linux-rockchip@lists.infradead.org; devicetree@vger.kernel.org; heiko@sntech.de; conor+dt@kernel.org; krzk+dt@kernel.org; robh@kernel.org; andriy.shevchenko@intel.com; Krzysztof Kozlowski
>Subject: Re: [PATCH V13 2/9] dt-bindings: iio: imu: icm42600: Add icm42607
>
>On Mon, Jun 22, 2026 at 09: 23: 28AM +0000, Jean-Baptiste Maneyrol wrote: > Hello Chris and Jonathan, > > concerning dt bindings, my initial understanding was that we had a file per > driver. But here, Chris is doing a new driver for
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>
>On Mon, Jun 22, 2026 at 09:23:28AM +0000, Jean-Baptiste Maneyrol wrote:
>> Hello Chris and Jonathan,
>>
>> concerning dt bindings, my initial understanding was that we had a file per
>> driver. But here, Chris is doing a new driver for icm42607 while adding new
>> bindings here.
>>
>> Does it means we don't have 1 binding file per driver, and there is no need
>> to create a new binding file for inv_icm42607 driver?
>>
>> Despite the naming, icm42607 chips are a complete new design very different
>> than all other icm42600 chips. It using similar IPs for things like the FIFO,
>> but all other parts are different. Especially, it doesn't use banks for
>> registers access but indirect access delegated to the chip internals for
>> accessing certain registers.
>
>For what it's worth I'm not using any of those registers in the driver
>currently; from what I see in the datasheets I was able to find on the
>web the 42607p doesn't do the indirect register access (again unless
>I'm misreading). To be fair I don't have any other icm42607 chips to
>test against. The 42607c does appear to do such register access.
>
>Thank you,
>Chris
Hello Chris,
here is a link to download ICM-42670-P datasheet, this chip is completely similar
to ICM-42607-P:
https://www.invensense.tdk.com/en-us/download-resource/ds-000451-icm-42670-p-datasheet
Indirect register access is required when you want to use the FIFO for configuring
which data is stored inside or when you want to update gyro/accel hardware
offsets (calibbias iio attribute usually). Also required for a lot of more
complex internal chip configuration.
I didn't had a chance to look at your driver currently. I hope to be able to
have a look soon.
I can you give the figures for the required maximum sleep time for accel and
gyro startups and stops. Usually, they are not provided in datasheet (only mean
values).
Thanks for your work,
JB
>
>>
>> Thanks,
>> JB
>>
>> >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.
>> >
>> >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
>> >
>> >--
>> >2.43.0
>
>
________________________________________
From: Chris Morgan <macromorgan@hotmail.com>
Sent: Tuesday, June 23, 2026 02:06
To: Jean-Baptiste Maneyrol
Cc: Jonathan Cameron; Chris Morgan; linux-iio@vger.kernel.org; andy@kernel.org; nuno.sa@analog.com; dlechner@baylibre.com; linux-rockchip@lists.infradead.org; devicetree@vger.kernel.org; heiko@sntech.de; conor+dt@kernel.org; krzk+dt@kernel.org; robh@kernel.org; andriy.shevchenko@intel.com; Krzysztof Kozlowski
Subject: Re: [PATCH V13 2/9] dt-bindings: iio: imu: icm42600: Add icm42607
On Mon, Jun 22, 2026 at 09: 23: 28AM +0000, Jean-Baptiste Maneyrol wrote: > Hello Chris and Jonathan, > > concerning dt bindings, my initial understanding was that we had a file per > driver. But here, Chris is doing a new driver for
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
ZjQcmQRYFpfptBannerEnd
On Mon, Jun 22, 2026 at 09:23:28AM +0000, Jean-Baptiste Maneyrol wrote:
> Hello Chris and Jonathan,
>
> concerning dt bindings, my initial understanding was that we had a file per
> driver. But here, Chris is doing a new driver for icm42607 while adding new
> bindings here.
>
> Does it means we don't have 1 binding file per driver, and there is no need
> to create a new binding file for inv_icm42607 driver?
>
> Despite the naming, icm42607 chips are a complete new design very different
> than all other icm42600 chips. It using similar IPs for things like the FIFO,
> but all other parts are different. Especially, it doesn't use banks for
> registers access but indirect access delegated to the chip internals for
> accessing certain registers.
For what it's worth I'm not using any of those registers in the driver
currently; from what I see in the datasheets I was able to find on the
web the 42607p doesn't do the indirect register access (again unless
I'm misreading). To be fair I don't have any other icm42607 chips to
test against. The 42607c does appear to do such register access.
Thank you,
Chris
>
> Thanks,
> JB
>
> >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.
> >
> >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
> >
> >--
> >2.43.0
^ permalink raw reply
* Re: [PATCH v3 2/2] iio: adc: add Axiado SARADC driver
From: Petar Stepanovic @ 2026-06-23 8:30 UTC (permalink / raw)
To: Joshua Crofts
Cc: Akhila Kavi, Prasad Bolisetty, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Harshit Shah, linux-iio, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20260622115554.000036a9@gmail.com>
On 6/22/2026 11:55 AM, Joshua Crofts wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> On Mon, 22 Jun 2026 00:47:28 -0700
> Petar Stepanovic <pstepanovic@axiado.com> wrote:
>
>> Add support for the SARADC controller found on Axiado AX3000 and
>> AX3005 SoCs.
>>
>> The driver supports single-shot voltage reads through the IIO
>> subsystem. The number of available input channels is selected from
>> the SoC match data, allowing AX3000 and AX3005 variants to use the
>> same driver.
>>
>> Signed-off-by: Petar Stepanovic <pstepanovic@axiado.com>
>> ---
>> + info->clk_rate = clk_get_rate(info->clk);
>> + if (!info->clk_rate)
>> + return dev_err_probe(dev, -EINVAL, "invalid clock rate\n");
>> +
>> + ret = devm_regulator_get_enable_read_voltage(dev, "vref");
>> + if (ret < 0)
>> + return dev_err_probe(dev, info->vref_uV,
>> + "failed to get vref voltage\n");
> Sashiko raised an issue that I've missed on previous reads - why
> are you using info->vref_uV in dev_err_probe()? The info struct
> is not zeroed out on initialization, which means that dev_err_probe
> will return a different value each time when read_voltage() fails.
> It was designed to accept the retval from whatever function we're
> checking.
Thank you for catching this.
You are right, |dev_err_probe()| should use the return value from |devm_regulator_get_enable_read_voltage()|, not |info->vref_uV|.
I will fix this in the next version by passing |ret| to |dev_err_probe()| and assigning |info->vref_uV| only after the call succeeds.
Regards,
Petar
^ permalink raw reply
* [PATCH v4 4/4] ARM: dts: mediatek: mt6323: add AUXADC support
From: Roman Vivchar via B4 Relay @ 2026-06-23 8:16 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar
In-Reply-To: <20260623-mt6323-adc-v4-0-299680ad3194@protonmail.com>
From: Roman Vivchar <rva333@protonmail.com>
Add the devicetree node for the mt6323 AUXADC.
Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
arch/arm/boot/dts/mediatek/mt6323.dtsi | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/mediatek/mt6323.dtsi b/arch/arm/boot/dts/mediatek/mt6323.dtsi
index c230c865116d..c070f4b0936c 100644
--- a/arch/arm/boot/dts/mediatek/mt6323.dtsi
+++ b/arch/arm/boot/dts/mediatek/mt6323.dtsi
@@ -14,6 +14,11 @@ pmic: mt6323 {
interrupt-controller;
#interrupt-cells = <2>;
+ mt6323_adc: adc {
+ compatible = "mediatek,mt6323-auxadc";
+ #io-channel-cells = <1>;
+ };
+
mt6323_leds: leds {
compatible = "mediatek,mt6323-led";
#address-cells = <1>;
--
2.54.0
^ permalink raw reply related
* [PATCH v4 3/4] mfd: mt6397-core: add mt6323 AUXADC support
From: Roman Vivchar via B4 Relay @ 2026-06-23 8:16 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar
In-Reply-To: <20260623-mt6323-adc-v4-0-299680ad3194@protonmail.com>
From: Roman Vivchar <rva333@protonmail.com>
The mt6323 PMIC includes an AUXADC. Register the AUXADC in the mt6323
devices array to allow the corresponding driver to probe using compatible
string.
Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
drivers/mfd/mt6397-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index 3e58d0764c7e..013b0857fb54 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -125,6 +125,9 @@ static const struct resource mt6323_pwrc_resources[] = {
static const struct mfd_cell mt6323_devs[] = {
{
+ .name = "mt6323-auxadc",
+ .of_compatible = "mediatek,mt6323-auxadc",
+ }, {
.name = "mt6323-rtc",
.num_resources = ARRAY_SIZE(mt6323_rtc_resources),
.resources = mt6323_rtc_resources,
--
2.54.0
^ permalink raw reply related
* [PATCH v4 1/4] dt-bindings: iio: adc: mediatek,mt6359-auxadc: add mt6323 PMIC AUXADC
From: Roman Vivchar via B4 Relay @ 2026-06-23 8:16 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar, Conor Dooley
In-Reply-To: <20260623-mt6323-adc-v4-0-299680ad3194@protonmail.com>
From: Roman Vivchar <rva333@protonmail.com>
The MediaTek mt6323 PMIC includes an AUXADC used for battery voltage,
temperature, and other internal measurements. The IP block is not
register-compatible with mt6359.
Add the devicetree binding documentation and the associated header file
defining the ADC channel constants.
Also change the description to 'MT6350 series and similar' because
the binding already includes more than mt635x series PMICs.
Finally, add the MAINTAINERS entry for the header with ADC constants.
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
.../bindings/iio/adc/mediatek,mt6359-auxadc.yaml | 3 ++-
MAINTAINERS | 6 ++++++
.../dt-bindings/iio/adc/mediatek,mt6323-auxadc.h | 24 ++++++++++++++++++++++
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
index 5d4ab701f51a..852eb7336a5a 100644
--- a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
@@ -4,7 +4,7 @@
$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: MediaTek MT6350 series PMIC AUXADC
+title: MediaTek MT6350 series and similar PMIC AUXADC
maintainers:
- AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
@@ -19,6 +19,7 @@ description:
properties:
compatible:
enum:
+ - mediatek,mt6323-auxadc
- mediatek,mt6357-auxadc
- mediatek,mt6358-auxadc
- mediatek,mt6359-auxadc
diff --git a/MAINTAINERS b/MAINTAINERS
index d1cc0e12fe1f..2551c8cd9e9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16256,6 +16256,12 @@ S: Maintained
F: Documentation/devicetree/bindings/mmc/mtk-sd.yaml
F: drivers/mmc/host/mtk-sd.c
+MEDIATEK MT6323 PMIC AUXADC DRIVER
+M: Roman Vivchar <rva333@protonmail.com>
+L: linux-iio@vger.kernel.org
+S: Maintained
+F: include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
+
MEDIATEK MT6735 CLOCK & RESET DRIVERS
M: Yassine Oudjana <y.oudjana@protonmail.com>
L: linux-clk@vger.kernel.org
diff --git a/include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
new file mode 100644
index 000000000000..6ee9a9ecffc1
--- /dev/null
+++ b/include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef _DT_BINDINGS_MEDIATEK_MT6323_AUXADC_H
+#define _DT_BINDINGS_MEDIATEK_MT6323_AUXADC_H
+
+#define MT6323_AUXADC_BATON2 0
+#define MT6323_AUXADC_CH6 1
+#define MT6323_AUXADC_BAT_TEMP 2
+#define MT6323_AUXADC_CHIP_TEMP 3
+#define MT6323_AUXADC_VCDT 4
+#define MT6323_AUXADC_BATON1 5
+#define MT6323_AUXADC_ISENSE 6
+#define MT6323_AUXADC_BATSNS 7
+#define MT6323_AUXADC_ACCDET 8
+#define MT6323_AUXADC_AUDIO0 9
+#define MT6323_AUXADC_AUDIO1 10
+#define MT6323_AUXADC_AUDIO2 11
+#define MT6323_AUXADC_AUDIO3 12
+#define MT6323_AUXADC_AUDIO4 13
+#define MT6323_AUXADC_AUDIO5 14
+#define MT6323_AUXADC_AUDIO6 15
+#define MT6323_AUXADC_AUDIO7 16
+
+#endif
--
2.54.0
^ permalink raw reply related
* [PATCH v4 2/4] iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver
From: Roman Vivchar via B4 Relay @ 2026-06-23 8:16 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar, Andy Shevchenko
In-Reply-To: <20260623-mt6323-adc-v4-0-299680ad3194@protonmail.com>
From: Roman Vivchar <rva333@protonmail.com>
The mt6323 AUXADC is a 15-bit ADC used for system monitoring. This driver
provides support for reading various channels including battery and
charger voltages, battery and chip temperature, current sensing and
accessory detection.
Add a driver for the AUXADC found in the MediaTek mt6323 PMIC.
Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 11 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mt6323-auxadc.c | 314 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 327 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 2551c8cd9e9d..fb40128451dd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16260,6 +16260,7 @@ MEDIATEK MT6323 PMIC AUXADC DRIVER
M: Roman Vivchar <rva333@protonmail.com>
L: linux-iio@vger.kernel.org
S: Maintained
+F: drivers/iio/adc/mt6323-auxadc.c
F: include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
MEDIATEK MT6735 CLOCK & RESET DRIVERS
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 60038ae8dfc4..a03614b46041 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1137,6 +1137,17 @@ config MCP3911
This driver can also be built as a module. If so, the module will be
called mcp3911.
+config MEDIATEK_MT6323_AUXADC
+ tristate "MediaTek MT6323 PMIC AUXADC driver"
+ depends on MFD_MT6397
+ help
+ Say yes here to enable support for MediaTek MT6323 PMIC Auxiliary ADC.
+ This driver provides multiple channels for system monitoring,
+ such as battery voltage, PMIC temperature, and others.
+
+ This driver can also be built as a module. If so, the module will be
+ called mt6323-auxadc.
+
config MEDIATEK_MT6359_AUXADC
tristate "MediaTek MT6359 PMIC AUXADC driver"
depends on MFD_MT6397
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index c76550415ff1..58161750d6e3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_MCP3422) += mcp3422.o
obj-$(CONFIG_MCP3564) += mcp3564.o
obj-$(CONFIG_MCP3911) += mcp3911.o
+obj-$(CONFIG_MEDIATEK_MT6323_AUXADC) += mt6323-auxadc.o
obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
diff --git a/drivers/iio/adc/mt6323-auxadc.c b/drivers/iio/adc/mt6323-auxadc.c
new file mode 100644
index 000000000000..c450fb6f09cb
--- /dev/null
+++ b/drivers/iio/adc/mt6323-auxadc.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2026 Roman Vivchar <rva333@protonmail.com>
+ *
+ * Based on drivers/iio/adc/mt6359-auxadc.c
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/stringify.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#include <linux/mfd/mt6323/registers.h>
+
+#include <dt-bindings/iio/adc/mediatek,mt6323-auxadc.h>
+
+#define AUXADC_STRUP_CON10_RSTB_SEL BIT(7)
+#define AUXADC_STRUP_CON10_RSTB_SW BIT(5)
+
+#define AUXADC_TOP_CKPDN2_CTL_CK BIT(5)
+
+#define AUXADC_TRIM_CH2_MASK GENMASK(11, 10)
+#define AUXADC_TRIM_CH4_MASK GENMASK(9, 8)
+#define AUXADC_TRIM_CH5_MASK GENMASK(5, 4)
+#define AUXADC_TRIM_CH6_MASK GENMASK(3, 2)
+
+#define AUXADC_CON27_VREF18_ENB_MD BIT(15)
+#define AUXADC_CON27_MD_STATUS BIT(0)
+
+#define AUXADC_CON19_GPS_STATUS BIT(1)
+
+#define AUXADC_CON26_VREF18_SELB BIT(1)
+#define AUXADC_CON26_DECI_GDLY_SEL BIT(0)
+
+#define AUXADC_CON11_VBUF_EN BIT(4)
+
+#define AUXADC_CON19_DECI_GDLY_MASK GENMASK(15, 14)
+#define AUXADC_ADC19_BUSY_MASK GENMASK(15, 1)
+#define AUXADC_READY_MASK BIT(15)
+#define AUXADC_DATA_MASK GENMASK(14, 0)
+
+#define AUXADC_CON9_OSR_MASK GENMASK(12, 10)
+#define AUXADC_DEFAULT_OSR 3
+
+#define MTK_PMIC_IIO_CHAN(_name, _chan, _addr) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = _chan, \
+ .address = _addr, \
+ .datasheet_name = __stringify(_name), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+}
+
+/*
+ * AUXADC reports everything in mV, including temperature and
+ * current channels. Channel macros are mapped such that their
+ * ID matches their respective hardware bit position in CON22.
+ */
+static const struct iio_chan_spec mt6323_auxadc_channels[] = {
+ MTK_PMIC_IIO_CHAN(baton2, MT6323_AUXADC_BATON2, MT6323_AUXADC_ADC6),
+ MTK_PMIC_IIO_CHAN(ch6, MT6323_AUXADC_CH6, MT6323_AUXADC_ADC11),
+ MTK_PMIC_IIO_CHAN(bat_temp, MT6323_AUXADC_BAT_TEMP, MT6323_AUXADC_ADC5),
+ MTK_PMIC_IIO_CHAN(chip_temp, MT6323_AUXADC_CHIP_TEMP, MT6323_AUXADC_ADC4),
+ MTK_PMIC_IIO_CHAN(vcdt, MT6323_AUXADC_VCDT, MT6323_AUXADC_ADC2),
+ MTK_PMIC_IIO_CHAN(baton1, MT6323_AUXADC_BATON1, MT6323_AUXADC_ADC3),
+ MTK_PMIC_IIO_CHAN(isense, MT6323_AUXADC_ISENSE, MT6323_AUXADC_ADC1),
+ MTK_PMIC_IIO_CHAN(batsns, MT6323_AUXADC_BATSNS, MT6323_AUXADC_ADC0),
+ MTK_PMIC_IIO_CHAN(accdet, MT6323_AUXADC_ACCDET, MT6323_AUXADC_ADC7),
+};
+
+/*
+ * The MediaTek MT6323 (as well as a lot of other PMICs) has the following hierarchy:
+ * PMIC AUXADC <- PMIC MFD <- SoC PWRAP (wrapper for PWRAP FSM)
+ *
+ * Therefore, PWRAP regmap should be obtained using dev->parent->parent.
+ */
+struct mt6323_auxadc {
+ struct regmap *regmap;
+ /* AUXADC doesn't support reading multiple channels simultaneously. */
+ struct mutex lock;
+};
+
+static int mt6323_auxadc_prepare_channel(struct mt6323_auxadc *auxadc)
+{
+ struct regmap *map = auxadc->regmap;
+ u32 val;
+ int ret;
+
+ ret = regmap_read(map, MT6323_AUXADC_CON19, &val);
+ if (ret)
+ return ret;
+
+ /* The ADC is idle. */
+ if (!(val & AUXADC_CON19_DECI_GDLY_MASK))
+ return 0;
+
+ ret = regmap_read_poll_timeout(map, MT6323_AUXADC_ADC19,
+ val, !(val & AUXADC_ADC19_BUSY_MASK),
+ 10, 500);
+ if (ret)
+ return ret;
+
+ return regmap_clear_bits(map, MT6323_AUXADC_CON19,
+ AUXADC_CON19_DECI_GDLY_MASK);
+}
+
+static int mt6323_auxadc_request(struct mt6323_auxadc *auxadc,
+ unsigned long channel)
+{
+ struct regmap *map = auxadc->regmap;
+ int ret;
+
+ ret = regmap_set_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
+ if (ret)
+ return ret;
+
+ return regmap_set_bits(map, MT6323_AUXADC_CON22, BIT(channel));
+}
+
+static int mt6323_auxadc_release(struct mt6323_auxadc *auxadc,
+ unsigned long channel)
+{
+ struct regmap *map = auxadc->regmap;
+ int ret;
+
+ ret = regmap_clear_bits(map, MT6323_AUXADC_CON22, BIT(channel));
+ if (ret)
+ return ret;
+
+ return regmap_clear_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
+}
+
+static int mt6323_auxadc_read(struct mt6323_auxadc *auxadc,
+ const struct iio_chan_spec *chan, int *out)
+{
+ struct regmap *map = auxadc->regmap;
+ u32 val;
+ int ret;
+
+ ret = regmap_read_poll_timeout(map, chan->address,
+ val, (val & AUXADC_READY_MASK),
+ 1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
+ if (ret)
+ return ret;
+
+ *out = FIELD_GET(AUXADC_DATA_MASK, val);
+
+ return 0;
+}
+
+static int mt6323_auxadc_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct mt6323_auxadc *auxadc = iio_priv(indio_dev);
+ int ret, mult;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ if (chan->channel == MT6323_AUXADC_ISENSE ||
+ chan->channel == MT6323_AUXADC_BATSNS)
+ mult = 4;
+ else
+ mult = 1;
+
+ /* 1800mV full range with 15-bit resolution. */
+ *val = mult * 1800;
+ *val2 = 15;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+ case IIO_CHAN_INFO_RAW: {
+ guard(mutex)(&auxadc->lock);
+
+ ret = mt6323_auxadc_prepare_channel(auxadc);
+ if (ret)
+ return ret;
+
+ ret = mt6323_auxadc_request(auxadc, chan->channel);
+ if (ret)
+ return ret;
+
+ /* Hardware limitation: the AUXADC needs a delay to become ready. */
+ fsleep(300);
+
+ ret = mt6323_auxadc_read(auxadc, chan, val);
+
+ if (mt6323_auxadc_release(auxadc, chan->channel))
+ dev_err(&indio_dev->dev,
+ "failed to release channel %d\n", chan->channel);
+
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mt6323_auxadc_init(struct mt6323_auxadc *auxadc)
+{
+ struct regmap *map = auxadc->regmap;
+ int ret;
+
+ ret = regmap_set_bits(map, MT6323_STRUP_CON10,
+ AUXADC_STRUP_CON10_RSTB_SW |
+ AUXADC_STRUP_CON10_RSTB_SEL);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_TOP_CKPDN2_CTL_CK);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(map, MT6323_AUXADC_CON10,
+ AUXADC_TRIM_CH2_MASK | AUXADC_TRIM_CH4_MASK |
+ AUXADC_TRIM_CH5_MASK | AUXADC_TRIM_CH6_MASK,
+ FIELD_PREP(AUXADC_TRIM_CH2_MASK, 1) |
+ FIELD_PREP(AUXADC_TRIM_CH4_MASK, 1) |
+ FIELD_PREP(AUXADC_TRIM_CH5_MASK, 1) |
+ FIELD_PREP(AUXADC_TRIM_CH6_MASK, 1));
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6323_AUXADC_CON27,
+ AUXADC_CON27_VREF18_ENB_MD |
+ AUXADC_CON27_MD_STATUS);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6323_AUXADC_CON19, AUXADC_CON19_GPS_STATUS);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6323_AUXADC_CON26,
+ AUXADC_CON26_VREF18_SELB |
+ AUXADC_CON26_DECI_GDLY_SEL);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(map, MT6323_AUXADC_CON9, AUXADC_CON9_OSR_MASK,
+ FIELD_PREP(AUXADC_CON9_OSR_MASK, AUXADC_DEFAULT_OSR));
+}
+
+static const struct iio_info mt6323_auxadc_iio_info = {
+ .read_raw = mt6323_auxadc_read_raw,
+};
+
+static int mt6323_auxadc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mt6323_auxadc *auxadc;
+ struct regmap *regmap;
+ struct iio_dev *iio;
+ int ret;
+
+ regmap = dev_get_regmap(dev->parent->parent, NULL);
+ if (!regmap)
+ return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
+
+ iio = devm_iio_device_alloc(dev, sizeof(*auxadc));
+ if (!iio)
+ return -ENOMEM;
+
+ auxadc = iio_priv(iio);
+ auxadc->regmap = regmap;
+
+ ret = devm_mutex_init(dev, &auxadc->lock);
+ if (ret)
+ return ret;
+
+ ret = mt6323_auxadc_init(auxadc);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to initialize auxadc\n");
+
+ iio->name = "mt6323-auxadc";
+ iio->info = &mt6323_auxadc_iio_info;
+ iio->modes = INDIO_DIRECT_MODE;
+ iio->channels = mt6323_auxadc_channels;
+ iio->num_channels = ARRAY_SIZE(mt6323_auxadc_channels);
+
+ return devm_iio_device_register(dev, iio);
+}
+
+static const struct of_device_id mt6323_auxadc_of_match[] = {
+ { .compatible = "mediatek,mt6323-auxadc" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mt6323_auxadc_of_match);
+
+static struct platform_driver mt6323_auxadc_driver = {
+ .driver = {
+ .name = "mt6323-auxadc",
+ .of_match_table = mt6323_auxadc_of_match,
+ },
+ .probe = mt6323_auxadc_probe,
+};
+module_platform_driver(mt6323_auxadc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MediaTek MT6323 PMIC AUXADC Driver");
--
2.54.0
^ permalink raw reply related
* [PATCH v4 0/4] AUXADC driver for the MediaTek mt6323 PMIC
From: Roman Vivchar via B4 Relay @ 2026-06-23 8:16 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar, Conor Dooley,
Andy Shevchenko
This series adds support for the 15-bit AUXADC hardware block found on
the MediaTek mt6323 PMIC.
The previous version of the series for all AUXADC, EFUSE and thermal
drivers was split after Krzysztof's comment [1].
Tested on the MediaTek mt6572 and mt8163 SoCs (Ben), both paired with a
mt6323.
[1]: https://lore.kernel.org/linux-mediatek/20260504-mt6323-v1-0-799b58b355ff@protonmail.com/T/#med30fad67a090be35f549231336b2dec295233f6
Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
Changes in v4:
- dt-bindings: Drop separate driver mention from the commit message (Conor)
- AUXADC driver: Break one more time 'regmap_read_poll_timeout' on logical boundaries (Andy)
- Link to v3: https://patch.msgid.link/20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com
Changes in v3:
- AUXADC driver:
- Add comment for channels table about voltage and channel IDs (Jonathan)
- Add comment for mutex in the 'mt6323_auxadc' struct (Jonathan)
- Break 'regmap_read_poll_timeout' on logical boundaries (Andy)
- Switch to 'guard' from 'scoped_guard' (Andy)
- Link to v2: https://patch.msgid.link/20260609-mt6323-adc-v2-0-aa93a22309f9@protonmail.com
Changes in v2:
- AUXADC driver:
- Drop channel type from the MTK_PMIC_IIO_CHAN macro (Nuno)
- Drop kerneldoc for the mt6323_auxadc struct (Nuno)
- Add channel release to save power (Sashiko, Jonathan)
- Drop 'reg' variable in the mt6323_auxadc_read (Jonathan)
- Sort variables in the mt6323_auxadc_probe (Jonathan)
- Maintainers:
- Drop linux-mediatek list (Andy)
- Split between dt-bindings and driver to avoid missing file (Nuno)
- Link to v1: https://patch.msgid.link/20260602-mt6323-adc-v1-0-68ec737508ee@protonmail.com
Changes after split:
- dt-bindings: Change 'MT63xx' to 'MT6350 series and similar' (Jonathan)
- AUXADC driver:
- Add missing headers (Andy)
- Fix AUXADC_TRIM_CH* values (Andy)
- Rename masks to include their register name (Jonathan)
- Fix formatting (Andy, Jonathan)
- Replace channel address with actual register value (Jonathan), align the table
- Replace IIO_TEMP with IIO_VOLTAGE, since the actual output is still mV, not mC
- Rename constants to match their registers (Jonathan)
- Remove 'if/else if/else' in the mt6323_auxadc_read_raw (Andy)
- Add comments for fsleep, ADC range and resolution (Andy, Jonathan)
- Remove useless error messages (Andy)
- Maintainers:
- Explicitly include mt6323 in the name (Jonathan)
- Squash with AUXADC driver commit (Krzysztof)
- Set status back to 'Maintained'
- Link to a previous series: https://patch.msgid.link/20260512-mt6323-v2-0-3efcba579e88@protonmail.com
---
Roman Vivchar (4):
dt-bindings: iio: adc: mediatek,mt6359-auxadc: add mt6323 PMIC AUXADC
iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver
mfd: mt6397-core: add mt6323 AUXADC support
ARM: dts: mediatek: mt6323: add AUXADC support
.../bindings/iio/adc/mediatek,mt6359-auxadc.yaml | 3 +-
MAINTAINERS | 7 +
arch/arm/boot/dts/mediatek/mt6323.dtsi | 5 +
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mt6323-auxadc.c | 314 +++++++++++++++++++++
drivers/mfd/mt6397-core.c | 3 +
.../dt-bindings/iio/adc/mediatek,mt6323-auxadc.h | 24 ++
8 files changed, 367 insertions(+), 1 deletion(-)
---
base-commit: 028ef9c96e96197026887c0f092424679298aae8
change-id: 20260525-mt6323-adc-3befce36cbf2
Best regards,
--
Roman Vivchar <rva333@protonmail.com>
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: iio: dac: Add AD5529R
From: Rodrigo Alencar @ 2026-06-23 8:09 UTC (permalink / raw)
To: Nuno Sá, Rodrigo Alencar
Cc: Jonathan Cameron, Conor Dooley, Janani Sunil, 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: <ajklksIDLsj0BZul@nsa>
On 22/06/26 13:20, Nuno Sá wrote:
> On Mon, Jun 22, 2026 at 12:51:20PM +0100, Rodrigo Alencar wrote:
> > On 22/06/26 11:29, Nuno Sá wrote:
> > > On Mon, Jun 22, 2026 at 10:24:05AM +0100, Rodrigo Alencar wrote:
> > > > On 21/06/26 15:33, 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.
> > > > >
> > > >
> > > > If the device-addressing on the same chip-select is to be handled
> > > > by the spi framework, wouldn't we lose device-specific features?
> > > >
> > > > I understand that this multi-device feature is there mostly to extend the
> > > > channel count from 16 to 32, 48 or 64. I suppose the command:
> > > >
> > > > "MULTI DEVICE SW LDAC MODE"
> > > >
> > > > exists so that software can update channel values accross multiple devices.
> > >
> > > Right! You do have a point! I agree the main driver for a feature like
> > > this is likely to extend the channel count and effectively "aggregate"
> > > devices.
> > >
> > > But I would say that even with the spi solution the MULTI DEVICE stuff
> > > should be doable (as we still need a sort of adi,pin-id property).
> >
> > I don't think we can have something like an IIO buffer shared by multiple
> > devices. Synchronizing separate devices would be doable with proper hardware
> > support for this (probably involving an FGPA).
>
> True!
>
> >
> > > But yes, I do feel that the whole feature is for aggregation so seeing
> > > one device with 32 channels is the expectation here? Rather than seeing
> > > two devices with 16 channels.
> >
> > Yes, I think aggregation is the whole point there... so that the IIO driver
> > is multi-device-aware.
>
> Which makes me feel that different pins per device might be possible
> from an HW point of view but does not make much sense. For example, for
> the buffer example I would expect LDAC to be shared between all the
> devices.
That is why I would still suggest the multi-dac node in the middle...
the parent node can hold shared resources, while the dac children can
have their own, overriding or inheriting stuff.
--
Kind regards,
Rodrigo Alencar
^ permalink raw reply
* Re: [PATCH V13 8/9] iio: imu: inv_icm42607: Add Gyroscope to icm42607
From: Andy Shevchenko @ 2026-06-23 6:52 UTC (permalink / raw)
To: Chris Morgan
Cc: Chris Morgan, linux-iio, andy, nuno.sa, dlechner, jic23,
jean-baptiste.maneyrol, linux-rockchip, devicetree, heiko,
conor+dt, krzk+dt, robh
In-Reply-To: <PH0PR19MB997338E86152468CE26F60953FA5E42@PH0PR19MB997338.namprd19.prod.outlook.com>
On Wed, Jun 17, 2026 at 04:10:49PM -0500, Chris Morgan wrote:
> On Tue, Jun 16, 2026 at 01:13:03PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 15, 2026 at 12:25:51PM -0500, Chris Morgan wrote:
...
> > Can be some of the code deduplicated between gyro and accel?
>
> Probably a fair amount, but the deduplication will likely need to be
> undone somewhat if we get buffer, WoM or apex support added back
> (I don't have any devices with such functionality, so if anyone will
> do it then it won't be me). I can refactor more if you want, or we
> can keep it split like this to make it easy if someone else wants to
> tackle the buffers/IRQs stuff later? Your call.
Just asking. Jonathan, David, Nuno, what's your opinion on this?
Personal opinion is to avoid solving the issues that do not exist.
If you are not committing into those features, let's not prepare
driver for them right now.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v5 2/3] counter: add GPIO-based counter driver
From: William Breathitt Gray @ 2026-06-23 5:37 UTC (permalink / raw)
To: Wadim Mueller
Cc: William Breathitt Gray, Oleksij Rempel, kernel, conor+dt, krzk+dt,
robh, linux-iio, devicetree, linux-kernel
In-Reply-To: <20260622205210.10317-1-wafgo01@gmail.com>
On Mon, Jun 22, 2026 at 10:51:40PM +0200, Wadim Mueller wrote:
> On Wed, 17 Jun 2026 16:49:25 +0900
> William Breathitt Gray <wbg@kernel.org> wrote:
>
> Hi William,
>
> thanks for the review. Three things before I spin v6.
>
> > One change I consider is whether to make Signal B optional. [...]
> > I wonder whether this is substantially different enough from
> > simply using the interrupt-cnt module on the respective IRQ?
> > I'm CCing Oleksij and the Pengutronix team in case they wish to
> > comment.
>
> I want to keep signal-b mandatory in v6 (if no concerns from Oleksij). The single-line case is
> already covered by interrupt-cnt.
All right, let's keep it required for v6. If for some reason we
determine it should be optional, we can address that after v6.
> > In such a configuration, we would have two Counts: Count 1 [...]
> > Count 2 supports only increase/decrease modes with a Synapse for
> > Signal B.
>
> Just to confirm, plan for v6 is:
>
> Count 0 "AB Count": A + B + optional index0, all 8 functions
> Count 1 "B Count": B + optional index1, increase/decrease only
>
> One counter_ops, dispatch on count->id. Per-count state in
> struct gpio_counter_count_priv (value, ceiling, preset, preset_enabled,
> enabled, function, direction), held in priv->count_priv[2] as you
> suggested. prev_a/prev_b stay on priv (they describe the wire, not
> the count).
Yes, that is correct. However, I would not name the Counts "AB Count"
and "B Count" -- it's possible that relationship will change in the
future if we add support for more function modes to Count 2. Naming them
"Count 1" and "Count 2" is adequate enough because the Signal
relationship is apparent via the Synapses.
> For the second Index in DT I would just let index-gpios take 1..2
> entries (first = count0, second = count1), no new property. Ok for
> you and Conor?
That seems okay to me. I'm generally indifferent to the DT so I'll leave
it up to what you and Conor decide.
> > Hmm, is it a problem that priv->enabled is changed to a false state
> > before the IRQs are actually disabled? Do any issues arise if an IRQ
> > is handled during that brief period of time?
>
> I guess it is a race. In v6 I will reorder:
>
> enable=1: enable_irq(); lock; enabled = true; unlock;
> enable=0: lock; enabled = false; unlock; disable_irq();
>
> Plus a mutex around enable_write so two writers can not interleave
> (disable_irq() can not run under the spinlock).
I think the race conditions still exist despite your reorder: after
enable_irq() an interrupt could fire before you take the lock to set
priv->enabled to true; similarly, after setting priv->enabled to false
and unlocking, an interrupt could fire before you call disable_irq().
We need some atomic way to update priv->enabled and enable/disable the
IRQ at the same time, but I'm not sure how to accomplish that off the
top of my head. I don't want to delay your v6, and I'm not even sure
this race is actually a problem in practice, so let's discuss this again
after your v6 submission.
William Breathitt Gray
^ permalink raw reply
* Re: [PATCH v5 2/3] counter: add GPIO-based counter driver
From: Oleksij Rempel @ 2026-06-23 5:20 UTC (permalink / raw)
To: Wadim Mueller
Cc: William Breathitt Gray, robh, conor+dt, devicetree, linux-iio,
linux-kernel, kernel, krzk+dt
In-Reply-To: <20260622205210.10317-1-wafgo01@gmail.com>
Hi,
On Mon, Jun 22, 2026 at 10:51:40PM +0200, Wadim Mueller wrote:
> On Wed, 17 Jun 2026 16:49:25 +0900
> William Breathitt Gray <wbg@kernel.org> wrote:
>
> Hi William,
>
> thanks for the review. Three things before I spin v6.
>
> > One change I consider is whether to make Signal B optional. [...]
> > I wonder whether this is substantially different enough from
> > simply using the interrupt-cnt module on the respective IRQ?
> > I'm CCing Oleksij and the Pengutronix team in case they wish to
> > comment.
>
> I want to keep signal-b mandatory in v6 (if no concerns from Oleksij).
> The single-line case is already covered by interrupt-cnt.
Hm, I have nothing against extending the existing driver.
If you wont to enforce and validate multi line concept, additional
compatible will be needed. May be interrupt-counter-multiline ?
Please add separate interrupt handler, interrupt_cnt_isr is already
"over optimized". Adding more code to this hot path, will break existing
configurations.
Best Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH V13 8/9] iio: imu: inv_icm42607: Add Gyroscope to icm42607
From: Chris Morgan @ 2026-06-23 2:37 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Chris Morgan, linux-iio, andy, nuno.sa, dlechner,
jean-baptiste.maneyrol, linux-rockchip, devicetree, heiko,
conor+dt, krzk+dt, robh, andriy.shevchenko
In-Reply-To: <20260621183408.032e18b4@jic23-huawei>
On Sun, Jun 21, 2026 at 06:34:08PM +0100, Jonathan Cameron wrote:
> 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.
Yep, for this and for the accel sensor it looks like it gets stuck on.
I've revamped this code for the next round and so far it looks like the
sensor is powering down correctly.
>
> > + 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;
> > +}
>
Thank you,
Chris
^ permalink raw reply
* Re: [PATCH V13 6/9] iio: imu: inv_icm42607: Add Temp Support in icm42607
From: Chris Morgan @ 2026-06-23 2:35 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Chris Morgan, linux-iio, andy, nuno.sa, dlechner,
jean-baptiste.maneyrol, linux-rockchip, devicetree, heiko,
conor+dt, krzk+dt, robh, andriy.shevchenko
In-Reply-To: <20260621182612.1a19278a@jic23-huawei>
On Sun, Jun 21, 2026 at 06:26:12PM +0100, Jonathan Cameron wrote:
> 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?
I'm going to take setting of state out of the power management function
entirely, as well as killing off any "temp_en" since technically there
is nothing to enable on the temp sensor. I'll do a check when you try
to read the temp to see if any sensors are on and if none are then I
will power up the accel sensor (since it uses about 5x less power than
the gyro per the datasheet).
>
> > + 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;
> > +}
Thank you,
Chris
^ permalink raw reply
* Re: [PATCH V13 5/9] iio: imu: inv_icm42607: Add PM support for icm42607
From: Chris Morgan @ 2026-06-23 2:33 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Chris Morgan, linux-iio, andy, nuno.sa, dlechner,
jean-baptiste.maneyrol, linux-rockchip, devicetree, heiko,
conor+dt, krzk+dt, robh, andriy.shevchenko
In-Reply-To: <20260621181948.21d40a09@jic23-huawei>
On Sun, Jun 21, 2026 at 06:19:48PM +0100, Jonathan Cameron wrote:
> 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.
I'm going to track whatever time the gyro started, and then if less
than 45ms has elapsed just pause the remaining amount of time.
>
> > + */
> > + 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...
>
I see a few other drivers adding one more call to
devm_pm_runtime_enable() so I'm going to see how that works out.
>
> > +
> > return 0;
> > }
> > EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");
Thank you,
Chris
^ permalink raw reply
* [PATCH] HID: hid-sensor-custom: Fix sysfs group leak on failure
From: Haoxiang Li @ 2026-06-23 2:19 UTC (permalink / raw)
To: jikos, jic23, srinivas.pandruvada, bentiss
Cc: linux-input, linux-iio, linux-kernel, Haoxiang Li, stable
hid_sensor_custom_add_attributes() creates one sysfs group for each
custom sensor field. If sysfs_create_group() fails after some groups
have already been created, the function currently breaks out of the
loop and returns the error directly.
Fix this by adding a local unwind path when sysfs_create_group() fails.
The unwind path removes all sysfs groups that were successfully created
before the failure and frees sensor_inst->fields.
Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
---
drivers/hid/hid-sensor-custom.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index afffea894021..cd676516e6b0 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -609,7 +609,7 @@ static int hid_sensor_custom_add_attributes(struct hid_sensor_custom
&sensor_inst->fields[i].
hid_custom_attribute_group);
if (ret)
- break;
+ goto err_remove_groups;
/* For power or report field store indexes */
if (sensor_inst->fields[i].attribute.attrib_id ==
@@ -621,6 +621,13 @@ static int hid_sensor_custom_add_attributes(struct hid_sensor_custom
}
return ret;
+
+err_remove_groups:
+ while (--i >= 0)
+ sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
+ &sensor_inst->fields[i].hid_custom_attribute_group);
+ kfree(sensor_inst->fields);
+ return ret;
}
static void hid_sensor_custom_remove_attributes(struct hid_sensor_custom *
--
2.25.1
^ permalink raw reply related
* [PATCH v11 5/5] iio: adc: versal-sysmon: add oversampling support
From: Salih Erim @ 2026-06-23 1:40 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim, Andy Shevchenko
In-Reply-To: <20260623014036.3865402-1-salih.erim@amd.com>
Add support for reading and writing the oversampling ratio through
the IIO oversampling_ratio attribute. The hardware supports averaging
2, 4, 8, or 16 samples, plus a ratio of 1 (no averaging).
Temperature and supply channels share oversampling configuration at
the type level (all temperature channels share one ratio, all supply
channels share another), exposed through info_mask_shared_by_type.
The hardware encoding uses sample_count / 2 in a 4-bit field within
the CONFIG register. Per-channel averaging enable registers must also
be updated to activate or deactivate averaging.
Signed-off-by: Salih Erim <salih.erim@amd.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
Changes in v11:
- Add oversampling info_mask bits to static SYSMON_CHAN_TEMP
macro (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 | 159 ++++++++++++++++++++++++++-
drivers/iio/adc/versal-sysmon.h | 17 +++
2 files changed, 174 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
index 8f2c502d9cb..1b55d343982 100644
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -28,6 +28,12 @@
#include "versal-sysmon.h"
+/*
+ * Oversampling ratio values exposed to userspace via IIO.
+ * Actual number of samples averaged: 1=none, 2=2x, 4=4x, 8=8x, 16=16x.
+ */
+static const int sysmon_oversampling_avail[] = { 1, 2, 4, 8, 16 };
+
/* TEMP hysteresis mode bit in SYSMON_TEMP_EV_CFG */
#define SYSMON_TEMP_HYST_MASK BIT(1)
@@ -42,7 +48,11 @@
.address = _address, \
.channel = _chan, \
.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 = _name, \
}
@@ -166,6 +176,12 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
guard(mutex)(&sysmon->lock);
+ if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO) {
+ *val = (chan->type == IIO_TEMP) ? sysmon->temp_oversampling :
+ sysmon->supply_oversampling;
+ return IIO_VAL_INT;
+ }
+
switch (chan->type) {
case IIO_TEMP:
if (mask == IIO_CHAN_INFO_SCALE) {
@@ -465,6 +481,132 @@ static int sysmon_write_event_value(struct iio_dev *indio_dev,
}
}
+static int sysmon_set_avg_enable(struct sysmon *sysmon,
+ u32 base, u32 count, u32 val)
+{
+ struct regmap *map = sysmon->regmap;
+ int ret;
+
+ for (unsigned int i = 0; i < count; i++) {
+ ret = regmap_write(map, base + i * SYSMON_REG_STRIDE, val);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int sysmon_osr_write_temp(struct sysmon *sysmon, unsigned int val)
+{
+ /*
+ * HW register encoding is sample_count / 2:
+ * 0=none, 1=2x, 2=4x, 4=8x, 8=16x (not log2-based).
+ */
+ unsigned int hw_val = val >> 1;
+ unsigned int readback;
+ int ret;
+
+ ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
+ SYSMON_CONFIG_TEMP_SAT_OSR,
+ FIELD_PREP(SYSMON_CONFIG_TEMP_SAT_OSR, hw_val));
+ if (ret)
+ return ret;
+
+ /*
+ * Readback fence: the SysMon CONFIG register resides in the
+ * PMC domain behind the NoC. A posted write may not reach the
+ * hardware before the next MMIO access. Reading the register
+ * back forces the interconnect to complete the write, preventing
+ * a bus hang on the subsequent access.
+ */
+ regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
+
+ return sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
+ SYSMON_TEMP_EN_AVG_COUNT,
+ hw_val ? ~0 : 0);
+}
+
+static int sysmon_osr_write_supply(struct sysmon *sysmon, unsigned int val)
+{
+ /* HW encoding: sample_count / 2 (see sysmon_osr_write_temp) */
+ unsigned int hw_val = val >> 1;
+ unsigned int readback;
+ int ret;
+
+ ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
+ SYSMON_CONFIG_SUPPLY_OSR,
+ FIELD_PREP(SYSMON_CONFIG_SUPPLY_OSR, hw_val));
+ if (ret)
+ return ret;
+
+ /* Readback fence -- see sysmon_osr_write_temp for details */
+ regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
+
+ return sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
+ SYSMON_SUPPLY_EN_AVG_COUNT,
+ hw_val ? ~0 : 0);
+}
+
+static int sysmon_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int i;
+ int ret;
+
+ if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+ return -EINVAL;
+
+ for (i = 0; i < ARRAY_SIZE(sysmon_oversampling_avail); i++) {
+ if (val == sysmon_oversampling_avail[i])
+ break;
+ }
+ if (i == ARRAY_SIZE(sysmon_oversampling_avail))
+ return -EINVAL;
+
+ guard(mutex)(&sysmon->lock);
+
+ if (chan->type == IIO_TEMP) {
+ ret = sysmon_osr_write_temp(sysmon, val);
+ if (ret)
+ return ret;
+ sysmon->temp_oversampling = val;
+ } else {
+ ret = sysmon_osr_write_supply(sysmon, val);
+ if (ret)
+ return ret;
+ sysmon->supply_oversampling = val;
+ }
+
+ return 0;
+}
+
+static int sysmon_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ long mask)
+{
+ if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+ return IIO_VAL_INT;
+
+ return -EINVAL;
+}
+
+static int sysmon_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type,
+ int *length, long mask)
+{
+ if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+ return -EINVAL;
+
+ *vals = sysmon_oversampling_avail;
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(sysmon_oversampling_avail);
+
+ return IIO_AVAIL_LIST;
+}
+
static int sysmon_read_label(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
char *label)
@@ -477,6 +619,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,
@@ -768,6 +913,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 ?
@@ -799,7 +948,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,
};
}
@@ -846,6 +999,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;
ret = devm_mutex_init(dev, &sysmon->lock);
if (ret)
diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
index 9fe2793757a..bb9a75bf71c 100644
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -23,11 +23,13 @@ struct regmap;
#define SYSMON_IMR 0x0048
#define SYSMON_IER 0x004C
#define SYSMON_IDR 0x0050
+#define SYSMON_CONFIG 0x0100
#define SYSMON_TEMP_MAX 0x1030
#define SYSMON_TEMP_MIN 0x1034
#define SYSMON_SUPPLY_BASE 0x1040
#define SYSMON_ALARM_FLAG 0x1018
#define SYSMON_ALARM_REG 0x1940
+#define SYSMON_SUPPLY_EN_AVG_BASE 0x1958
#define SYSMON_TEMP_TH_LOW 0x1970
#define SYSMON_TEMP_TH_UP 0x1974
#define SYSMON_SUPPLY_TH_LOW 0x1980
@@ -37,6 +39,7 @@ struct regmap;
#define SYSMON_TEMP_MAX_MAX 0x1F90
#define SYSMON_STATUS_RESET 0x1F94
#define SYSMON_TEMP_SAT_BASE 0x1FAC
+#define SYSMON_TEMP_EN_AVG_BASE 0x24B4
#define SYSMON_MAX_REG 0x24C0
/* NPI unlock value written to SYSMON_NPI_LOCK */
@@ -53,6 +56,16 @@ struct regmap;
/* ISR/IMR temperature alarm mask (bit 9) */
#define SYSMON_TEMP_INTR_MASK BIT(9)
+/* SYSMON_CONFIG: supply oversampling ratio */
+#define SYSMON_CONFIG_SUPPLY_OSR GENMASK(17, 14)
+
+/* SYSMON_CONFIG: temperature satellite oversampling ratio */
+#define SYSMON_CONFIG_TEMP_SAT_OSR GENMASK(27, 24)
+
+/* Per-channel averaging enable register counts */
+#define SYSMON_SUPPLY_EN_AVG_COUNT 5
+#define SYSMON_TEMP_EN_AVG_COUNT 2
+
/* Supply voltage conversion register fields */
#define SYSMON_MANTISSA_MASK GENMASK(15, 0)
#define SYSMON_FMT_MASK BIT(16)
@@ -77,6 +90,8 @@ struct regmap;
* @temp_mask: temperature interrupt configuration mask
* @temp_hysteresis: cached DEVICE_TEMP hysteresis in millicelsius
* @sysmon_unmask_work: re-enables events after alarm condition clears
+ * @temp_oversampling: current temp oversampling ratio
+ * @supply_oversampling: current supply oversampling ratio
*/
struct sysmon {
struct regmap *regmap;
@@ -96,6 +111,8 @@ struct sysmon {
unsigned int temp_mask;
int temp_hysteresis;
struct delayed_work sysmon_unmask_work;
+ unsigned int temp_oversampling;
+ unsigned int supply_oversampling;
};
int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap);
--
2.48.1
^ permalink raw reply related
* [PATCH v11 4/5] iio: adc: versal-sysmon: add threshold event support
From: Salih Erim @ 2026-06-23 1:40 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim, Andy Shevchenko
In-Reply-To: <20260623014036.3865402-1-salih.erim@amd.com>
Add threshold event support for temperature and supply voltage
channels.
Temperature events:
- Rising threshold with configurable value on the device
temperature channel (current max across all satellites)
- Per-channel hysteresis as a millicelsius value
- Event direction is IIO_EV_DIR_RISING (hysteresis mode)
Supply voltage events:
- Rising/falling threshold per supply channel
- Per-channel alarm enable via alarm configuration registers
The hardware supports both window and hysteresis alarm modes for
temperature. This driver uses hysteresis mode, where the upper
threshold triggers the alarm and the lower threshold clears it
(re-arm point). The hardware has a single ISR bit per temperature
channel with no indication of which threshold was crossed, so
hysteresis mode is the natural fit. The lower threshold register
is computed internally as (upper - hysteresis).
Hysteresis is stored in the driver as a millicelsius value,
initialized from the hardware registers at probe. Writing the
rising threshold or hysteresis recomputes the lower register.
ALARM_CONFIG is hard-coded to hysteresis mode during init.
The hardware also provides a separate over-temperature (OT)
threshold, but it is not exposed through IIO as it serves as a
hardware safety mechanism for platform shutdown. OT will be
exposed through the thermal framework in a follow-up series.
The interrupt handler masks active threshold interrupts (which are
level-sensitive) and schedules a delayed worker to poll for condition
clear before unmasking. When no hardware IRQ is available, event
specs are not attached and interrupt init is skipped, since the
I2C regmap backend cannot be called from atomic context.
When disabling a supply channel alarm, the group interrupt remains
active if any other channel in the same alarm group still has an
alarm enabled.
A devm cleanup action masks all interrupts on driver unbind to
prevent unhandled interrupt storms after the IRQ handler is freed.
Signed-off-by: Salih Erim <salih.erim@amd.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
Changes in v11:
- Add bounds check for temperature threshold writes; return
-EINVAL if out of Q8.7 range (Jonathan)
- Add bounds check for supply voltage threshold writes to
prevent integer overflow (Jonathan)
- Clamp computed lower threshold to Q8.7 range in
sysmon_update_temp_lower (found during audit)
- Add comment explaining that threshold register upper bits
(FMT/MODE) are ignored on write (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 | 613 ++++++++++++++++++++++++++-
drivers/iio/adc/versal-sysmon.h | 36 ++
2 files changed, 645 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
index 03a745d3fb4..8f2c502d9cb 100644
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -12,6 +12,9 @@
#include <linux/cleanup.h>
#include <linux/device.h>
#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/limits.h>
+#include <linux/minmax.h>
#include <linux/module.h>
#include <linux/overflow.h>
#include <linux/property.h>
@@ -20,10 +23,18 @@
#include <linux/sysfs.h>
#include <linux/units.h>
+#include <linux/iio/events.h>
#include <linux/iio/iio.h>
#include "versal-sysmon.h"
+/* TEMP hysteresis mode bit in SYSMON_TEMP_EV_CFG */
+#define SYSMON_TEMP_HYST_MASK BIT(1)
+
+/* Compute alarm register offset from a channel address */
+#define SYSMON_ALARM_OFFSET(addr) \
+ (SYSMON_ALARM_REG + ((addr) / SYSMON_ALARM_BITS_PER_REG) * SYSMON_REG_STRIDE)
+
#define SYSMON_CHAN_TEMP(_chan, _address, _name) \
{ \
.type = IIO_TEMP, \
@@ -35,6 +46,45 @@
.datasheet_name = _name, \
}
+enum sysmon_alarm_bit {
+ SYSMON_BIT_ALARM0 = 0,
+ SYSMON_BIT_ALARM1 = 1,
+ SYSMON_BIT_ALARM2 = 2,
+ SYSMON_BIT_ALARM3 = 3,
+ SYSMON_BIT_ALARM4 = 4,
+ SYSMON_BIT_TEMP = 9,
+};
+
+/* Temperature event specification: rising threshold + hysteresis only */
+static const struct iio_event_spec sysmon_temp_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_HYSTERESIS),
+ },
+};
+
+/* Supply event specifications */
+static const struct iio_event_spec sysmon_supply_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
/*
* Static temperature channels (always present).
*
@@ -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;
+
+ if (format)
+ tmp = clamp(tmp, S16_MIN, S16_MAX);
+ else
+ tmp = clamp(tmp, 0, U16_MAX);
+
+ *raw_data = (u16)tmp;
+}
+
+static int sysmon_supply_thresh_offset(unsigned long address, enum iio_event_direction dir)
+{
+ if (dir == IIO_EV_DIR_RISING)
+ return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_UP;
+ if (dir == IIO_EV_DIR_FALLING)
+ return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_LOW;
+
+ return -EINVAL;
+}
+
static int sysmon_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -115,6 +202,269 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
}
}
+static u32 sysmon_get_event_mask(const struct iio_chan_spec *chan)
+{
+ if (chan->type == IIO_TEMP)
+ return BIT(SYSMON_BIT_TEMP);
+
+ return BIT(chan->address / SYSMON_ALARM_BITS_PER_REG);
+}
+
+static int sysmon_read_alarm_config(struct sysmon *sysmon,
+ unsigned long address)
+{
+ u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
+ u32 offset = SYSMON_ALARM_OFFSET(address);
+
+ return regmap_test_bits(sysmon->regmap, offset, BIT(shift));
+}
+
+static int sysmon_write_alarm_config(struct sysmon *sysmon,
+ unsigned long address, bool enable)
+{
+ u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
+ u32 offset = SYSMON_ALARM_OFFSET(address);
+
+ return regmap_assign_bits(sysmon->regmap, offset, BIT(shift), enable);
+}
+
+static int sysmon_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ u32 mask = sysmon_get_event_mask(chan);
+ unsigned int imr;
+ int config_value;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+ if (ret)
+ return ret;
+
+ /* IMR bits are 1=masked, invert to get 1=enabled */
+ imr = ~imr;
+
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ config_value = sysmon_read_alarm_config(sysmon, chan->address);
+ if (config_value < 0)
+ return config_value;
+ return config_value && (imr & mask);
+
+ case IIO_TEMP:
+ /*
+ * Return the administrative state, not the hardware IMR.
+ * The IRQ handler temporarily masks the interrupt during
+ * the polling window; reading IMR would show it as disabled.
+ * temp_mask bit is set when administratively disabled.
+ */
+ return !(sysmon->temp_mask & mask);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sysmon_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ bool state)
+{
+ u32 offset = SYSMON_ALARM_OFFSET(chan->address);
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ u32 mask = sysmon_get_event_mask(chan);
+ unsigned int alarm_config;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ ret = sysmon_write_alarm_config(sysmon, chan->address, state);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, offset, &alarm_config);
+ if (ret)
+ return ret;
+
+ if (alarm_config)
+ return regmap_write(sysmon->regmap, SYSMON_IER, mask);
+
+ return regmap_write(sysmon->regmap, SYSMON_IDR, mask);
+
+ case IIO_TEMP:
+ if (state) {
+ ret = regmap_write(sysmon->regmap, SYSMON_IER, mask);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->temp_mask &= ~mask;
+ } else {
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, mask);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->temp_mask |= mask;
+ }
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+/*
+ * Recompute the lower threshold register from upper threshold and
+ * cached hysteresis. Called when either upper threshold or hysteresis
+ * is written.
+ */
+static int sysmon_update_temp_lower(struct sysmon *sysmon)
+{
+ unsigned int upper_reg;
+ int upper_mc, lower_mc;
+ u32 raw_val;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &upper_reg);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+ lower_mc = clamp(upper_mc - sysmon->temp_hysteresis, -256000, 255992);
+ sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
+
+ return regmap_write(sysmon->regmap, SYSMON_TEMP_TH_LOW, raw_val);
+}
+
+static int sysmon_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int reg_val;
+ int offset;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ switch (chan->type) {
+ case IIO_TEMP:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(reg_val, val);
+
+ return IIO_VAL_INT;
+
+ case IIO_EV_INFO_HYSTERESIS:
+ *val = sysmon->temp_hysteresis;
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+
+ case IIO_VOLTAGE:
+ offset = sysmon_supply_thresh_offset(chan->address, dir);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_rawtoprocessed(reg_val, val);
+
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sysmon_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int reg_val;
+ u32 raw_val;
+ int offset;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ switch (chan->type) {
+ case IIO_TEMP:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ /* Q8.7 signed range: -256000 to +255992 mC */
+ if (val < -256000 || val > 255992)
+ return -EINVAL;
+
+ sysmon_millicelsius_to_q8p7(&raw_val, val);
+
+ ret = regmap_write(sysmon->regmap, SYSMON_TEMP_TH_UP, raw_val);
+ if (ret)
+ return ret;
+
+ /* Recompute lower = upper - hysteresis */
+ return sysmon_update_temp_lower(sysmon);
+
+ case IIO_EV_INFO_HYSTERESIS:
+ if (val < 0)
+ return -EINVAL;
+
+ sysmon->temp_hysteresis = val;
+
+ return sysmon_update_temp_lower(sysmon);
+
+ default:
+ return -EINVAL;
+ }
+
+ case IIO_VOLTAGE:
+ offset = sysmon_supply_thresh_offset(chan->address, dir);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ /* Clamp to prevent overflow in processedtoraw conversion */
+ if (val < -32768 || val > 32767)
+ return -EINVAL;
+
+ sysmon_supply_processedtoraw(val, reg_val, &raw_val);
+
+ /*
+ * The hardware threshold register returns FMT and MODE
+ * bits in the upper 16 bits on read, but only the lower
+ * 16-bit mantissa is used on write.
+ */
+ return regmap_write(sysmon->regmap, offset, raw_val);
+
+ default:
+ return -EINVAL;
+ }
+}
+
static int sysmon_read_label(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
char *label)
@@ -128,20 +478,242 @@ static int sysmon_read_label(struct iio_dev *indio_dev,
static const struct iio_info sysmon_iio_info = {
.read_raw = sysmon_read_raw,
.read_label = sysmon_read_label,
+ .read_event_config = sysmon_read_event_config,
+ .write_event_config = sysmon_write_event_config,
+ .read_event_value = sysmon_read_event_value,
+ .write_event_value = sysmon_write_event_value,
};
+static void sysmon_push_event(struct iio_dev *indio_dev, u32 address)
+{
+ const struct iio_chan_spec *chan;
+ enum iio_event_direction dir;
+
+ for (unsigned int i = 0; i < indio_dev->num_channels; i++) {
+ if (indio_dev->channels[i].address != address)
+ continue;
+
+ chan = &indio_dev->channels[i];
+ /* Temp uses hysteresis mode (rising only), voltage uses window */
+ dir = (chan->type == IIO_TEMP) ? IIO_EV_DIR_RISING :
+ IIO_EV_DIR_EITHER;
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(chan->type,
+ chan->channel,
+ IIO_EV_TYPE_THRESH,
+ dir),
+ iio_get_time_ns(indio_dev));
+ }
+}
+
+static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
+{
+ u32 alarm_flag_offset = SYSMON_ALARM_FLAG + event * SYSMON_REG_STRIDE;
+ u32 alarm_reg_offset = SYSMON_ALARM_REG + event * SYSMON_REG_STRIDE;
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned long alarm_flag_reg;
+ unsigned int reg_val;
+ u32 address, bit;
+ int ret;
+
+ switch (event) {
+ case SYSMON_BIT_TEMP:
+ sysmon_push_event(indio_dev, SYSMON_TEMP_MAX);
+
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_TEMP));
+ if (ret)
+ return ret;
+
+ sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
+ return 0;
+
+ case SYSMON_BIT_ALARM0:
+ case SYSMON_BIT_ALARM1:
+ case SYSMON_BIT_ALARM2:
+ case SYSMON_BIT_ALARM3:
+ case SYSMON_BIT_ALARM4:
+ ret = regmap_read(sysmon->regmap, alarm_flag_offset, ®_val);
+ if (ret)
+ return ret;
+
+ alarm_flag_reg = reg_val;
+
+ for_each_set_bit(bit, &alarm_flag_reg, SYSMON_ALARM_BITS_PER_REG) {
+ address = bit + SYSMON_ALARM_BITS_PER_REG * event;
+ sysmon_push_event(indio_dev, address);
+ ret = regmap_clear_bits(sysmon->regmap, alarm_reg_offset, BIT(bit));
+ if (ret)
+ return ret;
+ }
+
+ return regmap_write(sysmon->regmap, alarm_flag_offset, alarm_flag_reg);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static void sysmon_handle_events(struct iio_dev *indio_dev,
+ unsigned long events)
+{
+ unsigned int bit;
+
+ for_each_set_bit(bit, &events, SYSMON_NO_OF_EVENTS)
+ sysmon_handle_event(indio_dev, bit);
+}
+
+static void sysmon_unmask_temp(struct sysmon *sysmon, unsigned int isr)
+{
+ unsigned int status;
+ u32 ier;
+
+ status = isr & SYSMON_TEMP_INTR_MASK;
+
+ ier = ~status & sysmon->masked_temp;
+ sysmon->masked_temp &= status;
+
+ /* Only unmask if not administratively disabled by userspace */
+ ier &= ~sysmon->temp_mask;
+
+ regmap_write(sysmon->regmap, SYSMON_IER, ier);
+}
+
+/*
+ * Versal threshold interrupts are level-sensitive. Active threshold
+ * interrupts are masked in the handler and polled via delayed work
+ * until the condition clears, then unmasked.
+ */
+static void sysmon_unmask_worker(struct work_struct *work)
+{
+ struct sysmon *sysmon =
+ container_of(work, struct sysmon, sysmon_unmask_work.work);
+ unsigned int isr;
+
+ /*
+ * If the ISR read fails, skip processing to avoid acting
+ * on undefined data.
+ */
+ scoped_guard(spinlock_irq, &sysmon->irq_lock) {
+ if (regmap_read(sysmon->regmap, SYSMON_ISR, &isr))
+ break;
+ regmap_write(sysmon->regmap, SYSMON_ISR, isr);
+ sysmon_unmask_temp(sysmon, isr);
+ }
+
+ if (sysmon->masked_temp)
+ schedule_delayed_work(&sysmon->sysmon_unmask_work,
+ msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
+ else
+ regmap_write(sysmon->regmap, SYSMON_STATUS_RESET, 1);
+}
+
+static irqreturn_t sysmon_iio_irq(int irq, void *data)
+{
+ struct iio_dev *indio_dev = data;
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int isr, imr;
+
+ guard(spinlock)(&sysmon->irq_lock);
+
+ if (regmap_read(sysmon->regmap, SYSMON_ISR, &isr) ||
+ regmap_read(sysmon->regmap, SYSMON_IMR, &imr))
+ return IRQ_NONE;
+
+ isr &= ~imr;
+ if (!isr)
+ return IRQ_NONE;
+
+ regmap_write(sysmon->regmap, SYSMON_ISR, isr);
+
+ sysmon_handle_events(indio_dev, isr);
+ schedule_delayed_work(&sysmon->sysmon_unmask_work,
+ msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
+
+ return IRQ_HANDLED;
+}
+
+static void sysmon_disable_interrupts(void *data)
+{
+ struct sysmon *sysmon = data;
+
+ regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->masked_temp = 0;
+
+ cancel_delayed_work_sync(&sysmon->sysmon_unmask_work);
+}
+
+static int sysmon_init_interrupt(struct sysmon *sysmon,
+ struct device *dev,
+ struct iio_dev *indio_dev,
+ int irq)
+{
+ unsigned int imr;
+ int ret;
+
+ /* Events not supported without IRQ (e.g. I2C path) */
+ if (!irq)
+ return 0;
+
+ INIT_DELAYED_WORK(&sysmon->sysmon_unmask_work, sysmon_unmask_worker);
+
+ ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+ if (ret)
+ return ret;
+ sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
+
+ ret = devm_request_irq(dev, irq, sysmon_iio_irq, 0, "sysmon-irq", indio_dev);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, sysmon_disable_interrupts, sysmon);
+}
+
+/*
+ * Initialize the cached hysteresis for a temperature channel from the
+ * current hardware threshold registers: hysteresis = upper - lower.
+ */
+static int sysmon_init_hysteresis(struct sysmon *sysmon, int *hysteresis)
+{
+ unsigned int upper_reg, lower_reg;
+ int upper_mc, lower_mc;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_UP, &upper_reg);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_TEMP_TH_LOW, &lower_reg);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+ sysmon_q8p7_to_millicelsius(lower_reg, &lower_mc);
+ *hysteresis = upper_mc - lower_mc;
+
+ return 0;
+}
+
/**
* sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
* @indio_dev: IIO device instance
* @dev: Parent device
+ * @irq: IRQ number (positive enables event channels, 0 disables)
*
* Reads voltage-channels and temperature-channels container nodes from
* firmware and builds the IIO channel array. Static temperature channels
- * are prepended, followed by supply and satellite channels from DT.
+ * and event channels are prepended, followed by supply and satellite
+ * channels from DT.
+ *
+ * Event channels and per-channel event specs are only added when the
+ * device has an IRQ. I2C devices have no interrupt line, and the I2C
+ * regmap cannot be called from atomic context, so events are not
+ * supported on that path.
*
* Return: 0 on success, negative errno on failure.
*/
-static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
+static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev, int irq)
{
unsigned int num_chan, num_static, num_supply, num_temp;
unsigned int idx, temp_chan_idx, volt_chan_idx;
@@ -164,8 +736,14 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
if (!sysmon_channels)
return -ENOMEM;
- /* Static temperature channels first */
memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
+
+ /* Attach event spec to channel 0 when IRQ is available */
+ if (irq > 0) {
+ sysmon_channels[0].event_spec = sysmon_temp_events;
+ sysmon_channels[0].num_event_specs = ARRAY_SIZE(sysmon_temp_events);
+ }
+
idx = num_static;
/* Supply channels from DT */
@@ -190,6 +768,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
.indexed = 1,
.address = reg,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ .event_spec = irq > 0 ?
+ sysmon_supply_events : NULL,
+ .num_event_specs = irq > 0 ?
+ ARRAY_SIZE(sysmon_supply_events) : 0,
.datasheet_name = label,
};
}
@@ -255,6 +837,7 @@ int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap)
{
struct iio_dev *indio_dev;
struct sysmon *sysmon;
+ int irq;
int ret;
indio_dev = devm_iio_device_alloc(dev, sizeof(*sysmon));
@@ -267,6 +850,7 @@ int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap)
ret = devm_mutex_init(dev, &sysmon->lock);
if (ret)
return ret;
+ spin_lock_init(&sysmon->irq_lock);
/* Disable all interrupts and clear pending status */
ret = regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
@@ -276,13 +860,34 @@ int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap)
if (ret)
return ret;
+ irq = fwnode_irq_get(dev_fwnode(dev), 0);
+ if (irq == -EPROBE_DEFER)
+ return dev_err_probe(dev, irq, "failed to get IRQ\n");
+
indio_dev->name = "versal-sysmon";
indio_dev->modes = INDIO_DIRECT_MODE;
- ret = sysmon_parse_fw(indio_dev, dev);
+ ret = sysmon_parse_fw(indio_dev, dev, irq);
if (ret)
return ret;
+ if (irq > 0) {
+ /* Set hysteresis mode for temperature threshold */
+ ret = regmap_set_bits(sysmon->regmap, SYSMON_TEMP_EV_CFG,
+ SYSMON_TEMP_HYST_MASK);
+ if (ret)
+ return ret;
+
+ /* Initialize cached hysteresis from hardware registers */
+ ret = sysmon_init_hysteresis(sysmon, &sysmon->temp_hysteresis);
+ if (ret)
+ return ret;
+
+ ret = sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
+ if (ret)
+ return ret;
+ }
+
return devm_iio_device_register(dev, indio_dev);
}
EXPORT_SYMBOL_NS_GPL(devm_versal_sysmon_core_probe, "VERSAL_SYSMON");
diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
index e27a5357575..9fe2793757a 100644
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -11,6 +11,8 @@
#include <linux/bits.h>
#include <linux/mutex.h>
+#include <linux/spinlock_types.h>
+#include <linux/workqueue.h>
struct device;
struct regmap;
@@ -18,12 +20,22 @@ struct regmap;
/* Register offsets (sorted by address) */
#define SYSMON_NPI_LOCK 0x000C
#define SYSMON_ISR 0x0044
+#define SYSMON_IMR 0x0048
+#define SYSMON_IER 0x004C
#define SYSMON_IDR 0x0050
#define SYSMON_TEMP_MAX 0x1030
#define SYSMON_TEMP_MIN 0x1034
#define SYSMON_SUPPLY_BASE 0x1040
+#define SYSMON_ALARM_FLAG 0x1018
+#define SYSMON_ALARM_REG 0x1940
+#define SYSMON_TEMP_TH_LOW 0x1970
+#define SYSMON_TEMP_TH_UP 0x1974
+#define SYSMON_SUPPLY_TH_LOW 0x1980
+#define SYSMON_SUPPLY_TH_UP 0x1C80
+#define SYSMON_TEMP_EV_CFG 0x1F84
#define SYSMON_TEMP_MIN_MIN 0x1F8C
#define SYSMON_TEMP_MAX_MAX 0x1F90
+#define SYSMON_STATUS_RESET 0x1F94
#define SYSMON_TEMP_SAT_BASE 0x1FAC
#define SYSMON_MAX_REG 0x24C0
@@ -35,8 +47,12 @@ struct regmap;
#define SYSMON_SUPPLY_IDX_MAX 159
#define SYSMON_TEMP_SAT_MAX 64
+#define SYSMON_NO_OF_EVENTS 32
#define SYSMON_INTR_ALL_MASK GENMASK(31, 0)
+/* ISR/IMR temperature alarm mask (bit 9) */
+#define SYSMON_TEMP_INTR_MASK BIT(9)
+
/* Supply voltage conversion register fields */
#define SYSMON_MANTISSA_MASK GENMASK(15, 0)
#define SYSMON_FMT_MASK BIT(16)
@@ -46,11 +62,21 @@ struct regmap;
#define SYSMON_FRACTIONAL_SHIFT 7U
#define SYSMON_SUPPLY_MANTISSA_BITS 16
+/* Bits per alarm register */
+#define SYSMON_ALARM_BITS_PER_REG 32
+
+#define SYSMON_UNMASK_WORK_DELAY_MS 500
+
/**
* struct sysmon - Driver data for Versal SysMon
* @regmap: register map for hardware access
* @lock: protects read-modify-write sequences on threshold registers
* and cached state that spans multiple regmap calls
+ * @irq_lock: protects interrupt mask register updates (MMIO path only)
+ * @masked_temp: currently masked temperature alarm bits
+ * @temp_mask: temperature interrupt configuration mask
+ * @temp_hysteresis: cached DEVICE_TEMP hysteresis in millicelsius
+ * @sysmon_unmask_work: re-enables events after alarm condition clears
*/
struct sysmon {
struct regmap *regmap;
@@ -60,6 +86,16 @@ struct sysmon {
* that spans multiple regmap calls.
*/
struct mutex lock;
+ /*
+ * Protects interrupt mask register updates. Only used on the
+ * MMIO path (fast_io regmap); I2C has no IRQ and never reaches
+ * the event code that takes this lock.
+ */
+ spinlock_t irq_lock;
+ unsigned int masked_temp;
+ unsigned int temp_mask;
+ int temp_hysteresis;
+ struct delayed_work sysmon_unmask_work;
};
int devm_versal_sysmon_core_probe(struct device *dev, struct regmap *regmap);
--
2.48.1
^ permalink raw reply related
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