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

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

> On 26 Mar 2025, at 15:56, Mark Brown <broonie@kernel.org> wrote:
> 
> 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...

Hi Mark, you were not on cc earlier this morning, which is why this is being
resent. I left a comment about this, but apparently b4 did not pick it up.

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

Ack, I'll include examples in v3.

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

map() returns the error to the caller, if any. Notice that the return type is
Result<EnabledRegulator>.

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

Yes, user code can call this. My understanding is that drivers may want to
disable the regulator at runtime, possibly to save power when the device is
idle?

What trouble are you referring to?


>> +    /// 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.

Ok, we can move the implementation to Regulator then.

— Daniel