dt-bindings: platform: Add bindings for Qcom's EC on IT8987
Subsystems (6)
PATCH v2 1/3 dt-bindings: platform: Add bindings for Qcom's EC on IT8987
This patch adds bindings for the EC firmware running on IT8987 present on most of X1E80100 devices
--- .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +52 -0
@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Embedded Controller on IT8987 chip. + +maintainers: + - Maya Matuszczyk <[email protected]> + +description: + Most x1e80100 laptops have an EC running on IT8987 MCU chip. The EC controls + minor functions, like fans, power LED, and on some laptops it also handles + keyboard hotkeys. + +properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ec + - items: + - const: lenovo,yoga-slim7x-ec + - const: qcom,x1e-it8987-ec + + reg: + const: 0x76 + + interrupts: + maxItems: 1 + +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 = "lenovo,yoga-slim7x-ec", "qcom,x1e-it8987-ec"; + reg = <0x76>; + + interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_HIGH>; + }; + }; +...
Signature · 1 line
2.45.2
PATCH v2 2/3 platform: arm64: Add driver for EC found in most X1E laptops
Currently it features only reporting that the AP is going to suspend, which results in keyboard backlight turning off and the power LED slowly blinking on the Lenovo Yoga Slim 7x.
Honor Magicbook Art 14 and Lenovo Yoga Slim 7x are known to have firmware with extensions which would need appropriate handling. For reverse engineering the firmware on them I have written a Rust utility:
https://github.com/Maccraft123/it8987-qcom-tool.git
--- MAINTAINERS | 6 + drivers/platform/arm64/Kconfig | 8 ++ drivers/platform/arm64/Makefile | 1 + drivers/platform/arm64/qcom-x1e-it8987.c | 158 +++++++++++++++++++++++ 4 files changed, 173 insertions(+) create mode 100644 drivers/platform/arm64/qcom-x1e-it8987.c
MAINTAINERS +6 -0
@@ -12890,6 +12890,12 @@ S: Maintained W: http://legousb.sourceforge.net/ F: drivers/usb/misc/legousbtower.c +QCOM IT8987 EC DRIVER +M: Maya Matuszczyk <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml +F: drivers/platform/arm64/qcom-x1e-it8987.c + LETSKETCH HID TABLET DRIVER M: Hans de Goede <[email protected]> L: [email protected]
drivers/platform/arm64/Kconfig +8 -0
@@ -49,4 +49,12 @@ config EC_LENOVO_YOGA_C630 Say M or Y here to include this support. +config EC_QCOM_X1E_IT8987 + tristate "Embedded Controller driver for most X1E80100 laptops" + depends on ARCH_QCOM || COMPILE_TEST + depends on I2C + help + This driver currently supports reporting device suspend to the EC so it + can take appropriate actions. + endif # ARM64_PLATFORM_DEVICES
drivers/platform/arm64/Makefile +1 -0
@@ -7,3 +7,4 @@ obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o +obj-$(CONFIG_EC_QCOM_X1E_IT8987) += qcom-x1e-it8987.o
drivers/platform/arm64/qcom-x1e-it8987.c new file +158 -0
158 lines not on this page · raw message
Signature · 1 line
2.45.2
Bryan O'Donoghue
reviewerRe: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops
Quoted 69 lines · Maya Matuszczyk
Currently it features only reporting that the AP is going to suspend, which results in keyboard backlight turning off and the power LED slowly blinking on the Lenovo Yoga Slim 7x.
Honor Magicbook Art 14 and Lenovo Yoga Slim 7x are known to have firmware with extensions which would need appropriate handling. For reverse engineering the firmware on them I have written a Rust utility:
https://github.com/Maccraft123/it8987-qcom-tool.git
Signed-off-by Maya Matuszczyk <[email protected]>--- MAINTAINERS | 6 + drivers/platform/arm64/Kconfig | 8 ++ drivers/platform/arm64/Makefile | 1 + drivers/platform/arm64/qcom-x1e-it8987.c | 158 +++++++++++++++++++++++ 4 files changed, 173 insertions(+)create mode 100644
drivers/platform/arm64/qcom-x1e-it8987.cMAINTAINERS +6
-0@@ -12890,6 +12890,12 @@ S: Maintained W: http://legousb.sourceforge.net/ F: drivers/usb/misc/legousbtower.c +QCOM IT8987 EC DRIVER +M: Maya Matuszczyk <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml +F: drivers/platform/arm64/qcom-x1e-it8987.c + LETSKETCH HID TABLET DRIVER M: Hans de Goede <[email protected]> L: [email protected]drivers/platform/arm64/Kconfig +8
-0@@ -49,4 +49,12 @@ config EC_LENOVO_YOGA_C630 Say M or Y here to include this support. +config EC_QCOM_X1E_IT8987 + tristate "Embedded Controller driver for most X1E80100 laptops" + depends on ARCH_QCOM || COMPILE_TEST + depends on I2C + help + This driver currently supports reporting device suspend to the EC so it + can take appropriate actions. + endif # ARM64_PLATFORM_DEVICESdrivers/platform/arm64/Makefile +1
-0@@ -7,3 +7,4 @@ obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o +obj-$(CONFIG_EC_QCOM_X1E_IT8987) += qcom-x1e-it8987.odrivers/platform/arm64/qcom-x1e-it8987.c new file +0
-0
drivers/platform/arm64/qcom-x1e-it8987.c line 8
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0@@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2024 Maya Matuszczyk <[email protected]> +*/ +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/input.h> +#include <linux/input/sparse-keymap.h>
Your includes should be alphabetised.
Quoted 29 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +29
-0+ +#define EC_IRQ_REASON_REG 0x05 +#define EC_SUSPEND_RESUME_REG 0x23 +#define EC_IRQ_ENABLE_REG 0x35 + +#define EC_NOTIFY_SUSPEND_ENTER 0x01 +#define EC_NOTIFY_SUSPEND_EXIT 0x00 +#define EC_NOTIFY_SCREEN_OFF 0x03 +#define EC_NOTIFY_SCREEN_ON 0x04 + +#define EC_IRQ_MICMUTE_BUTTON 0x04 +#define EC_IRQ_FAN1_STATUS_CHANGE 0x30 +#define EC_IRQ_FAN2_STATUS_CHANGE 0x31 +#define EC_IRQ_FAN1_SPEED_CHANGE 0x32 +#define EC_IRQ_FAN2_SPEED_CHANGE 0x33 +#define EC_IRQ_COMPLETED_LUT_UPDATE 0x34 +#define EC_IRQ_COMPLETED_FAN_PROFILE_SWITCH 0x35 +#define EC_IRQ_THERMISTOR_1_TEMP_THRESHOLD_CROSS 0x36 +#define EC_IRQ_THERMISTOR_2_TEMP_THRESHOLD_CROSS 0x37 +#define EC_IRQ_THERMISTOR_3_TEMP_THRESHOLD_CROSS 0x38 +#define EC_IRQ_THERMISTOR_4_TEMP_THRESHOLD_CROSS 0x39 +#define EC_IRQ_THERMISTOR_5_TEMP_THRESHOLD_CROSS 0x3a +#define EC_IRQ_THERMISTOR_6_TEMP_THRESHOLD_CROSS 0x3b +#define EC_IRQ_THERMISTOR_7_TEMP_THRESHOLD_CROSS 0x3c +#define EC_IRQ_RECOVERED_FROM_RESET 0x3d + +struct qcom_x1e_it8987_ec { +struct i2c_client *client; +struct input_dev *idev;
drivers/platform/arm64/qcom-x1e-it8987.c line 47
Quoted 10 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +10
-0+struct mutex lock; +}; + +static irqreturn_t qcom_x1e_it8987_ec_irq (int irq,void *data) +{ +struct qcom_x1e_it8987_ec *ec = data; +struct device *dev = &ec->client->dev; +int val; + +guard (mutex)(&ec->lock);
What's the thing that can execute at the same time as this procedure - there doesn't appear to be any concurrent candidate in this patch.
drivers/platform/arm64/qcom-x1e-it8987.c line 55
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0+ + val =i2c_smbus_read_byte_data (ec->client,EC_IRQ_REASON_REG ); +if (val <0 ) { +dev_err (dev,"Failed to get EC IRQ reason: %d\n" , val); +return IRQ_HANDLED ; + } + +dev_info (dev,"Unhandled EC IRQ reason: %d\n" , val);
Should an unhandled IRQ be an info or an err ?
Quoted 93 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +93
-0+ +return IRQ_HANDLED ; +} + +static int qcom_x1e_it8987_ec_probe (struct i2c_client *client) +{ +struct device *dev = &client->dev; +struct qcom_x1e_it8987_ec *ec; +int ret; + + ec =devm_kzalloc (dev,sizeof (*ec),GFP_KERNEL ); +if (!ec) +return -ENOMEM ; + +mutex_init (&ec->lock); + ec->client = client; + + ret =devm_request_threaded_irq (dev, client->irq, +NULL , qcom_x1e_it8987_ec_irq, +IRQF_ONESHOT ,"qcom_x1e_it8987_ec" , ec); +if (ret <0 ) +return dev_err_probe (dev, ret,"Unable to request irq\n" ); + + ret =i2c_smbus_write_byte_data (client,EC_IRQ_ENABLE_REG ,0x01 ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to enable interrupts\n" ); + +return 0 ; +} + +static void qcom_x1e_it8987_ec_remove (struct i2c_client *client) +{ +struct device *dev = &client->dev; +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_IRQ_ENABLE_REG ,0x00 ); +if (ret <0 ) +dev_err (dev,"Failed to disable interrupts: %d\n" , ret); +} + +static int qcom_x1e_it8987_ec_suspend (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_OFF ); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_ENTER ); +if (ret) +return ret; + +return 0 ; +} + +static int qcom_x1e_it8987_ec_resume (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_EXIT ); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_ON ); +if (ret) +return ret; + +return 0 ; +} + +static const struct of_device_id qcom_x1e_it8987_ec_of_match[] = { + { .compatible ="lenovo,yoga-slim7x-ec" }, + { .compatible ="qcom,x1e-it9897-ec" }, + {} +}; +MODULE_DEVICE_TABLE (of, qcom_x1e_it8987_ec_of_match); + +static const struct i2c_device_id qcom_x1e_it8987_ec_i2c_id_table[] = { + {"qcom-x1e-it8987-ec" , }, + {} +}; +MODULE_DEVICE_TABLE (i2c, qcom_x1e_it8987_ec_i2c_id_table); + +static DEFINE_SIMPLE_DEV_PM_OPS(qcom_x1e_it8987_ec_pm_ops, + qcom_x1e_it8987_ec_suspend, + qcom_x1e_it8987_ec_resume); + +static struct i2c_driver qcom_x1e_it8987_ec_i2c_driver = { + .driver = { + .name ="yoga-slim7x-ec" , + .of_match_table = qcom_x1e_it8987_ec_of_match,
drivers/platform/arm64/qcom-x1e-it8987.c line 158
Quoted 10 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +10
-0+ .pm = &qcom_x1e_it8987_ec_pm_ops + }, + .probe = qcom_x1e_it8987_ec_probe, + .remove = qcom_x1e_it8987_ec_remove, + .id_table = qcom_x1e_it8987_ec_i2c_id_table, +}; +module_i2c_driver (qcom_x1e_it8987_ec_i2c_driver); + +MODULE_DESCRIPTION ("Lenovo Yoga Slim 7x Embedded Controller" ); +MODULE_LICENSE ("GPL" );
Otherwise looks pretty good to me, nice hacking :)
---
bod
Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops
czw., 19 gru 2024 o 21:43 Bryan O'Donoghue
<[email protected]> napisał(a):
Quoted 116 lines · Maya Matuszczyk
Quoted 69 lines
Currently it features only reporting that the AP is going to suspend, which results in keyboard backlight turning off and the power LED slowly blinking on the Lenovo Yoga Slim 7x.
Honor Magicbook Art 14 and Lenovo Yoga Slim 7x are known to have firmware with extensions which would need appropriate handling. For reverse engineering the firmware on them I have written a Rust utility:
https://github.com/Maccraft123/it8987-qcom-tool.git
Signed-off-by Maya Matuszczyk <[email protected]>--- MAINTAINERS | 6 + drivers/platform/arm64/Kconfig | 8 ++ drivers/platform/arm64/Makefile | 1 + drivers/platform/arm64/qcom-x1e-it8987.c | 158 +++++++++++++++++++++++ 4 files changed, 173 insertions(+)create mode 100644
drivers/platform/arm64/qcom-x1e-it8987.cMAINTAINERS +6
-0@@ -12890,6 +12890,12 @@ S: Maintained W: http://legousb.sourceforge.net/ F: drivers/usb/misc/legousbtower.c +QCOM IT8987 EC DRIVER +M: Maya Matuszczyk <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml +F: drivers/platform/arm64/qcom-x1e-it8987.c + LETSKETCH HID TABLET DRIVER M: Hans de Goede <[email protected]> L: [email protected]drivers/platform/arm64/Kconfig +8
-0@@ -49,4 +49,12 @@ config EC_LENOVO_YOGA_C630 Say M or Y here to include this support. +config EC_QCOM_X1E_IT8987 + tristate "Embedded Controller driver for most X1E80100 laptops" + depends on ARCH_QCOM || COMPILE_TEST + depends on I2C + help + This driver currently supports reporting device suspend to the EC so it + can take appropriate actions. + endif # ARM64_PLATFORM_DEVICESdrivers/platform/arm64/Makefile +1
-0@@ -7,3 +7,4 @@ obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o +obj-$(CONFIG_EC_QCOM_X1E_IT8987) += qcom-x1e-it8987.odrivers/platform/arm64/qcom-x1e-it8987.c new file +0
-0drivers/platform/arm64/qcom-x1e-it8987.c line 8
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0@@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2024 Maya Matuszczyk <[email protected]> +*/ +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/input.h> +#include <linux/input/sparse-keymap.h> Your includes should be alphabetised.
Quoted 34 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +34
-0+ +#define EC_IRQ_REASON_REG 0x05 +#define EC_SUSPEND_RESUME_REG 0x23 +#define EC_IRQ_ENABLE_REG 0x35 + +#define EC_NOTIFY_SUSPEND_ENTER 0x01 +#define EC_NOTIFY_SUSPEND_EXIT 0x00 +#define EC_NOTIFY_SCREEN_OFF 0x03 +#define EC_NOTIFY_SCREEN_ON 0x04 + +#define EC_IRQ_MICMUTE_BUTTON 0x04 +#define EC_IRQ_FAN1_STATUS_CHANGE 0x30 +#define EC_IRQ_FAN2_STATUS_CHANGE 0x31 +#define EC_IRQ_FAN1_SPEED_CHANGE 0x32 +#define EC_IRQ_FAN2_SPEED_CHANGE 0x33 +#define EC_IRQ_COMPLETED_LUT_UPDATE 0x34 +#define EC_IRQ_COMPLETED_FAN_PROFILE_SWITCH 0x35 +#define EC_IRQ_THERMISTOR_1_TEMP_THRESHOLD_CROSS 0x36 +#define EC_IRQ_THERMISTOR_2_TEMP_THRESHOLD_CROSS 0x37 +#define EC_IRQ_THERMISTOR_3_TEMP_THRESHOLD_CROSS 0x38 +#define EC_IRQ_THERMISTOR_4_TEMP_THRESHOLD_CROSS 0x39 +#define EC_IRQ_THERMISTOR_5_TEMP_THRESHOLD_CROSS 0x3a +#define EC_IRQ_THERMISTOR_6_TEMP_THRESHOLD_CROSS 0x3b +#define EC_IRQ_THERMISTOR_7_TEMP_THRESHOLD_CROSS 0x3c +#define EC_IRQ_RECOVERED_FROM_RESET 0x3d + +struct qcom_x1e_it8987_ec { +struct i2c_client *client; +struct input_dev *idev; +struct mutex lock; +}; + +static irqreturn_t qcom_x1e_it8987_ec_irq (int irq,void *data) +{
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c line 47
Quoted 5 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +5
-0+struct qcom_x1e_it8987_ec *ec = data; +struct device *dev = &ec->client->dev; +int val; + +guard (mutex)(&ec->lock);What's the thing that can execute at the same time as this procedure - there doesn't appear to be any concurrent candidate in this patch.
The user could suspend the laptop and the EC could fire an IRQ at the same time... I'll need to add mutex locking to suspend/resume functions
Quoted 10 lines
drivers/platform/arm64/qcom-x1e-it8987.c line 55
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0+ + val =i2c_smbus_read_byte_data (ec->client,EC_IRQ_REASON_REG ); +if (val <0 ) { +dev_err (dev,"Failed to get EC IRQ reason: %d\n" , val); +return IRQ_HANDLED ; + } + +dev_info (dev,"Unhandled EC IRQ reason: %d\n" , val);Should an unhandled IRQ be an info or an err ?
I don't know, it's "unimplemented but doesn't really matter"
Quoted 110 lines
Quoted 94 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +93
-0+ +return IRQ_HANDLED ; +} + +static int qcom_x1e_it8987_ec_probe (struct i2c_client *client) +{ +struct device *dev = &client->dev; +struct qcom_x1e_it8987_ec *ec; +int ret; + + ec =devm_kzalloc (dev,sizeof (*ec),GFP_KERNEL ); +if (!ec) +return -ENOMEM ; + +mutex_init (&ec->lock); + ec->client = client; + + ret =devm_request_threaded_irq (dev, client->irq, +NULL , qcom_x1e_it8987_ec_irq, +IRQF_ONESHOT ,"qcom_x1e_it8987_ec" , ec); +if (ret <0 ) +return dev_err_probe (dev, ret,"Unable to request irq\n" ); + + ret =i2c_smbus_write_byte_data (client,EC_IRQ_ENABLE_REG ,0x01 ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to enable interrupts\n" ); + +return 0 ; +} + +static void qcom_x1e_it8987_ec_remove (struct i2c_client *client) +{ +struct device *dev = &client->dev; +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_IRQ_ENABLE_REG ,0x00 ); +if (ret <0 ) +dev_err (dev,"Failed to disable interrupts: %d\n" , ret); +} + +static int qcom_x1e_it8987_ec_suspend (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_OFF ); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_ENTER ); +if (ret) +return ret; + +return 0 ; +} + +static int qcom_x1e_it8987_ec_resume (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_EXIT ); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_ON ); +if (ret) +return ret; + +return 0 ; +} + +static const struct of_device_id qcom_x1e_it8987_ec_of_match[] = { + { .compatible ="lenovo,yoga-slim7x-ec" }, + { .compatible ="qcom,x1e-it9897-ec" }, + {} +}; +MODULE_DEVICE_TABLE (of, qcom_x1e_it8987_ec_of_match); + +static const struct i2c_device_id qcom_x1e_it8987_ec_i2c_id_table[] = { + {"qcom-x1e-it8987-ec" , }, + {} +}; +MODULE_DEVICE_TABLE (i2c, qcom_x1e_it8987_ec_i2c_id_table); + +static DEFINE_SIMPLE_DEV_PM_OPS(qcom_x1e_it8987_ec_pm_ops, + qcom_x1e_it8987_ec_suspend, + qcom_x1e_it8987_ec_resume); + +static struct i2c_driver qcom_x1e_it8987_ec_i2c_driver = { + .driver = { + .name ="yoga-slim7x-ec" , + .of_match_table = qcom_x1e_it8987_ec_of_match,drivers/platform/arm64/qcom-x1e-it8987.c line 158
Quoted 10 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +10
-0+ .pm = &qcom_x1e_it8987_ec_pm_ops + }, + .probe = qcom_x1e_it8987_ec_probe, + .remove = qcom_x1e_it8987_ec_remove, + .id_table = qcom_x1e_it8987_ec_i2c_id_table, +}; +module_i2c_driver (qcom_x1e_it8987_ec_i2c_driver); + +MODULE_DESCRIPTION ("Lenovo Yoga Slim 7x Embedded Controller" ); +MODULE_LICENSE ("GPL" );Otherwise looks pretty good to me, nice hacking :)
---
bod
Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops
Quoted 32 lines · Maya Matuszczyk
czw., 19 gru 2024 o 21:43 Bryan O'Donoghue
<[email protected]> napisał(a):Quoted 30 lines · Maya Matuszczyk
Quoted 28 lines
Currently it features only reporting that the AP is going to suspend, which results in keyboard backlight turning off and the power LED slowly blinking on the Lenovo Yoga Slim 7x.
Honor Magicbook Art 14 and Lenovo Yoga Slim 7x are known to have firmware with extensions which would need appropriate handling. For reverse engineering the firmware on them I have written a Rust utility:
https://github.com/Maccraft123/it8987-qcom-tool.git
Signed-off-by Maya Matuszczyk <[email protected]>--- MAINTAINERS | 6 + drivers/platform/arm64/Kconfig | 8 ++ drivers/platform/arm64/Makefile | 1 + drivers/platform/arm64/qcom-x1e-it8987.c | 158 +++++++++++++++++++++++ 4 files changed, 173 insertions(+)create mode 100644
drivers/platform/arm64/qcom-x1e-it8987.cMAINTAINERS +0
-0@@ -12890,6 +12890,12 @@ S: Maintained W: http://legousb.sourceforge.net/ F: drivers/usb/misc/legousbtower.c
Quoted 4 lines
Quoted 4 lines
MAINTAINERS S: Maintained · line 12896
Quoted 4 lines
MAINTAINERS +4
-0+QCOM IT8987 EC DRIVER +M: Maya Matuszczyk <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
Actually, the IT8987 is from ITE Tech. As far as I know, there are other platforms besides x1e that use this. So if this driver can be also applied for all ITE it8987, it might be better to say 'ITE IT8987' instead of 'QCOM IT8987'. This also applies to the file name and function.
Quoted 65 lines
Quoted 65 lines
Quoted 37 lines
MAINTAINERS +2
-0+F: drivers/platform/arm64/qcom-x1e-it8987.c + LETSKETCH HID TABLET DRIVER M: Hans de Goede <[email protected]> L: [email protected]drivers/platform/arm64/Kconfig +8
-0@@ -49,4 +49,12 @@ config EC_LENOVO_YOGA_C630 Say M or Y here to include this support. +config EC_QCOM_X1E_IT8987 + tristate "Embedded Controller driver for most X1E80100 laptops" + depends on ARCH_QCOM || COMPILE_TEST + depends on I2C + help + This driver currently supports reporting device suspend to the EC so it + can take appropriate actions. + endif # ARM64_PLATFORM_DEVICESdrivers/platform/arm64/Makefile +1
-0@@ -7,3 +7,4 @@ obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o +obj-$(CONFIG_EC_QCOM_X1E_IT8987) += qcom-x1e-it8987.odrivers/platform/arm64/qcom-x1e-it8987.c new file +0
-0drivers/platform/arm64/qcom-x1e-it8987.c line 8
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0@@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2024 Maya Matuszczyk <[email protected]> +*/ +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/input.h> +#include <linux/input/sparse-keymap.h> Your includes should be alphabetised.
Quoted 17 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +17
-0+ +#define EC_IRQ_REASON_REG 0x05 +#define EC_SUSPEND_RESUME_REG 0x23 +#define EC_IRQ_ENABLE_REG 0x35 + +#define EC_NOTIFY_SUSPEND_ENTER 0x01 +#define EC_NOTIFY_SUSPEND_EXIT 0x00 +#define EC_NOTIFY_SCREEN_OFF 0x03 +#define EC_NOTIFY_SCREEN_ON 0x04 + +#define EC_IRQ_MICMUTE_BUTTON 0x04 +#define EC_IRQ_FAN1_STATUS_CHANGE 0x30 +#define EC_IRQ_FAN2_STATUS_CHANGE 0x31 +#define EC_IRQ_FAN1_SPEED_CHANGE 0x32 +#define EC_IRQ_FAN2_SPEED_CHANGE 0x33 +#define EC_IRQ_COMPLETED_LUT_UPDATE 0x34 +#define EC_IRQ_COMPLETED_FAN_PROFILE_SWITCH 0x35
Quoted 8 lines
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c line 33
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0+#define EC_IRQ_THERMISTOR_1_TEMP_THRESHOLD_CROSS 0x36 +#define EC_IRQ_THERMISTOR_2_TEMP_THRESHOLD_CROSS 0x37 +#define EC_IRQ_THERMISTOR_3_TEMP_THRESHOLD_CROSS 0x38 +#define EC_IRQ_THERMISTOR_4_TEMP_THRESHOLD_CROSS 0x39 +#define EC_IRQ_THERMISTOR_5_TEMP_THRESHOLD_CROSS 0x3a +#define EC_IRQ_THERMISTOR_6_TEMP_THRESHOLD_CROSS 0x3b +#define EC_IRQ_THERMISTOR_7_TEMP_THRESHOLD_CROSS 0x3c +#define EC_IRQ_RECOVERED_FROM_RESET 0x3d
Could you provide more details or any document references for the IRQ reasons defined above? It seems incomplete to me.
By the way, I noticed this is a V2, but I couldn't find V1. Perhaps including a cover letter next time would be helpful.
Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +9
-0+ +struct qcom_x1e_it8987_ec { +struct i2c_client *client; +struct input_dev *idev; +struct mutex lock; +}; + +static irqreturn_t qcom_x1e_it8987_ec_irq (int irq,void *data) +{Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c line 47
Quoted 5 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +5
-0+struct qcom_x1e_it8987_ec *ec = data; +struct device *dev = &ec->client->dev; +int val; + +guard (mutex)(&ec->lock);What's the thing that can execute at the same time as this procedure - there doesn't appear to be any concurrent candidate in this patch.
Quoted 3 lines
The user could suspend the laptop and the EC could fire an IRQ at the same time... I'll need to add mutex locking to suspend/resume functions
Using a mutex lock inside an IRQ handler is not advisable. Additionally, since this IRQ appears to be only for logging purposes, is it really necessary for it to be triggered during suspend and resume?
Quoted 123 lines
Quoted 11 lines
drivers/platform/arm64/qcom-x1e-it8987.c line 55
Quoted 9 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0+ + val =i2c_smbus_read_byte_data (ec->client,EC_IRQ_REASON_REG ); +if (val <0 ) { +dev_err (dev,"Failed to get EC IRQ reason: %d\n" , val); +return IRQ_HANDLED ; + } + +dev_info (dev,"Unhandled EC IRQ reason: %d\n" , val);Should an unhandled IRQ be an info or an err ?
I don't know, it's "unimplemented but doesn't really matter"
Quoted 110 lines
Quoted 94 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +93
-0+ +return IRQ_HANDLED ; +} + +static int qcom_x1e_it8987_ec_probe (struct i2c_client *client) +{ +struct device *dev = &client->dev; +struct qcom_x1e_it8987_ec *ec; +int ret; + + ec =devm_kzalloc (dev,sizeof (*ec),GFP_KERNEL ); +if (!ec) +return -ENOMEM ; + +mutex_init (&ec->lock); + ec->client = client; + + ret =devm_request_threaded_irq (dev, client->irq, +NULL , qcom_x1e_it8987_ec_irq, +IRQF_ONESHOT ,"qcom_x1e_it8987_ec" , ec); +if (ret <0 ) +return dev_err_probe (dev, ret,"Unable to request irq\n" ); + + ret =i2c_smbus_write_byte_data (client,EC_IRQ_ENABLE_REG ,0x01 ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to enable interrupts\n" ); + +return 0 ; +} + +static void qcom_x1e_it8987_ec_remove (struct i2c_client *client) +{ +struct device *dev = &client->dev; +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_IRQ_ENABLE_REG ,0x00 ); +if (ret <0 ) +dev_err (dev,"Failed to disable interrupts: %d\n" , ret); +} + +static int qcom_x1e_it8987_ec_suspend (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_OFF ); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_ENTER ); +if (ret) +return ret; + +return 0 ; +} + +static int qcom_x1e_it8987_ec_resume (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_EXIT ); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_ON ); +if (ret) +return ret; + +return 0 ; +} + +static const struct of_device_id qcom_x1e_it8987_ec_of_match[] = { + { .compatible ="lenovo,yoga-slim7x-ec" }, + { .compatible ="qcom,x1e-it9897-ec" }, + {} +}; +MODULE_DEVICE_TABLE (of, qcom_x1e_it8987_ec_of_match); + +static const struct i2c_device_id qcom_x1e_it8987_ec_i2c_id_table[] = { + {"qcom-x1e-it8987-ec" , }, + {} +}; +MODULE_DEVICE_TABLE (i2c, qcom_x1e_it8987_ec_i2c_id_table); + +static DEFINE_SIMPLE_DEV_PM_OPS(qcom_x1e_it8987_ec_pm_ops, + qcom_x1e_it8987_ec_suspend, + qcom_x1e_it8987_ec_resume); + +static struct i2c_driver qcom_x1e_it8987_ec_i2c_driver = { + .driver = { + .name ="yoga-slim7x-ec" , + .of_match_table = qcom_x1e_it8987_ec_of_match,drivers/platform/arm64/qcom-x1e-it8987.c line 158
Quoted 10 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +10
-0+ .pm = &qcom_x1e_it8987_ec_pm_ops + }, + .probe = qcom_x1e_it8987_ec_probe, + .remove = qcom_x1e_it8987_ec_remove, + .id_table = qcom_x1e_it8987_ec_i2c_id_table, +}; +module_i2c_driver (qcom_x1e_it8987_ec_i2c_driver); + +MODULE_DESCRIPTION ("Lenovo Yoga Slim 7x Embedded Controller" ); +MODULE_LICENSE ("GPL" );Otherwise looks pretty good to me, nice hacking :)
---
bod
Signature · 2 lines
Thx and BRs, Aiqun(Maria) Yu
Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops
pt., 20 gru 2024 o 12:50 Aiqun(Maria) Yu <[email protected]> napisał(a):
Quoted 34 lines · Maya Matuszczyk
Quoted 32 lines
czw., 19 gru 2024 o 21:43 Bryan O'Donoghue
<[email protected]> napisał(a):Quoted 30 lines · Maya Matuszczyk
Quoted 28 lines
Currently it features only reporting that the AP is going to suspend, which results in keyboard backlight turning off and the power LED slowly blinking on the Lenovo Yoga Slim 7x.
Honor Magicbook Art 14 and Lenovo Yoga Slim 7x are known to have firmware with extensions which would need appropriate handling. For reverse engineering the firmware on them I have written a Rust utility:
https://github.com/Maccraft123/it8987-qcom-tool.git
Signed-off-by Maya Matuszczyk <[email protected]>--- MAINTAINERS | 6 + drivers/platform/arm64/Kconfig | 8 ++ drivers/platform/arm64/Makefile | 1 + drivers/platform/arm64/qcom-x1e-it8987.c | 158 +++++++++++++++++++++++ 4 files changed, 173 insertions(+)create mode 100644
drivers/platform/arm64/qcom-x1e-it8987.cMAINTAINERS +0
-0@@ -12890,6 +12890,12 @@ S: Maintained W: http://legousb.sourceforge.net/ F: drivers/usb/misc/legousbtower.c
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
MAINTAINERS S: Maintained · line 12896
Quoted 4 lines
MAINTAINERS +4
-0+QCOM IT8987 EC DRIVER +M: Maya Matuszczyk <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlActually, the IT8987 is from ITE Tech. As far as I know, there are other platforms besides x1e that use this. So if this driver can be also applied for all ITE it8987, it might be better to say 'ITE IT8987' instead of 'QCOM IT8987'. This also applies to the file name and function.
It is from ITE, however the firmware running on it is from Qualcomm and would not be compatible with devices that also have EC on IT8987 MCU with different firmware. I'm open for suggestions about the driver name which would reflect this.
Quoted 68 lines
Quoted 68 lines
Quoted 68 lines
Quoted 37 lines
MAINTAINERS +2
-0+F: drivers/platform/arm64/qcom-x1e-it8987.c + LETSKETCH HID TABLET DRIVER M: Hans de Goede <[email protected]> L: [email protected]drivers/platform/arm64/Kconfig +8
-0@@ -49,4 +49,12 @@ config EC_LENOVO_YOGA_C630 Say M or Y here to include this support. +config EC_QCOM_X1E_IT8987 + tristate "Embedded Controller driver for most X1E80100 laptops" + depends on ARCH_QCOM || COMPILE_TEST + depends on I2C + help + This driver currently supports reporting device suspend to the EC so it + can take appropriate actions. + endif # ARM64_PLATFORM_DEVICESdrivers/platform/arm64/Makefile +1
-0@@ -7,3 +7,4 @@ obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o +obj-$(CONFIG_EC_QCOM_X1E_IT8987) += qcom-x1e-it8987.odrivers/platform/arm64/qcom-x1e-it8987.c new file +0
-0drivers/platform/arm64/qcom-x1e-it8987.c line 8
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0@@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2024 Maya Matuszczyk <[email protected]> +*/ +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/input.h> +#include <linux/input/sparse-keymap.h> Your includes should be alphabetised.
Quoted 20 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +20
-0+ +#define EC_IRQ_REASON_REG 0x05 +#define EC_SUSPEND_RESUME_REG 0x23 +#define EC_IRQ_ENABLE_REG 0x35 + +#define EC_NOTIFY_SUSPEND_ENTER 0x01 +#define EC_NOTIFY_SUSPEND_EXIT 0x00 +#define EC_NOTIFY_SCREEN_OFF 0x03 +#define EC_NOTIFY_SCREEN_ON 0x04 + +#define EC_IRQ_MICMUTE_BUTTON 0x04 +#define EC_IRQ_FAN1_STATUS_CHANGE 0x30 +#define EC_IRQ_FAN2_STATUS_CHANGE 0x31 +#define EC_IRQ_FAN1_SPEED_CHANGE 0x32 +#define EC_IRQ_FAN2_SPEED_CHANGE 0x33 +#define EC_IRQ_COMPLETED_LUT_UPDATE 0x34 +#define EC_IRQ_COMPLETED_FAN_PROFILE_SWITCH 0x35 +#define EC_IRQ_THERMISTOR_1_TEMP_THRESHOLD_CROSS 0x36 +#define EC_IRQ_THERMISTOR_2_TEMP_THRESHOLD_CROSS 0x37 +#define EC_IRQ_THERMISTOR_3_TEMP_THRESHOLD_CROSS 0x38
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
drivers/platform/arm64/qcom-x1e-it8987.c line 33
Quoted 5 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +5
-0+#define EC_IRQ_THERMISTOR_4_TEMP_THRESHOLD_CROSS 0x39 +#define EC_IRQ_THERMISTOR_5_TEMP_THRESHOLD_CROSS 0x3a +#define EC_IRQ_THERMISTOR_6_TEMP_THRESHOLD_CROSS 0x3b +#define EC_IRQ_THERMISTOR_7_TEMP_THRESHOLD_CROSS 0x3c +#define EC_IRQ_RECOVERED_FROM_RESET 0x3dCould you provide more details or any document references for the IRQ reasons defined above? It seems incomplete to me.
I forgot to remove the micmute button one. All the rest appear to be shared between different devices.
https://github.com/aarch64-laptops/build/tree/master/misc has dumps of ACPI data and you can look for "EAPQ" method which is triggered on EC interrupts
Quoted 2 lines
By the way, I noticed this is a V2, but I couldn't find V1. Perhaps including a cover letter next time would be helpful.
https://lore.kernel.org/lkml/[email protected]/T/#me8af469bc1b7ffd25c25418f7206ed816d8d8c0b
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 4 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +4
-0+ +struct qcom_x1e_it8987_ec { +struct i2c_client *client; +struct input_dev *idev;drivers/platform/arm64/qcom-x1e-it8987.c line 47
Quoted 10 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +10
-0+struct mutex lock; +}; + +static irqreturn_t qcom_x1e_it8987_ec_irq (int irq,void *data) +{ +struct qcom_x1e_it8987_ec *ec = data; +struct device *dev = &ec->client->dev; +int val; + +guard (mutex)(&ec->lock);What's the thing that can execute at the same time as this procedure - there doesn't appear to be any concurrent candidate in this patch.
Quoted 7 lines
Quoted 3 lines
The user could suspend the laptop and the EC could fire an IRQ at the same time... I'll need to add mutex locking to suspend/resume functions
Using a mutex lock inside an IRQ handler is not advisable. Additionally, since this IRQ appears to be only for logging purposes, is it really necessary for it to be triggered during suspend and resume?
Okay I'll remove the mutex.
Quoted 128 lines
Quoted 11 lines
Quoted 11 lines
drivers/platform/arm64/qcom-x1e-it8987.c line 55
Quoted 9 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0+ + val =i2c_smbus_read_byte_data (ec->client,EC_IRQ_REASON_REG ); +if (val <0 ) { +dev_err (dev,"Failed to get EC IRQ reason: %d\n" , val); +return IRQ_HANDLED ; + } + +dev_info (dev,"Unhandled EC IRQ reason: %d\n" , val);Should an unhandled IRQ be an info or an err ?
Quoted 112 lines
I don't know, it's "unimplemented but doesn't really matter"
Quoted 110 lines
Quoted 94 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +93
-0+ +return IRQ_HANDLED ; +} + +static int qcom_x1e_it8987_ec_probe (struct i2c_client *client) +{ +struct device *dev = &client->dev; +struct qcom_x1e_it8987_ec *ec; +int ret; + + ec =devm_kzalloc (dev,sizeof (*ec),GFP_KERNEL ); +if (!ec) +return -ENOMEM ; + +mutex_init (&ec->lock); + ec->client = client; + + ret =devm_request_threaded_irq (dev, client->irq, +NULL , qcom_x1e_it8987_ec_irq, +IRQF_ONESHOT ,"qcom_x1e_it8987_ec" , ec); +if (ret <0 ) +return dev_err_probe (dev, ret,"Unable to request irq\n" ); + + ret =i2c_smbus_write_byte_data (client,EC_IRQ_ENABLE_REG ,0x01 ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to enable interrupts\n" ); + +return 0 ; +} + +static void qcom_x1e_it8987_ec_remove (struct i2c_client *client) +{ +struct device *dev = &client->dev; +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_IRQ_ENABLE_REG ,0x00 ); +if (ret <0 ) +dev_err (dev,"Failed to disable interrupts: %d\n" , ret); +} + +static int qcom_x1e_it8987_ec_suspend (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_OFF ); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_ENTER ); +if (ret) +return ret; + +return 0 ; +} + +static int qcom_x1e_it8987_ec_resume (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_EXIT ); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_ON ); +if (ret) +return ret; + +return 0 ; +} + +static const struct of_device_id qcom_x1e_it8987_ec_of_match[] = { + { .compatible ="lenovo,yoga-slim7x-ec" }, + { .compatible ="qcom,x1e-it9897-ec" }, + {} +}; +MODULE_DEVICE_TABLE (of, qcom_x1e_it8987_ec_of_match); + +static const struct i2c_device_id qcom_x1e_it8987_ec_i2c_id_table[] = { + {"qcom-x1e-it8987-ec" , }, + {} +}; +MODULE_DEVICE_TABLE (i2c, qcom_x1e_it8987_ec_i2c_id_table); + +static DEFINE_SIMPLE_DEV_PM_OPS(qcom_x1e_it8987_ec_pm_ops, + qcom_x1e_it8987_ec_suspend, + qcom_x1e_it8987_ec_resume); + +static struct i2c_driver qcom_x1e_it8987_ec_i2c_driver = { + .driver = { + .name ="yoga-slim7x-ec" , + .of_match_table = qcom_x1e_it8987_ec_of_match,drivers/platform/arm64/qcom-x1e-it8987.c line 158
Quoted 10 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +10
-0+ .pm = &qcom_x1e_it8987_ec_pm_ops + }, + .probe = qcom_x1e_it8987_ec_probe, + .remove = qcom_x1e_it8987_ec_remove, + .id_table = qcom_x1e_it8987_ec_i2c_id_table, +}; +module_i2c_driver (qcom_x1e_it8987_ec_i2c_driver); + +MODULE_DESCRIPTION ("Lenovo Yoga Slim 7x Embedded Controller" ); +MODULE_LICENSE ("GPL" );Otherwise looks pretty good to me, nice hacking :)
---
bod--
Thx and BRs,
Aiqun(Maria) Yu
Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops
In reply to Aiqun(Maria) YuQuoted 33 lines · Aiqun(Maria) Yu
Quoted 32 lines · Maya Matuszczyk
czw., 19 gru 2024 o 21:43 Bryan O'Donoghue
<[email protected]> napisał(a):Quoted 30 lines · Maya Matuszczyk
Quoted 28 lines
Currently it features only reporting that the AP is going to suspend, which results in keyboard backlight turning off and the power LED slowly blinking on the Lenovo Yoga Slim 7x.
Honor Magicbook Art 14 and Lenovo Yoga Slim 7x are known to have firmware with extensions which would need appropriate handling. For reverse engineering the firmware on them I have written a Rust utility:
https://github.com/Maccraft123/it8987-qcom-tool.git
Signed-off-by Maya Matuszczyk <[email protected]>--- MAINTAINERS | 6 + drivers/platform/arm64/Kconfig | 8 ++ drivers/platform/arm64/Makefile | 1 + drivers/platform/arm64/qcom-x1e-it8987.c | 158 +++++++++++++++++++++++ 4 files changed, 173 insertions(+)create mode 100644
drivers/platform/arm64/qcom-x1e-it8987.cMAINTAINERS +0
-0@@ -12890,6 +12890,12 @@ S: Maintained W: http://legousb.sourceforge.net/ F: drivers/usb/misc/legousbtower.c
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
MAINTAINERS S: Maintained · line 12896
Quoted 4 lines
MAINTAINERS +4
-0+QCOM IT8987 EC DRIVER +M: Maya Matuszczyk <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlActually, the IT8987 is from ITE Tech. As far as I know, there are other platforms besides x1e that use this. So if this driver can be also applied for all ITE it8987, it might be better to say 'ITE IT8987' instead of 'QCOM IT8987'. This also applies to the file name and function.
ECS Liva QC710 also uses IT8987 as EC, though DSDT doesn't seem to contain anything useful wrt. the EC.
Signature · 2 lines
With best wishes Dmitry
Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops
In reply to Bryan O'DonoghueQuoted 10 lines · Bryan O'Donoghue
drivers/platform/arm64/qcom-x1e-it8987.c line 55
Quoted 8 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0+ + val =i2c_smbus_read_byte_data (ec->client,EC_IRQ_REASON_REG ); +if (val <0 ) { +dev_err (dev,"Failed to get EC IRQ reason: %d\n" , val); +return IRQ_HANDLED ; + } + +dev_info (dev,"Unhandled EC IRQ reason: %d\n" , val);Should an unhandled IRQ be an info or an err ?
Should be debug or ratelimit at most.
Best regards,
Krzysztof
Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops
In reply to Maya MatuszczykQuoted 4 lines · Maya Matuszczyk
Currently it features only reporting that the AP is going to suspend, which results in keyboard backlight turning off and the power LED slowly blinking on the Lenovo Yoga Slim 7x.
Quoted 8 lines
Honor Magicbook Art 14 and Lenovo Yoga Slim 7x are known to have firmware with extensions which would need appropriate handling. For reverse engineering the firmware on them I have written a Rust utility:
https://github.com/Maccraft123/it8987-qcom-tool.git
Signed-off-by Maya Matuszczyk <[email protected]>
Thanks a lot for working on this!
Quoted 15 lines
--- MAINTAINERS | 6 + drivers/platform/arm64/Kconfig | 8 ++ drivers/platform/arm64/Makefile | 1 + drivers/platform/arm64/qcom-x1e-it8987.c | 158 +++++++++++++++++++++++ 4 files changed, 173 insertions(+) create mode 100644 drivers/platform/arm64/qcom-x1e-it8987.c[...]
drivers/platform/arm64/qcom-x1e-it8987.c new file +0
-0
drivers/platform/arm64/qcom-x1e-it8987.c line 17
Quoted 3 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +2
-0@@ -0,0 +1,158 @@ +#define EC_NOTIFY_SCREEN_OFF 0x03 +#define EC_NOTIFY_SCREEN_ON 0x04
I think these two are specific to the Yoga EC. The 0x03/0x04 value is in the DSDT of Yoga, but I don't see it for the devkit for example.
We should probably only send these commands for the "lenovo,yoga-slim7x-ec" compatible? What happens if you just send EC_NOTIFY_SUSPEND_ENTER and skip sending the screen ones? Keyboard backlight stays on?
drivers/platform/arm64/qcom-x1e-it8987.c line 129
Quoted 3 lines
[...]
drivers/platform/arm64/qcom-x1e-it8987.c new file +2
-0+static const struct of_device_id qcom_x1e_it8987_ec_of_match[] = { + { .compatible ="lenovo,yoga-slim7x-ec" },
If you're not assigning any special data to this compatible, you can just drop this line, since it will still probe based on the fallback compatible below. Or you add some .data here to make the EC_NOTIFY_SCREEN_ON/OFF conditional to the Yoga compatible. :-)
drivers/platform/arm64/qcom-x1e-it8987.c line 134
Quoted 5 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +5
-0+ { .compatible = "qcom,x1e-it9897-ec" }, + {} +}; +MODULE_DEVICE_TABLE (of, qcom_x1e_it8987_ec_of_match); +
Thanks,
Stephan
Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops
In reply to Maya MatuszczykQuoted 8 lines · Maya Matuszczyk
drivers/platform/arm64/qcom-x1e-it8987.c new file +8
-0+static int qcom_x1e_it8987_ec_resume (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SUSPEND_EXIT ); +if (ret) +return ret;
drivers/platform/arm64/qcom-x1e-it8987.c line 129
Quoted 10 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +10
-0+ + ret =i2c_smbus_write_byte_data (client,EC_SUSPEND_RESUME_REG ,EC_NOTIFY_SCREEN_ON ); +if (ret) +return ret; + +return 0 ; +} + +static const struct of_device_id qcom_x1e_it8987_ec_of_match[] = { + { .compatible ="lenovo,yoga-slim7x-ec" },
Drop, you added fallback for that exact purpose.
drivers/platform/arm64/qcom-x1e-it8987.c line 133
Quoted 4 lines
drivers/platform/arm64/qcom-x1e-it8987.c new file +4
-0+ { .compatible = "qcom,x1e-it9897-ec" }, + {} +}; +MODULE_DEVICE_TABLE (of, qcom_x1e_it8987_ec_of_match);
Best regards,
Krzysztof
PATCH v2 3/3 arm64: dts: qcom: x1e80100-lenovo-yoga-slim7x: Add the EC
In reply to Maya Matuszczyk--- .../dts/qcom/x1e80100-lenovo-yoga-slim7x.dts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts +22 -0
@@ -538,6 +538,22 @@ keyboard@3a { }; }; +&i2c5 { + clock-frequency = <400000>; + + status = "okay"; + + embedded-controller@76 { + compatible = "lenovo,yoga-slim7x-ec", "qcom,x1e-it8987-ec"; + reg = <0x76>; + + interrupts-extended = <&tlmm 66 IRQ_TYPE_EDGE_FALLING>; + + pinctrl-0 = <&ec_int_n_default>; + pinctrl-names = "default"; + }; +}; + &i2c8 { clock-frequency = <400000>;
@@ -796,6 +812,12 @@ &tlmm { <44 4>, /* SPI (TPM) */ <238 1>; /* UFS Reset */ + ec_int_n_default: ec-int-n-state { + pins = "gpio66"; + function = "gpio"; + bias-disable; + }; + edp_reg_en: edp-reg-en-state { pins = "gpio70"; function = "gpio";
Signature · 1 line
2.45.2
Re: [PATCH v2 3/3] arm64: dts: qcom: x1e80100-lenovo-yoga-slim7x: Add the EC
Quoted 1 line · Maya Matuszczyk
Signed-off-by Maya Matuszczyk <[email protected]>
Is the commit message missing for this patch?
Quoted 3 lines
--- .../dts/qcom/x1e80100-lenovo-yoga-slim7x.dts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
[...]
Signature · 2 lines
Thx and BRs, Aiqun(Maria) Yu
Re: [PATCH v2 3/3] arm64: dts: qcom: x1e80100-lenovo-yoga-slim7x: Add the EC
Quoted 4 lines · Aiqun(Maria) Yu
Quoted 1 line · Maya Matuszczyk
Signed-off-by Maya Matuszczyk <[email protected]>Is the commit message missing for this patch?
Yeah
Maya, please add a short summary (some backstory about the EC, what it does or so).
You'll probably be asked to do the same with the bindings commit, but you can mostly repeat the same thing both here and there.
Konrad
Rob Herring (Arm)
maintainerQuoted 9 lines · Maya Matuszczyk
This patch adds bindings for the EC firmware running on IT8987 present on most of X1E80100 devices
Signed-off-by Maya Matuszczyk <[email protected]>--- .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:./Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml:22:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
dtschema/dtc warnings/errors:
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Quoted 25 lines · Maya Matuszczyk
This patch adds bindings for the EC firmware running on IT8987 present on most of X1E80100 devices
Signed-off-by Maya Matuszczyk <[email protected]>--- .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlDocumentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +10
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Embedded Controller on IT8987 chip. + +maintainers: + - Maya Matuszczyk <[email protected]>
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 10 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +10
-0+ +description: + Most x1e80100 laptops have an EC running on IT8987 MCU chip. The EC controls + minor functions, like fans, power LED, and on some laptops it also handles + keyboard hotkeys. + +properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ec
Given that ECs tend to be somewhat device-specific and many vendors might have slightly customized the EC firmware(?), I think it would be better to disallow using this generic compatible without a more specific one. In other words, I would drop this line and just keep the case below:
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 23
Quoted 3 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +3
-0+ - items: + - const: lenovo,yoga-slim7x-ec + - const: qcom,x1e-it8987-ec
People can add compatible entries for other devices as needed.
Thanks,
Stephan
A message that is not in the archive (sent off-list or not yet received): CAO_MupJ7JtXNgGyXcxGa+EGAvsu-yG0O6MgneGUBdCEgKNG+MA@mail.gmail.com
Quoted 53 lines · Maya Matuszczyk
Excuse the formatting, I've typed this reply from my phone
pt., 20 gru 2024, 19:05 użytkownik Stephan Gerhold <
Quoted 35 lines · [email protected]> napisał:
Quoted 8 lines · Maya Matuszczyk
This patch adds bindings for the EC firmware running on IT8987 present on most of X1E80100 devices
Signed-off-by Maya Matuszczyk <[email protected]>--- .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlQuoted 2 lines
diff --git
a/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlb/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlQuoted 8 lines
new file mode 100644
index 000000000000..4a4f6eb63072Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +3
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +10
-0+$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Embedded Controller on IT8987 chip. + +maintainers: + - Maya Matuszczyk <[email protected]> + +description: + Most x1e80100 laptops have an EC running on IT8987 MCU chip. The ECcontrols
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 14
Quoted 1 line
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +1
-0+ minor functions, like fans, power LED, and on some laptops it alsohandles
Quoted 13 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 6 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +6
-0+ keyboard hotkeys. + +properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ecGiven that ECs tend to be somewhat device-specific and many vendors might have slightly customized the EC firmware(?), I think it would be better to disallow using this generic compatible without a more specific one. In other words, I would drop this line and just keep the case below:
Quoted 4 lines
I've looked at DSDT of other devices and they look to be compatible with what's on the devkit, with differences being extra features on magicbook art 14 and yoga slim 7x. Though this isn't a hill I'm willing to die on.
I think it's fine to keep qcom,x1e-it8987-ec as second compatible. However, without a more specific compatible, there is a risk we have nothing to match on in case device-specific handling becomes necessary in the driver at some point.
It's certainly subjective, but it might be better to play it safe here and have a specific compatible that one can match, even if the behavior is 99% the same. There will often be subtly different behavior across devices, you mentioned the "keyboard backlight turning off and the power LED slowly blinking", who knows what else exists.
I suppose worst case we could also use of_machine_is_compatible() to just match the device the EC is running on, but I'm not sure if that would be frowned upon.
Thanks,
Stephan
Quoted 54 lines · Stephan Gerhold
Quoted 53 lines · Maya Matuszczyk
Excuse the formatting, I've typed this reply from my phone
pt., 20 gru 2024, 19:05 użytkownik Stephan Gerhold <
Quoted 48 lines · [email protected]> napisał:
Quoted 8 lines · Maya Matuszczyk
This patch adds bindings for the EC firmware running on IT8987 present on most of X1E80100 devices
Signed-off-by Maya Matuszczyk <[email protected]>--- .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlQuoted 2 lines
diff --git
a/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlb/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlQuoted 8 lines
new file mode 100644
index 000000000000..4a4f6eb63072Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +3
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +10
-0+$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Embedded Controller on IT8987 chip. + +maintainers: + - Maya Matuszczyk <[email protected]> + +description: + Most x1e80100 laptops have an EC running on IT8987 MCU chip. The ECcontrols
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 14
Quoted 1 line
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +1
-0+ minor functions, like fans, power LED, and on some laptops it alsohandles
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 6 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +6
-0+ keyboard hotkeys. + +properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ecGiven that ECs tend to be somewhat device-specific and many vendors might have slightly customized the EC firmware(?), I think it would be better to disallow using this generic compatible without a more specific one. In other words, I would drop this line and just keep the case below:
Quoted 6 lines
Quoted 4 lines
I've looked at DSDT of other devices and they look to be compatible with what's on the devkit, with differences being extra features on magicbook art 14 and yoga slim 7x. Though this isn't a hill I'm willing to die on.
I think it's fine to keep qcom,x1e-it8987-ec as second compatible.
No, because:
1. There is no such thing as x1e
2. If there was a soc like this, this has nothing to do with SoC. It is not a component inside SoC and that is the only allowed case when you use SoC compatibles.
Quoted 10 lines
However, without a more specific compatible, there is a risk we have nothing to match on in case device-specific handling becomes necessary in the driver at some point.
It's certainly subjective, but it might be better to play it safe here and have a specific compatible that one can match, even if the behavior is 99% the same. There will often be subtly different behavior across devices, you mentioned the "keyboard backlight turning off and the power LED slowly blinking", who knows what else exists.
Quoted 3 lines
I suppose worst case we could also use
of_machine_is_compatible()to just match the device the EC is running on, but I'm not sure if that would be frowned upon.
Unless you have some sort of insights or secret knowledge from Qualcomm (Bjorn or Konrad can chime in here), I think these are pure guesses that this is a Qualcomm product (implied by vendor prefix) or some sort of re-usable generic firmware from Qualcomm present on multiple devices.
If the FW across devices is the same, then fallbacks for these are fine with me.
Best regards,
Krzysztof
niedz., 22 gru 2024 o 15:40 Krzysztof Kozlowski <[email protected]> napisał(a):
Quoted 61 lines · Stephan Gerhold
Quoted 59 lines · Maya Matuszczyk
Quoted 57 lines
Excuse the formatting, I've typed this reply from my phone
pt., 20 gru 2024, 19:05 użytkownik Stephan Gerhold <
Quoted 35 lines · [email protected]> napisał:
Quoted 8 lines · Maya Matuszczyk
This patch adds bindings for the EC firmware running on IT8987 present on most of X1E80100 devices
Signed-off-by Maya Matuszczyk <[email protected]>--- .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlQuoted 2 lines
diff --git
a/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlb/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlQuoted 8 lines
new file mode 100644
index 000000000000..4a4f6eb63072Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +3
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +10
-0+$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Embedded Controller on IT8987 chip. + +maintainers: + - Maya Matuszczyk <[email protected]> + +description: + Most x1e80100 laptops have an EC running on IT8987 MCU chip. The ECcontrols
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 14
Quoted 1 line
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +1
-0+ minor functions, like fans, power LED, and on some laptops it alsohandles
Quoted 13 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 6 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +6
-0+ keyboard hotkeys. + +properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ecGiven that ECs tend to be somewhat device-specific and many vendors might have slightly customized the EC firmware(?), I think it would be better to disallow using this generic compatible without a more specific one. In other words, I would drop this line and just keep the case below:
I've looked at DSDT of other devices and they look to be compatible with what's on the devkit, with differences being extra features on magicbook art 14 and yoga slim 7x. Though this isn't a hill I'm willing to die on.
Quoted 8 lines
Quoted 1 line
I think it's fine to keep qcom,x1e-it8987-ec as second compatible.
No, because:
1. There is no such thing as x1e
2. If there was a soc like this, this has nothing to do with SoC. It is not a component inside SoC and that is the only allowed case when you use SoC compatibles.
It was the closest thing I had for a "platform name"
Quoted 16 lines
Quoted 11 lines
However, without a more specific compatible, there is a risk we have nothing to match on in case device-specific handling becomes necessary in the driver at some point.
It's certainly subjective, but it might be better to play it safe here and have a specific compatible that one can match, even if the behavior is 99% the same. There will often be subtly different behavior across devices, you mentioned the "keyboard backlight turning off and the power LED slowly blinking", who knows what else exists.
Quoted 3 lines
I suppose worst case we could also use
of_machine_is_compatible()to just match the device the EC is running on, but I'm not sure if that would be frowned upon.
Quoted 4 lines
Unless you have some sort of insights or secret knowledge from Qualcomm (Bjorn or Konrad can chime in here), I think these are pure guesses that this is a Qualcomm product (implied by vendor prefix) or some sort of re-usable generic firmware from Qualcomm present on multiple devices.
The x elite devkit also has the IT8987 EC chip, and when comparing the firmware of it and Yoga Slim 7x's EC there are similarities when running them through strings
On both of them at the beginning there are strings that look like version identifiers:
Devkit:
UUBBK V:00.20.00$
BBK-V20$
Slim7x:
UUBBK V:00.21.00$
BBK-V21$
With similar ones at the end:
Devkit:
EC VER:00.29.00$
LsFv:00.29.00$
Qualcomm$
WoS 8c GenX$
ODM$
MB:A0$
BUILD DATE:
02/0//2/24$
TIME:
14:33:35$
Slim7x:
EC VER:00.60.00$
LsFv:00.20.00$
Qualcomm$
WoS 8c GenX$
ODM$
MB:A0$
BUILD DATE:
2024/07/25$
TIME:
09:58:00$
Quoted 3 lines
If the FW across devices is the same, then fallbacks for these are fine with me.
As the devkit has EC firmware that is handled the same way in DSDT tables of most of other x1e laptops with the same EC, and is a subset of what's done on Lenovo Yoga Slim 7x and Honor Magicbook Art 14 I think the devkit's compatible + -ec would be a good pick.
This conversation is getting long and I feel like I've said everything I wanted to say, I'll just do what you tell me to do about the fallback and binding filename.
Quoted 3 lines
Best regards,
Krzysztof
Quoted 32 lines · Maya Matuszczyk
With similar ones at the end:
Devkit:
EC VER:00.29.00$
LsFv:00.29.00$
Qualcomm$
WoS 8c GenX$
ODM$
MB:A0$
BUILD DATE:
02/0//2/24$
TIME:
14:33:35$Slim7x:
EC VER:00.60.00$
LsFv:00.20.00$
Qualcomm$
WoS 8c GenX$
ODM$
MB:A0$
BUILD DATE:
2024/07/25$
TIME:
09:58:00$Quoted 3 lines
If the FW across devices is the same, then fallbacks for these are fine with me.
Quoted 8 lines
As the devkit has EC firmware that is handled the same way in DSDT tables of most of other x1e laptops with the same EC, and is a subset of what's done on Lenovo Yoga Slim 7x and Honor Magicbook Art 14 I think the devkit's compatible + -ec would be a good pick.
This conversation is getting long and I feel like I've said everything I wanted to say, I'll just do what you tell me to do about the fallback and binding filename.
Go with a device specific compatible (I think I proposed in other email). It is up to you if you want to add fallbacks. If you add fallbacks, then please include some summary of above in commit msg or binding description, so there will be a trace of that explanation/reason for fallbacks.
Best regards,
Krzysztof
Quoted 138 lines · Maya Matuszczyk
niedz., 22 gru 2024 o 15:40 Krzysztof Kozlowski <[email protected]> napisał(a):
Quoted 61 lines · Stephan Gerhold
Quoted 59 lines · Maya Matuszczyk
Quoted 57 lines
Excuse the formatting, I've typed this reply from my phone
pt., 20 gru 2024, 19:05 użytkownik Stephan Gerhold <
Quoted 35 lines · [email protected]> napisał:
Quoted 8 lines · Maya Matuszczyk
This patch adds bindings for the EC firmware running on IT8987 present on most of X1E80100 devices
Signed-off-by Maya Matuszczyk <[email protected]>--- .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlQuoted 2 lines
diff --git
a/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlb/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlQuoted 8 lines
new file mode 100644
index 000000000000..4a4f6eb63072Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +3
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +10
-0+$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Embedded Controller on IT8987 chip. + +maintainers: + - Maya Matuszczyk <[email protected]> + +description: + Most x1e80100 laptops have an EC running on IT8987 MCU chip. The ECcontrols
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 14
Quoted 1 line
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +1
-0+ minor functions, like fans, power LED, and on some laptops it alsohandles
Quoted 13 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 6 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +6
-0+ keyboard hotkeys. + +properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ecGiven that ECs tend to be somewhat device-specific and many vendors might have slightly customized the EC firmware(?), I think it would be better to disallow using this generic compatible without a more specific one. In other words, I would drop this line and just keep the case below:
I've looked at DSDT of other devices and they look to be compatible with what's on the devkit, with differences being extra features on magicbook art 14 and yoga slim 7x. Though this isn't a hill I'm willing to die on.
Quoted 8 lines
Quoted 1 line
I think it's fine to keep qcom,x1e-it8987-ec as second compatible.
No, because:
1. There is no such thing as x1e
2. If there was a soc like this, this has nothing to do with SoC. It is not a component inside SoC and that is the only allowed case when you use SoC compatibles.It was the closest thing I had for a "platform name"
Quoted 16 lines
Quoted 11 lines
However, without a more specific compatible, there is a risk we have nothing to match on in case device-specific handling becomes necessary in the driver at some point.
It's certainly subjective, but it might be better to play it safe here and have a specific compatible that one can match, even if the behavior is 99% the same. There will often be subtly different behavior across devices, you mentioned the "keyboard backlight turning off and the power LED slowly blinking", who knows what else exists.
Quoted 3 lines
I suppose worst case we could also use
of_machine_is_compatible()to just match the device the EC is running on, but I'm not sure if that would be frowned upon.Quoted 4 lines
Unless you have some sort of insights or secret knowledge from Qualcomm (Bjorn or Konrad can chime in here), I think these are pure guesses that this is a Qualcomm product (implied by vendor prefix) or some sort of re-usable generic firmware from Qualcomm present on multiple devices.
The x elite devkit also has the IT8987 EC chip, and when comparing the firmware of it and Yoga Slim 7x's EC there are similarities when running them through strings
On both of them at the beginning there are strings that look like version identifiers:
Devkit:
UUBBK V:00.20.00$
BBK-V20$Slim7x:
UUBBK V:00.21.00$
BBK-V21$With similar ones at the end:
Devkit:
EC VER:00.29.00$
LsFv:00.29.00$
Qualcomm$
WoS 8c GenX$
ODM$
MB:A0$
BUILD DATE:
02/0//2/24$
TIME:
14:33:35$Slim7x:
EC VER:00.60.00$
LsFv:00.20.00$
Qualcomm$
WoS 8c GenX$
ODM$
MB:A0$
BUILD DATE:
2024/07/25$
TIME:
09:58:00$Quoted 3 lines
If the FW across devices is the same, then fallbacks for these are fine with me.
Quoted 9 lines
As the devkit has EC firmware that is handled the same way in DSDT tables of most of other x1e laptops with the same EC, and is a subset of what's done on Lenovo Yoga Slim 7x and Honor Magicbook Art 14 I think the devkit's compatible + -ec would be a good pick.
This conversation is getting long and I feel like I've said everything I wanted to say, I'll just do what you tell me to do about the fallback and binding filename.
I have raised this topic internally at Qualcomm. However, since some key personnel are currently on holiday, an immediate response is not possible. Rest assured, the topic is under review internally. Please stay tuned for updates.
I want to personally thank you for your current support and efforts.
Quoted 3 lines
Quoted 3 lines
Best regards,
Krzysztof
Signature · 2 lines
Thx and BRs, Aiqun(Maria) Yu
Quoted 1 line · Aiqun(Maria) Yu
On 12/22/2024 11:07 PM, Maya Matuszczyk wrote:
[...]
Quoted 6 lines
Quoted 1 line
I have raised this topic internally at Qualcomm. However, since some key personnel are currently on holiday, an immediate response is not possible. Rest assured, the topic is under review internally. Please stay tuned for updates.
Just want to update the thread to let you know: We are working on this. :)
The information that can be updated here is as follows:
1. Qualcomm has a reference spec design for EC (Embedded Controller), and OEMs are able to customize it according to their design. The reference spec design may regardless of ITE or other EC vendors.
2. the reference EC spec will be supported on Qualcomm Compute reference designs for other platform as well. This is irrespective of the underlying transport (I2C, I3C, or any other Qualcomm proprietary transport.
Some of the main functionalities in this patch series appear to align with the spec reference design. Therefore, it could be possibly extended to become a more generic EC driver that supports a wider range of devices.
Quoted 2 lines
I want to personally thank you for your current support and efforts.
[...]
Signature · 2 lines
Thx and BRs, Aiqun(Maria) Yu
Quoted 2 lines · Maya Matuszczyk
This patch adds bindings for the EC firmware running on IT8987 present on most of X1E80100 devices
Please do not use "This commit/patch/change", but imperative mood. See longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
Quoted 13 lines
Signed-off-by Maya Matuszczyk <[email protected]>--- .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yamlDocumentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +0
-0
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 7
Quoted 7 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +7
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Embedded Controller on IT8987 chip.
Drop full stop. Titles don't have them.
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 13 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +13
-0+ +maintainers: + - Maya Matuszczyk <[email protected]> + +description: + Most x1e80100 laptops have an EC running on IT8987 MCU chip. The EC controls + minor functions, like fans, power LED, and on some laptops it also handles + keyboard hotkeys. + +properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ec
That's not a SoC, so I don't understand:
1. referring to the SoC,
2. Having this alone and as fallback.
It does not look like you tested the bindings, at least after quick look. Please run 'make dt_binding_check' (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. Don't rely on distro packages for dtschema and be sure you are using the latest released dtschema.
Best regards,
Krzysztof
Quoted 9 lines · Krzysztof Kozlowski
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 4 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +4
-0+properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ecThat's not a SoC, so I don't understand:
1. referring to the SoC,
2. Having this alone and as fallback.
Quoted 6 lines
It does not look like you tested the bindings, at least after quick look. Please run 'make
dt_binding_check' (seeDocumentation/devicetree/bindings/writing-schema.rstfor instructions). Maybe you need to update your dtschema and yamllint. Don't rely on distro packages for dtschema and be sure you are using the latest released dtschema.
BTW, for sure Qualcomm did not develop/create it8987, so it cannot be used here. Come with specific compatible for this given, one product:
embedded controller on one Lenovo laptop and use it also as filename.
Best regards,
Krzysztof
niedz., 22 gru 2024 o 07:40 Krzysztof Kozlowski <[email protected]> napisał(a):
Quoted 18 lines · Krzysztof Kozlowski
Quoted 9 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 4 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +4
-0+properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ecThat's not a SoC, so I don't understand:
1. referring to the SoC,
2. Having this alone and as fallback.Quoted 6 lines
It does not look like you tested the bindings, at least after quick look. Please run 'make
dt_binding_check' (seeDocumentation/devicetree/bindings/writing-schema.rstfor instructions). Maybe you need to update your dtschema and yamllint. Don't rely on distro packages for dtschema and be sure you are using the latest released dtschema.
Quoted 3 lines
BTW, for sure Qualcomm did not develop/create it8987, so it cannot be used here. Come with specific compatible for this given, one product:
embedded controller on one Lenovo laptop and use it also as filename.
Under these assumptions:
- Qualcomm developed the firmware running on the IT8987 in most x1e machines
- IT8987 is also used in other machines with a non-compatible firmware
- The driver name should reflect the assumptions
I think the name qcom,x1e-it8987-ec is not the worst name for it, as "ite,it8987-ec" would imply compatibility with other devices running non-compatible firmware,
and names specifying only the device wouldn't reflect the "firmware is based on what qcom did and it's driven the same way" part
Which other names do you think would fit this better?
Quoted 3 lines
Best regards,
Krzysztof
niedz., 22 gru 2024 o 08:55 Maya Matuszczyk <[email protected]> napisał(a):
Quoted 30 lines
niedz., 22 gru 2024 o 07:40 Krzysztof Kozlowski <[email protected]> napisał(a):
Quoted 18 lines · Krzysztof Kozlowski
Quoted 9 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 4 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +4
-0+properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ecThat's not a SoC, so I don't understand:
1. referring to the SoC,
2. Having this alone and as fallback.Quoted 6 lines
It does not look like you tested the bindings, at least after quick look. Please run 'make
dt_binding_check' (seeDocumentation/devicetree/bindings/writing-schema.rstfor instructions). Maybe you need to update your dtschema and yamllint. Don't rely on distro packages for dtschema and be sure you are using the latest released dtschema.Quoted 3 lines
BTW, for sure Qualcomm did not develop/create it8987, so it cannot be used here. Come with specific compatible for this given, one product:
embedded controller on one Lenovo laptop and use it also as filename.Under these assumptions:
- Qualcomm developed the firmware running on the IT8987 in most x1e machines
- IT8987 is also used in other machines with a non-compatible firmware
- The driver name should reflect the assumptions
Quoted 7 lines
I think the name qcom,x1e-it8987-ec is not the worst name for it, as "ite,it8987-ec" would imply compatibility with other devices running non-compatible firmware,
and names specifying only the device wouldn't reflect the "firmware is based on what qcom did and it's driven the same way" partWhich other names do you think would fit this better?
In case it wasn't clear:
This was a genuine question - I have no idea how to name this driver and this was the best I could come up with
Quoted 4 lines
Quoted 4 lines
Best regards,
Krzysztof
Quoted 19 lines · Maya Matuszczyk
niedz., 22 gru 2024 o 07:40 Krzysztof Kozlowski <[email protected]> napisał(a):
Quoted 18 lines · Krzysztof Kozlowski
Quoted 15 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml line 20
Quoted 4 lines
Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml new file +4
-0+properties: + compatible: + oneOf: + - const: qcom,x1e-it8987-ecThat's not a SoC, so I don't understand:
1. referring to the SoC,
2. Having this alone and as fallback.It does not look like you tested the bindings, at least after quick look. Please run 'make
dt_binding_check' (seeDocumentation/devicetree/bindings/writing-schema.rstfor instructions). Maybe you need to update your dtschema and yamllint. Don't rely on distro packages for dtschema and be sure you are using the latest released dtschema.
Quoted 7 lines
Quoted 3 lines
BTW, for sure Qualcomm did not develop/create it8987, so it cannot be used here. Come with specific compatible for this given, one product:
embedded controller on one Lenovo laptop and use it also as filename.Under these assumptions:
- Qualcomm developed the firmware running on the IT8987 in most x1e machines
No one here knows whether most x1e machines have this chip...
Quoted 2 lines
- IT8987 is also used in other machines with a non-compatible firmware
- The driver name should reflect the assumptions
I don't care about driver here, so you can use it for the driver but these are not correct assumptions for the bindings.
Quoted 8 lines
I think the name qcom,x1e-it8987-ec is not the worst name for it, as "ite,it8987-ec" would imply compatibility with other devices running non-compatible firmware,
and names specifying only the device wouldn't reflect the "firmware is based on what qcom did and it's driven the same way" partWhich other names do you think would fit this better?
I suggested the one in second reply:
lenovo,yoga-slim-whatever-model-it-is-ec
If you have any indication that Qualcomm firmware from a reference board was used as a reference, it could be used as fallback, but I do not believe you can have such indication considering downstream source code does not exist and all other docs are confidential. Therefore lenovo EC is the first implementation we see and we know anything about.
Best regards,
Krzysztof