lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <BF6765C1-8D5C-4ABA-8796-195058C82E17@goldelico.com>
Date:	Wed, 19 Nov 2014 16:10:26 +0100
From:	"Dr. H. Nikolaus Schaller" <hns@...delico.com>
To:	Tomi Valkeinen <tomi.valkeinen@...com>
Cc:	Marek Belisko <marek@...delico.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Benoit Cousson <bcousson@...libre.com>,
	Tony Lindgren <tony@...mide.com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>,
	grant.likely@...aro.org, devicetree@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>, linux-omap@...r.kernel.org,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	linux-fbdev@...r.kernel.org, gta04-owner@...delico.com
Subject: Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver


Am 13.11.2014 um 17:41 schrieb Tomi Valkeinen <tomi.valkeinen@...com>:

> On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@...com>:
>> 
>>> On 13/11/14 00:10, Marek Belisko wrote:
>>>> 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 <hns@...delico.com>
>>>> ---
>>>> 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.
> 
>>>> +
>>>> +	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

>>> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ