From: Lars-Peter Clausen <lars@metafoo.de> To: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org, linux-kernel@vger.kernel.org, Liam Girdwood <lrg@slimlogic.co.uk>, alsa-devel@alsa-project.org Subject: Re: [RFC][PATCH 20/26] alsa: ASoC: Add JZ4740 codec driver Date: Fri, 04 Jun 2010 01:57:09 +0200 [thread overview] Message-ID: <4C084155.5010503@metafoo.de> (raw) In-Reply-To: <20100603174953.GH2762@rakim.wolfsonmicro.main> Hi Mark Brown wrote: > On Wed, Jun 02, 2010 at 09:12:26PM +0200, Lars-Peter Clausen wrote: > > >> This patch adds support for the JZ4740 internal codec. >> > > This looks very good, there are some issues but nothing too major. I > may be repeating some things others have said but hopefully not too > much. > > >> snd-soc-wm9712-objs := wm9712.o >> snd-soc-wm9713-objs := wm9713.o >> snd-soc-wm-hubs-objs := wm_hubs.o >> +snd-soc-jz4740-codec-objs := jz4740-codec.o >> > > Keep the devices sorted in both Makefile and Kconfig. > > >> +static int jz4740_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) >> +{ >> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { >> + case SND_SOC_DAIFMT_CBM_CFM: >> + break; >> + default: >> + return -EINVAL; >> + } >> > > This does nothing except validate some parameters. Is there actually an > externally visible DAI for this CODEC? If it's just integrated into the > SoC and there's nothing to configure then just omit the DAI > configuration since it's not even useful to document the signal format. > Nope, there is no externally visible DAI for the codec, but internally it is connected through the i2s controller of the JZ4740 which supports different operating modes. And if you'll do "snd_soc_dai_set_fmt(codec_dai, BOARD_DAIFMT); snd_soc_dai_set_fmt(cpu_dai, BOARD_DAIFMT);" with a wrong dai format it will be noticed. >> + .capture = { >> + .stream_name = "Capture", >> + .channels_min = 2, >> + .channels_max = 2, >> + .rates = SNDRV_PCM_RATE_8000_48000, >> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8, >> > > You listed an 18 bit format in hw_params - one or other of this and > hw_params is presumably out of date. > In theory the codec supports 18 bit for playback, but the i2s controller requires it to be 32 bit aligned, while alsa appears to have only support for a 24 bit aligned 18 bit format(Correct me if I'm wrong). So I dropped it, I'll remove the whole format check in hw_params as Liam suggested. >> +static int jz4740_codec_set_bias_level(struct snd_soc_codec *codec, >> + enum snd_soc_bias_level level) >> +{ >> + >> + if (codec->bias_level == SND_SOC_BIAS_OFF && level != SND_SOC_BIAS_OFF) { >> + snd_soc_update_bits(codec, JZ4740_REG_CODEC_1, >> + JZ4740_CODEC_1_RESET, JZ4740_CODEC_1_RESET); >> + udelay(2); >> > > I'd expect to see this as part of the _OFF in the main switch > statement. > Uhm, actually this code path is taken when switching from _OFF to another state. If it is guaranteed that _OFF is always followed by a certain other state I could put the reset code in its part of the switch statement. > >> + switch (level) { >> + case SND_SOC_BIAS_ON: >> + snd_soc_update_bits(codec, JZ4740_REG_CODEC_1, >> + JZ4740_CODEC_1_VREF_DISABLE | >> + JZ4740_CODEC_1_VREF_AMP_DISABLE | >> + JZ4740_CODEC_1_HEADPHONE_POWER_DOWN_M | >> + JZ4740_CODEC_1_VREF_LOW_CURRENT | >> + JZ4740_CODEC_1_VREF_HIGH_CURRENT, >> + 0); >> > > This looks suspiciously like you should be using DAPM for the headphone > at least, though if there's only headphone out that's possibly not worth > it. Also, are you sure that you want both low and high current VREF > configuring here? I'm not clear what these settings do but the way > they're being managed both here and in _PREPARE seems odd. > Hm, I'll take a look. > >> + codec = &jz4740_codec->codec; >> + >> + codec->dev = &pdev->dev; >> + codec->name = "jz-codec"; >> > > Seems a bit odd to use the part number in some places and not others. > I renamed the driver from jz-codec to jz4740-codec shortly before submitting it after I realized that the codec component on other jz47xx chips is completely different. Seems like I missed the codec name. Thanks for reviewing the patch - Lars
WARNING: multiple messages have this Message-ID (diff)
From: Lars-Peter Clausen <lars@metafoo.de> To: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: linux-mips@linux-mips.org, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, Ralf Baechle <ralf@linux-mips.org>, Liam Girdwood <lrg@slimlogic.co.uk> Subject: Re: [RFC][PATCH 20/26] alsa: ASoC: Add JZ4740 codec driver Date: Fri, 04 Jun 2010 01:57:09 +0200 [thread overview] Message-ID: <4C084155.5010503@metafoo.de> (raw) In-Reply-To: <20100603174953.GH2762@rakim.wolfsonmicro.main> Hi Mark Brown wrote: > On Wed, Jun 02, 2010 at 09:12:26PM +0200, Lars-Peter Clausen wrote: > > >> This patch adds support for the JZ4740 internal codec. >> > > This looks very good, there are some issues but nothing too major. I > may be repeating some things others have said but hopefully not too > much. > > >> snd-soc-wm9712-objs := wm9712.o >> snd-soc-wm9713-objs := wm9713.o >> snd-soc-wm-hubs-objs := wm_hubs.o >> +snd-soc-jz4740-codec-objs := jz4740-codec.o >> > > Keep the devices sorted in both Makefile and Kconfig. > > >> +static int jz4740_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) >> +{ >> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { >> + case SND_SOC_DAIFMT_CBM_CFM: >> + break; >> + default: >> + return -EINVAL; >> + } >> > > This does nothing except validate some parameters. Is there actually an > externally visible DAI for this CODEC? If it's just integrated into the > SoC and there's nothing to configure then just omit the DAI > configuration since it's not even useful to document the signal format. > Nope, there is no externally visible DAI for the codec, but internally it is connected through the i2s controller of the JZ4740 which supports different operating modes. And if you'll do "snd_soc_dai_set_fmt(codec_dai, BOARD_DAIFMT); snd_soc_dai_set_fmt(cpu_dai, BOARD_DAIFMT);" with a wrong dai format it will be noticed. >> + .capture = { >> + .stream_name = "Capture", >> + .channels_min = 2, >> + .channels_max = 2, >> + .rates = SNDRV_PCM_RATE_8000_48000, >> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8, >> > > You listed an 18 bit format in hw_params - one or other of this and > hw_params is presumably out of date. > In theory the codec supports 18 bit for playback, but the i2s controller requires it to be 32 bit aligned, while alsa appears to have only support for a 24 bit aligned 18 bit format(Correct me if I'm wrong). So I dropped it, I'll remove the whole format check in hw_params as Liam suggested. >> +static int jz4740_codec_set_bias_level(struct snd_soc_codec *codec, >> + enum snd_soc_bias_level level) >> +{ >> + >> + if (codec->bias_level == SND_SOC_BIAS_OFF && level != SND_SOC_BIAS_OFF) { >> + snd_soc_update_bits(codec, JZ4740_REG_CODEC_1, >> + JZ4740_CODEC_1_RESET, JZ4740_CODEC_1_RESET); >> + udelay(2); >> > > I'd expect to see this as part of the _OFF in the main switch > statement. > Uhm, actually this code path is taken when switching from _OFF to another state. If it is guaranteed that _OFF is always followed by a certain other state I could put the reset code in its part of the switch statement. > >> + switch (level) { >> + case SND_SOC_BIAS_ON: >> + snd_soc_update_bits(codec, JZ4740_REG_CODEC_1, >> + JZ4740_CODEC_1_VREF_DISABLE | >> + JZ4740_CODEC_1_VREF_AMP_DISABLE | >> + JZ4740_CODEC_1_HEADPHONE_POWER_DOWN_M | >> + JZ4740_CODEC_1_VREF_LOW_CURRENT | >> + JZ4740_CODEC_1_VREF_HIGH_CURRENT, >> + 0); >> > > This looks suspiciously like you should be using DAPM for the headphone > at least, though if there's only headphone out that's possibly not worth > it. Also, are you sure that you want both low and high current VREF > configuring here? I'm not clear what these settings do but the way > they're being managed both here and in _PREPARE seems odd. > Hm, I'll take a look. > >> + codec = &jz4740_codec->codec; >> + >> + codec->dev = &pdev->dev; >> + codec->name = "jz-codec"; >> > > Seems a bit odd to use the part number in some places and not others. > I renamed the driver from jz-codec to jz4740-codec shortly before submitting it after I realized that the codec component on other jz47xx chips is completely different. Seems like I missed the codec name. Thanks for reviewing the patch - Lars
next prev parent reply other threads:[~2010-06-03 23:57 UTC|newest] Thread overview: 97+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-06-02 19:02 [RFC][PATCH 00/26] *** SUBJECT HERE *** Lars-Peter Clausen 2010-06-02 19:02 ` [RFC][PATCH 01/26] MIPS: Add base support for Ingenic JZ4740 System-on-a-Chip Lars-Peter Clausen 2010-06-03 14:27 ` Florian Fainelli 2010-06-03 17:03 ` Lars-Peter Clausen 2010-06-02 19:02 ` [RFC][PATCH 02/26] MIPS: jz4740: Add IRQ handler code Lars-Peter Clausen 2010-06-03 14:29 ` Florian Fainelli 2010-06-02 19:02 ` [RFC][PATCH 03/26] MIPS: JZ4740: Add clock API support Lars-Peter Clausen 2010-06-02 22:45 ` Graham Gower 2010-06-03 17:20 ` Lars-Peter Clausen 2010-06-02 19:02 ` [RFC][PATCH 04/26] MIPS: JZ4740: Add timer support Lars-Peter Clausen 2010-06-02 19:02 ` [RFC][PATCH 05/26] MIPS: JZ4740: Add clocksource/clockevent support Lars-Peter Clausen 2010-06-02 19:02 ` [RFC][PATCH 06/26] MIPS: JZ4740: Add power-management and system reset support Lars-Peter Clausen 2010-06-02 19:02 ` [RFC][PATCH 07/26] MIPS: JZ4740: Add setup code Lars-Peter Clausen 2010-06-02 19:02 ` [RFC][PATCH 08/26] MIPS: JZ4740: Add gpio support Lars-Peter Clausen 2010-06-02 19:10 ` [RFC][PATCH 09/26] MIPS: JZ4740: Add DMA support Lars-Peter Clausen 2010-06-02 19:10 ` [RFC][PATCH 10/26] MIPS: JZ4740: Add PWM support Lars-Peter Clausen 2010-06-02 19:10 ` [RFC][PATCH 11/26] MIPS: JZ4740: Add serial support Lars-Peter Clausen 2010-06-02 19:10 ` [RFC][PATCH 12/26] MIPS: JZ4740: Add prom support Lars-Peter Clausen 2010-06-02 19:10 ` [RFC][PATCH 13/26] MIPS: JZ4740: Add platform devices Lars-Peter Clausen 2010-06-02 19:10 ` [RFC][PATCH 14/26] MIPS: JZ4740: Add Kbuild files Lars-Peter Clausen 2010-06-04 0:47 ` Ralf Baechle 2010-06-02 19:10 ` [RFC][PATCH 15/26] RTC: Add JZ4740 RTC driver Lars-Peter Clausen 2010-06-05 15:48 ` [rtc-linux] " Wan ZongShun 2010-06-05 17:26 ` Lars-Peter Clausen 2010-06-02 19:10 ` [RFC][PATCH 16/26] fbdev: Add JZ4740 framebuffer driver Lars-Peter Clausen 2010-06-02 19:10 ` Lars-Peter Clausen 2010-06-02 19:36 ` Andrew Morton 2010-06-02 19:36 ` Andrew Morton 2010-06-02 20:05 ` Lars-Peter Clausen 2010-06-02 20:05 ` Lars-Peter Clausen 2010-06-02 19:12 ` [RFC][PATCH 17/26] MTD: Nand: Add JZ4740 NAND driver Lars-Peter Clausen 2010-06-02 19:12 ` Lars-Peter Clausen 2010-06-13 9:40 ` Artem Bityutskiy 2010-06-13 9:40 ` Artem Bityutskiy 2010-06-02 19:12 ` [RFC][PATCH 18/26] MMC: Add JZ4740 mmc driver Lars-Peter Clausen 2010-06-02 19:12 ` [RFC][PATCH 19/26] USB: Add JZ4740 ohci support Lars-Peter Clausen 2010-06-02 19:12 ` [RFC][PATCH 20/26] alsa: ASoC: Add JZ4740 codec driver Lars-Peter Clausen 2010-06-02 19:12 ` Lars-Peter Clausen 2010-06-03 5:45 ` [alsa-devel] " Wan ZongShun 2010-06-03 12:03 ` Mark Brown 2010-06-03 12:03 ` Mark Brown 2010-06-03 12:32 ` [alsa-devel] " Liam Girdwood 2010-06-03 12:32 ` Liam Girdwood 2010-06-03 12:50 ` [alsa-devel] " Liam Girdwood 2010-06-03 12:50 ` Liam Girdwood 2010-06-03 16:58 ` [alsa-devel] " Lars-Peter Clausen 2010-06-03 16:58 ` Lars-Peter Clausen 2010-06-03 17:49 ` Mark Brown 2010-06-03 17:49 ` Mark Brown 2010-06-03 23:57 ` Lars-Peter Clausen [this message] 2010-06-03 23:57 ` Lars-Peter Clausen 2010-06-03 23:59 ` Mark Brown 2010-06-03 23:59 ` Mark Brown 2010-06-02 19:12 ` [RFC][PATCH 21/26] alsa: ASoC: Add JZ4740 ASoC support Lars-Peter Clausen 2010-06-02 19:12 ` Lars-Peter Clausen 2010-06-03 3:36 ` [alsa-devel] " Wan ZongShun 2010-06-03 3:36 ` Wan ZongShun 2010-06-03 12:48 ` Liam Girdwood 2010-06-03 12:48 ` Liam Girdwood 2010-06-03 16:50 ` Lars-Peter Clausen 2010-06-03 16:50 ` Lars-Peter Clausen 2010-06-03 17:03 ` Liam Girdwood 2010-06-03 17:16 ` Lars-Peter Clausen 2010-06-03 17:16 ` Lars-Peter Clausen 2010-06-03 17:25 ` Liam Girdwood 2010-06-03 17:25 ` Liam Girdwood 2010-06-03 17:37 ` Lars-Peter Clausen 2010-06-03 17:37 ` Lars-Peter Clausen 2010-06-03 18:14 ` [alsa-devel] " Troy Kisky 2010-06-03 18:14 ` Troy Kisky 2010-06-03 18:14 ` [alsa-devel] " Troy Kisky 2010-11-14 13:29 ` hi!!!! dkisky 2010-06-03 17:55 ` [RFC][PATCH 21/26] alsa: ASoC: Add JZ4740 ASoC support Mark Brown 2010-06-03 17:55 ` Mark Brown 2010-06-03 19:27 ` Lars-Peter Clausen 2010-06-02 19:12 ` [RFC][PATCH 22/26] hwmon: Add JZ4740 ADC driver Lars-Peter Clausen 2010-06-02 19:12 ` [lm-sensors] " Lars-Peter Clausen 2010-06-05 17:22 ` Jonathan Cameron 2010-06-05 17:22 ` [lm-sensors] " Jonathan Cameron 2010-06-05 19:08 ` Lars-Peter Clausen 2010-06-05 19:08 ` [lm-sensors] " Lars-Peter Clausen 2010-06-05 21:07 ` Jonathan Cameron 2010-06-05 21:07 ` Jonathan Cameron 2010-06-05 22:12 ` Lars-Peter Clausen 2010-06-05 22:12 ` Lars-Peter Clausen 2010-06-02 19:12 ` [RFC][PATCH 23/26] power: Add JZ4740 battery driver Lars-Peter Clausen 2010-06-14 15:51 ` Anton Vorontsov 2010-06-15 17:28 ` Lars-Peter Clausen 2010-06-15 17:34 ` Ralf Baechle 2010-06-16 12:20 ` Mark Brown 2010-06-19 3:48 ` Lars-Peter Clausen 2010-06-02 19:12 ` [RFC][PATCH 24/26] MIPS: JZ4740: Add qi_lb60 board support Lars-Peter Clausen 2010-06-02 19:15 ` [RFC][PATCH 25/26] MIPS: Add defconfig for the qi_lb60 board Lars-Peter Clausen 2010-06-02 19:15 ` [RFC][PATCH 26/26] alsa: ASoC: JZ4740: Add qi_lb60 board driver Lars-Peter Clausen 2010-06-02 19:15 ` Lars-Peter Clausen 2010-06-03 17:57 ` Mark Brown 2010-06-03 17:57 ` Mark Brown
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=4C084155.5010503@metafoo.de \ --to=lars@metafoo.de \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@opensource.wolfsonmicro.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mips@linux-mips.org \ --cc=lrg@slimlogic.co.uk \ --cc=ralf@linux-mips.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.