Re: [PATCH v7 05/10] rust: io: add IoLoc and IoWrite types

["Alexandre Courbot" <acourbot@nvidia.com>]

On Fri Mar 6, 2026 at 2:37 PM JST, Alexandre Courbot wrote:
> On Thu Mar 5, 2026 at 7:15 AM JST, Gary Guo wrote:
>> On Wed Mar 4, 2026 at 9:38 PM GMT, Danilo Krummrich wrote:
>>> On Wed Mar 4, 2026 at 10:13 PM CET, Gary Guo wrote:
>>>> On Wed Mar 4, 2026 at 8:37 PM GMT, Danilo Krummrich wrote:
>>>>> On Wed Mar 4, 2026 at 8:48 PM CET, Gary Guo wrote:
>>>>>> On Wed Mar 4, 2026 at 7:38 PM GMT, Danilo Krummrich wrote:
>>>>>>> On Wed Mar 4, 2026 at 7:58 PM CET, Gary Guo wrote:
>>>>>>>> On Wed Mar 4, 2026 at 6:39 PM GMT, Gary Guo wrote:
>>>>>>>>> On Wed Mar 4, 2026 at 4:18 PM GMT, Danilo Krummrich wrote:
>>>>>>>>>> On Tue Mar 3, 2026 at 3:55 PM CET, Alexandre Courbot wrote:
>>>>>>>>>>> So, to get a better idea of these two options I have converted this
>>>>>>>>>>> patchset to use the 2-arguments `write_with` method. Here is the
>>>>>>>>>>> difference between the two - it is particularly interesting to see how
>>>>>>>>>>> nova-core changes:
>>>>>>>>>>>
>>>>>>>>>>> https://github.com/Gnurou/linux/compare/register_1arg..Gnurou:linux:register_2args
>>>>>>>>>>
>>>>>>>>>> This looks good to me, but the fact that this turns out nicely has nothing to do
>>>>>>>>>> with write() now taking two arguments. I.e. there is no reason why we couldn't
>>>>>>>>>> have the exact same write_with() method together with the single argument
>>>>>>>>>> write() method.
>>>>>>>>>>
>>>>>>>>>> The contention point for me with a two arguments write() method still remains
>>>>>>>>>> that the arguments are redundant.
>>>>>>>>>>
>>>>>>>>>> I.e. you first have the location in form of an object instance of a ZST (which
>>>>>>>>>> in the end is just a "trick" to pass in the type itself) and then we have the
>>>>>>>>>> object that actually represents the entire register, describing both the
>>>>>>>>>> location *and* the value.
>>>>>>>>>>
>>>>>>>>>> So, let's say a driver creates a register object with a custom constructor
>>>>>>>>>>
>>>>>>>>>> 	let reset = regs::MyReg::reset();
>>>>>>>>>>
>>>>>>>>>> then the two argument approach would be
>>>>>>>>>>
>>>>>>>>>> 	(1) bar.write(regs::MyReg, regs::MyReg::reset());
>>>>>>>>>>
>>>>>>>>>> whereas the single argument approach would just be
>>>>>>>>>>
>>>>>>>>>> 	(2) bar.write(regs::MyReg::reset());
>>>>>>>>>
>>>>>>>>> That's only for bit field registers that has unique types. I still believe types
>>>>>>>>> of registers should not be tightly coupled with name of registeres.
>>>>>>>>>
>>>>>>>>> Allowing a value of register to be directly used for `write` is also confusing
>>>>>>>>> if a value is not created immediately before written to.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So, if I would have to write (1), I'd probably be tempted to implement a reset()
>>>>>>>>>> function that takes the bar as argument to hide this, i.e.
>>>>>>>>>>
>>>>>>>>>> 	regs::MyReg::reset(bar);
>>>>>>>>>>
>>>>>>>>>> I also can't agree with the argument that the notation of write(loc, val) - or
>>>>>>>>>> write(val, loc) as the C side does it - is common and we should stick to it.
>>>>>>>>>>
>>>>>>>>>> This notation is only common because it is necessary when operating on
>>>>>>>>>> primitives or when the two representing types are discrete.
>>>>>>>>>>
>>>>>>>>>> But this isn't the case here, a register object is already distinct in terms of
>>>>>>>>>> its location and value.
>>>>>>>>>
>>>>>>>>> I see no reason why register values for different locations have to be distinct
>>>>>>>>> in terms of value types.
>>>>>>>
>>>>>>> That's not what the register!() macro currently does, a register type always has
>>>>>>> a unique location, or is an array register, etc. In any case a register type is
>>>>>>> assoiciated with a location.
>>>>>>>
>>>>>>> If the proposal is to disconnect location and register type entirely, that would
>>>>>>> be a change to the current design.
>>>>>>
>>>>>> It's not what the macro do today, but I don't want to ask Alex to change it
>>>>>> further before landing the series. I do think it's a worthy follow-up to add the
>>>>>> ability to decouple the location and type. It's not incompatible with current
>>>>>> design anyway.
>>>>>
>>>>> I'm not sure there are any relevant use-cases for this. Do you have real
>>>>> examples that would not be represented with array registers?
>>>>
>>>> Even for the cases where there's a PIO register, I think it's beneficial to just
>>>> get a value without a type.
>>>>
>>>> I don't see why we want people to write
>>>>
>>>>     self.io.read(UART_RX).value()
>>>>
>>>> vs
>>>>
>>>>     self.io.read(UART_RX)
>>>>
>>>> or
>>>>
>>>>     self.io.write(UART_TX::from(byte))
>>>>
>>>> vs
>>>>
>>>>     self.io.write(UART_TX, byte)
>>>>
>>>> what benefit does additional type provide?
>>>
>>> Well, for FIFO registers this is indeed better. However, my main concern was
>>> this
>>>
>>> 	bar.write(regs::MyReg, regs::MyReg::foo())
>>
>> This specific case is indeed more cumbersome with the two argument approach,
>> although given Alex's nova diff I think the occurance shouldn't be that
>> frequent.
>>
>> It's also not that the two argument approach would preclude us from having a
>> single argument option. In fact, with the two-argument design as the basis, we
>> can implement such a helper function cleaner than Alex's PATCH 10/10 (which uses
>> `Into<IoWrite>`:
>>
>>     /// Indicates that this type is always associated with a specific fixed I/O
>>     /// location.
>>     ///
>>     /// This allows use of `io.bikeshed_shorthand_name(value)` instead of specifying
>>     /// the register name explicitly `io.write(REG, value)`.
>>     trait FixedIoLocation {
>>         type IoLocType: IoLoc<Self>;
>>         const IO_LOCATION: Self::IoLocType;
>>     }
>>
>>     trait Io {
>>         fn bikeshed_shorthand_name<T>(&self, value: T)
>>             where T: FixedIoLocation +
>>                   Self: IoCapable<<T::IoLocType as IoLoc<T>>::IoType>,
>>         {
>>             self.write(T::IO_LOCATION, value)
>>         }
>>     }
>>
>> No need for a `IoWrite` type, everything is done via traits.
>
> That's cool but will only work for fixed registers. If you work with, say, an
> array of registers, cannot implement this trait on a value as the value
> doesn't have an index assigned - meaning you would have to build a
> location in addition of it.
>
> So it only solves the problem partially.

... but the remainder of the solution can be implemented to look
something like this:

  // Write a fixed register.
  bar.write(regs::MyReg::foo());

  // Write into a registers array at index 10.
  bar.write(Reg::at(10, regs::MyRegArray::foo()));

  // Write a relative register at the `Gsp` instance.
  bar.write(Reg::of::<Gsp>(regs::MyRelativeReg::foo()));

Amongst other benefits (no closures and the complications they bring),
it also seems to be the shortest syntax so far.

This could coexist with a 2-arguments write method, or completely
supplant it.

Before doing and sharing a full implementation, I'd like to check from
this syntax that people don't hate it. WDYT?