Re: [alsa-devel] [PATCH v4] ASoC: Add Freescale SGTL5000 codec support

Hi Zeng,

I finally got some time testing this. I did not get it to work yet, but some comments inline.

Sascha

On Mon, Feb 21, 2011 at 08:09:25AM +0800, zhaoming.zeng@freescale.com wrote:

+/* regulator supplies for sgtl5000, VDDD is an option external supply */ +enum sgtl5000_regulator_supplies {

• VDDA,
• VDDIO,
• VDDD,
• SGTL5000_SUPPLY_NUM

+};

+/* vddd is optinal supply */

Should be 'optional'

• /* set devided factor of frame clock */

'divided'

• switch (sys_fs / frame_rate) {
• case 4:

• clk_ctl |= SGTL5000_RATE_MODE_DIV_4 << SGTL5000_RATE_MODE_SHIFT;
  

• break;
  

• case 2:

• clk_ctl |= SGTL5000_RATE_MODE_DIV_2 << SGTL5000_RATE_MODE_SHIFT;
  

• break;
  

• case 1:

• clk_ctl |= SGTL5000_RATE_MODE_DIV_1 << SGTL5000_RATE_MODE_SHIFT;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• /* set the sys_fs accroding to frame rate */

Should be 'according'. This typo is present three more times in the patch.

• switch (sys_fs) {
• case 32000:

• clk_ctl |= SGTL5000_SYS_FS_32k << SGTL5000_SYS_FS_SHIFT;
  

• break;
  

• case 44100:

• clk_ctl |= SGTL5000_SYS_FS_44_1k << SGTL5000_SYS_FS_SHIFT;
  

• break;
  

• case 48000:

• clk_ctl |= SGTL5000_SYS_FS_48k << SGTL5000_SYS_FS_SHIFT;
  

• break;
  

• case 96000:

• clk_ctl |= SGTL5000_SYS_FS_96k << SGTL5000_SYS_FS_SHIFT;
  

• break;
  

• default:

• dev_err(codec->dev, "frame rate %d not supported\n",
  

• 	frame_rate);
  

• return -EINVAL;
  

• }
• /*

• * calculate the divider of mclk/sample_freq,
  

• * factor of freq =96k can only be 256, since mclk in range (12m,27m)
  

• */
  

• switch (sgtl5000->sysclk / sys_fs) {
• case 256:

• clk_ctl |= SGTL5000_MCLK_FREQ_256FS <<
  

• 	SGTL5000_MCLK_FREQ_SHIFT;
  

• break;
  

• case 384:

• clk_ctl |= SGTL5000_MCLK_FREQ_384FS <<
  

• 	SGTL5000_MCLK_FREQ_SHIFT;
  

• break;
  

• case 512:

• clk_ctl |= SGTL5000_MCLK_FREQ_512FS <<
  

• 	SGTL5000_MCLK_FREQ_SHIFT;
  

• break;
  

• default:

• /* if mclk not satisify the divider, use pll */
  

• if (sgtl5000->master)
  

• 	clk_ctl |= SGTL5000_MCLK_FREQ_PLL <<
  

• 		SGTL5000_MCLK_FREQ_SHIFT;
  

• else {
  

If one branch of an if statement has braces than the other should have them too.

• 	pr_err("%s: PLL not supported in slave mode\n",
  

• 		__func__);
  

should be dev_err like already used elsewhere in this function.

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

• u16 reg;
• int ret;
• int rev, i;
• int external_vddd = 0;
• struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
• for (i = 0; i < ARRAY_SIZE(sgtl5000->supplies); i++)

• sgtl5000->supplies[i].supply = supply_names[i];
  

• ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),

• 		sgtl5000->supplies);
  

• if (!ret)

• external_vddd = 1;
  

• else {

• /* set internal ldo to 1.2v */
  

• int voltage = LDO_VOLTAGE;
  

• sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME;
  

• ret = ldo_regulator_register(codec, &ldo_init_data, voltage);
  

• if (ret) {
  

• 	dev_err(codec->dev,
  

• 	"Failed to register vddd internal supplies: %d\n",
  

• 		ret);
  

• 	return ret;
  

• }
  

• ret = regulator_bulk_get(codec->dev,
  

• 		ARRAY_SIZE(sgtl5000->supplies),
  

• 		sgtl5000->supplies);
  

• if (ret) {
  

• 	ldo_regulator_remove(codec);
  

• 	dev_err(codec->dev,
  

• 		"Failed to request supplies: %d\n", ret);
  

• 	return ret;
  

• }
  

• }
• ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),

• 			sgtl5000->supplies);
  

• if (ret)

• goto err_regulator_free;
  

• /* wait for all power rails bring up */
• udelay(10);
• /* read chip information */
• reg = snd_soc_read(codec, SGTL5000_CHIP_ID);
• if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) !=

•    SGTL5000_PARTID_PART_ID) {
  

• dev_err(codec->dev,
  

• 	"Device with ID register %x is not a sgtl5000\n", reg);
  

• ret = -ENODEV;
  

• goto err_regulator_disable;
  

• }
• rev = (reg & SGTL5000_REVID_MASK) >> SGTL5000_REVID_SHIFT;
• dev_info(codec->dev, "sgtl5000 revision %d\n", rev);
• /*

• * workaround for revision 0x11 and later,
  

• * roll back to use internal LDO
  

• */
  

• if (external_vddd && rev >= 0x11) {

• int voltage = LDO_VOLTAGE;
  

• /* disable all regulator first */
  

• regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
  

• 			sgtl5000->supplies);
  

• /* free VDDD regulator */
  

• regulator_put(sgtl5000->supplies[VDDD].consumer);
  

• sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME;
  

• ret = ldo_regulator_register(codec, &ldo_init_data, voltage);
  

• if (ret)
  

• 	return ret;
  

• ret = regulator_bulk_get(codec->dev,
  

• 		ARRAY_SIZE(sgtl5000->supplies),
  

• 		sgtl5000->supplies);
  

If I understand this correctly you want to replace VDDD with the just registered regulator. You freed one regulator from the array, so instead of requesting them all again this should be:

sgtl5000->supplies[VDDD].consumer = regulator_get(codec->dev, sgtl5000->supplies[VDDD].supply);

• if (ret) {
  

• 	ldo_regulator_remove(codec);
  

• 	dev_err(codec->dev,
  

• 		"Failed to request supplies: %d\n", ret);
  

• 	return ret;
  

• }
  

• ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),
  

• 				sgtl5000->supplies);
  

• if (ret)
  

• 	goto err_regulator_free;
  

• }
• return 0;

+err_regulator_disable:

• regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),

• 		sgtl5000->supplies);
  

+err_regulator_free:

• regulator_bulk_free(ARRAY_SIZE(sgtl5000->supplies),

• 		sgtl5000->supplies);
  

• if (external_vddd)

• ldo_regulator_remove(codec);
  

• return ret;

+} +static int sgtl5000_probe(struct snd_soc_codec *codec)

please add a blank line between two functions.

+{

• int ret;

Sascha