RE: [PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap

[Fabrizio Castro <fabrizio.castro@bp.renesas.com>]

Hi Laurent,

Thank you for your feedback!

> From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 13 December 2019 21:41
> Subject: Re: [PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Fri, Dec 06, 2019 at 04:32:51PM +0000, Fabrizio Castro wrote:
> > DT properties dual-lvds-even-pixels and dual-lvds-odd-pixels
> > can be used to work out if the driver needs to swap even
> > and odd pixels around.
> >
> > This patch makes use of the return value from function
> > drm_of_lvds_get_dual_link_pixel_order to determine if we
> > need to swap odd and even pixels around for things to work
> > properly.
> >
> > 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 | 46 +++++++++++++++++++++++++++++--------
> >  1 file changed, 37 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 6c1f171..cb2147c 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -71,6 +71,7 @@ struct rcar_lvds {
> >
> >  	struct drm_bridge *companion;
> >  	bool dual_link;
> > +	bool stripe_swap_data;
> 
> Should we merge those two fields in an int dual_link that would be set
> to DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS,
> DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS or a negative value (or maybe we the
> value of enum drm_lvds_dual_link_pixels could be modified so that 0
> could mean single link) ?

I see your point, and I think there is a third option, using definitions local to the
RCAR LVDS driver (via an enum?).
The reason for thinking about a third option is that option 1 could be a bit error prone,
as negative values usually have special meaning. I like option 2, I find it neat, but if
I did that then we would need to agree again on names, definitions, interfaces, etc.,
as the meaning of things will change slightly. Also, we will probably be fine with the
helper and definitions we already agreed on.

I think option 3 will offer a little bit of decoupling between the helper and the driver,
and should have a limited effect on previous uses of dual_link.

I'll make option 3 related changes in v5, if you don't like them then I think we should
try option 2.

> 
> >  };
> >
> >  #define bridge_to_rcar_lvds(b) \
> > @@ -441,12 +442,20 @@ 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.
> > +			 *
> > +			 * ST_SWAP from LVD1STRIPE is reserved, do not set
> > +			 * in the companion LVDS
> 
> Maybe "ST_SWAP is reserved for the companion encoder, only set it in the
> primary encoder." ?

sure

> 
> > +			 */
> > +			lvdstripe = LVDSTRIPE_ST_ON |
> > +				(lvds->companion && lvds->stripe_swap_data ?
> > +				 LVDSTRIPE_ST_SWAP : 0);
> 
> To match the coding style of the rest of the driver,

ok

> 
> 			lvdstripe = LVDSTRIPE_ST_ON
> 				  | (lvds->companion && lvds->stripe_swap_data ?
> 				     LVDSTRIPE_ST_SWAP : 0);
> 
> Even though not strictly required, could you surround this statement
> with { } as it spans quite a few lines with the comment ?

Will rework this slightly anyway to make room for option 3.

> 
> > +		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> >  	}
> >
> >  	/*
> > @@ -702,17 +711,33 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  	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) {
> > +
> > +	switch (dual_link) {
> > +	case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
> > +		/*
> > +		 * By default we generate even pixels from this encoder and odd
> > +		 * pixels from the companion encoder, but since p0 is connected
> > +		 * to the port expecting ood pixels, and p1 is connected to the
> > +		 * port expecting even pixels, we need to swap even and odd
> > +		 * pixels around.
> > +		 */
> > +		lvds->stripe_swap_data = true;
> > +		lvds->dual_link = true;
> > +		break;
> > +	case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
> >  		lvds->dual_link = true;
> > -	} else if (lvds->next_bridge && lvds->next_bridge->timings) {
> > +		break;
> > +	default:
> >  		/*
> >  		 * 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->next_bridge && lvds->next_bridge->timings)
> > +			lvds->dual_link = lvds->next_bridge->timings->dual_link;
> >  	}
> >
> > +
> 
> A single blank line is enough.

Oops

Thanks,
Fab

> 
> >  	if (!lvds->dual_link) {
> >  		dev_dbg(dev, "Single-link configuration detected\n");
> >  		goto done;
> > @@ -728,6 +753,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  		"Dual-link configuration detected (companion encoder %pOF)\n",
> >  		companion);
> >
> > +	if (lvds->stripe_swap_data)
> > +		dev_dbg(dev, "Data swapping required\n");
> > +
> >  	companion_lvds = bridge_to_rcar_lvds(lvds->companion);
> >
> >  	/*
> 
> --
> Regards,
> 
> Laurent Pinchart