Re: [PATCH v9 14/15] platform/x86: oxpec: Add charge threshold and behaviour to OneXPlayer

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 10929 bytes --]

On Thu, 24 Apr 2025, Antheas Kapenekakis wrote:

> On Thu, 24 Apr 2025 at 15:49, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Thu, 17 Apr 2025, 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
> > > 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.
> > >
> > > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> > > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > > ---
> > >  drivers/platform/x86/Kconfig |   1 +
> > >  drivers/platform/x86/oxpec.c | 155 ++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 155 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index 739740c4bb535..6c9e64a03aaef 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -1204,6 +1204,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 f0b9fff704de2..ce20bf70027df 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 (BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)             | \
> > > +                                      BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)    | \
> >
> > Please change the endings to:
> >
> > ...) | <tabs>\
> >
> > > +                                      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 */
> >
> > Please use tabs for aligning the values (there were a few other defines
> > in the earlier patches with spaces too). (I know the earlier ones used
> > space but they don't seem to be in the same group so lets just move to
> > tabs with new stuff, optionally, you can add a patch to change also the
> > pre-existing ones to use space).
> >
> > > +
> > > +#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.
> >
> > Reflow to fill 80 chars.
> >
> > > + */
> > > +#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)
> >
> > Align to (.
> 
> I made the corrections.
> 
> Should I send a revision now or wait?

Just send a new version please so I can apply these, I think people have 
had enough time to comment on them. :-) (I was basically applying these 
while I noticed those issues and some of them were a little awkward to 
change/tweak while applying so I left the updating in your hands instead).

-- 
 i.

> 
> Antheas
> 
> > --
> >  i.
> >
> > > +
> > >  static const struct dmi_system_id dmi_table[] = {
> > >       {
> > >               .matches = {
> > > @@ -507,6 +526,129 @@ 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 true;
> > > +     default:
> > > +             break;
> > > +     }
> > > +     return 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 < 0 || 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,
> > > +};
> > > +
> > > +static const 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,
> > > +     .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)
> > > +{
> > > +     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)
> > >  {
> > > @@ -847,11 +989,22 @@ static int oxp_platform_probe(struct platform_device *pdev)
> > >  {
> > >       struct device *dev = &pdev->dev;
> > >       struct device *hwdev;
> > > +     int ret;
> > >
> > > +     oxp_dev = dev;
> > >       hwdev = devm_hwmon_device_register_with_info(dev, "oxp_ec", NULL,
> > >                                                    &oxp_ec_chip_info, NULL);
> > >
> > > -     return PTR_ERR_OR_ZERO(hwdev);
> > > +     if (IS_ERR(hwdev))
> > > +             return PTR_ERR(hwdev);
> > > +
> > > +     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 = {
> > >
>