| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <AM5PR1001MB09941D80A91358F99189CB6D80450@AM5PR1001MB0994.EURPRD10.PROD.OUTLOOK.COM>
Date: Tue, 26 Nov 2019 17:02:12 +0000
From: Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>,
Adam Thomson <Adam.Thomson.Opensource@...semi.com>
CC: Support Opensource <Support.Opensource@...semi.com>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kernel@...labora.com" <kernel@...labora.com>
Subject: RE: [PATCHv2 2/6] ASoC: da7213: Add regulator support
On 22 November 2019 17:10, Sebastian Reichel wrote:
> Hi,
Ok, based on feedback I have no further comment so:
Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@...semi.com>
>
> On Thu, Nov 21, 2019 at 09:15:02PM +0000, Adam Thomson wrote:
> > On 20 November 2019 15:24, Sebastian Reichel wrote:
> >
> > > This adds support for most regulators of da7212 for improved
> > > power management. The only thing skipped was the speaker supply,
> > > which has some undocumented dependencies. It's supposed to be
> > > either always-enabled or always-disabled.
> > >
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>
> > > ---
> > > .../devicetree/bindings/sound/da7213.txt | 4 +
> > > sound/soc/codecs/da7213.c | 79 ++++++++++++++++++-
> > > sound/soc/codecs/da7213.h | 9 +++
> > > 3 files changed, 91 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/sound/da7213.txt
> > > b/Documentation/devicetree/bindings/sound/da7213.txt
> > > index 759bb04e0283..cc8200b7d748 100644
> > > --- a/Documentation/devicetree/bindings/sound/da7213.txt
> > > +++ b/Documentation/devicetree/bindings/sound/da7213.txt
> > > @@ -21,6 +21,10 @@ Optional properties:
> > > - dlg,dmic-clkrate : DMIC clock frequency (Hz).
> > > [<1500000>, <3000000>]
> > >
> > > + - VDDA-supply : Regulator phandle for Analogue power supply
> > > + - VDDMIC-supply : Regulator phandle for Mic Bias
> > > + - VDDIO-supply : Regulator phandle for I/O power supply
> > > +
> > > ======
> > >
> > > Example:
> > > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> > > index aff306bb58df..0359249118d0 100644
> > > --- a/sound/soc/codecs/da7213.c
> > > +++ b/sound/soc/codecs/da7213.c
> > > @@ -19,6 +19,7 @@
> > > #include <linux/module.h>
> > > #include <sound/pcm.h>
> > > #include <sound/pcm_params.h>
> > > +#include <linux/pm_runtime.h>
> > > #include <sound/soc.h>
> > > #include <sound/initval.h>
> > > #include <sound/tlv.h>
> > > @@ -806,6 +807,11 @@ static int da7213_dai_event(struct
> > > snd_soc_dapm_widget *w,
> > > */
> > >
> > > static const struct snd_soc_dapm_widget da7213_dapm_widgets[] = {
> > > + /*
> > > + * Power Supply
> > > + */
> > > + SND_SOC_DAPM_REGULATOR_SUPPLY("VDDMIC", 0, 0),
> > > +
> > > /*
> > > * Input & Output
> > > */
> > > @@ -932,6 +938,9 @@ static const struct snd_soc_dapm_route
> > > da7213_audio_map[] = {
> > > /* Dest Connecting Widget source */
> > >
> > > /* Input path */
> > > + {"Mic Bias 1", NULL, "VDDMIC"},
> > > + {"Mic Bias 2", NULL, "VDDMIC"},
> > > +
> > > {"MIC1", NULL, "Mic Bias 1"},
> > > {"MIC2", NULL, "Mic Bias 2"},
> > >
> > > @@ -1691,6 +1700,8 @@ static int da7213_probe(struct snd_soc_component
> > > *component)
> > > {
> > > struct da7213_priv *da7213 =
> > > snd_soc_component_get_drvdata(component);
> > >
> > > + pm_runtime_get_sync(component->dev);
> >
> > It seems that this function can return errors, although I do see lots of
> > instances of this being called where the return value isn't checked. Not had
> > time to walk the code fully but are we sure no errors are going to happen here?
>
> In this case, the runtime PM is already enabled because of
> pm_runtime_set_active() being called previously. So this only
> increases the usage counter.
>
> > > +
> > > /* Default to using ALC auto offset calibration mode. */
> > > snd_soc_component_update_bits(component, DA7213_ALC_CTRL1,
> > > DA7213_ALC_CALIB_MODE_MAN, 0);
> > > @@ -1811,6 +1822,8 @@ static int da7213_probe(struct snd_soc_component
> > > *component)
> > > DA7213_DMIC_CLK_RATE_MASK, dmic_cfg);
> > > }
> > >
> > > + pm_runtime_put_sync(component->dev);
> >
> > Same question here.
>
> da7213_runtime_suspend() always returns 0.
>
> > > +
> > > /* Check if MCLK provided */
> > > da7213->mclk = devm_clk_get(component->dev, "mclk");
> > > if (IS_ERR(da7213->mclk)) {
> > > @@ -1848,11 +1861,22 @@ static const struct regmap_config
> > > da7213_regmap_config = {
> > > .cache_type = REGCACHE_RBTREE,
> > > };
> > >
> > > +static void da7213_power_off(void *data)
> > > +{
> > > + struct da7213_priv *da7213 = data;
> > > + regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
> > > +}
> > > +
> > > +static const char *da7213_supply_names[DA7213_NUM_SUPPLIES] = {
> > > + [DA7213_SUPPLY_VDDA] = "VDDA",
> > > + [DA7213_SUPPLY_VDDIO] = "VDDIO",
> > > +};
> > > +
> > > static int da7213_i2c_probe(struct i2c_client *i2c,
> > > const struct i2c_device_id *id)
> > > {
> > > struct da7213_priv *da7213;
> > > - int ret;
> > > + int i, ret;
> > >
> > > da7213 = devm_kzalloc(&i2c->dev, sizeof(*da7213), GFP_KERNEL);
> > > if (!da7213)
> > > @@ -1860,6 +1884,25 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> > >
> > > i2c_set_clientdata(i2c, da7213);
> > >
> > > + /* Get required supplies */
> > > + for (i = 0; i < DA7213_NUM_SUPPLIES; ++i)
> > > + da7213->supplies[i].supply = da7213_supply_names[i];
> > > +
> > > + ret = devm_regulator_bulk_get(&i2c->dev, DA7213_NUM_SUPPLIES,
> > > + da7213->supplies);
> > > + if (ret) {
> > > + dev_err(&i2c->dev, "Failed to get supplies: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
> > > >supplies);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = devm_add_action_or_reset(&i2c->dev, da7213_power_off,
> > > da7213);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > da7213->regmap = devm_regmap_init_i2c(i2c, &da7213_regmap_config);
> > > if (IS_ERR(da7213->regmap)) {
> > > ret = PTR_ERR(da7213->regmap);
> > > @@ -1867,6 +1910,11 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> > > return ret;
> > > }
> > >
> > > + pm_runtime_set_autosuspend_delay(&i2c->dev, 100);
> > > + pm_runtime_use_autosuspend(&i2c->dev);
> > > + pm_runtime_set_active(&i2c->dev);
> >
> > Again this can return an error. Are we certain this won't fail?
>
> This only provides the information, that the device is running. The
> parent might be affected, but that is running anyways since we are
> probing a child device.
>
> > > + pm_runtime_enable(&i2c->dev);
> > > +
> > > ret = devm_snd_soc_register_component(&i2c->dev,
> > > &soc_component_dev_da7213, &da7213_dai, 1);
> > > if (ret < 0) {
> > > @@ -1876,6 +1924,34 @@ static int da7213_i2c_probe(struct i2c_client *i2c,
> > > return ret;
> > > }
> > >
> > > +static int __maybe_unused da7213_runtime_suspend(struct device *dev)
> > > +{
> > > + struct da7213_priv *da7213 = dev_get_drvdata(dev);
> > > +
> > > + regcache_cache_only(da7213->regmap, true);
> > > + regcache_mark_dirty(da7213->regmap);
> > > + regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int __maybe_unused da7213_runtime_resume(struct device *dev)
> > > +{
> > > + struct da7213_priv *da7213 = dev_get_drvdata(dev);
> > > + int ret;
> > > +
> > > + ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213-
> > > >supplies);
> > > + if (ret < 0)
> > > + return ret;
> > > + regcache_cache_only(da7213->regmap, false);
> > > + regcache_sync(da7213->regmap);
> > > + return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops da7213_pm = {
> > > + SET_RUNTIME_PM_OPS(da7213_runtime_suspend,
> > > da7213_runtime_resume, NULL)
> > > +};
> > > +
> > > static const struct i2c_device_id da7213_i2c_id[] = {
> > > { "da7213", 0 },
> > > { }
> > > @@ -1888,6 +1964,7 @@ static struct i2c_driver da7213_i2c_driver = {
> > > .name = "da7213",
> > > .of_match_table = of_match_ptr(da7213_of_match),
> > > .acpi_match_table = ACPI_PTR(da7213_acpi_match),
> > > + .pm = &da7213_pm,
> > > },
> > > .probe = da7213_i2c_probe,
> > > .id_table = da7213_i2c_id,
> > > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> > > index 3250a3821fcc..3890829dfb6e 100644
> > > --- a/sound/soc/codecs/da7213.h
> > > +++ b/sound/soc/codecs/da7213.h
> > > @@ -12,6 +12,7 @@
> > >
> > > #include <linux/clk.h>
> > > #include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > > #include <sound/da7213.h>
> > >
> > > /*
> > > @@ -521,9 +522,17 @@ enum da7213_sys_clk {
> > > DA7213_SYSCLK_PLL_32KHZ
> > > };
> > >
> > > +/* Regulators */
> > > +enum da7213_supplies {
> > > + DA7213_SUPPLY_VDDA = 0,
> > > + DA7213_SUPPLY_VDDIO,
> > > + DA7213_NUM_SUPPLIES,
> > > +};
> > > +
> > > /* Codec private data */
> > > struct da7213_priv {
> > > struct regmap *regmap;
> > > + struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
> > > struct clk *mclk;
> > > unsigned int mclk_rate;
> > > int clk_src;
> > > --
> > > 2.24.0
Powered by blists - more mailing lists