linux-kernel - Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code

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: <20150903113716.GU12027@sirena.org.uk>
Date:	Thu, 3 Sep 2015 12:37:16 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Songjun Wu <songjun.wu@...el.com>
Cc:	nicolas.ferre@...el.com, lgirdwood@...il.com, perex@...ex.cz,
	tiwai@...e.com, linux-kernel@...r.kernel.org,
	alsa-devel@...a-project.org, robh+dt@...nel.org,
	pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier
 code

On Tue, Sep 01, 2015 at 01:41:40PM +0800, Songjun Wu wrote:

> +static const char * const mono_text[] = {
> +	"stereo", "mono"
> +};
> +
> +static SOC_ENUM_SINGLE_DECL(classd_mono_enum,
> +			CLASSD_INTPMR, CLASSD_INTPMR_MONO_SHIFT,
> +			mono_text);

This looks like it should be a simple Switch control called something
like "Stereo Switch" or "Mono Switch".

> +static const char * const deemp_text[] = {
> +	"disabled", "enabled"
> +};
> +
> +static SOC_ENUM_SINGLE_DECL(classd_deemp_enum,
> +			CLASSD_INTPMR, CLASSD_INTPMR_DEEMP_SHIFT,
> +			deemp_text);

Similarly this looks like it should be "Deemph Switch".

> +static const char * const eqcfg_bass_text[] = {
> +	"-12 dB", "-6 dB", "0 dB", "+6 dB", "+12 dB"
> +};

> +static const unsigned int eqcfg_bass_value[] = {
> +	CLASSD_INTPMR_EQCFG_B_CUT_12,
> +	CLASSD_INTPMR_EQCFG_B_CUT_6, CLASSD_INTPMR_EQCFG_FLAT,
> +	CLASSD_INTPMR_EQCFG_B_BOOST_6, CLASSD_INTPMR_EQCFG_B_BOOST_12
> +};

This should be a Volume control with TLV information, as should the
following few controls.

> +static const struct snd_kcontrol_new atmel_classd_snd_controls[] = {
> +SOC_SINGLE_TLV("Left Volume", CLASSD_INTPMR,
> +		CLASSD_INTPMR_ATTL_SHIFT, 78, 1, classd_digital_tlv),
> +
> +SOC_SINGLE_TLV("Right Volume", CLASSD_INTPMR,
> +		CLASSD_INTPMR_ATTR_SHIFT, 78, 1, classd_digital_tlv),

This should be a single stereo control rather than separate left and
right controls.

> +static const char * const pwm_type[] = {
> +	"single-ended", "differential"
> +};

The normal style for ALSA controls is to capitalise strings so
"Single ended" and "Differential".

> +	if (pdata->non_overlap_enable) {
> +		val |= (CLASSD_MR_NON_OVERLAP_EN
> +			<< CLASSD_MR_NON_OVERLAP_SHIFT);
> +
> +		mask |= CLASSD_MR_NOVR_VAL_MASK;
> +		switch (pdata->non_overlap_time) {
> +		case 5:
> +			val |= (CLASSD_MR_NOVR_VAL_5NS
> +				<< CLASSD_MR_NOVR_VAL_SHIFT);
> +			break;
> +		case 10:
> +			val |= (CLASSD_MR_NOVR_VAL_10NS
> +				<< CLASSD_MR_NOVR_VAL_SHIFT);
> +			break;
> +		case 15:
> +			val |= (CLASSD_MR_NOVR_VAL_15NS
> +				<< CLASSD_MR_NOVR_VAL_SHIFT);
> +			break;
> +		case 20:
> +			val |= (CLASSD_MR_NOVR_VAL_20NS
> +				<< CLASSD_MR_NOVR_VAL_SHIFT);
> +			break;
> +		default:
> +			val |= (CLASSD_MR_NOVR_VAL_10NS
> +				<< CLASSD_MR_NOVR_VAL_SHIFT);
> +			break;
> +		}

I'd expect at least a warning if the user trys to specify an invalid
value (if they didn't specify any value then I'd expect the DT parsing
function to assign the default value).

> +static struct regmap *atmel_classd_codec_get_remap(struct device *dev)
> +{
> +	struct atmel_classd *dd = dev_get_drvdata(dev);
> +
> +	return dd->regmap;
> +}

Can you just use dev_get_regmap()?

> +static int atmel_classd_codec_dai_startup(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *codec_dai)
> +{
> +	struct atmel_classd *dd = snd_soc_dai_get_drvdata(codec_dai);
> +
> +	clk_prepare_enable(dd->aclk);
> +	clk_prepare_enable(dd->gclk);

Should check for errors here.

> +	dev_info(dev,
> +		"Atmel Class D Amplifier (CLASSD) device at 0x%p (irq %d)\n",
> +		io_base, dd->irq);

This is a bit noisy and not really based on interaction with the
hardware...  dev_dbg() seems better.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)