linux-kernel - Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20171213062037.ti2w2rharpjji2bf@localhost.localdomain>
Date:   Wed, 13 Dec 2017 00:20:39 -0600
From:   Steven Eckhoff <steven.eckhoff.opensource@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     alsa-devel@...a-project.org, Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's
 TSCS42xx audio CODEC

On Tue, Dec 12, 2017 at 04:51:35PM -0600, Steven Eckhoff wrote:

> > > > +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
> > > > +{
> > > > +	struct snd_soc_codec *codec = dai->codec;
> > > > +	int ret;
> > > > +
> > > > +	if (mute)
> > > > +		if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > > +			ret = dac_mute(codec);
> > > > +		else
> > > > +			ret = adc_mute(codec);
> > > > +	else
> > > > +		if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > > +			ret = dac_unmute(codec);
> > > > +		else
> > > > +			ret = adc_unmute(codec);
> > > > +
> > > > +	return ret;
> > > > +}
> > > 
> > > All these mute functions also shut down the PLLs which since...
> > > 
> > This is a bit funky since it looks like if you unmuted the dac and then
> > muted the adc that the PLLs would be powered off on the playback stream,
> > but the logic in the chip is a bit funky in that it wont allow this unless
> > the playback interface is also powered off.
> > 
> > This should definitely be fixed since it is confusing. The PLL
> > powered up/down stuff will be removed from the mute functions in the
> > next version.
> > 
> > What are your thoughts on turning the PLL into a DAPM widget and using
> > the event callback to power up/down the PLLs? I have tested this and it
> > seems to work fine.
> > 
> > > > +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > > > +	case SND_SOC_DAIFMT_CBM_CFM:
> > > > +		ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> > > > +				RV_AIC1_MS_MASTER);
> > > > +		if (ret < 0)
> > > > +			dev_err(codec->dev,
> > > > +				"Failed to set codec DAI master (%d)\n", ret);
> > > > +		else
> > > > +			ret = 0;
> > > > +		break;
> > > > +	default:
> > > 
> > > ...we only support the CODEC being the clock master seems like it might
> > > mean we stop clocking the DAI?  If that's the case it's better to just
> > > not have the mute control and allow the user to just control these as
> > > normal mutes.
> > > 
> > I am going to put slave mode support in, but I may need some time to
> > test how it works.
> > 
> Okay I had to refresh my memory on why slave was not being supported.
> Our slave mode needs the audio clocks to stay active to avoid pops. This
> has something to do with how the charge pump is setup. In master mode
> this is not an issue. I will keep the slave mode support out of the next
> version.
> 
> Also I am not sure what you mean with the mute controls. Could you
> elaborate more on this?
> 
Okay I belive I know what this was all about. 

Were you asking why the mute functions are powering up/down the PLLs?

The answer is that is was a vestige of the very first version of the
driver where I was told I needed to power PLLs before unmuting and
muting before powering them down, which is correct I just didn't have
them in the correct callback. In the next version I have moved the
powering of the PLLs into the event call back of a DAPM supply widget.
If there is a better way I can implement that.

The previous comment on slave mode is still valid, so the next version
will only suppor the codec in master mode.

Sorry for any confusion that I may have caused.