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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> + 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
