Re: [PATCH v2 1/5] ASoC: add mt6351 codec driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3244 bytes --]

On Mon, Apr 16, 2018 at 08:32:48AM +0800, KaiChieh Chuang wrote:

This looks pretty good, a couple of small things but nothing major here:

> +	offset = idx > old_idx ? idx - old_idx : old_idx - idx;
> +	reg_idx = old_idx;

These ternery statements would probably be clearer as regular if
statements.

> +/* dl pga gain */
> +static const char *const dl_pga_gain[] = {
> +	"8Db", "7Db", "6Db", "5Db", "4Db",
> +	"3Db", "2Db", "1Db", "0Db", "-1Db",
> +	"-2Db", "-3Db", "-4Db", "-5Db", "-6Db",
> +	"-7Db", "-8Db", "-9Db", "-10Db", "-40Db",
> +};

These gains should be put in TLVs rather than an enum so userspace can
handle them (also it should be dB not Db).  You can handle irregular
step sizes like these with DECLARE_TLV_DB_SCALE(), there's several
examples in the code already.

> +static int dl_pga_get(struct snd_kcontrol *kcontrol,
> +		      struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
> +	struct mt6351_priv *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	ucontrol->value.integer.value[0] = priv->ana_gain[kcontrol->id.device];
> +
> +	if (ucontrol->value.integer.value[0] == 0x1f)	/* reg idx for -40dB*/
> +		ucontrol->value.integer.value[0] = ARRAY_SIZE(dl_pga_gain) - 1;

Why do this rewriting?

> +/* ul pga gain */
> +static const char *const ul_pga_gain[] = {
> +	"0Db", "6Db", "12Db", "18Db", "24Db", "30Db",
> +};

This should be a regular control with a TLV too.

> +static const struct snd_kcontrol_new mt6351_snd_ul_controls[] = {
> +	MT_SOC_ENUM_EXT_ID("Audio_PGA1_Setting", ul_pga_enum[0],
> +			   ul_pga_get, ul_pga_set,
> +			   AUDIO_ANALOG_VOLUME_MICAMP1),

Volume controls should have names ending with " Volume" so userspace
knows how to handle them.

> +static int mt6351_codec_probe(struct snd_soc_codec *codec)
> +{
> +	struct mt6351_priv *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	/* add codec controls */
> +	snd_soc_add_codec_controls(codec,
> +				   mt6351_snd_controls,
> +				   ARRAY_SIZE(mt6351_snd_controls));
> +	snd_soc_add_codec_controls(codec,
> +				   mt6351_snd_ul_controls,
> +				   ARRAY_SIZE(mt6351_snd_ul_controls));
> +
> +	mt6351_codec_init_reg(codec);
> +
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL] = 8;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR] = 8;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP1] = 3;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP2] = 3;
> +
> +	return 0;
> +}

Can we read the configuration of the device back from the hardware?
It's better to just use the defaults rather than set things up for a
particular use case, that way there's a standard that can be agreed even
if it's not good for every use case.

> +static struct snd_soc_codec_driver mt6351_soc_codec_driver = {
> +	.probe = mt6351_codec_probe,
> +	.get_regmap = mt6351_get_regmap,

We're just about to remove CODEC drivers entirely and replace them with
components - nothing else is using the get_regmap() callback.  Do you
really need that callback, if you do we should just add it to the
component interface?

> +	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);

This should be the default behaviour so I'm guessing you don't need the
callback?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]