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);
+ 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);
--
2.7.4
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);
> }
>
> /*
> @@ -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 Laurent,
On Fri, Aug 2, 2019 at 10:06 AM Laurent Pinchart
<[email protected]> 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 <[email protected]>
> > --- 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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
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 <[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?
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);
--
Regards,
Laurent Pinchart