Re: [PATCH v2 3/7] drm/bridge: Add drm_atomic_get_new_crtc_for_bridge() helper

From: Maxime Ripard <maxime@cerno.tech>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: dri-devel@lists.freedesktop.org,
	Andrzej Hajda <andrzej.hajda@intel.com>,
	Chun-Kuang Hu <chunkuang.hu@kernel.org>,
	Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Jitao Shi <jitao.shi@mediatek.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Philip Chen <philipchen@chromium.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Robert Foss <robert.foss@linaro.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Andrzej Hajda <a.hajda@samsung.com>
Subject: Re: [PATCH v2 3/7] drm/bridge: Add drm_atomic_get_new_crtc_for_bridge() helper
Date: Fri, 22 Oct 2021 13:03:55 +0200	[thread overview]
Message-ID: <20211022110355.3q2yhlaygz4lgzhe@gilmour> (raw)
In-Reply-To: <20211020181901.2114645-4-sam@ravnborg.org>

Hi,

On Wed, Oct 20, 2021 at 08:18:57PM +0200, Sam Ravnborg wrote:
> drm_atomic_get_new_crtc_for_bridge() will be used by bridge
> drivers to provide easy access to the mode from the
> drm_bridge_funcs operations.
> 
> The helper will be useful in the conversion to the atomic
> operations of struct drm_bridge_funcs.
> 
> v2:
>   - Renamed to drm_atomic_get_new_crtc_for_bridge (Maxime)
>   - Use atomic_state, not bridge_State (Maxime)
>   - Drop WARN on crtc_state as it is a valid case (Maxime)
>   - Added small code snip to help readers
>   - Updated description, fixed kernel-doc and exported the symbol
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h     |  3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ff1416cd609a..8b107194b157 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1134,6 +1134,48 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
>  
> +/**
> + * drm_atomic_get_new_crtc_for_bridge - get new crtc_state for the bridge
> + * @state: state of the bridge
> + * @bridge: bridge object

I appreciate that the function name is fairly long already, but given
its name I'd expect it to return a drm_crtc, not drm_crtc_state.

All the other state related functions are named using the pattern
drm_atomic_get_(old|new)_$object_state.

Moreover, we already have a drm_atomic_get_new_connector_for_encoder
function that does return a drm_connector, so I think we should make it
consistent there and call it drm_atomic_get_new_crtc_state_for_bridge

Maxime

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek