diff mbox

[v3,06/13] drm: bridge: Add LVDS encoder driver

Message ID 1480410283-28698-7-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Laurent Pinchart Nov. 29, 2016, 9:04 a.m. UTC
The LVDS encoder driver is a DRM bridge driver that supports the
parallel to LVDS encoders that don't require any configuration. The
driver thus doesn't interact with the device, but creates an LVDS
connector for the panel and exposes its size and timing based on
information retrieved from DT.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/Kconfig        |   8 ++
 drivers/gpu/drm/bridge/Makefile       |   1 +
 drivers/gpu/drm/bridge/lvds-encoder.c | 217 ++++++++++++++++++++++++++++++++++
 3 files changed, 226 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/lvds-encoder.c

Comments

Daniel Vetter Nov. 29, 2016, 9:54 a.m. UTC | #1
On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote:
> The LVDS encoder driver is a DRM bridge driver that supports the
> parallel to LVDS encoders that don't require any configuration. The
> driver thus doesn't interact with the device, but creates an LVDS
> connector for the panel and exposes its size and timing based on
> information retrieved from DT.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Since it's 100% dummy, why put LVDS into the name? This little thing here
could be our generic "wrap drm_panel and attach it to a chain" helper. So
what about calling this _The_ drm_panel_bridge, and also linking it into
docs to feature it a bit more prominently.

I came up with this because I spotted some refactoring belows for building
this helper, until I realized that this driver _is_ the helper I think we
want ;-) Only thing missing is an exported function to instantiate a
bridge with just a drm_panel as the parameter. And putting it into the
drm_kms_helper.ko module.

> +static enum drm_connector_status
> +lvds_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	return connector_status_connected;
> +}

We have piles of this exact dummy callback all over, maybe make it the
default and rip them all out?
-Daniel
Laurent Pinchart Nov. 29, 2016, 8:57 p.m. UTC | #2
Hi Daniel,

On Tuesday 29 Nov 2016 10:54:09 Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote:
> > The LVDS encoder driver is a DRM bridge driver that supports the
> > parallel to LVDS encoders that don't require any configuration. The
> > driver thus doesn't interact with the device, but creates an LVDS
> > connector for the panel and exposes its size and timing based on
> > information retrieved from DT.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Since it's 100% dummy, why put LVDS into the name? This little thing here
> could be our generic "wrap drm_panel and attach it to a chain" helper. So
> what about calling this _The_ drm_panel_bridge, and also linking it into
> docs to feature it a bit more prominently.

I'm not opposed to that, except that this driver should not be considered as 
just a helper to link a panel. It should only be used to support a real 
hardware bridge that requires no control.

> I came up with this because I spotted some refactoring belows for building
> this helper, until I realized that this driver _is_ the helper I think we
> want ;-) Only thing missing is an exported function to instantiate a
> bridge with just a drm_panel as the parameter. And putting it into the
> drm_kms_helper.ko module.

What would that be used for ? The bridge should be instantiated by this bridge 
driver, bound to a bridge device instantiated from DT (or the platform 
firmware of your choice).

> > +static enum drm_connector_status
> > +lvds_connector_detect(struct drm_connector *connector, bool force)
> > +{
> > +	return connector_status_connected;
> > +}
> 
> We have piles of this exact dummy callback all over, maybe make it the
> default and rip them all out?

Done, "[PATCH] drm: Make the connector .detect() callback optional".
Laurent Pinchart Jan. 4, 2017, 1:33 a.m. UTC | #3
Hi Daniel,

On Tuesday 29 Nov 2016 22:57:07 Laurent Pinchart wrote:
> On Tuesday 29 Nov 2016 10:54:09 Daniel Vetter wrote:
> > On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote:
> >> The LVDS encoder driver is a DRM bridge driver that supports the
> >> parallel to LVDS encoders that don't require any configuration. The
> >> driver thus doesn't interact with the device, but creates an LVDS
> >> connector for the panel and exposes its size and timing based on
> >> information retrieved from DT.
> >> 
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > Since it's 100% dummy, why put LVDS into the name? This little thing here
> > could be our generic "wrap drm_panel and attach it to a chain" helper. So
> > what about calling this _The_ drm_panel_bridge, and also linking it into
> > docs to feature it a bit more prominently.
> 
> I'm not opposed to that, except that this driver should not be considered as
> just a helper to link a panel. It should only be used to support a real
> hardware bridge that requires no control.
> 
> > I came up with this because I spotted some refactoring belows for building
> > this helper, until I realized that this driver _is_ the helper I think we
> > want ;-) Only thing missing is an exported function to instantiate a
> > bridge with just a drm_panel as the parameter. And putting it into the
> > drm_kms_helper.ko module.
> 
> What would that be used for ? The bridge should be instantiated by this
> bridge driver, bound to a bridge device instantiated from DT (or the
> platform firmware of your choice).

Ping ?

> >> +static enum drm_connector_status
> >> +lvds_connector_detect(struct drm_connector *connector, bool force)
> >> +{
> >> +	return connector_status_connected;
> >> +}
> > 
> > We have piles of this exact dummy callback all over, maybe make it the
> > default and rip them all out?
> 
> Done, "[PATCH] drm: Make the connector .detect() callback optional".
Daniel Vetter Jan. 4, 2017, 8:18 a.m. UTC | #4
On Tue, Nov 29, 2016 at 10:57:07PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Tuesday 29 Nov 2016 10:54:09 Daniel Vetter wrote:
> > On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote:
> > > The LVDS encoder driver is a DRM bridge driver that supports the
> > > parallel to LVDS encoders that don't require any configuration. The
> > > driver thus doesn't interact with the device, but creates an LVDS
> > > connector for the panel and exposes its size and timing based on
> > > information retrieved from DT.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > Since it's 100% dummy, why put LVDS into the name? This little thing here
> > could be our generic "wrap drm_panel and attach it to a chain" helper. So
> > what about calling this _The_ drm_panel_bridge, and also linking it into
> > docs to feature it a bit more prominently.
> 
> I'm not opposed to that, except that this driver should not be considered as 
> just a helper to link a panel. It should only be used to support a real 
> hardware bridge that requires no control.
> 
> > I came up with this because I spotted some refactoring belows for building
> > this helper, until I realized that this driver _is_ the helper I think we
> > want ;-) Only thing missing is an exported function to instantiate a
> > bridge with just a drm_panel as the parameter. And putting it into the
> > drm_kms_helper.ko module.
> 
> What would that be used for ? The bridge should be instantiated by this bridge 
> driver, bound to a bridge device instantiated from DT (or the platform 
> firmware of your choice).

Atm every driver using drm_panel needs a bit of glue to bind it into it's
display chain. With this code, and bridge chaining, you'd get that for
free, and I think that would be rather useful. And from a software point
of view I'd say if it quacks like a bridge, and walks like a bridge, it
probably _is_ a bridge. Even if no one calls that, or if physical the only
thing on the board at that spot are a bunch of dumb wires.
-Daniel
Laurent Pinchart Jan. 4, 2017, 1:08 p.m. UTC | #5
Hi Daniel,

On Wednesday 04 Jan 2017 09:18:18 Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 10:57:07PM +0200, Laurent Pinchart wrote:
> > On Tuesday 29 Nov 2016 10:54:09 Daniel Vetter wrote:
> >> On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote:
> >>> The LVDS encoder driver is a DRM bridge driver that supports the
> >>> parallel to LVDS encoders that don't require any configuration. The
> >>> driver thus doesn't interact with the device, but creates an LVDS
> >>> connector for the panel and exposes its size and timing based on
> >>> information retrieved from DT.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >> 
> >> Since it's 100% dummy, why put LVDS into the name? This little thing
> >> here could be our generic "wrap drm_panel and attach it to a chain"
> >> helper. So what about calling this _The_ drm_panel_bridge, and also
> >> linking it into docs to feature it a bit more prominently.
> > 
> > I'm not opposed to that, except that this driver should not be considered
> > as just a helper to link a panel. It should only be used to support a
> > real hardware bridge that requires no control.
> > 
> >> I came up with this because I spotted some refactoring belows for
> >> building this helper, until I realized that this driver _is_ the helper I
> >> think we want ;-) Only thing missing is an exported function to
> >> instantiate a bridge with just a drm_panel as the parameter. And putting
> >> it into the drm_kms_helper.ko module.
> > 
> > What would that be used for ? The bridge should be instantiated by this
> > bridge driver, bound to a bridge device instantiated from DT (or the
> > platform firmware of your choice).
> 
> Atm every driver using drm_panel needs a bit of glue to bind it into it's
> display chain. With this code, and bridge chaining, you'd get that for
> free, and I think that would be rather useful.

You would trade the bit of panel glue for a bit of bridge glue. Bridge 
chaining could perhaps help slightly at runtime there, but at init time the 
amount of glue would be similar.

I've noticed one piece of code that is LVDS-specific in this driver, and 
that's connector creation. The connector type is hardcoded to LVDS. To make 
the driver more generic, we would need a way to find the connector type at 
runtime. I'm wondering whether I should do this now, or keep the driver LVDS-
specific until someone comes up with a similar need. Without several potential 
users it's often hard to design a properly generic API.

> And from a software point of view I'd say if it quacks like a bridge, and
> walks like a bridge, it probably _is_ a bridge. Even if no one calls that,
> or if physical the only thing on the board at that spot are a bunch of dumb
> wires.

I dream of moving all encoders to DRM bridge, and then unifying drm_bridge and 
drm_panel with a common set of operations that could be invoked on both 
objects. That way the core could chain bridges and panels quite easily. I plan 
to experiment with this when moving the omapdrm driver to DRM bridge and DRM 
panel (it currently includes a bunch of custom encoder and panel drivers).
Daniel Vetter Jan. 4, 2017, 1:51 p.m. UTC | #6
On Wed, Jan 4, 2017 at 2:08 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Daniel,
>
> On Wednesday 04 Jan 2017 09:18:18 Daniel Vetter wrote:
>> On Tue, Nov 29, 2016 at 10:57:07PM +0200, Laurent Pinchart wrote:
>> > On Tuesday 29 Nov 2016 10:54:09 Daniel Vetter wrote:
>> >> On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote:
>> >>> The LVDS encoder driver is a DRM bridge driver that supports the
>> >>> parallel to LVDS encoders that don't require any configuration. The
>> >>> driver thus doesn't interact with the device, but creates an LVDS
>> >>> connector for the panel and exposes its size and timing based on
>> >>> information retrieved from DT.
>> >>>
>> >>> Signed-off-by: Laurent Pinchart
>> >>> <laurent.pinchart+renesas@ideasonboard.com>
>> >>
>> >> Since it's 100% dummy, why put LVDS into the name? This little thing
>> >> here could be our generic "wrap drm_panel and attach it to a chain"
>> >> helper. So what about calling this _The_ drm_panel_bridge, and also
>> >> linking it into docs to feature it a bit more prominently.
>> >
>> > I'm not opposed to that, except that this driver should not be considered
>> > as just a helper to link a panel. It should only be used to support a
>> > real hardware bridge that requires no control.
>> >
>> >> I came up with this because I spotted some refactoring belows for
>> >> building this helper, until I realized that this driver _is_ the helper I
>> >> think we want ;-) Only thing missing is an exported function to
>> >> instantiate a bridge with just a drm_panel as the parameter. And putting
>> >> it into the drm_kms_helper.ko module.
>> >
>> > What would that be used for ? The bridge should be instantiated by this
>> > bridge driver, bound to a bridge device instantiated from DT (or the
>> > platform firmware of your choice).
>>
>> Atm every driver using drm_panel needs a bit of glue to bind it into it's
>> display chain. With this code, and bridge chaining, you'd get that for
>> free, and I think that would be rather useful.
>
> You would trade the bit of panel glue for a bit of bridge glue. Bridge
> chaining could perhaps help slightly at runtime there, but at init time the
> amount of glue would be similar.

Hm, something like drm_bridge_panel_bridge_init(dev, panel) should be
enough, or not? My idea is to use this for the case where the only
thing in dt is the panel, with no real bridge chip. And I think we
don't need anything beyond that one _init function, plus maybe some
additional paramaters ...

> I've noticed one piece of code that is LVDS-specific in this driver, and
> that's connector creation. The connector type is hardcoded to LVDS. To make
> the driver more generic, we would need a way to find the connector type at
> runtime. I'm wondering whether I should do this now, or keep the driver LVDS-
> specific until someone comes up with a similar need. Without several potential
> users it's often hard to design a properly generic API.

... like whether the panel is dsi or lvds or soemthing else. Or maybe
we should just use LVDS for everything, because it's a panel, and
userspace shouldn't really care beyond that ;-)

>> And from a software point of view I'd say if it quacks like a bridge, and
>> walks like a bridge, it probably _is_ a bridge. Even if no one calls that,
>> or if physical the only thing on the board at that spot are a bunch of dumb
>> wires.
>
> I dream of moving all encoders to DRM bridge, and then unifying drm_bridge and
> drm_panel with a common set of operations that could be invoked on both
> objects. That way the core could chain bridges and panels quite easily. I plan
> to experiment with this when moving the omapdrm driver to DRM bridge and DRM
> panel (it currently includes a bunch of custom encoder and panel drivers).

Agreed on that dream, auto-wrapping panels in dummy bridges would be a
first step towards that. The other one is making drm_encoders entirely
dummies, and I think we're 90% there already.

End result isn't quite as clean as a complete rewrite, but probably
good enough. And because uapi we can't get rid of drm_encoder anyway,
and keeping drm_panel as the internal thing embedded into a
drm_panel_bridge struct seems reasonable too. That way panel drivers
can cope with a slightly simpler interface than full bridges.
-Daniel
Laurent Pinchart Jan. 4, 2017, 2:33 p.m. UTC | #7
Hi Daniel,

On Wednesday 04 Jan 2017 14:51:48 Daniel Vetter wrote:
> On Wed, Jan 4, 2017 at 2:08 PM, Laurent Pinchart wrote:
> > On Wednesday 04 Jan 2017 09:18:18 Daniel Vetter wrote:
> >> On Tue, Nov 29, 2016 at 10:57:07PM +0200, Laurent Pinchart wrote:
> >>> On Tuesday 29 Nov 2016 10:54:09 Daniel Vetter wrote:
> >>>> On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote:
> >>>>> The LVDS encoder driver is a DRM bridge driver that supports the
> >>>>> parallel to LVDS encoders that don't require any configuration. The
> >>>>> driver thus doesn't interact with the device, but creates an LVDS
> >>>>> connector for the panel and exposes its size and timing based on
> >>>>> information retrieved from DT.
> >>>>> 
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>> 
> >>>> Since it's 100% dummy, why put LVDS into the name? This little thing
> >>>> here could be our generic "wrap drm_panel and attach it to a chain"
> >>>> helper. So what about calling this _The_ drm_panel_bridge, and also
> >>>> linking it into docs to feature it a bit more prominently.
> >>> 
> >>> I'm not opposed to that, except that this driver should not be
> >>> considered as just a helper to link a panel. It should only be used to
> >>> support a real hardware bridge that requires no control.
> >>> 
> >>>> I came up with this because I spotted some refactoring belows for
> >>>> building this helper, until I realized that this driver _is_ the
> >>>> helper I think we want ;-) Only thing missing is an exported function
> >>>> to instantiate a bridge with just a drm_panel as the parameter. And
> >>>> putting it into the drm_kms_helper.ko module.
> >>> 
> >>> What would that be used for ? The bridge should be instantiated by this
> >>> bridge driver, bound to a bridge device instantiated from DT (or the
> >>> platform firmware of your choice).
> >> 
> >> Atm every driver using drm_panel needs a bit of glue to bind it into it's
> >> display chain. With this code, and bridge chaining, you'd get that for
> >> free, and I think that would be rather useful.
> > 
> > You would trade the bit of panel glue for a bit of bridge glue. Bridge
> > chaining could perhaps help slightly at runtime there, but at init time
> > the
> > amount of glue would be similar.
> 
> Hm, something like drm_bridge_panel_bridge_init(dev, panel) should be
> enough, or not? My idea is to use this for the case where the only
> thing in dt is the panel, with no real bridge chip. And I think we
> don't need anything beyond that one _init function, plus maybe some
> additional paramaters ...

There should be no bridge then. If you want the DRM core to manage panels 
automatically, then we should create specific helpers for that, not abuse the 
bridge infrastructure. Bridges should be instantiated from a hardware device 
and bound to drivers as usual.

> > I've noticed one piece of code that is LVDS-specific in this driver, and
> > that's connector creation. The connector type is hardcoded to LVDS. To
> > make the driver more generic, we would need a way to find the connector
> > type at runtime. I'm wondering whether I should do this now, or keep the
> > driver LVDS- specific until someone comes up with a similar need. Without
> > several potential users it's often hard to design a properly generic API.
> 
> ... like whether the panel is dsi or lvds or soemthing else. Or maybe
> we should just use LVDS for everything, because it's a panel, and
> userspace shouldn't really care beyond that ;-)

Feel free to propose a panel connector type :-)

> >> And from a software point of view I'd say if it quacks like a bridge, and
> >> walks like a bridge, it probably _is_ a bridge. Even if no one calls
> >> that, or if physical the only thing on the board at that spot are a bunch
> >> of dumb wires.
> > 
> > I dream of moving all encoders to DRM bridge, and then unifying drm_bridge
> > and drm_panel with a common set of operations that could be invoked on
> > both objects. That way the core could chain bridges and panels quite
> > easily. I plan to experiment with this when moving the omapdrm driver to
> > DRM bridge and DRM panel (it currently includes a bunch of custom encoder
> > and panel drivers).
>
> Agreed on that dream, auto-wrapping panels in dummy bridges would be a
> first step towards that.

But I think that's a step in the wrong direction. Ideally I'd like the 
encoders/bridges not to have to care about connectors and panels. There are 
multiple options, but a dummy bridge isn't a good idea in my opinion.

> The other one is making drm_encoders entirely dummies, and I think we're 90%
> there already.

We need to convert everything to drm_bridge first, we're not quite there yet 
:-)

> End result isn't quite as clean as a complete rewrite, but probably
> good enough. And because uapi we can't get rid of drm_encoder anyway,
> and keeping drm_panel as the internal thing embedded into a
> drm_panel_bridge struct seems reasonable too. That way panel drivers
> can cope with a slightly simpler interface than full bridges.
Daniel Vetter Jan. 4, 2017, 2:58 p.m. UTC | #8
On Wed, Jan 04, 2017 at 04:33:57PM +0200, Laurent Pinchart wrote:
> On Wednesday 04 Jan 2017 14:51:48 Daniel Vetter wrote:
> > Hm, something like drm_bridge_panel_bridge_init(dev, panel) should be
> > enough, or not? My idea is to use this for the case where the only
> > thing in dt is the panel, with no real bridge chip. And I think we
> > don't need anything beyond that one _init function, plus maybe some
> > additional paramaters ...
> 
> There should be no bridge then. If you want the DRM core to manage panels 
> automatically, then we should create specific helpers for that, not abuse the 
> bridge infrastructure. Bridges should be instantiated from a hardware device 
> and bound to drivers as usual.

I guess that's the part where I disagree: Just because there's physically
no bridge doesn't mean we shouldn't just treat it as one in the software
abstraction. If it looks and acts like a bridge (even an empty one), then
imo it can be a bridge.

If you insist on panels being panels, then I guess we need some other kind
of glue to bind them into arbitrary bridge chains. But given that the
callbacks match very closely, I don't see the point.

In an idea world a panel would probably derive from a drm_bridge, but
we're not in that universe unfortunately ;-)
-Daniel
Laurent Pinchart Jan. 4, 2017, 3:13 p.m. UTC | #9
Hi Daniel,

On Wednesday 04 Jan 2017 15:58:25 Daniel Vetter wrote:
> On Wed, Jan 04, 2017 at 04:33:57PM +0200, Laurent Pinchart wrote:
> > On Wednesday 04 Jan 2017 14:51:48 Daniel Vetter wrote:
> >> Hm, something like drm_bridge_panel_bridge_init(dev, panel) should be
> >> enough, or not? My idea is to use this for the case where the only
> >> thing in dt is the panel, with no real bridge chip. And I think we
> >> don't need anything beyond that one _init function, plus maybe some
> >> additional paramaters ...
> > 
> > There should be no bridge then. If you want the DRM core to manage panels
> > automatically, then we should create specific helpers for that, not abuse
> > the bridge infrastructure. Bridges should be instantiated from a hardware
> > device and bound to drivers as usual.
> 
> I guess that's the part where I disagree: Just because there's physically
> no bridge doesn't mean we shouldn't just treat it as one in the software
> abstraction. If it looks and acts like a bridge (even an empty one), then
> imo it can be a bridge.
> 
> If you insist on panels being panels, then I guess we need some other kind
> of glue to bind them into arbitrary bridge chains. But given that the
> callbacks match very closely, I don't see the point.
> 
> In an idea world a panel would probably derive from a drm_bridge, but
> we're not in that universe unfortunately ;-)

Or both would derive from another object, but I agree that's how it should 
work. That's what I want to achieve, one step at a time. Creating dummy 
bridges isn't a step in that direction in my opinion, so I'd rather not do 
that, but work towards the right abstraction.
Laurent Pinchart March 2, 2017, 12:30 a.m. UTC | #10
Hi Daniel,

On Wednesday 04 Jan 2017 17:13:23 Laurent Pinchart wrote:
> On Wednesday 04 Jan 2017 15:58:25 Daniel Vetter wrote:
> > On Wed, Jan 04, 2017 at 04:33:57PM +0200, Laurent Pinchart wrote:
> >> On Wednesday 04 Jan 2017 14:51:48 Daniel Vetter wrote:
> >>> Hm, something like drm_bridge_panel_bridge_init(dev, panel) should be
> >>> enough, or not? My idea is to use this for the case where the only
> >>> thing in dt is the panel, with no real bridge chip. And I think we
> >>> don't need anything beyond that one _init function, plus maybe some
> >>> additional paramaters ...
> >> 
> >> There should be no bridge then. If you want the DRM core to manage
> >> panels automatically, then we should create specific helpers for that,
> >> not abuse the bridge infrastructure. Bridges should be instantiated from
> >> a hardware device and bound to drivers as usual.
> > 
> > I guess that's the part where I disagree: Just because there's physically
> > no bridge doesn't mean we shouldn't just treat it as one in the software
> > abstraction. If it looks and acts like a bridge (even an empty one), then
> > imo it can be a bridge.
> > 
> > If you insist on panels being panels, then I guess we need some other kind
> > of glue to bind them into arbitrary bridge chains. But given that the
> > callbacks match very closely, I don't see the point.
> > 
> > In an idea world a panel would probably derive from a drm_bridge, but
> > we're not in that universe unfortunately ;-)
> 
> Or both would derive from another object, but I agree that's how it should
> work. That's what I want to achieve, one step at a time. Creating dummy
> bridges isn't a step in that direction in my opinion, so I'd rather not do
> that, but work towards the right abstraction.

Do you object getting this patch merged as-is as a first step in the right 
direction ? :-)
Daniel Vetter March 2, 2017, 7:05 a.m. UTC | #11
On Thu, Mar 02, 2017 at 02:30:09AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wednesday 04 Jan 2017 17:13:23 Laurent Pinchart wrote:
> > On Wednesday 04 Jan 2017 15:58:25 Daniel Vetter wrote:
> > > On Wed, Jan 04, 2017 at 04:33:57PM +0200, Laurent Pinchart wrote:
> > >> On Wednesday 04 Jan 2017 14:51:48 Daniel Vetter wrote:
> > >>> Hm, something like drm_bridge_panel_bridge_init(dev, panel) should be
> > >>> enough, or not? My idea is to use this for the case where the only
> > >>> thing in dt is the panel, with no real bridge chip. And I think we
> > >>> don't need anything beyond that one _init function, plus maybe some
> > >>> additional paramaters ...
> > >> 
> > >> There should be no bridge then. If you want the DRM core to manage
> > >> panels automatically, then we should create specific helpers for that,
> > >> not abuse the bridge infrastructure. Bridges should be instantiated from
> > >> a hardware device and bound to drivers as usual.
> > > 
> > > I guess that's the part where I disagree: Just because there's physically
> > > no bridge doesn't mean we shouldn't just treat it as one in the software
> > > abstraction. If it looks and acts like a bridge (even an empty one), then
> > > imo it can be a bridge.
> > > 
> > > If you insist on panels being panels, then I guess we need some other kind
> > > of glue to bind them into arbitrary bridge chains. But given that the
> > > callbacks match very closely, I don't see the point.
> > > 
> > > In an idea world a panel would probably derive from a drm_bridge, but
> > > we're not in that universe unfortunately ;-)
> > 
> > Or both would derive from another object, but I agree that's how it should
> > work. That's what I want to achieve, one step at a time. Creating dummy
> > bridges isn't a step in that direction in my opinion, so I'd rather not do
> > that, but work towards the right abstraction.
> 
> Do you object getting this patch merged as-is as a first step in the right 
> direction ? :-)

Well, I'm definitely no fan at all of typing code that's only there to
satisfy some fairly arbitrary normative choices we attach to the words we
pick for our abstraction.

But I'm also not going to stop you, just don't complain if I ask the next
dummy panel to reuse your LVDS bridge :-)
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index bd6acc829f97..a4d9657c9477 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -24,6 +24,14 @@  config DRM_DUMB_VGA_DAC
 	help
 	  Support for RGB to VGA DAC based bridges
 
+config DRM_LVDS_ENCODER
+	tristate "Transparent parallel to LVDS encoder support"
+	depends on OF
+	select DRM_KMS_HELPER
+	help
+	  Support for transparent parallel to LVDS encoders that don't require
+	  any configuration.
+
 config DRM_DW_HDMI
 	tristate
 	select DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 97ed1a5fea9a..7e0bf849c30d 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -2,6 +2,7 @@  ccflags-y := -Iinclude/drm
 
 obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
+obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
 obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
 obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
new file mode 100644
index 000000000000..576770ea8d7e
--- /dev/null
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -0,0 +1,217 @@ 
+/*
+ * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_panel.h>
+
+#include <linux/of_graph.h>
+
+struct lvds_encoder {
+	struct device *dev;
+
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct drm_panel *panel;
+};
+
+static inline struct lvds_encoder *
+drm_bridge_to_lvds_encoder(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct lvds_encoder, bridge);
+}
+
+static inline struct lvds_encoder *
+drm_connector_to_lvds_encoder(struct drm_connector *connector)
+{
+	return container_of(connector, struct lvds_encoder, connector);
+}
+
+static int lvds_connector_get_modes(struct drm_connector *connector)
+{
+	struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector);
+
+	return drm_panel_get_modes(lvds->panel);
+}
+
+static const struct drm_connector_helper_funcs lvds_connector_helper_funcs = {
+	.get_modes = lvds_connector_get_modes,
+};
+
+static enum drm_connector_status
+lvds_connector_detect(struct drm_connector *connector, bool force)
+{
+	return connector_status_connected;
+}
+
+static const struct drm_connector_funcs lvds_connector_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
+	.detect = lvds_connector_detect,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int lvds_encoder_attach(struct drm_bridge *bridge)
+{
+	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
+	struct drm_connector *connector = &lvds->connector;
+	int ret;
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Missing encoder\n");
+		return -ENODEV;
+	}
+
+	drm_connector_helper_add(connector, &lvds_connector_helper_funcs);
+
+	ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs,
+				 DRM_MODE_CONNECTOR_LVDS);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector\n");
+		return ret;
+	}
+
+	drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder);
+
+	ret = drm_panel_attach(lvds->panel, &lvds->connector);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static void lvds_encoder_detach(struct drm_bridge *bridge)
+{
+	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
+
+	drm_panel_detach(lvds->panel);
+}
+
+static void lvds_encoder_pre_enable(struct drm_bridge *bridge)
+{
+	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
+
+	drm_panel_prepare(lvds->panel);
+}
+
+static void lvds_encoder_enable(struct drm_bridge *bridge)
+{
+	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
+
+	drm_panel_enable(lvds->panel);
+}
+
+static void lvds_encoder_disable(struct drm_bridge *bridge)
+{
+	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
+
+	drm_panel_disable(lvds->panel);
+}
+
+static void lvds_encoder_post_disable(struct drm_bridge *bridge)
+{
+	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
+
+	drm_panel_unprepare(lvds->panel);
+}
+
+static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = {
+	.attach = lvds_encoder_attach,
+	.detach = lvds_encoder_detach,
+	.pre_enable = lvds_encoder_pre_enable,
+	.enable = lvds_encoder_enable,
+	.disable = lvds_encoder_disable,
+	.post_disable = lvds_encoder_post_disable,
+};
+
+static int lvds_encoder_probe(struct platform_device *pdev)
+{
+	struct lvds_encoder *lvds;
+	struct device_node *port;
+	struct device_node *endpoint;
+	struct device_node *panel;
+
+	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
+	if (!lvds)
+		return -ENOMEM;
+
+	lvds->dev = &pdev->dev;
+	platform_set_drvdata(pdev, lvds);
+
+	lvds->bridge.funcs = &lvds_encoder_bridge_funcs;
+	lvds->bridge.of_node = pdev->dev.of_node;
+	lvds->bridge.encoder_type = DRM_MODE_ENCODER_LVDS;
+
+	/* Locate the panel DT node. */
+	port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
+	if (!port) {
+		dev_dbg(&pdev->dev, "port 1 not found\n");
+		return -ENXIO;
+	}
+
+	endpoint = of_get_child_by_name(port, "endpoint");
+	of_node_put(port);
+	if (!endpoint) {
+		dev_dbg(&pdev->dev, "no endpoint for port 1\n");
+		return -ENXIO;
+	}
+
+	panel = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+	if (!panel) {
+		dev_dbg(&pdev->dev, "no remote endpoint for port 1\n");
+		return -ENXIO;
+	}
+
+	lvds->panel = of_drm_find_panel(panel);
+	of_node_put(panel);
+	if (!lvds->panel) {
+		dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
+		return -EPROBE_DEFER;
+	}
+
+	/* Register the bridge. */
+	return drm_bridge_add(&lvds->bridge);
+}
+
+static int lvds_encoder_remove(struct platform_device *pdev)
+{
+	struct lvds_encoder *encoder = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&encoder->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id lvds_encoder_match[] = {
+	{ .compatible = "lvds-encoder" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, lvds_encoder_match);
+
+struct platform_driver lvds_encoder_driver = {
+	.probe	= lvds_encoder_probe,
+	.remove	= lvds_encoder_remove,
+	.driver		= {
+		.name		= "lvds-encoder",
+		.of_match_table	= lvds_encoder_match,
+	},
+};
+module_platform_driver(lvds_encoder_driver);
+
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
+MODULE_DESCRIPTION("Transparent parallel to LVDS encoder");
+MODULE_LICENSE("GPL");