* [PATCH v2 6/9] iio: mcp9600: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-27 21:41 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Johannes Berg, David Laight, Nuno Sá,
Andy Shevchenko, Ping-Ke Shih, Richard Cochran, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexandre Belloni, Yury Norov, Rasmus Villemoes, Hans de Goede,
Linus Walleij, Sakari Ailus, Salah Triki, Achim Gratz,
Ben Collins, x86, linux-kernel, linux-iio, linux-wireless, netdev,
linux-rtc
Cc: Yury Norov
In-Reply-To: <20260427214127.406067-1-ynorov@nvidia.com>
Switch from sign_extend32(FIELD_GET()) to the dedicated
FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
drivers/iio/temperature/mcp9600.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index aa42c2b1a369..69baf654c9c0 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -297,7 +297,7 @@ static int mcp9600_read_thresh(struct iio_dev *indio_dev,
* Temperature is stored in two’s complement format in
* bits(15:2), LSB is 0.25 degree celsius.
*/
- *val = sign_extend32(FIELD_GET(MCP9600_ALERT_LIMIT_MASK, ret), 13);
+ *val = FIELD_GET_SIGNED(MCP9600_ALERT_LIMIT_MASK, ret);
*val2 = 4;
return IIO_VAL_FRACTIONAL;
case IIO_EV_INFO_HYSTERESIS:
--
2.51.0
^ permalink raw reply related
* [PATCH v2 5/9] iio: pressure: bmp280: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-27 21:41 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Johannes Berg, David Laight, Nuno Sá,
Andy Shevchenko, Ping-Ke Shih, Richard Cochran, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexandre Belloni, Yury Norov, Rasmus Villemoes, Hans de Goede,
Linus Walleij, Sakari Ailus, Salah Triki, Achim Gratz,
Ben Collins, x86, linux-kernel, linux-iio, linux-wireless, netdev,
linux-rtc
Cc: Yury Norov
In-Reply-To: <20260427214127.406067-1-ynorov@nvidia.com>
Switch from sign_extend32(FIELD_GET()) to the dedicated
FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
drivers/iio/pressure/bmp280-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index d983ce9c0b99..f722aea16e0e 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -392,7 +392,7 @@ static int bme280_read_calib(struct bmp280_data *data)
h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW, tmp_1);
calib->H4 = sign_extend32(h4_upper | h4_lower, 11);
tmp_3 = get_unaligned_le16(&data->bme280_humid_cal_buf[H5]);
- calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, tmp_3), 11);
+ calib->H5 = FIELD_GET_SIGNED(BME280_COMP_H5_MASK, tmp_3);
calib->H6 = data->bme280_humid_cal_buf[H6];
return 0;
--
2.51.0
^ permalink raw reply related
* [PATCH v2 4/9] iio: magnetometer: yas530: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-27 21:41 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Johannes Berg, David Laight, Nuno Sá,
Andy Shevchenko, Ping-Ke Shih, Richard Cochran, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexandre Belloni, Yury Norov, Rasmus Villemoes, Hans de Goede,
Linus Walleij, Sakari Ailus, Salah Triki, Achim Gratz,
Ben Collins, x86, linux-kernel, linux-iio, linux-wireless, netdev,
linux-rtc
Cc: Yury Norov
In-Reply-To: <20260427214127.406067-1-ynorov@nvidia.com>
Switch from sign_extend32(FIELD_GET()) to the dedicated
FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
Reviewed-by: Linus Walleij <linusw@kernel.org>
Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
drivers/iio/magnetometer/yamaha-yas530.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c
index d49e37edcbed..6a80042602c6 100644
--- a/drivers/iio/magnetometer/yamaha-yas530.c
+++ b/drivers/iio/magnetometer/yamaha-yas530.c
@@ -859,9 +859,9 @@ static int yas530_get_calibration_data(struct yas5xx *yas5xx)
c->f[0] = FIELD_GET(GENMASK(22, 21), val);
c->f[1] = FIELD_GET(GENMASK(14, 13), val);
c->f[2] = FIELD_GET(GENMASK(6, 5), val);
- c->r[0] = sign_extend32(FIELD_GET(GENMASK(28, 23), val), 5);
- c->r[1] = sign_extend32(FIELD_GET(GENMASK(20, 15), val), 5);
- c->r[2] = sign_extend32(FIELD_GET(GENMASK(12, 7), val), 5);
+ c->r[0] = FIELD_GET_SIGNED(GENMASK(28, 23), val);
+ c->r[1] = FIELD_GET_SIGNED(GENMASK(20, 15), val);
+ c->r[2] = FIELD_GET_SIGNED(GENMASK(12, 7), val);
return 0;
}
@@ -914,9 +914,9 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx)
c->f[0] = FIELD_GET(GENMASK(24, 23), val);
c->f[1] = FIELD_GET(GENMASK(16, 15), val);
c->f[2] = FIELD_GET(GENMASK(8, 7), val);
- c->r[0] = sign_extend32(FIELD_GET(GENMASK(30, 25), val), 5);
- c->r[1] = sign_extend32(FIELD_GET(GENMASK(22, 17), val), 5);
- c->r[2] = sign_extend32(FIELD_GET(GENMASK(14, 7), val), 5);
+ c->r[0] = FIELD_GET_SIGNED(GENMASK(30, 25), val);
+ c->r[1] = FIELD_GET_SIGNED(GENMASK(22, 17), val);
+ c->r[2] = FIELD_GET_SIGNED(GENMASK(14, 7), val);
return 0;
}
--
2.51.0
^ permalink raw reply related
* [PATCH v2 3/9] iio: intel_dc_ti_adc: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-27 21:41 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Johannes Berg, David Laight, Nuno Sá,
Andy Shevchenko, Ping-Ke Shih, Richard Cochran, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexandre Belloni, Yury Norov, Rasmus Villemoes, Hans de Goede,
Linus Walleij, Sakari Ailus, Salah Triki, Achim Gratz,
Ben Collins, x86, linux-kernel, linux-iio, linux-wireless, netdev,
linux-rtc
Cc: Yury Norov
In-Reply-To: <20260427214127.406067-1-ynorov@nvidia.com>
Switch from sign_extend32(FIELD_GET()) to the dedicated
FIELD_GET_SIGNED() and don't provide the fields length explicitly.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
drivers/iio/adc/intel_dc_ti_adc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/intel_dc_ti_adc.c b/drivers/iio/adc/intel_dc_ti_adc.c
index 0fe34f1c338e..b5afad713e2d 100644
--- a/drivers/iio/adc/intel_dc_ti_adc.c
+++ b/drivers/iio/adc/intel_dc_ti_adc.c
@@ -290,8 +290,8 @@ static int dc_ti_adc_probe(struct platform_device *pdev)
if (ret)
return ret;
- info->vbat_zse = sign_extend32(FIELD_GET(DC_TI_VBAT_ZSE, val), 3);
- info->vbat_ge = sign_extend32(FIELD_GET(DC_TI_VBAT_GE, val), 3);
+ info->vbat_zse = FIELD_GET_SIGNED(DC_TI_VBAT_ZSE, val);
+ info->vbat_ge = FIELD_GET_SIGNED(DC_TI_VBAT_GE, val);
dev_dbg(dev, "vbat-zse %d vbat-ge %d\n", info->vbat_zse, info->vbat_ge);
--
2.51.0
^ permalink raw reply related
* [PATCH v2 2/9] x86/extable: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-27 21:41 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Johannes Berg, David Laight, Nuno Sá,
Andy Shevchenko, Ping-Ke Shih, Richard Cochran, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexandre Belloni, Yury Norov, Rasmus Villemoes, Hans de Goede,
Linus Walleij, Sakari Ailus, Salah Triki, Achim Gratz,
Ben Collins, x86, linux-kernel, linux-iio, linux-wireless, netdev,
linux-rtc
Cc: Yury Norov
In-Reply-To: <20260427214127.406067-1-ynorov@nvidia.com>
The EX_DATA register is laid out such that EX_DATA_IMM occupied MSB.
It's done to make sure that FIELD_GET() will sign-extend the IMM
field during extraction.
To enforce that, all EX_DATA masks are made signed integers. This
works, but relies on the particular implementation of FIELD_GET(),
i.e. masking then shifting, not vice versa; and the particular
placement of the fields in the register.
Switch to using the dedicated FIELD_GET_SIGNED(), and relax those
limitations.
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
arch/x86/include/asm/extable_fixup_types.h | 13 ++++---------
arch/x86/mm/extable.c | 2 +-
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
index 906b0d5541e8..fd0cfb472103 100644
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -2,15 +2,10 @@
#ifndef _ASM_X86_EXTABLE_FIXUP_TYPES_H
#define _ASM_X86_EXTABLE_FIXUP_TYPES_H
-/*
- * Our IMM is signed, as such it must live at the top end of the word. Also,
- * since C99 hex constants are of ambiguous type, force cast the mask to 'int'
- * so that FIELD_GET() will DTRT and sign extend the value when it extracts it.
- */
-#define EX_DATA_TYPE_MASK ((int)0x000000FF)
-#define EX_DATA_REG_MASK ((int)0x00000F00)
-#define EX_DATA_FLAG_MASK ((int)0x0000F000)
-#define EX_DATA_IMM_MASK ((int)0xFFFF0000)
+#define EX_DATA_TYPE_MASK (0x000000FF)
+#define EX_DATA_REG_MASK (0x00000F00)
+#define EX_DATA_FLAG_MASK (0x0000F000)
+#define EX_DATA_IMM_MASK (0xFFFF0000)
#define EX_DATA_REG_SHIFT 8
#define EX_DATA_FLAG_SHIFT 12
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 6b9ff1c6cafa..ceb8d03191ab 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -322,7 +322,7 @@ int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
type = FIELD_GET(EX_DATA_TYPE_MASK, e->data);
reg = FIELD_GET(EX_DATA_REG_MASK, e->data);
- imm = FIELD_GET(EX_DATA_IMM_MASK, e->data);
+ imm = FIELD_GET_SIGNED(EX_DATA_IMM_MASK, e->data);
switch (type) {
case EX_TYPE_DEFAULT:
--
2.51.0
^ permalink raw reply related
* [PATCH v2 1/9] bitfield: add FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-27 21:41 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Johannes Berg, David Laight, Nuno Sá,
Andy Shevchenko, Ping-Ke Shih, Richard Cochran, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexandre Belloni, Yury Norov, Rasmus Villemoes, Hans de Goede,
Linus Walleij, Sakari Ailus, Salah Triki, Achim Gratz,
Ben Collins, x86, linux-kernel, linux-iio, linux-wireless, netdev,
linux-rtc
Cc: Yury Norov
In-Reply-To: <20260427214127.406067-1-ynorov@nvidia.com>
The bitfields are designed in assumption that fields contain unsigned
integer values, thus extracting the values from the field implies
zero-extending.
Some drivers need to sign-extend their fields, and currently do it like:
dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
It's error-prone because it relies on user to provide the correct
index of the most significant bit and proper 32 vs 64 function flavor.
Thus, introduce a FIELD_GET_SIGNED(). With the new API, the above
snippet turns into the more convenient:
dc_re += FIELD_GET_SIGNED(0xfff000, tmp);
dc_im += FIELD_GET_SIGNED(0xfff, tmp);
It compiles (on x86_64) into just a couple instructions: shl and sar.
When the mask includes MSB, the '<< __builtin_clzll(mask)' part becomes
a NOP, and the compiler only emits a single sar:
long long foo(long long reg)
{
10: f3 0f 1e fa endbr64
return FIELD_GET_SIGNED(GENMASK_ULL(63, 60), reg);
14: 48 89 f8 mov %rdi,%rax
17: 48 c1 f8 3c sar $0x3c,%rax
}
32-bit code generation is equally well. On arm32:
long long foo(long long reg)
{
return FIELD_GET_SIGNED(0x00f00000ULL, reg);
}
generates:
foo(long long):
lsls r1, r0, #8
asrs r0, r1, #28
asrs r1, r1, #31
bx lr
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
include/linux/bitfield.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 54aeeef1f0ec..cd44013281c7 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -178,6 +178,22 @@
__FIELD_GET(_mask, _reg, "FIELD_GET: "); \
})
+/**
+ * FIELD_GET_SIGNED() - extract a signed bitfield element
+ * @mask: shifted mask defining the field's length and position
+ * @reg: value of entire bitfield
+ *
+ * Returns the sign-extended field specified by @_mask from the
+ * bitfield passed in as @_reg by masking and shifting it down.
+ */
+#define FIELD_GET_SIGNED(mask, reg) \
+ ({ \
+ __BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: "); \
+ ((__signed_scalar_typeof(mask)) \
+ (((long long)(reg) << __builtin_clzll(mask)) >> \
+ (__builtin_clzll(mask) + __builtin_ctzll(mask)))); \
+ })
+
/**
* FIELD_MODIFY() - modify a bitfield element
* @_mask: shifted mask defining the field's length and position
--
2.51.0
^ permalink raw reply related
* [PATCH v2 0/9] bitfield: add FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-27 21:41 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Johannes Berg, David Laight, Nuno Sá,
Andy Shevchenko, Ping-Ke Shih, Richard Cochran, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexandre Belloni, Yury Norov, Rasmus Villemoes, Hans de Goede,
Linus Walleij, Sakari Ailus, Salah Triki, Achim Gratz,
Ben Collins, x86, linux-kernel, linux-iio, linux-wireless, netdev,
linux-rtc
Cc: Yury Norov
The bitfields are designed in assumption that fields contain unsigned
integer values, thus extracting the values from the field implies
zero-extending.
Some drivers need to sign-extend their fields, and currently do it like:
dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
It's error-prone because it relies on user to provide the correct
index of the most significant bit and proper 32 vs 64 function flavor.
Thus, introduce a FIELD_GET_SIGNED(). With the new API, the above
snippet turns into the more convenient:
dc_re += FIELD_GET_SIGNED(0xfff000, tmp);
dc_im += FIELD_GET_SIGNED(0xfff, tmp);
It compiles (on x86_64) into just a couple instructions: shl and sar.
When the mask includes MSB, the '<< __builtin_clzll(mask)' part becomes
a NOP, and the compiler only emits a single sar:
long long foo(long long reg)
{
10: f3 0f 1e fa endbr64
return FIELD_GET_SIGNED(GENMASK_ULL(63, 60), reg);
14: 48 89 f8 mov %rdi,%rax
17: 48 c1 f8 3c sar $0x3c,%rax
}
32-bit code generation is equally well. On arm32:
long long foo(long long reg)
{
return FIELD_GET_SIGNED(0x00f00000ULL, reg);
}
generates:
foo(long long):
lsls r1, r0, #8
asrs r0, r1, #28
asrs r1, r1, #31
bx lr
Immutable branch:
https://github.com/norov/linux/pull/new/fgsv2
v1: https://lore.kernel.org/all/20260417173621.368914-1-ynorov@nvidia.com/
v2:
- more examples of the new API and code generation (Andy, David);
- fix #7 FIELD_GET() / FIELD_GET_SIGNED() typo (Ping-Ke);
- re-indent the macro (Andy, Peter);
Yury Norov (9):
bitfield: add FIELD_GET_SIGNED()
x86/extable: switch to using FIELD_GET_SIGNED()
iio: intel_dc_ti_adc: switch to using FIELD_GET_SIGNED()
iio: magnetometer: yas530: switch to using FIELD_GET_SIGNED()
iio: pressure: bmp280: switch to using FIELD_GET_SIGNED()
iio: mcp9600: switch to using FIELD_GET_SIGNED()
wifi: rtw89: switch to using FIELD_GET_SIGNED()
rtc: rv3032: switch to using FIELD_GET_SIGNED()
ptp: switch to using FIELD_GET_SIGNED()
arch/x86/include/asm/extable_fixup_types.h | 13 ++++---------
arch/x86/mm/extable.c | 2 +-
drivers/iio/adc/intel_dc_ti_adc.c | 4 ++--
drivers/iio/magnetometer/yamaha-yas530.c | 12 ++++++------
drivers/iio/pressure/bmp280-core.c | 2 +-
drivers/iio/temperature/mcp9600.c | 2 +-
.../net/wireless/realtek/rtw89/rtw8852a_rfk.c | 4 ++--
.../net/wireless/realtek/rtw89/rtw8852b_common.c | 4 ++--
.../net/wireless/realtek/rtw89/rtw8852b_rfk.c | 4 ++--
drivers/net/wireless/realtek/rtw89/rtw8852c.c | 4 ++--
drivers/ptp/ptp_fc3.c | 4 ++--
drivers/rtc/rtc-rv3032.c | 2 +-
include/linux/bitfield.h | 16 ++++++++++++++++
13 files changed, 42 insertions(+), 31 deletions(-)
--
2.51.0
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: David Laight @ 2026-04-27 9:42 UTC (permalink / raw)
To: Johannes Berg
Cc: Yury Norov, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86
In-Reply-To: <dc1b12e74b3f487eb531bf7def806f10d9a18b5e.camel@sipsolutions.net>
On Mon, 27 Apr 2026 10:29:21 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2026-04-24 at 12:35 -0400, Yury Norov wrote:
> > > I (personally) tend to prefer the "__MAKE_OP" versions (*_get_bits()
> > > etc.), in particular because WiFi and firmware interfaces deal a lot
> > > with fixed endian fields.
> >
> > I don't like that __MAKE_OP magic because whatever it generates is not
> > greppable. And because we disable strict type checks for kernel, but
> > this API claims to typecheck the parameters for the user. So, the
> > following compiles well:
> >
> > u64 val = 0;
> > ret = le16_get_bits(val, GENMASK(15, 10));
> >
> > I don't like autogeneration in general. We generate, for example,
> > be32_get_bits(), but never use it.
>
> That's a lot of "I don't like", but whatever.
>
>
> > We don't even know the level of the bloat.
>
> These are static inlines so there's no binary cost, and given that
> you're complaining about them being generated you can't really *also*
> complain about too much code...
There is a measurable compile-time cost for processing the definitions
of static inlines even when they aren't used.
While processing the definitions of #defines is cheap, processing the
expansions can be measurable. Particularly when expansions get nested
as often happens when GENMASK() is used as an argument to FIELD_xxx().
Even trivial #defines can affect compile times.
The 'size' check in READ_ONCE() (etc) costs about 1%.
(1% may not sound much, but find 10 of them and it becomes significant.)
I suspect a lot of that is adding the extra external function to hold
the error message.
David
>
> > > Any chance it'd be simple to generate u32_get_bits_signed() etc.? Could
> > > be especially useful for le32_get_bits_signed() for example, to have the
> > > endian conversion built-in unlike FIELD_GET_SIGNED().
> >
> > Maybe this:
> >
> > FIELD_GET_SIGNED(mask, le32_to_cpu(reg))
>
> Awful. "I don't like". But we rarely deal with bit-packed signed values
> anyway.
>
> johannes
>
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: Johannes Berg @ 2026-04-27 8:29 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
In-Reply-To: <aeub59FBHbCy-KKP@yury>
On Fri, 2026-04-24 at 12:35 -0400, Yury Norov wrote:
> > I (personally) tend to prefer the "__MAKE_OP" versions (*_get_bits()
> > etc.), in particular because WiFi and firmware interfaces deal a lot
> > with fixed endian fields.
>
> I don't like that __MAKE_OP magic because whatever it generates is not
> greppable. And because we disable strict type checks for kernel, but
> this API claims to typecheck the parameters for the user. So, the
> following compiles well:
>
> u64 val = 0;
> ret = le16_get_bits(val, GENMASK(15, 10));
>
> I don't like autogeneration in general. We generate, for example,
> be32_get_bits(), but never use it.
That's a lot of "I don't like", but whatever.
> We don't even know the level of the bloat.
These are static inlines so there's no binary cost, and given that
you're complaining about them being generated you can't really *also*
complain about too much code...
> > Any chance it'd be simple to generate u32_get_bits_signed() etc.? Could
> > be especially useful for le32_get_bits_signed() for example, to have the
> > endian conversion built-in unlike FIELD_GET_SIGNED().
>
> Maybe this:
>
> FIELD_GET_SIGNED(mask, le32_to_cpu(reg))
Awful. "I don't like". But we rarely deal with bit-packed signed values
anyway.
johannes
^ permalink raw reply
* RE: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
From: Biju Das @ 2026-04-27 6:07 UTC (permalink / raw)
To: Alexandre Belloni
Cc: John Madieu, ryan@bluewatersys.com, akpm@linux-foundation.org,
m.grzeschik@pengutronix.de, Denis.Osterland@diehl.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
john.madieu@gmail.com, open list:PIN CONTROLLER - RENESAS
In-Reply-To: <202604251836574d655eb1@mail.local>
Hi Alexandre Belloni,
Thanks for the feedback.
> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: 25 April 2026 19:37
> Subject: Re: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
>
> On 25/04/2026 16:39:16+0000, Biju Das wrote:
> > Hi John,
> >
> > > -----Original Message-----
> > > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > > Sent: 25 April 2026 16:50
> > > Subject: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with
> > > disable_irq_wake() on cleanup
> > >
> > > isl1208_setup_irq() calls enable_irq_wake() after a successful IRQ
> > > request, but the driver has no remove path that balances it.
> > > The driver is devm-only, so on unbind devm releases the IRQ - but
> > > enable_irq_wake() is not undone by IRQ release, so the wake count for that IRQ stays incremented.
> > >
> > > Each rebind therefore leaks one wake reference; the leak doubles for
> > > the chip variant that has a separate evdet IRQ, since
> > > isl1208_setup_irq() is then called twice during probe.
> >
> > Is removal of RTC device possible [1]?
> >
> > [1]
> > https://patchwork.ozlabs.org/project/rtc-linux/patch/20230922081208.26
> > 334-1-biju.das.jz@bp.renesas.com/#3195765
> >
>
> I'd say yes if this is not the RTC that is backing alarmtimer or alarmtimer is not compiled in the
> kernel.
obj-y += timeconv.o timecounter.o alarmtimer.o
Alarm timer is always compiled.
Now, On RZ/G3E SMARC EVK, only single RTC that have backing
alarmtimer support. On this platform, it cannot wake up the device
from deepsleep on RTC ALARM. But it can do timekeeping.
Cheers,
Biju
^ permalink raw reply
* RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
From: Biju Das @ 2026-04-27 5:51 UTC (permalink / raw)
To: John Madieu, alexandre.belloni@bootlin.com
Cc: ryan@bluewatersys.com, akpm@linux-foundation.org,
m.grzeschik@pengutronix.de, Denis.Osterland@diehl.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
john.madieu@gmail.com, open list:PIN CONTROLLER - RENESAS
In-Reply-To: <TY6PR01MB173776D008E8D0D5DAD622A0BFF292@TY6PR01MB17377.jpnprd01.prod.outlook.com>
Hi John,
> -----Original Message-----
> From: John Madieu <john.madieu.xa@bp.renesas.com>
> Sent: 26 April 2026 18:48
> To: Biju Das <biju.das.jz@bp.renesas.com>; alexandre.belloni@bootlin.com
> Cc: ryan@bluewatersys.com; akpm@linux-foundation.org; m.grzeschik@pengutronix.de;
> Denis.Osterland@diehl.com; linux-rtc@vger.kernel.org; linux-kernel@vger.kernel.org;
> john.madieu@gmail.com
> Subject: RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
>
> Hi Biju,
>
> Thanks fort he review.
>
> > -----Original Message-----
> > From: Biju Das <biju.das.jz@bp.renesas.com>
> > Sent: Samstag, 25. April 2026 19:16
> > To: John Madieu <john.madieu.xa@bp.renesas.com>;
> > alexandre.belloni@bootlin.com
> > Subject: RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as
> > irqreturn_t in IRQ handler
> >
> > Hi John,
> >
> > Thanks for the patch.
> >
> > > -----Original Message-----
> > > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > > Sent: 25 April 2026 16:50
> > > Subject: [PATCH 1/2] rtc: isl1208: Fix returning errno as
> > > irqreturn_t in IRQ handler
> > >
> > > isl1208_rtc_interrupt() is of irqreturn_t type but two paths return
> > > a negative i2c errno instead of an
> > > IRQ_* value:
> > >
> > > - The SR-poll loop on timeout: `return sr;`
> > > - The post-alarm cleanup path: `return err;`
> > >
> > > genirq's note_interrupt() casts the return to unsigned int and flags
> > > any value above IRQ_HANDLED|IRQ_WAKE_THREAD as a bogus return,
> > > logging "irq event N: bogus return value X" each time it happens.
> > >
> > > Return IRQ_NONE when the SR read failed (no progress, can't claim
> > > the
> > > interrupt) and IRQ_HANDLED when toggle_alarm failed.
> > >
> > > Fixes: cf044f0ed526 ("drivers/rtc/rtc-isl1208.c: add alarm support")
> > > Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
> > > ---
> > > drivers/rtc/rtc-isl1208.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> > > index f71a6bb77b2a..c93998c53e7a
> > > 100644
> > > --- a/drivers/rtc/rtc-isl1208.c
> > > +++ b/drivers/rtc/rtc-isl1208.c
> > > @@ -654,7 +654,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> > > if (time_after(jiffies, timeout)) {
> > > dev_err(&client->dev, "%s: reading SR failed\n",
> > > __func__);
> > > - return sr;
> > > + return IRQ_NONE;
> >
> > Maybe you can use a goto statement?? that will take care of handled
> > IRQ's
> >
> > goto err_irq:
> >
> > err_irq:
> > return handled ? IRQ_HANDLED : IRQ_NONE;
>
> Agreed. I'll do it your way in v2.
>
> >
> > > }
> > > }
> > >
> > > @@ -666,7 +666,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> > > /* Disable the alarm */
> > > err = isl1208_rtc_toggle_alarm(client, 0);
> > > if (err)
> > > - return err;
> > > + return IRQ_HANDLED;
> >
> > Same as above.
> >
>
> I'll set handled = 1 so that goto can return IRQ_HANDLED.
Looks this is change in behaviour compared to original code,
previous code does not set, handled = 1 for this path.
Cheers,
Biju
^ permalink raw reply
* Re: [GIT PULL] RTC for 7.1
From: pr-tracker-bot @ 2026-04-26 19:35 UTC (permalink / raw)
To: Alexandre Belloni; +Cc: Linus Torvalds, linux-rtc, linux-kernel
In-Reply-To: <20260425183335392f3a5c@mail.local>
The pull request you sent on Sat, 25 Apr 2026 20:33:35 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git tags/rtc-7.1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/211d5933141197b37a7501271e49e4b88540615f
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* RE: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
From: John Madieu @ 2026-04-26 18:23 UTC (permalink / raw)
To: Alexandre Belloni, Biju Das
Cc: ryan@bluewatersys.com, akpm@linux-foundation.org,
m.grzeschik@pengutronix.de, Denis.Osterland@diehl.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
john.madieu@gmail.com
In-Reply-To: <202604251836574d655eb1@mail.local>
Hi Biju, Alexandre,
Thanks for the feedback.
> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: Samstag, 25. April 2026 20:37
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Subject: Re: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with
> disable_irq_wake() on cleanup
>
> On 25/04/2026 16:39:16+0000, Biju Das wrote:
> > Hi John,
> >
> > > -----Original Message-----
> > > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > > Sent: 25 April 2026 16:50
> > > Subject: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with
> > > disable_irq_wake() on cleanup
> > >
> > > isl1208_setup_irq() calls enable_irq_wake() after a successful IRQ
> > > request, but the driver has no remove path that balances it.
> > > The driver is devm-only, so on unbind devm releases the IRQ - but
> > > enable_irq_wake() is not undone by IRQ release, so the wake count for
> that IRQ stays incremented.
> > >
> > > Each rebind therefore leaks one wake reference; the leak doubles for
> > > the chip variant that has a separate evdet IRQ, since
> > > isl1208_setup_irq() is then called twice during probe.
> >
> > Is removal of RTC device possible [1]?
> >
This patch addresses the per-IRQ wake refcount leak, which I
think is independent of whether alarmtimer is holding the RTC.
Found this by code inspection while working on patch [1/2]'s
issue.
> > [1]
> > https://patchwork.ozlabs.org/project/rtc-linux/patch/20230922081208.26
> > 334-1-biju.das.jz@bp.renesas.com/#3195765
> >
>
> I'd say yes if this is not the RTC that is backing alarmtimer or
> alarmtimer is not compiled in the kernel.
>
Thanks for the clarification. That said, looking around I noticed
most RTC drivers don't bother balancing enable_irq_wake() either.
Alexandre, do you want this patch as-is, or would you rather I
drop it? I'm fine either way.
Regards,
John
^ permalink raw reply
* RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
From: John Madieu @ 2026-04-26 17:48 UTC (permalink / raw)
To: Biju Das, alexandre.belloni@bootlin.com
Cc: ryan@bluewatersys.com, akpm@linux-foundation.org,
m.grzeschik@pengutronix.de, Denis.Osterland@diehl.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
john.madieu@gmail.com
In-Reply-To: <TY3PR01MB1134639F6A3A38551180B626386282@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Hi Biju,
Thanks fort he review.
> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Samstag, 25. April 2026 19:16
> To: John Madieu <john.madieu.xa@bp.renesas.com>;
> alexandre.belloni@bootlin.com
> Subject: RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t
> in IRQ handler
>
> Hi John,
>
> Thanks for the patch.
>
> > -----Original Message-----
> > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > Sent: 25 April 2026 16:50
> > Subject: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t
> > in IRQ handler
> >
> > isl1208_rtc_interrupt() is of irqreturn_t type but two paths return a
> > negative i2c errno instead of an
> > IRQ_* value:
> >
> > - The SR-poll loop on timeout: `return sr;`
> > - The post-alarm cleanup path: `return err;`
> >
> > genirq's note_interrupt() casts the return to unsigned int and flags
> > any value above IRQ_HANDLED|IRQ_WAKE_THREAD as a bogus return, logging
> > "irq event N: bogus return value X" each time it happens.
> >
> > Return IRQ_NONE when the SR read failed (no progress, can't claim the
> > interrupt) and IRQ_HANDLED when toggle_alarm failed.
> >
> > Fixes: cf044f0ed526 ("drivers/rtc/rtc-isl1208.c: add alarm support")
> > Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
> > ---
> > drivers/rtc/rtc-isl1208.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> > index f71a6bb77b2a..c93998c53e7a
> > 100644
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
> > @@ -654,7 +654,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> > if (time_after(jiffies, timeout)) {
> > dev_err(&client->dev, "%s: reading SR failed\n",
> > __func__);
> > - return sr;
> > + return IRQ_NONE;
>
> Maybe you can use a goto statement?? that will take care of handled IRQ's
>
> goto err_irq:
>
> err_irq:
> return handled ? IRQ_HANDLED : IRQ_NONE;
Agreed. I'll do it your way in v2.
>
> > }
> > }
> >
> > @@ -666,7 +666,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> > /* Disable the alarm */
> > err = isl1208_rtc_toggle_alarm(client, 0);
> > if (err)
> > - return err;
> > + return IRQ_HANDLED;
>
> Same as above.
>
I'll set handled = 1 so that goto can return IRQ_HANDLED.
Regards,
John
> Cheers,
> Biju
>
> >
> > fsleep(275);
> >
> > --
> > 2.25.1
^ permalink raw reply
* Re: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
From: Alexandre Belloni @ 2026-04-25 18:36 UTC (permalink / raw)
To: Biju Das
Cc: John Madieu, ryan@bluewatersys.com, akpm@linux-foundation.org,
m.grzeschik@pengutronix.de, Denis.Osterland@diehl.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
john.madieu@gmail.com
In-Reply-To: <TY3PR01MB1134607A936EAE3F7F2185D3086282@TY3PR01MB11346.jpnprd01.prod.outlook.com>
On 25/04/2026 16:39:16+0000, Biju Das wrote:
> Hi John,
>
> > -----Original Message-----
> > From: John Madieu <john.madieu.xa@bp.renesas.com>
> > Sent: 25 April 2026 16:50
> > Subject: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
> >
> > isl1208_setup_irq() calls enable_irq_wake() after a successful IRQ request, but the driver has no
> > remove path that balances it.
> > The driver is devm-only, so on unbind devm releases the IRQ - but enable_irq_wake() is not undone by
> > IRQ release, so the wake count for that IRQ stays incremented.
> >
> > Each rebind therefore leaks one wake reference; the leak doubles for the chip variant that has a
> > separate evdet IRQ, since
> > isl1208_setup_irq() is then called twice during probe.
>
> Is removal of RTC device possible [1]?
>
> [1]
> https://patchwork.ozlabs.org/project/rtc-linux/patch/20230922081208.26334-1-biju.das.jz@bp.renesas.com/#3195765
>
I'd say yes if this is not the RTC that is backing alarmtimer or
alarmtimer is not compiled in the kernel.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* [GIT PULL] RTC for 7.1
From: Alexandre Belloni @ 2026-04-25 18:33 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-rtc, linux-kernel
Hello Linus,
Here is the RTC subsystem pull request for 7.1. It is super late, I
hoped I could squeeze a few more patches in but I didn't have the time.
It is quite small and half of the changes are in device tree bindings.
The following changes since commit 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f:
Linux 7.0-rc1 (2026-02-22 13:18:59 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git tags/rtc-7.1
for you to fetch changes up to 0fedce7244e4b85c049ce579c87e298a1b0b811d:
rtc: abx80x: Disable alarm feature if no interrupt attached (2026-04-13 00:02:59 +0200)
----------------------------------------------------------------
RTC for 7.1
Subsystem:
- add data_race() in rtc_dev_poll()
Drivers:
- remove i2c_match_id usage
- abx80x: Disable alarm feature if no interrupt attached
- ti-k3: support resuming from IO DDR low power mode
----------------------------------------------------------------
Akashdeep Kaur (1):
rtc: ti-k3: Add support to resume from IO DDR low power mode
Andrew Davis (6):
rtc: abx80x: Remove use of i2c_match_id()
rtc: m41t80: Remove use of i2c_match_id()
rtc: pcf2127: Remove use of i2c_match_id()
rtc: rs5c372: Remove use of i2c_match_id()
rtc: rv8803: Remove use of i2c_match_id()
rtc: rx8025: Remove use of i2c_match_id()
Anthony Pighin (Nokia) (1):
rtc: abx80x: Disable alarm feature if no interrupt attached
Anushka Badhe (1):
dt-bindings: rtc: add olpc,xo1-rtc to trivial-rtc
Brian Masney (1):
rtc: pic32: allow driver to be compiled with COMPILE_TEST
Conor Dooley (1):
dt-bindings: rtc: mpfs-rtc: permit resets
Frieder Schrempf (1):
dt-bindings: rtc: microcrystal,rv3028: Allow to specify vdd-supply
Johan Hovold (1):
rtc: ntxec: fix OF node reference imbalance
Mauricio Faria de Oliveira (1):
rtc: add data_race() in rtc_dev_poll()
Otto Pflüger (1):
dt-bindings: rtc: sc2731: Add compatible for SC2730
Piyush Patle (1):
dt-bindings: rtc: isl12026: convert to YAML schema
Rafael J. Wysocki (1):
rtc: cmos: Use platform_get_irq_optional() in cmos_platform_probe()
Rosen Penev (1):
rtc: armada38x: zalloc + calloc to single allocation
Svyatoslav Ryhel (1):
rtc: max77686: convert to i2c_new_ancillary_device
.../devicetree/bindings/rtc/isil,isl12026.txt | 28 ----------
.../devicetree/bindings/rtc/isil,isl12026.yaml | 59 ++++++++++++++++++++++
.../bindings/rtc/microchip,mpfs-rtc.yaml | 3 ++
.../bindings/rtc/microcrystal,rv3028.yaml | 2 +
.../devicetree/bindings/rtc/olpc-xo1-rtc.txt | 5 --
.../devicetree/bindings/rtc/sprd,sc2731-rtc.yaml | 7 ++-
.../devicetree/bindings/rtc/trivial-rtc.yaml | 2 +
drivers/rtc/Kconfig | 2 +-
drivers/rtc/dev.c | 11 +++-
drivers/rtc/rtc-abx80x.c | 5 +-
drivers/rtc/rtc-armada38x.c | 9 +---
drivers/rtc/rtc-cmos.c | 13 ++++-
drivers/rtc/rtc-m41t80.c | 8 +--
drivers/rtc/rtc-max77686.c | 14 ++++-
drivers/rtc/rtc-ntxec.c | 2 +-
drivers/rtc/rtc-pcf2127.c | 23 +++------
drivers/rtc/rtc-rs5c372.c | 7 +--
drivers/rtc/rtc-rv8803.c | 8 +--
drivers/rtc/rtc-rx8025.c | 4 +-
drivers/rtc/rtc-ti-k3.c | 10 +++-
20 files changed, 132 insertions(+), 90 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/rtc/isil,isl12026.txt
create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl12026.yaml
delete mode 100644 Documentation/devicetree/bindings/rtc/olpc-xo1-rtc.txt
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* RE: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
From: Biju Das @ 2026-04-25 17:16 UTC (permalink / raw)
To: John Madieu, alexandre.belloni@bootlin.com
Cc: ryan@bluewatersys.com, akpm@linux-foundation.org,
m.grzeschik@pengutronix.de, Denis.Osterland@diehl.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
john.madieu@gmail.com
In-Reply-To: <20260425154959.2796261-2-john.madieu.xa@bp.renesas.com>
Hi John,
Thanks for the patch.
> -----Original Message-----
> From: John Madieu <john.madieu.xa@bp.renesas.com>
> Sent: 25 April 2026 16:50
> Subject: [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
>
> isl1208_rtc_interrupt() is of irqreturn_t type but two paths return a negative i2c errno instead of an
> IRQ_* value:
>
> - The SR-poll loop on timeout: `return sr;`
> - The post-alarm cleanup path: `return err;`
>
> genirq's note_interrupt() casts the return to unsigned int and flags any value above
> IRQ_HANDLED|IRQ_WAKE_THREAD as a bogus return, logging "irq event N: bogus return value X" each time it
> happens.
>
> Return IRQ_NONE when the SR read failed (no progress, can't claim the interrupt) and IRQ_HANDLED when
> toggle_alarm failed.
>
> Fixes: cf044f0ed526 ("drivers/rtc/rtc-isl1208.c: add alarm support")
> Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
> ---
> drivers/rtc/rtc-isl1208.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c index f71a6bb77b2a..c93998c53e7a
> 100644
> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -654,7 +654,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> if (time_after(jiffies, timeout)) {
> dev_err(&client->dev, "%s: reading SR failed\n",
> __func__);
> - return sr;
> + return IRQ_NONE;
Maybe you can use a goto statement?? that will take care of handled IRQ's
goto err_irq:
err_irq:
return handled ? IRQ_HANDLED : IRQ_NONE;
> }
> }
>
> @@ -666,7 +666,7 @@ isl1208_rtc_interrupt(int irq, void *data)
> /* Disable the alarm */
> err = isl1208_rtc_toggle_alarm(client, 0);
> if (err)
> - return err;
> + return IRQ_HANDLED;
Same as above.
Cheers,
Biju
>
> fsleep(275);
>
> --
> 2.25.1
^ permalink raw reply
* RE: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
From: Biju Das @ 2026-04-25 16:39 UTC (permalink / raw)
To: John Madieu, alexandre.belloni@bootlin.com
Cc: ryan@bluewatersys.com, akpm@linux-foundation.org,
m.grzeschik@pengutronix.de, Denis.Osterland@diehl.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
john.madieu@gmail.com
In-Reply-To: <20260425154959.2796261-3-john.madieu.xa@bp.renesas.com>
Hi John,
> -----Original Message-----
> From: John Madieu <john.madieu.xa@bp.renesas.com>
> Sent: 25 April 2026 16:50
> Subject: [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
>
> isl1208_setup_irq() calls enable_irq_wake() after a successful IRQ request, but the driver has no
> remove path that balances it.
> The driver is devm-only, so on unbind devm releases the IRQ - but enable_irq_wake() is not undone by
> IRQ release, so the wake count for that IRQ stays incremented.
>
> Each rebind therefore leaks one wake reference; the leak doubles for the chip variant that has a
> separate evdet IRQ, since
> isl1208_setup_irq() is then called twice during probe.
Is removal of RTC device possible [1]?
[1]
https://patchwork.ozlabs.org/project/rtc-linux/patch/20230922081208.26334-1-biju.das.jz@bp.renesas.com/#3195765
Cheers,
Biju
^ permalink raw reply
* [PATCH 2/2] rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on cleanup
From: John Madieu @ 2026-04-25 15:49 UTC (permalink / raw)
To: alexandre.belloni
Cc: ryan, akpm, m.grzeschik, Denis.Osterland, linux-rtc, linux-kernel,
biju.das.jz, john.madieu, John Madieu
In-Reply-To: <20260425154959.2796261-1-john.madieu.xa@bp.renesas.com>
isl1208_setup_irq() calls enable_irq_wake() after a successful
IRQ request, but the driver has no remove path that balances it.
The driver is devm-only, so on unbind devm releases the IRQ -
but enable_irq_wake() is not undone by IRQ release, so the wake
count for that IRQ stays incremented.
Each rebind therefore leaks one wake reference; the leak doubles
for the chip variant that has a separate evdet IRQ, since
isl1208_setup_irq() is then called twice during probe.
Register a devm action that calls disable_irq_wake() per IRQ.
While at it, check enable_irq_wake()'s return value:
on failure, propagate the error rather than silently registering
a disable action for an IRQ whose wake state was never enabled.
Fixes: 9ece7cd833a3 ("rtc: isl1208: Add "evdet" interrupt source for isl1219")
Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
---
drivers/rtc/rtc-isl1208.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index c93998c53e7a..1de80fc9c9c9 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -822,6 +822,11 @@ static const struct nvmem_config isl1208_nvmem_config = {
.reg_write = isl1208_nvmem_write,
};
+static void isl1208_disable_irq_wake_action(void *data)
+{
+ disable_irq_wake((unsigned long)data);
+}
+
static int isl1208_setup_irq(struct i2c_client *client, int irq)
{
int rc = devm_request_threaded_irq(&client->dev, irq, NULL,
@@ -831,7 +836,15 @@ static int isl1208_setup_irq(struct i2c_client *client, int irq)
client);
if (!rc) {
device_init_wakeup(&client->dev, true);
- enable_irq_wake(irq);
+ rc = enable_irq_wake(irq);
+ if (rc)
+ return rc;
+
+ rc = devm_add_action_or_reset(&client->dev,
+ isl1208_disable_irq_wake_action,
+ (void *)(unsigned long)irq);
+ if (rc)
+ return rc;
} else {
dev_err(&client->dev,
"Unable to request irq %d, no alarm support\n",
--
2.25.1
^ permalink raw reply related
* [PATCH 0/2] rtc: isl1208: Fix IRQ return value and wake reference leak
From: John Madieu @ 2026-04-25 15:49 UTC (permalink / raw)
To: alexandre.belloni
Cc: ryan, akpm, m.grzeschik, Denis.Osterland, linux-rtc, linux-kernel,
biju.das.jz, john.madieu, John Madieu
Hi all,
This series fixes two issues in rtc-isl1208. The first is
isl1208_rtc_interrupt() returning negative i2c errnos where
irqreturn_t is expected, which genirq flags as bogus and logs
on each IRQ. The second is a wake-reference leak: setup_irq()
calls enable_irq_wake() on success but the driver has no remove
path that balances it, so each rebind cycle leaks one wake
reference per IRQ.
John Madieu (2):
rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
rtc: isl1208: Balance enable_irq_wake() with disable_irq_wake() on
cleanup
drivers/rtc/rtc-isl1208.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH 1/2] rtc: isl1208: Fix returning errno as irqreturn_t in IRQ handler
From: John Madieu @ 2026-04-25 15:49 UTC (permalink / raw)
To: alexandre.belloni
Cc: ryan, akpm, m.grzeschik, Denis.Osterland, linux-rtc, linux-kernel,
biju.das.jz, john.madieu, John Madieu
In-Reply-To: <20260425154959.2796261-1-john.madieu.xa@bp.renesas.com>
isl1208_rtc_interrupt() is of irqreturn_t type but two paths
return a negative i2c errno instead of an IRQ_* value:
- The SR-poll loop on timeout: `return sr;`
- The post-alarm cleanup path: `return err;`
genirq's note_interrupt() casts the return to unsigned int and
flags any value above IRQ_HANDLED|IRQ_WAKE_THREAD as a bogus
return, logging "irq event N: bogus return value X" each time it
happens.
Return IRQ_NONE when the SR read failed (no progress, can't claim
the interrupt) and IRQ_HANDLED when toggle_alarm failed.
Fixes: cf044f0ed526 ("drivers/rtc/rtc-isl1208.c: add alarm support")
Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
---
drivers/rtc/rtc-isl1208.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index f71a6bb77b2a..c93998c53e7a 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -654,7 +654,7 @@ isl1208_rtc_interrupt(int irq, void *data)
if (time_after(jiffies, timeout)) {
dev_err(&client->dev, "%s: reading SR failed\n",
__func__);
- return sr;
+ return IRQ_NONE;
}
}
@@ -666,7 +666,7 @@ isl1208_rtc_interrupt(int irq, void *data)
/* Disable the alarm */
err = isl1208_rtc_toggle_alarm(client, 0);
if (err)
- return err;
+ return IRQ_HANDLED;
fsleep(275);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: David Laight @ 2026-04-24 21:37 UTC (permalink / raw)
To: Yury Norov
Cc: Johannes Berg, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Ping-Ke Shih, Richard Cochran, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexandre Belloni,
Yury Norov, Rasmus Villemoes, Hans de Goede, Linus Walleij,
Sakari Ailus, Salah Triki, Achim Gratz, Ben Collins, linux-kernel,
linux-iio, linux-wireless, netdev, linux-rtc
In-Reply-To: <aeub59FBHbCy-KKP@yury>
On Fri, 24 Apr 2026 12:35:51 -0400
Yury Norov <ynorov@nvidia.com> wrote:
...
> > Any chance it'd be simple to generate u32_get_bits_signed() etc.? Could
> > be especially useful for le32_get_bits_signed() for example, to have the
> > endian conversion built-in unlike FIELD_GET_SIGNED().
>
> Maybe this:
>
> x = FIELD_GET_SIGNED(mask, le32_to_cpu(reg))
But if you are going to follow it by:
x1 = FIELD_GET_SIGNED(mask1, le32_to_cpu(reg))
you really want to to the byteswap once, best as:
reg = le32_to_cpu(struct->member);
David
>
> Thanks,
> Yury
>
^ permalink raw reply
* Re: [PATCH 0/9] bitfield: add FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-24 17:10 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <aeuRMiws8zCdkGXX@yury>
On Fri, Apr 24, 2026 at 11:50:10AM -0400, Yury Norov wrote:
> On Fri, Apr 24, 2026 at 01:09:27PM +0100, Jonathan Cameron wrote:
> > On Fri, 17 Apr 2026 13:36:11 -0400
> > Yury Norov <ynorov@nvidia.com> wrote:
> >
> > > The bitfields are designed in assumption that fields contain unsigned
> > > integer values, thus extracting the values from the field implies
> > > zero-extending.
> > >
> > > Some drivers need to sign-extend their fields, and currently do it like:
> > >
> > > dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > > dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
> > >
> > > It's error-prone because it relies on user to provide the correct
> > > index of the most significant bit.
> > >
> > > This series adds a signed version of FIELD_GET(), which is the more
> > > convenient and compiles (on x86_64) to just a couple instructions:
> > > shl and sar.
> > >
> > > Patch #1 adds FIELD_GET_SIGNED(), and the rest of the series applies it
> > > tree-wide.
> > >
> >
> > Just a quick heads up that I'm beginning to assume that this series
> > will land in some form. If it does can we do it as an immutable branch
> > as I'm suggesting it gets used in some other patches in that should land
> > in the new cycle.
>
> I'm going to submit v2 soon, as seemingly the discussion is boiled
> down, and then will likely merge it with my tree. I'll create an
> immutable branch for you before the end of day.
Here it is:
https://github.com/norov/linux/pull/new/fgsv2
It builds well for me, but I'll wait for a while for robots feedback
before making it 'officially' immutable.
Thanks,
Yury
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-24 16:35 UTC (permalink / raw)
To: Johannes Berg
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
In-Reply-To: <6170788fcab2ec835597e3d7411928d36850c20a.camel@sipsolutions.net>
On Mon, Apr 20, 2026 at 10:43:08AM +0200, Johannes Berg wrote:
> On Fri, 2026-04-17 at 13:36 -0400, Yury Norov wrote:
> > The bitfields are designed in assumption that fields contain unsigned
> > integer values, thus extracting the values from the field implies
> > zero-extending.
> >
> > Some drivers need to sign-extend their fields, and currently do it like:
> >
> > dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
>
> That's indeed pretty awful...
>
>
> > +#define FIELD_GET_SIGNED(mask, reg) \
> >
>
> [...]
>
> I (personally) tend to prefer the "__MAKE_OP" versions (*_get_bits()
> etc.), in particular because WiFi and firmware interfaces deal a lot
> with fixed endian fields.
I don't like that __MAKE_OP magic because whatever it generates is not
greppable. And because we disable strict type checks for kernel, but
this API claims to typecheck the parameters for the user. So, the
following compiles well:
u64 val = 0;
ret = le16_get_bits(val, GENMASK(15, 10));
I don't like autogeneration in general. We generate, for example,
be32_get_bits(), but never use it. We don't even know the level of
the bloat.
> Any chance it'd be simple to generate u32_get_bits_signed() etc.? Could
> be especially useful for le32_get_bits_signed() for example, to have the
> endian conversion built-in unlike FIELD_GET_SIGNED().
Maybe this:
FIELD_GET_SIGNED(mask, le32_to_cpu(reg))
Thanks,
Yury
^ permalink raw reply
* Re: [PATCH 0/9] bitfield: add FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-24 15:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260424130927.349ad3ae@jic23-huawei>
On Fri, Apr 24, 2026 at 01:09:27PM +0100, Jonathan Cameron wrote:
> On Fri, 17 Apr 2026 13:36:11 -0400
> Yury Norov <ynorov@nvidia.com> wrote:
>
> > The bitfields are designed in assumption that fields contain unsigned
> > integer values, thus extracting the values from the field implies
> > zero-extending.
> >
> > Some drivers need to sign-extend their fields, and currently do it like:
> >
> > dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
> >
> > It's error-prone because it relies on user to provide the correct
> > index of the most significant bit.
> >
> > This series adds a signed version of FIELD_GET(), which is the more
> > convenient and compiles (on x86_64) to just a couple instructions:
> > shl and sar.
> >
> > Patch #1 adds FIELD_GET_SIGNED(), and the rest of the series applies it
> > tree-wide.
> >
>
> Just a quick heads up that I'm beginning to assume that this series
> will land in some form. If it does can we do it as an immutable branch
> as I'm suggesting it gets used in some other patches in that should land
> in the new cycle.
I'm going to submit v2 soon, as seemingly the discussion is boiled
down, and then will likely merge it with my tree. I'll create an
immutable branch for you before the end of day.
Thanks,
Yury
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox