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

["Holalu Yogendra, Niranjan" <niranjan.hy@ti.com>]

> On 01:28-20260429, Pierre-Louis Bossart wrote:
> Subject: Re: [PATCH v11 2/4] ASoC: tac5xx2-sdw: add soundwire based codec driver

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

 I think comment needs update to remove the hw_params related description here. ...

> > +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 =
> > +	return ret;
> 
> so as I said above there's nothing here that might conflict with the interrupt
> callback?
> What am I missing?

...  uaj_lock is still needed to handle potential race condition  in case .set_jack is called with jack as NULL
(potentially to disable the jack functionality ) when the interrupt is being handled.
This was suggested in earlier review comment and it made sense to me as well 

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

I will drop this. 

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

I have already dropped this in v12 patch to remove the hw_init check and let regcache handle this.

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

commented above.

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

commented above

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

For Speaker protection modules running on the firmware based devices, we need to have 
different firmware for different projects as there are firmware differences.
We need a way to uniquely identify firmware for each of these projects.
Currently this is done through the  "subsystem id" which is part of the pci device.
So we find the pci bus and then identify the subsystem id.

I see similar changes in class driver as well sound/soc/sdca/sdca_fdl.c.

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

I thought I will keep it here so that all pm_runtime_* initializations are in one place.

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

Yes, currently only required in case of debugging. 

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

Already taken care in v12.

Thanks 
 
Regards
Niranjan