Am 13.11.2014 um 17:41 schrieb Tomi Valkeinen [off-list ref]:

On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:

quoted

Hi,

Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen [off-list ref]:

quoted

On 13/11/14 00:10, Marek Belisko wrote:

quoted

opa362 is amplifier for video and can be connected to the tvout pads
of the OMAP3. It has one gpio control for enable/disable of the output
(high impedance).

Signed-off-by: H. Nikolaus Schaller <redacted>
---
drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
.../fbdev/omap2/displays-new/amplifier-opa362.c    | 343 +++++++++++++++++++++

I think it would be better to rename this to encoder-opa362.c. It's not

When developing this driver we did simply rename the encoder-tfp410 file,
but thent hough that it does not fit into the ?encoder? category, because we
would expect something digital or digital to analog ?encoding? which it does not.

That is true, but we already have other "encoders" like
encoder-tpd12s015.c, which also do not encode. "encoder" in this context
means something that takes a video input, and has a video output. In
contrast to a panel or a connector.

quoted

quoted

quoted

+
+	in->ops.atv->set_timings(in, &ddata->timings);
+	/* fixme: should we receive the invert from our consumer, i.e. the connector? */
+	in->ops.atv->invert_vid_out_polarity(in, true);

Well this does seem to be broken. I don't know what the answer to the
question above is, but the code doesn't work properly.

In the opa362_invert_vid_out_polarity function below, you get the invert
boolean from the connector. This you pass to the OMAP venc. However,
above you always override that value in venc with true.

So, either the invert_vid_out_polarity value has to be always true or
false, because _OPA362_ requires it to be true or false, OR you need use
the value from the connector.

Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
latter, and the call above should be removed.

Yes, you are right - this is not systematic.

But the problem is that we can?t ask the connector here what it wants
to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
which we can then forward to overwrite the inital state of this opa362_enable().

You don't need to ask. The connector calls invert_vid_out_polarity
before enabling the output.

Unfortunately it doesn?t. At least not always.

It does only for pdata systems but not for DT based systems:

	if (!ddata->dev->of_node) {
		in->ops.atv->set_type(in, ddata->connector_type);
		in->ops.atv->invert_vid_out_polarity(in,
			ddata->invert_polarity);
	}

Not calling is in our case different from calling with ddata->invert_polarity == 0.

You can just pass it forward inverted, as
you already do in this driver. If it doesn't, it's either a bug or you
can just rely on the value that is already programmed to venc.

Therefore it is not called with ?false? which would make our invert_vid_out_polarity
invert it and send ?true? towards the VENC. So VENC remains non-inverted.

We will also add a patch for the connector-analog.c

quoted

quoted

We are going to support only DT boot at some point. Thus I think the
whole platform data code should be left out.

Is there already a decision? I think it should not be done before. And it
does not harm to still have it.

It's just a matter of time. I don't accept any new boards using platform
data for display, or new display drivers using platform data, because I
don't want to spend my time converting them later. And as this is a new
driver, no existing board can be using the opa362 platform_data. So we
can support DT only.

Ok, that looks reasonable - as long as we can rely on that all mainline DSS
drivers are already fully converted to DT :)

BR,
Nikolaus