Hi Fabrizio,
Thank you for the patch.
On Fri, Aug 02, 2019 at 08:34:04AM +0100, Fabrizio Castro wrote:
> If the display comes with two ports, assume it supports dual
> link.
>
> Signed-off-by: Fabrizio Castro <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 2d54ae5..97c51c2 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -751,6 +751,9 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> ret = -EPROBE_DEFER;
> goto done;
> }
> + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> + lvds->dual_link = of_graph_get_endpoint_count(remote)
> + == 2;
This is a bit of a hack, as I think the information should be queried
from the panel, like we do for bridges. I'd say we can live with this
for now, but as the data swap flag should come from the panel as well,
we will need infrastructure for that, and we can as well through the
dual link flag there at the same time.
I think we should use the drm_bridge_timings structure for this purpose,
as it would make life more difficult for users of drm_bridge and
drm_panel to have two different structures (especially when wrapping a
drm_panel with drm_panel_bridge_add()). The structure could be renamed
if desired.
> }
>
> if (lvds->dual_link) {
--
Regards,
Laurent Pinchart
Hi Laurent,
Thank you for your feedback!
> From: Laurent Pinchart <[email protected]>
> Sent: 02 August 2019 09:20
> Subject: Re: [PATCH/RFC 07/12] drm: rcar-du: lvds: Add support for dual link panels
>
> Hi Fabrizio,
>
> Thank you for the patch.
>
> On Fri, Aug 02, 2019 at 08:34:04AM +0100, Fabrizio Castro wrote:
> > If the display comes with two ports, assume it supports dual
> > link.
> >
> > Signed-off-by: Fabrizio Castro <[email protected]>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 2d54ae5..97c51c2 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -751,6 +751,9 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > ret = -EPROBE_DEFER;
> > goto done;
> > }
> > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > + lvds->dual_link = of_graph_get_endpoint_count(remote)
> > + == 2;
>
> This is a bit of a hack, as I think the information should be queried
> from the panel, like we do for bridges. I'd say we can live with this
> for now, but as the data swap flag should come from the panel as well,
> we will need infrastructure for that, and we can as well through the
> dual link flag there at the same time.
I totally agree, this is a nasty hack, my series is missing the infrastructure
for describing this information
>
> I think we should use the drm_bridge_timings structure for this purpose,
> as it would make life more difficult for users of drm_bridge and
> drm_panel to have two different structures (especially when wrapping a
> drm_panel with drm_panel_bridge_add()). The structure could be renamed
> if desired.
I am not too sure using drm_bridge_timings for panels would make everybody
happy? Is there any alternative? Perhaps this calls for a new struct we could
embed in both drm_bridge_timings and some drm_panel_<whatever> data
structure?
Thanks,
Fab
>
> > }
> >
> > if (lvds->dual_link) {
>
> --
> Regards,
>
> Laurent Pinchart
Hi Fabrizio,
On Mon, Aug 05, 2019 at 09:12:34AM +0000, Fabrizio Castro wrote:
> > From: Laurent Pinchart <[email protected]>
> > Sent: 02 August 2019 09:20
> > Subject: Re: [PATCH/RFC 07/12] drm: rcar-du: lvds: Add support for dual link panels
> >
> > Hi Fabrizio,
> >
> > Thank you for the patch.
> >
> > On Fri, Aug 02, 2019 at 08:34:04AM +0100, Fabrizio Castro wrote:
> > > If the display comes with two ports, assume it supports dual
> > > link.
> > >
> > > Signed-off-by: Fabrizio Castro <[email protected]>
> > > ---
> > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > index 2d54ae5..97c51c2 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > @@ -751,6 +751,9 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > > ret = -EPROBE_DEFER;
> > > goto done;
> > > }
> > > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > > + lvds->dual_link = of_graph_get_endpoint_count(remote)
> > > + == 2;
> >
> > This is a bit of a hack, as I think the information should be queried
> > from the panel, like we do for bridges. I'd say we can live with this
> > for now, but as the data swap flag should come from the panel as well,
> > we will need infrastructure for that, and we can as well through the
> > dual link flag there at the same time.
>
> I totally agree, this is a nasty hack, my series is missing the infrastructure
> for describing this information
>
> > I think we should use the drm_bridge_timings structure for this purpose,
> > as it would make life more difficult for users of drm_bridge and
> > drm_panel to have two different structures (especially when wrapping a
> > drm_panel with drm_panel_bridge_add()). The structure could be renamed
> > if desired.
>
> I am not too sure using drm_bridge_timings for panels would make everybody
> happy? Is there any alternative? Perhaps this calls for a new struct we could
> embed in both drm_bridge_timings and some drm_panel_<whatever> data
> structure?
I think we could simply rename the structure, all its fields apply to
panels too.
> > > }
> > >
> > > if (lvds->dual_link) {
--
Regards,
Laurent Pinchart
Hi Laurent,
> From: Laurent Pinchart <[email protected]>
> Sent: 05 August 2019 10:49
> Subject: Re: [PATCH/RFC 07/12] drm: rcar-du: lvds: Add support for dual link panels
>
> Hi Fabrizio,
>
> On Mon, Aug 05, 2019 at 09:12:34AM +0000, Fabrizio Castro wrote:
> > > From: Laurent Pinchart <[email protected]>
> > > Sent: 02 August 2019 09:20
> > > Subject: Re: [PATCH/RFC 07/12] drm: rcar-du: lvds: Add support for dual link panels
> > >
> > > Hi Fabrizio,
> > >
> > > Thank you for the patch.
> > >
> > > On Fri, Aug 02, 2019 at 08:34:04AM +0100, Fabrizio Castro wrote:
> > > > If the display comes with two ports, assume it supports dual
> > > > link.
> > > >
> > > > Signed-off-by: Fabrizio Castro <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > index 2d54ae5..97c51c2 100644
> > > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > @@ -751,6 +751,9 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > > > ret = -EPROBE_DEFER;
> > > > goto done;
> > > > }
> > > > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > > > + lvds->dual_link = of_graph_get_endpoint_count(remote)
> > > > + == 2;
> > >
> > > This is a bit of a hack, as I think the information should be queried
> > > from the panel, like we do for bridges. I'd say we can live with this
> > > for now, but as the data swap flag should come from the panel as well,
> > > we will need infrastructure for that, and we can as well through the
> > > dual link flag there at the same time.
> >
> > I totally agree, this is a nasty hack, my series is missing the infrastructure
> > for describing this information
> >
> > > I think we should use the drm_bridge_timings structure for this purpose,
> > > as it would make life more difficult for users of drm_bridge and
> > > drm_panel to have two different structures (especially when wrapping a
> > > drm_panel with drm_panel_bridge_add()). The structure could be renamed
> > > if desired.
> >
> > I am not too sure using drm_bridge_timings for panels would make everybody
> > happy? Is there any alternative? Perhaps this calls for a new struct we could
> > embed in both drm_bridge_timings and some drm_panel_<whatever> data
> > structure?
>
> I think we could simply rename the structure, all its fields apply to
> panels too.
Ok, will give this a try.
Thanks,
Fab
>
> > > > }
> > > >
> > > > if (lvds->dual_link) {
>
> --
> Regards,
>
> Laurent Pinchart