Onur Özkan <[email protected]> writes:
> On rk3588s, `dmesg | grep 'tyr'` logs:
>
> tyr fb000000.gpu: supply SRAM not found, using dummy regulator
>
> This happens because Tyr calls Regulator<Enabled>::get() for SRAM,
> which goes through the non-optional regulator_get() path. If the
> device tree doesn't provide sram-supply, regulator core falls back
> to a dummy regulator and writes that log.
>
> Panthor handles SRAM as optional and tolerates missing sram-supply.
> This patch matches that behavior in Tyr by using optional regulator
> lookup and storing SRAM as Option<Regulator<Enabled>> which avoids
> dummy-regulator fallback/noise when SRAM is not described inside
> the device tree.
>
> Link:
> https://rust-for-linux.zulipchat.com/#narrow/stream/x/topic/x/near/573210018
> Signed-off-by: Onur Özkan <[email protected]>
> ---
> drivers/gpu/drm/tyr/driver.rs | 5 +++--
> rust/kernel/regulator.rs | 40 +++++++++++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 0389c558c036..e0856deb83ec 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -113,7 +113,8 @@ fn probe(
> coregroup_clk.prepare_enable()?;
>
> let mali_regulator =
> Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("mali"))?;
> - let sram_regulator =
> Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("sram"))?;
> + let sram_regulator =
> + Regulator::<regulator::Enabled>::get_optional(pdev.as_ref(),
> c_str!("sram"))?;
>
> let request = pdev.io_request_by_index(0).ok_or(ENODEV)?;
> let iomem = Arc::pin_init(request.iomap_sized::<SZ_2M>(),
> GFP_KERNEL)?;
> @@ -201,5 +202,5 @@ struct Clocks {
> #[pin_data]
> struct Regulators {
> mali: Regulator<regulator::Enabled>,
> - sram: Regulator<regulator::Enabled>,
> + sram: Option<Regulator<regulator::Enabled>>,
> }
> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> index 2c44827ad0b7..8d95e5e80051 100644
> --- a/rust/kernel/regulator.rs
> +++ b/rust/kernel/regulator.rs
> @@ -283,6 +283,29 @@ fn get_internal(dev: &Device, name: &CStr) ->
> Result<Regulator<T>> {
> })
> }
>
> + fn get_optional_internal(dev: &Device, name: &CStr) ->
> Result<Option<Regulator<T>>> {
> + // SAFETY: It is safe to call `regulator_get_optional()`, on a
> + // device pointer received from the C code.
> + let inner = from_err_ptr(unsafe {
> + bindings::regulator_get_optional(dev.as_raw(),
> name.as_char_ptr())
> + });
Hello,
When CONFIG_REGULATOR is disabled, regulator_get_optional() becomes a
static inline stub in consumer.h, and bindgen cannot export it as a
symbol. The other regulator functions all have C helpers for this but
regulator_get_optional() is missing one.
So it causes a E0425 with CONFIG_REGULATOR not set.
error[E0425]: cannot find function `regulator_get_optional` in crate
`bindings`
--> rust/kernel/regulator.rs:290:23
|
290 | bindings::regulator_get_optional(dev.as_raw(),
name.as_char_ptr())
| ^^^^^^^^^^^^^^^^^^^^^^ help: a function with a
similar name exists: `regulator_get_voltage`
> +
> + let inner = match inner {
> + Ok(inner) => inner,
> + Err(ENODEV) => return Ok(None),
> + Err(err) => return Err(err),
> + };
> +
> + // SAFETY: We can safely trust `inner` to be a pointer to a valid
> + // regulator if `ERR_PTR` was not returned.
> + let inner = unsafe { NonNull::new_unchecked(inner) };
> +
> + Ok(Some(Self {
> + inner,
> + _phantom: PhantomData,
> + }))
> + }
The Regulator struct invariant currently says:
/// - `inner` is a non-null wrapper over a pointer to a `struct
/// regulator` obtained from [`regulator_get()`].
Since get_optional_internal() creates a Regulator from
regulator_get_optional(), should we also update it to mention it?
Cheers,
C. Mitrodimas
> +
> fn enable_internal(&self) -> Result {
> // SAFETY: Safe as per the type invariants of `Regulator`.
> to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })
> @@ -300,6 +323,11 @@ pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
> Regulator::get_internal(dev, name)
> }
>
> + /// Obtains an optional [`Regulator`] instance from the system.
> + pub fn get_optional(dev: &Device, name: &CStr) -> Result<Option<Self>> {
> + Regulator::get_optional_internal(dev, name)
> + }
> +
> /// Attempts to convert the regulator to an enabled state.
> pub fn try_into_enabled(self) -> Result<Regulator<Enabled>,
> Error<Disabled>> {
> // We will be transferring the ownership of our `regulator_get()`
> count to
> @@ -329,6 +357,18 @@ pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
> .map_err(|error| error.error)
> }
>
> + /// Obtains an optional [`Regulator`] instance from the system and
> enables it.
> + pub fn get_optional(dev: &Device, name: &CStr) -> Result<Option<Self>> {
> + match Regulator::<Disabled>::get_optional_internal(dev, name)? {
> + Some(regulator) => {
> + let enabled_regulator =
> + regulator.try_into_enabled().map_err(|error|
> error.error)?;
> + Ok(Some(enabled_regulator))
> + }
> + None => Ok(None),
> + }
> + }
> +
> /// Attempts to convert the regulator to a disabled state.
> pub fn try_into_disabled(self) -> Result<Regulator<Disabled>,
> Error<Enabled>> {
> // We will be transferring the ownership of our `regulator_get()`
> count
