Hi Laurent,
Thank you for your feedback!
From: Laurent Pinchart laurent.pinchart@ideasonboard.com Sent: 02 August 2019 09:06 Subject: Re: [PATCH/RFC 05/12] drm: rcar-du: lvds: Add data swap support
Hi Fabrizio,
Thank you for the patch.
On Fri, Aug 02, 2019 at 08:34:02AM +0100, Fabrizio Castro wrote:
When in vertical stripe mode of operation, there is the option of swapping even data and odd data on the two LVDS interfaces used to drive the video output. Add data swap support by exposing a new DT property named "renesas,swap-data".
Signed-off-by: Fabrizio Castro fabrizio.castro@bp.renesas.com
drivers/gpu/drm/rcar-du/rcar_lvds.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 3aeaf9e..c306fab 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -69,6 +69,7 @@ struct rcar_lvds {
struct drm_bridge *companion; bool dual_link;
- bool stripe_swap_data;
};
#define bridge_to_rcar_lvds(bridge) \ @@ -439,12 +440,16 @@ static void rcar_lvds_enable(struct drm_bridge *bridge) rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
/** 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);
u32 lvdstripe = 0;if (lvds->dual_link)/** Configure vertical stripe based on the mode of* operation of the connected device.*/lvdstripe = LVDSTRIPE_ST_ON | (lvds->stripe_swap_data ?LVDSTRIPE_ST_SWAP : 0);Would the following be simpler ?
lvdstripe = (lvds->dual_link ? LVDSTRIPE_ST_ON : 0) | (lvds->stripe_swap_data ? LVDSTRIPE_ST_SWAP : 0);
rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
I actually think I need to rework this patch slightly, because the user manual says that ST_SWAP is reserved for LVD1STRIPE, so I need to make sure we don't set it for LVDS1.
So perhaps, this could translate to something like: If (lvds->dual_link) lvdstripe = LVDSTRIPE_ST_ON | (<swap-whatever> && lvds->companion) ? LVDSTRIPE_ST_SWAP : 0);
I don't think we should be setting LVDSTRIPE_ST_SWAP without LVDSTRIPE_ST_ON, this solution would allow us to test lvds->dual_link only once, and will prevent us from setting LVDSTRIPE_ST_SWAP if LVDSTRIPE_ST_ON is not set or if we are touching LVDS1.
What do you think?
Thanks, Fab
}
/* @@ -770,8 +775,12 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds) } }
- if (lvds->dual_link)
- if (lvds->dual_link) {
lvds->stripe_swap_data = of_property_read_bool(lvds->dev->of_node,"renesas,swap-data");- }
As explained in the review of the corresponding DT bindings, I think this should be queried from the remote device rather than specified in DT.
done: of_node_put(local_output);
-- Regards,
Laurent Pinchart