On Thu, Oct 08, 2009 at 02:58:56PM +0300, Eduardo Valentin wrote:
This patch adds initial usage of regulator framework to control avdd_dac inside tlv320aic3x ASoC codec driver.
If you're going to do this you should add support for all the supplies of the device, not just one of them. The extra effort required is low and it avoids compatibility problems when someone wants to set up other supplies, causing existing boards to need to specify them.
- aic3x->regulator = regulator_get(&i2c->dev, "avdd_dac");
- if (IS_ERR(aic3x->regulator)) {
dev_warn(&i2c->dev, "No regulator to supply avdd_dac."" Assuming always on.\n");
You shouldn't split error messages over multiple lines like this, it breaks grepping to try to find where the message came from.
I'd also rather see failure to get the regulator treated as a hard error. When the regulator API compiled out it is stubbed so that for simple get/enable/disable/put usage it will return success but do nothing. If the regulator API is compiled in and we're not able to acquire regulators there's a good chance that things will break (eg, due to supplies being turned off because they appear to be unused) so flagging the error immediately is less likely to result in runtime fragility.
- /*
* REVISIT: Need to add proper code to put into sleep mode* avdd_dac regulator. For now, just leave it on.*/- if (aic3x->regulator) {
int err;err = regulator_enable(aic3x->regulator);if (err < 0)return err;- }
The best way to handle this is to push the enable/disable into the bias level configuration so that the regulators are enabled when the chip goes off->standby and disabled during standby->off. This will have the same effect for the moment but will mean that we'll be able to add core support for fully powering down the audio subsystem at runtime in the future. -- To unsubscribe from this list: send the line "unsubscribe alsa-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html