Re: [alsa-devel] [PATCH 2/2] ASoC: Add support for CS4265 CODEC

On Fri, May 23, 2014 at 02:16:54PM -0500, Paul Handrigan wrote:

This patch adds support for the Cirrus Logic Stereo I2C CODEC

This all looks pretty clean and nice, I have got a few comments below but they're all pretty small things.

• SOC_SINGLE("De-emp 44.1kHz", CS4265_DAC_CTL, 1,

• 		1, 0),
  

• SOC_SINGLE("DAC INV", CS4265_DAC_CTL2, 5,

• 		1, 0),
  

• SOC_SINGLE("DAC Zero Cross", CS4265_DAC_CTL2, 6,

• 		1, 0),
  

• SOC_SINGLE("DAC Soft Ramp", CS4265_DAC_CTL2, 7,

• 		1, 0),
  

All boolean controls should have Switch at the end of the name.

• SOC_SINGLE("ADC HPF Disable", CS4265_ADC_CTL, 1,

• 		1, 0),
  

Invert this one and call it ADC HPF Switch (similarly for other disable controls).

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

• if (clk_map_table[i].rate == rate &&
  

• 	clk_map_table[i].mclk == mclk)
  

• 	return i;
  

Indent the second line of the if condition with the (.

+static int cs4265_set_sysclk(struct snd_soc_dai *codec_dai, int clk_id,

• 	unsigned int freq, int dir)
  

+{

• struct snd_soc_codec *codec = codec_dai->codec;
• struct cs4265_private *cs4265 = snd_soc_codec_get_drvdata(codec);
• int i;
• for (i = 0; i < ARRAY_SIZE(clk_map_table); i++) {

• if (clk_map_table[i].mclk == freq) {
  

It's better to check that clk_id is 0 here, just in case you think of another clock to control in future (perhaps with a new device that can share the same driver even if it's not possible for this one).

• {

• .name = "cs4265-spdif",
  

• .playback = {
  

• 	.stream_name = "SPDIF Playback",
  

• 	.channels_min = 1,
  

• 	.channels_max = 2,
  

• 	.rates = CS4265_SPDIF_RATES,
  

• 	.formats = CS4265_FORMATS,
  

• },
  

• .ops = &cs4265_ops,
  

• },

You should have separate operations for the DAC and S/PDIF DAIs and only mute the relevant interfaces. If you can't mute the DAC DAIs independently just don't provide an operation and let the user control it as they like. This avoids problems if more than one stream is running at once.

+static int cs4265_probe(struct snd_soc_codec *codec) +{

• return 0;

+}

Remove empty operations.

• } else {

• pdata = devm_kzalloc(&i2c_client->dev,
  

• 		     sizeof(struct cs4265_platform_data),
  

• 		GFP_KERNEL);
  

• if (!pdata) {
  

• 	dev_err(&i2c_client->dev, "could not allocate pdata\n");
  

• 	return -ENOMEM;
  

• }
  

• pdata->reset_gpio = of_get_named_gpio(i2c_client->dev.of_node,
  

• 				"reset-gpio", 0);
  

• cs4265->pdata = *pdata;
  

• }

Can you convert this to use the new gpiod_ APIs and directly request by name? There's also the -gpios thing I mentioned for the binding.

• ret = snd_soc_register_codec(&i2c_client->dev,

• 	&soc_codec_dev_cs4265, cs4265_dai,
  

• 	ARRAY_SIZE(cs4265_dai));
  

• return ret;

devm_