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

[PATCH v3 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 v3:
- Supress warning on few no-op events.
- Invert the fn key behavior (Rob, Conor)
- Link to v2: https://lore.kernel.org/r/20231212-aspire1-ec-v2-0-ca495ea0a7ac@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     |  69 ++++
 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             | 453 +++++++++++++++++++++
 5 files changed, 576 insertions(+), 1 deletion(-)
---
base-commit: 2d5c7b7eb345249cb34d42cbc2b97b4c57ea944e
change-id: 20231206-aspire1-ec-6b3d2cac1a72

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

[PATCH v3 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     | 69 ++++++++++++++++++++++
 1 file changed, 69 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..984cf19cf806
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
@@ -0,0 +1,69 @@
+# 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,fn-selects-media-keys:
+    description: Configure the keyboard layout to invert the Fn key.
+      By default the function row of the keyboard inputs media keys
+      (i.e Vol-Up) when Fn is not pressed. With this option set, pressing
+      the key without Fn would input function keys (i.e. F11). 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

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

[Conor Dooley]

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

Rob,

On Tue, Feb 20, 2024 at 04:57:12PM +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     | 69 ++++++++++++++++++++++
>  1 file changed, 69 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..984cf19cf806
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> @@ -0,0 +1,69 @@
> +# 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,fn-selects-media-keys:
> +    description: Configure the keyboard layout to invert the Fn key.
> +      By default the function row of the keyboard inputs media keys
> +      (i.e Vol-Up) when Fn is not pressed. With this option set, pressing
> +      the key without Fn would input function keys (i.e. F11). The
> +      firmware may choose to add this property when user selects the fn
> +      mode in the firmware setup utility.
> +    type: boolean

We both had some comments on this property, and Nikita tried to follow
up on yours (which was much more substantive than mine) but got no
response:
https://lore.kernel.org/all/20231214220210.GA988134-robh@kernel.org/

Reading what you said, I'm not entirely sure what you were looking for,
my guess is that you were wanted something controllable from userspace,
but I'm not sure how you figured that should work where the firmware
alone is able to control this.

Cheers,
Conor.

> +
> +  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 v3 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

[Rob Herring]

On Tue, Feb 20, 2024 at 06:41:06PM +0000, Conor Dooley wrote:
> Rob,
> 
> On Tue, Feb 20, 2024 at 04:57:12PM +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     | 69 ++++++++++++++++++++++
> >  1 file changed, 69 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..984cf19cf806
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> > @@ -0,0 +1,69 @@
> > +# 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,fn-selects-media-keys:
> > +    description: Configure the keyboard layout to invert the Fn key.
> > +      By default the function row of the keyboard inputs media keys
> > +      (i.e Vol-Up) when Fn is not pressed. With this option set, pressing
> > +      the key without Fn would input function keys (i.e. F11). The
> > +      firmware may choose to add this property when user selects the fn
> > +      mode in the firmware setup utility.
> > +    type: boolean
> 
> We both had some comments on this property, and Nikita tried to follow
> up on yours (which was much more substantive than mine) but got no
> response:
> https://lore.kernel.org/all/20231214220210.GA988134-robh@kernel.org/
> 
> Reading what you said, I'm not entirely sure what you were looking for,
> my guess is that you were wanted something controllable from userspace,
> but I'm not sure how you figured that should work where the firmware
> alone is able to control this.

I replied there, but what I want is whatever the solution is to work on 
any laptop, not just this Acer device.

Rob

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

[Krzysztof Kozlowski]

On 20/02/2024 12:57, Nikita Travkin wrote:
> Add binding for the EC found in the Acer Aspire 1 laptop.
> 
> Signed-off-by: Nikita Travkin <nikita@trvn.ru>

...

> +
> +  connector:
> +    $ref: /schemas/connector/usb-connector.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+

If there is going to be new posting: drop "+"

Best regards,
Krzysztof

[PATCH v3 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 | 453 +++++++++++++++++++++++++++++++++
 3 files changed, 468 insertions(+)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 3e31375491d5..e91a3acecb41 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -985,4 +985,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..f4a141ec44b8
--- /dev/null
+++ b/drivers/power/supply/acer-aspire1-ec.c
@@ -0,0 +1,453 @@
+// 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_KBD_BKL_ON	0x57
+#define ASPIRE_EC_EVENT_KBD_BKL_OFF	0x58
+#define ASPIRE_EC_EVENT_LID_CLOSE	0x9b
+#define ASPIRE_EC_EVENT_LID_OPEN	0x9c
+#define ASPIRE_EC_EVENT_BKL_UNBLANKED	0x9d
+#define ASPIRE_EC_EVENT_BKL_BLANKED	0x9e
+#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;
+
+	case ASPIRE_EC_EVENT_BKL_BLANKED:
+	case ASPIRE_EC_EVENT_BKL_UNBLANKED:
+		/* Display backlight blanked on FN+F6. No action needed. */
+		break;
+
+	case ASPIRE_EC_EVENT_KBD_BKL_ON:
+	case ASPIRE_EC_EVENT_KBD_BKL_OFF:
+		/*
+		 * There is a keyboard backlight connector
+		 * on Aspire 1 that is controlled by FN+F8.
+		 * There is no kb backlight on the device
+		 * though. Seems like this is used on other
+		 * devices like Acer Spin 7. No action needed.
+		 */
+		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,fn-selects-media-keys"))
+		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

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

[Sebastian Reichel]

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

Hi,

On Tue, Feb 20, 2024 at 04:57:13PM +0500, Nikita Travkin wrote:
> 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 | 453 +++++++++++++++++++++++++++++++++

I think this belongs into drivers/platform, as it handles all bits of
the EC.

[...]

>  3 files changed, 468 insertions(+)
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 3e31375491d5..e91a3acecb41 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -985,4 +985,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

I did not see any AC status bits?

> [...]

> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = 1;

You have an unused ASPIRE_EC_FG_FLAG_PRESENT, that looks like it
should be used here?

> [...]

Otherwise the power-supply bits LGTM.

-- Sebastian

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

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

[Nikita Travkin]

Sebastian Reichel писал(а) 22.02.2024 04:41:
> Hi,
> 
> On Tue, Feb 20, 2024 at 04:57:13PM +0500, Nikita Travkin wrote:
>> 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 | 453 +++++++++++++++++++++++++++++++++
> 
> I think this belongs into drivers/platform, as it handles all bits of
> the EC.
> 

Hm, I initially submitted it to power/supply following the c630 driver,
but I think you're right... Though I'm not sure where in platform/ I'd
put this driver... (+CC Hans)

Seems like most of the things live in platform/x86 but there is no i.e.
platform/arm64...

Hans, (as a maintainer for most things in platform/) what do you think
would be the best place to put this (and at least two more I'd expect)
driver in inside platform/? And can we handle it through the
platform-driver-x86 list?

> [...]
> 
>>  3 files changed, 468 insertions(+)
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index 3e31375491d5..e91a3acecb41 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -985,4 +985,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
> 
> I did not see any AC status bits?
> 

I was referring to whatever ACPI spec calls "AC Adapter" but I guess
I should have used the word "charger" instead... Will reword this.

>> [...]
> 
>> +	case POWER_SUPPLY_PROP_PRESENT:
>> +		val->intval = 1;
> 
> You have an unused ASPIRE_EC_FG_FLAG_PRESENT, that looks like it
> should be used here?
> 

Oh, you're right! I think I initially didn't have this property and
added it like this as a reaction to that upower change that made it
consider everything not explicitly present as absent.

I've just checked what is reported after unplugging the battery and
seems like the flag (as well as everything else) is gone. Will change
the driver to read the flag.

Thanks for your review!
Nikita

>> [...]
> 
> Otherwise the power-supply bits LGTM.
> 
> -- Sebastian

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

[Sebastian Reichel]

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

Hi,

On Fri, Feb 23, 2024 at 07:32:17PM +0500, Nikita Travkin wrote:
> >> + This driver provides battery and AC status support for the mentioned
> > 
> > I did not see any AC status bits?
> 
> I was referring to whatever ACPI spec calls "AC Adapter" but I guess
> I should have used the word "charger" instead... Will reword this.

But you only register a power-supply device for the battery and not
for the AC adapter/charger. When you write "and AC status support" I
would have expected something similar to this (that's from ACPI AC
adapter driver):

$ cat /sys/class/power_supply/AC/uevent
POWER_SUPPLY_NAME=AC
POWER_SUPPLY_TYPE=Mains
POWER_SUPPLY_ONLINE=1

-- Sebastian

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

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

[Nikita Travkin]

Sebastian Reichel писал(а) 23.02.2024 20:04:
> Hi,
> 
> On Fri, Feb 23, 2024 at 07:32:17PM +0500, Nikita Travkin wrote:
>> >> + This driver provides battery and AC status support for the mentioned
>> >
>> > I did not see any AC status bits?
>>
>> I was referring to whatever ACPI spec calls "AC Adapter" but I guess
>> I should have used the word "charger" instead... Will reword this.
> 
> But you only register a power-supply device for the battery and not
> for the AC adapter/charger. When you write "and AC status support" I
> would have expected something similar to this (that's from ACPI AC
> adapter driver):
> 
> $ cat /sys/class/power_supply/AC/uevent
> POWER_SUPPLY_NAME=AC
> POWER_SUPPLY_TYPE=Mains
> POWER_SUPPLY_ONLINE=1
> 

Ah, I see... Yeah looking at it one more time, I mistakenly assumed the
acpi ac code uses the same data fields as the battery but seems like it
reads the single online flag from a different place. I don't think there
is really a point on implementing that field since we can still easily
track the battery charging/discharging status so I will probably omit it
for now. Will reword the help text to not mention charger/ac adapter.

Thanks for clarifying!
Nikita

> -- Sebastian

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

[Sebastian Reichel]

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

Hi,

On Fri, Feb 23, 2024 at 08:34:29PM +0500, Nikita Travkin wrote:
> Sebastian Reichel писал(а) 23.02.2024 20:04:
> > On Fri, Feb 23, 2024 at 07:32:17PM +0500, Nikita Travkin wrote:
> >> >> + This driver provides battery and AC status support for the mentioned
> >> > I did not see any AC status bits?
> >>
> >> I was referring to whatever ACPI spec calls "AC Adapter" but I guess
> >> I should have used the word "charger" instead... Will reword this.
> > 
> > But you only register a power-supply device for the battery and not
> > for the AC adapter/charger. When you write "and AC status support" I
> > would have expected something similar to this (that's from ACPI AC
> > adapter driver):
> > 
> > $ cat /sys/class/power_supply/AC/uevent
> > POWER_SUPPLY_NAME=AC
> > POWER_SUPPLY_TYPE=Mains
> > POWER_SUPPLY_ONLINE=1
> 
> Ah, I see... Yeah looking at it one more time, I mistakenly assumed the
> acpi ac code uses the same data fields as the battery but seems like it
> reads the single online flag from a different place. I don't think there
> is really a point on implementing that field since we can still easily
> track the battery charging/discharging status so I will probably omit it
> for now. Will reword the help text to not mention charger/ac adapter.

If you have the information easily available, it's a good plan to
expose it.

Without a charger reporting online status at least the kernel's
power_supply_is_system_supplied() will return false (which is e.g.
used by AMD GPU driver to select power profile).

Generic userspace (i.e. upower) probably behaves similar, since
battery status is not the same as AC connceted. A system might
not charge the battery but still run from AC itself.

-- Sebastian

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

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

[Hans de Goede]

Hi,

+Ilpo (fellow pdx86 maintainer)

On 2/23/24 15:32, Nikita Travkin wrote:
> Sebastian Reichel писал(а) 22.02.2024 04:41:
>> Hi,
>>
>> On Tue, Feb 20, 2024 at 04:57:13PM +0500, Nikita Travkin wrote:
>>> 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 | 453 +++++++++++++++++++++++++++++++++
>>
>> I think this belongs into drivers/platform, as it handles all bits of
>> the EC.
>>
> 
> Hm, I initially submitted it to power/supply following the c630 driver,
> but I think you're right... Though I'm not sure where in platform/ I'd
> put this driver... (+CC Hans)
> 
> Seems like most of the things live in platform/x86 but there is no i.e.
> platform/arm64...
> 
> Hans, (as a maintainer for most things in platform/) what do you think
> would be the best place to put this (and at least two more I'd expect)
> driver in inside platform/? And can we handle it through the
> platform-driver-x86 list?

I guess that adding a drivers/platform/aarch64 map for this makes
sense, with some comments in the Makefile and in the Kconfig
help explaining that this is for PC/laptop style EC drivers,
which combine multiple logical functions in one, only!

Assuming that we are only going to use this for such EC drivers,
using the platform-driver-x86 mailinglist for this makes sense
since that is where are the people are with knowledge of e.g.
userspace APIs for various typical EC functionalities.

It might even make sense to also use:

git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git

As git tree for this and send pull-reqs to Linus for this
together with the other pdx86 for the same reasons.

I would be open to that as long as this is strictly limited to
EC (like) drivers.

Ilpo, what do you think about this ?

Regards,

Hans








> 
>> [...]
>>
>>>  3 files changed, 468 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>> index 3e31375491d5..e91a3acecb41 100644
>>> --- a/drivers/power/supply/Kconfig
>>> +++ b/drivers/power/supply/Kconfig
>>> @@ -985,4 +985,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
>>
>> I did not see any AC status bits?
>>
> 
> I was referring to whatever ACPI spec calls "AC Adapter" but I guess
> I should have used the word "charger" instead... Will reword this.
> 
>>> [...]
>>
>>> +	case POWER_SUPPLY_PROP_PRESENT:
>>> +		val->intval = 1;
>>
>> You have an unused ASPIRE_EC_FG_FLAG_PRESENT, that looks like it
>> should be used here?
>>
> 
> Oh, you're right! I think I initially didn't have this property and
> added it like this as a reaction to that upower change that made it
> consider everything not explicitly present as absent.
> 
> I've just checked what is reported after unplugging the battery and
> seems like the flag (as well as everything else) is gone. Will change
> the driver to read the flag.
> 
> Thanks for your review!
> Nikita
> 
>>> [...]
>>
>> Otherwise the power-supply bits LGTM.
>>
>> -- Sebastian
> 

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

[Nikita Travkin]

Hans de Goede писал(а) 26.02.2024 15:59:
> Hi,
> 
> +Ilpo (fellow pdx86 maintainer)
> 
> On 2/23/24 15:32, Nikita Travkin wrote:
>> Sebastian Reichel писал(а) 22.02.2024 04:41:
>>> Hi,
>>>
>>> On Tue, Feb 20, 2024 at 04:57:13PM +0500, Nikita Travkin wrote:
>>>> 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 | 453 +++++++++++++++++++++++++++++++++
>>>
>>> I think this belongs into drivers/platform, as it handles all bits of
>>> the EC.
>>>
>>
>> Hm, I initially submitted it to power/supply following the c630 driver,
>> but I think you're right... Though I'm not sure where in platform/ I'd
>> put this driver... (+CC Hans)
>>
>> Seems like most of the things live in platform/x86 but there is no i.e.
>> platform/arm64...
>>
>> Hans, (as a maintainer for most things in platform/) what do you think
>> would be the best place to put this (and at least two more I'd expect)
>> driver in inside platform/? And can we handle it through the
>> platform-driver-x86 list?
> 
> I guess that adding a drivers/platform/aarch64 map for this makes
> sense, with some comments in the Makefile and in the Kconfig
> help explaining that this is for PC/laptop style EC drivers,
> which combine multiple logical functions in one, only!
> 
> Assuming that we are only going to use this for such EC drivers,
> using the platform-driver-x86 mailinglist for this makes sense
> since that is where are the people are with knowledge of e.g.
> userspace APIs for various typical EC functionalities.
> 
> It might even make sense to also use:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git
> 
> As git tree for this and send pull-reqs to Linus for this
> together with the other pdx86 for the same reasons.
> 
> I would be open to that as long as this is strictly limited to
> EC (like) drivers.

Yes, I believe the EC are the only "boad-specific" drivers we need for
the Windows-on-Arm devices as of today. I expect at least two more EC
drivers to be added later.

Then I will re-target this series to platform-driver-x86:

- Will add a new drivers/platform/aarch64/ dir with a Makefile and Kconfig
  that would explicitly note it's only for EC-like drivers. Will update
  the "X86 PLATFORM DRIVERS" entry in MAINTAINERS. (Or should I add a new
  entry?)
- Will add this driver there, also updating per the last Sebastian's
  comments.
- Will also move the dt binding to a new bindings/platform/ dir.

Thanks!
Nikita

> 
> Ilpo, what do you think about this ?
> 
> Regards,
> 
> Hans
> 

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

[Hans de Goede]

Hi Nikita,

On 2/28/24 16:49, Nikita Travkin wrote:
> Hans de Goede писал(а) 26.02.2024 15:59:
>> Hi,
>>
>> +Ilpo (fellow pdx86 maintainer)
>>
>> On 2/23/24 15:32, Nikita Travkin wrote:
>>> Sebastian Reichel писал(а) 22.02.2024 04:41:
>>>> Hi,
>>>>
>>>> On Tue, Feb 20, 2024 at 04:57:13PM +0500, Nikita Travkin wrote:
>>>>> 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 | 453 +++++++++++++++++++++++++++++++++
>>>>
>>>> I think this belongs into drivers/platform, as it handles all bits of
>>>> the EC.
>>>>
>>>
>>> Hm, I initially submitted it to power/supply following the c630 driver,
>>> but I think you're right... Though I'm not sure where in platform/ I'd
>>> put this driver... (+CC Hans)
>>>
>>> Seems like most of the things live in platform/x86 but there is no i.e.
>>> platform/arm64...
>>>
>>> Hans, (as a maintainer for most things in platform/) what do you think
>>> would be the best place to put this (and at least two more I'd expect)
>>> driver in inside platform/? And can we handle it through the
>>> platform-driver-x86 list?
>>
>> I guess that adding a drivers/platform/aarch64 map for this makes
>> sense, with some comments in the Makefile and in the Kconfig
>> help explaining that this is for PC/laptop style EC drivers,
>> which combine multiple logical functions in one, only!
>>
>> Assuming that we are only going to use this for such EC drivers,
>> using the platform-driver-x86 mailinglist for this makes sense
>> since that is where are the people are with knowledge of e.g.
>> userspace APIs for various typical EC functionalities.
>>
>> It might even make sense to also use:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git
>>
>> As git tree for this and send pull-reqs to Linus for this
>> together with the other pdx86 for the same reasons.
>>
>> I would be open to that as long as this is strictly limited to
>> EC (like) drivers.
> 
> Yes, I believe the EC are the only "board-specific" drivers we need for
> the Windows-on-Arm devices as of today. I expect at least two more EC
> drivers to be added later.
> 
> Then I will re-target this series to platform-driver-x86:
> 
> - Will add a new drivers/platform/aarch64/ dir with a Makefile and Kconfig
>   that would explicitly note it's only for EC-like drivers. Will update
>   the "X86 PLATFORM DRIVERS" entry in MAINTAINERS. (Or should I add a new
>   entry?)

Please add a new entry (you can simply copy most of the "X86 PLATFORM DRIVERS"
entry), putting this under the "X86 PLATFORM DRIVERS" entry feels weird.

> - Will add this driver there, also updating per the last Sebastian's
>   comments.

This sounds good.

> - Will also move the dt binding to a new bindings/platform/ dir.

Please consult with the DT binding maintainers about this bit.

Regards,

Hans

[PATCH v3 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 5afcb8212f49..3f0d3e33894a 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts
@@ -255,7 +255,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 {
@@ -419,6 +437,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";
@@ -857,6 +888,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 v3 3/3] arm64: dts: qcom: acer-aspire1: Add embedded controller

[Konrad Dybcio]

On 20.02.2024 12:57, Nikita Travkin wrote:
> 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>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad