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

[Murphy, Dan]

On 07/14/2014 01:43 PM, Mark Brown wrote:
> 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.

OK.  I was trying to stay away from a conflict by adding to the
bottom of the file.

>
>> +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?

I will verify that the calls get called.

>
>> +	pm_runtime_get_sync(codec->dev);
> Check the return value here.

OK

>
>> +	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)?

Good catch.  I will make sure the PLL Is disabled in the shutdown call.

>
>> +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.

OK

>
>> +	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.

Will add

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

Missed this one

-- 
------------------
Dan Murphy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/