| Message ID | 1df5a7bcafa091e008edb439ee9de4262ae4d5d1.1596101672.git-series.maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: Add support for dual-link LVDS on the A20 | expand |
On Thu, Jul 30, 2020 at 5:35 PM Maxime Ripard <maxime@cerno.tech> 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@cerno.tech> Reviewed-by: Chen-Yu Tsai <wens@csie.org> > --- > 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")) > + return sun4i_rgb_init(drm, tcon); Slightly related: IIRC there are a few LVDS panels supported in panel-simple so they don't use the panel-lvds compatible. Any idea how to deal with those?
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@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@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; > }
Hi Chen-Yu, Sorry for the delay On Sat, Aug 29, 2020 at 02:43:53PM +0800, Chen-Yu Tsai wrote: > > +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")) > > + return sun4i_rgb_init(drm, tcon); > > Slightly related: IIRC there are a few LVDS panels supported in panel-simple > so they don't use the panel-lvds compatible. Any idea how to deal with those? I agree that this is an issue, I'm not entirely sure how to fix it. The proper fix would be to have multiple output ports, one for each output type, but given how our current binding looks like I'm not sure how we could retro-fit that without some horror-looking code. Maybe we can add a property on the endpoint? Maxime
On Mon, Oct 5, 2020 at 10:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Chen-Yu, > > Sorry for the delay > > On Sat, Aug 29, 2020 at 02:43:53PM +0800, Chen-Yu Tsai wrote: > > > +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")) > > > + return sun4i_rgb_init(drm, tcon); > > > > Slightly related: IIRC there are a few LVDS panels supported in panel-simple > > so they don't use the panel-lvds compatible. Any idea how to deal with those? > > I agree that this is an issue, I'm not entirely sure how to fix it. The > proper fix would be to have multiple output ports, one for each output > type, but given how our current binding looks like I'm not sure how we > could retro-fit that without some horror-looking code. > > Maybe we can add a property on the endpoint? I guess that works. :) Since the LCD and LVDS outputs use the same pins, it's not like we really would have multiple output ports. ChenYu
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")) + 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. + * + * 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"); + 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; }
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@cerno.tech> --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 139 +++++++++++++++--------------- 1 file changed, 70 insertions(+), 69 deletions(-)