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

[Alexandre Courbot]

On Fri Mar 6, 2026 at 7:42 PM JST, Gary Guo wrote:
> On Fri Mar 6, 2026 at 5:37 AM GMT, 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:
>>>>> 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.
>
> For array registers I think it makes more sense to use the two-argument version,
> no?
>
> The example here is to demonstrate that we can add a shorthand version for the
> fixed register version that can write a value to register without mentioning its
> name (as a supplemental helper), and the basic write method is the two-argument
> one.
>
> For cases where the type doesn't guarantee a fixed location like FIFO register
> or an array register, mentioning the name twice is fine.

It's still tedious, and a step back compared to the one-argument version
imho.

>
> [
>
>     For array case, you *could* also do
>
>     impl IoLoc<RegisterName> for usize {
>         fn offset(self) -> usize {
>             self * stride + fixed_base
>         }
>     }
>
>
>     and now you can do `self.write(index, reg_value)`, although I think this
>     might confuse some people.

Yes, in this case the semantics of write's first argument would be
dependent on the second argument... I think that's a potential footgun.

>
>     For the fixed case you could do
>
>     impl IoLoc<RegisterName> for () {
>         fn offset(self) -> usize {
>             fixed_loc
>         }
>     }
>
>     which means you can do `self.write((), value)`. I think this looks a bit
>     uglier compared to a dedicated method, but TBH it isn't too terrible.

There is another potential solution I have played with:

    https://lore.kernel.org/all/DGVJ7VQX3TD5.2UYW004QJPI6N@xxxxxxxxxx/

It would let us keep the 2-arguments as a base, while letting us take
advantage of the one-argument syntax you proposed in [1] and extending
it to support other register types.

[1] https://lore.kernel.org/all/DGU9AZ43QK6Y.115RDSK0M9JY5@xxxxxxxxxxx/

I think it turns out pretty nice and the supporting code is also not too
ugly. I'll try to send a diff a bit later so you can see how it behaves
in practice.