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

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

On Wed Mar 4, 2026 at 8:48 PM CET, Gary Guo wrote:
> 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.

I'm not sure there are any relevant use-cases for this. Do you have real
examples that would not be represented with array registers?

Otherwise, I think this disconnect has no advantages. Actually, I think it may
even be the opposite, as it would allow for confusing the location.

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

We should be able to just do

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