[PATCH] ASoC: tas2783-sdw: do not cache read-only Controls
Andrey Golovko <[email protected]>
| Newsgroups | org.kernel.vger.linux-sound,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
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. Nothing in the driver reads these Controls - it performs no register reads at all - so the change is visible only through the regmap. Signed-off-by: Andrey Golovko <[email protected]> --- Applies on top of "ASoC: tas2783-sdw: do not treat read-only Controls as writable", Message-ID <[email protected]>, whose list of Controls this patch moves into a shared helper. Tested on an ASUS ProArt PX13 HN7306EAC, two TAS2783 aggregated on SDW1, on top of broonie/sound for-next. After a cold start the firmware loads as before and the calibration registers read back byte for byte what they read without the patch. A regmap read of the PDE23 actual power state now answers 0x0 on both amplifiers - the state a cache-bypassing read over the bus reports - where it answered the 0x3 placeholder from the defaults table before. sound/soc/codecs/tas2783-sdw.c | 62 ++++++++++++---------------------- 1 file changed, 21 insertions(+), 41 deletions(-) diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c index 6ed6121a93ee..549f5e621d65 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): @@ -539,15 +508,26 @@ 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) { case 0x000 ... 0x080: /* Data port 0. */ case 0x100 ... 0x140: /* Data port 1. */ -- 2.53.0