Message ID | 20171129185015.5304-1-afd@ti.com |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | [v2,1/2] ASoC: codecs: Add PCM186x binding documentation | expand |
On Wed, Nov 29, 2017 at 12:50:15PM -0600, Andrew F. Davis wrote: > + case SND_SOC_BIAS_STANDBY: > + pcm186x_power_on(codec); > + break; > + case SND_SOC_BIAS_OFF: > + pcm186x_power_off(codec); > + break; > + } > +/* > + * The PCM186x's page register is located on every page, allowing to program it > + * without having to switch pages. Take advantage of this by defining the range > + * such to have this register located inside the data window. > + */ That sounds like a normal page register? > +int pcm186x_probe(struct device *dev, enum pcm186x_type type, int irq, > + struct regmap *regmap) > +{ > + ret = regulator_bulk_disable(ARRAY_SIZE(priv->supplies), > + priv->supplies); > + if (ret) { > + dev_err(dev, "failed disable supplies: %d\n", ret); > + return ret; > + } > +static int __maybe_unused pcm186x_resume(struct device *dev) > +{ > + struct pcm186x_priv *priv = dev_get_drvdata(dev); > + int ret; > + > + ret = regulator_bulk_enable(ARRAY_SIZE(priv->supplies), > + priv->supplies); > + if (ret != 0) { > + dev_err(dev, "failed to enable supplies: %d\n", ret); > + return ret; > + } > +const struct dev_pm_ops pcm186x_pm_ops = { > + SET_RUNTIME_PM_OPS(pcm186x_suspend, pcm186x_resume, NULL) > +}; > +EXPORT_SYMBOL_GPL(pcm186x_pm_ops); There's no code in the driver that enables runtime PM so this isn't going to do anything. I'm also not clear that the power management handling is in general joined up - we leave the regulators disabled at the end of probe, relying on the bias level configuration to reenable them but then the runtime PM configuration also tries to enable and disable them. Based on what I think the intention is I'd suggest removing the bias level handling and then having probe enable runtime PM with the device flagged as active, letting runtime PM do any disabling if the device is idle. I'd also expect to see some system suspend handling.
On 11/30/2017 06:20 AM, Mark Brown wrote: > On Wed, Nov 29, 2017 at 12:50:15PM -0600, Andrew F. Davis wrote: > >> + case SND_SOC_BIAS_STANDBY: >> + pcm186x_power_on(codec); >> + break; >> + case SND_SOC_BIAS_OFF: >> + pcm186x_power_off(codec); >> + break; >> + } > >> +/* >> + * The PCM186x's page register is located on every page, allowing to program it >> + * without having to switch pages. Take advantage of this by defining the range >> + * such to have this register located inside the data window. >> + */ > > That sounds like a normal page register? > It is, I believe Andreas commented this for his own reference, I'll drop it. >> +int pcm186x_probe(struct device *dev, enum pcm186x_type type, int irq, >> + struct regmap *regmap) >> +{ > >> + ret = regulator_bulk_disable(ARRAY_SIZE(priv->supplies), >> + priv->supplies); >> + if (ret) { >> + dev_err(dev, "failed disable supplies: %d\n", ret); >> + return ret; >> + } > >> +static int __maybe_unused pcm186x_resume(struct device *dev) >> +{ >> + struct pcm186x_priv *priv = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = regulator_bulk_enable(ARRAY_SIZE(priv->supplies), >> + priv->supplies); >> + if (ret != 0) { >> + dev_err(dev, "failed to enable supplies: %d\n", ret); >> + return ret; >> + } > >> +const struct dev_pm_ops pcm186x_pm_ops = { >> + SET_RUNTIME_PM_OPS(pcm186x_suspend, pcm186x_resume, NULL) >> +}; >> +EXPORT_SYMBOL_GPL(pcm186x_pm_ops); > > There's no code in the driver that enables runtime PM so this isn't > going to do anything. I'm also not clear that the power management > handling is in general joined up - we leave the regulators disabled > at the end of probe, relying on the bias level configuration to reenable > them but then the runtime PM configuration also tries to enable and > disable them. Based on what I think the intention is I'd suggest > removing the bias level handling and then having probe enable runtime > PM with the device flagged as active, letting runtime PM do any > disabling if the device is idle. > I beleive this was meant to be be SIMPLE_DEV_PM_OPS and not SET_RUNTIME_PM_OPS. I'll fix this all up for v3. Just thinking, the sound core sets SND_SOC_BIAS_OFF before suspend anyway, right? So the results would be similar just having all the PM stuff in the bias level handling for consistency, but I'm open to whatever is the preferred way. > I'd also expect to see some system suspend handling. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 30, 2017 at 09:56:08AM -0600, Andrew F. Davis wrote: > On 11/30/2017 06:20 AM, Mark Brown wrote: > > disable them. Based on what I think the intention is I'd suggest > > removing the bias level handling and then having probe enable runtime > > PM with the device flagged as active, letting runtime PM do any > > disabling if the device is idle. > I beleive this was meant to be be SIMPLE_DEV_PM_OPS and not > SET_RUNTIME_PM_OPS. I'll fix this all up for v3. I was wondering that. > Just thinking, the sound core sets SND_SOC_BIAS_OFF before suspend > anyway, right? So the results would be similar just having all the PM > stuff in the bias level handling for consistency, but I'm open to > whatever is the preferred way. It doesn't matter that much, if you do it only in set_bias_level() then unless you set idle_bias_off there will be no runtime PM which may or may not be what you want and you'll also not give the user the ability to control if runtime PM happens via the sysfs files but I'm not convinced that anyone ever actually does that. Either approach is fine really.
diff --git a/Documentation/devicetree/bindings/sound/pcm186x.txt b/Documentation/devicetree/bindings/sound/pcm186x.txt new file mode 100644 index 000000000000..1087f4855980 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/pcm186x.txt @@ -0,0 +1,42 @@ +Texas Instruments PCM186x Universal Audio ADC + +These devices support both I2C and SPI (configured with pin strapping +on the board). + +Required properties: + + - compatible : "ti,pcm1862", + "ti,pcm1863", + "ti,pcm1864", + "ti,pcm1865" + + - reg : The I2C address of the device for I2C, the chip select + number for SPI. + + - avdd-supply: Analog core power supply (3.3v) + - dvdd-supply: Digital core power supply + - iovdd-supply: Digital IO power supply + See regulator/regulator.txt for more information + +CODEC input pins: + * VINL1 + * VINR1 + * VINL2 + * VINR2 + * VINL3 + * VINR3 + * VINL4 + * VINR4 + +The pins can be used in referring sound node's audio-routing property. + +Example: + + pcm186x: audio-codec@4a { + compatible = "ti,pcm1865"; + reg = <0x4a>; + + avdd-supply = <®_3v3_analog>; + dvdd-supply = <®_3v3>; + iovdd-supply = <®_1v8>; + };