Mark Brown broonie@opensource.wolfsonmicro.com writes:
Hi,
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.
I'm using i2c_smbus_read_i2c_block_data() and didn't notice it in the soc-cache.c file. I didn't look further so it's possible that there's actually something working for my case.
- if (ret != MK_CHIP_REV(CHIP_ID, CHIP_REV)) {
Are you sure there's only one device revision in production?
According to the specs there are 2 revs id, the A and B. As I've got a rev B, I assumed that the rev A has not been on prod. I've changed the check to handle both revs.
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.
I'll make it log the device revision then. I prefer having a message telling us something was found than no message at all. It's usefull when debugging.
- 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?
The default reset state is good except for the bits I'm changing few lines later.
- /*
* DAC configuration (right place to do that ?)It's OK to do this for things that the user would unconditionally want.
* - Use signal processorWhat does this mean? It sounds like something that would need to be power managed via DAPM?
there are 3 options for DAC: 00 - PCM Serial Port to DAC 01 - Signal Processing Engine to DAC 10 - ADC Serial Port to DAC
but only the "signal processing engine to DAC" configuration is useful. Using others are dropping some functionnalities.
* - auto muteThis is probably OK, but exposing as a user visible control would be better.
there's a control for that. I'm only setting the register in a known good state.
* - vol changes immediateThis is OK.
* - no de-emphasizeThis is normally exposed as a user selectable switch.
idem
- /* 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.
ok. Default is 0db with channel enabled so it's fine I guess.
- /* route microphone */
- ret = cs42l51_i2c_write(codec, ADC_INPUT, 0xF0);
- if (ret < 0)
goto error;Ditto.
That's the only way to get microphone support, that's why I didn't consider it as an option.
+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?
1 and 2 are 2 values for the same things. (L+R)/2 signal is sent to both channels.
+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.
I wondered about this too and I choose to not use it. My thoughts was that if I was to go and implement the DAI mute, it should rather mute PCM playback and Analog playback channels at the same time I guess.
- 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.
It's to power down (or not) the 2 preamplifiers of the microphone channels and not to power down the microphone. Maybe the name is not the best one. Better names accepted. With this new explanation, should I convert it to DAPM or is it fine as mixer control ?
Arnaud