| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6193cf3a-f5a3-2e91-50d0-ef980cad334d@linux.intel.com>
Date: Fri, 25 Apr 2025 14:05:14 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Antheas Kapenekakis <lkml@...heas.dev>
cc: platform-driver-x86@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-doc@...r.kernel.org, linux-pm@...r.kernel.org,
Guenter Roeck <linux@...ck-us.net>, Jean Delvare <jdelvare@...e.com>,
Jonathan Corbet <corbet@....net>,
Joaquin Ignacio Aramendia <samsagax@...il.com>,
Derek J Clark <derekjohn.clark@...il.com>,
Kevin Greenberg <kdgreenberg234@...tonmail.com>,
Joshua Tam <csinaction@...me>, Parth Menon <parthasarathymenon@...il.com>,
Eileen <eileen@...-netbook.com>, LKML <linux-kernel@...r.kernel.org>,
sre@...nel.org, linux@...ssschuh.net, Hans de Goede <hdegoede@...hat.com>,
mario.limonciello@....com
Subject: Re: [PATCH v9 14/15] platform/x86: oxpec: Add charge threshold and
behaviour to OneXPlayer
On Thu, 24 Apr 2025, Antheas Kapenekakis wrote:
> On Thu, 24 Apr 2025 at 15:49, Ilpo Järvinen
> <ilpo.jarvinen@...ux.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@...ssschuh.net>
> > > Reviewed-by: Derek J. Clark <derekjohn.clark@...il.com>
> > > Signed-off-by: Antheas Kapenekakis <lkml@...heas.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 = {
> > >
>
Powered by blists - more mailing lists