Message-Id: <DG222YZ4WT33.21A5GG66K9X5O@garyguo.net>
Date: Fri, 30 Jan 2026 16:14:39 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Alexandre Courbot" <acourbot@...dia.com>, "Gary Guo" <gary@...yguo.net>
Cc: "Danilo Krummrich" <dakr@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Daniel Almeida" <daniel.almeida@...labora.com>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>, "Yury Norov" <yury.norov@...il.com>, "John
 Hubbard" <jhubbard@...dia.com>, "Alistair Popple" <apopple@...dia.com>,
 "Joel Fernandes" <joelagnelf@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 "Edwin Peer" <epeer@...dia.com>, "Eliot Courtney" <ecourtney@...dia.com>,
 "Dirk Behme" <dirk.behme@...bosch.com>, "Steven Price"
 <steven.price@....com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 5/7] rust: io: add `register!` macro

On Fri Jan 30, 2026 at 6:55 AM GMT, Alexandre Courbot wrote:
> On Thu Jan 29, 2026 at 11:10 PM JST, Gary Guo wrote:
>> On Thu Jan 29, 2026 at 8:00 AM GMT, Alexandre Courbot wrote:
>>> On Thu Jan 29, 2026 at 1:16 AM JST, Gary Guo wrote:
>>>> On Wed Jan 28, 2026 at 2:37 AM GMT, Alexandre Courbot wrote:
>>>>> Add a macro for defining hardware register types with I/O accessors.
>>>>>
>>>>> Each register field is represented as a `Bounded` of the appropriate bit
>>>>> width, ensuring field values are never silently truncated.
>>>>>
>>>>> Fields can optionally be converted to/from custom types, either fallibly
>>>>> or infallibly.
>>>>>
>>>>> The address of registers can be direct, relative, or indexed, supporting
>>>>> most of the patterns in which registers are arranged.
>>>>>
>>>>> Tested-by: Dirk Behme <dirk.behme@...bosch.com>
>>>>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>>>>> ---
>>>>>  rust/kernel/io.rs          |    1 +
>>>>>  rust/kernel/io/register.rs | 1287 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>  2 files changed, 1288 insertions(+)
>>>>>
>>>>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>>>>> index 056a3ec71647..112f43ecbf88 100644
>>>>> --- a/rust/kernel/io.rs
>>>>> +++ b/rust/kernel/io.rs
>>>>> @@ -11,6 +11,7 @@
>>>>>  
>>>>>  pub mod mem;
>>>>>  pub mod poll;
>>>>> +pub mod register;
>>>>>  pub mod resource;
>>>>>  
>>>>>  pub use resource::Resource;
>>>>> diff --git a/rust/kernel/io/register.rs b/rust/kernel/io/register.rs
>>>>> new file mode 100644
>>>>> index 000000000000..fc85dcd1f09a
>>>>> --- /dev/null
>>>>> +++ b/rust/kernel/io/register.rs
>>>>> @@ -0,0 +1,1287 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +//! A macro to define register layout and accessors.
>>>>> +//!
>>>>> +//! A single register typically includes several fields, which are accessed through a combination
>>>>> +//! of bit-shift and mask operations that introduce a class of potential mistakes, notably because
>>>>> +//! not all possible field values are necessarily valid.
>>>>> +//!
>>>>> +//! The [`register!`] macro in this module provides an intuitive and readable syntax for defining a
>>>>> +//! dedicated type for each register. Each such type comes with its own field accessors that can
>>>>> +//! return an error if a field's value is invalid.
>>>>> +//!
>>>>> +//! [`register!`]: kernel::register!
>>>>> +
>>>>> +use core::ops::Deref;
>>>>> +
>>>>> +use crate::io::{
>>>>> +    IoCapable,
>>>>> +    IoKnownSize, //
>>>>> +};
>>>>> +
>>>>> +/// Trait providing a base address to be added to the offset of a relative register to obtain
>>>>> +/// its actual offset.
>>>>> +///
>>>>> +/// The `T` generic argument is used to distinguish which base to use, in case a type provides
>>>>> +/// several bases. It is given to the `register!` macro to restrict the use of the register to
>>>>> +/// implementors of this particular variant.
>>>>> +pub trait RegisterBase<T> {
>>>>> +    /// Base address to which register offsets are added.
>>>>> +    const BASE: usize;
>>>>> +}
>>>>> +
>>>>> +/// Trait providing I/O read/write operations for register storage types.
>>>>> +///
>>>>> +/// This trait is implemented for all integer types on which I/O can be performed, allowing the
>>>>> +/// `register!` macro to generate appropriate I/O accessor methods based on the register's storage
>>>>> +/// type.
>>>>> +pub trait RegisterIo: Sized {
>>>>
>>>> Is this trait intended for public usage or just internal detail of `register!()`
>>>> macro?
>>>>
>>>> If it's the former, then we should probably just put the method into `IoCapable`
>>>> and allow generic-read in Io. If it's the latter, let's `#[doc(hidden)]` this so
>>>> it won't get abused.
>>>
>>> It is an internal detail of `register!()` and not supposed to be used by anyone
>>> else.
>>>
>>>>
>>>>> +    /// Read a value from the given offset in the I/O region.
>>>>> +    fn read<T, I>(io: &T, offset: usize) -> Self
>>>>> +    where
>>>>> +        T: Deref<Target = I>,
>>>>> +        I: IoKnownSize + IoCapable<Self>;
>>>>
>>>> I think generally `Deref` bound shouldn't be exposed to user. What smart
>>>> pointers are involved here, and can we implement forwarding impls of `Io`?
>>>
>>> Removing the requirement for `Deref` in the `RegisterIo` trait is simple - we
>>> can just call `Deref` in the register IO accessors.
>>
>> The issue with `Deref` bound here is that now you *require* a level of
>> indirection. If something implements `Io` directly, you cannot use it with the
>> method. While `&Bar` is accepted, `&Mmio` is not because `Mmio` is a direct
>> implementor of `Io` and not deref to it.
>>
>> A `Deref` bound also does not help if, say, a type is `Arc<Bar>` which needs two
>> level of dereffing before it is Io. For consistency I think it's best to avoid
>> `Deref` call all together
>
> Do you mean that `Bar` should implement `Io`, `IoKnownSize`, and all the
> required `IoCapable`s? That's a lot of boilerplate I'm afraid, and that
> would need to be repeated for all other I/O proxy types.
>
> No, I'm starting to believe that the fundamental issue is that the
> register interface does its I/O backwards, and that design issue is only
> exacerbated by the recent I/O redesign. I.e. instead of doing
>
>   regs::NV_PMC_BOOT_0::read(bar);
>
> We should really do
>
>   bar.read_reg::<regs::NV_PMC_BOOT_0>();
>
> Because that way we can use deref coercion.
>
> That's quite a big redesign though, which means I don't believe
> `register!` can make it this cycle... I'll give it a try though.

Hmm, that's unfortunate, but I think this is indeed a big design change that we
should iron out before merging...

I think you're right that if we put the methods on `Io` then all of the deref
issue would just went away.

BTW, there's a tricky that would allow you to avoid turbofish in this case, i.e.
you would be able to write

    bar.read_reg(regs::NV_PMC_BOOT_0)

Or

    bar.write_reg(regs::PV_PMC_BOOT_0, ...)

The trick is to note that the same name can present in both type and value
namesapce. For example, when you define

    struct Foo;

you create `Foo` type in type namespace and a `const Foo: Foo` in value
namespace. Similarly

    struct Foo(u32);

creates a `Foo` in type namespace and a `fn(u32) -> Foo` in value namespace.
This process doesn't need to be automatic. So if you create

    struct Foo { value: u32 }

then you're only creating `Foo` in type namespace and not in value namespace.
This means that you could do something like:

    // Use PhantomData as example, could be other token/marker types.
    const Foo: PhantomData<Foo> = PhantomData;

    trait Io {
        fn read_reg<Reg>(&self, _: PhantomData<Reg>) -> Reg { ... }
    }

    // Now no turbofish!
    bar.read_reg(NV_PMC_BOOT_0)

This trick is heavily used in `syn`, so you do `input.peek(type)` instead of
`input.peek::<Type>()`, which I find quite handy.

>
> <snip>
>>>>> +        $vis struct $name($storage);
>>>>> +
>>>>> +        #[allow(dead_code)]
>>>>> +        impl $name {
>>>>> +            /// Creates a bitfield from a raw value.
>>>>> +            $vis const fn from_raw(value: $storage) -> Self {
>>>>> +                Self(value)
>>>>> +            }
>>>>> +
>>>>> +            /// Creates a zeroed bitfield value.
>>>>> +            ///
>>>>> +            /// This is a const alternative to the `Zeroable::zeroed()` trait method.
>>>>> +            $vis const fn zeroed() -> Self {
>>>>> +                Self(0)
>>>>> +            }
>>>>
>>>> All types that impl `Zeroable` automatically have the `::zeroed()` fn provided
>>>> via the trait.
>>>
>>> Yes, but that method from the trait cannot be used in const context, and
>>> `zeroed` is the starting point for building register values from scratch (and
>>> thus constant values).
>>
>> `pin_init:::zeroed()` is a const function.
>
> Ah, I thought about using `Zeroable::zeroed()` at first, but indeed we
> can use the function instead.
>
> Strangely though, I expected to be able to do
>
>   const V: BOOT_0 = pin_init::zeroed()
>
> and have the compiler infer the generic type, but for some reason I have
> to specify it explicitly:
>
>   const V: BOOT_0 = pin_init::zeroed::<BOOT_0>()
>
> Not a big deal, but a const method would reduce this to 
>
>   const V: BOOT_0 = BOOT_0::zeroed()
>
> (for variables `let v: BOOT_0 = pin_init::zeroed()` works fine.)

That's strange. Do you have a MCVE for this? I can't reproduce what you
described.

Best,
Gary

Powered by blists - more mailing lists