[U-Boot] Fwd: TI:OMAP: [PATCH 5/7] Add DSS driver for OMAP3

[Tom <Tom.Rix@windriver.com>]

Khasim Syed Mohammed wrote:
> On Sun, Jan 24, 2010 at 5:48 AM, Tom <Tom.Rix@windriver.com> wrote:
>> Khasim Syed Mohammed wrote:
>>> From cf8fa28973de7609d27146730d9e019b7c919b51 Mon Sep 17 00:00:00 2001
>>> From: Syed Mohammed Khasim <khasim@ti.com>
>>> Date: Tue, 12 Jan 2010 23:57:28 +0530
>>> Subject: [PATCH] Add DSS driver for OMAP3
>>>
>>> Supports dynamic panel configuration
>>> Supports dynamic tv standard selection
>>> Adds support for DSS register access through generic APIs
>>>
>>> Incorporated DSS register access using structures.
>>>
>>> Previous discussions are here
>>> http://www.mail-archive.com/u-boot at lists.denx.de/msg27150.html
>>>
>>> Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
>>> ---
>>>  drivers/video/Makefile           |    1 +
>>>  drivers/video/omap3_dss.c        |  130 ++++++++++++++++++++++++++++
>>>  include/asm-arm/arch-omap3/dss.h |  173
>>> ++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 304 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/video/omap3_dss.c
>>>  create mode 100644 include/asm-arm/arch-omap3/dss.h
>>>


<snip>

>>> +/*
>>> + * DSS Base Registers
>>> + */
>>> +#define OMAP3_DSS_BASE         0x48050040
>>> +#define OMAP3_DISPC_BASE       0x48050440
>>> +#define OMAP3_VENC_BASE                0x48050C00
>>> +
>>> +/* DSS Registers */
>>> +struct dss_regs {
>>> +       u32 control;                            /* 0x40 */
>> Add a comment to explain why base starts at 0x40
>> could also be handled with changing code
>> OMAP3_DSS_BASE 0x48050000
>> struct dss_regs {
>> u8 reserved_1[0x40];
>> ..
> may be,.. let me try.

This can just be fixed by a simple comment like
/* Note : 0x40 is offset from OMAP3_DSS_BASE */
This helps avoid people wondering why 0x40 and not 0x0

> 
>>> +       u32 sdi_control;                        /* 0x44 */
>>> +       u32 pll_control;                        /* 0x48 */
>>> +};
>>> +
>>> +/* DISPC Registers */
>>> +struct dispc_regs {
>> similar
> 
> ok
>>> +       u32 control;                            /* 0x40 */
>> Tom
> 
> Thanks for very detailed review. Mainly Licensing.

Yes.

> 
> Regards,
> Khasim

Tom