| 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: <1e9d5311-45f9-15fa-ce3e-8098c288acbd@linux.intel.com>
Date: Wed, 5 Nov 2025 15:24:56 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Antheas Kapenekakis <lkml@...heas.dev>
cc: Armin Wolf <W_Armin@....de>, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, linux-hwmon@...r.kernel.org,
Hans de Goede <hansg@...nel.org>,
Derek John Clark <derekjohn.clark@...il.com>,
Joaquín Ignacio Aramendía <samsagax@...il.com>,
Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v3 4/6] platform/x86: ayaneo-ec: Add controller power
and modules attributes
On Wed, 5 Nov 2025, Antheas Kapenekakis wrote:
> On Tue, 4 Nov 2025 at 21:04, Armin Wolf <W_Armin@....de> wrote:
> >
> > Am 02.11.25 um 19:46 schrieb Antheas Kapenekakis:
> >
> > > On Sun, 2 Nov 2025 at 19:30, Armin Wolf <W_Armin@....de> wrote:
> > >> Am 31.10.25 um 17:36 schrieb Antheas Kapenekakis:
> > >>
> > >>> The Ayaneo 3 features hot-swappable controller modules. The ejection
> > >>> and management is done through HID. However, after ejecting the modules,
> > >>> the controller needs to be power cycled via the EC to re-initialize.
> > >>>
> > >>> For this, the EC provides a variable that holds whether the left or
> > >>> right modules are connected, and a power control register to turn
> > >>> the controller on or off. After ejecting the modules, the controller
> > >>> should be turned off. Then, after both modules are reinserted,
> > >>> the controller may be powered on again to re-initialize.
> > >>>
> > >>> This patch introduces two new sysfs attributes:
> > >>> - `controller_modules`: a read-only attribute that indicates whether
> > >>> the left and right modules are connected (none, left, right, both).
> > >>> - `controller_power`: a read-write attribute that allows the user
> > >>> to turn the controller on or off (with '1'/'0').
> > >>>
> > >>> Therefore, after ejection is complete, userspace can power off the
> > >>> controller, then wait until both modules have been reinserted
> > >>> (`controller_modules` will return 'both') to turn on the controller.
> > >>>
> > >>> Reviewed-by: Armin Wolf <W_Armin@....de>
> > >>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> > >>> ---
> > >>> .../ABI/testing/sysfs-platform-ayaneo-ec | 19 ++++
> > >>> MAINTAINERS | 1 +
> > >>> drivers/platform/x86/ayaneo-ec.c | 106 ++++++++++++++++++
> > >>> 3 files changed, 126 insertions(+)
> > >>> create mode 100644 Documentation/ABI/testing/sysfs-platform-ayaneo-ec
> > >>>
> > >>> diff --git a/Documentation/ABI/testing/sysfs-platform-ayaneo-ec b/Documentation/ABI/testing/sysfs-platform-ayaneo-ec
> > >>> new file mode 100644
> > >>> index 000000000000..3c9c3580c685
> > >>> --- /dev/null
> > >>> +++ b/Documentation/ABI/testing/sysfs-platform-ayaneo-ec
> > >>> @@ -0,0 +1,19 @@
> > >>> +What: /sys/devices/platform/ayaneo-ec/controller_power
> > >>> +Date: Oct 2025
> > >> I think you need to update those dates.
> > >>
> > >>> +KernelVersion: 6.19
> > >>> +Contact: "Antheas Kapenekakis" <lkml@...heas.dev>
> > >>> +Description:
> > >>> + Current controller power state. Allows turning on and off
> > >>> + the controller power (e.g. for power savings). Write 1 to
> > >>> + turn on, 0 to turn off. File is readable and writable.
> > >>> +
> > >>> +What: /sys/devices/platform/ayaneo-ec/controller_modules
> > >>> +Date: Oct 2025
> > >>> +KernelVersion: 6.19
> > >>> +Contact: "Antheas Kapenekakis" <lkml@...heas.dev>
> > >>> +Description:
> > >>> + Shows which controller modules are currently connected to
> > >>> + the device. Possible values are "left", "right" and "both".
> > >>> + File is read-only. The Windows software for this device
> > >>> + will only set controller power to 1 if both module sides
> > >>> + are connected (i.e. this file returns "both").
> > >>> diff --git a/MAINTAINERS b/MAINTAINERS
> > >>> index da9498d8cc89..b4d62ea9a926 100644
> > >>> --- a/MAINTAINERS
> > >>> +++ b/MAINTAINERS
> > >>> @@ -4191,6 +4191,7 @@ AYANEO PLATFORM EC DRIVER
> > >>> M: Antheas Kapenekakis <lkml@...heas.dev>
> > >>> L: platform-driver-x86@...r.kernel.org
> > >>> S: Maintained
> > >>> +F: Documentation/ABI/testing/sysfs-platform-ayaneo
> > >>> F: drivers/platform/x86/ayaneo-ec.c
> > >>>
> > >>> AZ6007 DVB DRIVER
> > >>> diff --git a/drivers/platform/x86/ayaneo-ec.c b/drivers/platform/x86/ayaneo-ec.c
> > >>> index 697bb053a7d6..0652c044ad76 100644
> > >>> --- a/drivers/platform/x86/ayaneo-ec.c
> > >>> +++ b/drivers/platform/x86/ayaneo-ec.c
> > >>> @@ -8,6 +8,7 @@
> > >>> */
> > >>>
> > >>> #include <linux/acpi.h>
> > >>> +#include <linux/bits.h>
> > >>> #include <linux/dmi.h>
> > >>> #include <linux/err.h>
> > >>> #include <linux/hwmon.h>
> > >>> @@ -16,6 +17,7 @@
> > >>> #include <linux/module.h>
> > >>> #include <linux/platform_device.h>
> > >>> #include <linux/power_supply.h>
> > >>> +#include <linux/sysfs.h>
> > >>> #include <acpi/battery.h>
> > >>>
> > >>> #define AYANEO_PWM_ENABLE_REG 0x4A
> > >>> @@ -32,9 +34,17 @@
> > >>> #define AYANEO_CHARGE_VAL_AUTO 0xaa
> > >>> #define AYANEO_CHARGE_VAL_INHIBIT 0x55
> > >>>
> > >>> +#define AYANEO_POWER_REG 0x2d
> > >>> +#define AYANEO_POWER_OFF 0xfe
> > >>> +#define AYANEO_POWER_ON 0xff
> > >>> +#define AYANEO_MODULE_REG 0x2f
> > >>> +#define AYANEO_MODULE_LEFT BIT(0)
> > >>> +#define AYANEO_MODULE_RIGHT BIT(1)
> > >> Using GENMASK() would make sense here.
> > > Only a single bit is being used though? GENMASK is used for a contiguous series?
> >
> > I was thinking of using GENMASK() for both bits:
> >
> > #define AYANEO_MODULE_MASK GENMASK(1, 0)
> >
> > You can then retrieve both bits using FIELD_GET() and simply use a switch statement
> > together with an enum in controller_modules_show().
>
> I will look at it closer if I revise the first 5 patches. The logic of
> the register is complicated due to the flip, so I would not be eager
> to.
>
> I could do #define AYANEO_MODULE_MASK (AYANEO_MODULE_LEFT |
> AYANEO_MODULE_RIGHT) for stylistic reasons.
Hi,
Quickly looking at the code below, a mask define certainly be useful to
avoid having to embed that into the switch itself below.
> @Ilpo: for the first 5 of this series is there something missing other
> than perhaps the month? If not, I can respin the last patch on its own
> later today. I would like this driver + the asus stuff to go into 6.19
> if possible (there: i dealt with the asusctl bug by skipping the
> quirk), I am unsure for the timeline for that. I want to say merge
> window starts in two weeks?
I've been sick for almost a week so I'm a bit lost with all the progress
and status with various series so it takes time to catch up until can
say something when it comes to any particular series (I'm sorry).
However, we're only at -rc4 (since Nov 2) so there's 4 or 5 weeks until
the merge window. And this is new driver so no regression potential other
than build failures so I don't see big schedule pressure yet, tbh.
I try to take it into the consideration with my review scheduling even
under normal circumstances that submitters have the opportunity to make it
assuming one makes suggested changes (if any) in a reasonable time frame.
It generally helps to pay attention to the kernel cycle as closer to the
merge window we get, it usually helps to prioritize things right, e.g.,
sending a series on the last week or close to it, getting trivial
feedback, and not addressing it timely with an update will result in
missing that kernel cycle; honestly, I always get a bit sad when I see
people do that.
> Antheas
>
> > Thanks,
> > Armin Wolf
> >
> > >> With those issues being fixed:
> > >> Reviewed-by: Armin Wolf <W_Armin@....de>
> > >>
> > >>> +
> > >>> struct ayaneo_ec_quirk {
> > >>> bool has_fan_control;
> > >>> bool has_charge_control;
> > >>> + bool has_magic_modules;
> > >>> };
> > >>>
> > >>> struct ayaneo_ec_platform_data {
> > >>> @@ -46,6 +56,7 @@ struct ayaneo_ec_platform_data {
> > >>> static const struct ayaneo_ec_quirk quirk_ayaneo3 = {
> > >>> .has_fan_control = true,
> > >>> .has_charge_control = true,
> > >>> + .has_magic_modules = true,
> > >>> };
> > >>>
> > >>> static const struct dmi_system_id dmi_table[] = {
> > >>> @@ -266,6 +277,100 @@ static int ayaneo_remove_battery(struct power_supply *battery,
> > >>> return 0;
> > >>> }
> > >>>
> > >>> +static ssize_t controller_power_store(struct device *dev,
> > >>> + struct device_attribute *attr,
> > >>> + const char *buf,
> > >>> + size_t count)
> > >>> +{
> > >>> + bool value;
> > >>> + int ret;
> > >>> +
> > >>> + ret = kstrtobool(buf, &value);
> > >>> + if (ret)
> > >>> + return ret;
> > >>> +
> > >>> + ret = ec_write(AYANEO_POWER_REG, value ? AYANEO_POWER_ON : AYANEO_POWER_OFF);
> > >>> + if (ret)
> > >>> + return ret;
> > >>> +
> > >>> + return count;
> > >>> +}
> > >>> +
> > >>> +static ssize_t controller_power_show(struct device *dev,
> > >>> + struct device_attribute *attr,
> > >>> + char *buf)
> > >>> +{
> > >>> + int ret;
> > >>> + u8 val;
> > >>> +
> > >>> + ret = ec_read(AYANEO_POWER_REG, &val);
> > >>> + if (ret)
> > >>> + return ret;
> > >>> +
> > >>> + return sysfs_emit(buf, "%d\n", val == AYANEO_POWER_ON);
> > >>> +}
> > >>> +
> > >>> +static DEVICE_ATTR_RW(controller_power);
> > >>> +
> > >>> +static ssize_t controller_modules_show(struct device *dev,
> > >>> + struct device_attribute *attr, char *buf)
> > >>> +{
> > >>> + char *out;
> > >>> + int ret;
> > >>> + u8 val;
> > >>> +
> > >>> + ret = ec_read(AYANEO_MODULE_REG, &val);
> > >>> + if (ret)
> > >>> + return ret;
> > >>> +
> > >>> + switch (~val & (AYANEO_MODULE_LEFT | AYANEO_MODULE_RIGHT)) {
Why's the extra inversion here? Do those bits actually have the opposite
meaning compared with their names?
--
i.
> > >>> + case AYANEO_MODULE_LEFT | AYANEO_MODULE_RIGHT:
> > >>> + out = "both";
> > >>> + break;
> > >>> + case AYANEO_MODULE_LEFT:
> > >>> + out = "left";
> > >>> + break;
> > >>> + case AYANEO_MODULE_RIGHT:
> > >>> + out = "right";
> > >>> + break;
> > >>> + default:
> > >>> + out = "none";
> > >>> + break;
> > >>> + }
> > >>> +
> > >>> + return sysfs_emit(buf, "%s\n", out);
> > >>> +}
> > >>> +
> > >>> +static DEVICE_ATTR_RO(controller_modules);
> > >>> +
> > >>> +static struct attribute *aya_mm_attrs[] = {
> > >>> + &dev_attr_controller_power.attr,
> > >>> + &dev_attr_controller_modules.attr,
> > >>> + NULL
> > >>> +};
> > >>> +
> > >>> +static umode_t aya_mm_is_visible(struct kobject *kobj,
> > >>> + struct attribute *attr, int n)
> > >>> +{
> > >>> + struct device *dev = kobj_to_dev(kobj);
> > >>> + struct platform_device *pdev = to_platform_device(dev);
> > >>> + struct ayaneo_ec_platform_data *data = platform_get_drvdata(pdev);
> > >>> +
> > >>> + if (data->quirks->has_magic_modules)
> > >>> + return attr->mode;
> > >>> + return 0;
> > >>> +}
> > >>> +
> > >>> +static const struct attribute_group aya_mm_attribute_group = {
> > >>> + .is_visible = aya_mm_is_visible,
> > >>> + .attrs = aya_mm_attrs,
> > >>> +};
> > >>> +
> > >>> +static const struct attribute_group *ayaneo_ec_groups[] = {
> > >>> + &aya_mm_attribute_group,
> > >>> + NULL
> > >>> +};
> > >>> +
> > >>> static int ayaneo_ec_probe(struct platform_device *pdev)
> > >>> {
> > >>> const struct dmi_system_id *dmi_entry;
> > >>> @@ -307,6 +412,7 @@ static int ayaneo_ec_probe(struct platform_device *pdev)
> > >>> static struct platform_driver ayaneo_platform_driver = {
> > >>> .driver = {
> > >>> .name = "ayaneo-ec",
> > >>> + .dev_groups = ayaneo_ec_groups,
> > >>> },
> > >>> .probe = ayaneo_ec_probe,
> > >>> };
> >
>
Powered by blists - more mailing lists