[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector
Hi Jean-Fran?ois, Sorry for the late reply. On Thursday 27 March 2014 12:34:49 Jean-Francois Moine wrote: > On Wed, 26 Mar 2014 18:33:09 +0100 Laurent Pinchart wrote: > > That could work in your case, but I don't really like that. > > > > We need to describe the hardware topology, that might be the only point we > > all agree on. There are various hardware topologies we need to support > > with different levels of complexity, and several ways to describe them, > > also with a wide range of complexities and features. > > > > The more complex the hardware topology, the more complex its description > > needs to be, and the more complex the code that will parse the > > description and handle the hardware will be. I don't think there's any > > doubt here. > > But also, the simpler is the topology and the simpler should be its > description. I wouldn't put it so strongly. I believe we can slightly relax our requirements regarding DT bindings complexity as long as drivers remain simple. There's of course no reason to use more complex bindings just for the sake of it. > In your approach, the connections between endpoints are described in the > components themselves, which makes hard for the DT maintainers to have a > global understanding of the video subsystem. > > Having the topology described in the master device is clearer, even if it is > complex. First of all, let's clarify what a "master device" is. In the "simple-video- card" example you've proposed the master device is a logical device (with a DT node that has no corresponding hardware device). The second approach I can think of is to make the IP core that implements the CRTC (which I usually call display controller) be the master device. Let's note that the second case makes both the link and "global description" DT binding styles possible. My concern with the "global description" bindings style is that the approach only applies to simple hardware and can't be generalized. Now, I'm not too concerned about using that approach for simple hardware and a link-based approach for more complex hardware. The "slave" components, however, need to use a single DT bindings style, regardless of the DT bindings used by the master device. That's why I believe we should try to standardize one DT bindings style for master devices, even if it results in a slightly more complex DT description than strictly needed in some cases. > > A given device can be integrated in a wide variety of hardware with > > different complexities. A driver can't thus always assume that the whole > > composite display device will match a given class of topologies. This is > > especially true for encoders and connectors, they can be hooked up > > directly at the output of a very simple display controller, or can be > > part of a very complex graph. Encoder and connector drivers can't assume > > much about how they will be integrated. I want to avoid a situation where > > using an HDMI encoder already supported in mainline with a different SoC > > will result in having to rewrite the HDMI encoder driver. > > The tda998x chips are simple enough for being used in many boards: one video > input and one serial digital output. I don't see in which circumstance the > driver should be rewritten. It shouldn't, that's the whole point ;-) I wasn't talking about the tda998x only, but about encoder drivers in general. I have a display controller in a Renesas SoC that has two LVDS encoders that output LVDS signals out of the SoC. One LVDS port is connected to an LVDS panel (a connector from a DRM point of view), the second one to an LVDS receiver that outputs parallel RGB data to an HDMI encoder. The LVDS encoder can't thus assume any particular downstream topology and its driver thus can't create DRM encoder and connector instances. The same could be true for an HDMI encoder in theory, although an HDMI encoder connected on the same board directly to an HDMI decoder that outputs RGB data to a panel is probably a case that will be rarely seen in practice. In the general case an encoder driver can't assume any specific upstream or downstream topology. As long as DRM can't expose the real hardware topology to userspace, someone needs to decide on how to map the hardware topology to the DRM topology exposed to userspace. Encoder drivers can't take that role and thus shouldn't create DRM encoder and connector instances themselves. > > The story is a bit different for display controllers. While the hardware > > topology outside (and sometimes inside as well) of the SoC can vary, a > > display controller often approximately implies a given level of > > complexity. A cheap SoC with a simple display controller will likely not > > be used to drive a 4k display requiring 4 encoders running in parallel > > for instance. While I also want to avoid having to make significant > > changes to a display controller driver when using the SoC on a different > > board, I believe the
[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector
Hi Laurent, On Wed, 26 Mar 2014 18:33:09 +0100 Laurent Pinchart wrote: > That could work in your case, but I don't really like that. > > We need to describe the hardware topology, that might be the only point we > all > agree on. There are various hardware topologies we need to support with > different levels of complexity, and several ways to describe them, also with > a > wide range of complexities and features. > > The more complex the hardware topology, the more complex its description > needs > to be, and the more complex the code that will parse the description and > handle the hardware will be. I don't think there's any doubt here. But also, the simpler is the topology and the simpler should be its description. In your approach, the connections between endpoints are described in the components themselves, which makes hard for the DT maintainers to have a global understanding of the video subsystem. Having the topology described in the master device is clearer, even if it is complex. > A given device can be integrated in a wide variety of hardware with different > complexities. A driver can't thus always assume that the whole composite > display device will match a given class of topologies. This is especially > true > for encoders and connectors, they can be hooked up directly at the output of > a > very simple display controller, or can be part of a very complex graph. > Encoder and connector drivers can't assume much about how they will be > integrated. I want to avoid a situation where using an HDMI encoder already > supported in mainline with a different SoC will result in having to rewrite > the HDMI encoder driver. The tda998x chips are simple enough for being used in many boards: one video input and one serial digital output. I don't see in which circumstance the driver should be rewritten. > The story is a bit different for display controllers. While the hardware > topology outside (and sometimes inside as well) of the SoC can vary, a > display > controller often approximately implies a given level of complexity. A cheap > SoC with a simple display controller will likely not be used to drive a 4k > display requiring 4 encoders running in parallel for instance. While I also > want to avoid having to make significant changes to a display controller > driver when using the SoC on a different board, I believe the requirement can > be slightly relaxed here, and the display controller driver(s) can assume > more > about the hardware topology than the encoder drivers. I don't think that the display controllers or the encoders have to know about the topology. They have well-knwon specific functions and the way they are interconnected should not impact these functions. If that would be the case, they should offer a particular configuration interface to the master driver. > I've asked myself whether we needed a single, one-size-fits-them-all DT > representation of the hardware topology. The view of the world from an > external encoder point of view must not depend on the SoC it is hooked up to > (this would prevent using a single encoder driver with multiple SoCs), which > calls for at least some form of standardization. The topology representation > on the display controller side may vary from display controller to display > controller, but I believe this would just result in code duplication and > having to solve the same problem in multiple drivers. For those reasons I > believe that the OF graph proposal to represent the display hardware topology > would be a good choice. The bindings are flexible enough to represent both > simple and complex hardware. Your OF graph proposal is rather complex, and it does not even indicate which way the data flows. > Now, I don't want to force all display device drivers to implement complex > code when they only need to support simple hardware and simple hardware > topologies. Not only would that be rightfully rejected, I would be among the > ones nack'ing that approach. My opinion is that this calls for the creation > of > helpers to handle common cases. Several (possibly many) display systems only > need (or want) to support linear pipelines at their output(s), made of a > single encoder and a single connector. There's no point in duplicating DT > parsing or encoder/connector instantiation code in several drivers in that > case where helpers could be reused. Several sets of helpers could support > different kinds of topologies, with the driver author selecting a set of > helpers depending on the expected hardware topology complexity. I don't like this 'helper' notion. See below. > We also need to decide on who (as in which driver) will be responsible for > binding all devices together. As DRM exposes a single device to userspace, > there needs to be a single driver that will perform front line handling of > the > userspace API and delegate calls to the other drivers involved. I believe it >
[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector
Hi Jean-Fran?ois,
On Tuesday 25 March 2014 16:55:48 Jean-Francois Moine wrote:
> On Mon, 24 Mar 2014 23:39:01 +0100 Laurent Pinchart wrote:
> > On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> > > The 'slave encoder' structure of the tda998x driver asks for glue
> > > between the DRM driver and the encoder/connector structures.
> > >
> > > This patch changes the driver to a normal DRM encoder/connector
> > > thanks to the infrastructure for componentised subsystems.
> >
> > I like the idea, but I'm not really happy with the implementation. Let me
> > try to explain why below.
> >
> > > Signed-off-by: Jean-Francois Moine
> > > ---
> > >
> > > drivers/gpu/drm/i2c/tda998x_drv.c | 323 ---
> > > 1 file changed, 188 insertions(+), 135 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> >
> > [snip]
> >
> > > @@ -44,10 +45,14 @@ struct tda998x_priv {
> > > wait_queue_head_t wq_edid;
> > > volatile int wq_edid_wait;
> > >
> > > - struct drm_encoder *encoder;
> > > + struct drm_encoder encoder;
> > > + struct drm_connector connector;
> > >
> > > };
> >
> > [snip]
> >
> > > -static int
> > > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id
> > > *id)
> > > +static int tda_bind(struct device *dev, struct device *master, void
> > > *data)
> > > {
> > > + struct drm_device *drm = data;
> >
> > This is the part that bothers me. You're making two assumptions here, that
> > the DRM driver will pass a struct drm_device pointer to the bind
> > operation, and that the I2C encoder driver can take control of DRM
> > encoder and connector creation. Although it could become problematic
> > later, the first assumption isn't too much of an issue for now. I'll thus
> > focus on the second one.
> >
> > The component framework isolate the encoder and DRM master drivers as far
> > as component creation and binding is concerned, but doesn't provide a way
> > for the two drivers to communicate together (nor should it). You're
> > solving this by passing a pointer to the DRM device to the encoder bind
> > operation, making the encoder driver create a DRM encoder and connector,
> > and relying on the DRM core to orchestrate CRTCs, encoders and
> > connectors. You thus assume that the encoder hardware should be
> > represented by a DRM encoder object, and that its output is connected to
> > a connector that should be represented by a DRM connector object. While
> > this can work in your use case, that won't always hold true. Hardware
> > encoders can be chained together, while DRM encoders can't. The DRM core
> > has recently received support for bridge objects to overcome that
> > limitation. Depending on the hardware topology, a given hardware encoder
> > should be modeled as a DRM encoder or as a DRM bridge. That decision
> > shouldn't be taken by the encoder driver but by the DRM master driver.
> > The I2C encoder driver thus shouldn't create the DRM encoder and DRM
> > connector itself.
> >
> > I believe the encoder/master communication problem should be solved
> > differently. Instead of passing a pointer to the DRM device to the encoder
> > driver and making the encoder driver control DRM encoder and connector
> > creation, the encoder driver should instead create an object not visible
> > to userspace that can be retrieved by the DRM master driver (possibly
> > through registration with the DRM core, or by going through drvdata in the
> > encoder's struct device). The DRM master could use that object to
> > communicate with the encoder, and would register the DRM encoder and DRM
> > connector itself based on hardware topology.
> >
> > > + struct i2c_client *i2c_client = to_i2c_client(dev);
> > > + struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> > > + struct drm_connector *connector = >connector;
> > > + struct drm_encoder *encoder = >encoder;
> > > + int ret;
> > > +
> > > + if (!try_module_get(THIS_MODULE)) {
> > > + dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = drm_connector_init(drm, connector,
> > > + _funcs,
> > > + DRM_MODE_CONNECTOR_HDMIA);
> >
> > This is one example of the shortcomings I've explained above. An encoder
> > driver can't always know what connector type it is connected to. If I'm
> > not mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It
> > should be up to the master driver to select the connector type based on
> > its overall view of the hardware, or even to a connector driver that would
> > be bound to a connector DT node (as proposed in
> > https://www.mail-archive.com/devicetree at vger.kernel.org/msg16585.html).
> [snip]
>
> The tda998x, as a HDMI transmitter, has to
[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector
On Mon, 24 Mar 2014 23:39:01 +0100
Laurent Pinchart wrote:
> Hi Jean-Fran?ois,
Hi Laurent,
> Thank you for the patch.
>
> On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> > The 'slave encoder' structure of the tda998x driver asks for glue
> > between the DRM driver and the encoder/connector structures.
> >
> > This patch changes the driver to a normal DRM encoder/connector
> > thanks to the infrastructure for componentised subsystems.
>
> I like the idea, but I'm not really happy with the implementation. Let me try
> to explain why below.
>
> > Signed-off-by: Jean-Francois Moine
> > ---
> > drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++
> > 1 file changed, 188 insertions(+), 135 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>
> [snip]
>
> > @@ -44,10 +45,14 @@ struct tda998x_priv {
> >
> > wait_queue_head_t wq_edid;
> > volatile int wq_edid_wait;
> > - struct drm_encoder *encoder;
> > + struct drm_encoder encoder;
> > + struct drm_connector connector;
> > };
>
> [snip]
>
> > -static int
> > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > +static int tda_bind(struct device *dev, struct device *master, void *data)
> > {
> > + struct drm_device *drm = data;
>
> This is the part that bothers me. You're making two assumptions here, that
> the
> DRM driver will pass a struct drm_device pointer to the bind operation, and
> that the I2C encoder driver can take control of DRM encoder and connector
> creation. Although it could become problematic later, the first assumption
> isn't too much of an issue for now. I'll thus focus on the second one.
>
> The component framework isolate the encoder and DRM master drivers as far as
> component creation and binding is concerned, but doesn't provide a way for
> the
> two drivers to communicate together (nor should it). You're solving this by
> passing a pointer to the DRM device to the encoder bind operation, making the
> encoder driver create a DRM encoder and connector, and relying on the DRM
> core
> to orchestrate CRTCs, encoders and connectors. You thus assume that the
> encoder hardware should be represented by a DRM encoder object, and that its
> output is connected to a connector that should be represented by a DRM
> connector object. While this can work in your use case, that won't always
> hold
> true. Hardware encoders can be chained together, while DRM encoders can't.
> The
> DRM core has recently received support for bridge objects to overcome that
> limitation. Depending on the hardware topology, a given hardware encoder
> should be modeled as a DRM encoder or as a DRM bridge. That decision
> shouldn't
> be taken by the encoder driver but by the DRM master driver. The I2C encoder
> driver thus shouldn't create the DRM encoder and DRM connector itself.
>
> I believe the encoder/master communication problem should be solved
> differently. Instead of passing a pointer to the DRM device to the encoder
> driver and making the encoder driver control DRM encoder and connector
> creation, the encoder driver should instead create an object not visible to
> userspace that can be retrieved by the DRM master driver (possibly through
> registration with the DRM core, or by going through drvdata in the encoder's
> struct device). The DRM master could use that object to communicate with the
> encoder, and would register the DRM encoder and DRM connector itself based on
> hardware topology.
>
> > + struct i2c_client *i2c_client = to_i2c_client(dev);
> > + struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> > + struct drm_connector *connector = >connector;
> > + struct drm_encoder *encoder = >encoder;
> > + int ret;
> > +
> > + if (!try_module_get(THIS_MODULE)) {
> > + dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> > + return -EINVAL;
> > + }
> > +
> > + ret = drm_connector_init(drm, connector,
> > + _funcs,
> > + DRM_MODE_CONNECTOR_HDMIA);
>
> This is one example of the shortcomings I've explained above. An encoder
> driver can't always know what connector type it is connected to. If I'm not
> mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be
> up to the master driver to select the connector type based on its overall
> view
> of the hardware, or even to a connector driver that would be bound to a
> connector DT node (as proposed in https://www.mail-archive.com/devicetree at
> vger.kernel.org/msg16585.html).
[snip]
The tda998x, as a HDMI transmitter, has to deal with both video and
audio.
Whereas the hardware connection schemes are the same in both worlds,
the way they are translated to computer objects
[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector
Hi Jean-Fran?ois,
Thank you for the patch.
On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> The 'slave encoder' structure of the tda998x driver asks for glue
> between the DRM driver and the encoder/connector structures.
>
> This patch changes the driver to a normal DRM encoder/connector
> thanks to the infrastructure for componentised subsystems.
I like the idea, but I'm not really happy with the implementation. Let me try
to explain why below.
> Signed-off-by: Jean-Francois Moine
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++
> 1 file changed, 188 insertions(+), 135 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
[snip]
> @@ -44,10 +45,14 @@ struct tda998x_priv {
>
> wait_queue_head_t wq_edid;
> volatile int wq_edid_wait;
> - struct drm_encoder *encoder;
> + struct drm_encoder encoder;
> + struct drm_connector connector;
> };
[snip]
> -static int
> -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +static int tda_bind(struct device *dev, struct device *master, void *data)
> {
> + struct drm_device *drm = data;
This is the part that bothers me. You're making two assumptions here, that the
DRM driver will pass a struct drm_device pointer to the bind operation, and
that the I2C encoder driver can take control of DRM encoder and connector
creation. Although it could become problematic later, the first assumption
isn't too much of an issue for now. I'll thus focus on the second one.
The component framework isolate the encoder and DRM master drivers as far as
component creation and binding is concerned, but doesn't provide a way for the
two drivers to communicate together (nor should it). You're solving this by
passing a pointer to the DRM device to the encoder bind operation, making the
encoder driver create a DRM encoder and connector, and relying on the DRM core
to orchestrate CRTCs, encoders and connectors. You thus assume that the
encoder hardware should be represented by a DRM encoder object, and that its
output is connected to a connector that should be represented by a DRM
connector object. While this can work in your use case, that won't always hold
true. Hardware encoders can be chained together, while DRM encoders can't. The
DRM core has recently received support for bridge objects to overcome that
limitation. Depending on the hardware topology, a given hardware encoder
should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't
be taken by the encoder driver but by the DRM master driver. The I2C encoder
driver thus shouldn't create the DRM encoder and DRM connector itself.
I believe the encoder/master communication problem should be solved
differently. Instead of passing a pointer to the DRM device to the encoder
driver and making the encoder driver control DRM encoder and connector
creation, the encoder driver should instead create an object not visible to
userspace that can be retrieved by the DRM master driver (possibly through
registration with the DRM core, or by going through drvdata in the encoder's
struct device). The DRM master could use that object to communicate with the
encoder, and would register the DRM encoder and DRM connector itself based on
hardware topology.
> + struct i2c_client *i2c_client = to_i2c_client(dev);
> + struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> + struct drm_connector *connector = >connector;
> + struct drm_encoder *encoder = >encoder;
> + int ret;
> +
> + if (!try_module_get(THIS_MODULE)) {
> + dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> + return -EINVAL;
> + }
> +
> + ret = drm_connector_init(drm, connector,
> + _funcs,
> + DRM_MODE_CONNECTOR_HDMIA);
This is one example of the shortcomings I've explained above. An encoder
driver can't always know what connector type it is connected to. If I'm not
mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be
up to the master driver to select the connector type based on its overall view
of the hardware, or even to a connector driver that would be bound to a
connector DT node (as proposed in https://www.mail-archive.com/devicetree at
vger.kernel.org/msg16585.html).
> + if (ret < 0)
> + return ret;
> + drm_connector_helper_add(connector, _helper_funcs);
> +
> + ret = drm_encoder_init(drm, encoder,
> + _funcs,
> + DRM_MODE_ENCODER_TMDS);
> +
> + encoder->possible_crtcs = 1;// 1 << lcd_num
> +
> + if (ret < 0)
> + goto err;
> + drm_encoder_helper_add(encoder, _helper_funcs);
> +
> + ret =
[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector
The 'slave encoder' structure of the tda998x driver asks for glue
between the DRM driver and the encoder/connector structures.
This patch changes the driver to a normal DRM encoder/connector
thanks to the infrastructure for componentised subsystems.
Signed-off-by: Jean-Francois Moine
---
drivers/gpu/drm/i2c/tda998x_drv.c | 323 ++
1 file changed, 188 insertions(+), 135 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index fd6751c..1c25e40 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,11 +20,12 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
-#include
#include
#include
@@ -44,10 +45,14 @@ struct tda998x_priv {
wait_queue_head_t wq_edid;
volatile int wq_edid_wait;
- struct drm_encoder *encoder;
+ struct drm_encoder encoder;
+ struct drm_connector connector;
};
-#define to_tda998x_priv(x) ((struct tda998x_priv
*)to_encoder_slave(x)->slave_priv)
+#define connector_priv(e) \
+ container_of(connector, struct tda998x_priv, connector)
+#define encoder_priv(e) \
+ container_of(encoder, struct tda998x_priv, encoder)
/* The TDA9988 series of devices use a paged register scheme.. to simplify
* things we encode the page # in upper bits of the register #. To read/
@@ -559,9 +564,8 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
if ((flag2 & INT_FLAGS_2_EDID_BLK_RD) && priv->wq_edid_wait) {
priv->wq_edid_wait = 0;
wake_up(>wq_edid);
- } else if (cec != 0) { /* HPD change */
- if (priv->encoder && priv->encoder->dev)
- drm_helper_hpd_irq_event(priv->encoder->dev);
+ } else if (cec != 0 && priv->encoder.dev) { /* HPD change */
+ drm_helper_hpd_irq_event(priv->encoder.dev);
}
return IRQ_HANDLED;
}
@@ -731,9 +735,8 @@ tda998x_configure_audio(struct tda998x_priv *priv,
/* DRM encoder functions */
static void
-tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
+tda998x_encoder_set_config(struct tda998x_priv *priv, void *params)
{
- struct tda998x_priv *priv = to_tda998x_priv(encoder);
struct tda998x_encoder_params *p = params;
priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
@@ -755,7 +758,7 @@ tda998x_encoder_set_config(struct drm_encoder *encoder,
void *params)
static void
tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
{
- struct tda998x_priv *priv = to_tda998x_priv(encoder);
+ struct tda998x_priv *priv = encoder_priv(encoder);
/* we only care about on or off: */
if (mode != DRM_MODE_DPMS_ON)
@@ -786,18 +789,6 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
priv->dpms = mode;
}
-static void
-tda998x_encoder_save(struct drm_encoder *encoder)
-{
- DBG("");
-}
-
-static void
-tda998x_encoder_restore(struct drm_encoder *encoder)
-{
- DBG("");
-}
-
static bool
tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
const struct drm_display_mode *mode,
@@ -806,11 +797,14 @@ tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
return true;
}
-static int
-tda998x_encoder_mode_valid(struct drm_encoder *encoder,
- struct drm_display_mode *mode)
+static void tda998x_encoder_prepare(struct drm_encoder *encoder)
{
- return MODE_OK;
+ tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
+}
+
+static void tda998x_encoder_commit(struct drm_encoder *encoder)
+{
+ tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
}
static void
@@ -818,7 +812,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode)
{
- struct tda998x_priv *priv = to_tda998x_priv(encoder);
+ struct tda998x_priv *priv = encoder_priv(encoder);
uint16_t ref_pix, ref_line, n_pix, n_line;
uint16_t hs_pix_s, hs_pix_e;
uint16_t vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1006,10 +1000,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
}
static enum drm_connector_status
-tda998x_encoder_detect(struct drm_encoder *encoder,
- struct drm_connector *connector)
+tda998x_connector_detect(struct drm_connector *connector, bool force)
{
- struct tda998x_priv *priv = to_tda998x_priv(encoder);
+ struct tda998x_priv *priv = connector_priv(connector);
uint8_t val = cec_read(priv, REG_CEC_RXSHPDLEV);
return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
@@ -1017,9 +1010,8 @@ tda998x_encoder_detect(struct drm_encoder *encoder,
}
static int
-read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
+read_edid_block(struct tda998x_priv
