Open
Conversation
added 4 commits
March 17, 2026 14:32
Add power_save parameter to enable automatic power saving mode. The value of power_save is timeout value of automatic power-saving (in second, 0 is disable). Default value is 1. Mainline: NA Signed-off-by: Dai Jingtao <daijingtao1503@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Change-Id: Ib2e3da17a96bc5a654a998fc90bd2d76e915e63a
Add runtime status inquiry node. Use "cat runtime_status" to inquire the runtime status of codec. D0 is working, and D3 is in IDLE. Mainline: NA Signed-off-by: Dai Jingtao <daijingtao1503@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Change-Id: I3fe0ce7eb99c5f45dbcfe0e74a76795883fa5ae7
HDA driver will use schedule work to register codec driver used by input_register_device function. There will be a conflict if sysfs_create_group is operated at the same time. So put off creating runtime inquiry node to avoid this conflict. Mainline: NA Signed-off-by: Dai Jingtao <daijingtao1503@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Change-Id: I9b6d5711a874cbd3e15eb2b19595b69285fbc129
Enable async suspend so that different driver can resume parallely. Mainline: NA Signed-off-by: Dai Jingtao <daijingtao1503@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Change-Id: I581ebf5972236c4357a7d34ab215d7e58053e79b
Reviewer's GuideEnable configurable power-saving and runtime status reporting for the Phytium HDA controller, including a tunable power_save module parameter, a sysfs runtime_status attribute, and asynchronous suspend support, while wiring these features into the controller’s probe and card-list management paths. Sequence diagram for power_save module parameter handlingsequenceDiagram
actor Admin
participant Kernel as Linux_kernel
participant Module as hda_phytium_module
participant List as card_list
participant HDA as hda_ft
participant Chip as azx
participant Bus as hdac_bus
Admin->>Kernel: write to module parameter power_save
Kernel->>Module: param_set_xint(val, kp)
Module->>Module: param_set_int(val, kp)
Module->>Module: compare prev and power_save
alt power_save changed and success
Module->>List: mutex_lock(card_list_lock)
loop for each hda in card_list
List->>HDA: iterate list entry
HDA->>Chip: access chip
Module->>Chip: check probe_continued and disabled
alt probe_continued and not disabled
Chip->>Bus: get bus reference
Module->>Bus: snd_hda_set_power_save(bus, power_save * 1000)
end
end
Module->>List: mutex_unlock(card_list_lock)
else unchanged or error
Module-->>Kernel: return ret
end
Module-->>Kernel: return 0 or error
Kernel-->>Admin: write completes
Sequence diagram for runtime_status sysfs querysequenceDiagram
actor Admin
participant Sysfs as sysfs_runtime_status
participant Dev as hda_ft_device
participant Card as snd_card
participant Chip as azx
participant Bus as hdac_bus
Admin->>Sysfs: cat /sys/.../runtime_status
Sysfs->>Dev: runtime_status_show(dev, buf)
Dev->>Card: dev_get_drvdata(dev)
Card->>Chip: access private_data
Chip->>Bus: azx_bus(chip)
Bus->>Bus: mutex_lock(cmd_mutex)
Bus->>Bus: snd_hdac_bus_send_cmd(cmd=0x001f0500)
Bus->>Bus: snd_hdac_bus_get_response(addr=0, res)
Bus->>Bus: mutex_unlock(cmd_mutex)
Bus-->>Dev: response res
Dev->>Dev: decode res & 0x3 to status D0..D3
Dev-->>Sysfs: sprintf(buf, status)
Sysfs-->>Admin: D0 or D1 or D2 or D3
Updated class diagram for Phytium HDA power management and runtime statusclassDiagram
class hda_ft {
+struct azx chip
+struct device* dev
+struct work_struct probe_work
+int probe_continued
+struct list_head list
}
class azx {
+struct hdac_bus bus
+int dev_index
+bool disabled
}
class hdac_bus {
+struct mutex cmd_mutex
+int irq
}
class snd_card {
+void* private_data
}
class attribute_group {
+struct attribute** attrs
}
class device_attribute_runtime_status {
+const char* name = runtime_status
+mode_t mode = 0444
+ssize_t runtime_status_show(struct device* dev, struct device_attribute* attr, char* buf)
}
class hda_ft_runtime_status_group {
+struct attribute_group hda_ft_runtime_status_group
+struct attribute* runtime_status_attrs[2]
}
class hda_phytium_module_params {
+int power_save
+int param_set_xint(const char* val, const struct kernel_param* kp)
}
hda_ft --> azx : has
azx --> hdac_bus : has
snd_card --> azx : private_data points_to
hda_ft_runtime_status_group --> attribute_group : wraps
hda_ft_runtime_status_group --> device_attribute_runtime_status : contains
hda_phytium_module_params --> hda_ft : iterates_card_list
hda_phytium_module_params --> azx : uses_chip
hda_phytium_module_params --> hdac_bus : calls_snd_hda_set_power_save
device_attribute_runtime_status --> hdac_bus : sends_codec_cmd
hda_ft --> hda_ft_runtime_status_group : sysfs_group_lifecycle
hda_ft --> snd_card : owns_via_probe
hda_ft --> hdac_bus : via_chip_bus
class platform_device_probe_flow {
+int hda_ft_probe(struct platform_device* pdev)
+int azx_probe_continue(struct azx* chip)
+int hda_ft_remove(struct platform_device* pdev)
+int azx_first_init(struct azx* chip)
}
platform_device_probe_flow --> hda_ft : creates_and_initializes
platform_device_probe_flow --> hda_ft_runtime_status_group : create_and_remove_sysfs
platform_device_probe_flow --> hda_phytium_module_params : honors_power_save
platform_device_probe_flow --> hdac_bus : uses_irq_and_cmd_mutex
platform_device_probe_flow --> azx : controller_lifecycle
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
runtime_status_show,statuscan be left uninitialized ifres & 0x3somehow falls outside 0–3 andresis also pre-initialized to-1(which will report D3 even if no valid response is received); it would be safer to check the return fromsnd_hdac_bus_get_response()and provide a defined fallback string (e.g.,"unknown") instead of relying on the masked value. - Using
dev_info()on every read of theruntime_statussysfs attribute is likely to be noisy in production; consider switching this todev_dbg()or dropping the log entirely to avoid log spam when the attribute is polled.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `runtime_status_show`, `status` can be left uninitialized if `res & 0x3` somehow falls outside 0–3 and `res` is also pre-initialized to `-1` (which will report D3 even if no valid response is received); it would be safer to check the return from `snd_hdac_bus_get_response()` and provide a defined fallback string (e.g., `"unknown"`) instead of relying on the masked value.
- Using `dev_info()` on every read of the `runtime_status` sysfs attribute is likely to be noisy in production; consider switching this to `dev_dbg()` or dropping the log entirely to avoid log spam when the attribute is polled.
## Individual Comments
### Comment 1
<location path="sound/pci/hda/hda_phytium.c" line_range="968-969" />
<code_context>
+
+ dev_info(dev, "Inquire codec status!\n");
+ mutex_lock(&bus->cmd_mutex);
+ snd_hdac_bus_send_cmd(bus, cmd);
+ snd_hdac_bus_get_response(bus, 0, &res);
+ mutex_unlock(&bus->cmd_mutex);
+
</code_context>
<issue_to_address>
**issue:** Handle possible errors or timeouts from `snd_hdac_bus_get_response` instead of always trusting `res`.
If `snd_hdac_bus_get_response` fails or times out, `res` could be stale, yet the code still uses it to derive a power state. Please check the return value (and whether a valid response was received) before using `res`, and on failure return an error (e.g. `-EIO`) or a clearly invalid status string instead of reporting a normal power state.
</issue_to_address>
### Comment 2
<location path="sound/pci/hda/hda_phytium.c" line_range="966" />
<code_context>
+ unsigned int res = -1;
+ char *status;
+
+ dev_info(dev, "Inquire codec status!\n");
+ mutex_lock(&bus->cmd_mutex);
+ snd_hdac_bus_send_cmd(bus, cmd);
</code_context>
<issue_to_address>
**suggestion:** This `dev_info` will be emitted on every sysfs read and may be too noisy.
Because this runs on every read of the sysfs attribute, this log may be excessively noisy. Consider switching to `dev_dbg()` (leveraging dynamic debug) or removing the message to avoid flooding the logs.
```suggestion
dev_dbg(dev, "Inquire codec status!\n");
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+968
to
+969
| snd_hdac_bus_send_cmd(bus, cmd); | ||
| snd_hdac_bus_get_response(bus, 0, &res); |
There was a problem hiding this comment.
issue: Handle possible errors or timeouts from snd_hdac_bus_get_response instead of always trusting res.
If snd_hdac_bus_get_response fails or times out, res could be stale, yet the code still uses it to derive a power state. Please check the return value (and whether a valid response was received) before using res, and on failure return an error (e.g. -EIO) or a clearly invalid status string instead of reporting a normal power state.
| unsigned int res = -1; | ||
| char *status; | ||
|
|
||
| dev_info(dev, "Inquire codec status!\n"); |
There was a problem hiding this comment.
suggestion: This dev_info will be emitted on every sysfs read and may be too noisy.
Because this runs on every read of the sysfs attribute, this log may be excessively noisy. Consider switching to dev_dbg() (leveraging dynamic debug) or removing the message to avoid flooding the logs.
Suggested change
| dev_info(dev, "Inquire codec status!\n"); | |
| dev_dbg(dev, "Inquire codec status!\n"); |
There was a problem hiding this comment.
Pull request overview
This PR adds configurable power-saving and runtime status reporting capabilities to the Phytium HD-audio controller driver, and enables async suspend to improve suspend/resume behavior.
Changes:
- Add a
power_savemodule parameter (with live reconfiguration across active cards) to control automatic power-saving timeout. - Add a read-only sysfs attribute group to query codec power state, created after probe continuation.
- Enable asynchronous suspend for the Phytium HDA platform device.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+967
to
+970
| mutex_lock(&bus->cmd_mutex); | ||
| snd_hdac_bus_send_cmd(bus, cmd); | ||
| snd_hdac_bus_get_response(bus, 0, &res); | ||
| mutex_unlock(&bus->cmd_mutex); |
| unsigned int res = -1; | ||
| char *status; | ||
|
|
||
| dev_info(dev, "Inquire codec status!\n"); |
| break; | ||
| } | ||
|
|
||
| return sprintf(buf, "%s\n", status); |
| static int single_cmd = -1; | ||
| static int enable_msi = -1; | ||
| #ifdef CONFIG_PM | ||
| static int power_save = 1; |
Comment on lines
+1097
to
1105
| if (sysfs_create_group(&hda->dev->kobj, &hda_ft_runtime_status_group)) { | ||
| dev_warn(hda->dev, "failed create sysfs\n"); | ||
| goto err_sysfs; | ||
| } | ||
|
|
||
| return err; | ||
|
|
||
| err_sysfs: | ||
| sysfs_remove_group(&hda->dev->kobj, &hda_ft_runtime_status_group); | ||
| out_free: |
Comment on lines
+964
to
+969
| char *status; | ||
|
|
||
| dev_info(dev, "Inquire codec status!\n"); | ||
| mutex_lock(&bus->cmd_mutex); | ||
| snd_hdac_bus_send_cmd(bus, cmd); | ||
| snd_hdac_bus_get_response(bus, 0, &res); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add parameters to enable automatic power-saving features.
Add runtime status query nodes.
Defer creation of runtime query nodes.
Enable asynchronous suspend feature.
Summary by Sourcery
Enable configurable power saving and runtime status reporting for the Phytium HDA controller driver.
New Features:
Enhancements: