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

[Gary Guo]

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. 

>
> If we'd have this clear separation, I would obviously not object to this change,
> but currently it's just unnecessary redundancy.
>
>>> Even Nova today has quite a few registers that are just bitfields of a single
>>> field that spans all bits. I think many simple driver would probably want to
>>> just operate on primitives for these.
>>
>> I shall add that I think the fact that the registers that are *not* fields still
>> gain their dedicated type in Nova driver is due to the limitation of the initial
>> `register!` API design that *requires* unique types due to the `value.op(io)`
>> design as opposed to `io.op(value)`.
>>
>> I think even these ones should eventually be replaced by just primitives
>> eventually. I see no benefit of
>>
>>     bar.write(REG.init(|x| x.with_value(value)))
>>
>> as opposed to just
>>
>>     bar.write(REG, value)
>
> Well, you don't have to make that we have to use init() with a closure for such
> cases. We can also do something like:
>
> 	bar.write(Reg::from(value))

This won't work for the array case, right? For array you'd have

    bar.write(ARRAY.try_at(idx).ok_or(EINVAL)?.set(Reg::from(value)))

and now the register name is repeating twice rather than just

    bar.write(ARRAY.try_at(idx).ok_or(EINVAL)?, value)

Best,
Gary