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()`.