From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kukjin Kim Subject: RE: [PATCH 07/13] ARM: S5P64X0: Update Audio support Date: Thu, 02 Sep 2010 16:30:37 +0900 Message-ID: <004601cb4a70$c0d72970$42857c50$%kim@samsung.com> References: <1283324960-21629-1-git-send-email-kgene.kim@samsung.com> <1283324960-21629-8-git-send-email-kgene.kim@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7BIT Return-path: Received: from mailout1.samsung.com ([203.254.224.24]:55838 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751483Ab0IBHa3 (ORCPT ); Thu, 2 Sep 2010 03:30:29 -0400 Received: from epmmp2 (mailout1.samsung.com [203.254.224.24]) by mailout1.samsung.com (Sun Java(tm) System Messaging Server 7u3-15.01 64bit (built Feb 12 2010)) with ESMTP id <0L8300KFYZIRTG70@mailout1.samsung.com> for linux-samsung-soc@vger.kernel.org; Thu, 02 Sep 2010 16:30:27 +0900 (KST) Received: from kgenekim ([12.23.103.96]) by mmp2.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTPA id <0L83002ZVZIRPD@mmp2.samsung.com> for linux-samsung-soc@vger.kernel.org; Thu, 02 Sep 2010 16:30:27 +0900 (KST) In-reply-to: Content-language: ko Sender: linux-samsung-soc-owner@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org To: 'Jassi Brar' Cc: linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, 'Jassi Brar' , ben-linux@fluff.org Jassi Brar wrote: > Hi Jassi :-) Thanks for your review. > On Wed, Sep 1, 2010 at 4:09 PM, Kukjin Kim wrote: > > This patch updates Audio and SPI for S5P6440 and S5P6450 SoCs. > > > > Signed-off-by: Kukjin Kim > > Cc: Jassi Brar > > --- > > arch/arm/mach-s5p6440/dev-audio.c | 127 ----------- > > arch/arm/mach-s5p6440/dev-spi.c | 176 --------------- > > arch/arm/mach-s5p6440/include/mach/spi-clocks.h | 17 -- > > arch/arm/mach-s5p64x0/dev-audio.c | 164 ++++++++++++++ > > arch/arm/mach-s5p64x0/dev-spi.c | 275 > +++++++++++++++++++++++ > > arch/arm/mach-s5p64x0/include/mach/spi-clocks.h | 20 ++ > > arch/arm/plat-samsung/include/plat/devs.h | 5 + > > 7 files changed, 464 insertions(+), 320 deletions(-) > > delete mode 100644 arch/arm/mach-s5p6440/dev-audio.c > > delete mode 100644 arch/arm/mach-s5p6440/dev-spi.c > > delete mode 100644 arch/arm/mach-s5p6440/include/mach/spi-clocks.h > > create mode 100644 arch/arm/mach-s5p64x0/dev-audio.c > > create mode 100644 arch/arm/mach-s5p64x0/dev-spi.c > > create mode 100644 arch/arm/mach-s5p64x0/include/mach/spi-clocks.h > > (snip) > > S5P6450 has 2 I2S_v2 and 1 I2S_v4, the first two are missing. > Also missing are 3 PCM device definitions. > Yeah, I know. But the remaining items will be added later. Maybe Mr. Youn will do it ;-) (snip) > > +struct platform_device s5p6440_device_iis = { > > + .name = "s3c64x0-iis-v4", > > The assigned name is wrong. It should be "s3c64xx-iis-v4" > You're right..will fix it. > > > + .id = -1, > > + .num_resources = ARRAY_SIZE(s5p64x0_iis0_resource), > > + .resource = s5p64x0_iis0_resource, > > + .dev = { > > + .platform_data = &s5p6440_i2s_pdata, > > + }, > > +}; > > + > > +struct platform_device s5p6450_device_iis0 = { > > + .name = "s3c64x0-iis-v4", > > The assigned name is wrong. It should be "s3c64xx-iis-v4" > ok.. > > + .id = -1, > > + .num_resources = ARRAY_SIZE(s5p64x0_iis0_resource), > > + .resource = s5p64x0_iis0_resource, > > + .dev = { > > + .platform_data = &s5p6450_i2s_pdata, > > + }, > > +}; > > + (snip) > > + > > +static char *s5p6440_spi_src_clks[] = { > > + [S5P64X0_SPI_SRCCLK_PCLK] = "pclk", > > + [S5P64X0_SPI_SRCCLK_SCLK] = "spi_epll", > > +}; > > + > > +static char *s5p6450_spi_src_clks[] = { > > + [S5P64X0_SPI_SRCCLK_PCLK] = "pclk", > > + [S5P64X0_SPI_SRCCLK_SCLK] = "sclk_spi", > > +}; > > Maybe we can drop one and call the other s5p64x0_spi_src_clks > The second clock is the same but only named differently. > As you know, we use the clock name as there is in data sheet. Anyway...will sort out. (snip) > > + > > +static struct s3c64x0_spi_info s5p6440_spi0_pdata = { > > + .cfg_gpio = s5p6440_spi_cfg_gpio, > > + .fifo_lvl_mask = 0x1ff, > > + .rx_lvl_offset = 15, > > +}; > > I think you forgot to compile check in hurry ? hahaha, I'm always remembering it :-) about all of s3c and s5p defconfigs. Actually, there is no build error...and kernel booting works well on the board. But I didn't audio test...just moved your original code into the new machine directory. ...And now, not selected S3C64XX_DEV_SPI...so not compiled dev-spi.c. Anyway, I will check all files ;-) Thanks. > There is no s3c64x0_spi_info, but only s3c64xx_spi_info > Yeah, you're right...will fix it. > > + > > +static struct s3c64x0_spi_info s5p6450_spi0_pdata = { > > + .cfg_gpio = s5p6450_spi_cfg_gpio, > > + .fifo_lvl_mask = 0x1ff, > > + .rx_lvl_offset = 15, > > +}; > > + > > +static u64 spi_dmamask = DMA_BIT_MASK(32); > > + > > +struct platform_device s5p6440_device_spi0 = { > > + .name = "s3c64x0-spi", > > %s/64xx/64x0 ? :) Maybe...yes? ;-) > The device name should be kept same "s3c64xx-spi" > It's my mistake...will fix it. > > + .id = 0, > > + .num_resources = ARRAY_SIZE(s5p64x0_spi0_resource), > > + .resource = s5p64x0_spi0_resource, > > + .dev = { > > + .dma_mask = &spi_dmamask, > > + .coherent_dma_mask = DMA_BIT_MASK(32), > > + .platform_data = &s5p6440_spi0_pdata, > > + }, > > +}; > > + > > +struct platform_device s5p6450_device_spi0 = { > > + .name = "s3c64x0-spi", > > same here > ok... > > + .id = 0, > > + .num_resources = ARRAY_SIZE(s5p64x0_spi0_resource), > > + .resource = s5p64x0_spi0_resource, > > + .dev = { > > + .dma_mask = &spi_dmamask, > > + .coherent_dma_mask = DMA_BIT_MASK(32), > > + .platform_data = &s5p6450_spi0_pdata, > > + }, > > +}; (snip) > > + > > +struct platform_device s5p6440_device_spi1 = { > > + .name = "s3c64x0-spi", > > same here... > ok... > > + .id = 1, > > + .num_resources = ARRAY_SIZE(s5p64x0_spi1_resource), > > + .resource = s5p64x0_spi1_resource, > > + .dev = { > > + .dma_mask = &spi_dmamask, > > + .coherent_dma_mask = DMA_BIT_MASK(32), > > + .platform_data = &s5p6440_spi1_pdata, > > + }, > > +}; > > + > > +struct platform_device s5p6450_device_spi1 = { > > + .name = "s3c64x0-spi", > > same here .... > ok... > > + .id = 1, > > + .num_resources = ARRAY_SIZE(s5p64x0_spi1_resource), > > + .resource = s5p64x0_spi1_resource, > > + .dev = { > > + .dma_mask = &spi_dmamask, > > + .coherent_dma_mask = DMA_BIT_MASK(32), > > + .platform_data = &s5p6450_spi1_pdata, > > + }, > > +}; > > + > > +void __init s5p6440_spi_set_info(int cntrlr, int src_clk_nr, int num_cs) > > maybe we could call it s5p64x0_spi_set_info, and remove other other one ? > mm...will sort out. > > +{ > > + struct s3c64x0_spi_info *pd; > > + > > + /* Reject invalid configuration */ > > + if (!num_cs || src_clk_nr < 0 > > + || src_clk_nr > S5P6440_SPI_SRCCLK_SCLK) { > you need to change all references to S5P6440_SPI_SRCCLK_..... as well. > Yeah, ok. > > + printk(KERN_ERR "%s: Invalid SPI configuration\n", __func__); > > + return; > > + } > > + (snip) > > just to say the least. Thanks. Best regards, Kgene. -- Kukjin Kim , Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.