| Message ID | 1564731249-22671-6-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, 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); > } > > /* > @@ -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);
Hi Laurent, On Fri, Aug 2, 2019 at 10:06 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > 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> > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > @@ -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); From the point of view of "wc -l": yes. From the point of view of readability, I'd go for: if (lvds->dual_link) lvdstripe |= LVDSTRIPE_ST_ON; if (lvds->stripe_swap_data) lvdstripe |= LVDSTRIPE_ST_SWAP; > > + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe); > > } > > > > /* > > @@ -770,8 +775,12 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds) Gr{oetje,eeting}s, Geert
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"); > > 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
Hi Fabrizio, On Mon, Aug 05, 2019 at 09:32:42AM +0000, Fabrizio Castro wrote: > On 02 August 2019 09:06 Laurent Pinchart wrote: > > 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? I was thinking that lvds->stripe_swap_data should only be set when lvds->dual_link is set and lvds->companion is non-NULL, so both are roughly equivalent. It's a detail anyway, it doesn't matter too much. > > > } > > > > > > /* > > > @@ -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);
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); + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe); } /* @@ -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); + } done: of_node_put(local_output);
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(-)