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

[Nishanth Menon]

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 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.
>
>   
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 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
>   
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.
The recomendation here is to move from #defines to struct based register 
usage. I am ok with the rest(except for need to split).


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

> 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.
b) move away from #defines to struct based reg access.
> Regards,
> Khasim
>
>   

Regards,
Nishanth Menon