[PATCH v2 0/3] power: supply: Acer Aspire 1 embedded controller

[PATCH v2 0/3] power: supply: Acer Aspire 1 embedded controller

[Nikita Travkin]

The laptop contains an embedded controller that provides a set of
features:

- Battery and charger monitoring
- USB Type-C DP alt mode HPD monitoring
- Lid status detection
- Small amount of keyboard configuration*

[*] The keyboard is handled by the same EC but it has a dedicated i2c
bus and is already enabled. This port only provides fn key behavior
configuration.

Unfortunately, while all this functionality is implemented in ACPI, it's
currently not possible to use ACPI to boot Linux on such Qualcomm
devices. Thus this series implements and enables a new driver that
provides support for the EC features.

The EC would be one of the last pieces to get almost full support for the
Acer Aspire 1 laptop in the upstream Linux kernel.

This series is similar to the EC driver for Lenovo Yoga C630, proposed
in [1] but seemingly never followed up...

[1] https://lore.kernel.org/all/20230205152809.2233436-1-dmitry.baryshkov@linaro.org/

Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
Changes in v2:
- Drop incorrectly allowed reg in the ec connector binding (Krzysztof)
- Minor style changes (Konrad)
- Link to v1: https://lore.kernel.org/r/20231207-aspire1-ec-v1-0-ba9e1c227007@trvn.ru

---
Nikita Travkin (3):
      dt-bindings: power: supply: Add Acer Aspire 1 EC
      power: supply: Add Acer Aspire 1 embedded controller driver
      arm64: dts: qcom: acer-aspire1: Add embedded controller

 .../bindings/power/supply/acer,aspire1-ec.yaml     |  67 ++++
 arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts   |  40 +-
 drivers/power/supply/Kconfig                       |  14 +
 drivers/power/supply/Makefile                      |   1 +
 drivers/power/supply/acer-aspire1-ec.c             | 433 +++++++++++++++++++++
 5 files changed, 554 insertions(+), 1 deletion(-)
---
base-commit: abb240f7a2bd14567ab53e602db562bb683391e6
change-id: 20231206-aspire1-ec-6b3d2cac1a72

Best regards,
-- 
Nikita Travkin <nikita@trvn.ru>

[PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Nikita Travkin]

Add binding for the EC found in the Acer Aspire 1 laptop.

Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
 .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
new file mode 100644
index 000000000000..1fbf1272a00f
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Acer Aspire 1 Embedded Controller
+
+maintainers:
+  - Nikita Travkin <nikita@trvn.ru>
+
+description:
+  The Acer Aspire 1 laptop uses an embedded controller to control battery
+  and charging as well as to provide a set of misc features such as the
+  laptop lid status and HPD events for the USB Type-C DP alt mode.
+
+properties:
+  compatible:
+    const: acer,aspire1-ec
+
+  reg:
+    const: 0x76
+
+  interrupts:
+    maxItems: 1
+
+  acer,media-keys-on-top:
+    description: Configure the keyboard layout to use media features of
+      the fn row when the fn key is not pressed. The firmware may choose
+      to add this property when user selects the fn mode in the firmware
+      setup utility.
+    type: boolean
+
+  connector:
+    $ref: /schemas/connector/usb-connector.yaml#
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |+
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        embedded-controller@76 {
+            compatible = "acer,aspire1-ec";
+            reg = <0x76>;
+
+            interrupts-extended = <&tlmm 30 IRQ_TYPE_LEVEL_LOW>;
+
+            connector {
+                compatible = "usb-c-connector";
+
+                port {
+                    ec_dp_in: endpoint {
+                        remote-endpoint = <&mdss_dp_out>;
+                    };
+                };
+            };
+        };
+    };

-- 
2.43.0

[PATCH v2 2/3] power: supply: Add Acer Aspire 1 embedded controller driver

[Nikita Travkin]

Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded
controller to control the charging and battery management, as well as to
perform a set of misc functions.

Unfortunately, while all this functionality is implemented in ACPI, it's
currently not possible to use ACPI to boot Linux on such Qualcomm
devices. To allow Linux to still support the features provided by EC,
this driver reimplments the relevant ACPI parts. This allows us to boot
the laptop with Device Tree and retain all the features.

Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
 drivers/power/supply/Kconfig           |  14 ++
 drivers/power/supply/Makefile          |   1 +
 drivers/power/supply/acer-aspire1-ec.c | 433 +++++++++++++++++++++++++++++++++
 3 files changed, 448 insertions(+)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index f21cb05815ec..47d2ad14447b 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -984,4 +984,18 @@ config FUEL_GAUGE_MM8013
 	  the state of charge, temperature, cycle count, actual and design
 	  capacity, etc.
 
+config EC_ACER_ASPIRE1
+	tristate "Acer Aspire 1 Emedded Controller driver"
+	depends on I2C
+	depends on DRM
+	help
+	  Say Y here to enable the EC driver for the (Snapdragon-based)
+	  Acer Aspire 1 laptop. The EC handles battery and charging
+	  monitoring as well as some misc functions like the lid sensor
+	  and USB Type-C DP HPD events.
+
+	  This driver provides battery and AC status support for the mentioned
+	  laptop where this information is not properly exposed via the
+	  standard ACPI devices.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 58b567278034..6049c87820c0 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -114,3 +114,4 @@ obj-$(CONFIG_CHARGER_SURFACE)	+= surface_charger.o
 obj-$(CONFIG_BATTERY_UG3105)	+= ug3105_battery.o
 obj-$(CONFIG_CHARGER_QCOM_SMB2)	+= qcom_pmi8998_charger.o
 obj-$(CONFIG_FUEL_GAUGE_MM8013)	+= mm8013.o
+obj-$(CONFIG_EC_ACER_ASPIRE1)	+= acer-aspire1-ec.o
diff --git a/drivers/power/supply/acer-aspire1-ec.c b/drivers/power/supply/acer-aspire1-ec.c
new file mode 100644
index 000000000000..efc773a21a2f
--- /dev/null
+++ b/drivers/power/supply/acer-aspire1-ec.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2023, Nikita Travkin <nikita@trvn.ru> */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/delay.h>
+
+#include <linux/usb/typec_mux.h>
+#include <drm/drm_bridge.h>
+
+#define ASPIRE_EC_EVENT			0x05
+
+#define ASPIRE_EC_EVENT_WATCHDOG	0x20
+#define ASPIRE_EC_EVENT_LID_CLOSE	0x9b
+#define ASPIRE_EC_EVENT_LID_OPEN	0x9c
+#define ASPIRE_EC_EVENT_FG_INF_CHG	0x85
+#define ASPIRE_EC_EVENT_FG_STA_CHG	0xc6
+#define ASPIRE_EC_EVENT_HPD_DIS		0xa3
+#define ASPIRE_EC_EVENT_HPD_CON		0xa4
+
+#define ASPIRE_EC_FG_DYNAMIC		0x07
+#define ASPIRE_EC_FG_STATIC		0x08
+
+#define ASPIRE_EC_FG_FLAG_PRESENT	BIT(0)
+#define ASPIRE_EC_FG_FLAG_FULL		BIT(1)
+#define ASPIRE_EC_FG_FLAG_DISCHARGING	BIT(2)
+#define ASPIRE_EC_FG_FLAG_CHARGING	BIT(3)
+
+#define ASPIRE_EC_RAM_READ		0x20
+#define ASPIRE_EC_RAM_WRITE		0x21
+
+#define ASPIRE_EC_RAM_WATCHDOG		0x19
+#define ASPIRE_EC_RAM_KBD_MODE		0x43
+
+#define ASPIRE_EC_RAM_KBD_FN_EN		BIT(0)
+#define ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP	BIT(5)
+#define ASPIRE_EC_RAM_KBD_ALWAYS_SET	BIT(6)
+#define ASPIRE_EC_RAM_KBD_NUM_LAYER_EN	BIT(7)
+
+#define ASPIRE_EC_RAM_KBD_MODE_2	0x60
+
+#define ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY	BIT(3)
+
+#define ASPIRE_EC_RAM_HPD_STATUS	0xf4
+#define ASPIRE_EC_HPD_CONNECTED		0x03
+
+#define ASPIRE_EC_RAM_LID_STATUS	0x4c
+#define ASPIRE_EC_LID_OPEN		BIT(6)
+
+struct aspire_ec {
+	struct i2c_client *client;
+	struct power_supply *psy;
+	struct input_dev *idev;
+
+	bool bridge_configured;
+	struct drm_bridge bridge;
+	struct work_struct work;
+};
+
+struct aspire_ec_psy_static_data {
+	u8 unk1;
+	u8 flags;
+	__le16 unk2;
+	__le16 voltage_design;
+	__le16 capacity_full;
+	__le16 unk3;
+	__le16 serial;
+	u8 model_id;
+	u8 vendor_id;
+} __packed;
+
+static const char * const aspire_ec_psy_battery_model[] = {
+	"AP18C4K",
+	"AP18C8K",
+	"AP19B8K",
+	"AP16M4J",
+	"AP16M5J",
+};
+
+static const char * const aspire_ec_psy_battery_vendor[] = {
+	"SANYO",
+	"SONY",
+	"PANASONIC",
+	"SAMSUNG",
+	"SIMPLO",
+	"MOTOROLA",
+	"CELXPERT",
+	"LGC",
+	"GETAC",
+	"MURATA",
+};
+
+struct aspire_ec_psy_dynamic_data {
+	u8 unk1;
+	u8 flags;
+	u8 unk2;
+	__le16 capacity_now;
+	__le16 voltage_now;
+	__le16 current_now;
+	__le16 unk3;
+	__le16 unk4;
+} __packed;
+
+static int aspire_ec_psy_get_property(struct power_supply *psy,
+				      enum power_supply_property psp,
+				      union power_supply_propval *val)
+{
+	struct aspire_ec *ec = power_supply_get_drvdata(psy);
+	struct aspire_ec_psy_static_data sdat;
+	struct aspire_ec_psy_dynamic_data ddat;
+
+	i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_STATIC, sizeof(sdat), (u8 *)&sdat);
+	i2c_smbus_read_i2c_block_data(ec->client, ASPIRE_EC_FG_DYNAMIC, sizeof(ddat), (u8 *)&ddat);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+		if (ddat.flags & ASPIRE_EC_FG_FLAG_CHARGING)
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		else if (ddat.flags & ASPIRE_EC_FG_FLAG_DISCHARGING)
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		else if (ddat.flags & ASPIRE_EC_FG_FLAG_FULL)
+			val->intval = POWER_SUPPLY_STATUS_FULL;
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = le16_to_cpu(ddat.voltage_now) * 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		val->intval = le16_to_cpu(sdat.voltage_design) * 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		val->intval = le16_to_cpu(ddat.capacity_now) * 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		val->intval = le16_to_cpu(sdat.capacity_full) * 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = le16_to_cpu(ddat.capacity_now) * 100;
+		val->intval /= le16_to_cpu(sdat.capacity_full);
+		break;
+
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
+
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_psy_battery_model))
+			val->strval = aspire_ec_psy_battery_model[sdat.model_id - 1];
+		else
+			val->strval = "Unknown";
+		break;
+
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_psy_battery_vendor))
+			val->strval = aspire_ec_psy_battery_vendor[sdat.vendor_id - 3];
+		else
+			val->strval = "Unknown";
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property aspire_ec_psy_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_CHARGE_FULL,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_SCOPE,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+};
+
+static const struct power_supply_desc aspire_ec_psy_desc = {
+	.name		= "aspire-ec",
+	.type		= POWER_SUPPLY_TYPE_BATTERY,
+	.get_property	= aspire_ec_psy_get_property,
+	.properties	= aspire_ec_psy_props,
+	.num_properties	= ARRAY_SIZE(aspire_ec_psy_props),
+};
+
+static int aspire_ec_ram_read(struct i2c_client *client, u8 off, u8 *data, u8 data_len)
+{
+	i2c_smbus_write_byte_data(client, ASPIRE_EC_RAM_READ, off);
+	i2c_smbus_read_i2c_block_data(client, ASPIRE_EC_RAM_READ, data_len, data);
+	return 0;
+}
+
+static int aspire_ec_ram_write(struct i2c_client *client, u8 off, u8 data)
+{
+	u8 tmp[2] = {off, data};
+
+	i2c_smbus_write_i2c_block_data(client, ASPIRE_EC_RAM_WRITE, sizeof(tmp), tmp);
+	return 0;
+}
+
+static irqreturn_t aspire_ec_irq_handler(int irq, void *data)
+{
+	struct aspire_ec *ec = data;
+	int id;
+	u8 tmp;
+
+	/*
+	 * The original ACPI firmware actually has a small sleep in the handler.
+	 *
+	 * It seems like in most cases it's not needed but when the device
+	 * just exits suspend, our i2c driver has a brief time where data
+	 * transfer is not possible yet. So this delay allows us to suppress
+	 * quite a bunch of spurious error messages in dmesg. Thus it's kept.
+	 */
+	usleep_range(15000, 30000);
+
+	id = i2c_smbus_read_byte_data(ec->client, ASPIRE_EC_EVENT);
+	if (id < 0) {
+		dev_err(&ec->client->dev, "Failed to read event id: %pe\n", ERR_PTR(id));
+		return IRQ_HANDLED;
+	}
+
+	switch (id) {
+	case 0x0: /* No event */
+		break;
+
+	case ASPIRE_EC_EVENT_WATCHDOG:
+		/*
+		 * Here acpi responds to the event and clears some bit.
+		 * Notify (\_SB.I2C3.BAT1, 0x81) // Information Change
+		 * Notify (\_SB.I2C3.ADP1, 0x80) // Status Change
+		 */
+		aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_WATCHDOG, &tmp, sizeof(tmp));
+		aspire_ec_ram_write(ec->client, ASPIRE_EC_RAM_WATCHDOG, tmp & 0xbf);
+		break;
+
+	case ASPIRE_EC_EVENT_LID_CLOSE:
+		/* Notify (\_SB.LID0, 0x80) // Status Change */
+		input_report_switch(ec->idev, SW_LID, 1);
+		input_sync(ec->idev);
+		break;
+
+	case ASPIRE_EC_EVENT_LID_OPEN:
+		/* Notify (\_SB.LID0, 0x80) // Status Change */
+		input_report_switch(ec->idev, SW_LID, 0);
+		input_sync(ec->idev);
+		break;
+
+	case ASPIRE_EC_EVENT_FG_INF_CHG:
+		/* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */
+	case ASPIRE_EC_EVENT_FG_STA_CHG:
+		/* Notify (\_SB.I2C3.BAT1, 0x80) // Status Change */
+		power_supply_changed(ec->psy);
+		break;
+
+	case ASPIRE_EC_EVENT_HPD_DIS:
+		if (ec->bridge_configured)
+			drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected);
+		break;
+
+	case ASPIRE_EC_EVENT_HPD_CON:
+		if (ec->bridge_configured)
+			drm_bridge_hpd_notify(&ec->bridge, connector_status_connected);
+		break;
+
+	default:
+		dev_warn(&ec->client->dev, "Unknown event id=0x%x\n", id);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int aspire_ec_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags)
+{
+	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
+}
+
+static void aspire_ec_bridge_update_hpd_work(struct work_struct *work)
+{
+	struct aspire_ec *ec = container_of(work, struct aspire_ec, work);
+	u8 tmp;
+
+	aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_HPD_STATUS, &tmp, sizeof(tmp));
+	if (tmp == ASPIRE_EC_HPD_CONNECTED)
+		drm_bridge_hpd_notify(&ec->bridge, connector_status_connected);
+	else
+		drm_bridge_hpd_notify(&ec->bridge, connector_status_disconnected);
+}
+
+static void aspire_ec_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+	struct aspire_ec *ec = container_of(bridge, struct aspire_ec, bridge);
+
+	schedule_work(&ec->work);
+}
+
+static const struct drm_bridge_funcs aspire_ec_bridge_funcs = {
+	.hpd_enable = aspire_ec_bridge_hpd_enable,
+	.attach = aspire_ec_bridge_attach,
+};
+
+static int aspire_ec_probe(struct i2c_client *client)
+{
+	struct power_supply_config psy_cfg = {0};
+	struct device *dev = &client->dev;
+	struct fwnode_handle *fwnode;
+	struct aspire_ec *ec;
+	int ret;
+	u8 tmp;
+
+	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	ec->client = client;
+	i2c_set_clientdata(client, ec);
+
+	/* Battery status reports */
+	psy_cfg.drv_data = ec;
+	ec->psy = devm_power_supply_register(dev, &aspire_ec_psy_desc, &psy_cfg);
+	if (IS_ERR(ec->psy))
+		return dev_err_probe(dev, PTR_ERR(ec->psy),
+				     "Failed to register power supply\n");
+
+	/* Lid switch */
+	ec->idev = devm_input_allocate_device(dev);
+	if (!ec->idev)
+		return -ENOMEM;
+
+	ec->idev->name = "aspire-ec";
+	ec->idev->phys = "aspire-ec/input0";
+	input_set_capability(ec->idev, EV_SW, SW_LID);
+
+	ret = input_register_device(ec->idev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Input device register failed\n");
+
+	/* Enable the keyboard fn keys */
+	tmp = ASPIRE_EC_RAM_KBD_FN_EN | ASPIRE_EC_RAM_KBD_ALWAYS_SET;
+	if (device_property_read_bool(dev, "acer,media-keys-on-top"))
+		tmp |= ASPIRE_EC_RAM_KBD_MEDIA_ON_TOP;
+	aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE, tmp);
+
+	aspire_ec_ram_read(client, ASPIRE_EC_RAM_KBD_MODE_2, &tmp, sizeof(tmp));
+	tmp |= ASPIRE_EC_RAM_KBD_MEDIA_NOTIFY;
+	aspire_ec_ram_write(client, ASPIRE_EC_RAM_KBD_MODE_2, tmp);
+
+	/* External Type-C display attach reports */
+	fwnode = device_get_named_child_node(dev, "connector");
+	if (fwnode) {
+		INIT_WORK(&ec->work, aspire_ec_bridge_update_hpd_work);
+		ec->bridge.funcs = &aspire_ec_bridge_funcs;
+		ec->bridge.of_node = to_of_node(fwnode);
+		ec->bridge.ops = DRM_BRIDGE_OP_HPD;
+		ec->bridge.type = DRM_MODE_CONNECTOR_USB;
+
+		ret = devm_drm_bridge_add(dev, &ec->bridge);
+		if (ret) {
+			fwnode_handle_put(fwnode);
+			return dev_err_probe(dev, ret, "Failed to register drm bridge\n");
+		}
+
+		ec->bridge_configured = true;
+	}
+
+	ret = devm_request_threaded_irq(dev, client->irq, NULL,
+					aspire_ec_irq_handler, IRQF_ONESHOT,
+					dev_name(dev), ec);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request irq\n");
+
+	return 0;
+}
+
+static int aspire_ec_resume(struct device *dev)
+{
+	struct aspire_ec *ec = i2c_get_clientdata(to_i2c_client(dev));
+	u8 tmp;
+
+	aspire_ec_ram_read(ec->client, ASPIRE_EC_RAM_LID_STATUS, &tmp, sizeof(tmp));
+	input_report_switch(ec->idev, SW_LID, !!(tmp & ASPIRE_EC_LID_OPEN));
+	input_sync(ec->idev);
+
+	return 0;
+}
+
+static const struct i2c_device_id aspire_ec_id[] = {
+	{ "aspire1-ec", },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, aspire_ec_id);
+
+static const struct of_device_id aspire_ec_of_match[] = {
+	{ .compatible = "acer,aspire1-ec", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, aspire_ec_of_match);
+
+static DEFINE_SIMPLE_DEV_PM_OPS(aspire_ec_pm_ops, NULL, aspire_ec_resume);
+
+static struct i2c_driver aspire_ec_driver = {
+	.driver = {
+		.name = "aspire-ec",
+		.of_match_table = aspire_ec_of_match,
+		.pm = pm_sleep_ptr(&aspire_ec_pm_ops),
+	},
+	.probe = aspire_ec_probe,
+	.id_table = aspire_ec_id,
+};
+module_i2c_driver(aspire_ec_driver);
+
+MODULE_DESCRIPTION("Acer Aspire 1 embedded controller");
+MODULE_AUTHOR("Nikita Travkin <nikita@trvn.ru>");
+MODULE_LICENSE("GPL");

-- 
2.43.0

[PATCH v2 3/3] arm64: dts: qcom: acer-aspire1: Add embedded controller

[Nikita Travkin]

The laptop contains an embedded controller that provides a set of
features:

- Battery and charger monitoring
- USB Type-C DP alt mode HPD monitoring
- Lid status detection
- Small amount of keyboard configuration*

[*] The keyboard is handled by the same EC but it has a dedicated i2c
bus and is already enabled. This port only provides fn key behavior
configuration.

Add the EC to the device tree and describe the relationship between the
EC-managed type-c port and the SoC DisplayPort.

Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
 arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts | 40 +++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts b/arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts
index dbb48934d499..a29f542fa612 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts
@@ -147,7 +147,25 @@ &i2c2 {
 	clock-frequency = <400000>;
 	status = "okay";
 
-	/* embedded-controller@76 */
+	embedded-controller@76 {
+		compatible = "acer,aspire1-ec";
+		reg = <0x76>;
+
+		interrupts-extended = <&tlmm 30 IRQ_TYPE_LEVEL_LOW>;
+
+		pinctrl-0 = <&ec_int_default>;
+		pinctrl-names = "default";
+
+		connector {
+			compatible = "usb-c-connector";
+
+			port {
+				ec_dp_in: endpoint {
+					remote-endpoint = <&mdss_dp_out>;
+				};
+			};
+		};
+	};
 };
 
 &i2c4 {
@@ -298,6 +316,19 @@ &mdss {
 	status = "okay";
 };
 
+&mdss_dp {
+	data-lanes = <0 1>;
+
+	vdda-1p2-supply = <&vreg_l3c_1p2>;
+	vdda-0p9-supply = <&vreg_l4a_0p8>;
+
+	status = "okay";
+};
+
+&mdss_dp_out {
+	remote-endpoint = <&ec_dp_in>;
+};
+
 &mdss_dsi0 {
 	vdda-supply = <&vreg_l3c_1p2>;
 	status = "okay";
@@ -687,6 +718,13 @@ codec_irq_default: codec-irq-deault-state {
 		bias-disable;
 	};
 
+	ec_int_default: ec-int-default-state {
+		pins = "gpio30";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+	};
+
 	edp_bridge_irq_default: edp-bridge-irq-default-state {
 		pins = "gpio11";
 		function = "gpio";

-- 
2.43.0

Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Conor Dooley]

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

Hey,

On Tue, Dec 12, 2023 at 05:49:09PM +0500, Nikita Travkin wrote:
> Add binding for the EC found in the Acer Aspire 1 laptop.
> 
> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> ---
>  .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> new file mode 100644
> index 000000000000..1fbf1272a00f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Acer Aspire 1 Embedded Controller
> +
> +maintainers:
> +  - Nikita Travkin <nikita@trvn.ru>
> +
> +description:
> +  The Acer Aspire 1 laptop uses an embedded controller to control battery
> +  and charging as well as to provide a set of misc features such as the
> +  laptop lid status and HPD events for the USB Type-C DP alt mode.
> +
> +properties:
> +  compatible:
> +    const: acer,aspire1-ec
> +
> +  reg:
> +    const: 0x76
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  acer,media-keys-on-top:
> +    description: Configure the keyboard layout to use media features of
> +      the fn row when the fn key is not pressed. The firmware may choose
> +      to add this property when user selects the fn mode in the firmware
> +      setup utility.

I'm not a huge fan of the property name/description here.
For the description, I'm not sure from reading this what the default
behaviour is and how the fn row works when the fn key is pressed.
Is the default behaviour that pressing the fn key enables the media keys
and the row by default provides the fn functionality? And then when this
option is set in firmware the behaviour is inverted?

For the name, the "on-top" bit seems a bit weird. I would prefer it to
be reworded in terms of the behavioural change of the fn key. The media
keys are physically at the top of the keyboard whether or not this
option is enabled, hence the "on-top" seeming a bit weird to me.

Thanks,
Conor.

> +    type: boolean
> +
> +  connector:
> +    $ref: /schemas/connector/usb-connector.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        embedded-controller@76 {
> +            compatible = "acer,aspire1-ec";
> +            reg = <0x76>;
> +
> +            interrupts-extended = <&tlmm 30 IRQ_TYPE_LEVEL_LOW>;
> +
> +            connector {
> +                compatible = "usb-c-connector";
> +
> +                port {
> +                    ec_dp_in: endpoint {
> +                        remote-endpoint = <&mdss_dp_out>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> 
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Nikita Travkin]

Conor Dooley писал(а) 12.12.2023 22:24:
> Hey,
> 
> On Tue, Dec 12, 2023 at 05:49:09PM +0500, Nikita Travkin wrote:
>> Add binding for the EC found in the Acer Aspire 1 laptop.
>>
>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>> ---
>>  .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
>>  1 file changed, 67 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
>> new file mode 100644
>> index 000000000000..1fbf1272a00f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
>> @@ -0,0 +1,67 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Acer Aspire 1 Embedded Controller
>> +
>> +maintainers:
>> +  - Nikita Travkin <nikita@trvn.ru>
>> +
>> +description:
>> +  The Acer Aspire 1 laptop uses an embedded controller to control battery
>> +  and charging as well as to provide a set of misc features such as the
>> +  laptop lid status and HPD events for the USB Type-C DP alt mode.
>> +
>> +properties:
>> +  compatible:
>> +    const: acer,aspire1-ec
>> +
>> +  reg:
>> +    const: 0x76
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  acer,media-keys-on-top:
>> +    description: Configure the keyboard layout to use media features of
>> +      the fn row when the fn key is not pressed. The firmware may choose
>> +      to add this property when user selects the fn mode in the firmware
>> +      setup utility.
> 
> I'm not a huge fan of the property name/description here.
> For the description, I'm not sure from reading this what the default
> behaviour is and how the fn row works when the fn key is pressed.
> Is the default behaviour that pressing the fn key enables the media keys
> and the row by default provides the fn functionality? And then when this
> option is set in firmware the behaviour is inverted?
> 

Yes. By default pressing i.e. F11 would create F11, Fn+F11 would be vol+.
With this option set, the default for F11 would be vol+ and Fn+f11 would
be F11.

Perhaps this description would be better?

  Configure the keyboard layout to invert the Fn key. By default the
  function row of the keyboard inputs function keys (i.e F11) when
  Fn is not pressed. With this option set, pressing the key without
  Fn would input media keys (i.e. Vol-Up). Firmware may (...) 

> For the name, the "on-top" bit seems a bit weird. I would prefer it to
> be reworded in terms of the behavioural change of the fn key. The media
> keys are physically at the top of the keyboard whether or not this
> option is enabled, hence the "on-top" seeming a bit weird to me.
> 

I was trying to be unambiguous with this name without making
it too long. The implied meaning of "on-top" was "On the top
keyboard layer" (in the opposition of the "Fn" layer). Perhaps
"acer,media-keys-on-top-layer" would have been better then?

I was trying to define this property via the word "media"
since using "fn" is somewhat ambiguous (fn row key vs F(1..12) key
vs Fn key), and something like `inverted-fn-key" would not reflect
what is the impact on the layout. Using "fn-selects-function-keys"
also felt a bit ambiguous to me.

FWIW I can also invert the default and make it
"fn-selects-media-keys" which would be clear. I haven't yet
written the firmware that would pass this property from the
setup so it shouldn't be a problem from the DT POV, but this
is not the "reset" default for the EC.

Do any of those suggestions sound better to you?

Nikita

> Thanks,
> Conor.
> 
>> +    type: boolean
>> +
>> +  connector:
>> +    $ref: /schemas/connector/usb-connector.yaml#
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |+
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        embedded-controller@76 {
>> +            compatible = "acer,aspire1-ec";
>> +            reg = <0x76>;
>> +
>> +            interrupts-extended = <&tlmm 30 IRQ_TYPE_LEVEL_LOW>;
>> +
>> +            connector {
>> +                compatible = "usb-c-connector";
>> +
>> +                port {
>> +                    ec_dp_in: endpoint {
>> +                        remote-endpoint = <&mdss_dp_out>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +    };
>>
>> --
>> 2.43.0
>>

Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Rob Herring]

On Tue, Dec 12, 2023 at 05:49:09PM +0500, Nikita Travkin wrote:
> Add binding for the EC found in the Acer Aspire 1 laptop.
> 
> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> ---
>  .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> new file mode 100644
> index 000000000000..1fbf1272a00f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Acer Aspire 1 Embedded Controller
> +
> +maintainers:
> +  - Nikita Travkin <nikita@trvn.ru>
> +
> +description:
> +  The Acer Aspire 1 laptop uses an embedded controller to control battery
> +  and charging as well as to provide a set of misc features such as the
> +  laptop lid status and HPD events for the USB Type-C DP alt mode.
> +
> +properties:
> +  compatible:
> +    const: acer,aspire1-ec
> +
> +  reg:
> +    const: 0x76
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  acer,media-keys-on-top:
> +    description: Configure the keyboard layout to use media features of
> +      the fn row when the fn key is not pressed. The firmware may choose
> +      to add this property when user selects the fn mode in the firmware
> +      setup utility.
> +    type: boolean

Besides the naming, this isn't really a property of the EC, but really 
part of the keyboard layout. It seems you just stuck it here because 
this is part of the specific device.

It is also hardly a feature unique to this device. I'm typing this from 
a device with the exact same thing (M1 Macbook Pro). Actually, all 3 
laptops I have in front of me have the same thing. The other 2 have 
a Fnlock (Fn+ESC) though.  On the M1, it's just a module param which I 
set as persistent. Though I now wonder if the Fnlock could be 
implemented on it too. Being able to switch whenever I want would be 
nice. That would probably have to be in Linux where as these other 
laptops probably implement this in their EC/firmware?

What I'm getting at is controlling changing this in firmware is not a 
great experience and this should all be common.

Rob

Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Nikita Travkin]

Rob Herring писал(а) 15.12.2023 03:02:
> On Tue, Dec 12, 2023 at 05:49:09PM +0500, Nikita Travkin wrote:
>> Add binding for the EC found in the Acer Aspire 1 laptop.
>>
>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>> ---
>>  .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
>>  1 file changed, 67 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
>> new file mode 100644
>> index 000000000000..1fbf1272a00f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
>> @@ -0,0 +1,67 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Acer Aspire 1 Embedded Controller
>> +
>> +maintainers:
>> +  - Nikita Travkin <nikita@trvn.ru>
>> +
>> +description:
>> +  The Acer Aspire 1 laptop uses an embedded controller to control battery
>> +  and charging as well as to provide a set of misc features such as the
>> +  laptop lid status and HPD events for the USB Type-C DP alt mode.
>> +
>> +properties:
>> +  compatible:
>> +    const: acer,aspire1-ec
>> +
>> +  reg:
>> +    const: 0x76
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  acer,media-keys-on-top:
>> +    description: Configure the keyboard layout to use media features of
>> +      the fn row when the fn key is not pressed. The firmware may choose
>> +      to add this property when user selects the fn mode in the firmware
>> +      setup utility.
>> +    type: boolean
> 
> Besides the naming, this isn't really a property of the EC, but really 
> part of the keyboard layout. It seems you just stuck it here because 
> this is part of the specific device.
> 

The EC on this device is also a keyboard controller, but the keyboard
part has a dedicated i2c bus with hid-over-i2c. Since this is the
"management" bus of the same device, I decided that it fits here.

> It is also hardly a feature unique to this device. I'm typing this from 
> a device with the exact same thing (M1 Macbook Pro). Actually, all 3 
> laptops I have in front of me have the same thing. The other 2 have 
> a Fnlock (Fn+ESC) though.  On the M1, it's just a module param which I 
> set as persistent. Though I now wonder if the Fnlock could be 
> implemented on it too. Being able to switch whenever I want would be 
> nice. That would probably have to be in Linux where as these other 
> laptops probably implement this in their EC/firmware?
> 
> What I'm getting at is controlling changing this in firmware is not a 
> great experience and this should all be common.
> 

You may be right, however my goal here is to support the original
firmware feature that is lost when we use DT.

This is a WoA laptop with UEFI/ACPI and, as usual for "Windows"
machines, there is a setting in the firmware setup utility ("bios") to
set the fn behavior. But it works by setting an ACPI value, and for
Snapdragon devices we can't use that now.

Long term I want to have a EFI driver that would automatically
detect/load DT and my plan is to handle things like this (and i.e. mac
address, different touchpad vendor, etc) there. Thus I'm adding this
property already, as an equivalent of that weird acpi bit that original
firmware sets.

If we only provide a module param, the "intended by OEM" way of setting
the fn mode will be broken, and one would need to know how to write a
magic special config file to set a kernel module param. I think it's not
the best UX. (and just adds to the silly "arm/dt bad, x86/uefi/acpi
"just works"" argument many people sadly have)

If you think I shouldn't use DT to pass this info, feel free to say so.
I will drop this property and see if there is something else I can do
to still support this without relying on Linux cooperation.

Looking forward to your opinion,
Nikita

> Rob

Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Nikita Travkin]

Nikita Travkin писал(а) 15.12.2023 10:29:
> Rob Herring писал(а) 15.12.2023 03:02:
>> On Tue, Dec 12, 2023 at 05:49:09PM +0500, Nikita Travkin wrote:
>>> Add binding for the EC found in the Acer Aspire 1 laptop.
>>>
>>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>>> ---
>>>  .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
>>>  1 file changed, 67 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
>>> new file mode 100644
>>> index 000000000000..1fbf1272a00f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
>>> @@ -0,0 +1,67 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Acer Aspire 1 Embedded Controller
>>> +
>>> +maintainers:
>>> +  - Nikita Travkin <nikita@trvn.ru>
>>> +
>>> +description:
>>> +  The Acer Aspire 1 laptop uses an embedded controller to control battery
>>> +  and charging as well as to provide a set of misc features such as the
>>> +  laptop lid status and HPD events for the USB Type-C DP alt mode.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: acer,aspire1-ec
>>> +
>>> +  reg:
>>> +    const: 0x76
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  acer,media-keys-on-top:
>>> +    description: Configure the keyboard layout to use media features of
>>> +      the fn row when the fn key is not pressed. The firmware may choose
>>> +      to add this property when user selects the fn mode in the firmware
>>> +      setup utility.
>>> +    type: boolean
>>
>> Besides the naming, this isn't really a property of the EC, but really
>> part of the keyboard layout. It seems you just stuck it here because
>> this is part of the specific device.
>>
> 
> The EC on this device is also a keyboard controller, but the keyboard
> part has a dedicated i2c bus with hid-over-i2c. Since this is the
> "management" bus of the same device, I decided that it fits here.
> 
>> It is also hardly a feature unique to this device. I'm typing this from
>> a device with the exact same thing (M1 Macbook Pro). Actually, all 3
>> laptops I have in front of me have the same thing. The other 2 have
>> a Fnlock (Fn+ESC) though.  On the M1, it's just a module param which I
>> set as persistent. Though I now wonder if the Fnlock could be
>> implemented on it too. Being able to switch whenever I want would be
>> nice. That would probably have to be in Linux where as these other
>> laptops probably implement this in their EC/firmware?
>>
>> What I'm getting at is controlling changing this in firmware is not a
>> great experience and this should all be common.
>>
> 
> You may be right, however my goal here is to support the original
> firmware feature that is lost when we use DT.
> 
> This is a WoA laptop with UEFI/ACPI and, as usual for "Windows"
> machines, there is a setting in the firmware setup utility ("bios") to
> set the fn behavior. But it works by setting an ACPI value, and for
> Snapdragon devices we can't use that now.
> 
> Long term I want to have a EFI driver that would automatically
> detect/load DT and my plan is to handle things like this (and i.e. mac
> address, different touchpad vendor, etc) there. Thus I'm adding this
> property already, as an equivalent of that weird acpi bit that original
> firmware sets.
> 
> If we only provide a module param, the "intended by OEM" way of setting
> the fn mode will be broken, and one would need to know how to write a
> magic special config file to set a kernel module param. I think it's not
> the best UX. (and just adds to the silly "arm/dt bad, x86/uefi/acpi
> "just works"" argument many people sadly have)
> 
> If you think I shouldn't use DT to pass this info, feel free to say so.
> I will drop this property and see if there is something else I can do
> to still support this without relying on Linux cooperation.
> 

Hi Rob, Conor,

I'd still appreciate hearing your opinion before proceeding with this.

Nikita

> Looking forward to your opinion,
> Nikita
> 
>> Rob

Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Rob Herring]

On Fri, Dec 15, 2023 at 10:29:22AM +0500, Nikita Travkin wrote:
> Rob Herring писал(а) 15.12.2023 03:02:
> > On Tue, Dec 12, 2023 at 05:49:09PM +0500, Nikita Travkin wrote:
> >> Add binding for the EC found in the Acer Aspire 1 laptop.
> >>
> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> >> ---
> >>  .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
> >>  1 file changed, 67 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> >> new file mode 100644
> >> index 000000000000..1fbf1272a00f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> >> @@ -0,0 +1,67 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Acer Aspire 1 Embedded Controller
> >> +
> >> +maintainers:
> >> +  - Nikita Travkin <nikita@trvn.ru>
> >> +
> >> +description:
> >> +  The Acer Aspire 1 laptop uses an embedded controller to control battery
> >> +  and charging as well as to provide a set of misc features such as the
> >> +  laptop lid status and HPD events for the USB Type-C DP alt mode.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: acer,aspire1-ec
> >> +
> >> +  reg:
> >> +    const: 0x76
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> >> +  acer,media-keys-on-top:
> >> +    description: Configure the keyboard layout to use media features of
> >> +      the fn row when the fn key is not pressed. The firmware may choose
> >> +      to add this property when user selects the fn mode in the firmware
> >> +      setup utility.
> >> +    type: boolean
> > 
> > Besides the naming, this isn't really a property of the EC, but really 
> > part of the keyboard layout. It seems you just stuck it here because 
> > this is part of the specific device.
> > 
> 
> The EC on this device is also a keyboard controller, but the keyboard
> part has a dedicated i2c bus with hid-over-i2c. Since this is the
> "management" bus of the same device, I decided that it fits here.

So there's also a hid-over-i2c DT node? Then why wouldn't you put this 
there?

> 
> > It is also hardly a feature unique to this device. I'm typing this from 
> > a device with the exact same thing (M1 Macbook Pro). Actually, all 3 
> > laptops I have in front of me have the same thing. The other 2 have 
> > a Fnlock (Fn+ESC) though.  On the M1, it's just a module param which I 
> > set as persistent. Though I now wonder if the Fnlock could be 
> > implemented on it too. Being able to switch whenever I want would be 
> > nice. That would probably have to be in Linux where as these other 
> > laptops probably implement this in their EC/firmware?
> > 
> > What I'm getting at is controlling changing this in firmware is not a 
> > great experience and this should all be common.
> > 
> 
> You may be right, however my goal here is to support the original
> firmware feature that is lost when we use DT.
> 
> This is a WoA laptop with UEFI/ACPI and, as usual for "Windows"
> machines, there is a setting in the firmware setup utility ("bios") to
> set the fn behavior. But it works by setting an ACPI value, and for
> Snapdragon devices we can't use that now.
> 
> Long term I want to have a EFI driver that would automatically
> detect/load DT and my plan is to handle things like this (and i.e. mac
> address, different touchpad vendor, etc) there. Thus I'm adding this
> property already, as an equivalent of that weird acpi bit that original
> firmware sets.
> 
> If we only provide a module param, the "intended by OEM" way of setting
> the fn mode will be broken, and one would need to know how to write a
> magic special config file to set a kernel module param. I think it's not
> the best UX. (and just adds to the silly "arm/dt bad, x86/uefi/acpi
> "just works"" argument many people sadly have)

But it always works, it is just a question of what is the default mode 
and I, as a user, want to decide that, not the OEM. And I want to change 
it at run-time, not reboot into BIOS to change it.

I wasn't suggesting you do a module param either. That's still specific 
to the module. Something like a sysfs file would be nice:

echo 1 >  /sys/class/input/input1/fnlock


> If you think I shouldn't use DT to pass this info, feel free to say so.
> I will drop this property and see if there is something else I can do
> to still support this without relying on Linux cooperation.

Not saying no to being in DT, but if it is, it should be a common 
property because it is a common thing on all laptops.

Rob

Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Nikita Travkin]

Rob Herring писал(а) 23.02.2024 09:52:
> On Fri, Dec 15, 2023 at 10:29:22AM +0500, Nikita Travkin wrote:
>> Rob Herring писал(а) 15.12.2023 03:02:
>> > On Tue, Dec 12, 2023 at 05:49:09PM +0500, Nikita Travkin wrote:
>> >> Add binding for the EC found in the Acer Aspire 1 laptop.
>> >>
>> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>> >> ---
>> >>  .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
>> >>  1 file changed, 67 insertions(+)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
>> >> new file mode 100644
>> >> index 000000000000..1fbf1272a00f
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
>> >> @@ -0,0 +1,67 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: Acer Aspire 1 Embedded Controller
>> >> +
>> >> +maintainers:
>> >> +  - Nikita Travkin <nikita@trvn.ru>
>> >> +
>> >> +description:
>> >> +  The Acer Aspire 1 laptop uses an embedded controller to control battery
>> >> +  and charging as well as to provide a set of misc features such as the
>> >> +  laptop lid status and HPD events for the USB Type-C DP alt mode.
>> >> +
>> >> +properties:
>> >> +  compatible:
>> >> +    const: acer,aspire1-ec
>> >> +
>> >> +  reg:
>> >> +    const: 0x76
>> >> +
>> >> +  interrupts:
>> >> +    maxItems: 1
>> >> +
>> >> +  acer,media-keys-on-top:
>> >> +    description: Configure the keyboard layout to use media features of
>> >> +      the fn row when the fn key is not pressed. The firmware may choose
>> >> +      to add this property when user selects the fn mode in the firmware
>> >> +      setup utility.
>> >> +    type: boolean
>> >
>> > Besides the naming, this isn't really a property of the EC, but really
>> > part of the keyboard layout. It seems you just stuck it here because
>> > this is part of the specific device.
>> >
>>
>> The EC on this device is also a keyboard controller, but the keyboard
>> part has a dedicated i2c bus with hid-over-i2c. Since this is the
>> "management" bus of the same device, I decided that it fits here.
> 
> So there's also a hid-over-i2c DT node? Then why wouldn't you put this 
> there?
> 

Yes indeed, however I wasn't sure of a nice way to make two drivers
interact so I opted to having the prop here instead given I was trying
to reflect "device specific" (in terms of uefi setup -> os code
interaction) property.

>>
>> > It is also hardly a feature unique to this device. I'm typing this from
>> > a device with the exact same thing (M1 Macbook Pro). Actually, all 3
>> > laptops I have in front of me have the same thing. The other 2 have
>> > a Fnlock (Fn+ESC) though.  On the M1, it's just a module param which I
>> > set as persistent. Though I now wonder if the Fnlock could be
>> > implemented on it too. Being able to switch whenever I want would be
>> > nice. That would probably have to be in Linux where as these other
>> > laptops probably implement this in their EC/firmware?
>> >
>> > What I'm getting at is controlling changing this in firmware is not a
>> > great experience and this should all be common.
>> >
>>
>> You may be right, however my goal here is to support the original
>> firmware feature that is lost when we use DT.
>>
>> This is a WoA laptop with UEFI/ACPI and, as usual for "Windows"
>> machines, there is a setting in the firmware setup utility ("bios") to
>> set the fn behavior. But it works by setting an ACPI value, and for
>> Snapdragon devices we can't use that now.
>>
>> Long term I want to have a EFI driver that would automatically
>> detect/load DT and my plan is to handle things like this (and i.e. mac
>> address, different touchpad vendor, etc) there. Thus I'm adding this
>> property already, as an equivalent of that weird acpi bit that original
>> firmware sets.
>>
>> If we only provide a module param, the "intended by OEM" way of setting
>> the fn mode will be broken, and one would need to know how to write a
>> magic special config file to set a kernel module param. I think it's not
>> the best UX. (and just adds to the silly "arm/dt bad, x86/uefi/acpi
>> "just works"" argument many people sadly have)
> 
> But it always works, it is just a question of what is the default mode 
> and I, as a user, want to decide that, not the OEM. And I want to change 
> it at run-time, not reboot into BIOS to change it.
> 
> I wasn't suggesting you do a module param either. That's still specific 
> to the module. Something like a sysfs file would be nice:
> 
> echo 1 >  /sys/class/input/input1/fnlock
> 

I see your point. Having a generic way of switching fnlock, attached to
the correct input device would be great. However as I wasn't sure how
exactly that can be implemented in a generic manner, I opted to just
make sure that at least the user manual for this device still "works".

> 
>> If you think I shouldn't use DT to pass this info, feel free to say so.
>> I will drop this property and see if there is something else I can do
>> to still support this without relying on Linux cooperation.
> 
> Not saying no to being in DT, but if it is, it should be a common 
> property because it is a common thing on all laptops.
> 

Right. Then I think I will drop the property for now and will see if
we can introduce something better and generic later.

Thanks for explaining!
Nikita

> Rob