[PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Dr. H. Nikolaus Schaller]

Am 11.11.2013 um 15:13 schrieb Tomi Valkeinen:

> On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
>> Hi Tomi,
>> 
>> Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:
>> 
>>> Hi,
>>> 
>>> On 2013-11-05 09:24, Belisko Marek wrote:
>>>> Hi,
>>>> 
>>>> ping.
>>>> 
>>>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek at goldelico.com> wrote:
>>>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>>>> is used on gta04 board and set bypass + acbias.
>>>> Is there a chance to get this series to 3.13? Thanks.
>>> 
>>> Sorry, I haven't had time to do much reviewing.
>>> 
>>> The code in omap3-tvout.c should be included in the display.c file,
>>> which already contains some things like muxing.
>> 
>> Ok, that might be better - but we were not sure where to place code to
>> modify the DEVCONF1 registers. It is not directly DSS related but a special
>> control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c
> 
> The display.c file is not strictly DSS stuff, but DSS related "glue" for
> the SoC. For example, the muxing of the DSI pads is also done on the
> CONTROL module, and it's also in display.c.
> 
> The file is getting a bit large, so I'm not against splitting it. But I
> don't think there's point to add omap3-tvout.c file, which most likely
> will ever contain only that one function.
> 
>>> Also, func(bool, bool)
>>> style functions are rather confusing to read. Maybe an enum would be
>>> better, so you'd instead have something like:
>>> 
>>> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)
>> 
>> We have no special preference on that.
> 
> It's just about readability. Which one tells you better what the code does:
> 
> func(true, true);
> 
> or
> 
> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN);
> 
>>> But the main issue is: while this series probably works well, I really
>>> don't like it that the OPA driver needs to pass bypass and acbias. It
>>> shouldn't know anything about such things. I'm just not certain how to
>>> implement that with the current omapdss driver.
>> 
>> Well, it must know bypass and acbias because it requires the OMAP
>> CPU to be configured accordingly. I.e. it is the one who pushes the
>> button. Or we get a problem that people might misconfigure it.
> 
> While the omapdss display drivers are currently OMAP specific, I want to
> (or at least try to) keep them platform independent. Afaik, acbias and
> bypass are OMAP VENC specific things, not something that every SoC with
> an analog TV-out have. Thus, the OPA driver should not know about them,
> and it should be something that only the DSS glue code in mach-omap2/
> and the venc driver know about.

What about calling it prepare_venc_for_external_amplifier (or similar). Then,
venc.c can hide that really has to be done and call whatever SoC API is
needed to program DEFCONF1?

> 
>> I would see it similar as a driver must be able to control power. Here
>> it must be able to control bypass and acbias in a special way that it works.
> 
> The difference is that on all possible SoCs where you could add OPA
> chip, you'll need to manage the power for OPA, and it's done in a common
> way. Whereas the bypass and acbias are OMAP specific things.
> 
>>> I'll try to find time to think about this more, but I don't think I can
>>> merge this for 3.13.
>> 
>> Please take your time.
>> 
>> And please also consider the approach to merge it into 3.13 as it is
>> and we can then fix it in the weeks while release candidates are pushed.
> 
> If it was purely omapdss code, I could possibly take it. But it contains
> arch/arm code also, and I don't want to cause needless changes on code
> that I do not maintain.
> 
> Tomi
> 
>