Re: [PATCH v3 16/16] rust: io: register: unify handling of register with/without bitfields

"Alexandre Courbot" <[email protected]>
Newsgroups dev.linux.lists.nova-gpu,dev.linux.lists.driver-core,org.freedesktop.lists.dri-devel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-pci,org.kernel.vger.rust-for-linux
Message-ID <[email protected]>
On Wed Aug 19, 2026 at 8:09 PM JST, Gary Guo wrote:
> Move the `FixedRegister` to be a property of register to become a property

s/to be/from being?

> of type. Name the new trait `FixedIoLoc` indicating if I/O location of a
> type is unique for a specific base. Thus, bitfields become just a special
> case of this (where type is unique because we're generating it in the
> register macro), and expose feature to registers without inline bitfield
> definition with the `#[unique]` attribute.
>
> Signed-off-by: Gary Guo <[email protected]>

The Sashiko findings are legit but not blocking, so:

Reviewed-by: Alexandre Courbot <[email protected]>

<...>
> -/// Helper function for register alias implementation.
> -///
> -/// This is used to enforce base matching. Only called during const eval.
> -#[doc(hidden)]
> -#[inline(always)]
> -pub const fn alias_offset<Base: ?Sized, Alias: FixedRegister<Base = Base>>() -> usize {
> -    Alias::OFFSET
> -}
> -
>  /// Helper function for register element alias implementation.
>  ///
>  /// This is used to enforce base matching and provide bounds checking. Only called during const
> @@ -419,6 +380,31 @@ pub const fn element_alias_offset<Base: ?Sized, Alias: RegisterArray<Base = Base
>  /// }
>  /// ```
>  ///
> +/// In case there is a fixed register associated with a specific type in the base, you can apply
> +/// `#[unique]` attribute which enables `write_reg` shorthand. This is automatically applied to
> +/// bitfields instantiated via the `register!` macro.

As Sashiko pointed out, let's add a sentence explaining that this is not
intended to be used on primitive types. I guess that could enable some
interesting syntax in the case of FIFO sub-regions, but generally
speaking this should be a Bad Idea.

<...>
> @@ -217,27 +230,30 @@ pub(crate) fn register(def: RegDef) -> Result<TokenStream> {
>          }
>  
>          match array {
> -            None if bitfield.is_none() => outputs.extend(quote!(
> -                #(#attrs)* #vis const #name: ::kernel::io::register::OffsetLoc<#base, #ty> =
> -                    ::kernel::io::register::OffsetLoc::new(#offset);
> -            )),
> -
> -            _ if bitfield.is_none() => Err(Error::new_spanned(
> -                ty,
> -                "defining without bitfield is not yet supported for this type of register",
> -            ))?,
> -
> -            None => outputs.extend(quote_spanned!(span =>
> -                impl ::kernel::io::register::FixedRegister for #name {
> -                    type Base = #base;
> -                    const OFFSET: usize = #offset;
> +            None => {
> +                if unique {
> +                    outputs.extend(quote!(
> +                        impl ::kernel::io::register::FixedIoLoc<#base> for #ty {
> +                            type Location = ::kernel::io::register::OffsetLoc<#base, #ty>;
> +                            const LOCATION: Self::Location = #name;
> +                        }
> +                    ))
>                  }
>  
> -                #(#attrs)* #vis const #name: ::kernel::io::register::FixedRegisterLoc<#name> =
> -                    ::kernel::io::register::FixedRegisterLoc::<#name>::new();
> -            )),
> +                outputs.extend(quote_spanned!(span =>
> +                    #(#attrs)* #vis const #name: ::kernel::io::register::OffsetLoc<#base, #ty> =
> +                        ::kernel::io::register::OffsetLoc::new(#offset);
> +                ));
> +            }
>  
>              Some(def) => {
> +                if !unique {
> +                    Err(Error::new_spanned(
> +                        &ty,
> +                        "defining without bitfield is not yet supported for this type of register",
> +                    ))?
> +                }

Sashiko's comment about the array validation bypass looks legit. IIUC
the guard also needs to test for `bitfield.is_none()`.
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.