[Deepin-Kernel-SIG] [linux 6.6-y] [UOS] sync and refactor ec support for phytium platform#1973
Conversation
deepin inclusion category: bugfix ACPI: EC: remove clean up handlers on probe failure in acpi_ec_setup Signed-off-by: huangbibo <huangbibo@uniontech.com> Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
Reviewer's GuideAdds a Phytium-specific EC / laptop platform driver for ARM64, including EC command helpers, sensor exposure, power and hotkey handling, and integrates new chassis-type helpers plus a Phytium ACPI EC quirk; wires the new platform directory into the ARM64 drivers tree. 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 |
95d6c52 to
b5c1f36
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
lib/classtypes.c,get_chassis_types()usesdmi_get_system_info()but the file does not include<linux/dmi.h>, which will break the build on some configs; include the appropriate header explicitly rather than relying on indirect inclusion. - In
ft_lpc_event_probe()the conditionif (!read_cpuid_implementor() == ARM_CPU_IMP_PHYTIUM)is almost certainly wrong due to operator precedence (the!applies before==); this should be rewritten as an explicit inequality comparison, e.g.if (read_cpuid_implementor() != ARM_CPU_IMP_PHYTIUM)or by using the newcpu_is_phytium()helper. - In
ec_sensor.c, the various*_databuffers are allocated withkzalloc()and never freed; consider usingdevm_kzalloc()(tied topdev->dev) or passingNULLas the hwmon data pointer to avoid persistent allocations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `lib/classtypes.c`, `get_chassis_types()` uses `dmi_get_system_info()` but the file does not include `<linux/dmi.h>`, which will break the build on some configs; include the appropriate header explicitly rather than relying on indirect inclusion.
- In `ft_lpc_event_probe()` the condition `if (!read_cpuid_implementor() == ARM_CPU_IMP_PHYTIUM)` is almost certainly wrong due to operator precedence (the `!` applies before `==`); this should be rewritten as an explicit inequality comparison, e.g. `if (read_cpuid_implementor() != ARM_CPU_IMP_PHYTIUM)` or by using the new `cpu_is_phytium()` helper.
- In `ec_sensor.c`, the various `*_data` buffers are allocated with `kzalloc()` and never freed; consider using `devm_kzalloc()` (tied to `pdev->dev`) or passing `NULL` as the hwmon data pointer to avoid persistent allocations.
## Individual Comments
### Comment 1
<location path="drivers/platform/arm64/phytium/phyt_laptop.c" line_range="1113-1114" />
<code_context>
+ return sprintf(buf, "%d\n", x_val);
+}
+
+static struct kobj_attribute gsensor_attr =
+ __ATTR(value_x, 0644, gsensor_show, NULL);
+
+static struct attribute *gsensor_attrs[] = {
</code_context>
<issue_to_address>
**issue (bug_risk):** Use read-only sysfs permissions or add a store handler
The attribute is exposed as 0644 but has no store callback, so any write attempts will fail with -EIO. Either change the mode to 0444/S_IRUGO if it’s meant to be read-only, or add a store handler if writes are required.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| static struct kobj_attribute gsensor_attr = | ||
| __ATTR(value_x, 0644, gsensor_show, NULL); |
There was a problem hiding this comment.
issue (bug_risk): Use read-only sysfs permissions or add a store handler
The attribute is exposed as 0644 but has no store callback, so any write attempts will fail with -EIO. Either change the mode to 0444/S_IRUGO if it’s meant to be read-only, or add a store handler if writes are required.
7c21cf2 to
54f03ac
Compare
uos inclusion category: feature Bring back the IT8528 EC and laptop platform support, switching the EC transport from the removed Phytium LPC path to the Phytium PIO path. This provides battery, AC, lid, hotkey, backlight and hwmon support for Phytium laptop platforms. based on these commits: commit 4c3ee4f195ca7 uos: phyt/ec_it8528: replace readb/writeb with inb/outb [T381729] commit 9953278324c81 uos: phytium/lpc: Calc iobase and offsets in one function [T381729] commit 7bca99aa60b9b phytium/lpc: Support various lpc address/offset [T366853] commit ba88c0d55b8f3 uos: phyt: port UOS_PHYTIUM_DESKTOP_SUPPORT to V25 [T366853] Assisted-by: kimi-cli:kimi-k2.7 code # reformat patch Signed-off-by: huangbibo <huangbibo@uniontech.com> Signed-off-by: hmy <huanglin@uniontech.com> Signed-off-by: wenlunpeng <wenlunpeng@uniontech.com> Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
deepin inclusion category: feature Refactor the IT8528 EC driver to reduce duplication and boilerplate: - Replace the six nearly-identical read/write helpers with a single it8528_transaction() routine, mirroring the transaction model used by drivers/acpi/ec.c. - Table-drive battery state updates instead of a large switch/case. - Table-drive EC event dispatch and make the three legacy handlers return void, since their return values were never used. - Remove the unused ft_ec_irq_hanlde() export. Assisted-by: kimi-cli:kimi-k2.7 code Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
deepin inclusion category: feature Simplify probe error handling and remove the last checkpatch warning: - Make ec_gsonsor_init() take the platform_device and create the gsensor sysfs group under &dev->dev.kobj instead of the private devices_kset. This eliminates the extern struct kset *devices_kset declaration. - Switch power_info, backlight and power_supply registrations to devm_* variants so most probe resources are auto-released on failure. - Collapse the long goto chain in ft_lpc_event_probe() to a single fail_after_gsensor cleanup path. - Remove the bogus platform_driver_unregister() call from probe's error path. Assisted-by: kimi-cli:kimi-k2.7 code Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
Relocate the Phytium EC/laptop drivers from the vendor-specific UOS directory tree to drivers/platform/arm64/phytium, following the upstream convention of architecture-specific platform driver directories (e.g. drivers/platform/x86, drivers/platform/loongarch). - Add ARM64_PLATFORM_DEVICES menuconfig under drivers/platform/arm64. - Drop the UOS_PHYTIUM_DESKTOP_SUPPORT indirection and the drivers/uos/ tree entirely. - Update MAINTAINERS and the defconfig accordingly. Assisted-by: kimi-cli:kimi-k2.7 code Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
54f03ac to
16e19a7
Compare
Summary by Sourcery
Add Phytium ARM64 laptop EC support and platform driver integration, including chassis-type helpers and EC-based power, backlight, hotkey, sensor, and event handling.
New Features:
Bug Fixes:
Enhancements: