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

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

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.