Re: [PATCH v6] ASoC: tas2552: Support TI TAS2552 Amplifier

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature