On 26/09/13 14:01, Dr. H. Nikolaus Schaller wrote:
> So this essentially means that the "invert" property and the "bypass", "acen" properties
> should be defined for the VENC (if it also controls the DAC-Stage).
>
> I.e. I am not sure if invert is really a property (control) of the connector or an amplifier.
Invert bit is programmed to VENC, so it is a property of VENC. At least
in the end.
The question is, does it need to be changed dynamically at runtime? I
don't think so. OPA always wants its input inverted. Without OPA, the TV
out signal is always un-inverted. So we can have it as a property of
VENC, just like bypass and acen.
True, it's currently set at the connector, but I think that's wrong.
>>> The OPA itself then should also participate in suspend/resume. Well, that
>>> would be something important for proper power management. Only then
>>> we can make sure the OPA is disabled correctly.
>>
>> There isn't really anything to suspend/resume on that level. If the
>> display is enabled, it stays enabled, there's no automatic suspend. If
>> there's a system suspend, omapfb (or similar component) will disable the
>> displays.
>>
>> So it's only about enable/disable on this level.
>
> Ok. I think this is also sufficient since the OPA362 is in low power if it is
> disabled. I.e. suspend and disable are the same.
I think that fits more or less all hardware.
Well, sometimes there could be different power-states, and waking up
from those takes different amounts of time. I think that's normally not
needed for display, it doesn't matter if enabling a display encoder
takes 15ms or 100ms.
> Although one could argue that one is just controlling the GPIO and the other
> one is controlling some regulator...
Sorry, didn't understand that one...
>> Well, this is perhaps a bit about semantics, but it is a driver for
>> OPA362 hardware. Sure, we can make a more generic driver if we see that
>> there are other external amps that have very similar controls.
>
> That was my assumption. Analog amplifiers just have an input and an output.
> And can be powered on or off. This would imho fit 99% of all such amplifiers.
> Well, one could think about switching different signal strenghts but this is AFAIK
> not defined by the composite video standards.
Ok.
But I would presume all of them have power and gpio inputs. Does the
power need to be enabled before enabling the gpio, and if so, for how
long does the power need to be enabled until the gpio can be set? Etc. I
don't know if simple components like analog amp needs those, but very
often display component datasheets list quite specifically the order and
timing of all the powers and gpios.
That said... Maybe a generic amp driver would work for the 99% of cases.
It's always difficult to guess what kind of hardware there is out there =).
>> But it's
>> still an OPA362 driver, but it would also be OPA123, OPA321, etc. driver.
>>
>> Making it "external-video-amplifier" driver is probably taking it too
>> far. We don't know what kind of amps there are. Maybe some are
>> controlled via i2c. Dumping all the different functionality for
>> different amps into one driver would just make one messy driver.
>
> Ok, I will start to write an "amplifier-opa362.c" based on the "encoder-ftp410.c"
> code.
Yep, I don't have a strong opinion on whether to create opa362, or more
generic driver. Try one =).
>> That said, if the feature, "invert" in this case, never needs to be
>> changed at runtime, there's no real reason to have that kind of method
>> for OPA to change the inversion. So the board file could just pass the
>> invert flag as a parameter to VENC.
>
> Yes, that is what I now also think. And the flag should/could be removed from
> the connector-analog-tv at all. I.e. I think the opa362 driver should have a call
>
> in->ops.atv->invert_vid_out_polarity(in, true);
>
> in the opa362_enable() code because it knows that it wants to see an inverted
> input.
I think the whole function should be removed. Again, if there's no need
to ever change it during runtime by OPA, the call is quite unnecessary.
And the invert flag could just be passed to VENC in platform data.
And note that with "change it during runtime" I don't mean VENC could
not change it during runtime. If the bit needs to be cleared when the
output is disabled, VENC can do that. But that is VENC's internal thing,
no need for a invert_vid_out_polarity() API for it.
>>> Now how can we proceed?
>>>
>>> For the moment we could try to get the DEVCONF1 setup into the board_init
>>> until a DAC Stage driver and some platform independent API for DEVCONF1
>>> modifications exists.
>>
>> Well, as I don't see the need for the DAC driver, I would just add the
>> function pointers to change DEVCONF1 to struct omap_dss_board_info.
>> Also, the flags to enable/disable invert, bypass and AC would be added
>> to the same struct.
>
> An alternative could be that the opa362 driver can ask the VENC to set these
> values each time it is enabled. This would follow the backwards call chain you
> did describe and would be similar to invert_vid_out_polarity.
I don't know... Again, not so familiar with analog TV signal, but the
ac_en and bypass sound very much like OMAP VENC specific things.
If they can be considered as generic features with analog TV signaling,
at least the names should probably be re-thought. "Bypass" means
VENC/DAC bypasses something. It doesn't sound that it has anything to do
with the signal. If this is something that OPA would touch, the naming
conventions should be something generic. I have no idea what that would
be, but somehow it would need to describe the signal or the connection,
not OMAP VENC/DAC's internal functionality.
But anyway, as I said, I think these can be just set for VENC in
platform data, and I don't see a need for OPA to have any notion of these.
> But for proper power management I am not sure if we should give this responsibility
> to a second stage (and optional) driver.
>
> And: not every opa362 will be connected the same way to a DM3730. I.e. this
> would introduce another set of dependencies.
What options are there? In the TRM I see only one picture, the bypass
one, which depicts an amplifier. The other pictures show a connector, no
amp.
>> Note that at the moment we have just struct omap_dss_board_info, which
>> is platform data for the whole DSS driver, i.e. we don't have separate
>> platform data for VENC. That will probably change at some point in the
>> future.
>
> Ok. I think this can be a second step (if enabling the bypass in the board file
> works). We may require it soon since we are trying to squeeze out every mA
> from the platform and therefore we should have optimal power management
> here as well.
Well, you can try setting it in the board file, but I think the patches
for mainline should have the function pointers. Board files do not exist
when booting with device tree, and I will probably reject any changes
that will not work with DT. No need to add any DT support yet, but the
model should be DT-friendly.
>>> For the external amplifier (OPA362) enable, we can write a simple driver (it just
>>> needs to control a GPIO whose number is passed from the platform data).
>>
>> Yes, and also the regulator code to handle V+.
>
> In our design the V+ is tied to a 3.3V rail and the enable-gpio is also a
> "power-down" input. Like the TFP410 has no dedicated regulator control (yet).
> So I think we don't add this yet (since we can't test).
Yes, I think that's fine.
>>> What I don't know is how such a driver should be integrated into the pipeline
>>
>> Look at the board files. The display components there have "source"
>> field, which points to the source component in the pipeline.
>
> Here is a draft based on your description how I plan to make it work:
>
> static struct connector_dvi_platform_data gta04_dvi_connector_pdata = {
> .name = "dvi",
> .source = "tfp410.0",
> .i2c_bus_num = 3,
> };
>
> static struct platform_device gta04_dvi_connector_device = {
> .name = "connector-dvi",
> .id = 0,
> .dev.platform_data = >a04_dvi_connector_pdata,
> };
>
> static struct encoder_tfp410_platform_data gta04_tfp410_pdata = {
> .name = "tfp410.0",
> .source = "dpi.0",
> .data_lines = 24,
> .power_down_gpio = -1,
> };
>
> static struct platform_device gta04_tfp410_device = {
> .name = "encoder-tfp410",
> .id = 0,
> .dev.platform_data = >a04_tfp410_pdata,
> };
>
> static struct amplifier_opa362_platform_data gta04_opa362_pdata = {
> .name = "opa362.0",
> .source = "venc.0",
> .enable_gpio = TV_OUT_GPIO,
> };
>
> static struct platform_device gta04_opa362_device = {
> .name = "amplifier-opa362",
> .id = 0,
> .dev.platform_data = >a04_opa362_pdata,
> };
>
> static struct connector_atv_platform_data gta04_tv_pdata = {
> .name = "tv",
> .source = "opa362.0",
> .connector_type = OMAP_DSS_VENC_TYPE_COMPOSITE,
> .invert_polarity = true, /* needed if we use external video driver */
> };
Looks fine, except I think you should try to move invert_polarity to
omap_dss_board_info. At least after initial testing.
I don't want to make this more confusing, but... I wonder about the
connector_type field. It's only function is to pass the value to VENC,
which will then work differently. And it's also something that cannot be
changed at runtime. Perhaps that, too, should be moved to VENC's
platform data.
Tomi
Attachment:
signature.asc
Description: OpenPGP digital signature
