Re: [PATCH 2/3] drm: rcar-du: lvds: Move LVDS enable code to separate code section

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 14/02/2023 02:37, Laurent Pinchart wrote:
> To prepare for a rework of the LVDS disable code, which will need to be
> called from rcar_lvds_pclk_disable(), move the LVDS enable code,
> currently stored in the __rcar_lvds_atomic_enable() function, to a
> separate code section separate from bridge operations. It will be then
> extended with the LVDS disable code.
>
> As part of this rework the __rcar_lvds_atomic_enable() function is
> renamed to rcar_lvds_enable() to more clearly indicate its purpose.
>
> No functional change intended.

The desc is a bit confusing, as it talks about moving LVDS enable code, 
but the diff shows you moving the lvds pclk enable/disable code. Other 
than that:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>

 Tomi

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/rcar-du/rcar_lvds.c | 97 +++++++++++++++--------------
>   1 file changed, 50 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 61de18af62e6..70cdd5ec64d5 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -311,46 +311,7 @@ static void rcar_lvds_pll_setup_d3_e3(struct rcar_lvds *lvds, unsigned int freq)
>   }
>   
>   /* -----------------------------------------------------------------------------
> - * Clock - D3/E3 only
> - */
> -
> -int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq)
> -{
> -	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> -	int ret;
> -
> -	if (WARN_ON(!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)))
> -		return -ENODEV;
> -
> -	dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq);
> -
> -	ret = pm_runtime_resume_and_get(lvds->dev);
> -	if (ret)
> -		return ret;
> -
> -	__rcar_lvds_pll_setup_d3_e3(lvds, freq, true);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(rcar_lvds_pclk_enable);
> -
> -void rcar_lvds_pclk_disable(struct drm_bridge *bridge)
> -{
> -	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> -
> -	if (WARN_ON(!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)))
> -		return;
> -
> -	dev_dbg(lvds->dev, "disabling LVDS PLL\n");
> -
> -	rcar_lvds_write(lvds, LVDPLLCR, 0);
> -
> -	pm_runtime_put_sync(lvds->dev);
> -}
> -EXPORT_SYMBOL_GPL(rcar_lvds_pclk_disable);
> -
> -/* -----------------------------------------------------------------------------
> - * Bridge
> + * Enable/disable
>    */
>   
>   static enum rcar_lvds_mode rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds,
> @@ -394,10 +355,10 @@ static enum rcar_lvds_mode rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds,
>   	return mode;
>   }
>   
> -static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> -				      struct drm_atomic_state *state,
> -				      struct drm_crtc *crtc,
> -				      struct drm_connector *connector)
> +static void rcar_lvds_enable(struct drm_bridge *bridge,
> +			     struct drm_atomic_state *state,
> +			     struct drm_crtc *crtc,
> +			     struct drm_connector *connector)
>   {
>   	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>   	u32 lvdhcr;
> @@ -410,8 +371,7 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
>   
>   	/* Enable the companion LVDS encoder in dual-link mode. */
>   	if (lvds->link_type != RCAR_LVDS_SINGLE_LINK && lvds->companion)
> -		__rcar_lvds_atomic_enable(lvds->companion, state, crtc,
> -					  connector);
> +		rcar_lvds_enable(lvds->companion, state, crtc, connector);
>   
>   	/*
>   	 * Hardcode the channels and control signals routing for now.
> @@ -531,6 +491,49 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
>   	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
>   }
>   
> +/* -----------------------------------------------------------------------------
> + * Clock - D3/E3 only
> + */
> +
> +int rcar_lvds_pclk_enable(struct drm_bridge *bridge, unsigned long freq)
> +{
> +	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> +	int ret;
> +
> +	if (WARN_ON(!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)))
> +		return -ENODEV;
> +
> +	dev_dbg(lvds->dev, "enabling LVDS PLL, freq=%luHz\n", freq);
> +
> +	ret = pm_runtime_resume_and_get(lvds->dev);
> +	if (ret)
> +		return ret;
> +
> +	__rcar_lvds_pll_setup_d3_e3(lvds, freq, true);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(rcar_lvds_pclk_enable);
> +
> +void rcar_lvds_pclk_disable(struct drm_bridge *bridge)
> +{
> +	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> +
> +	if (WARN_ON(!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)))
> +		return;
> +
> +	dev_dbg(lvds->dev, "disabling LVDS PLL\n");
> +
> +	rcar_lvds_write(lvds, LVDPLLCR, 0);
> +
> +	pm_runtime_put_sync(lvds->dev);
> +}
> +EXPORT_SYMBOL_GPL(rcar_lvds_pclk_disable);
> +
> +/* -----------------------------------------------------------------------------
> + * Bridge
> + */
> +
>   static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
>   				    struct drm_bridge_state *old_bridge_state)
>   {
> @@ -542,7 +545,7 @@ static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
>   							     bridge->encoder);
>   	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
>   
> -	__rcar_lvds_atomic_enable(bridge, state, crtc, connector);
> +	rcar_lvds_enable(bridge, state, crtc, connector);
>   }
>   
>   static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,