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

[Holalu Yogendra, Niranjan]

> On 03:11-20260324, Pierre-Louis Bossart wrote:
> Subject: Re: [PATCH v1 2/4] ASoC: tac5xx2-sdw: add soundwire
> based codec driver
> 
> > +#define TAC5XX2_PROBE_TIMEOUT 10000
> 
> Best to include the unit in the define, e.g.
> #define TAC5XX2_PROBE_TIMEOUT_MS 10000
I will fix it in next version

> 
> > +#define TAS2883_DEFAULT_FW_NAME	 "tas2883-default.bin"
> 
> it wouldn't hurt to add a comment, in theory firmware is supposed to be
> extracted using platform-specific ACPI mappings.
I will drop this unused string.
 
> > +	struct mutex pde_lock;
> 
> it wouldn't hurt to explain why you need a lock here.
> In theory the PDE needs to be activated based on DAPM information.
...
> > +	/* lock for fw download */
> > +	struct mutex fw_lock;
> 
> same, why do you need a lock for fw download?
> Isn't this chip using the SDCA method to download firmware with UMP?
> If not, a note would help...
Currently, UMP based firmware download is not used. I will add a note.
I will refactor to have a single lock between fw and pde to prevent PDE
operations during the firmware download.

> > +	ucontrol->value.enumerated.item[0] = 1; /* Default to DC:1 */
> 
> If DC refers to "defined constant", what's the point of exposing this control?
> 
> And you should explain how the clock selection is supposed to work, it's not
> something we've seen before in SDCA chips.
I will double check this initialization to default value.

> > +/* Convert dB to Q7.8 format (16-bit signed value) */
> > +static inline u16 db_to_q7_8(int db_value_times_100)
> > +{
...
> > +/* Convert Q7.8 format to dB*100 */
> > +static inline int q7_8_to_db_times_100(u16 q7_8_value)
> > +{
> > +	s16 signed_val = (s16)q7_8_value;
> > +
> > +	return (signed_val * 100) / 256;
> > +}
> 
> Didn't Charles Keepax add some macros for the Q7.8 format?
I will explore this option to reuse the existing code.
Looks like it is not currently exposed as macros.

> > +		fu_entity = TAC_SDCA_ENT_FU13;
> > +		function_number = TAC_FUNCTION_ID_SM;
> > +	} else if (strstr(w->name, "FU41")) {
> > +		fu_entity = TAC_SDCA_ENT_FU41;
> > +		function_number = TAC_FUNCTION_ID_UAJ;
> > +	} else if (strstr(w->name, "FU36")) {
> > +		fu_entity = TAC_SDCA_ENT_FU36;
> > +		function_number = TAC_FUNCTION_ID_UAJ;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> 
> when I see this code, it makes me wonder if it's not better to have one driver
> per function, rather than one driver that takes care of multiple functions?
I will explore this option or try to move these to widgets as suggested.

> isn't a wait on the ACTUAL_PS required here?
> 
...
> > +		usleep_range(2000, 2200);
> > +	} while (retry--);
> 
> and here there's a loop but it keeps writing the REQUESTED_PS without
> checking the ACTUAL_PS.
> 
> This doesn't seem to be aligned with SDCA concepts, either the hardware does
> something different that should be documented or the code should be
> updated?
...
> > +		pde_entity = TAC_SDCA_ENT_PDE23;
> > +	}
> > +	mutex_lock(&tac_dev->pde_lock);
> > +	ret = regmap_write(tac_dev->regmap,
> > +			   SDW_SDCA_CTL(function_id, pde_entity, 0x01, 0),
> > +			   0x03);
> > +	mutex_unlock(&tac_dev->pde_lock);
> 
> I don't see the point of this lock?
> The PDE should be defined as a power supply that gets activated when paths
> become active, no?
I observed -ENODATA error sometimes for the REQUESTED_PS while testing.
I will double check. I will test ACTUAL_PS status change and add it.

> > +
> > +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, value;
> > +	int btn_type = 0;
> > +	unsigned int sdca_int1, sdca_int2, sdca_int3, sdca_int4;

> > +
> > +	/* read jack status */
> > +	ret = tac5xx2_sdca_headset_detect(tac_dev);
> > +	if (ret < 0)
> > +		goto clear;
> > +
> > +	btn_type = tac5xx2_sdca_button_detect(tac_dev);
> > +	if (btn_type < 0)
> > +		btn_type = 0;
> > +
> > +	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,
> > +			    SND_JACK_HEADSET | SND_JACK_BTN_0 |
> > +				SND_JACK_BTN_1 | SND_JACK_BTN_2 |
> > +				SND_JACK_BTN_3 | SND_JACK_BTN_4);
> > +
> > +clear:
> > +	for (int i = 1; i <= 4; i++) {
> > +		int control_selector = 0x10;
> 
> shouldn't you clear only the interrupts that were detected earlier?
> 
> This loop could lead you to clear an interrupt you haven't dealt with, no great
> for jack detection...

I will double check this. 
As I remember, I read jack events in one go (button type and headset type).
And I am force clearing all the interrupts so that I do not miss the next interrupt callback 
as currently I am only using the interrupt for jack detection.

> >
> > +#if IS_ENABLED(CONFIG_PCI)
> > +static struct pci_dev *tac_get_pci_dev(struct sdw_slave *peripheral)
> > +{
> > +	struct device *dev = &peripheral->dev;
> > +
> > +	for (; dev; dev = dev->parent) {
> > +		if (dev->bus == &pci_bus_type)
> > +			return to_pci_dev(dev);
> > +	}
> > +
> > +	return NULL;
> > +}
> > +#else
> 
> Is this CONFIG_PCI stuff relevant for an SDCA device? The PCI stuff looks
> copy/pasted from an HDAudio codec driver, I don't see how this might work
> for a SoundWire device?

There was a compilation error reported earlier for other driver.
https://lore.kernel.org/oe-kbuild-all/202601190756.IpoMY5AJ-lkp@xxxxxxxxx/
So I add this to avoid this issue in this driver as well. 

> > +
> > +static s32 tac_io_init(struct device *dev, struct sdw_slave *slave, bool first)
> > +{
> > +	struct tac5xx2_prv *tac_dev = dev_get_drvdata(dev);
> > +	s32 ret = 0;
> > +
> > +	if (tac_dev->hw_init) {
> > +		dev_dbg(dev, "early return hw_init already done..");
> > +		return 0;
> > +	}
> > +
> > +	if (tac_has_dsp_algo(tac_dev)) {
> > +		tac_generate_fw_name(slave, tac_dev->fw_binaryname,
> > +				     sizeof(tac_dev->fw_binaryname));
> > +
> > +		if (!tac_dev->fw_cached) {
> > +			ret = tac_load_and_cache_firmware(tac_dev);
> > +			if (ret)
> > +				dev_dbg(dev, "failed to load fw: %d, use rom
> mode\n", ret);
> > +		}
> > +
> > +		if (tac_dev->fw_cached) {
> > +			ret = tac_download_fw_to_hw(tac_dev);
> > +			if (ret) {
> > +				dev_err(dev, "FW download failed, fw: %d\n",
> ret);
> > +				goto io_init_err;
> > +			}
> > +		}
> > +	}
> 
> shouldn't the driver read the NEEDS_FIRMARE status bit before downloading
> firmware?
Currently,  we are not using UMP based FW download via state machine and BRA.
So, we are not using this bit.

> > +
> > +	if (tac_dev->sm_func_data) {
> > +		ret = sdca_regmap_write_init(dev, tac_dev->regmap,
> > +					     tac_dev->sm_func_data);
> > +		if (ret) {
> > +			dev_err(dev, "smartmic init table update failed\n");
> > +			goto io_init_err;
> > +		} else {
> > +			dev_dbg(dev, "smartmic init done\n");
> > +		}
> > +
> > +		/* Set default value to DC:1 */
> > +		tac_dev->cx11_default_value = 1;
> > +
> > +		ret = regmap_write(tac_dev->regmap,
> > +				   SDW_SDCA_CTL(TAC_FUNCTION_ID_SM,
> > +						TAC_SDCA_ENT_CX11,
> > +						TAC_SDCA_CTL_CX_CLK_SEL,
> 0),
> > +				   tac_dev->cx11_default_value);
> > +		if (ret)
> > +			dev_warn(dev, "Failed to set CX11 default: %d", ret);
> > +
> > +		if (first) {
> > +			ret = regmap_multi_reg_write(tac_dev->regmap,
> tac_sm_seq,
> > +						     ARRAY_SIZE(tac_sm_seq));
> > +			if (ret) {
> > +				dev_err(tac_dev->dev,
> > +					"init writes failed, err=%d", ret);
> > +				goto io_init_err;
> > +			}
> > +		}
> 
> this is mixing table inits, clock setup and regmap init, is this intentional or could
> this be refactors with other bits done in hw_params?
The initialization to default value is intentional to set the default value.
I will check if this can be refactored.

Regards
Niranjan H Y