Re: [PATCH v11 2/4] ASoC: tac5xx2-sdw: add soundwire based codec driver

Pierre-Louis Bossart
2026-04-28T20:02:02Z

> +struct tac5xx2_prv {
> + struct snd_soc_component *component;
> + struct sdw_slave *sdw_peripheral;
> + struct sdca_function_data *sa_func_data;
> + struct sdca_function_data *sm_func_data;
> + struct sdca_function_data *uaj_func_data;
> + struct sdca_function_data *hid_func_data;
> + enum sdw_slave_status status;
> + /* serialize potential concurrent uaj detection in .hw_params
> + * during playback session and .interrupt_callback.
> + */
> + struct mutex uaj_lock;

is this uaj_lock really needed, if you look below at hw_params there's nothing that looks even remotely tied to jack detection...

> + struct regmap *regmap;
> + struct device *dev;
> + bool hw_init;
> + bool first_hw_init;
> + u32 part_id;
> + struct snd_soc_jack *hs_jack;
> + int jack_type;
> + /* Custom fw binary. UMP File Download is not used. */
> + unsigned int fw_file_cnt;
> + struct tac_fw_file *fw_files;
> + struct completion fw_caching_complete;
> + bool fw_dl_success;
> + u8 fw_binaryname[64];
> +};

> +static int tac_sdw_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct snd_soc_component *component = dai->component;
> + struct tac5xx2_prv *tac_dev = snd_soc_component_get_drvdata(component);
> + struct sdw_stream_config stream_config = {0};
> + struct sdw_port_config port_config = {0};
> + struct sdw_stream_runtime *sdw_stream;
> + struct sdw_slave *sdw_peripheral = tac_dev->sdw_peripheral;
> + int ret;
> + int function_id;
> + int pde_entity;
> + int port_num;
> + u8 sample_rate_idx = 0;
> +
> + if (!tac_dev->hw_init) {
> + dev_err(tac_dev->dev,
> + "error: operation without hw initialization");
> + return -EINVAL;
> + }
> +
> + sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> + if (!sdw_stream) {
> + dev_err(tac_dev->dev, "failed to get dma data");
> + return -EINVAL;
> + }
> +
> + ret = tac_clear_latch(tac_dev);
> + if (ret)
> + dev_warn(tac_dev->dev, "clear latch failed, err=%d", ret);
> +
> + switch (dai->id) {
> + case TAC5XX2_DMIC:
> + function_id = TAC_FUNCTION_ID_SM;
> + pde_entity = TAC_SDCA_ENT_PDE11;
> + port_num = TAC_SDW_PORT_NUM_DMIC;
> + break;
> + case TAC5XX2_UAJ:
> + function_id = TAC_FUNCTION_ID_UAJ;
> + pde_entity = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> + TAC_SDCA_ENT_PDE47 : TAC_SDCA_ENT_PDE34;
> + port_num = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> + TAC_SDW_PORT_NUM_UAJ_PLAYBACK :
> + TAC_SDW_PORT_NUM_UAJ_CAPTURE;
> + break;
> + case TAC5XX2_SPK:
> + function_id = TAC_FUNCTION_ID_SA;
> + pde_entity = TAC_SDCA_ENT_PDE23;
> + port_num = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> + TAC_SDW_PORT_NUM_SPK_PLAYBACK :
> + TAC_SDW_PORT_NUM_SPK_CAPTURE;
> + break;
> + default:
> + dev_err(tac_dev->dev, "Invalid dai id: %d", dai->id);
> + return -EINVAL;
> + }
> +
> + snd_sdw_params_to_config(substream, params, &stream_config, &port_config);
> + port_config.num = port_num;
> + ret = sdw_stream_add_slave(sdw_peripheral, &stream_config,
> + &port_config, 1, sdw_stream);
> + if (ret) {
> + dev_err(dai->dev,
> + "Unable to configure port %d: %d\n", port_num, ret);
> + return ret;
> + }
> +
> + switch (params_rate(params)) {
> + case 48000:
> + sample_rate_idx = 0x01;
> + break;
> + case 44100:
> + sample_rate_idx = 0x02;
> + break;
> + case 96000:
> + sample_rate_idx = 0x03;
> + break;
> + case 88200:
> + sample_rate_idx = 0x04;
> + break;
> + default:
> + dev_dbg(tac_dev->dev, "Unsupported sample rate: %d Hz",
> + params_rate(params));
> + return -EINVAL;
> + }
> +
> + switch (function_id) {
> + case TAC_FUNCTION_ID_SM:
> + ret = regmap_write(tac_dev->regmap,
> + SDW_SDCA_CTL(function_id, TAC_SDCA_ENT_CS113,
> + TAC_SDCA_CTL_CS_SAMP_RATE_IDX, 0),
> + sample_rate_idx);
> + if (ret) {
> + dev_err(tac_dev->dev, "Failed to set CS113 sample rate: %d", ret);
> + return ret;
> + }
> +
> + break;
> + case TAC_FUNCTION_ID_UAJ:
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + ret = regmap_write(tac_dev->regmap,
> + SDW_SDCA_CTL(function_id, TAC_SDCA_ENT_CS41,
> + TAC_SDCA_CTL_CS_SAMP_RATE_IDX, 0),
> + sample_rate_idx);
> + if (ret) {
> + dev_err(tac_dev->dev, "Failed to set CS41 sample rate: %d", ret);
> + return ret;
> + }
> + } else {
> + ret = regmap_write(tac_dev->regmap,
> + SDW_SDCA_CTL(function_id, TAC_SDCA_ENT_CS36,
> + TAC_SDCA_CTL_CS_SAMP_RATE_IDX, 0),
> + sample_rate_idx);
> + if (ret) {
> + dev_err(tac_dev->dev, "Failed to set CS36 sample rate: %d", ret);
> + return ret;
> + }
> + }
> + break;
> + case TAC_FUNCTION_ID_SA:
> + /* SmartAmp: no additional sample rate configuration needed */
> + break;
> + }
> +
> + ret = regmap_write(tac_dev->regmap, SDW_SDCA_CTL(function_id, pde_entity,
> + TAC_SDCA_REQUESTED_PS, 0), 0);
> + if (ret) {
> + dev_err(tac_dev->dev, "failed to set PS to 0: %d\n", ret);
> + return ret;
> + }
> +
> + ret = sdca_asoc_pde_poll_actual_ps(tac_dev->dev, tac_dev->regmap, function_id, pde_entity,
> + SDCA_PDE_PS3, SDCA_PDE_PS0, NULL, 0);
> + if (ret)
> + dev_err(tac_dev->dev,
> + "failed to transition to PS0, err= %d\n", ret);
> + return ret;

so as I said above there's nothing here that might conflict with the interrupt callback?
What am I missing?

> +}
> +
> +static s32 tac_sdw_pcm_hw_free(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + s32 ret;

style nit-pick: reverse xmas-tree

> + struct snd_soc_component *component = dai->component;
> + struct tac5xx2_prv *tac_dev =
> + snd_soc_component_get_drvdata(component);
> + struct sdw_stream_runtime *sdw_stream =
> + snd_soc_dai_get_dma_data(dai, substream);
> + int pde_entity, function_id;
> +
> + sdw_stream_remove_slave(tac_dev->sdw_peripheral, sdw_stream);
> +
> + switch (dai->id) {
> + case TAC5XX2_DMIC:
> + pde_entity = TAC_SDCA_ENT_PDE11;
> + function_id = TAC_FUNCTION_ID_SM;
> + break;
> + case TAC5XX2_UAJ:
> + function_id = TAC_FUNCTION_ID_UAJ;
> + pde_entity = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> + TAC_SDCA_ENT_PDE47 : TAC_SDCA_ENT_PDE34;
> + break;
> + default:
> + function_id = TAC_FUNCTION_ID_SA;
> + pde_entity = TAC_SDCA_ENT_PDE23;
> + break;
> + }
> +
> + ret = regmap_write(tac_dev->regmap, SDW_SDCA_CTL(function_id, pde_entity,
> + TAC_SDCA_REQUESTED_PS, 0),
> + SDCA_PDE_PS3);
> + if (ret)
> + return ret;
> +
> + ret = sdca_asoc_pde_poll_actual_ps(tac_dev->dev, tac_dev->regmap, function_id,
> + pde_entity, SDCA_PDE_PS0, SDCA_PDE_PS3,
> + NULL, 0);
> +
> + return ret;
> +}
> +
> +static const struct snd_soc_dai_ops tac_dai_ops = {
> + .hw_params = tac_sdw_hw_params,
> + .hw_free = tac_sdw_pcm_hw_free,
> + .set_stream = tac_set_sdw_stream,
> + .shutdown = tac_sdw_shutdown,

so set_jack is now dynamically added...

> +};
> +

> +static int tac5xx2_sdca_headset_detect(struct tac5xx2_prv *tac_dev)
> +{
> + int val, ret;
> +
> + if (!tac_has_uaj_support(tac_dev))
> + return 0;

... but here the test for uaj_support is still there? Is this needed?

> +
> + ret = regmap_read(tac_dev->regmap,
> + SDW_SDCA_CTL(TAC_FUNCTION_ID_UAJ, TAC_SDCA_ENT_GE35,
> + TAC_SDCA_CTL_DET_MODE, 0), &val);
> + if (ret) {
> + dev_err(tac_dev->dev, "Failed to read the detect mode");
> + return ret;
> + }
> +
> + switch (val) {
> + case 4:
> + tac_dev->jack_type = SND_JACK_MICROPHONE;
> + break;
> + case 5:
> + tac_dev->jack_type = SND_JACK_HEADPHONE;
> + break;
> + case 6:
> + tac_dev->jack_type = SND_JACK_HEADSET;
> + break;
> + case 0:
> + default:
> + tac_dev->jack_type = 0;
> + break;
> + }
> +
> + ret = regmap_write(tac_dev->regmap,
> + SDW_SDCA_CTL(TAC_FUNCTION_ID_UAJ, TAC_SDCA_ENT_GE35,
> + TAC_SDCA_CTL_SEL_MODE, 0), val);
> + if (ret)
> + dev_err(tac_dev->dev, "Failed to update the jack type to device");
> +
> + return 0;
> +}
> +
> +static int tac5xx2_set_jack(struct snd_soc_component *component,
> + struct snd_soc_jack *hs_jack, void *data)
> +{
> + struct tac5xx2_prv *tac_dev = snd_soc_component_get_drvdata(component);
> + int ret;
> +
> + guard(mutex)(&tac_dev->uaj_lock);
> + if (!hs_jack) {
> + if (tac_dev->hs_jack) {
> + tac_dev->hs_jack = NULL;
> + ret = 0;
> + goto disable_interrupts;
> + }
> + return 0;
> + }
> +
> + tac_dev->hs_jack = hs_jack;
> + if (!tac_dev->hw_init) {
> + dev_err(tac_dev->dev, "jack init failed, hw not initialized");
> + return 0;
> + }

should this test go up to the start of the function?
And isn't there a case where .set_jack can be invoked before the device is fully initialized?

I have a vague memory this was the case with other codecs, please double-check.

> +
> + ret = regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INTMASK2,
> + SDW_SCP_SDCA_INTMASK_SDCA_11);
> + if (ret) {
> + dev_warn(tac_dev->dev,
> + "Failed to register jack detection interrupt");
> + goto disable_interrupts;
> + }
> +
> + ret = regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INTMASK3,
> + SDW_SCP_SDCA_INTMASK_SDCA_16);
> + if (ret) {
> + dev_warn(tac_dev->dev,
> + "Failed to register for button detect interrupt");
> + goto disable_interrupts;
> + }
> +
> + return 0;
> +
> +disable_interrupts:
> + /* ignore errors while disabling interrupts */
> + regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INTMASK2, 0);
> + regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INTMASK3, 0);
> +
> + return ret;
> +}
> +
> +static int tac_interrupt_callback(struct sdw_slave *slave,
> + struct sdw_slave_intr_status *status)
> +{
> + struct tac5xx2_prv *tac_dev = dev_get_drvdata(&slave->dev);
> + struct device *dev = &slave->dev;
> + int ret = 0;
> + int btn_type = 0;
> + unsigned int sdca_int2, sdca_int3, jack_report_mask = 0;
> +
> + guard(mutex)(&tac_dev->uaj_lock);

again what does this uaj_lock protect?

> + if (status->control_port) {
> + if (status->control_port & SDW_SCP_INT1_PARITY)
> + dev_warn(dev, "SCP: Parity error interrupt");
> + if (status->control_port & SDW_SCP_INT1_BUS_CLASH)
> + dev_warn(dev, "SCP: Bus clash interrupt");
> + }
> +
> + ret = regmap_read(tac_dev->regmap, SDW_SCP_SDCA_INT2, &sdca_int2);
> + if (ret) {
> + dev_err(dev, "Failed to read UAJ Interrupt, reg:%#x err=%d\n",
> + SDW_SCP_SDCA_INT2, ret);
> + return ret;
> + }
> +
> + ret = regmap_read(tac_dev->regmap, SDW_SCP_SDCA_INT3, &sdca_int3);
> + if (ret) {
> + dev_err(dev, "Failed to read HID interrupt reg=%#x: err=%d",
> + SDW_SCP_SDCA_INT3, ret);
> + return ret;
> + }
> +
> + dev_dbg(dev, "SDCA_INT2: 0x%02x, SDCA_INT3: 0x%02x\n",
> + sdca_int2, sdca_int3);
> +
> + if (sdca_int2 & SDW_SCP_SDCA_INT_SDCA_11) {
> + ret = tac5xx2_sdca_headset_detect(tac_dev);
> + if (ret < 0)
> + goto clear;
> + jack_report_mask |= SND_JACK_HEADSET;
> + }
> +
> + if (sdca_int3 & SDW_SCP_SDCA_INT_SDCA_16) {
> + btn_type = tac5xx2_sdca_button_detect(tac_dev);
> + if (btn_type < 0)
> + btn_type = 0;
> + jack_report_mask |= SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> + SND_JACK_BTN_2 | SND_JACK_BTN_3 | SND_JACK_BTN_4;
> + }
> +
> + if (tac_dev->jack_type == 0)
> + btn_type = 0;
> +
> + dev_dbg(tac_dev->dev, "in %s, jack_type=%d\n", __func__, tac_dev->jack_type);
> + dev_dbg(tac_dev->dev, "in %s, btn_type=0x%x\n", __func__, btn_type);
> +
> + if (!tac_dev->hs_jack)
> + goto clear;
> +
> + snd_soc_jack_report(tac_dev->hs_jack, tac_dev->jack_type | btn_type,
> + jack_report_mask);
> +
> +clear:
> + if (sdca_int2) {
> + ret = regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INT2, sdca_int2);
> + if (ret)
> + dev_dbg(tac_dev->dev, "Failed to clear jack interrupt\n");
> + }
> +
> + if (sdca_int3) {
> + ret = regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INT3, sdca_int3);
> + if (ret)
> + dev_dbg(tac_dev->dev, "failed to clear hid interrupt\n");
> + }
> +
> + return 0;
> +}

> +static s32 tac_component_probe(struct snd_soc_component *component)
> +{
> + struct tac5xx2_prv *tac_dev =
> + snd_soc_component_get_drvdata(component);
> + struct device *dev = tac_dev->dev;
> + struct sdw_slave *slave = tac_dev->sdw_peripheral;
> + unsigned long time;
> + int ret;
> +
> + /* Wait for SoundWire hw initialization to complete */
> + time = wait_for_completion_timeout(&slave->initialization_complete,
> + msecs_to_jiffies(TAC5XX2_PROBE_TIMEOUT_MS));
> + if (!time) {
> + dev_warn(dev, "%s: hw initialization timeout\n", __func__);
> + return -ETIMEDOUT;
> + }
> +
> + if (!tac_has_uaj_support(tac_dev))
> + goto done_comp_probe;
> +
> + ret = snd_soc_dapm_new_controls(snd_soc_component_to_dapm(component),
> + tac_uaj_widgets,
> + ARRAY_SIZE(tac_uaj_widgets));
> + if (ret) {
> + dev_err(component->dev, "Failed to add UAJ widgets: %d\n", ret);
> + return ret;
> + }
> +
> + ret = snd_soc_dapm_add_routes(snd_soc_component_to_dapm(component),
> + tac_uaj_routes, ARRAY_SIZE(tac_uaj_routes));
> + if (ret) {
> + dev_err(component->dev, "Failed to add UAJ routes: %d\n", ret);
> + return ret;
> + }
> +
> + ret = snd_soc_add_component_controls(component, tac_uaj_controls,
> + ARRAY_SIZE(tac_uaj_controls));
> + if (ret) {
> + dev_err(dev, "Failed to add UAJ controls: %d\n", ret);
> + return ret;
> + }
> +
> +done_comp_probe:
> + tac_dev->component = component;
> + return 0;
> +}
> +
> +static void tac_component_remove(struct snd_soc_component *codec)
> +{
> + struct tac5xx2_prv *tac_dev = snd_soc_component_get_drvdata(codec);
> +
> + tac_dev->component = NULL;
> +}
> +
> +static const struct snd_soc_component_driver soc_codec_driver_tacdevice = {
> + .probe = tac_component_probe,
> + .remove = tac_component_remove,
> + .controls = tac5xx2_snd_controls,
> + .num_controls = ARRAY_SIZE(tac5xx2_snd_controls),
> + .dapm_widgets = tac5xx2_common_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(tac5xx2_common_widgets),
> + .dapm_routes = tac5xx2_common_routes,
> + .num_dapm_routes = ARRAY_SIZE(tac5xx2_common_routes),
> + .idle_bias_on = 0,
> + .endianness = 1,
> +};
> +
> +static s32 tac_init(struct tac5xx2_prv *tac_dev)
> +{
> + s32 ret;
> + struct snd_soc_dai_driver *dai_drv;
> + struct snd_soc_component_driver *component_driver;
> + int num_dais;
> +
> + dev_set_drvdata(tac_dev->dev, tac_dev);
> +
> + switch (tac_dev->part_id) {
> + case 0x5572:
> + dai_drv = tac5572_dai_driver;
> + num_dais = ARRAY_SIZE(tac5572_dai_driver);
> + break;
> + case 0x5672:
> + dai_drv = tac5672_dai_driver;
> + num_dais = ARRAY_SIZE(tac5672_dai_driver);
> + break;
> + case 0x5682:
> + dai_drv = tac5682_dai_driver;
> + num_dais = ARRAY_SIZE(tac5682_dai_driver);
> + break;
> + case 0x2883:
> + dai_drv = tas2883_dai_driver;
> + num_dais = ARRAY_SIZE(tas2883_dai_driver);
> + break;
> + default:
> + dev_err(tac_dev->dev, "Unsupported device: 0x%x\n",
> + tac_dev->part_id);
> + return -EINVAL;
> + }
> +
> + component_driver = devm_kzalloc(tac_dev->dev, sizeof(*component_driver),
> + GFP_KERNEL);
> + if (!component_driver)
> + return -ENOMEM;
> +
> + memcpy(component_driver, &soc_codec_driver_tacdevice, sizeof(*component_driver));
> + if (tac_has_uaj_support(tac_dev))
> + component_driver->set_jack = tac5xx2_set_jack;
> +
> + ret = devm_snd_soc_register_component(tac_dev->dev, component_driver,
> + dai_drv, num_dais);
> + if (ret) {
> + dev_err(tac_dev->dev, "%s: codec register error:%d.\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static s32 tac5xx2_sdca_dev_suspend(struct device *dev)
> +{
> + struct tac5xx2_prv *tac_dev = dev_get_drvdata(dev);
> +
> + if (!tac_dev->hw_init)
> + return 0;
> +
> + regcache_cache_only(tac_dev->regmap, true);
> + return 0;
> +}
> +
> +static s32 tac5xx2_sdca_dev_system_suspend(struct device *dev)
> +{
> + return tac5xx2_sdca_dev_suspend(dev);
> +}
> +
> +static s32 tac5xx2_sdca_dev_resume(struct device *dev)
> +{
> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
> + struct tac5xx2_prv *tac_dev = dev_get_drvdata(dev);
> + unsigned long t;
> + int ret;
> + bool had_unattached = false;
> +
> + if (!tac_dev->first_hw_init) {
> + dev_dbg(dev, "Device not initialized yet, skipping resume sync\n");
> + return 0;
> + }
> +
> + if (!slave->unattach_request)
> + goto regmap_sync;
> +
> + had_unattached = true;
> + t = wait_for_completion_timeout(&slave->initialization_complete,
> + msecs_to_jiffies(TAC5XX2_PROBE_TIMEOUT_MS));
> + if (!t) {
> + dev_err(&slave->dev, "resume: initialization timed out\n");
> + sdw_show_ping_status(slave->bus, true);
> + return -ETIMEDOUT;
> + }
> + slave->unattach_request = 0;
> + dev_dbg(tac_dev->dev, "%s wait for resume complete, starting writes\n", __func__);
> +
> + /* Detect and set jack type for UAJ path before playback.
> + * This is required as jack detection does not trigger interrupt
> + * when device is in runtime_pm suspend with bus in clock stop mode.
> + */
> + mutex_lock(&tac_dev->uaj_lock);
> + tac5xx2_sdca_headset_detect(tac_dev);
> + mutex_unlock(&tac_dev->uaj_lock);

again what does this mutex protect? there is no interrupt so there cannot be any conflict/race with the interrupt callback and even less with hw_params.

> +
> +regmap_sync:
> + regcache_cache_only(tac_dev->regmap, false);
> + if (!had_unattached) {
> + regcache_mark_dirty(tac_dev->regmap);
> + ret = regcache_sync(tac_dev->regmap);
> + if (ret < 0)
> + dev_warn(dev, "Failed to sync regcache: %d\n", ret);
> + }
> +
> + return 0;
> +}

> +static struct pci_dev *tac_get_pci_dev(struct sdw_slave *peripheral)
> +{
> + struct device *dev = &peripheral->dev;
> +
> + for (; dev; dev = dev->parent) {
> + if (dev_is_pci(dev))
> + return to_pci_dev(dev);

I find this surprising, what is the purpose of going up in the device hierarchy to find a PCI device?
Why would this driver care?

> + }
> +
> + return NULL;
> +}
> +static int tac_update_status(struct sdw_slave *slave,
> + enum sdw_slave_status status)
> +{
> + int ret;
> + bool first = false;
> + struct tac5xx2_prv *tac_dev = dev_get_drvdata(&slave->dev);
> + struct device *dev = &slave->dev;
> +
> + tac_dev->status = status;
> + if (status == SDW_SLAVE_UNATTACHED) {
> + tac_dev->hw_init = false;
> + tac_dev->fw_dl_success = false;
> + }
> +
> + if (tac_dev->hw_init || tac_dev->status != SDW_SLAVE_ATTACHED) {
> + dev_dbg(dev, "%s: early return, hw_init=%d, status=%d",
> + __func__, tac_dev->hw_init, tac_dev->status);
> + return 0;
> + }
> +
> + if (!tac_dev->first_hw_init) {
> + pm_runtime_set_autosuspend_delay(tac_dev->dev, 3000);
> + pm_runtime_use_autosuspend(tac_dev->dev);
> + pm_runtime_mark_last_busy(tac_dev->dev);
> + pm_runtime_set_active(tac_dev->dev);
> + pm_runtime_enable(tac_dev->dev);

I suggest you look at other codec drivers, all the pm_runtime configuration is done in the .probe callback and only set_active done upon a change in status.

> + tac_dev->first_hw_init = true;
> + first = true;
> + }
> +
> + pm_runtime_get_noresume(tac_dev->dev);
> +
> + regcache_mark_dirty(tac_dev->regmap);
> + regcache_cache_only(tac_dev->regmap, false);
> + ret = tac_io_init(&slave->dev, slave, first);
> + if (ret) {
> + dev_err(dev, "Device initialization failed: %d\n", ret);
> + goto err_out;
> + }
> +
> + ret = regcache_sync(tac_dev->regmap);
> + if (ret)
> + dev_warn(dev, "Failed to sync regcache after init: %d\n", ret);
> +
> +err_out:
> + pm_runtime_mark_last_busy(tac_dev->dev);
> + pm_runtime_put_autosuspend(tac_dev->dev);
> +
> + return ret;
> +}
> +
> +static int tac5xx2_sdw_clk_stop(struct sdw_slave *peripheral,
> + enum sdw_clk_stop_mode mode,
> + enum sdw_clk_stop_type type)
> +{
> + struct tac5xx2_prv *tac_dev = dev_get_drvdata(&peripheral->dev);
> +
> + dev_dbg(tac_dev->dev, "%s: mode:%d type:%d", __func__, mode, type);
> + return 0;

this looks rather useless, no?

> +}
> +
> +static int tac5xx2_sdw_read_prop(struct sdw_slave *peripheral)
> +{
> + struct device *dev = &peripheral->dev;
> + int ret;
> +
> + ret = sdw_slave_read_prop(peripheral);
> + if (ret) {
> + dev_err(dev, "sdw_slave_read_prop failed: %d", ret);
> + return ret;
> + }
> +
> + return 0;
> +}

> +static s32 tac_sdw_probe(struct sdw_slave *peripheral,
> + const struct sdw_device_id *id)
> +{
> + struct regmap *regmap;
> + struct device *dev = &peripheral->dev;
> + struct tac5xx2_prv *tac_dev;
> + struct sdca_function_data *function_data = NULL;
> + int ret, i;
> +
> + tac_dev = devm_kzalloc(dev, sizeof(*tac_dev), GFP_KERNEL);
> + if (!tac_dev)
> + return dev_err_probe(dev, -ENOMEM,
> + "Failed devm_kzalloc");
> +
> + if (peripheral->sdca_data.num_functions > 0) {
> + dev_dbg(dev, "SDCA functions found: %d",
> + peripheral->sdca_data.num_functions);
> +
> + for (i = 0; i < peripheral->sdca_data.num_functions; i++) {
> + struct sdca_function_data **func_ptr;
> + const char *func_name;
> +
> + switch (peripheral->sdca_data.function[i].type) {
> + case SDCA_FUNCTION_TYPE_SMART_AMP:
> + func_ptr = &tac_dev->sa_func_data;
> + func_name = "smartamp";
> + break;
> + case SDCA_FUNCTION_TYPE_SMART_MIC:
> + func_ptr = &tac_dev->sm_func_data;
> + func_name = "smartmic";
> + break;
> + case SDCA_FUNCTION_TYPE_UAJ:
> + func_ptr = &tac_dev->uaj_func_data;
> + func_name = "uaj";
> + break;
> + case SDCA_FUNCTION_TYPE_HID:
> + func_ptr = &tac_dev->hid_func_data;
> + func_name = "hid";
> + break;
> + default:
> + continue;
> + }
> +
> + function_data = devm_kzalloc(dev, sizeof(*function_data),
> + GFP_KERNEL);
> + if (!function_data)
> + return dev_err_probe(dev, -ENOMEM,
> + "failed to allocate %s function data",
> + func_name);
> +
> + ret = sdca_parse_function(dev, peripheral,
> + &peripheral->sdca_data.function[i],
> + function_data);
> + if (!ret)
> + *func_ptr = function_data;
> + else
> + devm_kfree(dev, function_data);
> + }
> + }
> +
> + dev_dbg(dev, "SDCA functions enabled: SA=%s SM=%s UAJ=%s HID=%s",
> + tac_dev->sa_func_data ? "yes" : "no",
> + tac_dev->sm_func_data ? "yes" : "no",
> + tac_dev->uaj_func_data ? "yes" : "no",
> + tac_dev->hid_func_data ? "yes" : "no");
> +
> + tac_dev->dev = dev;
> + tac_dev->sdw_peripheral = peripheral;
> + tac_dev->hw_init = false;
> + tac_dev->first_hw_init = false;
> + mutex_init(&tac_dev->uaj_lock);
> + tac_dev->part_id = id->part_id;
> + dev_set_drvdata(dev, tac_dev);
> +
> + regmap = devm_regmap_init_sdw_mbq_cfg(&peripheral->dev, peripheral,
> + &tac_regmap, &tac_mbq_cfg);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed devm_regmap_init_sdw\n");
> +
> + regcache_cache_only(regmap, true);
> + tac_dev->regmap = regmap;
> + tac_dev->jack_type = 0;
> + init_completion(&tac_dev->fw_caching_complete);
> +
> + if (tac_has_dsp_algo(tac_dev)) {
> + tac_generate_fw_name(peripheral, tac_dev->fw_binaryname,
> + sizeof(tac_dev->fw_binaryname));
> +
> + ret = tac_load_and_cache_firmware_async(tac_dev);
> + if (ret) {
> + complete_all(&tac_dev->fw_caching_complete);
> + dev_dbg(dev, "failed to load fw: %d, use rom mode\n", ret);
> + }
> + } else {
> + complete_all(&tac_dev->fw_caching_complete);
> + }
> +
> + ret = tac_init(tac_dev);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to initialize tac device\n");
> +
> + return 0;
> +}
> +
> +static void tac_sdw_remove(struct sdw_slave *peripheral)
> +{
> + struct tac5xx2_prv *tac_dev = dev_get_drvdata(&peripheral->dev);
> +
> + mutex_destroy(&tac_dev->uaj_lock);
> + dev_set_drvdata(&peripheral->dev, NULL);

if you move all the runtime_pm stuff to the probe above, this remove() would also need to be modified to add a pm_runtime_disable()

> +}