Hi Laurent,
a small note.
On Sun, May 12, 2019 at 12:06:58AM +0300, Laurent Pinchart wrote:
> In dual-link mode the LVDS0 encoder transmits even-numbered pixels, and
> sends odd-numbered pixels to the LVDS1 encoder for transmission on a
> separate link.
>
> To implement support for this mode of operation, determine if the LVDS
> connection operates in dual-link mode by querying the next device in the
> pipeline, locate the companion encoder, and control it directly through
> its bridge operations.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> ---
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 104 ++++++++++++++++++++++++----
> drivers/gpu/drm/rcar-du/rcar_lvds.h | 5 ++
> 2 files changed, 96 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index a331f0c32187..f7e4710fe33f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -66,6 +66,9 @@ struct rcar_lvds {
>
> struct drm_display_mode display_mode;
> enum rcar_lvds_mode mode;
> +
> + struct drm_bridge *companion;
> + bool dual_link;
> };
>
> #define bridge_to_rcar_lvds(bridge) \
> @@ -400,11 +403,6 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> {
> struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> const struct drm_display_mode *mode = &lvds->display_mode;
> - /*
> - * FIXME: We should really retrieve the CRTC through the state, but how
> - * do we get a state pointer?
> - */
> - struct drm_crtc *crtc = lvds->bridge.encoder->crtc;
> u32 lvdhcr;
> u32 lvdcr0;
> int ret;
> @@ -413,6 +411,10 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> if (ret < 0)
> return;
>
> + /* Enable the companion LVDS encoder in dual-link mode. */
> + if (lvds->dual_link && lvds->companion)
> + lvds->companion->funcs->enable(lvds->companion);
> +
> /*
> * Hardcode the channels and control signals routing for now.
> *
> @@ -435,17 +437,33 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
>
> if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> - /* Disable dual-link mode. */
> - rcar_lvds_write(lvds, LVDSTRIPE, 0);
> + /*
> + * Configure vertical stripe based on the mode of operation of
> + * the connected device.
> + */
> + rcar_lvds_write(lvds, LVDSTRIPE,
> + lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> }
>
> - /* PLL clock configuration. */
> - lvds->info->pll_setup(lvds, mode->clock * 1000);
> + /*
> + * PLL clock configuration on all instances but the companion in
> + * dual-link mode.
> + */
> + if (!lvds->dual_link || lvds->companion)
> + lvds->info->pll_setup(lvds, mode->clock * 1000);
>
> /* Set the LVDS mode and select the input. */
> lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
> - if (drm_crtc_index(crtc) == 2)
> - lvdcr0 |= LVDCR0_DUSEL;
> +
> + if (lvds->bridge.encoder) {
> + /*
> + * FIXME: We should really retrieve the CRTC through the state,
> + * but how do we get a state pointer?
> + */
> + if (drm_crtc_index(lvds->bridge.encoder->crtc) == 2)
> + lvdcr0 |= LVDCR0_DUSEL;
> + }
> +
> rcar_lvds_write(lvds, LVDCR0, lvdcr0);
>
> /* Turn all the channels on. */
> @@ -512,6 +530,10 @@ static void rcar_lvds_disable(struct drm_bridge *bridge)
> rcar_lvds_write(lvds, LVDCR1, 0);
> rcar_lvds_write(lvds, LVDPLLCR, 0);
>
> + /* Disable the companion LVDS encoder in dual-link mode. */
> + if (lvds->dual_link && lvds->companion)
> + lvds->companion->funcs->disable(lvds->companion);
> +
> clk_disable_unprepare(lvds->clocks.mod);
> }
>
> @@ -628,10 +650,54 @@ static const struct drm_bridge_funcs
> rcar_lvds_bridge_ops = {
> .mode_set = rcar_lvds_mode_set,
> };
>
> +bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> +{
> + struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> +
> + return lvds->dual_link;
> +}
> +EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
> +
> /*
> -----------------------------------------------------------------------------
> * Probe & Remove
> */
>
> +static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> +{
> + const struct of_device_id *match;
> + struct device_node *companion;
> + struct device *dev = lvds->dev;
> + int ret = 0;
> +
> + /* Locate the companion LVDS encoder for dual-link operation, if any. */
> + companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
> + if (!companion)
> + return -ENODEV;
> +
> + /*
> + * Sanity check: the companion encoder must have the same compatible
> + * string.
> + */
> + match = of_match_device(dev->driver->of_match_table, dev);
> + if (!of_device_is_compatible(companion, match->compatible)) {
> + ret = -ENODEV;
> + 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);
> +
> +done:
> + of_node_put(companion);
> +
> + return ret;
> +}
> +
> static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> {
> struct device_node *local_output = NULL;
> @@ -682,14 +748,26 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>
> if (is_bridge) {
> lvds->next_bridge = of_drm_find_bridge(remote);
> - if (!lvds->next_bridge)
> + if (!lvds->next_bridge) {
> ret = -EPROBE_DEFER;
> + goto done;
> + }
> +
> + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> + lvds->dual_link = lvds->next_bridge->timings
> + ? lvds->next_bridge->timings->dual_link
> + : false;
> } else {
> lvds->panel = of_drm_find_panel(remote);
> - if (IS_ERR(lvds->panel))
> + if (IS_ERR(lvds->panel)) {
> ret = PTR_ERR(lvds->panel);
> + goto done;
> + }
> }
>
> + if (lvds->dual_link)
> + ret = rcar_lvds_parse_dt_companion(lvds);
Looking at the error path here below, for E3/D3, -ENODEV gets
sanitized to return 0 as we want this method to return success even if
no endpoint is there, when using the LVDS encoder provided clock to
back-feed the DU.
This is not the case for the companion property imho. If the property is
specified, it should be sane and -ENODEV is worth being propagated to
the caller.
You could move the error handling bits in the error path:
/*
* On D3/E3 the LVDS encoder provides a clock to the DU, which can be
* used for the DPAD output even when the LVDS output is not connected.
* Don't fail probe in that case as the DU will need the bridge to
* control the clock.
*/
if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
return ret == -ENODEV ? 0 : ret;
before the of_node_put() sequences, and add one more label, to skip
the above part in case parse_dt_companion() fails.
Apart from this you can retain my tag if you like.
Thanks
j
> +
> done:
> of_node_put(local_output);
> of_node_put(remote_input);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.h
> b/drivers/gpu/drm/rcar-du/rcar_lvds.h
> index a709cae1bc32..222ec0e60785 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.h
> @@ -15,6 +15,7 @@ struct drm_bridge;
> #if IS_ENABLED(CONFIG_DRM_RCAR_LVDS)
> int rcar_lvds_clk_enable(struct drm_bridge *bridge, unsigned long freq);
> void rcar_lvds_clk_disable(struct drm_bridge *bridge);
> +bool rcar_lvds_dual_link(struct drm_bridge *bridge);
> #else
> static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
> unsigned long freq)
> @@ -22,6 +23,10 @@ static inline int rcar_lvds_clk_enable(struct drm_bridge
> *bridge,
> return -ENOSYS;
> }
> static inline void rcar_lvds_clk_disable(struct drm_bridge *bridge) { }
> +static inline bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> +{
> + return false;
> +}
> #endif /* CONFIG_DRM_RCAR_LVDS */
>
> #endif /* __RCAR_LVDS_H__ */
> --
> Regards,
>
> Laurent Pinchart
>
signature.asc
Description: PGP signature
