Re: [PATCH v2 01/19] gpu: nova-core: register: minor grammar and spelling fixes

[Daniel Almeida <daniel.almeida@collabora.com>]

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 <acourbot@nvidia.com> wrote:
> 
> From: John Hubbard <jhubbard@nvidia.com>
> 
> 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 <acourbot@nvidia.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> 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 <daniel.almeida@collabora.com>