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

[Holalu Yogendra, Niranjan]

> On 01:54-20260501, Pierre-Louis Bossart wrote:
> Subject: Re: [PATCH v14 2/4] ASoC: tac5xx2-sdw: add soundwire based codec driver
> 
> > +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;
> > +	}
> 
> are you sure this is needed? Or if this even works, if the card creation happens
> after the bus goes to clock-stop, then you will always timeout here.
> 
> If you look at commit 011e397 "ASoC: codecs: soundwire: call
> pm_runtime_resume() in component probe" it's simpler to force the bus+codec
> to resume, no?

Thanks. I will adopt this fix which you've added for other drivers.

> 
> > +
> > +	if (!tac_has_uaj_support(tac_dev))
> > +		goto done_comp_probe;
> 
> Is this really the case that you only have controls+routes for the UAJ function?

At this place, I do not see problem. But, interrupt handler needs fix to handle
interrupts only when UAJ is supported. Right now it reads uaj interrupts 
regardless of whether UAJ is supported or not. I will fix it


> +static s32 tac5xx2_sdca_dev_resume(struct device *dev)
> +{
...
> > +
> > +	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;
> > +
...
> > +
> > +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);
> > +	}
> 
> can you explain why you have this extra test with had_unattached and if this is
> really useful?
> I don't recall having seen this before on any other driver.

This was supposed to be a small optimization, but I realized that this will lead to
bug that regcache not being called if device was not detached for the suspend -> resume scenario.
I will drop this check

Regards
Niranjan