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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
