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

["Danilo Krummrich" <dakr@kernel.org>]

On Tue Mar 10, 2026 at 11:33 PM CET, Gary Guo wrote:
> 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

s/also pairs with/also works with/

> 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.

IIUC, the return value could be a generic tuple struct with <L: IoLoc<T>, V:
IntoIoVal<T, L>> that dereferences to V?

I.e. the current write_val() already allows this:

	bar.write_val((WithBase::of::<E>(), reg))

so, a corresponding read_val() - both write_val() and read_val() should really
have different names - could return the same thing, just as a generic type that
dereferences to self.1.

With this you could do

	let reg = bar.read_reg(regs::NV_PFALCON_FALCON_RM::of::<E>());

	reg.modify();

	bar.write_reg(reg);

without repeating the of::<E>() part while

	let val = regs::NV_PFALCON_FALCON_RM::zeroed();

	// Set a couple of fields.

	bar.write(WithBase::of::<E>(), val);

is still possible.

> 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`.

That's why I said I only flipped it because write_val() is an odd name for what
it does currently. Please also note the below.

>> 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)
>>> +    }