Re: [PATCH] drm: rcar_lvds: Fix dual link mode operations
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 > > Signed-off-by: Jacopo Mondi > > > > --- > > 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 signature.asc Description: PGP signature
Re: [PATCH] drm: rcar_lvds: Fix dual link mode operations
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 > Signed-off-by: Jacopo Mondi > > --- > 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
Re: [PATCH] drm: rcar_lvds: Fix dual link mode operations
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 > 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 > > Signed-off-by: Jacopo Mondi > > > > --- > > 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 -- > ge...@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 signature.asc Description: PGP signature
RE: [PATCH] drm: rcar_lvds: Fix dual link mode operations
Hi Jacopo, Thank you for your patch! > From: linux-renesas-soc-ow...@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 > Signed-off-by: Jacopo Mondi Reviewed-by: Fabrizio Castro Tested-by: Fabrizio Castro > > --- > 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
Re: [PATCH] drm: rcar_lvds: Fix dual link mode operations
Hi Jacopo, On Wed, Jul 24, 2019 at 3:38 AM 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. > > 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 > Signed-off-by: Jacopo Mondi > > --- > 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 -- ge...@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