Re: [PATCH v6 1/2] rust: regulator: add a bare minimum regulator abstraction

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Alice,

> On 2 Jul 2025, at 07:35, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Fri, Jun 27, 2025 at 7:11 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> Add a bare minimum regulator abstraction to be used by Rust drivers.
>> This abstraction adds a small subset of the regulator API, which is
>> thought to be sufficient for the drivers we have now.
>> 
>> Regulators provide the power needed by many hardware blocks and thus are
>> likely to be needed by a lot of drivers.
>> 
>> It was tested on rk3588, where it was used to power up the "mali"
>> regulator in order to power up the GPU.
>> 
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> Overall looks reasonable to me.
> 
>> +/// A trait that abstracts the ability to check if a [`Regulator`] is enabled.
>> +pub trait IsEnabled: RegulatorState {}
>> +impl IsEnabled for Disabled {}
>> +impl IsEnabled for Dynamic {}
> 
> Naming-wise, it's a bit weird that IsEnabled applies to everything
> *but* enabled. And also, the is_enabled() method should probably exist
> for only Dynamic anyway?

I think it's the other way around? Enabled doesn't need this impl precisely
because of the Enabled token. IOW:

Regulator<Enabled>::is_enabled() doesn't make sense.

>  And also, the is_enabled() method should probably exist for only Dynamic anyway?

Also no, because Regulator<Disabled> isn't necessarily disabled. It just was
not enabled by us, but might have been enabled by somebody else in the system.


[…]

> 
> 
>> +impl<T: RegulatorState + 'static> Drop for Regulator<T> {
>> +    fn drop(&mut self) {
>> +        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {
> 
> I would avoid this kind of logic. Instead, you can add an
> `disable_on_drop()` method or constant to the trait and check it here.
> 
> Alice
> 

Can you expand on this?

— Daniel