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

[Charalampos Mitrodimas <charmitro@posteo.net>]

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`

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