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

[Antheas Kapenekakis]

On Mon, 17 Mar 2025 at 19:13, Thomas Weißschuh <linux@xxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> >  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