Re: [PATCH v1] ASoc: tas2781: Add new Kontrol to set tas2563 digital gain

[Mark Brown]

On Fri, Jun 28, 2024 at 12:18:43PM +0800, Shenghao Ding wrote:

> -static int tas2781_force_fwload_get(struct snd_kcontrol *kcontrol,
> +static int tasdev_force_fwload_get(struct snd_kcontrol *kcontrol,
>  	struct snd_ctl_elem_value *ucontrol)
>  {
>  	struct snd_soc_component *component =
> @@ -118,7 +119,7 @@ static int tas2781_force_fwload_get(struct snd_kcontrol *kcontrol,
>  	return 0;
>  }
>  
> -static int tas2781_force_fwload_put(struct snd_kcontrol *kcontrol,
> +static int tasdev_force_fwload_put(struct snd_kcontrol *kcontrol,
>  	struct snd_ctl_elem_value *ucontrol)

Are these fwload changes really related to the rest of the change?  They
feel like they should be a separate patch, even if it's a peparatory one
for this.

> +static int tas2563_digital_gain_put(
> +	struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{

> +	for (i = 0; i < tas_dev->ndev; i++) {
> +		ret = tasdevice_dev_bulk_write(tas_dev, i, reg,
> +			(unsigned char *)tas2563_dvc_table[vol], 4);
> +		if (ret)
> +			dev_err(tas_dev->dev,
> +				"%s, set digital vol error in device %d\n",
> +				__func__, i);
> +	}
> +
> +out:
> +	mutex_unlock(&tas_dev->codec_lock);
> +	return ret;
> +}

This needs to return 1 if the value is being changed, the mixer-test
selftest will tell you about this and other issues.

> +static const struct snd_kcontrol_new tas2563_snd_controls[] = {
> +	SOC_SINGLE_RANGE_EXT_TLV("Speaker Digital Gain", TAS2563_DVC_LVL, 0,
> +		0, ARRAY_SIZE(tas2563_dvc_table) - 1, 0,
> +		tas2563_digital_gain_get, tas2563_digital_gain_put,
> +		tas2563_dvc_tlv),
>  };

Volume controls should end in Volume, again mixer-test will tell you
this - please run mixer-test.

>  static int tasdevice_codec_probe(struct snd_soc_component *codec)
>  {
>  	struct tasdevice_priv *tas_priv = snd_soc_component_get_drvdata(codec);
> +	int rc;
> +
> +	if (tas_priv->chip_id == TAS2781) {
> +	} else {

If you write this as a switch statement it'll be more extensible.

Attachment: signature.asc
Description: PGP signature