linux-kernel - Re: [PATCH v7] ASoC: tas2552: Support TI TAS2552 Amplifier

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 PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 17 Jul 2014 17:07:07 +0000
From:	"Murphy, Dan" <dmurphy@...com>
To:	Mark Brown <broonie@...nel.org>
CC:	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v7] ASoC: tas2552: Support TI TAS2552 Amplifier

Mark


On 07/17/2014 11:58 AM, Mark Brown wrote:
> On Mon, Jul 14, 2014 at 03:10:45PM -0500, Dan Murphy wrote:
>
> There's a few smallish issues below but this is basically good so I've
> applied it, please send incremental fixed for the things below.
>
>> +	/* Turn on Class D amplifier */
>> +	snd_soc_update_bits(codec, TAS2552_CFG_2, TAS2552_CLASSD_EN_MASK,
>> +						TAS2552_CLASSD_EN);
>> +
> Why is this being done in hw_params() and not using DAPM?
>
>> +static int tas2552_runtime_suspend(struct device *dev)
>> +{
>> +	struct tas2552_data *tas2552 = dev_get_drvdata(dev);
>> +
>> +	tas2552_sw_shutdown(tas2552, 0);
>> +
>> +	if (tas2552->enable_gpio)
>> +		gpiod_set_value(tas2552->enable_gpio, 0);
>> +
>> +	regcache_cache_only(tas2552->regmap, true);
>> +	regcache_mark_dirty(tas2552->regmap);
> It's better to do the GPIO set after making the device cache only in
> order to be sure nothing can come in and try to use the register map
> between the two.
>
>> +static void tas2552_shutdown(struct snd_pcm_substream *substream,
>> +			   struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_codec *codec = dai->codec;
>> +
>> +	snd_soc_update_bits(codec, TAS2552_CFG_2, TAS2552_PLL_ENABLE, 0);
>> +}
> I'd also expect the PLL power to be managed via DAPM.
>
>> +	ret = pm_runtime_get_sync(codec->dev);
>> +	if (ret < 0) {
>> +		dev_err(codec->dev, "Enabling device failed: %d\n",
>> +			ret);
>> +		goto probe_fail;
>> +	}
> There's no matching put for this in remove().
>
>> +	snd_soc_write(codec, TAS2552_CFG_2, TAS2552_CLASSD_EN |
>> +				  TAS2552_BOOST_EN | TAS2552_APT_EN |
>> +				  TAS2552_LIM_EN);
>> +	return 0;
> The class D is still being enabled here.

Thanks will send updates in a day or two.

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

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