[PATCH v2 2/4] drm/sun4i: tcon: Refactor the LVDS and panel probing

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

On Thu, Jul 30, 2020 at 11:35:02AM +0200, Maxime Ripard wrote:
> The current code to parse the DT, deal with the older device trees, and
> register either the RGB or LVDS output has so far grown organically into
> the bind function and has become quite hard to extend properly.
> 
> Let's move it into a single function that grabs all the resources it needs
> and registers the proper panel output.
> 
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 139 +++++++++++++++---------------
>  1 file changed, 70 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 2a5a9903c4c6..d03ad75f9900 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -875,6 +875,75 @@ static int sun4i_tcon_init_regmap(struct device *dev,
>  	return 0;
>  }
>  
> +static int sun4i_tcon_register_panel(struct drm_device *drm,
> +				     struct sun4i_tcon *tcon)
> +{
> +	struct device_node *companion;
> +	struct device_node *remote;
> +	struct device *dev = tcon->dev;
> +	bool has_lvds_alt;
> +	bool has_lvds_rst;
> +	int ret;
> +
> +	/*
> +	 * If we have an LVDS panel connected to the TCON, we should
> +	 * just probe the LVDS connector. Otherwise, let's just register
> +	 * an RGB panel.
> +	 */
> +	remote = of_graph_get_remote_node(dev->of_node, 1, 0);
> +	if (!tcon->quirks->supports_lvds ||
> +	    !of_device_is_compatible(remote, "panel-lvds"))

This isn't very nice :-S Not a candidate for a fix in this patch, but
something that should be addressed in the future. As Chen-Yu mentioned,
there are LVDS panels supported by the panel-simple driver.

> +		return sun4i_rgb_init(drm, tcon);
> +
> +	/*
> +	 * This can only be made optional since we've had DT
> +	 * nodes without the LVDS reset properties.
> +	 *
> +	 * If the property is missing, just disable LVDS, and
> +	 * print a warning.
> +	 */
> +	tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds");
> +	if (IS_ERR(tcon->lvds_rst)) {
> +		dev_err(dev, "Couldn't get our reset line\n");
> +		return PTR_ERR(tcon->lvds_rst);
> +	} else if (tcon->lvds_rst) {
> +		has_lvds_rst = true;
> +		reset_control_reset(tcon->lvds_rst);
> +	} else {
> +		has_lvds_rst = false;
> +	}
> +
> +	/*
> +	 * This can only be made optional since we've had DT
> +	 * nodes without the LVDS reset properties.

Shouldn't this mention clock, not reset ?

> +	 *
> +	 * If the property is missing, just disable LVDS, and
> +	 * print a warning.
> +	 */
> +	if (tcon->quirks->has_lvds_alt) {
> +		tcon->lvds_pll = devm_clk_get(dev, "lvds-alt");
> +		if (IS_ERR(tcon->lvds_pll)) {
> +			if (PTR_ERR(tcon->lvds_pll) == -ENOENT) {
> +				has_lvds_alt = false;
> +			} else {
> +				dev_err(dev, "Couldn't get the LVDS PLL\n");
> +				return PTR_ERR(tcon->lvds_pll);
> +			}
> +		} else {
> +			has_lvds_alt = true;
> +		}
> +	}
> +
> +	if (!has_lvds_rst ||
> +	    (tcon->quirks->has_lvds_alt && !has_lvds_alt)) {
> +		dev_warn(dev, "Missing LVDS properties, Please upgrade your DT\n");
> +		dev_warn(dev, "LVDS output disabled\n");

Would it make sense to move this to the has_lvds_rst = false and
has_lvds_alt = false code sections about ? You could then print which
property is missing.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		return -ENODEV;
> +	}
> +
> +	return sun4i_lvds_init(drm, tcon);
> +}
> +
>  /*
>   * On SoCs with the old display pipeline design (Display Engine 1.0),
>   * the TCON is always tied to just one backend. Hence we can traverse
> @@ -1122,10 +1191,8 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  	struct drm_device *drm = data;
>  	struct sun4i_drv *drv = drm->dev_private;
>  	struct sunxi_engine *engine;
> -	struct device_node *remote;
>  	struct sun4i_tcon *tcon;
>  	struct reset_control *edp_rstc;
> -	bool has_lvds_rst, has_lvds_alt, can_lvds;
>  	int ret;
>  
>  	engine = sun4i_tcon_find_engine(drv, dev->of_node);
> @@ -1170,58 +1237,6 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  		return ret;
>  	}
>  
> -	if (tcon->quirks->supports_lvds) {
> -		/*
> -		 * This can only be made optional since we've had DT
> -		 * nodes without the LVDS reset properties.
> -		 *
> -		 * If the property is missing, just disable LVDS, and
> -		 * print a warning.
> -		 */
> -		tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds");
> -		if (IS_ERR(tcon->lvds_rst)) {
> -			dev_err(dev, "Couldn't get our reset line\n");
> -			return PTR_ERR(tcon->lvds_rst);
> -		} else if (tcon->lvds_rst) {
> -			has_lvds_rst = true;
> -			reset_control_reset(tcon->lvds_rst);
> -		} else {
> -			has_lvds_rst = false;
> -		}
> -
> -		/*
> -		 * This can only be made optional since we've had DT
> -		 * nodes without the LVDS reset properties.
> -		 *
> -		 * If the property is missing, just disable LVDS, and
> -		 * print a warning.
> -		 */
> -		if (tcon->quirks->has_lvds_alt) {
> -			tcon->lvds_pll = devm_clk_get(dev, "lvds-alt");
> -			if (IS_ERR(tcon->lvds_pll)) {
> -				if (PTR_ERR(tcon->lvds_pll) == -ENOENT) {
> -					has_lvds_alt = false;
> -				} else {
> -					dev_err(dev, "Couldn't get the LVDS PLL\n");
> -					return PTR_ERR(tcon->lvds_pll);
> -				}
> -			} else {
> -				has_lvds_alt = true;
> -			}
> -		}
> -
> -		if (!has_lvds_rst ||
> -		    (tcon->quirks->has_lvds_alt && !has_lvds_alt)) {
> -			dev_warn(dev, "Missing LVDS properties, Please upgrade your DT\n");
> -			dev_warn(dev, "LVDS output disabled\n");
> -			can_lvds = false;
> -		} else {
> -			can_lvds = true;
> -		}
> -	} else {
> -		can_lvds = false;
> -	}
> -
>  	ret = sun4i_tcon_init_clocks(dev, tcon);
>  	if (ret) {
>  		dev_err(dev, "Couldn't init our TCON clocks\n");
> @@ -1256,21 +1271,7 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  	}
>  
>  	if (tcon->quirks->has_channel_0) {
> -		/*
> -		 * If we have an LVDS panel connected to the TCON, we should
> -		 * just probe the LVDS connector. Otherwise, just probe RGB as
> -		 * we used to.
> -		 */
> -		remote = of_graph_get_remote_node(dev->of_node, 1, 0);
> -		if (of_device_is_compatible(remote, "panel-lvds"))
> -			if (can_lvds)
> -				ret = sun4i_lvds_init(drm, tcon);
> -			else
> -				ret = -EINVAL;
> -		else
> -			ret = sun4i_rgb_init(drm, tcon);
> -		of_node_put(remote);
> -
> +		ret = sun4i_tcon_register_panel(drm, tcon);
>  		if (ret < 0)
>  			goto err_free_dotclock;
>  	}

-- 
Regards,

Laurent Pinchart