From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 82FDF23A566 for ; Mon, 17 Mar 2025 12:33:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1742214796; cv=none; b=AzPJ8kbEpoRFipeN5c48NH76gNhYe9r5Ao76fHdbNE0TEbqG9M4ESwN+26xNxBAoircqN/vUjZL3UID0PuJcn3lReO3cE6xH8sRjo5K7BDRDF3fS4og4UFvNhBQGES4DJuKOgGgDIciUFusG1tQ4oSyPf0tBQyKKpLXcWkQdOEs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1742214796; c=relaxed/simple; bh=HaoDdTNElxGgiWHcCwujbysz5G/lNSZtyXhMi7JJJig=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Tp9VZ6IjIbxrOyFnu7INv7OztIJIOFa3Nw8pLPHVF9iZoBn/RsQiBzTiacHszQNPf+oDZHwzr1sgQazZmwt3+OHHTmKG9w9Wvq/ry6oIYw6iwMEHh/K32zX/RF07ELKVUno9U7I8L2ToE2CyP4AForEAsZMYk8n0hh2Ar8QOd9o= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=GHS2M9Sp; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="GHS2M9Sp" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1742214793; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=JezHzOr4cH5CC3OkxH2pDIucxBNDf0tG1TD0C+sq9bs=; b=GHS2M9SpO3946coHXRks2K62EdGoFXpQlYRs8xpI2dCUpXY0+7Av8TDBsmTQhltOXhF8tj Bu7RjMMGdsLz0Cfatv7KPXrnTk/C2dni5ATxTYCPZImJgMn5dZIqxsxzjSPe010hzYICaZ mtJJvrlJ1wWvWxG94t9TJjSDa+9utS8= Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-160-Zm4sO1YSOtGhRUSUrSHtRQ-1; Mon, 17 Mar 2025 08:33:10 -0400 X-MC-Unique: Zm4sO1YSOtGhRUSUrSHtRQ-1 X-Mimecast-MFC-AGG-ID: Zm4sO1YSOtGhRUSUrSHtRQ_1742214788 Received: by mail-ej1-f69.google.com with SMTP id a640c23a62f3a-ac316d639c6so285904566b.2 for ; Mon, 17 Mar 2025 05:33:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1742214788; x=1742819588; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=JezHzOr4cH5CC3OkxH2pDIucxBNDf0tG1TD0C+sq9bs=; b=PrQNiYbD8GOW1HF9+fd7bLpE3HJs1RjDLPfIKJdQXruCtZML5FTh7jdhfQ+5pT/ZEr MZOvvLx2iwqram/GW/O9wXqoHdpyAMjCd4qv/Vt/3OmTCdVemFq3vRxwpGO0VY1G70kD ajhmK7kEA/v9AZ/Er0uV+Im+ty2ZYKoOtTrlBS4Xi30uCEKIYfBBbFIc0/NcqZkONoA4 USUGAzRWwIgkZE+MMetUMh8jXmMidA52Ja8fMzWRvOOhuI8R9nyoA5U9b8ka+RVQ+Ik0 VRAmpcSkErpT2rA1xcK6R1BMTutC0c3+3GJwR3bZIv8JTznItd8O+BdFDBWv+fFowoZi 9LhA== X-Gm-Message-State: AOJu0Yz3hiyYD7nEa3u9mbAxWBC56O4skJyaXpcJZmcWnc1+79MPnCH/ cQcyiJh+JwCAFfO4VYvyFBr6Xp0eEGMvrzCTkN4srfbwzc5x0jnna9V3y4RrQzvbISAOVyIV/5n T7qjGa8bD81ywqaMV/mcXPqASyMDrvRxqavMWOGXtl1v+X75p5VRi7Nfd5+g9 X-Gm-Gg: ASbGncuhGxtZgB+MPyjbgmYivqVbeV8F8CivOxHryvSU6n9c8Ygh5RizFc7OZWfzmg3 Pum8OlJezqZYYsmPa6s4rBx0n0qkB8N8my48Ijdaui7JhaJotBuA2k/b9pyAXmBk1jh266bgeFp nR3Mgm9d7ZNLexV7/CU73M9aMcFG1kA+m1LrE1DaAocxfW7MZKNYpKKg3Y1poioxMUSYTsFIGNe XXnmn7woJqlmwuk44fy1CyGh8T9WyUponvvzuJv3eGLFb68IumYgotTfhLtz1SeFuivfeYSx0aL Qwom4OuFQy8QzYVEbxw= X-Received: by 2002:a17:907:d92:b0:abf:6b14:6cf0 with SMTP id a640c23a62f3a-ac3301de27bmr1318319166b.12.1742214787897; Mon, 17 Mar 2025 05:33:07 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEWN4ituXBhYLFP8L5JaUI3RJRAtsiwYlQaRBm96BxIsMNoPk9NSO6TYSOqRrxmgRK86+9NZw== X-Received: by 2002:a17:907:d92:b0:abf:6b14:6cf0 with SMTP id a640c23a62f3a-ac3301de27bmr1318315766b.12.1742214787390; Mon, 17 Mar 2025 05:33:07 -0700 (PDT) Received: from [10.40.98.122] ([78.108.130.194]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-ac314a4bf4dsm659816866b.154.2025.03.17.05.33.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 Mar 2025 05:33:06 -0700 (PDT) Message-ID: Date: Mon, 17 Mar 2025 13:32:55 +0100 Precedence: bulk X-Mailing-List: linux-hwmon@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 06/13] platform/x86: oxpec: Add charge threshold and behaviour to OneXPlayer To: Antheas Kapenekakis , platform-driver-x86@vger.kernel.org Cc: linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org, linux-pm@vger.kernel.org, Guenter Roeck , Jean Delvare , Jonathan Corbet , Joaquin Ignacio Aramendia , Derek J Clark , Kevin Greenberg , Joshua Tam , Parth Menon , Eileen References: <20250311165406.331046-1-lkml@antheas.dev> <20250311165406.331046-7-lkml@antheas.dev> Content-Language: en-US, nl From: Hans de Goede In-Reply-To: <20250311165406.331046-7-lkml@antheas.dev> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi, On 11-Mar-25 17:53, Antheas Kapenekakis 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. Again please don't use the word bypass, use inhibit instead. > 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. > > 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 > --- > drivers/platform/x86/Kconfig | 1 + > drivers/platform/x86/oxpec.c | 217 +++++++++++++++++++++++++++++++++++ > 2 files changed, 218 insertions(+) > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 82cfc76bc5c9..f4d993658c01 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 dc3a0871809c..d73a10598d8f 100644 > --- a/drivers/platform/x86/oxpec.c > +++ b/drivers/platform/x86/oxpec.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > > /* 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 Again avoid the word BYPASS please, if OneXPlayer are calling this bypass in their own documentation maybe add a note here when defining the registers that OneXPlayer calls this bypass and then use inhibit from there on. > +/* > + * 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 Ok, so suspend is treated as off, but that is for S3 suspend, what about s2idle, or does this hw not do s2idle ? > +#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 charge behaviour attributes */ > +static bool charge_behaviour_supported(void) > +{ > + 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) > +{ > + unsigned int available; > + long val, s0, always; > + int ret; > + u8 reg; > + > + 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; Since these are always the same this does not seem useful, please use the defines directly below. > + default: > + return -EINVAL; > + } > + > + ret = power_supply_charge_behaviour_parse(available, buf); > + if (ret < 0) > + 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) > + return ret; > + > + return count; > +} > + > +static ssize_t charge_behaviour_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + long val, s0, always, sel; > + unsigned int available; > + int ret; > + u8 reg; > + > + 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) > + 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) > + 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) > + return ret; > + > + return count; > +} > + > +static ssize_t charge_control_end_threshold_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + long val; > + int ret; > + u8 reg; > + > + 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) > + 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", > +}; > + Since this is new code it should use the new power-supply extension support instead of the old battery_hook mechanism: https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6037802bbae892f3ad0c7b4c4faee39b967e32b0 > /* 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; Regards, Hans