Re: [PATCH 24/37] drm/atomic: move drm_atomic_helper_disable_all() and drm_atomic_helper_shutdown() from drm_atomic_helper to drm_atomic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Outdated function references in kernel-doc blocks after rename.
- [Low] Use of return with an expression in a void function.
--

commit 8c33c13c9d809fde9c0dc4ffcc59c09b05d47d10
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/atomic: move drm_atomic_helper_disable_all() and drm_atomic_helper_shutdown() from drm_atomic_helper to drm_atomic

This commit moves drm_atomic_helper_shutdown() and
drm_atomic_helper_disable_all() to the drm_atomic module, renaming them to
drop the "_helper" infix. The move resolves a module dependency loop to
support upcoming DRM bridge hotplug features, replacing the original
functions with deprecated wrappers.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 170de30c28ae4..36fc57e147cb5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1987,6 +1987,121 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
[ ... ]
> + * This is used for example in suspend/resume to disable all currently active
> + * functions when suspending. If you just want to shut down everything at e.g.
> + * driver unload, look at drm_atomic_helper_shutdown().

This isn't a bug, but since the functions were renamed, should this
kernel-doc comment be updated to point to the new drm_atomic_shutdown()
function instead of the deprecated drm_atomic_helper_shutdown() wrapper?

[ ... ]
> + * See also:
> + * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> + * drm_atomic_helper_shutdown().
> + */
> +int drm_atomic_disable_all(struct drm_device *dev,

Similar to the above, should this refer to drm_atomic_shutdown()?

[ ... ]
> +/**
> + * drm_atomic_shutdown - shutdown all CRTC
> + * @dev: DRM device
> + *
> + * This shuts down all CRTC, which is useful for driver unloading. Shutdown on
> + * suspend should instead be handled with drm_atomic_helper_suspend(), since
> + * that also takes a snapshot of the modeset state to be restored on resume.
> + *
> + * This is just a convenience wrapper around drm_atomic_helper_disable_all(),
> + * and it is the atomic version of drm_helper_force_disable_all().
> + */
> +void drm_atomic_shutdown(struct drm_device *dev)

Should this documentation state that it is a wrapper around the new
drm_atomic_disable_all() rather than the deprecated
drm_atomic_helper_disable_all()?

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 51f39edc31edc..afedfab9b5682 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
[ ... ]
> @@ -3673,21 +3623,7 @@ EXPORT_SYMBOL(drm_atomic_helper_reset_crtc);
>   */
>  void drm_atomic_helper_shutdown(struct drm_device *dev)
>  {
> -	struct drm_modeset_acquire_ctx ctx;
> -	int ret;
> -
> -	if (dev == NULL)
> -		return;
> -
> -	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> -
> -	ret = drm_atomic_helper_disable_all(dev, &ctx);
> -	if (ret)
> -		drm_err(dev,
> -			"Disabling all crtc's during unload failed with %i\n",
> -			ret);
> -
> -	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> +	return drm_atomic_shutdown(dev);
>  }

Since both drm_atomic_helper_shutdown() and drm_atomic_shutdown() are declared
as returning void, does returning an expression here violate ISO C standards?
It might be better to just call drm_atomic_shutdown(dev) without the return
keyword to avoid compiler warnings from stricter configurations like sparse or
Clang.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-drm-bridge-hotplug-v1-0-45e2bdb3dfb4@bootlin.com?part=24