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

[Khasim Syed Mohammed]

On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon <menon.nishanth at gmail.com> wrote:
> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed
> <khasim at beagleboard.org> wrote:
>> From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001
>> From: Syed Mohammed Khasim <khasim at 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.

Kindly remember these patches will be tested on B4, C2, C3, C4, EVM
before releasing.

>
> but NAK to this patch.
>
>>
>> Signed-off-by: Syed Mohammed Khasim <khasim at 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 at 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

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.

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.

Give more reasons for NAK.

Regards,
Khasim