[PATCH v2 0/2] ASoC: tas2783-sdw: fix the read-only Control handling

[PATCH v2 0/2] ASoC: tas2783-sdw: fix the read-only Control handling

[Andrey Golovko]

The two read-only Control patches from 13 August, resent together as a
series because the second one depends on the first, and with the change
Mark asked for.

He pointed out that the XU22 Control 0x14 in the read-only list is
SDCA_CTL_XU_FDL_STATUS, which sdca_fdl_process() writes.  He is right,
and the description I gave it - "firmware download status" - reads like
a status the device owns, which it is not.  It is a handshake: the
device sets the bits it owns, and the host writes its response back into
the same Control, keeping the device bits, which is exactly what
sdca_fdl_process() does with response | (status & ~SDCA_CTL_XU_FDLH_MASK).
So it is out of the list in 1/2.

Whether the write is reachable on this device today: the driver
registers no SDCA interrupts and never calls into sdca_fdl.c.  It
downloads its firmware itself, with request_firmware_nowait() and its
own container format in tas2783_fw_ready(), and the machines I have
describe no Smart Amp SDCA function in the BIOS at all, so nothing
populates the interrupt that would drive the state machine.  But the
Function does implement the FDL Controls - the driver lists 0x10 and
0x12 through 0x16 in its defaults - so the state machine is a plausible
thing for this driver to grow, and _regmap_write() answers -EIO for a
register the callback refuses.  That would break the handshake at the
first response.  Not a trap worth planting for one entry in a list.

2/2 still keeps the Control out of the cache, and now says why on its
own: sdca_fdl_process() reads it to see what the device asked for, and a
cached answer would hide the device half of the handshake.  Writable and
volatile, which is the normal shape for a handshake register.

The two are otherwise unchanged.  The behaviour I measured on an ASUS
ProArt PX13 for v1 stands: the calibration data written at probe is byte
for byte what it was before the series, and a read of the PDE23 actual
power state answers 0x0 with the Function powered, where before it
answered the 0x3 placeholder from the defaults.  The only difference in
v2 is that a write to the File Download Status is no longer rejected,
and nothing in the driver issues one.

The questions in 1/2 stand as well: the list is static because
sdca_regmap_writeable() is not available to a driver running its
fallback tables, so it would be good to have it confirmed against the
documentation, and to know whether the read-only Controls were meant to
be in tas2783_reg_default[] at all.

Andrey Golovko (2):
  ASoC: tas2783-sdw: do not treat read-only Controls as writable
  ASoC: tas2783-sdw: do not cache read-only Controls

 sound/soc/codecs/tas2783-sdw.c | 99 +++++++++++++++++++++++-----------
 1 file changed, 68 insertions(+), 31 deletions(-)

[PATCH v2 1/2] ASoC: tas2783-sdw: do not treat read-only Controls as writable

[Andrey Golovko]

The regmap has no writeable_reg callback, so regmap considers every
register up to max_register writable.  That includes the read-only SDCA
Controls the driver itself describes: the Latency of every Entity, the
Clock Valid of every Clock Source, the actual power state of the Power
Domain Entity, the protection status, the algorithm ready flag and the
Extension Unit id and version.  Most of them are also listed in
tas2783_reg_default[] with a placeholder of zero, even though a default
for, say, a latency reading is meaningless.

Reading such a Control caches its real value, which no longer matches
the placeholder, so regcache_sync() then tries to write it back.  The
peripheral rejects the transaction with -ENODATA and the sync aborts,
leaving the rest of the cache unrestored.

Add a writeable_reg callback that refuses the read-only Controls and
otherwise keeps the previous behaviour.  Every selector it lists is the
read-only Control of its Entity type in sdca_function.h, and none of the
Controls the driver writes is affected: the requested power state, the
mutes, the Cluster Index, the protection mode, the algorithm enable and
the file download Controls all stay writable.

The list is static because the BIOS on the affected machines describes
no Smart Amp SDCA function, so the driver runs its fallback tables and
sdca_regmap_writeable() is not available to it.  It would be good to
have the list confirmed against the hardware documentation, and to know
whether the read-only Controls belong in tas2783_reg_default[] at all.

Signed-off-by: Andrey Golovko <andrey.golovko@gmail.com>
---
v2: dropped the XU22 File Download Status (0x14) from the list; Mark
    pointed out that the host writes it.  Reworded the changelog to
    match.
 sound/soc/codecs/tas2783-sdw.c | 51 ++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index c217da5fccdf..caf8fe1bf4db 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -495,6 +495,56 @@ static bool tas2783_readable_register(struct device *dev, unsigned int reg)
 	return tas2783_sdca_mbq_size(dev, reg) > 0;
 }
 
+static bool tas2783_writeable_register(struct device *dev, unsigned int reg)
+{
+	/*
+	 * The Latency Control of every Entity, together with the Power Domain
+	 * actual state and the protection status, is read-only. They are
+	 * listed in tas2783_reg_default[] with a placeholder value, so without
+	 * this a regcache_sync() would try to write them back and the
+	 * peripheral would reject the transaction, aborting the sync.
+	 */
+	switch (reg) {
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_FU21, 0x10, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_FU23, 0x10, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_FU26, 0x10, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_XU22, 0x06, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_XU22, 0x07, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_XU22, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_CS24, 0x02, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_CS21, 0x02, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_CS25, 0x02, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_CS26, 0x02, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_CS28, 0x02, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_PDE23, 0x10, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_UDMPU23, 0x06, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_SAPU29, 0x05, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_SAPU29, 0x11, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_PPU21, 0x06, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_PPU26, 0x06, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_IT21, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_IT29, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_IT26, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_IT28, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_OT24, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_OT23, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_OT25, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_OT28, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_MU26, 0x06, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_OT127, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_FU127, 0x10, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_CS127, 0x02, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_MFPU21, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_MFPU21, 0x04, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_MFPU26, 0x08, 0):
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_MFPU26, 0x04, 0):
+		return false;
+
+	default:
+		return tas2783_sdca_mbq_size(dev, reg) > 0;
+	}
+}
+
 static bool tas2783_volatile_register(struct device *dev, u32 reg)
 {
 	switch (reg) {
@@ -516,6 +566,7 @@ static const struct regmap_config tas_regmap = {
 	.reg_bits = 32,
 	.val_bits = 8,
 	.readable_reg = tas2783_readable_register,
+	.writeable_reg = tas2783_writeable_register,
 	.volatile_reg = tas2783_volatile_register,
 	.reg_defaults = tas2783_reg_default,
 	.num_reg_defaults = ARRAY_SIZE(tas2783_reg_default),
-- 
2.53.0

[PATCH v2 2/2] ASoC: tas2783-sdw: do not cache read-only Controls

[Andrey Golovko]

The read-only SDCA Controls are readings of device state, but the regmap
caches them and tas2783_reg_default[] gives each one a placeholder, so a
read is answered from the cache and never reaches the peripheral.  The
answer is whatever the cache happens to hold: on an ASUS ProArt PX13 a
read of the PDE23 actual power state returns the 0x3 placeholder, i.e.
PS3, while the Function is powered and a cache-bypassing read of the
same Control over the bus answers 0x0.  Once the cache has been given a
value it keeps it, so the Control reports a state the device left long
ago.

Mark those Controls volatile and drop their entries from the defaults.
regcache_read() and regcache_write() skip volatile registers, so a
read-only Control no longer enters the cache at all: reads reach the
peripheral, and a sync cannot attempt to write a reading back.  That is
what regcache_sync() asks for in the first place - "any registers that
should not be synced should be marked as volatile".

The list is the same one the writeable_reg callback needs, so move it
into a helper both callbacks use.  The File Download Status is not on
it, because the host writes that Control, but it is volatile all the
same: the device raises its own bits on its own schedule, and
sdca_fdl_process() reads the Control to see them.  A cached answer would
hide the device half of the handshake.

Nothing in the driver reads any of these Controls - it performs no
register reads at all - so the change is visible only through the
regmap.

Signed-off-by: Andrey Golovko <andrey.golovko@gmail.com>
---
v2: File Download Status is no longer read-only, so mark it volatile
    on its own; its entry stays out of tas2783_reg_default[].
 sound/soc/codecs/tas2783-sdw.c | 68 ++++++++++++++--------------------
 1 file changed, 27 insertions(+), 41 deletions(-)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index caf8fe1bf4db..f3485ce4b0c5 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -185,35 +185,17 @@ static const struct reg_default tas2783_reg_default[] = {
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU26, 0x01, 1), 0x1},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU26, 0x0b, 1), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x01, 0), 0x1},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x06, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x07, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x09, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x0a, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS24, 0x02, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS21, 0x02, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS26, 0x02, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS28, 0x02, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_PDE23, 0x1, 0), 0x3},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_SAPU29, 0x05, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_PPU21, 0x06, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_PPU26, 0x06, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_IT21, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_IT21, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_IT29, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_IT29, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_IT26, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_IT26, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_IT28, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_IT28, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT24, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT24, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT23, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT23, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT25, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT25, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT28, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT28, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MU26, 0x01, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MU26, 0x01, 1), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MU26, 0x01, 2), 0x0},
@@ -222,40 +204,28 @@ static const struct reg_default tas2783_reg_default[] = {
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MU26, 0x01, 5), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MU26, 0x01, 6), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MU26, 0x01, 7), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MU26, 0x06, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT127, 0x04, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT127, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU127, 0x01, 0), 0x1},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU127, 0x01, 1), 0x1},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU127, 0x01, 2), 0x1},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU127, 0x0b, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU127, 0x0b, 1), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU127, 0x0b, 2), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS127, 0x02, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU21, 0x01, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU21, 0x04, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU21, 0x05, 0), 0x1},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU21, 0x08, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU26, 0x01, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU26, 0x04, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU26, 0x05, 0), 0x1},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU26, 0x08, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU23, 0x10, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU26, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x10, 0), 0x1},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x12, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x13, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x14, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x15, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x16, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS24, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS21, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS26, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS28, 0x10, 0), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_PDE23, 0x10, 0), 0x3},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_UDMPU23, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_SAPU29, 0x10, 0), 0x1},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_SAPU29, 0x11, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_SAPU29, 0x12, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_PPU21, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_PPU21, 0x11, 0), 0x0},
@@ -286,7 +256,6 @@ static const struct reg_default tas2783_reg_default[] = {
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT127, 0x12, 5), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT127, 0x12, 6), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT127, 0x12, 7), 0x0},
-	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU127, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_CS127, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU21, 0x10, 0), 0x0},
 	{SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_MFPU21, 0x11, 0), 0x0},
@@ -495,15 +464,15 @@ static bool tas2783_readable_register(struct device *dev, unsigned int reg)
 	return tas2783_sdca_mbq_size(dev, reg) > 0;
 }
 
-static bool tas2783_writeable_register(struct device *dev, unsigned int reg)
+/*
+ * The read-only SDCA Controls the driver describes: the Latency of every
+ * Entity, the Clock Valid of every Clock Source, the actual power state of
+ * the Power Domain Entity, the protection status, the algorithm ready flag
+ * and the Extension Unit id, version and firmware download status.  None of
+ * them is a setting; every one is a reading of device state.
+ */
+static bool tas2783_read_only_control(unsigned int reg)
 {
-	/*
-	 * The Latency Control of every Entity, together with the Power Domain
-	 * actual state and the protection status, is read-only. They are
-	 * listed in tas2783_reg_default[] with a placeholder value, so without
-	 * this a regcache_sync() would try to write them back and the
-	 * peripheral would reject the transaction, aborting the sync.
-	 */
 	switch (reg) {
 	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_FU21, 0x10, 0):
 	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_FU23, 0x10, 0):
@@ -538,16 +507,33 @@ static bool tas2783_writeable_register(struct device *dev, unsigned int reg)
 	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_MFPU21, 0x04, 0):
 	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_MFPU26, 0x08, 0):
 	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_MFPU26, 0x04, 0):
-		return false;
+		return true;
 
 	default:
-		return tas2783_sdca_mbq_size(dev, reg) > 0;
+		return false;
 	}
 }
 
+static bool tas2783_writeable_register(struct device *dev, unsigned int reg)
+{
+	if (tas2783_read_only_control(reg))
+		return false;
+
+	return tas2783_sdca_mbq_size(dev, reg) > 0;
+}
+
 static bool tas2783_volatile_register(struct device *dev, u32 reg)
 {
+	if (tas2783_read_only_control(reg))
+		return true;
+
 	switch (reg) {
+	/*
+	 * The File Download Status is a handshake: the device raises its own
+	 * bits on its own schedule and the host writes back a response, so it
+	 * is writable, but it must never be answered from the cache.
+	 */
+	case SDW_SDCA_CTL(FUNC_NUM_SMART_AMP, TAS2783_SDCA_ENT_XU22, 0x14, 0):
 	case 0x000 ... 0x080: /* Data port 0. */
 	case 0x100 ... 0x140: /* Data port 1. */
 	case 0x200 ... 0x240: /* Data port 2. */
-- 
2.53.0

Re: [PATCH v2 2/2] ASoC: tas2783-sdw: do not cache read-only Controls

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On Fri, Aug 14, 2026 at 09:40:00AM +0300, Andrey Golovko wrote:
> The read-only SDCA Controls are readings of device state, but the regmap
> caches them and tas2783_reg_default[] gives each one a placeholder, so a
> read is answered from the cache and never reaches the peripheral.  The

>  static bool tas2783_volatile_register(struct device *dev, u32 reg)
>  {
> +	if (tas2783_read_only_control(reg))
> +		return true;
> +

Does this work for multibyte reads?  The regmap has val_size set to 8
but some of the read only registers are multibyte.  For SoundWire
val_bits doesn't really matter so you should just be able to set that to
32.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH v2 0/2] ASoC: tas2783-sdw: fix the read-only Control handling

[Mark Brown]

On Fri, 14 Aug 2026 09:40:00 +0300, Andrey Golovko wrote:
> ASoC: tas2783-sdw: fix the read-only Control handling
> 
> The two read-only Control patches from 13 August, resent together as a
> series because the second one depends on the first, and with the change
> Mark asked for.
> 
> He pointed out that the XU22 Control 0x14 in the read-only list is
> SDCA_CTL_XU_FDL_STATUS, which sdca_fdl_process() writes.  He is right,
> and the description I gave it - "firmware download status" - reads like
> a status the device owns, which it is not.  It is a handshake: the
> device sets the bits it owns, and the host writes its response back into
> the same Control, keeping the device bits, which is exactly what
> sdca_fdl_process() does with response | (status & ~SDCA_CTL_XU_FDLH_MASK).
> So it is out of the list in 1/2.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-7.3

Thanks!

[1/2] ASoC: tas2783-sdw: do not treat read-only Controls as writable
      https://git.kernel.org/broonie/sound/c/0c7aeb0f5ece

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2 2/2] ASoC: tas2783-sdw: do not cache read-only Controls

[Andrey Golovko]

On Fri, Aug 14, 2026 at 04:13:42PM +0100, Mark Brown wrote:

> Does this work for multibyte reads?  The regmap has val_size set to 8
> but some of the read only registers are multibyte.  For SoundWire
> val_bits doesn't really matter so you should just be able to set that to
> 32.

It does not, thank you.  Twenty of the Controls in the list are wider
than one byte, and with val_bits = 8 the MBQ layer refuses each of them
before anything reaches the bus, so the patch on its own would replace
an answer from the cache with -EINVAL.

The widening is a patch of its own because the calibration write relied
on the old width, so I have sent both as v3 with it in front, against
for-7.3:

  https://lore.kernel.org/linux-sound/20260815112000.4180-1-andrey.golovko@gmail.com/

The cover letter has the list of the multi-byte Controls and what the
two patches were measured to do on the machine here.

One thing I noticed while checking the sizes, unrelated to this series:
FU21 0x10 and UDMPU23 0x06 are in the read-only list but appear nowhere
in tas2783_sdca_mbq_size(), which returns 0 for them, so
tas2783_readable_register() refuses them and they are unreachable
through the regmap in either direction.  Harmless as it stands, nothing
touches them, but the size callback looks incomplete rather than
deliberate.  I can add them, or leave that to TI along with the rest of
the reg_defaults questions I owe them a mail about.

Andrey

Re: [PATCH v2 2/2] ASoC: tas2783-sdw: do not cache read-only Controls

[Charles Keepax]

On Sat, Aug 15, 2026 at 02:45:00PM +0300, Andrey Golovko wrote:
> On Fri, Aug 14, 2026 at 04:13:42PM +0100, Mark Brown wrote:
> 
> > Does this work for multibyte reads?  The regmap has val_size set to 8
> > but some of the read only registers are multibyte.  For SoundWire
> > val_bits doesn't really matter so you should just be able to set that to
> > 32.
> 
> It does not, thank you.  Twenty of the Controls in the list are wider
> than one byte, and with val_bits = 8 the MBQ layer refuses each of them
> before anything reaches the bus, so the patch on its own would replace
> an answer from the cache with -EINVAL.
> 
> The widening is a patch of its own because the calibration write relied
> on the old width, so I have sent both as v3 with it in front, against
> for-7.3:
> 
>   https://lore.kernel.org/linux-sound/20260815112000.4180-1-andrey.golovko@gmail.com/
> 
> The cover letter has the list of the multi-byte Controls and what the
> two patches were measured to do on the machine here.
> 
> One thing I noticed while checking the sizes, unrelated to this series:
> FU21 0x10 and UDMPU23 0x06 are in the read-only list but appear nowhere
> in tas2783_sdca_mbq_size(), which returns 0 for them, so
> tas2783_readable_register() refuses them and they are unreachable
> through the regmap in either direction.  Harmless as it stands, nothing
> touches them, but the size callback looks incomplete rather than
> deliberate.  I can add them, or leave that to TI along with the rest of
> the reg_defaults questions I owe them a mail about.

One thing that is worth noting, is that sometimes in SDCA values
are DisCo constants, these are registers where the value is just
hard coded in the ACPI and typically not implemented in the
hardware. Generally, these are implemented as non-read/non-write
registers with an entry in the default array. This allows the
constant value to be read through the regmap, and driver code
generally doesn't need to care that the register is fictional.

Both of the controls you list are latency which is commonly done
as a DisCo constant, although that said it is also not a control
that is AFAIK used by any implementation so could also just have
been forgotten.

Thanks,
Charles