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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 14 Jul 2014 19:43:21 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Dan Murphy <dmurphy@...com>
Cc:	linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org,
	alsa-devel@...a-project.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v6] ASoC: tas2552: Support TI TAS2552 Amplifier

On Fri, Jul 11, 2014 at 10:44:49AM -0500, Dan Murphy wrote:

> @@ -754,4 +755,8 @@ config SND_SOC_TPA6130A2
>  	tristate "Texas Instruments TPA6130A2 headphone amplifier"
>  	depends on I2C
>  
> +config SND_SOC_TAS2552
> +	tristate "Texas Instruments TAS2552 Mono Audio amplifier"
> +	depends on I2C
> +
>  endmenu

Keep this and Makefile sorted - since this is a proper CODEC driver it
should be in with the CODECs and also sorted in alphabetical order.

> +static int tas2552_startup(struct snd_pcm_substream *substream,
> +			   struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +
> +	pm_runtime_get_sync(codec->dev);
> +
> +	return 0;
> +}
> +
> +static void tas2552_shutdown(struct snd_pcm_substream *substream,
> +			   struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +
> +	pm_runtime_put(codec->dev);
> +}

These runtime calls should be redundant, the framework should hold a
runtime PM reference on devices while they are active.  Does this not
work for you?

> +	pm_runtime_get_sync(codec->dev);

Check the return value here.

> +	snd_soc_write(codec, TAS2552_CFG_2, TAS2552_CLASSD_EN |
> +				  TAS2552_BOOST_EN | TAS2552_APT_EN |
> +				  TAS2552_PLL_ENABLE | TAS2552_LIM_EN);

The PLL is enabled all the time not via DAPM or similar (it's never
disabled)?

> +static int tas2552_resume(struct snd_soc_codec *codec)
> +{
> +	struct tas2552_data *tas2552 = snd_soc_codec_get_drvdata(codec);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(tas2552->supplies),
> +				    tas2552->supplies);
> +
> +	if (ret != 0) {
> +		dev_err(codec->dev, "Failed to enable supplies: %d\n",
> +			ret);
> +	}
> +
> +	pm_runtime_get_sync(codec->dev);

Remove these runtime PM calls from suspend and resume, they're not doing
what you think (and will prevent runtime PM from doing anything).  Let
the frameworks worry about it for now, or explicitly call the runtime
suspend and resume operators if and only if the device is runtime
active.

> +	for (i = 0; i < ARRAY_SIZE(data->supplies); i++)
> +		data->supplies[i].supply = tas2552_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
> +				      data->supplies);
> +	if (ret != 0)
> +		dev_err(dev, "Failed to request supplies: %d\n", ret);

These supplies are mandatory (as they should be) but weren't mentioned
in the DT binding - please add them there.

> +static const struct i2c_device_id tas2552_id[] = {
> +	{ "tas2552-codec", 0 },
> +	{ }
> +};

No -codec.

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists