[PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops

[PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops

[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 <maccraft123mc@gmail.com>
---
 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

diff --git a/MAINTAINERS b/MAINTAINERS
index b878ddc99f94..08d170e2e1e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12890,6 +12890,12 @@ S:	Maintained
 W:	http://legousb.sourceforge.net/
 F:	drivers/usb/misc/legousbtower.c
 
+QCOM IT8987 EC DRIVER
+M:	Maya Matuszczyk <maccraft123mc@gmail.com>
+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 <hdegoede@redhat.com>
 L:	linux-input@vger.kernel.org
diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
index f88395ea3376..ebb7b4f70ca0 100644
--- a/drivers/platform/arm64/Kconfig
+++ b/drivers/platform/arm64/Kconfig
@@ -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
diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
index b2ae9114fdd8..b9aa195bc1e6 100644
--- a/drivers/platform/arm64/Makefile
+++ b/drivers/platform/arm64/Makefile
@@ -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
diff --git a/drivers/platform/arm64/qcom-x1e-it8987.c b/drivers/platform/arm64/qcom-x1e-it8987.c
new file mode 100644
index 000000000000..d27067d6326a
--- /dev/null
+++ b/drivers/platform/arm64/qcom-x1e-it8987.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Maya Matuszczyk <maccraft123mc@gmail.com>
+ */
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+
+#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)
+{
+	struct qcom_x1e_it8987_ec *ec = data;
+	struct device *dev = &ec->client->dev;
+	int val;
+
+	guard(mutex)(&ec->lock);
+
+	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);
+
+	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,
+		.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");
-- 
2.45.2

Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops

[Bryan O'Donoghue]

On 19/12/2024 20:08, Maya Matuszczyk wrote:
> 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 <maccraft123mc@gmail.com>
> ---
>   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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b878ddc99f94..08d170e2e1e3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12890,6 +12890,12 @@ S:	Maintained
>   W:	http://legousb.sourceforge.net/
>   F:	drivers/usb/misc/legousbtower.c
>   
> +QCOM IT8987 EC DRIVER
> +M:	Maya Matuszczyk <maccraft123mc@gmail.com>
> +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 <hdegoede@redhat.com>
>   L:	linux-input@vger.kernel.org
> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index f88395ea3376..ebb7b4f70ca0 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -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
> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> index b2ae9114fdd8..b9aa195bc1e6 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -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
> diff --git a/drivers/platform/arm64/qcom-x1e-it8987.c b/drivers/platform/arm64/qcom-x1e-it8987.c
> new file mode 100644
> index 000000000000..d27067d6326a
> --- /dev/null
> +++ b/drivers/platform/arm64/qcom-x1e-it8987.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Maya Matuszczyk <maccraft123mc@gmail.com>
> + */
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>

Your includes should be alphabetised.

> +
> +#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)
> +{
> +	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.
> +
> +	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 ?

> +
> +	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,
> +		.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

[Maya Matuszczyk]

czw., 19 gru 2024 o 21:43 Bryan O'Donoghue
<bryan.odonoghue@linaro.org> napisał(a):
>
> On 19/12/2024 20:08, Maya Matuszczyk wrote:
> > 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 <maccraft123mc@gmail.com>
> > ---
> >   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
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b878ddc99f94..08d170e2e1e3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12890,6 +12890,12 @@ S:   Maintained
> >   W:  http://legousb.sourceforge.net/
> >   F:  drivers/usb/misc/legousbtower.c
> >
> > +QCOM IT8987 EC DRIVER
> > +M:   Maya Matuszczyk <maccraft123mc@gmail.com>
> > +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 <hdegoede@redhat.com>
> >   L:  linux-input@vger.kernel.org
> > diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> > index f88395ea3376..ebb7b4f70ca0 100644
> > --- a/drivers/platform/arm64/Kconfig
> > +++ b/drivers/platform/arm64/Kconfig
> > @@ -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
> > diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> > index b2ae9114fdd8..b9aa195bc1e6 100644
> > --- a/drivers/platform/arm64/Makefile
> > +++ b/drivers/platform/arm64/Makefile
> > @@ -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
> > diff --git a/drivers/platform/arm64/qcom-x1e-it8987.c b/drivers/platform/arm64/qcom-x1e-it8987.c
> > new file mode 100644
> > index 000000000000..d27067d6326a
> > --- /dev/null
> > +++ b/drivers/platform/arm64/qcom-x1e-it8987.c
> > @@ -0,0 +1,158 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2024 Maya Matuszczyk <maccraft123mc@gmail.com>
> > + */
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/input.h>
> > +#include <linux/input/sparse-keymap.h>
>
> Your includes should be alphabetised.
>
> > +
> > +#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)
> > +{
> > +     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


> > +
> > +     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"

>
> > +
> > +     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,
> > +             .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

[Aiqun(Maria) Yu]

On 12/20/2024 4:58 AM, Maya Matuszczyk wrote:
> czw., 19 gru 2024 o 21:43 Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> napisał(a):
>>
>> On 19/12/2024 20:08, Maya Matuszczyk wrote:
>>> 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 <maccraft123mc@gmail.com>
>>> ---
>>>   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
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index b878ddc99f94..08d170e2e1e3 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -12890,6 +12890,12 @@ S:   Maintained
>>>   W:  http://legousb.sourceforge.net/
>>>   F:  drivers/usb/misc/legousbtower.c
>>>
>>> +QCOM IT8987 EC DRIVER
>>> +M:   Maya Matuszczyk <maccraft123mc@gmail.com>
>>> +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.
>>> +F:   drivers/platform/arm64/qcom-x1e-it8987.c
>>> +
>>>   LETSKETCH HID TABLET DRIVER
>>>   M:  Hans de Goede <hdegoede@redhat.com>
>>>   L:  linux-input@vger.kernel.org
>>> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
>>> index f88395ea3376..ebb7b4f70ca0 100644
>>> --- a/drivers/platform/arm64/Kconfig
>>> +++ b/drivers/platform/arm64/Kconfig
>>> @@ -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
>>> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
>>> index b2ae9114fdd8..b9aa195bc1e6 100644
>>> --- a/drivers/platform/arm64/Makefile
>>> +++ b/drivers/platform/arm64/Makefile
>>> @@ -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
>>> diff --git a/drivers/platform/arm64/qcom-x1e-it8987.c b/drivers/platform/arm64/qcom-x1e-it8987.c
>>> new file mode 100644
>>> index 000000000000..d27067d6326a
>>> --- /dev/null
>>> +++ b/drivers/platform/arm64/qcom-x1e-it8987.c
>>> @@ -0,0 +1,158 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2024 Maya Matuszczyk <maccraft123mc@gmail.com>
>>> + */
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/input.h>
>>> +#include <linux/input/sparse-keymap.h>
>>
>> Your includes should be alphabetised.
>>
>>> +
>>> +#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

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.
>>> +
>>> +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)
>>> +{
>>> +     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
> 

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?
> 
>>> +
>>> +     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"
> 
>>
>>> +
>>> +     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,
>>> +             .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

[Maya Matuszczyk]

pt., 20 gru 2024 o 12:50 Aiqun(Maria) Yu <quic_aiquny@quicinc.com> napisał(a):
>
> On 12/20/2024 4:58 AM, Maya Matuszczyk wrote:
> > czw., 19 gru 2024 o 21:43 Bryan O'Donoghue
> > <bryan.odonoghue@linaro.org> napisał(a):
> >>
> >> On 19/12/2024 20:08, Maya Matuszczyk wrote:
> >>> 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 <maccraft123mc@gmail.com>
> >>> ---
> >>>   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
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index b878ddc99f94..08d170e2e1e3 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -12890,6 +12890,12 @@ S:   Maintained
> >>>   W:  http://legousb.sourceforge.net/
> >>>   F:  drivers/usb/misc/legousbtower.c
> >>>
> >>> +QCOM IT8987 EC DRIVER
> >>> +M:   Maya Matuszczyk <maccraft123mc@gmail.com>
> >>> +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.
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.

> >>> +F:   drivers/platform/arm64/qcom-x1e-it8987.c
> >>> +
> >>>   LETSKETCH HID TABLET DRIVER
> >>>   M:  Hans de Goede <hdegoede@redhat.com>
> >>>   L:  linux-input@vger.kernel.org
> >>> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> >>> index f88395ea3376..ebb7b4f70ca0 100644
> >>> --- a/drivers/platform/arm64/Kconfig
> >>> +++ b/drivers/platform/arm64/Kconfig
> >>> @@ -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
> >>> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> >>> index b2ae9114fdd8..b9aa195bc1e6 100644
> >>> --- a/drivers/platform/arm64/Makefile
> >>> +++ b/drivers/platform/arm64/Makefile
> >>> @@ -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
> >>> diff --git a/drivers/platform/arm64/qcom-x1e-it8987.c b/drivers/platform/arm64/qcom-x1e-it8987.c
> >>> new file mode 100644
> >>> index 000000000000..d27067d6326a
> >>> --- /dev/null
> >>> +++ b/drivers/platform/arm64/qcom-x1e-it8987.c
> >>> @@ -0,0 +1,158 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-only
> >>> +/*
> >>> + * Copyright (c) 2024 Maya Matuszczyk <maccraft123mc@gmail.com>
> >>> + */
> >>> +#include <linux/i2c.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/input.h>
> >>> +#include <linux/input/sparse-keymap.h>
> >>
> >> Your includes should be alphabetised.
> >>
> >>> +
> >>> +#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
>
> Could 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

> 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/20240927185345.3680-1-maccraft123mc@gmail.com/T/#me8af469bc1b7ffd25c25418f7206ed816d8d8c0b

> >>> +
> >>> +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)
> >>> +{
> >>> +     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
> >
>
> 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.

> >
> >>> +
> >>> +     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"
> >
> >>
> >>> +
> >>> +     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,
> >>> +             .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

[Dmitry Baryshkov]

On Fri, Dec 20, 2024 at 07:50:45PM +0800, Aiqun(Maria) Yu wrote:
> On 12/20/2024 4:58 AM, Maya Matuszczyk wrote:
> > czw., 19 gru 2024 o 21:43 Bryan O'Donoghue
> > <bryan.odonoghue@linaro.org> napisał(a):
> >>
> >> On 19/12/2024 20:08, Maya Matuszczyk wrote:
> >>> 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 <maccraft123mc@gmail.com>
> >>> ---
> >>>   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
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index b878ddc99f94..08d170e2e1e3 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -12890,6 +12890,12 @@ S:   Maintained
> >>>   W:  http://legousb.sourceforge.net/
> >>>   F:  drivers/usb/misc/legousbtower.c
> >>>
> >>> +QCOM IT8987 EC DRIVER
> >>> +M:   Maya Matuszczyk <maccraft123mc@gmail.com>
> >>> +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.

ECS Liva QC710 also uses IT8987 as EC, though DSDT doesn't seem to
contain anything useful wrt. the EC.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] platform: arm64: Add driver for EC found in most X1E laptops

[Krzysztof Kozlowski]

On Thu, Dec 19, 2024 at 08:43:16PM +0000, Bryan O'Donoghue wrote:

> > +
> > +	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

[Stephan Gerhold]

On Thu, Dec 19, 2024 at 09:08:19PM +0100, Maya Matuszczyk wrote:
> 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 <maccraft123mc@gmail.com>

Thanks a lot for working on this!

> ---
>  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
> 
> [...]
> diff --git a/drivers/platform/arm64/qcom-x1e-it8987.c b/drivers/platform/arm64/qcom-x1e-it8987.c
> new file mode 100644
> index 000000000000..d27067d6326a
> --- /dev/null
> +++ b/drivers/platform/arm64/qcom-x1e-it8987.c
> @@ -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?

> [...]
> +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. :-)

> +	{ .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

[Krzysztof Kozlowski]

On Thu, Dec 19, 2024 at 09:08:19PM +0100, Maya Matuszczyk wrote:
> +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" },

Drop, you added fallback for that exact purpose.

> +	{ .compatible = "qcom,x1e-it9897-ec" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_x1e_it8987_ec_of_match);

Best regards,
Krzysztof