On Thu, 12 Feb 2026 02:04:23 +0000 Charalampos Mitrodimas <[email protected]> wrote:
> 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` 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
