[alsa-devel] [PATCH] ASoC: Add support for Cirrus Logic CS42L73 Codec
Austin, Brian
Brian.Austin at cirrus.com
Fri Sep 16 16:26:11 CEST 2011
On Sep 16, 2011, at 5:17 AM, Mark Brown wrote:
> On Thu, Sep 15, 2011 at 09:40:48AM -0500, Brian Austin wrote:
>> ---
>
> This patch can't be applied as you haven't signed it off. Please follow
> the process in Documentation/SubmittingPatches, including the bit about
> always CCing maintainers on patches.
>
>> +#include <linux/i2c.h>
>> +#include <linux/platform_device.h>
>
> Platform device?
>
>> +struct cs42l73_private {
>> + enum snd_soc_control_type control_type;
>> + void *control_data;
>> + u8 reg_cache[CS42L73_CACHEREGNUM];
>
> Use the standard cache infrastructure, don't open code it.
>
>> +int cs42l73_write(struct snd_soc_codec *codec, unsigned reg, u_int val)
>> +{
>> + u8 data[2];
>> +
>> + if (reg > CS42L73_CACHEREGNUM)
>> + return -EINVAL;
>
> Use the standard I/O infrastructure too.
>
>> +int cs42l73_get_volsw(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_value *ucontrol)
>> +{
>> + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
>> + struct soc_mixer_control *mc =
>> + (struct soc_mixer_control *)kcontrol->private_value;
>> +
>> + unsigned int reg = mc->reg;
>> + unsigned int shift = mc->shift;
>> + unsigned int rshift = mc->rshift;
>> + int max = mc->max;
>> + int min = mc->min;
>> + int mmax = (max > min) ? max:min;
>> + unsigned int mask = (1 << fls(mmax)) - 1;
>> +
>> + ucontrol->value.integer.value[0] =
>> + ((cs42l73_read(codec, reg) >> shift) - min) & mask;
>> + if (shift != rshift)
>> + ucontrol->value.integer.value[1] =
>> + ((cs42l73_read(codec, reg) >> rshift) - min) & mask;
>
> Why is this open coded? This doesn't look driver specific and...
>
>> +#define SOC_SINGLE_S8_C_TLV(xname, xreg, xshift, xmax, xmin, tlv_array) \
>> +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
>
> ...the names given are obviously not driver specific names.
>
>> +/* ESL-ASP, ESL-XSP, SPK-ASP, SPK-XSP Mono Mixer Selects */
>> +static const struct soc_enum mono_mixer_enum[] =
>> +{
>> + SOC_ENUM_SINGLE(CS42L73_MMIXCTL, 6,
>> + ARRAY_SIZE (cs42l73_mono_mixer_text), cs42l73_mono_mixer_text),
>> + SOC_ENUM_SINGLE(CS42L73_MMIXCTL, 4,
>> + ARRAY_SIZE (cs42l73_mono_mixer_text), cs42l73_mono_mixer_text),
>> + SOC_ENUM_SINGLE(CS42L73_MMIXCTL, 2,
>> + ARRAY_SIZE (cs42l73_mono_mixer_text), cs42l73_mono_mixer_text),
>> + SOC_ENUM_SINGLE(CS42L73_MMIXCTL, 0,
>> + ARRAY_SIZE (cs42l73_mono_mixer_text), cs42l73_mono_mixer_text),
>> +};
>
> Don't use arrays of controls, use named variables. Arrays are error
> prone when referencing.
>
>> +static const struct snd_kcontrol_new cs42l73_snd_controls[] = {
>> +/*
>> + SOC_DOUBLE_R_S8_C (..., max, min)
>> + min - min from CS datasheet
>> + max - fls(max) - min + max from CS datasheet
>> +*/
>
> In general you've got rather a lot of comments pointing out very obvious
> things.
>
>> +/* Volume */
>
> Indentation here and throughout the table.
>
>> + SND_SOC_DAPM_MICBIAS("MIC1 Bias", CS42L73_PWRCTL2, 6, 1),
>> + SND_SOC_DAPM_INPUT("MIC2"),
>> + SND_SOC_DAPM_MICBIAS("MIC2 Bias", CS42L73_PWRCTL2, 7, 1),
>
> Just use supply widgets.
>
>> +static int cs42l73_add_widgets(struct snd_soc_codec *codec)
>> +{
>> + struct snd_soc_dapm_context *dapm = &codec->dapm;
>> +
>> + snd_soc_dapm_new_controls(dapm, cs42l73_dapm_widgets,
>> + ARRAY_SIZE(cs42l73_dapm_widgets));
>> + snd_soc_dapm_add_routes(dapm, audio_map, ARRAY_SIZE(audio_map));
>
> Just initialize these using the driver structure.
>
>> + /* MCLKX -> MCLK */
>> + mclkx_coeff = cs42l74_get_mclkx_coeff(priv->sysclk);
>> +
>> + if (mclkx_coeff < 0)
>> + return -EINVAL;
>
> Better to pass through the error code you got.
>
>> +static int cs42l73_set_sysclk(struct snd_soc_dai *dai,
>> + int clk_id, unsigned int freq, int dir)
>> +{
>> + struct snd_soc_codec *codec = dai->codec;
>> + struct cs42l73_private *priv = snd_soc_codec_get_drvdata(codec);
>> +
>> + if (clk_id != CS42L73_CLKID_MCLK1 && clk_id != CS42L73_CLKID_MCLK2) {
>> + dev_err(codec->dev, "Invalid clk_id %u\n", clk_id);
>> + return -EINVAL;
>> + }
>> +
>> + if ((cs42l74_get_mclkx_coeff(freq) < 0)) {
>> + dev_err(codec->dev, "Invalid sysclk %u\n", freq);
>> + return -EINVAL;
>> + }
>> +
>
> This ought to be a switch statement I think.
>
>> + priv->sysclk = freq;
>> + priv->mclksel = clk_id;
>> +
>> + return cs42l73_set_mclk(dai);
>
> With this we could assign the variables and then fail.
>
>> +/*
>> + cs42l73_set_dai_clkdiv()
>> + Setup MCLKx, MMCC dividers.
>> + The dividers are selected from the MCLKX and the
>> + sample rate.
>> +*/
>
> Coding style.
>
>> +static int cs42l73_set_clkdiv(struct snd_soc_dai *codec_dai,
>> + int div_id, int div)
>> +{
>> + struct snd_soc_codec *codec = codec_dai->codec;
>> + int id = codec_dai->id;
>> + u8 reg;
>
> Indentation.
>
>> + switch (div_id) {
>> + case CS42L73_MCLKXDIV:
>> + /* MCLKDIV */
>> + reg = cs42l73_read(codec, CS42L73_DMMCC) & 0xf1;
>> + cs42l73_write(codec, CS42L73_DMMCC,
>> + reg | ((div & 0x07) << 1));
>
> Coding style and indentation are both bad here. checkpatch should spot
> some of this. You should use snd_soc_update_bits() too (and throughout
> the code), and the comment should be fairly obvious but actually doesn't
> agree with the constant.
>
> Also, why are users having to explicitly set this stuff, can't the
> driver work this out?
>
>> +static u32 cs42l73_asrc_rates[] = {
>> + 8000, 11025, 12000, 16000, 22050,
>> + 24000, 32000, 44100, 48000
>> +};
>> +
>> +static unsigned int cs42l73_get_xspfs_coeff(u32 rate)
>> +{
>> + int i;
>> + for (i = 0; i < ARRAY_SIZE(cs42l73_asrc_rates); i++) {
>> + if (cs42l73_asrc_rates[i] == rate)
>> + return (i + 1);
>> + }
>> + return 0; /* 0 = Don't know */
>> +}
>
> Shouldn't you return an error?
>
>> +
>> +static void cs42l73_update_asrc(struct snd_soc_codec *codec, int id, int srate)
>> +{
>> + u8 spfs = 0;
>> + u8 reg;
>> +
>> + if (srate > 0)
>> + spfs = cs42l73_get_xspfs_coeff(srate);
>> +
>> + switch (id) {
>> + case CS42L73_XSP:
>> + reg = cs42l73_read(codec, CS42L73_VXSPFS);
>
> Coding style. There's a lot more issues than I've been pointing out, in
> general if your code doesn't visually resemble other Linux code there's
> an issue.
>
>> + case SND_SOC_BIAS_STANDBY:
>> + /* Powerdown all ports, inputs and outputs */
>> + cs42l73_write(codec, CS42L73_DMMCC, dmmcc & ~ MCLKDIS);
>> +
>> + cs42l73_write(codec, CS42L73_PWRCTL3,
>> + PDN_THMS | PDN_SPKLO | PDN_EAR |
>> + PDN_SPK | PDN_LO | PDN_HP);
>> +
>> + cs42l73_write(codec, CS42L73_PWRCTL2,
>> + PDN_MIC2_BIAS | PDN_MIC1_BIAS |
>> + PDN_VSP | PDN_ASP_SDOUT | PDN_ASP_SDIN |
>> + PDN_XSP_SDOUT | PDN_XSP_SDIN);
>> +
>> + cs42l73_write(codec, CS42L73_PWRCTL1,
>> + PDN_ADCB | PDN_ADCA | PDN_DMICB |
>> + PDN_DMICA | PDN);
>> + break;
>
> This looks like it should all be managed by DAPM.
>
>> +static void cs42l73_shutdown(struct snd_pcm_substream *substream,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct snd_soc_codec *codec = dai->codec;
>> + int id = dai->id;
>> + struct cs42l73_private *priv = snd_soc_codec_get_drvdata(codec);
>> + priv->config[id].srate = 0;
>> + cs42l73_update_asrc(codec,id,0);
>> +}
>
> What happens with simultaneous playback and record?
>
>> +static int cs42l73_resume(struct snd_soc_codec *codec)
>> +{
>> + int i;
>> + u8 *cache = codec->reg_cache;
>> +
>> + /* Sync reg_cache with the hardware */
>> + for (i = CS42L73_PWRCTL1; i < ARRAY_SIZE(cs42l73_reg); i++) {
>> + cs42l73_write(codec, i, cache[i]);
>> + }
>
> There's standard infrastructure for this too.
>
>> +/* CS42L73_PWRCTL1 */
>> +#define PDN_ADCB (1 << 7)
>> +#define PDN_DMICB (1 << 6)
>> +#define PDN_ADCA (1 << 5)
>> +#define PDN_DMICA (1 << 4)
>> +#define PDN_LDO (1 << 2)
>> +#define DISCHG_FILT (1 << 1)
>> +#define PDN (1 << 0)
>
> Namespacing here and elsewhere in the header.
>
Thanks for the quick feedback. I will fix it up and resend. Sorry about the sloppy formatting.
Brian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3917 bytes
Desc: not available
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20110916/e66f6f91/attachment.p7s
More information about the Alsa-devel
mailing list