Revert "soundwire: record Peripheral scale_index"#5744
Revert "soundwire: record Peripheral scale_index"#5744bardliao wants to merge 1 commit intothesofproject:topic/sof-devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates SoundWire peripheral state tracking so each device retains bus clock scaling state for both SoundWire register banks (bank 0 and bank 1).
Changes:
- Change
sdw_slave::scale_indexfrom a single value to an array storing per-bank values. - Update stream programming logic to compare/update the cached
scale_indexfor the active bank. - Update frequency-setting path to cache scale indices for both banks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| include/linux/soundwire/sdw.h | Expand scale_index storage to per-bank state and update struct documentation. |
| drivers/soundwire/stream.c | Track active bank and use per-bank scale_index for comparison/update. |
| drivers/soundwire/bus.c | Update cached scale indices after setting bus frequency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dev_err(&slave->dev, | ||
| "SDW_SCP_BUSCLOCK_SCALE_B1 write failed:%d\n", ret); | ||
|
|
||
| slave->scale_index = scale_index; | ||
| slave->scale_index[0] = scale_index; | ||
| slave->scale_index[1] = scale_index; | ||
| return ret; |
There was a problem hiding this comment.
This updates both cached bank values unconditionally, even when the B1 write failed (and possibly when only one bank was actually programmed). This can desynchronize software state from hardware. Update the cache only for the bank(s) successfully written, and only on success (e.g., after both writes succeed, or set [0]/[1] immediately after each corresponding successful write).
| * @sdw_dev_lock: mutex used to protect callbacks/remove races | ||
| * @sdca_data: structure containing all device data for SDCA helpers | ||
| * @scale_index: current bus clock scaling index | ||
| * @scale_index: current bus clock scaling index on bank 0 and 1 |
There was a problem hiding this comment.
The array size 2 is a magic number tied to the number of banks. Consider introducing a named constant (e.g., SDW_NUM_BANKS) and using it for scale_index[SDW_NUM_BANKS] to make future changes safer and self-documenting.
| struct mutex sdw_dev_lock; /* protect callbacks/remove races */ | ||
| struct sdca_device_data sdca_data; | ||
| unsigned int scale_index; | ||
| unsigned int scale_index[2]; |
There was a problem hiding this comment.
The array size 2 is a magic number tied to the number of banks. Consider introducing a named constant (e.g., SDW_NUM_BANKS) and using it for scale_index[SDW_NUM_BANKS] to make future changes safer and self-documenting.
| * @sdw_dev_lock: mutex used to protect callbacks/remove races | ||
| * @sdca_data: structure containing all device data for SDCA helpers | ||
| * @scale_index: current bus clock scaling index | ||
| * @scale_index: current bus clock scaling index on bank 0 and 1 |
There was a problem hiding this comment.
Minor grammar fix: change 'on bank 0 and 1' to 'on banks 0 and 1' (or 'for banks 0 and 1') to match the fact this now represents two banks.
| * @scale_index: current bus clock scaling index on bank 0 and 1 | |
| * @scale_index: current bus clock scaling index on banks 0 and 1 |
bardliao
force-pushed
the
sdw-scale_index
branch
from
e523ef9 to
3def65a
Compare
The commit doesn't add much value and will increase the maintain effort. This reverts commit 903cbf5. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
bardliao
force-pushed
the
sdw-scale_index
branch
from
3def65a to
23d24d4
Compare
bardliao
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (bus->params.next_bank) | ||
| addr1 = SDW_SCP_BUSCLOCK_SCALE_B1; | ||
| else | ||
| addr1 = SDW_SCP_BUSCLOCK_SCALE_B0; | ||
|
|
||
| /* Program SDW_SCP_BUSCLOCK_SCALE if all Peripherals comply with SDCA */ | ||
| list_for_each_entry(slave, &bus->slaves, node) { | ||
| int scale_index; | ||
| u8 base; | ||
|
|
||
| if (!slave->dev_num_sticky) | ||
| continue; | ||
| scale_index = sdw_slave_get_scale_index(slave, &base); | ||
| if (scale_index < 0) | ||
| return scale_index; | ||
|
|
||
| if (scale_index == slave->scale_index) | ||
| continue; | ||
|
|
||
| /* Skip the unattached Peripherals */ | ||
| if (!completion_done(&slave->enumeration_complete)) { | ||
| dev_warn(&slave->dev, | ||
| "Not enumerated, skip programming BUSCLOCK_SCALE\n"); | ||
| continue; | ||
| } | ||
|
|
||
| ret = sdw_write_no_pm(slave, addr1, scale_index); | ||
| if (ret < 0) { | ||
| dev_err(&slave->dev, "SDW_SCP_BUSCLOCK_SCALE register write failed\n"); | ||
| return ret; | ||
| } | ||
| slave->scale_index = scale_index; | ||
| } |
There was a problem hiding this comment.
With the per-slave scale_index cache removed, sdw_program_params() will now write SDW_SCP_BUSCLOCK_SCALE_B0/B1 for every attached Peripheral on every call, even when the value for the target bank is unchanged. If this is performance-sensitive on large topologies, consider tracking the last programmed scale index per bank (e.g., a 2-element array keyed by bank) and skipping the write only when the cached value for that bank matches.
The commit doesn't add much value and will increase the maintain effort.