Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Mark Brown]

On Wed, Mar 26, 2025 at 03:39:33PM -0300, Daniel Almeida wrote:

This is flagged as a resend but appears to be the first copy I've
seen...

> 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 are modeled with two types: [`Regulator`] and
> +//! [`EnabledRegulator`].

It would be helpful to see what the client code actually looks like
here.

> +    /// Enables the regulator.
> +    pub fn enable(self) -> Result<EnabledRegulator> {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        let res = to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) });
> +        res.map(|()| EnabledRegulator { inner: self })
> +    }

I assume this map does soemthing to report errors?

> +impl EnabledRegulator {

> +    /// Disables the regulator.
> +    pub fn disable(self) -> Result<Regulator> {
> +        // Keep the count on `regulator_get()`.
> +        let regulator = ManuallyDrop::new(self);
> +
> +        // SAFETY: Safe as per the type invariants of `Self`.
> +        let res = to_result(unsafe { bindings::regulator_disable(regulator.as_ptr()) });
> +
> +        res.map(|()| Regulator {
> +            inner: regulator.inner.inner,
> +        })
> +    }

This looks like user code could manually call it which feels like asking
for trouble?

> +    /// Sets the voltage for the regulator.
> +    pub fn set_voltage(&self, min_uv: Microvolt, max_uv: Microvolt) -> Result {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        to_result(unsafe { bindings::regulator_set_voltage(self.as_ptr(), min_uv.0, max_uv.0) })
> +    }

set_voltage() is only implemented for enabled regulators.  It is
reasonable for a driver to want to set the voltage for a regulator prior
to enabling it in order to ensure that the device powers up cleanly,
there may be different requirements for power on from on and idle so the
constraints may not be enough to ensure that a device can power on
cleanly.

> +    /// Gets the current voltage of the regulator.
> +    pub fn get_voltage(&self) -> Result<Microvolt> {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        let voltage = unsafe { bindings::regulator_get_voltage(self.as_ptr()) };
> +        if voltage < 0 {
> +            Err(Error::from_errno(voltage))
> +        } else {
> +            Ok(Microvolt(voltage))
> +        }
> +    }

Same issue here.

Attachment: signature.asc
Description: PGP signature