[PATCH v4 00/13] hwmon: (oxpsensors) Add devices, features, fix ABI and move to platform/x86

[PATCH v4 00/13] hwmon: (oxpsensors) Add devices, features, fix ABI and move to platform/x86

[Antheas Kapenekakis]

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.

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 (13):
  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: add tt_toggle and tt_led entries
  power: supply: add inhibit-charge-s0 to charge_behaviour
  platform/x86: oxpec: Add charge threshold and behaviour to OneXPlayer
  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

 Documentation/ABI/testing/sysfs-class-power   |  11 +-
 Documentation/ABI/testing/sysfs-platform-oxp  |  26 +
 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}    | 683 ++++++++++++++----
 drivers/power/supply/power_supply_sysfs.c     |   1 +
 drivers/power/supply/test_power.c             |   1 +
 include/linux/power_supply.h                  |   1 +
 13 files changed, 600 insertions(+), 249 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} (51%)


base-commit: 4d872d51bc9d7b899c1f61534e3dbde72613f627
-- 
2.48.1

[PATCH v4 01/13] hwmon: (oxp-sensors) Distinguish the X1 variants

[Antheas Kapenekakis]

Currently, the oxp-sensors driver fuzzy matches the X1 variants. Luckily,
X1 and X1 mini share most hardware features so this works. However, they
are completely different product lines, and there is an expectation that
OneXPlayer will release more devices in the X1 line that may have
differences.

Therefore, distinguish the 3 devices that currently exist in the market.
These are the OneXPlayer X1 AMD and Intel variants, and the X1 mini which
only has an AMD variant. As far as registers go, all three support the
current driver functionality.

Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hwmon/oxp-sensors.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index 83730d931824..5a4230ad3757 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -205,7 +205,28 @@ static const struct dmi_system_id dmi_table[] = {
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
-			DMI_MATCH(DMI_BOARD_NAME, "ONEXPLAYER X1"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER X1 A"),
+		},
+		.driver_data = (void *)oxp_x1,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER X1 i"),
+		},
+		.driver_data = (void *)oxp_x1,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER X1 mini"),
+		},
+		.driver_data = (void *)oxp_x1,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER X1Pro"),
 		},
 		.driver_data = (void *)oxp_x1,
 	},
-- 
2.48.1

[PATCH v4 02/13] hwmon: (oxp-sensors) Add all OneXFly variants

[Antheas Kapenekakis]

Currently, the driver only has the F1 OneXFly variant, which was based
on the 7000 AMD platform. Add its special editions: F1 EVA-01, F1 OLED.
F1 OLED might have been a dev unit, but it is supported by OneXConsole
with the same features so add it. Then add the F1L variant which is
based on the 8000 AMD platform and the F1Pro and its special edition
EVA-02.

One might ask why not just fuzzy match. Well, EVA-02 is a variant of
F1Pro which is a Strix Point handheld, but does not have F1Pro in its
name. This makes it risky to fuzzy match, as special variants in the
future from different platforms might not have the same feature set
or registers.

By happenstance, all current devices use the same registers. For the
charge limitting feature on this series, only F1Pro/X1 (AMD) were
released with it, but OneXPlayer is providing bios updates for F1, F1L,
X1 Mini units that use the same register, so treat all of them the same.

Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/hwmon/oxp-sensors.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index 5a4230ad3757..f7a64fbc8f33 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -188,6 +188,41 @@ static const struct dmi_system_id dmi_table[] = {
 		},
 		.driver_data = (void *)oxp_fly,
 	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER F1 EVA-01"),
+		},
+		.driver_data = (void *)oxp_fly,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER F1 OLED"),
+		},
+		.driver_data = (void *)oxp_fly,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER F1L"),
+		},
+		.driver_data = (void *)oxp_fly,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER F1Pro"),
+		},
+		.driver_data = (void *)oxp_fly,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER F1 EVA-02"),
+		},
+		.driver_data = (void *)oxp_fly,
+	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
-- 
2.48.1

[PATCH v4 03/13] platform/x86: oxpec: Move hwmon/oxp-sensors to platform/x86

[Antheas Kapenekakis]

The EC of OneXPlayer devices used to only control the fan.
This is no longer the case, with the EC of OneXPlayer gaining
additional functionality (turbo button, turbo led, battery controls).

As it will be beneficial from a complexity perspective
to retain this driver as a single unit, move it out
of hwmon, and into platform/x86. Also, remove the
hwmon documentation to avoid it becoming stale.

While at it, add myself to the maintainer's file.

Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 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                  | 12 +++
 drivers/platform/x86/Makefile                 |  3 +
 .../oxp-sensors.c => platform/x86/oxpec.c}    | 10 +--
 8 files changed, 24 insertions(+), 111 deletions(-)
 delete mode 100644 Documentation/hwmon/oxp-sensors.rst
 rename drivers/{hwmon/oxp-sensors.c => platform/x86/oxpec.c} (98%)

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 874f8fd26325..dd7a54d5f281 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -186,7 +186,7 @@ Hardware Monitoring Kernel Drivers
    nzxt-kraken3
    nzxt-smart2
    occ
-   oxp-sensors
+   oxpec
    pc87360
    pc87427
    pcf8591
diff --git a/Documentation/hwmon/oxp-sensors.rst b/Documentation/hwmon/oxp-sensors.rst
deleted file mode 100644
index 581c4dafbfa1..000000000000
--- a/Documentation/hwmon/oxp-sensors.rst
+++ /dev/null
@@ -1,89 +0,0 @@
-.. SPDX-License-Identifier: GPL-2.0-or-later
-
-Kernel driver oxp-sensors
-=========================
-
-Authors:
-    - Derek John Clark <derekjohn.clark@gmail.com>
-    - Joaquín Ignacio Aramendía <samsagax@gmail.com>
-
-Description:
-------------
-
-Handheld devices from OneNetbook, AOKZOE, AYANEO, And OrangePi provide fan
-readings and fan control through their embedded controllers.
-
-Currently supports OneXPlayer devices, AOKZOE, AYANEO, and OrangePi
-handheld devices. AYANEO devices preceding the AIR and OneXPlayer devices
-preceding the Mini A07 are not supportable as the EC model is different
-and do not have manual control capabilities.
-
-Some OneXPlayer and AOKZOE models have a toggle for changing the behaviour
-of the "Turbo/Silent" button of the device. It will change the key event
-that it triggers with a flip of the `tt_toggle` attribute. See below for
-boards that support this function.
-
-Supported devices
------------------
-
-Currently the driver supports the following handhelds:
-
- - AOKZOE A1
- - AOKZOE A1 PRO
- - AYANEO 2
- - AYANEO 2S
- - AYANEO AIR
- - AYANEO AIR 1S
- - AYANEO AIR Plus (Mendocino)
- - AYANEO AIR Pro
- - AYANEO Flip DS
- - AYANEO Flip KB
- - AYANEO Geek
- - AYANEO Geek 1S
- - AYANEO KUN
- - OneXPlayer 2
- - OneXPlayer 2 Pro
- - OneXPlayer AMD
- - OneXPlayer mini AMD
- - OneXPlayer mini AMD PRO
- - OneXPlayer OneXFly
- - OneXPlayer X1 A
- - OneXPlayer X1 i
- - OneXPlayer X1 mini
- - OrangePi NEO-01
-
-"Turbo/Silent" button behaviour toggle is only supported on:
- - AOK ZOE A1
- - AOK ZOE A1 PRO
- - OneXPlayer 2
- - OneXPlayer 2 Pro
- - OneXPlayer mini AMD (only with updated alpha BIOS)
- - OneXPlayer mini AMD PRO
- - OneXPlayer OneXFly
- - OneXPlayer X1 A
- - OneXPlayer X1 i
- - OneXPlayer X1 mini
-
-Sysfs entries
--------------
-
-The following attributes are supported:
-
-fan1_input
-  Read Only. Reads current fan RPM.
-
-pwm1_enable
-  Read Write. Enable manual fan control. Write "1" to set to manual, write "0"
-  to let the EC control de fan speed. Read this attribute to see current status.
-
-pwm1
-  Read Write. Read this attribute to see current duty cycle in the range [0-255].
-  When pwm1_enable is set to "1" (manual) write any value in the range [0-255]
-  to set fan speed.
-
-tt_toggle
-  Read Write. Read this attribute to check the status of the turbo/silent
-  button behaviour function. Write "1" to activate the switch and "0" to
-  deactivate it. The specific keycodes and behaviour is specific to the device
-  both with this function on and off. This attribute is attached to the platform
-  driver and not to the hwmon driver (/sys/devices/platform/oxp-platform/tt_toggle)
diff --git a/MAINTAINERS b/MAINTAINERS
index ed7aa6867674..e78df39a10a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17638,12 +17638,13 @@ S:	Maintained
 F:	drivers/mtd/nand/onenand/
 F:	include/linux/mtd/onenand*.h
 
-ONEXPLAYER FAN DRIVER
+ONEXPLAYER PLATFORM EC DRIVER
+M:	Antheas Kapenekakis <lkml@antheas.dev>
 M:	Derek John Clark <derekjohn.clark@gmail.com>
 M:	Joaquín Ignacio Aramendía <samsagax@gmail.com>
-L:	linux-hwmon@vger.kernel.org
+L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
-F:	drivers/hwmon/oxp-sensors.c
+F:	drivers/platform/x86/oxpec.c
 
 ONIE TLV NVMEM LAYOUT DRIVER
 M:	Miquel Raynal <miquel.raynal@bootlin.com>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4cbaba15d86e..09f7aed96d15 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1774,17 +1774,6 @@ config SENSORS_NZXT_SMART2
 
 source "drivers/hwmon/occ/Kconfig"
 
-config SENSORS_OXP
-	tristate "OneXPlayer EC fan control"
-	depends on ACPI_EC
-	depends on X86
-	help
-		If you say yes here you get support for fan readings and control over
-		OneXPlayer handheld devices. Only OneXPlayer mini AMD handheld variant
-		boards are supported.
-
-		Can also be built as a module. In that case it will be called oxp-sensors.
-
 config SENSORS_PCF8591
 	tristate "Philips PCF8591 ADC/DAC"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b7ef0f0562d3..0edb08824b17 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -181,7 +181,6 @@ obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
 obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
 obj-$(CONFIG_SENSORS_NZXT_KRAKEN3) += nzxt-kraken3.o
 obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
-obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0258dd879d64..82cfc76bc5c9 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1186,6 +1186,18 @@ config SEL3350_PLATFORM
 	  To compile this driver as a module, choose M here: the module
 	  will be called sel3350-platform.
 
+config OXP_EC
+	tristate "OneXPlayer EC platform control"
+	depends on ACPI_EC
+	depends on HWMON
+	depends on X86
+	help
+		Enables support for the platform EC of OneXPlayer and AOKZOE
+		handheld devices. This includes fan speed, fan controls, and
+		disabling the default TDP behavior of the device. Due to legacy
+		reasons, this driver also provides hwmon functionality to Ayaneo
+		devices and the OrangePi Neo.
+
 endif # X86_PLATFORM_DEVICES
 
 config P2SB
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e1b142947067..f64a191c1162 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
 
 # SEL
 obj-$(CONFIG_SEL3350_PLATFORM)		+= sel3350-platform.o
+
+# OneXPlayer
+obj-$(CONFIG_OXP_EC)		+= oxpec.o
\ No newline at end of file
diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/platform/x86/oxpec.c
similarity index 98%
rename from drivers/hwmon/oxp-sensors.c
rename to drivers/platform/x86/oxpec.c
index f7a64fbc8f33..dc3a0871809c 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/platform/x86/oxpec.c
@@ -1,11 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * Platform driver for OneXPlayer, AOKZOE, AYANEO, and OrangePi Handhelds
- * that expose fan reading and control via hwmon sysfs.
- *
- * Old OXP boards have the same DMI strings and they are told apart by
- * the boot cpu vendor (Intel/AMD). Of these older models only AMD is
- * supported.
+ * Platform driver for OneXPlayer and AOKZOE devices. For the time being,
+ * it also exposes fan controls for AYANEO, and OrangePi Handhelds via
+ * hwmon sysfs.
  *
  * Fan control is provided via pwm interface in the range [0-255].
  * Old AMD boards use [0-100] as range in the EC, the written value is
@@ -16,6 +13,7 @@
  *
  * Copyright (C) 2022 Joaquín I. Aramendía <samsagax@gmail.com>
  * Copyright (C) 2024 Derek J. Clark <derekjohn.clark@gmail.com>
+ * Copyright (C) 2025 Antheas Kapenekakis <lkml@antheas.dev>
  */
 
 #include <linux/acpi.h>
-- 
2.48.1

[PATCH v4 04/13] ABI: testing: add tt_toggle and tt_led entries

[Antheas Kapenekakis]

When tt_toggle was introduced, it was not added to the platform sysfs.
Add it, then add documentation for tt_led.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 Documentation/ABI/testing/sysfs-platform-oxp | 26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-oxp

diff --git a/Documentation/ABI/testing/sysfs-platform-oxp b/Documentation/ABI/testing/sysfs-platform-oxp
new file mode 100644
index 000000000000..776b7dee3d59
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-oxp
@@ -0,0 +1,26 @@
+What:		/sys/devices/platform/<platform>/tt_toggle
+Date:		Jun 2023
+KernelVersion:	6.5
+Contact:	"Antheas Kapenekakis" <lkml@antheas.dev>
+Description:
+		Takeover TDP controls from the device. OneXPlayer devices have a
+		turbo button that can be used to switch between two TDP modes
+		(usually 15W and 25W). By setting this attribute to 1, this
+		functionality is disabled, handing TDP control over to (Windows)
+		userspace software and the Turbo button turns into a keyboard
+		shortcut over the AT keyboard of the device. In addition,
+		using this setting is a prerequisite for PWM control for most
+		newer models (otherwise it NOOPs).
+
+What:		/sys/devices/platform/<platform>/tt_led
+Date:		Feb 2025
+KernelVersion:	6.15
+Contact:	"Antheas Kapenekakis" <lkml@antheas.dev>
+Description:
+		Some OneXPlayer devices (e.g., X1 series) feature a little LED
+		nested in the Turbo button. This LED is illuminated when the
+		device is in the higher TDP mode (e.g., 25W). Once tt_toggle
+		is engaged, this LED is left dangling to its last state. This
+		attribute allows userspace to control the LED state manually
+		(either with 1 or 0). Only a subset of devices contain this LED.
+
-- 
2.48.1

[PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Antheas Kapenekakis]

OneXPlayer devices have a charge bypass feature
that allows the user to select between it being
active always or only when the device is on.

Therefore, add attribute inhibit-charge-s0 to
charge_behaviour to allow the user to select
that bypass should only be on when the device is
in the s0 state.

Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 Documentation/ABI/testing/sysfs-class-power | 11 ++++++-----
 drivers/power/supply/power_supply_sysfs.c   |  1 +
 drivers/power/supply/test_power.c           |  1 +
 include/linux/power_supply.h                |  1 +
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index 2a5c1a09a28f..4a187ca11f92 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -508,11 +508,12 @@ Description:
 		Access: Read, Write
 
 		Valid values:
-			================ ====================================
-			auto:            Charge normally, respect thresholds
-			inhibit-charge:  Do not charge while AC is attached
-			force-discharge: Force discharge while AC is attached
-			================ ====================================
+			================== =====================================
+			auto:              Charge normally, respect thresholds
+			inhibit-charge:    Do not charge while AC is attached
+			inhibit-charge-s0: same as inhibit-charge but only in S0
+			force-discharge:   Force discharge while AC is attached
+			================== =====================================
 
 What:		/sys/class/power_supply/<supply_name>/technology
 Date:		May 2007
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index edb058c19c9c..1a98fc26ce96 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
 static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = {
 	[POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO]		= "auto",
 	[POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE]	= "inhibit-charge",
+	[POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0]	= "inhibit-charge-s0",
 	[POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE]	= "force-discharge",
 };
 
diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 2a975a110f48..4bc5ab84a9d6 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = {
 		.property_is_writeable = test_power_battery_property_is_writeable,
 		.charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
 				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
+				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0)
 				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
 	},
 	[TEST_USB] = {
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 6ed53b292162..b1ca5e148759 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -212,6 +212,7 @@ enum power_supply_usb_type {
 enum power_supply_charge_behaviour {
 	POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0,
 	POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE,
+	POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0,
 	POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE,
 };
 
-- 
2.48.1

[PATCH v4 06/13] platform/x86: oxpec: Add charge threshold and behaviour to OneXPlayer

[Antheas Kapenekakis]

With the X1 (AMD), OneXPlayer added a charge limit and charge bypass to
their devices. Charge limit allows for choosing an arbitrary battery
charge setpoint in percentages. Charge bypass allows to instruct the
device to stop charging either when it is in s0 or always.

This feature was then extended for the F1Pro as well. OneXPlayer also
released BIOS updates for the X1 Mini, X1 (Intel), and F1 devices that
add support for this feature. Therefore, enable it for all F1 and
X1 devices.

Add both of these under the standard sysfs battery endpoints for them,
by looking for the battery. OneXPlayer devices have a single battery.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/Kconfig |   1 +
 drivers/platform/x86/oxpec.c | 217 +++++++++++++++++++++++++++++++++++
 2 files changed, 218 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 82cfc76bc5c9..f4d993658c01 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1189,6 +1189,7 @@ config SEL3350_PLATFORM
 config OXP_EC
 	tristate "OneXPlayer EC platform control"
 	depends on ACPI_EC
+	depends on ACPI_BATTERY
 	depends on HWMON
 	depends on X86
 	help
diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
index dc3a0871809c..d73a10598d8f 100644
--- a/drivers/platform/x86/oxpec.c
+++ b/drivers/platform/x86/oxpec.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/processor.h>
+#include <acpi/battery.h>
 
 /* Handle ACPI lock mechanism */
 static u32 oxp_mutex;
@@ -87,6 +88,24 @@ static enum oxp_board board;
 
 #define OXP_TURBO_RETURN_VAL           0x00 /* Common return val */
 
+/* Battery bypass settings */
+#define EC_CHARGE_CONTROL_BEHAVIOURS_X1	(BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)             | \
+					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)    | \
+					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0))
+
+#define OXP_X1_CHARGE_LIMIT_REG      0xA3 /* X1 charge limit (%) */
+#define OXP_X1_CHARGE_BYPASS_REG     0xA4 /* X1 bypass charging */
+
+#define OXP_X1_CHARGE_BYPASS_MASK_S0 0x01
+/*
+ * Cannot control S3, S5 individually.
+ * X1 Mask is 0x0A, OneXFly F1Pro is just 0x02
+ * but the extra bit on the X1 does nothing.
+ */
+#define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02
+#define OXP_X1_CHARGE_BYPASS_MASK_ALWAYS (OXP_X1_CHARGE_BYPASS_MASK_S0 | \
+	OXP_X1_CHARGE_BYPASS_MASK_S3S5)
+
 static const struct dmi_system_id dmi_table[] = {
 	{
 		.matches = {
@@ -434,6 +453,194 @@ static ssize_t tt_toggle_show(struct device *dev,
 
 static DEVICE_ATTR_RW(tt_toggle);
 
+/* Callbacks for charge behaviour attributes */
+static bool charge_behaviour_supported(void)
+{
+	switch (board) {
+	case oxp_x1:
+	case oxp_fly:
+		return 1;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static ssize_t charge_behaviour_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+			       size_t count)
+{
+	unsigned int available;
+	long val, s0, always;
+	int ret;
+	u8 reg;
+
+	switch (board) {
+	case oxp_x1:
+	case oxp_fly:
+		s0 = OXP_X1_CHARGE_BYPASS_MASK_S0;
+		always = OXP_X1_CHARGE_BYPASS_MASK_ALWAYS;
+		reg = OXP_X1_CHARGE_BYPASS_REG;
+		available = EC_CHARGE_CONTROL_BEHAVIOURS_X1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = power_supply_charge_behaviour_parse(available, buf);
+	if (ret < 0)
+		return ret;
+
+	switch (ret) {
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
+		val = 0;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0:
+		val = s0;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
+		val = always;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = write_to_ec(reg, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t charge_behaviour_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	long val, s0, always, sel;
+	unsigned int available;
+	int ret;
+	u8 reg;
+
+	switch (board) {
+	case oxp_x1:
+	case oxp_fly:
+		s0 = OXP_X1_CHARGE_BYPASS_MASK_S0;
+		always = OXP_X1_CHARGE_BYPASS_MASK_ALWAYS;
+		reg = OXP_X1_CHARGE_BYPASS_REG;
+		available = EC_CHARGE_CONTROL_BEHAVIOURS_X1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = read_from_ec(reg, 1, &val);
+	if (ret)
+		return ret;
+
+	if ((val & always) == always)
+		sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
+	else if ((val & s0) == s0)
+		sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0;
+	else
+		sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+
+	return power_supply_charge_behaviour_show(dev, available, sel, buf);
+}
+
+static DEVICE_ATTR_RW(charge_behaviour);
+
+static ssize_t charge_control_end_threshold_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+			       size_t count)
+{
+	u64 val, reg;
+	int ret;
+
+	ret = kstrtou64(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	if (val > 100)
+		return -EINVAL;
+
+	switch (board) {
+	case oxp_x1:
+	case oxp_fly:
+		reg = OXP_X1_CHARGE_LIMIT_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = write_to_ec(reg, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t charge_control_end_threshold_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	long val;
+	int ret;
+	u8 reg;
+
+	switch (board) {
+	case oxp_x1:
+	case oxp_fly:
+		reg = OXP_X1_CHARGE_LIMIT_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = read_from_ec(reg, 1, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%ld\n", val);
+}
+
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+
+static int oxp_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	/* OneXPlayer devices only have one battery. */
+	if (strcmp(battery->desc->name, "BAT0") != 0 &&
+	    strcmp(battery->desc->name, "BAT1") != 0 &&
+	    strcmp(battery->desc->name, "BATC") != 0 &&
+	    strcmp(battery->desc->name, "BATT") != 0)
+		return -ENODEV;
+
+	if (device_create_file(&battery->dev,
+	    &dev_attr_charge_control_end_threshold))
+		return -ENODEV;
+
+	if (device_create_file(&battery->dev,
+	    &dev_attr_charge_behaviour)) {
+		device_remove_file(&battery->dev,
+				&dev_attr_charge_control_end_threshold);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int oxp_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	device_remove_file(&battery->dev,
+			   &dev_attr_charge_control_end_threshold);
+	device_remove_file(&battery->dev,
+			   &dev_attr_charge_behaviour);
+	return 0;
+}
+
+static struct acpi_battery_hook battery_hook = {
+	.add_battery = oxp_battery_add,
+	.remove_battery = oxp_battery_remove,
+	.name = "OneXPlayer Battery",
+};
+
 /* PWM enable/disable functions */
 static int oxp_pwm_enable(void)
 {
@@ -716,15 +923,25 @@ static int oxp_platform_probe(struct platform_device *pdev)
 	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
 						     &oxp_ec_chip_info, NULL);
 
+	if (charge_behaviour_supported())
+		battery_hook_register(&battery_hook);
+
 	return PTR_ERR_OR_ZERO(hwdev);
 }
 
+static void oxp_platform_remove(struct platform_device *device)
+{
+	if (charge_behaviour_supported())
+		battery_hook_unregister(&battery_hook);
+}
+
 static struct platform_driver oxp_platform_driver = {
 	.driver = {
 		.name = "oxp-platform",
 		.dev_groups = oxp_ec_groups,
 	},
 	.probe = oxp_platform_probe,
+	.remove = oxp_platform_remove,
 };
 
 static struct platform_device *oxp_platform_device;
-- 
2.48.1

[PATCH v4 07/13] platform/x86: oxpec: Rename ec group to tt_toggle

[Antheas Kapenekakis]

Currently, the EC group is used for the turbo button. However, the next
patch in the series adds support for the LED button in X1 devices, which
is only applicable for X1 devices. Therefore, rename it to prepare for
adding the second group.

Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/oxpec.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
index d73a10598d8f..95b0c946b7f8 100644
--- a/drivers/platform/x86/oxpec.c
+++ b/drivers/platform/x86/oxpec.c
@@ -888,18 +888,18 @@ static const struct hwmon_channel_info * const oxp_platform_sensors[] = {
 	NULL,
 };
 
-static struct attribute *oxp_ec_attrs[] = {
+static struct attribute *oxp_tt_toggle_attrs[] = {
 	&dev_attr_tt_toggle.attr,
 	NULL
 };
 
-static struct attribute_group oxp_ec_attribute_group = {
+static struct attribute_group oxp_tt_toggle_attribute_group = {
 	.is_visible = tt_toggle_is_visible,
-	.attrs = oxp_ec_attrs,
+	.attrs = oxp_tt_toggle_attrs,
 };
 
 static const struct attribute_group *oxp_ec_groups[] = {
-	&oxp_ec_attribute_group,
+	&oxp_tt_toggle_attribute_group,
 	NULL
 };
 
-- 
2.48.1

[PATCH v4 08/13] platform/x86: oxpec: Add turbo led support to X1 devices

[Antheas Kapenekakis]

The X1 and X1 mini lineups feature an LED nested within their turbo
button. When turbo takeover is not enabled, the turbo button allows
the device to switch from 18W to 25W TDP. When the device is in the
25W TDP mode, the LED is turned on.

However, when we engage turbo takeover, the turbo led remains on its
last state, which might be illuminated and cannot be currently
controlled. Therefore, add the register that controls it under sysfs,
to allow userspace to turn it off once engaging turbo takeover and
then control it as they wish.

2024 OneXPlayer devices, other than the X1s, do not have a turbo LED.
However, earlier models do, so this can be extended to them as well
when the register for it is found.

Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/oxpec.c | 84 ++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
index 95b0c946b7f8..58a1919aec15 100644
--- a/drivers/platform/x86/oxpec.c
+++ b/drivers/platform/x86/oxpec.c
@@ -106,6 +106,12 @@ static enum oxp_board board;
 #define OXP_X1_CHARGE_BYPASS_MASK_ALWAYS (OXP_X1_CHARGE_BYPASS_MASK_S0 | \
 	OXP_X1_CHARGE_BYPASS_MASK_S3S5)
 
+/* X1 Turbo LED */
+#define OXP_X1_TURBO_LED_REG           0x57
+
+#define OXP_X1_TURBO_LED_OFF           0x01
+#define OXP_X1_TURBO_LED_ON            0x02
+
 static const struct dmi_system_id dmi_table[] = {
 	{
 		.matches = {
@@ -453,6 +459,73 @@ static ssize_t tt_toggle_show(struct device *dev,
 
 static DEVICE_ATTR_RW(tt_toggle);
 
+/* Callbacks for turbo LED attribute */
+static umode_t tt_led_is_visible(struct kobject *kobj,
+				    struct attribute *attr, int n)
+{
+	switch (board) {
+	case oxp_x1:
+		return attr->mode;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static ssize_t tt_led_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+			       size_t count)
+{
+	u8 reg, val;
+	bool value;
+	int rval;
+
+	rval = kstrtobool(buf, &value);
+	if (rval)
+		return rval;
+
+	switch (board) {
+	case oxp_x1:
+		reg = OXP_X1_TURBO_LED_REG;
+		val = value ? OXP_X1_TURBO_LED_ON : OXP_X1_TURBO_LED_OFF;
+		break;
+	default:
+		return -EINVAL;
+	}
+	rval = write_to_ec(reg, val);
+
+	if (rval)
+		return rval;
+
+	return count;
+}
+
+static ssize_t tt_led_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	int retval;
+	long enval;
+	long val;
+	u8 reg;
+
+	switch (board) {
+	case oxp_x1:
+		reg = OXP_X1_TURBO_LED_REG;
+		enval = OXP_X1_TURBO_LED_ON;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	retval = read_from_ec(reg, 1, &val);
+	if (retval)
+		return retval;
+
+	return sysfs_emit(buf, "%d\n", val == enval);
+}
+
+static DEVICE_ATTR_RW(tt_led);
+
 /* Callbacks for charge behaviour attributes */
 static bool charge_behaviour_supported(void)
 {
@@ -898,8 +971,19 @@ static struct attribute_group oxp_tt_toggle_attribute_group = {
 	.attrs = oxp_tt_toggle_attrs,
 };
 
+static struct attribute *oxp_tt_led_attrs[] = {
+	&dev_attr_tt_led.attr,
+	NULL
+};
+
+static struct attribute_group oxp_tt_led_attribute_group = {
+	.is_visible = tt_led_is_visible,
+	.attrs = oxp_tt_led_attrs,
+};
+
 static const struct attribute_group *oxp_ec_groups[] = {
 	&oxp_tt_toggle_attribute_group,
+	&oxp_tt_led_attribute_group,
 	NULL
 };
 
-- 
2.48.1

[PATCH v4 09/13] platform/x86: oxpec: Move pwm_enable read to its own function

[Antheas Kapenekakis]

Currently, this driver breaks ABI by using auto as 0 and manual as 1.
However, for pwm_enable, 0 is full speed, 1 is manual, and 2 is auto.
For the correction to be possible, this means that the pwm_enable
endpoint will need access to both pwm enable and value (as for
the 0th value, the fan needs to be set to full power).

Therefore, begin by moving the current pwm_enable read to its own
function, oxp_pwm_enable.

Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/oxpec.c | 50 +++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
index 58a1919aec15..31c7d0041492 100644
--- a/drivers/platform/x86/oxpec.c
+++ b/drivers/platform/x86/oxpec.c
@@ -766,6 +766,32 @@ static int oxp_pwm_disable(void)
 	}
 }
 
+static int oxp_pwm_read(long *val)
+{
+	switch (board) {
+	case orange_pi_neo:
+		return read_from_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, 1, val);
+	case aok_zoe_a1:
+	case aya_neo_2:
+	case aya_neo_air:
+	case aya_neo_air_1s:
+	case aya_neo_air_plus_mendo:
+	case aya_neo_air_pro:
+	case aya_neo_flip:
+	case aya_neo_geek:
+	case aya_neo_kun:
+	case oxp_2:
+	case oxp_fly:
+	case oxp_mini_amd:
+	case oxp_mini_amd_a07:
+	case oxp_mini_amd_pro:
+	case oxp_x1:
+		return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 /* Callbacks for hwmon interface */
 static umode_t oxp_ec_hwmon_is_visible(const void *drvdata,
 				       enum hwmon_sensor_types type, u32 attr, int channel)
@@ -863,29 +889,7 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 			}
 			return 0;
 		case hwmon_pwm_enable:
-			switch (board) {
-			case orange_pi_neo:
-				return read_from_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, 1, val);
-			case aok_zoe_a1:
-			case aya_neo_2:
-			case aya_neo_air:
-			case aya_neo_air_1s:
-			case aya_neo_air_plus_mendo:
-			case aya_neo_air_pro:
-			case aya_neo_flip:
-			case aya_neo_geek:
-			case aya_neo_kun:
-			case oxp_2:
-			case oxp_fly:
-			case oxp_mini_amd:
-			case oxp_mini_amd_a07:
-			case oxp_mini_amd_pro:
-			case oxp_x1:
-				return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
-			default:
-				break;
-			}
-			break;
+			return oxp_pwm_read(val);
 		default:
 			break;
 		}
-- 
2.48.1

[PATCH v4 10/13] platform/x86: oxpec: Move pwm value read/write to separate functions

[Antheas Kapenekakis]

Currently, this driver breaks hwmon ABI by using auto as 0 and manual
as 1. However, for pwm_enable, 0 is full speed, 1 is manual, and 2 is
auto. For the correction to be possible, this means that the pwm_enable
endpoint will need access to both pwm enable and value (as for
the 0th value, the fan needs to be set to full power).

Therefore, move the pwm value read/write to separate functions.

Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/oxpec.c | 162 +++++++++++++++++++----------------
 1 file changed, 87 insertions(+), 75 deletions(-)

diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
index 31c7d0041492..f0e70205ad19 100644
--- a/drivers/platform/x86/oxpec.c
+++ b/drivers/platform/x86/oxpec.c
@@ -806,6 +806,91 @@ static umode_t oxp_ec_hwmon_is_visible(const void *drvdata,
 	}
 }
 
+/* PWM input read/write functions */
+static int oxp_pwm_input_write(long val)
+{
+	if (val < 0 || val > 255)
+		return -EINVAL;
+	switch (board) {
+	case orange_pi_neo:
+		/* scale to range [1-244] */
+		val = ((val - 1) * 243 / 254) + 1;
+		return write_to_ec(ORANGEPI_SENSOR_PWM_REG, val);
+	case oxp_2:
+	case oxp_x1:
+		/* scale to range [0-184] */
+		val = (val * 184) / 255;
+		return write_to_ec(OXP_SENSOR_PWM_REG, val);
+	case aya_neo_2:
+	case aya_neo_air:
+	case aya_neo_air_1s:
+	case aya_neo_air_plus_mendo:
+	case aya_neo_air_pro:
+	case aya_neo_flip:
+	case aya_neo_geek:
+	case aya_neo_kun:
+	case oxp_mini_amd:
+	case oxp_mini_amd_a07:
+		/* scale to range [0-100] */
+		val = (val * 100) / 255;
+		return write_to_ec(OXP_SENSOR_PWM_REG, val);
+	case aok_zoe_a1:
+	case oxp_fly:
+	case oxp_mini_amd_pro:
+		return write_to_ec(OXP_SENSOR_PWM_REG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int oxp_pwm_input_read(long *val)
+{
+	int ret;
+
+	switch (board) {
+	case orange_pi_neo:
+		ret = read_from_ec(ORANGEPI_SENSOR_PWM_REG, 1, val);
+		if (ret)
+			return ret;
+		/* scale from range [1-244] */
+		*val = ((*val - 1) * 254 / 243) + 1;
+		break;
+	case oxp_2:
+	case oxp_x1:
+		ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
+		if (ret)
+			return ret;
+		/* scale from range [0-184] */
+		*val = (*val * 255) / 184;
+		break;
+	case aya_neo_2:
+	case aya_neo_air:
+	case aya_neo_air_1s:
+	case aya_neo_air_plus_mendo:
+	case aya_neo_air_pro:
+	case aya_neo_flip:
+	case aya_neo_geek:
+	case aya_neo_kun:
+	case oxp_mini_amd:
+	case oxp_mini_amd_a07:
+		ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
+		if (ret)
+			return ret;
+		/* scale from range [0-100] */
+		*val = (*val * 255) / 100;
+		break;
+	case aok_zoe_a1:
+	case oxp_fly:
+	case oxp_mini_amd_pro:
+	default:
+		ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
+		if (ret)
+			return ret;
+		break;
+	}
+	return 0;
+}
+
 static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 			     u32 attr, int channel, long *val)
 {
@@ -846,48 +931,7 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 	case hwmon_pwm:
 		switch (attr) {
 		case hwmon_pwm_input:
-			switch (board) {
-			case orange_pi_neo:
-				ret = read_from_ec(ORANGEPI_SENSOR_PWM_REG, 1, val);
-				if (ret)
-					return ret;
-				/* scale from range [1-244] */
-				*val = ((*val - 1) * 254 / 243) + 1;
-				break;
-			case oxp_2:
-			case oxp_x1:
-				ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
-				if (ret)
-					return ret;
-				/* scale from range [0-184] */
-				*val = (*val * 255) / 184;
-				break;
-			case aya_neo_2:
-			case aya_neo_air:
-			case aya_neo_air_1s:
-			case aya_neo_air_plus_mendo:
-			case aya_neo_air_pro:
-			case aya_neo_flip:
-			case aya_neo_geek:
-			case aya_neo_kun:
-			case oxp_mini_amd:
-			case oxp_mini_amd_a07:
-				ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
-				if (ret)
-					return ret;
-				/* scale from range [0-100] */
-				*val = (*val * 255) / 100;
-				break;
-			case aok_zoe_a1:
-			case oxp_fly:
-			case oxp_mini_amd_pro:
-			default:
-				ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
-				if (ret)
-					return ret;
-				break;
-			}
-			return 0;
+			return oxp_pwm_input_read(val);
 		case hwmon_pwm_enable:
 			return oxp_pwm_read(val);
 		default:
@@ -913,39 +957,7 @@ static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
 				return oxp_pwm_disable();
 			return -EINVAL;
 		case hwmon_pwm_input:
-			if (val < 0 || val > 255)
-				return -EINVAL;
-			switch (board) {
-			case orange_pi_neo:
-				/* scale to range [1-244] */
-				val = ((val - 1) * 243 / 254) + 1;
-				return write_to_ec(ORANGEPI_SENSOR_PWM_REG, val);
-			case oxp_2:
-			case oxp_x1:
-				/* scale to range [0-184] */
-				val = (val * 184) / 255;
-				return write_to_ec(OXP_SENSOR_PWM_REG, val);
-			case aya_neo_2:
-			case aya_neo_air:
-			case aya_neo_air_1s:
-			case aya_neo_air_plus_mendo:
-			case aya_neo_air_pro:
-			case aya_neo_flip:
-			case aya_neo_geek:
-			case aya_neo_kun:
-			case oxp_mini_amd:
-			case oxp_mini_amd_a07:
-				/* scale to range [0-100] */
-				val = (val * 100) / 255;
-				return write_to_ec(OXP_SENSOR_PWM_REG, val);
-			case aok_zoe_a1:
-			case oxp_fly:
-			case oxp_mini_amd_pro:
-				return write_to_ec(OXP_SENSOR_PWM_REG, val);
-			default:
-				break;
-			}
-			break;
+			return oxp_pwm_input_write(val);
 		default:
 			break;
 		}
-- 
2.48.1

[PATCH v4 11/13] platform/x86: oxpec: Move fan speed read to separate function

[Antheas Kapenekakis]

While not necessary for fixing the ABI hwmon issue, fan speed will be
the only remaining value without a function. Therefore, finish the
refactor by moving it to a separate function.

Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/oxpec.c | 53 ++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
index f0e70205ad19..97dc7b7c70c7 100644
--- a/drivers/platform/x86/oxpec.c
+++ b/drivers/platform/x86/oxpec.c
@@ -806,6 +806,34 @@ static umode_t oxp_ec_hwmon_is_visible(const void *drvdata,
 	}
 }
 
+/* Fan speed read function */
+static int oxp_pwm_fan_speed(long *val)
+{
+	switch (board) {
+	case orange_pi_neo:
+		return read_from_ec(ORANGEPI_SENSOR_FAN_REG, 2, val);
+	case oxp_2:
+	case oxp_x1:
+		return read_from_ec(OXP_2_SENSOR_FAN_REG, 2, val);
+	case aok_zoe_a1:
+	case aya_neo_2:
+	case aya_neo_air:
+	case aya_neo_air_1s:
+	case aya_neo_air_plus_mendo:
+	case aya_neo_air_pro:
+	case aya_neo_flip:
+	case aya_neo_geek:
+	case aya_neo_kun:
+	case oxp_fly:
+	case oxp_mini_amd:
+	case oxp_mini_amd_a07:
+	case oxp_mini_amd_pro:
+		return read_from_ec(OXP_SENSOR_FAN_REG, 2, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 /* PWM input read/write functions */
 static int oxp_pwm_input_write(long val)
 {
@@ -900,30 +928,7 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 	case hwmon_fan:
 		switch (attr) {
 		case hwmon_fan_input:
-			switch (board) {
-			case orange_pi_neo:
-				return read_from_ec(ORANGEPI_SENSOR_FAN_REG, 2, val);
-			case oxp_2:
-			case oxp_x1:
-				return read_from_ec(OXP_2_SENSOR_FAN_REG, 2, val);
-			case aok_zoe_a1:
-			case aya_neo_2:
-			case aya_neo_air:
-			case aya_neo_air_1s:
-			case aya_neo_air_plus_mendo:
-			case aya_neo_air_pro:
-			case aya_neo_flip:
-			case aya_neo_geek:
-			case aya_neo_kun:
-			case oxp_fly:
-			case oxp_mini_amd:
-			case oxp_mini_amd_a07:
-			case oxp_mini_amd_pro:
-				return read_from_ec(OXP_SENSOR_FAN_REG, 2, val);
-			default:
-				break;
-			}
-			break;
+			return oxp_pwm_fan_speed(val);
 		default:
 			break;
 		}
-- 
2.48.1

[PATCH v4 12/13] platform/x86: oxpec: Adhere to sysfs-class-hwmon and enable pwm on 2

[Antheas Kapenekakis]

Currently, the driver does not adhere to the sysfs-class-hwmon
specification: 0 is used for auto fan control and 1 is used for manual
control. However, it is expected that 0 sets the fan to full speed,
1 sets the fan to manual, and then 2 is used for automatic control.

Therefore, change the sysfs API to reflect this and enable pwm on 2.

As we are breaking the ABI for this driver, rename oxpec to oxp_ec,
reflecting the naming convention used by other drivers, to allow for
a smooth migration in current userspace programs.

Closes: https://lore.kernel.org/linux-hwmon/20241027174836.8588-1-derekjohn.clark@gmail.com/
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/oxpec.c | 37 ++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
index 97dc7b7c70c7..273589f8b2fa 100644
--- a/drivers/platform/x86/oxpec.c
+++ b/drivers/platform/x86/oxpec.c
@@ -938,7 +938,27 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 		case hwmon_pwm_input:
 			return oxp_pwm_input_read(val);
 		case hwmon_pwm_enable:
-			return oxp_pwm_read(val);
+			ret = oxp_pwm_read(val);
+			if (ret)
+				return ret;
+
+			/* Check for auto and return 2 */
+			if (!*val) {
+				*val = 2;
+				return 0;
+			}
+
+			/* Return 0 if at full fan speed, 1 otherwise */
+			ret = oxp_pwm_fan_speed(val);
+			if (ret)
+				return ret;
+
+			if (*val == 255)
+				*val = 0;
+			else
+				*val = 1;
+
+			return 0;
 		default:
 			break;
 		}
@@ -952,15 +972,24 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
 			      u32 attr, int channel, long val)
 {
+	int ret;
+
 	switch (type) {
 	case hwmon_pwm:
 		switch (attr) {
 		case hwmon_pwm_enable:
 			if (val == 1)
 				return oxp_pwm_enable();
-			else if (val == 0)
+			else if (val == 2)
 				return oxp_pwm_disable();
-			return -EINVAL;
+			else if (val != 0)
+				return -EINVAL;
+
+			/* Enable PWM and set to max speed */
+			ret = oxp_pwm_enable();
+			if (ret)
+				return ret;
+			return oxp_pwm_input_write(255);
 		case hwmon_pwm_input:
 			return oxp_pwm_input_write(val);
 		default:
@@ -1025,7 +1054,7 @@ static int oxp_platform_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device *hwdev;
 
-	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
+	hwdev = devm_hwmon_device_register_with_info(dev, "oxp_ec", NULL,
 						     &oxp_ec_chip_info, NULL);
 
 	if (charge_behaviour_supported())
-- 
2.48.1

[PATCH v4 13/13] platform/x86: oxpec: Follow reverse xmas convention for tt_toggle

[Antheas Kapenekakis]

Since the rest of the driver follows this convention, apply it to the
tt_toggle attribute as well.

Suggested-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
 drivers/platform/x86/oxpec.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
index 273589f8b2fa..a3c945c73fc5 100644
--- a/drivers/platform/x86/oxpec.c
+++ b/drivers/platform/x86/oxpec.c
@@ -294,9 +294,9 @@ static const struct dmi_system_id dmi_table[] = {
 /* Helper functions to handle EC read/write */
 static int read_from_ec(u8 reg, int size, long *val)
 {
-	int i;
-	int ret;
 	u8 buffer;
+	int ret;
+	int i;
 
 	if (!lock_global_acpi_lock())
 		return -EBUSY;
@@ -408,8 +408,8 @@ static ssize_t tt_toggle_store(struct device *dev,
 			       struct device_attribute *attr, const char *buf,
 			       size_t count)
 {
-	int rval;
 	bool value;
+	int rval;
 
 	rval = kstrtobool(buf, &value);
 	if (rval)
@@ -430,8 +430,8 @@ static ssize_t tt_toggle_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
 	int retval;
-	u8 reg;
 	long val;
+	u8 reg;
 
 	switch (board) {
 	case oxp_mini_amd_a07:
-- 
2.48.1

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Antheas Kapenekakis]

On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>
> OneXPlayer devices have a charge bypass feature
> that allows the user to select between it being
> active always or only when the device is on.
>
> Therefore, add attribute inhibit-charge-s0 to
> charge_behaviour to allow the user to select
> that bypass should only be on when the device is
> in the s0 state.
>
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>  Documentation/ABI/testing/sysfs-class-power | 11 ++++++-----
>  drivers/power/supply/power_supply_sysfs.c   |  1 +
>  drivers/power/supply/test_power.c           |  1 +
>  include/linux/power_supply.h                |  1 +
>  4 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> index 2a5c1a09a28f..4a187ca11f92 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -508,11 +508,12 @@ Description:
>                 Access: Read, Write
>
>                 Valid values:
> -                       ================ ====================================
> -                       auto:            Charge normally, respect thresholds
> -                       inhibit-charge:  Do not charge while AC is attached
> -                       force-discharge: Force discharge while AC is attached
> -                       ================ ====================================
> +                       ================== =====================================
> +                       auto:              Charge normally, respect thresholds
> +                       inhibit-charge:    Do not charge while AC is attached
> +                       inhibit-charge-s0: same as inhibit-charge but only in S0
> +                       force-discharge:   Force discharge while AC is attached
> +                       ================== =====================================
>
>  What:          /sys/class/power_supply/<supply_name>/technology
>  Date:          May 2007
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index edb058c19c9c..1a98fc26ce96 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
>  static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = {
>         [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO]            = "auto",
>         [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE]  = "inhibit-charge",
> +       [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0]       = "inhibit-charge-s0",
>         [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge",
>  };
>
> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> index 2a975a110f48..4bc5ab84a9d6 100644
> --- a/drivers/power/supply/test_power.c
> +++ b/drivers/power/supply/test_power.c
> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = {
>                 .property_is_writeable = test_power_battery_property_is_writeable,
>                 .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
>                                    | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
> +                                  | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0)
>                                    | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
>         },
>         [TEST_USB] = {
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 6ed53b292162..b1ca5e148759 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -212,6 +212,7 @@ enum power_supply_usb_type {
>  enum power_supply_charge_behaviour {
>         POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0,
>         POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE,
> +       POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0,
>         POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE,
>  };
>
> --
> 2.48.1
>

Hi Guenter,
I think I need an ack here, and then someone from platform-x86 to
triage the series.

Do I need to cc anyone extra?

Thanks,
Antheas

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Guenter Roeck]

On 3/16/25 04:40, Antheas Kapenekakis wrote:
> On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>
>> OneXPlayer devices have a charge bypass feature
>> that allows the user to select between it being
>> active always or only when the device is on.
>>
>> Therefore, add attribute inhibit-charge-s0 to
>> charge_behaviour to allow the user to select
>> that bypass should only be on when the device is
>> in the s0 state.
>>
>> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>> ---
>>   Documentation/ABI/testing/sysfs-class-power | 11 ++++++-----
>>   drivers/power/supply/power_supply_sysfs.c   |  1 +
>>   drivers/power/supply/test_power.c           |  1 +
>>   include/linux/power_supply.h                |  1 +
>>   4 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
>> index 2a5c1a09a28f..4a187ca11f92 100644
>> --- a/Documentation/ABI/testing/sysfs-class-power
>> +++ b/Documentation/ABI/testing/sysfs-class-power
>> @@ -508,11 +508,12 @@ Description:
>>                  Access: Read, Write
>>
>>                  Valid values:
>> -                       ================ ====================================
>> -                       auto:            Charge normally, respect thresholds
>> -                       inhibit-charge:  Do not charge while AC is attached
>> -                       force-discharge: Force discharge while AC is attached
>> -                       ================ ====================================
>> +                       ================== =====================================
>> +                       auto:              Charge normally, respect thresholds
>> +                       inhibit-charge:    Do not charge while AC is attached
>> +                       inhibit-charge-s0: same as inhibit-charge but only in S0
>> +                       force-discharge:   Force discharge while AC is attached
>> +                       ================== =====================================
>>
>>   What:          /sys/class/power_supply/<supply_name>/technology
>>   Date:          May 2007
>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>> index edb058c19c9c..1a98fc26ce96 100644
>> --- a/drivers/power/supply/power_supply_sysfs.c
>> +++ b/drivers/power/supply/power_supply_sysfs.c
>> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
>>   static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = {
>>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO]            = "auto",
>>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE]  = "inhibit-charge",
>> +       [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0]       = "inhibit-charge-s0",
>>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge",
>>   };
>>
>> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
>> index 2a975a110f48..4bc5ab84a9d6 100644
>> --- a/drivers/power/supply/test_power.c
>> +++ b/drivers/power/supply/test_power.c
>> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = {
>>                  .property_is_writeable = test_power_battery_property_is_writeable,
>>                  .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
>>                                     | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
>> +                                  | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0)
>>                                     | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
>>          },
>>          [TEST_USB] = {
>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> index 6ed53b292162..b1ca5e148759 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -212,6 +212,7 @@ enum power_supply_usb_type {
>>   enum power_supply_charge_behaviour {
>>          POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0,
>>          POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE,
>> +       POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0,
>>          POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE,
>>   };
>>
>> --
>> 2.48.1
>>
> 
> Hi Guenter,
> I think I need an ack here, and then someone from platform-x86 to
> triage the series.
> 
> Do I need to cc anyone extra?
> 

You need to cc the maintainers of affected subsystems. Copying the mailing
list is insufficient.

Guenter

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Antheas Kapenekakis]

On Sun, 16 Mar 2025 at 14:56, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 3/16/25 04:40, Antheas Kapenekakis wrote:
> > On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote:
> >>
> >> OneXPlayer devices have a charge bypass feature
> >> that allows the user to select between it being
> >> active always or only when the device is on.
> >>
> >> Therefore, add attribute inhibit-charge-s0 to
> >> charge_behaviour to allow the user to select
> >> that bypass should only be on when the device is
> >> in the s0 state.
> >>
> >> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> >> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> >> ---
> >>   Documentation/ABI/testing/sysfs-class-power | 11 ++++++-----
> >>   drivers/power/supply/power_supply_sysfs.c   |  1 +
> >>   drivers/power/supply/test_power.c           |  1 +
> >>   include/linux/power_supply.h                |  1 +
> >>   4 files changed, 9 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> >> index 2a5c1a09a28f..4a187ca11f92 100644
> >> --- a/Documentation/ABI/testing/sysfs-class-power
> >> +++ b/Documentation/ABI/testing/sysfs-class-power
> >> @@ -508,11 +508,12 @@ Description:
> >>                  Access: Read, Write
> >>
> >>                  Valid values:
> >> -                       ================ ====================================
> >> -                       auto:            Charge normally, respect thresholds
> >> -                       inhibit-charge:  Do not charge while AC is attached
> >> -                       force-discharge: Force discharge while AC is attached
> >> -                       ================ ====================================
> >> +                       ================== =====================================
> >> +                       auto:              Charge normally, respect thresholds
> >> +                       inhibit-charge:    Do not charge while AC is attached
> >> +                       inhibit-charge-s0: same as inhibit-charge but only in S0
> >> +                       force-discharge:   Force discharge while AC is attached
> >> +                       ================== =====================================
> >>
> >>   What:          /sys/class/power_supply/<supply_name>/technology
> >>   Date:          May 2007
> >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> >> index edb058c19c9c..1a98fc26ce96 100644
> >> --- a/drivers/power/supply/power_supply_sysfs.c
> >> +++ b/drivers/power/supply/power_supply_sysfs.c
> >> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
> >>   static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = {
> >>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO]            = "auto",
> >>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE]  = "inhibit-charge",
> >> +       [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0]       = "inhibit-charge-s0",
> >>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge",
> >>   };
> >>
> >> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> >> index 2a975a110f48..4bc5ab84a9d6 100644
> >> --- a/drivers/power/supply/test_power.c
> >> +++ b/drivers/power/supply/test_power.c
> >> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = {
> >>                  .property_is_writeable = test_power_battery_property_is_writeable,
> >>                  .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
> >>                                     | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
> >> +                                  | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0)
> >>                                     | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
> >>          },
> >>          [TEST_USB] = {
> >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> >> index 6ed53b292162..b1ca5e148759 100644
> >> --- a/include/linux/power_supply.h
> >> +++ b/include/linux/power_supply.h
> >> @@ -212,6 +212,7 @@ enum power_supply_usb_type {
> >>   enum power_supply_charge_behaviour {
> >>          POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0,
> >>          POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE,
> >> +       POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0,
> >>          POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE,
> >>   };
> >>
> >> --
> >> 2.48.1
> >>
> >
> > Hi Guenter,
> > I think I need an ack here, and then someone from platform-x86 to
> > triage the series.
> >
> > Do I need to cc anyone extra?
> >
>
> You need to cc the maintainers of affected subsystems. Copying the mailing
> list is insufficient.
>
> Guenter
>

Can you tell me who to cc from platform-x86 and linux-pm?

Is it Armin and Rafael?

Best,
Antheas

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Antheas Kapenekakis]

On Sun, 16 Mar 2025 at 17:50, Derek John Clark
<derekjohn.clark@gmail.com> wrote:
>
>
>
> On Sun, Mar 16, 2025, 09:47 Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>
>> On Sun, 16 Mar 2025 at 14:56, Guenter Roeck <linux@roeck-us.net> wrote:
>> >
>> > On 3/16/25 04:40, Antheas Kapenekakis wrote:
>> > > On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>> > >>
>> > >> OneXPlayer devices have a charge bypass feature
>> > >> that allows the user to select between it being
>> > >> active always or only when the device is on.
>> > >>
>> > >> Therefore, add attribute inhibit-charge-s0 to
>> > >> charge_behaviour to allow the user to select
>> > >> that bypass should only be on when the device is
>> > >> in the s0 state.
>> > >>
>> > >> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
>> > >> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>> > >> ---
>> > >>   Documentation/ABI/testing/sysfs-class-power | 11 ++++++-----
>> > >>   drivers/power/supply/power_supply_sysfs.c   |  1 +
>> > >>   drivers/power/supply/test_power.c           |  1 +
>> > >>   include/linux/power_supply.h                |  1 +
>> > >>   4 files changed, 9 insertions(+), 5 deletions(-)
>> > >>
>> > >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
>> > >> index 2a5c1a09a28f..4a187ca11f92 100644
>> > >> --- a/Documentation/ABI/testing/sysfs-class-power
>> > >> +++ b/Documentation/ABI/testing/sysfs-class-power
>> > >> @@ -508,11 +508,12 @@ Description:
>> > >>                  Access: Read, Write
>> > >>
>> > >>                  Valid values:
>> > >> -                       ================ ====================================
>> > >> -                       auto:            Charge normally, respect thresholds
>> > >> -                       inhibit-charge:  Do not charge while AC is attached
>> > >> -                       force-discharge: Force discharge while AC is attached
>> > >> -                       ================ ====================================
>> > >> +                       ================== =====================================
>> > >> +                       auto:              Charge normally, respect thresholds
>> > >> +                       inhibit-charge:    Do not charge while AC is attached
>> > >> +                       inhibit-charge-s0: same as inhibit-charge but only in S0
>> > >> +                       force-discharge:   Force discharge while AC is attached
>> > >> +                       ================== =====================================
>> > >>
>> > >>   What:          /sys/class/power_supply/<supply_name>/technology
>> > >>   Date:          May 2007
>> > >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>> > >> index edb058c19c9c..1a98fc26ce96 100644
>> > >> --- a/drivers/power/supply/power_supply_sysfs.c
>> > >> +++ b/drivers/power/supply/power_supply_sysfs.c
>> > >> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
>> > >>   static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = {
>> > >>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO]            = "auto",
>> > >>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE]  = "inhibit-charge",
>> > >> +       [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0]       = "inhibit-charge-s0",
>> > >>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge",
>> > >>   };
>> > >>
>> > >> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
>> > >> index 2a975a110f48..4bc5ab84a9d6 100644
>> > >> --- a/drivers/power/supply/test_power.c
>> > >> +++ b/drivers/power/supply/test_power.c
>> > >> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = {
>> > >>                  .property_is_writeable = test_power_battery_property_is_writeable,
>> > >>                  .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
>> > >>                                     | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
>> > >> +                                  | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0)
>> > >>                                     | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
>> > >>          },
>> > >>          [TEST_USB] = {
>> > >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> > >> index 6ed53b292162..b1ca5e148759 100644
>> > >> --- a/include/linux/power_supply.h
>> > >> +++ b/include/linux/power_supply.h
>> > >> @@ -212,6 +212,7 @@ enum power_supply_usb_type {
>> > >>   enum power_supply_charge_behaviour {
>> > >>          POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0,
>> > >>          POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE,
>> > >> +       POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0,
>> > >>          POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE,
>> > >>   };
>> > >>
>> > >> --
>> > >> 2.48.1
>> > >>
>> > >
>> > > Hi Guenter,
>> > > I think I need an ack here, and then someone from platform-x86 to
>> > > triage the series.
>> > >
>> > > Do I need to cc anyone extra?
>> > >
>> >
>> > You need to cc the maintainers of affected subsystems. Copying the mailing
>> > list is insufficient.
>> >
>> > Guenter
>> >
>>
>> Can you tell me who to cc from platform-x86 and linux-pm?
>>
>> Is it Armin and Rafael?
>
>
> Hans, Ilpo, Armin, and Mario
>
> - Derek

Sure. Full series on [1].

Antheas

btw, your reply was non-text.

[1] https://lore.kernel.org/all/20250311165406.331046-1-lkml@antheas.dev/

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Guenter Roeck]

On 3/16/25 09:46, Antheas Kapenekakis wrote:
[ ... ]
>>> Do I need to cc anyone extra?
>>>
>>
>> You need to cc the maintainers of affected subsystems. Copying the mailing
>> list is insufficient.
>>
>> Guenter
>>
> 
> Can you tell me who to cc from platform-x86 and linux-pm?
> 

Just use scripts/get_maintainer.pl.

Guenter

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Hans de Goede]

Hi Antheas,

Thanks you for your work on this.

On 16-Mar-25 17:46, Antheas Kapenekakis wrote:
> On Sun, 16 Mar 2025 at 14:56, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 3/16/25 04:40, Antheas Kapenekakis wrote:
>>> On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>>>>
>>>> OneXPlayer devices have a charge bypass feature
>>>> that allows the user to select between it being
>>>> active always or only when the device is on.
>>>>
>>>> Therefore, add attribute inhibit-charge-s0 to
>>>> charge_behaviour to allow the user to select
>>>> that bypass should only be on when the device is
>>>> in the s0 state.
>>>>
>>>> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
>>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>>> ---
>>>>   Documentation/ABI/testing/sysfs-class-power | 11 ++++++-----
>>>>   drivers/power/supply/power_supply_sysfs.c   |  1 +
>>>>   drivers/power/supply/test_power.c           |  1 +
>>>>   include/linux/power_supply.h                |  1 +
>>>>   4 files changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
>>>> index 2a5c1a09a28f..4a187ca11f92 100644
>>>> --- a/Documentation/ABI/testing/sysfs-class-power
>>>> +++ b/Documentation/ABI/testing/sysfs-class-power
>>>> @@ -508,11 +508,12 @@ Description:
>>>>                  Access: Read, Write
>>>>
>>>>                  Valid values:
>>>> -                       ================ ====================================
>>>> -                       auto:            Charge normally, respect thresholds
>>>> -                       inhibit-charge:  Do not charge while AC is attached
>>>> -                       force-discharge: Force discharge while AC is attached
>>>> -                       ================ ====================================
>>>> +                       ================== =====================================
>>>> +                       auto:              Charge normally, respect thresholds
>>>> +                       inhibit-charge:    Do not charge while AC is attached
>>>> +                       inhibit-charge-s0: same as inhibit-charge but only in S0
>>>> +                       force-discharge:   Force discharge while AC is attached
>>>> +                       ================== =====================================
>>>>
>>>>   What:          /sys/class/power_supply/<supply_name>/technology
>>>>   Date:          May 2007
>>>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>>>> index edb058c19c9c..1a98fc26ce96 100644
>>>> --- a/drivers/power/supply/power_supply_sysfs.c
>>>> +++ b/drivers/power/supply/power_supply_sysfs.c
>>>> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
>>>>   static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = {
>>>>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO]            = "auto",
>>>>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE]  = "inhibit-charge",
>>>> +       [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0]       = "inhibit-charge-s0",
>>>>          [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge",
>>>>   };
>>>>
>>>> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
>>>> index 2a975a110f48..4bc5ab84a9d6 100644
>>>> --- a/drivers/power/supply/test_power.c
>>>> +++ b/drivers/power/supply/test_power.c
>>>> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = {
>>>>                  .property_is_writeable = test_power_battery_property_is_writeable,
>>>>                  .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
>>>>                                     | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
>>>> +                                  | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0)
>>>>                                     | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
>>>>          },
>>>>          [TEST_USB] = {
>>>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>>>> index 6ed53b292162..b1ca5e148759 100644
>>>> --- a/include/linux/power_supply.h
>>>> +++ b/include/linux/power_supply.h
>>>> @@ -212,6 +212,7 @@ enum power_supply_usb_type {
>>>>   enum power_supply_charge_behaviour {
>>>>          POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0,
>>>>          POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE,
>>>> +       POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0,
>>>>          POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE,
>>>>   };
>>>>
>>>> --
>>>> 2.48.1
>>>>
>>>
>>> Hi Guenter,
>>> I think I need an ack here, and then someone from platform-x86 to
>>> triage the series.
>>>
>>> Do I need to cc anyone extra?
>>>
>>
>> You need to cc the maintainers of affected subsystems. Copying the mailing
>> list is insufficient.
>>
>> Guenter
>>
> 
> Can you tell me who to cc from platform-x86 and linux-pm?

Sebastian Reichel <sre@kernel.org> (maintainer:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS)
linux-pm@vger.kernel.org (open list:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS)
linux-kernel@vger.kernel.org (open list)

And something which the tools will not tell you, since charge_behaviour support
was added by Thomas Thomas should be added to the Cc too:

Thomas Weißschuh <linux@weissschuh.net>

Note I also have some generic remarks about this patch, I'll do a top-level
reply to the patch for those.

Regards,

Hans

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Hans de Goede]

Hi Antheas,

On 11-Mar-25 17:53, Antheas Kapenekakis wrote:
> OneXPlayer devices have a charge bypass

The term "charge bypass" is typically used for the case where the
external charger gets directly connected to the battery cells,
bypassing the charge-IC inside the device, in making
the external charger directly responsible for battery/charge
management.

Yet you name the feature inhibit charge, so I guess it simply
disables charging of the battery rather then doing an actual
chaerger-IC bypass ?

Assuming I have this correct, please stop using the term
charge-bypass as that has a specific (different) meaning.

> feature
> that allows the user to select between it being
> active always or only when the device is on.
> 
> Therefore, add attribute inhibit-charge-s0 to
> charge_behaviour to allow the user to select
> that bypass should only be on when the device is

Also don't use bypass here please.

> in the s0 state.
> 
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>  Documentation/ABI/testing/sysfs-class-power | 11 ++++++-----
>  drivers/power/supply/power_supply_sysfs.c   |  1 +
>  drivers/power/supply/test_power.c           |  1 +
>  include/linux/power_supply.h                |  1 +
>  4 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> index 2a5c1a09a28f..4a187ca11f92 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -508,11 +508,12 @@ Description:
>  		Access: Read, Write
>  
>  		Valid values:
> -			================ ====================================
> -			auto:            Charge normally, respect thresholds
> -			inhibit-charge:  Do not charge while AC is attached
> -			force-discharge: Force discharge while AC is attached
> -			================ ====================================
> +			================== =====================================
> +			auto:              Charge normally, respect thresholds
> +			inhibit-charge:    Do not charge while AC is attached
> +			inhibit-charge-s0: same as inhibit-charge but only in S0

Only in S0 suggests that charging gets disabled when the device is on / in-use,
I guess this is intended to avoid generating extra heat while the device is on?

What about when the device is suspended, should the battery charge then ?

On x86 we've 2 sorts of suspends S3, and the current name suggests that the
device will charge (no inhibit) then. But modern hw almost always uses
s0i3 / suspend to idle suspend and the name suggests charging would then
still be inhibited?

Also s0 is an ACPI specific term, so basically 2 remarks here:

1. The name should probably be "inhibit-charge-when-on" since the power_supply
   calls is platform agnositic and "S0" is not.

2. We need to clearly define what happens when the device is suspended and then
   make sure that the driver matches this (e.g. if we want to *not* inhibit during
   suspend we may need to turn this feature off during suspend).

Regards,

Hans



> +			force-discharge:   Force discharge while AC is attached
> +			================== =====================================
>  
>  What:		/sys/class/power_supply/<supply_name>/technology
>  Date:		May 2007
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index edb058c19c9c..1a98fc26ce96 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
>  static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = {
>  	[POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO]		= "auto",
>  	[POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE]	= "inhibit-charge",
> +	[POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0]	= "inhibit-charge-s0",
>  	[POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE]	= "force-discharge",
>  };
>  
> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> index 2a975a110f48..4bc5ab84a9d6 100644
> --- a/drivers/power/supply/test_power.c
> +++ b/drivers/power/supply/test_power.c
> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = {
>  		.property_is_writeable = test_power_battery_property_is_writeable,
>  		.charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
>  				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
> +				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0)
>  				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
>  	},
>  	[TEST_USB] = {
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 6ed53b292162..b1ca5e148759 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -212,6 +212,7 @@ enum power_supply_usb_type {
>  enum power_supply_charge_behaviour {
>  	POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0,
>  	POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE,
> +	POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0,
>  	POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE,
>  };
>  

Re: [PATCH v4 06/13] platform/x86: oxpec: Add charge threshold and behaviour to OneXPlayer

[Hans de Goede]

Hi,

On 11-Mar-25 17:53, Antheas Kapenekakis wrote:
> With the X1 (AMD), OneXPlayer added a charge limit and charge bypass to
> their devices. Charge limit allows for choosing an arbitrary battery
> charge setpoint in percentages. Charge bypass allows to instruct the
> device to stop charging either when it is in s0 or always.

Again please don't use the word bypass, use inhibit instead.

> This feature was then extended for the F1Pro as well. OneXPlayer also
> released BIOS updates for the X1 Mini, X1 (Intel), and F1 devices that
> add support for this feature. Therefore, enable it for all F1 and
> X1 devices.
> 
> Add both of these under the standard sysfs battery endpoints for them,
> by looking for the battery. OneXPlayer devices have a single battery.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>  drivers/platform/x86/Kconfig |   1 +
>  drivers/platform/x86/oxpec.c | 217 +++++++++++++++++++++++++++++++++++
>  2 files changed, 218 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 82cfc76bc5c9..f4d993658c01 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1189,6 +1189,7 @@ config SEL3350_PLATFORM
>  config OXP_EC
>  	tristate "OneXPlayer EC platform control"
>  	depends on ACPI_EC
> +	depends on ACPI_BATTERY
>  	depends on HWMON
>  	depends on X86
>  	help
> diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
> index dc3a0871809c..d73a10598d8f 100644
> --- a/drivers/platform/x86/oxpec.c
> +++ b/drivers/platform/x86/oxpec.c
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/processor.h>
> +#include <acpi/battery.h>
>  
>  /* Handle ACPI lock mechanism */
>  static u32 oxp_mutex;
> @@ -87,6 +88,24 @@ static enum oxp_board board;
>  
>  #define OXP_TURBO_RETURN_VAL           0x00 /* Common return val */
>  
> +/* Battery bypass settings */
> +#define EC_CHARGE_CONTROL_BEHAVIOURS_X1	(BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)             | \
> +					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)    | \
> +					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0))
> +
> +#define OXP_X1_CHARGE_LIMIT_REG      0xA3 /* X1 charge limit (%) */
> +#define OXP_X1_CHARGE_BYPASS_REG     0xA4 /* X1 bypass charging */
> +
> +#define OXP_X1_CHARGE_BYPASS_MASK_S0 0x01

Again avoid the word BYPASS please, if OneXPlayer are calling this bypass in their 
own documentation maybe add a note here when defining the registers that OneXPlayer
calls this bypass and then use inhibit from there on.

> +/*
> + * Cannot control S3, S5 individually.
> + * X1 Mask is 0x0A, OneXFly F1Pro is just 0x02
> + * but the extra bit on the X1 does nothing.
> + */
> +#define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02

Ok, so suspend is treated as off, but that is for S3 suspend, what about
s2idle, or does this hw not do s2idle ?

> +#define OXP_X1_CHARGE_BYPASS_MASK_ALWAYS (OXP_X1_CHARGE_BYPASS_MASK_S0 | \
> +	OXP_X1_CHARGE_BYPASS_MASK_S3S5)
> +
>  static const struct dmi_system_id dmi_table[] = {
>  	{
>  		.matches = {
> @@ -434,6 +453,194 @@ static ssize_t tt_toggle_show(struct device *dev,
>  
>  static DEVICE_ATTR_RW(tt_toggle);
>  
> +/* Callbacks for charge behaviour attributes */
> +static bool charge_behaviour_supported(void)
> +{
> +	switch (board) {
> +	case oxp_x1:
> +	case oxp_fly:
> +		return 1;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t charge_behaviour_store(struct device *dev,
> +			       struct device_attribute *attr, const char *buf,
> +			       size_t count)
> +{
> +	unsigned int available;
> +	long val, s0, always;
> +	int ret;
> +	u8 reg;
> +
> +	switch (board) {
> +	case oxp_x1:
> +	case oxp_fly:
> +		s0 = OXP_X1_CHARGE_BYPASS_MASK_S0;
> +		always = OXP_X1_CHARGE_BYPASS_MASK_ALWAYS;
> +		reg = OXP_X1_CHARGE_BYPASS_REG;
> +		available = EC_CHARGE_CONTROL_BEHAVIOURS_X1;
> +		break;

Since these are always the same this does not seem useful, please
use the defines directly below.

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = power_supply_charge_behaviour_parse(available, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
> +		val = 0;
> +		break;
> +	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0:
> +		val = s0;
> +		break;
> +	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
> +		val = always;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = write_to_ec(reg, val);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t charge_behaviour_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	long val, s0, always, sel;
> +	unsigned int available;
> +	int ret;
> +	u8 reg;
> +
> +	switch (board) {
> +	case oxp_x1:
> +	case oxp_fly:
> +		s0 = OXP_X1_CHARGE_BYPASS_MASK_S0;
> +		always = OXP_X1_CHARGE_BYPASS_MASK_ALWAYS;
> +		reg = OXP_X1_CHARGE_BYPASS_REG;
> +		available = EC_CHARGE_CONTROL_BEHAVIOURS_X1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = read_from_ec(reg, 1, &val);
> +	if (ret)
> +		return ret;
> +
> +	if ((val & always) == always)
> +		sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
> +	else if ((val & s0) == s0)
> +		sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0;
> +	else
> +		sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +
> +	return power_supply_charge_behaviour_show(dev, available, sel, buf);
> +}
> +
> +static DEVICE_ATTR_RW(charge_behaviour);
> +
> +static ssize_t charge_control_end_threshold_store(struct device *dev,
> +			       struct device_attribute *attr, const char *buf,
> +			       size_t count)
> +{
> +	u64 val, reg;
> +	int ret;
> +
> +	ret = kstrtou64(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val > 100)
> +		return -EINVAL;
> +
> +	switch (board) {
> +	case oxp_x1:
> +	case oxp_fly:
> +		reg = OXP_X1_CHARGE_LIMIT_REG;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = write_to_ec(reg, val);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t charge_control_end_threshold_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	long val;
> +	int ret;
> +	u8 reg;
> +
> +	switch (board) {
> +	case oxp_x1:
> +	case oxp_fly:
> +		reg = OXP_X1_CHARGE_LIMIT_REG;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = read_from_ec(reg, 1, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%ld\n", val);
> +}
> +
> +static DEVICE_ATTR_RW(charge_control_end_threshold);
> +
> +static int oxp_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	/* OneXPlayer devices only have one battery. */
> +	if (strcmp(battery->desc->name, "BAT0") != 0 &&
> +	    strcmp(battery->desc->name, "BAT1") != 0 &&
> +	    strcmp(battery->desc->name, "BATC") != 0 &&
> +	    strcmp(battery->desc->name, "BATT") != 0)
> +		return -ENODEV;
> +
> +	if (device_create_file(&battery->dev,
> +	    &dev_attr_charge_control_end_threshold))
> +		return -ENODEV;
> +
> +	if (device_create_file(&battery->dev,
> +	    &dev_attr_charge_behaviour)) {
> +		device_remove_file(&battery->dev,
> +				&dev_attr_charge_control_end_threshold);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int oxp_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	device_remove_file(&battery->dev,
> +			   &dev_attr_charge_control_end_threshold);
> +	device_remove_file(&battery->dev,
> +			   &dev_attr_charge_behaviour);
> +	return 0;
> +}
> +
> +static struct acpi_battery_hook battery_hook = {
> +	.add_battery = oxp_battery_add,
> +	.remove_battery = oxp_battery_remove,
> +	.name = "OneXPlayer Battery",
> +};
> +

Since this is new code it should use the new power-supply extension support instead
of the old battery_hook mechanism:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6037802bbae892f3ad0c7b4c4faee39b967e32b0



>  /* PWM enable/disable functions */
>  static int oxp_pwm_enable(void)
>  {
> @@ -716,15 +923,25 @@ static int oxp_platform_probe(struct platform_device *pdev)
>  	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
>  						     &oxp_ec_chip_info, NULL);
>  
> +	if (charge_behaviour_supported())
> +		battery_hook_register(&battery_hook);
> +
>  	return PTR_ERR_OR_ZERO(hwdev);
>  }
>  
> +static void oxp_platform_remove(struct platform_device *device)
> +{
> +	if (charge_behaviour_supported())
> +		battery_hook_unregister(&battery_hook);
> +}
> +
>  static struct platform_driver oxp_platform_driver = {
>  	.driver = {
>  		.name = "oxp-platform",
>  		.dev_groups = oxp_ec_groups,
>  	},
>  	.probe = oxp_platform_probe,
> +	.remove = oxp_platform_remove,
>  };
>  
>  static struct platform_device *oxp_platform_device;

Regards,

Hans

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Antheas Kapenekakis]

On Mon, 17 Mar 2025 at 13:27, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Antheas,
>
> On 11-Mar-25 17:53, Antheas Kapenekakis wrote:
> > OneXPlayer devices have a charge bypass
>
> The term "charge bypass" is typically used for the case where the
> external charger gets directly connected to the battery cells,
> bypassing the charge-IC inside the device, in making
> the external charger directly responsible for battery/charge
> management.
>
> Yet you name the feature inhibit charge, so I guess it simply
> disables charging of the battery rather then doing an actual
> chaerger-IC bypass ?
>
> Assuming I have this correct, please stop using the term
> charge-bypass as that has a specific (different) meaning.

Unfortunately, this is how the feature is called in Windows. On both
OneXPlayer and Ayaneo. Manufacturers are centralizing around that
term.

Under the hood, it should be bypassing the charger circuitry, but it
is not obvious during use. The user behavior mirrors `inhibit-charge`,
as the battery just stops charging, so the endpoint is appropriate.

The previous versions of this patch used the charge_type endpoint.
However, it does not have autodetection, so it needs to be hardcoded
[1].

There is also an ayaneo-platform out-of-tree driver with a PR for that
endpoint, which is where I found out about it [2]. So I thought it
would be more appropriate.

I can rewrite the commit message to not include the word bypass.

[1] https://github.com/hhd-dev/adjustor/blob/6a4a192898c4f9d8495e6554d1f0bc41fef550c7/src/adjustor/drivers/battery/__init__.py#L141-L156
[2] https://github.com/ShadowBlip/ayaneo-platform

> > feature
> > that allows the user to select between it being
> > active always or only when the device is on.
> >
> > Therefore, add attribute inhibit-charge-s0 to
> > charge_behaviour to allow the user to select
> > that bypass should only be on when the device is
>
> Also don't use bypass here please.
>
> > in the s0 state.
> >
> > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> >  Documentation/ABI/testing/sysfs-class-power | 11 ++++++-----
> >  drivers/power/supply/power_supply_sysfs.c   |  1 +
> >  drivers/power/supply/test_power.c           |  1 +
> >  include/linux/power_supply.h                |  1 +
> >  4 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> > index 2a5c1a09a28f..4a187ca11f92 100644
> > --- a/Documentation/ABI/testing/sysfs-class-power
> > +++ b/Documentation/ABI/testing/sysfs-class-power
> > @@ -508,11 +508,12 @@ Description:
> >               Access: Read, Write
> >
> >               Valid values:
> > -                     ================ ====================================
> > -                     auto:            Charge normally, respect thresholds
> > -                     inhibit-charge:  Do not charge while AC is attached
> > -                     force-discharge: Force discharge while AC is attached
> > -                     ================ ====================================
> > +                     ================== =====================================
> > +                     auto:              Charge normally, respect thresholds
> > +                     inhibit-charge:    Do not charge while AC is attached
> > +                     inhibit-charge-s0: same as inhibit-charge but only in S0
>
> Only in S0 suggests that charging gets disabled when the device is on / in-use,
> I guess this is intended to avoid generating extra heat while the device is on?
>
> What about when the device is suspended, should the battery charge then ?
>
> On x86 we've 2 sorts of suspends S3, and the current name suggests that the
> device will charge (no inhibit) then. But modern hw almost always uses
> s0i3 / suspend to idle suspend and the name suggests charging would then
> still be inhibited?
>
> Also s0 is an ACPI specific term, so basically 2 remarks here:
>
> 1. The name should probably be "inhibit-charge-when-on" since the power_supply
>    calls is platform agnositic and "S0" is not.

I tried to be minimal. If we want to make the name longer, I vote for
"inhibit-charge-awake". I can spin a v5 with that.

The device does not charge while asleep. Only when it is off.

> 2. We need to clearly define what happens when the device is suspended and then
>    make sure that the driver matches this (e.g. if we want to *not* inhibit during
>    suspend we may need to turn this feature off during suspend).

This is handled by the device when it comes to OneXPlayer. No driver
changes are needed.

> Regards,
>
> Hans
>
>
> snip
> >
>

Best,
Antheas

Re: [PATCH v4 06/13] platform/x86: oxpec: Add charge threshold and behaviour to OneXPlayer

[Antheas Kapenekakis]

On Mon, 17 Mar 2025 at 13:33, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11-Mar-25 17:53, Antheas Kapenekakis wrote:
> > With the X1 (AMD), OneXPlayer added a charge limit and charge bypass to
> > their devices. Charge limit allows for choosing an arbitrary battery
> > charge setpoint in percentages. Charge bypass allows to instruct the
> > device to stop charging either when it is in s0 or always.
>
> Again please don't use the word bypass, use inhibit instead.
>
> > This feature was then extended for the F1Pro as well. OneXPlayer also
> > released BIOS updates for the X1 Mini, X1 (Intel), and F1 devices that
> > add support for this feature. Therefore, enable it for all F1 and
> > X1 devices.
> >
> > Add both of these under the standard sysfs battery endpoints for them,
> > by looking for the battery. OneXPlayer devices have a single battery.
> >
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> >  drivers/platform/x86/Kconfig |   1 +
> >  drivers/platform/x86/oxpec.c | 217 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 218 insertions(+)
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 82cfc76bc5c9..f4d993658c01 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -1189,6 +1189,7 @@ config SEL3350_PLATFORM
> >  config OXP_EC
> >       tristate "OneXPlayer EC platform control"
> >       depends on ACPI_EC
> > +     depends on ACPI_BATTERY
> >       depends on HWMON
> >       depends on X86
> >       help
> > diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
> > index dc3a0871809c..d73a10598d8f 100644
> > --- a/drivers/platform/x86/oxpec.c
> > +++ b/drivers/platform/x86/oxpec.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/processor.h>
> > +#include <acpi/battery.h>
> >
> >  /* Handle ACPI lock mechanism */
> >  static u32 oxp_mutex;
> > @@ -87,6 +88,24 @@ static enum oxp_board board;
> >
> >  #define OXP_TURBO_RETURN_VAL           0x00 /* Common return val */
> >
> > +/* Battery bypass settings */
> > +#define EC_CHARGE_CONTROL_BEHAVIOURS_X1      (BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)             | \
> > +                                      BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)    | \
> > +                                      BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0))
> > +
> > +#define OXP_X1_CHARGE_LIMIT_REG      0xA3 /* X1 charge limit (%) */
> > +#define OXP_X1_CHARGE_BYPASS_REG     0xA4 /* X1 bypass charging */
> > +
> > +#define OXP_X1_CHARGE_BYPASS_MASK_S0 0x01
>
> Again avoid the word BYPASS please, if OneXPlayer are calling this bypass in their
> own documentation maybe add a note here when defining the registers that OneXPlayer
> calls this bypass and then use inhibit from there on.

Sure, I can do that.

> > +/*
> > + * Cannot control S3, S5 individually.
> > + * X1 Mask is 0x0A, OneXFly F1Pro is just 0x02
> > + * but the extra bit on the X1 does nothing.
> > + */
> > +#define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02
>
> Ok, so suspend is treated as off, but that is for S3 suspend, what about
> s2idle, or does this hw not do s2idle ?

I will change it to OFF. Sorry, s3 is a typo it should be S4.

> > +#define OXP_X1_CHARGE_BYPASS_MASK_ALWAYS (OXP_X1_CHARGE_BYPASS_MASK_S0 | \
> > +     OXP_X1_CHARGE_BYPASS_MASK_S3S5)
> > +
> >  static const struct dmi_system_id dmi_table[] = {
> >       {
> >               .matches = {
> > @@ -434,6 +453,194 @@ static ssize_t tt_toggle_show(struct device *dev,
> >
> >  static DEVICE_ATTR_RW(tt_toggle);
> >
> > +/* Callbacks for charge behaviour attributes */
> > +static bool charge_behaviour_supported(void)
> > +{
> > +     switch (board) {
> > +     case oxp_x1:
> > +     case oxp_fly:
> > +             return 1;
> > +     default:
> > +             break;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static ssize_t charge_behaviour_store(struct device *dev,
> > +                            struct device_attribute *attr, const char *buf,
> > +                            size_t count)
> > +{
> > +     unsigned int available;
> > +     long val, s0, always;
> > +     int ret;
> > +     u8 reg;
> > +
> > +     switch (board) {
> > +     case oxp_x1:
> > +     case oxp_fly:
> > +             s0 = OXP_X1_CHARGE_BYPASS_MASK_S0;
> > +             always = OXP_X1_CHARGE_BYPASS_MASK_ALWAYS;
> > +             reg = OXP_X1_CHARGE_BYPASS_REG;
> > +             available = EC_CHARGE_CONTROL_BEHAVIOURS_X1;
> > +             break;
>
> Since these are always the same this does not seem useful, please
> use the defines directly below.

Ack

> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = power_supply_charge_behaviour_parse(available, buf);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     switch (ret) {
> > +     case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
> > +             val = 0;
> > +             break;
> > +     case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0:
> > +             val = s0;
> > +             break;
> > +     case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
> > +             val = always;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = write_to_ec(reg, val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return count;
> > +}
> > +
> > +static ssize_t charge_behaviour_show(struct device *dev,
> > +                           struct device_attribute *attr, char *buf)
> > +{
> > +     long val, s0, always, sel;
> > +     unsigned int available;
> > +     int ret;
> > +     u8 reg;
> > +
> > +     switch (board) {
> > +     case oxp_x1:
> > +     case oxp_fly:
> > +             s0 = OXP_X1_CHARGE_BYPASS_MASK_S0;
> > +             always = OXP_X1_CHARGE_BYPASS_MASK_ALWAYS;
> > +             reg = OXP_X1_CHARGE_BYPASS_REG;
> > +             available = EC_CHARGE_CONTROL_BEHAVIOURS_X1;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = read_from_ec(reg, 1, &val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if ((val & always) == always)
> > +             sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
> > +     else if ((val & s0) == s0)
> > +             sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0;
> > +     else
> > +             sel = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> > +
> > +     return power_supply_charge_behaviour_show(dev, available, sel, buf);
> > +}
> > +
> > +static DEVICE_ATTR_RW(charge_behaviour);
> > +
> > +static ssize_t charge_control_end_threshold_store(struct device *dev,
> > +                            struct device_attribute *attr, const char *buf,
> > +                            size_t count)
> > +{
> > +     u64 val, reg;
> > +     int ret;
> > +
> > +     ret = kstrtou64(buf, 10, &val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (val > 100)
> > +             return -EINVAL;
> > +
> > +     switch (board) {
> > +     case oxp_x1:
> > +     case oxp_fly:
> > +             reg = OXP_X1_CHARGE_LIMIT_REG;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = write_to_ec(reg, val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return count;
> > +}
> > +
> > +static ssize_t charge_control_end_threshold_show(struct device *dev,
> > +                           struct device_attribute *attr, char *buf)
> > +{
> > +     long val;
> > +     int ret;
> > +     u8 reg;
> > +
> > +     switch (board) {
> > +     case oxp_x1:
> > +     case oxp_fly:
> > +             reg = OXP_X1_CHARGE_LIMIT_REG;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = read_from_ec(reg, 1, &val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return sysfs_emit(buf, "%ld\n", val);
> > +}
> > +
> > +static DEVICE_ATTR_RW(charge_control_end_threshold);
> > +
> > +static int oxp_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> > +{
> > +     /* OneXPlayer devices only have one battery. */
> > +     if (strcmp(battery->desc->name, "BAT0") != 0 &&
> > +         strcmp(battery->desc->name, "BAT1") != 0 &&
> > +         strcmp(battery->desc->name, "BATC") != 0 &&
> > +         strcmp(battery->desc->name, "BATT") != 0)
> > +             return -ENODEV;
> > +
> > +     if (device_create_file(&battery->dev,
> > +         &dev_attr_charge_control_end_threshold))
> > +             return -ENODEV;
> > +
> > +     if (device_create_file(&battery->dev,
> > +         &dev_attr_charge_behaviour)) {
> > +             device_remove_file(&battery->dev,
> > +                             &dev_attr_charge_control_end_threshold);
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int oxp_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
> > +{
> > +     device_remove_file(&battery->dev,
> > +                        &dev_attr_charge_control_end_threshold);
> > +     device_remove_file(&battery->dev,
> > +                        &dev_attr_charge_behaviour);
> > +     return 0;
> > +}
> > +
> > +static struct acpi_battery_hook battery_hook = {
> > +     .add_battery = oxp_battery_add,
> > +     .remove_battery = oxp_battery_remove,
> > +     .name = "OneXPlayer Battery",
> > +};
> > +
>
> Since this is new code it should use the new power-supply extension support instead
> of the old battery_hook mechanism:
>
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6037802bbae892f3ad0c7b4c4faee39b967e32b0
>

Ack

>
> >  /* PWM enable/disable functions */
> >  static int oxp_pwm_enable(void)
> >  {
> > @@ -716,15 +923,25 @@ static int oxp_platform_probe(struct platform_device *pdev)
> >       hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
> >                                                    &oxp_ec_chip_info, NULL);
> >
> > +     if (charge_behaviour_supported())
> > +             battery_hook_register(&battery_hook);
> > +
> >       return PTR_ERR_OR_ZERO(hwdev);
> >  }
> >
> > +static void oxp_platform_remove(struct platform_device *device)
> > +{
> > +     if (charge_behaviour_supported())
> > +             battery_hook_unregister(&battery_hook);
> > +}
> > +
> >  static struct platform_driver oxp_platform_driver = {
> >       .driver = {
> >               .name = "oxp-platform",
> >               .dev_groups = oxp_ec_groups,
> >       },
> >       .probe = oxp_platform_probe,
> > +     .remove = oxp_platform_remove,
> >  };
> >
> >  static struct platform_device *oxp_platform_device;
>
> Regards,
>
> Hans
>
>

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Hans de Goede]

Hi,

On 17-Mar-25 13:38, Antheas Kapenekakis wrote:
> On Mon, 17 Mar 2025 at 13:27, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Antheas,
>>
>> On 11-Mar-25 17:53, Antheas Kapenekakis wrote:
>>> OneXPlayer devices have a charge bypass
>>
>> The term "charge bypass" is typically used for the case where the
>> external charger gets directly connected to the battery cells,
>> bypassing the charge-IC inside the device, in making
>> the external charger directly responsible for battery/charge
>> management.
>>
>> Yet you name the feature inhibit charge, so I guess it simply
>> disables charging of the battery rather then doing an actual
>> chaerger-IC bypass ?
>>
>> Assuming I have this correct, please stop using the term
>> charge-bypass as that has a specific (different) meaning.
> 
> Unfortunately, this is how the feature is called in Windows. On both
> OneXPlayer and Ayaneo. Manufacturers are centralizing around that
> term.

Ok, so I just did a quick duckduckgo for this and it looks like
you are right.

> Under the hood, it should be bypassing the charger circuitry, but it
> is not obvious during use.

Ack reading up on this it seems the idea is not to connect the external
charger directly to the battery to allow fast-charging without
the charge-IC inside the device adding heat, which is the traditional
bypass mode.

Instead the whole battery + charging-IC are cut out of the circuit
(so bypassed) and the charger is now directly powering the device
without the battery acting as a buffer if the power-draw superseeds
what the external charger can deliver.

> The user behavior mirrors `inhibit-charge`,
> as the battery just stops charging, so the endpoint is appropriate.

Hmm this new bypass mode indeed does seem to mirror inhibit charge
from a user pov, but it does more. It reminds me of the battery disconnect
option which some charge-ICs have which just puts the battery FET in
high impedance mode effectively disconnecting the battery. Now that
feature is intended for long term storage of devices with a builtin
battery and it typically also immediately powers off the device ...

Still I wonder if it would make sense to add a new "disconnect"
charge_behaviour or charge_types enum value for this ?




<snip>

>>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
>>> index 2a5c1a09a28f..4a187ca11f92 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-power
>>> +++ b/Documentation/ABI/testing/sysfs-class-power
>>> @@ -508,11 +508,12 @@ Description:
>>>               Access: Read, Write
>>>
>>>               Valid values:
>>> -                     ================ ====================================
>>> -                     auto:            Charge normally, respect thresholds
>>> -                     inhibit-charge:  Do not charge while AC is attached
>>> -                     force-discharge: Force discharge while AC is attached
>>> -                     ================ ====================================
>>> +                     ================== =====================================
>>> +                     auto:              Charge normally, respect thresholds
>>> +                     inhibit-charge:    Do not charge while AC is attached
>>> +                     inhibit-charge-s0: same as inhibit-charge but only in S0
>>
>> Only in S0 suggests that charging gets disabled when the device is on / in-use,
>> I guess this is intended to avoid generating extra heat while the device is on?
>>
>> What about when the device is suspended, should the battery charge then ?
>>
>> On x86 we've 2 sorts of suspends S3, and the current name suggests that the
>> device will charge (no inhibit) then. But modern hw almost always uses
>> s0i3 / suspend to idle suspend and the name suggests charging would then
>> still be inhibited?
>>
>> Also s0 is an ACPI specific term, so basically 2 remarks here:
>>
>> 1. The name should probably be "inhibit-charge-when-on" since the power_supply
>>    calls is platform agnositic and "S0" is not.
> 
> I tried to be minimal. If we want to make the name longer, I vote for
> "inhibit-charge-awake". I can spin a v5 with that.
> 
> The device does not charge while asleep. Only when it is off.

Is suspend awake though ? 

>> 2. We need to clearly define what happens when the device is suspended and then
>>    make sure that the driver matches this (e.g. if we want to *not* inhibit during
>>    suspend we may need to turn this feature off during suspend).
> 
> This is handled by the device when it comes to OneXPlayer. No driver
> changes are needed.

Well you say no charging is done when suspended, the question also is what
behavior do we want here?  I'm fine with the default behaviour, but a case
could be made that charging while suspended might be desirable (dependent on
the use case) in which case we would need to disable the inhibit when
suspending to get the desired behavior.

Also what if other firmware interfaces with a bypass^W inhibit option work
differently and do charge during suspend ?

It is important that we clearly define the expected behavior now so that
future devices can be made to behave the same.

Regards,

Hans

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Antheas Kapenekakis]

On Mon, 17 Mar 2025 at 14:31, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 17-Mar-25 13:38, Antheas Kapenekakis wrote:
> > On Mon, 17 Mar 2025 at 13:27, Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Antheas,
> >>
> >> On 11-Mar-25 17:53, Antheas Kapenekakis wrote:
> >>> OneXPlayer devices have a charge bypass
> >>
> >> The term "charge bypass" is typically used for the case where the
> >> external charger gets directly connected to the battery cells,
> >> bypassing the charge-IC inside the device, in making
> >> the external charger directly responsible for battery/charge
> >> management.
> >>
> >> Yet you name the feature inhibit charge, so I guess it simply
> >> disables charging of the battery rather then doing an actual
> >> chaerger-IC bypass ?
> >>
> >> Assuming I have this correct, please stop using the term
> >> charge-bypass as that has a specific (different) meaning.
> >
> > Unfortunately, this is how the feature is called in Windows. On both
> > OneXPlayer and Ayaneo. Manufacturers are centralizing around that
> > term.
>
> Ok, so I just did a quick duckduckgo for this and it looks like
> you are right.
>
> > Under the hood, it should be bypassing the charger circuitry, but it
> > is not obvious during use.
>
> Ack reading up on this it seems the idea is not to connect the external
> charger directly to the battery to allow fast-charging without
> the charge-IC inside the device adding heat, which is the traditional
> bypass mode.
>
> Instead the whole battery + charging-IC are cut out of the circuit
> (so bypassed) and the charger is now directly powering the device
> without the battery acting as a buffer if the power-draw superseeds
> what the external charger can deliver.
>
> > The user behavior mirrors `inhibit-charge`,
> > as the battery just stops charging, so the endpoint is appropriate.
>
> Hmm this new bypass mode indeed does seem to mirror inhibit charge
> from a user pov, but it does more. It reminds me of the battery disconnect
> option which some charge-ICs have which just puts the battery FET in
> high impedance mode effectively disconnecting the battery. Now that
> feature is intended for long term storage of devices with a builtin
> battery and it typically also immediately powers off the device ...
>
> Still I wonder if it would make sense to add a new "disconnect"
> charge_behaviour or charge_types enum value for this ?
>

The battery is not disconnected. It still provides backup. Unplugging
the charger does not turn off the device. So it is more murky.

From a userspace perspective it is inhibit-charge 1-1.

>
>
> <snip>
>
> >>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> >>> index 2a5c1a09a28f..4a187ca11f92 100644
> >>> --- a/Documentation/ABI/testing/sysfs-class-power
> >>> +++ b/Documentation/ABI/testing/sysfs-class-power
> >>> @@ -508,11 +508,12 @@ Description:
> >>>               Access: Read, Write
> >>>
> >>>               Valid values:
> >>> -                     ================ ====================================
> >>> -                     auto:            Charge normally, respect thresholds
> >>> -                     inhibit-charge:  Do not charge while AC is attached
> >>> -                     force-discharge: Force discharge while AC is attached
> >>> -                     ================ ====================================
> >>> +                     ================== =====================================
> >>> +                     auto:              Charge normally, respect thresholds
> >>> +                     inhibit-charge:    Do not charge while AC is attached
> >>> +                     inhibit-charge-s0: same as inhibit-charge but only in S0
> >>
> >> Only in S0 suggests that charging gets disabled when the device is on / in-use,
> >> I guess this is intended to avoid generating extra heat while the device is on?
> >>
> >> What about when the device is suspended, should the battery charge then ?
> >>
> >> On x86 we've 2 sorts of suspends S3, and the current name suggests that the
> >> device will charge (no inhibit) then. But modern hw almost always uses
> >> s0i3 / suspend to idle suspend and the name suggests charging would then
> >> still be inhibited?
> >>
> >> Also s0 is an ACPI specific term, so basically 2 remarks here:
> >>
> >> 1. The name should probably be "inhibit-charge-when-on" since the power_supply
> >>    calls is platform agnositic and "S0" is not.
> >
> > I tried to be minimal. If we want to make the name longer, I vote for
> > "inhibit-charge-awake". I can spin a v5 with that.
> >
> > The device does not charge while asleep. Only when it is off.
>
> Is suspend awake though ?

Sorry I mispoke. When inhibit-charge-awake, the device only charges
while in s0i0. When inhibit-charge, it never charges. This includes
s0i3, S4, and S5. The devices that support this only support modern
standby.

I just verified this.

> >> 2. We need to clearly define what happens when the device is suspended and then
> >>    make sure that the driver matches this (e.g. if we want to *not* inhibit during
> >>    suspend we may need to turn this feature off during suspend).
> >
> > This is handled by the device when it comes to OneXPlayer. No driver
> > changes are needed.
>
> Well you say no charging is done when suspended, the question also is what
> behavior do we want here?  I'm fine with the default behaviour, but a case
> could be made that charging while suspended might be desirable (dependent on
> the use case) in which case we would need to disable the inhibit when
> suspending to get the desired behavior.
>
> Also what if other firmware interfaces with a bypass^W inhibit option work
> differently and do charge during suspend ?
>
> It is important that we clearly define the expected behavior now so that
> future devices can be made to behave the same.

Sorry I mispoke. Charging happens under modern standby under -awake.

So -awake would mean awake (s0i0) here.

If other devices charge during sleep and awake, another option could be added.

> Regards,
>
> Hans
>
>

Re: [PATCH v4 05/13] power: supply: add inhibit-charge-s0 to charge_behaviour

[Hans de Goede]

Hi,

On 17-Mar-25 14:50, Antheas Kapenekakis wrote:
> On Mon, 17 Mar 2025 at 14:31, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 17-Mar-25 13:38, Antheas Kapenekakis wrote:
>>> On Mon, 17 Mar 2025 at 13:27, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi Antheas,
>>>>
>>>> On 11-Mar-25 17:53, Antheas Kapenekakis wrote:
>>>>> OneXPlayer devices have a charge bypass
>>>>
>>>> The term "charge bypass" is typically used for the case where the
>>>> external charger gets directly connected to the battery cells,
>>>> bypassing the charge-IC inside the device, in making
>>>> the external charger directly responsible for battery/charge
>>>> management.
>>>>
>>>> Yet you name the feature inhibit charge, so I guess it simply
>>>> disables charging of the battery rather then doing an actual
>>>> chaerger-IC bypass ?
>>>>
>>>> Assuming I have this correct, please stop using the term
>>>> charge-bypass as that has a specific (different) meaning.
>>>
>>> Unfortunately, this is how the feature is called in Windows. On both
>>> OneXPlayer and Ayaneo. Manufacturers are centralizing around that
>>> term.
>>
>> Ok, so I just did a quick duckduckgo for this and it looks like
>> you are right.
>>
>>> Under the hood, it should be bypassing the charger circuitry, but it
>>> is not obvious during use.
>>
>> Ack reading up on this it seems the idea is not to connect the external
>> charger directly to the battery to allow fast-charging without
>> the charge-IC inside the device adding heat, which is the traditional
>> bypass mode.
>>
>> Instead the whole battery + charging-IC are cut out of the circuit
>> (so bypassed) and the charger is now directly powering the device
>> without the battery acting as a buffer if the power-draw superseeds
>> what the external charger can deliver.
>>
>>> The user behavior mirrors `inhibit-charge`,
>>> as the battery just stops charging, so the endpoint is appropriate.
>>
>> Hmm this new bypass mode indeed does seem to mirror inhibit charge
>> from a user pov, but it does more. It reminds me of the battery disconnect
>> option which some charge-ICs have which just puts the battery FET in
>> high impedance mode effectively disconnecting the battery. Now that
>> feature is intended for long term storage of devices with a builtin
>> battery and it typically also immediately powers off the device ...
>>
>> Still I wonder if it would make sense to add a new "disconnect"
>> charge_behaviour or charge_types enum value for this ?
>>
> 
> The battery is not disconnected. It still provides backup. Unplugging
> the charger does not turn off the device. So it is more murky.
> 
> From a userspace perspective it is inhibit-charge 1-1.

Ok, lets go with inhibit then.

>> <snip>
>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
>>>>> index 2a5c1a09a28f..4a187ca11f92 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-class-power
>>>>> +++ b/Documentation/ABI/testing/sysfs-class-power
>>>>> @@ -508,11 +508,12 @@ Description:
>>>>>               Access: Read, Write
>>>>>
>>>>>               Valid values:
>>>>> -                     ================ ====================================
>>>>> -                     auto:            Charge normally, respect thresholds
>>>>> -                     inhibit-charge:  Do not charge while AC is attached
>>>>> -                     force-discharge: Force discharge while AC is attached
>>>>> -                     ================ ====================================
>>>>> +                     ================== =====================================
>>>>> +                     auto:              Charge normally, respect thresholds
>>>>> +                     inhibit-charge:    Do not charge while AC is attached
>>>>> +                     inhibit-charge-s0: same as inhibit-charge but only in S0
>>>>
>>>> Only in S0 suggests that charging gets disabled when the device is on / in-use,
>>>> I guess this is intended to avoid generating extra heat while the device is on?
>>>>
>>>> What about when the device is suspended, should the battery charge then ?
>>>>
>>>> On x86 we've 2 sorts of suspends S3, and the current name suggests that the
>>>> device will charge (no inhibit) then. But modern hw almost always uses
>>>> s0i3 / suspend to idle suspend and the name suggests charging would then
>>>> still be inhibited?
>>>>
>>>> Also s0 is an ACPI specific term, so basically 2 remarks here:
>>>>
>>>> 1. The name should probably be "inhibit-charge-when-on" since the power_supply
>>>>    calls is platform agnositic and "S0" is not.
>>>
>>> I tried to be minimal. If we want to make the name longer, I vote for
>>> "inhibit-charge-awake". I can spin a v5 with that.
>>>
>>> The device does not charge while asleep. Only when it is off.
>>
>> Is suspend awake though ?
> 
> Sorry I mispoke. When inhibit-charge-awake, the device only charges
> while in s0i0. When inhibit-charge, it never charges. This includes
> s0i3, S4, and S5. The devices that support this only support modern
> standby.
> 
> I just verified this.

Ok that sounds good / sane, it likely just disables charging while in s0i0
to avoid generating extra heat while in s0i0, so inhibit-charge-awake sounds
good to me.


> 
>>>> 2. We need to clearly define what happens when the device is suspended and then
>>>>    make sure that the driver matches this (e.g. if we want to *not* inhibit during
>>>>    suspend we may need to turn this feature off during suspend).
>>>
>>> This is handled by the device when it comes to OneXPlayer. No driver
>>> changes are needed.
>>
>> Well you say no charging is done when suspended, the question also is what
>> behavior do we want here?  I'm fine with the default behaviour, but a case
>> could be made that charging while suspended might be desirable (dependent on
>> the use case) in which case we would need to disable the inhibit when
>> suspending to get the desired behavior.
>>
>> Also what if other firmware interfaces with a bypass^W inhibit option work
>> differently and do charge during suspend ?
>>
>> It is important that we clearly define the expected behavior now so that
>> future devices can be made to behave the same.
> 
> Sorry I mispoke. Charging happens under modern standby under -awake.
> 
> So -awake would mean awake (s0i0) here.
> 
> If other devices charge during sleep and awake, another option could be added.

ack, as mentioned above inhibit-charge-awake sounds good to me,
thank you for clarifying.

Regards,

Hans