From: Mark Brown broonie@kernel.org Sent: Friday, June 6, 2025 6:17 PM Subject: Re: [PATCH v1 1/4] ASoc: tac5x1x: add codec driver tac5x1x family

On Fri, Jun 06, 2025 at 12:21:33PM +0530, Niranjan H Y wrote:

...

There's a few issues below but nothing that's *hugely* structural, the bulk of the code here looks good.

Thank you for your time and review comments. I will add fixes for most the comments in the next patch. Request you to take a look at some comments below.

...

+config SND_SOC_TAC5X1X

• tristate "Texas Instruments TAC5X1X family codecs"
• depends on I2C && REGMAP_I2C

+config SND_SOC_TAC5X1X_I2C

• tristate "Texas Instruments TAC5X1X family driver based on I2C"
• depends on I2C && REGMAP_I2C
• select SND_SOC_TAC5X1X

You need to select REGMAP_I2C if you use it, it can't be turned on independently. If the device is I2C only then there's no need to have the second option for I2C, that's used for devices that support both I2C and SPI to avoid problems with dependencies on the I2C and SPI core code.

Many of these devices support SPI interface as well. So I thought I will make library and I2C interface file. But currently SPI support is not added in the driver. Is this okay to still keep this ? please suggest.

...

...

+static const char * const out_ch2_ltch[] = {

• "OUT_CH2 OUT2P Short circuit Fault",
• "OUT_CH2 OUT2M Short circuit Fault",
• "OUT_CH2 DRVRP Virtual Ground Fault",
• "OUT_CH2 DRVRM Virtual ground Fault",
• "Reserved",
• "Reserved",
• "AREG SC Fault Mask",
• "AREG SC Fault",

+};

For ones where the reserved values are in the middle of the set of values you can use _VAL_ENUM() which lets you skip over the values that are invalid.

Is there some example which I can refer? I could not find anything in the kernel source with _VAL_ENUM. I will try to refactor to avoid this.

...

+static s32 tac5x1x_regmap_write(struct tac5x1x_priv *tac5x1x,

• 		u32 reg, u32 value)
  

+{

• s32 ret;
• s32 retry_count = 5;
• while (retry_count--) {

• ret = regmap_write(tac5x1x->regmap, reg,
  

• 		   value);
  

• if (ret >= 0)
  

• 	break;
  

• usleep_range(5000, 5050);
  

• }
• if (retry_count == -1)

• return 3;
  

• else

• return ret;
  

+}

Is the hardware genuinely so unstable that we need to retry all the I/O? This seems really concerning.

I think this can be dropped as this is used for some legacy devices for some customer platforms. I will remove this in next patch and redo the tests.

...

+static s32 tac5x1x_set_GPO1_gpio(struct snd_kcontrol *kcontrol,

• 		 struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component =

snd_kcontrol_chip(kcontrol);

...

• s32 gpio_check, val;
• val = snd_soc_component_read(component, TAC5X1X_GPO1);
• gpio_check = ((val & TAC5X1X_GPIOX_CFG_MASK) >> 0);
• if (gpio_check != TAC5X1X_GPIO_GPO) {

• dev_err(component->dev,
  

• 	"%s: GPO1 is not configure as a GPO output\n",
  

• 	__func__);
  

• return -EINVAL;
  

• }
• if (ucontrol->value.integer.value[0])

• val = 0;
  

• else

• val = TAC5X1X_GPO1_VAL;
  

This seems to be exposing a GPIO directly to userspace, which will prevent using that GPIO with other kernel subsystems. It would be better to expose this via gpiolib, then if userspace control is needed that can be done through gpiolib.

Thanks for the suggestion. But, in our case, codec chip has some GPIO pins which are specific to the codec. We do not intend to expose to the other kernel subsystem. It is more of codec chip configuration to use these GPIO pins as - for e.g. daisy-chain input, audio data output, PLL input clock source, interrupt, digital audio input(PDM) etc. Depending on the hardware connection, we need to configure some registers so that the pin can function as any one of the features which mentioned in the data sheet. More info in section "5 Pin Configuration and Functions" & "Table 7-70. Multifunction Pin Assignments" in the data sheet. https://www.ti.com/lit/ds/symlink/tac5212.pdf If it feels right, I can make this also as part of the dts configuration. Please suggest if I still need to expose this as gpiolib.

...

+static const char *const tac5x1x_slot_select_text[] = {

• "Slot 0", "Slot 1", "Slot 2", "Slot 3",
• "Slot 4", "Slot 5", "Slot 6", "Slot 7",
• "Slot 8", "Slot 9", "Slot 10", "Slot 11",
• "Slot 12", "Slot 13", "Slot 14", "Slot 15",
• "Slot 16", "Slot 17", "Slot 18", "Slot 19",
• "Slot 20", "Slot 21", "Slot 22", "Slot 23",
• "Slot 24", "Slot 25", "Slot 26", "Slot 27",
• "Slot 28", "Slot 29", "Slot 30", "Slot 31",

+};

TDM slot control would usually be done via set_tdm_slot().

We need the slots to be configurable in case we need to swap capture channels. Is it okay to keep this ?

...

+static const char *const out2x_vcom_text[] = {

• "0.6 * Vref",
• "AVDD by 2",

+};

This can be device tree setting. I will move it.

...

+static const char *const diag_cfg_text[] = {

• "0mv", "30mv", "60mv", "90mv",
• "120mv", "150mv", "180mv", "210mv",
• "240mv", "270mv", "300mv", "330mv",
• "360mv", "390mv", "420mv", "450mv",

+};

+static const char *const diag_cfg_gnd_text[] = {

• "0mv", "60mv", "120mv", "180mv",
• "240mv", "300mv", "360mv", "420mv",
• "480mv", "540mv", "600mv", "660mv",
• "720mv", "780mv", "840mv", "900mv",

+};

Are these controls that should be device tree data?

We need these values to be configurable via mixer controls.

Thanks, Niranjan H Y