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

[Charles Keepax]

On Thu, Mar 26, 2026 at 11:47:10PM +0530, Niranjan H Y wrote:
> +/* TLV for volume control */
> +static const DECLARE_TLV_DB_SCALE(tac5xx2_amp_tlv, 0, 50, 0);
> +static const DECLARE_TLV_DB_SCALE(tac5xx2_dvc_tlv, -7200, 50, 0);
> +
> +/* Q7.8 volume control parameters: range -72dB to +6dB, step 0.5dB */
> +#define TAC_DVC_STEP		128	/* 0.5 dB in Q7.8 format */
> +#define TAC_DVC_MIN		(-144)	/* -72 dB / 0.5 dB step */
> +#define TAC_DVC_MAX		12	/* +6 dB / 0.5 dB step */
> +
> +#define SOC_SINGLE_Q78_TLV(xname, xreg, tlv_array) { \
> +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> +	.name = xname, \
> +	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | SNDRV_CTL_ELEM_ACCESS_READWRITE, \
> +	.tlv.p = (tlv_array), \
> +	.info = snd_soc_info_volsw, .get = q78_get_volsw, .put = q78_put_volsw, \
> +	.private_value = (unsigned long)&(struct soc_mixer_control) { \
> +		.reg = (xreg), .rreg = (xreg), \
> +		.min = TAC_DVC_MIN, .max = TAC_DVC_MAX, \
> +		.platform_max = TAC_DVC_MAX - TAC_DVC_MIN, .shift = TAC_DVC_STEP, \

What is the purpose of the platform_max here? Doesn't feel like
we should be setting that from the driver.

> +		.sign_bit = 15 \
> +	} \
> +}

Probably makes sense to parameterise min,max,step here and then
put this macro in the sdca_asoc.h header, perhaps rename to
something like SDCA_SINGLE_Q78_TLV as well. That way if others
are creating these controls directly they can just reuse that.

> +static int tac5xx2_sdca_button_detect(struct tac5xx2_prv *tac_dev)
> +{
> +	unsigned int btn_type, offset, idx;
> +	int ret, value, owner;
> +	u8 buf[2];
> +
> +	ret = regmap_read(tac_dev->regmap,
> +			  SDW_SDCA_CTL(TAC_FUNCTION_ID_HID, TAC_SDCA_ENT_HID1,
> +				       TAC_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), &owner);
> +	if (ret) {
> +		dev_err(tac_dev->dev,
> +			"Failed to read current UMP message owner 0x%x", ret);
> +		return ret;
> +	}
> +
> +	if (owner == 1) {

Would be nice to use the SDCA_UMP_OWNER_DEVICE define here.

> +		dev_dbg(tac_dev->dev, "current owner is host, skipping..");

I think this message should read either "current owner is device"
or "current owner is not host", as a value of 1 is device and it
makes more sense in the context.

> +		return 0;
> +	}
> +
> +	ret = regmap_read(tac_dev->regmap,
> +			  SDW_SDCA_CTL(TAC_FUNCTION_ID_HID, TAC_SDCA_ENT_HID1,
> +				       TAC_SDCA_CTL_HIDTX_MESSAGE_OFFSET, 0), &value);
> +	if (ret) {
> +		dev_err(tac_dev->dev,
> +			"Failed to read current UMP message offset: %d", ret);
> +		goto end_btn_det;
> +	}
> +
> +	dev_dbg(tac_dev->dev, "btn_ message offset = %x", value);
> +	offset = value;
> +
> +	for (idx = 0; idx < sizeof(buf); idx++) {
> +		ret = regmap_read(tac_dev->regmap,
> +				  TAC_BUF_ADDR_HID1 + offset + idx, &value);
> +		if (ret) {
> +			dev_err(tac_dev->dev,
> +				"Failed to read HID buffer: %d", ret);
> +			goto end_btn_det;
> +		}
> +		buf[idx] = value & 0xff;
> +	}
> +
> +	if (buf[0] == 0x1) {
> +		btn_type = tac5xx2_sdca_btn_type(&buf[1], tac_dev);
> +		ret = btn_type;
> +	}
> +
> +end_btn_det:
> +	if (!owner)

Does this if actually make sense? owner can only be 0,1 by the spec
and you check for 1 earlier in the function, so isn't this always
true?

> +		regmap_write(tac_dev->regmap,
> +			     SDW_SDCA_CTL(TAC_FUNCTION_ID_HID, TAC_SDCA_ENT_HID1,
> +					  TAC_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), 0x01);
> +
> +	return ret;
> +}

> +static const struct sdw_device_id tac_sdw_id[] = {
> +	SDW_SLAVE_ENTRY(0x0102, 0x5572, 0),
> +	SDW_SLAVE_ENTRY(0x0102, 0x5672, 0),
> +	SDW_SLAVE_ENTRY(0x0102, 0x5682, 0),
> +	SDW_SLAVE_ENTRY(0x0102, 0x2883, 0),
> +	{},
> +};

Not that I am pushing for this now on this driver but would be
good for you guys to perhaps poke what happens binding your parts
with the class driver itself (sdca_class.c). The hope would be
in time we can support all new SDCA parts out of there by just
adding the slave entry. So good to start finding out what does
and doesn't work for you guys. At the moment the class driver is
still in active development and a lot of shipping products have
dodgy DisCo or hardware issues that make use difficult, so
this is more of a long term goal. From scanning the driver here
I am guessing perhaps the firmware download might not work so
well with your current parts. But a lot of the rest looks like
it would assuming the DisCo is all correct.

Thanks,
Charles