Hi Laurent, Thank you for your feedback!
> From: Laurent Pinchart <[email protected]> > 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 <[email protected]> > > --- > > 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"); > > ret = rcar_lvds_parse_dt_companion(lvds); > > + } > > 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
