* [PATCH v3] iio: magnetometer: bmc150: use FIELD_PREP and FIELD_GET helpers
From: Hungyu Lin @ 2026-06-09 2:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: David Lechner, Nuno Sá, Andy Shevchenko, linux-iio,
linux-kernel, Hungyu Lin
Replace open-coded bitfield operations with FIELD_PREP() and
FIELD_GET() helpers where appropriate.
Also simplify bmc150_magn_set_odr() by returning directly from
the matching table entry.
Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
---
v3:
- Add missing include <linux/bitfield.h>
v2:
- Use FIELD_PREP() and FIELD_GET() helpers as suggested by
Jonathan Cameron
- Simplify bmc150_magn_set_odr() by returning directly from
the matching table entry
drivers/iio/magnetometer/bmc150_magn.c | 34 ++++++++++++--------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index bf2551988008..fff7e88ef6a8 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -13,6 +13,7 @@
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/cleanup.h>
+#include <linux/bitfield.h>
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/pm.h>
@@ -245,14 +246,14 @@ static int bmc150_magn_set_power_mode(struct bmc150_magn_data *data,
return regmap_update_bits(data->regmap,
BMC150_MAGN_REG_OPMODE_ODR,
BMC150_MAGN_MASK_OPMODE,
- BMC150_MAGN_MODE_SLEEP <<
- BMC150_MAGN_SHIFT_OPMODE);
+ FIELD_PREP(BMC150_MAGN_MASK_OPMODE,
+ BMC150_MAGN_MODE_SLEEP));
case BMC150_MAGN_POWER_MODE_NORMAL:
return regmap_update_bits(data->regmap,
BMC150_MAGN_REG_OPMODE_ODR,
BMC150_MAGN_MASK_OPMODE,
- BMC150_MAGN_MODE_NORMAL <<
- BMC150_MAGN_SHIFT_OPMODE);
+ FIELD_PREP(BMC150_MAGN_MASK_OPMODE,
+ BMC150_MAGN_MODE_NORMAL));
}
return -EINVAL;
@@ -290,7 +291,8 @@ static int bmc150_magn_get_odr(struct bmc150_magn_data *data, int *val)
ret = regmap_read(data->regmap, BMC150_MAGN_REG_OPMODE_ODR, ®_val);
if (ret < 0)
return ret;
- odr_val = (reg_val & BMC150_MAGN_MASK_ODR) >> BMC150_MAGN_SHIFT_ODR;
+
+ odr_val = FIELD_GET(BMC150_MAGN_MASK_ODR, reg_val);
for (i = 0; i < ARRAY_SIZE(bmc150_magn_samp_freq_table); i++)
if (bmc150_magn_samp_freq_table[i].reg_val == odr_val) {
@@ -303,21 +305,17 @@ static int bmc150_magn_get_odr(struct bmc150_magn_data *data, int *val)
static int bmc150_magn_set_odr(struct bmc150_magn_data *data, int val)
{
- int ret;
u8 i;
for (i = 0; i < ARRAY_SIZE(bmc150_magn_samp_freq_table); i++) {
- if (bmc150_magn_samp_freq_table[i].freq == val) {
- ret = regmap_update_bits(data->regmap,
- BMC150_MAGN_REG_OPMODE_ODR,
- BMC150_MAGN_MASK_ODR,
- bmc150_magn_samp_freq_table[i].
- reg_val <<
- BMC150_MAGN_SHIFT_ODR);
- if (ret < 0)
- return ret;
- return 0;
- }
+ if (bmc150_magn_samp_freq_table[i].freq != val)
+ continue;
+
+ return regmap_update_bits(data->regmap,
+ BMC150_MAGN_REG_OPMODE_ODR,
+ BMC150_MAGN_MASK_ODR,
+ FIELD_PREP(BMC150_MAGN_MASK_ODR,
+ bmc150_magn_samp_freq_table[i].reg_val));
}
return -EINVAL;
@@ -800,7 +798,7 @@ static int bmc150_magn_data_rdy_trigger_set_state(struct iio_trigger *trig,
ret = regmap_update_bits(data->regmap, BMC150_MAGN_REG_INT_DRDY,
BMC150_MAGN_MASK_DRDY_EN,
- state << BMC150_MAGN_SHIFT_DRDY_EN);
+ FIELD_PREP(BMC150_MAGN_MASK_DRDY_EN, state));
if (ret < 0)
return ret;
--
2.34.1
^ permalink raw reply related
* [PATCH v2] iio: magnetometer: bmc150: use FIELD_PREP and FIELD_GET helpers
From: Hungyu Lin @ 2026-06-09 1:21 UTC (permalink / raw)
To: Jonathan Cameron
Cc: David Lechner, Nuno Sá, Andy Shevchenko, linux-iio,
linux-kernel, Hungyu Lin
Replace open-coded bitfield operations with FIELD_PREP() and
FIELD_GET() helpers where appropriate.
Also simplify bmc150_magn_set_odr() by returning directly from
the matching table entry.
Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
---
v2:
- Use FIELD_PREP() and FIELD_GET() helpers as suggested by
Jonathan Cameron
- Simplify bmc150_magn_set_odr() by returning directly from
the matching table entry
drivers/iio/magnetometer/bmc150_magn.c | 33 ++++++++++++--------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index bf2551988008..716b6a088c33 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -245,14 +245,14 @@ static int bmc150_magn_set_power_mode(struct bmc150_magn_data *data,
return regmap_update_bits(data->regmap,
BMC150_MAGN_REG_OPMODE_ODR,
BMC150_MAGN_MASK_OPMODE,
- BMC150_MAGN_MODE_SLEEP <<
- BMC150_MAGN_SHIFT_OPMODE);
+ FIELD_PREP(BMC150_MAGN_MASK_OPMODE,
+ BMC150_MAGN_MODE_SLEEP));
case BMC150_MAGN_POWER_MODE_NORMAL:
return regmap_update_bits(data->regmap,
BMC150_MAGN_REG_OPMODE_ODR,
BMC150_MAGN_MASK_OPMODE,
- BMC150_MAGN_MODE_NORMAL <<
- BMC150_MAGN_SHIFT_OPMODE);
+ FIELD_PREP(BMC150_MAGN_MASK_OPMODE,
+ BMC150_MAGN_MODE_NORMAL));
}
return -EINVAL;
@@ -290,7 +290,8 @@ static int bmc150_magn_get_odr(struct bmc150_magn_data *data, int *val)
ret = regmap_read(data->regmap, BMC150_MAGN_REG_OPMODE_ODR, ®_val);
if (ret < 0)
return ret;
- odr_val = (reg_val & BMC150_MAGN_MASK_ODR) >> BMC150_MAGN_SHIFT_ODR;
+
+ odr_val = FIELD_GET(BMC150_MAGN_MASK_ODR, reg_val);
for (i = 0; i < ARRAY_SIZE(bmc150_magn_samp_freq_table); i++)
if (bmc150_magn_samp_freq_table[i].reg_val == odr_val) {
@@ -303,21 +304,17 @@ static int bmc150_magn_get_odr(struct bmc150_magn_data *data, int *val)
static int bmc150_magn_set_odr(struct bmc150_magn_data *data, int val)
{
- int ret;
u8 i;
for (i = 0; i < ARRAY_SIZE(bmc150_magn_samp_freq_table); i++) {
- if (bmc150_magn_samp_freq_table[i].freq == val) {
- ret = regmap_update_bits(data->regmap,
- BMC150_MAGN_REG_OPMODE_ODR,
- BMC150_MAGN_MASK_ODR,
- bmc150_magn_samp_freq_table[i].
- reg_val <<
- BMC150_MAGN_SHIFT_ODR);
- if (ret < 0)
- return ret;
- return 0;
- }
+ if (bmc150_magn_samp_freq_table[i].freq != val)
+ continue;
+
+ return regmap_update_bits(data->regmap,
+ BMC150_MAGN_REG_OPMODE_ODR,
+ BMC150_MAGN_MASK_ODR,
+ FIELD_PREP(BMC150_MAGN_MASK_ODR,
+ bmc150_magn_samp_freq_table[i].reg_val));
}
return -EINVAL;
@@ -800,7 +797,7 @@ static int bmc150_magn_data_rdy_trigger_set_state(struct iio_trigger *trig,
ret = regmap_update_bits(data->regmap, BMC150_MAGN_REG_INT_DRDY,
BMC150_MAGN_MASK_DRDY_EN,
- state << BMC150_MAGN_SHIFT_DRDY_EN);
+ FIELD_PREP(BMC150_MAGN_MASK_DRDY_EN, state));
if (ret < 0)
return ret;
--
2.34.1
^ permalink raw reply related
* [PATCH v2] iio: pressure: bm1390: replace short msleeps with fsleep
From: Hungyu Lin @ 2026-06-09 0:57 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
linux-iio, linux-kernel, Hungyu Lin
Replace msleep(1) with fsleep(1000) for the driver's short
delays.
The BM1390 datasheet specifies a 1 ms reset cancel wait time
(tSC1) during the power-on sequence. Use fsleep() for these
short delays because it automatically selects the most
appropriate sleep mechanism and provides standardized timing
slack.
Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
---
v2:
- Replace usleep_range(1000, 2000) with fsleep(1000)
as suggested by Jonathan Cameron
drivers/iio/pressure/rohm-bm1390.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
index 08146ca0f91d..71b08333b746 100644
--- a/drivers/iio/pressure/rohm-bm1390.c
+++ b/drivers/iio/pressure/rohm-bm1390.c
@@ -486,21 +486,21 @@ static int bm1390_chip_init(struct bm1390_data *data)
if (ret)
return ret;
- msleep(1);
+ fsleep(1000);
ret = regmap_write_bits(data->regmap, BM1390_REG_RESET,
BM1390_MASK_RESET, BM1390_RESET);
if (ret)
return ret;
- msleep(1);
+ fsleep(1000);
ret = regmap_write_bits(data->regmap, BM1390_REG_RESET,
BM1390_MASK_RESET, BM1390_RESET_RELEASE);
if (ret)
return ret;
- msleep(1);
+ fsleep(1000);
ret = regmap_reinit_cache(data->regmap, &bm1390_regmap);
if (ret) {
@@ -575,7 +575,7 @@ static int bm1390_fifo_disable(struct iio_dev *idev)
struct bm1390_data *data = iio_priv(idev);
int ret;
- msleep(1);
+ fsleep(1000);
guard(mutex)(&data->mutex);
ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH RFC v4 0/6] iio: add Open Sensor Fusion IIO driver
From: Kim Jinseob @ 2026-06-08 23:27 UTC (permalink / raw)
To: Conor Dooley
Cc: Jonathan Cameron, linux-iio, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Corbet, Shuah Khan, devicetree, linux-kernel, linux-doc
In-Reply-To: <20260608-catnap-thinness-e25c9b8983c3@spud>
> Other than the fact that new revisions must not be sent as a diff on top
> of a prior revision, please stop sending new versions without actually
> replying to my v1 comments.
You are right, sorry for the noise.
I made a process mistake here. I prepared v4 on top of the previously sent
series instead of preparing it as a full standalone replacement series from
the proper base. I will not ask you to review v4 in that form, and I will
prepare the next revision as a full series from a clean base.
I also should have answered your protocol versioning questions directly before
sending another revision.
> What does "v0" mean here? Is the data format not complete yet?
> Are versions of the protocol likely to be backwards compatible?
> Will the device identify what version of the protocol it implements?
The current OSF wire header starts with a fixed 4-byte magic, "OSF0", at a
fixed offset. The same header also carries explicit protocol_major and
protocol_minor fields at fixed offsets.
For the currently supported firmware and driver, protocol_major is 0. The "0"
in "OSF0" is intended to denote the current major wire-format revision, not
the Linux driver identity.
The binding is intended for devices implementing this discoverable OSF header
layout. The driver currently supports protocol major version 0. Minor version
changes are intended to be backward compatible. Incompatible wire-format
changes require a new protocol_major value.
If a future major revision cannot be discovered using the same fixed header
layout, or is not compatible with this binding, it should use a new compatible
string.
I will spell this out in the binding commit message and documentation in the
next revision.
Jinseob
^ permalink raw reply
* Re: [PATCH 0/7] HID: iio: basic clean up for usage_id
From: Maxwell Doose @ 2026-06-08 22:03 UTC (permalink / raw)
To: Sanjay Chitroda
Cc: Jiri Kosina, Jonathan Cameron, Srinivas Pandruvada, David Lechner,
Nuno Sá, Andy Shevchenko, linux-input, linux-iio,
linux-kernel
In-Reply-To: <20260606-6-june-hid-iio-correct-usage-id-v1-0-dd4a6820b674@gmail.com>
On Sat, 06 Jun 2026 17:47:41 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
> Hi all,
>
> This series updates all HID IIO drivers to use 'u32' instead of
> bare 'unsigned' for the usage_id parameter.
>
> This improves type clarity and ensures consistency with kernel
> coding style, as HID usage IDs are defined as 32-bit values.
>
> No functional changes are introduced.
>
> Testing:
> - Compiled with W=1 for each patch in the series
>
> ---
> Sanjay Chitroda (7):
> iio: gyro: hid-sensor-gyro-3d: use u32 instead of unsigned
> iio: accel: hid-sensor-accel-3d: use u32 instead of unsigned
> iio: light: hid-sensor-als: use u32 instead of unsigned
> iio: light: hid-sensor-prox: use u32 instead of unsigned
> iio: orientation: hid-sensor-incl-3d: use u32 instead of unsigned
> iio: orientation: hid-sensor-rotation: use u32 instead of unsigned
> iio: pressure: hid-sensor-press: use u32 instead of unsigned
>
> drivers/iio/accel/hid-sensor-accel-3d.c | 6 +++---
> drivers/iio/gyro/hid-sensor-gyro-3d.c | 6 +++---
> drivers/iio/light/hid-sensor-als.c | 6 +++---
> drivers/iio/light/hid-sensor-prox.c | 4 ++--
> drivers/iio/orientation/hid-sensor-incl-3d.c | 6 +++---
> drivers/iio/orientation/hid-sensor-rotation.c | 6 +++---
> drivers/iio/pressure/hid-sensor-press.c | 6 +++---
> 7 files changed, 20 insertions(+), 20 deletions(-)
> ---
> base-commit: ae696dfa47c30016cd429b9db5e70b259b8f509e
> change-id: 20260606-6-june-hid-iio-correct-usage-id-57ce92cb102b
>
> Best regards,
> --
> Sanjay Chitroda <sanjayembeddedse@gmail.com>
>
>
This series:
Reviewed-by: Maxwell Doose <m32285159@gmail.com>
--
best regards,
max
^ permalink raw reply
* Re: [PATCH 3/7] iio: light: hid-sensor-als: use u32 instead of unsigned
From: Maxwell Doose @ 2026-06-08 22:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Joshua Crofts, Sanjay Chitroda, Jiri Kosina, Srinivas Pandruvada,
David Lechner, Nuno Sá, Andy Shevchenko, linux-input,
linux-iio, linux-kernel
In-Reply-To: <20260608194047.23791938@jic23-huawei>
On Mon, 8 Jun 2026 19:40:47 +0100
Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 8 Jun 2026 08:37:38 +0200
> Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> > On Sun, 7 Jun 2026 at 22:54, Maxwell Doose <m32285159@gmail.com> wrote:
> > >
> > > On Sat, Jun 6, 2026 at 7:19 AM Sanjay Chitroda
> > > <sanjayembeddedse@gmail.com> wrote:
> > > >
> > > > From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> > > >
> > > > Prefer 'u32' instead of bare 'unsigned' variable to improve code
> > > > clarity and consistency with kernel style.
> > > >
> > > > Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> > > > ---
> > > > drivers/iio/light/hid-sensor-als.c | 6 +++---
> > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > >
> > > Do we *need* u32 though? Usually these types are for places where we
> > > require specific bit-widths for a reason (e.g., reading a hardware
> > > register) but I'm not sure that's the case here. I could be totally
> > > wrong, in that case please correct me but otherwise I unfortunately
> > > don't see much value in this. That's just my personal opinion though,
> > > Jonathan may think otherwise.
> >
> > Aside from the array of usage ids that are u32 defined here:
> > https://elixir.bootlin.com/linux/v7.1-rc6/source/drivers/iio/light/hid-sensor-als.c#L46
> >
> > there are additional structs used in the HID drivers that take u32 (I
> > had a similar
> > patch moving `unsigned` to `unsigned int`, but it was recommended to use the
> > types that the structs use, hence u32).
> >
> Here the reason for the change is even simpler. These are callbacks assigned to:
>
> struct hid_sensor_hub_callbacks {
> struct platform_device *pdev;
> int (*suspend)(struct hid_sensor_hub_device *hsdev, void *priv);
> int (*resume)(struct hid_sensor_hub_device *hsdev, void *priv);
> int (*capture_sample)(struct hid_sensor_hub_device *hsdev,
> u32 usage_id, size_t raw_len, char *raw_data,
> void *priv);
> int (*send_event)(struct hid_sensor_hub_device *hsdev, u32 usage_id,
> void *priv);
> };
>
> So given those signatures, these should always have been u32.
>
Ah then perhaps I was a bit harsh on Sanjay.
>
> I would like that clearly stated in the patch descriptions. Consistency
> is a bit too vague.
>
Agree though. But given this
Reviewed-by: Maxwell Doose <m32285159@gmail.com>
--
best regards,
max
^ permalink raw reply
* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
From: Maxwell Doose @ 2026-06-08 21:51 UTC (permalink / raw)
To: Nuno Sá
Cc: Andy Shevchenko, Dan Carpenter, Jonathan Cameron, Nuno Sa,
Olivier Moysan, David Lechner, Andy Shevchenko, linux-iio,
linux-kernel, kernel-janitors
In-Reply-To: <da949cdf51727f74d1f0f5b3e8248d5dc34e2219.camel@gmail.com>
On Mon, 08 Jun 2026 21:31:14 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Fri, 2026-06-05 at 11:28 +0300, Andy Shevchenko wrote:
> > On Fri, Jun 05, 2026 at 09:12:38AM +0300, Dan Carpenter wrote:
> > > On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote:
> > > > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
> > > > > > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
> > > > > > ^^^
> > > > > > Uninitialized variable.
> > > > >
> > > > > s/variable/data/.
> > > >
> > > > With what I asked in the previous reply and what you explained there
> > > > (thanks, btw!) I still think your patches are not fully correct. They
> > > > will require to atomically write all or nothing. If we want support
> > > > partial writes we need to go with that differently (reset ppos when
> > > > we got enough or more than enough data).
> > >
> > > Requiring writes to syfs and debugfs be atomic is pretty normal and
> > > works well in practice. These are very small writes.
> >
> > Perhaps. In any case your patch will break existing partial writes, right?
> > I'm still considering that resetting ppos is the right thing to do. Just
> > need to find where the best place is to do that.
>
> I think anyone doing partial writes on a debugfs interface like this one is very
> unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No
> userspace app should be relying on debugfs in order to work (though I know that
> actually happens).
>
> Anyways, this is one of those interesting edge cases and easy enough to get wrong. I
> guess we should either:
>
> 1. Improve simple_write_to_buffer() docs;
> 2. Or come up with a new simple_write_once_to_buffer() helper?
>
When you say "simple_write_once_to_buffer()" do you mean a wrapper
around simple_write_to_buffer() that also checks if *ppos is 0 and
returns -EINVAL if *ppos isn't 0? If so I can start writing that
function. Though I guess we probably want to ask Jonathan about it
first since obviously he'll have his own opinions.
--
best regards,
max
^ permalink raw reply
* Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
From: Chris Morgan @ 2026-06-08 20:52 UTC (permalink / raw)
To: Andy Shevchenko
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: <aiL0g8d0Y_JeAC5c@ashevche-desk.local>
On Fri, Jun 05, 2026 at 07:08:35PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 04, 2026 at 03:18:25PM -0500, Chris Morgan wrote:
>
> > Add the core component of a new inv_icm42607 driver. This includes
> > a few setup functions and the full register definition in the
> > header file.
>
> ...
>
> > +#ifndef INV_ICM42607_H_
> > +#define INV_ICM42607_H_
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
>
> I haven't found users for these two.
>
> + bits.h // BIT() / GENMASK()
I'll correct this.
>
> > +#include <linux/iio/iio.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
>
>
> > +#include <linux/regulator/consumer.h>
>
> No users for this one.
>
> + types.h // for bool
>
> ...
>
And this.
> > +enum inv_icm42607_sensor_mode {
> > + INV_ICM42607_SENSOR_MODE_OFF,
> > + INV_ICM42607_SENSOR_MODE_STANDBY,
> > + INV_ICM42607_SENSOR_MODE_LOW_POWER,
> > + INV_ICM42607_SENSOR_MODE_LOW_NOISE,
>
> Are those enums map 1:1 to HW bits or bitfields? If so, assign explicitly each
> of them.
I'll explicitly set every value in the enums to be clear.
>
> > + INV_ICM42607_SENSOR_MODE_NB
>
> Is this a terminator like NUMBER_OF ?
This is a terminator, yes. I will not set these explicitly and keep
commas off of them to keep them easily identifable as terminators.
>
> > +};
>
> ...
>
> > +/* ODR values */
> > +enum inv_icm42607_odr {
> > + INV_ICM42607_ODR_1600HZ = 5,
>
> See above. This one is problematic. No one should rely on Linux/C enums when
> it's about HW bits. All HW related stuff has to be explicit.
>
I will explicitly set the values here.
> > + INV_ICM42607_ODR_800HZ,
> > + INV_ICM42607_ODR_400HZ,
> > + INV_ICM42607_ODR_200HZ,
> > + INV_ICM42607_ODR_100HZ,
> > + INV_ICM42607_ODR_50HZ,
> > + INV_ICM42607_ODR_25HZ,
> > + INV_ICM42607_ODR_12_5HZ,
> > + INV_ICM42607_ODR_6_25HZ_LP,
> > + INV_ICM42607_ODR_3_125HZ_LP,
> > + INV_ICM42607_ODR_1_5625HZ_LP,
> > + INV_ICM42607_ODR_NB
> > +};
>
> ...
>
> > +struct inv_icm42607_sensor_conf {
> > + int mode;
> > + int fs;
> > + int odr;
> > + int filter;
>
> All of them are supposed to be signed? Why?
>
> > +};
>
A later commit will use -1 for invalid values.
> ...
>
> > +struct inv_icm42607_hw {
> > + uint8_t whoami;
>
> What's wrong with u8?
Nothing, I'll make the change.
>
> > + const char *name;
> > + const struct inv_icm42607_conf *conf;
> > +};
>
> ...
>
> > +#include <linux/delay.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
>
> IWYU, please.
>
> ...
>
This may sound like a dumb question, but what's the best way to invoke
iwyu for a kernel? None of the readmes or man pages seem to be getting
me anywhere...
> > +/**
> > + * inv_icm42607_setup() - check and setup chip
> > + * @st: driver internal state
> > + * @bus_setup: callback for setting up bus specific registers
> > + *
> > + * Returns 0 on success, a negative error code otherwise.
>
> If you do kernel-doc, validate it. Return section is missing here.
>
I'll just nuke it, most of the stuff is fairly self explanatory anyway.
> > + */
>
> ...
>
> > +{
> > + const struct device *dev = regmap_get_device(st->map);
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
> > + if (ret)
> > + return ret;
> > +
> > + if (val != st->hw->whoami)
> > + dev_warn(dev, "Unknown whoami %#02x expected %#02x (%s)\n",
> > + val, st->hw->whoami, st->hw->name);
>
> dev_warn_probe() ?
>
> > + ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
> > + INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
> > + if (ret)
> > + return ret;
>
> > + fsleep(INV_ICM42607_RESET_TIME_MS * 1000);
>
> USEC_PER_MSEC (needs time.h)
>
> > + ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
> > + val, val & INV_ICM42607_INT_STATUS_RESET_DONE,
> > + INV_ICM42607_RESET_TIME_MS * 100,
> > + INV_ICM42607_RESET_TIME_MS * 10000);
>
> These are weird, as in the first case it's actually 1/10th of _RESET_TIME_MS.
> Perhaps you need to reconsider what you use as that constant. Personally I
> prefer to see just plain values with the multipliers (to convert to µs).
>
I honestly don't know what a good wait period between polling should
be, so I guessed at 1/10th the timeout period. I will however change
the timeout to us.
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "reset error, reset done bit not set\n");
> > +
> > + /* Sync the regcache again after a reset. */
> > + regcache_mark_dirty(st->map);
> > + ret = regcache_sync(st->map);
> > + if (ret)
> > + return ret;
> > +
> > + ret = bus_setup(st);
>
> Hmm... This is bad name with potential of name collision in the future (in case
> driver bus code wants to have the same name for the function).
Thanks, I'll rename the function.
>
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_set_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
> > + INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG1,
> > + INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK,
> > + INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL);
> > + if (ret)
> > + return ret;
> > +
> > + return inv_icm42607_set_conf(st, st->hw->conf);
> > +}
>
> ...
>
> > +int inv_icm42607_core_probe(struct regmap *regmap,
> > + const struct inv_icm42607_hw *hw,
> > + inv_icm42607_bus_setup bus_setup)
> > +{
> > + struct device *dev = regmap_get_device(regmap);
> > + struct inv_icm42607_state *st;
> > + int ret;
> > +
> > + st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > + if (!st)
> > + return -ENOMEM;
> > +
> > + ret = devm_mutex_init(dev, &st->lock);
> > + if (ret)
> > + return ret;
> > +
> > + st->hw = hw;
> > + st->map = regmap;
> > +
> > + ret = iio_read_mount_matrix(dev, &st->orientation);
> > + if (ret)
>
> > + return dev_err_probe(dev, ret,
> > + "failed to retrieve mounting matrix %d\n", ret);
>
> Remove duplicate ret printing.
>
Understood.
> > +
> > + ret = devm_regulator_get_enable(dev, "vdd");
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Failed to get vdd regulator\n");
> > +
> > + st->vddio_supply = devm_regulator_get(dev, "vddio");
> > + if (IS_ERR(st->vddio_supply))
> > + return dev_err_probe(dev, PTR_ERR(st->vddio_supply),
> > + "Failed to get vddio regulator\n");
> > +
> > + ret = inv_icm42607_enable_vddio_reg(st);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(dev, inv_icm42607_disable_vddio_reg, st);
> > + if (ret)
> > + return ret;
>
> > + /* Setup chip registers (includes WHOAMI check, reset check, bus setup) */
> > + ret = inv_icm42607_setup(st, bus_setup);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> Just
>
> return inv_icm42607_setup(st, bus_setup);
>
> ? Or is it going to be extended in the next changes?
>
> > +}
It will get extended, so I'll keep this for now if that's okay.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thank you for your feedback. And please let me know about the iwyu
tool, that seems like it would be useful.
- Chris
^ permalink raw reply
* Re: [PATCH v16 08/14] iio: core: add decimal value formatting into 64-bit value
From: Nuno Sá @ 2026-06-08 20:32 UTC (permalink / raw)
To: rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc,
linux
Cc: Jonathan Cameron, David Lechner, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton,
Petr Mladek, Steven Rostedt, Andy Shevchenko, Rasmus Villemoes,
Sergey Senozhatsky, Shuah Khan
In-Reply-To: <20260604-adf41513-iio-driver-v16-8-1a7d09143bc2@analog.com>
On Thu, 2026-06-04 at 10:59 +0100, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
>
> Create new format types for iio values (IIO_VAL_DECIMAL64_*), which
> defines the representation of fixed decimal point values into a single
> 64-bit number. This new format increases the range of represented values,
> allowing for integer parts greater than 2^32, as bits are not "wasted"
> in the fractional part, which can be seen in IIO_VAL_INT_PLUS_MICRO and
> IIO_VAL_INT_PLUS_NANO. Helpers are created to compose and decompose 64-bit
> decimals into integer values used in IIO formatting interfaces, which
> creates consistency and avoid error-prone manual assignments when using
> wordpart macros. When doing the parsing, kstrtodec64() is used with the
> scale defined by the specific decimal format type.
>
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> ---
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> drivers/iio/industrialio-core.c | 49 ++++++++++++++++++++++++++++++++---------
> include/linux/iio/types.h | 20 +++++++++++++++++
> 2 files changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index ffe0dc49c4b9..93c2540d4cd2 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -19,6 +19,7 @@
> #include <linux/idr.h>
> #include <linux/kdev_t.h>
> #include <linux/kernel.h>
> +#include <linux/math64.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
> @@ -26,7 +27,6 @@
> #include <linux/sched.h>
> #include <linux/slab.h>
> #include <linux/wait.h>
> -#include <linux/wordpart.h>
>
> #include <linux/iio/buffer.h>
> #include <linux/iio/buffer_impl.h>
> @@ -656,6 +656,7 @@ static ssize_t __iio_format_value(char *buf, size_t offset,
> unsigned int type,
> int size, const int *vals)
> {
> int tmp0, tmp1;
> + int l = 0;
> s64 tmp2;
> bool scale_db = false;
>
> @@ -699,7 +700,6 @@ static ssize_t __iio_format_value(char *buf, size_t offset,
> unsigned int type,
> case IIO_VAL_INT_MULTIPLE:
> {
> int i;
> - int l = 0;
>
> for (i = 0; i < size; ++i)
> l += sysfs_emit_at(buf, offset + l, "%d ", vals[i]);
> @@ -708,8 +708,25 @@ static ssize_t __iio_format_value(char *buf, size_t offset,
> unsigned int type,
> case IIO_VAL_CHAR:
> return sysfs_emit_at(buf, offset, "%c", (char)vals[0]);
> case IIO_VAL_INT_64:
> - tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
> - return sysfs_emit_at(buf, offset, "%lld", tmp2);
> + return sysfs_emit_at(buf, offset, "%lld",
> + iio_val_s64_compose(vals[0], vals[1]));
> + case IIO_VAL_DECIMAL64_MILLI:
> + case IIO_VAL_DECIMAL64_MICRO:
> + case IIO_VAL_DECIMAL64_NANO:
> + case IIO_VAL_DECIMAL64_PICO:
> + {
> + int scale = type - IIO_VAL_DECIMAL64_BASE;
> + s64 frac;
> +
> + tmp2 = div64_s64_rem(iio_val_s64_compose(vals[0], vals[1]),
> + int_pow(10, scale), &frac);
> + if (tmp2 == 0 && frac < 0)
> + l += sysfs_emit_at(buf, offset, "-");
> +
> + l += sysfs_emit_at(buf, offset + l, "%lld.%0*lld", tmp2, scale,
> + abs(frac));
> + return l;
> + }
> default:
> return 0;
> }
> @@ -979,6 +996,7 @@ static ssize_t iio_write_channel_info(struct device *dev,
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> int ret, fract_mult = 100000;
> + int type, dec_scale = 0;
> int integer, fract = 0;
> long long integer64;
> bool is_char = false;
> @@ -989,9 +1007,11 @@ static ssize_t iio_write_channel_info(struct device *dev,
> if (!indio_dev->info->write_raw)
> return -EINVAL;
>
> - if (indio_dev->info->write_raw_get_fmt)
> - switch (indio_dev->info->write_raw_get_fmt(indio_dev,
> - this_attr->c, this_attr->address)) {
> + if (indio_dev->info->write_raw_get_fmt) {
> + type = indio_dev->info->write_raw_get_fmt(indio_dev,
> + this_attr->c,
> + this_attr->address);
> + switch (type) {
> case IIO_VAL_INT:
> fract_mult = 0;
> break;
> @@ -1007,12 +1027,19 @@ static ssize_t iio_write_channel_info(struct device *dev,
> case IIO_VAL_CHAR:
> is_char = true;
> break;
> + case IIO_VAL_DECIMAL64_MILLI:
> + case IIO_VAL_DECIMAL64_MICRO:
> + case IIO_VAL_DECIMAL64_NANO:
> + case IIO_VAL_DECIMAL64_PICO:
> + dec_scale = type - IIO_VAL_DECIMAL64_BASE;
> + fallthrough;
> case IIO_VAL_INT_64:
> is_64bit = true;
> break;
> default:
> return -EINVAL;
> }
> + }
>
> if (is_char) {
> char ch;
> @@ -1021,12 +1048,14 @@ static ssize_t iio_write_channel_info(struct device *dev,
> return -EINVAL;
> integer = ch;
> } else if (is_64bit) {
> - ret = kstrtoll(buf, 0, &integer64);
> + if (dec_scale)
> + ret = kstrtodec64(buf, dec_scale, &integer64);
> + else
> + ret = kstrtoll(buf, 0, &integer64);
> if (ret)
> return ret;
>
> - fract = upper_32_bits(integer64);
> - integer = lower_32_bits(integer64);
> + iio_val_s64_decompose(integer64, &integer, &fract);
> } else {
> ret = __iio_str_to_fixpoint(buf, fract_mult, &integer, &fract,
> scale_db);
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 4e3099defc1d..924ac9dc6893 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -7,6 +7,9 @@
> #ifndef _IIO_TYPES_H_
> #define _IIO_TYPES_H_
>
> +#include <linux/types.h>
> +#include <linux/wordpart.h>
> +
> #include <uapi/linux/iio/types.h>
>
> enum iio_event_info {
> @@ -34,6 +37,23 @@ enum iio_event_info {
> #define IIO_VAL_FRACTIONAL_LOG2 11
> #define IIO_VAL_CHAR 12
>
> +#define IIO_VAL_DECIMAL64_BASE 32
> +#define IIO_VAL_DECIMAL64_MILLI (IIO_VAL_DECIMAL64_BASE + 3)
> +#define IIO_VAL_DECIMAL64_MICRO (IIO_VAL_DECIMAL64_BASE + 6)
> +#define IIO_VAL_DECIMAL64_NANO (IIO_VAL_DECIMAL64_BASE + 9)
> +#define IIO_VAL_DECIMAL64_PICO (IIO_VAL_DECIMAL64_BASE + 12)
> +
> +static inline s64 iio_val_s64_compose(s32 val0, s32 val1)
> +{
> + return (s64)(((u64)val1 << 32) | (u32)val0);
> +}
> +
> +static inline void iio_val_s64_decompose(s64 dec64, s32 *val0, s32 *val1)
> +{
> + *val0 = lower_32_bits(dec64);
> + *val1 = upper_32_bits(dec64);
> +}
> +
> enum iio_available_type {
> IIO_AVAIL_LIST,
> IIO_AVAIL_RANGE,
^ permalink raw reply
* Re: [PATCH] iio: backend: fix uninitialized data in debugfs
From: Nuno Sá @ 2026-06-08 20:31 UTC (permalink / raw)
To: Andy Shevchenko, Dan Carpenter
Cc: Jonathan Cameron, Maxwell Doose, Nuno Sa, Olivier Moysan,
David Lechner, Andy Shevchenko, linux-iio, linux-kernel,
kernel-janitors
In-Reply-To: <aiKIoqR0IQBZ-1ZD@ashevche-desk.local>
On Fri, 2026-06-05 at 11:28 +0300, Andy Shevchenko wrote:
> On Fri, Jun 05, 2026 at 09:12:38AM +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote:
> > > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
> > > > > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
> > > > > ^^^
> > > > > Uninitialized variable.
> > > >
> > > > s/variable/data/.
> > >
> > > With what I asked in the previous reply and what you explained there
> > > (thanks, btw!) I still think your patches are not fully correct. They
> > > will require to atomically write all or nothing. If we want support
> > > partial writes we need to go with that differently (reset ppos when
> > > we got enough or more than enough data).
> >
> > Requiring writes to syfs and debugfs be atomic is pretty normal and
> > works well in practice. These are very small writes.
>
> Perhaps. In any case your patch will break existing partial writes, right?
> I'm still considering that resetting ppos is the right thing to do. Just
> need to find where the best place is to do that.
I think anyone doing partial writes on a debugfs interface like this one is very
unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No
userspace app should be relying on debugfs in order to work (though I know that
actually happens).
Anyways, this is one of those interesting edge cases and easy enough to get wrong. I
guess we should either:
1. Improve simple_write_to_buffer() docs;
2. Or come up with a new simple_write_once_to_buffer() helper?
- Nuno Sá
^ permalink raw reply
* Re: [PATCH v2] iio: imu: bmi323: remove unnecessary cast in watermark limit
From: Nuno Sá @ 2026-06-08 20:20 UTC (permalink / raw)
To: Hungyu Lin, jagathjog1996, jic23
Cc: dlechner, nuno.sa, andy, linux-iio, linux-kernel
In-Reply-To: <20260608121558.85429-1-dennylin0707@gmail.com>
On Mon, 2026-06-08 at 12:15 +0000, Hungyu Lin wrote:
> Remove the explicit u32 cast in the watermark limit calculation.
>
> The BMI323_FIFO_FULL_IN_FRAMES macro can be used directly with
> min() without triggering type issues.
>
> Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
> ---
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> Changes in v2:
> - Drop the unnecessary cast instead of using min_t().
> - Follow reviewer feedback from David Laight and Andy Shevchenko.
>
> drivers/iio/imu/bmi323/bmi323_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/bmi323/bmi323_core.c
> b/drivers/iio/imu/bmi323/bmi323_core.c
> index f3d499423399..72955a697a93 100644
> --- a/drivers/iio/imu/bmi323/bmi323_core.c
> +++ b/drivers/iio/imu/bmi323/bmi323_core.c
> @@ -1128,7 +1128,7 @@ static int bmi323_set_watermark(struct iio_dev *indio_dev,
> unsigned int val)
> {
> struct bmi323_data *data = iio_priv(indio_dev);
>
> - val = min(val, (u32)BMI323_FIFO_FULL_IN_FRAMES);
> + val = min(val, BMI323_FIFO_FULL_IN_FRAMES);
>
> guard(mutex)(&data->mutex);
> data->watermark = val;
^ permalink raw reply
* Re: [PATCH v2] iio: buffer: Ensure bounce buffer used for unaligned case is zeroed.
From: Nuno Sá @ 2026-06-08 20:00 UTC (permalink / raw)
To: Andy Shevchenko, Jonathan Cameron
Cc: linux-iio, Jinseob Kim, Joshua Crofts, Sanjay Chitroda,
David Lechner, Nuno Sá, Andy Shevchenko, sashiko-bot
In-Reply-To: <aiMg71I2O6a94g3o@ashevche-desk.local>
On Fri, 2026-06-05 at 22:18 +0300, Andy Shevchenko wrote:
> On Fri, Jun 05, 2026 at 02:50:00PM +0100, Jonathan Cameron wrote:
> > On Thu, 4 Jun 2026 12:10:42 +0300
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > On Thu, Jun 04, 2026 at 09:43:07AM +0100, Jonathan Cameron wrote:
>
> ...
>
> > > > indio_dev->scan_bytes, GFP_KERNEL);
> > > > if (!bb)
> > > > return -ENOMEM;
> > > > + memset(bb, 0, indio_dev->scan_bytes);
> > >
> > > May I suggest different approach, id est use __GFP_ZERO instead of hunting
> > > correct pointers?
> >
> > That's a weird beast when combined with a krealloc so I was a bit
> > nervous about readability (and less so whether it was correct).
> > It should be fine in that we will either get stale data or zeros
> > because we always use this path to allocate the buffer so if you
> > think it is obviously fine then I don't mind.
> >
> > The oddities are that if an object grows within a slab but doesn't need
> > a new one we are relying on those bits happening to be zero based on the
> > original allocation doing __GFP_ZERO as well (as it's the same call)
> >
> > I'm nervous though as that region off the end is sometimes used for
> > debug objects and I really don't understand that bit of slab well enough.
> > If it actually does this, then seems like we'd end up with a lot of
> > nasty corner cases so I assume it doesn't.
>
> Such a bug will be a serious issue in mm. I don't think it exists.
> But, of course, chances are not completely 0.
>
> Current users (not a comprehensive list)
>
> arch/arm64/kvm/nested.c:92
> drivers/firmware/efi/capsule-loader.c:61
> drivers/gpio/gpiolib.c:5198
> drivers/media/platform/amphion/vdec.c:329
> drivers/net/ethernet/intel/ice/ice_lib.c:3044
> drivers/net/ethernet/mellanox/mlx5/core/steering/hws/bwc.c:785
> drivers/net/wireless/ti/wl18xx/main.c:1582
> drivers/nvme/target/pci-epf.c:708
> drivers/spi/spi.c:1411
> drivers/usb/gadget/function/uvc_configfs.c:880
> fs/ceph/mdsmap.c:322
> fs/smb/server/ksmbd_work.c:123
> lib/tests/slub_kunit.c:273
> sound/soc/meson/meson-card-utils.c:49
>
> Also note the kernel-doc for krealloc_array(). It has some note WRT __GFP_ZERO
> and it means the flag must be supported. Also there is a note in mm/slub.c.
>
> > However, I couldn't convince myself enough not to just force a memset of the
> > whole thing.
>
> I personally would go with __GFP_ZERO.
If I'm not missing anything, this should also be proof that the flag should be fine:
https://elixir.bootlin.com/linux/v7.1-rc7/source/lib/tests/slub_kunit.c#L275
- Nuno Sá
^ permalink raw reply
* Re: [PULL] IIO: New device support, features and cleanup for 7.2
From: Greg KH @ 2026-06-08 19:29 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
In-Reply-To: <20260607182215.3e6e849e@jic23-huawei>
On Sun, Jun 07, 2026 at 06:22:15PM +0100, Jonathan Cameron wrote:
> The following changes since commit e43ffb69e0438cddd72aaa30898b4dc446f664f8:
>
> Linux 7.1-rc6 (2026-05-31 15:14:24 -0700)
>
> are available in the Git repository at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git tags/iio-for-7.2a
Pulled and pushed out, thanks.
greg k-h
^ permalink raw reply
* Re: [GIT PULL] Counter updates for 7.2
From: Greg KH @ 2026-06-08 19:26 UTC (permalink / raw)
To: William Breathitt Gray; +Cc: linux-iio
In-Reply-To: <20260602024614.853294-1-wbg@kernel.org>
On Tue, Jun 02, 2026 at 11:46:02AM +0900, William Breathitt Gray wrote:
> The following changes since commit 254f49634ee16a731174d2ae34bc50bd5f45e731:
>
> Linux 7.1-rc1 (2026-04-26 14:19:00 -0700)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wbg/counter.git tags/counter-updates-for-7.2
Pulled and pushed out, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH 0/7] HID: iio: basic clean up for usage_id
From: Jonathan Cameron @ 2026-06-08 18:41 UTC (permalink / raw)
To: Sanjay Chitroda
Cc: Jiri Kosina, Srinivas Pandruvada, David Lechner, Nuno Sá,
Andy Shevchenko, linux-input, linux-iio, linux-kernel
In-Reply-To: <20260606-6-june-hid-iio-correct-usage-id-v1-0-dd4a6820b674@gmail.com>
On Sat, 06 Jun 2026 17:47:41 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
> Hi all,
>
> This series updates all HID IIO drivers to use 'u32' instead of
> bare 'unsigned' for the usage_id parameter.
>
> This improves type clarity and ensures consistency with kernel
> coding style, as HID usage IDs are defined as 32-bit values.
Other than adding some more detail to the consistency part
(make sure to mention the callback signatures), this looks fine to
me.
Jonathan
>
> No functional changes are introduced.
>
> Testing:
> - Compiled with W=1 for each patch in the series
>
> ---
> Sanjay Chitroda (7):
> iio: gyro: hid-sensor-gyro-3d: use u32 instead of unsigned
> iio: accel: hid-sensor-accel-3d: use u32 instead of unsigned
> iio: light: hid-sensor-als: use u32 instead of unsigned
> iio: light: hid-sensor-prox: use u32 instead of unsigned
> iio: orientation: hid-sensor-incl-3d: use u32 instead of unsigned
> iio: orientation: hid-sensor-rotation: use u32 instead of unsigned
> iio: pressure: hid-sensor-press: use u32 instead of unsigned
>
> drivers/iio/accel/hid-sensor-accel-3d.c | 6 +++---
> drivers/iio/gyro/hid-sensor-gyro-3d.c | 6 +++---
> drivers/iio/light/hid-sensor-als.c | 6 +++---
> drivers/iio/light/hid-sensor-prox.c | 4 ++--
> drivers/iio/orientation/hid-sensor-incl-3d.c | 6 +++---
> drivers/iio/orientation/hid-sensor-rotation.c | 6 +++---
> drivers/iio/pressure/hid-sensor-press.c | 6 +++---
> 7 files changed, 20 insertions(+), 20 deletions(-)
> ---
> base-commit: ae696dfa47c30016cd429b9db5e70b259b8f509e
> change-id: 20260606-6-june-hid-iio-correct-usage-id-57ce92cb102b
>
> Best regards,
> --
> Sanjay Chitroda <sanjayembeddedse@gmail.com>
>
^ permalink raw reply
* Re: [PATCH 3/7] iio: light: hid-sensor-als: use u32 instead of unsigned
From: Jonathan Cameron @ 2026-06-08 18:40 UTC (permalink / raw)
To: Joshua Crofts
Cc: Maxwell Doose, Sanjay Chitroda, Jiri Kosina, Srinivas Pandruvada,
David Lechner, Nuno Sá, Andy Shevchenko, linux-input,
linux-iio, linux-kernel
In-Reply-To: <CALoEA-yiDwjkKxUE3B=qiTdBhbvinfq-UN8h1H-HysXGm0prSg@mail.gmail.com>
On Mon, 8 Jun 2026 08:37:38 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> On Sun, 7 Jun 2026 at 22:54, Maxwell Doose <m32285159@gmail.com> wrote:
> >
> > On Sat, Jun 6, 2026 at 7:19 AM Sanjay Chitroda
> > <sanjayembeddedse@gmail.com> wrote:
> > >
> > > From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> > >
> > > Prefer 'u32' instead of bare 'unsigned' variable to improve code
> > > clarity and consistency with kernel style.
> > >
> > > Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> > > ---
> > > drivers/iio/light/hid-sensor-als.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> >
> > Do we *need* u32 though? Usually these types are for places where we
> > require specific bit-widths for a reason (e.g., reading a hardware
> > register) but I'm not sure that's the case here. I could be totally
> > wrong, in that case please correct me but otherwise I unfortunately
> > don't see much value in this. That's just my personal opinion though,
> > Jonathan may think otherwise.
>
> Aside from the array of usage ids that are u32 defined here:
> https://elixir.bootlin.com/linux/v7.1-rc6/source/drivers/iio/light/hid-sensor-als.c#L46
>
> there are additional structs used in the HID drivers that take u32 (I
> had a similar
> patch moving `unsigned` to `unsigned int`, but it was recommended to use the
> types that the structs use, hence u32).
>
Here the reason for the change is even simpler. These are callbacks assigned to:
struct hid_sensor_hub_callbacks {
struct platform_device *pdev;
int (*suspend)(struct hid_sensor_hub_device *hsdev, void *priv);
int (*resume)(struct hid_sensor_hub_device *hsdev, void *priv);
int (*capture_sample)(struct hid_sensor_hub_device *hsdev,
u32 usage_id, size_t raw_len, char *raw_data,
void *priv);
int (*send_event)(struct hid_sensor_hub_device *hsdev, u32 usage_id,
void *priv);
};
So given those signatures, these should always have been u32.
I would like that clearly stated in the patch descriptions. Consistency
is a bit too vague.
^ permalink raw reply
* [PATCH v5 5/5] iio: adc: versal-sysmon: add oversampling support
From: Salih Erim @ 2026-06-08 18:38 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
In-Reply-To: <20260608183801.1257051-1-salih.erim@amd.com>
Add support for reading and writing the oversampling ratio through
the IIO oversampling_ratio attribute. The hardware supports averaging
2, 4, 8, or 16 samples, plus a ratio of 1 (no averaging).
Temperature and supply channels share oversampling configuration at
the type level (all temperature channels share one ratio, all supply
channels share another), exposed through info_mask_shared_by_type.
The hardware encoding uses sample_count / 2 in a 4-bit field within
the CONFIG register. Per-channel averaging enable registers must also
be updated to activate or deactivate averaging.
Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in 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 | 147 ++++++++++++++++++++++++++-
drivers/iio/adc/versal-sysmon.h | 17 ++++
2 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
index 5fbd509089b..389a8887811 100644
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -26,6 +26,12 @@
#include "versal-sysmon.h"
+/*
+ * Oversampling ratio values exposed to userspace via IIO.
+ * Actual number of samples averaged: 1=none, 2=2x, 4=4x, 8=8x, 16=16x.
+ */
+static const int sysmon_oversampling_avail[] = { 1, 2, 4, 8, 16 };
+
/* OT and TEMP hysteresis mode bits in SYSMON_TEMP_EV_CFG */
#define SYSMON_OT_HYST_MASK BIT(0)
#define SYSMON_TEMP_HYST_MASK BIT(1)
@@ -194,6 +200,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) {
@@ -496,6 +508,127 @@ static int sysmon_write_event_value(struct iio_dev *indio_dev,
return -EINVAL;
}
+static int sysmon_set_avg_enable(struct sysmon *sysmon,
+ u32 base, u32 count, u32 val)
+{
+ 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(struct sysmon *sysmon, int channel_type, int val)
+{
+ /*
+ * HW register encoding is sample_count / 2:
+ * 0=none, 1=2x, 2=4x, 4=8x, 8=16x (not log2-based).
+ */
+ int hw_val = val >> 1;
+ unsigned int readback;
+ int ret;
+
+ switch (channel_type) {
+ case IIO_TEMP:
+ ret = regmap_update_bits(sysmon->regmap, SYSMON_CONFIG,
+ SYSMON_CONFIG_TEMP_SAT_OSR,
+ FIELD_PREP(SYSMON_CONFIG_TEMP_SAT_OSR,
+ hw_val));
+ if (ret)
+ return ret;
+
+ /*
+ * Readback fence: the SysMon CONFIG register resides in the
+ * PMC domain behind the NoC. A posted write may not reach the
+ * hardware before the next MMIO access. Reading the register
+ * back forces the interconnect to complete the write, preventing
+ * a bus hang on the subsequent access.
+ */
+ regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
+
+ return sysmon_set_avg_enable(sysmon, SYSMON_TEMP_EN_AVG_BASE,
+ SYSMON_TEMP_EN_AVG_COUNT,
+ hw_val ? ~0U : 0);
+ case IIO_VOLTAGE:
+ 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 above */
+ regmap_read(sysmon->regmap, SYSMON_CONFIG, &readback);
+
+ return sysmon_set_avg_enable(sysmon, SYSMON_SUPPLY_EN_AVG_BASE,
+ SYSMON_SUPPLY_EN_AVG_COUNT,
+ hw_val ? ~0U : 0);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sysmon_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ int i, ret;
+
+ if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+ return -EINVAL;
+
+ for (i = 0; i < ARRAY_SIZE(sysmon_oversampling_avail); i++) {
+ if (val == sysmon_oversampling_avail[i])
+ break;
+ }
+ if (i == ARRAY_SIZE(sysmon_oversampling_avail))
+ return -EINVAL;
+
+ guard(mutex)(&sysmon->lock);
+
+ ret = sysmon_osr_write(sysmon, chan->type, val);
+ if (ret)
+ return ret;
+
+ if (chan->type == IIO_TEMP)
+ sysmon->temp_oversampling = val;
+ else
+ sysmon->supply_oversampling = val;
+
+ return 0;
+}
+
+static int sysmon_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ long mask)
+{
+ if (mask == IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+ return IIO_VAL_INT;
+
+ return -EINVAL;
+}
+
+static int sysmon_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type,
+ int *length, long mask)
+{
+ if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
+ return -EINVAL;
+
+ *vals = sysmon_oversampling_avail;
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(sysmon_oversampling_avail);
+
+ return IIO_AVAIL_LIST;
+}
+
static int sysmon_read_label(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
char *label)
@@ -508,6 +641,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,
@@ -811,6 +947,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
.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 ?
@@ -843,7 +983,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev,
(reg - 1) * SYSMON_REG_STRIDE,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
.info_mask_shared_by_type =
- BIT(IIO_CHAN_INFO_SCALE),
+ 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,
};
}
@@ -890,6 +1033,8 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
sysmon = iio_priv(indio_dev);
sysmon->regmap = regmap;
+ sysmon->temp_oversampling = 1;
+ sysmon->supply_oversampling = 1;
ret = devm_mutex_init(dev, &sysmon->lock);
if (ret)
diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
index b5861f7e882..ad97ff699dc 100644
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -24,11 +24,13 @@ struct regmap;
#define SYSMON_IMR 0x0048
#define SYSMON_IER 0x004C
#define SYSMON_IDR 0x0050
+#define SYSMON_CONFIG 0x0100
#define SYSMON_TEMP_MAX 0x1030
#define SYSMON_TEMP_MIN 0x1034
#define SYSMON_SUPPLY_BASE 0x1040
#define SYSMON_ALARM_FLAG 0x1018
#define SYSMON_ALARM_REG 0x1940
+#define SYSMON_SUPPLY_EN_AVG_BASE 0x1958
#define SYSMON_TEMP_TH_LOW 0x1970
#define SYSMON_TEMP_TH_UP 0x1974
#define SYSMON_OT_TH_LOW 0x1978
@@ -40,6 +42,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 */
@@ -56,6 +59,16 @@ struct regmap;
/* ISR/IMR temperature and OT alarm mask (bits 9:8) */
#define SYSMON_TEMP_INTR_MASK GENMASK(9, 8)
+/* 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)
@@ -85,6 +98,8 @@ struct regmap;
* @temp_hysteresis: cached DEVICE_TEMP hysteresis in millicelsius
* @ot_hysteresis: cached OT hysteresis in millicelsius
* @sysmon_unmask_work: re-enables events after alarm condition clears
+ * @temp_oversampling: current temp oversampling ratio
+ * @supply_oversampling: current supply oversampling ratio
*/
struct sysmon {
struct regmap *regmap;
@@ -105,6 +120,8 @@ struct sysmon {
int temp_hysteresis;
int ot_hysteresis;
struct delayed_work sysmon_unmask_work;
+ unsigned int temp_oversampling;
+ unsigned int supply_oversampling;
};
int sysmon_core_probe(struct device *dev, struct regmap *regmap);
--
2.48.1
^ permalink raw reply related
* [PATCH v5 2/5] iio: adc: add Versal SysMon driver
From: Salih Erim @ 2026-06-08 18:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
In-Reply-To: <20260608183801.1257051-1-salih.erim@amd.com>
Add the core driver and MMIO platform driver for the AMD/Xilinx Versal
System Monitor (SysMon) block.
The SysMon block resides in the platform management controller (PMC) and
provides on-chip voltage and temperature monitoring through a 10-bit,
200 kSPS ADC. It can monitor up to 160 voltage channels and 64
temperature satellites distributed across the SoC, with a consistent
sample rate of 8 kSPS per channel regardless of how many channels are
enabled.
The driver is split into three compilation units:
- versal-sysmon-core: Channel parsing, IIO registration, read_raw
- versal-sysmon: MMIO platform driver with custom regmap accessors
Voltage results are stored in a 19-bit modified floating-point format
and converted to millivolts. Temperature results are stored in Q8.7
signed fixed-point Celsius format and converted to millicelsius.
The MMIO regmap backend uses a custom reg_write accessor that
automatically unlocks the NPI (NoC programming interface) lock
register before each write, as required by the hardware. The regmap
is configured with fast_io since the underlying MMIO accessors are
safe to call from atomic context.
Co-developed-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in v5:
- Add err.h include to core (IWYU) (Andy)
- Drop (int) cast on MILLI in scale assignment (Andy)
- sign_extend32() instead of (s16) cast for temperature raw (Andy)
- Remove unneeded parentheses in voltage address calculation (Andy)
- Drop NULL checks before fwnode_get_child_node_count (Andy)
- Nested size_add() for overflow-safe allocation (Andy)
- if (ret) instead of if (ret < 0) for fwnode property reads (Andy)
- Remove outer parentheses in satellite address calculation (Andy)
- Loop index declared in for() scope (Andy)
- MMIO: add err.h, types.h includes (IWYU) (Andy)
- Header: remove unused types.h include and struct iio_dev
forward declaration (Andy)
Changes in v4:
- Temperature: RAW + SCALE (IIO_VAL_FRACTIONAL, 1000/128) instead
of PROCESSED (Jonathan)
- Voltage: PROCESSED only, drop RAW (Jonathan)
- Drop scan_type from all channel macros (Jonathan)
- Move __free(fwnode_handle) declarations down to just above use
(Jonathan)
- devm_regmap_init() on one line (Jonathan)
- Lock comment: describe RMW sequences and cached state (Jonathan)
- Remove sysmon_q8p7_to_millicelsius() from this patch; the function
is now introduced in P4 where it is first used
Changes in v3:
- IWYU: add array_size.h, string.h, types.h to core; audit and
fix header and MMIO driver includes (Andy)
- Rename _ext to _name in SYSMON_CHAN_TEMP macro parameter (Andy,
Jonathan)
- Use .info_mask_separate = BIT() style in SYSMON_CHAN_TEMP (Andy)
- Use s16 parameter in sysmon_q8p7_to_millicelsius (Andy)
- Use sign_extend32() in sysmon_supply_rawtoprocessed (Andy)
- Split sysmon_read_raw parameters logically across lines (Andy)
- Remove redundant (int) casts on regval (Andy)
- Split num_supply/num_temp initialization (Andy)
- Use __free(fwnode_handle) cleanup, remove goto err_put (Andy)
- Use size_add() for overflow-safe allocation (Andy)
- Use dev_err_probe() in sysmon_parse_fw error paths (Jonathan)
- Move fwnode_irq_get() to core_probe, remove irq parameter
from bus driver interfaces (Jonathan)
- Use (int)MILLI at call sites, drop SYSMON_MILLI define (Andy,
Jonathan)
- Remove sysmon->dev, sysmon->indio_dev, sysmon->irq from struct;
pass as local variables or use regmap_get_device() (Jonathan)
- Use struct device *dev local in sysmon_platform_probe (Andy)
- Describe protected data in lock comment (Jonathan)
- Add comment explaining RAW+PROCESSED co-exposure (Jonathan)
Changes in v2:
- Split into core (versal-sysmon-core.c) + MMIO platform driver
(versal-sysmon.c) + shared header (versal-sysmon.h)
- Uses regmap API instead of direct readl/writel
- MMIO regmap uses custom callbacks with NPI unlock in write path
- Reverse Christmas Tree variable ordering throughout
- Header include order fixed
- MAINTAINERS entry folded in with wildcard F: pattern
- Kconfig: hidden VERSAL_SYSMON_CORE + VERSAL_SYSMON selects it
- Kconfig/Makefile: alphabetical ordering (VERSAL before VF610)
- Bounds validation on DT reg values
- Named constants replace magic numbers (SYSMON_REG_STRIDE,
SYSMON_SUPPLY_MANTISSA_BITS, SYSMON_MILLI)
- kernel-doc for exported sysmon_core_probe() and sysmon_parse_fw()
- Supply voltage conversion uses proper two's complement sign
extension (s16 cast) matching the hardware specification
- Register offsets sorted by address in header
- Each patch introduces only the defines, fields, and includes
it uses (no dead code in any commit)
- Removed unused linux/limits.h and linux/units.h includes
- Renamed iio_dev_info to sysmon_iio_info
- regmap_write return values checked in probe init path
MAINTAINERS | 7 +
drivers/iio/adc/Kconfig | 20 ++
drivers/iio/adc/Makefile | 2 +
drivers/iio/adc/versal-sysmon-core.c | 285 +++++++++++++++++++++++++++
drivers/iio/adc/versal-sysmon.c | 93 +++++++++
drivers/iio/adc/versal-sysmon.h | 67 +++++++
6 files changed, 474 insertions(+)
create mode 100644 drivers/iio/adc/versal-sysmon-core.c
create mode 100644 drivers/iio/adc/versal-sysmon.c
create mode 100644 drivers/iio/adc/versal-sysmon.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 2fb1c75afd1..46762c8496d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29216,6 +29216,13 @@ F: Documentation/devicetree/bindings/memory-controllers/xlnx,versal-net-ddrmc5.y
F: drivers/edac/versalnet_edac.c
F: include/linux/cdx/edac_cdx_pcol.h
+XILINX VERSAL SYSMON DRIVER
+M: Salih Erim <salih.erim@amd.com>
+L: linux-iio@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
+F: drivers/iio/adc/versal-sysmon*
+
XILINX WATCHDOG DRIVER
M: Srinivas Neeli <srinivas.neeli@amd.com>
R: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a9dedbb8eb4..c7f19057484 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1943,6 +1943,26 @@ config TWL6030_GPADC
This driver can also be built as a module. If so, the module will be
called twl6030-gpadc.
+config VERSAL_SYSMON_CORE
+ tristate
+ select REGMAP
+
+config VERSAL_SYSMON
+ tristate "AMD Versal SysMon driver"
+ depends on ARCH_ZYNQMP || COMPILE_TEST
+ depends on HAS_IOMEM
+ select VERSAL_SYSMON_CORE
+ help
+ Say yes here to have support for the AMD/Xilinx Versal System
+ Monitor (SysMon). This driver provides voltage and temperature
+ monitoring through the IIO subsystem.
+
+ The SysMon measures up to 160 supply voltages and reads up to
+ 64 temperature satellites distributed across the SoC.
+
+ To compile this driver as a module, choose M here: the module
+ will be called versal-sysmon.
+
config VF610_ADC
tristate "Freescale vf610 ADC driver"
depends on HAS_IOMEM
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 097357d146b..d7696b1b157 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -167,6 +167,8 @@ obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o
obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
+obj-$(CONFIG_VERSAL_SYSMON_CORE) += versal-sysmon-core.o
+obj-$(CONFIG_VERSAL_SYSMON) += versal-sysmon.o
obj-$(CONFIG_VF610_ADC) += vf610_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
new file mode 100644
index 00000000000..c7b5eecc93c
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Versal SysMon core driver
+ *
+ * Copyright (C) 2019 - 2022, Xilinx, Inc.
+ * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+
+#include "versal-sysmon.h"
+
+#define SYSMON_CHAN_TEMP(_chan, _address, _name) { \
+ .type = IIO_TEMP, \
+ .indexed = 1, \
+ .address = _address, \
+ .channel = _chan, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .datasheet_name = _name, \
+}
+
+/* Static temperature channels (always present) */
+static const struct iio_chan_spec temp_channels[] = {
+ SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
+ SYSMON_CHAN_TEMP(1, SYSMON_TEMP_MIN, "min"),
+ SYSMON_CHAN_TEMP(2, SYSMON_TEMP_MAX_MAX, "max_max"),
+ SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
+};
+
+static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
+{
+ int mantissa, format, exponent;
+
+ mantissa = FIELD_GET(SYSMON_MANTISSA_MASK, raw_data);
+ exponent = SYSMON_SUPPLY_MANTISSA_BITS - FIELD_GET(SYSMON_MODE_MASK, raw_data);
+ format = FIELD_GET(SYSMON_FMT_MASK, raw_data);
+ /*
+ * When format bit is set the mantissa is two's complement
+ * (per hardware spec); sign-extend to int for correct arithmetic.
+ */
+ if (format)
+ mantissa = sign_extend32(mantissa, 15);
+
+ *val = (mantissa * (int)MILLI) >> exponent;
+}
+
+static int sysmon_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int regval;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ switch (chan->type) {
+ case IIO_TEMP:
+ if (mask == IIO_CHAN_INFO_SCALE) {
+ /* Q8.7 to millicelsius: raw * 1000 / 128 */
+ *val = MILLI;
+ *val2 = BIT(SYSMON_FRACTIONAL_SHIFT);
+ return IIO_VAL_FRACTIONAL;
+ }
+ if (mask != IIO_CHAN_INFO_RAW)
+ return -EINVAL;
+
+ ret = regmap_read(sysmon->regmap, chan->address, ®val);
+ if (ret)
+ return ret;
+
+ *val = sign_extend32(regval, 15);
+ return IIO_VAL_INT;
+
+ case IIO_VOLTAGE:
+ if (mask != IIO_CHAN_INFO_PROCESSED)
+ return -EINVAL;
+
+ ret = regmap_read(sysmon->regmap,
+ chan->address * SYSMON_REG_STRIDE +
+ SYSMON_SUPPLY_BASE, ®val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_rawtoprocessed(regval, val);
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sysmon_read_label(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ char *label)
+{
+ if (chan->datasheet_name)
+ return sysfs_emit(label, "%s\n", chan->datasheet_name);
+
+ return -EINVAL;
+}
+
+static const struct iio_info sysmon_iio_info = {
+ .read_raw = sysmon_read_raw,
+ .read_label = sysmon_read_label,
+};
+
+/**
+ * sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
+ * @indio_dev: IIO device instance
+ * @dev: Parent device
+ *
+ * Reads voltage-channels and temperature-channels container nodes from
+ * firmware and builds the IIO channel array. Static temperature channels
+ * are prepended, followed by supply and satellite channels from DT.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
+{
+ unsigned int num_supply = 0, num_temp = 0;
+ unsigned int idx, temp_chan_idx, volt_chan_idx;
+ struct iio_chan_spec *sysmon_channels;
+ const char *label;
+ u32 reg;
+ int ret;
+
+ struct fwnode_handle *supply_node __free(fwnode_handle) =
+ device_get_named_child_node(dev, "voltage-channels");
+ num_supply = fwnode_get_child_node_count(supply_node);
+
+ struct fwnode_handle *temp_node __free(fwnode_handle) =
+ device_get_named_child_node(dev, "temperature-channels");
+ num_temp = fwnode_get_child_node_count(temp_node);
+
+ sysmon_channels = devm_kcalloc(dev,
+ size_add(size_add(ARRAY_SIZE(temp_channels),
+ num_supply), num_temp),
+ sizeof(*sysmon_channels), GFP_KERNEL);
+ if (!sysmon_channels)
+ return -ENOMEM;
+
+ /* Static temperature channels first (fixed indices) */
+ idx = 0;
+ memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
+ idx += ARRAY_SIZE(temp_channels);
+
+ /* Supply channels from DT */
+ fwnode_for_each_child_node_scoped(supply_node, child) {
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "missing reg for supply channel\n");
+
+ if (reg > SYSMON_SUPPLY_IDX_MAX)
+ return dev_err_probe(dev, -EINVAL,
+ "supply reg %u exceeds max %u\n",
+ reg, SYSMON_SUPPLY_IDX_MAX);
+
+ ret = fwnode_property_read_string(child, "label", &label);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "missing label for supply channel\n");
+
+ sysmon_channels[idx++] = (struct iio_chan_spec) {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .address = reg,
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_PROCESSED),
+ .datasheet_name = label,
+ };
+ }
+
+ /* Temperature satellite channels from DT */
+ fwnode_for_each_child_node_scoped(temp_node, child) {
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "missing reg for temp channel\n");
+
+ if (reg < 1 || reg > SYSMON_TEMP_SAT_MAX)
+ return dev_err_probe(dev, -EINVAL,
+ "temp reg %u out of range [1..%u]\n",
+ reg, SYSMON_TEMP_SAT_MAX);
+
+ ret = fwnode_property_read_string(child, "label", &label);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "missing label for temp channel\n");
+
+ sysmon_channels[idx++] = (struct iio_chan_spec) {
+ .type = IIO_TEMP,
+ .indexed = 1,
+ .address = SYSMON_TEMP_SAT_BASE +
+ (reg - 1) * SYSMON_REG_STRIDE,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_type =
+ BIT(IIO_CHAN_INFO_SCALE),
+ .datasheet_name = label,
+ };
+ }
+
+ indio_dev->num_channels = idx;
+ indio_dev->info = &sysmon_iio_info;
+
+ /*
+ * Assign per-type sequential channel numbers.
+ * IIO sysfs uses type prefix (in_tempN, in_voltageN)
+ * so numbers only need to be unique within each type.
+ */
+ temp_chan_idx = 0;
+ volt_chan_idx = 0;
+ for (unsigned int idx = 0; idx < indio_dev->num_channels; idx++) {
+ if (sysmon_channels[idx].type == IIO_TEMP)
+ sysmon_channels[idx].channel = temp_chan_idx++;
+ else
+ sysmon_channels[idx].channel = volt_chan_idx++;
+ }
+
+ indio_dev->channels = sysmon_channels;
+
+ return 0;
+}
+
+/**
+ * sysmon_core_probe() - Initialize Versal SysMon core
+ * @dev: Parent device
+ * @regmap: Register map for hardware access
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int sysmon_core_probe(struct device *dev, struct regmap *regmap)
+{
+ struct iio_dev *indio_dev;
+ struct sysmon *sysmon;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*sysmon));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ sysmon = iio_priv(indio_dev);
+ sysmon->regmap = regmap;
+
+ ret = devm_mutex_init(dev, &sysmon->lock);
+ if (ret)
+ return ret;
+
+ /* Disable all interrupts and clear pending status */
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
+ if (ret)
+ return ret;
+ ret = regmap_write(sysmon->regmap, SYSMON_ISR, SYSMON_INTR_ALL_MASK);
+ if (ret)
+ return ret;
+
+ indio_dev->name = "versal-sysmon";
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = sysmon_parse_fw(indio_dev, dev);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+EXPORT_SYMBOL_GPL(sysmon_core_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Versal SysMon Core Driver");
+MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");
diff --git a/drivers/iio/adc/versal-sysmon.c b/drivers/iio/adc/versal-sysmon.c
new file mode 100644
index 00000000000..0c2704326a4
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Versal SysMon MMIO platform driver
+ *
+ * Copyright (C) 2019 - 2022, Xilinx, Inc.
+ * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#include "versal-sysmon.h"
+
+struct sysmon_mmio {
+ void __iomem *base;
+};
+
+static int sysmon_mmio_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct sysmon_mmio *mmio = context;
+
+ *val = readl(mmio->base + reg);
+ return 0;
+}
+
+static int sysmon_mmio_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct sysmon_mmio *mmio = context;
+
+ /* NPI must be unlocked before any register write except to NPI_LOCK */
+ if (reg != SYSMON_NPI_LOCK)
+ writel(SYSMON_NPI_UNLOCK_CODE, mmio->base + SYSMON_NPI_LOCK);
+ writel(val, mmio->base + reg);
+
+ return 0;
+}
+
+static const struct regmap_config sysmon_mmio_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = SYSMON_REG_STRIDE,
+ .max_register = SYSMON_MAX_REG,
+ .reg_read = sysmon_mmio_reg_read,
+ .reg_write = sysmon_mmio_reg_write,
+ .fast_io = true,
+};
+
+static int sysmon_platform_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct sysmon_mmio *mmio;
+ struct regmap *regmap;
+
+ mmio = devm_kzalloc(dev, sizeof(*mmio), GFP_KERNEL);
+ if (!mmio)
+ return -ENOMEM;
+
+ mmio->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(mmio->base))
+ return PTR_ERR(mmio->base);
+
+ regmap = devm_regmap_init(dev, NULL, mmio, &sysmon_mmio_regmap_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return sysmon_core_probe(dev, regmap);
+}
+
+static const struct of_device_id sysmon_of_match_table[] = {
+ { .compatible = "xlnx,versal-sysmon" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, sysmon_of_match_table);
+
+static struct platform_driver sysmon_platform_driver = {
+ .probe = sysmon_platform_probe,
+ .driver = {
+ .name = "versal-sysmon",
+ .of_match_table = sysmon_of_match_table,
+ },
+};
+module_platform_driver(sysmon_platform_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Versal SysMon Platform Driver");
+MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");
diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
new file mode 100644
index 00000000000..0c0c89f6e0c
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * AMD Versal SysMon driver
+ *
+ * Copyright (C) 2019 - 2022, Xilinx, Inc.
+ * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#ifndef _VERSAL_SYSMON_H_
+#define _VERSAL_SYSMON_H_
+
+#include <linux/bits.h>
+#include <linux/mutex.h>
+
+struct device;
+struct regmap;
+
+/* Register offsets (sorted by address) */
+#define SYSMON_NPI_LOCK 0x000C
+#define SYSMON_ISR 0x0044
+#define SYSMON_IDR 0x0050
+#define SYSMON_TEMP_MAX 0x1030
+#define SYSMON_TEMP_MIN 0x1034
+#define SYSMON_SUPPLY_BASE 0x1040
+#define SYSMON_TEMP_MIN_MIN 0x1F8C
+#define SYSMON_TEMP_MAX_MAX 0x1F90
+#define SYSMON_TEMP_SAT_BASE 0x1FAC
+#define SYSMON_MAX_REG 0x24C0
+
+/* NPI unlock value written to SYSMON_NPI_LOCK */
+#define SYSMON_NPI_UNLOCK_CODE 0xF9E8D7C6
+
+/* Register stride: 4 bytes per 32-bit register */
+#define SYSMON_REG_STRIDE 4
+
+#define SYSMON_SUPPLY_IDX_MAX 159
+#define SYSMON_TEMP_SAT_MAX 64
+#define SYSMON_INTR_ALL_MASK GENMASK(31, 0)
+
+/* Supply voltage conversion register fields */
+#define SYSMON_MANTISSA_MASK GENMASK(15, 0)
+#define SYSMON_FMT_MASK BIT(16)
+#define SYSMON_MODE_MASK GENMASK(18, 17)
+
+/* Q8.7 fractional shift */
+#define SYSMON_FRACTIONAL_SHIFT 7U
+#define SYSMON_SUPPLY_MANTISSA_BITS 16
+
+/**
+ * struct sysmon - Driver data for Versal SysMon
+ * @regmap: register map for hardware access
+ * @lock: protects read-modify-write sequences on threshold registers
+ * and cached state that spans multiple regmap calls
+ */
+struct sysmon {
+ struct regmap *regmap;
+ /*
+ * Protects read-modify-write sequences on threshold registers
+ * and cached state (oversampling ratios, hysteresis values)
+ * that spans multiple regmap calls.
+ */
+ struct mutex lock;
+};
+
+int sysmon_core_probe(struct device *dev, struct regmap *regmap);
+
+#endif /* _VERSAL_SYSMON_H_ */
--
2.48.1
^ permalink raw reply related
* [PATCH v5 4/5] iio: adc: versal-sysmon: add threshold event support
From: Salih Erim @ 2026-06-08 18:38 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
In-Reply-To: <20260608183801.1257051-1-salih.erim@amd.com>
Add threshold event support for temperature and supply voltage
channels.
Temperature events:
- Rising threshold with configurable value
- Over-temperature (OT) alarm with separate threshold
- Per-channel hysteresis as a millicelsius value
- Event direction is IIO_EV_DIR_RISING (hysteresis mode)
Supply voltage events:
- Rising/falling threshold per supply channel
- Per-channel alarm enable via alarm configuration registers
The hardware supports both window and hysteresis alarm modes for
temperature. This driver uses hysteresis mode, where the upper
threshold triggers the alarm and the lower threshold clears it
(re-arm point). The hardware has a single ISR bit per temperature
channel with no indication of which threshold was crossed, so
hysteresis mode is the natural fit. The lower threshold register
is computed internally as (upper - hysteresis).
Hysteresis is stored in the driver as a millicelsius value,
initialized from the hardware registers at probe. Writing the
rising threshold or hysteresis recomputes the lower register.
ALARM_CONFIG is hard-coded to hysteresis mode during init.
The interrupt handler masks active threshold interrupts (which are
level-sensitive) and schedules a delayed worker to poll for condition
clear before unmasking. When no hardware IRQ is available, event
channels are not created and interrupt init is skipped, since the
I2C regmap backend cannot be called from atomic context.
When disabling a supply channel alarm, the group interrupt remains
active if any other channel in the same alarm group still has an
alarm enabled.
Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in 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 | 680 ++++++++++++++++++++++++++-
drivers/iio/adc/versal-sysmon.h | 45 ++
2 files changed, 716 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
index c7b5eecc93c..5fbd509089b 100644
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -11,7 +11,9 @@
#include <linux/bitops.h>
#include <linux/cleanup.h>
#include <linux/device.h>
+#include <linux/devm-helpers.h>
#include <linux/err.h>
+#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/property.h>
#include <linux/regmap.h>
@@ -19,10 +21,19 @@
#include <linux/sysfs.h>
#include <linux/units.h>
+#include <linux/iio/events.h>
#include <linux/iio/iio.h>
#include "versal-sysmon.h"
+/* OT and TEMP hysteresis mode bits in SYSMON_TEMP_EV_CFG */
+#define SYSMON_OT_HYST_MASK BIT(0)
+#define SYSMON_TEMP_HYST_MASK BIT(1)
+
+/* Compute alarm register offset from a channel address */
+#define SYSMON_ALARM_OFFSET(addr) \
+ (SYSMON_ALARM_REG + ((addr) / SYSMON_ALARM_BITS_PER_REG) * SYSMON_REG_STRIDE)
+
#define SYSMON_CHAN_TEMP(_chan, _address, _name) { \
.type = IIO_TEMP, \
.indexed = 1, \
@@ -33,14 +44,86 @@
.datasheet_name = _name, \
}
+#define SYSMON_CHAN_TEMP_EVENT(_chan, _address, _name, _events) {\
+ .type = IIO_TEMP, \
+ .indexed = 1, \
+ .address = _address, \
+ .channel = _chan, \
+ .event_spec = _events, \
+ .num_event_specs = ARRAY_SIZE(_events), \
+ .datasheet_name = _name, \
+}
+
+enum sysmon_alarm_bit {
+ SYSMON_BIT_ALARM0 = 0,
+ SYSMON_BIT_ALARM1 = 1,
+ SYSMON_BIT_ALARM2 = 2,
+ SYSMON_BIT_ALARM3 = 3,
+ SYSMON_BIT_ALARM4 = 4,
+ SYSMON_BIT_OT = 8,
+ SYSMON_BIT_TEMP = 9,
+};
+
+/* Temperature event specification: rising threshold + hysteresis only */
+static const struct iio_event_spec sysmon_temp_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_HYSTERESIS),
+ },
+};
+
+/* Supply event specifications */
+static const struct iio_event_spec sysmon_supply_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
/* Static temperature channels (always present) */
-static const struct iio_chan_spec temp_channels[] = {
+static const struct iio_chan_spec temp_channels_no_events[] = {
+ SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
+ SYSMON_CHAN_TEMP(1, SYSMON_TEMP_MIN, "min"),
+ SYSMON_CHAN_TEMP(2, SYSMON_TEMP_MAX_MAX, "max_max"),
+ SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
+};
+
+/* Static temperature channels with event support (when IRQ available) */
+static const struct iio_chan_spec temp_channels_with_events[] = {
SYSMON_CHAN_TEMP(0, SYSMON_TEMP_MAX, "temp"),
SYSMON_CHAN_TEMP(1, SYSMON_TEMP_MIN, "min"),
SYSMON_CHAN_TEMP(2, SYSMON_TEMP_MAX_MAX, "max_max"),
SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
+ SYSMON_CHAN_TEMP_EVENT(4, SYSMON_ADDR_TEMP_EVENT, "temp",
+ sysmon_temp_events),
+ SYSMON_CHAN_TEMP_EVENT(5, SYSMON_ADDR_OT_EVENT, "ot",
+ sysmon_temp_events),
};
+static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
+{
+ *val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;
+}
+
+static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
+{
+ *raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / (int)MILLI;
+}
+
static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
{
int mantissa, format, exponent;
@@ -58,6 +141,49 @@ static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
*val = (mantissa * (int)MILLI) >> exponent;
}
+static void sysmon_supply_processedtoraw(int val, u32 reg_val, u32 *raw_data)
+{
+ int exponent = FIELD_GET(SYSMON_MODE_MASK, reg_val);
+ int format = FIELD_GET(SYSMON_FMT_MASK, reg_val);
+ int scale, tmp;
+
+ scale = BIT(SYSMON_SUPPLY_MANTISSA_BITS - exponent);
+ tmp = (val * scale) / (int)MILLI;
+
+ if (format)
+ tmp = clamp(tmp, S16_MIN, S16_MAX);
+ else
+ tmp = clamp(tmp, 0, U16_MAX);
+
+ *raw_data = (u16)tmp;
+}
+
+static int sysmon_temp_thresh_offset(int address,
+ enum iio_event_direction dir)
+{
+ switch (address) {
+ case SYSMON_ADDR_TEMP_EVENT:
+ return (dir == IIO_EV_DIR_RISING) ? SYSMON_TEMP_TH_UP :
+ SYSMON_TEMP_TH_LOW;
+ case SYSMON_ADDR_OT_EVENT:
+ return (dir == IIO_EV_DIR_RISING) ? SYSMON_OT_TH_UP :
+ SYSMON_OT_TH_LOW;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sysmon_supply_thresh_offset(int address,
+ enum iio_event_direction dir)
+{
+ if (dir == IIO_EV_DIR_RISING)
+ return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_UP;
+ if (dir == IIO_EV_DIR_FALLING)
+ return (address * SYSMON_REG_STRIDE) + SYSMON_SUPPLY_TH_LOW;
+
+ return -EINVAL;
+}
+
static int sysmon_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -104,6 +230,272 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
}
}
+static int sysmon_get_event_mask(unsigned long address)
+{
+ if (address == SYSMON_ADDR_TEMP_EVENT)
+ return BIT(SYSMON_BIT_TEMP);
+ if (address == SYSMON_ADDR_OT_EVENT)
+ return BIT(SYSMON_BIT_OT);
+
+ return BIT(address / SYSMON_ALARM_BITS_PER_REG);
+}
+
+static int sysmon_read_alarm_config(struct sysmon *sysmon,
+ unsigned long address)
+{
+ u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
+ u32 offset = SYSMON_ALARM_OFFSET(address);
+ unsigned int reg_val;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ return !!(reg_val & BIT(shift));
+}
+
+static int sysmon_write_alarm_config(struct sysmon *sysmon,
+ unsigned long address, bool enable)
+{
+ u32 shift = address % SYSMON_ALARM_BITS_PER_REG;
+ u32 offset = SYSMON_ALARM_OFFSET(address);
+
+ return regmap_assign_bits(sysmon->regmap, offset, BIT(shift), enable);
+}
+
+static int sysmon_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ u32 alarm_event_mask = sysmon_get_event_mask(chan->address);
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int imr;
+ int config_value;
+ int ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+ if (ret)
+ return ret;
+
+ /* IMR bits are 1=masked, invert to get 1=enabled */
+ imr = ~imr;
+
+ if (chan->type == IIO_VOLTAGE) {
+ config_value = sysmon_read_alarm_config(sysmon, chan->address);
+ if (config_value < 0)
+ return config_value;
+ return config_value && (imr & alarm_event_mask);
+ }
+
+ return !!(imr & alarm_event_mask);
+}
+
+static int sysmon_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ bool state)
+{
+ u32 offset = SYSMON_ALARM_OFFSET(chan->address);
+ u32 ier = sysmon_get_event_mask(chan->address);
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int alarm_config;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ if (chan->type == IIO_VOLTAGE) {
+ ret = sysmon_write_alarm_config(sysmon, chan->address, state);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, offset, &alarm_config);
+ if (ret)
+ return ret;
+
+ if (alarm_config)
+ return regmap_write(sysmon->regmap, SYSMON_IER, ier);
+
+ return regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+ }
+
+ if (chan->type == IIO_TEMP) {
+ if (state) {
+ ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->temp_mask &= ~ier;
+ } else {
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
+ if (ret)
+ return ret;
+
+ scoped_guard(spinlock_irq, &sysmon->irq_lock)
+ sysmon->temp_mask |= ier;
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * Recompute the lower threshold register from upper threshold and
+ * cached hysteresis. Called when either upper threshold or hysteresis
+ * is written.
+ */
+static int sysmon_update_temp_lower(struct sysmon *sysmon, int address)
+{
+ unsigned int upper_reg;
+ int upper_mc, lower_mc, hysteresis;
+ u32 raw_val;
+ int upper_off, lower_off, ret;
+
+ upper_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_RISING);
+ if (upper_off < 0)
+ return upper_off;
+ lower_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_FALLING);
+ if (lower_off < 0)
+ return lower_off;
+
+ if (address == SYSMON_ADDR_OT_EVENT)
+ hysteresis = sysmon->ot_hysteresis;
+ else
+ hysteresis = sysmon->temp_hysteresis;
+
+ ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+
+ lower_mc = upper_mc - hysteresis;
+ sysmon_millicelsius_to_q8p7(&raw_val, lower_mc);
+
+ return regmap_write(sysmon->regmap, lower_off, raw_val);
+}
+
+static int sysmon_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int reg_val;
+ int offset;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ if (chan->type == IIO_TEMP) {
+ if (info == IIO_EV_INFO_VALUE) {
+ /* Only rising threshold is exposed */
+ offset = sysmon_temp_thresh_offset(chan->address,
+ IIO_EV_DIR_RISING);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(reg_val, val);
+
+ return IIO_VAL_INT;
+ }
+ if (info == IIO_EV_INFO_HYSTERESIS) {
+ if (chan->address == SYSMON_ADDR_OT_EVENT)
+ *val = sysmon->ot_hysteresis;
+ else
+ *val = sysmon->temp_hysteresis;
+ return IIO_VAL_INT;
+ }
+ }
+
+ if (chan->type == IIO_VOLTAGE) {
+ offset = sysmon_supply_thresh_offset(chan->address, dir);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_rawtoprocessed(reg_val, val);
+
+ return IIO_VAL_INT;
+ }
+
+ return -EINVAL;
+}
+
+static int sysmon_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int reg_val;
+ u32 raw_val;
+ int offset;
+ int ret;
+
+ guard(mutex)(&sysmon->lock);
+
+ if (chan->type == IIO_TEMP) {
+ if (info == IIO_EV_INFO_VALUE) {
+ /* Only rising threshold is exposed */
+ offset = sysmon_temp_thresh_offset(chan->address,
+ IIO_EV_DIR_RISING);
+ if (offset < 0)
+ return offset;
+
+ sysmon_millicelsius_to_q8p7(&raw_val, val);
+
+ ret = regmap_write(sysmon->regmap, offset, raw_val);
+ if (ret)
+ return ret;
+
+ /* Recompute lower = upper - hysteresis */
+ return sysmon_update_temp_lower(sysmon, chan->address);
+ }
+ if (info == IIO_EV_INFO_HYSTERESIS) {
+ if (val < 0)
+ return -EINVAL;
+
+ if (chan->address == SYSMON_ADDR_OT_EVENT)
+ sysmon->ot_hysteresis = val;
+ else
+ sysmon->temp_hysteresis = val;
+
+ return sysmon_update_temp_lower(sysmon, chan->address);
+ }
+ }
+
+ if (chan->type == IIO_VOLTAGE) {
+ offset = sysmon_supply_thresh_offset(chan->address, dir);
+ if (offset < 0)
+ return offset;
+
+ ret = regmap_read(sysmon->regmap, offset, ®_val);
+ if (ret)
+ return ret;
+
+ sysmon_supply_processedtoraw(val, reg_val, &raw_val);
+
+ return regmap_write(sysmon->regmap, offset, raw_val);
+ }
+
+ return -EINVAL;
+}
+
static int sysmon_read_label(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
char *label)
@@ -117,23 +509,252 @@ static int sysmon_read_label(struct iio_dev *indio_dev,
static const struct iio_info sysmon_iio_info = {
.read_raw = sysmon_read_raw,
.read_label = sysmon_read_label,
+ .read_event_config = sysmon_read_event_config,
+ .write_event_config = sysmon_write_event_config,
+ .read_event_value = sysmon_read_event_value,
+ .write_event_value = sysmon_write_event_value,
};
+static void sysmon_push_event(struct iio_dev *indio_dev, u32 address)
+{
+ const struct iio_chan_spec *chan;
+ enum iio_event_direction dir;
+
+ for (unsigned int i = 0; i < indio_dev->num_channels; i++) {
+ if (indio_dev->channels[i].address != address)
+ continue;
+
+ chan = &indio_dev->channels[i];
+ /* Temp uses hysteresis mode (rising only), voltage uses window */
+ dir = (chan->type == IIO_TEMP) ? IIO_EV_DIR_RISING :
+ IIO_EV_DIR_EITHER;
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(chan->type,
+ chan->channel,
+ IIO_EV_TYPE_THRESH,
+ dir),
+ iio_get_time_ns(indio_dev));
+ }
+}
+
+static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
+{
+ u32 alarm_flag_offset = SYSMON_ALARM_FLAG + event * SYSMON_REG_STRIDE;
+ u32 alarm_reg_offset = SYSMON_ALARM_REG + event * SYSMON_REG_STRIDE;
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned long alarm_flag_reg;
+ unsigned int reg_val;
+ u32 address, bit;
+ int ret;
+
+ switch (event) {
+ case SYSMON_BIT_TEMP:
+ sysmon_push_event(indio_dev, SYSMON_ADDR_TEMP_EVENT);
+
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_TEMP));
+ if (ret)
+ return ret;
+
+ sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
+ return 0;
+
+ case SYSMON_BIT_OT:
+ sysmon_push_event(indio_dev, SYSMON_ADDR_OT_EVENT);
+
+ ret = regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_OT));
+ if (ret)
+ return ret;
+
+ sysmon->masked_temp |= BIT(SYSMON_BIT_OT);
+ return 0;
+
+ case SYSMON_BIT_ALARM0:
+ case SYSMON_BIT_ALARM1:
+ case SYSMON_BIT_ALARM2:
+ case SYSMON_BIT_ALARM3:
+ case SYSMON_BIT_ALARM4:
+ ret = regmap_read(sysmon->regmap, alarm_flag_offset, ®_val);
+ if (ret)
+ return ret;
+
+ alarm_flag_reg = reg_val;
+
+ for_each_set_bit(bit, &alarm_flag_reg, SYSMON_ALARM_BITS_PER_REG) {
+ address = bit + SYSMON_ALARM_BITS_PER_REG * event;
+ sysmon_push_event(indio_dev, address);
+ ret = regmap_clear_bits(sysmon->regmap, alarm_reg_offset, BIT(bit));
+ if (ret)
+ return ret;
+ }
+
+ return regmap_write(sysmon->regmap, alarm_flag_offset, alarm_flag_reg);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static void sysmon_handle_events(struct iio_dev *indio_dev,
+ unsigned long events)
+{
+ unsigned int bit;
+
+ for_each_set_bit(bit, &events, SYSMON_NO_OF_EVENTS)
+ sysmon_handle_event(indio_dev, bit);
+}
+
+static void sysmon_unmask_temp(struct sysmon *sysmon, unsigned int isr)
+{
+ unsigned int unmask, status;
+
+ status = isr & SYSMON_TEMP_INTR_MASK;
+
+ unmask = ~status & sysmon->masked_temp;
+ sysmon->masked_temp &= status;
+
+ /* Only unmask if not administratively disabled by userspace */
+ unmask &= ~sysmon->temp_mask;
+
+ regmap_write(sysmon->regmap, SYSMON_IER, unmask);
+}
+
+/*
+ * Versal threshold interrupts are level-sensitive. Active threshold
+ * interrupts are masked in the handler and polled via delayed work
+ * until the condition clears, then unmasked.
+ */
+static void sysmon_unmask_worker(struct work_struct *work)
+{
+ struct sysmon *sysmon =
+ container_of(work, struct sysmon, sysmon_unmask_work.work);
+ unsigned int isr;
+
+ /*
+ * regmap errors are not checked here because the worker and IRQ
+ * handler cannot propagate errors. The MMIO regmap uses fast_io
+ * with direct readl/writel which cannot fail.
+ */
+ spin_lock_irq(&sysmon->irq_lock);
+ regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
+ regmap_write(sysmon->regmap, SYSMON_ISR, isr);
+ sysmon_unmask_temp(sysmon, isr);
+ spin_unlock_irq(&sysmon->irq_lock);
+
+ if (sysmon->masked_temp)
+ schedule_delayed_work(&sysmon->sysmon_unmask_work,
+ msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
+ else
+ regmap_write(sysmon->regmap, SYSMON_STATUS_RESET, 1);
+}
+
+static irqreturn_t sysmon_iio_irq(int irq, void *data)
+{
+ struct iio_dev *indio_dev = data;
+ struct sysmon *sysmon = iio_priv(indio_dev);
+ unsigned int isr, imr;
+
+ guard(spinlock)(&sysmon->irq_lock);
+
+ regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
+ regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+
+ 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 int sysmon_init_interrupt(struct sysmon *sysmon,
+ struct device *dev,
+ struct iio_dev *indio_dev,
+ int irq)
+{
+ unsigned int imr;
+ int ret;
+
+ /* Events not supported without IRQ (e.g. I2C path) */
+ if (!irq)
+ return 0;
+
+ ret = devm_delayed_work_autocancel(dev, &sysmon->sysmon_unmask_work,
+ sysmon_unmask_worker);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
+ if (ret)
+ return ret;
+ sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
+
+ return devm_request_irq(dev, irq, sysmon_iio_irq, 0,
+ "sysmon-irq", indio_dev);
+}
+
+/*
+ * Initialize the cached hysteresis for a temperature channel from the
+ * current hardware threshold registers: hysteresis = upper - lower.
+ */
+static int sysmon_init_hysteresis(struct sysmon *sysmon, unsigned int address,
+ int *hysteresis)
+{
+ unsigned int upper_reg, lower_reg;
+ int upper_mc, lower_mc;
+ int upper_off, lower_off;
+ int ret;
+
+ upper_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_RISING);
+ if (upper_off < 0)
+ return upper_off;
+ lower_off = sysmon_temp_thresh_offset(address, IIO_EV_DIR_FALLING);
+ if (lower_off < 0)
+ return lower_off;
+
+ ret = regmap_read(sysmon->regmap, upper_off, &upper_reg);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(sysmon->regmap, lower_off, &lower_reg);
+ if (ret)
+ return ret;
+
+ sysmon_q8p7_to_millicelsius(upper_reg, &upper_mc);
+ sysmon_q8p7_to_millicelsius(lower_reg, &lower_mc);
+ *hysteresis = upper_mc - lower_mc;
+
+ return 0;
+}
+
/**
* sysmon_parse_fw() - Parse firmware nodes and configure IIO channels.
* @indio_dev: IIO device instance
* @dev: Parent device
+ * @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)
{
+ const struct iio_chan_spec *temp_chans;
unsigned int num_supply = 0, num_temp = 0;
- unsigned int idx, temp_chan_idx, volt_chan_idx;
+ unsigned int num_static, idx, temp_chan_idx, volt_chan_idx;
struct iio_chan_spec *sysmon_channels;
const char *label;
u32 reg;
@@ -147,17 +768,25 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
device_get_named_child_node(dev, "temperature-channels");
num_temp = fwnode_get_child_node_count(temp_node);
+ if (irq > 0) {
+ temp_chans = temp_channels_with_events;
+ num_static = ARRAY_SIZE(temp_channels_with_events);
+ } else {
+ temp_chans = temp_channels_no_events;
+ num_static = ARRAY_SIZE(temp_channels_no_events);
+ }
+
sysmon_channels = devm_kcalloc(dev,
- size_add(size_add(ARRAY_SIZE(temp_channels),
+ size_add(size_add(num_static,
num_supply), num_temp),
sizeof(*sysmon_channels), GFP_KERNEL);
if (!sysmon_channels)
return -ENOMEM;
- /* Static temperature channels first (fixed indices) */
+ /* Static temperature channels (with or without events) */
idx = 0;
- memcpy(sysmon_channels, temp_channels, sizeof(temp_channels));
- idx += ARRAY_SIZE(temp_channels);
+ memcpy(sysmon_channels, temp_chans, num_static * sizeof(*temp_chans));
+ idx += num_static;
/* Supply channels from DT */
fwnode_for_each_child_node_scoped(supply_node, child) {
@@ -182,6 +811,10 @@ static int sysmon_parse_fw(struct iio_dev *indio_dev, struct device *dev)
.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,
};
}
@@ -248,6 +881,7 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
{
struct iio_dev *indio_dev;
struct sysmon *sysmon;
+ int irq;
int ret;
indio_dev = devm_iio_device_alloc(dev, sizeof(*sysmon));
@@ -260,6 +894,7 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
ret = devm_mutex_init(dev, &sysmon->lock);
if (ret)
return ret;
+ spin_lock_init(&sysmon->irq_lock);
/* Disable all interrupts and clear pending status */
ret = regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
@@ -269,13 +904,40 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
if (ret)
return ret;
+ irq = fwnode_irq_get(dev_fwnode(dev), 0);
+ if (irq < 0)
+ 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 both temperature channels */
+ ret = regmap_set_bits(sysmon->regmap, SYSMON_TEMP_EV_CFG,
+ SYSMON_OT_HYST_MASK |
+ SYSMON_TEMP_HYST_MASK);
+ if (ret)
+ return ret;
+
+ /* Initialize cached hysteresis from hardware registers */
+ ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_TEMP_EVENT,
+ &sysmon->temp_hysteresis);
+ if (ret)
+ return ret;
+ ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_OT_EVENT,
+ &sysmon->ot_hysteresis);
+ if (ret)
+ return ret;
+
+ ret = sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
+ if (ret)
+ return ret;
+ }
+
return devm_iio_device_register(dev, indio_dev);
}
EXPORT_SYMBOL_GPL(sysmon_core_probe);
diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
index 0c0c89f6e0c..b5861f7e882 100644
--- a/drivers/iio/adc/versal-sysmon.h
+++ b/drivers/iio/adc/versal-sysmon.h
@@ -11,6 +11,9 @@
#include <linux/bits.h>
#include <linux/mutex.h>
+#include <linux/spinlock_types.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
struct device;
struct regmap;
@@ -18,12 +21,24 @@ struct regmap;
/* Register offsets (sorted by address) */
#define SYSMON_NPI_LOCK 0x000C
#define SYSMON_ISR 0x0044
+#define SYSMON_IMR 0x0048
+#define SYSMON_IER 0x004C
#define SYSMON_IDR 0x0050
#define SYSMON_TEMP_MAX 0x1030
#define SYSMON_TEMP_MIN 0x1034
#define SYSMON_SUPPLY_BASE 0x1040
+#define SYSMON_ALARM_FLAG 0x1018
+#define SYSMON_ALARM_REG 0x1940
+#define SYSMON_TEMP_TH_LOW 0x1970
+#define SYSMON_TEMP_TH_UP 0x1974
+#define SYSMON_OT_TH_LOW 0x1978
+#define SYSMON_OT_TH_UP 0x197C
+#define SYSMON_SUPPLY_TH_LOW 0x1980
+#define SYSMON_SUPPLY_TH_UP 0x1C80
+#define SYSMON_TEMP_EV_CFG 0x1F84
#define SYSMON_TEMP_MIN_MIN 0x1F8C
#define SYSMON_TEMP_MAX_MAX 0x1F90
+#define SYSMON_STATUS_RESET 0x1F94
#define SYSMON_TEMP_SAT_BASE 0x1FAC
#define SYSMON_MAX_REG 0x24C0
@@ -35,8 +50,12 @@ struct regmap;
#define SYSMON_SUPPLY_IDX_MAX 159
#define SYSMON_TEMP_SAT_MAX 64
+#define SYSMON_NO_OF_EVENTS 32
#define SYSMON_INTR_ALL_MASK GENMASK(31, 0)
+/* ISR/IMR temperature and OT alarm mask (bits 9:8) */
+#define SYSMON_TEMP_INTR_MASK GENMASK(9, 8)
+
/* Supply voltage conversion register fields */
#define SYSMON_MANTISSA_MASK GENMASK(15, 0)
#define SYSMON_FMT_MASK BIT(16)
@@ -46,11 +65,26 @@ struct regmap;
#define SYSMON_FRACTIONAL_SHIFT 7U
#define SYSMON_SUPPLY_MANTISSA_BITS 16
+/* Event address IDs for temp event channels */
+#define SYSMON_ADDR_TEMP_EVENT 160
+#define SYSMON_ADDR_OT_EVENT 161
+
+/* Bits per alarm register */
+#define SYSMON_ALARM_BITS_PER_REG 32
+
+#define SYSMON_UNMASK_WORK_DELAY_MS 500
+
/**
* struct sysmon - Driver data for Versal SysMon
* @regmap: register map for hardware access
* @lock: protects 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
+ * @ot_hysteresis: cached OT hysteresis in millicelsius
+ * @sysmon_unmask_work: re-enables events after alarm condition clears
*/
struct sysmon {
struct regmap *regmap;
@@ -60,6 +94,17 @@ 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;
+ int ot_hysteresis;
+ struct delayed_work sysmon_unmask_work;
};
int sysmon_core_probe(struct device *dev, struct regmap *regmap);
--
2.48.1
^ permalink raw reply related
* [PATCH v5 0/5] iio: adc: add AMD/Xilinx Versal SysMon driver
From: Salih Erim @ 2026-06-08 18:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
This series adds a new IIO driver for the AMD/Xilinx Versal System
Monitor (SysMon), providing on-chip voltage and temperature monitoring.
The Versal SysMon measures up to 160 supply voltages and reads up to
64 temperature satellites distributed across the SoC. The hardware
also provides aggregated device temperature registers: the current
max and min across all active satellites, and peak/trough values
recorded since last hardware reset. The device can be accessed via
memory-mapped I/O or via an I2C interface.
The driver is split into a bus-agnostic core module using the regmap
API, an MMIO platform driver, and an I2C driver. This allows the
same IIO logic to be shared across different bus transports.
Previous submissions:
v4: https://lore.kernel.org/all/20260606051707.535281-1-salih.erim@amd.com/
v3: https://lore.kernel.org/all/20260527114211.174288-1-salih.erim@amd.com/
v2: https://lore.kernel.org/all/20260502111951.538488-1-salih.erim@amd.com/
v1: https://lore.kernel.org/all/cover.1757061697.git.michal.simek@amd.com/
Changes in v5:
- Core: add err.h include (IWYU) (Andy)
- Core: drop (int) cast on MILLI in scale assignment (Andy)
- Core: sign_extend32() instead of (s16) cast (Andy)
- Core: remove unneeded parentheses in voltage address
calculation (Andy)
- Core: drop NULL checks before fwnode_get_child_node_count
(NULL-aware) (Andy)
- Core: nested size_add() for overflow-safe allocation (Andy)
- Core: if (ret) instead of if (ret < 0) for fwnode property
reads (Andy)
- Core: remove outer parentheses in satellite address
calculation (Andy)
- Core: loop index declared in for() scope (Andy)
- MMIO: add err.h, types.h includes (IWYU) (Andy)
- Header: remove unused types.h include and struct iio_dev
forward declaration at P2 stage (Andy)
- I2C: add err.h, mod_devicetable.h includes (IWYU) (Andy)
- Events: clamp() instead of clamp_t() (Andy)
- Events: regmap_assign_bits() instead of separate set/clear (Andy)
- Events: remove unneeded parentheses (2 places) (Andy)
- Events: for_each_set_bit on single line (Andy)
- Events: regmap_clear_bits() instead of regmap_update_bits() (Andy)
- Events: simplify unmask XOR to ~status & masked_temp (Andy)
- Events: add comment explaining unmask &= ~temp_mask logic (Andy)
- Events: split container_of across two lines (Andy)
- Events: move ISR write after !isr check (Andy)
- Events: unsigned int for init_hysteresis address param (Andy)
- Events: add comment explaining error check policy in
worker/IRQ (Andy)
- Events: nested size_add() for overflow-safe allocation (Andy)
- Events: propagate negative from fwnode_irq_get() for
EPROBE_DEFER (Andy)
- Events: pass irq instead of has_irq to sysmon_parse_fw (Andy)
- Oversampling: remove unneeded parentheses (Andy)
- Oversampling: use struct regmap *map local variable (Andy)
- Oversampling: switch instead of redundant if/if on
channel_type (Andy)
- Oversampling: add CONFIG register readback fence after
oversampling update to prevent NoC bus hang from posted
writes (found during hardware stress testing)
Changes in v4:
- Core: temperature channels use RAW + SCALE (IIO_VAL_FRACTIONAL,
1000/128) instead of PROCESSED; voltage channels use PROCESSED
only, drop RAW; drop scan_type from all channel macros (Jonathan)
- Core: move __free(fwnode_handle) declarations down to just
above use; devm_regmap_init() on one line; lock comment
describes RMW sequences and cached state (Jonathan)
- Events: merge event channels into static temp array -- two
arrays (with/without events) selected by has_irq; event-only
channels have no info_mask (Jonathan)
- Events: blank lines, fit under 80 chars, default returns error,
return early in each case, guard(spinlock) in IRQ handler
(Jonathan)
- Events: take irq_lock in write_event_config for temp_mask
updates (Sashiko)
- I2C: replace enum with defines, use unaligned accessors for
data and register offset packing, named initializer in
i2c_device_id (Jonathan)
- I2C: drop bitfield.h, add unaligned.h
- Oversampling: return directly, remove else after early returns,
rename mask defines, blank lines (Jonathan)
- Oversampling: move oversampling read inside guard(mutex) scope
- Fix v2 lore link in cover letter
Changes in v3:
- DT binding: single compatible, voltage-channels rename, single
quotes, drop label/bipolar/xlnx,aie-temp (Krzysztof)
- Core: IWYU throughout, __free(fwnode_handle), sign_extend32(),
size_add(), dev_err_probe(), s16 param, remove (int) casts,
drop SYSMON_MILLI in favor of (int)MILLI, rename _ext to _name
in SYSMON_CHAN_TEMP macro (Andy, Jonathan)
- Core: fwnode_irq_get() moved to core_probe, remove sysmon->dev/
indio_dev/irq from struct, describe protected data in lock
comment, add RAW+PROCESSED comment (Jonathan)
- I2C: IWYU, remove wrapper struct, explicit enum values, sizeof()
for buffers, = { } initializers, adapt to core_probe interface
change (Andy, Krzysztof)
- Events: IWYU, FIELD_GET/FIELD_PREP, regmap_set/clear_bits,
clamp_t, !!, IRQ_RETVAL(), devm_delayed_work_autocancel,
loop var scope, error checks, remove redundant else, logical
param splits, spinlock safety comment (Andy)
- Events: hysteresis rework -- store as millicelsius, hardcode
ALARM_CONFIG to hysteresis mode, compute lower threshold from
(upper - hysteresis), remove falling threshold for temperature,
single event spec per channel with IIO_EV_DIR_RISING, push
IIO_EV_DIR_RISING for temp and IIO_EV_DIR_EITHER for voltage
(Jonathan)
Tested on VCK190 (single SLR, MMIO path, 7 supplies, 10 temperature
satellites) and VPK180 (System Controller, I2C path, 7 supplies).
A follow-up series will add thermal zone integration, secure firmware
access, and I2C remote monitoring.
Salih Erim (5):
dt-bindings: iio: adc: add xlnx,versal-sysmon binding
iio: adc: add Versal SysMon driver
iio: adc: versal-sysmon: add I2C driver
iio: adc: versal-sysmon: add threshold event support
iio: adc: versal-sysmon: add oversampling support
.../bindings/iio/adc/xlnx,versal-sysmon.yaml | 154 +++
MAINTAINERS | 7 +
drivers/iio/adc/Kconfig | 33 +
drivers/iio/adc/Makefile | 3 +
drivers/iio/adc/versal-sysmon-core.c | 1092 +++++++++++++++++
drivers/iio/adc/versal-sysmon-i2c.c | 127 ++
drivers/iio/adc/versal-sysmon.c | 93 ++
drivers/iio/adc/versal-sysmon.h | 129 ++
8 files changed, 1638 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
create mode 100644 drivers/iio/adc/versal-sysmon-core.c
create mode 100644 drivers/iio/adc/versal-sysmon-i2c.c
create mode 100644 drivers/iio/adc/versal-sysmon.c
create mode 100644 drivers/iio/adc/versal-sysmon.h
--
2.48.1
^ permalink raw reply
* [PATCH v5 3/5] iio: adc: versal-sysmon: add I2C driver
From: Salih Erim @ 2026-06-08 18:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim
In-Reply-To: <20260608183801.1257051-1-salih.erim@amd.com>
Add an I2C transport driver for the Versal SysMon block. The SysMon
provides an I2C slave interface that allows an external master to
read voltage and temperature measurements through the same register
map used by the MMIO path.
The I2C command frame is an 8-byte structure containing a 4-byte data
payload, a 2-byte register offset, and a 1-byte instruction field.
Read operations send the frame with a read instruction, then receive
a 4-byte response containing the register value.
Events are not supported on the I2C path because there is no
interrupt line and the I2C regmap backend cannot be called from
atomic context.
Co-developed-by: Conall O'Griofa <conall.ogriofa@amd.com>
Signed-off-by: Conall O'Griofa <conall.ogriofa@amd.com>
Signed-off-by: Salih Erim <salih.erim@amd.com>
---
Changes in v5:
- Add err.h, mod_devicetable.h includes (IWYU) (Andy)
Changes in v4:
- Replace enum with defines for I2C frame offsets (Jonathan)
- Use get_unaligned_le32() for read data reassembly (Jonathan)
- Use put_unaligned_le32/le16() for write data and register offset
packing (Jonathan)
- Named initializer in i2c_device_id (Jonathan)
- Drop bitfield.h, add unaligned.h (FIELD_GET/FIELD_PREP replaced
by unaligned accessors)
Changes in v3:
- IWYU: fix includes (Andy)
- Enum: assign all values explicitly for HW-mapped fields (Andy)
- Remove sysmon_i2c wrapper struct, pass i2c_client directly
(Andy)
- Use sizeof() for I2C buffer lengths instead of defines (Andy)
- Use = { } instead of = { 0 } for initializers (Andy)
- Use single compatible xlnx,versal-sysmon (Krzysztof)
- Adapt to core_probe interface change: irq moved to core,
remove irq parameter from bus driver (Jonathan)
Changes in v2:
- New patch (I2C was deferred to Series B in v1)
- Uses regmap API with custom I2C read/write callbacks
- Shares core module with MMIO driver via sysmon_core_probe()
- No event support (I2C has no interrupt line)
- Separate VERSAL_SYSMON_I2C Kconfig symbol
- Reverse Christmas Tree variable ordering in read/write functions
drivers/iio/adc/Kconfig | 13 +++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/versal-sysmon-i2c.c | 127 ++++++++++++++++++++++++++++
3 files changed, 141 insertions(+)
create mode 100644 drivers/iio/adc/versal-sysmon-i2c.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index c7f19057484..8f9fc9de74a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1963,6 +1963,19 @@ config VERSAL_SYSMON
To compile this driver as a module, choose M here: the module
will be called versal-sysmon.
+config VERSAL_SYSMON_I2C
+ tristate "AMD Versal SysMon I2C driver"
+ depends on I2C
+ select VERSAL_SYSMON_CORE
+ help
+ Say yes here to have support for the AMD/Xilinx Versal System
+ Monitor (SysMon) via I2C interface. This driver enables voltage
+ and temperature monitoring when the Versal chip has SysMon
+ configured with I2C access.
+
+ To compile this driver as a module, choose M here: the module
+ will be called versal-sysmon-i2c.
+
config VF610_ADC
tristate "Freescale vf610 ADC driver"
depends on HAS_IOMEM
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d7696b1b157..5abb611fe46 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -169,6 +169,7 @@ obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
obj-$(CONFIG_VERSAL_SYSMON_CORE) += versal-sysmon-core.o
obj-$(CONFIG_VERSAL_SYSMON) += versal-sysmon.o
+obj-$(CONFIG_VERSAL_SYSMON_I2C) += versal-sysmon-i2c.o
obj-$(CONFIG_VF610_ADC) += vf610_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
new file mode 100644
index 00000000000..1d1e97e1931
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon-i2c.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Versal SysMon I2C driver
+ *
+ * Copyright (C) 2023 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/unaligned.h>
+
+#include "versal-sysmon.h"
+
+#define SYSMON_I2C_INSTR_READ BIT(2)
+#define SYSMON_I2C_INSTR_WRITE BIT(3)
+
+/*
+ * I2C command frame layout (8 bytes):
+ * [0..3] data payload (little-endian u32)
+ * [4..5] register offset >> 2 (little-endian u16)
+ * [6] instruction (read/write)
+ * [7] reserved
+ */
+#define SYSMON_I2C_DATA_OFS 0
+#define SYSMON_I2C_REG_OFS 4
+#define SYSMON_I2C_INSTR_OFS 6
+
+static int sysmon_i2c_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct i2c_client *client = context;
+ u8 write_buf[8] = { };
+ u8 read_buf[4];
+ int ret;
+
+ put_unaligned_le16(reg >> 2, &write_buf[SYSMON_I2C_REG_OFS]);
+ write_buf[SYSMON_I2C_INSTR_OFS] = SYSMON_I2C_INSTR_READ;
+
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+ if (ret != sizeof(write_buf))
+ return -EIO;
+
+ ret = i2c_master_recv(client, read_buf, sizeof(read_buf));
+ if (ret < 0)
+ return ret;
+ if (ret != sizeof(read_buf))
+ return -EIO;
+
+ *val = get_unaligned_le32(read_buf);
+
+ return 0;
+}
+
+static int sysmon_i2c_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct i2c_client *client = context;
+ u8 write_buf[8] = { };
+ int ret;
+
+ put_unaligned_le32(val, &write_buf[SYSMON_I2C_DATA_OFS]);
+ put_unaligned_le16(reg >> 2, &write_buf[SYSMON_I2C_REG_OFS]);
+ write_buf[SYSMON_I2C_INSTR_OFS] = SYSMON_I2C_INSTR_WRITE;
+
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+ if (ret != sizeof(write_buf))
+ return -EIO;
+
+ return 0;
+}
+
+static const struct regmap_config sysmon_i2c_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = SYSMON_REG_STRIDE,
+ .max_register = SYSMON_MAX_REG,
+ .reg_read = sysmon_i2c_reg_read,
+ .reg_write = sysmon_i2c_reg_write,
+};
+
+static int sysmon_i2c_probe(struct i2c_client *client)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init(&client->dev, NULL, client,
+ &sysmon_i2c_regmap_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ /* I2C has no IRQ connection; events are not supported */
+ return sysmon_core_probe(&client->dev, regmap);
+}
+
+static const struct of_device_id sysmon_i2c_of_match_table[] = {
+ { .compatible = "xlnx,versal-sysmon" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, sysmon_i2c_of_match_table);
+
+static const struct i2c_device_id sysmon_i2c_id_table[] = {
+ { .name = "versal-sysmon" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, sysmon_i2c_id_table);
+
+static struct i2c_driver sysmon_i2c_driver = {
+ .probe = sysmon_i2c_probe,
+ .driver = {
+ .name = "versal-sysmon-i2c",
+ .of_match_table = sysmon_i2c_of_match_table,
+ },
+ .id_table = sysmon_i2c_id_table,
+};
+module_i2c_driver(sysmon_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Versal SysMon I2C Driver");
+MODULE_AUTHOR("Conall O'Griofa <conall.ogriofa@amd.com>");
+MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");
--
2.48.1
^ permalink raw reply related
* [PATCH v5 1/5] dt-bindings: iio: adc: add xlnx,versal-sysmon binding
From: Salih Erim @ 2026-06-08 18:37 UTC (permalink / raw)
To: jic23, andy
Cc: dlechner, nuno.sa, robh, krzk+dt, conor+dt, conall.ogriofa,
michal.simek, linux, erimsalih, linux-iio, devicetree,
linux-kernel, Salih Erim, Krzysztof Kozlowski
In-Reply-To: <20260608183801.1257051-1-salih.erim@amd.com>
Add devicetree binding for the AMD/Xilinx Versal System Monitor (SysMon).
The Versal SysMon is the successor to the Zynq UltraScale+ AMS block,
providing on-chip voltage and temperature monitoring. The hardware
supports up to 160 supply voltage measurement points and up to 64
temperature satellites distributed across the SoC, with configurable
threshold alarms and oversampling. The device can be accessed via
memory-mapped I/O or via an I2C interface.
Supply and temperature channels are described as child nodes under
container nodes, referencing the standard adc.yaml binding for
channel properties.
Co-developed-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Salih Erim <salih.erim@amd.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
Changes in v5:
- No code changes
Changes in v4:
- Add Reviewed-by tag from Krzysztof Kozlowski
Changes in v3:
- Use single compatible (xlnx,versal-sysmon only), remove
xlnx,versal-sysmon-i2c (Krzysztof)
- Rename supply-channels container to voltage-channels (Krzysztof)
- Use single quotes in patternProperties regex (Krzysztof)
- Drop label description from channel properties (Krzysztof)
- Drop bipolar from channel properties (Krzysztof)
- Remove xlnx,aie-temp property from binding and example (Krzysztof)
Changes in v2:
- Restructured to container nodes (supply-channels, temperature-channels)
with channel@N children referencing adc.yaml
- Added xlnx,versal-sysmon-i2c compatible
- Descriptions rewritten to describe hardware only
- Example simplified to #address-cells = <1>
- Interrupt example uses GIC_SPI/IRQ_TYPE_LEVEL_HIGH constants
- Commit description explains hardware context instead of schema layout
- reg required for both MMIO and I2C, interrupts optional
- Hex unit-addresses (channel@a not channel@10) per DTSpec
- patternProperties regex updated to accept hex digits [0-9a-f]
- Example trimmed to minimal variants (one basic + one bipolar supply,
one AIE temperature channel)
.../bindings/iio/adc/xlnx,versal-sysmon.yaml | 154 ++++++++++++++++++
1 file changed, 154 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
diff --git a/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml b/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
new file mode 100644
index 00000000000..1ad58e3d616
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
@@ -0,0 +1,154 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/xlnx,versal-sysmon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD/Xilinx Versal System Monitor
+
+maintainers:
+ - Salih Erim <salih.erim@amd.com>
+
+description:
+ The AMD/Xilinx Versal System Monitor (SysMon) is the successor to the
+ Zynq UltraScale+ AMS block. It provides on-chip voltage and temperature
+ monitoring with up to 160 voltage measurement points and up to
+ 64 temperature satellites distributed across the SoC. The hardware
+ supports configurable threshold alarms and oversampling. The device
+ can be accessed via memory-mapped I/O or via an I2C interface.
+
+properties:
+ compatible:
+ const: xlnx,versal-sysmon
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ '#io-channel-cells':
+ const: 1
+
+ voltage-channels:
+ type: object
+ description:
+ Container for voltage measurement channels.
+
+ properties:
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ patternProperties:
+ '^channel@([0-9a-f]|[1-9][0-9a-f])$':
+ $ref: adc.yaml
+
+ description:
+ Measures a voltage rail. The register index and rail
+ name are assigned by the hardware design tool (Vivado).
+
+ properties:
+ reg:
+ minimum: 0
+ maximum: 159
+ description:
+ Voltage measurement register index assigned by the hardware
+ design tool.
+
+ required:
+ - reg
+ - label
+
+ unevaluatedProperties: false
+
+ required:
+ - '#address-cells'
+ - '#size-cells'
+
+ additionalProperties: false
+
+ temperature-channels:
+ type: object
+ description:
+ Container for temperature satellite measurement channels.
+
+ properties:
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ patternProperties:
+ '^channel@([1-9a-f]|[1-3][0-9a-f]|40)$':
+ $ref: adc.yaml
+
+ description:
+ Reads a temperature satellite sensor. Each satellite monitors
+ a specific region of the SoC die.
+
+ properties:
+ reg:
+ minimum: 1
+ maximum: 64
+ description:
+ Temperature satellite number (1-based hardware index).
+
+ required:
+ - reg
+ - label
+
+ unevaluatedProperties: false
+
+ required:
+ - '#address-cells'
+ - '#size-cells'
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ sysmon@f1270000 {
+ compatible = "xlnx,versal-sysmon";
+ reg = <0xf1270000 0x4000>;
+ interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>;
+ #io-channel-cells = <1>;
+
+ voltage-channels {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 {
+ reg = <0>;
+ label = "vccaux";
+ };
+
+ channel@3 {
+ reg = <3>;
+ label = "vcc_ram";
+ bipolar;
+ };
+ };
+
+ temperature-channels {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@a {
+ reg = <10>;
+ label = "aie-temp-ch1";
+ };
+ };
+ };
--
2.48.1
^ permalink raw reply related
* Re: [PATCH 3/7] iio: light: hid-sensor-als: use u32 instead of unsigned
From: Jonathan Cameron @ 2026-06-08 18:34 UTC (permalink / raw)
To: Joshua Crofts
Cc: Sanjay Chitroda, Jiri Kosina, Srinivas Pandruvada, David Lechner,
Nuno Sá, Andy Shevchenko, linux-input, linux-iio,
linux-kernel
In-Reply-To: <CALoEA-yasQhXE9o-iYL5h8w0wPMrP4f-iva-YOJ2KZV9UuGe3g@mail.gmail.com>
On Mon, 8 Jun 2026 08:41:07 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> On Sat, 6 Jun 2026 at 14:19, Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
> >
> > From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> >
> > Prefer 'u32' instead of bare 'unsigned' variable to improve code
> > clarity and consistency with kernel style.
> >
> > Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> > ---
> > drivers/iio/light/hid-sensor-als.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> > index d72e260b8266..ae2fda8dc500 100644
> > --- a/drivers/iio/light/hid-sensor-als.c
> > +++ b/drivers/iio/light/hid-sensor-als.c
> > @@ -241,7 +241,7 @@ static const struct iio_info als_info = {
> >
> > /* Callback handler to send event after all samples are received and captured */
> > static int als_proc_event(struct hid_sensor_hub_device *hsdev,
> > - unsigned usage_id,
> > + u32 usage_id,
> > void *priv)
>
> Code-wise fine, however looking at this function, usage_id is never
> actually used,
> though not sure whether removing it would break the ABI... up to you
> to check it.
>
It is a callback so signature needs to match.
^ permalink raw reply
* Re: [PATCH 0/3] iio: adc: Extend ti-ads1100 driver
From: Jonathan Cameron @ 2026-06-08 18:25 UTC (permalink / raw)
To: Jakub Szczudlo
Cc: linux-iio, dlechner, nuno.sa, andy, marcelo.schmitt, robh,
krzk+dt, conor+dt, mike.looijmans, devicetree, linux-kernel,
jorge.marques, antoniu.miclaus, mazziesaccount, jishnu.prakash,
duje, wens, sakari.ailus, linusw
In-Reply-To: <20260607183542.368184-1-jakubszczudlo40@gmail.com>
On Sun, 7 Jun 2026 20:35:39 +0200
Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
> Extend ADS 1100 driver to support ADS1110, which is a pin-to-pin
> compatible device with higher resolution. This patch also updates the
> device tree bindings and Kconfig description to reflect the new
> supported device.
>
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
Look at your series title vs others that add support of new
devices to an existing driver. It needs to be a lot more
specific than 'extend'.
J
> ---
> jszczudlo (3):
> dt-bindings: iio: adc: Update title and enum
> iio: adc: Update Kconfig description for TI_ADS1100
> iio: adc: Add ti-ads1110 support to ti-ads1100 driver
>
> .../bindings/iio/adc/ti,ads1100.yaml | 3 +-
> drivers/iio/adc/Kconfig | 6 +-
> drivers/iio/adc/ti-ads1100.c | 165 +++++++++++++-----
> 3 files changed, 131 insertions(+), 43 deletions(-)
>
^ permalink raw reply
* Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver
From: Jonathan Cameron @ 2026-06-08 18:23 UTC (permalink / raw)
To: Jakub Szczudlo
Cc: linux-iio, dlechner, nuno.sa, andy, marcelo.schmitt, robh,
krzk+dt, conor+dt, mike.looijmans, devicetree, linux-kernel,
jorge.marques, antoniu.miclaus, mazziesaccount, jishnu.prakash,
duje, wens, sakari.ailus, linusw
In-Reply-To: <20260607183542.368184-4-jakubszczudlo40@gmail.com>
On Sun, 7 Jun 2026 20:35:42 +0200
Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
> From: jszczudlo <jakubszczudlo40@gmail.com>
>
> add ADS1100 support
> make changing gain and datarate wait for new reading
> fix unbalanced regulator disable when removing in singleshot mode
>
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
Hi Jakub,
A few additional things for me.
Note that the IIO tree is closed for this cycle and even if it wasn't
don't rush on sending a new version (at least a few days for a patch like
this between versions). That will give more time for other reviewers
to spot things Joshua and I missed!
Thanks,
Jonathan
> ---
> drivers/iio/adc/ti-ads1100.c | 165 ++++++++++++++++++++++++++---------
> 1 file changed, 126 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index aa8946063c7d..11d6fe1e8abc 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -5,18 +5,15 @@
> * Copyright (c) 2023, Topic Embedded Products
> *
> * Datasheet: https://www.ti.com/lit/gpn/ads1100
> - * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
> + * IIO driver for ADS1100, ADS1000 and ADS1110 ADC 16-bit I2C
> */
>
> #include <linux/bitfield.h>
> #include <linux/bits.h>
> -#include <linux/cleanup.h>
> -#include <linux/delay.h>
> -#include <linux/module.h>
> -#include <linux/init.h>
> #include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> #include <linux/mutex.h>
> -#include <linux/property.h>
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/units.h>
> @@ -39,17 +36,39 @@
> #define ADS1100_SINGLESHOT ADS1100_CFG_SC
>
> #define ADS1100_SLEEP_DELAY_MS 2000
> +#define ADS1110_REFERENCE_VOLTAGE_MICROVOLT 2048000
Why not express this in milivolts and move the divisor for the
regulator into the place it is read?
> +
> +/* Timeout based on the minimum sample rate of 8 SPS (7500000us) */
> +#define ADS11x0_MAX_DRDY_TIMEOUT 7500000
>
> static const int ads1100_data_rate[] = { 128, 32, 16, 8 };
> +static const int ads1110_data_rate[] = { 240, 60, 30, 15 };
> static const int ads1100_data_rate_bits[] = { 12, 14, 15, 16 };
>
> +struct ads11x0_config {
> + const int *data_rate;
> + bool has_reference_voltage;
const char *name;
(see below)
> +};
> +
> +static const struct ads11x0_config ads1100_config = {
> + .data_rate = ads1100_data_rate,
> + .has_reference_voltage = false,
> +};
> +
> +static const struct ads11x0_config ads1110_config = {
> + .data_rate = ads1110_data_rate,
> + .has_reference_voltage = true,
> +};
> +
> struct ads1100_data {
> struct i2c_client *client;
> struct regulator *reg_vdd;
> struct mutex lock;
> int scale_avail[2 * 4]; /* 4 gain settings */
> u8 config;
> - bool supports_data_rate; /* Only the ADS1100 can select the rate */
> + bool supports_data_rate; /* Only the ADS1100/ADS1110 can select the rate */
> + bool has_reference_voltage; /* The ADS1110 has an internal reference, so fixed scale */
As suggested below
struct ads1100_config *config;
rather than fields to copy.
> + const int *data_rate;
> };
>
> static const struct iio_chan_spec ads1100_channel = {
> @@ -59,12 +78,6 @@ static const struct iio_chan_spec ads1100_channel = {
> BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
> .info_mask_shared_by_all_available =
> BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
> - .scan_type = {
> - .sign = 's',
> - .realbits = 16,
> - .storagebits = 16,
> - .endianness = IIO_CPU,
> - },
Unrelated change so separate patch.
> .datasheet_name = "AIN",
> };
>
> @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
> return 0;
> };
>
> +static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
> +{
> + if (data->has_reference_voltage)
> + return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
> + else
> + return regulator_get_voltage(data->reg_vdd);
> +}
> +
> +static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
> +{
> + return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
> +}
> +
> +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> +{
> + int ret;
> + u8 buffer[3];
> +
> + ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> + if (ret < sizeof(buffer)) {
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return 0;
> + }
> +
> + int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +
> + return return_val;
Commented on this in Joshua's thread.
> +}
> +
> +static int ads11x0_poll_data_ready(struct ads1100_data *data)
> +{
> + bool data_ready;
> + u8 buffer[3];
> + int datarate = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> + unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
Why that number?
> +
> + /* To be sure that polled value will have value after config change */
> + i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
Why no return value check? Even if it is just to ensure new data, nice
to know if the bus is falling over.
> +
> + return read_poll_timeout(ads11x0_new_data_ready, data_ready,
> + !data_ready, wait_time,
> + ADS11x0_MAX_DRDY_TIMEOUT, false, data);
> +}
> +
> static int ads1100_data_bits(struct ads1100_data *data)
> {
> return ads1100_data_rate_bits[FIELD_GET(ADS1100_DR_MASK, data->config)];
> @@ -105,9 +162,10 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>
> ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
>
> + pm_runtime_mark_last_busy(&data->client->dev);
See below. This is probably an issue with forward porting from an old kernel.
Make sure to check for things like this in upstream as you should have
wondered why something so obvious was missing! If it were missing this
would have been a fix (it's not missing).
> pm_runtime_put_autosuspend(&data->client->dev);
>
> - if (ret < 0) {
> + if (ret < sizeof(buffer)) {
> dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> return ret;
> }
> @@ -127,7 +185,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
> {
> int microvolts;
> int gain;
> -
> + int ret;
Blank line here.
> /* With Vdd between 2.7 and 5V, the scale is always below 1 */
> if (val)
> return -EINVAL;
> @@ -135,7 +193,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
> if (!val2)
> return -EINVAL;
>
> - microvolts = regulator_get_voltage(data->reg_vdd);
> + microvolts = ads11x0_get_voltage_microvolts(data);
> /*
> * val2 is in 'micro' units, n = val2 / 1000000
> * result must be millivolts, d = microvolts / 1000
> @@ -147,34 +205,49 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
> if (gain < BIT(0) || gain > BIT(3))
> return -EINVAL;
>
> + ret = pm_runtime_resume_and_get(&data->client->dev);
> + if (ret < 0)
> + return ret;
> +
> ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>
> - return 0;
> + ret = ads11x0_poll_data_ready(data);
As below - this applies to existing parts so if it makes sense, separate
patch before you add the new device support. That can have a description
that tells us why this is needed.
> +
> + pm_runtime_mark_last_busy(&data->client->dev);
> + pm_runtime_put_autosuspend(&data->client->dev);
> +
> + return ret;
> }
>
> 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 (data->data_rate[i] == rate) {
Flip logic.
if (data->data_rate[i] != rate)
continue;
ret = ...
reduces indent and generally simplifies things.
> + ret = pm_runtime_resume_and_get(&data->client->dev);
> + if (ret < 0)
> + return ret;
> +
> + ads1100_set_config_bits(data, ADS1100_DR_MASK,
> + FIELD_PREP(ADS1100_DR_MASK, i));
> + ret = ads11x0_poll_data_ready(data);
Why is the more complex handling needed for existing devices?
If it is then smells like it should be a precursor patch with that
well explained.
> +
> + pm_runtime_mark_last_busy(&data->client->dev);
> + pm_runtime_put_autosuspend(&data->client->dev);
Joshua called this out already. I guess you are forward porting form an old
kernel as for a while the mark_last_busy() has been called by
put_autosuspend().
> + return ret;
> + }
> }
>
> return -EINVAL;
> }
>
> -static int ads1100_get_vdd_millivolts(struct ads1100_data *data)
> -{
> - return regulator_get_voltage(data->reg_vdd) / (MICRO / MILLI);
> -}
> -
> static void ads1100_calc_scale_avail(struct ads1100_data *data)
> {
> - int millivolts = ads1100_get_vdd_millivolts(data);
> + int millivolts = ads11x0_get_voltage_milivolts(data);
As below. No to the wild card. If you want to rename that's fine
but almost certainly wants to be a separate patch with a description
of why the rename makes sense.
> unsigned int i;
>
> for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
> @@ -196,7 +269,7 @@ static int ads1100_read_avail(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_SAMP_FREQ:
> *type = IIO_VAL_INT;
> - *vals = ads1100_data_rate;
> + *vals = data->data_rate;
> if (data->supports_data_rate)
> *length = ARRAY_SIZE(ads1100_data_rate);
> else
> @@ -233,12 +306,11 @@ static int ads1100_read_raw(struct iio_dev *indio_dev,
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> /* full-scale is the supply voltage in millivolts */
> - *val = ads1100_get_vdd_millivolts(data);
> + *val = ads11x0_get_voltage_milivolts(data);
Nope. We don't use wild card naming in IIO drivers. It goes wrong
too often as manufacturers have very inconsistent naming schemes.
+ The wild card is already wrong for the ads1000.
Sticking to existing prefix that matches the driver name is the
way to go.
> *val2 = 15 + FIELD_GET(ADS1100_PGA_MASK, data->config);
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_SAMP_FREQ:
> - *val = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK,
> - data->config)];
> + *val = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
Unrelated. Check your patches for stuff like this. It is noise
that makes for lower quality reviews.
> return IIO_VAL_INT;
> default:
> return -EINVAL;
> @@ -280,8 +352,8 @@ static int ads1100_setup(struct ads1100_data *data)
> return ret;
>
> ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> - if (ret < 0)
> - return ret;
> + if (ret < sizeof(buffer))
> + return -1;
Proper error returns and first check for ret < 0 as if it is you should return
that rather than eating the more informative error. This check is then only
on postive but too small values - so the rare case of a short recieve.
>
> /* Config register returned in third byte, strip away the busy status */
> data->config = buffer[2] & ~ADS1100_CFG_ST_BSY;
> @@ -292,9 +364,13 @@ static int ads1100_setup(struct ads1100_data *data)
> return 0;
> }
>
> -static void ads1100_reg_disable(void *reg)
> +static void ads1100_reg_disable(void *data)
> {
> - regulator_disable(reg);
> + struct ads1100_data *ads1100_data = data;
> +
> + /* Disable when not already disabled by the driver */
> + if (!(ads1100_data->config & ADS1100_CFG_SC))
Not obvious why regulator power is correlated with that register(?)
bit? I'd imagine this is where issue Joshua called out that sashiko
reported is coming from.
> + regulator_disable(ads1100_data->reg_vdd);
> }
>
> static void ads1100_disable_continuous(void *data)
> @@ -307,6 +383,7 @@ static int ads1100_probe(struct i2c_client *client)
> struct iio_dev *indio_dev;
> struct ads1100_data *data;
> struct device *dev = &client->dev;
> + const struct ads11x0_config *model;
> int ret;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
> return dev_err_probe(dev, ret,
> "Failed to enable vdd regulator\n");
>
> - ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> + ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
Unrelated change.
> if (ret)
> return ret;
>
> + model = device_get_match_data(dev);
> + if (!model)
> + return dev_err_probe(dev, ret,
> + "Can't set device data\n");
Discussion of this in Joshua's thread.
> +
The name should where possible reflect the actual device. Easy way to do
this is normally to put a string in the model data and use that for
iio_dev->name. There is a slightly ABI quirk that the driver doesn't do
this for the existing pair of support parts. We should probably leave
that alone, but we can at least do better for this one.
> + data->data_rate = model->data_rate;
> + data->has_reference_voltage = model->has_reference_voltage;
Probably just stash a pointer to model rather that copying elements.
The element copying route tends not to scale as a driver gets more complex.
> +
> ret = ads1100_setup(data);
> if (ret)
> return dev_err_probe(dev, ret,
> @@ -400,16 +485,18 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ads1100_pm_ops,
> NULL);
>
> static const struct i2c_device_id ads1100_id[] = {
> - { "ads1100" },
> - { "ads1000" },
> + { .name = "ads1100", .driver_data = (kernel_ulong_t)&ads1100_config },
> + { .name = "ads1000", .driver_data = (kernel_ulong_t)&ads1100_config },
> + { .name = "ads1110", .driver_data = (kernel_ulong_t)&ads1110_config },
> { }
> };
>
> MODULE_DEVICE_TABLE(i2c, ads1100_id);
>
> static const struct of_device_id ads1100_of_match[] = {
> - {.compatible = "ti,ads1100" },
> - {.compatible = "ti,ads1000" },
> + { .compatible = "ti,ads1100", .data = &ads1100_config },
> + { .compatible = "ti,ads1000", .data = &ads1100_config },
> + { .compatible = "ti,ads1110", .data = &ads1110_config },
As with binding, fix the ordering to be alphanumeric whilst we are here
> { }
> };
>
^ permalink raw reply
page: next (older)
- 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