lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 13 Jul 2009 16:12:14 +0800 From: 宋宝华 <21cnbao@...il.com> To: Mark Brown <broonie@...nsource.wolfsonmicro.com> Cc: lrg@...mlogic.co.uk, alsa-devel@...a-project.org, linux-kernel@...r.kernel.org, Mike Frysinger <vapier.adi@...il.com> Subject: Re: [PATCH] New ASoC Drivers for ADI AD1938 codec 2009/6/19 Mark Brown <broonie@...nsource.wolfsonmicro.com>: > On Fri, Jun 19, 2009 at 05:28:15PM +0800, Barry Song wrote: >> 1. add AD1938 codec driver (codec) >> 2. add blackfin SPORT-TDM DAI and PCM driver (platform) >> 3. add bf5xx board with AD1938 driver (machine) > > As Liam said you really need to submit this as a patch series rather > than as a single big patch - as your commit log here indicates you've > got several different things going on here. > >> +++ b/include/sound/soc-dai.h >> @@ -30,6 +30,7 @@ struct snd_pcm_substream; >> #define SND_SOC_DAIFMT_DSP_A 3 /* L data msb after FRM LRC */ >> #define SND_SOC_DAIFMT_DSP_B 4 /* L data msb during FRM LRC */ >> #define SND_SOC_DAIFMT_AC97 5 /* AC97 */ >> +#define SND_SOC_DAIFMT_SPORT_TDM 6 /* SPORT TDM for ADI parts */ > > If you're going to add a new DAI format that really needs more > explanation than this explaining what the DAI format is. It'd be very > surprising to see hardware needing a new format. > > Looking at the datasheet for the ad1938 it appears that the actual > format here is just normal I2S with TDM. This does not need a new DAI > format or new CPU DAI, you just need to add suport for TDM to the > Blackfin I2S driver. The format is fairly standard and implemented by a > number of other devices. > > See set_tdm_slot() for setting up the higher channel counts - there's > some ongoing revisions to that API so you'll want to also ensure that > the code is set up so that it can cope with specification of the sample > width for each slot in set_tdm_slot(). > > Given this I've only looked at the CODEC driver below. > >> diff --git a/sound/soc/codecs/ad1938.c b/sound/soc/codecs/ad1938.c >> new file mode 100644 >> index 0000000..9aa78e1 >> --- /dev/null >> +++ b/sound/soc/codecs/ad1938.c > >> + * >> + * Revision history >> + * 4 June 2009 Initial version. > > Don't include this, git provides code history for us. > >> +struct snd_soc_device *ad1938_socdev; >> + >> +/* dac de-emphasis enum control */ >> +static const char *ad1938_deemp[] = {"flat", "48kHz", "44.1kHz", "32kHz"}; > > For consistency with other drivers "flat" should be "None". > >> +/* AD1938 volume/mute/de-emphasis etc. controls */ >> +static const struct snd_kcontrol_new ad1938_snd_controls[] = { >> + /* DAC volume control */ >> + SOC_SINGLE("DAC L1 Volume", AD1938_DAC_L1_VOL, 0, 0xFF, 1), >> + SOC_SINGLE("DAC R1 Volume", AD1938_DAC_R1_VOL, 0, 0xFF, 1), > > These (and the other stereo pairs below) should be SOC_DOUBLE_R(). This > allows ALSA to represent them as stereo controls to applications rather > than as two separate controls. You should also provide TLV information > so actually SOC_DOUBLE_R_TLV() if possible. > >> + /* DAC mute control */ >> + SOC_SINGLE("DAC L1 Switch", AD1938_DAC_CHNL_MUTE, 0, 1, 1), >> + SOC_SINGLE("DAC R1 Switch", AD1938_DAC_CHNL_MUTE, 1, 1, 1), > > These should be stereo controls too - SOC_DOUBLE() since they're in the > same register. > >> + /* ADC mute control */ >> + SOC_SINGLE("ADC L1 Switch", AD1938_ADC_CTRL0, ADC0_MUTE, 1, 1), >> + SOC_SINGLE("ADC R1 Switch", AD1938_ADC_CTRL0, ADC1_MUTE, 1, 1), > > These too. > >> + /* DAC de-emphasis */ >> + SOC_ENUM("Playback Deemphasis", ad1938_enum[0]), > > Don't put your enums in an array, use named variables for them. This > makes drivers easier to maintian when you get a lot of enums. > >> +static int ad1938_add_controls(struct snd_soc_codec *codec) >> +{ >> + int err, i; >> + >> + for (i = 0; i < ARRAY_SIZE(ad1938_snd_controls); i++) { >> + err = snd_ctl_add(codec->card, >> + snd_soc_cnew(&ad1938_snd_controls[i], codec, NULL)); > > Use snd_soc_add_controls() here - you can replace the entire function > with a call to that. > >> +/* dac/adc/pll poweron/off functions */ >> +static int ad1938_dac_powerctrl(struct snd_soc_codec *codec, int cmd) >> +{ >> + int reg; >> + >> + reg = codec->read(codec, AD1938_DAC_CTRL0); >> + if (cmd) >> + reg &= ~DAC_POWERDOWN; >> + else >> + reg |= DAC_POWERDOWN; >> + codec->write(codec, AD1938_DAC_CTRL0, reg); > > This should be handled by DAPM - either have a single DAC widget > representing all the channels (since you don't appear to have > independant control anyway) or have a bunch of dummy DAC widgets and a > supply widget representing the actual power control. The same thing > applies to the ADCs. I want to use ADC/DAC widgets. static const struct snd_soc_dapm_widget ad1938_dapm_widgets[] = { SND_SOC_DAPM_DAC("DAC", "HiFi Playback", AD1938_DAC_CTRL0, 0, 1), SND_SOC_DAPM_ADC("ADC", "HiFi Capture", AD1938_ADC_CTRL0, 0, 1), }; But for this AD1938 codec, DAC's work depends on ADC is powered on in hardware. I think there is no any mechanism to handle this kind of strange depending now. So is there a generic way to handle this? > >> +static int ad1938_set_pll(struct snd_soc_dai *codec_dai, >> + int pll_id, unsigned int freq_in, unsigned int freq_out) >> +{ >> + struct snd_soc_codec *codec = codec_dai->codec; >> + >> + if (freq_out) >> + ad1938_pll_powerctrl(codec, 1); >> + else { >> + /* playing while recording, framework will poweroff-poweron pll redundantly */ >> + if ((!codec_dai->capture.active) && (!codec_dai->playback.active)) >> + ad1938_pll_powerctrl(codec, 0); >> + } > > Hrm. This appears to completely ignore the frequencies supplied for the > PLL and just provide power control. I suspect that you can just handle > the PLL as a SND_SOC_DAPM_SUPPLY(), there seems to be no need to expose > the set_pll() operation and make machine drivers call it given that > there isn't any frequency configuration going on. > >> +static int ad1938_mute(struct snd_soc_dai *dai, int mute) >> +{ >> + struct snd_soc_codec *codec = dai->codec; >> + >> + if (!mute) >> + codec->write(codec, AD1938_DAC_CHNL_MUTE, 0); >> + else >> + codec->write(codec, AD1938_DAC_CHNL_MUTE, 0xff); >> + >> + return 0; >> +} > > This isn't going to play well with the explicit mute controls you've got > above - it's writing to the same register bits without any coordination. > One or the other set of controls ought to be removed. > >> +static int ad1938_tdm_set(struct snd_soc_codec *codec) >> +{ >> + codec->write(codec, AD1938_DAC_CTRL0, (codec->read(codec, AD1938_DAC_CTRL0) & >> + (~DAC_SERFMT_MASK)) | DAC_SERFMT_TDM); >> + codec->write(codec, AD1938_DAC_CTRL1, 0x84); /* invert bclk, 256bclk/frame, latch in mid */ >> + codec->write(codec, AD1938_ADC_CTRL1, 0x43); /* sata delay=1, adc aux mode */ >> + codec->write(codec, AD1938_ADC_CTRL2, 0x6F); /* left high, driver on rising edge */ >> + >> + return 0; >> +} > > If you use set_tdm_slot() then the BCLK/frame ratio will be set by that. > > Inversion of BCLK (and any other clocks) should be handled by the > set_dai_fmt() operation based on the machine driver request rather than > done unconditionally. > >> + /* bit size */ >> + switch (params_format(params)) { >> + case SNDRV_PCM_FORMAT_S16_LE: >> + word_len = 3; >> + break; > > Once you implement set_tdm_slot() you should allow the word length to be > configured there if it's called or otherwise keep this code here - see > Daniel Ribeiro's patche "change set_tdm_slot api to allow slot_width > override" posted to the ALSA list this week. > >> +static int __devinit ad1938_spi_probe(struct spi_device *spi) >> +{ >> + spi->dev.power.power_state = PMSG_ON; >> + ad1938_socdev->card->codec->control_data = spi; >> + >> + return 0; >> +} >> + >> +static int __devexit ad1938_spi_remove(struct spi_device *spi) >> +{ >> + return 0; >> +} > > Your device probing should all be restructured so that the SPI device > for the CODEC is registered as any other SPI device rather than being > set up as part of probing the ASoC device. See the wm8731 driver for > an example of doing this for a SPI device. > > This will require that the arch code for any systems with the ad1938 > do the setup of the device. > >> + .name = "AD1938", >> + .playback = { >> + .stream_name = "Playback", >> + .channels_min = 2, >> + .channels_max = 8, >> + .rates = SNDRV_PCM_RATE_48000, >> + .formats = SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S24_LE, }, > > Please keep your lines to under 80 columns. > >> +#define AD1938_PLL_CLK_CTRL0 0 >> +#define PLL_POWERDOWN 0x01 >> +#define AD1938_PLL_CLK_CTRL1 1 >> +#define AD1938_DAC_CTRL0 2 >> +#define DAC_POWERDOWN 0x01 >> +#define DAC_SERFMT_MASK 0xC0 >> +#define DAC_SERFMT_STEREO (0 << 6) >> +#define DAC_SERFMT_TDM (1 << 6) > > These defines need namespacing if they're going to appear in the headers > - everything should have the AD1938_ prefix. > -- 宋宝华 21cnbao@...n.com http://21cnbao.blog.51cto.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists