Re: [alsa-devel] [patch 4/6] cs42l51: add asoc driver

On Tue, May 11, 2010 at 06:23:46PM +0200, apatard@mandriva.com wrote:

This patch is adding a ASoC driver for the cs42l51 from Cirrus Logic. Master mode and spi mode are not supported.

This is mostly OK - there's quite a few comments below but there's no massive structural things in here, it's generally small, local things.

• • Based on cs4270.c - Copyright (c) Timur Tabi timur@freescale.com

While we're picking up on the copyright stuff IIRC it's actually Freescale copyright rather than Timur's personal copyright :)

+enum funct_mode {

• MODE_SLAVE,
• MODE_SLAVE_AUTO,
• MODE_MASTER,

+};

I'd prefer a more meaningful name than "funct_mode" here - I don't really know what a funct is.

+static unsigned int cs42l51_read_reg_cache(struct snd_soc_codec *codec,

• unsigned int reg)

+{

• u8 *cache = codec->reg_cache;
• if ((reg < CS42L51_FIRSTREG) || (reg > CS42L51_LASTREG))

• return -EIO;
  

• return cache[reg - CS42L51_FIRSTREG];

Please just use the standard register cache access stuff in soc-cache.c. The first register is 1 so you're only burning a single byte of RAM by using the generic code.

• if (ret != MK_CHIP_REV(CHIP_ID, CHIP_REV)) {

Are you sure there's only one device revision in production?

• dev_err(&i2c_client->dev, "Invalid chip id\n");
  

• ret = -ENODEV;
  

• goto error;
  

• }

• dev_info(&i2c_client->dev, "found device at I2C address %X\n",

• 		i2c_client->addr);
  

The dev_() will already be including the I2C address in the log message anyway. I'd be inclined to just drop this, or make it log the device revision.

• ret = cs42l51_fill_cache(codec);
• if (ret < 0) {

• dev_err(&i2c_client->dev, "failed to fill register cache\n");
  

• goto error;
  

• }

Normal practice is to reset the chip so it's in a known sensible state when the driver starts trying to use it. Is there a good reason not to do this here?

• /*

• * DAC configuration (right place to do that ?)
  

It's OK to do this for things that the user would unconditionally want.

• * - Use signal processor
  

What does this mean? It sounds like something that would need to be power managed via DAPM?

• * - auto mute
  

This is probably OK, but exposing as a user visible control would be better.

• * - vol changes immediate
  

This is OK.

• * - no de-emphasize
  

This is normally exposed as a user selectable switch.

• /* Unmute PCM-A & PCM-B and set default vol to */
• ret = cs42l51_i2c_write(codec, PCMA_VOL, 0x60);
• if (ret < 0)

• goto error;
  

This should not be done in the driver, leave it up to userspace. This driver will be used by all systems using this CODEC so just rely on the hardware defaults to provide a sane power on configuration - the designers will usually ensure that this is at least not harmful.

• /* route microphone */
• ret = cs42l51_i2c_write(codec, ADC_INPUT, 0xF0);
• if (ret < 0)

• goto error;
  

Ditto.

+error_reg:

• snd_soc_unregister_codec(codec);

+error:

• return ret;

You're missing some error handling here - there's no free of the device data, for example.

+static int cs42l51_get_chan_mix(struct snd_kcontrol *kcontrol,

• 	struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
• unsigned long value = snd_soc_read(codec, PCM_MIXER)&3;
• switch (value) {
• default:
• case 0:

• ucontrol->value.integer.value[0] = 0;
  

• break;
  

• case 1:
• case 2:

• ucontrol->value.integer.value[0] = 1;
  

• break;
  

• case 3:

• ucontrol->value.integer.value[0] = 2;
  

• break;
  

• }
• return 0;

+}

Some comments here might be a little clearer... Why is the difference between 1 and 2 not interesting?

+static const char *mic_boost[] = { "+16dB", "+32dB"};

Could use TLV for this too.

+static const struct soc_enum cs42l51_mix[] = {

• SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(chan_mix), chan_mix),
• SOC_ENUM_DOUBLE(MIC_CTL, 0, 1, 2, mic_boost),

+};

Don't do this, declare separate variables for each. Indexing into a table of mixer elements doesn't help leigibility and is asking for off by one errors. Some drivers do this for historical reasons but modern drivers don't.

• SOC_DOUBLE_R("PCM Playback Switch", PCMA_VOL, PCMB_VOL, 7, 1, 1),

I suspect that this should be managed via the DAI mute function.

• SOC_SINGLE("Playback Deemphasis", DAC_CTL, 3, 1, 0),

Should have "Switch" at the end of the control name.

• SOC_SINGLE("Mic Bias", MIC_POWER_CTL, 1, 1, 1),

This should be a DAPM MICBIAS control.

• SOC_DOUBLE("Mic Powerdown", MIC_POWER_CTL, 2, 3, 1, 0),

What does this do? Sounds like it should be DAPM controls.

• default:

• printk(KERN_ERR "cs4270: invalid DAI format\n");
  

Cut'n'paste.

+/* Fill me */ +static struct cs42l51_ratios master_ratios[] = { {}, +};

Just remove this stuff for master mode if it's not implemented.

• for (i = 0; i < ARRAY_SIZE(cs42l51_snd_controls); i++) {

• ret = snd_ctl_add(codec->card,
  

• 		snd_soc_cnew(&cs42l51_snd_controls[i],
  

• 		codec, NULL));
  

• if (ret < 0) {
  

• 	dev_err(&pdev->dev, "failed to add controls\n");
  

• 	snd_soc_free_pcms(socdev);
  

• 	return ret;
  

• }
  

• }

snd_soc_add_controls() rather than open coding.

+#define BEEP_FREQ 0x12 +#define BEEP_VOL 0x13 +#define BEEP_CONF 0x14

Please namespace everything in the header - things like this are asking for namespace clashes.