RE: [PATCH 2/2] ASoC: max98396: add amplifier driver

["Lee, RyanS" <RyanS.Lee@analog.com>]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Wednesday, April 20, 2022 9:18 AM
> To: Ryan Lee <ryan.lee.analog@gmail.com>
> Cc: lgirdwood@gmail.com; robh+dt@kernel.org; krzk+dt@kernel.org;
> perex@perex.cz; tiwai@suse.com; srinivas.kandagatla@linaro.org;
> ckeepax@opensource.cirrus.com; tanureal@opensource.cirrus.com;
> cy_huang@richtek.com; pierre-louis.bossart@linux.intel.com;
> drhodes@opensource.cirrus.com; pbrobinson@gmail.com;
> hdegoede@redhat.com; lukas.bulwahn@gmail.com;
> stephan@gerhold.net; arnd@arndb.de; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Lee, RyanS
> <RyanS.Lee@analog.com>
> Subject: Re: [PATCH 2/2] ASoC: max98396: add amplifier driver
> 
> [External]
> 
> On Fri, Apr 15, 2022 at 05:40:24PM -0700, Ryan Lee wrote:
> 
> This looks mostly good - some issues below but nothing structural.
> 
> > +	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> > +	case SND_SOC_DAIFMT_NB_NF:
> > +	case SND_SOC_DAIFMT_NB_IF:
> > +		break;
> 
> One of these must be wrong - the device needs to know if it's
> handling a normal or inverted frame clock, otherwise the audio will
> be corrupted.

Thanks for pointing this out.
BCLK and FSYNC clock polarity configuration was inappropriate.
I shall fix this.

> 
> > +static int max98396_mux_put(struct snd_kcontrol *kcontrol,
> > +			    struct snd_ctl_elem_value *ucontrol) {
> > +	struct snd_soc_component *component =
> > +		snd_soc_dapm_kcontrol_component(kcontrol);
> > +	struct snd_soc_dapm_context *dapm =
> snd_soc_dapm_kcontrol_dapm(kcontrol);
> > +	struct max98396_priv *max98396 =
> snd_soc_component_get_drvdata(component);
> > +	struct soc_enum *e = (struct soc_enum *)kcontrol-
> >private_value;
> > +	unsigned int *item = ucontrol->value.enumerated.item;
> > +	int reg, val;
> > +
> > +	if (item[0] >= e->items)
> > +		return -EINVAL;
> > +
> > +	val = snd_soc_enum_item_to_val(e, item[0]) << e->shift_l;
> > +
> > +	if (max98396->device_id == CODEC_TYPE_MAX98396)
> > +		reg = MAX98396_R2055_PCM_RX_SRC1;
> > +	else
> > +		reg = MAX98397_R2056_PCM_RX_SRC1;
> > +
> > +	regmap_update_bits(max98396->regmap, reg,
> > +			   MAX98396_PCM_RX_MASK, val);
> > +
> > +	snd_soc_dapm_mux_update_power(dapm, kcontrol, item[0],
> e, NULL);
> > +
> > +	return 0;
> > +}
> 
> This should return 1 if the value changed - you should get an error
> reported by mixer-test from selftests if you run them on a sound card
> with the driver.

Thanks. Shall fix this on the v2.

> 
> > +	/* L/R mix configuration */
> > +	if (max98396->device_id == CODEC_TYPE_MAX98396) {
> > +		regmap_write(max98396->regmap,
> > +			     MAX98396_R2055_PCM_RX_SRC1, 0x02);
> > +		regmap_write(max98396->regmap,
> > +			     MAX98396_R2056_PCM_RX_SRC2, 0x10);
> > +	} else {
> > +		regmap_write(max98396->regmap,
> > +			     MAX98397_R2056_PCM_RX_SRC1, 0x02);
> > +		regmap_write(max98396->regmap,
> > +			     MAX98397_R2057_PCM_RX_SRC2, 0x10);
> > +	}
> 
> Shouldn't these be user controllable?  Most of the setup being done
> here looks system specific, especially the routing stuff.

MAX98396 is a mono amplifer and 'DAI MUX Sel' mixer control is provided for
audio mono mix. The chip default configuration take the left channel only,
so I wanted to set (L+R)/2 as default. The user still can change this value.
0x2057 is an extra register for the left and the right channel selection.
The user do not need to control this register because 'DAI MUX Sel' already
supports same function. Thank you.