Re: [PATCH v1] drm/tyr: make SRAM supply optional like panthor

["Onur Özkan" <work@onurozkan.dev>]

On Thu, 12 Feb 2026 02:04:23 +0000
Charalampos Mitrodimas <charmitro@posteo.net> wrote:

> Onur Özkan <work@onurozkan.dev> 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 <work@onurozkan.dev> ---
> >  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`

Yeah, I missed that.

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

I think we should, will send v2 and cover both changes.

Thanks,
Onur

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