From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nishanth Menon Date: Sat, 09 Jan 2010 08:48:34 -0600 Subject: [U-Boot] [beagleboard] TI:OMAP: [PATCH 4/4] Minimal Display driver for OMAP3 In-Reply-To: References: <782515bb1001081201j56cfa78dyd136e0daab3adcb9@mail.gmail.com> Message-ID: <4B489742.8040104@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM: > On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon wrote: > >> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed >> wrote: >> >>> From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 >>> From: Syed Mohammed Khasim >>> 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 >>> --- >>> 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, >>> + * Syed Mohammed Khasim >>> + * >>> + * 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