Hi Alex. Thank you and John for working on this in general. It will be useful for the whole ecosystem! :)
> On 18 Jul 2025, at 04:26, Alexandre Courbot <[email protected]> wrote: > > From: John Hubbard <[email protected]> > > There is only one top-level macro in this file at the moment, but the > "macros.rs" file name allows for more. Change the wording so that it > will remain valid even if additional macros are added to the file. > > Fix a couple of spelling errors and grammatical errors, and break up a > run-on sentence, for clarity. > > Cc: Alexandre Courbot <[email protected]> > Cc: Danilo Krummrich <[email protected]> > Signed-off-by: John Hubbard <[email protected]> > Signed-off-by: Alexandre Courbot <[email protected]> > --- > drivers/gpu/nova-core/regs/macros.rs | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/nova-core/regs/macros.rs > b/drivers/gpu/nova-core/regs/macros.rs > index > cdf668073480ed703c89ffa8628f5c9de6494687..864d1e83bed2979f5661e038f4c9fd87d33f69a7 > 100644 > --- a/drivers/gpu/nova-core/regs/macros.rs > +++ b/drivers/gpu/nova-core/regs/macros.rs > @@ -1,17 +1,17 @@ > // SPDX-License-Identifier: GPL-2.0 > > -//! Macro to define register layout and accessors. > +//! `register!` macro to define register layout and accessors. I would have kept this line as-is. Users will most likely know the name of the macro already. At this point, they will be looking for what it does, so mentioning "register" here is a bit redundant IMHO. > //! > //! 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 macro in this module allow to define, using an intruitive and > readable syntax, a dedicated > -//! type for each register with its own field accessors that can return an > error is a field's value > -//! is invalid. > +//! 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. > > -/// Defines a dedicated type for a register with an absolute offset, > alongside with getter and > -/// setter methods for its fields and methods to read and write it from an > `Io` region. > +/// Defines a dedicated type for a register with an absolute offset, > including getter and setter > +/// methods for its fields and methods to read and write it from an `Io` > region. +cc Steven Price, Sorry for hijacking this patch, but I think that we should be more flexible and allow for non-literal offsets in the macro. In Tyr, for example, some of the offsets need to be computed at runtime, i.e.: +pub(crate) struct AsRegister(usize); + +impl AsRegister { + fn new(as_nr: usize, offset: usize) -> Result<Self> { + if as_nr >= 32 { + Err(EINVAL) + } else { + Ok(AsRegister(mmu_as(as_nr) + offset)) + } + } Or: +pub(crate) struct Doorbell(usize); + +impl Doorbell { + pub(crate) fn new(doorbell_id: usize) -> Self { + Doorbell(0x80000 + (doorbell_id * 0x10000)) + } I don't think this will work with the current macro, JFYI. > /// > /// Example: > /// > @@ -24,7 +24,7 @@ > /// ``` > /// > /// This defines a `BOOT_0` type which can be read or written from offset > `0x100` of an `Io` > -/// region. It is composed of 3 fields, for instance `minor_revision` is > made of the 4 less > +/// region. It is composed of 3 fields, for instance `minor_revision` is > made of the 4 least > /// significant bits of the register. Each field can be accessed and modified > using accessor > /// methods: > /// > > -- > 2.50.1 > Reviewed-by: Daniel Almeida <[email protected]>
