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

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

On Thu Mar 12, 2026 at 2:53 PM CET, Alexandre Courbot wrote:
> On Wed Mar 11, 2026 at 11:56 PM JST, Danilo Krummrich wrote:
>> And yes, I am aware that the above wording around the difference between
>> regs::NV_PFALCON_FALCON_RM::of::<E>() and WithBase::of::<E>() is at least a bit
>> vague technically, but my point is about how this appears to users.
>>
>> In any case, the fact that you can write WithBase::of::<E>() as a location
>> argument in the first place proves that `reg` is not *only* a value.
>
> But that is only true in the case of registers.

Agree with this and all the above. But - and this is why I wrote "my point is
about how this appears to users" - users use this API with registers only and
except for the FIFO use-case, which is not implemented yet, it is obvious to
users that part of the location comes from the value type.

I'm not saying that this is a problem. Actually, quite the opposite.

When I proposed the register!() macro, one of the problems I wanted to solve was
that people can't read from a certain location and accidentally treat it as
something semantically different.

Or in other words, if you want to read register A, you shouldn't be able to
accidentally read from B and treat it as A.

> The I/O module can and does cover things other than registers - currently
> primitive values, but we might want to extend that in future (don't ask me
> with what, but I don't see a reason to close the possibility :)).

Neither do I. :)

But, as mentioned above, we should be aware that people notice that for
registers part of the location information comes from the value type (except for
the FIFO case of course) and registers are the thing that people deal with most.

Other use-cases such as I/O memcpy, etc. will be accssible through other APIs,
such as IoSlice, or more generally, IoView.

Having that said, my point is that considering the use-cases it also makes sense
to think about how this API will be perceived.

And from this context something like bar.write((), reg) looks pretty odd.

>> Is this really less confusing than an additional bar.write_reg(reg) that just
>> works with any register?
>
> I don't think it is less or more confusing, in the end they are really
> equivalent.

From your point of view being an expert on the I/O and register!()
implementation details, but please also consider the above.

Also, look at the Tyr patches for instance, they exclusively use write_val().

> What I would like to avoid is having register-specific functions spill
> into the `io` module. I mean, I am not strictly opposed to it if we
> reach the conclusion that it is more convenient to users - in this case
> we could add an `impl Io` block in `register.rs` and add a `write_reg`
> method (and possibly a `read` variant returning the full position
> information?). But if we can just use the same 2 methods everywhere,
> that's better IMHO.

Just leave it in the io module I'd say, register access is probably the most
essential part of I/O, I think there is no need to factor it out.

>>> which is exactly the same length as the `write_val` equivalent - it's
>>> just that you need to remember that `()` can be used in this case. But
>>> if you can remember that your register type can be used with
>>> `write_val`, then why not this? This actually makes me doubt that
>>> `write_val` is needed at all, and if we get rid of it, then we have a
>>> symmetric API.
>>
>> Still not symmetric, and I also don't think we will have a lot of fun explaining
>> people why they have to call it as bar.write((), reg). :(
>>
>> OOC, how would you explain it when the question is raised without arguing with
>> implementation details?
>
> This seems to indicate that instead of a `Io::write_val` method in `io.rs`,
> we might need a `Io::write_reg` method in `register.rs` that is
> dedicated to writing unambiguous registers exclusively. How does that
> sound to you?

I don't see it adds any value to factor it out with an extention trait, rename
to write_reg() seems fine.

Additionally, I'd like to leave it open for the future to add read_reg() method
returning a generic (loc, val) tuple dereferencing to the value in case we see a
repetition of the following pattern.

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

	// modify reg

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

The reason is the same as mentioned above. In most cases drivers don't want to
switch the base location between a read and a write, i.e. write the value from A
to B.

The most common case is to read from A and write back to A. For instance,
talking to the Tyr folks, they told me that for the array registers they will
have, they will never have the case that they want to write a register value
from A to B.

So, I still think it would be good to provide an option for drivers to prevent
any mistakes in the first place.

Now, obviously this would require that we also provide register accessors that
take a mutable reference for this to work, but that doesn't seem like a big
deal.

I also don't think we have to do this now, but I'd like to have something like
this in the future.

For now s/write_val/write_reg/ and we should be good to go. :)