[PATCH v1] ASoC: tas2781: Add "Acoustic Tuning" kcontrol for acoustic application

"Acoustic Tuning" kcontrol is a bridge to the acoustic tuning application tool which can tune the chips' acoustic effect.
Signed-off-by: Shenghao Ding shenghao-ding@ti.com --- include/sound/tas2781.h | 13 ++++ sound/soc/codecs/tas2781-i2c.c | 138 ++++++++++++++++++++++++++++++++- 2 files changed, 149 insertions(+), 2 deletions(-)
diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h index eff011444cc8..a9c236ab8cba 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -32,6 +32,8 @@ SNDRV_PCM_FMTBIT_S24_LE | \ SNDRV_PCM_FMTBIT_S32_LE)
+#define TASDEV_DATA_PAYLOAD_SIZE 128 + /* PAGE Control Register (available in page0 of each book) */ #define TASDEVICE_PAGE_SELECT 0x00 #define TASDEVICE_BOOKCTL_PAGE 0x00 @@ -159,10 +161,21 @@ struct calidata { unsigned int cali_dat_sz_per_dev; };
+struct acoustic_data { + unsigned char len; + unsigned char id; + unsigned char addr; + unsigned char book; + unsigned char page; + unsigned char reg; + unsigned char data[TASDEV_DATA_PAYLOAD_SIZE]; +}; + struct tasdevice_priv { struct tasdevice tasdevice[TASDEVICE_MAX_CHANNELS]; struct tasdevice_rca rcabin; struct calidata cali_data; + struct acoustic_data acou_data; struct tasdevice_fw *fmw; struct gpio_desc *speaker_id; struct gpio_desc *reset; diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c index b950914b7d48..0d52cfc12743 100644 --- a/sound/soc/codecs/tas2781-i2c.c +++ b/sound/soc/codecs/tas2781-i2c.c @@ -328,6 +328,24 @@ static int tasdev_cali_data_get(struct snd_kcontrol *kcontrol, return 0; }
+static int tasdev_acoustic_data_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol); + struct tasdevice_priv *priv = snd_soc_component_get_drvdata(comp); + struct soc_bytes_ext *bytes_ext = + (struct soc_bytes_ext *) kcontrol->private_value; + unsigned char *dst = ucontrol->value.bytes.data; + struct acoustic_data *p = &priv->acou_data; + + if (p->id == 'r' && p->len <= bytes_ext->max) + memcpy(dst, p, p->len); + else + dev_err(priv->dev, "Not ready for get.\n"); + + return 0; +} + static int calib_data_get(struct tasdevice_priv *tas_priv, int reg, unsigned char *dst) { @@ -700,6 +718,84 @@ static int tasdev_tf_data_get(struct snd_kcontrol *kcontrol, return calib_data_get(tas_priv, reg, &dst[1]); }
+static int tasdev_acoustic_data_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol); + struct tasdevice_priv *priv = snd_soc_component_get_drvdata(comp); + struct soc_bytes_ext *bytes_ext = + (struct soc_bytes_ext *) kcontrol->private_value; + unsigned char *src = ucontrol->value.bytes.data; + int j, len, ret, value, reg; + unsigned short chn; + + if (src[0] > bytes_ext->max) { + dev_err(priv->dev, "pkg len(%u) is larger than max(%d).\n", + src[0], bytes_ext->max); + return 0; + } + + switch (src[1]) { + case 'r': + /* length of data to read */ + len = src[6]; + break; + case 'w': + /* Skip 6 bytes for package type and register address */ + len = src[0] - 6; + break; + default: + dev_err(priv->dev, "%s Wrong code %02x.\n", __func__, src[1]); + return 0; + } + + if (len < 1) { + dev_err(priv->dev, "pkg fmt invalid %02x.\n", len); + return 0; + } + + for (j = 0; j < priv->ndev; j++) + if (src[2] == priv->tasdevice[j].dev_addr) { + chn = j; + break; + } + if (j >= priv->ndev) { + dev_err(priv->dev, "no such device 0x%02x.\n", src[2]); + return 0; + } + + reg = TASDEVICE_REG(src[3], src[4], src[5]); + + guard(mutex)(&priv->codec_lock); + + if (src[1] == 'w') { + if (len > 1) + ret = tasdevice_dev_bulk_write(priv, chn, reg, + &src[6], len); + else + ret = tasdevice_dev_write(priv, chn, reg, src[6]); + } else { + struct acoustic_data *p = &priv->acou_data; + + memcpy(p, src, 6); + if (len > 1) { + ret = tasdevice_dev_bulk_read(priv, chn, reg, + p->data, len); + } else { + ret = tasdevice_dev_read(priv, chn, reg, &value); + p->data[0] = value; + } + p->len = len + 6; + } + + if (ret) { + dev_err(priv->dev, "i2c communication error.\n"); + return 0; + } + + return 1; +} + static int tasdev_re_data_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { @@ -1043,10 +1139,11 @@ static int tasdevice_get_chip_id(struct snd_kcontrol *kcontrol, static int tasdevice_create_control(struct tasdevice_priv *tas_priv) { struct snd_kcontrol_new *prof_ctrls; - int nr_controls = 1; + struct soc_bytes_ext *ext_acoustic; + char *acoustic_name, *name; + int nr_controls = 2; int mix_index = 0; int ret; - char *name;
prof_ctrls = devm_kcalloc(tas_priv->dev, nr_controls, sizeof(prof_ctrls[0]), GFP_KERNEL); @@ -1068,6 +1165,43 @@ static int tasdevice_create_control(struct tasdevice_priv *tas_priv) prof_ctrls[mix_index].put = tasdevice_set_profile_id; mix_index++;
+ ext_acoustic = devm_kzalloc(tas_priv->dev, sizeof(*ext_acoustic), + GFP_KERNEL); + if (!ext_acoustic) + return -ENOMEM; + + /* + * This kcontrol is a bridge to the acoustic tuning application + * tool which can tune the chips' acoustic effect. + */ + acoustic_name = devm_kstrdup(tas_priv->dev, "Acoustic Tuning", + GFP_KERNEL); + if (!acoustic_name) + return -ENOMEM; + /* + * package structure for PPC3 communications: + * Pkg len (1 byte) + * Pkg id (1 byte, 'r' or 'w') + * Dev id (1 byte, i2c address) + * Book id (1 byte) + * Page id (1 byte) + * Reg id (1 byte) + * switch (pkg id) { + * case 'w': + * 1 byte, length of data to read + * case 'r': + * data payload (1~128 bytes) + * } + */ + ext_acoustic->max = sizeof(tas_priv->acou_data); + prof_ctrls[mix_index].name = acoustic_name; + prof_ctrls[mix_index].iface = SNDRV_CTL_ELEM_IFACE_MIXER; + prof_ctrls[mix_index].info = snd_soc_bytes_info_ext; + prof_ctrls[mix_index].put = tasdev_acoustic_data_put; + prof_ctrls[mix_index].get = tasdev_acoustic_data_get; + prof_ctrls[mix_index].private_value = (unsigned long)ext_acoustic; + mix_index++; + ret = snd_soc_add_component_controls(tas_priv->codec, prof_ctrls, nr_controls < mix_index ? nr_controls : mix_index);

On Thu, Apr 17, 2025 at 06:47:53PM +0800, Shenghao Ding wrote:
"Acoustic Tuning" kcontrol is a bridge to the acoustic tuning application tool which can tune the chips' acoustic effect.
- reg = TASDEVICE_REG(src[3], src[4], src[5]);
- guard(mutex)(&priv->codec_lock);
- if (src[1] == 'w') {
if (len > 1)
ret = tasdevice_dev_bulk_write(priv, chn, reg,
&src[6], len);
else
ret = tasdevice_dev_write(priv, chn, reg, src[6]);
So, this is basically just unrestricted register I/O to the device. That's not a great interface - it just lets userspace do anything, including conflicting with things that the driver is managing itself. Usually a coefficient interface would just expose a specific set of registers with the coefficient data as a control.
I know other CODEC vendors use the regmap debugfs interface for this, if you change the kernel to define REGMAP_ALLOW_WRITE_DEBUGFS then you can do both writes and reads for the tuning. The tuning tool then produces a file with just the coefficients which is loaded at runtime, with anything that's normal routing or whatever being configured via the control API.

Hi Brownie. Thanks for your comments, my question is inline.
-----Original Message----- From: Mark Brown broonie@kernel.org Sent: Thursday, April 17, 2025 8:32 PM To: Ding, Shenghao shenghao-ding@ti.com Cc: andriy.shevchenko@linux.intel.com; tiwai@suse.de; 13916275206@139.com; 13564923607@139.com; alsa-devel@alsa- project.org; Xu, Baojun baojun.xu@ti.com Subject: [EXTERNAL] Re: [PATCH v1] ASoC: tas2781: Add "Acoustic Tuning" kcontrol for acoustic application
On Thu, Apr 17, 2025 at 06:47:53PM +0800, Shenghao Ding wrote:
"Acoustic Tuning" kcontrol is a bridge to the acoustic tuning application tool which can tune the chips' acoustic effect.
- reg = TASDEVICE_REG(src[3], src[4], src[5]);
- guard(mutex)(&priv->codec_lock);
- if (src[1] == 'w') {
if (len > 1)
ret = tasdevice_dev_bulk_write(priv, chn, reg,
&src[6], len);
else
ret = tasdevice_dev_write(priv, chn, reg, src[6]);
So, this is basically just unrestricted register I/O to the device. That's not a great interface - it just lets userspace do anything, including conflicting with things that the driver is managing itself. Usually a coefficient interface would just expose a specific set of registers with the coefficient data as a control.
I know other CODEC vendors use the regmap debugfs interface for this, if you change the kernel to define REGMAP_ALLOW_WRITE_DEBUGFS then you can do both writes and reads for the tuning. The tuning tool then produces a file with just the coefficients which is loaded at runtime, with anything that's normal routing or whatever being configured via the control API.
REGMAP_ALLOW_WRITE_DEBUGFS is a good option for the Acoustic Tool. But in the case of multiple pieces of tas2781, our devicetree for such case is an aggregated one: audio-codec@38 { compatible = "ti,tas2781"; reg = <0x38>, /* dev0 for Audio slot 0 */ <0x3a>, /* dev1 for Audio slot 1 */ <0x39>, /* dev2 for Audio slot 2 */ <0x3b>; /* dev3 for Audio slot 3 */
#sound-dai-cells = <0>; reset-gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>; interrupt-parent = <&gpio1>; interrupts = <15>; }; There creates only one device node for 0x38 device in debug/regmap. How to switch the i2c address to other devices(such as dev1, dev2 or dev3)?

On Fri, Apr 18, 2025 at 10:18:59AM +0000, Ding, Shenghao wrote:
So, this is basically just unrestricted register I/O to the device. That's not a great interface - it just lets userspace do anything, including conflicting with things that the driver is managing itself. Usually a coefficient interface would just expose a specific set of registers with the coefficient data as a control.
I know other CODEC vendors use the regmap debugfs interface for this, if you change the kernel to define REGMAP_ALLOW_WRITE_DEBUGFS then you can do both writes and reads for the tuning. The tuning tool then produces a file with just the coefficients which is loaded at runtime, with anything that's normal routing or whatever being configured via the control API.
REGMAP_ALLOW_WRITE_DEBUGFS is a good option for the Acoustic Tool. But in the case of multiple pieces of tas2781, our devicetree for such case is an aggregated one: audio-codec@38 { compatible = "ti,tas2781"; reg = <0x38>, /* dev0 for Audio slot 0 */ <0x3a>, /* dev1 for Audio slot 1 */ <0x39>, /* dev2 for Audio slot 2 */ <0x3b>; /* dev3 for Audio slot 3 */
#sound-dai-cells = <0>; reset-gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>; interrupt-parent = <&gpio1>; interrupts = <15>; };
There creates only one device node for 0x38 device in debug/regmap. How to switch the i2c address to other devices(such as dev1, dev2 or dev3)?
Right, the combination within the driver rather than combining things at the subsystem level is causing you trouble here. It's sufficiently non standard that you probably do need to open code something here, but the broad point that we shouldn't have this unrestricted write feature available in production kernels remains - something similar to what regmap is doing for example.

Thanks, Broonie. New question from one of my customers. Looking forward to your comments. Thanks.
-----Original Message----- From: Mark Brown broonie@kernel.org Sent: Friday, April 18, 2025 11:53 PM To: Ding, Shenghao shenghao-ding@ti.com Cc: andriy.shevchenko@linux.intel.com; tiwai@suse.de; 13916275206@139.com; 13564923607@139.com; alsa-devel@alsa-project.org; Xu, Baojun baojun.xu@ti.com Subject: Re: [EXTERNAL] Re: [PATCH v1] ASoC: tas2781: Add "Acoustic Tuning" kcontrol for acoustic application
On Fri, Apr 18, 2025 at 10:18:59AM +0000, Ding, Shenghao wrote:
So, this is basically just unrestricted register I/O to the device. That's not a great interface - it just lets userspace do anything, including conflicting with things that the driver is managing itself. Usually a coefficient interface would just expose a specific set of registers with the coefficient data as a control.
I know other CODEC vendors use the regmap debugfs interface for this, if you change the kernel to define REGMAP_ALLOW_WRITE_DEBUGFS then you can do both writes and reads for the tuning. The tuning tool then produces a file with just the coefficients which is loaded at runtime, with anything that's normal routing or whatever being
configured via the control API.
REGMAP_ALLOW_WRITE_DEBUGFS is a good option for the Acoustic Tool. But in the case of multiple pieces of tas2781, our devicetree for such case is an aggregated one: audio-codec@38 { compatible = "ti,tas2781"; reg = <0x38>, /* dev0 for Audio slot 0 */ <0x3a>, /* dev1 for Audio slot 1 */ <0x39>, /* dev2 for Audio slot 2 */ <0x3b>; /* dev3 for Audio slot 3 */
#sound-dai-cells = <0>; reset-gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>; interrupt-parent = <&gpio1>; interrupts = <15>; };
There creates only one device node for 0x38 device in debug/regmap. How to switch the i2c address to other devices(such as dev1, dev2 or dev3)?
Right, the combination within the driver rather than combining things at the subsystem level is causing you trouble here. It's sufficiently non standard that you probably do need to open code something here, but the broad point that we shouldn't have this unrestricted write feature available in production kernels remains - something similar to what regmap is doing for example.
May I create a debugfs node in the driver for "Acoustic tuning"?

On Mon, Apr 21, 2025 at 12:52:54AM +0000, Ding, Shenghao wrote:
Thanks, Broonie. New question from one of my customers.
Right, the combination within the driver rather than combining things at the subsystem level is causing you trouble here. It's sufficiently non standard that you probably do need to open code something here, but the broad point that we shouldn't have this unrestricted write feature available in production kernels remains - something similar to what regmap is doing for example.
May I create a debugfs node in the driver for "Acoustic tuning"?
Sure, something in debugfs is fine - it's just the unrestricted writes in production thing that was a concern.
participants (3)
-
Ding, Shenghao
-
Mark Brown
-
Shenghao Ding