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:

tac5x1x family are series of low-power and high performance mono/stereo audio codecs consists of ADC and DAC combinations. The patch adds supports for Codecs(DAC & ADC), ADC only and DAC only configurations available in the tac5x1x family.

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

+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.

snd-soc-tas2781-i2c-y := tas2781-i2c.o +snd-soc-tac5x1x-y := tac5x1x.o +snd-soc-tac5x1x-i2c-y := tac5x1x-i2c.o snd-soc-tfa9879-y := tfa9879.o

Please keep these files alphanumerically sorted.

obj-$(CONFIG_SND_SOC_LPASS_TX_MACRO) += snd-soc-lpass-tx-macro.o

# Mux -obj-$(CONFIG_SND_SOC_SIMPLE_MUX) += snd-soc-simple-mux.o \ No newline at end of file +obj-$(CONFIG_SND_SOC_SIMPLE_MUX) += snd-soc-simple-mux.o diff --git a/sound/soc/codecs/tac5x1x-i2c.c b/sound/soc/codecs/tac5x1x-i2c.c

Looks like this whitespace change crept in accidentally.

+static int tac5x1x_i2c_probe(struct i2c_client *i2c) +{

• int ret;
• enum tac5x1x_type type;
• struct regmap *regmap;
• const struct regmap_config *config = &tac5x1x_regmap;
• regmap = devm_regmap_init_i2c(i2c, config);
• type = (uintptr_t)i2c_get_match_data(i2c);
• dev_info(&i2c->dev, "probing %s codec_type = %d\n",

•  i2c->name, type);
  

Just drop this print, it's probably a bit noisy.

+static const char * const int_ltch0[] = {

• "Clock Error",
• "PLL Lock",
• "Boost Over Temperature",
• "Boost Over Current",
• "Boost Mode",
• "Reserved",
• "Reserved",
• "Reserved",

+};

You can just set the maximum value for the enum to be whatever the maximum valid value is and then not need to list the Reserved entries at all.

+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.

+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.

+static s32 tac5x1x_regmap_read(struct tac5x1x_priv *tac5x1x,

• 	       u32 reg, u32 *value)
  

+{

• s32 ret;
• s32 retry_count = 5;
• ret = regmap_reinit_cache(tac5x1x->regmap, &tac5x1x_regmap);
• if (ret) {

• dev_err(tac5x1x->dev, "Failed to reinit reg cache\n");
  

• return ret;
  

• }
• while (retry_count--) {

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

• 		  value);
  

• if (ret >= 0)
  

• 	break;
  

• usleep_range(5000, 5050);
  

• }

This seems *really* scary and confusing, why would we reset the register cache in what looks like a normal read operation.

+static s32 tac5x1x_dev_read(struct tac5x1x_priv *tac5x1x,

• 	    u32 dev_no, u32 reg,
  

• 	    u32 *ref_value)
  

+{

• s32 ret;
• guard(mutex)(&tac5x1x->dev_lock);
• if (dev_no < tac5x1x->ndev) {

• ret = tac5x1x_regmap_write(tac5x1x,
  

• 			   TAC_PAGE_SELECT, 0);
  

• if (ret < 0) {
  

• 	dev_err(tac5x1x->dev, "%s, E=%d\n",
  

• 		__func__, ret);
  

• 	return ret;
  

• }
  

This doesn't actually appear to do anything to support or choose between multiple devices? The code all looks like it only supports one device, and this is only used in the interrupt handler.

+static void tac5x1x_irq_work_func(struct tac5x1x_priv *tac5x1x) +{

• u32 reg_val, array_size, i, index = 0, bit = 0;
• s32 rc;
• tac5x1x_enable_irq(tac5x1x, false);
• array_size = ARRAY_SIZE(int_reg_array);
• for (i = 0; i < array_size; i++) {

• rc = tac5x1x_dev_read(tac5x1x, index,
  

• 		      int_reg_array[i], &reg_val);
  

...

• }
• tac5x1x_enable_irq(tac5x1x, true);

The interrupt disabling here seems odd - what's the story there? I see this is a work function rather than an irq thread to add some delay and introduce some debouncing but it's not clear why the disable during handling.

+const struct regmap_config tac5x1x_regmap = {

• .max_register = 12 * 128,

+EXPORT_SYMBOL(tac5x1x_regmap);

EXPORT_SYMBOL_GPL().

+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.

+/* Impedance Selection */ +static const char *const resistor_text[] = {

• "5 kOhm",
• "10 kOhm",
• "40 kOhm",

+};

+static SOC_ENUM_SINGLE_DECL(adc1_resistor_enum, TAC5X1X_ADCCH1C0, 4,

• resistor_text);
  

+static SOC_ENUM_SINGLE_DECL(adc2_resistor_enum, TAC5X1X_ADCCH2C0, 4,

• resistor_text);
  

Is this something that would be adjusted at runtime, or should it be a device tree setting?

+static const char *const rx_ch3_asi_cfg_text[] = {

• "Disable",
  

• "DAC channel data",
  

+};

On/off switches should be plain controls ending in "Switch" rather than enums.

+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().

+static const char *const out2x_vcom_text[] = {

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

+};

+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?

• switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {

SND_SOC_DAIFMT_CLOCK_MASK.

+#ifdef CONFIG_PM +static s32 tac5x1x_soc_suspend(struct snd_soc_component *component) +{

• struct tac5x1x_priv *tac5x1x =

• snd_soc_component_get_drvdata(component);
  

• regcache_cache_only(tac5x1x->regmap, true);
• regcache_mark_dirty(tac5x1x->regmap);
• regulator_disable(tac5x1x->supply_avdd);
• regulator_disable(tac5x1x->supply_iovdd);

Consider using the regulator_bulk_ APIs - the current code is fine, but I don't see any cases where you're controlling the regulators separately so it'd be a little simpler and allows the core to do things like bring multiple regulators up in parallel.

+static s32 tac5x1x_soc_resume(struct snd_soc_component *component) +{

• struct tac5x1x_priv *tac5x1x =

• snd_soc_component_get_drvdata(component);
  

• s32 ret;
• regcache_cache_only(tac5x1x->regmap, false);
• snd_soc_component_cache_sync(component);
• ret = regulator_enable(tac5x1x->supply_avdd);
• if (ret) {

• regulator_disable(tac5x1x->supply_avdd);
  

• return ret;
  

• }
• ret = regulator_enable(tac5x1x->supply_iovdd);
• if (ret) {

• regulator_disable(tac5x1x->supply_iovdd);
  

• return ret;
  

• }

This will try to do the cache sync with the regulators disabled which won't work if they were actually turned off.