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

[Alice Ryhl]

On Wed, Jul 2, 2025 at 3:03 PM Daniel Almeida
<daniel.almeida@xxxxxxxxxxxxx> wrote:
>
> Hi Alice,
>
> > On 2 Jul 2025, at 07:35, Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> >
> > On Fri, Jun 27, 2025 at 7:11 PM Daniel Almeida
> > <daniel.almeida@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >
> > 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.

Okay.

> >> +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?

Along these lines:

pub trait RegulatorState: 'static {
    const DISABLE_ON_DROP: bool;
}

impl RegulatorState for Enabled {
    const DISABLE_ON_DROP: bool = true;
}
impl RegulatorState for Disabled {
    const DISABLE_ON_DROP: bool = false;
}
impl RegulatorState for Dynamic {
    const DISABLE_ON_DROP: bool = false;
}

impl<T: RegulatorState> Drop for Regulator<T> {
    fn drop(&mut self) {
        if T::DISABLE_ON_DROP {
            unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
        }
        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
    }
}