From: Kukjin Kim <kgene.kim@samsung.com>
To: 'Jassi Brar' <jassisinghbrar@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org,
'Jassi Brar' <jassi.brar@samsung.com>,
ben-linux@fluff.org
Subject: RE: [PATCH 07/13] ARM: S5P64X0: Update Audio support
Date: Thu, 02 Sep 2010 16:30:37 +0900 [thread overview]
Message-ID: <004601cb4a70$c0d72970$42857c50$%kim@samsung.com> (raw)
In-Reply-To: <AANLkTin=fmeRZt=SSrxk9YzrhZs1nLxc9M30PYO3ONEb@mail.gmail.com>
Jassi Brar wrote:
>
Hi Jassi :-)
Thanks for your review.
> On Wed, Sep 1, 2010 at 4:09 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > This patch updates Audio and SPI for S5P6440 and S5P6450 SoCs.
> >
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > Cc: Jassi Brar <jassi.brar@samsung.com>
> > ---
> > 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 <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
next prev parent reply other threads:[~2010-09-02 7:30 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-01 7:09 [PATCH 00/13] ARM: S5P64X0: Add new S5P6450 Support Kukjin Kim
2010-09-01 7:09 ` [PATCH 01/13] ARM: S5P: Moves initial map for merging S5P64X0 Kukjin Kim
2010-09-01 9:42 ` Kyungmin Park
2010-09-01 7:09 ` [PATCH 02/13] ARM: S5P64X0: Update Kconfig and Makefile Kukjin Kim
2010-09-01 9:47 ` Kyungmin Park
2010-09-02 8:25 ` Kukjin Kim
2010-09-01 7:09 ` [PATCH 03/13] ARM: S5P64X0: Add S5P64X0(S5P6440 and S5P6450) initialization support Kukjin Kim
2010-09-01 7:09 ` [PATCH 04/13] ARM: S5P64X0: Update Clock for S5P6440 and S5P6450 Kukjin Kim
2010-09-01 7:09 ` [PATCH 05/13] ARM: S5P64X0: Update IRQ support Kukjin Kim
2010-09-01 7:09 ` [PATCH 06/13] ARM: S5P64X0: Update Timer support Kukjin Kim
2010-09-01 7:09 ` [PATCH 07/13] ARM: S5P64X0: Update Audio support Kukjin Kim
2010-09-02 1:44 ` Jassi Brar
2010-09-02 7:30 ` Kukjin Kim [this message]
2010-09-01 7:09 ` [PATCH 08/13] ARM: S5P64X0: Move DMA support for S5P64X0 Kukjin Kim
2010-09-02 1:08 ` Jassi Brar
2010-09-02 7:32 ` Kukjin Kim
2010-09-01 7:09 ` [PATCH 09/13] ARM: S5P64X0: Add S5P6450 I2C support Kukjin Kim
2010-09-01 7:09 ` [PATCH 10/13] ARM: S5P64X0: Move GPIO support files for merge S5P64X0 Kukjin Kim
2010-09-01 7:09 ` [PATCH 11/13] ARM: S5P64X0: Move SMDK6440 board file and Add SMDK6450 board file Kukjin Kim
2010-09-01 10:00 ` Kyungmin Park
2010-09-02 8:02 ` Kukjin Kim
2010-09-01 7:09 ` [PATCH 12/13] ARM: S5P64X0: Add UART serial support for S5P6450 Kukjin Kim
2010-09-02 1:43 ` Kyungmin Park
2010-09-01 7:09 ` [PATCH 13/13] ARM: s5p64x0_defconfig: Update for support S5P6440 and S5P6450 Kukjin Kim
2010-09-01 11:45 ` [PATCH 00/13] ARM: S5P64X0: Add new S5P6450 Support Ben Dooks
2010-09-02 10:21 ` Kukjin Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='004601cb4a70$c0d72970$42857c50$%kim@samsung.com' \
--to=kgene.kim@samsung.com \
--cc=ben-linux@fluff.org \
--cc=jassi.brar@samsung.com \
--cc=jassisinghbrar@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-samsung-soc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox