On Tue, Sep 01, 2015 at 01:41:40PM +0800, Songjun Wu wrote:Thank you, the code will be modified as you suggest.
+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".
Yes, it also will be modified.+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".
The Volume control with TLV information is not suitable for this case.+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.
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 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.
Accept. It will be modified.+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. 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.+ 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).
Thank you for your reminder, it's better to use dev_get_regmap().+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()?
Accept.+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.
This information will occur only once when linux kernel starts.+ 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.