On Tue, Jun 24, 2025 at 09:07:39PM +0530, Niranjan H Y wrote:
This looks mostly good, a few issues below but none super substantial.
+static void process_one_interrupt(struct tac5x1x_priv *tac5x1x, s32 index,
s32 value)+{
- u32 map_count, i;
- const struct mask_to_txt *map_items;
- map_count = intr_info_list[index].count;
- map_items = intr_info_list[index].mask_str_map;
- for (i = 0; i < map_count; i++) {
if (value & map_items[i].mask)dev_err(tac5x1x->dev, "Interrupt %s detected\n",map_items[i].name);- }
+}
This should probably be a dev_dbg() at most if it's not for a specific interrupt.
- ret = regmap_multi_reg_read(tac5x1x->regmap,
tac5x1x->irqinfo.latch_regs,tac5x1x->irqinfo.latch_data, latch_count);- if (ret) {
dev_err(tac5x1x->dev,"interrupt: latch register read failed");return IRQ_HANDLED;- }
IRQ_NONE is probably a better choice for a failed read, that'll mean we retry (or shut the interrupt off if it fails continually).
+static s32 tac5x1x_get_GPIO1_gpio(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
- s32 val;
- val = snd_soc_component_read(component, TAC5X1X_GPIOVAL);
- ucontrol->value.integer.value[0] = !!(val & TAC5X1X_GPIO1_MON);
- return 0;
+};
As I said on the prior version the GPIOs should be exposed via gpiolib. Please just remove this, if you want to discuss it'd be better to make it a separate incremental patch so that it doesn't hold up the rest of the driver.
+/* PDM Data input pin Selection */ +static const char *const pdm_pin_text[] = {
- "Disable",
- "GPIO1",
- "GPIO2",
- "GPI1",
+};
This looks like something that should be configured via the DT rather than from userspace, I'd expected this will be determined by the hardware design. It looks like the DT bit is already there so the controls should just be dropped.
+static s32 tac5x1x_set_dai_fmt(struct snd_soc_dai *codec_dai, u32 fmt) +{
- struct snd_soc_component *component = codec_dai->component;
- s32 iface_reg_1 = 0;
- s32 iface_reg_2 = 0;
- s32 iface_reg_3 = 0;
- int right_slot = 1;
- dev_info(component->dev, "[tac5x1x] %s(), fmt=%d\n", __func__, fmt);
There shouldn't be any dev_info() or higher messages in the normal playback/record flow. Error reports are fine, but when things are working the logs should be quiet so we don't obscure problems.
+static s32 tac5x1x_reset(struct snd_soc_component *component) +{
- s32 ret, index;
- ret = snd_soc_component_write(component, TAC5X1X_RESET, 1);
- if (ret < 0)
return ret;- /* Wait >= 10 ms after entering sleep mode. */
- usleep_range(10000, 100000);
- for (index = 0; index < ARRAY_SIZE(tac5x1x_reg_defaults); index++) {
ret = snd_soc_component_write(component,tac5x1x_reg_defaults[index].reg,tac5x1x_reg_defaults[index].def);if (ret < 0)return ret;- }
You can use regcache_drop_region() to discard the cache without having to write out all the values to the hardware, that should speed things up a lot.
- if (!(tac5x1x->codec_type == TAD5212 ||
tac5x1x->codec_type == TAD5112)) {ret =snd_soc_dapm_new_controls(dapm, tac5x1x_common_widgets,ARRAY_SIZE(tac5x1x_common_widgets));if (ret)return ret;ret = snd_soc_dapm_add_routes(dapm, tac_common_routes,ARRAY_SIZE(tac_common_routes));if (ret)return ret;- } else {
It's better to use switch statements for these, it's easier to extend for new chips.
+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);
- ret = tac5x1x_probe(&i2c->dev, regmap, type);
- if (ret)
dev_err(&i2c->dev, "probe failed");- return ret;
+}
It'd be better to move the resets, regulator gets and pin configuration to the I2C probe - that means that even if the sound card doesn't start for some reason the chip will be in a known good state, and if it takes some deferred probes for the card to come up we won't be bouncing the chip on and off.