DSS display-new custom enable/disable hooks

DSS display-new custom enable/disable hooks

[Belisko Marek]

Hi,

we're using connector-analog-tv driver to enable TV out on gta04
board. There is exception that we need to change some twl registers +
some gpio when enable/disable TV output. My question is if there is
some way how to do that or do we need to copy'n'paste code from
connector-analog-tv driver and extend it for handling we need (let's
call it hack)?

Thanks,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: DSS display-new custom enable/disable hooks

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On 24/09/13 23:04, Belisko Marek wrote:
> Hi,
> 
> we're using connector-analog-tv driver to enable TV out on gta04
> board. There is exception that we need to change some twl registers +
> some gpio when enable/disable TV output. My question is if there is
> some way how to do that or do we need to copy'n'paste code from
> connector-analog-tv driver and extend it for handling we need (let's
> call it hack)?

What are those TWL registers and GPIO used for? If they are board
specific things, then I see two options:

- Presuming you're using board files, you could add
platform_enable/disable callbacks to the connector's platform_data, and
make the connector driver call those callbacks.

- Create a new display encoder driver, which handles the TWL and GPIO,
and position it in the video pipeline between VENC and the connector.

The first option is obviously not upstreamable, as it won't work with
DT. And I'm not enthusiastic about merging board specific display
drivers either, but so far I haven't figured better ways to implement
board specific oddities.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: DSS display-new custom enable/disable hooks

[Belisko Marek]

CC - Nikolaus Schaller

On Wed, Sep 25, 2013 at 8:12 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 24/09/13 23:04, Belisko Marek wrote:
>> Hi,
>>
>> we're using connector-analog-tv driver to enable TV out on gta04
>> board. There is exception that we need to change some twl registers +
>> some gpio when enable/disable TV output. My question is if there is
>> some way how to do that or do we need to copy'n'paste code from
>> connector-analog-tv driver and extend it for handling we need (let's
>> call it hack)?
>
> What are those TWL registers and GPIO used for? If they are board
> specific things, then I see two options:
>
> - Presuming you're using board files, you could add
> platform_enable/disable callbacks to the connector's platform_data, and
> make the connector driver call those callbacks.
>
> - Create a new display encoder driver, which handles the TWL and GPIO,
> and position it in the video pipeline between VENC and the connector.
>
> The first option is obviously not upstreamable, as it won't work with
> DT. And I'm not enthusiastic about merging board specific display
> drivers either, but so far I haven't figured better ways to implement
> board specific oddities.
>
>  Tomi
>
>

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: DSS display-new custom enable/disable hooks

[Dr. H. Nikolaus Schaller]

Hi,

Am 25.09.2013 um 09:09 schrieb Belisko Marek:

> CC - Nikolaus Schaller
> 
> On Wed, Sep 25, 2013 at 8:12 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 24/09/13 23:04, Belisko Marek wrote:
>>> Hi,
>>> 
>>> we're using connector-analog-tv driver to enable TV out on gta04
>>> board. There is exception that we need to change some twl registers +
>>> some gpio when enable/disable TV output. My question is if there is
>>> some way how to do that or do we need to copy'n'paste code from
>>> connector-analog-tv driver and extend it for handling we need (let's
>>> call it hack)?
>> 
>> What are those TWL registers and GPIO used for?

The GTA04 devices uses an OPA362 video amplifier connected to the TV_VFB1 pin
of the DM3730. According to some not well known discussion with TI [1] we must
therefore configure the AC bias and Bypass through the DEVCONF1 register.

[1] https://e2e.ti.com/support/dsp/omap_applications_processors/f/447/p/94072/343691.aspx

And we have to disable (High-Z) the OPA362 amplifier if TVout is not in use since the
line is shared with a different function (microphone input).

The reasons to use the OPA362 at all are to do short circuit protection and to high-Z
the driver (both are n/a with the DM3730 as far as we know).

The code we did use successfully in the 2.6.32 board file is:

120 #define TV_OUT_GPIO     23

374 static int gta04_panel_enable_tv(struct omap_dss_device *dssdev)
375 {
376         u32 reg;
377 
378 #define ENABLE_VDAC_DEDICATED           0x03
379 #define ENABLE_VDAC_DEV_GRP             0x20
380 #define OMAP2_TVACEN                            (1 << 11)
381 #define OMAP2_TVOUTBYPASS                       (1 << 18)
382 
383         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
384                         ENABLE_VDAC_DEDICATED,
385                         TWL4030_VDAC_DEDICATED);
386         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
387                         ENABLE_VDAC_DEV_GRP, TWL4030_VDAC_DEV_GRP);
388 
389         /* idea taken from https://e2e.ti.com/support/dsp/omap_applications_processors/f/447/p/94072/343691.aspx */
390         reg = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
391 //      printk(KERN_INFO "Value of DEVCONF1 was: %08x\n", reg);
392         reg |= OMAP2_TVOUTBYPASS;       /* enable TV bypass mode for external video driver (for OPA362 driver) */
393         reg |= OMAP2_TVACEN;            /* assume AC coupling to remove DC offset */
394         omap_ctrl_writel(reg, OMAP343X_CONTROL_DEVCONF1);
395         reg = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
396 //      printk(KERN_INFO "Value of DEVCONF1 now: %08x\n", reg);
397 
398         gpio_set_value(TV_OUT_GPIO, 1); /* enable output driver (OPA362) */
399 
400         return 0;
401 }
402 
403 static void gta04_panel_disable_tv(struct omap_dss_device *dssdev)
404 {
405         gpio_set_value(TV_OUT_GPIO, 0); /* disable output driver (and re-enable microphone) */
406 
407         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER, 0x00,
408                         TWL4030_VDAC_DEDICATED);
409         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER, 0x00,
410                         TWL4030_VDAC_DEV_GRP);
411 }
412
413 static struct omap_dss_device gta04_tv_device = {
414         .name = "tv",
415         .driver_name = "venc",
416         .type = OMAP_DISPLAY_TYPE_VENC,
417         /* GTA04 has a single composite output (with external video driver) */
418         .phy.venc.type = OMAP_DSS_VENC_TYPE_COMPOSITE,
419         .phy.venc.invert_polarity = true,       /* needed if we use external video driver */
420         .platform_enable = gta04_panel_enable_tv,
421         .platform_disable = gta04_panel_disable_tv,
422 };

I think (but don't know) that controlling the TWL/VDAC is no longer needed because
it should be done through the connector-analog-tv driver.

So we need to
1. control a GPIO - very similar to a TFT panel
2. invert polarity
3. configure the OMAP343X_CONTROL_DEVCONF1 register as needed

I don't know if we must do step 3. each time we enable TVout (because we can't
assume that no other part of the kernel code writes to the DEVCONF1 register) so that
it could be done in the board init code.

>> If they are board
>> specific things, then I see two options:
>> 
>> - Presuming you're using board files, you could add
>> platform_enable/disable callbacks to the connector's platform_data, and
>> make the connector driver call those callbacks.
>> 
>> - Create a new display encoder driver, which handles the TWL and GPIO,
>> and position it in the video pipeline between VENC and the connector.
>> 
>> The first option is obviously not upstreamable, as it won't work with
>> DT. And I'm not enthusiastic about merging board specific display
>> drivers either, but so far I haven't figured better ways to implement
>> board specific oddities.
>> 
>> Tomi
>> 
>> 
> 
> marek


-- hns

Re: DSS display-new custom enable/disable hooks

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 6060 bytes --]

On 25/09/13 10:45, Dr. H. Nikolaus Schaller wrote:
> Hi,
> 
> Am 25.09.2013 um 09:09 schrieb Belisko Marek:
> 
>> CC - Nikolaus Schaller
>>
>> On Wed, Sep 25, 2013 at 8:12 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> On 24/09/13 23:04, Belisko Marek wrote:
>>>> Hi,
>>>>
>>>> we're using connector-analog-tv driver to enable TV out on gta04
>>>> board. There is exception that we need to change some twl registers +
>>>> some gpio when enable/disable TV output. My question is if there is
>>>> some way how to do that or do we need to copy'n'paste code from
>>>> connector-analog-tv driver and extend it for handling we need (let's
>>>> call it hack)?
>>>
>>> What are those TWL registers and GPIO used for?
> 
> The GTA04 devices uses an OPA362 video amplifier connected to the TV_VFB1 pin
> of the DM3730. According to some not well known discussion with TI [1] we must
> therefore configure the AC bias and Bypass through the DEVCONF1 register.

Ah, OPA. A blast from the past =).

Well, OPA is a distinct hardware block in the video path, I see no issue
in having an OPA encoder driver, that sits in between VENC and the
connector.

The driver should handle things described in the OPA datasheet. With a
quick glance, there seems to be one GPIO and one power line to OPA.

> [1] https://e2e.ti.com/support/dsp/omap_applications_processors/f/447/p/94072/343691.aspx
> 
> And we have to disable (High-Z) the OPA362 amplifier if TVout is not in use since the
> line is shared with a different function (microphone input).
> 
> The reasons to use the OPA362 at all are to do short circuit protection and to high-Z
> the driver (both are n/a with the DM3730 as far as we know).
> 
> The code we did use successfully in the 2.6.32 board file is:
> 
> 120 #define TV_OUT_GPIO     23
> 
> 374 static int gta04_panel_enable_tv(struct omap_dss_device *dssdev)
> 375 {
> 376         u32 reg;
> 377 
> 378 #define ENABLE_VDAC_DEDICATED           0x03
> 379 #define ENABLE_VDAC_DEV_GRP             0x20
> 380 #define OMAP2_TVACEN                            (1 << 11)
> 381 #define OMAP2_TVOUTBYPASS                       (1 << 18)
> 382 
> 383         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
> 384                         ENABLE_VDAC_DEDICATED,
> 385                         TWL4030_VDAC_DEDICATED);
> 386         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
> 387                         ENABLE_VDAC_DEV_GRP, TWL4030_VDAC_DEV_GRP);

All that i2c stuff should be handled by using the normal regulator
framework.

> 388 
> 389         /* idea taken from https://e2e.ti.com/support/dsp/omap_applications_processors/f/447/p/94072/343691.aspx */
> 390         reg = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
> 391 //      printk(KERN_INFO "Value of DEVCONF1 was: %08x\n", reg);
> 392         reg |= OMAP2_TVOUTBYPASS;       /* enable TV bypass mode for external video driver (for OPA362 driver) */
> 393         reg |= OMAP2_TVACEN;            /* assume AC coupling to remove DC offset */
> 394         omap_ctrl_writel(reg, OMAP343X_CONTROL_DEVCONF1);
> 395         reg = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
> 396 //      printk(KERN_INFO "Value of DEVCONF1 now: %08x\n", reg);
> 397 
> 398         gpio_set_value(TV_OUT_GPIO, 1); /* enable output driver (OPA362) */
> 399 
> 400         return 0;
> 401 }
> 402 
> 403 static void gta04_panel_disable_tv(struct omap_dss_device *dssdev)
> 404 {
> 405         gpio_set_value(TV_OUT_GPIO, 0); /* disable output driver (and re-enable microphone) */
> 406 
> 407         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER, 0x00,
> 408                         TWL4030_VDAC_DEDICATED);
> 409         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER, 0x00,
> 410                         TWL4030_VDAC_DEV_GRP);
> 411 }
> 412
> 413 static struct omap_dss_device gta04_tv_device = {
> 414         .name = "tv",
> 415         .driver_name = "venc",
> 416         .type = OMAP_DISPLAY_TYPE_VENC,
> 417         /* GTA04 has a single composite output (with external video driver) */
> 418         .phy.venc.type = OMAP_DSS_VENC_TYPE_COMPOSITE,
> 419         .phy.venc.invert_polarity = true,       /* needed if we use external video driver */
> 420         .platform_enable = gta04_panel_enable_tv,
> 421         .platform_disable = gta04_panel_disable_tv,
> 422 };
> 
> I think (but don't know) that controlling the TWL/VDAC is no longer needed because
> it should be done through the connector-analog-tv driver.

I don't think VDAC is related to the connector (although I didn't
actually look at the analog video when I created the new
connector-analog-driver). If there's something specifically related to
an analog tv out connector that requires power, then, yes, it's needed.
But if it's all about OMAP's VENC and OPA, then the connector itself
does not require anything.

As an example, DVI connector does require a +5V (if I recall right), and
that +5V is not related to the DVI video source, it's only about the
connector. So in that case, the DVI connector driver should handle the
+5V (it doesn't, currently, but will soon be added).

> So we need to
> 1. control a GPIO - very similar to a TFT panel
> 2. invert polarity
> 3. configure the OMAP343X_CONTROL_DEVCONF1 register as needed
> 
> I don't know if we must do step 3. each time we enable TVout (because we can't
> assume that no other part of the kernel code writes to the DEVCONF1 register) so that
> it could be done in the board init code.

So, is the GPIO the one that goes to OPA? Is VDAC the power to OPA? VDAC
is already used by VENC, which manages it, so if VDAC is only used for
VENC, there's no need for to handle it anywhere else.

The DEVCONF1 register is problematic. For that we need some kind of
platform hooks. We have some already for PM and DSI pins, so we could
add a new one for TV-out. However, the DSI pins (hopefully) could be
handled by the pinmuxing framework. Maybe that would be applicable here
also.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: DSS display-new custom enable/disable hooks

[Dr. H. Nikolaus Schaller]

Hi Tomi,

Am 25.09.2013 um 10:29 schrieb Tomi Valkeinen:

> On 25/09/13 10:45, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> Am 25.09.2013 um 09:09 schrieb Belisko Marek:
>> 
>>> CC - Nikolaus Schaller
>>> 
>>> On Wed, Sep 25, 2013 at 8:12 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>> On 24/09/13 23:04, Belisko Marek wrote:
>>>>> Hi,
>>>>> 
>>>>> we're using connector-analog-tv driver to enable TV out on gta04
>>>>> board. There is exception that we need to change some twl registers +
>>>>> some gpio when enable/disable TV output. My question is if there is
>>>>> some way how to do that or do we need to copy'n'paste code from
>>>>> connector-analog-tv driver and extend it for handling we need (let's
>>>>> call it hack)?
>>>> 
>>>> What are those TWL registers and GPIO used for?
>> 
>> The GTA04 devices uses an OPA362 video amplifier connected to the TV_VFB1 pin
>> of the DM3730. According to some not well known discussion with TI [1] we must
>> therefore configure the AC bias and Bypass through the DEVCONF1 register.
> 
> Ah, OPA. A blast from the past =).
> 
> Well, OPA is a distinct hardware block in the video path, I see no issue
> in having an OPA encoder driver, that sits in between VENC and the
> connector.
> 
> The driver should handle things described in the OPA datasheet. With a
> quick glance, there seems to be one GPIO and one power line to OPA.

Hm. Why need a driver for a specific piece of hardware? For software it is just an
analog TV out with additional control options.

And, it must not be the OPA362 - it could be something completely different.

> 
>> [1] https://e2e.ti.com/support/dsp/omap_applications_processors/f/447/p/94072/343691.aspx
>> 
>> And we have to disable (High-Z) the OPA362 amplifier if TVout is not in use since the
>> line is shared with a different function (microphone input).
>> 
>> The reasons to use the OPA362 at all are to do short circuit protection and to high-Z
>> the driver (both are n/a with the DM3730 as far as we know).
>> 
>> The code we did use successfully in the 2.6.32 board file is:
>> 
>> 120 #define TV_OUT_GPIO     23
>> 
>> 374 static int gta04_panel_enable_tv(struct omap_dss_device *dssdev)
>> 375 {
>> 376         u32 reg;
>> 377 
>> 378 #define ENABLE_VDAC_DEDICATED           0x03
>> 379 #define ENABLE_VDAC_DEV_GRP             0x20
>> 380 #define OMAP2_TVACEN                            (1 << 11)
>> 381 #define OMAP2_TVOUTBYPASS                       (1 << 18)
>> 382 
>> 383         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
>> 384                         ENABLE_VDAC_DEDICATED,
>> 385                         TWL4030_VDAC_DEDICATED);
>> 386         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
>> 387                         ENABLE_VDAC_DEV_GRP, TWL4030_VDAC_DEV_GRP);
> 
> All that i2c stuff should be handled by using the normal regulator
> framework.
> 
>> 388 
>> 389         /* idea taken from https://e2e.ti.com/support/dsp/omap_applications_processors/f/447/p/94072/343691.aspx */
>> 390         reg = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
>> 391 //      printk(KERN_INFO "Value of DEVCONF1 was: %08x\n", reg);
>> 392         reg |= OMAP2_TVOUTBYPASS;       /* enable TV bypass mode for external video driver (for OPA362 driver) */
>> 393         reg |= OMAP2_TVACEN;            /* assume AC coupling to remove DC offset */
>> 394         omap_ctrl_writel(reg, OMAP343X_CONTROL_DEVCONF1);
>> 395         reg = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
>> 396 //      printk(KERN_INFO "Value of DEVCONF1 now: %08x\n", reg);
>> 397 
>> 398         gpio_set_value(TV_OUT_GPIO, 1); /* enable output driver (OPA362) */
>> 399 
>> 400         return 0;
>> 401 }
>> 402 
>> 403 static void gta04_panel_disable_tv(struct omap_dss_device *dssdev)
>> 404 {
>> 405         gpio_set_value(TV_OUT_GPIO, 0); /* disable output driver (and re-enable microphone) */
>> 406 
>> 407         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER, 0x00,
>> 408                         TWL4030_VDAC_DEDICATED);
>> 409         twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER, 0x00,
>> 410                         TWL4030_VDAC_DEV_GRP);
>> 411 }
>> 412
>> 413 static struct omap_dss_device gta04_tv_device = {
>> 414         .name = "tv",
>> 415         .driver_name = "venc",
>> 416         .type = OMAP_DISPLAY_TYPE_VENC,
>> 417         /* GTA04 has a single composite output (with external video driver) */
>> 418         .phy.venc.type = OMAP_DSS_VENC_TYPE_COMPOSITE,
>> 419         .phy.venc.invert_polarity = true,       /* needed if we use external video driver */
>> 420         .platform_enable = gta04_panel_enable_tv,
>> 421         .platform_disable = gta04_panel_disable_tv,
>> 422 };
>> 
>> I think (but don't know) that controlling the TWL/VDAC is no longer needed because
>> it should be done through the connector-analog-tv driver.
> 
> I don't think VDAC is related to the connector (although I didn't
> actually look at the analog video when I created the new
> connector-analog-driver). If there's something specifically related to
> an analog tv out connector that requires power, then, yes, it's needed.
> But if it's all about OMAP's VENC and OPA, then the connector itself
> does not require anything.
> 
> As an example, DVI connector does require a +5V (if I recall right), and
> that +5V is not related to the DVI video source, it's only about the
> connector. So in that case, the DVI connector driver should handle the
> +5V (it doesn't, currently, but will soon be added).
> 
>> So we need to
>> 1. control a GPIO - very similar to a TFT panel
>> 2. invert polarity
>> 3. configure the OMAP343X_CONTROL_DEVCONF1 register as needed
>> 
>> I don't know if we must do step 3. each time we enable TVout (because we can't
>> assume that no other part of the kernel code writes to the DEVCONF1 register) so that
>> it could be done in the board init code.
> 
> So, is the GPIO the one that goes to OPA?

Yes. Like the enable_gpio in struct panel_dpi_platform_data.

So I now think we would be happy if the connector-analog-tv would have a gpio
in its platform data that can handle such an enable/disable.

> Is VDAC the power to OPA?

No. The OPA is powered by an external 3.3V rail. I don't really remember why
we control VDAC in that piece of code. It was copied from the beagle board
validation kernel. Maybe it was not done automatically by the 2.6.32 DSS code.

> VDAC
> is already used by VENC, which manages it, so if VDAC is only used for
> VENC, there's no need for to handle it anywhere else.

That is what I also would assume.

> 
> The DEVCONF1 register is problematic. For that we need some kind of
> platform hooks. We have some already for PM and DSI pins, so we could
> add a new one for TV-out. However, the DSI pins (hopefully) could be
> handled by the pinmuxing framework. Maybe that would be applicable here
> also.

Hm. Looks like a complex solution for a simple problem and isn't related to
pinmuxing (IMHO).

Could we just add a "enable_ac" and "enable_bypass"  to struct connector_atv_platform_data,
that sets/resets these flags in the DEVCONF1 register each time the tvout is enabled?

I think this would be very similar to the "invert_polarity" property.

Then, the platform data has more control over the shape of the signals created by
the VENC (independently of that we use an OPA362).

IMHO this topic is very similar to controlling the polarity and timing of the
HSYNC/VSYNC of a LCD (.flags in struct display_timing), i.e. shaping the
interface signals so that they are accepted by the hardware connected to
the DM3730.

If you think this would be a reasonable approach, we can try to prepare a
patch to connector-analog-tv.c for further discussion.

BR,
Nikolaus

Re: DSS display-new custom enable/disable hooks

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5475 bytes --]

On 25/09/13 12:00, Dr. H. Nikolaus Schaller wrote:

>> Well, OPA is a distinct hardware block in the video path, I see no issue
>> in having an OPA encoder driver, that sits in between VENC and the
>> connector.
>>
>> The driver should handle things described in the OPA datasheet. With a
>> quick glance, there seems to be one GPIO and one power line to OPA.
> 
> Hm. Why need a driver for a specific piece of hardware? For software it is just an
> analog TV out with additional control options.

I'm not sure I follow. Linux device drivers are drivers for specific
pieces of hardware. If the hardware needs to be controlled somehow, like
setting gpios, enabling regulators, then we need a driver for that piece
of hardware.

> And, it must not be the OPA362 - it could be something completely different.

Right. If it's something completely different, then you need a driver
for the completely different piece of hardware. At the board code, or
board's device tree data, you specify which pieces of hardware you have
on your board, and the respective drivers are then used.

>> So, is the GPIO the one that goes to OPA?
> 
> Yes. Like the enable_gpio in struct panel_dpi_platform_data.
> 
> So I now think we would be happy if the connector-analog-tv would have a gpio
> in its platform data that can handle such an enable/disable.

Sure, but the gpio is not related to the connector, it's related to
another component. And say, if we added the enable gpio to the
connector. What if the next board uses another version of OPA that
requires two gpios. Should we then add another gpio to the connector?
Maybe third also? How about a bunch of regulators? Where does it end?

>> The DEVCONF1 register is problematic. For that we need some kind of
>> platform hooks. We have some already for PM and DSI pins, so we could
>> add a new one for TV-out. However, the DSI pins (hopefully) could be
>> handled by the pinmuxing framework. Maybe that would be applicable here
>> also.
> 
> Hm. Looks like a complex solution for a simple problem and isn't related to
> pinmuxing (IMHO).

Right, I only suggested pinmuxing as that's why we need the callback for
DSI. I don't remember the details about DEVCONF1 and tv-out.

And I disagree that it's a simple problem =). Making generic, reusable
drivers is not simple. Making a hack to make it work on your particular
platform and board is simple, though.

> Could we just add a "enable_ac" and "enable_bypass"  to struct connector_atv_platform_data,
> that sets/resets these flags in the DEVCONF1 register each time the tvout is enabled?

No, those are properties of the SoC, as far as I understand, nothing to
do with the connector. Although I have to say I have no idea what "TV AC
coupled load enable" or "Active high enable AVDAC TV out bypass" do.

I guess I need to refresh my memory on the VENC, although if I recall
right, it wasn't explained very well in the TRM.

Also, note that drivers cannot touch DEVCONF1 register. That can only be
touched by the OMAP's platform code. So there has to be some kind of
callback or API to do that. And while the display drivers for OMAP are
currently OMAP specific, they will be made generic at some point, and
actually are already designed to be so. So they may not contain any
OMAP'isms.

> I think this would be very similar to the "invert_polarity" property.

I have to say that I don't quite remember what the invert_polarity is
used for, but I think invert_polarity might be misplaced also. Is the
analog video signal that goes out from the connector inverted? I don't
think so. Does the connector require an inverted signal? No, as the
connector is really nothing else than a pass-though connector.

Correct me if I'm wrong, but I think the invert_polarity is required if
there's something on the board that requires the signal to be inverted
(OPA?), and it'll be re-inverterted before the signal goes to the analog
connector.

> Then, the platform data has more control over the shape of the signals created by
> the VENC (independently of that we use an OPA362).
> 
> IMHO this topic is very similar to controlling the polarity and timing of the
> HSYNC/VSYNC of a LCD (.flags in struct display_timing), i.e. shaping the
> interface signals so that they are accepted by the hardware connected to
> the DM3730.

Well, I was never too well into the analog TV-out side, so please argue
if I say something silly here. But generally speaking, there are two
different kinds of properties here.

Consider DPI output from the SoC, and a DPI panel. Let's say the DPI
panel requires horizontal sync signal to be active high. It's a property
of the panel, and it makes sense (at least to me) to think that the DPI
panel asks the DPI encoder to provide a signal with hsync active high.
Also, it doesn't matter what kind of DPI encoder there is, the hsync
active high is a "universal" property.

Then, let's say the board has some electrical issues and requires the
DPI signals to be of slightly higher voltage than normally. I think OMAP
at least has bits to driver some pins with higher voltage. This property
is not about panel. There could be lots of different kinds of tweaks for
different SoCs, and they are SoC (or DPI encoder) specific things. Here
is makes no sense to have a property for the panel.

To me, enable_ac and enable_bypass sound like the latter kind.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: DSS display-new custom enable/disable hooks

[Dr. H. Nikolaus Schaller]

Hi Tomi,

Am 25.09.2013 um 12:50 schrieb Tomi Valkeinen:

> On 25/09/13 12:00, Dr. H. Nikolaus Schaller wrote:
> 
>>> Well, OPA is a distinct hardware block in the video path, I see no issue
>>> in having an OPA encoder driver, that sits in between VENC and the
>>> connector.
>>> 
>>> The driver should handle things described in the OPA datasheet. With a
>>> quick glance, there seems to be one GPIO and one power line to OPA.
>> 
>> Hm. Why need a driver for a specific piece of hardware? For software it is just an
>> analog TV out with additional control options.
> 
> I'm not sure I follow. Linux device drivers are drivers for specific
> pieces of hardware. If the hardware needs to be controlled somehow, like
> setting gpios, enabling regulators, then we need a driver for that piece
> of hardware.

> 
>> And, it must not be the OPA362 - it could be something completely different.
> 
> Right. If it's something completely different, then you need a driver
> for the completely different piece of hardware. At the board code, or
> board's device tree data, you specify which pieces of hardware you have
> on your board, and the respective drivers are then used

> 
>>> So, is the GPIO the one that goes to OPA?
>> 
>> Yes. Like the enable_gpio in struct panel_dpi_platform_data.
>> 
>> So I now think we would be happy if the connector-analog-tv would have a gpio
>> in its platform data that can handle such an enable/disable.
> 
> Sure, but the gpio is not related to the connector, it's related to
> another component.

I would see it as a part of the interface.

> And say, if we added the enable gpio to the
> connector. What if the next board uses another version of OPA that
> requires two gpios. Should we then add another gpio to the connector?
> Maybe third also? How about a bunch of regulators? Where does it end?

Yes. Because we did have this already for the LCD panels (before display-new)
and it was reduced in the latest updates to a single gpio for reasons I don't know.

But why should we hesitate to solve an existing problem because there could
arrive a not-yet existing one of low likelihood?

> 
>>> The DEVCONF1 register is problematic. For that we need some kind of
>>> platform hooks. We have some already for PM and DSI pins, so we could
>>> add a new one for TV-out. However, the DSI pins (hopefully) could be
>>> handled by the pinmuxing framework. Maybe that would be applicable here
>>> also.
>> 
>> Hm. Looks like a complex solution for a simple problem and isn't related to
>> pinmuxing (IMHO).
> 
> Right, I only suggested pinmuxing as that's why we need the callback for
> DSI. I don't remember the details about DEVCONF1 and tv-out.
> 
> And I disagree that it's a simple problem =). Making generic, reusable
> drivers is not simple.
> Making a hack to make it work on your particular
> platform and board is simple, though.

But could be a good starting point until someone else has a real need for
a different solution :)

And in my view our "hack" would just be an add-on, not removing any existing
functionality. I.e. all those who were happy with the existing solution will be
as well in the future.

The only conflict I currently see is that if they don't initialize the enable_gpio to -1
and happen to have a gpio #0 there would be an initialization problem.

Anyways the connector-analog-tv is really new in the kernel and we are probably
the first ones who try to use it in real life.

> 
>> Could we just add a "enable_ac" and "enable_bypass"  to struct connector_atv_platform_data,
>> that sets/resets these flags in the DEVCONF1 register each time the tvout is enabled?
> 
> No, those are properties of the SoC, as far as I understand, nothing to
> do with the connector.

Hm. I again get the impression that we have a different view on what a "connector" is.

For me "the connector" that should be seen in kernel code are the analog video
outputs of the SoC (i.e. TVOUT1, TV_VFB1, TV_OU2, TV_VFB2). Not the physical
one going to some TV screen or other video signal consumer outside the board.

Like with UARTs. They are not drivers for a 9 or 25 pin socket. And there may be
a level shifetr chip in between that the software does not see.

> Although I have to say I have no idea what "TV AC
> coupled load enable" or "Active high enable AVDAC TV out bypass" do.
> 
> I guess I need to refresh my memory on the VENC, although if I recall
> right, it wasn't explained very well in the TRM.

There is a post-amplifier stage right after the Video DAC on the DM3730 chip
and these bits we discuss control some electrical properties of that post-amplifier.

I.e. bypass means that it is turned off and the VDAC signal is directly fed to the
output pin.

Section 7.2.3 and 7-58 to 7-61 depict this.

> Also, note that drivers cannot touch DEVCONF1 register. That can only be
> touched by the OMAP's platform code.

Ah, I see.

That may be one of the reasons why it did end up in our 2.6.32 board file,
because there we know that it is an OMAP3 and can directly read/write registers.

But I wonder how drivers access the DISPC to modify e.g. sync polarity or
the video timing?

Is this also an API exposed by the OMAP core driver or is it part of the DSS
drivers? So somewhere in the complex thing someone must know the
register address and be able to write to it.

> So there has to be some kind of
> callback or API to do that. And while the display drivers for OMAP are
> currently OMAP specific, they will be made generic at some point, and
> actually are already designed to be so. So they may not contain any
> OMAP'isms.
> 
>> I think this would be very similar to the "invert_polarity" property.
> 
> I have to say that I don't quite remember what the invert_polarity is
> used for, but I think invert_polarity might be misplaced also. Is the
> analog video signal that goes out from the connector inverted? I don't
> think so. Does the connector require an inverted signal? No, as the
> connector is really nothing else than a pass-though connector.

Hm. Could you define the word "connector" here? Do you mean the
physical plug (S-Video socket or RCA/Chinch composite video)?

Or a logical connector (i.e. the DM3730 port ignoring the hardware
that may come behind until we end at some physical connector)?

> 
> Correct me if I'm wrong, but I think the invert_polarity is required if
> there's something on the board that requires the signal to be inverted
> (OPA?), and it'll be re-inverterted before the signal goes to the analog
> connector.

Yes. There can be three different hardware designs. One with an inverting
amplifier (like we have) and some without. And some without any amplifier
(AFAIK like the Beagle Board).

Depending on that, they need AC-Bias, invert and bypass or don't to
create a standards compliant composite video signal.

Apparently this should be configured through some platform data, because
it is board specific - but there is no reasonable need to switch it during
operation.

> 
>> Then, the platform data has more control over the shape of the signals created by
>> the VENC (independently of that we use an OPA362).
>> 
>> IMHO this topic is very similar to controlling the polarity and timing of the
>> HSYNC/VSYNC of a LCD (.flags in struct display_timing), i.e. shaping the
>> interface signals so that they are accepted by the hardware connected to
>> the DM3730.
> 
> Well, I was never too well into the analog TV-out side, so please argue
> if I say something silly here. But generally speaking, there are two
> different kinds of properties here.
> 
> Consider DPI output from the SoC, and a DPI panel. Let's say the DPI
> panel requires horizontal sync signal to be active high. It's a property
> of the panel, and it makes sense (at least to me) to think that the DPI
> panel asks the DPI encoder to provide a signal with hsync active high.

Yes. I don't see much difference to connecting a video amplifier to the VENC
output pins from connecting a panel to the DSS output pins and configuring
them. They need some specific signal shapes.

> Also, it doesn't matter what kind of DPI encoder there is, the hsync
> active high is a "universal" property.

Yes and therefore I would suggest to see inverted, ac-bias, bypass as
such "universal" properties of the VENC driver.

> Then, let's say the board has some electrical issues and requires the
> DPI signals to be of slightly higher voltage than normally. I think OMAP
> at least has bits to driver some pins with higher voltage.

I think an even better example would be a panel that requires 3.3V I/O
levels instead of the 1.8V levels the DM3730 provides.

Now there could be some scenarios:
a) the DM3730 were able to be switched to provide either 1.8V or 3.3V (like on
the MMC interface)
b) In such a case one would add some level shifter chips (that usually don't
invert anything) and there is no need to switch anything by software
c) there could be level shifters that invert all signals. Then, there would be
need for a different control bit in the DM3730.

But in practice the DM3730 has no control for that at all and there are no
inverting level shifter chips. So software doesn't see these options at all.

> This property
> is not about panel.

Well, it would be imposed by choosing a specific panel - either a 1.8V or
a 3.3V model. So I'd argue that from platform data point of view it is no
difference to specifying different hsync polarity or front porch timing. All
those are found and defined in the panel data sheet.

> There could be lots of different kinds of tweaks for
> different SoCs, and they are SoC (or DPI encoder) specific things. Here
> is makes no sense to have a property for the panel.

Yes, as long as the hardware does a modification that is transparent (i.e. no
change in logic) to the software, it does not need a control...

> To me, enable_ac and enable_bypass sound like the latter kind.

But the controls are part of the DM3730 and not our external hardware. I.e. they
should IMHO be part of the set of the DM3730 drivers and not a separate
one related to our hardware around. It is of course our hardware that asks
to modify the DM3730 settings during boot or operation.

Hm. Indeed diffiicult for an apparently simple problem and I hadn't expected
that it gets so a long discussion :) But really good solutions need that before.

BR,
Nikolaus

Re: DSS display-new custom enable/disable hooks

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 7970 bytes --]

On 25/09/13 15:26, Dr. H. Nikolaus Schaller wrote:

>> Sure, but the gpio is not related to the connector, it's related to
>> another component.
> 
> I would see it as a part of the interface.

I don't know what you mean here with "interface".

> But why should we hesitate to solve an existing problem because there could
> arrive a not-yet existing one of low likelihood?

Even if a solution works, but is clearly not correct in design-sense, I
don't want to merge it. Why? In the future, if and when the wrong
solution creates problems, somebody needs to fix it. Who'll it be? Yes,
me, the maintainer of the software. And so I want to avoid merging
anything that I know might cause problems in the future.

>> And I disagree that it's a simple problem =). Making generic, reusable
>> drivers is not simple.
>> Making a hack to make it work on your particular
>> platform and board is simple, though.
> 
> But could be a good starting point until someone else has a real need for
> a different solution :)

Well, see my comment above about who'll need to fix it =). Even if we
don't find a good solution, and merge the hack, I want first to try to
find a good solution.

> Anyways the connector-analog-tv is really new in the kernel and we are probably
> the first ones who try to use it in real life.

Possibly. I did try it with Beagle, if I recall right, but I guess it's
maybe the simplest possible hardware setup.

>>> Could we just add a "enable_ac" and "enable_bypass"  to struct connector_atv_platform_data,
>>> that sets/resets these flags in the DEVCONF1 register each time the tvout is enabled?
>>
>> No, those are properties of the SoC, as far as I understand, nothing to
>> do with the connector.
> 
> Hm. I again get the impression that we have a different view on what a "connector" is.

"Connector" here means the physical component that is the
S-video/composite connector, to which the cable is connected. In this
case it doesn't really do much at all, but we have a driver for it
because the display device model requires it. We need a kind of
"terminator" at the end of the video pipeline, and that is either a
panel driver or a connector driver.

Let me describe how DVI works on Beagle/Panda, to give a better example.
We have the following components, arrows showing the dataflow:

DPI -> TFP410 -> DVI connector

DPI is the DPI encoder in the SoC's DSS. TFP410 takes DPI input, and
outputs DVI. And DVI connector represents the physical connector on the
board.

We have drivers for each of those components, and they handle things
related only to that particular component:

- DPI driver handles programming the DPI registers on the SoC, and any
regulators required to power the DPI encoder.

- TFP410 driver handles power-down GPIO to enable/disable TFP410, and
regulators related to it.

- DVI connector driver uses i2c to read the EDID from the monitor, and
also supplies the required +5V to the DVI connector. It could also
handle hotplug detection.

Each driver should only be concerned about things related to that
particular component. For example, you could change the SoC, you could
change the TFP410, but DVI connector would still be the same.

> For me "the connector" that should be seen in kernel code are the analog video
> outputs of the SoC (i.e. TVOUT1, TV_VFB1, TV_OU2, TV_VFB2). Not the physical
> one going to some TV screen or other video signal consumer outside the board.
> 
> Like with UARTs. They are not drivers for a 9 or 25 pin socket. And there may be
> a level shifetr chip in between that the software does not see.

Right. For UART, it works. For display, we have so complex video
pipelines that such a simple model does not work.

Of course, if the video pipeline has so trivial components that require
no control and no regulators, well, I don't see a need to have drivers
for those.

>> Although I have to say I have no idea what "TV AC
>> coupled load enable" or "Active high enable AVDAC TV out bypass" do.
>>
>> I guess I need to refresh my memory on the VENC, although if I recall
>> right, it wasn't explained very well in the TRM.
> 
> There is a post-amplifier stage right after the Video DAC on the DM3730 chip
> and these bits we discuss control some electrical properties of that post-amplifier.
> 
> I.e. bypass means that it is turned off and the VDAC signal is directly fed to the
> output pin.
> 
> Section 7.2.3 and 7-58 to 7-61 depict this.

Yep, I see it, but I don't understand it =). As I said, I'm not familiar
with analog TV signal, and I'm a SW guy.

Let me put it this way: In your case, there's the following video pipeline:

VENC -> OPA -> Connector

Here VENC is part of the SoC's DSS, and OPA and connector are external
components on the board.

VENC outputs a video signal, and obviously any register changes required
which affect the signal need to be done directly or indirectly by the
VENC driver.

If OPA requires an inverted signal, it's between VENC and OPA to handle
that. Connector is not involved.

The question here is how to handle the above. Should OPA driver request
VENC driver to invert the signal? Or should VENC driver just be passed
parameters from the board code making it invert the signal? This is
something I don't have an answer, and I've been wondering about this
design question for other video interfaces also.

In both cases the VENC driver needs somehow get the DEVCONF1 register to
be changed. For that we need some support from the core platform code,
probably a function pointer in the DSS platform data, similar to, say
"dsi_enable_pads".

Or alternatively (but not so nicely), if there's no need to change the
DEVCONF1 bits during normal operation, the DEVCONF1 could be configured
once at boot time (although I don't know how to do that with DT boot),
and it would be considered as a fixed setting.

Sometimes the latter approach is ok, but I dislike hardcoding things
like that, as more often than not, it is a problem at some point in the
future. Here, with analog TV out, I presume nobody will be using it in
the future so perhaps it's not a problem =).

>> Also, note that drivers cannot touch DEVCONF1 register. That can only be
>> touched by the OMAP's platform code.
> 
> Ah, I see.
> 
> That may be one of the reasons why it did end up in our 2.6.32 board file,
> because there we know that it is an OMAP3 and can directly read/write registers.
> 
> But I wonder how drivers access the DISPC to modify e.g. sync polarity or
> the video timing?

DISPC is handled by the DISPC driver (i.e. omapdss driver).

> Is this also an API exposed by the OMAP core driver or is it part of the DSS
> drivers? So somewhere in the complex thing someone must know the
> register address and be able to write to it.

DEVCONF1 is part of the CONTROL block, which is a core part of OMAP.
DISPC is part of DSS block, which is not considered to be part of OMAP
core. The idea here is that, in theory, the same DSS could be used in
some other SoC, which has different methods to manage the things
DEVCONF1 does.

So the DSS driver can just touch the DSS registers. Changing DEVCONF1
must happen indirectly in a platform independent way.


I think the rest of the comments in your email were more about our
different understanding of what connector here means and of the display
driver model in general. If there's something unclear, and I didn't
answer to it, just ask again.

> Hm. Indeed diffiicult for an apparently simple problem and I hadn't expected
> that it gets so a long discussion :) But really good solutions need that before.

Well, video is complex. People always think video is simple =). And the
case here is even of the simpler ones. Just think about DSI bus with
encoders with multiple inputs or outputs, and the complexity goes
through the roof...

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: DSS display-new custom enable/disable hooks

[Dr. H. Nikolaus Schaller]

Hi Tomi,

Am 25.09.2013 um 15:57 schrieb Tomi Valkeinen:

> On 25/09/13 15:26, Dr. H. Nikolaus Schaller wrote:
> 
>>> Sure, but the gpio is not related to the connector, it's related to
>>> another component.
>> 
>> I would see it as a part of the interface.
> 
> I don't know what you mean here with "interface".

interface = set of mechanical, electrical and logical definitions so that two components can be connected and work together for communication

Here I refer mostly to the signals coming out / going into the DM3730 chip.

> 
>> But why should we hesitate to solve an existing problem because there could
>> arrive a not-yet existing one of low likelihood?
> 
> Even if a solution works, but is clearly not correct in design-sense, I
> don't want to merge it. Why? In the future, if and when the wrong
> solution creates problems, somebody needs to fix it. Who'll it be? Yes,
> me, the maintainer of the software. And so I want to avoid merging
> anything that I know might cause problems in the future.

Yes, I understand that and therefore we discuss it before we expect that
you accept any code.

> 
>>> And I disagree that it's a simple problem =). Making generic, reusable
>>> drivers is not simple.
>>> Making a hack to make it work on your particular
>>> platform and board is simple, though.
>> 
>> But could be a good starting point until someone else has a real need for
>> a different solution :)
> 
> Well, see my comment above about who'll need to fix it =). Even if we
> don't find a good solution, and merge the hack, I want first to try to
> find a good solution.
> 
>> Anyways the connector-analog-tv is really new in the kernel and we are probably
>> the first ones who try to use it in real life.
> 
> Possibly. I did try it with Beagle, if I recall right, but I guess it's
> maybe the simplest possible hardware setup.
> 
>>>> Could we just add a "enable_ac" and "enable_bypass"  to struct connector_atv_platform_data,
>>>> that sets/resets these flags in the DEVCONF1 register each time the tvout is enabled?
>>> 
>>> No, those are properties of the SoC, as far as I understand, nothing to
>>> do with the connector.
>> 
>> Hm. I again get the impression that we have a different view on what a "connector" is.
> 
> "Connector" here means the physical component that is the
> S-video/composite connector, to which the cable is connected. In this
> case it doesn't really do much at all, but we have a driver for it
> because the display device model requires it. We need a kind of
> "terminator" at the end of the video pipeline, and that is either a
> panel driver or a connector driver.

Hm. I a not sure if this model is complete if it ends at the physical connector.
But that would be a different topic.

> Let me describe how DVI works on Beagle/Panda, to give a better example.
> We have the following components, arrows showing the dataflow:
> 
> DPI -> TFP410 -> DVI connector
> 
> DPI is the DPI encoder in the SoC's DSS. TFP410 takes DPI input, and
> outputs DVI. And DVI connector represents the physical connector on the
> board.

Ok!

> We have drivers for each of those components, and they handle things
> related only to that particular component:
> 
> - DPI driver handles programming the DPI registers on the SoC, and any
> regulators required to power the DPI encoder.

Ok!

> 
> - TFP410 driver handles power-down GPIO to enable/disable TFP410, and
> regulators related to it.

Ok!

> 
> - DVI connector driver uses i2c to read the EDID from the monitor, and
> also supplies the required +5V to the DVI connector. It could also
> handle hotplug detection.

Ah, I start to understand the way how you are thinking: a signal-transformation
pipeline starting with the framebuffer RAM and ending with something we can see.

This is much more fine grained than I had expected.

> Each driver should only be concerned about things related to that
> particular component. For example, you could change the SoC, you could
> change the TFP410, but DVI connector would still be the same.
> 
>> For me "the connector" that should be seen in kernel code are the analog video
>> outputs of the SoC (i.e. TVOUT1, TV_VFB1, TV_OU2, TV_VFB2). Not the physical
>> one going to some TV screen or other video signal consumer outside the board.
>> 
>> Like with UARTs. They are not drivers for a 9 or 25 pin socket. And there may be
>> a level shifetr chip in between that the software does not see.
> 
> Right. For UART, it works. For display, we have so complex video
> pipelines that such a simple model does not work.
> 
> Of course, if the video pipeline has so trivial components that require
> no control and no regulators, well, I don't see a need to have drivers
> for those.

Ok!

> 
>>> Although I have to say I have no idea what "TV AC
>>> coupled load enable" or "Active high enable AVDAC TV out bypass" do.
>>> 
>>> I guess I need to refresh my memory on the VENC, although if I recall
>>> right, it wasn't explained very well in the TRM.
>> 
>> There is a post-amplifier stage right after the Video DAC on the DM3730 chip
>> and these bits we discuss control some electrical properties of that post-amplifier.
>> 
>> I.e. bypass means that it is turned off and the VDAC signal is directly fed to the
>> output pin.
>> 
>> Section 7.2.3 and 7-58 to 7-61 depict this.
> 
> Yep, I see it, but I don't understand it =). As I said, I'm not familiar
> with analog TV signal, and I'm a SW guy.
> 
> Let me put it this way: In your case, there's the following video pipeline:
> 
> VENC -> OPA -> Connector
> 
> Here VENC is part of the SoC's DSS, and OPA and connector are external
> components on the board.

Well, I understand it that way:

RAM -> DISPC -> Video Encoder -> DAC-Stage (the DAC and an output amplifier within the OMAP chip) -> OMAP pins -> external amplifier (OPA) -> physical Connector -> cable -> TVset connector -> some more processing -> panel/screen -> Consumer

or simplified to the most important parts:

DISPC -> Video Encoder -> DAC-Stage -> external amplifier -> physical Connector

the external amplifier is something specific to our board so that we have to insert it into the pipeline.

As you said above, if it would not have any controls we wouldn't have to care about it. But it needs to be enabled/disabled.

The other controls are:

"Bypass" means to bypass something in the DAC-Stage
"AC" means to modify the DAC-Stage output level.
"Invert" is probably a property of the VENC or could also be part of the DAC stage (I don't know).

BTW: I have seen a CAUTION block that describes in the last section how some registers
must be set to avoid current leakage. That should be done (if not yet) in the suspend
code.

> VENC outputs a video signal, and obviously any register changes required
> which affect the signal need to be done directly or indirectly by the
> VENC driver.
> 
> If OPA requires an inverted signal, it's between VENC and OPA to handle
> that. Connector is not involved.

So we are not really missing an OPA362 driver but the DAC Stage driver within the DM3730 SoC.

And a mechanism that enabling/disabling the "tv" display automatically enables/disables
all those stages + including an external OPA362.

This raises some questions: So how can such a pipeline be individually set up
by platform data? How does enable/disable work along the chain?

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.

But then I would not call it opa362 driver but "external-video-amplifier"
with an enable-gpio that can be defined in the platform data or -1 if it is not
required. In the latter case the driver simply does nothing functional.

> The question here is how to handle the above. Should OPA driver request
> VENC driver to invert the signal? Or should VENC driver just be passed
> parameters from the board code making it invert the signal?

Well, I think that it is the responsibility of those who have designed such a
board and they have to configure the drivers for each pipeline stage in a
consistent way (i.e. if one is inverted the other should be as well, unless
they want to see an inverted signal).

Your idea of the OPA driver notifying the VENC driver would introduce an
information flow that does not exist at all in hardware. I.e. there is no "invert me"-wire
in either direction. And that inversion could even happen behind the connector...

They are both defined and set to a fixed state once by the board designer.

> This is
> something I don't have an answer, and I've been wondering about this
> design question for other video interfaces also.
> 
> In both cases the VENC driver needs somehow get the DEVCONF1 register to
> be changed. For that we need some support from the core platform code,
> probably a function pointer in the DSS platform data, similar to, say
> "dsi_enable_pads".

Yes, that sounds reasonable.

> Or alternatively (but not so nicely), if there's no need to change the
> DEVCONF1 bits during normal operation, the DEVCONF1 could be configured
> once at boot time (although I don't know how to do that with DT boot),
> and it would be considered as a fixed setting.

Well, for the moment (before the DAC stage driver exists) we can try to
do that in the board_init function. We don't use the DT yet.

> Sometimes the latter approach is ok, but I dislike hardcoding things
> like that, as more often than not, it is a problem at some point in the
> future. Here, with analog TV out, I presume nobody will be using it in
> the future so perhaps it's not a problem =).
> 
>>> Also, note that drivers cannot touch DEVCONF1 register. That can only be
>>> touched by the OMAP's platform code.
>> 
>> Ah, I see.
>> 
>> That may be one of the reasons why it did end up in our 2.6.32 board file,
>> because there we know that it is an OMAP3 and can directly read/write registers.
>> 
>> But I wonder how drivers access the DISPC to modify e.g. sync polarity or
>> the video timing?
> 
> DISPC is handled by the DISPC driver (i.e. omapdss driver).
> 
>> Is this also an API exposed by the OMAP core driver or is it part of the DSS
>> drivers? So somewhere in the complex thing someone must know the
>> register address and be able to write to it.
> 
> DEVCONF1 is part of the CONTROL block, which is a core part of OMAP.
> DISPC is part of DSS block, which is not considered to be part of OMAP
> core. The idea here is that, in theory, the same DSS could be used in
> some other SoC, which has different methods to manage the things
> DEVCONF1 does.
> 
> So the DSS driver can just touch the DSS registers. Changing DEVCONF1
> must happen indirectly in a platform independent way.

Ah, I see.

> I think the rest of the comments in your email were more about our
> different understanding of what connector here means and of the display
> driver model in general. If there's something unclear, and I didn't
> answer to it, just ask again.

I think I have now understood your way of thinking. And the TFP410 example
is very similar. So an inverting analog amplifier is similar to an digital "encoder"
driver...

> 
>> Hm. Indeed diffiicult for an apparently simple problem and I hadn't expected
>> that it gets so a long discussion :) But really good solutions need that before.
> 
> Well, video is complex. People always think video is simple =). And the
> case here is even of the simpler ones. Just think about DSI bus with
> encoders with multiple inputs or outputs, and the complexity goes
> through the roof...

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.

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

What I don't know is how such a driver should be integrated into the pipeline
and by which means it gets notified that the "tv" display is enabled/disabled/suspended/resumed.
Or does it simply receive its individual enable/disable calls?

And is this similar to configuring and running the TFP410 driver? Then, we could
try to take parts of that driver.

And if I understand the TFP410 -> DVI example correctly such drivers are chained
and share the same video timings (even if they are not relevant for a specific stage)?

BR,
Nikolaus

Re: DSS display-new custom enable/disable hooks

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 9285 bytes --]

On 25/09/13 19:11, Dr. H. Nikolaus Schaller wrote:

> Hm. I a not sure if this model is complete if it ends at the physical connector.
> But that would be a different topic.

Yes, we currently model the components that exist on the board. I've
toyed with the idea of hotplugging a monitor "entity" after the
connector. There's nothing in the model that would strictly prevent
that, but hotplug in general would require more work.

> Well, I understand it that way:
> 
> RAM -> DISPC -> Video Encoder -> DAC-Stage (the DAC and an output amplifier within the OMAP chip) -> OMAP pins -> external amplifier (OPA) -> physical Connector -> cable -> TVset connector -> some more processing -> panel/screen -> Consumer
> 
> or simplified to the most important parts:
> 
> DISPC -> Video Encoder -> DAC-Stage -> external amplifier -> physical Connector

Well, this brings up the question, how small parts is it necessary to
split the pipeline.

For example, DISPC consists of multiple stages. In theory, we could
model all those stages with individual SW entities. In practice, that
doesn't really give us anything, they are always one whole entity, DISPC.

I think the same applies for VENC. There's no need to separate DAC from
VENC, they are always one whole entity (as far as I see).

Now, you could argue that DISPC and VENC (and the other DSS components)
also form one whole entity, the DSS. But here the difference is that we
already have different versions of DSS, that have different components.
Some don't have VENC, etc. But in all the different DSS versions, VENC
and DAC go together.

> the external amplifier is something specific to our board so that we have to insert it into the pipeline.
> 
> As you said above, if it would not have any controls we wouldn't have to care about it. But it needs to be enabled/disabled.

Note also that even if there weren't any controls (like gpios), the
components usually require power. On one board the power could come from
VBAT or some other "always on" source, but on some other, the regulator
needs to be turned on. So even if there are no controls, there could be
need for a driver.

That said, it feels a bit silly to have a driver, whose only function
would be to turn on one regulator...

> The other controls are:
> 
> "Bypass" means to bypass something in the DAC-Stage
> "AC" means to modify the DAC-Stage output level.
> "Invert" is probably a property of the VENC or could also be part of the DAC stage (I don't know).
> 
> BTW: I have seen a CAUTION block that describes in the last section how some registers
> must be set to avoid current leakage. That should be done (if not yet) in the suspend
> code.

Yes, I don't think we manage VENC very well at the moment.

>> VENC outputs a video signal, and obviously any register changes required
>> which affect the signal need to be done directly or indirectly by the
>> VENC driver.
>>
>> If OPA requires an inverted signal, it's between VENC and OPA to handle
>> that. Connector is not involved.
> 
> So we are not really missing an OPA362 driver but the DAC Stage driver within the DM3730 SoC.

As I said, I think we can consider DAC as part of VENC.

And I think we are really missing OPA362 driver. OPA requires someone to
control the enable GPIO (and maybe the regulators), and OPA driver is
the only logical place for those.

> And a mechanism that enabling/disabling the "tv" display automatically enables/disables
> all those stages + including an external OPA362.
> 
> This raises some questions: So how can such a pipeline be individually set up
> by platform data? How does enable/disable work along the chain?

The pipeline is setup in the board file. Each component is given
component specific parameters, and the name of its source component.

Enable/disable works "backwards". Omapfb (or some other component) calls
enable on the last entity in the pipeline (connector in this case). The
connector driver in turn calls enable on its source entity, which would
be OPA. And so on.

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

> But then I would not call it opa362 driver but "external-video-amplifier"
> with an enable-gpio that can be defined in the platform data or -1 if it is not
> required. In the latter case the driver simply does nothing functional.

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

>> The question here is how to handle the above. Should OPA driver request
>> VENC driver to invert the signal? Or should VENC driver just be passed
>> parameters from the board code making it invert the signal?
> 
> Well, I think that it is the responsibility of those who have designed such a
> board and they have to configure the drivers for each pipeline stage in a
> consistent way (i.e. if one is inverted the other should be as well, unless
> they want to see an inverted signal).
> 
> Your idea of the OPA driver notifying the VENC driver would introduce an
> information flow that does not exist at all in hardware. I.e. there is no "invert me"-wire
> in either direction. And that inversion could even happen behind the connector...

Well, there doesn't need to be a hardware information flow that would
match the SW. In fact, if there was, there would be no need for the SW
to do it.

Consider this:

DPI output (i.e. parallel RGB) and a DPI panel, connected with 24
datalines. The panel can be controlled via i2c, and you can send
commands to it to function in 16 or 24 bit modes. Here I think it makes
sense that when the user, via some method, commands the panel to use 16
bit mode, the panel driver would send the command to the panel hardware
and would then tell the DPI output to use 16 datalines. There's no
"use-16-bit"-wire from the panel.

I know there are different schools of thought how (and by who) the above
should happen, but that's the model current used in omap display drivers.

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.

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

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.

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

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

> and by which means it gets notified that the "tv" display is enabled/disabled/suspended/resumed.
> Or does it simply receive its individual enable/disable calls?

OPA would receive enable from the Connector driver. And OPA would need
to call enable in the VENC driver.

> And is this similar to configuring and running the TFP410 driver? Then, we could
> try to take parts of that driver.

Yes, TFP410 can be used as an example.

> And if I understand the TFP410 -> DVI example correctly such drivers are chained
> and share the same video timings (even if they are not relevant for a specific stage)?

No, they don't (need to) share the video timings. The TFP410 example
does share, because there are no real buffers or such in the pipeline
which would allow to change the timings. But in some cases there are
line buffers, and there, for example, the horizontal blanking intervals
may be changed. And sometimes there are full frame buffers, in which
case the timings can change totally.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: DSS display-new custom enable/disable hooks

[Dr. H. Nikolaus Schaller]

Hi Tomi,

Am 26.09.2013 um 10:20 schrieb Tomi Valkeinen:

> On 25/09/13 19:11, Dr. H. Nikolaus Schaller wrote:
> 
>> Well, I understand it that way:
>> 
>> RAM -> DISPC -> Video Encoder -> DAC-Stage (the DAC and an output amplifier within the OMAP chip) -> OMAP pins -> external amplifier (OPA) -> physical Connector -> cable -> TVset connector -> some more processing -> panel/screen -> Consumer
>> 
>> or simplified to the most important parts:
>> 
>> DISPC -> Video Encoder -> DAC-Stage -> external amplifier -> physical Connector
> 
> Well, this brings up the question, how small parts is it necessary to
> split the pipeline.
> 
> For example, DISPC consists of multiple stages. In theory, we could
> model all those stages with individual SW entities. In practice, that
> doesn't really give us anything, they are always one whole entity, DISPC.
> 
> I think the same applies for VENC. There's no need to separate DAC from
> VENC, they are always one whole entity (as far as I see).

I agree.

> Now, you could argue that DISPC and VENC (and the other DSS components)
> also form one whole entity, the DSS. But here the difference is that we
> already have different versions of DSS, that have different components.
> Some don't have VENC, etc. But in all the different DSS versions, VENC
> and DAC go together.

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.

> 
>> the external amplifier is something specific to our board so that we have to insert it into the pipeline.
>> 
>> As you said above, if it would not have any controls we wouldn't have to care about it. But it needs to be enabled/disabled.
> 
> Note also that even if there weren't any controls (like gpios), the
> components usually require power. On one board the power could come from
> VBAT or some other "always on" source, but on some other, the regulator
> needs to be turned on. So even if there are no controls, there could be
> need for a driver.
> 
> That said, it feels a bit silly to have a driver, whose only function
> would be to turn on one regulator...

Yes, essentially it is - not through a regulator but an enable-GPIO.

> 
>> The other controls are:
>> 
>> "Bypass" means to bypass something in the DAC-Stage
>> "AC" means to modify the DAC-Stage output level.
>> "Invert" is probably a property of the VENC or could also be part of the DAC stage (I don't know).
>> 
>> BTW: I have seen a CAUTION block that describes in the last section how some registers
>> must be set to avoid current leakage. That should be done (if not yet) in the suspend
>> code.
> 
> Yes, I don't think we manage VENC very well at the moment.
> 
>>> VENC outputs a video signal, and obviously any register changes required
>>> which affect the signal need to be done directly or indirectly by the
>>> VENC driver.
>>> 
>>> If OPA requires an inverted signal, it's between VENC and OPA to handle
>>> that. Connector is not involved.
>> 
>> So we are not really missing an OPA362 driver but the DAC Stage driver within the DM3730 SoC.
> 
> As I said, I think we can consider DAC as part of VENC.

Ok.

> And I think we are really missing OPA362 driver. OPA requires someone to
> control the enable GPIO (and maybe the regulators), and OPA driver is
> the only logical place for those.
> 
>> And a mechanism that enabling/disabling the "tv" display automatically enables/disables
>> all those stages + including an external OPA362.
>> 
>> This raises some questions: So how can such a pipeline be individually set up
>> by platform data? How does enable/disable work along the chain?
> 
> The pipeline is setup in the board file. Each component is given
> component specific parameters, and the name of its source component.

Ok, I think I did understand it by looking into the DVI encoder and connector
setup in the beagle board boardfile.

> Enable/disable works "backwards". Omapfb (or some other component) calls
> enable on the last entity in the pipeline (connector in this case). The
> connector driver in turn calls enable on its source entity, which would
> be OPA. And so on.

This appears to fit exactly.

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

Although one could argue that one is just controlling the GPIO and the other
one is controlling some regulator...

> 
>> But then I would not call it opa362 driver but "external-video-amplifier"
>> with an enable-gpio that can be defined in the platform data or -1 if it is not
>> required. In the latter case the driver simply does nothing functional.
> 
> 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.

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

> 
>>> The question here is how to handle the above. Should OPA driver request
>>> VENC driver to invert the signal? Or should VENC driver just be passed
>>> parameters from the board code making it invert the signal?
>> 
>> Well, I think that it is the responsibility of those who have designed such a
>> board and they have to configure the drivers for each pipeline stage in a
>> consistent way (i.e. if one is inverted the other should be as well, unless
>> they want to see an inverted signal).
>> 
>> Your idea of the OPA driver notifying the VENC driver would introduce an
>> information flow that does not exist at all in hardware. I.e. there is no "invert me"-wire
>> in either direction. And that inversion could even happen behind the connector...
> 
> Well, there doesn't need to be a hardware information flow that would
> match the SW. In fact, if there was, there would be no need for the SW
> to do it.
> 
> Consider this:
> 
> DPI output (i.e. parallel RGB) and a DPI panel, connected with 24
> datalines. The panel can be controlled via i2c, and you can send
> commands to it to function in 16 or 24 bit modes. Here I think it makes
> sense that when the user, via some method, commands the panel to use 16
> bit mode, the panel driver would send the command to the panel hardware
> and would then tell the DPI output to use 16 datalines. There's no
> "use-16-bit"-wire from the panel.
> 
> I know there are different schools of thought how (and by who) the above
> should happen, but that's the model current used in omap display drivers.
> 
> 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.

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

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.

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

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

> 
>> 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      = &gta04_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      = &gta04_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      = &gta04_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 */
};

static struct platform_device gta04_tv_connector_device = {
	.name                   = "connector-analog-tv",
	.id                     = 0,
	.dev.platform_data      = &gta04_tv_pdata,
};

static struct omap_dss_board_info gta04_dss_data = {
	.default_display_name = "lcd",
};

static struct regulator_consumer_supply gta04_vdvi_supplies[] = {
	REGULATOR_SUPPLY("vdds_sdi", "omapdss"),
	REGULATOR_SUPPLY("vdds_dsi", "omapdss_dpi.0"),
	REGULATOR_SUPPLY("vdds_dsi", "omapdss_dsi.0"),
};


and omap-panel-data.h will be augmented by:

/**
 * amplifier_opa362_platform_data platform data
 * @name: name for this display entity
 * @source: name of the display entity used as a video source
 * @enable_gpio: gpio number for enable pin (or -1 if not available - but then you don't need this driver)
 */
struct amplifier_opa362_platform_data {
	const char *name;
	const char *source;
	int enable_gpio;
};


> 
>> and by which means it gets notified that the "tv" display is enabled/disabled/suspended/resumed.
>> Or does it simply receive its individual enable/disable calls?
> 
> OPA would receive enable from the Connector driver. And OPA would need
> to call enable in the VENC driver.
> 
>> And is this similar to configuring and running the TFP410 driver? Then, we could
>> try to take parts of that driver.
> 
> Yes, TFP410 can be used as an example.

Ok, the code looks quite straightforward (as soon as we did understand the architecture).

> 
>> And if I understand the TFP410 -> DVI example correctly such drivers are chained
>> and share the same video timings (even if they are not relevant for a specific stage)?
> 
> No, they don't (need to) share the video timings. The TFP410 example
> does share, because there are no real buffers or such in the pipeline
> which would allow to change the timings. But in some cases there are
> line buffers, and there, for example, the horizontal blanking intervals
> may be changed. And sometimes there are full frame buffers, in which
> case the timings can change totally.

I see. So the opa362 driver can also just pass it along and ignore it.

We will submit real patches as soon as we have plumbed those things together
so that they compile well.

-- hns

Re: DSS display-new custom enable/disable hooks

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 9986 bytes --]

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      = &gta04_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      = &gta04_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      = &gta04_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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: DSS display-new custom enable/disable hooks

[Dr. H. Nikolaus Schaller]

Am 26.09.2013 um 13:49 schrieb Tomi Valkeinen:

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

Yes, this is how I would have argued as well.

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

I meant that there could be a differece between enable/disable and power-on/off
(e.g. retaining register values vs. loosing everything). But I can't imagine that for
a simple video amplifier.

> 
>>> 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 =).

Yes, and therefore I think we should focus on the 362 first like the 410.

> 
>>> 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 =).

Well, let's start with amplifier-opa362 and see if someone else ever needs
a different one. And "amplifier-generic" would be a quite strange name...

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

Yes, that is how I also would prefer it.

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

Yes.

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

Yes, I think we don't have to follow up on this idea.

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

Well, there could be systems where an OPA363 is not connected to an OMAP3
but some other video source. And then the source would not have the bypass and
acen flags.

This is IMHO another argument for having all this in the VENC platform data.

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

Well, we can't boot w/o the board file yet for other reasons, i.e. a DT only
approach isn't our target yet.

> 
>>>> 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      = &gta04_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      = &gta04_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      = &gta04_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.

I was also thinking about this topic. Well, the connector type is correctly
a property of the connector... But it logically controls some registers
in the DAC Output stage. So I had thought that the opa362 driver will
hand it over to the VENC and could check if it is really a composite
video (since AFAIK the OPA can't be used in a S-Video sytem).
But that again would be another chain of information flow from the
connector to the VENC making their APIs dependent.

BR,
Nikolaus Schaller

Re: DSS display-new custom enable/disable hooks

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2618 bytes --]

On 26/09/13 15:37, Dr. H. Nikolaus Schaller wrote:

>>> 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...
> 
> I meant that there could be a differece between enable/disable and power-on/off
> (e.g. retaining register values vs. loosing everything). But I can't imagine that for
> a simple video amplifier.

Ah, I see.

I'd still say there's just enable/disable from outside point of view.
It's the driver's job to make things work so that configuration values
are retained. If the hardware component is turned off so that it loses
register values, the driver should first store the configuration into
memory.

So internally there may be different power levels, maybe some kind of
timers to switch the component totally off if it's been disabled for
some time, or such. Externally, I don't think those need to be visible.

> Well, we can't boot w/o the board file yet for other reasons, i.e. a DT only
> approach isn't our target yet.

Sure, I'm fine with board file only version. My point was just that the
solution should not be such that it's impossible to support with DT. For
example, callbacks to board files are not possible with DT, so a
solution that relies on callbacks to board files will not be accepted.
And similarly board init calls are not supported with DT, so not acceptable.

>> 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.
> 
> I was also thinking about this topic. Well, the connector type is correctly
> a property of the connector... But it logically controls some registers
> in the DAC Output stage. So I had thought that the opa362 driver will
> hand it over to the VENC and could check if it is really a composite
> video (since AFAIK the OPA can't be used in a S-Video sytem).
> But that again would be another chain of information flow from the
> connector to the VENC making their APIs dependent.

Right. I'm a bit leaning towards having it in the VENC platform data,
and removing it from the connector.

Well, it's not directly related to the issue at hand. You may just pass
the connector type through OPA, and we can look at changing the
connector type later. But if you want to have a try with the connector
type at the same time, please go ahead.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]