Message-Id: <B1AA6359-7854-4284-B533-F5CA3C18AF34@collabora.com>
Date: Fri, 25 Jul 2025 13:14:49 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Danilo Krummrich <dakr@...nel.org>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 Beata Michalska <beata.michalska@....com>,
 nouveau@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 John Hubbard <jhubbard@...dia.com>,
 steven.price@....com
Subject: Re: [PATCH v2 01/19] gpu: nova-core: register: minor grammar and
 spelling fixes

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@...dia.com> wrote:
> 
> From: John Hubbard <jhubbard@...dia.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@...dia.com>
> Cc: Danilo Krummrich <dakr@...nel.org>
> Signed-off-by: John Hubbard <jhubbard@...dia.com>
> Signed-off-by: Alexandre Courbot <acourbot@...dia.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@...labora.com>

Powered by blists - more mailing lists