[U-Boot] [beagleboard] TI:OMAP: [PATCH 4/4] Minimal Display driver for OMAP3

[Khasim Syed Mohammed <khasim@beagleboard.org>]

On Sat, Jan 9, 2010 at 8:18 PM, Nishanth Menon <menon.nishanth@gmail.com> wrote:
> Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM:
>>
>> On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon <menon.nishanth@gmail.com>
>> wrote:
>>
>>>
>>> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed
>>> <khasim@beagleboard.org> wrote:
>>>
>>>>
>>>> From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001
>>>> From: Syed Mohammed Khasim <khasim@ti.com>
>>>> Date: Fri, 8 Jan 2010 21:01:44 +0530
>>>> Subject: [PATCH] Minimal Display driver for OMAP3
>>>>
>>>> Supports dynamic configuration of Panel and Video Encoder
>>>> Supports Background color on DVID
>>>> Supports Color bar on S-Video
>>>>
>>>
>>> We are getting there.. thanks a bunch. if you can split this series
>>> into two sets:
>>> a) introducing DSS layer
>>> b) introduce beagle support for the same
>>> it will be better.
>>>
>>
>> Can you complete your review review comments here, I will take up this
>> when in-corporate all review comments together.
>>
>>
>
> if you can give it a week before posting again, you could probably collate
> comments from various quarters. I just am a part time reviewer ;)..
>
>
>> Kindly remember these patches will be tested on B4, C2, C3, C4, EVM
>> before releasing.
>>
>
> great. good to know.
>>
>>
>>>
>>> but NAK to this patch.
>>>
>>>
>>>>
>>>> Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
>>>> ---
>>>> ?board/ti/beagle/beagle.c ? ? ? ? | ? 13 +++
>>>> ?board/ti/beagle/beagle.h ? ? ? ? | ? 73 ++++++++++++++
>>>> ?drivers/video/Makefile ? ? ? ? ? | ? ?1 +
>>>> ?drivers/video/omap3_dss.c ? ? ? ?| ?128 +++++++++++++++++++++++++
>>>> ?include/asm-arm/arch-omap3/dss.h | ?193
>>>> ++++++++++++++++++++++++++++++++++++++
>>>> ?include/configs/omap3_beagle.h ? | ? ?1 +
>>>> ?6 files changed, 409 insertions(+), 0 deletions(-)
>>>> ?create mode 100644 drivers/video/omap3_dss.c
>>>> ?create mode 100644 include/asm-arm/arch-omap3/dss.h
>>>>
>>>>
>>>
>>> [...]
>>>
>>>>
>>>> diff --git a/include/asm-arm/arch-omap3/dss.h
>>>> b/include/asm-arm/arch-omap3/dss.h
>>>> new file mode 100644
>>>> index 0000000..08c7d8d
>>>> --- /dev/null
>>>> +++ b/include/asm-arm/arch-omap3/dss.h
>>>> @@ -0,0 +1,193 @@
>>>> +/*
>>>> + * (C) Copyright 2010
>>>> + * Texas Instruments, <www.ti.com>
>>>> + * Syed Mohammed Khasim <khasim@ti.com>
>>>> + *
>>>> + * Referred to Linux DSS driver files for OMAP3
>>>> + *
>>>> + * See file CREDITS for list of people who contributed to this
>>>> + * project.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License as
>>>> + * published by the Free Software Foundation's version 2 of
>>>> + * the License.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program; if not, write to the Free Software
>>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>>> + * MA 02111-1307 USA
>>>> + */
>>>> +
>>>> +#ifndef DSS_H
>>>> +#define DSS_H
>>>> +
>>>> +/* VENC Register address */
>>>> +#define VENC_REV_ID ? ? ? ? ? ? ? ? ? ? ? ? ? ?0x48050C00
>>>>
>>>
>>> NAK. why do you need this if you have a struct?
>>>
>>
>> You need to read patch more carefully before giving NAK.
>>
>> Structure is used to give multiple instance of Panels or different
>> VENC settings like NTSC or PAL
>>
>
> Thanks for clarifying
> That is the crux of the problem. if you use struct to describe the venc ->
> then you'd be staying with how the rest of u-boot accesses are:
> using struct dereference. if you look at nand.c as I pointed out, Dirk had
> done a great job of converting register offsets
> to struct and referencing off it. This was required to get the driver
> mainlined.
As said before, I agree. NAND and GPIO needs struct based development
as it has multiple instances. It will look odd to implement in
#defines.

> The recomendation here is to move from #defines to struct based register
> usage. I am ok with the rest(except for need to split).
Split is done, posted yesterday.

Struct based register needs more comments, not that I am lazy to
implement that. I need to know the reason for doing the same when no
multiple instances are used.

>
>> You can add a new panel or a new tv standard with these structures
>> easily. Structures are not used for register accesses.
>>
>>
>>>
>>> here is what I think:
>>> venc_config {
>>> }
>>>
>>> if it is organized as the register definitions,
>>>
>>> configure_venc(struct venc_config *values)
>>> struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC;
>>> writel(values->regx, &d->regx);
>>>
>>> refer: drivers/mtd/nand/omap_gpmc.c
>>>
>>>
>>
>> GPIO, GPMC and other controllers have multiple instances in OMAP, it
>> makes sense to organize such register set in structure mode. I did
>> start with that but found no use for DSS as it is just one instance.
>> Structures don't give any value here.
>>
>
> there were other reasons mentioned when nand got split -> one of them had to
> do with the compiler or something. Dirk might remember - unfortunately, this
> was more than a year back.. if I recollect right..
Will try doing a google. May be some one can point me to that
decision. It would help developing drivers which have single instance
of controller being used.

>>
>> More over I am introducing minimal DSS driver with minimal register
>> set. It doesn't help any to give structure based register access for
>> single instance drivers.
>>
>
> moving to struct based is easy and done once and improves your chance of
> your driver getting upstreamed :).
DSS in OMAP3 has following register domains.

DSI Protocol Engine           0x4804 FC00            512 bytes
DSI_PHY                           0x4804 FE00             64 bytes
DSI PLL Controller              0x4804 FF00             32 bytes
Display Subsystem            0x4805 0000            512 bytes
Display Controller               0x4805 0400             1K byte
Display Controller VID1       0x4805 0400             1K byte
Display Controller VID2       0x4805 0400             1K byte
RFBI                                 0x4805 0800            256 bytes
Video Encode                    0x4805 0C00           256 bytes

I am not sure why one would ask me to give struct definitions for
these 500 (approx) registers when only 50 of these are required to
implement background and color bar. As I am saying all the way, DSS is
not multiple instance module like GPMC (NAND) and GPIO it is just one
module.

Will wait for others in list to comment on these.

>> Give more reasons for NAK.
>>
>
> This is the only reason I have other than the need to split this patch up
> into logical chunks. my NAK still stands unfortunately for the following
> generic reasons:
> a) split the patch as mentioned above.
Done.
> b) move away from #defines to struct based reg access.
Reasons given above. Will wait for others in community to comment on
the above implementation.

Many thanks for the review.

Regards,
Khasim