On Tue 2011-02-22 14:43:19, Sascha Hauer wrote:
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.
sorry for so many typos, I will enable spell check in my emacs configure.
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.
I will fix it in v5 patch.
+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);
You are right. I prefer to using regulator_bulk_free() to avoid accessing data structure of regulator_bulk_data.
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
Thanks.
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |