On Thu, 20 Mar 2025 at 18:33, Derek J. Clark <derekjohn.clark@xxxxxxxxx> wrote: > > > > On March 19, 2025 7:54:55 AM HST, Antheas Kapenekakis <lkml@xxxxxxxxxxx> wrote: > >This four part series updates the oxpsensors module to bring it in line > >with its Windows OneXPlayer counterpart. First, it adds support for all > >2024, 2025 OneXPlayer handhelds and their special variants. Then, it moves > >the module to platform/x86 to allow for including more EC features. > > > >Then, it adds the new charge limiting and bypass features that were first > >introduced in the X1 and retrofit to older OneXFly variants and for > >controlling the turbo led found in the X1 models. For Bypass, it adds a new > >charge_behaviour variant called inhibit-charge-s0. > > > >Finally, it performs a minor refactor by moving around switch statements > >into their own functions, in order to allow for fixing the pwm1_enable ABI > >in the final patch. Currently, pwm1_enable sets the fan to auto with the > >value 0 and allows manual control with the value 1. This patch makes it > >so 0 sets the fan to full speed, 1 sets the fan to manual control, and > >2 sets the fan to auto. This requires both setting enable and the fan > >speed when the enable sysfs is written to as 0, hence the refactor. > > > >As this is a minor ABI break and there is userspace software relying > >on this previous behavior, the last patch also changes the /name of the > >hwmon endpoint to "oxp_ec" from "oxpec" (mirroring WMI module conventions) > >such that userspace software that relied on the previous behavior can be > >retrofit to the new kernel while enabling correct functionality on old > >and new kernels. Failing that, software that is not updated will just > >stop controlling the fans, ensuring no malignant behavior. > > > >--- > >V5: https://lore.kernel.org/all/20250317155349.1236188-1-lkml@xxxxxxxxxxx/ > >V4: https://lore.kernel.org/all/20250311165406.331046-1-lkml@xxxxxxxxxxx/ > >V3: https://lore.kernel.org/all/20250309112114.1177361-1-lkml@xxxxxxxxxxx/ > > > >Changes since V5: > > - Separate doc entries with Fixes as by Mario > > - Add sysfs file name to subject as per Thomas > > - Make tt_led and tt_turbo const as per Thomas > > - Align a couple of structs as per Thomas > > - Remove excess battery check as per Thomas > > - For Thomas: devices without a BIOS update battery control is a NOOP > > OXP is a boutique manufacturer for now, so gathering information > > about old devices to add BIOS checks is not practical unfortunately > > Antheas, > This sort of begs the question on how this feature was tested on those devices? That question includes whether or not it is really a no-op in unsupported BIOS. My old contacts at OXP are no longer employed there, are you in contact with anyone at OXP currently that can potentially provide the data? > > I'm still of the opinion that the attribute should be explicitly enabled only on a known supported BIOS. IMO there is a general assumption that a driver exposed attribute fd will work and having a no-op will confuse users and lead to spurious bug reports. We shouldn't be exposing a no-op in the sysfs for a driver if we can avoid it. If we add the BIOS checks we can also print to dmesg if a BIOS is too low a version so they will know why it isn't there. > > That being said, it does seem likely low risk, so I'm not nacking the feature as is if the subsystem maintainers are okay with it. > > - Derek Hi Derek, I have tested this patch series on an X1 (AMD) personally. I also have multiple people that have tested it on an F1 Pro, X1 Intel, and an X1 Mini. And I am pretty sure that at least 1 or 2 people have tested on an older BIOS too without an ill effect. The older F1s are a bit of a question mark. The BIOS update for those came out around Xmas and I do not have that many users with them. However, they act identically to the new ones when it comes to the EC and the controller, so I do not think that is that big of an issue. Also, no one has asked yet about it not working on those devices. And they ask about a lot of things. Yanking charging support from those will just hurt those users unnecessarily as they will get it later. Eileen is from OneXPlayer and is cc'd in this series. Antheas > >Changes since V4: > > - Fix nits by Hans > > - change inhibit-charge-s0 to inhibit-charge-awake > > - use devm_battery_hook_register and power_supply_unregister_extension > > (based on cros driver) > > - move charge behavior patches to the end to make the rest of the series > > easier to merge > > - CC platform-x86 and power maintainers > > > >Changes since V3: > > - Fix nits by Derek > > - Remove the hwmon documentation as it is not required for platform > > drivers (suggested by Guenter) > > - Add ACPI_BATTERY and HWMON depends to Kconfig > > (reported by kernel robot) > > - Homogenize driver into following reverse xmas convention > > > >Changes since V2: > > - Add ack by Guenter, move platform move patch to be third (not first > > to allow for device support backport to lts kernels) > > - Rework patch text, especially in the refactor patches as per Derek > > - Change bypass to use charge_behaviour instead of charge_type, as that > > ABI supports capability detection and is more appropriate > > - Move battery attach to probe instead of init > > - Fix bug where reading tt_led would instead use the turbo register > > > >Changes since V1: > > - Add X1 Pro, F1 Pro variants > > - Fix minor typo in initial patches > > - Convert oxp-sensors into a platform driver, as it is no longer > > considered a hwmon driver. > > - Add sysfs documentation and myself to the MAINTAINERS file > > - Update documentation to state that this is the OneXPlayer/AOKZOE > > platform driver, and that support for Ayaneo/OPI is provided until > > they gain their own platform driver. > > > >Antheas Kapenekakis (14): > > hwmon: (oxp-sensors) Distinguish the X1 variants > > hwmon: (oxp-sensors) Add all OneXFly variants > > platform/x86: oxpec: Move hwmon/oxp-sensors to platform/x86 > > ABI: testing: sysfs-class-oxp: add missing documentation > > ABI: testing: sysfs-class-oxp: add tt_led attribute documentation > > platform/x86: oxpec: Rename ec group to tt_toggle > > platform/x86: oxpec: Add turbo led support to X1 devices > > platform/x86: oxpec: Move pwm_enable read to its own function > > platform/x86: oxpec: Move pwm value read/write to separate functions > > platform/x86: oxpec: Move fan speed read to separate function > > platform/x86: oxpec: Adhere to sysfs-class-hwmon and enable pwm on 2 > > platform/x86: oxpec: Follow reverse xmas convention for tt_toggle > > power: supply: add inhibit-charge-awake to charge_behaviour > > platform/x86: oxpec: Add charge threshold and behaviour to OneXPlayer > > > > Documentation/ABI/testing/sysfs-class-power | 11 +- > > Documentation/ABI/testing/sysfs-platform-oxp | 25 + > > Documentation/hwmon/index.rst | 2 +- > > Documentation/hwmon/oxp-sensors.rst | 89 --- > > MAINTAINERS | 7 +- > > drivers/hwmon/Kconfig | 11 - > > drivers/hwmon/Makefile | 1 - > > drivers/platform/x86/Kconfig | 13 + > > drivers/platform/x86/Makefile | 3 + > > .../oxp-sensors.c => platform/x86/oxpec.c} | 624 ++++++++++++++---- > > drivers/power/supply/power_supply_sysfs.c | 7 +- > > drivers/power/supply/test_power.c | 1 + > > include/linux/power_supply.h | 1 + > > 13 files changed, 540 insertions(+), 255 deletions(-) > > create mode 100644 Documentation/ABI/testing/sysfs-platform-oxp > > delete mode 100644 Documentation/hwmon/oxp-sensors.rst > > rename drivers/{hwmon/oxp-sensors.c => platform/x86/oxpec.c} (52%) > > > > > >base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1
