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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Mar 9, 2025 at 4:21 AM Antheas Kapenekakis <lkml@xxxxxxxxxxx> 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.
>
> 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.
>

As noted in your previous patch, I think checking for BIOS support is
a wise move here.

> 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@xxxxxxxxxxx>
> ---
>  drivers/platform/x86/oxpec.c | 217 +++++++++++++++++++++++++++++++++++
>  1 file changed, 217 insertions(+)
>
> diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
> index dc3a0871809c..dd6d333ebcfa 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 turbo toggle attribute */

This comment is not correct for the section. I think it was a copy/paste?

> +static bool charge_behaviour_supported(void)

Attribute groups support .is_visible. This blocks invocation from
userspace, vice doing it in probe() manually.

> +{
> +       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)
> +{
> +       int ret;
> +       u8 reg;
> +       long val, s0, always;
> +       unsigned int available;
> +

Convention is to order variables in reverse xmas tree, with the
longest line first and shortest line last.

> +       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)

Does ret ever return > 0? I think you can just if (ret)

> +               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 < 0)

if (ret)

> +               return ret;
> +
> +       return count;
> +}
> +
> +static ssize_t charge_behaviour_show(struct device *dev,
> +                             struct device_attribute *attr, char *buf)
> +{
> +       int ret;
> +       u8 reg;
> +       long val, s0, always, sel;
> +       unsigned int available;
> +

Reverse xmas tree here too.

> +       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 < 0)

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 < 0)

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 < 0)
> +               return ret;
> +
> +       return count;
> +}
> +
> +static ssize_t charge_control_end_threshold_show(struct device *dev,
> +                             struct device_attribute *attr, char *buf)
> +{
> +       int ret;
> +       u8 reg;
> +       long val;
> +

Reverse xmas tree here too.

> +       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 < 0)

if (ret)

Cheers,
- Derek

> +               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
>





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux