From mboxrd@z Thu Jan 1 00:00:00 1970 From: rajeev Subject: Re: [PATCH 1/2] sound: asoc: Adding support for SPEAr13XX ASoC driver Date: Fri, 18 Mar 2011 12:49:04 +0530 Message-ID: <4D830768.3040208@st.com> References: <1300361016-26242-1-git-send-email-rajeev-dlh.kumar@st.com> <1300361016-26242-2-git-send-email-rajeev-dlh.kumar@st.com> <20110317150539.GF31411@opensource.wolfsonmicro.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from eu1sys200aog118.obsmtp.com (eu1sys200aog118.obsmtp.com [207.126.144.145]) by alsa0.perex.cz (Postfix) with ESMTP id 4064F243BA for ; Fri, 18 Mar 2011 08:21:20 +0100 (CET) In-Reply-To: <20110317150539.GF31411@opensource.wolfsonmicro.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Mark Brown Cc: "tiwai@suse.de" , "alsa-devel@alsa-project.org" , "lrg@slimlogic.co.uk" List-Id: alsa-devel@alsa-project.org Hi Mark Thanks for your review comment. Please find my answer inline On 3/17/2011 8:35 PM, Mark Brown wrote: > On Thu, Mar 17, 2011 at 04:53:35PM +0530, Rajeev Kumar wrote: > >> This patch contains the following support as per the ASoC framework: >> 1. Platform Driver (I2S based). >> 2. Machine Driver. > > These should be split into separate patches... > OK >> The codec driver for STA529 is present in a separate patch. > > ...and this should come before the machine driver patch as the machine > driver depends on the CODEC. > OK >> -config SND_SOC_CACHE_LZO >> - bool "Support LZO compression for register caches" >> - select LZO_COMPRESS >> - select LZO_DECOMPRESS >> - ---help--- >> - Select this to enable LZO compression for register caches. >> - This will allow machine or CODEC drivers to compress register >> - caches in memory, reducing the memory consumption at the >> - expense of performance. If this is not present and is used >> - the system will fall back to uncompressed caches. >> - >> - Usually it is safe to disable this option, where cache >> - compression in used the rbtree option will typically perform >> - better. > > Hrm? > Oops.. That's a mistake, will be corrected in V2 >> @@ -50,12 +35,11 @@ source "sound/soc/jz4740/Kconfig" >> source "sound/soc/nuc900/Kconfig" >> source "sound/soc/omap/Kconfig" >> source "sound/soc/kirkwood/Kconfig" >> -source "sound/soc/mid-x86/Kconfig" >> source "sound/soc/pxa/Kconfig" >> -source "sound/soc/samsung/Kconfig" >> +#source "sound/soc/s3c24xx/Kconfig" >> source "sound/soc/s6000/Kconfig" >> source "sound/soc/sh/Kconfig" >> -source "sound/soc/tegra/Kconfig" >> +source "sound/soc/spear/Kconfig" >> source "sound/soc/txx9/Kconfig" >> > > Interesting too.... same as above > >> +config SND_SPEAR_SOC_EVM >> + tristate "SoC Audio support for Spear EVM" >> + depends on SND_SPEAR_SOC >> + select SND_SPEAR_SOC_I2S >> + select SND_SOC_STA529 >> + help >> + Say Y if you want to add support for SoC audio on ST SPEAR >> + platform > > The description here isn't terribly good, this is actually a driver for > the EVM machine. > OK >> @@ -0,0 +1,6 @@ >> +# SPEAR Platform Support >> +obj-$(CONFIG_SND_SPEAR_SOC) += spear13xx-pcm.o >> +obj-$(CONFIG_SND_SPEAR_SOC_I2S) += spear13xx-i2s.o >> + >> +# SPEAR Machine Support >> +obj-$(CONFIG_SND_SPEAR_SOC_EVM) += evb_sta529.o > > The modules should always be snd-soc-whatever. > OK >> + * Copyright (C) 2010 ST Microelectronics >> + * Rajeev Kumar > > Space between your name and e-mail. > OK >> + format = params_format(params); >> + rate = params_rate(params); >> + channel = params_channels(params); >> + freq = format * rate * channel * 8; >> + ref_clock = freq * 8; > > Use the utility functions in soc-util to do the format, rate and channel > number calculation. > OK >> + /* set the codec system clock for DAC */ >> + ret = snd_soc_dai_set_sysclk(codec_dai, 0 , ref_clock, >> + SND_SOC_CLOCK_IN); >> + if (ret < 0) >> + return ret; > > Should be "0, " not "0 , ". > OK >> + /*setting ref clock in 278 offset*/ >> + val = readl(PERIP2_CLK_ENB); >> + val |= 0x80; >> + writel(val, PERIP2_CLK_ENB); >> + >> + /*setting mode 0 in conf regiter: 32c offset*/ >> + val = readl(PERIP_CFG); >> + val |= 0x0; >> + writel(val, PERIP_CFG); > > These should be exposed by the relevant driver. > OK >> +static int __init spear_init(void) >> +{ >> + int ret; >> + >> + /* Create and register platform device */ >> + evb_snd_device = platform_device_alloc("soc-audio", 0); >> + if (!evb_snd_device) { >> + printk(KERN_ERR "platform_device_alloc fails\n"); >> + return -ENOMEM; >> + } > > Please register the machine using snd_soc_register_card() rather than > soc-audio. soc-audio is being phased out. > OK, will be corrected in V2 >> +static int >> +spear13xx_i2s_set_dai_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, >> + unsigned int freq, int dir) >> +{ >> + struct spear13xx_i2s_dev *dev = snd_soc_dai_get_drvdata(cpu_dai); >> + struct clk *sclk_clk, *src_clk; >> + int ret = -EINVAL; >> + >> + sclk_clk = clk_get_sys(NULL, "i2s_sclk_clk"); >> + if (IS_ERR(sclk_clk)) { >> + dev_err(dev->dev, "couldn't get i2s_sclk\n"); >> + return PTR_ERR(sclk_clk); >> + } >> + >> + src_clk = clk_get_sys(NULL, "i2s_src_clk"); >> + if (IS_ERR(src_clk)) { >> + ret = PTR_ERR(src_clk); >> + dev_err(dev->dev, "couldn't get i2s_src_sclk\n"); >> + goto put_sclk_clk; >> + } >> + >> + if (clk_set_parent(sclk_clk, src_clk)) >> + goto put_src_clk; >> + >> + ret = clk_enable(sclk_clk); >> + if (ret < 0) { >> + dev_err(dev->dev, "enable i2s_sclk fail\n"); >> + goto put_src_clk; >> + } >> + return 0; > > The clock requests should be being done as part of the probe, not here - > think what will happen when the driver is unloaded, the references will > never be released. Similarly, the clock should be being enabled and > disabled as part of the normal driver flow. > > Since the driver is just completely ignoring the clock rate that's > configured there probably shouldn't be a set_sysclk() function at all. > OK, This is basically platform specific part so I need to move it in plat code. and remove this function. >> +void >> +i2s_start_play(struct spear13xx_i2s_dev *dev, >> + struct snd_pcm_substream *substream) >> +{ > > Non-exported functions should be static. > Ok >> + /* for 2.0 audio*/ >> + if (dev->mode <= 2) { >> + i2s_write_reg(dev->i2s_base, CCR, 0x00); >> + if (!val) { >> + i2s_write_reg(dev->i2s_base, RCR0, 0x2); > > Some of these register writes are also done by the playback path but > there's no synchronisation between the playback and record paths. > OK I will take care of that. >> +void >> +i2s_stop(struct spear13xx_i2s_dev *dev, struct snd_pcm_substream *substream) >> +{ >> + i2s_write_reg(dev->i2s_base, IER, 0); >> + i2s_write_reg(dev->i2s_base, IMR0, 0x33); >> + i2s_write_reg(dev->i2s_base, IMR1, 0x33); >> + i2s_write_reg(dev->i2s_base, ITER, 0); >> + i2s_write_reg(dev->i2s_base, IRER, 0); >> + i2s_write_reg(dev->i2s_base, CER, 0); >> +} > > This appears to stop both playback and capture streams but the two > should be independant. > some flag should be there to avoid this. will be corrected in V2 >> +static irqreturn_t i2s_play_irq(int irq, void *_dev) >> +{ >> + struct spear13xx_i2s_dev *dev = (struct spear13xx_i2s_dev *)_dev; >> + u32 ch0, ch1; >> + >> + /* check for the tx data overrun condition */ >> + ch0 = i2s_read_reg(dev->i2s_base, ISR0) & 0x20; >> + ch1 = i2s_read_reg(dev->i2s_base, ISR1) & 0x20; >> + if (ch0 || ch1) { >> + >> + /* disable i2s block */ >> + i2s_write_reg(dev->i2s_base, IER, 0); >> + >> + /* disable tx block */ >> + i2s_write_reg(dev->i2s_base, ITER, 0); >> + >> + /* flush all the tx fifo */ >> + i2s_write_reg(dev->i2s_base, TXFFR, 1); >> + >> + /* clear tx data overrun interrupt: channel 0 */ >> + i2s_read_reg(dev->i2s_base, TOR0); >> + >> + /* clear tx data overrun interrupt: channel 1 */ >> + i2s_read_reg(dev->i2s_base, TOR1); >> + } > > If we're overrunning or underrunning I'd expect to see some sort of > error report happening even if it's just a log. > ok >> +static int >> +spear13xx_i2s_startup(struct snd_pcm_substream *substream, >> + struct snd_soc_dai *cpu_dai) >> +{ >> + struct spear13xx_i2s_dev *dev = snd_soc_dai_get_drvdata(cpu_dai); >> + u32 ret = 0; >> + >> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { >> + ret = request_irq(dev->play_irq, i2s_play_irq, 0, >> + "spear13xx-i2s", dev); >> + } else { >> + ret = request_irq(dev->capture_irq, i2s_capture_irq, >> + 0, "spear13xx-i2s", dev); >> + } > > The interrupts should be acquired on probe, constantly requesting and > releasing them is undesirable. > Ok, will be moved to probe function. >> + if (ret) { >> + dev_err(dev->dev, "irq registration failure\n"); >> + iounmap(dev->i2s_base); >> + clk_disable(dev->clk); >> + clk_put(dev->clk); >> + kfree(dev); >> + release_mem_region(dev->res->start, resource_size(dev->res)); >> + return ret; >> + } > > This error handling is not associated with anything the startup() > function has done... > This is related to request_irq and will removed from this place. >> +static int spear13xx_i2s_hw_params(struct snd_pcm_substream *substream, >> + struct snd_pcm_hw_params *params, >> + struct snd_soc_dai *dai) >> +{ >> + struct spear13xx_i2s_dev *dev = snd_soc_dai_get_drvdata(dai); >> + u32 channel; >> + >> + channel = params_channels(params); >> + >> + dev->mode = channel; > > Having channel here isn't doing a lot... I was actually planning to expand this routine in future. Should I remove it from the current version? > >> + /* mask i2s interrupt for channel 0 */ >> + i2s_write_reg(dev->i2s_base, IMR0, 0x33); >> + >> + /* mask i2s interrupt for channel 1 */ >> + i2s_write_reg(dev->i2s_base, IMR1, 0x33); >> + >> + i2s_stop(dev, substream); > > Again, simultaneous playback and capture. > Ok >> +MODULE_AUTHOR("Rajeev Kumar"); >> +MODULE_DESCRIPTION("SPEAr I2S SoC Interface"); >> +MODULE_LICENSE("GPL"); > > Should have a MODULE_ALIAS() for autoloading. > OK >> +#ifndef SPEAR_I2S_H >> +#define SPEAR_I2S_H >> + >> +#define MAX_CHANNEL_NUM 2 >> +#define MIN_CHANNEL_NUM 2 > > These need namespacing. > >> + .rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 | >> + SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 | >> + SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | >> + SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000 | >> + SNDRV_PCM_RATE_KNOT), >> + .rate_min = 48000, >> + .rate_max = 48000, > > These constraints don't agree with each other - the rate limits claim to > support 48kHz only, the rates setting tells a different story. > At present I checked STA529(codec) only with 48Khz,I need to check with other data rates also. >> +static int spear13xx_pcm_hw_params(struct snd_pcm_substream *substream, >> + struct snd_pcm_hw_params *params) >> +{ >> + struct snd_pcm_runtime *runtime = substream->runtime; >> + struct spear13xx_runtime_data *prtd = runtime->private_data; >> + int ret; >> + >> + ret = snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params)); >> + if (ret < 0) >> + return ret; >> + >> + prtd->substream = substream; >> + prtd->pos = 0; >> + return 0; >> +} > > Rather than passing the channel count through from the I2S driver you > could just get it directly here. > Are you talking about this "struct spear13xx_runtime_data *prtd = runtime->private_data" Could you please elaborate little bit more. >> +static void dma_configure(struct snd_pcm_substream *substream) >> +{ >> + struct spear13xx_runtime_data *prtd = substream->runtime->private_data; >> + >> + dma_cap_zero(prtd->mask); >> + dma_cap_set(DMA_SLAVE, prtd->mask); >> + >> + prtd->slaves = &data; >> + /* we need to pass physical address here */ >> + prtd->slaves->mem2i2s_slave.tx_reg = (dma_addr_t)prtd->txdma; >> + prtd->slaves->mem2i2s_slave.rx_reg = 0; >> + prtd->slaves->i2s2mem_slave.tx_reg = 0; >> + prtd->slaves->i2s2mem_slave.rx_reg = (dma_addr_t)prtd->rxdma; >> + >> + substream->runtime->private_data = prtd; >> +} >> +static bool filter(struct dma_chan *chan, void *slave) > > Blank lines between functions please. > OK >> + dev_info(buf->dev.dev, >> + " preallocate_dma_buffer: area=%p, addr=%p, size=%d\n", >> + (void *)buf->area, (void *)buf->addr, size); > > dev_dbg() > OK >> +static struct platform_driver spear13xx_pcm_driver = { >> + .driver = { >> + .name = "spear-pcm-audio", >> + .owner = THIS_MODULE, >> + }, > > Fix indentation here. > OK >> +MODULE_AUTHOR("Rajeev Kumar"); >> +MODULE_DESCRIPTION("spear PCM DMA module"); >> +MODULE_LICENSE("GPL"); > > MODULE_ALIAS() again please. > . OK Best Rgds Rajeev >