linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm: rcar_lvds: Fix dual link mode operations
@ 2019-07-23 16:57 Jacopo Mondi
  2019-07-24  7:28 ` Geert Uytterhoeven
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jacopo Mondi @ 2019-07-23 16:57 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, David Airlie, Daniel Vetter,
	fabrizio.castro
  Cc: Jacopo Mondi, open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS, open list

The R-Car LVDS encoder units support dual-link operations by splitting
the pixel output between the primary encoder and the companion one.

In order for the primary encoder to succesfully control the companion's
operations this should not fail at probe time and register itself its
associated drm bridge so that the primary one can find it.

Currently the companion encoder fails at probe time, causing the
registration of the primary to fail preventing the whole DU unit to be
registered correctly.

Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
Reported-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

---
The "Fixes" tag refers to a patch currently part of the
renesas-drivers-2019-07-09-v5.2 branch of Geert's renesas-drivers tree.

 drivers/gpu/drm/rcar-du/rcar_lvds.c | 31 +++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index bada7ee98544..8b015ba95895 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -767,14 +767,29 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 	of_node_put(remote_input);
 	of_node_put(remote);

-	/*
-	 * On D3/E3 the LVDS encoder provides a clock to the DU, which can be
-	 * used for the DPAD output even when the LVDS output is not connected.
-	 * Don't fail probe in that case as the DU will need the bridge to
-	 * control the clock.
-	 */
-	if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
-		return ret == -ENODEV ? 0 : ret;
+	switch (ret) {
+	case -ENODEV:
+		/*
+		 * On D3/E3 the LVDS encoder provides a clock to the DU, which
+		 * can be used for the DPAD output even when the LVDS output is
+		 * not connected. Don't fail probe in that case as the DU will
+		 * need the bridge to control the clock.
+		 */
+		if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
+			ret = 0;
+		break;
+	case -ENXIO:
+		/*
+		 * When the LVDS output is used in dual link mode, the
+		 * companion encoder fails at
+		 * 'rcar_lvds_parse_dt_companion()'. Don't fail probe in
+		 * that case as the master encoder will need the companion's
+		 * bridge to control its operations.
+		 */
+		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
+			ret = 0;
+		break;
+	}

 	return ret;
 }
--
2.22.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm: rcar_lvds: Fix dual link mode operations
  2019-07-23 16:57 [PATCH] drm: rcar_lvds: Fix dual link mode operations Jacopo Mondi
@ 2019-07-24  7:28 ` Geert Uytterhoeven
  2019-07-24  8:37   ` Jacopo Mondi
  2019-07-24  8:10 ` Fabrizio Castro
  2019-08-01 15:11 ` Laurent Pinchart
  2 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2019-07-24  7:28 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Laurent Pinchart, Kieran Bingham, David Airlie, Daniel Vetter,
	Fabrizio Castro, open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS, open list

Hi Jacopo,

On Wed, Jul 24, 2019 at 3:38 AM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> The R-Car LVDS encoder units support dual-link operations by splitting
> the pixel output between the primary encoder and the companion one.
>
> In order for the primary encoder to succesfully control the companion's
> operations this should not fail at probe time and register itself its
> associated drm bridge so that the primary one can find it.
>
> Currently the companion encoder fails at probe time, causing the
> registration of the primary to fail preventing the whole DU unit to be
> registered correctly.
>
> Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
> Reported-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> ---
> The "Fixes" tag refers to a patch currently part of the
> renesas-drivers-2019-07-09-v5.2 branch of Geert's renesas-drivers tree.

The broken commit is also present in v5.3-rc1.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] drm: rcar_lvds: Fix dual link mode operations
  2019-07-23 16:57 [PATCH] drm: rcar_lvds: Fix dual link mode operations Jacopo Mondi
  2019-07-24  7:28 ` Geert Uytterhoeven
@ 2019-07-24  8:10 ` Fabrizio Castro
  2019-08-01 15:11 ` Laurent Pinchart
  2 siblings, 0 replies; 6+ messages in thread
From: Fabrizio Castro @ 2019-07-24  8:10 UTC (permalink / raw)
  To: Jacopo Mondi, Laurent Pinchart, Kieran Bingham, David Airlie,
	Daniel Vetter
  Cc: open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS, open list

Hi Jacopo,

Thank you for your patch!

> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Jacopo Mondi
> Sent: 23 July 2019 17:57
> Subject: [PATCH] drm: rcar_lvds: Fix dual link mode operations
> 
> The R-Car LVDS encoder units support dual-link operations by splitting
> the pixel output between the primary encoder and the companion one.
> 
> In order for the primary encoder to succesfully control the companion's
> operations this should not fail at probe time and register itself its
> associated drm bridge so that the primary one can find it.
> 
> Currently the companion encoder fails at probe time, causing the
> registration of the primary to fail preventing the whole DU unit to be
> registered correctly.
> 
> Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
> Reported-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Tested-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

> 
> ---
> The "Fixes" tag refers to a patch currently part of the
> renesas-drivers-2019-07-09-v5.2 branch of Geert's renesas-drivers tree.
> 
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 31 +++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index bada7ee98544..8b015ba95895 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -767,14 +767,29 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  	of_node_put(remote_input);
>  	of_node_put(remote);
> 
> -	/*
> -	 * On D3/E3 the LVDS encoder provides a clock to the DU, which can be
> -	 * used for the DPAD output even when the LVDS output is not connected.
> -	 * Don't fail probe in that case as the DU will need the bridge to
> -	 * control the clock.
> -	 */
> -	if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
> -		return ret == -ENODEV ? 0 : ret;
> +	switch (ret) {
> +	case -ENODEV:
> +		/*
> +		 * On D3/E3 the LVDS encoder provides a clock to the DU, which
> +		 * can be used for the DPAD output even when the LVDS output is
> +		 * not connected. Don't fail probe in that case as the DU will
> +		 * need the bridge to control the clock.
> +		 */
> +		if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
> +			ret = 0;
> +		break;
> +	case -ENXIO:
> +		/*
> +		 * When the LVDS output is used in dual link mode, the
> +		 * companion encoder fails at
> +		 * 'rcar_lvds_parse_dt_companion()'. Don't fail probe in
> +		 * that case as the master encoder will need the companion's
> +		 * bridge to control its operations.
> +		 */
> +		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> +			ret = 0;
> +		break;
> +	}
> 
>  	return ret;
>  }
> --
> 2.22.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm: rcar_lvds: Fix dual link mode operations
  2019-07-24  7:28 ` Geert Uytterhoeven
@ 2019-07-24  8:37   ` Jacopo Mondi
  0 siblings, 0 replies; 6+ messages in thread
From: Jacopo Mondi @ 2019-07-24  8:37 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Jacopo Mondi, Laurent Pinchart, Kieran Bingham, David Airlie,
	Daniel Vetter, Fabrizio Castro,
	open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS, open list

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

Hi Geert,

On Wed, Jul 24, 2019 at 09:28:58AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Wed, Jul 24, 2019 at 3:38 AM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > The R-Car LVDS encoder units support dual-link operations by splitting
> > the pixel output between the primary encoder and the companion one.
> >
> > In order for the primary encoder to succesfully control the companion's
> > operations this should not fail at probe time and register itself its
> > associated drm bridge so that the primary one can find it.
> >
> > Currently the companion encoder fails at probe time, causing the
> > registration of the primary to fail preventing the whole DU unit to be
> > registered correctly.
> >
> > Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
> > Reported-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > The "Fixes" tag refers to a patch currently part of the
> > renesas-drivers-2019-07-09-v5.2 branch of Geert's renesas-drivers tree.
>
> The broken commit is also present in v5.3-rc1.

You're right... This is then entitled to be collected for rc2 then.

Thanks
   j
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm: rcar_lvds: Fix dual link mode operations
  2019-07-23 16:57 [PATCH] drm: rcar_lvds: Fix dual link mode operations Jacopo Mondi
  2019-07-24  7:28 ` Geert Uytterhoeven
  2019-07-24  8:10 ` Fabrizio Castro
@ 2019-08-01 15:11 ` Laurent Pinchart
  2019-08-05  7:51   ` Jacopo Mondi
  2 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2019-08-01 15:11 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Kieran Bingham, David Airlie, Daniel Vetter, fabrizio.castro,
	open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS, open list

Hello Jacopo,

Thank you for the patch.

On Tue, Jul 23, 2019 at 06:57:00PM +0200, Jacopo Mondi wrote:
> The R-Car LVDS encoder units support dual-link operations by splitting
> the pixel output between the primary encoder and the companion one.

s/the companion one/its companion/

> 
> In order for the primary encoder to succesfully control the companion's
> operations this should not fail at probe time and register itself its
> associated drm bridge so that the primary one can find it.

This is hard to parse.

> Currently the companion encoder fails at probe time, causing the
> registration of the primary to fail preventing the whole DU unit to be
> registered correctly.
> 
> Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
> Reported-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> ---
> The "Fixes" tag refers to a patch currently part of the
> renesas-drivers-2019-07-09-v5.2 branch of Geert's renesas-drivers tree.
> 
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 31 +++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index bada7ee98544..8b015ba95895 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -767,14 +767,29 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  	of_node_put(remote_input);
>  	of_node_put(remote);
> 
> -	/*
> -	 * On D3/E3 the LVDS encoder provides a clock to the DU, which can be
> -	 * used for the DPAD output even when the LVDS output is not connected.
> -	 * Don't fail probe in that case as the DU will need the bridge to
> -	 * control the clock.
> -	 */
> -	if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
> -		return ret == -ENODEV ? 0 : ret;
> +	switch (ret) {
> +	case -ENODEV:
> +		/*
> +		 * On D3/E3 the LVDS encoder provides a clock to the DU, which
> +		 * can be used for the DPAD output even when the LVDS output is
> +		 * not connected. Don't fail probe in that case as the DU will
> +		 * need the bridge to control the clock.
> +		 */
> +		if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
> +			ret = 0;
> +		break;
> +	case -ENXIO:
> +		/*
> +		 * When the LVDS output is used in dual link mode, the
> +		 * companion encoder fails at
> +		 * 'rcar_lvds_parse_dt_companion()'. Don't fail probe in
> +		 * that case as the master encoder will need the companion's
> +		 * bridge to control its operations.
> +		 */
> +		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> +			ret = 0;

As -ENXIO can only be returned by rcar_lvds_parse_dt_companion(), and
rcar_lvds_parse_dt_companion() is only called when the
RCAR_LVDS_QUIRK_DUAL_LINK flag is set, this essentially means that you
always zero the error returned from rcar_lvds_parse_dt_companion(). This
is both complicated and too drastic as the second -ENXIO error case
shouldn't be ignored. It would be better and simpler to return 0 from
rcar_lvds_parse_dt_companion() when the renesas,companion property can't
be found.

> +		break;
> +	}
> 
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm: rcar_lvds: Fix dual link mode operations
  2019-08-01 15:11 ` Laurent Pinchart
@ 2019-08-05  7:51   ` Jacopo Mondi
  0 siblings, 0 replies; 6+ messages in thread
From: Jacopo Mondi @ 2019-08-05  7:51 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, Kieran Bingham, David Airlie, Daniel Vetter,
	fabrizio.castro, open list:DRM DRIVERS FOR RENESAS,
	open list:DRM DRIVERS FOR RENESAS, open list

[-- Attachment #1: Type: text/plain, Size: 3971 bytes --]

Hi Laurent,

On Thu, Aug 01, 2019 at 06:11:29PM +0300, Laurent Pinchart wrote:
> Hello Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jul 23, 2019 at 06:57:00PM +0200, Jacopo Mondi wrote:
> > The R-Car LVDS encoder units support dual-link operations by splitting
> > the pixel output between the primary encoder and the companion one.
>
> s/the companion one/its companion/
>
> >
> > In order for the primary encoder to succesfully control the companion's
> > operations this should not fail at probe time and register itself its
> > associated drm bridge so that the primary one can find it.
>
> This is hard to parse.
>

Re-reading the whole commit message, I would actually drop it
completely, it's enough what we have here below.

> > Currently the companion encoder fails at probe time, causing the
> > registration of the primary to fail preventing the whole DU unit to be
> > registered correctly.
> >
> > Fixes: fa440d870358 ("drm: rcar-du: lvds: Add support for dual-link mode")
> > Reported-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > The "Fixes" tag refers to a patch currently part of the
> > renesas-drivers-2019-07-09-v5.2 branch of Geert's renesas-drivers tree.
> >
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 31 +++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index bada7ee98544..8b015ba95895 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -767,14 +767,29 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  	of_node_put(remote_input);
> >  	of_node_put(remote);
> >
> > -	/*
> > -	 * On D3/E3 the LVDS encoder provides a clock to the DU, which can be
> > -	 * used for the DPAD output even when the LVDS output is not connected.
> > -	 * Don't fail probe in that case as the DU will need the bridge to
> > -	 * control the clock.
> > -	 */
> > -	if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
> > -		return ret == -ENODEV ? 0 : ret;
> > +	switch (ret) {
> > +	case -ENODEV:
> > +		/*
> > +		 * On D3/E3 the LVDS encoder provides a clock to the DU, which
> > +		 * can be used for the DPAD output even when the LVDS output is
> > +		 * not connected. Don't fail probe in that case as the DU will
> > +		 * need the bridge to control the clock.
> > +		 */
> > +		if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
> > +			ret = 0;
> > +		break;
> > +	case -ENXIO:
> > +		/*
> > +		 * When the LVDS output is used in dual link mode, the
> > +		 * companion encoder fails at
> > +		 * 'rcar_lvds_parse_dt_companion()'. Don't fail probe in
> > +		 * that case as the master encoder will need the companion's
> > +		 * bridge to control its operations.
> > +		 */
> > +		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > +			ret = 0;
>
> As -ENXIO can only be returned by rcar_lvds_parse_dt_companion(), and
> rcar_lvds_parse_dt_companion() is only called when the
> RCAR_LVDS_QUIRK_DUAL_LINK flag is set, this essentially means that you
> always zero the error returned from rcar_lvds_parse_dt_companion(). This

Not totally correct, as rcar_lvds_parse_dt_companion() might also
return EPROBE_DEFER, but...

> is both complicated and too drastic as the second -ENXIO error case
> shouldn't be ignored. It would be better and simpler to return 0 from
> rcar_lvds_parse_dt_companion() when the renesas,companion property can't
> be found.

I agree, returning 0 when when the property is not specified is enough
and much simpler. I got dragged away by the idea of centralizing error
handling at the end of the function, but it's ugly and also wrongly
zeroes the second -ENXIO error returned by the parse_companion
function.

I'll change to what you suggested!
Thanks
  j

>
> > +		break;
> > +	}
> >
> >  	return ret;
> >  }
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-08-05  7:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-23 16:57 [PATCH] drm: rcar_lvds: Fix dual link mode operations Jacopo Mondi
2019-07-24  7:28 ` Geert Uytterhoeven
2019-07-24  8:37   ` Jacopo Mondi
2019-07-24  8:10 ` Fabrizio Castro
2019-08-01 15:11 ` Laurent Pinchart
2019-08-05  7:51   ` Jacopo Mondi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).