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

["Wu, Songjun" <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>]

On 9/3/2015 19:37, Mark Brown wrote:
> 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".
>
Thank you, the code will be modified as you suggest.

>> +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".
>
Yes, it also will be modified.

>> +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.
>
The Volume control with TLV information is not suitable for this case.
Bass, Medium, and treble are mutually exclusive.
So I think the SOC_ENUM control is suitable for this case.
The register layout is not very good,
The register is defined as below.
•  EQCFG: Equalization Selection
Value Name       Description
0     FLAT       Flat Response
1     BBOOST12   Bass boost +12 dB
2     BBOOST6    Bass boost +6 dB
3     BCUT12     Bass cut -12 dB
4     BCUT6      Bass cut -6 dB
5     MBOOST3    Medium boost +3 dB
6     MBOOST8    Medium boost +8 dB
7     MCUT3      Medium cut -3 dB
8     MCUT8      Medium cut -8 dB
9     TBOOST12   Treble boost +12 dB
10    TBOOST6    Treble boost +6 dB
11    TCUT12     Treble cut -12 dB
12    TCUT6      Treble cut -6 dB

>> +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.
>
Since the classD IP defines two register fields to control left volume 
and right volume respectively, I think it's better to provide two 
controls to user.

>> +static const char * const pwm_type[] = {
>> +	"single-ended", "differential"
>> +};
>
> The normal style for ALSA controls is to capitalise strings so
> "Single ended" and "Differential".
>
Accept. It will be modified.

>> +	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).
>
Accept. A warning will be added if the user trys to sepcify an invalid 
value. This function is optional, So if the user did not specify any 
value, this function will be disabled.

>> +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()?
>
Thank you for your reminder, it's better to use dev_get_regmap().
The code will be modified.

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

>> +	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.
>
This information will occur only once when linux kernel starts.
It shows the classD is loaded to linux kernel.
I think it's better to provide more information to user.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html