Re: [PATCH v8 07/10] rust: io: introduce `IntoIoVal` trait and single-argument `write_val`

[Gary Guo]

On Tue Mar 10, 2026 at 4:34 PM GMT, Danilo Krummrich wrote:
> On Mon Mar 9, 2026 at 4:14 PM CET, Alexandre Courbot wrote:
>> +    /// Generic infallible write of value bearing its location, with compile-time bounds check.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// Tuples carrying a location and a value can be used with this method:
>> +    ///
>> +    /// ```no_run
>> +    /// use kernel::io::{Io, Mmio};
>> +    ///
>> +    /// fn do_writes(io: &Mmio) {
>> +    ///     // 32-bit write of value `1` at address `0x10`.
>> +    ///     io.write_val((0x10, 1u32));
>> +    /// }
>> +    /// ```
>> +    #[inline(always)]
>> +    fn write_val<T, L, V>(&self, value: V)
>
> Still not super satisfied with the name write_val() plus I have some more
> considerations, sorry. :)
>
> Let's look at this:
>
> 	let reg = bar.read(regs::NV_PMC_BOOT_0);
>
> 	// Pass around, do some modifications, etc.
>
> 	bar.write_val(reg);
>
> In this case read() returns an object that encodes the absolute location and
> value, i.e. read() pairs with write_val(), which is a bit odd.

I mean, it also pairs with `bar.write(regs::NV_PMC_BOOT_0, reg)`, so it's not
totally odd. We just need to teach that "write_val" infers the register location
from value.

>
> In this example:
>
> 	let reg = bar.read(regs::NV_PFALCON_FALCON_RM::of::<E>());
>
> 	// Pass around, do some modifications, etc.
>
> 	bar.write(WithBase::of::<E>(), reg);
>
> read() pairs with write(), since the object returned by read() does only encode
> a relative location, so we have to supply the base location through the first
> argument of write().
>
> Well, technically it also pairs with write_val(), as we could also pass this as
> tuple to write_val().
>
> 	bar.write_val((WithBase::of::<E>(), reg));
>
> However, my point is that this is a bit inconsistent and I think a user would
> expect something more symmetric.
>
> For instance, let's say write() would become the single argument one, then
> read() could return an impl of IntoIoVal, so we would have
>
> 	let reg = bar.read(regs::NV_PMC_BOOT_0);
> 	bar.write(reg);
>
> 	let reg = bar.read(regs::NV_PFALCON_FALCON_RM::of::<E>());
> 	bar.write(reg);

How would you modify the value then? `reg` becomes a combined pair with both
location and value, and we would start need to use a closure to mutate inner
values again, which in my opinion is bad.

This would again become the `IoWrite` design of the last iteration which is the
part that I disliked most.

>
> and additionally we can have
>
> 	let val = bar.read_val(regs::NV_PFALCON_FALCON_RM::of::<E>());
> 	bar.write_val(WithBase::of::<E>(), val);
>
> The same goes for
>
> 	let val = bar.read_val(regs::NV_PMC_BOOT_0);
> 	bar.write_val(regs::NV_PMC_BOOT_0, val);
>
> which for complex values does not achieve anything, but makes sense for the FIFO
> register use-case, as val could also be a primitive, e.g. u32.
>
> With a dynamic base, and a single field we could just do the following to
> support such primitives:
>
> 	let val = bar.read_val(regs::NV_PFALCON_FALCON_RM::of::<E>());
> 	bar.write_val(regs::NV_PFALCON_FALCON_RM::of::<E>(), val);
>
> I think adding this symmetry should be trivial, as most of this already compiles
> with the current code.
>
> Also, let's not get into a discussion whether we name on or the other write()
> vs.  write_foo(). I just turned it around as I think with the specific name
> write_val() it makes more sense this way around.

To me flipping this around is the odd one. If this is flipped around then this
should be named `write_at`, but then it becomes odd as it is the dual of `read`
and we won't have `read_at`.

Best,
Gary

>
> But I'm perfectly happy either way as long as we find descriptive names that
> actually make sense.
>
> Unfortunately, I can't really think of a good name for write_val() with its
> current semantics. If we flip it around as in my examples above, the _val()
> prefix seems fine. Maybe write_reg() and read_reg()?
>
>> +    where
>> +        L: IoLoc<T>,
>> +        V: IntoIoVal<T, L>,
>> +        Self: IoKnownSize + IoCapable<L::IoType>,
>> +    {
>> +        let (location, value) = value.into_io_val();
>> +
>> +        self.write(location, value)
>> +    }