Re: [PATCH v2 4/6] platform/x86: ayaneo-ec: Add controller power and modules attributes

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

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

On Wed, 15 Oct 2025, Ilpo Järvinen wrote:

> On Wed, 15 Oct 2025, Antheas Kapenekakis wrote:
> 
> > 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.
> > 
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> >  .../ABI/testing/sysfs-platform-ayaneo         |  19 ++++
> >  MAINTAINERS                                   |   1 +
> >  drivers/platform/x86/ayaneo-ec.c              | 100 ++++++++++++++++++
> >  3 files changed, 120 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-platform-ayaneo
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-ayaneo b/Documentation/ABI/testing/sysfs-platform-ayaneo
> > new file mode 100644
> > index 000000000000..1fa32ba60fd0
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-ayaneo
> > @@ -0,0 +1,19 @@
> > +What:		/sys/devices/platform/<platform>/controller_power
> > +Date:		Oct 2025
> > +KernelVersion:	6.9
> > +Contact:	"Antheas Kapenekakis" <lkml@antheas.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/<platform>/controller_modules
> > +Date:		Oct 2025
> > +KernelVersion:	6.9
> > +Contact:	"Antheas Kapenekakis"  <lkml@antheas.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 8c4d0c26ca77..3dfa004555dd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4191,6 +4191,7 @@ AYANEO PLATFORM EC DRIVER
> >  M:	Antheas Kapenekakis <lkml@antheas.dev>
> >  L:	platform-driver-x86@vger.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 23c283f5eb61..363b61fc6e12 100644
> > --- a/drivers/platform/x86/ayaneo-ec.c
> > +++ b/drivers/platform/x86/ayaneo-ec.c
> > @@ -30,9 +30,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)

Also, add include for BIT().

> > +
> >  struct ayaneo_ec_quirk {
> >  	bool has_fan_control;
> >  	bool has_charge_control;
> > +	bool has_magic_modules;
> >  };
> >  
> >  struct ayaneo_ec_platform_data {
> > @@ -44,6 +52,7 @@ struct ayaneo_ec_platform_data {
> >  static const struct ayaneo_ec_quirk ayaneo3 = {
> >  	.has_fan_control = true,
> >  	.has_charge_control = true,
> > +	.has_magic_modules = true,
> >  };
> >  
> >  static const struct dmi_system_id dmi_table[] = {
> > @@ -262,6 +271,96 @@ 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);

Add include.

-- 
 i.

> > +}
> > +
> > +static DEVICE_ATTR_RW(controller_power);
> > +
> > +static ssize_t controller_modules_show(struct device *dev,
> > +				       struct device_attribute *attr, char *buf)
> > +{
> > +	bool left, right;
> > +	char *out;
> > +	int ret;
> > +	u8 val;
> > +
> > +	ret = ec_read(AYANEO_MODULE_REG, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	left = !(val & AYANEO_MODULE_LEFT);
> > +	right = !(val & AYANEO_MODULE_RIGHT);
> > +
> > +	if (left && right)
> > +		out = "both";
> > +	else if (left)
> > +		out = "left";
> > +	else if (right)
> > +		out = "right";
> > +	else
> > +		out = "none";
> 
> I suggest using switch/case for this so you don't need the internal 
> booleans at all.
> 
> BTW, I appreciate it very much this series is split per feature, it's so 
> much less daunting to start a review when I know I can actually finish 
> before the next interruption (and having to flush context from my
> mind). :-)
> 
>