| Message ID | 20250317155349.1236188-14-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | hwmon: (oxpsensors) Add devices, features, fix ABI and move to platform/x86 | expand |
On 2025-03-17 16:53:49+0100, Antheas Kapenekakis wrote: > With the X1 (AMD), OneXPlayer added a charge limit and charge inhibit > feature to their devices. Charge limit allows for choosing an arbitrary > battery charge setpoint in percentages. Charge ihibit allows to instruct inhibit > the device to stop charging either when it is awake 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. What happens for devices without the BIOS update? Can the availability be detected during probe and handled properly? > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > drivers/platform/x86/Kconfig | 1 + > drivers/platform/x86/oxpec.c | 164 ++++++++++++++++++++++++++++++++++- > 2 files changed, 161 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 82cfc76bc5c9f..f4d993658c01f 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 39a29295f9cfe..88d839c2a4834 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; > @@ -60,6 +61,7 @@ enum oxp_board { > }; > > static enum oxp_board board; > +static struct device *oxp_dev; Using a global variable is ugly. An explicit parameter passed through battery_hook_register() -> add_battery() would be nicer. It would require changes to the core code and all its users, though. > > /* Fan reading and PWM */ > #define OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */ > @@ -93,6 +95,23 @@ static enum oxp_board board; > #define OXP_X1_TURBO_LED_OFF 0x01 > #define OXP_X1_TURBO_LED_ON 0x02 > > +/* Battery extension 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_AWAKE)) > + > +#define OXP_X1_CHARGE_LIMIT_REG 0xA3 /* X1 charge limit (%) */ > +#define OXP_X1_CHARGE_INHIBIT_REG 0xA4 /* X1 bypass charging */ > + > +#define OXP_X1_CHARGE_INHIBIT_MASK_AWAKE 0x01 > +/* > + * X1 Mask is 0x0A, OneXFly F1Pro is just 0x02 > + * but the extra bit on the X1 does nothing. > + */ > +#define OXP_X1_CHARGE_INHIBIT_MASK_OFF 0x02 > +#define OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS (OXP_X1_CHARGE_INHIBIT_MASK_AWAKE | \ > + OXP_X1_CHARGE_INHIBIT_MASK_OFF) > + > static const struct dmi_system_id dmi_table[] = { > { > .matches = { > @@ -507,6 +526,136 @@ static ssize_t tt_led_show(struct device *dev, > > static DEVICE_ATTR_RW(tt_led); > > +/* Callbacks for charge behaviour attributes */ > +static bool oxp_psy_ext_supported(void) > +{ > + switch (board) { > + case oxp_x1: > + case oxp_fly: > + return 1; > + default: > + break; > + } > + return 0; For 'bool' use 'true' and 'false. > +} > + > +static int oxp_psy_ext_get_prop(struct power_supply *psy, > + const struct power_supply_ext *ext, > + void *data, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + long raw_val; > + int ret; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: > + ret = read_from_ec(OXP_X1_CHARGE_LIMIT_REG, 1, &raw_val); > + if (ret) > + return ret; > + if (raw_val > 100) > + return -EINVAL; > + val->intval = raw_val; > + return 0; > + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: > + ret = read_from_ec(OXP_X1_CHARGE_INHIBIT_REG, 1, &raw_val); > + if (ret) > + return ret; > + if ((raw_val & OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS) == > + OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS) > + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE; > + else if ((raw_val & OXP_X1_CHARGE_INHIBIT_MASK_AWAKE) == > + OXP_X1_CHARGE_INHIBIT_MASK_AWAKE) > + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_AWAKE; > + else > + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO; > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static int oxp_psy_ext_set_prop(struct power_supply *psy, > + const struct power_supply_ext *ext, > + void *data, > + enum power_supply_property psp, > + const union power_supply_propval *val) > +{ > + long raw_val; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: > + if (val->intval > 100) > + return -EINVAL; > + return write_to_ec(OXP_X1_CHARGE_LIMIT_REG, val->intval); > + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: > + switch (val->intval) { > + case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO: > + raw_val = 0; > + break; > + case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_AWAKE: > + raw_val = OXP_X1_CHARGE_INHIBIT_MASK_AWAKE; > + break; > + case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE: > + raw_val = OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS; > + break; > + default: > + return -EINVAL; > + } > + > + return write_to_ec(OXP_X1_CHARGE_INHIBIT_REG, raw_val); > + default: > + return -EINVAL; > + } > +} > + > +static int oxp_psy_prop_is_writeable(struct power_supply *psy, > + const struct power_supply_ext *ext, > + void *data, > + enum power_supply_property psp) > +{ > + return true; > +} > + > +static const enum power_supply_property oxp_psy_ext_props[] = { > + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, > + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, > +}; > + > +struct power_supply_ext oxp_psy_ext = { static const > + .name = "oxp-charge-control", > + .properties = oxp_psy_ext_props, > + .num_properties = ARRAY_SIZE(oxp_psy_ext_props), > + .charge_behaviours = EC_CHARGE_CONTROL_BEHAVIOURS_X1, The charge control behaviours are named "X1", but nothing else. Seems inconsistent. > + .get_property = oxp_psy_ext_get_prop, > + .set_property = oxp_psy_ext_set_prop, > + .property_is_writeable = oxp_psy_prop_is_writeable, > +}; > + > +static int oxp_add_battery(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 they only have one battery, why is the check necessary? > + > + return power_supply_register_extension(battery, &oxp_psy_ext, oxp_dev, NULL); > +} > + > +static int oxp_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook) > +{ > + power_supply_unregister_extension(battery, &oxp_psy_ext); > + return 0; > +} > + > +static struct acpi_battery_hook battery_hook = { > + .add_battery = oxp_add_battery, > + .remove_battery = oxp_remove_battery, > + .name = "OneXPlayer Battery", This struct can also be aligned. > +}; > + > /* PWM enable/disable functions */ > static int oxp_pwm_enable(void) > { > @@ -845,12 +994,19 @@ static const struct hwmon_chip_info oxp_ec_chip_info = { > static int oxp_platform_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > - struct device *hwdev; > + int ret; > > - hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL, > - &oxp_ec_chip_info, NULL); > + oxp_dev = dev; > + ret = PTR_ERR_OR_ZERO(devm_hwmon_device_register_with_info( > + dev, "oxp_ec", NULL, &oxp_ec_chip_info, NULL)); > > - return PTR_ERR_OR_ZERO(hwdev); > + if (ret) > + return ret; > + > + if (oxp_psy_ext_supported()) > + return devm_battery_hook_register(dev, &battery_hook); If the driver is extended in the future this line will need to be touch again as it is an unconditional early return. This is more future-proof: if (oxp_psy_ext_supported()) { ret = devm_battery_hook_register(dev, &battery_hook); if (ret) return ret; } > + > + return 0; > } > > static struct platform_driver oxp_platform_driver = { > -- > 2.48.1 >
On Mon, 17 Mar 2025 at 19:13, Thomas Weißschuh <linux@weissschuh.net> wrote: > > On 2025-03-17 16:53:49+0100, Antheas Kapenekakis wrote: > > With the X1 (AMD), OneXPlayer added a charge limit and charge inhibit > > feature to their devices. Charge limit allows for choosing an arbitrary > > battery charge setpoint in percentages. Charge ihibit allows to instruct > inhibit > > > the device to stop charging either when it is awake 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. > > What happens for devices without the BIOS update? > Can the availability be detected during probe and handled properly? > > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > drivers/platform/x86/Kconfig | 1 + > > drivers/platform/x86/oxpec.c | 164 ++++++++++++++++++++++++++++++++++- > > 2 files changed, 161 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > index 82cfc76bc5c9f..f4d993658c01f 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 39a29295f9cfe..88d839c2a4834 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; > > @@ -60,6 +61,7 @@ enum oxp_board { > > }; > > > > static enum oxp_board board; > > +static struct device *oxp_dev; > > Using a global variable is ugly. > An explicit parameter passed through > battery_hook_register() -> add_battery() > would be nicer. > It would require changes to the core code and all its users, though. I debated doing this. Unfortunately, this driver uses a global variable already (see board), so introducing a struct here seemed a bit excessive. During a refactor, removing the board global variable would introduce a features struct, which can then be used for the battery hook. So I think they should be done together in a future series. > > > > /* Fan reading and PWM */ > > #define OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */ > > @@ -93,6 +95,23 @@ static enum oxp_board board; > > #define OXP_X1_TURBO_LED_OFF 0x01 > > #define OXP_X1_TURBO_LED_ON 0x02 > > > > +/* Battery extension 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_AWAKE)) > > + > > +#define OXP_X1_CHARGE_LIMIT_REG 0xA3 /* X1 charge limit (%) */ > > +#define OXP_X1_CHARGE_INHIBIT_REG 0xA4 /* X1 bypass charging */ > > + > > +#define OXP_X1_CHARGE_INHIBIT_MASK_AWAKE 0x01 > > +/* > > + * X1 Mask is 0x0A, OneXFly F1Pro is just 0x02 > > + * but the extra bit on the X1 does nothing. > > + */ > > +#define OXP_X1_CHARGE_INHIBIT_MASK_OFF 0x02 > > +#define OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS (OXP_X1_CHARGE_INHIBIT_MASK_AWAKE | \ > > + OXP_X1_CHARGE_INHIBIT_MASK_OFF) > > + > > static const struct dmi_system_id dmi_table[] = { > > { > > .matches = { > > @@ -507,6 +526,136 @@ static ssize_t tt_led_show(struct device *dev, > > > > static DEVICE_ATTR_RW(tt_led); > > > > +/* Callbacks for charge behaviour attributes */ > > +static bool oxp_psy_ext_supported(void) > > +{ > > + switch (board) { > > + case oxp_x1: > > + case oxp_fly: > > + return 1; > > + default: > > + break; > > + } > > + return 0; > > For 'bool' use 'true' and 'false. > > > +} > > + > > +static int oxp_psy_ext_get_prop(struct power_supply *psy, > > + const struct power_supply_ext *ext, > > + void *data, > > + enum power_supply_property psp, > > + union power_supply_propval *val) > > +{ > > + long raw_val; > > + int ret; > > + > > + switch (psp) { > > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: > > + ret = read_from_ec(OXP_X1_CHARGE_LIMIT_REG, 1, &raw_val); > > + if (ret) > > + return ret; > > + if (raw_val > 100) > > + return -EINVAL; > > + val->intval = raw_val; > > + return 0; > > + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: > > + ret = read_from_ec(OXP_X1_CHARGE_INHIBIT_REG, 1, &raw_val); > > + if (ret) > > + return ret; > > + if ((raw_val & OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS) == > > + OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS) > > + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE; > > + else if ((raw_val & OXP_X1_CHARGE_INHIBIT_MASK_AWAKE) == > > + OXP_X1_CHARGE_INHIBIT_MASK_AWAKE) > > + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_AWAKE; > > + else > > + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO; > > + return 0; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int oxp_psy_ext_set_prop(struct power_supply *psy, > > + const struct power_supply_ext *ext, > > + void *data, > > + enum power_supply_property psp, > > + const union power_supply_propval *val) > > +{ > > + long raw_val; > > + > > + switch (psp) { > > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: > > + if (val->intval > 100) > > + return -EINVAL; > > + return write_to_ec(OXP_X1_CHARGE_LIMIT_REG, val->intval); > > + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: > > + switch (val->intval) { > > + case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO: > > + raw_val = 0; > > + break; > > + case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_AWAKE: > > + raw_val = OXP_X1_CHARGE_INHIBIT_MASK_AWAKE; > > + break; > > + case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE: > > + raw_val = OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return write_to_ec(OXP_X1_CHARGE_INHIBIT_REG, raw_val); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int oxp_psy_prop_is_writeable(struct power_supply *psy, > > + const struct power_supply_ext *ext, > > + void *data, > > + enum power_supply_property psp) > > +{ > > + return true; > > +} > > + > > +static const enum power_supply_property oxp_psy_ext_props[] = { > > + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, > > + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, > > +}; > > + > > +struct power_supply_ext oxp_psy_ext = { > > static const > > > + .name = "oxp-charge-control", > > + .properties = oxp_psy_ext_props, > > + .num_properties = ARRAY_SIZE(oxp_psy_ext_props), > > + .charge_behaviours = EC_CHARGE_CONTROL_BEHAVIOURS_X1, > > The charge control behaviours are named "X1", but nothing else. > Seems inconsistent. > > > + .get_property = oxp_psy_ext_get_prop, > > + .set_property = oxp_psy_ext_set_prop, > > + .property_is_writeable = oxp_psy_prop_is_writeable, > > +}; > > + > > +static int oxp_add_battery(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 they only have one battery, why is the check necessary? Leftover from when I modelled the battery hook from asus-wmi. If the battery hook only runs for system batteries and not e.g., for peripherals, I will remove this. > > + > > + return power_supply_register_extension(battery, &oxp_psy_ext, oxp_dev, NULL); > > +} > > + > > +static int oxp_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook) > > +{ > > + power_supply_unregister_extension(battery, &oxp_psy_ext); > > + return 0; > > +} > > + > > +static struct acpi_battery_hook battery_hook = { > > + .add_battery = oxp_add_battery, > > + .remove_battery = oxp_remove_battery, > > + .name = "OneXPlayer Battery", > > This struct can also be aligned. Can you expand on that? > > +}; > > + > > /* PWM enable/disable functions */ > > static int oxp_pwm_enable(void) > > { > > @@ -845,12 +994,19 @@ static const struct hwmon_chip_info oxp_ec_chip_info = { > > static int oxp_platform_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > - struct device *hwdev; > > + int ret; > > > > - hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL, > > - &oxp_ec_chip_info, NULL); > > + oxp_dev = dev; > > + ret = PTR_ERR_OR_ZERO(devm_hwmon_device_register_with_info( > > + dev, "oxp_ec", NULL, &oxp_ec_chip_info, NULL)); > > > > - return PTR_ERR_OR_ZERO(hwdev); > > + if (ret) > > + return ret; > > + > > + if (oxp_psy_ext_supported()) > > + return devm_battery_hook_register(dev, &battery_hook); > > If the driver is extended in the future this line will need to be touch > again as it is an unconditional early return. > > This is more future-proof: > > if (oxp_psy_ext_supported()) { > ret = devm_battery_hook_register(dev, &battery_hook); > if (ret) > return ret; > } > > > + > > + return 0; > > } > > > > static struct platform_driver oxp_platform_driver = { > > -- > > 2.48.1 > > Thanks for the comments, I will try to fix them on a V6. Hopefully you can clarify the 3 here. Best, Antheas
On 2025-03-17 19:20:46+0100, Antheas Kapenekakis wrote: > On Mon, 17 Mar 2025 at 19:13, Thomas Weißschuh <linux@weissschuh.net> wrote: > > On 2025-03-17 16:53:49+0100, Antheas Kapenekakis wrote: > > > @@ -60,6 +61,7 @@ enum oxp_board { > > > }; > > > > > > static enum oxp_board board; > > > +static struct device *oxp_dev; > > > > Using a global variable is ugly. > > An explicit parameter passed through > > battery_hook_register() -> add_battery() > > would be nicer. > > It would require changes to the core code and all its users, though. > > I debated doing this. Unfortunately, this driver uses a global > variable already (see board), so introducing a struct here seemed a > bit excessive. > > During a refactor, removing the board global variable would introduce > a features struct, which can then be used for the battery hook. > > So I think they should be done together in a future series. Fine by me. <snip> > > > +static int oxp_add_battery(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 they only have one battery, why is the check necessary? > > Leftover from when I modelled the battery hook from asus-wmi. If the > battery hook only runs for system batteries and not e.g., for > peripherals, I will remove this. The battery hook runs for all batteries discovered through ACPI. These should only be system batteries. > > > + > > > + return power_supply_register_extension(battery, &oxp_psy_ext, oxp_dev, NULL); > > > +} > > > + > > > +static int oxp_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook) > > > +{ > > > + power_supply_unregister_extension(battery, &oxp_psy_ext); > > > + return 0; > > > +} > > > + > > > +static struct acpi_battery_hook battery_hook = { > > > + .add_battery = oxp_add_battery, > > > + .remove_battery = oxp_remove_battery, > > > + .name = "OneXPlayer Battery", > > > > This struct can also be aligned. > > Can you expand on that? It is about lining up the "=" characters all in one vertical line. Same as in oxp_psy_ext; <snip>
On Mon, 17 Mar 2025 at 22:02, Thomas Weißschuh <linux@weissschuh.net> wrote: > > On 2025-03-17 19:20:46+0100, Antheas Kapenekakis wrote: > > On Mon, 17 Mar 2025 at 19:13, Thomas Weißschuh <linux@weissschuh.net> wrote: > > > On 2025-03-17 16:53:49+0100, Antheas Kapenekakis wrote: > > > > > @@ -60,6 +61,7 @@ enum oxp_board { > > > > }; > > > > > > > > static enum oxp_board board; > > > > +static struct device *oxp_dev; > > > > > > Using a global variable is ugly. > > > An explicit parameter passed through > > > battery_hook_register() -> add_battery() > > > would be nicer. > > > It would require changes to the core code and all its users, though. > > > > I debated doing this. Unfortunately, this driver uses a global > > variable already (see board), so introducing a struct here seemed a > > bit excessive. > > > > During a refactor, removing the board global variable would introduce > > a features struct, which can then be used for the battery hook. > > > > So I think they should be done together in a future series. > > Fine by me. > > <snip> > > > > > +static int oxp_add_battery(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 they only have one battery, why is the check necessary? > > > > Leftover from when I modelled the battery hook from asus-wmi. If the > > battery hook only runs for system batteries and not e.g., for > > peripherals, I will remove this. > > The battery hook runs for all batteries discovered through ACPI. > These should only be system batteries. > > > > > + > > > > + return power_supply_register_extension(battery, &oxp_psy_ext, oxp_dev, NULL); > > > > +} > > > > + > > > > +static int oxp_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook) > > > > +{ > > > > + power_supply_unregister_extension(battery, &oxp_psy_ext); > > > > + return 0; > > > > +} > > > > + > > > > +static struct acpi_battery_hook battery_hook = { > > > > + .add_battery = oxp_add_battery, > > > > + .remove_battery = oxp_remove_battery, > > > > + .name = "OneXPlayer Battery", > > > > > > This struct can also be aligned. > > > > Can you expand on that? > > It is about lining up the "=" characters all in one vertical line. > Same as in oxp_psy_ext; I am mixing up my spaces and my tabs. What is the canonical way to render tabs so I can make sure they are aligned? > <snip> Thanks, Antheas
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 82cfc76bc5c9f..f4d993658c01f 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 39a29295f9cfe..88d839c2a4834 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; @@ -60,6 +61,7 @@ enum oxp_board { }; static enum oxp_board board; +static struct device *oxp_dev; /* Fan reading and PWM */ #define OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */ @@ -93,6 +95,23 @@ static enum oxp_board board; #define OXP_X1_TURBO_LED_OFF 0x01 #define OXP_X1_TURBO_LED_ON 0x02 +/* Battery extension 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_AWAKE)) + +#define OXP_X1_CHARGE_LIMIT_REG 0xA3 /* X1 charge limit (%) */ +#define OXP_X1_CHARGE_INHIBIT_REG 0xA4 /* X1 bypass charging */ + +#define OXP_X1_CHARGE_INHIBIT_MASK_AWAKE 0x01 +/* + * X1 Mask is 0x0A, OneXFly F1Pro is just 0x02 + * but the extra bit on the X1 does nothing. + */ +#define OXP_X1_CHARGE_INHIBIT_MASK_OFF 0x02 +#define OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS (OXP_X1_CHARGE_INHIBIT_MASK_AWAKE | \ + OXP_X1_CHARGE_INHIBIT_MASK_OFF) + static const struct dmi_system_id dmi_table[] = { { .matches = { @@ -507,6 +526,136 @@ static ssize_t tt_led_show(struct device *dev, static DEVICE_ATTR_RW(tt_led); +/* Callbacks for charge behaviour attributes */ +static bool oxp_psy_ext_supported(void) +{ + switch (board) { + case oxp_x1: + case oxp_fly: + return 1; + default: + break; + } + return 0; +} + +static int oxp_psy_ext_get_prop(struct power_supply *psy, + const struct power_supply_ext *ext, + void *data, + enum power_supply_property psp, + union power_supply_propval *val) +{ + long raw_val; + int ret; + + switch (psp) { + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: + ret = read_from_ec(OXP_X1_CHARGE_LIMIT_REG, 1, &raw_val); + if (ret) + return ret; + if (raw_val > 100) + return -EINVAL; + val->intval = raw_val; + return 0; + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: + ret = read_from_ec(OXP_X1_CHARGE_INHIBIT_REG, 1, &raw_val); + if (ret) + return ret; + if ((raw_val & OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS) == + OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS) + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE; + else if ((raw_val & OXP_X1_CHARGE_INHIBIT_MASK_AWAKE) == + OXP_X1_CHARGE_INHIBIT_MASK_AWAKE) + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_AWAKE; + else + val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO; + return 0; + default: + return -EINVAL; + } +} + +static int oxp_psy_ext_set_prop(struct power_supply *psy, + const struct power_supply_ext *ext, + void *data, + enum power_supply_property psp, + const union power_supply_propval *val) +{ + long raw_val; + + switch (psp) { + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: + if (val->intval > 100) + return -EINVAL; + return write_to_ec(OXP_X1_CHARGE_LIMIT_REG, val->intval); + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR: + switch (val->intval) { + case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO: + raw_val = 0; + break; + case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_AWAKE: + raw_val = OXP_X1_CHARGE_INHIBIT_MASK_AWAKE; + break; + case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE: + raw_val = OXP_X1_CHARGE_INHIBIT_MASK_ALWAYS; + break; + default: + return -EINVAL; + } + + return write_to_ec(OXP_X1_CHARGE_INHIBIT_REG, raw_val); + default: + return -EINVAL; + } +} + +static int oxp_psy_prop_is_writeable(struct power_supply *psy, + const struct power_supply_ext *ext, + void *data, + enum power_supply_property psp) +{ + return true; +} + +static const enum power_supply_property oxp_psy_ext_props[] = { + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, +}; + +struct power_supply_ext oxp_psy_ext = { + .name = "oxp-charge-control", + .properties = oxp_psy_ext_props, + .num_properties = ARRAY_SIZE(oxp_psy_ext_props), + .charge_behaviours = EC_CHARGE_CONTROL_BEHAVIOURS_X1, + .get_property = oxp_psy_ext_get_prop, + .set_property = oxp_psy_ext_set_prop, + .property_is_writeable = oxp_psy_prop_is_writeable, +}; + +static int oxp_add_battery(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; + + return power_supply_register_extension(battery, &oxp_psy_ext, oxp_dev, NULL); +} + +static int oxp_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook) +{ + power_supply_unregister_extension(battery, &oxp_psy_ext); + return 0; +} + +static struct acpi_battery_hook battery_hook = { + .add_battery = oxp_add_battery, + .remove_battery = oxp_remove_battery, + .name = "OneXPlayer Battery", +}; + /* PWM enable/disable functions */ static int oxp_pwm_enable(void) { @@ -845,12 +994,19 @@ static const struct hwmon_chip_info oxp_ec_chip_info = { static int oxp_platform_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device *hwdev; + int ret; - hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL, - &oxp_ec_chip_info, NULL); + oxp_dev = dev; + ret = PTR_ERR_OR_ZERO(devm_hwmon_device_register_with_info( + dev, "oxp_ec", NULL, &oxp_ec_chip_info, NULL)); - return PTR_ERR_OR_ZERO(hwdev); + if (ret) + return ret; + + if (oxp_psy_ext_supported()) + return devm_battery_hook_register(dev, &battery_hook); + + return 0; } static struct platform_driver oxp_platform_driver = {
With the X1 (AMD), OneXPlayer added a charge limit and charge inhibit feature to their devices. Charge limit allows for choosing an arbitrary battery charge setpoint in percentages. Charge ihibit allows to instruct the device to stop charging either when it is awake 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. Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- drivers/platform/x86/Kconfig | 1 + drivers/platform/x86/oxpec.c | 164 ++++++++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 4 deletions(-)