| Message ID | 1575649974-31472-4-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Add dual-LVDS panel support to EK874 | expand |
Hi Fabrizio, Thank you for the patch. On Fri, Dec 06, 2019 at 04:32:50PM +0000, Fabrizio Castro wrote: > For dual-LVDS configurations, it is now possible to mark the > DT port nodes for the sink with boolean properties (like > dual-lvds-even-pixels and dual-lvds-odd-pixels) to let drivers > know the encoders need to be configured in dual-LVDS mode. > > Rework the implementation of rcar_lvds_parse_dt_companion > to make use of the DT markers while keeping backward > compatibility. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > --- > v3->v4: > * New patch extracted from patch: > "drm: rcar-du: lvds: Add dual-LVDS panels support" > --- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 56 +++++++++++++++++++++++++++++++------ > 1 file changed, 47 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index 3cb0a83..6c1f171 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -669,8 +669,10 @@ EXPORT_SYMBOL_GPL(rcar_lvds_dual_link); > static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > { > const struct of_device_id *match; > - struct device_node *companion; > + struct device_node *companion, *p0, *p1; Could you rename p0 and p1 to port0 and port1, and spit them to a separate line of variable declaration ? > + struct rcar_lvds *companion_lvds; > struct device *dev = lvds->dev; > + int dual_link; > int ret = 0; > > /* Locate the companion LVDS encoder for dual-link operation, if any. */ > @@ -689,13 +691,55 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > goto done; > } > > + /* > + * We need to work out if the sink is expecting us to function in > + * dual-link mode. We do this by looking at the DT port nodes we are > + * connected to, if they are marked as expecting even pixels and > + * odd pixels than we need to enable vertical stripe output. > + */ > + p0 = of_graph_get_port_by_id(dev->of_node, 1); > + p1 = of_graph_get_port_by_id(companion, 1); > + dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1); > + of_node_put(p0); > + of_node_put(p1); > + if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) { > + lvds->dual_link = true; > + } else if (lvds->next_bridge && lvds->next_bridge->timings) { > + /* > + * Early dual-link bridge specific implementations populate the > + * timings field of drm_bridge, read the dual_link flag off the > + * bridge directly for backward compatibility. > + */ > + lvds->dual_link = lvds->next_bridge->timings->dual_link; > + } > + > + if (!lvds->dual_link) { > + dev_dbg(dev, "Single-link configuration detected\n"); > + goto done; > + } > + > lvds->companion = of_drm_find_bridge(companion); > if (!lvds->companion) { > ret = -EPROBE_DEFER; > goto done; > } > > - dev_dbg(dev, "Found companion encoder %pOF\n", companion); > + dev_dbg(dev, > + "Dual-link configuration detected (companion encoder %pOF)\n", > + companion); > + > + companion_lvds = bridge_to_rcar_lvds(lvds->companion); Could you move this line after the FIXME comment ? With these small issues fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + /* > + * FIXME: We should not be messing with the companion encoder private > + * data from the primary encoder, we should rather let the companion > + * encoder work things out on its own. However, the companion encoder > + * doesn't hold a reference to the primary encoder, and > + * drm_of_lvds_get_dual_link_pixel_order needs to be given references > + * to the output ports of both encoders, therefore leave it like this > + * for the time being. > + */ > + companion_lvds->dual_link = true; > > done: > of_node_put(companion); > @@ -739,13 +783,7 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds) > if (ret) > goto done; > > - if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) && > - lvds->next_bridge) > - lvds->dual_link = lvds->next_bridge->timings > - ? lvds->next_bridge->timings->dual_link > - : false; > - > - if (lvds->dual_link) > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) > ret = rcar_lvds_parse_dt_companion(lvds); > > done:
Hi Laurent, Thank you for your feedback! > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 13 December 2019 21:30 > Subject: Re: [PATCH v4 3/7] drm: rcar-du: lvds: Get dual link configuration from DT > > Hi Fabrizio, > > Thank you for the patch. > > On Fri, Dec 06, 2019 at 04:32:50PM +0000, Fabrizio Castro wrote: > > For dual-LVDS configurations, it is now possible to mark the > > DT port nodes for the sink with boolean properties (like > > dual-lvds-even-pixels and dual-lvds-odd-pixels) to let drivers > > know the encoders need to be configured in dual-LVDS mode. > > > > Rework the implementation of rcar_lvds_parse_dt_companion > > to make use of the DT markers while keeping backward > > compatibility. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > --- > > v3->v4: > > * New patch extracted from patch: > > "drm: rcar-du: lvds: Add dual-LVDS panels support" > > --- > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 56 +++++++++++++++++++++++++++++++------ > > 1 file changed, 47 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > index 3cb0a83..6c1f171 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > @@ -669,8 +669,10 @@ EXPORT_SYMBOL_GPL(rcar_lvds_dual_link); > > static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > > { > > const struct of_device_id *match; > > - struct device_node *companion; > > + struct device_node *companion, *p0, *p1; > > Could you rename p0 and p1 to port0 and port1, and spit them to a > separate line of variable declaration ? sure > > > + struct rcar_lvds *companion_lvds; > > struct device *dev = lvds->dev; > > + int dual_link; > > int ret = 0; > > > > /* Locate the companion LVDS encoder for dual-link operation, if any. */ > > @@ -689,13 +691,55 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > > goto done; > > } > > > > + /* > > + * We need to work out if the sink is expecting us to function in > > + * dual-link mode. We do this by looking at the DT port nodes we are > > + * connected to, if they are marked as expecting even pixels and > > + * odd pixels than we need to enable vertical stripe output. > > + */ > > + p0 = of_graph_get_port_by_id(dev->of_node, 1); > > + p1 = of_graph_get_port_by_id(companion, 1); > > + dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1); > > + of_node_put(p0); > > + of_node_put(p1); > > + if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) { > > + lvds->dual_link = true; > > + } else if (lvds->next_bridge && lvds->next_bridge->timings) { > > + /* > > + * Early dual-link bridge specific implementations populate the > > + * timings field of drm_bridge, read the dual_link flag off the > > + * bridge directly for backward compatibility. > > + */ > > + lvds->dual_link = lvds->next_bridge->timings->dual_link; > > + } > > + > > + if (!lvds->dual_link) { > > + dev_dbg(dev, "Single-link configuration detected\n"); > > + goto done; > > + } > > + > > lvds->companion = of_drm_find_bridge(companion); > > if (!lvds->companion) { > > ret = -EPROBE_DEFER; > > goto done; > > } > > > > - dev_dbg(dev, "Found companion encoder %pOF\n", companion); > > + dev_dbg(dev, > > + "Dual-link configuration detected (companion encoder %pOF)\n", > > + companion); > > + > > + companion_lvds = bridge_to_rcar_lvds(lvds->companion); > > Could you move this line after the FIXME comment ? Will do Thanks, Fab > > With these small issues fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > + /* > > + * FIXME: We should not be messing with the companion encoder private > > + * data from the primary encoder, we should rather let the companion > > + * encoder work things out on its own. However, the companion encoder > > + * doesn't hold a reference to the primary encoder, and > > + * drm_of_lvds_get_dual_link_pixel_order needs to be given references > > + * to the output ports of both encoders, therefore leave it like this > > + * for the time being. > > + */ > > + companion_lvds->dual_link = true; > > > > done: > > of_node_put(companion); > > @@ -739,13 +783,7 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds) > > if (ret) > > goto done; > > > > - if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) && > > - lvds->next_bridge) > > - lvds->dual_link = lvds->next_bridge->timings > > - ? lvds->next_bridge->timings->dual_link > > - : false; > > - > > - if (lvds->dual_link) > > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) > > ret = rcar_lvds_parse_dt_companion(lvds); > > > > done: > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 3cb0a83..6c1f171 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -669,8 +669,10 @@ EXPORT_SYMBOL_GPL(rcar_lvds_dual_link); static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) { const struct of_device_id *match; - struct device_node *companion; + struct device_node *companion, *p0, *p1; + struct rcar_lvds *companion_lvds; struct device *dev = lvds->dev; + int dual_link; int ret = 0; /* Locate the companion LVDS encoder for dual-link operation, if any. */ @@ -689,13 +691,55 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) goto done; } + /* + * We need to work out if the sink is expecting us to function in + * dual-link mode. We do this by looking at the DT port nodes we are + * connected to, if they are marked as expecting even pixels and + * odd pixels than we need to enable vertical stripe output. + */ + p0 = of_graph_get_port_by_id(dev->of_node, 1); + p1 = of_graph_get_port_by_id(companion, 1); + dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1); + of_node_put(p0); + of_node_put(p1); + if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) { + lvds->dual_link = true; + } else if (lvds->next_bridge && lvds->next_bridge->timings) { + /* + * Early dual-link bridge specific implementations populate the + * timings field of drm_bridge, read the dual_link flag off the + * bridge directly for backward compatibility. + */ + lvds->dual_link = lvds->next_bridge->timings->dual_link; + } + + if (!lvds->dual_link) { + dev_dbg(dev, "Single-link configuration detected\n"); + goto done; + } + lvds->companion = of_drm_find_bridge(companion); if (!lvds->companion) { ret = -EPROBE_DEFER; goto done; } - dev_dbg(dev, "Found companion encoder %pOF\n", companion); + dev_dbg(dev, + "Dual-link configuration detected (companion encoder %pOF)\n", + companion); + + companion_lvds = bridge_to_rcar_lvds(lvds->companion); + + /* + * FIXME: We should not be messing with the companion encoder private + * data from the primary encoder, we should rather let the companion + * encoder work things out on its own. However, the companion encoder + * doesn't hold a reference to the primary encoder, and + * drm_of_lvds_get_dual_link_pixel_order needs to be given references + * to the output ports of both encoders, therefore leave it like this + * for the time being. + */ + companion_lvds->dual_link = true; done: of_node_put(companion); @@ -739,13 +783,7 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds) if (ret) goto done; - if ((lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) && - lvds->next_bridge) - lvds->dual_link = lvds->next_bridge->timings - ? lvds->next_bridge->timings->dual_link - : false; - - if (lvds->dual_link) + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) ret = rcar_lvds_parse_dt_companion(lvds); done:
For dual-LVDS configurations, it is now possible to mark the DT port nodes for the sink with boolean properties (like dual-lvds-even-pixels and dual-lvds-odd-pixels) to let drivers know the encoders need to be configured in dual-LVDS mode. Rework the implementation of rcar_lvds_parse_dt_companion to make use of the DT markers while keeping backward compatibility. Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- v3->v4: * New patch extracted from patch: "drm: rcar-du: lvds: Add dual-LVDS panels support" --- drivers/gpu/drm/rcar-du/rcar_lvds.c | 56 +++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 9 deletions(-)